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 |