Hotfix/sponsors permissions - #582
Open
smarcet wants to merge 20 commits into
Open
Conversation
…sync - addSponsorUserToGroup: add member to the global group BEFORE writing permissions/eager-creating the Sponsor_Users row, so Sponsor::addUser's group validation passes for brand-new sponsor users (the group is delivered by this very event). - addSponsorUser: stop swallowing exceptions so the MQ job retry / failed_jobs machinery applies instead of losing membership events. - Tests: red-green covered in SponsorUserPermissionTrackingTest.
A swallowed removal failure silently leaves the user with access they should have lost. Remove the catch-and-log so RemoveSponsorMemberMQJob (tries = 3) retries and records the failure in failed_jobs. Red-green covered by testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist.
When a sponsor-users-api event arrives for a brand-new IDP user whose Member row was never synced (the user has not logged in yet), the sync exhausted its MQ retries against a missing member and the access grant was lost for good. SponsorUserSyncService now resolves members via resolveMember(): local lookup with a fallback to IMemberService::registerExternalUserById, which fetches the user from the IDP and creates the Member row. EntityNotFoundException is now only thrown when the user does not exist at the IDP either. Propagation tests updated accordingly: they now mock the IDP user API returning null (user unknown at the IDP).
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughSponsor synchronization now provisions missing members through the IDP, propagates failures, scopes revocation to summits, and orders group permission updates. Sponsor-service jobs preserve event types across delayed retries and redeliveries. ChangesSponsor synchronization
Sponsor-service message retries
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SponsorServicesMQJob
participant RabbitMQ
participant DelayQueue
participant SponsorServiceHandler
SponsorServicesMQJob->>RabbitMQ: Publish delayed payload with event type
RabbitMQ->>DelayQueue: Route message for delayed retry
DelayQueue->>RabbitMQ: Redeliver message
RabbitMQ->>SponsorServiceHandler: Resolve event type and handle message
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Hotfix to improve sponsor-permission synchronization reliability by (a) ensuring sponsor-group membership is granted before Sponsor_Users row creation/validation paths run, (b) registering missing Members on-demand from the IDP, and (c) allowing failures to propagate so MQ retry/failed_jobs handling applies.
Changes:
- Add
resolveMember()and injectIMemberServiceso sponsor sync can register missing users from the IDP. - Stop swallowing exceptions in
addSponsorUser/removeSponsorUserto enable MQ retry semantics. - Expand integration tests to cover “no sponsor group yet”, “member missing locally”, and “propagate missing-member failures”.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| app/Services/Model/Imp/SponsorUserSyncService.php | Adds on-demand member resolution/registration and changes error-propagation + group-grant ordering for sponsor permission sync. |
| tests/Unit/Services/SponsorUserPermissionTrackingTest.php | Adds coverage for new edge cases: chicken-and-egg group validation, on-demand member registration, and exception propagation. |
Suppressed comments (1)
app/Services/Model/Imp/SponsorUserSyncService.php:234
- Same concern as addSponsorUserToGroup: resolveMember() may register the member (nested transaction) and dispatch jobs while the outer transaction is still open. Resolve/register before opening the transaction, then load the Member inside the transaction to apply permission/group removals.
$this->tx_service->transaction(function () use ($user_id, $group_slug, $sponsor_id, $summit_id) {
Log::debug(
"SponsorUserSyncService::removeSponsorUserFromGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}");
$member = $this->resolveMember($user_id);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/Model/Imp/SponsorUserSyncService.php (1)
182-203: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winUpdate the group-membership cache before eager sponsor-user creation.
Line 182 caches
falsefor$group_slug. Line 187 adds the group only to the Doctrine collection.Member::add2Group()does not invalidate or updategroupMembershipCache.When line 203 calls
Sponsor::addUser, its group validation can read the cachedfalsevalue. The new-user flow then fails before it creates theSponsor_Usersrow.Update
Member::add2Group()to invalidate or set the cache entry for the added group. This lets the subsequent membership validation observe the new group.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/Model/Imp/SponsorUserSyncService.php` around lines 182 - 203, Update Member::add2Group() to invalidate or set the groupMembershipCache entry for the newly added group slug after adding it to the Doctrine collection. Ensure subsequent group-membership validation during addSponsorUser observes the new membership, including the eager creation path in SponsorUserSyncService.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 274-294: Wrap the dynamic-member registration, assertions, and
cleanup in a finally block. In the finally block, retrieve the member using
$external_id and remove and flush it when present, ensuring cleanup runs even if
a later assertion fails.
---
Outside diff comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 182-203: Update Member::add2Group() to invalidate or set the
groupMembershipCache entry for the newly added group slug after adding it to the
Doctrine collection. Ensure subsequent group-membership validation during
addSponsorUser observes the new membership, including the eager creation path in
SponsorUserSyncService.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 031f5f18-9b09-4a40-92ac-f3f773ada122
📒 Files selected for processing (2)
app/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
romanetar
reviewed
Aug 10, 2026
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
registerExternalUserById opens its own transaction and dispatches NewMember / MemberDataUpdatedExternally right after it, whose listeners enqueue MemberAssocSummitOrders, UpdateAttendeeInfo and CleanMemberCacheJob. Those pushes are not deferred to commit: JobDispatcher's afterCommit flag only works for transactions Laravel's DatabaseTransactionsManager can see, and DoctrineTransactionService opens directly on the DBAL connection. Registering the member inside addSponsorUserToGroup / removeSponsorUserFromGroup's transaction therefore left the jobs pointing at a member id that a later rollback erased, failing them permanently. Resolve the member before opening the transaction and re-load it by id inside, so an on-demand registration is always committed before the jobs that reference it.
smarcet
force-pushed
the
hotfix/sponsors-permissions
branch
from
August 10, 2026 17:37
58e7583 to
a2e9227
Compare
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/Services/Model/Imp/SponsorUserSyncService.php (1)
151-159: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winSnapshot
$member->getSponsorMemberships()before mutating sponsor user permissions.
$member->getSponsorMemberships()returns a Doctrine collection, andSummitSponsorService::removeSponsorUser()callssummit_sponsor->removeUser($member), which can mutate the same collection during the loop. A removal can skip later memberships, leaving sponsor-level access intact. Copy the memberships before iterating, e.g.[$sponsor_memberships] = $member->getSponsorMemberships()->toArray();, then iterate$sponsor_memberships.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/Model/Imp/SponsorUserSyncService.php` around lines 151 - 159, Snapshot the Doctrine collection returned by member->getSponsorMemberships() into an array before the loop, then iterate that snapshot while calling SummitSponsorService::removeSponsorUser(). Preserve the existing sponsor ID assignment and logging behavior.
🧹 Nitpick comments (2)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
352-373: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: assert the queued jobs as well.
The test proves the member row survives the rollback. The regression it documents is that jobs referencing the member id are pushed before commit. An assertion on the queue makes the contract explicit. Add
Queue::fake()before the call, then assert the member-related jobs were pushed and that the member exists.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php` around lines 352 - 373, Add queue assertions to the transaction rollback test around addSponsorUserToGroup: call Queue::fake() before invoking the service, then assert the expected member-related jobs were dispatched and reference the surviving member. Keep the existing EntityNotFoundException and member persistence assertions intact.app/Services/Model/Imp/SponsorUserSyncService.php (1)
102-111: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRename or document the write side effect in
validateParams.
validateParamsis public and now registers a member through the IDP when the local row is missing. The name states validation only. A caller can trigger a member creation and job dispatch without expecting it. Rename to something likeresolveParams, or state the side effect in the docblock.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Services/Model/Imp/SponsorUserSyncService.php` around lines 102 - 111, Rename the public validateParams method and all its call sites to reflect that resolveMember may register missing members and dispatch work, using a name such as resolveParams; alternatively, add a docblock explicitly documenting this write side effect while preserving the existing summit and member resolution behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 241-257: Update removeSponsorUserFromGroup and the shared
removeSponsorUser/validateParams flow to avoid calling resolveMember with
on-demand IDP registration enabled. For removal events, look up the existing
local Member and return without changes when it is absent; alternatively, add
and use an explicit resolver option that disables registration while preserving
registration for add paths.
---
Outside diff comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 151-159: Snapshot the Doctrine collection returned by
member->getSponsorMemberships() into an array before the loop, then iterate that
snapshot while calling SummitSponsorService::removeSponsorUser(). Preserve the
existing sponsor ID assignment and logging behavior.
---
Nitpick comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 102-111: Rename the public validateParams method and all its call
sites to reflect that resolveMember may register missing members and dispatch
work, using a name such as resolveParams; alternatively, add a docblock
explicitly documenting this write side effect while preserving the existing
summit and member resolution behavior.
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 352-373: Add queue assertions to the transaction rollback test
around addSponsorUserToGroup: call Queue::fake() before invoking the service,
then assert the expected member-related jobs were dispatched and reference the
surviving member. Keep the existing EntityNotFoundException and member
persistence assertions intact.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 9c7b5a82-efa0-48da-89d2-0f50b342f13f
📒 Files selected for processing (2)
app/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
Member::getSponsorMemberships() is a plain ManyToMany to Sponsor with no summit scoping, so the null-sponsor_id branch (the auth_user_removed_from_summit event, which carries no sponsor_id) also iterated sponsors belonging to OTHER summits. Those do not resolve against the event's summit, so SummitSponsorService::removeSponsorUser threw "Sponsor not found." and aborted the loop, leaving this summit's own memberships un-revoked. Iteration order is not guaranteed, so it could abort on the first pass and revoke nothing. Multi-summit sponsor users are legitimate: addSponsorUser only rejects summits whose dates overlap, so the same member can sponsor across different years. With the surrounding try/catch now removed, that abort no longer fails silently - it exhausts the job's 3 tries and lands in failed_jobs. Filter the loop by summit, and stop shadowing the $sponsor_id parameter with the loop variable so the log line reports the sponsor actually processed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
resolveMember() registers the member from the IDP when it was never synced. That is right for the add paths - it is what this branch set out to fix - but both revocation entry points went through it too, so a removal event for a member that does not exist locally would create a Member row, run a full synchronizeGroups and dispatch NewMember / MemberDataUpdatedExternally (and with them MemberAssocSummitOrders, UpdateAttendeeInfo, CleanMemberCacheJob) only to then revoke nothing: a member that did not exist owns no Sponsor_Users row and no group membership. The IDP-deleted case was worse. sponsor-users-api emits one removal event per access right when a user is deleted, and by then PublishUserDeleted has already removed the local Member, so getUserById returns null, resolveMember throws, and the job burns its 3 tries into a permanently unresolvable failed_jobs entry for an event that had nothing to do. Add findMember() (lookup without registration) and use it in removeSponsorUser and removeSponsorUserFromGroup: an unknown member is now a logged no-op. Extract resolveSummit() so removeSponsorUser keeps validating the summit without going through validateParams. The add paths and validateParams keep resolveMember. Skipping an unknown member does not turn these into swallow-everything handlers - a genuine failure still propagates, covered by testRemoveSponsorUserPropagatesErrorWhenSummitDoesNotExist. testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist asserted the old behaviour and is rewritten as testRemoveSponsorUserIsNoOpWhenMemberWasNeverSynced.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
There was a problem hiding this comment.
♻️ Duplicate comments (1)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
302-323: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winRun dynamic-member cleanup in a
finallyblock.If the service call or an assertion fails after registration, lines 320-322 do not run. The dynamically created
Membercan affect later tests.This repeats a prior review finding that is still present in the current code.
Proposed fix
- $this->getService()->addSponsorUserToGroup( - $external_id, - IGroup::Sponsors, - $sponsor_id, - $summit_id - ); + try { + $this->getService()->addSponsorUserToGroup( + $external_id, + IGroup::Sponsors, + $sponsor_id, + $summit_id + ); - // Member must have been registered on demand from the IDP... - // (clear first: the in-service instance memoizes a pre-grant - // belongsToGroup(false) in its groupMembershipCache) - self::$em->clear(); - $member = self::$member_repository->getByExternalId($external_id); - $this->assertNotNull($member, 'Member should have been registered on demand'); + self::$em->clear(); + $member = self::$member_repository->getByExternalId($external_id); + $this->assertNotNull($member, 'Member should have been registered on demand'); - // ...with the Sponsor_Users row + permission written and the group granted. - $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member->getId())); - $this->assertTrue($member->belongsToGroup(IGroup::Sponsors)); - - // Cleanup: this member is created outside the trait's tearDown scope. - self::$em->remove($member); - self::$em->flush(); + $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member->getId())); + $this->assertTrue($member->belongsToGroup(IGroup::Sponsors)); + } finally { + self::$em = self::reopenEntityManager(); + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php` around lines 302 - 323, Wrap the dynamic-member registration, assertions, and cleanup in a finally block so the created Member is removed and flushed even when the service call or an assertion fails. Update the test method around addSponsorUserToGroup and the subsequent member lookup, preserving the existing assertions while ensuring cleanup runs only when a member was successfully registered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 302-323: Wrap the dynamic-member registration, assertions, and
cleanup in a finally block so the created Member is removed and flushed even
when the service call or an assertion fails. Update the test method around
addSponsorUserToGroup and the subsequent member lookup, preserving the existing
assertions while ensuring cleanup runs only when a member was successfully
registered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 1c6039c6-7823-4140-aaff-a32481b504a3
📒 Files selected for processing (2)
app/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
🚧 Files skipped from review as they are similar to previous changes (1)
- app/Services/Model/Imp/SponsorUserSyncService.php
Two problems behind the same symptom: AddSponsorMemberMQJob failing and the
sponsor user never getting their Sponsor_Users row.
1. Stale local groups.
Sponsor::addUser rejects a member belonging to none of its AllowedMemberGroups.
sponsor-users-api grants that group at the IDP before publishing the membership
event (_sync_user_groups), so the IDP is already right - it is summit-api's copy
that is stale, and it only refreshes through the IDP's own user-updated event,
which races this one.
resolveMember already covers the member that does not exist locally: registering
it pulls fresh groups. The member that DOES exist was returned untouched and hit
the validation. ensureSponsorGroupMembership() now re-reads it from the IDP in
that case.
Nothing downstream would have repaired the failure: the producers of
auth_user_added_to_sponsor_and_summit (_import_user, _notify_approval) publish no
companion group event, so no eager-create path runs and the access is lost - this
is not just noise in failed_jobs.
2. The retry policy was inert.
Job::maxTries() and Job::backoff() read the job PAYLOAD, not the properties of
the handler class the payload names, so the `public int $tries = 3` on the four
SponsorServices handlers was never seen by the worker. With nothing in the
payload the worker falls back to the command options, and the entry point runs
`doctrine:queue:work sponsor_users_sync_consumer` with no flags - i.e. the
--tries=1 / --backoff=0 defaults. One failure was terminal.
Put maxTries and backoff ('30,120') in the payload so the declared policy
applies. This takes all three handlers from a single attempt to three spaced
ones; they are idempotent (addUser/removeUser early-return, add/removeSponsorPermission
are idempotent by design), so replaying them is safe.
Tests cover the refresh path and assert maxTries()/backoff() - what the worker
actually consults - rather than the shape of the payload array.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing removed the member it provisions only after its last assertion, so any failure above leaked it into the next test's database. That member is created on demand and therefore lives outside the trait's tearDown scope, so nothing else reclaims it. Not hypothetical: this database already carried a member from an earlier run (smarcet+ondemand_sfxvesem@gmail.com) left behind exactly this way. Leaked fixture rows are expensive here - a stray Group with a duplicate Code makes getBySlug return the wrong row and silently breaks an unrelated test. Wrap the body in try/finally and look the member up by external id in the finally rather than reusing $member, since the failure may predate its assignment. Verified by forcing an assertion failure: the leftover count stayed flat instead of growing. No manual Sponsor_Users cleanup is needed - measured before and after, Doctrine already clears the join table rows when the member is removed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Suppressed comments (3)
app/Services/Model/Imp/SponsorUserSyncService.php:237
- Log message contains a grammatical typo ("removed from to summit").
"SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from to summit {$summit_id} for sponsor {$sponsor_id}");
app/Services/Model/Imp/SponsorUserSyncService.php:330
- $summit_id is captured into the removeSponsorUserFromGroup transaction closure but never used, which adds noise and can confuse future edits.
$this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) {
tests/Unit/Services/SponsorUserPermissionTrackingTest.php:85
- This test helper creates Mockery mocks but the test class never calls Mockery::close(), which can cause Mockery to report an unclosed container / unmet expectations at the end of the test run. Register a once-per-test callback to close Mockery when the application is destroyed.
$api = \Mockery::mock(\App\Services\Apis\IExternalUserApi::class)
->shouldIgnoreMissing();
$api->shouldReceive('getUserById')->andReturn($user_data);
$this->app->instance(\App\Services\Apis\IExternalUserApi::class, $api);
$this->app->forgetInstance(\App\Services\Model\IMemberService::class);
The single-sponsor branch read "removed from to summit" and interpolated the raw $summit_id parameter while its sibling branch uses $summit->getId(). Fix the wording and use the resolved summit so both branches emit the same shape, which matters for grepping and alerting on these lines. Flagged by Copilot on PR #582; the thread was resolved without the change being applied.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
… default exchange The delay queue laterRaw() declares dead-letters into the consumer exchange (sponsor-users-api-message-broker) with the QUEUE NAME as routing key. That exchange is direct and only binds the five auth_user_* routing keys, so every released retry was unroutable and silently dropped - worse than the old tries=1 behavior, which at least parked the failure in queue_failed_jobs. SponsorServicesMQJob::release() now declares the delay queue first (the declared-names cache keeps laterRaw from re-declaring it) dead-lettering through the DEFAULT exchange, which routes by queue name with no binding required. Since redelivery rewrites the routing key to the queue name, the original event type is preserved in the republished body (x_event_type) and getEventType() recovers it - payload() and both handlers that branch on the event type (RemoveSponsorMemberMQJob would otherwise treat a retried sponsor-scoped removal as a summit-wide one, UpdateSponsorMemberGroupsMQJob would match no branch and delete the job) now resolve through it.
sponsor-users-api's metamodel reconciler reaps sponsors summit-api stopped returning THROUGH remove_sponsor_show_permissions, so it emits auth_user_removed_from_sponsor_and_summit precisely when the sponsor no longer exists on this side. getSummitSponsorById() then returns null and SummitSponsorService::removeSponsorUser threw "Sponsor not found." on every attempt - burning the job's retries to revoke something already gone and parking a permanently unresolvable entry in failed_jobs. A sponsor that no longer resolves against the event's summit is now nothing-to-revoke (warn + skip), same contract as the never-synced member. A missing SUMMIT still propagates: summit_deleted flows to sponsor-users-api, which stops emitting for it, so an unknown summit remains a genuine anomaly.
…re-sync ensureSponsorGroupMembership used registerExternalUserById, whose synchronizeGroups(allow_removals: true) strips every non-skip-listed local group absent from the IDP payload - a sponsor-membership event could remove e.g. summit-administrators as a side effect (the previous test even baked that stripping in as expected behavior). Fetch the IDP profile and run the already-existing additive mode (synchronizeGroups(..., false)) instead: this event only ever grants access; removals stay owned by the IDP's own user_updated flow (PublishUserUpdated). resolveMember keeps the full registration - there the member is brand new and a complete sync is correct.
The MQ payload's group_slug was granted (or stripped) as-is: the shared broker vhost gives write access to several service users, so a forged or buggy auth_user_added_to_group / auth_user_removed_from_group event could add a member to - or remove one from - an arbitrary group like administrators. Both group entry points now reject any slug outside Sponsor::AllowedMemberGroups with a ValidationException, so a producer bug stays visible in failed_jobs instead of silently mutating memberships. The gate runs before resolveMember so a rejected event can never provision a member from the IDP as a side effect; the rollback-survival test now triggers its in-transaction failure with an unknown sponsor instead of an unknown group slug.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 6 out of 6 changed files in this pull request and generated no new comments.
Suppressed comments (1)
app/Jobs/SponsorServices/SponsorServicesMQJob.php:66
getEventType()assumesjson_decode($this->getRawBody(), true)always returns an array. If the body is invalid JSON,$bodybecomesnulland$body[self::EventTypeKey]will trigger an “array offset on null” error, potentially breaking handler selection on retries/redeliveries. Default to an empty array when decoding fails.
$body = json_decode($this->getRawBody(), true);
return $body[self::EventTypeKey] ?? $routing_key;
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Jobs/SponsorServices/SponsorServicesMQJob.php`:
- Around line 148-153: Update the retry metadata assignment in
SponsorServicesMQJob’s release flow to always overwrite body[self::EventTypeKey]
with the resolved getEventType() value, rather than retaining a supplied
x_event_type. Preserve redelivery idempotency and add a regression test covering
a first-delivery message whose x_event_type conflicts with the routing-derived
event type.
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 300-303: Update
app/Services/Model/Imp/SponsorUserSyncService.php:300-303 in the group grant
handler to resolve summit_id and verify it owns sponsor_id before provisioning
or writing permissions; apply the same ownership check in
app/Services/Model/Imp/SponsorUserSyncService.php:375-375 before removing
permissions or global group membership. Add cross-summit sponsor grant and
removal coverage in
tests/Unit/Services/SponsorUserPermissionTrackingTest.php:207-239, asserting
neither Sponsor_Users permissions nor global group membership changes.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 2e615965-64e7-4d1d-9d65-2182674e35f2
📒 Files selected for processing (6)
app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.phpapp/Jobs/SponsorServices/SponsorServicesMQJob.phpapp/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Services/Model/Imp/SponsorUserSyncService.phptests/Unit/Jobs/SponsorServicesMQJobRetryTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
Two broker-side failure modes in the release() path, both invisible to the mocked unit tests and caught by the new live-broker integration test: 1. RabbitMQQueue::declareQueue() declares on the broker but does NOT record the name in the declared-names cache (only isQueueExists() populates it), so laterRaw() re-declared the delay queue release() had just created with the library's own dead-letter arguments, and the broker rejected the inequivalent x-dead-letter-exchange with PRECONDITION_FAILED on every single release. 2. Suppressing that re-declare by priming the cache is no fix: the delay queue carries x-expires, so the broker deletes it when idle - a once-per- worker-process declare means every release after an expiry publishes into a deleted queue and the retry is dropped silently. (laterRaw survives this only because it re-declares unconditionally.) release() now declares the delay queue and publishes the retry directly on the channel, bypassing laterRaw(): an unconditional queue_declare per release re-creates the queue when it expired and is a no-op (equivalent args) when it did not. The integration test red-greens both modes against the real broker: publish -> pop -> release(1) -> redelivered with the original event type and attempts=2, and again after sleeping past x-expires. It skips itself when no broker is reachable.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
The group handlers validated group_slug but never that sponsor_id belongs to the event's summit_id: a forged or buggy event carrying another summit's sponsor could write - or remove - that sponsor's Permissions entry, and the removal path could strip the member's global group. The producer derives both ids from the same AccessRight, so a mismatch is never legitimate. Grant path: reject with a ValidationException when the sponsor does not resolve on the event summit, BEFORE resolveMember - a rejected event must not provision a member from the IDP as a side effect, and the failure stays visible in failed_jobs. Removal path: skip (warn) ONLY when the sponsor exists on a DIFFERENT summit. A sponsor deleted entirely must still run the removal: that is what recomputes the remaining permission count and strips the global sponsors group when this was the member's last sponsor - requiring existence would leave residual show-admin access forever (pinned by the new cleanup test). Also in the touched test class: the rollback-survival test now triggers its in-transaction failure via an allowed group slug with no Group row (its previous trigger dies at the new ownership gate), and the force-initialize workaround in the global-group removal test is gone - its ORM-blaming comment misdiagnosed what was actually leaked duplicate Group fixture rows (fixed in the next commit); on a clean database the removal works without it. Originally flagged by CodeRabbit; severity assessed lower (the producer cannot emit a mismatch - the trigger is a forged/buggy publisher on the shared vhost), fix applied for defense in depth.
clearMemberTestData / clearSummitTestData already reopen the entity manager when a failed tx_service transaction closed it, but kept using the repository instances captured at setup - which are bound to the CLOSED manager. Every find() then threw "EntityManager is closed", clearMemberTestData's empty catch swallowed it, and the fixtures leaked into the shared test database. Those leaks are not benign: the local DB had accumulated 7 duplicate Group rows with Code='sponsors' (and ~1200 fixture members). With duplicates, getBySlug() resolves the oldest stale row while the member belongs to the fixture's row, so Member::removeFromGroup's identity-based contains() returns false and group removals become silent no-ops - the failure mode previously misattributed to ORM 3 EXTRA_LAZY collection semantics and worked around with a force-initialize in the removal test. Re-resolve the repositories from the fresh manager after a reset, and log cleanup failures to STDERR instead of swallowing them, so a future leak is visible the day it starts.
… on release release() preserved an x_event_type already present in the producer body. On a first delivery the routing key is authoritative and getEventType() ignores the body - but the preserved value would take over on the RETRY, so a forged or buggy body key could make a retried event run a different handler than its original delivery (e.g. an add retried as a remove). Always write the resolved event type instead: on a redelivery getEventType() already resolves from the body, so the rewrite stays idempotent (covered by the existing second-release test). No new capability for an attacker who can publish to the vhost (they already control the routing key) - this closes the inconsistency, not a privilege path. Originally flagged by CodeRabbit, confirmed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
1 similar comment
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
Collaborator
Author
|
@coderabbitai full review |
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
tests/Unit/Services/SponsorUserPermissionTrackingTest.php (1)
676-715: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse
createCrossSummitSponsoranddeleteCrossSummitSponsorhere.Lines 677-682 duplicate
createCrossSummitSponsor, and thefinallyat lines 711-715 deletes the sponsor with raw SQL. The docblock ofdeleteCrossSummitSponsorat lines 260-266 states why raw SQL is wrong here: the managedSponsorentity stays in the unit of work and references aSummitthat teardown removes, so the next flush fails and the fixtures leak into the shared test database.The
self::$em->clear()at line 701 hides that only on the success path. IfremoveSponsorUserthrows, or if the pre-condition assertions fail, the entity is still managed when the raw DELETE runs.♻️ Proposed fix
- // setUp() ends with an em->clear(), so the static fixture entities are - // detached: re-fetch them or Doctrine treats them as new on persist. - $member = self::$member_repository->find(self::$member->getId()); - $summit2 = self::$summit_repository->getById(self::$summit2->getId()); - $company = self::$em->find(\models\main\Company::class, self::$companies[1]->getId()); - - // A second sponsor, belonging to a DIFFERENT summit, with the same member. - $other_sponsor = new \models\summit\Sponsor(); - $other_sponsor->setCompany($company); - $summit2->addSummitSponsor($other_sponsor); - $other_sponsor->addUser($member); - self::$em->persist($other_sponsor); - self::$em->flush(); + // setUp() ends with an em->clear(), so the static fixture entities are + // detached: re-fetch them or Doctrine treats them as new on persist. + $member = self::$member_repository->find(self::$member->getId()); + + // A second sponsor, belonging to a DIFFERENT summit, with the same member. + $other_sponsor = $this->createCrossSummitSponsor(); + $other_sponsor->addUser($member); + self::$em->flush();} finally { - $conn = self::$em->getConnection(); - $conn->executeStatement('DELETE FROM Sponsor_Users WHERE SponsorID = ?', [$other_sponsor_id]); - $conn->executeStatement('DELETE FROM Sponsor WHERE ID = ?', [$other_sponsor_id]); + $this->deleteCrossSummitSponsor($other_sponsor); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php` around lines 676 - 715, Replace the duplicated sponsor setup with the existing createCrossSummitSponsor helper, retaining its returned sponsor for IDs and assertions. Replace the raw SQL cleanup in the finally block with deleteCrossSummitSponsor, and ensure cleanup clears or detaches managed entities before deletion so it runs safely even when removeSponsorUser or precondition assertions fail.tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php (1)
35-65: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the broker test out of the unit suite or tag it.
This test opens a real AMQP connection, declares broker topology, and waits up to 10 seconds per poll. It sits in
tests/Unit, so a normal unit run becomes slow and dependent on external infrastructure. Add a PHPUnit group so CI can exclude it, or move the file to an integration directory.The
uniqid()hit from static analysis is a false positive. The value only makes fixture names unique.Proposed change
+use PHPUnit\Framework\Attributes\Group; + +#[Group('integration')] final class SponsorServicesMQJobReleaseIntegrationTest extends TestCase🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php` around lines 35 - 65, Tag SponsorServicesMQJobReleaseIntegrationTest as an integration or broker-dependent PHPUnit group so standard unit runs can exclude it, while preserving the existing test behavior and unique uniqid()-based fixture names.Source: Linters/SAST tools
tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php (1)
146-206: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test for the zero-delay floor and the attempts header.
Two behaviors of
release()have no coverage in this file:
release(0)takes the$ttl <= 0branch and must publish into<queue>.delay.1000. That branch prevents an unroutable publish, so it deserves a pinned test.- The republished message carries
application_headerswithlaravel.attempts. No assertion checks that header, so a regression that drops it would only fail in the integration test, which skips when no broker is reachable.Proposed additional test
public function testReleaseWithoutDelayStillUsesTheDelayQueueAndCarriesTheAttemptHeader(): void { $queue_name = 'sponsor-users-api-summit-api-badge-scans-queue'; $message = new AMQPMessage(json_encode(['user_external_id' => 1])); $message->setDeliveryInfo(1, false, 'sponsor_users', EventTypes::AUTH_USER_ADDED_TO_GROUP); $rabbitmq = Mockery::mock(RabbitMQQueue::class); $channel = Mockery::mock(\PhpAmqpLib\Channel\AMQPChannel::class); $rabbitmq->shouldReceive('getChannel')->andReturn($channel); $channel->shouldReceive('queue_declare')->once(); $published = null; $channel->shouldReceive('basic_publish')->once()->with( Mockery::on(function ($msg) use (&$published) { $published = $msg; return $msg instanceof AMQPMessage; }), '', $queue_name . '.delay.1000' ); $rabbitmq->shouldReceive('ack')->once(); (new SponsorServicesMQJob(app(), $rabbitmq, $message, 'rabbitmq', $queue_name))->release(0); $headers = $published->get('application_headers')->getNativeData(); $this->assertSame(1, $headers['laravel']['attempts']); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php` around lines 146 - 206, Add a unit test alongside testReleaseDeadLettersBackThroughTheDefaultExchange to call SponsorServicesMQJob::release(0), assert the message is published through the default exchange with the queue’s .delay.1000 routing key, and verify application_headers contains laravel.attempts equal to 1. Configure the existing RabbitMQ/channel mocks to expect queue declaration and acknowledgment.app/Jobs/SponsorServices/SponsorServicesMQJob.php (1)
166-180: 🩺 Stability & Availability | 🔵 Trivial | ⚖️ Poor tradeoffConsider publisher confirms before the ack.
The code publishes the retry and then acks the original delivery. Without publisher confirms, a broker-side publish failure is not reported, and the ack then drops the only copy of the event. If you want the retry path to be loss-free, enable confirm mode on the channel and ack only after the confirm.
The
uniqid('', true)hit from static analysis is a false positive here. The value is a correlation identifier for tracing, not a security token.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/Jobs/SponsorServices/SponsorServicesMQJob.php` around lines 166 - 180, Update the retry publish flow in the job method containing basic_publish so the channel uses publisher-confirm mode and waits for the broker confirmation before calling $this->rabbitmq->ack($this). Preserve the existing message payload and correlation identifier, and only acknowledge the original delivery after the publish is confirmed; propagate publish-confirmation failures without acknowledging it.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/Services/Model/Imp/SponsorUserSyncService.php`:
- Around line 228-229: Remove PII from the debug logs in
SponsorUserSyncService::addSponsorUser and
SponsorUserSyncService::removeSponsorUser by replacing member email
interpolation with member ID interpolation. Apply this change at
app/Services/Model/Imp/SponsorUserSyncService.php lines 228-229 and 263-264,
preserving the existing log context.
In `@tests/InsertMemberTestData.php`:
- Around line 196-201: Update the exception handler around clearMemberTestData()
to rethrow the caught exception after writing the diagnostic to STDERR, ensuring
tearDown() propagates the cleanup failure and PHPUnit marks the test as failed.
- Around line 163-170: Refresh cleanup repositories from the current self::$em
before any find() calls in both clearMemberTestData() and clearSummitTestData().
In tests/InsertMemberTestData.php:163-170, resolve Group and Member
repositories; in tests/InsertSummitTestData.php:1054-1059, resolve Summit,
SummitAdministratorPermissionGroup, and SummitMediaFileType repositories. Ensure
both cleanup methods always use repositories from the current open entity
manager.
---
Nitpick comments:
In `@app/Jobs/SponsorServices/SponsorServicesMQJob.php`:
- Around line 166-180: Update the retry publish flow in the job method
containing basic_publish so the channel uses publisher-confirm mode and waits
for the broker confirmation before calling $this->rabbitmq->ack($this). Preserve
the existing message payload and correlation identifier, and only acknowledge
the original delivery after the publish is confirmed; propagate
publish-confirmation failures without acknowledging it.
In `@tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php`:
- Around line 35-65: Tag SponsorServicesMQJobReleaseIntegrationTest as an
integration or broker-dependent PHPUnit group so standard unit runs can exclude
it, while preserving the existing test behavior and unique uniqid()-based
fixture names.
In `@tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php`:
- Around line 146-206: Add a unit test alongside
testReleaseDeadLettersBackThroughTheDefaultExchange to call
SponsorServicesMQJob::release(0), assert the message is published through the
default exchange with the queue’s .delay.1000 routing key, and verify
application_headers contains laravel.attempts equal to 1. Configure the existing
RabbitMQ/channel mocks to expect queue declaration and acknowledgment.
In `@tests/Unit/Services/SponsorUserPermissionTrackingTest.php`:
- Around line 676-715: Replace the duplicated sponsor setup with the existing
createCrossSummitSponsor helper, retaining its returned sponsor for IDs and
assertions. Replace the raw SQL cleanup in the finally block with
deleteCrossSummitSponsor, and ensure cleanup clears or detaches managed entities
before deletion so it runs safely even when removeSponsorUser or precondition
assertions fail.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: fd922644-b222-41cc-9252-ed830a534a74
📒 Files selected for processing (9)
app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.phpapp/Jobs/SponsorServices/SponsorServicesMQJob.phpapp/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.phpapp/Services/Model/Imp/SponsorUserSyncService.phptests/InsertMemberTestData.phptests/InsertSummitTestData.phptests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.phptests/Unit/Jobs/SponsorServicesMQJobRetryTest.phptests/Unit/Services/SponsorUserPermissionTrackingTest.php
The previous hardening re-resolved the repositories only when self::$em was closed. That misses the case where the manager was reset mid-test and a test finally already reopened it: self::$em is then fresh and OPEN, the isOpen() check skips the refresh, and the repositories captured at setup still point at the closed manager - the cleanup can fail and leak fixtures all the same. Re-resolve them unconditionally at cleanup entry in both traits; the isOpen() check remains only to decide whether the manager itself needs a reset. Flagged by CodeRabbit on the previous hardening commit, confirmed.
Logging the failure to STDERR was not enough: a green test with leaked fixtures is still green, and nobody reads stderr in CI. The silent catch is how the shared test database accumulated months of leaked rows (7 duplicate 'sponsors' Group rows, ~1200 fixture members) that turned group removals into silent no-ops and got misdiagnosed as an ORM bug. Rethrow after logging so a cleanup failure fails the test the day it starts happening - clearSummitTestData already propagates, this makes both paths symmetric. Flagged by CodeRabbit, confirmed.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
The rethrow in 491a885 did its job on CI: EntityModelUnitTests failed on SummitAttendeeTest::testAddSummitAttendee because its cleanup flush was ALREADY broken - the test builds an unpersisted object graph (ticket, ticket type, badge) hanging off managed fixtures, and clearMemberTestData's flush choked on it with 'non-persisted new entities found through the association graph'. The old empty catch had been swallowing exactly this for who knows how long, leaking the member/group fixtures every run of that test. Clear the entity manager at cleanup entry in both traits, then reload by id: the cleanup must only ever flush its own removals, never whatever the test left pending in the unit of work. Verified locally against the failing CI shard (tests/Unit/Entities/ 40/40), plus tests/Unit/Jobs/, tests/Unit/Services/ and tests/Repositories/ - all green, zero leaked fixture rows after the runs.
|
📘 OpenAPI / Swagger preview ➡️ https://OpenStackweb.github.io/summit-api/openapi/pr-582/ This page is automatically updated on each push to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
ref: https://app.clickup.com/t/9014802374/86bbag3p8
Summary by CodeRabbit
New Features
Bug Fixes