-
Notifications
You must be signed in to change notification settings - Fork 159
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
refactor: remove share role checks #11364
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. |
463fa2e
to
aa657fc
Compare
ed9d7ae
to
1401f72
Compare
21cb63f
to
0c76d43
Compare
if (resources[0].disabled) { | ||
return false | ||
} | ||
|
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.
This is already being checked in canEditReadme
.
7b965f0
to
ad41acc
Compare
0bc1c92
to
136c7e7
Compare
136c7e7
to
58f0563
Compare
canDownload: () => | ||
sharePermissions.includes(GraphSharePermission.readBasic) && | ||
!shareRoles.some((shareRole) => shareRole.id === GraphShareRoleIdMap.SecureViewer), | ||
canDownload: () => sharePermissions.includes(GraphSharePermission.readContent), |
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.
So do I get it right that readContent
permission action is not given with a secure view share? Which permission action does the secure view share have for secure-viewing content?
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.
I need to check, not sure if there are any permissions 😅
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.
Okay so the permissions of a secure view share are these:
* libre.graph/driveItem/path/read
* libre.graph/driveItem/children/read
* libre.graph/driveItem/basic/read
packages/web-app-admin-settings/src/components/Spaces/SideBar/MembersPanel.vue
Show resolved
Hide resolved
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.
Thank you for this! I can imagine that it has been a tedious chunk of work. 🤗
Quality Gate passedIssues Measures |
Description
Use share permissions instead of roles since roles are a very loose concept and just act as a container for a specific set of share permissions.
This affects 2 main scenarios:
SpaceResource
doesn't have properties likemanager
,editor
anymore. The same goes for methods likeisManager
. Instead, there is themembers
property that holds information about all members. When checking if the current user is allowed to do certain actions, we need check for specific permissions now. E.g. when sharing, don't check forisManager
but forcanShare
.GraphShareRoleIdMap
is being removed.You may notice a discrepancy in our naming and the Graph naming. IMO we should align... but that's a another story, I guess. Because ideally, I would also rename
Space
toDrive
. It's just so confusing all the time.There is one exception where Web still needs to assume and work with roles. That is when listing the managers of a space. Here we imply that if the user can remove space members, they are a space manager.
Related Issue
Types of changes