Feat/setup permissions #86

EdoStorm96 · 2025-10-02T12:18:39Z

fixes #78

So here is my suggestion for how we handle permissions. I suggest:

Every ObjectType (eg. StudyType) receives specific methods for receiving a queryset which a user can access if they have specific permission types (eg. edit or view). These are viewable_queryset() and editable_queryset(). These are called via the object type's get_queryset() method. This method also receives, alongside the usual stuff, a permission type from MRPermissionTypes.
Every model receives (eg. Study) has specific methods for checking if a user has access to an instance of a model aka an object. These are can_be_viewed_by() and can_be_edited_by(). These are called via the model's can_be_accessed_by() method. This method receives a user and a permission type from MRPermissionTypes.
For queries of multiple objects, the ObjectType's get_queryset() is used and for doing queries or mutations on single models, the can_be_accessed_by() method gets used. These two methods might need to get placed in a mixin, since I foresee duplicating these for every ObjectType and Model will get redundant, but I am not a 100% sure if that will always work, so for now I've just left these where they are.

To get a proof of concept to work I've implemented a really basic Study model. I've also made queries and mutations for studies. For now, this can be tested using the GraphQLView (localhost:5000/backend/api/graphql). It all works fine in my experience.

I have also added 2 groups, which we'll definitely need according to our stakeholders:

Privacy officer
fetc member
I've added a migration which automatically loads these groups. Be aware of that :)

I have not yet added the Study model or users which are members of these groups to create_dev_data, although that would be better ...

How I would test this is to create a couple of superusers, so you can login via the admin interface, make one of them a member of the privacy officer group via the admin interface and start making studies through mutations and then running some queries to see if they act as they should.

I think this design is pretty nice, but I think we need to carefully consider the naming as this will probably be re-used quite a bit.

I'm looking forward to hearing your feedback!

fixes #77
I also added a markdown table with the overview of permissions for the MVP and possible future permissions once we absorb the FETC.

EdoStorm96 · 2025-10-02T12:26:29Z

Oh and there is a problem with black? It can't parse create_dev_data, which I haven't toouched in this branch ...

tijmenbaarda

I love the design! I still have to check whether it works properly by testing it with the some GraphQL queries, but here are some comments on the code.

backend/main/models.py

backend/study/mutations/StudyMutations.py

backend/study/models.py

backend/study/queries.py

backend/study/models.py

docs/MyResearchPermissions.md

backend/study/mutations/StudyMutations.py

RoordinkFrank · 2025-10-17T13:10:12Z

backend/study/types/StudyType.py

+
+    @classmethod
+    def viewable_queryset(
+        cls, user: User, queryset: QuerySet[Study]


edited

This overlaps with can_be_viewed_by. I assume GraphQL is the reason that is has to be this way? Would it be possible to get these two if statements in a separate method?

Will work on this. Xander is also asking me.

backend/study/types/StudyType.py

RoordinkFrank

I do really like the naming of everything you have made so far, looks great. Commnents are in the code.

XanderVertegaal

Thanks for putting all the work in; it already works quite well and I love the human-readable syntax! <3

Most of my comments are nitpicks, but the one about reusing shared logic is one I would insist on. Feel free to fight me on anything else.

backend/study/mutations/StudyMutations.py

backend/study/queries.py

backend/main/models.py

XanderVertegaal · 2025-10-21T08:49:40Z

backend/study/queries.py

+        StudyType,
+    )
+
+    def resolve_study(root, info: ResolveInfo, id) -> Optional[Study]:


Decorate these resolvers with @staticmethod. That gets rid of the yellow squiggly (if you have one): "Instance methods should take a "self" parameter."

XanderVertegaal · 2025-10-21T08:52:08Z

backend/study/queries.py

+        )
+        return queryset.all()
+
+    def resolve_my_study(root, info: ResolveInfo, id) -> Optional[Study]:


Instead of using 4 resolvers, it is shorter 2 and add an optional parameter editable to only see studies that you are allowed to edit.

Also, you may want to type id as id: str (or id: str | None if it is an optional param).

But id is an int right?

Django object IDs are int, but GraphQL IDs are treated as str, I believe. At least in DIAPP, where I tested this, type(id) yields str.

XanderVertegaal · 2025-10-21T09:28:32Z

backend/study/mutations/StudyMutations.py

+            )
+            return cls(errors=[error])
+
+        # If permission check gets passed, save() and return the study


I don't think this comment is needed.

backend/study/mutations/StudyMutations.py

XanderVertegaal · 2025-10-21T09:32:02Z

backend/study/mutations/StudyMutations.py

+
+        user = info.context.user
+        # Initialize the study, but do not save()
+        study = Study(title=title, created_by=user)


The GraphQL endpoint is not authenticated, so user may be AnonymousUser. When I try to create a user without logging in, I see: "Cannot assign \"<SimpleLazyObject: <django.contrib.auth.models.AnonymousUser object at 0x00000255E7457BF0>>\": \"Study.created_by\" must be a \"User\" instance."

Perhaps it's good to do an early return if the user is None or AnonymousUser.

XanderVertegaal · 2025-10-21T09:36:56Z

backend/main/models.py

+    PRIVACY_OFFICER = "Privacy officer"
+    FETC_MEMBER = "FETC member"


Could you put these constants in a TextChoices enum?

XanderVertegaal · 2025-10-21T09:38:44Z

backend/main/migrations/0002_load_group_fixtures.py

Interesting! I had no idea you could create dumps-as-migrations in this way. I'm not sure whether I am a fan of this. I think I'd rather keep fixtures/data separate from DB migrations. What do you think @miggol?

I'm a fan of using data migrations over fixtures wherever possible! Will elaborate in the meeting

Co-authored-by: Xander Vertegaal <[email protected]>

…umanities/myresearch into feat/setup-permissions

EdoStorm96 · 2025-10-22T10:12:06Z

Alright, thanks for all your valuable feedback. I've made a major overhaul of the design, which does retain the nice syntax, but keeps permission logic in a centralized place. To achieve this, I've chosen to always reason from querysets.

I've made a custom manager, which has some new methods. The most important one being accessible_objects(). This method can be called like so for example: Study.objects.accessible_objects(user, MRPermission.EDIT) and will only return objects that the user has access to based on the permission type. I've made a base version of this manager in main.utils.permission_utils, which can be inherited from for specific objects, see StudyManager for an example of this.

I've also retained the User.has_access_to() method trough a little workaround, but it all uses the same logic. This method is not used currently, but can come in handy.

Talking to @XanderVertegaal and @miggol, we realized this PR does not really cover create permissions. I have made a placeholder for this (Study.can_be_created_by: it just checks if the user is authenticated.), but this should be implemented more extensively later.

From my testing it is all still working well, but the code has really fallen into place much nicer! I'm curious to hear your further thoughts!

miggol

Just comments for now! Let's discuss this further in the meeting

miggol · 2025-10-21T12:40:40Z

docs/MyResearchPermissions.md

+| 1.2 Create                          | Access to own          | No access                  | Access to own     | Access to own     |
+| 1.3 Update                          | Access to own          | No access                  | Access to own     | Access to own     |
+| 1.4 Delete                          | Access to own          | No access                  | Access to own     | Access to own     |
+| 1.5 View PDF                        | Access to own          | No access                  | Access to own     | Access to own     |


You've explained to me that these two tables represent the pre and post submission situations. That could be made clearer in this documentation.

miggol · 2025-10-22T11:19:27Z

backend/myresearch/settings/generic_settings.py

 LOCAL_APPS = [
    "main",
    "form",
+    "study",


Now we're adding models I think it's worth having a talk about project structure, and how many distinct apps we want to end up with

miggol · 2025-10-22T12:32:48Z

backend/main/migrations/0002_load_group_fixtures.py

I'm a fan of using data migrations over fixtures wherever possible! Will elaborate in the meeting

miggol · 2025-10-22T12:43:39Z

backend/main/models.py

+    def has_access_to(self, object, mrpermission):
+        """
+        Utility function to check if a user has access to a specific object,
+        with a specific permission.
+        """
+        return object in object.__class__.objects.accessible_objects(self, mrpermission)


This could be (complemented by) can_view(self, object,) and can_edit(self, object,)

Personally I'd like to omit the opaque mrpermission argument wherever possible.

miggol · 2025-10-22T12:50:47Z

backend/main/models.py

+class MRPermission(models.TextChoices):
+    """
+    PermissionTypes are used in ObjectType's get_queryset method, to decide
+    which queryset gets returned or whether a user has access to an object,
+    based on the user's permissions.
+    """
+
+    VIEW = "View"
+    EDIT = "Edit"


I don't like mister permission. Passing around this extra argument, that can only have one of two values, to determine what you actually want to do? I would rather use two separate methods for viewing and editing wherever possible and forget about the argument. This makes for easier debugging as well.

For example, we could agree that StudyType.get_queryset just always means "view" by convention, and have a separate method for when we specifically want to access editable Studies.

Or we could annotate the get_queryset() results with an attribute stating if each Study is editable by the requesting user, provided both methods assuming editable studies will always be a subset of visible studies.

I don't know if that makes Graphene sense though, perhaps mrpermission is the right way of doing things over in GraphQL land. But even then I would prefer integrating that into the info object, perhaps @XanderVertegaal knows if this is possible.

EdoStorm96 added 7 commits October 1, 2025 16:13

feat: add group fixtures via migration

c306b8a

feat: create study app and Study model

1e026d9

feat: implement StudyType and StudyQuery

3c9eb45

fix: is_authenticated is property, not a function

c899d61

feat: add mutations

0228b8a

style: black

f0198c1

feat: add a markdown file with an overview of permissions.

749bf25

EdoStorm96 requested review from RoordinkFrank, XanderVertegaal, miggol and tijmenbaarda October 2, 2025 12:18

EdoStorm96 added 2 commits October 2, 2025 14:20

fix: remove print statement

fef9050

fix: improve docstring

5f35745

style: black

3175f61

EdoStorm96 mentioned this pull request Oct 15, 2025

Implement VWR registration and Roadmap models #88

Open

tijmenbaarda requested changes Oct 15, 2025

View reviewed changes

EdoStorm96 added 8 commits October 16, 2025 16:12

feat: make group name fixtures in User model

d1917d7

style: remove comma's after self arg for nicer look

43025cc

fix: try except in mutations for getting a study

10f41bf

fix: return study, not Study

8521110

fix: set max_length for Study.title

a540464

fix: fetc_members can view studies

805bfa8

fix: filenname for MyResearchPermissions

cabffc4

style: black

0b576cf

RoordinkFrank reviewed Oct 17, 2025

View reviewed changes

docs/MyResearchPermissions.md Show resolved Hide resolved

RoordinkFrank reviewed Oct 17, 2025

View reviewed changes

backend/study/mutations/StudyMutations.py Outdated Show resolved Hide resolved

RoordinkFrank reviewed Oct 17, 2025

View reviewed changes

backend/study/types/StudyType.py Outdated Show resolved Hide resolved

RoordinkFrank reviewed Oct 17, 2025

View reviewed changes

XanderVertegaal requested changes Oct 21, 2025

View reviewed changes

EdoStorm96 and others added 16 commits October 21, 2025 11:49

Update backend/study/types/StudyType.py

6e420c2

Co-authored-by: Xander Vertegaal <[email protected]>

Update docs/MyResearchPermissions.md

62e42c7

Co-authored-by: Xander Vertegaal <[email protected]>

Update backend/study/mutations/StudyMutations.py

3e70578

Co-authored-by: Xander Vertegaal <[email protected]>

feat: refactor mutations

b19ba66

Update backend/study/queries.py

9a07d0c

Co-authored-by: Xander Vertegaal <[email protected]>

Merge branch 'feat/setup-permissions' of github.com:CentreForDigitalH…

297f7df

…umanities/myresearch into feat/setup-permissions

fix use graphene.ID for study queries

1d8a398

fix: refactor MRPermission

00feb9e

fix: also rename references to MRPermission

d61f34b

feat: redo create permission

8255c09

feat: major overhaul, all querysets and custom managers

4df9a1a

fix: User.has_access_to()

b149c2f

fix: always return empty list (thx Xander)

b155618

fix: CreateStudyMutation

16bac4c

feat: group constants in MRGroups

054e7a5

style: black

7be80ad

EdoStorm96 requested review from RoordinkFrank, XanderVertegaal and tijmenbaarda October 22, 2025 12:04

miggol reviewed Oct 22, 2025

View reviewed changes

EdoStorm96 added 7 commits October 23, 2025 10:35

fix: rename load_fixtures function

743b8d0

fix: remove mrpermission from get_queryset

625421c

fix: rename mrpermission > mr_permission

e1b887e

feat: rename study app to research

2c373d4

Merge branch 'develop' into feat/setup-permissions

e268ed4

style: black

86d4b97

fix: clarify permission table

6857fa6

		PRIVACY_OFFICER = "Privacy officer"
		FETC_MEMBER = "FETC member"

Uh oh!

Feat/setup permissions #86

Are you sure you want to change the base?

Feat/setup permissions #86

Uh oh!

Conversation

EdoStorm96 commented Oct 2, 2025

Uh oh!

EdoStorm96 commented Oct 2, 2025

Uh oh!

tijmenbaarda left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

RoordinkFrank Oct 17, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RoordinkFrank left a comment

Choose a reason for hiding this comment

Uh oh!

XanderVertegaal left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

EdoStorm96 commented Oct 22, 2025

Uh oh!

miggol left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

RoordinkFrank Oct 17, 2025 •

edited

Loading