fix(members): drop sponsor groups and Sponsor_Users access on full IDP sync by smarcet · Pull Request #627 · OpenStackweb/summit-api

smarcet · 2026-10-10T19:46:51Z

ref: https://app.clickup.com/t/9014802374/86bcgcfre

Summary

Stale sponsors groups were never removed in summit-api. MemberService::synchronizeGroups skipped IGroup::Sponsors on the full IDP sync ("managed by SS side"), so removing the group at the IDP never removed it locally, and the member's Sponsor_Users access stayed too. A leftover local sponsors group sends the member through SponsorMemberSummitStrategy, which hides the summits they get from admin access groups.

The IDP payload is the source of truth: on the full sync (allow_removals = true), when sponsors or sponsors-external-users is not in the payload, summit-api now removes the local group and strips that slug from the member's Sponsor_Users.Permissions. The summits then disappear from SponsorMemberSummitStrategy, which reads Permissions.

Changes

MemberService::synchronizeGroups: sponsors is no longer in the removal skip list. When a sponsor group is removed, Member::removeSponsorPermission() runs for each of the member's sponsor memberships, the same primitive the sponsor-users-api removal event uses (SponsorUserSyncService::removeSponsorUserFromGroup).
The Sponsor_Users rows are NOT deleted: their lifecycle belongs to the sponsor-users-api access right events (AUTH_USER_REMOVED_FROM_SPONSOR_AND_SUMMIT). A group missing at the IDP does not mean the access right is gone.
Not changed: the per-request token sync (ResourceServerContext::checkGroups) and SponsorUserSyncService::ensureSponsorGroupMembership stay additive-only (allow_removals = false).

Risk

A sponsor whose IDP profile does not carry the group yet (the MQ grant arrived first) loses the group and the permission slug on the next full sync, until the next sponsor-users-api event or additive sync re-adds it.

One-off cleanup of existing stale data

Report-only, run it and share the result before deleting anything:

SELECT m.ID, m.Email, g.Code, gm.ID AS GroupMembersID
FROM Group_Members gm
INNER JOIN `Group` g ON g.ID = gm.GroupID
INNER JOIN Member m ON m.ID = gm.MemberID
WHERE g.Code IN ('sponsors', 'sponsors-external-users')
  AND NOT EXISTS (
    SELECT 1 FROM Sponsor_Users su
    WHERE su.MemberID = gm.MemberID
      AND JSON_CONTAINS(COALESCE(su.Permissions, '[]'), JSON_QUOTE(g.Code))
  );

Tests

New tests/Unit/Services/MemberServiceSponsorGroupSyncTest.php (5 tests): full sync without the group removes the group and strips the slug (row stays); with the group keeps both; additive sync keeps both; a row with another slug keeps it; same for sponsors-external-users.

docker exec summit-api bash -lc "cd /var/www && vendor/bin/phpunit tests/Unit/Services/MemberServiceSponsorGroupSyncTest.php"

tests/Unit/Services/SponsorUserPermissionTrackingTest.php already fails 2 tests (testRemoveSponsorUserFromGroupStillCleansUpWhenSponsorWasDeleted, testRemoveSponsorUserFromGroupRemovesGlobalGroupWhenLastSponsor) on main without this change; they cover removeSponsorUserFromGroup, which this PR does not touch.

Summary by CodeRabbit

Bug Fixes
- Full group synchronization now removes sponsor and external-user groups that are absent from the identity provider’s group list, and removes only the corresponding permission from sponsor memberships while preserving membership records and other permissions.
- Additive synchronization continues to retain existing groups and permissions.

…P sync MemberService::synchronizeGroups skipped IGroup::Sponsors on the full IDP sync, so removing the group at the IDP never removed it locally and the member's Sponsor_Users access stayed too. The IDP payload is the source of truth: when sponsors or sponsors-external-users is missing from it, the local group is removed and that slug is stripped from the member's Sponsor_Users.Permissions on every sponsor. Rows left without any permission lose the membership; rows that still hold another slug are kept. The additive per-request sync is unchanged.

coderabbitai · 2026-10-10T19:47:16Z

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults
Review profile: CHILL
Plan: Advanced
Run ID: 0e9287cb-90a8-43c2-8129-a0e81834010d

📥 Commits

Reviewing files that changed from the base of the PR and between 514b802 and 0e9f1e3.

📒 Files selected for processing (2)

app/Services/Model/Imp/MemberService.php
tests/Unit/Services/MemberServiceSponsorGroupSyncTest.php

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

📝 Walkthrough

Walkthrough

Full group synchronization now removes absent Sponsors and SponsorExternalUsers groups and their corresponding permission slugs from sponsor membership rows. The membership rows remain. New tests cover full and additive synchronization.

Changes

Sponsor group synchronization

Layer / File(s)	Summary
Synchronization integration and coverage `app/Services/Model/Imp/MemberService.php`, `tests/Unit/Services/MemberServiceSponsorGroupSyncTest.php`	Full synchronization can remove an absent `Sponsors` group. Before removing either sponsor group, the service removes its permission slug from sponsor membership rows and retains the rows. Tests cover retained groups and slugs, additive sync behavior, and removal of an omitted external-users group.

Priority: ⬇️ Low

Estimated code review effort: 2 (Simple) | ~12 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 0e9f1

Absent sponsor groups are removed during the designated authoritative IDP update; other sync paths remain additive. No actionable merge-blocking issue is established.

Pre-merge checks | 4 | 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage		Docstring coverage is 28.57% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 14 functions across 3 files.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check		Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check		Check skipped because no linked issues were found for this pull request.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.
Title check		The title clearly and concisely describes the main change: full IDP synchronization removes omitted sponsor groups and their Sponsor_Users access.

Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

Commit to this branch
Create a new PR

🧪 Generate unit tests (beta)

Commit to this branch
Create a new PR

Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

github-actions · 2026-10-10T19:48:36Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-627/

This page is automatically updated on each push to this PR.

Copilot

🟡 Changes recommended

The cleanup has a concurrent-grant consistency race and can hydrate entire sponsor membership collections.

2 open findings

What changed in this PR

Updates authoritative IDP synchronization to remove stale sponsor groups and permissions.

Changes:

Removes missing sponsor groups during full synchronization.
Cleans corresponding Sponsor_Users.Permissions entries and empty memberships.
Adds tests for full and additive synchronization behavior.

File	Description
`app/Services/Model/Imp/MemberService.php`	Triggers sponsor-permission cleanup during full sync.
`app/Models/Foundation/Main/Member.php`	Removes permissions across sponsor memberships.
`tests/Unit/Services/MemberServiceSponsorGroupSyncTest.php`	Tests sponsor-group synchronization scenarios.

🧠 Review effort: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+SELECT SponsorID FROM Sponsor_Users
+WHERE MemberID = :member_id
+AND JSON_CONTAINS(COALESCE(Permissions, '[]'), JSON_QUOTE(:group_slug))
+FOR UPDATE


+        foreach ($this->sponsor_memberships->toArray() as $sponsor) {
+            if (in_array(intval($sponsor->getId()), $to_remove, true)) {
+                $sponsor->removeUser($this);


…nsor_Users rows Reuse Member::removeSponsorPermission per sponsor membership, the same primitive the sponsor-users-api removal event uses, instead of a new raw SQL method that also deleted the memberships left without permissions. Group absence does not mean the access right is gone: the Sponsor_Users row lifecycle belongs to the access right events (AUTH_USER_REMOVED_FROM_SPONSOR_AND_SUMMIT). Stripping the slug is enough to hide the summits, since the sponsor summit strategy reads Permissions.

…ponsors

github-actions · 2026-10-10T22:16:59Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-627/

This page is automatically updated on each push to this PR.

github-actions · 2026-10-10T22:19:22Z

📘 OpenAPI / Swagger preview

➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-627/

This page is automatically updated on each push to this PR.

romanetar

LGTM

smarcet self-assigned this Oct 10, 2026

smarcet requested review from romanetar and a balanced review from Copilot and removed request for romanetar October 10, 2026 21:53

Copilot started reviewing on behalf of smarcet October 10, 2026 21:54 View session

Copilot AI reviewed Oct 10, 2026

View reviewed changes

smarcet added 2 commits October 10, 2026 19:14

chore(members): remove unused Member::removeSponsorPermissionFromAllS…

0e9f1e3

…ponsors

romanetar approved these changes Oct 11, 2026

View reviewed changes

Navigation Menu

Repository navigation

fix(members): drop sponsor groups and Sponsor_Users access on full IDP sync - #627

fix(members): drop sponsor groups and Sponsor_Users access on full IDP sync#627
smarcet wants to merge 3 commits into
mainfrom
fix/sponsor-groups-idp-sync-cleanup

smarcet commented Oct 10, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Oct 10, 2026 •

edited

Loading

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

romanetar left a comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

smarcet commented Oct 10, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Changes

Risk

One-off cleanup of existing stale data

Tests

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Oct 10, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

❌ Failed checks (1 warning)

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

github-actions Bot commented Oct 10, 2026

Uh oh!

romanetar left a comment

Choose a reason for hiding this comment

Uh oh!

This branch has not been deployed

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

smarcet commented Oct 10, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Oct 10, 2026 •

edited

Loading