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:

  1. Policy over-scoping. Groups::GroupLinks::DestroyService#execute authorized removal against :admin_group_member. That ability is unconditionally prevented 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 one CreateService already uses for the symmetric create action, and the one the controller's own authorize_admin_group! before-action already implicitly agrees with). admin_group_member's LDAP-sync lock was never meant to also gate this.
  2. Both the API and the UI controller silently ignored the service's failure result. DELETE /groups/:id/share/:group_id (lib/api/groups.rb) called no_content! unconditionally after execute, regardless of whether the service actually succeeded — the sibling Projects::GroupLinks::DestroyService endpoint a few lines up already checks its result correctly, this one didn't. Same story in Groups::GroupLinksController#destroy, which always redirected with a "Group invite removed" notice.

The fix

  • Groups::GroupLinks::DestroyService#execute now authorizes against :delete_group_link instead of :admin_group_member.
  • The API endpoint and the controller action now check the service result and surface a proper error (render_api_error! / an alert redirect) 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.rb

The 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)

  1. Create two groups: Group A and Group B.
  2. From Group A's Members page, go to the "Invite a group" tab and invite Group B.
  3. Enable LDAP group sync on Group A (Admin Area > enable LDAP, then add an LDAP group link to Group A).
  4. From Group A's Members page, attempt to remove the Group B share link.
  5. Observe the buggy behavior: a "Group invite removed" success notice appears, but refreshing the page shows the link is still there.
  6. API check: send DELETE /groups/:id/share/:group_id (Group A's id and Group B's id). It returns 204, but a follow-up GET still lists the share link.

After the fix

  1. Repeat the same setup as "Before the fix."
  2. From Group A's Members page, remove the Group B share link.
  3. Verify the link is actually removed and the page reflects it.
  4. API check: DELETE /groups/:id/share/:group_id returns 204 and the link is genuinely gone.
  5. Test as an unauthorized user (non-owner):
    • UI shows a real error instead of a false success.
    • API returns 404 Not Found instead of a false 204.

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.

Edited by Sergey Pechenko

Merge request reports

Loading
Loading