Skip to content

Update Permissions and Roles #502

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 21 commits into from
Mar 28, 2025
Merged

Update Permissions and Roles #502

Show file tree
Hide file tree
Changes from 15 commits
Commits
Show all changes
21 commits
Select commit Hold shift + click to select a range
d382663
Extract out non-authorization specific constants and functions
PGijsbers Mar 18, 2025
6b15ea3
Also allow for no one to be logged in
PGijsbers Mar 18, 2025
7910bfa
Add start test for deletion permissions
PGijsbers Mar 18, 2025
bb0fcdc
Provide defaults for creating a test asset
PGijsbers Mar 18, 2025
84e9a0b
Enforce ownership over asset for deletion
PGijsbers Mar 18, 2025
829ae1d
Remove the need for the `create_` role
PGijsbers Mar 19, 2025
c300a9d
Require platform editor role for platform resource router
PGijsbers Mar 19, 2025
042dac9
Remove `crud_` and `edit_aiod_resources` roles from PUT and DEL
PGijsbers Mar 19, 2025
3b81a7a
Everyone is authorized to post metadata now
PGijsbers Mar 19, 2025
469cf79
Remove last references to edit_aoid_resources role
PGijsbers Mar 19, 2025
a3a2173
Undo special case for platform creation, use regular role format
PGijsbers Mar 20, 2025
d611a80
Clean up PR
PGijsbers Mar 20, 2025
7dad71d
Remove variable that was never used
PGijsbers Mar 20, 2025
30b75f7
Merge branch 'develop' into feat/update-delete-permissions
PGijsbers Mar 20, 2025
3bdab43
Fix error introduced by merge commit
PGijsbers Mar 20, 2025
1b1196f
Remove assertion as it already happens on import
PGijsbers Mar 28, 2025
e7f4037
Update documentation with new use of roles.
PGijsbers Mar 28, 2025
31b43c3
Remove edit_aiod_resources role from test files
PGijsbers Mar 28, 2025
4e6c372
Assert on startup of REST API instead of on load
PGijsbers Mar 28, 2025
11b5165
Remove edit_aiod_resource from keycloak
PGijsbers Mar 28, 2025
1a0ae33
Correct name of reviewer role
PGijsbers Mar 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion .env
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ AIBUILDER_API_TOKEN=""
ES_USER=elastic
ES_PASSWORD=changeme
ES_DISCOVERY_TYPE=single-node
ES_ROLE="edit_aiod_resources"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more. 

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

ES_JAVA_OPTS="-Xmx256m -Xms256m"
AIOD_ES_HTTP_PORT=9200
AIOD_ES_TRANSPORT_PORT=9300
Expand Down
11 changes: 8 additions & 3 deletions src/authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,15 +35,20 @@
oidc = OpenIdConnect(openIdConnectUrl=KEYCLOAK_CONFIG.get("openid_connect_url"), auto_error=False)


# These variables are required for the API to function, so we provide context beyond KeyError.
# Should be managed together with other settings in the future (#67)
REVIEWER_ROLE = os.getenv("REVIEWER_ROLE_NAME")
assert REVIEWER_ROLE, "Environment variable 'REVIEWER_ROLE_NAME' not set." # noqa: S101
client_secret = os.getenv("KEYCLOAK_CLIENT_SECRET")
assert client_secret, "Environment variable 'KEYCLOAK_CLIENT_SECRET' not set." # noqa: S101

keycloak_openid = KeycloakOpenID(
server_url=KEYCLOAK_CONFIG.get("server_url"),
client_id=KEYCLOAK_CONFIG.get("client_id"),
client_secret_key=client_secret,
realm_name=KEYCLOAK_CONFIG.get("realm"),
verify=True,
)
_REVIEWER_ROLE = os.getenv("REVIEWER_ROLE_NAME")


@dataclasses.dataclass
Expand All @@ -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
Copy link
Collaborator

@mrorro mrorro Mar 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

return REVIEWER_ROLE in self.roles


async def _get_user(token) -> KeycloakUser:
Expand Down
1 change: 0 additions & 1 deletion src/config.default.toml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,4 +23,3 @@ client_id = "aiod-api" # a private client, used by the backend
client_id_swagger = "aiod-api-swagger" # a public client, used by the Swagger Frontend
openid_connect_url = "http://localhost/aiod-auth/realms/aiod/.well-known/openid-configuration"
scopes = "openid profile roles"
role = "edit_aiod_resources"
46 changes: 18 additions & 28 deletions src/routers/resource_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@
set_permission,
register_user,
PermissionType,
user_can_write,
)
from database.model.ai_resource.resource import AIResource
from database.model.concept.aiod_entry import AIoDEntryORM, EntryStatus
Expand Down Expand Up @@ -404,15 +405,6 @@ def register_resource(
resource_create: clz_create, # type: ignore
user: KeycloakUser = Depends(get_user_or_raise),
):
if not user.has_any_role(
KEYCLOAK_CONFIG.get("role"),
f"create_{self.resource_name_plural}",
f"crud_{self.resource_name_plural}",
):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to create {self.resource_name_plural}.",
)
try:
with DbSession() as session:
try:
Expand Down Expand Up @@ -453,19 +445,18 @@ def put_resource(
resource_create_instance: clz_create, # type: ignore
user: KeycloakUser = Depends(get_user_or_raise),
):
if not user.has_any_role(
KEYCLOAK_CONFIG.get("role"),
f"update_{self.resource_name_plural}",
f"crud_{self.resource_name_plural}",
):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to edit {self.resource_name_plural}.",
)

with DbSession() as session:
try:
resource: Any = self._retrieve_resource(session, identifier)
if not (
user_can_write(user, resource.aiod_entry)
or user.has_role(f"update_{self.resource_name_plural}")
):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to edit {self.resource_name_plural}.",
)

if resource.aiod_entry.status == EntryStatus.SUBMITTED:
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
Expand Down Expand Up @@ -503,18 +494,17 @@ def delete_resource(
user: KeycloakUser = Depends(get_user_or_raise),
):
with DbSession() as session:
if not user.has_any_role(
KEYCLOAK_CONFIG.get("role"),
f"delete_{self.resource_name_plural}",
f"crud_{self.resource_name_plural}",
):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to delete {self.resource_name_plural}.",
)
try:
# Raise error if it does not exist
resource: Any = self._retrieve_resource(session, identifier)
if not (
user_can_administer(user, resource.aiod_entry)
or user.has_role(f"delete_{self.resource_name_plural}")
):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to delete {self.resource_name_plural}.",
)
if (
hasattr(self.resource_class, "__deletion_config__")
and not self.resource_class.__deletion_config__["soft_delete"]
Expand Down
19 changes: 3 additions & 16 deletions src/routers/resource_routers/platform_router.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,6 @@
from sqlmodel import SQLModel, Session, select

from authentication import KeycloakUser, get_user_or_raise
from config import KEYCLOAK_CONFIG
from database.model.platform.platform import Platform
from database.model.resource_read_and_create import resource_create, resource_read
from database.model.serializers import deserialize_resource_relationships
Expand Down Expand Up @@ -178,11 +177,7 @@ def register_resource(
resource_create: clz_create, # type: ignore
user: KeycloakUser = Depends(get_user_or_raise),
):
if not user.has_any_role(
KEYCLOAK_CONFIG.get("role"),
f"create_{self.resource_name_plural}",
f"crud_{self.resource_name_plural}",
):
if not user.has_role("create_platforms"):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to create {self.resource_name_plural}.",
Expand Down Expand Up @@ -222,11 +217,7 @@ def put_resource(
resource_create_instance: clz_create, # type: ignore
user: KeycloakUser = Depends(get_user_or_raise),
):
if not user.has_any_role(
KEYCLOAK_CONFIG.get("role"),
f"update_{self.resource_name_plural}",
f"crud_{self.resource_name_plural}",
):
if not user.has_role("update_platforms"):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to edit {self.resource_name_plural}.",
Expand Down Expand Up @@ -265,11 +256,7 @@ def delete_resource(
user: KeycloakUser = Depends(get_user_or_raise),
):
with DbSession() as session:
if not user.has_any_role(
KEYCLOAK_CONFIG.get("role"),
f"delete_{self.resource_name_plural}",
f"crud_{self.resource_name_plural}",
):
if not user.has_role("delete_platforms"):
raise HTTPException(
status_code=status.HTTP_403_FORBIDDEN,
detail=f"You do not have permission to delete {self.resource_name_plural}.",
Expand Down
90 changes: 11 additions & 79 deletions src/tests/authorization/test_authorization.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,59 +1,21 @@
import contextlib
import json
import os
from http import HTTPStatus
from unittest.mock import Mock

import pytest
from dotenv import load_dotenv
from starlette.testclient import TestClient

from authentication import keycloak_openid, KeycloakUser
from authentication import KeycloakUser
from database.authorization import (
register_user,
set_permission, PermissionType, user_can_read, user_can_write, user_can_administer,
PermissionType, user_can_read, user_can_write, user_can_administer,
)
from database.model.concept.aiod_entry import EntryStatus
from database.model.concept.concept import AIoDConcept
from database.review import Review, Decision, ReviewCreate, Submission
from database.review import Decision, ReviewCreate
from database.session import DbSession
from database.model.knowledge_asset.publication import Publication
from routers.review_router import ListMode

load_dotenv()

ALICE = KeycloakUser("Alice", {"edit_aiod_resources"}, "alice-sub")
BOB = KeycloakUser("Bob", {"edit_aiod_resources"}, "bob-sub")
review_role = os.getenv("REVIEWER_ROLE_NAME")
assert review_role, "The REVIEWER_ROLE_NAME environment variable must be set"
REVIEWER = KeycloakUser("Reviewer", {review_role, "edit_aiod_resources"}, "reviewer-sub")


def _register_user_in_db(user: KeycloakUser) -> KeycloakUser:
with DbSession() as session:
register_user(user, session)
session.commit()
return user


@contextlib.contextmanager
def logged_in_user(user: KeycloakUser):
original = keycloak_openid.introspect
keycloak_openid.introspect = Mock(
return_value={
"realm_access": {"roles": user.roles},
"resource_access": {
"account": {"roles": ["manage-account", "manage-account-links", "view-profile"]}
},
"scope": "openid profile email",
"username": user.name,
"token_type": "Bearer",
"active": True,
"sub": user._subject_identifier,
}
)
yield
keycloak_openid.introspect = original
from tests.testutils.users import ALICE, BOB, REVIEWER, _register_user_in_db, \
logged_in_user, register_asset


def test_user_must_be_logged_in_to_publish(client, publication):
Expand Down Expand Up @@ -213,10 +175,10 @@ def test_unknown_submission_raises_404(client):
[
(REVIEWER, ListMode.PENDING, [2, 3], "Reviewer can see all pending reviews."),
(REVIEWER, ListMode.COMPLETED, [1], "Reviewer can see all completed reviews."),
(REVIEWER, ListMode.ALL, [1,2,3], "Reviewer can see all reviews."),
(REVIEWER, ListMode.ALL, [1, 2, 3], "Reviewer can see all reviews."),
(ALICE, ListMode.PENDING, [2], "Alice has one pending submission and can not see Bob's."),
(ALICE, ListMode.COMPLETED, [1], "Alice only has one completed submission."),
(ALICE, ListMode.ALL, [1,2], "Alice can see all her reviews, but not Bob's."),
(ALICE, ListMode.ALL, [1, 2], "Alice can see all her reviews, but not Bob's."),
(BOB, ListMode.PENDING, [3], "Bob has one pending submission and can not see Alice's."),
(BOB, ListMode.COMPLETED, [], "Bob has no completed submission."),
(BOB, ListMode.ALL, [3], "Bob can see all his reviews, but not Alice's."),
Expand Down Expand Up @@ -255,12 +217,12 @@ def test_an_published_asset_is_not_pending_for_review(client, publication):
@pytest.mark.parametrize(
("user", "mode", "asset", "reason"),
[
(REVIEWER, ListMode.OLDEST, 2,"Reviewer can see both Alice and Bob's submission." ),
(REVIEWER, ListMode.OLDEST, 2, "Reviewer can see both Alice and Bob's submission."),
(REVIEWER, ListMode.NEWEST, 3, "Reviewer can see both Alice and Bob's submission."),
(ALICE, ListMode.OLDEST, 2, "Alice only has one pending submission."),
(ALICE, ListMode.NEWEST, 2,"Alice only has one pending submission."),
(BOB, ListMode.OLDEST, 3,"Bob only has one pending submission." ),
(BOB, ListMode.NEWEST, 3,"Bob only has one pending submission."),
(ALICE, ListMode.NEWEST, 2, "Alice only has one pending submission."),
(BOB, ListMode.OLDEST, 3, "Bob only has one pending submission."),
(BOB, ListMode.NEWEST, 3, "Bob only has one pending submission."),
]
)
def test_retrieving_single_submission_works(user: KeycloakUser, mode: ListMode, asset: int, reason: str, client: TestClient, publication_factory):
Expand Down Expand Up @@ -318,36 +280,6 @@ def test_user_can_always_delete_asset(status: EntryStatus, publication, client):
)
assert response.status_code == HTTPStatus.OK, response.json()


def register_asset(asset: AIoDConcept, /, *, owner: KeycloakUser, status: EntryStatus):
with DbSession() as session:
session.add(asset)
session.commit()

register_user(owner, session)
set_permission(owner, asset.aiod_entry, session, type_=PermissionType.ADMIN)

asset.aiod_entry.status = status
if status in [EntryStatus.SUBMITTED, EntryStatus.PUBLISHED, EntryStatus.REJECTED]:
submission = Submission(
requestee_identifier=owner._subject_identifier,
aiod_entry_identifier=asset.aiod_entry.identifier,
asset_type=asset.__tablename__,
)
session.add(submission)
if status == EntryStatus.PUBLISHED:
register_user(REVIEWER, session)
review = Review(
decision=Decision.ACCEPTED,
reviewer_identifier=REVIEWER._subject_identifier,
comment="foo",
)
review.submission = submission
session.add(review)
session.commit()
return asset.identifier


def test_user_can_edit_asset_in_draft(publication, client):
identifier = register_asset(publication, owner=ALICE, status=EntryStatus.DRAFT)
new_name = "Alice in Wonderland"
Expand Down
60 changes: 0 additions & 60 deletions src/tests/routers/generic/test_authentication.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,8 @@ def test_platform_get_unauthenticated(
@pytest.mark.parametrize(
"mocked_token",
[
["edit_aiod_resources"],
["delete_test_resources"],
["crud_test_resources"],
["delete_test_resources", "create_datasets"],
["edit_aiod_resources", "crud_test_resources"],
],
indirect=True,
)
Expand All @@ -59,60 +56,6 @@ def test_delete_authorized(
assert response.status_code == 200, response.json()


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."
Comment on lines -62 to -79
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered in test_router_delete.py below.



@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()
Comment on lines -82 to -99
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Covered in test_router_post



@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."
Comment on lines -102 to -113
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No longer relevant (no permissions required).



def test_post_unauthenticated(client_test_resource: TestClient):
response = client_test_resource.post("/test_resources/v0", json={"title": "example"})
assert response.status_code == 401, response.json()
Expand All @@ -126,11 +69,8 @@ def test_post_unauthenticated(client_test_resource: TestClient):
@pytest.mark.parametrize(
"mocked_token",
[
["edit_aiod_resources"],
["update_test_resources"],
["crud_test_resources"],
["update_test_resources", "delete_datasets"],
["edit_aiod_resources", "crud_test_resources"],
],
indirect=True,
)
Expand Down
Loading