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

Space Management Permissions #3609

Merged
merged 7 commits into from
Jan 25, 2023
Merged

Conversation

kobergj
Copy link
Contributor

@kobergj kobergj commented Jan 18, 2023

implements "manage properties" and "disable space" permissions (naming wip)

@update-docs
Copy link

update-docs bot commented Jan 18, 2023

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.

@kobergj kobergj force-pushed the SpaceManagementPermissions branch 4 times, most recently from 5ddf5f0 to c51c8ab Compare January 20, 2023 12:27
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj marked this pull request as ready for review January 24, 2023 09:39
Signed-off-by: jkoberg <jkoberg@owncloud.com>

// CreateSpace returns true when the user is allowed to create the space
func (p Permissions) CreateSpace(ctx context.Context, spaceid string) bool {
return p.checkPermission(ctx, "create-space", spaceRef(spaceid))
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer to use a naming scheme for permissions that is similar to ms graph permissions: {resource}.{operation}.{constraint}. Here we want to check if the user has the permission to create project spaces

Suggested change
return p.checkPermission(ctx, "create-space", spaceRef(spaceid))
return p.checkPermission(ctx, "drive.create.all", spaceRef(spaceid))

When using spicedb, this code would change to sth like

resp, err := client.CheckPermission(ctx, &pb.CheckPermissionRequest{
		Resource: organization,
		Permission: "drive.create.all",
		Subject: user,
	})

In SpiceDB we would model our permissions in relation to an organization: see the can_admin example at https://authzed.com/blog/check-it-out/

Anyway, naming is hard ... and ms may not have gotten it right as well. While they have eg. Files.ReadWrite.All the resources this affects are actually drive and driveItem, so the actual permissions should be Drive.ReadWrite.All and DriveItem.ReadWrite.All. But they also don't have explicit permissions to manage drives.

@C0rby just educated me on scopes vs permissions:

The permission check here is one of two checks that should ultimately be done:

  1. a scope check to make sure the user gave his consent, if not redirect him to the idp and ask for the scope. way out of scope of this PR though ;-) the scope check can be done as early as in the ocis graph service.
  2. an actual permissions check if the user is allowed to access a resource or has a certain role. This is done when the concrete subject, permission and resource ids have been determined.

These are two different things which we need to clarify in the docs.

Trying to wrap my head around how this relates to customizing roles and permissions relationships with spacedb:

  • we would define permissions in a spacedb schema as a spicedb permission in an organization/tenant/instance? class
  • the composition of permissions that belong to a role, aka user defined roles, see https://authzed.com/blog/user-defined-roles/ for how that is modeled with spicedb

It seems to me that we can reuse the permissions naming scheme from the graph api for organizational permissions.

Copy link
Contributor Author

@kobergj kobergj Jan 25, 2023

Choose a reason for hiding this comment

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

As discussed following new naming conventions (Drive.(Read)(Write){Property}) for new permissions while old ones stay the same


// SetSpaceQuota returns true when the user is allowed to change the spaces quota
func (p Permissions) SetSpaceQuota(ctx context.Context, spaceid string) bool {
return p.checkPermission(ctx, "set-space-quota", spaceRef(spaceid))
Copy link
Contributor

@butonic butonic Jan 25, 2023

Choose a reason for hiding this comment

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

why do we need to differentiate between set quota and manage space properties?

Suggested change
return p.checkPermission(ctx, "set-space-quota", spaceRef(spaceid))
return p.checkPermission(ctx, "drive.writequota", spaceRef(spaceid))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is a question I cannot answer. It has been a requirement for the ticket.

Copy link
Contributor

@butonic butonic left a comment

Choose a reason for hiding this comment

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

We should merge this without renaming existing permissions. Maybe you can prefix the new permissions with Drive. and then use (Read)(Write){Property}, eg. Drive.ReadWriteEnabled and Drive.ReadWriteQuota for permissions on specific properties.? Renaming existing permissions should be deferred until moving to spicedb.

Signed-off-by: jkoberg <jkoberg@owncloud.com>
@kobergj kobergj merged commit 2d125a5 into cs3org:edge Jan 25, 2023
@kobergj kobergj deleted the SpaceManagementPermissions branch January 25, 2023 14:20
@micbar micbar mentioned this pull request May 3, 2023
89 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.

2 participants