graph/sharing: Add check for role conditions

changelog/unreleased/sharing-ng-roleconditions.md

@@ -0,0 +1,8 @@
+Bugfix: apply role constraints when creating shares via the graph API
+
+We fixed a bug in the graph API for creating and updating shares so that
+Spaceroot specific roles like 'Manager' and 'Co-owner' can no longer be
+assigned for shares on files or directories.
+
+https://github.com/owncloud/ocis/pull/8247
+https://github.com/owncloud/ocis/issues/8131

services/graph/pkg/service/v0/driveitems.go

@@ -378,6 +378,11 @@ func (g Graph) ListPermissions(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	condition := unifiedrole.UnifiedRoleConditionGrantee
+	if IsSpaceRoot(statResponse.GetInfo().GetId()) {
+		condition = unifiedrole.UnifiedRoleConditionOwner
+	}
+
 	permissionSet := *statResponse.GetInfo().GetPermissionSet()
 	allowedActions := unifiedrole.CS3ResourcePermissionsToLibregraphActions(permissionSet)
 
@@ -386,7 +391,7 @@ func (g Graph) ListPermissions(w http.ResponseWriter, r *http.Request) {
 		LibreGraphPermissionsRolesAllowedValues: conversions.ToValueSlice(
 			unifiedrole.GetApplicableRoleDefinitionsForActions(
 				allowedActions,
-				unifiedrole.UnifiedRoleConditionGrantee,
+				condition,
 				g.config.FilesSharing.EnableResharing,
 				false,
 			),
@@ -452,6 +457,18 @@ func (g Graph) Invite(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 
+	statResponse, err := gatewayClient.Stat(ctx, &storageprovider.StatRequest{Ref: &storageprovider.Reference{ResourceId: &itemID}})
+	if errCode := errorcode.FromStat(statResponse, err); errCode != nil {
+		g.logger.Warn().Err(errCode).Interface("stat.res", statResponse).Msg("stat failed")
+		errCode.Render(w, r)
+		return
+	}
+
+	condition := unifiedrole.UnifiedRoleConditionGrantee
+	if IsSpaceRoot(statResponse.GetInfo().GetId()) {
+		condition = unifiedrole.UnifiedRoleConditionOwner
+	}
+
 	unifiedRolePermissions := []*libregraph.UnifiedRolePermission{{AllowedResourceActions: driveItemInvite.LibreGraphPermissionsActions}}
 	for _, roleID := range driveItemInvite.GetRoles() {
 		role, err := unifiedrole.NewUnifiedRoleFromID(roleID, g.config.FilesSharing.EnableResharing)
@@ -461,14 +478,13 @@ func (g Graph) Invite(w http.ResponseWriter, r *http.Request) {
 			return
 		}
 
-		unifiedRolePermissions = append(unifiedRolePermissions, conversions.ToPointerSlice(role.GetRolePermissions())...)
-	}
+		allowedResourceActions := unifiedrole.GetAllowedResourceActions(role, condition)
+		if len(allowedResourceActions) == 0 {
+			errorcode.InvalidRequest.Render(w, r, http.StatusBadRequest, "role not applicable to this resource")
+			return
+		}
 
-	statResponse, err := gatewayClient.Stat(ctx, &storageprovider.StatRequest{Ref: &storageprovider.Reference{ResourceId: &itemID}})
-	if errCode := errorcode.FromStat(statResponse, err); errCode != nil {
-		g.logger.Warn().Err(errCode).Interface("stat.res", statResponse).Msg("stat failed")
-		errCode.Render(w, r)
-		return
+		unifiedRolePermissions = append(unifiedRolePermissions, conversions.ToPointerSlice(role.GetRolePermissions())...)
 	}
 
 	driveRecipient := driveItemInvite.GetRecipients()[0]
@@ -486,7 +502,7 @@ func (g Graph) Invite(w http.ResponseWriter, r *http.Request) {
 	}
 
 	permission := &libregraph.Permission{}
-	if role := unifiedrole.CS3ResourcePermissionsToUnifiedRole(*cs3ResourcePermissions, unifiedrole.UnifiedRoleConditionGrantee, g.config.FilesSharing.EnableResharing); role != nil {
+	if role := unifiedrole.CS3ResourcePermissionsToUnifiedRole(*cs3ResourcePermissions, condition, g.config.FilesSharing.EnableResharing); role != nil {
 		permission.Roles = []string{role.GetId()}
 	}
 

services/graph/pkg/service/v0/driveitems_test.go

@@ -685,6 +685,25 @@ var _ = Describe("Driveitems", func() {
 			_, ok := res.GetRolesOk()
 			Expect(ok).To(BeTrue())
 		})
+		It("fails to update the share permissions for a file share when setting a space specific role", func() {
+			updateShareMock := gatewayClient.On("UpdateShare",
+				mock.Anything,
+				mock.MatchedBy(func(req *collaboration.UpdateShareRequest) bool {
+					return req.GetShare().GetId().GetOpaqueId() == "permissionid"
+				}),
+			)
+			updateShareMock.Return(updateShareMockResponse, nil)
+
+			driveItemPermission.SetRoles([]string{unifiedrole.NewSpaceViewerUnifiedRole().GetId()})
+			body, err := driveItemPermission.MarshalJSON()
+			Expect(err).To(BeNil())
+			svc.UpdatePermission(
+				rr,
+				httptest.NewRequest(http.MethodPatch, "/", strings.NewReader(string(body))).
+					WithContext(ctx),
+			)
+			Expect(rr.Code).To(Equal(http.StatusBadRequest))
+		})
 		It("updates the share permissions when changing the resource permission actions", func() {
 			updateShareMock := gatewayClient.On("UpdateShare",
 				mock.Anything,
@@ -1007,6 +1026,17 @@ var _ = Describe("Driveitems", func() {
 			Expect(jsonData.Get("0.roles.0").String()).To(Equal(unifiedrole.NewViewerUnifiedRole(true).GetId()))
 		})
 
+		It("fails with wrong role", func() {
+			driveItemInvite.Roles = []string{unifiedrole.NewCoownerUnifiedRole().GetId()}
+			svc.Invite(
+				rr,
+				httptest.NewRequest(http.MethodPost, "/", toJSONReader(driveItemInvite)).
+					WithContext(ctx),
+			)
+
+			Expect(rr.Code).To(Equal(http.StatusBadRequest))
+		})
+
 		It("with actions (happy path)", func() {
 			driveItemInvite.Roles = nil
 			driveItemInvite.LibreGraphPermissionsActions = []string{unifiedrole.DriveItemContentRead}

services/graph/pkg/unifiedrole/unifiedrole.go

@@ -153,7 +153,7 @@ func NewCoownerUnifiedRole() *libregraph.UnifiedRoleDefinition {
 		RolePermissions: []libregraph.UnifiedRolePermission{
 			{
 				AllowedResourceActions: convert(r),
-				Condition:              proto.String(UnifiedRoleConditionGrantee),
+				Condition:              proto.String(UnifiedRoleConditionOwner),
 			},
 		},
 		LibreGraphWeight: proto.Int32(0),
@@ -185,10 +185,6 @@ func NewManagerUnifiedRole() *libregraph.UnifiedRoleDefinition {
 		Description: proto.String("Grants manager permissions on a resource. Semantically equivalent to co-owner"),
 		DisplayName: displayName(r),
 		RolePermissions: []libregraph.UnifiedRolePermission{
-			{
-				AllowedResourceActions: convert(r),
-				Condition:              proto.String(UnifiedRoleConditionGrantee),
-			},
 			{
 				AllowedResourceActions: convert(r),
 				Condition:              proto.String(UnifiedRoleConditionOwner),

services/graph/pkg/unifiedrole/unifiedrole_test.go

@@ -27,8 +27,7 @@ var _ = Describe("unifiedroles", func() {
 		Entry(rConversions.RoleViewer, rConversions.NewViewerRole(true), unifiedrole.NewViewerUnifiedRole(true), unifiedrole.UnifiedRoleConditionGrantee),
 		Entry(rConversions.RoleEditor, rConversions.NewEditorRole(true), unifiedrole.NewEditorUnifiedRole(true), unifiedrole.UnifiedRoleConditionGrantee),
 		Entry(rConversions.RoleFileEditor, rConversions.NewFileEditorRole(true), unifiedrole.NewFileEditorUnifiedRole(true), unifiedrole.UnifiedRoleConditionGrantee),
-		Entry(rConversions.RoleCoowner, rConversions.NewCoownerRole(), unifiedrole.NewCoownerUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee),
-		Entry(rConversions.RoleManager, rConversions.NewManagerRole(), unifiedrole.NewManagerUnifiedRole(), unifiedrole.UnifiedRoleConditionGrantee),
+		Entry(rConversions.RoleCoowner, rConversions.NewCoownerRole(), unifiedrole.NewCoownerUnifiedRole(), unifiedrole.UnifiedRoleConditionOwner),
 		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),
@@ -208,6 +207,17 @@ var _ = Describe("unifiedroles", func() {
 					unifiedrole.NewViewerUnifiedRole(false),
 					unifiedrole.NewFileEditorUnifiedRole(false),
 					unifiedrole.NewEditorUnifiedRole(false),
+				},
+			),
+
+			Entry(
+				"GetBuiltinRoleDefinitionList",
+				rolesToAction(unifiedrole.GetBuiltinRoleDefinitionList(false)...),
+				unifiedrole.UnifiedRoleConditionOwner,
+				false,
+				[]*libregraph.UnifiedRoleDefinition{
+					unifiedrole.NewSpaceViewerUnifiedRole(),
+					unifiedrole.NewSpaceEditorUnifiedRole(),
 					unifiedrole.NewCoownerUnifiedRole(),
 					unifiedrole.NewManagerUnifiedRole(),
 				},
@@ -223,8 +233,6 @@ var _ = Describe("unifiedroles", func() {
 					unifiedrole.NewViewerUnifiedRole(true),
 					unifiedrole.NewFileEditorUnifiedRole(true),
 					unifiedrole.NewEditorUnifiedRole(true),
-					unifiedrole.NewCoownerUnifiedRole(),
-					unifiedrole.NewManagerUnifiedRole(),
 				},
 			),
 

tests/acceptance/expected-failures-localAPI-on-OCIS-storage.md

@@ -270,10 +270,10 @@ The expected failures in this file are from features in the owncloud/ocis repo.
 - [apiSharingNg/linkShare.feature:453](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/linkShare.feature#L453)
 - [apiSharingNg/linkShare.feature:455](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/linkShare.feature#L455)
 - [apiSharingNg/linkShare.feature:456](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/linkShare.feature#L456)
-- [apiSharingNg/deletePermissions.feature:138](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L138)
-- [apiSharingNg/deletePermissions.feature:155](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L155)
-- [apiSharingNg/deletePermissions.feature:176](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L176)
-- [apiSharingNg/deletePermissions.feature:195](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L195)
+- [apiSharingNg/deletePermissions.feature:130](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L130)
+- [apiSharingNg/deletePermissions.feature:147](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L147)
+- [apiSharingNg/deletePermissions.feature:168](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L168)
+- [apiSharingNg/deletePermissions.feature:187](https://github.com/owncloud/ocis/blob/master/tests/acceptance/features/apiSharingNg/deletePermissions.feature#L187)
 
 ### [sharee (editor role) MOVE a file by file-id into same shared folder returns 403](https://github.com/owncloud/ocis/issues/7617)
 

tests/acceptance/features/apiSharingNg/deletePermissions.feature

@@ -29,9 +29,7 @@ Feature: Remove access to a drive item
       | File Editor     | file          | textfile.txt  |
       | Viewer          | folder        | FolderToShare |
       | Editor          | folder        | FolderToShare |
-      | Co Owner        | folder        | FolderToShare |
       | Uploader        | folder        | FolderToShare |
-      | Manager         | folder        | FolderToShare |
 
 
   Scenario Outline: user removes access to resource inside of a project space in the user share
@@ -56,9 +54,7 @@ Feature: Remove access to a drive item
       | File Editor     | file          | textfile.txt  |
       | Viewer          | folder        | FolderToShare |
       | Editor          | folder        | FolderToShare |
-      | Co Owner        | folder        | FolderToShare |
       | Uploader        | folder        | FolderToShare |
-      | Manager         | folder        | FolderToShare |
 
 
   Scenario Outline: user removes access to a resource in a group share
@@ -84,9 +80,7 @@ Feature: Remove access to a drive item
       | File Editor     | file          | textfile.txt  |
       | Viewer          | folder        | FolderToShare |
       | Editor          | folder        | FolderToShare |
-      | Co Owner        | folder        | FolderToShare |
       | Uploader        | folder        | FolderToShare |
-      | Manager         | folder        | FolderToShare |
 
 
   Scenario Outline: user removes access to a resource inside of a project space in group share
@@ -114,9 +108,7 @@ Feature: Remove access to a drive item
       | File Editor     | file          | textfile.txt  |
       | Viewer          | folder        | FolderToShare |
       | Editor          | folder        | FolderToShare |
-      | Co Owner        | folder        | FolderToShare |
       | Uploader        | folder        | FolderToShare |
-      | Manager         | folder        | FolderToShare |
 
 
   Scenario Outline: user removes access to a folder in link share

tests/acceptance/features/apiSharingNg/shareInvitations.feature

@@ -93,9 +93,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: send share invitation to group with different roles
@@ -187,9 +185,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: send share invitation for a file to user with different permissions
@@ -665,9 +661,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: send share invitation with expiration date to group with different roles
@@ -763,9 +757,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
   @issue-7962
   Scenario Outline: send share invitation to disabled user
@@ -848,9 +840,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: send sharing invitation to a deleted group with different roles
@@ -909,9 +899,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: send share invitation to deleted user
@@ -962,9 +950,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: try to send sharing invitation to multiple groups
@@ -1090,9 +1076,7 @@ Feature: Send a sharing invitations
       | File Editor      | file          | /textfile1.txt |
       | Viewer           | folder        | FolderToShare  |
       | Editor           | folder        | FolderToShare  |
-      | Co Owner         | folder        | FolderToShare  |
       | Uploader         | folder        | FolderToShare  |
-      | Manager          | folder        | FolderToShare  |
 
 
   Scenario Outline: send sharing invitation to non-existing group