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

[MyPyDavid]

Description / Beschreibung

It seems that in a multisite setup, where a user with role of editor for site "foo" can also (mistakenly) see the management in site "bar".
The cause of the bug seems to be in the is_legacy_reviewer because the perms that are checked in there do only check for the is_editor and not for the is_editor_for_current_site .

In addition, also the management viewsets should be protected by higher permissions than only "is authenticated".

Expected behaviour / Erwartetes Verhalten

The management access should be site specific according to the assigned roles of the user.

Steps to reproduce / Schritte zum Reproduzieren

1. Assign an editor to site Foo
2. Login as that user in site Bar and see the Management in navbar

Context / Kontext

2.3.2

References / Verweise

• rdmo/rdmo/management/rules.py

  Line 92 in 79917de

  is_legacy_reviewer))

  • rdmo/rdmo/management/rules.py

    Line 97 in 79917de

    rules.add_perm('auth.view_group', is_editor | is_reviewer)

• rdmo/rdmo/management/viewsets.py

  Line 34 in 79917de

  permission_classes = (IsAuthenticated, )

• rdmo/rdmo/management/viewsets.py

  Line 69 in 79917de

  permission_classes = (IsAuthenticated, )

[MyPyDavid]

proposed solution:
to use the cached version of Role.is_editor that checks if the user is an editor for the current site.
The current is_editor_for_current_site can be renamed to is_editor and the filter logic goes into a new Role models' @cached_property is_editor method. The caching is an optimization since this query was made a lot of times for each request.
So that it will always be checked for the current site (via settings.SITE_ID).

I still think that the is_legacy_reviewer should not be checked at all when settings.MULTISITE is True because it should not be evaluated at all in that case.

[jochenklar]

I still think that the is_legacy_reviewer should not be checked at all when settings.MULTISITE is True because it should not be evaluated at all in that case.

fine with me.