-
Notifications
You must be signed in to change notification settings - Fork 156
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
Implement the spaces permission concept #6531
Conversation
7da85df
to
7a4b939
Compare
a4ceff9
to
e311926
Compare
Results for e2e-tests oC10 https://drone.owncloud.com/owncloud/web/23342/10/1 💥 To see the trace, please open the link in the console ...
npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23342/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-alice-2022-3-7-11-42-49.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23342/tracing/alice-creates-public-link-to-a-folder-alice-2022-3-7-11-43-56.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23342/tracing/alice-shares-file-to-brian-alice-2022-3-7-11-45-06.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23342/tracing/alice-shares-folder-with-file-to-brian-alice-2022-3-7-11-46-15.zip |
e311926
to
b079b22
Compare
Results for e2e-tests oC10 https://drone.owncloud.com/owncloud/web/23379/10/1 💥 To see the trace, please open the link in the console ...
npx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23379/tracing/alice-can-share-this-weeks-meal-plan-with-all-parents-alice-2022-3-8-08-16-38.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23379/tracing/alice-creates-public-link-to-a-folder-alice-2022-3-8-08-17-48.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23379/tracing/alice-shares-file-to-brian-alice-2022-3-8-08-19-04.zipnpx playwright show-trace https://cache.owncloud.com/public/owncloud/web/23379/tracing/alice-shares-folder-with-file-to-brian-alice-2022-3-8-08-20-19.zip |
Results for oC10SharingExternal https://drone.owncloud.com/owncloud/web/23380/41/1 |
Results for oC10SharingFolderAdvPermsGrp https://drone.owncloud.com/owncloud/web/23380/39/1 |
Results for oC10SharingPublic1 https://drone.owncloud.com/owncloud/web/23380/36/1 |
Results for oC10SharingIntGroups https://drone.owncloud.com/owncloud/web/23383/27/1 |
Results for oC10SharingExternal https://drone.owncloud.com/owncloud/web/23383/41/1 |
Results for oC10SharingIntUsers1 https://drone.owncloud.com/owncloud/web/23383/29/1 |
Results for oC10Upload https://drone.owncloud.com/owncloud/web/23383/38/1 |
const graphClient = clientService.graphAuthenticated(instance, token) | ||
|
||
let graphUser | ||
if (context.rootState.user.version.edition === 'reva') { |
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.
That's a no-no. We don't want to differentiate backends. Either you can determine if the graph api is available (e.g. graphClient
could have an available()
getter which makes a ping (basically any request) via graph API, if that fails we remember in a variable that the graphClient is not available. Or you find some other attribute how to check if the graph API is available. E.g. introduce a capability in ocis.
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.
Ahh okay, I though this is legit as we have several isOcis
-checks floating around. I'll discuss with the backend team what's most suitable here. A capability sounds nice.
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.
As discussed in chat: We can use capabilities.spaces.enabled
for now.
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.
But I like the idea with the available()
getter. Let me introduce that one at least.
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.
The existing isOcis
checks are deprecated and will be removed soonish. :-)
available()
getter which wraps the capabilities check is superb 👍 then it's easy to switch over to proper graph
permissions later on.
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.
However, I'd need to inject the capabilities into the graphClient
(hence editing all places where we fetch it). There is no way around it, right @kulmann ?
In conrete: clientService.graphAuthenticated(instance, token)
-> clientService.graphAuthenticated(instance, token, capabilities)
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've created #6551 to keep track on this. I think this PR is fine without for now.
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.
see comment
e1e2389
to
44bfb7d
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.
LGTM
Results for oCISTrashbinUploadMoveJourney https://drone.owncloud.com/owncloud/web/23387/69/1
|
e226886
to
9292650
Compare
Kudos, SonarCloud Quality Gate passed! |
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.
💪
Description
We've implemented the spaces permission concept and improved the code structure for further permission changes.
Note that this PR does not yet include the permission check for the "Create New Space"-button. The permission for this is bound to the current user role, which needs some more in-depth work.
Related Issue
Types of changes
Checklist: