Fix management UI and API access permissions for #1425 #1431

MyPyDavid · 2025-09-22T14:34:44Z

Description

Related issue: #1425

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid · 2025-09-24T13:05:16Z

rdmo/management/rules.py



 # Add rules
-rules.add_rule('management.can_view_management',


this will break the backwards compatibility on the base_navigation template, should I keep the the add_rule in there just to not break the possible deployed custom templates?
Otherwise instance admins would need to update the theme for that. Think that it is better to use add_perm than add_rule..

I agree, but can we have both and remove one in RDMO 3?

Actually, I think add_rule fits better since this is just a predicate "exposed" to the user instance in views etc. I think we should use add_rule for all things can_... and add_perm for model/object permissions. management.can_view_management has no input model or instance. Same for management.can_upload_files and management.can_import_elements.

ChatGPT explained it nicely:

🔹 rules.add_rule(name, function)

What it does: Registers a predicate function under a name.

A predicate is just a Python function that takes (user, obj) and returns True or False.

Predicates are reusable building blocks.

Example:

import rules # Define a predicate @rules.predicate def is_staff(user): return user.is_staff # Register it under a name rules.add_rule('is_staff', is_staff)

Now you can reference "is_staff" as a condition in permissions or combine it with others.

🔹 rules.add_perm(name, rule)

What it does: Registers a permission under a name and attaches it to one or more rules (predicates).

A permission is basically a "named check" that can be used in your app (e.g. in user.has_perm() or decorators).

Example:

# Define a permission based on our predicate rules.add_perm('app.view_secret', is_staff)

Now, anywhere in Django, you can do:

user.has_perm('app.view_secret')

and it will use the is_staff predicate internally.

✅ Summary

add_rule → defines a predicate (logic building block).

add_perm → defines a permission that Django can check, usually built from rules/predicates.

Think of it like this:

Rule = ingredient

Permission = finished dish 🍲

ok now I see that its actually used as a permission in ManagementView so I guess has_perm is the better idea. Maybe we should remove the can_ from the perms then:

management.view_management, management.upload_files, management.import_elements

maybe even management.upload_elements?

Then the naming is consistent and management and elements are kind of "fake" models, if you know what I mean.

Same for projects.can_view_all_projects to projects.view_projects.

ok, Ive renamed the permissions like that. The has_perm is better for when it will be used on a user object anyway..

MyPyDavid · 2025-09-24T13:09:26Z

rdmo/core/permissions.py

        return super().has_permission(request, view)
+
+
+class HasRulesPermission(BasePermission):


this is for the Upload and Import viewsets

Nice! I checked the drf and rules code, because I was wondering if there is something like this already. This just introduces the permission_required class attribute in the same way as the regular django views, right? I would rename the class to HasPermission, since also all the other Permissions somehow depend on rules.

ok, HasPermission it is..

MyPyDavid · 2025-09-24T13:10:11Z

rdmo/management/viewsets.py

-
-    permission_classes = (IsAuthenticated, )
+    permission_classes = [HasRulesPermission]
+    permission_required = 'management.can_view_management'


is the meta used anywhere outside of the management as well??

No, I don't thin so.

Meta should stay IsAuthenticated since it is just the static information about fields and help texts etc.

jochenklar

Very nice, thanks! I have the usual naming requests and the lengthy discussion with myself (and ChatGPT) which I want to share with you 😃

jochenklar · 2025-09-28T08:43:56Z

rdmo/management/rules.py



 # Add rules
-rules.add_rule('management.can_view_management',


I agree, but can we have both and remove one in RDMO 3?

jochenklar · 2025-09-28T08:49:07Z

rdmo/management/rules.py



 # Add rules
-rules.add_rule('management.can_view_management',


Actually, I think add_rule fits better since this is just a predicate "exposed" to the user instance in views etc. I think we should use add_rule for all things can_... and add_perm for model/object permissions. management.can_view_management has no input model or instance. Same for management.can_upload_files and management.can_import_elements.

ChatGPT explained it nicely:

🔹 rules.add_rule(name, function)

What it does: Registers a predicate function under a name.

A predicate is just a Python function that takes (user, obj) and returns True or False.

Predicates are reusable building blocks.

Example:

import rules # Define a predicate @rules.predicate def is_staff(user): return user.is_staff # Register it under a name rules.add_rule('is_staff', is_staff)

Now you can reference "is_staff" as a condition in permissions or combine it with others.

🔹 rules.add_perm(name, rule)

What it does: Registers a permission under a name and attaches it to one or more rules (predicates).

A permission is basically a "named check" that can be used in your app (e.g. in user.has_perm() or decorators).

Example:

# Define a permission based on our predicate rules.add_perm('app.view_secret', is_staff)

Now, anywhere in Django, you can do:

user.has_perm('app.view_secret')

and it will use the is_staff predicate internally.

✅ Summary

add_rule → defines a predicate (logic building block).

add_perm → defines a permission that Django can check, usually built from rules/predicates.

Think of it like this:

Rule = ingredient

Permission = finished dish 🍲

jochenklar · 2025-09-28T08:51:06Z

rdmo/management/viewsets.py

-
-    permission_classes = (IsAuthenticated, )
+    permission_classes = [HasRulesPermission]
+    permission_required = 'management.can_view_management'


No, I don't thin so.

jochenklar · 2025-09-28T08:52:03Z

rdmo/management/viewsets.py

-
-    permission_classes = (IsAuthenticated, )
+    permission_classes = [HasRulesPermission]
+    permission_required = 'management.can_view_management'


Meta should stay IsAuthenticated since it is just the static information about fields and help texts etc.

jochenklar · 2025-09-28T08:58:52Z

rdmo/management/rules.py



 # Add rules
-rules.add_rule('management.can_view_management',


ok now I see that its actually used as a permission in ManagementView so I guess has_perm is the better idea. Maybe we should remove the can_ from the perms then:

management.view_management, management.upload_files, management.import_elements

maybe even management.upload_elements?

Then the naming is consistent and management and elements are kind of "fake" models, if you know what I mean.

jochenklar · 2025-09-28T08:59:45Z

rdmo/management/rules.py



 # Add rules
-rules.add_rule('management.can_view_management',


Same for projects.can_view_all_projects to projects.view_projects.

jochenklar · 2025-09-28T09:12:22Z

rdmo/core/permissions.py

+            return False
+
+        # prefer a view-level override when present
+        perm = getattr(view, 'permission_required', None)


The None is not needed and I would turn it around, check the perm if perm and raise the error at the bottom.

I now simply return:

# the viewset needs to set permission_required return request.user.has_perm(view.permission_required)

I think it should only be used like that anyway and break when we would forget to set a permission_required on the viewset.

jochenklar · 2025-09-28T09:13:12Z

rdmo/core/permissions.py

        return super().has_permission(request, view)
+
+
+class HasRulesPermission(BasePermission):


Nice! I checked the drf and rules code, because I was wondering if there is something like this already. This just introduces the permission_required class attribute in the same way as the regular django views, right? I would rename the class to HasPermission, since also all the other Permissions somehow depend on rules.

Signed-off-by: David Wallace <mypydavid@proton.me>

jochenklar

Great work! I think we are done here.

jochenklar · 2025-10-09T14:16:52Z

rdmo/core/templates/core/base_navigation.html

                {% back_to_project_link %}

-                {% test_rule 'management.can_view_management' request.user request.site as can_view_management %}
+                {% comment %}the test_rule will be removed in RDMO 3.0{% endcomment %}


I think here we can just use {% has_perm 'management.view_management' request.user as can_view_management %} only test_rule should still exist for the legacy templates.

ah yes, that would work without breaking it! 😅

yes, thanks, Ive changed it now to that.

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

jochenklar

🚀

…nterview-fixes Refactor ProjectOverviewSerializer

jochenklar · 2025-10-30T18:07:30Z

Ok I merged my subbranch so this can be merged now, I guess.

MyPyDavid added 7 commits September 22, 2025 16:20

tests(management): update tests for new rules #1425

7200381

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

accounts(models): add is_editor and is_reviewer as cached props

93d4b62

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

core(permissions): add HasRulesPermission for rules in DRF viewsets

060158a

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

core(tests): add multisite management and import perms

f343a59

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

mangement(rules): fix #1425 and add perms for upload and import

be85453

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

mangement(views): use permission_required instead

e656be5

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

mangement(viewsets): use HasRulesPermission for viewsets

74b902d

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid added this to the RDMO 2.3.3 milestone Sep 22, 2025

MyPyDavid self-assigned this Sep 22, 2025

MyPyDavid added 6 commits September 22, 2025 16:44

core(templates): use has_perm in base_navigation

c2d237a

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

accounts(tests): add tests for is_editor and is_reviewer props

a97e40e

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

tests(multisite): fix viewset tests detail and delete

20b2ccc

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

core(tests,multisite): update perms map and get function

99b9005

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

tests(multisite): fix import tests

8687feb

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

tests(multisite): fix assert line break syntax

6767a52

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid commented Sep 24, 2025

View reviewed changes

MyPyDavid requested a review from jochenklar September 24, 2025 13:17

jochenklar approved these changes Sep 28, 2025

View reviewed changes

David Wallace added 6 commits September 29, 2025 11:26

projects(rules): remove redundant rule

b6e2dc7

Signed-off-by: David Wallace <mypydavid@proton.me>

Revert MetaViewSet to IsAuthenticated

33ea939

Keep legacy rule in base navi

f0d714a

Rename to HasPermission and get perm directly from view

c95c317

Rename management perms

26b3c63

style, fix end of file

93220ee

Signed-off-by: David Wallace <mypydavid@proton.me>

MyPyDavid requested a review from jochenklar September 29, 2025 13:48

jochenklar approved these changes Oct 9, 2025

View reviewed changes

Refactor ProjectOverviewSerializer

55cffc1

MyPyDavid linked an issue Oct 27, 2025 that may be closed by this pull request

multisite: bug in can view management permission because of legacy rule (and extension of permissions to management api) #1425

Closed

Use view_management perm instead of can_view...

9d9dba1

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

MyPyDavid mentioned this pull request Oct 29, 2025

Refactor ProjectOverviewSerializer #1452

Merged

jochenklar and others added 2 commits October 30, 2025 17:24

Partially revert 9d9dba1

3f7ae88

core(templates): remove legacy test_rule from base_navigation

3558622

Signed-off-by: David Wallace <david.wallace@tu-darmstadt.de>

jochenklar approved these changes Oct 30, 2025

View reviewed changes

Merge pull request #1452 from rdmorganiser/fix-management-ui-access+i…

69385c7

…nterview-fixes Refactor ProjectOverviewSerializer

MyPyDavid merged commit f343dfc into 2.3.3 Oct 31, 2025
28 of 30 checks passed

MyPyDavid deleted the fix-management-ui-access branch October 31, 2025 08:41

		return super().has_permission(request, view)


		class HasRulesPermission(BasePermission):

Fix management UI and API access permissions for #1425 #1431

Fix management UI and API access permissions for #1425 #1431

Uh oh!

Conversation

MyPyDavid commented Sep 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

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

🔹 rules.add_rule(name, function)

🔹 rules.add_perm(name, rule)

✅ Summary

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!

Choose a reason for hiding this comment

Uh oh!

jochenklar Sep 28, 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!

jochenklar Sep 28, 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!

jochenklar 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

🔹 rules.add_rule(name, function)

🔹 rules.add_perm(name, rule)

✅ Summary

Uh oh!

jochenklar Sep 28, 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!

jochenklar Sep 28, 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!

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!

jochenklar 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!

jochenklar left a comment

Choose a reason for hiding this comment

Uh oh!

jochenklar commented Oct 30, 2025

MyPyDavid commented Sep 22, 2025 •

edited

Loading

🔹 `rules.add_rule(name, function)`

🔹 `rules.add_perm(name, rule)`

jochenklar Sep 28, 2025 •

edited

Loading

jochenklar Sep 28, 2025 •

edited

Loading

🔹 `rules.add_rule(name, function)`

🔹 `rules.add_perm(name, rule)`

jochenklar Sep 28, 2025 •

edited

Loading

jochenklar Sep 28, 2025 •

edited

Loading