Fix removing a group-to-group share link when LDAP sync is enabled
What does this MR do and why?
Closes #592428 (closed): once LDAP synchronisation is enabled on a group, an existing
group-to-group share invite (GroupGroupLink) could no longer be removed —
the UI silently reverted, and the API returned 204 while the link stayed
in place.
Two bugs stacked to produce this:
- Policy over-scoping.
Groups::GroupLinks::DestroyService#executeauthorized removal against:admin_group_member. That ability is unconditionallyprevented whenever a group has LDAP sync enabled (ee/app/policies/ee/group_policy.rb), since the lock is meant to keep individual member management authoritative-from-sync. Group-to-group sharing is an unrelated concept — it already has its own ability,:delete_group_link(the same oneCreateServicealready uses for the symmetric create action, and the one the controller's ownauthorize_admin_group!before-action already implicitly agrees with).admin_group_member's LDAP-sync lock was never meant to also gate this. - Both the API and the UI controller silently ignored the service's
failure result.
DELETE /groups/:id/share/:group_id(lib/api/groups.rb) calledno_content!unconditionally afterexecute, regardless of whether the service actually succeeded — the siblingProjects::GroupLinks::DestroyServiceendpoint a few lines up already checks its result correctly, this one didn't. Same story inGroups::GroupLinksController#destroy, which always redirected with a "Group invite removed" notice.
The fix
Groups::GroupLinks::DestroyService#executenow authorizes against:delete_group_linkinstead of:admin_group_member.- The API endpoint and the controller action now check the service result
and surface a proper error (
render_api_error!/ analertredirect) instead of unconditionally reporting success.
The service's return contract on the success path (a bare Array of the
destroyed links) is deliberately left unchanged — EE::Groups::GroupLinks::DestroyService#execute
(audit event, add-on seat refresh, UserGroupMemberRole cleanup) branches
on links.is_a?(Array), so wrapping the success return in a Hash would have
silently broken that EE post-destroy behavior. Failure is still
distinguishable via result.is_a?(Hash) && result[:status] == :error.
Screenshots or screen recordings
N/A — backend-only bug fix, no UI changes.
How to set up and validate locally
bin/rspec spec/services/groups/group_links/destroy_service_spec.rb \
ee/spec/services/ee/groups/group_links/destroy_service_spec.rb \
spec/controllers/groups/group_links_controller_spec.rb \
ee/spec/controllers/groups/group_links_controller_spec.rb \
spec/requests/api/groups_spec.rb \
ee/spec/requests/api/groups_spec.rbThe new/changed contexts to look at specifically: spec/requests/api/groups_spec.rb's
"when the user is not the owner of the group" (now correctly asserts a
404 and no change, instead of the previous 204-but-nothing-happened),
and the new "when the shared group has LDAP sync enabled" /
"when the group has LDAP sync enabled" contexts in
ee/spec/services/ee/groups/group_links/destroy_service_spec.rb,
ee/spec/controllers/groups/group_links_controller_spec.rb, and
ee/spec/requests/api/groups_spec.rb.
How to test manually
Before the fix (reproduce the bug)
- Create two groups: Group A and Group B.
- From Group A's Members page, go to the "Invite a group" tab and invite Group B.
- Enable LDAP group sync on Group A (Admin Area > enable LDAP, then add an LDAP group link to Group A).
- From Group A's Members page, attempt to remove the Group B share link.
- Observe the buggy behavior: a "Group invite removed" success notice appears, but refreshing the page shows the link is still there.
- API check: send
DELETE /groups/:id/share/:group_id(Group A's id and Group B's id). It returns204, but a follow-upGETstill lists the share link.
After the fix
- Repeat the same setup as "Before the fix."
- From Group A's Members page, remove the Group B share link.
- Verify the link is actually removed and the page reflects it.
- API check:
DELETE /groups/:id/share/:group_idreturns204and the link is genuinely gone. - Test as an unauthorized user (non-owner):
- UI shows a real error instead of a false success.
- API returns
404 Not Foundinstead of a false204.
Disclosure
This merge request was prepared with the assistance of Claude Code
(Anthropic) — root-cause tracing (including reading GroupGroupLinkPolicy,
ProjectGroupLinkPolicy, and the Authz::RolePermissions role-permission
catalog to confirm :delete_group_link was the correct, already-existing
ability to reuse rather than inventing a new one), implementation, and test
authoring. Verified via a full regression sweep (929 examples across the
touched service/controller/API spec files, 0 failures) and rubocop, and by
independently checking the reported root cause against the actual policy
and service code rather than taking the issue's Duo-generated root-cause
comment at face value.