Feat/authorization #435

PGijsbers · 2025-01-30T15:31:02Z

Change

Adds a review workflow to the REST API, in short:

A user's uploaded asset is in draft.
The user can submit it for review.
A user can review it (reject -> back to draft, accept -> published)

Not included:

An easy way for reviewers to see what needs reviewing.
An easy for way for users to see the review result.
Developer documentation. I'll wait until we had a few follow up PRs iterating over this implementation.

How to Test

I added extra unit tests. If you want to test this through the API, please follow these instructions to make sure there is a reviewer account with a reviewer role (from the root of the repo):

git fetch origin
./scripts/down.sh
git checkout auth/defaults  # if #445 is not merged yet, otherwise `develop`
./scripts/clean.sh
./scripts/up.sh
./scripts/down.sh
git checkout feat/authorization
./scripts/up.sh

Now there is a user (user:password) and a reviewer (reviewer:password) that can make use of the new endpoints.

user: create a case_study (or other asset) and note its identifier.
user: submit the case_study for review (by its identifier) using the new /RESOURCE_TYPE/submit/v1/{identifier} endpoint. Remember the "submission_identifier" that's returned.
either: List all case studies, you should see it with status "submitted".
reviewer: Submit a review with the /RESOURCE_TYPE/review/v1 endpoint, use the "submission_identifier" obtained earlier.

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

Related PRs:

Auth/defaults #445 adds reviewer role to default instance
feat/reviewqueue branch is the first follow up PR to this, and will introduce endpoints for reviewers to see what needs reviewing.

The considered alternative is to not differentiate, but then you would normally want to do an extra database roundtrip on every authentication request, which seems rather wasteful. In many cases this may not be needed at all.

The correct workflow would be to retract the asset first, then edit it. Currently no guard against editing published assets since that would require a more comprehensive review process.

PGijsbers · 2025-02-04T15:22:03Z

src/authentication.py

+class KeycloakUser:
+    name: str
+    roles: set[str]
+    _subject_identifier: str


Two changes:

Rename User -> KeycloakUser: this allows me to introduce a new User class in our permission model. The distinction is as follows: KeycloakUser is provided in API calls and identifiers the user that makes a request, including its roles. The User class only retains the information required to identify a user uniquely (i.e., the subject_identifier), nothing more. This is primarily because we do not want to duplicate information such name or roles between our database and the identity provider.

Addition of the _subject_identifier field: this is required so we can unique identify a user making the request. Which is needed because now permissions about editing may be constrained to specific users.

PGijsbers · 2025-02-04T15:24:08Z

src/database/authorization.py

@@ -0,0 +1,81 @@
+import enum


This module contains the broad definitions required for authorization: who is a user and what are its permissions.
A Permission has information about what a User can do with a specific asset, as identified by its aiod_entry.
There are some place-holder comments/stubs for future work with permissions. This PR largely does not concern that (except for administrator permissions).

PGijsbers · 2025-02-04T15:27:44Z

src/database/review.py

@@ -0,0 +1,69 @@
+import enum


Module for the reviewing components. In particular, a User may submit their asset for review, resulting in a Submission. The Submission may be reviewed by a reviewer, resulting in a Review.

PGijsbers · 2025-02-04T15:28:33Z

src/main.py

@@ -132,7 +133,7 @@ def create_app() -> FastAPI:

        drop_database = args.build_db == "drop-then-build"
        create_database(delete_first=drop_database)
-        AIoDConcept.metadata.create_all(EngineSingleton().engine, checkfirst=True)
+        SQLModel.metadata.create_all(EngineSingleton().engine, checkfirst=True)


The User/Permission/Review entities do not derive from AIoDConcept, but still need to be included in the database creation logic.

PGijsbers · 2025-02-04T15:31:20Z

src/routers/resource_router.py

@@ -132,6 +134,30 @@ def create(self, url_prefix: str) -> APIRouter:
            description=f"Retrieve the number of {self.resource_name_plural}.",
            **default_kwargs,
        )
+        router.add_api_route(


I want to experiment with creating dedicated endpoints instead, where it's the review endpoints are not split by asset. I'll do that in a follow-up PR.

Taniya-Das

Overall, it looks good. It works as intended.
One suggestion - Once the reviewer rejects a submission, and puts a comment, I think it would be useful if the user could see the comments.

Taniya-Das · 2025-02-17T16:01:58Z

src/database/review.py

+    decision_date: datetime = Field(default_factory=lambda: datetime.now(timezone.utc))
+
+    # Not sure if size should be limited, especially if we want to use this for structured data.
+    comment: str | None = Field()


I think it makes sense to define a size, to make error handling better.

PGijsbers · 2025-02-18T11:43:41Z

Once the reviewer rejects a submission, and puts a comment, I think it would be useful if the user could see the comments.

I agree! But we can set up endpoints for that in a follow up PR. The reason to split it off is to get the main user/permission model into the main branch, which will allow us to more easily make independent smaller PRs for features such as those.

I think it makes sense to define a size, to make error handling better.

We should probably chat about it sometime in the WP3 discussion.

PGijsbers added 18 commits January 30, 2025 15:47

Add concept of users and permissions, non-functional

39bc949

Create link between AIoD entry and Permission

1b6abbf

Be explicit about difference of logged in vs registered user

c7b2e18

Differentiate between Keycloak and AIoD users

ed74062

The considered alternative is to not differentiate, but then you would normally want to do an extra database roundtrip on every authentication request, which seems rather wasteful. In many cases this may not be needed at all.

Add test outlines

8f56ef0

Add subject identifier to the user

539a9fd

Add tests for upload and status

fd49a26

Allow entry to be submitted

68ccae4

Add retract endpoint to remove asset from SUBMITTED or PUBLISHED

739d9bb

Add test for deleting assets regardless of stage

baa9ee0

Clean up tests

908ded7

Disallow editing assets already submitted for review

4fde7fd

The correct workflow would be to retract the asset first, then edit it. Currently no guard against editing published assets since that would require a more comprehensive review process.

Add a review endpoint for reviewers

3a2b154

Disallow reviewing assets you own

5c04d1a

Draft out a review dataclass

f413019

Store the review request

c00aff2

Store retraction metadata for the corresponding review request

61eb589

Update the review information when a review is left

5791532

PGijsbers force-pushed the feat/authorization branch from 1142f17 to 5791532 Compare January 30, 2025 15:53

Split up ReviewRequest into Submission and Review

7e62364

PGijsbers commented Feb 4, 2025

View reviewed changes

PGijsbers mentioned this pull request Feb 11, 2025

Auth/defaults #445

Merged

4 tasks

PGijsbers added 4 commits February 12, 2025 12:05

Remove the need to specify asset identifier for review

d2ac5f6

Return submission identifier instead of the submitted asset

1d9cbe1

Provide relevant review information instead of known resource info

7e83786

Register reviewer before leaving review

d403ecd

Merge branch 'develop' into feat/authorization

10490bf

PGijsbers marked this pull request as ready for review February 12, 2025 13:41

PGijsbers requested a review from Taniya-Das February 12, 2025 13:42

PGijsbers mentioned this pull request Feb 12, 2025

Establish Linkage between Keycloak Users and Metadata Catalogue Database #306

Open

Taniya-Das reviewed Feb 17, 2025

View reviewed changes

PGijsbers merged commit c4eca25 into develop Feb 18, 2025
1 check passed

PGijsbers deleted the feat/authorization branch February 18, 2025 11:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feat/authorization #435

Feat/authorization #435

PGijsbers commented Jan 30, 2025 •

edited

Loading

PGijsbers Feb 4, 2025

PGijsbers Feb 4, 2025

PGijsbers Feb 4, 2025

PGijsbers Feb 4, 2025

PGijsbers Feb 4, 2025

Taniya-Das left a comment

Taniya-Das Feb 17, 2025

PGijsbers commented Feb 18, 2025

Feat/authorization #435

Feat/authorization #435

Conversation

PGijsbers commented Jan 30, 2025 • edited Loading

Change

How to Test

Checklist

Related Issues

PGijsbers Feb 4, 2025

Choose a reason for hiding this comment

PGijsbers Feb 4, 2025

Choose a reason for hiding this comment

PGijsbers Feb 4, 2025

Choose a reason for hiding this comment

PGijsbers Feb 4, 2025

Choose a reason for hiding this comment

PGijsbers Feb 4, 2025

Choose a reason for hiding this comment

Taniya-Das left a comment

Choose a reason for hiding this comment

Taniya-Das Feb 17, 2025

Choose a reason for hiding this comment

PGijsbers commented Feb 18, 2025

PGijsbers commented Jan 30, 2025 •

edited

Loading