diff --git a/changelog/unreleased/fix-graph-delete-permission-superfluous-call.md b/changelog/unreleased/fix-graph-delete-permission-superfluous-call.md new file mode 100644 index 00000000000..657bf151d39 --- /dev/null +++ b/changelog/unreleased/fix-graph-delete-permission-superfluous-call.md @@ -0,0 +1,9 @@ +Bugfix: Avoid superfluous GetPublicShare call when deleting space permissions + +We fixed `DeletePermission` to recognise space permission IDs (prefixed with +`u:` or `g:`) by their format before making any gateway calls. Previously, +deleting a space member always triggered a `GetPublicShare` lookup that was +guaranteed to fail, producing a confusing error log. + +https://github.com/owncloud/ocis/pull/12122 +https://github.com/owncloud/ocis/issues/12012 diff --git a/services/graph/pkg/service/v0/api_driveitem_permissions.go b/services/graph/pkg/service/v0/api_driveitem_permissions.go index a95067472ea..ba18cca8350 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions.go @@ -556,40 +556,13 @@ func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemI ctx, span := s.tp.Tracer("graph").Start(ctx, "DeletePermission") defer span.End() - var permissionType permissionType - - sharedResourceID, err := s.getLinkPermissionResourceID(ctx, permissionID) - switch { - // Check if the ID is referring to a public share - case err == nil: - permissionType = Public - // If the item id is referring to a space root and this is not a public share - // we have to deal with space permissions - case IsSpaceRoot(itemID): - permissionType = Space - sharedResourceID = itemID - err = nil - // If this is neither a public share nor a space permission, check if this is a - // user share - default: - sharedResourceID, err = s.getUserPermissionResourceID(ctx, permissionID) - if err == nil { - permissionType = User - } - } - - if sharedResourceID == nil && s.config.IncludeOCMSharees { - sharedResourceID, err = s.getOCMPermissionResourceID(ctx, permissionID) - if err == nil { - permissionType = OCM - } - } + permType, sharedResourceID, err := s.resolveDeletePermission(ctx, itemID, permissionID) switch { case err != nil: s.logger.Error().Err(err).Msg("permission error") return err - case permissionType == Unknown: + case permType == Unknown: s.logger.Error().Err(err).Msg("permission not found") return errorcode.New(errorcode.ItemNotFound, "permission not found") case sharedResourceID == nil: @@ -605,7 +578,7 @@ func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemI return err } - switch permissionType { + switch permType { case User: return s.removeUserShare(ctx, permissionID) case Public: @@ -622,6 +595,54 @@ func (s DriveItemPermissionsService) DeletePermission(ctx context.Context, itemI } } +// resolveDeletePermission determines the type and shared resource ID for a permission +// that is about to be deleted. +func (s DriveItemPermissionsService) resolveDeletePermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string) (permissionType, *storageprovider.ResourceId, error) { + if IsSpaceRoot(itemID) { + return s.resolveSpaceRootPermission(ctx, itemID, permissionID) + } + return s.resolveItemPermission(ctx, permissionID) +} + +// resolveSpaceRootPermission resolves permissions on a space root. Space roots +// carry space permissions (member/group grants with "u:"/"g:" prefixed IDs) and +// public links. Space permissions are recognised by their ID format to avoid a +// superfluous gateway call to GetPublicShare. +func (s DriveItemPermissionsService) resolveSpaceRootPermission(ctx context.Context, itemID *storageprovider.ResourceId, permissionID string) (permissionType, *storageprovider.ResourceId, error) { + if _, parseErr := spacePermissionIdToCS3Grantee(permissionID); parseErr == nil { + return Space, itemID, nil + } + + sharedResourceID, err := s.getLinkPermissionResourceID(ctx, permissionID) + if err == nil { + return Public, sharedResourceID, nil + } + return Unknown, nil, err +} + +// resolveItemPermission resolves permissions on a non-space-root item by trying +// public link, user share, then OCM share lookups in order. +func (s DriveItemPermissionsService) resolveItemPermission(ctx context.Context, permissionID string) (permissionType, *storageprovider.ResourceId, error) { + sharedResourceID, err := s.getLinkPermissionResourceID(ctx, permissionID) + if err == nil { + return Public, sharedResourceID, nil + } + + sharedResourceID, err = s.getUserPermissionResourceID(ctx, permissionID) + if err == nil { + return User, sharedResourceID, nil + } + + if s.config.IncludeOCMSharees { + sharedResourceID, err = s.getOCMPermissionResourceID(ctx, permissionID) + if err == nil { + return OCM, sharedResourceID, nil + } + } + + return Unknown, nil, err +} + // DeleteSpaceRootPermission deletes a permission on the root item of a project space func (s DriveItemPermissionsService) DeleteSpaceRootPermission(ctx context.Context, driveID *storageprovider.ResourceId, permissionID string) error { 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 e81138e06c6..425a8950190 100644 --- a/services/graph/pkg/service/v0/api_driveitem_permissions_test.go +++ b/services/graph/pkg/service/v0/api_driveitem_permissions_test.go @@ -822,9 +822,8 @@ var _ = Describe("DriveItemPermissionsService", func() { Expect(err).ToNot(HaveOccurred()) }) It("deletes a space permission as expected", func() { - getPublicShareResponse.Status = status.NewNotFound(context.Background(), "") - gatewayClient.On("GetPublicShare", mock.Anything, mock.Anything).Return(&getPublicShareResponse, nil) - + // No GetPublicShare mock needed — space permissions (u:/g: prefixed IDs) + // are recognised by their format and skip the public share lookup. gatewayClient.On("RemoveShare", mock.Anything, mock.Anything,