Skip to content
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.

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

Seperate user and non-user roles #1006

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Dec 11, 2024

Fixes #905, #561.

Changes the access policy tab of various details modals. User roles get displayed in a seperate table from the other roles, with user name and email. This should make it more intuitive to "give someone access to a video/ series". Functionality should not change, the resulting ACLs will look the same as before.

Caveat 1: If roles are sanitized, we cannot reliably derive the user from their user role. Therefore, this feature will be disabled if user role sanitization is enabled in Opencast. (The old admin ui allowed for sanitized roles and did some best effort guesswork to derive users from roles. Even though that approach worked for many, I'd rather not reintroduce such brittle code here.)

Caveat 2: I did not find a performant way to query for user information for each and every user. Therefore, if there are upwards of multiple thousands of users, the access policy tab will take several seconds to load.

Requires backend changes. Will break the UI otherwise. opencast/opencast#6382

How to test this patch

As this requires backend changes, the automatically generated stuff from github-actions below will not work.

Install the backend PR in your Opencast. Run this PR against your Opencast, like so PROXY_TARGET=http://localhost:8080 npm start. The sanitize flag can be found in your Opencast configuration files in etc/org.opencastproject.userdirectory.UserIdRoleProvider.cfg.

Screenshots:
Bildschirmfoto vom 2024-12-11 14-51-03

Bildschirmfoto vom 2024-12-11 14-52-56

Changes the access policy tab of various
details modals. User roles get displayed in a
seperate table from the other roles, with user
name and email. This should make it more
intuitive to "give someone access to a video/
series". Functionality should not change,
the resulting ACLs will look the same as before.

Caveat 1:  If roles are sanitized, we cannot
reliably derive the user from their user
role. Therefore, this feature will be disabled if
user role sanitization is enabled in Opencast.

Caveat 2: I did not find a performant  way to
query for user information for each and every
user.
Therefore, if there are upwards of multiple
thousands of users, the access policy tab will
take several seconds to load.
@Arnei Arnei added the type:enhancement New feature or request label Dec 11, 2024
Copy link
Contributor

Use docker or podman to test this pull request locally.

Run test server using develop.opencast.org as backend:

podman run --rm -it -p 127.0.0.1:3000:3000 ghcr.io/opencast/opencast-admin-interface:pr-1006

Specify a different backend like stable.opencast.org:

podman run --rm -it -p 127.0.0.1:3000:3000 -e PROXY_TARGET=https://stable.opencast.org ghcr.io/opencast/opencast-admin-interface:pr-1006

It may take a few seconds for the interface to spin up.
It will then be available at http://127.0.0.1:3000.
For more options you can pass on to the proxy, take a look at the README.md.

Copy link
Contributor

github-actions bot commented Dec 11, 2024

This pull request is deployed at test.admin-interface.opencast.org/1006/2025-01-17_11-28-12/ .
It might take a few minutes for it to become available.

Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@snoesberger
Copy link
Contributor

I have tested this PR together with opencast/opencast#6382 and have a few remarks.

  • With the implementation in the old admin UI, we had the ability to hide either the non-user or the user role part from users.
    This was possible with the following roles:

    • ROLE_UI_SERIES_DETAILS_ACL_NONUSER_ROLES_VIEW
    • ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW
    • ROLE_UI_SERIES_DETAILS_ACL_USER_ROLES_VIEW
    • ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW

    These roles don't have any effect in the new admin UI. In our case, users are only allowed to add user roles. So it's important to us that users don't have the ability to change non-user roles.

  • When I add a new role, only the role ID is displayed in the dropdown for selection. In the case of user roles, this makes it difficult to know which user the ID represents. It would be better if the name and email address were displayed in the dropdown. When a role is selected, the name and email address are already displayed.

grafik

@Arnei
Copy link
Member Author

Arnei commented Dec 19, 2024

Thanks for testing, will look into these issues!

The options in the dropdown for selecting a user in the
access policy tab would show the user role
(aka the thing actually being selected) instead of
the user information like in the input field.
This fixes that.

Also adds some sensible fallbacks in case someone
decides to have users without names.
Add support for the access roles
ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW
ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW
ROLE_UI_SERIES_DETAILS_ACL_USER_ROLES_VIEW
ROLE_UI_SERIES_DETAILS_ACL_NONUSER_ROLES_VIEW
@Arnei
Copy link
Member Author

Arnei commented Dec 19, 2024

Both issues should now be fixed.

Copy link
Contributor

github-actions bot commented Jan 3, 2025

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@gregorydlogan
Copy link
Member

Should an event with ROLE_USER_x be visible to user x? I would, perhaps naively, assume that if I give a user read and/or write that they'd be able to see it in the admin UI...

Otherwise works correctly.

@Arnei
Copy link
Member Author

Arnei commented Jan 15, 2025

Could you specify what exactly is different from how you expect it? Which combination of role and actions does not do what you would think it do?

@snoesberger
Copy link
Contributor

Thanks @Arnei for addressing my comments.
I'm now able to hide the non-user roles in the event/series modal and I now have the proper user name and email to choose from. But I did also found some additional problems:

  • When I create a new event, the Access Policy tab to add roles is displayed even though the user hasn't assigned ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW, ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW and/or ROLE_UI_EVENTS_DETAILS_ACL_EDIT:
    grafik
  • In the dropdown to add a new user role, I can only search for the user role ID. It would be much more convenient if I could search by name or email:
    grafik

@Arnei
Copy link
Member Author

Arnei commented Jan 17, 2025

When I create a new event, the Access Policy tab to add roles is displayed even though the user hasn't assigned ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW, ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW and/or ROLE_UI_EVENTS_DETAILS_ACL_EDIT

To make sure I understand you correctly: If the user does not have any of the roles mentioned above, the "Access Policy" tab should not be displayed at all. For example in your screenshot, the user would go from "Processing" straight to "Summary". Is that right? What roles should the event have in that case?

I would be tempted to leave fixing that to a different PR, since for me this raises questions about what the role ROLE_UI_EVENTS_DETAILS_ACL_EDIT means, but also about what the role ROLE_UI_EVENTS_DETAILS_ACL_VIEW means. At least the more specific roles ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW and ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW are working as intended?

@snoesberger
Copy link
Contributor

To make sure I understand you correctly: If the user does not have any of the roles mentioned above, the "Access Policy" tab should not be displayed at all. For example in your screenshot, the user would go from "Processing" straight to "Summary". Is that right? What roles should the event have in that case?

Yes, the user would go from "Processing" straight to "Summary". In our case we have the ACL merge mode "actions" active, this means the roles for the series are valid for such an event (and the series is mandatory for each event).

I would be tempted to leave fixing that to a different PR, since for me this raises questions about what the role ROLE_UI_EVENTS_DETAILS_ACL_EDIT means, but also about what the role ROLE_UI_EVENTS_DETAILS_ACL_VIEW means. At least the more specific roles ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW and ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW are working as intended?

Personally, I see the purpose of the different roles as follows:

  • ROLE_UI_EVENTS_DETAILS_ACL_VIEW: The "Access Policy" tab in the create or edit dialog is displayed to the user. Without the roles ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW and ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW only the "Templates" dropdown is displayed to the user. The user can't edit the ACLs.
  • ROLE_UI_EVENTS_DETAILS_ACL_EDIT: The user is allowed to edit the ACLs of an event. Without the roles ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW and ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW only the "Template" can be changed by the user.
  • ROLE_UI_EVENTS_DETAILS_ACL_USER_ROLES_VIEW: The user roles part in the "Access Policy" tab in the create or edit dialog is displayed to the user. If the user has ROLE_UI_EVENTS_DETAILS_ACL_EDIT assigned, they are also allowed to edit the user roles.
  • ROLE_UI_EVENTS_DETAILS_ACL_NONUSER_ROLES_VIEW: The non-user roles part in the "Access Policy" tab in the create or edit dialog is displayed to the user. If the user has ROLE_UI_EVENTS_DETAILS_ACL_EDIT assigned, they are also allowed to edit the non-user roles.

For us, the split view for ACLs only makes sense if we can hide the non-user roles part in both the create and edit dialog. So I'd prefer to have the ACL view roles issue fixed in this PR. But if it is much easier for you to address this in another PR, then go ahead.

Arnei added 2 commits January 17, 2025 12:07
This should make the search for searchable
dropdowns work as expected, meaning the
search string is now matched correctly against
the existing labels. The custom filtering this patch
removes only serves to break the search, and what
it tries to achieve the 'react-select' search already
does out of the box.
In the create event modal, do not display
the step "Access policy" if the user is missing
the role "ROLE_UI_EVENTS_DETAILS_ACL_VIEW".
The step gets completely skipped. Per default,
this results in an ACL with read/write access for
the user.
@Arnei
Copy link
Member Author

Arnei commented Jan 17, 2025

Thanks for the very detailed explanation on the roles, that helped me a lot. I made a change so that the user requires the role "ROLE_UI_EVENTS_DETAILS_ACL_VIEW" to see the "Access Policy" step in the "Create Event" modal. This should work the default merge mode as well, and result in the other roles working as intended. Please test again.

In the dropdown to add a new user role, I can only search for the user role ID. It would be much more convenient if I could search by name or email:

Searching should now work as intended.

Copy link
Contributor

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Seperate user and non-user roles in the access policy modal
3 participants