From 9ca9b78b2b1f3265b399b7be82ba301c3bba64a7 Mon Sep 17 00:00:00 2001 From: Ralf Haferkamp Date: Tue, 9 Apr 2024 16:47:53 +0200 Subject: [PATCH] fix(sharing-ng): Adapt role conditions to match resource types This switches our hardcode unfied role conditions to better reflect what we're actually using them before. The new conditions also allow to differentiate between roles elgitible for files, folders or drive roots. Which means that the `/permissions` endpoint is now able to populate the `roles.allowedValues` field with the correct roles for type of the resource it is called for. Fixes: #8331 --- .../unreleased/fix-graph-allowed-roles.md | 9 +++ .../service/v0/api_driveitem_permissions.go | 17 ++-- .../v0/api_driveitem_permissions_test.go | 76 ++++++++++++++--- services/graph/pkg/service/v0/base.go | 68 ++++++++++------ .../graph/pkg/service/v0/sharedbyme_test.go | 3 +- services/graph/pkg/service/v0/utils.go | 32 ++++++-- services/graph/pkg/unifiedrole/unifiedrole.go | 56 +++++++------ .../graph/pkg/unifiedrole/unifiedrole_test.go | 81 +++++++------------ .../apiSharingNg/listPermissions.feature | 41 +--------- .../apiSharingNg/shareInvitations.feature | 8 +- 10 files changed, 217 insertions(+), 174 deletions(-) create mode 100644 changelog/unreleased/fix-graph-allowed-roles.md diff --git a/changelog/unreleased/fix-graph-allowed-roles.md b/changelog/unreleased/fix-graph-allowed-roles.md new file mode 100644 index 000000000..bfd334e41 --- /dev/null +++ b/changelog/unreleased/fix-graph-allowed-roles.md @@ -0,0 +1,9 @@ +Bugfix: Validate conditions for sharing roles by resource type + +We improved the validation of the allowed sharing roles for specific resource type +for various sharing related graph API endpoints. This allows e.g. the web client to +restrict the sharing roles presented to the user based on the type of the resource +that is being shared. + +https://github.com/owncloud/ocis/pull/8815 +https://github.com/owncloud/ocis/issues/8331 diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index 1fd6caa56..6070dd3ae 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -88,10 +88,9 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId stor return libregraph.Permission{}, *errCode } - resourceInfo := statResponse.GetInfo() - condition := unifiedrole.UnifiedRoleConditionGrantee - if IsSpaceRoot(resourceInfo.GetId()) { - condition = unifiedrole.UnifiedRoleConditionOwner + var condition string + if condition, err = roleConditionForResourceType(statResponse.GetInfo()); err != nil { + return libregraph.Permission{}, err } unifiedRolePermissions := []*libregraph.UnifiedRolePermission{{AllowedResourceActions: invite.LibreGraphPermissionsActions}} @@ -116,7 +115,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId stor cs3ResourcePermissions := unifiedrole.PermissionsToCS3ResourcePermissions(unifiedRolePermissions) createShareRequest := &collaboration.CreateShareRequest{ - ResourceInfo: resourceInfo, + ResourceInfo: statResponse.GetInfo(), Grant: &collaboration.ShareGrant{ Permissions: &collaboration.SharePermissions{ Permissions: cs3ResourcePermissions, @@ -185,7 +184,7 @@ func (s DriveItemPermissionsService) Invite(ctx context.Context, resourceId stor if id := createShareResponse.GetShare().GetId().GetOpaqueId(); id != "" { permission.Id = conversions.ToPointer(id) - } else if IsSpaceRoot(resourceInfo.GetId()) { + } else if IsSpaceRoot(statResponse.GetInfo().GetId()) { // permissions on a space root are not handled by a share manager so // they don't get a share-id permission.SetId(identitySetToSpacePermissionID(permission.GetGrantedToV2())) @@ -232,9 +231,9 @@ func (s DriveItemPermissionsService) ListPermissions(ctx context.Context, itemID return collectionOfPermissions, err } - condition := unifiedrole.UnifiedRoleConditionGrantee - if IsSpaceRoot(statResponse.GetInfo().GetId()) { - condition = unifiedrole.UnifiedRoleConditionOwner + var condition string + if condition, err = roleConditionForResourceType(statResponse.GetInfo()); err != nil { + return collectionOfPermissions, err } permissionSet := *statResponse.GetInfo().GetPermissionSet() diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go index 3c2d25bb8..4b6764280 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go @@ -103,6 +103,10 @@ var _ = Describe("DriveItemPermissionsService", func() { BeforeEach(func() { gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) + statResponse.Info = &provider.ResourceInfo{ + Id: &driveItemId, + Type: provider.ResourceType_RESOURCE_TYPE_FILE, + } getGroupResponse = &grouppb.GetGroupResponse{ Status: status.NewOK(ctx), @@ -158,7 +162,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission.GrantedToV2.Group.GetId()).To(Equal("2")) }) - It("with roles (happy path)", func() { + It("succeeds with file roles (happy path)", func() { gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) driveItemInvite.Recipients = []libregraph.DriveRecipient{ @@ -173,7 +177,23 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission.GetRoles()[0]).To(Equal(unifiedrole.NewViewerUnifiedRole().GetId())) }) - It("fails with wrong role", func() { + It("succeeds with folder roles (happy path)", func() { + statResponse.Info.Type = provider.ResourceType_RESOURCE_TYPE_CONTAINER + gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) + gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + driveItemInvite.Roles = []string{unifiedrole.NewEditorUnifiedRole().GetId()} + + permission, err := driveItemPermissionsService.Invite(context.Background(), driveItemId, driveItemInvite) + Expect(err).ToNot(HaveOccurred()) + + Expect(permission.GetRoles()).To(HaveLen(1)) + Expect(permission.GetRoles()[0]).To(Equal(unifiedrole.NewEditorUnifiedRole().GetId())) + }) + + It("fails with when trying to set a space role on a file", func() { driveItemInvite.Recipients = []libregraph.DriveRecipient{ {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, } @@ -184,6 +204,29 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(permission).To(BeZero()) }) + It("fails with when trying to set a folder role on a file", func() { + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + driveItemInvite.Roles = []string{unifiedrole.NewEditorUnifiedRole().GetId()} + permission, err := driveItemPermissionsService.Invite(context.Background(), driveItemId, driveItemInvite) + + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource"))) + Expect(permission).To(BeZero()) + }) + + It("fails with when trying to set a file role on a folder", func() { + statResponse.Info.Type = provider.ResourceType_RESOURCE_TYPE_CONTAINER + driveItemInvite.Recipients = []libregraph.DriveRecipient{ + {ObjectId: libregraph.PtrString("1"), LibreGraphRecipientType: libregraph.PtrString("user")}, + } + driveItemInvite.Roles = []string{unifiedrole.NewFileEditorUnifiedRole().GetId()} + permission, err := driveItemPermissionsService.Invite(context.Background(), driveItemId, driveItemInvite) + + Expect(err).To(MatchError(errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource"))) + Expect(permission).To(BeZero()) + }) + It("with actions (happy path)", func() { gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) gatewayClient.On("CreateShare", mock.Anything, mock.Anything).Return(createShareResponse, nil) @@ -245,6 +288,7 @@ var _ = Describe("DriveItemPermissionsService", func() { createShareResponse = &collaboration.CreateShareResponse{ Status: status.NewOK(ctx), } + statResponse.Info = &provider.ResourceInfo{} }) It("adds a user to a space as expected (happy path)", func() { @@ -254,6 +298,10 @@ var _ = Describe("DriveItemPermissionsService", func() { SpaceId: "2", OpaqueId: "3", } + statResponse.Info.Id = listSpacesResponse.StorageSpaces[0].Root + statResponse.Info.Space = &provider.StorageSpace{ + Root: listSpacesResponse.StorageSpaces[0].Root, + } gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) @@ -307,12 +355,13 @@ var _ = Describe("DriveItemPermissionsService", func() { Status: status.NewOK(ctx), Shares: []*collaboration.Share{}, } - }) - It("populates allowedValues for files that are not shared", func() { statResponse.Info = &provider.ResourceInfo{ Id: &itemID, + Type: provider.ResourceType_RESOURCE_TYPE_FILE, PermissionSet: roleconversions.NewViewerRole().CS3ResourcePermissions(), } + }) + It("populates allowedValues for files that are not shared", func() { gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) gatewayClient.On("ListShares", mock.Anything, mock.Anything).Return(listSharesResponse, nil) gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) @@ -322,10 +371,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(len(permissions.LibreGraphPermissionsRolesAllowedValues)).ToNot(BeZero()) }) It("returns one permission per share", func() { - statResponse.Info = &provider.ResourceInfo{ - Id: &itemID, - PermissionSet: roleconversions.NewEditorRole().CS3ResourcePermissions(), - } + statResponse.Info.PermissionSet = roleconversions.NewEditorRole().CS3ResourcePermissions() listSharesResponse.Shares = []*collaboration.Share{ { Id: &collaboration.ShareId{OpaqueId: "1"}, @@ -394,6 +440,10 @@ var _ = Describe("DriveItemPermissionsService", func() { }, }, } + statResponse.Info = &provider.ResourceInfo{ + Type: provider.ResourceType_RESOURCE_TYPE_FILE, + PermissionSet: roleconversions.NewViewerRole().CS3ResourcePermissions(), + } }) It("adds a user to a space as expected (happy path)", func() { @@ -406,10 +456,7 @@ var _ = Describe("DriveItemPermissionsService", func() { gatewayClient.On("ListStorageSpaces", mock.Anything, mock.Anything).Return(listSpacesResponse, nil) gatewayClient.On("ListPublicShares", mock.Anything, mock.Anything).Return(listPublicSharesResponse, nil) - statResponse.Info = &provider.ResourceInfo{ - Id: listSpacesResponse.StorageSpaces[0].Root, - PermissionSet: roleconversions.NewViewerRole().CS3ResourcePermissions(), - } + statResponse.Info.Id = listSpacesResponse.StorageSpaces[0].Root gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) permissions, err := driveItemPermissionsService.ListSpaceRootPermissions(context.Background(), driveId) Expect(err).ToNot(HaveOccurred()) @@ -714,6 +761,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(res.Link.GetLibreGraphDisplayName() == TestLinkName) }) It("updates the expiration date", func() { + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) getPublicShareMockResponse.Share = nil getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") @@ -743,6 +791,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(res.GetExpirationDateTime().Equal(expiration)).To(BeTrue()) }) It("deletes the expiration date", func() { + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) gatewayClient.On("GetUser", mock.Anything, mock.Anything).Return(getUserResponse, nil) getPublicShareMockResponse.Share = nil getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") @@ -770,6 +819,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(ok).To(BeFalse()) }) It("fails to update the share permissions for a file share when setting a space specific role", func() { + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) getPublicShareMockResponse.Share = nil getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") gatewayClient.On("GetPublicShare", mock.Anything, mock.MatchedBy(func(req *link.GetPublicShareRequest) bool { @@ -788,6 +838,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(res).To(BeZero()) }) It("fails to update the space permissions for a space share when setting a file specific role", func() { + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) getPublicShareMockResponse.Share = nil getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) @@ -809,6 +860,7 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(res).To(BeZero()) }) It("updates the share permissions when changing the resource permission actions", func() { + gatewayClient.On("Stat", mock.Anything, mock.Anything).Return(statResponse, nil) getPublicShareMockResponse.Share = nil getPublicShareMockResponse.Status = status.NewNotFound(ctx, "not found") gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(getPublicShareMockResponse, nil) diff --git a/services/graph/pkg/service/v0/base.go b/services/graph/pkg/service/v0/base.go index e1bec9109..9ea53da3b 100644 --- a/services/graph/pkg/service/v0/base.go +++ b/services/graph/pkg/service/v0/base.go @@ -165,7 +165,7 @@ func (g BaseGraphService) cs3SpacePermissionsToLibreGraph(ctx context.Context, s p.SetExpirationDateTime(time.Unix(int64(exp.GetSeconds()), int64(exp.GetNanos()))) } - if role := unifiedrole.CS3ResourcePermissionsToUnifiedRole(*perm, unifiedrole.UnifiedRoleConditionOwner); role != nil { + if role := unifiedrole.CS3ResourcePermissionsToUnifiedRole(*perm, unifiedrole.UnifiedRoleConditionDrive); role != nil { switch apiVersion { case APIVersion_1: if r := unifiedrole.GetLegacyName(*role); r != "" { @@ -287,7 +287,17 @@ func (g BaseGraphService) cs3UserSharesToDriveItems(ctx context.Context, shares } item = *itemptr } - perm, err := g.cs3UserShareToPermission(ctx, s, false) + + var condition string + switch { + case item.Root != nil: + condition = unifiedrole.UnifiedRoleConditionDrive + case item.Folder != nil: + condition = unifiedrole.UnifiedRoleConditionFolder + case item.File != nil: + condition = unifiedrole.UnifiedRoleConditionFile + } + perm, err := g.cs3UserShareToPermission(ctx, s, condition) var errcode errorcode.Error switch { @@ -303,10 +313,10 @@ func (g BaseGraphService) cs3UserSharesToDriveItems(ctx context.Context, shares return driveItems, nil } -func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *collaboration.Share, isSpacePermission bool) (*libregraph.Permission, error) { +func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *collaboration.Share, roleCondition string) (*libregraph.Permission, error) { perm := libregraph.Permission{} perm.SetRoles([]string{}) - if !isSpacePermission { + if roleCondition != unifiedrole.UnifiedRoleConditionDrive { perm.SetId(share.GetId().GetOpaqueId()) } grantedTo := libregraph.SharePointIdentitySet{} @@ -322,7 +332,7 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c return nil, errorcode.New(errorcode.GeneralException, err.Error()) default: grantedTo.SetUser(user) - if isSpacePermission { + if roleCondition == unifiedrole.UnifiedRoleConditionDrive { perm.SetId("u:" + user.GetId()) } } @@ -337,7 +347,7 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c return nil, errorcode.New(errorcode.GeneralException, err.Error()) default: grantedTo.SetGroup(group) - if isSpacePermission { + if roleCondition == unifiedrole.UnifiedRoleConditionDrive { perm.SetId("g:" + group.GetId()) } } @@ -347,13 +357,9 @@ func (g BaseGraphService) cs3UserShareToPermission(ctx context.Context, share *c if share.GetExpiration() != nil { perm.SetExpirationDateTime(cs3TimestampToTime(share.GetExpiration())) } - condition := unifiedrole.UnifiedRoleConditionGrantee - if isSpacePermission { - condition = unifiedrole.UnifiedRoleConditionOwner - } role := unifiedrole.CS3ResourcePermissionsToUnifiedRole( *share.GetPermissions().GetPermissions(), - condition, + roleCondition, ) if role != nil { perm.SetRoles([]string{role.GetId()}) @@ -534,8 +540,13 @@ func (g BaseGraphService) getCS3UserShareByID(ctx context.Context, permissionID } func (g BaseGraphService) getPermissionByID(ctx context.Context, permissionID string, itemID *storageprovider.ResourceId) (*libregraph.Permission, *storageprovider.ResourceId, error) { - publicShare, err := g.getCS3PublicShareByID(ctx, permissionID) var errcode errorcode.Error + gatewayClient, err := g.gatewaySelector.Next() + if err != nil { + g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") + return nil, nil, err + } + publicShare, err := g.getCS3PublicShareByID(ctx, permissionID) switch { case err == nil: // the id is referencing a public share @@ -546,13 +557,7 @@ func (g BaseGraphService) getPermissionByID(ctx context.Context, permissionID st return permission, publicShare.GetResourceId(), nil case IsSpaceRoot(itemID): // itemID is referencing a spaceroot this is a space permission. Handle - // that next - gatewayClient, err := g.gatewaySelector.Next() - if err != nil { - g.logger.Debug().Err(err).Msg("selecting gatewaySelector failed") - return nil, nil, err - } - // get space id + // that here and get space id resourceInfo, err := utils.GetResourceByID(ctx, itemID, gatewayClient) if err != nil { return nil, nil, err @@ -573,7 +578,15 @@ func (g BaseGraphService) getPermissionByID(ctx context.Context, permissionID st if err != nil { return nil, nil, err } - permission, err := g.cs3UserShareToPermission(ctx, share, false) + resourceInfo, err := utils.GetResourceByID(ctx, itemID, gatewayClient) + if err != nil { + return nil, nil, err + } + condition, err := roleConditionForResourceType(resourceInfo) + if err != nil { + return nil, nil, err + } + permission, err := g.cs3UserShareToPermission(ctx, share, condition) if err != nil { return nil, nil, err } @@ -590,6 +603,14 @@ func (g BaseGraphService) updateUserShare(ctx context.Context, permissionID stri return nil, err } + resourceInfo, err := utils.GetResourceByID(ctx, itemID, gatewayClient) + if err != nil { + return nil, err + } + condition, err := roleConditionForResourceType(resourceInfo) + if err != nil { + return nil, err + } var cs3UpdateShareReq collaboration.UpdateShareRequest // When updating a space root we need to reference the share by resourceId and grantee if IsSpaceRoot(itemID) { @@ -632,11 +653,6 @@ func (g BaseGraphService) updateUserShare(ctx context.Context, permissionID stri return nil, err } - condition := unifiedrole.UnifiedRoleConditionGrantee - if IsSpaceRoot(itemID) { - condition = unifiedrole.UnifiedRoleConditionOwner - } - // FIXME: When setting permissions on a space, we need to use UnifiedRoleConditionOwner here allowedResourceActions = unifiedrole.GetAllowedResourceActions(role, condition) if len(allowedResourceActions) == 0 { return nil, errorcode.New(errorcode.InvalidRequest, "role not applicable to this resource") @@ -671,7 +687,7 @@ func (g BaseGraphService) updateUserShare(ctx context.Context, permissionID stri return nil, *errCode } - permission, err := g.cs3UserShareToPermission(ctx, updateUserShareResp.GetShare(), IsSpaceRoot(itemID)) + permission, err := g.cs3UserShareToPermission(ctx, updateUserShareResp.GetShare(), condition) if err != nil { return nil, err } diff --git a/services/graph/pkg/service/v0/sharedbyme_test.go b/services/graph/pkg/service/v0/sharedbyme_test.go index 6d4840686..e87ef7094 100644 --- a/services/graph/pkg/service/v0/sharedbyme_test.go +++ b/services/graph/pkg/service/v0/sharedbyme_test.go @@ -176,7 +176,8 @@ var _ = Describe("sharedbyme", func() { Return(&provider.StatResponse{ Status: status.NewOK(ctx), Info: &provider.ResourceInfo{ - Id: userShare.ResourceId, + Id: userShare.ResourceId, + Type: provider.ResourceType_RESOURCE_TYPE_CONTAINER, }, }, nil) diff --git a/services/graph/pkg/service/v0/utils.go b/services/graph/pkg/service/v0/utils.go index 55311185b..733cf9361 100644 --- a/services/graph/pkg/service/v0/utils.go +++ b/services/graph/pkg/service/v0/utils.go @@ -177,7 +177,7 @@ func cs3ReceivedSharesToDriveItems(ctx context.Context, return errCode } - driveItem, err := fillDriveItemPropertiesFromReceivedShare(ctx, logger, identityCache, receivedShares) + driveItem, err := fillDriveItemPropertiesFromReceivedShare(ctx, logger, identityCache, receivedShares, shareStat.GetInfo()) if err != nil { return err } @@ -313,7 +313,8 @@ func cs3ReceivedSharesToDriveItems(ctx context.Context, } func fillDriveItemPropertiesFromReceivedShare(ctx context.Context, logger *log.Logger, - identityCache identity.IdentityCache, receivedShares []*collaboration.ReceivedShare) (*libregraph.DriveItem, error) { + identityCache identity.IdentityCache, receivedShares []*collaboration.ReceivedShare, + resourceInfo *storageprovider.ResourceInfo) (*libregraph.DriveItem, error) { driveItem := libregraph.NewDriveItem() permissions := make([]libregraph.Permission, 0, len(receivedShares)) @@ -327,7 +328,7 @@ func fillDriveItemPropertiesFromReceivedShare(ctx context.Context, logger *log.L oldestReceivedShare = receivedShare } - permission, err := cs3ReceivedShareToLibreGraphPermissions(ctx, logger, identityCache, receivedShare) + permission, err := cs3ReceivedShareToLibreGraphPermissions(ctx, logger, identityCache, receivedShare, resourceInfo) if err != nil { return driveItem, err } @@ -387,7 +388,8 @@ func fillDriveItemPropertiesFromReceivedShare(ctx context.Context, logger *log.L } func cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, logger *log.Logger, - identityCache identity.IdentityCache, receivedShare *collaboration.ReceivedShare) (*libregraph.Permission, error) { + identityCache identity.IdentityCache, receivedShare *collaboration.ReceivedShare, + resourceInfo *storageprovider.ResourceInfo) (*libregraph.Permission, error) { permission := libregraph.NewPermission() if id := receivedShare.GetShare().GetId().GetOpaqueId(); id != "" { permission.SetId(id) @@ -398,10 +400,11 @@ func cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, logger *log.Lo } if permissionSet := receivedShare.GetShare().GetPermissions().GetPermissions(); permissionSet != nil { - role := unifiedrole.CS3ResourcePermissionsToUnifiedRole( - *permissionSet, - unifiedrole.UnifiedRoleConditionGrantee, - ) + condition, err := roleConditionForResourceType(resourceInfo) + if err != nil { + return nil, err + } + role := unifiedrole.CS3ResourcePermissionsToUnifiedRole(*permissionSet, condition) if role != nil { permission.SetRoles([]string{role.GetId()}) @@ -433,3 +436,16 @@ func cs3ReceivedShareToLibreGraphPermissions(ctx context.Context, logger *log.Lo return permission, nil } + +func roleConditionForResourceType(ri *storageprovider.ResourceInfo) (string, error) { + switch { + case utils.ResourceIDEqual(ri.GetSpace().GetRoot(), ri.GetId()): + return unifiedrole.UnifiedRoleConditionDrive, nil + case ri.Type == storageprovider.ResourceType_RESOURCE_TYPE_CONTAINER: + return unifiedrole.UnifiedRoleConditionFolder, nil + case ri.Type == storageprovider.ResourceType_RESOURCE_TYPE_FILE: + return unifiedrole.UnifiedRoleConditionFile, nil + default: + return "", errorcode.New(errorcode.InvalidRequest, "unsupported resource type") + } +} diff --git a/services/graph/pkg/unifiedrole/unifiedrole.go b/services/graph/pkg/unifiedrole/unifiedrole.go index b4c1a3056..c06d3d4bb 100644 --- a/services/graph/pkg/unifiedrole/unifiedrole.go +++ b/services/graph/pkg/unifiedrole/unifiedrole.go @@ -28,12 +28,12 @@ const ( // UnifiedRoleManagerID Unified role manager id. UnifiedRoleManagerID = "312c0871-5ef7-4b3a-85b6-0e4074c64049" - // UnifiedRoleConditionSelf defines constraint where the principal matches the target resource - UnifiedRoleConditionSelf = "@Subject.objectId == @Resource.objectId" - // UnifiedRoleConditionOwner defines constraints when the principal is the owner of the target resource - UnifiedRoleConditionOwner = "@Subject.objectId Any_of @Resource.owners" - // UnifiedRoleConditionGrantee does not exist in MS Graph, but we use it to express permissions on shared resources - UnifiedRoleConditionGrantee = "@Subject.objectId Any_of @Resource.grantee" + // UnifiedRoleConditionDrive defines constraint that matches a Driveroot/Spaceroot + UnifiedRoleConditionDrive = "exists @Resource.Root" + // UnifiedRoleConditionFolder defines constraints that matches a DriveItem representing a Folder + UnifiedRoleConditionFolder = "exists @Resource.Folder" + // UnifiedRoleConditionFile defines a constraint that matches a DriveItem representing a File + UnifiedRoleConditionFile = "exists @Resource.File" DriveItemPermissionsCreate = "libre.graph/driveItem/permissions/create" DriveItemChildrenCreate = "libre.graph/driveItem/children/create" @@ -78,7 +78,11 @@ func NewViewerUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionGrantee), + Condition: proto.String(UnifiedRoleConditionFile), + }, + { + AllowedResourceActions: convert(r), + Condition: proto.String(UnifiedRoleConditionFolder), }, }, LibreGraphWeight: proto.Int32(0), @@ -95,7 +99,7 @@ func NewSpaceViewerUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionOwner), + Condition: proto.String(UnifiedRoleConditionDrive), }, }, LibreGraphWeight: proto.Int32(0), @@ -112,7 +116,7 @@ func NewEditorUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionGrantee), + Condition: proto.String(UnifiedRoleConditionFolder), }, }, LibreGraphWeight: proto.Int32(0), @@ -129,7 +133,7 @@ func NewSpaceEditorUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionOwner), + Condition: proto.String(UnifiedRoleConditionDrive), }, }, LibreGraphWeight: proto.Int32(0), @@ -146,7 +150,7 @@ func NewFileEditorUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionGrantee), + Condition: proto.String(UnifiedRoleConditionFile), }, }, LibreGraphWeight: proto.Int32(0), @@ -163,7 +167,7 @@ func NewUploaderUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionGrantee), + Condition: proto.String(UnifiedRoleConditionFolder), }, }, LibreGraphWeight: proto.Int32(0), @@ -180,7 +184,7 @@ func NewManagerUnifiedRole() *libregraph.UnifiedRoleDefinition { RolePermissions: []libregraph.UnifiedRolePermission{ { AllowedResourceActions: convert(r), - Condition: proto.String(UnifiedRoleConditionOwner), + Condition: proto.String(UnifiedRoleConditionDrive), }, }, LibreGraphWeight: proto.Int32(0), @@ -219,19 +223,20 @@ func GetApplicableRoleDefinitionsForActions(actions []string, constraints string definitions := make([]*libregraph.UnifiedRoleDefinition, 0, len(builtin)) for _, definition := range builtin { - definitionMatch := true + var definitionMatch bool for _, permission := range definition.GetRolePermissions() { if permission.GetCondition() != constraints { - definitionMatch = false continue } - for _, action := range permission.GetAllowedResourceActions() { + for i, action := range permission.GetAllowedResourceActions() { if !slices.Contains(actions, action) { - definitionMatch = false break } + if i == len(permission.GetAllowedResourceActions())-1 { + definitionMatch = true + } } if definitionMatch { @@ -239,30 +244,33 @@ func GetApplicableRoleDefinitionsForActions(actions []string, constraints string } } - if !definitionMatch { - continue + if definitionMatch { + definitions = append(definitions, definition) } - definitions = append(definitions, definition) } - return WeightRoleDefinitions(definitions, descending) + return WeightRoleDefinitions(definitions, constraints, descending) } // WeightRoleDefinitions sorts the provided role definitions by the number of permissions[n].actions they grant, // the implementation is optimistic and assumes that the weight relies on the number of available actions. // descending - false - sorts the roles from least to most permissions // descending - true - sorts the roles from most to least permissions -func WeightRoleDefinitions(roleDefinitions []*libregraph.UnifiedRoleDefinition, descending bool) []*libregraph.UnifiedRoleDefinition { +func WeightRoleDefinitions(roleDefinitions []*libregraph.UnifiedRoleDefinition, constraints string, descending bool) []*libregraph.UnifiedRoleDefinition { slices.SortFunc(roleDefinitions, func(i, j *libregraph.UnifiedRoleDefinition) int { var ia []string for _, rp := range i.GetRolePermissions() { - ia = append(ia, rp.GetAllowedResourceActions()...) + if rp.GetCondition() == constraints { + ia = append(ia, rp.GetAllowedResourceActions()...) + } } var ja []string for _, rp := range j.GetRolePermissions() { - ja = append(ja, rp.GetAllowedResourceActions()...) + if rp.GetCondition() == constraints { + ja = append(ja, rp.GetAllowedResourceActions()...) + } } switch descending { diff --git a/services/graph/pkg/unifiedrole/unifiedrole_test.go b/services/graph/pkg/unifiedrole/unifiedrole_test.go index 3c87114ab..5b0e21ba9 100644 --- a/services/graph/pkg/unifiedrole/unifiedrole_test.go +++ b/services/graph/pkg/unifiedrole/unifiedrole_test.go @@ -24,12 +24,13 @@ var _ = Describe("unifiedroles", func() { Expect(r.GetId()).To(Equal(unifiedRole.GetId())) }, - Entry(rConversions.RoleViewer, rConversions.NewViewerRole(), unifiedrole.NewViewerUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee), - Entry(rConversions.RoleEditor, rConversions.NewEditorRole(), unifiedrole.NewEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee), - Entry(rConversions.RoleFileEditor, rConversions.NewFileEditorRole(), unifiedrole.NewFileEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee), - Entry(rConversions.RoleManager, rConversions.NewManagerRole(), unifiedrole.NewManagerUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner), - Entry(rConversions.RoleSpaceViewer, rConversions.NewSpaceViewerRole(), unifiedrole.NewSpaceViewerUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner), - Entry(rConversions.RoleSpaceEditor, rConversions.NewSpaceEditorRole(), unifiedrole.NewSpaceEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner), + Entry(rConversions.RoleViewer, rConversions.NewViewerRole(), unifiedrole.NewViewerUnifiedRole(), unifiedrole.UnifiedRoleConditionFile), + Entry(rConversions.RoleViewer, rConversions.NewViewerRole(), unifiedrole.NewViewerUnifiedRole(), unifiedrole.UnifiedRoleConditionFolder), + Entry(rConversions.RoleEditor, rConversions.NewEditorRole(), unifiedrole.NewEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionFolder), + Entry(rConversions.RoleFileEditor, rConversions.NewFileEditorRole(), unifiedrole.NewFileEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionFile), + Entry(rConversions.RoleManager, rConversions.NewManagerRole(), unifiedrole.NewManagerUnifiedRole(), unifiedrole.UnifiedRoleConditionDrive), + Entry(rConversions.RoleSpaceViewer, rConversions.NewSpaceViewerRole(), unifiedrole.NewSpaceViewerUnifiedRole(), unifiedrole.UnifiedRoleConditionDrive), + Entry(rConversions.RoleSpaceEditor, rConversions.NewSpaceEditorRole(), unifiedrole.NewSpaceEditorUnifiedRole(), unifiedrole.UnifiedRoleConditionDrive), ) DescribeTable("UnifiedRolePermissionsToCS3ResourcePermissions", @@ -57,9 +58,9 @@ var _ = Describe("unifiedroles", func() { ) DescribeTable("WeightRoleDefinitions", - func(roleDefinitions []*libregraph.UnifiedRoleDefinition, descending bool, expectedDefinitions []*libregraph.UnifiedRoleDefinition) { + func(roleDefinitions []*libregraph.UnifiedRoleDefinition, constraint string, descending bool, expectedDefinitions []*libregraph.UnifiedRoleDefinition) { - for i, generatedDefinition := range unifiedrole.WeightRoleDefinitions(roleDefinitions, descending) { + for i, generatedDefinition := range unifiedrole.WeightRoleDefinitions(roleDefinitions, constraint, descending) { Expect(generatedDefinition.Id).To(Equal(expectedDefinitions[i].Id)) } }, @@ -69,6 +70,7 @@ var _ = Describe("unifiedroles", func() { unifiedrole.NewViewerUnifiedRole(), unifiedrole.NewFileEditorUnifiedRole(), }, + unifiedrole.UnifiedRoleConditionFile, false, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewViewerUnifiedRole(), @@ -81,6 +83,7 @@ var _ = Describe("unifiedroles", func() { unifiedrole.NewViewerUnifiedRole(), unifiedrole.NewFileEditorUnifiedRole(), }, + unifiedrole.UnifiedRoleConditionFile, true, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewFileEditorUnifiedRole(), @@ -130,7 +133,7 @@ var _ = Describe("unifiedroles", func() { Entry( "ViewerUnifiedRole", rolesToAction(unifiedrole.NewViewerUnifiedRole()), - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFolder, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewViewerUnifiedRole(), }, @@ -139,7 +142,7 @@ var _ = Describe("unifiedroles", func() { Entry( "ViewerUnifiedRole | share", rolesToAction(unifiedrole.NewViewerUnifiedRole()), - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFile, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewViewerUnifiedRole(), }, @@ -148,17 +151,7 @@ var _ = Describe("unifiedroles", func() { Entry( "NewFileEditorUnifiedRole", rolesToAction(unifiedrole.NewFileEditorUnifiedRole()), - unifiedrole.UnifiedRoleConditionGrantee, - []*libregraph.UnifiedRoleDefinition{ - unifiedrole.NewViewerUnifiedRole(), - unifiedrole.NewFileEditorUnifiedRole(), - }, - ), - - Entry( - "NewFileEditorUnifiedRole - share", - rolesToAction(unifiedrole.NewFileEditorUnifiedRole()), - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFile, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewViewerUnifiedRole(), unifiedrole.NewFileEditorUnifiedRole(), @@ -168,23 +161,10 @@ var _ = Describe("unifiedroles", func() { Entry( "NewEditorUnifiedRole", rolesToAction(unifiedrole.NewEditorUnifiedRole()), - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFolder, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewUploaderUnifiedRole(), unifiedrole.NewViewerUnifiedRole(), - unifiedrole.NewFileEditorUnifiedRole(), - unifiedrole.NewEditorUnifiedRole(), - }, - ), - - Entry( - "NewEditorUnifiedRole - share", - rolesToAction(unifiedrole.NewEditorUnifiedRole()), - unifiedrole.UnifiedRoleConditionGrantee, - []*libregraph.UnifiedRoleDefinition{ - unifiedrole.NewUploaderUnifiedRole(), - unifiedrole.NewViewerUnifiedRole(), - unifiedrole.NewFileEditorUnifiedRole(), unifiedrole.NewEditorUnifiedRole(), }, ), @@ -192,11 +172,20 @@ var _ = Describe("unifiedroles", func() { Entry( "GetBuiltinRoleDefinitionList", rolesToAction(unifiedrole.GetBuiltinRoleDefinitionList()...), - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFile, + []*libregraph.UnifiedRoleDefinition{ + unifiedrole.NewViewerUnifiedRole(), + unifiedrole.NewFileEditorUnifiedRole(), + }, + ), + + Entry( + "GetBuiltinRoleDefinitionList", + rolesToAction(unifiedrole.GetBuiltinRoleDefinitionList()...), + unifiedrole.UnifiedRoleConditionFolder, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewUploaderUnifiedRole(), unifiedrole.NewViewerUnifiedRole(), - unifiedrole.NewFileEditorUnifiedRole(), unifiedrole.NewEditorUnifiedRole(), }, ), @@ -204,7 +193,7 @@ var _ = Describe("unifiedroles", func() { Entry( "GetBuiltinRoleDefinitionList", rolesToAction(unifiedrole.GetBuiltinRoleDefinitionList()...), - unifiedrole.UnifiedRoleConditionOwner, + unifiedrole.UnifiedRoleConditionDrive, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewSpaceViewerUnifiedRole(), unifiedrole.NewSpaceEditorUnifiedRole(), @@ -212,29 +201,17 @@ var _ = Describe("unifiedroles", func() { }, ), - Entry( - "GetBuiltinRoleDefinitionList - share", - rolesToAction(unifiedrole.GetBuiltinRoleDefinitionList()...), - unifiedrole.UnifiedRoleConditionGrantee, - []*libregraph.UnifiedRoleDefinition{ - unifiedrole.NewUploaderUnifiedRole(), - unifiedrole.NewViewerUnifiedRole(), - unifiedrole.NewFileEditorUnifiedRole(), - unifiedrole.NewEditorUnifiedRole(), - }, - ), - Entry( "single", []string{unifiedrole.DriveItemQuotaRead}, - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFile, []*libregraph.UnifiedRoleDefinition{}, ), Entry( "mixed", append(rolesToAction(unifiedrole.NewUploaderUnifiedRole()), unifiedrole.DriveItemQuotaRead), - unifiedrole.UnifiedRoleConditionGrantee, + unifiedrole.UnifiedRoleConditionFolder, []*libregraph.UnifiedRoleDefinition{ unifiedrole.NewUploaderUnifiedRole(), }, diff --git a/tests/acceptance/features/apiSharingNg/listPermissions.feature b/tests/acceptance/features/apiSharingNg/listPermissions.feature index a3877df6f..5efcbb718 100644 --- a/tests/acceptance/features/apiSharingNg/listPermissions.feature +++ b/tests/acceptance/features/apiSharingNg/listPermissions.feature @@ -48,8 +48,8 @@ Feature: List a sharing permissions }, "@libre.graph.permissions.roles.allowedValues": { "type": "array", - "minItems": 4, - "maxItems": 4, + "minItems": 3, + "maxItems": 3, "uniqueItems": true, "items": { "oneOf": [ @@ -138,41 +138,6 @@ Feature: List a sharing permissions 3 ] }, - "description": { - "type": "string", - "enum": [ - "View, download and edit." - ] - }, - "displayName": { - "type": "string", - "enum": [ - "Can edit" - ] - }, - "id": { - "type": "string", - "enum": [ - "2d00ce52-1fc2-4dbc-8b95-a73b73395f5a" - ] - } - } - }, - { - "type": "object", - "required": [ - "@libre.graph.weight", - "description", - "displayName", - "id" - ], - "properties": { - "@libre.graph.weight": { - "type": "integer", - "enum": [ - 4 - ] - }, "description": { "type": "string", "enum": [ @@ -594,4 +559,4 @@ Feature: List a sharing permissions } } } - """ \ No newline at end of file + """ diff --git a/tests/acceptance/features/apiSharingNg/shareInvitations.feature b/tests/acceptance/features/apiSharingNg/shareInvitations.feature index b6307f87b..187a62122 100644 --- a/tests/acceptance/features/apiSharingNg/shareInvitations.feature +++ b/tests/acceptance/features/apiSharingNg/shareInvitations.feature @@ -1570,7 +1570,7 @@ Feature: Send a sharing invitations "message": { "type": "string", "enum": [ - "cannot set the requested permissions on that type of resource" + "role not applicable to this resource" ] } } @@ -2356,9 +2356,9 @@ Feature: Send a sharing invitations """ Examples: | permissions-role | error-message | - | Space Viewer | space type is not eligible for sharing | - | Space Editor | space type is not eligible for sharing | - | Manager | space type is not eligible for sharing | + | Space Viewer | role not applicable to this resource | + | Space Editor | role not applicable to this resource | + | Manager | role not applicable to this resource | | Co Owner | Key: 'DriveItemInvite.Roles' Error:Field validation for 'Roles' failed on the 'available_role' tag |