Skip to content

fix(graph): avoid superfluous GetPublicShare call when deleting space permissions#12122

Open
paul43210 wants to merge 3 commits intoowncloud:masterfrom
paul43210:fix/graph-delete-permission-superfluous-call
Open

fix(graph): avoid superfluous GetPublicShare call when deleting space permissions#12122
paul43210 wants to merge 3 commits intoowncloud:masterfrom
paul43210:fix/graph-delete-permission-superfluous-call

Conversation

@paul43210
Copy link
Copy Markdown
Contributor

Summary

  • DeletePermission unconditionally called getLinkPermissionResourceID (→ GetPublicShare) as its first step, even when deleting space member permissions on space roots — producing a confusing "GetPublicShare failed" error log
  • Recognise space permission IDs (u: / g: prefixed) by their format via spacePermissionIdToCS3Grantee before making any gateway calls
  • Public link lookups on space roots now only happen when the ID does not match the space permission format

Fixes: #12012

Test plan

  • Existing deletes a space permission as expected test updated — no longer mocks GetPublicShare (verifying it's not called)
  • All 424 graph service tests pass
  • Manual: remove a space member → verify no "GetPublicShare failed" error in logs
  • Manual: delete a public link on a space root → verify it still works

🤖 Generated with Claude Code

@sonarqubecloud
Copy link
Copy Markdown

@mmattel mmattel requested review from jvillafanez and mklos-kw March 16, 2026 16:40
@jvillafanez
Copy link
Copy Markdown
Member

I'm out of the loop at the moment so I can't test and verify the solution, but code looks good.

@paul43210
Copy link
Copy Markdown
Contributor Author

@jvillafanez Thanks for the review! Here's a quick summary:

The issue: When deleting a space-level permission (member/group grant), DeletePermission was calling GetPublicShare first to check if the permission ID belonged to a public link. This always failed for space permissions, adding a needless round-trip and returning a confusing "not found" error instead of cleanly resolving the permission type.

The fix: Space permission IDs have a distinctive format — they're prefixed with u: (user) or g: (group). We now check that format via spacePermissionIdToCS3Grantee before making any gateway calls. If it matches, we know immediately it's a space permission and skip straight to RemoveShare. Public link and user share lookups only happen for non-space-root items or when the ID doesn't match the space permission format.

What changed:

  • Extracted resolveDeletePermissionresolveSpaceRootPermission / resolveItemPermission helpers (also brought SonarCloud complexity under the threshold)
  • Updated the existing test to confirm GetPublicShare is no longer called for space permissions
  • No behaviour change for public links, user shares, or OCM shares — those paths are untouched

Copy link
Copy Markdown
Member

@jvillafanez jvillafanez left a comment

Choose a reason for hiding this comment

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

👍

@paul43210 paul43210 force-pushed the fix/graph-delete-permission-superfluous-call branch from a172793 to 42ed77b Compare March 26, 2026 15:45
paul43210 and others added 3 commits March 26, 2026 17:07
… permissions

DeletePermission unconditionally called getLinkPermissionResourceID as its
first step, even for space permissions on space roots.  This always failed
and logged a confusing "GetPublicShare failed" error.

Recognise space permission IDs (u:/g: prefixed) by their format via
spacePermissionIdToCS3Grantee before making any gateway calls.  Public
link lookups on space roots now only happen when the ID does not match
the space permission format.

Fixes: owncloud#12012

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
…lexity

Split DeletePermission into resolveDeletePermission,
resolveSpaceRootPermission, and resolveItemPermission to bring
cognitive complexity under the SonarCloud limit of 15.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
@mmattel mmattel force-pushed the fix/graph-delete-permission-superfluous-call branch from 42ed77b to 5c59f2b Compare March 26, 2026 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Graph's delete permission needs a refactor

2 participants