Update Permissions and Roles #502

PGijsbers · 2025-03-19T15:07:08Z

Change

Many different roles were defined:

create_*, update_*, and delete_* roles, to allow create/update/delete right for individual asset types (e.g., datasets)
crud_* role, which encompassed the previous roles
edit_aiod_resources, which mostly seemed to provide the same rights as crud_* but for all asset types at once

With the introduction of using permissions and a review process, we will rely less on roles and thus remove some. In this PR specifically, we remove the use of the edit_aiod_resources and crud_* roles. They should be implemented as Role Groups in keycloak, giving the users the individual, more granular permissions. The create_* roles are also no longer used (with an exception for Platforms), as users do not need special roles to create assets (instead, they now undergo a review process).
Since connectors (currently) do not use the API, but directly insert into the database, they are not affected by these changes. In the future, we will rely on specific roles for each platform to allow which users can upload/edit assets associated with a specific platform (in general, users will not be able to upload/edit anything from a platform that is not the default "aiod").

The PR then also adds the checks for the update and delete endpoints to ensure that edit requests either have the correct role (i.e., it is some editor making the modification) or they are the owner (through the AIoD permission system).

Documentation will be updated in a separate PR.

How to Test

Unit tests are updated, probably the most important thing is to do a code review.
Otherwise, monkey testing using the user and reviewer in default keycloak.
The user can upload, edit, delete their (draft) asset, and the reviewer (because it's simply a different user with no privileges other than reviewing) cannot edit or delete the asset uploaded by user (and vice versa).

Checklist

Tests have been added or updated to reflect the changes, or their absence is explicitly explained.
Documentation has been added or updated to reflect the changes, or their absence is explicitly explained.
A self-review has been conducted checking:
- No unintended changes have been committed.
- The changes in isolation seem reasonable.
- Anything that may be odd or unintuitive is provided with a GitHub comment explaining it (but consider if this should not be a code comment or in the documentation instead).
All CI checks pass before pinging a reviewer, or provide an explanation if they do not.

Related Issues

Progress on #449.

Instead, these roles should be managed through Group Roles, which are expected to provide the pre-existing more granular `update_` and `delete_` roles.

PGijsbers · 2025-03-20T07:39:50Z

src/tests/routers/generic/test_authentication.py

-@pytest.mark.parametrize(
-    "mocked_token", [["delete_test_resources"], ["create_datasets"]], indirect=True
-)
-def test_post_unauthorized(client_test_resource: TestClient, mocked_token: Mock):
-    response = client_test_resource.post(
-        "/test_resources/v0",
-        json={"title": "example"},
-        headers={"Authorization": "fake-token"},
-    )
-    assert response.status_code == 403, response.json()
-    response_json = response.json()
-    assert response_json["detail"] == "You do not have permission to create test_resources."


No longer relevant (no permissions required).

PGijsbers · 2025-03-20T07:40:22Z

src/tests/routers/generic/test_authentication.py

-def test_delete_unauthenticated(
-    client_test_resource: TestClient, engine_test_resource_filled: Engine
-):
-    response = client_test_resource.delete("/test_resources/v0/1")
-    assert response.status_code == 401, response.json()
-
-
-@pytest.mark.parametrize(
-    "mocked_token", [["create_test_resources"], ["delete_datasets"]], indirect=True
-)
-def test_delete_unauthorized(client_test_resource: TestClient, mocked_token: Mock):
-    response = client_test_resource.delete(
-        "/test_resources/v0/1",
-        headers={"Authorization": "fake-token"},
-    )
-    assert response.status_code == 403, response.json()
-    response_json = response.json()
-    assert response_json["detail"] == "You do not have permission to delete test_resources."


Covered in test_router_delete.py below.

PGijsbers · 2025-03-20T07:42:07Z

src/tests/routers/generic/test_authentication.py

-@pytest.mark.parametrize(
-    "mocked_token",
-    [
-        ["edit_aiod_resources"],
-        ["create_test_resources"],
-        ["crud_test_resources"],
-        ["create_test_resources", "delete_datasets"],
-        ["edit_aiod_resources", "crud_test_resources"],
-    ],
-    indirect=True,
-)
-def test_post_authorized(client_test_resource, mocked_token: Mock):
-    response = client_test_resource.post(
-        "/test_resources/v0",
-        json={"title": "example"},
-        headers={"Authorization": "fake-token"},
-    )
-    assert response.status_code == 200, response.json()


Covered in test_router_post

Taniya-Das

The code changes look okay to me, I do not see any issues. The tests work as well.

mrorro · 2025-03-27T10:09:37Z

src/authentication.py

@@ -60,8 +65,8 @@ def has_any_role(self, *roles: str) -> bool:

    @property
    def is_reviewer(self):
-        assert _REVIEWER_ROLE is not None, "Must configure role `reviewer` in config.toml file."  # noqa: S101
-        return _REVIEWER_ROLE in self.roles
+        assert REVIEWER_ROLE is not None, "Must configure role `reviewer` in config.toml file."  # noqa: S101


REVIEWER_ROLE is not longer defined in the toml file.
the assertion on line 41 is not enough?

Good catch! I initially didn't have the assert on import, but decided that doing it on import is better since we really do not want to start the REST API without proper configuration anyway. Then forgot to remove this.

I also moved validation to a separate function to avoid an issue where it prohibited some services which inadvertendly imported this module wouldn't function (because the environment variable wasn't forwarded to their container).

mrorro · 2025-03-27T14:33:41Z

I see that a user can see assets in draft of other users. is this normal?

mrorro · 2025-03-27T14:36:30Z

.env

@@ -28,7 +28,6 @@ AIBUILDER_API_TOKEN=""
 ES_USER=elastic
 ES_PASSWORD=changeme
 ES_DISCOVERY_TYPE=single-node
-ES_ROLE="edit_aiod_resources"


There are many "edit_aiod_resources" around in the code that I guess could also be deleted

As far as I can tell, I only missed a few in test code and documentation (sigh, IDE search), but otherwise there should be no mention. I'll clear those up now as well. Unless there's usage in the metadata catalogue source that you think I missed?

data/keycloak/data/import/aiod-realm.json: "name" : "edit_aiod_resources", data/keycloak/data/import/aiod-users-0.json: "realmRoles" : [ "default-roles-aiod", "edit_aiod_resources" ], docs/developer/authentication.md:| user | password | edit_aiod_resources, default-roles-aiod, offline_access, uma_authorization | | docs/hosting/authentication.md:Currently, only the ` edit_aiod_resources` role is defined, granting users the ability to upload and edit resources. src/tests/resources/authentication/user_privileged.json: "edit_aiod_resources" src/tests/resources/authentication/user_inactive.json: "edit_aiod_resources" src/tests/test_authentication.py: "edit_aiod_resources", src/tests/testutils/default_sqlalchemy.py

Sorry wasn't done yet updating the PR. I removed the roles from keycloak, tests, and updated the documentation.

PGijsbers · 2025-03-28T13:04:14Z

I see that a user can see assets in draft of other users. is this normal?

Yes. I deliberately left that out of this PR. Read permissions affect many endpoints and in different ways, so I figured it made more sense to chunk it for separate review, I'm already working on that.

Since loading is also done by some other services which do not need all the configurations to be set.

PGijsbers · 2025-03-28T15:43:13Z

@mrorro answered your questions and finished processing the feedback. sorry for being unclear on the state earlier.

mrorro

LGTM

PGijsbers added 11 commits March 18, 2025 16:02

Extract out non-authorization specific constants and functions

d382663

Also allow for no one to be logged in

6b15ea3

Add start test for deletion permissions

7910bfa

Provide defaults for creating a test asset

bb0fcdc

Enforce ownership over asset for deletion

84e9a0b

Remove the need for the create_ role

829ae1d

Require platform editor role for platform resource router

c300a9d

Remove crud_ and edit_aiod_resources roles from PUT and DEL

042dac9

Instead, these roles should be managed through Group Roles, which are expected to provide the pre-existing more granular `update_` and `delete_` roles.

Everyone is authorized to post metadata now

3b81a7a

Remove last references to edit_aoid_resources role

469cf79

Undo special case for platform creation, use regular role format

a3a2173

PGijsbers commented Mar 20, 2025

View reviewed changes

PGijsbers added 2 commits March 20, 2025 09:45

Clean up PR

d611a80

Remove variable that was never used

7dad71d

PGijsbers mentioned this pull request Feb 19, 2025

Reviewing and Permissions meta-issue #449

Open

9 tasks

PGijsbers added 2 commits March 20, 2025 09:56

Merge branch 'develop' into feat/update-delete-permissions

30b75f7

Fix error introduced by merge commit

3bdab43

PGijsbers marked this pull request as ready for review March 20, 2025 08:01

PGijsbers requested review from Taniya-Das and mrorro March 20, 2025 08:03

Taniya-Das approved these changes Mar 25, 2025

View reviewed changes

mrorro reviewed Mar 27, 2025

View reviewed changes

PGijsbers added 3 commits March 28, 2025 14:12

Remove assertion as it already happens on import

1b1196f

Update documentation with new use of roles.

e7f4037

Remove edit_aiod_resources role from test files

31b43c3

PGijsbers added 3 commits March 28, 2025 16:30

Assert on startup of REST API instead of on load

4e6c372

Since loading is also done by some other services which do not need all the configurations to be set.

Remove edit_aiod_resource from keycloak

11b5165

Correct name of reviewer role

1a0ae33

PGijsbers requested a review from mrorro March 28, 2025 15:42

mrorro reviewed Mar 28, 2025

View reviewed changes

PGijsbers merged commit e88eaa0 into develop Mar 28, 2025
1 check passed

PGijsbers deleted the feat/update-delete-permissions branch March 28, 2025 15:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update Permissions and Roles #502

Update Permissions and Roles #502

PGijsbers commented Mar 19, 2025 •

edited

Loading

PGijsbers Mar 20, 2025

PGijsbers Mar 20, 2025

PGijsbers Mar 20, 2025

Taniya-Das left a comment •

edited

Loading

mrorro Mar 27, 2025 •

edited

Loading

PGijsbers Mar 28, 2025

PGijsbers Mar 28, 2025

mrorro commented Mar 27, 2025

mrorro Mar 27, 2025

PGijsbers Mar 28, 2025

mrorro Mar 28, 2025

PGijsbers Mar 28, 2025

PGijsbers commented Mar 28, 2025

PGijsbers commented Mar 28, 2025

mrorro left a comment

Update Permissions and Roles #502

Update Permissions and Roles #502

Conversation

PGijsbers commented Mar 19, 2025 • edited Loading

Change

How to Test

Checklist

Related Issues

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Taniya-Das left a comment • edited Loading

Choose a reason for hiding this comment

mrorro Mar 27, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrorro commented Mar 27, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PGijsbers commented Mar 28, 2025

PGijsbers commented Mar 28, 2025

mrorro left a comment

Choose a reason for hiding this comment

PGijsbers commented Mar 19, 2025 •

edited

Loading

Taniya-Das left a comment •

edited

Loading

mrorro Mar 27, 2025 •

edited

Loading