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

feat: add secure view share role #4643

Merged
merged 2 commits into from
Apr 22, 2024

Conversation

JammingBen
Copy link

@JammingBen JammingBen commented Apr 18, 2024

Adds a new share role "Secure view". This role only allows viewing resources, no downloading, editing or deleting.

I'm not 100% sure how the exact permissions will end up looking once this role has been refined (see owncloud/ocis#8855), but it's a placeholder to get started.

Adds a new share role "Secure view". This role only allows viewing resources, no downloading, editing or deleting.
@JammingBen JammingBen marked this pull request as ready for review April 18, 2024 11:45
@JammingBen JammingBen requested review from labkode, glpatcern and a team as code owners April 18, 2024 11:45
@fschade fschade requested review from butonic and micbar and removed request for labkode and glpatcern April 18, 2024 12:00
@micbar
Copy link
Member

micbar commented Apr 18, 2024

@JammingBen Thanks for your first contribution to reva 🎉

Although, i wonder why we need that in reva. IMHO we need that in the ocis graph package.

@kobergj @rhafer why reva?

@rhafer
Copy link
Contributor

rhafer commented Apr 18, 2024

@kobergj @rhafer why reva?

@micbar Current every role defined in graph (https://github.com/owncloud/ocis/blob/master/services/graph/pkg/unifiedrole/unifiedrole.go) maps to a reva role. Why would it be different for the SecureView role?

Copy link
Member

@micbar micbar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. From my POV we should work with these permissions now.

pkg/conversions/role.go Show resolved Hide resolved
Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍 Just question about naming

@@ -52,6 +52,8 @@ const (
RoleUploader = "uploader"
// RoleManager grants manager permissions on a resource. Semantically equivalent to co-owner.
RoleManager = "manager"
// RoleSecureView grants secure view permissions on a resource or space.
RoleSecureView = "secure-view"
Copy link
Contributor

@kobergj kobergj Apr 22, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we sure about this naming? I mean "viewer", "editor", "uploader", "manager", ... Shouldn't this be something like "secure-viewer"?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch, yes

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True, I'll adjust the naming 👍

Copy link
Contributor

@kobergj kobergj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@JammingBen JammingBen merged commit 51ab765 into cs3org:edge Apr 22, 2024
9 checks passed
nirajacharya2 pushed a commit to JankariTech/reva that referenced this pull request Apr 30, 2024
@micbar micbar mentioned this pull request Jun 19, 2024
24 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants