-
Notifications
You must be signed in to change notification settings - Fork 196
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
Enhancement: Unified Roles Management #9727
Conversation
Thanks for opening this pull request! The maintainers of this repository would appreciate it if you would create a changelog item based on your changes. |
259328c
to
24f9894
Compare
c158848
to
03dc9c3
Compare
@AlexAndBear, @JammingBen, @kulmann, @butonic, @tbsbdr FYI: the task could lead to some side-effects, please read my description above, specially the caveats (role disabling 1 & 2). I'm on vacation tomorrow, i will be back on Monday, happy to get some feedback. |
03dc9c3
to
960fb8f
Compare
I don't like that. The sole purpose if this field is to indicate which roles can be used for sharing. Since a disabled role can't be used though, it would be very confusing. Still, we need to somehow inform the user about the permissions of a disabled role. Simply naming it "Disabled role" won't be enough I think. Is there a way to get this kind of information? Web is querying all graph permissions globally via In general, how I would expect the API: The server always provides a role for an existing share, no matter if enabled or disabled. It also provides all roles via the global Also, note that #9765 adds to more role definitions for OCM. I guess those will be deactivated by default? In this case we need to add this to the OCM documentation. And we need to think about the case when OCM is enabled without the OCM roles. Which basically means that no OCM shares can be created. |
i talked to @butonic today, and we came to the point that a role is nothing more than a set of permissions. Therefore, it makes little sense to list a disabled role via Right after that, @tbsbdr , @JammingBen and I had a discussion what we should do with the roles that are not enabled anymore (e.g., the role permissions). We came to the solution that we list permissions in the sharing dialog if there is no matching role (that only applies to orphaned role-permissions). Jannik and myself pair on it tomorrow and make it happen. |
84e8931
to
a559fed
Compare
from now on, not all unified roles are enabled by default, instead the available roles are hand-picked in the default setup. For advanced use-cases, the administrator is capable to enable the desired set of available roles. Picking roles is not easy since the uid is NOT humanly readable, therefore a cli is contained which lists the available, disabled and enabled roles.
a559fed
to
4f26783
Compare
…g space permissions to libregraph permissions
8629ecc
to
43ba951
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Needs a section in the graph readme. If you just copy your changelog you already have 90% imo.
cc @mmattel
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good codewise but please talk to @mmattel if and how we need to document this.
2a970af
to
803a0a5
Compare
Sorry for the accidental close - merging owncloud/web#11387 caused this because of the GitHub "fixes" keyword. |
@fschade with regards to documentation:
I will of course help you and when set take care for the admin docs part as usual 😄 |
6d28324
to
4b09399
Compare
|
Enhancement: Unified Roles Management
Description
Improved management of unified roles with the introduction of default enabled/disabled states and a new command for listing available roles.
It is important to note that a disabled role does not lose previously assigned permissions;
it only means that the role is not available for new assignments.
The following roles are now enabled by default:
The following roles are now disabled by default:
To enable the UnifiedRoleSecureViewer role, you must provide a list of all available roles through one of the following methods:
To enable a role, include the UID of the role in the list of available roles.
A new command has been introduced to simplify the process of finding out which UID belongs to which role. The command is:
The output of this command includes the following information for each role:
Caveats:
Role disabling 1
Let's imagine the following scenario:
which leads to the following consequences:
we need to answer the following questions:
ideas:
screenshots:
Role disabling 2
With the current implementation, a disabled role is ignored in the system, like if it doesn't exist (the role, not the permissions).
This means that if a role is disabled, it is not possible to assign it, and it is not possible to check if that role is assigned.
It is also important to know that a disabled role won't be part of ANY api response, so if a share has a role assigned and that role is disabled, the user will not see that role in the response.
the same applies to requests (e.g. POST, PUT, ...), the role is considered not existing,
the validation fails if such a role is part of the request.
I talked to alex about this, and he said that he would prefer:
@libre.graph.permissions.roles.allowedValues
list (which is used by web to display the role dropdown), but is marked as disabled.I'm not against that, its fairly simple change, but I'm not sure if it is the best approach because:
@libre.graph.permissions.roles.allowedValues
list.Tasks:
Related Issue
How Has This Been Tested?
Types of changes