From 2bc1e3b1a253bdadf960a0a9b8306561d46bc5d2 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 12:41:47 -0300 Subject: [PATCH 01/20] fix: grant sponsor group before eager Sponsor_Users creation in user 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. --- .../Model/Imp/SponsorUserSyncService.php | 44 +++++++------- .../SponsorUserPermissionTrackingTest.php | 58 +++++++++++++++++++ 2 files changed, 81 insertions(+), 21 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 98708a478..a72b2e3ba 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -90,22 +90,21 @@ public function validateParams(int $summit_id, int $user_id): array */ public function addSponsorUser(int $summit_id, int $sponsor_id, int $user_id): void { - try { - Log::debug( - "SponsorUserSyncService::addSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}"); + // Do NOT swallow failures here: the MQ job (tries = 3) needs the + // exception to apply its retry / failed_jobs machinery. A swallowed + // failure loses the membership event silently. + Log::debug( + "SponsorUserSyncService::addSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}"); - list($summit, $member) = $this->validateParams($summit_id, $user_id); + list($summit, $member) = $this->validateParams($summit_id, $user_id); - Log::debug( - "SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); + Log::debug( + "SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); - $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); + $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); - Log::info( - "SponsorUserSyncService::addSponsorUser member {$member->getId()} successfully added to sponsor {$sponsor_id}"); - } catch (\Exception $ex) { - Log::error($ex); - } + Log::info( + "SponsorUserSyncService::addSponsorUser member {$member->getId()} successfully added to sponsor {$sponsor_id}"); } /** @@ -155,6 +154,18 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo throw new EntityNotFoundException("Member with id {$user_id} not found"); } + // Grant the global group FIRST: Sponsor::addUser (reached through the + // eager-create path below) validates the member already belongs to a + // sponsor group, and for a brand-new sponsor user this very event is + // what delivers that group. + if (!$member->belongsToGroup($group_slug)) { + $group = $this->group_repository->getBySlug($group_slug); + if (is_null($group)) { + throw new EntityNotFoundException("Group {$group_slug} not found"); + } + $member->add2Group($group); + } + // Add permission entry to the Sponsor_Users JSON column for this sponsor-member pair. // If the row does not exist yet (MQ ordering race: group event arrived before membership // event), create it eagerly so the permission is never silently dropped. @@ -184,15 +195,6 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo } } - // Add to global group only if not already a member. - if (!$member->belongsToGroup($group_slug)) { - $group = $this->group_repository->getBySlug($group_slug); - if (is_null($group)) { - throw new EntityNotFoundException("Group {$group_slug} not found"); - } - $member->add2Group($group); - } - Log::info( "SponsorUserSyncService::addSponsorUserToGroup member {$member->getId()} added to group {$group_slug} via sponsor {$sponsor_id}"); }); diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 68a3acef6..9f9948142 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -123,6 +123,44 @@ public function testAddSponsorUserToGroupEagerlyCreatesRowAndWritesPermissionOnR $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member_id)); } + /** + * Brand-new sponsor user: the member exists but does NOT belong to any + * sponsor group yet - the group grant is exactly what this event delivers. + * The eager-create path must not fail Sponsor::addUser's group validation + * (chicken-and-egg: the validation requires the group this handler grants). + */ + public function testAddSponsorUserToGroupCreatesRowWhenMemberHasNoSponsorGroupYet(): void + { + // member2 belongs only to SummitAdministrators - no sponsor group, + // exactly the state of a brand-new sponsor user's member row. + $member_id = self::$member2->getId(); + $external_id = self::$member2->getUserExternalId(); + $sponsor_id = self::$sponsors[1]->getId(); // no Sponsor_Users row + $summit_id = self::$summit->getId(); + + // Pre-condition: the member must NOT belong to any sponsor group. + $this->assertFalse( + self::$member_repository->find($member_id)->belongsToGroup(IGroup::Sponsors), + 'Pre-condition: member should not belong to the sponsors group' + ); + + $this->getService()->addSponsorUserToGroup( + $external_id, + IGroup::Sponsors, + $sponsor_id, + $summit_id + ); + + // Row created + permission written... + $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member_id)); + + // ...and the member ended up in the global group. + self::$em->clear(); + $this->assertTrue( + self::$member_repository->find($member_id)->belongsToGroup(IGroup::Sponsors) + ); + } + /** * The group slug must be written into the Sponsor_Users.Permissions JSON * column for the correct (SponsorID, MemberID) row. @@ -164,6 +202,26 @@ public function testAddSponsorUserToGroupIsIdempotent(): void $this->assertCount(1, $occurrences); } + // ------------------------------------------------------------------------- + // addSponsorUser (membership event) + // ------------------------------------------------------------------------- + + /** + * When the member does not exist yet in summit-api (brand-new IDP user whose + * member row has not been synced), the failure must PROPAGATE so the MQ job's + * retry/failed_jobs machinery applies - not be swallowed and lost silently. + */ + public function testAddSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void + { + $this->expectException(\models\exceptions\EntityNotFoundException::class); + + $this->getService()->addSponsorUser( + self::$summit->getId(), + self::$sponsors[1]->getId(), + PHP_INT_MAX // external user id with no matching Member row + ); + } + // ------------------------------------------------------------------------- // removeSponsorUserFromGroup // ------------------------------------------------------------------------- From a44f9cd31d7ebefd77e9e016e87a1616cc6e900f Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 12:47:06 -0300 Subject: [PATCH 02/20] fix: propagate removeSponsorUser failures to MQ job retry machinery 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. --- .../Model/Imp/SponsorUserSyncService.php | 37 +++++++++---------- .../SponsorUserPermissionTrackingTest.php | 16 ++++++++ 2 files changed, 34 insertions(+), 19 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index a72b2e3ba..2b0fb3410 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -112,31 +112,30 @@ public function addSponsorUser(int $summit_id, int $sponsor_id, int $user_id): v */ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id = null): void { - try { - Log::debug( - "SponsorUserSyncService::removeSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}"); - - list($summit, $member) = $this->validateParams($summit_id, $user_id); + // Do NOT swallow failures here: a lost removal event silently leaves + // the user with access they should have lost. Propagate so the MQ job + // (tries = 3) applies its retry / failed_jobs machinery. + Log::debug( + "SponsorUserSyncService::removeSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}"); - Log::debug( - "SponsorUserSyncService::removeSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); + list($summit, $member) = $this->validateParams($summit_id, $user_id); - if (is_null($sponsor_id)) { - foreach ($member->getSponsorMemberships() as $sponsor_membership) { - $sponsor_id = $sponsor_membership->getId(); - $this->summit_sponsor_service->removeSponsorUser($summit, $sponsor_id, $member->getId()); + Log::debug( + "SponsorUserSyncService::removeSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); - Log::info( - "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from summit {$summit->getId()} for sponsor {$sponsor_id}" - ); - } - } else { + if (is_null($sponsor_id)) { + foreach ($member->getSponsorMemberships() as $sponsor_membership) { + $sponsor_id = $sponsor_membership->getId(); $this->summit_sponsor_service->removeSponsorUser($summit, $sponsor_id, $member->getId()); + Log::info( - "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from to summit {$summit_id} for sponsor {$sponsor_id}"); + "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from summit {$summit->getId()} for sponsor {$sponsor_id}" + ); } - } catch (\Exception $ex) { - Log::error($ex); + } else { + $this->summit_sponsor_service->removeSponsorUser($summit, $sponsor_id, $member->getId()); + Log::info( + "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from to summit {$summit_id} for sponsor {$sponsor_id}"); } } diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 9f9948142..d3ac820a5 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -222,6 +222,22 @@ public function testAddSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void ); } + /** + * A swallowed removal failure is worse than a swallowed addition: the user + * silently RETAINS access they should have lost. The failure must propagate + * so the MQ job's retry / failed_jobs machinery applies. + */ + public function testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void + { + $this->expectException(\models\exceptions\EntityNotFoundException::class); + + $this->getService()->removeSponsorUser( + self::$summit->getId(), + PHP_INT_MAX, // external user id with no matching Member row + self::$sponsors[0]->getId() + ); + } + // ------------------------------------------------------------------------- // removeSponsorUserFromGroup // ------------------------------------------------------------------------- From c2c0e35ce8d6e9fcfd53963c04deea33b91e04d4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 12:54:53 -0300 Subject: [PATCH 03/20] feat: register member on demand from IDP in sponsor user sync 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). --- .../Model/Imp/SponsorUserSyncService.php | 43 ++++++++--- .../SponsorUserPermissionTrackingTest.php | 75 +++++++++++++++++++ 2 files changed, 106 insertions(+), 12 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 2b0fb3410..61e3a0452 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -13,6 +13,7 @@ **/ use App\Services\Model\AbstractService; +use App\Services\Model\IMemberService; use App\Services\Model\ISponsorUserSyncService; use Illuminate\Support\Facades\Log; use LaravelDoctrine\ORM\Facades\Registry; @@ -20,6 +21,7 @@ use models\exceptions\EntityNotFoundException; use models\main\IGroupRepository; use models\main\IMemberRepository; +use models\main\Member; use models\summit\ISummitRepository; use models\summit\Summit; use models\utils\SilverstripeBaseModel; @@ -41,12 +43,15 @@ final class SponsorUserSyncService private ISummitSponsorService $summit_sponsor_service; + private IMemberService $member_service; + /** * SponsorUserSyncService constructor. * @param ISummitRepository $summit_repository * @param IMemberRepository $member_repository * @param IGroupRepository $group_repository * @param ISummitSponsorService $summit_sponsor_service + * @param IMemberService $member_service * @param ITransactionService $tx_service */ public function __construct @@ -55,6 +60,7 @@ public function __construct IMemberRepository $member_repository, IGroupRepository $group_repository, ISummitSponsorService $summit_sponsor_service, + IMemberService $member_service, ITransactionService $tx_service ) { @@ -63,6 +69,28 @@ public function __construct $this->member_repository = $member_repository; $this->group_repository = $group_repository; $this->summit_sponsor_service = $summit_sponsor_service; + $this->member_service = $member_service; + } + + /** + * Resolves the local Member for an IDP user id, registering it on demand + * from the IDP when it was never synced (brand-new user that has not + * logged in yet). Throws EntityNotFoundException when the user does not + * exist at the IDP either. + * + * @param int $user_id external (IDP) user id + * @return Member + * @throws EntityNotFoundException + */ + private function resolveMember(int $user_id): Member + { + $member = $this->member_repository->getByExternalId($user_id); + if (!is_null($member)) return $member; + + Log::warning( + "SponsorUserSyncService::resolveMember member with external id {$user_id} not found locally - registering on demand from IDP"); + + return $this->member_service->registerExternalUserById($user_id); } /** @@ -78,10 +106,7 @@ public function validateParams(int $summit_id, int $user_id): array throw new EntityNotFoundException("Summit {$summit_id} not found"); } - $member = $this->member_repository->getByExternalId($user_id); - if (is_null($member)) { - throw new EntityNotFoundException("Member with id {$user_id} not found"); - } + $member = $this->resolveMember($user_id); return array($summit, $member); } @@ -148,10 +173,7 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo Log::debug( "SponsorUserSyncService::addSponsorUserToGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); - $member = $this->member_repository->getByExternalId($user_id); - if (is_null($member)) { - throw new EntityNotFoundException("Member with id {$user_id} not found"); - } + $member = $this->resolveMember($user_id); // Grant the global group FIRST: Sponsor::addUser (reached through the // eager-create path below) validates the member already belongs to a @@ -208,10 +230,7 @@ public function removeSponsorUserFromGroup(int $user_id, string $group_slug, int Log::debug( "SponsorUserSyncService::removeSponsorUserFromGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); - $member = $this->member_repository->getByExternalId($user_id); - if (is_null($member)) { - throw new EntityNotFoundException("Member with id {$user_id} not found"); - } + $member = $this->resolveMember($user_id); // Remove permission entry from JSON and get remaining sponsor count. $remaining = $member->removeSponsorPermission($sponsor_id, $group_slug); diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index d3ac820a5..a40e4bc63 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -71,6 +71,21 @@ private function getService(): ISponsorUserSyncService return app(ISponsorUserSyncService::class); } + /** + * Replaces the IDP user API with a mock returning $user_data (null = the + * user does not exist at the IDP) and forces the services that may have + * been resolved against the real API to rebuild. + */ + private function mockExternalUserApi(?array $user_data): void + { + $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); + $this->app->forgetInstance(\App\Services\Model\ISponsorUserSyncService::class); + } + /** * Returns the decoded Permissions JSON array for a given (SponsorID, MemberID) * row in Sponsor_Users, or an empty array when the column is NULL. @@ -213,6 +228,9 @@ public function testAddSponsorUserToGroupIsIdempotent(): void */ public function testAddSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void { + // User does not exist locally NOR at the IDP. + $this->mockExternalUserApi(null); + $this->expectException(\models\exceptions\EntityNotFoundException::class); $this->getService()->addSponsorUser( @@ -222,6 +240,60 @@ public function testAddSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void ); } + /** + * Brand-new IDP user whose Member row was never synced to summit-api + * (the user never logged in): the sync must register the member on demand + * from the IDP instead of failing - otherwise both MQ events exhaust their + * retries against a missing member and the access grant is lost for good. + */ + public function testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing(): void + { + $external_id = mt_rand(1500000000, 2000000000); // no local Member row + $sponsor_id = self::$sponsors[1]->getId(); + $summit_id = self::$summit->getId(); + $email = sprintf("smarcet+ondemand_%s@gmail.com", str_random(8)); + + $this->mockExternalUserApi([ + 'id' => $external_id, + 'email' => $email, + 'first_name' => 'On', + 'last_name' => 'Demand', + 'bio' => '', + 'active' => true, + 'email_verified' => true, + 'groups' => [], + 'public_profile_show_photo' => false, + 'public_profile_show_fullname' => false, + 'public_profile_show_email' => false, + 'public_profile_show_telephone_number' => false, + 'public_profile_show_bio' => false, + 'public_profile_show_social_media_info' => false, + 'public_profile_allow_chat_with_me' => false, + ]); + + $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'); + + // ...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(); + } + /** * A swallowed removal failure is worse than a swallowed addition: the user * silently RETAINS access they should have lost. The failure must propagate @@ -229,6 +301,9 @@ public function testAddSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void */ public function testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void { + // User does not exist locally NOR at the IDP. + $this->mockExternalUserApi(null); + $this->expectException(\models\exceptions\EntityNotFoundException::class); $this->getService()->removeSponsorUser( From a2e9227eefc7f9bac68c116143add4dc8f958d54 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 14:35:53 -0300 Subject: [PATCH 04/20] fix: resolve member outside the sponsor group sync transaction 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. --- .../Model/Imp/SponsorUserSyncService.php | 42 +++++++-- .../SponsorUserPermissionTrackingTest.php | 87 +++++++++++++++++++ 2 files changed, 120 insertions(+), 9 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 61e3a0452..73df30c3b 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -169,11 +169,25 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id */ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $sponsor_id, int $summit_id): void { - $this->tx_service->transaction(function () use ($user_id, $group_slug, $sponsor_id, $summit_id) { - Log::debug( - "SponsorUserSyncService::addSponsorUserToGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); - - $member = $this->resolveMember($user_id); + Log::debug( + "SponsorUserSyncService::addSponsorUserToGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); + + // Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below. + // registerExternalUserById opens its own transaction and dispatches NewMember / + // MemberDataUpdatedExternally right after it. Those jobs are pushed immediately: + // afterCommit only defers dispatch for Eloquent-managed transactions, and this + // service uses the Doctrine DBAL connection directly. Keeping the registration + // outside guarantees the Member row is committed before any job references its id. + $member_id = $this->resolveMember($user_id)->getId(); + + $this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) { + + // Re-load inside the transaction: the tx service may have reset the entity + // manager, which would leave an entity resolved outside it detached. + $member = $this->member_repository->getById($member_id); + if (!$member instanceof Member) { + throw new EntityNotFoundException("Member with id {$member_id} not found"); + } // Grant the global group FIRST: Sponsor::addUser (reached through the // eager-create path below) validates the member already belongs to a @@ -226,11 +240,21 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo */ public function removeSponsorUserFromGroup(int $user_id, string $group_slug, int $sponsor_id, int $summit_id): void { - $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}"); + 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); + // See addSponsorUserToGroup: resolve outside the transaction so an on-demand + // registration is committed before the jobs it dispatches reference the member id. + $member_id = $this->resolveMember($user_id)->getId(); + + $this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) { + + // Re-load inside the transaction: the tx service may have reset the entity + // manager, which would leave an entity resolved outside it detached. + $member = $this->member_repository->getById($member_id); + if (!$member instanceof Member) { + throw new EntityNotFoundException("Member with id {$member_id} not found"); + } // Remove permission entry from JSON and get remaining sponsor count. $remaining = $member->removeSponsorPermission($sponsor_id, $group_slug); diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index a40e4bc63..083ae9c4e 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -86,6 +86,22 @@ private function mockExternalUserApi(?array $user_data): void $this->app->forgetInstance(\App\Services\Model\ISponsorUserSyncService::class); } + /** + * A failed tx_service transaction closes the entity manager and resets it in the + * registry (see DoctrineTransactionService::transaction), leaving the static one + * captured at setUp time unusable. Returns a usable manager either way. + */ + private static function reopenEntityManager(): \Doctrine\ORM\EntityManagerInterface + { + if (!self::$em->isOpen()) { + return \LaravelDoctrine\ORM\Facades\Registry::resetManager( + \models\utils\SilverstripeBaseModel::EntityManager + ); + } + self::$em->clear(); + return self::$em; + } + /** * Returns the decoded Permissions JSON array for a given (SponsorID, MemberID) * row in Sponsor_Users, or an empty array when the column is NULL. @@ -294,6 +310,77 @@ public function testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing(): v self::$em->flush(); } + /** + * On-demand registration must happen OUTSIDE addSponsorUserToGroup's transaction. + * + * registerExternalUserById dispatches NewMember / MemberDataUpdatedExternally, whose + * listeners enqueue MemberAssocSummitOrders, UpdateAttendeeInfo and CleanMemberCacheJob. + * Those pushes are NOT deferred to commit: afterCommit only works for transactions that + * Laravel's DatabaseTransactionsManager can see, and this service opens its transaction + * straight on the Doctrine DBAL connection. So if the member were registered inside the + * transaction and the transaction later rolled back, the Member row would vanish while + * the already-queued jobs kept pointing at its id - they would fail forever. + * + * Here the group slug does not exist, so the transaction throws AFTER the member was + * resolved. The member must still be present afterwards. + */ + public function testAddSponsorUserToGroupKeepsOnDemandMemberWhenTransactionFails(): void + { + $external_id = mt_rand(1500000000, 2000000000); // no local Member row + $sponsor_id = self::$sponsors[1]->getId(); + $summit_id = self::$summit->getId(); + $email = sprintf("smarcet+rollback_%s@gmail.com", str_random(8)); + + $this->mockExternalUserApi([ + 'id' => $external_id, + 'email' => $email, + 'first_name' => 'Roll', + 'last_name' => 'Back', + 'bio' => '', + 'active' => true, + 'email_verified' => true, + 'groups' => [], + 'public_profile_show_photo' => false, + 'public_profile_show_fullname' => false, + 'public_profile_show_email' => false, + 'public_profile_show_telephone_number' => false, + 'public_profile_show_bio' => false, + 'public_profile_show_social_media_info' => false, + 'public_profile_allow_chat_with_me' => false, + ]); + + try { + $thrown = null; + try { + $this->getService()->addSponsorUserToGroup( + $external_id, + 'non-existent-group-slug-' . str_random(8), // makes the transaction throw + $sponsor_id, + $summit_id + ); + } catch (\models\exceptions\EntityNotFoundException $ex) { + $thrown = $ex; + } + + $this->assertNotNull($thrown, 'The unknown group slug should have failed the transaction'); + + // The on-demand member was committed by its own transaction, so the jobs + // already dispatched for it reference a row that exists. + self::$em = self::reopenEntityManager(); + $this->assertNotNull( + self::$member_repository->getByExternalId($external_id), + 'On-demand member must survive the rolled back outer transaction' + ); + } finally { + self::$em = self::reopenEntityManager(); + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + } + } + /** * A swallowed removal failure is worse than a swallowed addition: the user * silently RETAINS access they should have lost. The failure must propagate From 67fce36df98d0184416c9e260f1d70121037adef Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 15:27:50 -0300 Subject: [PATCH 05/20] fix: scope removeSponsorUser membership loop to the event summit 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. --- .../Model/Imp/SponsorUserSyncService.php | 12 ++- .../SponsorUserPermissionTrackingTest.php | 73 +++++++++++++++++++ 2 files changed, 82 insertions(+), 3 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 73df30c3b..2a304c0d4 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -149,12 +149,18 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id "SponsorUserSyncService::removeSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); if (is_null($sponsor_id)) { + // getSponsorMemberships() is a plain ManyToMany to Sponsor with no summit + // scoping, so it also yields sponsors of OTHER summits. Those do not resolve + // against $summit and would make removeSponsorUser throw "Sponsor not found.", + // aborting the loop and leaving this summit's own memberships un-revoked. foreach ($member->getSponsorMemberships() as $sponsor_membership) { - $sponsor_id = $sponsor_membership->getId(); - $this->summit_sponsor_service->removeSponsorUser($summit, $sponsor_id, $member->getId()); + if ($sponsor_membership->getSummit()->getId() !== $summit->getId()) continue; + + $current_sponsor_id = $sponsor_membership->getId(); + $this->summit_sponsor_service->removeSponsorUser($summit, $current_sponsor_id, $member->getId()); Log::info( - "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from summit {$summit->getId()} for sponsor {$sponsor_id}" + "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from summit {$summit->getId()} for sponsor {$current_sponsor_id}" ); } } else { diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 083ae9c4e..01fece6ab 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -120,6 +120,18 @@ private function getPermissions(int $sponsor_id, int $member_id): array return json_decode($raw, true) ?? []; } + /** + * Whether a Sponsor_Users row exists for the given (SponsorID, MemberID) pair, + * regardless of its Permissions value. + */ + private function hasSponsorUserRow(int $sponsor_id, int $member_id): bool + { + return (bool)self::$em->getConnection()->executeQuery( + 'SELECT 1 FROM Sponsor_Users WHERE SponsorID = ? AND MemberID = ?', + [$sponsor_id, $member_id] + )->fetchOne(); + } + // ------------------------------------------------------------------------- // addSponsorUserToGroup // ------------------------------------------------------------------------- @@ -381,6 +393,67 @@ public function testAddSponsorUserToGroupKeepsOnDemandMemberWhenTransactionFails } } + /** + * removeSponsorUser with a null sponsor_id (the auth_user_removed_from_summit + * event, which carries no sponsor_id) must revoke ONLY the memberships that + * belong to the summit the event is about. + * + * Member::getSponsorMemberships() is a plain ManyToMany to Sponsor with no + * summit scoping, so it also returns sponsors of other summits. Those do not + * resolve against $summit and make SummitSponsorService::removeSponsorUser + * throw "Sponsor not found.", aborting the loop and leaving the memberships + * of the event's own summit un-revoked. + */ + public function testRemoveSponsorUserWithoutSponsorIdOnlyRevokesTheEventSummit(): void + { + // 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(); + + $member_id = $member->getId(); + $external_id = $member->getUserExternalId(); + $event_sponsor_id = self::$sponsors[0]->getId(); // belongs to self::$summit + $other_sponsor_id = $other_sponsor->getId(); // belongs to self::$summit2 + + // Pre-condition: the member holds a membership in BOTH summits. + $this->assertTrue($this->hasSponsorUserRow($event_sponsor_id, $member_id)); + $this->assertTrue($this->hasSponsorUserRow($other_sponsor_id, $member_id)); + + try { + // auth_user_removed_from_summit: no sponsor_id, only the summit. + $this->getService()->removeSponsorUser( + self::$summit->getId(), + $external_id, + null + ); + + self::$em->clear(); + + $this->assertFalse( + $this->hasSponsorUserRow($event_sponsor_id, $member_id), + 'the membership of the event summit must have been revoked' + ); + $this->assertTrue( + $this->hasSponsorUserRow($other_sponsor_id, $member_id), + 'the membership of an unrelated summit must be left untouched' + ); + } 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]); + } + } + /** * A swallowed removal failure is worse than a swallowed addition: the user * silently RETAINS access they should have lost. The failure must propagate From 9b49d4a4b14f927f771e6dcc5a592b3211744f52 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 15:40:23 -0300 Subject: [PATCH 06/20] fix: do not provision members from the IDP on revocation events 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. --- .../Model/Imp/SponsorUserSyncService.php | 58 ++++++-- .../SponsorUserPermissionTrackingTest.php | 129 +++++++++++++++++- 2 files changed, 172 insertions(+), 15 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 2a304c0d4..7e54d8038 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -93,21 +93,44 @@ private function resolveMember(int $user_id): Member return $this->member_service->registerExternalUserById($user_id); } + /** + * Looks up the local Member WITHOUT registering it on demand. Revocation + * paths use this: a member that was never synced owns no Sponsor_Users row + * and no group membership, so there is nothing to revoke and provisioning + * one from the IDP would be a pure side effect (Member row, full + * synchronizeGroups, NewMember / MemberDataUpdatedExternally jobs). + * + * @param int $user_id external (IDP) user id + * @return Member|null null when the member was never synced + */ + private function findMember(int $user_id): ?Member + { + return $this->member_repository->getByExternalId($user_id); + } + /** * @param int $summit_id - * @param int $user_id - * @return array + * @return Summit * @throws EntityNotFoundException */ - public function validateParams(int $summit_id, int $user_id): array + private function resolveSummit(int $summit_id): Summit { $summit = $this->summit_repository->getById($summit_id); if (!$summit instanceof Summit) { throw new EntityNotFoundException("Summit {$summit_id} not found"); } + return $summit; + } - $member = $this->resolveMember($user_id); - return array($summit, $member); + /** + * @param int $summit_id + * @param int $user_id + * @return array + * @throws EntityNotFoundException + */ + public function validateParams(int $summit_id, int $user_id): array + { + return array($this->resolveSummit($summit_id), $this->resolveMember($user_id)); } /** @@ -143,7 +166,18 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id Log::debug( "SponsorUserSyncService::removeSponsorUser summit {$summit_id} sponsor {$sponsor_id} user_id {$user_id}"); - list($summit, $member) = $this->validateParams($summit_id, $user_id); + $summit = $this->resolveSummit($summit_id); + + // Revocation must not provision (see findMember). Skipping is also what + // keeps a deleted IDP user from parking an unresolvable entry in + // failed_jobs: sponsor-users-api emits a removal event per access right + // when a user is deleted, and by then the local Member is gone too. + $member = $this->findMember($user_id); + if (is_null($member)) { + Log::warning( + "SponsorUserSyncService::removeSponsorUser member with external id {$user_id} was never synced - nothing to revoke, skipping"); + return; + } Log::debug( "SponsorUserSyncService::removeSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); @@ -249,9 +283,15 @@ public function removeSponsorUserFromGroup(int $user_id, string $group_slug, int Log::debug( "SponsorUserSyncService::removeSponsorUserFromGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); - // See addSponsorUserToGroup: resolve outside the transaction so an on-demand - // registration is committed before the jobs it dispatches reference the member id. - $member_id = $this->resolveMember($user_id)->getId(); + // Revocation must not provision (see findMember): a member that was never + // synced holds no permission entry and no group membership to remove. + $member = $this->findMember($user_id); + if (is_null($member)) { + Log::warning( + "SponsorUserSyncService::removeSponsorUserFromGroup member with external id {$user_id} was never synced - nothing to revoke, skipping"); + return; + } + $member_id = $member->getId(); $this->tx_service->transaction(function () use ($member_id, $group_slug, $sponsor_id, $summit_id) { diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 01fece6ab..ab56c4096 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -455,22 +455,139 @@ public function testRemoveSponsorUserWithoutSponsorIdOnlyRevokesTheEventSummit() } /** - * A swallowed removal failure is worse than a swallowed addition: the user - * silently RETAINS access they should have lost. The failure must propagate - * so the MQ job's retry / failed_jobs machinery applies. + * A member that was never synced holds no Sponsor_Users row and no group + * membership, so a revocation event for it has nothing to revoke: it is a + * no-op, not a failure. Turning it into an exception would burn the job's + * 3 tries and park a permanently unresolvable entry in failed_jobs. */ - public function testRemoveSponsorUserPropagatesErrorWhenMemberDoesNotExist(): void + public function testRemoveSponsorUserIsNoOpWhenMemberWasNeverSynced(): void { // User does not exist locally NOR at the IDP. $this->mockExternalUserApi(null); - $this->expectException(\models\exceptions\EntityNotFoundException::class); - $this->getService()->removeSponsorUser( self::$summit->getId(), PHP_INT_MAX, // external user id with no matching Member row self::$sponsors[0]->getId() ); + + $this->assertNull(self::$member_repository->getByExternalId(PHP_INT_MAX)); + } + + /** + * Revocation must never PROVISION. When the member was never synced but the + * user does still exist at the IDP, registering it on demand would create a + * Member row, run a full synchronizeGroups and dispatch NewMember / + * MemberDataUpdatedExternally jobs - all to then revoke nothing, since a + * member that did not exist owns no Sponsor_Users row. + */ + public function testRemoveSponsorUserDoesNotProvisionMemberFromIdp(): void + { + $external_id = mt_rand(1500000000, 2000000000); // no local Member row + $email = sprintf("smarcet+norevokeprov_%s@gmail.com", str_random(8)); + + // The user DOES exist at the IDP - on-demand registration would succeed. + $this->mockExternalUserApi([ + 'id' => $external_id, + 'email' => $email, + 'first_name' => 'No', + 'last_name' => 'Provision', + 'bio' => '', + 'active' => true, + 'email_verified' => true, + 'groups' => [], + 'public_profile_show_photo' => false, + 'public_profile_show_fullname' => false, + 'public_profile_show_email' => false, + 'public_profile_show_telephone_number' => false, + 'public_profile_show_bio' => false, + 'public_profile_show_social_media_info' => false, + 'public_profile_allow_chat_with_me' => false, + ]); + + try { + $this->getService()->removeSponsorUser( + self::$summit->getId(), + $external_id, + self::$sponsors[0]->getId() + ); + + self::$em->clear(); + $this->assertNull( + self::$member_repository->getByExternalId($external_id), + 'a revocation event must not create a Member row' + ); + } finally { + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + } + } + + /** + * Same contract for the group-scoped revocation entry point. + */ + public function testRemoveSponsorUserFromGroupDoesNotProvisionMemberFromIdp(): void + { + $external_id = mt_rand(1500000000, 2000000000); // no local Member row + $email = sprintf("smarcet+norevokegrp_%s@gmail.com", str_random(8)); + + $this->mockExternalUserApi([ + 'id' => $external_id, + 'email' => $email, + 'first_name' => 'No', + 'last_name' => 'Provision', + 'bio' => '', + 'active' => true, + 'email_verified' => true, + 'groups' => [], + 'public_profile_show_photo' => false, + 'public_profile_show_fullname' => false, + 'public_profile_show_email' => false, + 'public_profile_show_telephone_number' => false, + 'public_profile_show_bio' => false, + 'public_profile_show_social_media_info' => false, + 'public_profile_allow_chat_with_me' => false, + ]); + + try { + $this->getService()->removeSponsorUserFromGroup( + $external_id, + IGroup::Sponsors, + self::$sponsors[0]->getId(), + self::$summit->getId() + ); + + self::$em->clear(); + $this->assertNull( + self::$member_repository->getByExternalId($external_id), + 'a group revocation event must not create a Member row' + ); + } finally { + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + } + } + + /** + * Skipping an unknown member must not turn removeSponsorUser into a + * swallow-everything handler: a genuine failure still has to propagate so + * the MQ job's retry / failed_jobs machinery applies. + */ + public function testRemoveSponsorUserPropagatesErrorWhenSummitDoesNotExist(): void + { + $this->expectException(\models\exceptions\EntityNotFoundException::class); + + $this->getService()->removeSponsorUser( + PHP_INT_MAX, // no such summit + self::$member->getUserExternalId(), + self::$sponsors[0]->getId() + ); } // ------------------------------------------------------------------------- From 8dbc1d2d0c88442776f1ef27515e02824391e44b Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 16:02:35 -0300 Subject: [PATCH 07/20] fix: refresh stale member groups from the IDP and make MQ retries real 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. --- .../SponsorServices/SponsorServicesMQJob.php | 21 +++- .../Model/Imp/SponsorUserSyncService.php | 34 ++++++ .../Jobs/SponsorServicesMQJobRetryTest.php | 101 ++++++++++++++++++ .../SponsorUserPermissionTrackingTest.php | 58 ++++++++++ 4 files changed, 213 insertions(+), 1 deletion(-) create mode 100644 tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php diff --git a/app/Jobs/SponsorServices/SponsorServicesMQJob.php b/app/Jobs/SponsorServices/SponsorServicesMQJob.php index b7fd8b8f9..a8ae4d830 100644 --- a/app/Jobs/SponsorServices/SponsorServicesMQJob.php +++ b/app/Jobs/SponsorServices/SponsorServicesMQJob.php @@ -20,9 +20,26 @@ class SponsorServicesMQJob extends BaseJob { public int $tries = 3; + /** + * Seconds to wait before the 2nd and 3rd attempt, as the comma-separated + * string Worker::calculateBackoff() explodes. + * + * These events race the IDP's own user-updated event, which is what refreshes + * a member's groups locally. Retrying immediately loses that race every time; + * for auth_user_added_to_sponsor_and_summit that is terminal, because no + * companion group event follows to repair the state. + */ + const RetryBackoff = '30,120'; + /** * Get the decoded body of the job. * + * Note the maxTries/backoff keys: Job::maxTries() and Job::backoff() read the + * PAYLOAD, not the properties of the handler class this payload names. Without + * them the worker falls back to the command's own options, and the entry point + * runs `doctrine:queue:work sponsor_users_sync_consumer` with no flags - so the + * defaults of --tries=1 and --backoff=0 apply and a single failure is final. + * * @return array */ public function payload(): array @@ -47,7 +64,9 @@ public function payload(): array } return [ 'job' => $job, - 'data' => json_decode($this->getRawBody(), true) + 'data' => json_decode($this->getRawBody(), true), + 'maxTries' => $this->tries, + 'backoff' => self::RetryBackoff, ]; } } diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 7e54d8038..f4f906231 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -23,6 +23,7 @@ use models\main\IMemberRepository; use models\main\Member; use models\summit\ISummitRepository; +use models\summit\Sponsor; use models\summit\Summit; use models\utils\SilverstripeBaseModel; use services\model\ISummitSponsorService; @@ -108,6 +109,37 @@ private function findMember(int $user_id): ?Member return $this->member_repository->getByExternalId($user_id); } + /** + * Sponsor::addUser rejects a member that belongs to none of its + * AllowedMemberGroups. sponsor-users-api grants that group at the IDP before + * publishing the membership event (_sync_user_groups), but summit-api only + * learns about it through the IDP's own user-updated event, which races this + * one. resolveMember covers the member that does not exist yet - this covers + * the member that exists with a stale local group set: re-read it from the + * IDP, which is the source of truth, instead of failing. + * + * Nothing downstream would repair that failure: the producers of + * auth_user_added_to_sponsor_and_summit (_import_user, _notify_approval) emit + * no companion group event, so no eager-create path ever runs and the access + * is lost once the job exhausts its tries. + * + * @param Member $member + * @param int $user_id external (IDP) user id + * @return Member the same member, or the refreshed one + * @throws \Exception + */ + private function ensureSponsorGroupMembership(Member $member, int $user_id): Member + { + foreach (Sponsor::AllowedMemberGroups as $group_slug) { + if ($member->belongsToGroup($group_slug)) return $member; + } + + Log::warning( + "SponsorUserSyncService::ensureSponsorGroupMembership member {$member->getId()} belongs to none of the allowed sponsor groups - refreshing groups from the IDP"); + + return $this->member_service->registerExternalUserById($user_id); + } + /** * @param int $summit_id * @return Summit @@ -149,6 +181,8 @@ public function addSponsorUser(int $summit_id, int $sponsor_id, int $user_id): v Log::debug( "SponsorUserSyncService::addSponsorUser summit {$summit->getName()} member {$member->getEmail()}"); + $member = $this->ensureSponsorGroupMembership($member, $user_id); + $this->summit_sponsor_service->addSponsorUser($summit, $sponsor_id, $member->getId()); Log::info( diff --git a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php new file mode 100644 index 000000000..f190693b7 --- /dev/null +++ b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php @@ -0,0 +1,101 @@ +setDeliveryInfo(1, false, 'sponsor_users', $routing_key); + + $job = Mockery::mock(SponsorServicesMQJob::class)->makePartial(); + $job->shouldReceive('getRabbitMQMessage')->andReturn($message); + $job->shouldReceive('getRawBody')->andReturn(json_encode([ + 'data' => [ + 'user_external_id' => 1, + 'sponsor_id' => 2, + 'summit_id' => 3, + 'group_slug' => 'sponsors', + ], + ])); + + return $job; + } + + public static function routingKeyProvider(): array + { + return [ + 'added to sponsor and summit' => [EventTypes::AUTH_USER_ADDED_TO_SPONSOR_AND_SUMMIT], + 'added to group' => [EventTypes::AUTH_USER_ADDED_TO_GROUP], + 'removed from group' => [EventTypes::AUTH_USER_REMOVED_FROM_GROUP], + 'removed from summit' => [EventTypes::AUTH_USER_REMOVED_FROM_SUMMIT], + ]; + } + + #[DataProvider('routingKeyProvider')] + public function testWorkerSeesMoreThanASingleAttempt(string $routing_key): void + { + $this->assertSame(3, $this->jobForRoutingKey($routing_key)->maxTries()); + } + + #[DataProvider('routingKeyProvider')] + public function testWorkerSeesAnIncreasingBackoff(string $routing_key): void + { + $backoff = $this->jobForRoutingKey($routing_key)->backoff(); + + // Worker::calculateBackoff() explodes this on commas and indexes it by attempt. + $delays = array_map('intval', explode(',', (string)$backoff)); + + $this->assertCount(2, $delays, 'one delay per retry after the first attempt'); + $this->assertGreaterThan(0, $delays[0], 'the first retry must not be immediate'); + $this->assertGreaterThan($delays[0], $delays[1], 'the backoff must grow'); + } + + /** + * An unknown routing key returns an empty payload and must not claim a retry + * policy - there is no handler to retry. + */ + public function testUnknownRoutingKeyCarriesNoRetryPolicy(): void + { + $job = $this->jobForRoutingKey('some_unknown_routing_key'); + + $this->assertNull($job->maxTries()); + $this->assertNull($job->backoff()); + } +} diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index ab56c4096..a34b849c4 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -454,6 +454,64 @@ public function testRemoveSponsorUserWithoutSponsorIdOnlyRevokesTheEventSummit() } } + /** + * The membership event (auth_user_added_to_sponsor_and_summit) is emitted by + * sponsor-users-api WITHOUT any companion group event - see _import_user and + * _notify_approval, neither of which publishes auth_user_added_to_group. So + * nothing downstream repairs a failure here: if Sponsor::addUser rejects the + * member for not belonging to an allowed sponsor group, the Sponsor_Users row + * is never created and the access is lost for good. + * + * The producer does add the group at the IDP before publishing, so the IDP is + * already correct; it is the local copy that is stale. A member that does not + * exist yet is covered by resolveMember's on-demand registration - this covers + * the member that DOES exist locally with outdated groups. + */ + public function testAddSponsorUserRefreshesStaleGroupsFromIdp(): void + { + // member2 belongs only to SummitAdministrators - not an allowed sponsor group. + $member_id = self::$member2->getId(); + $external_id = self::$member2->getUserExternalId(); + $sponsor_id = self::$sponsors[1]->getId(); // no Sponsor_Users row yet + + $this->assertFalse( + self::$member_repository->find($member_id)->belongsToGroup(IGroup::Sponsors), + 'Pre-condition: member must not belong to an allowed sponsor group' + ); + $this->assertFalse($this->hasSponsorUserRow($sponsor_id, $member_id)); + + // The IDP already carries the group (the producer syncs it before publishing). + $this->mockExternalUserApi([ + 'id' => $external_id, + 'email' => self::$member2->getEmail(), + 'first_name' => self::$member2->getFirstName(), + 'last_name' => self::$member2->getLastName(), + 'bio' => '', + 'active' => true, + 'email_verified' => true, + 'groups' => [IGroup::Sponsors], + 'public_profile_show_photo' => false, + 'public_profile_show_fullname' => false, + 'public_profile_show_email' => false, + 'public_profile_show_telephone_number' => false, + 'public_profile_show_bio' => false, + 'public_profile_show_social_media_info' => false, + 'public_profile_allow_chat_with_me' => false, + ]); + + $this->getService()->addSponsorUser( + self::$summit->getId(), + $sponsor_id, + $external_id + ); + + self::$em->clear(); + $this->assertTrue( + $this->hasSponsorUserRow($sponsor_id, $member_id), + 'the Sponsor_Users row must have been created after refreshing groups from the IDP' + ); + } + /** * A member that was never synced holds no Sponsor_Users row and no group * membership, so a revocation event for it has nothing to revoke: it is a From 0330462831318d1896a82fbedaa29841f353df4d Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 16:14:38 -0300 Subject: [PATCH 08/20] test: clean up the on-demand member in a finally 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. --- .../SponsorUserPermissionTrackingTest.php | 46 +++++++++++-------- 1 file changed, 27 insertions(+), 19 deletions(-) diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index a34b849c4..a161e5b1d 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -299,27 +299,35 @@ public function testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing(): v 'public_profile_allow_chat_with_me' => false, ]); - $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'); + try { + $this->getService()->addSponsorUserToGroup( + $external_id, + IGroup::Sponsors, + $sponsor_id, + $summit_id + ); - // ...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)); + // 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'); - // Cleanup: this member is created outside the trait's tearDown scope. - self::$em->remove($member); - self::$em->flush(); + // ...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)); + } finally { + // This member is created on demand, outside the trait's tearDown scope, + // so it has to be removed here - and in a finally, or a failing assertion + // above leaks it into the next test's database. Look it up again instead + // of reusing $member: the failure may have happened before it was set. + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + } } /** From 690f895f511000170132d32649e7eb10c22d5006 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 16:23:52 -0300 Subject: [PATCH 09/20] fix: correct removeSponsorUser log message 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. --- app/Services/Model/Imp/SponsorUserSyncService.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index f4f906231..b5168dea2 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -234,7 +234,7 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id } else { $this->summit_sponsor_service->removeSponsorUser($summit, $sponsor_id, $member->getId()); Log::info( - "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from to summit {$summit_id} for sponsor {$sponsor_id}"); + "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from summit {$summit->getId()} for sponsor {$sponsor_id}"); } } From 9e6fc4309dc4016c9a751bc26eb95043937b4fee Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 17:26:07 -0300 Subject: [PATCH 10/20] fix: make MQ retries actually redeliver by dead-lettering through the 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. --- .../RemoveSponsorMemberMQJob.php | 4 +- .../SponsorServices/SponsorServicesMQJob.php | 88 ++++++++++- .../UpdateSponsorMemberGroupsMQJob.php | 4 +- .../Jobs/SponsorServicesMQJobRetryTest.php | 139 +++++++++++++++++- 4 files changed, 229 insertions(+), 6 deletions(-) diff --git a/app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.php b/app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.php index 6fab40201..824041af9 100644 --- a/app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.php +++ b/app/Jobs/SponsorServices/RemoveSponsorMemberMQJob.php @@ -54,7 +54,9 @@ public function __construct(ISponsorUserSyncService $service) public function handle(SponsorServicesMQJob $job): void { try { - $event_type = $job->getRabbitMQMessage()->getRoutingKey(); + // getEventType, not the raw routing key: a redelivered (released) + // message carries the queue name as routing key. + $event_type = $job->getEventType(); $payload = $job->payload(); $json = json_encode($payload); Log::debug("RemoveSponsorMemberMQJob::handle payload {$json}"); diff --git a/app/Jobs/SponsorServices/SponsorServicesMQJob.php b/app/Jobs/SponsorServices/SponsorServicesMQJob.php index a8ae4d830..dac2b2552 100644 --- a/app/Jobs/SponsorServices/SponsorServicesMQJob.php +++ b/app/Jobs/SponsorServices/SponsorServicesMQJob.php @@ -31,6 +31,41 @@ class SponsorServicesMQJob extends BaseJob */ const RetryBackoff = '30,120'; + /** + * Body key that carries the original event type across a release(): a + * released message is dead-lettered back through the default exchange, so + * it is redelivered with the QUEUE NAME as its routing key and the event + * type would otherwise be lost (see release()). + */ + const EventTypeKey = 'x_event_type'; + + private const KnownEventTypes = [ + EventTypes::AUTH_USER_ADDED_TO_GROUP, + EventTypes::AUTH_USER_REMOVED_FROM_GROUP, + EventTypes::AUTH_USER_ADDED_TO_SPONSOR_AND_SUMMIT, + EventTypes::AUTH_USER_REMOVED_FROM_SPONSOR_AND_SUMMIT, + EventTypes::AUTH_USER_REMOVED_FROM_SUMMIT, + ]; + + /** + * The event type driving handler selection. On a first delivery it is the + * message's routing key; on a redelivery after release() the routing key is + * the queue name and the original event type travels in the body. Handlers + * that branch on the event type must use this, never the raw routing key. + * + * @return string + */ + public function getEventType(): string + { + $routing_key = $this->getRabbitMQMessage()->getRoutingKey(); + if (in_array($routing_key, self::KnownEventTypes, true)) { + return $routing_key; + } + + $body = json_decode($this->getRawBody(), true); + return $body[self::EventTypeKey] ?? $routing_key; + } + /** * Get the decoded body of the job. * @@ -44,7 +79,7 @@ class SponsorServicesMQJob extends BaseJob */ public function payload(): array { - $routing_key = $this->getRabbitMQMessage()->getRoutingKey(); + $routing_key = $this->getEventType(); switch ($routing_key) { case EventTypes::AUTH_USER_ADDED_TO_GROUP: @@ -69,4 +104,55 @@ public function payload(): array 'backoff' => self::RetryBackoff, ]; } + + /** + * Release the job back into the queue for a retry. + * + * The library implementation publishes into a delay queue whose dead-letter + * exchange is the consumer exchange (sponsor-users-api-message-broker) and + * whose dead-letter routing key is this queue's NAME. That exchange is + * direct and only binds the five auth_user_* routing keys, so the expired + * retry is unroutable and RabbitMQ silently drops it - the retry policy + * would lose every failed event instead of retrying it. + * + * Dead-letter through the DEFAULT exchange instead: it routes by queue name + * with no binding required. Redelivery rewrites the routing key to the queue + * name, so the original event type is preserved in the body (EventTypeKey) + * for getEventType() to recover. + * + * @param int $delay + */ + public function release($delay = 0): void + { + $this->released = true; + + $ttl = $this->secondsUntil($delay) * 1000; + if ($ttl <= 0) { + // laterRaw's ttl<=0 path publishes straight to the consumer exchange + // with the queue name as routing key - unroutable (see above). Force + // the minimum delay so the delay-queue path is always taken. + $delay = 1; + $ttl = 1000; + } + + // Declare the delay queue FIRST: RabbitMQQueue::laterRaw() skips + // re-declaring a queue already in its declared-names cache, so these + // arguments win over the library defaults. + $this->rabbitmq->declareQueue($this->queue . '.delay.' . $ttl, true, false, [ + 'x-dead-letter-exchange' => '', + 'x-dead-letter-routing-key' => $this->queue, + 'x-message-ttl' => $ttl, + 'x-expires' => $ttl * 2, + ]); + + // Preserve the original event type across the redelivery (idempotent: + // a second release keeps the value written by the first one). + $body = json_decode($this->getRawBody(), true) ?? []; + $body[self::EventTypeKey] = $body[self::EventTypeKey] ?? $this->getEventType(); + + $this->rabbitmq->laterRaw($delay, json_encode($body), $this->queue, $this->attempts()); + + // The retry was republished as a new message; ack the current one. + $this->rabbitmq->ack($this); + } } diff --git a/app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php b/app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php index 2442aa732..482d816dc 100644 --- a/app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php +++ b/app/Jobs/SponsorServices/UpdateSponsorMemberGroupsMQJob.php @@ -54,7 +54,9 @@ public function __construct(ISponsorUserSyncService $service) public function handle(SponsorServicesMQJob $job): void { try { - $event_type = $job->getRabbitMQMessage()->getRoutingKey(); + // getEventType, not the raw routing key: a redelivered (released) + // message carries the queue name as routing key. + $event_type = $job->getEventType(); $payload = $job->payload(); $json = json_encode($payload); Log::debug("UpdateSponsorMemberGroupsMQJob::handle payload {$json}"); diff --git a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php index f190693b7..0d411e79d 100644 --- a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php +++ b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php @@ -18,6 +18,7 @@ use PhpAmqpLib\Message\AMQPMessage; use PHPUnit\Framework\Attributes\DataProvider; use Tests\TestCase; +use VladimirYuldashev\LaravelQueueRabbitMQ\Queue\RabbitMQQueue; /** * The queue worker reads the retry policy from Job::maxTries() and @@ -39,21 +40,21 @@ protected function tearDown(): void parent::tearDown(); } - private function jobForRoutingKey(string $routing_key): SponsorServicesMQJob + private function jobForRoutingKey(string $routing_key, array $extra_body = []): SponsorServicesMQJob { $message = new AMQPMessage(''); $message->setDeliveryInfo(1, false, 'sponsor_users', $routing_key); $job = Mockery::mock(SponsorServicesMQJob::class)->makePartial(); $job->shouldReceive('getRabbitMQMessage')->andReturn($message); - $job->shouldReceive('getRawBody')->andReturn(json_encode([ + $job->shouldReceive('getRawBody')->andReturn(json_encode(array_merge([ 'data' => [ 'user_external_id' => 1, 'sponsor_id' => 2, 'summit_id' => 3, 'group_slug' => 'sponsors', ], - ])); + ], $extra_body))); return $job; } @@ -98,4 +99,136 @@ public function testUnknownRoutingKeyCarriesNoRetryPolicy(): void $this->assertNull($job->maxTries()); $this->assertNull($job->backoff()); } + + // ------------------------------------------------------------------------- + // Redelivery after release() + // ------------------------------------------------------------------------- + + public static function redeliveredEventProvider(): array + { + return [ + 'added to sponsor and summit' => [EventTypes::AUTH_USER_ADDED_TO_SPONSOR_AND_SUMMIT, 'App\Jobs\SponsorServices\AddSponsorMemberMQJob@handle'], + 'added to group' => [EventTypes::AUTH_USER_ADDED_TO_GROUP, 'App\Jobs\SponsorServices\UpdateSponsorMemberGroupsMQJob@handle'], + 'removed from group' => [EventTypes::AUTH_USER_REMOVED_FROM_GROUP, 'App\Jobs\SponsorServices\UpdateSponsorMemberGroupsMQJob@handle'], + 'removed from summit' => [EventTypes::AUTH_USER_REMOVED_FROM_SUMMIT, 'App\Jobs\SponsorServices\RemoveSponsorMemberMQJob@handle'], + 'removed from sponsor + summit' => [EventTypes::AUTH_USER_REMOVED_FROM_SPONSOR_AND_SUMMIT, 'App\Jobs\SponsorServices\RemoveSponsorMemberMQJob@handle'], + ]; + } + + /** + * A released message is dead-lettered back through the DEFAULT exchange, so + * it is redelivered with the QUEUE NAME as its routing key - the original + * event type only survives inside the body (release() puts it there). The + * job must still resolve the right handler and keep its retry policy. + */ + #[DataProvider('redeliveredEventProvider')] + public function testRedeliveredMessageResolvesOriginalEventType(string $event_type, string $expected_handler): void + { + $job = $this->jobForRoutingKey( + 'sponsor-users-api-summit-api-badge-scans-queue', // != any EventTypes constant + [SponsorServicesMQJob::EventTypeKey => $event_type] + ); + + $this->assertSame($event_type, $job->getEventType()); + $this->assertSame($expected_handler, $job->payload()['job'] ?? null); + $this->assertSame(3, $job->maxTries()); + $this->assertNotNull($job->backoff()); + } + + /** + * release() must NOT use the library's delay-queue arguments: those + * dead-letter into the consumer exchange (direct, only the five auth_user_* + * bindings) with the queue name as routing key - unroutable, silently + * dropped. It must dead-letter through the DEFAULT exchange (routes by + * queue name, no binding needed) and preserve the original event type in + * the republished body, because redelivery overwrites the routing key. + */ + public function testReleaseDeadLettersBackThroughTheDefaultExchange(): void + { + $queue_name = 'sponsor-users-api-summit-api-badge-scans-queue'; + $original_body = ['user_external_id' => 1, 'sponsor_id' => 2, 'summit_id' => 3]; + + $message = new AMQPMessage(json_encode($original_body)); + $message->setDeliveryInfo(1, false, 'sponsor_users', EventTypes::AUTH_USER_ADDED_TO_SPONSOR_AND_SUMMIT); + + $rabbitmq = Mockery::mock(RabbitMQQueue::class); + + $rabbitmq->shouldReceive('declareQueue')->once()->with( + $queue_name . '.delay.30000', + true, + false, + [ + 'x-dead-letter-exchange' => '', + 'x-dead-letter-routing-key' => $queue_name, + 'x-message-ttl' => 30000, + 'x-expires' => 60000, + ] + ); + + $republished = null; + $rabbitmq->shouldReceive('laterRaw')->once()->with( + 30, + Mockery::on(function ($payload) use (&$republished) { + $republished = $payload; + return is_string($payload); + }), + $queue_name, + 1 // first attempt + ); + + $rabbitmq->shouldReceive('ack')->once(); + + $job = new SponsorServicesMQJob(app(), $rabbitmq, $message, 'rabbitmq', $queue_name); + $job->release(30); + + $this->assertTrue($job->isReleased()); + + $body = json_decode($republished, true); + $this->assertSame( + EventTypes::AUTH_USER_ADDED_TO_SPONSOR_AND_SUMMIT, + $body[SponsorServicesMQJob::EventTypeKey] ?? null, + 'the original event type must survive redelivery in the body' + ); + foreach ($original_body as $key => $value) { + $this->assertSame($value, $body[$key] ?? null, "original payload key {$key} must be preserved"); + } + } + + /** + * A second release (the message already carries the event type from the + * first one) must keep the ORIGINAL event type, not overwrite it with the + * redelivered routing key (the queue name). + */ + public function testReleaseOfARedeliveredMessageKeepsTheOriginalEventType(): void + { + $queue_name = 'sponsor-users-api-summit-api-badge-scans-queue'; + + // As redelivered: routing key already the queue name, event type in body. + $message = new AMQPMessage(json_encode([ + 'user_external_id' => 1, + SponsorServicesMQJob::EventTypeKey => EventTypes::AUTH_USER_ADDED_TO_GROUP, + ])); + $message->setDeliveryInfo(1, false, 'sponsor_users', $queue_name); + + $rabbitmq = Mockery::mock(RabbitMQQueue::class); + $rabbitmq->shouldReceive('declareQueue')->once(); + + $republished = null; + $rabbitmq->shouldReceive('laterRaw')->once()->with( + 120, + Mockery::on(function ($payload) use (&$republished) { + $republished = $payload; + return is_string($payload); + }), + $queue_name, + 1 + ); + $rabbitmq->shouldReceive('ack')->once(); + + $job = new SponsorServicesMQJob(app(), $rabbitmq, $message, 'rabbitmq', $queue_name); + $job->release(120); + + $body = json_decode($republished, true); + $this->assertSame(EventTypes::AUTH_USER_ADDED_TO_GROUP, $body[SponsorServicesMQJob::EventTypeKey] ?? null); + } } From 76d8d831afed221a827552e52d2a6169f91da2c7 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 17:27:53 -0300 Subject: [PATCH 11/20] fix: treat removal events for an already-deleted sponsor as a no-op 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. --- .../Model/Imp/SponsorUserSyncService.php | 10 +++++++ .../SponsorUserPermissionTrackingTest.php | 27 +++++++++++++++++++ 2 files changed, 37 insertions(+) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index b5168dea2..c026ee97c 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -232,6 +232,16 @@ public function removeSponsorUser(int $summit_id, int $user_id, ?int $sponsor_id ); } } else { + // sponsor-users-api's metamodel reconciler emits this event precisely + // when the sponsor no longer exists here (it reaps sponsors summit-api + // stopped returning, routing them through remove_sponsor_show_permissions + // so the domain events still fire). Nothing left to revoke - not a + // failure to burn retries on and park in failed_jobs. + if (is_null($summit->getSummitSponsorById($sponsor_id))) { + Log::warning( + "SponsorUserSyncService::removeSponsorUser sponsor {$sponsor_id} no longer exists on summit {$summit->getId()} - nothing to revoke, skipping"); + return; + } $this->summit_sponsor_service->removeSponsorUser($summit, $sponsor_id, $member->getId()); Log::info( "SponsorUserSyncService::removeSponsorUser: member {$member->getId()} successfully removed from summit {$summit->getId()} for sponsor {$sponsor_id}"); diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index a161e5b1d..64d235a1e 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -640,6 +640,33 @@ public function testRemoveSponsorUserFromGroupDoesNotProvisionMemberFromIdp(): v } } + /** + * sponsor-users-api's metamodel reconciler emits removal events precisely + * when the sponsor no longer exists here (it reaps the sponsors summit-api + * stopped returning, routing them through remove_sponsor_show_permissions + * so the domain events still fire). A missing sponsor is therefore + * nothing-to-revoke - not a failure that burns the job's retries and parks + * a permanently unresolvable entry in failed_jobs. + */ + public function testRemoveSponsorUserIsNoOpWhenSponsorNoLongerExists(): void + { + $member_id = self::$member->getId(); + $sponsor_id = self::$sponsors[0]->getId(); + + // Pre-condition: the member holds a membership on this summit. + $this->assertTrue($this->hasSponsorUserRow($sponsor_id, $member_id)); + + // Removal event pointing at a sponsor that no longer exists on the summit. + $this->getService()->removeSponsorUser( + self::$summit->getId(), + self::$member->getUserExternalId(), + PHP_INT_MAX + ); + + // No exception, and the member's existing membership is untouched. + $this->assertTrue($this->hasSponsorUserRow($sponsor_id, $member_id)); + } + /** * Skipping an unknown member must not turn removeSponsorUser into a * swallow-everything handler: a genuine failure still has to propagate so From a4b0b4ee6f5f2d416090db1d0b2c800186e8b66f Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 17:30:03 -0300 Subject: [PATCH 12/20] fix: refresh stale sponsor groups additively instead of via full IDP 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. --- .../Model/Imp/SponsorUserSyncService.php | 26 ++++++++++++++++--- .../SponsorUserPermissionTrackingTest.php | 14 ++++++++++ 2 files changed, 36 insertions(+), 4 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index c026ee97c..4c1a788a0 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -12,6 +12,7 @@ * limitations under the License. **/ +use App\Services\Apis\IExternalUserApi; use App\Services\Model\AbstractService; use App\Services\Model\IMemberService; use App\Services\Model\ISponsorUserSyncService; @@ -46,6 +47,8 @@ final class SponsorUserSyncService private IMemberService $member_service; + private IExternalUserApi $external_user_api; + /** * SponsorUserSyncService constructor. * @param ISummitRepository $summit_repository @@ -53,6 +56,7 @@ final class SponsorUserSyncService * @param IGroupRepository $group_repository * @param ISummitSponsorService $summit_sponsor_service * @param IMemberService $member_service + * @param IExternalUserApi $external_user_api * @param ITransactionService $tx_service */ public function __construct @@ -62,6 +66,7 @@ public function __construct IGroupRepository $group_repository, ISummitSponsorService $summit_sponsor_service, IMemberService $member_service, + IExternalUserApi $external_user_api, ITransactionService $tx_service ) { @@ -71,6 +76,7 @@ public function __construct $this->group_repository = $group_repository; $this->summit_sponsor_service = $summit_sponsor_service; $this->member_service = $member_service; + $this->external_user_api = $external_user_api; } /** @@ -115,17 +121,23 @@ private function findMember(int $user_id): ?Member * publishing the membership event (_sync_user_groups), but summit-api only * learns about it through the IDP's own user-updated event, which races this * one. resolveMember covers the member that does not exist yet - this covers - * the member that exists with a stale local group set: re-read it from the - * IDP, which is the source of truth, instead of failing. + * the member that exists with a stale local group set: re-read the groups + * from the IDP, which is the source of truth, instead of failing. * * Nothing downstream would repair that failure: the producers of * auth_user_added_to_sponsor_and_summit (_import_user, _notify_approval) emit * no companion group event, so no eager-create path ever runs and the access * is lost once the job exhausts its tries. * + * The sync is ADDITIVE (allow_removals = false) on purpose: this event only + * ever GRANTS access, and removals stay owned by the IDP's own user_updated + * flow (PublishUserUpdated). A full authoritative re-sync here would strip + * locally-held groups absent from the IDP payload as a side effect of a + * sponsor-membership event. + * * @param Member $member * @param int $user_id external (IDP) user id - * @return Member the same member, or the refreshed one + * @return Member the same member, with its groups refreshed when stale * @throws \Exception */ private function ensureSponsorGroupMembership(Member $member, int $user_id): Member @@ -137,7 +149,13 @@ private function ensureSponsorGroupMembership(Member $member, int $user_id): Mem Log::warning( "SponsorUserSyncService::ensureSponsorGroupMembership member {$member->getId()} belongs to none of the allowed sponsor groups - refreshing groups from the IDP"); - return $this->member_service->registerExternalUserById($user_id); + $user_data = $this->external_user_api->getUserById($user_id); + if (is_null($user_data)) { + throw new EntityNotFoundException( + "SponsorUserSyncService::ensureSponsorGroupMembership user {$user_id} does not exist at the IDP"); + } + + return $this->member_service->synchronizeGroups($member, $user_data['groups'] ?? [], false); } /** diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 64d235a1e..c852ab2e8 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -518,6 +518,20 @@ public function testAddSponsorUserRefreshesStaleGroupsFromIdp(): void $this->hasSponsorUserRow($sponsor_id, $member_id), 'the Sponsor_Users row must have been created after refreshing groups from the IDP' ); + + $member = self::$member_repository->find($member_id); + $this->assertTrue( + $member->belongsToGroup(IGroup::Sponsors), + 'the sponsor group must have been refreshed from the IDP' + ); + // The refresh must be ADDITIVE: this event only ever grants access, and + // removals stay owned by the IDP's own user_updated flow. A full + // authoritative re-sync here would strip locally-held groups absent + // from the IDP payload (like this one) as a side effect. + $this->assertTrue( + $member->belongsToGroup(IGroup::SummitAdministrators), + 'unrelated local groups must not be stripped by the refresh' + ); } /** From 61769562d14bfcfad6267e53893ef3163b1af7a7 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 17:33:09 -0300 Subject: [PATCH 13/20] fix: restrict sponsor group sync to Sponsor::AllowedMemberGroups 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. --- .../Model/Imp/SponsorUserSyncService.php | 29 ++++++++++++ .../SponsorUserPermissionTrackingTest.php | 47 ++++++++++++++++--- 2 files changed, 70 insertions(+), 6 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 4c1a788a0..47edee66c 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -20,6 +20,7 @@ use LaravelDoctrine\ORM\Facades\Registry; use libs\utils\ITransactionService; use models\exceptions\EntityNotFoundException; +use models\exceptions\ValidationException; use models\main\IGroupRepository; use models\main\IMemberRepository; use models\main\Member; @@ -158,6 +159,28 @@ private function ensureSponsorGroupMembership(Member $member, int $user_id): Mem return $this->member_service->synchronizeGroups($member, $user_data['groups'] ?? [], false); } + /** + * The MQ payload's group_slug is producer-controlled input arriving over a + * broker vhost several services can write to: without this gate a forged or + * buggy auth_user_added_to_group / auth_user_removed_from_group event could + * grant - or strip - membership of an arbitrary group like administrators. + * + * @param string $group_slug + * @throws ValidationException + */ + private function assertAllowedSponsorGroup(string $group_slug): void + { + if (!in_array($group_slug, Sponsor::AllowedMemberGroups, true)) { + throw new ValidationException( + sprintf( + "Group %s is not an allowed sponsor group (%s).", + $group_slug, + implode(', ', Sponsor::AllowedMemberGroups) + ) + ); + } + } + /** * @param int $summit_id * @return Summit @@ -274,6 +297,10 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo Log::debug( "SponsorUserSyncService::addSponsorUserToGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); + // Gate BEFORE resolveMember: a rejected slug must not provision a member + // from the IDP as a side effect. + $this->assertAllowedSponsorGroup($group_slug); + // Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below. // registerExternalUserById opens its own transaction and dispatches NewMember / // MemberDataUpdatedExternally right after it. Those jobs are pushed immediately: @@ -345,6 +372,8 @@ public function removeSponsorUserFromGroup(int $user_id, string $group_slug, int Log::debug( "SponsorUserSyncService::removeSponsorUserFromGroup user_id {$user_id} group_slug {$group_slug} sponsor_id {$sponsor_id} summit_id {$summit_id}"); + $this->assertAllowedSponsorGroup($group_slug); + // Revocation must not provision (see findMember): a member that was never // synced holds no permission entry and no group membership to remove. $member = $this->findMember($user_id); diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index c852ab2e8..516b24b23 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -204,6 +204,40 @@ public function testAddSponsorUserToGroupCreatesRowWhenMemberHasNoSponsorGroupYe ); } + /** + * The MQ payload's group_slug is attacker/producer-controlled input: the + * shared broker vhost grants write access to several services, so a forged + * or buggy auth_user_added_to_group with e.g. 'administrators' must never + * be granted. Only Sponsor::AllowedMemberGroups may flow through this sync. + */ + public function testAddSponsorUserToGroupRejectsNonSponsorGroup(): void + { + $this->expectException(\models\exceptions\ValidationException::class); + + $this->getService()->addSponsorUserToGroup( + self::$member->getUserExternalId(), + IGroup::Administrators, + self::$sponsors[0]->getId(), + self::$summit->getId() + ); + } + + /** + * Same contract on the removal path: a forged removal event must not be + * able to strip a member from an arbitrary group like 'administrators'. + */ + public function testRemoveSponsorUserFromGroupRejectsNonSponsorGroup(): void + { + $this->expectException(\models\exceptions\ValidationException::class); + + $this->getService()->removeSponsorUserFromGroup( + self::$member->getUserExternalId(), + IGroup::Administrators, + self::$sponsors[0]->getId(), + self::$summit->getId() + ); + } + /** * The group slug must be written into the Sponsor_Users.Permissions JSON * column for the correct (SponsorID, MemberID) row. @@ -341,13 +375,14 @@ public function testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing(): v * transaction and the transaction later rolled back, the Member row would vanish while * the already-queued jobs kept pointing at its id - they would fail forever. * - * Here the group slug does not exist, so the transaction throws AFTER the member was - * resolved. The member must still be present afterwards. + * Here the sponsor does not exist, so the eager-create path throws inside the + * transaction AFTER the member was resolved. The member must still be present + * afterwards. */ public function testAddSponsorUserToGroupKeepsOnDemandMemberWhenTransactionFails(): void { $external_id = mt_rand(1500000000, 2000000000); // no local Member row - $sponsor_id = self::$sponsors[1]->getId(); + $sponsor_id = PHP_INT_MAX; // no such sponsor: eager-create throws inside the tx $summit_id = self::$summit->getId(); $email = sprintf("smarcet+rollback_%s@gmail.com", str_random(8)); @@ -374,15 +409,15 @@ public function testAddSponsorUserToGroupKeepsOnDemandMemberWhenTransactionFails try { $this->getService()->addSponsorUserToGroup( $external_id, - 'non-existent-group-slug-' . str_random(8), // makes the transaction throw - $sponsor_id, + IGroup::Sponsors, + $sponsor_id, // unknown sponsor: the eager-create makes the transaction throw $summit_id ); } catch (\models\exceptions\EntityNotFoundException $ex) { $thrown = $ex; } - $this->assertNotNull($thrown, 'The unknown group slug should have failed the transaction'); + $this->assertNotNull($thrown, 'The unknown sponsor should have failed the transaction'); // The on-demand member was committed by its own transaction, so the jobs // already dispatched for it reference a row that exists. From 7f4d0a5cda33a47c12e51ce4b7a05174c9161a1a Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 17:41:02 -0300 Subject: [PATCH 14/20] fix: publish retries directly to the delay queue instead of via laterRaw 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. --- .../SponsorServices/SponsorServicesMQJob.php | 42 ++-- ...sorServicesMQJobReleaseIntegrationTest.php | 182 ++++++++++++++++++ .../Jobs/SponsorServicesMQJobRetryTest.php | 58 +++--- 3 files changed, 248 insertions(+), 34 deletions(-) create mode 100644 tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php diff --git a/app/Jobs/SponsorServices/SponsorServicesMQJob.php b/app/Jobs/SponsorServices/SponsorServicesMQJob.php index dac2b2552..5a45252b9 100644 --- a/app/Jobs/SponsorServices/SponsorServicesMQJob.php +++ b/app/Jobs/SponsorServices/SponsorServicesMQJob.php @@ -14,6 +14,8 @@ **/ use Illuminate\Support\Facades\Log; +use PhpAmqpLib\Message\AMQPMessage; +use PhpAmqpLib\Wire\AMQPTable; use VladimirYuldashev\LaravelQueueRabbitMQ\Queue\Jobs\RabbitMQJob as BaseJob; class SponsorServicesMQJob extends BaseJob @@ -128,29 +130,47 @@ public function release($delay = 0): void $ttl = $this->secondsUntil($delay) * 1000; if ($ttl <= 0) { - // laterRaw's ttl<=0 path publishes straight to the consumer exchange - // with the queue name as routing key - unroutable (see above). Force - // the minimum delay so the delay-queue path is always taken. - $delay = 1; + // Never skip the delay queue: publishing straight to the consumer + // exchange with the queue name as routing key is unroutable (above). $ttl = 1000; } - - // Declare the delay queue FIRST: RabbitMQQueue::laterRaw() skips - // re-declaring a queue already in its declared-names cache, so these - // arguments win over the library defaults. - $this->rabbitmq->declareQueue($this->queue . '.delay.' . $ttl, true, false, [ + $delay_queue = $this->queue . '.delay.' . $ttl; + + // Declare + publish DIRECTLY on the channel, bypassing laterRaw(): + // - laterRaw() re-declares the delay queue with its own dead-letter + // arguments, which the broker rejects (PRECONDITION_FAILED, + // inequivalent args) once the queue exists with ours; + // - suppressing that re-declare by priming the declared-names cache + // would mean the queue is declared only once per worker process, + // while x-expires DELETES it when idle - every later release would + // then publish into a deleted queue and be dropped silently. + // An unconditional queue_declare per release re-creates the queue when + // it expired and is a no-op (equivalent args) when it did not. + $channel = $this->rabbitmq->getChannel(); + + $channel->queue_declare($delay_queue, false, true, false, false, false, new AMQPTable([ 'x-dead-letter-exchange' => '', 'x-dead-letter-routing-key' => $this->queue, 'x-message-ttl' => $ttl, 'x-expires' => $ttl * 2, - ]); + ])); // Preserve the original event type across the redelivery (idempotent: // a second release keeps the value written by the first one). $body = json_decode($this->getRawBody(), true) ?? []; $body[self::EventTypeKey] = $body[self::EventTypeKey] ?? $this->getEventType(); - $this->rabbitmq->laterRaw($delay, json_encode($body), $this->queue, $this->attempts()); + $channel->basic_publish( + new AMQPMessage(json_encode($body), [ + 'content_type' => 'application/json', + 'delivery_mode' => AMQPMessage::DELIVERY_MODE_PERSISTENT, + 'correlation_id' => uniqid('', true), + // attempts() reads this header on the next delivery. + 'application_headers' => new AMQPTable(['laravel' => ['attempts' => $this->attempts()]]), + ]), + '', // default exchange: routes by queue name, no binding required + $delay_queue + ); // The retry was republished as a new message; ack the current one. $this->rabbitmq->ack($this); diff --git a/tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php b/tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php new file mode 100644 index 000000000..52ecf7091 --- /dev/null +++ b/tests/Unit/Jobs/SponsorServicesMQJobReleaseIntegrationTest.php @@ -0,0 +1,182 @@ +getChannel(); // force the lazy connection open + } catch (\Throwable $ex) { + $this->markTestSkipped("RabbitMQ broker not reachable: {$ex->getMessage()}"); + } + + $this->queue = $queue; + + $suffix = uniqid(); + $this->queue_name = "test-sponsor-users-release-{$suffix}"; + $this->exchange_name = "test-sponsor-users-ex-{$suffix}"; + + // A direct exchange bound only by the event-type routing key, mirroring + // the production topology (sponsor-users-api-message-broker). + $channel->exchange_declare($this->exchange_name, 'direct', false, false, true); + $channel->queue_declare($this->queue_name, false, true, false, false); + $channel->queue_bind($this->queue_name, $this->exchange_name, EventTypes::AUTH_USER_ADDED_TO_GROUP); + } + + protected function tearDown(): void + { + if (!is_null($this->queue)) { + foreach ([$this->queue_name, $this->queue_name . '.delay.1000'] as $queue) { + try { + $this->queue->getChannel(true)->queue_delete($queue); + } catch (\Throwable $ex) { + // already gone (x-expires) or never created - nothing to clean + } + } + try { + $this->queue->getChannel(true)->exchange_delete($this->exchange_name); + } catch (\Throwable $ex) { + } + } + parent::tearDown(); + } + + public function testReleasedJobIsRedeliveredWithItsOriginalEventType(): void + { + $body = [ + 'user_external_id' => 1, + 'sponsor_id' => 2, + 'summit_id' => 3, + 'group_slug' => 'sponsors', + ]; + + $this->queue->getChannel()->basic_publish( + new AMQPMessage(json_encode($body), [ + 'content_type' => 'application/json', + 'delivery_mode' => AMQPMessage::DELIVERY_MODE_PERSISTENT, + ]), + $this->exchange_name, + EventTypes::AUTH_USER_ADDED_TO_GROUP + ); + + // First delivery: the routing key IS the event type. + $job = $this->popWithinSeconds(5); + $this->assertInstanceOf(SponsorServicesMQJob::class, $job); + $this->assertSame(EventTypes::AUTH_USER_ADDED_TO_GROUP, $job->getEventType()); + $this->assertSame(1, $job->attempts()); + + // The real broker enforces queue-argument equivalence and binding + // resolution - the two failure modes the mocked unit tests cannot see. + $job->release(1); + + $redelivered = $this->popWithinSeconds(10); + $this->assertNotNull($redelivered, 'the released message must be redelivered after the delay'); + $this->assertSame( + EventTypes::AUTH_USER_ADDED_TO_GROUP, + $redelivered->getEventType(), + 'the original event type must survive redelivery' + ); + $this->assertSame(2, $redelivered->attempts(), 'the attempt count must carry over'); + $this->assertSame(3, $redelivered->maxTries()); + + $data = $redelivered->payload()['data']; + foreach ($body as $key => $value) { + $this->assertSame($value, $data[$key] ?? null, "original payload key {$key} must be preserved"); + } + + $redelivered->delete(); // ack, leave the queue clean + } + + /** + * The delay queue carries x-expires, so the broker DELETES it once idle. + * A later release in the same long-lived worker process must re-create it + * - an implementation that only declares once per process (e.g. by caching + * the declared name) publishes the retry into a deleted queue and the + * default exchange drops it silently. + */ + public function testASecondReleaseAfterTheDelayQueueExpiredStillRedelivers(): void + { + $this->queue->getChannel()->basic_publish( + new AMQPMessage(json_encode(['user_external_id' => 1]), [ + 'content_type' => 'application/json', + 'delivery_mode' => AMQPMessage::DELIVERY_MODE_PERSISTENT, + ]), + $this->exchange_name, + EventTypes::AUTH_USER_ADDED_TO_GROUP + ); + + $job = $this->popWithinSeconds(5); + $this->assertNotNull($job); + $job->release(1); // creates .delay.1000 (x-expires 2000) + + $redelivered = $this->popWithinSeconds(10); + $this->assertNotNull($redelivered, 'first retry must be redelivered'); + + // Let the (now idle) delay queue hit its x-expires and be deleted. + sleep(3); + + $redelivered->release(1); + + $third = $this->popWithinSeconds(10); + $this->assertNotNull($third, 'a release after the delay queue expired must re-create it and still redeliver'); + $this->assertSame(EventTypes::AUTH_USER_ADDED_TO_GROUP, $third->getEventType()); + $this->assertSame(3, $third->attempts()); + + $third->delete(); + } + + private function popWithinSeconds(int $max_seconds): ?SponsorServicesMQJob + { + $deadline = microtime(true) + $max_seconds; + do { + $job = $this->queue->pop($this->queue_name); + if (!is_null($job)) { + return $job; + } + usleep(200000); + } while (microtime(true) < $deadline); + + return null; + } +} diff --git a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php index 0d411e79d..1106b2a15 100644 --- a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php +++ b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php @@ -152,28 +152,39 @@ public function testReleaseDeadLettersBackThroughTheDefaultExchange(): void $message->setDeliveryInfo(1, false, 'sponsor_users', EventTypes::AUTH_USER_ADDED_TO_SPONSOR_AND_SUMMIT); $rabbitmq = Mockery::mock(RabbitMQQueue::class); - - $rabbitmq->shouldReceive('declareQueue')->once()->with( + $channel = Mockery::mock(\PhpAmqpLib\Channel\AMQPChannel::class); + $rabbitmq->shouldReceive('getChannel')->andReturn($channel); + + // Re-declared on EVERY release (x-expires deletes the idle queue), with + // dead-lettering through the default exchange - the consumer exchange is + // direct and only binds the auth_user_* keys, so anything else drops the + // retried message. + $channel->shouldReceive('queue_declare')->once()->with( $queue_name . '.delay.30000', + false, true, false, - [ - 'x-dead-letter-exchange' => '', - 'x-dead-letter-routing-key' => $queue_name, - 'x-message-ttl' => 30000, - 'x-expires' => 60000, - ] + false, + false, + Mockery::on(function ($arguments) use ($queue_name) { + return $arguments instanceof \PhpAmqpLib\Wire\AMQPTable + && $arguments->getNativeData() == [ + 'x-dead-letter-exchange' => '', + 'x-dead-letter-routing-key' => $queue_name, + 'x-message-ttl' => 30000, + 'x-expires' => 60000, + ]; + }) ); $republished = null; - $rabbitmq->shouldReceive('laterRaw')->once()->with( - 30, - Mockery::on(function ($payload) use (&$republished) { - $republished = $payload; - return is_string($payload); + $channel->shouldReceive('basic_publish')->once()->with( + Mockery::on(function ($msg) use (&$republished) { + $republished = $msg instanceof AMQPMessage ? $msg->getBody() : null; + return $msg instanceof AMQPMessage; }), - $queue_name, - 1 // first attempt + '', // default exchange + $queue_name . '.delay.30000' ); $rabbitmq->shouldReceive('ack')->once(); @@ -211,17 +222,18 @@ public function testReleaseOfARedeliveredMessageKeepsTheOriginalEventType(): voi $message->setDeliveryInfo(1, false, 'sponsor_users', $queue_name); $rabbitmq = Mockery::mock(RabbitMQQueue::class); - $rabbitmq->shouldReceive('declareQueue')->once(); + $channel = Mockery::mock(\PhpAmqpLib\Channel\AMQPChannel::class); + $rabbitmq->shouldReceive('getChannel')->andReturn($channel); + $channel->shouldReceive('queue_declare')->once(); $republished = null; - $rabbitmq->shouldReceive('laterRaw')->once()->with( - 120, - Mockery::on(function ($payload) use (&$republished) { - $republished = $payload; - return is_string($payload); + $channel->shouldReceive('basic_publish')->once()->with( + Mockery::on(function ($msg) use (&$republished) { + $republished = $msg instanceof AMQPMessage ? $msg->getBody() : null; + return $msg instanceof AMQPMessage; }), - $queue_name, - 1 + '', + $queue_name . '.delay.120000' ); $rabbitmq->shouldReceive('ack')->once(); From c43ce971283c93edcf11874e3862b30831b74371 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 18:40:21 -0300 Subject: [PATCH 15/20] fix: validate sponsor/summit ownership on group events 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. --- .../Model/Imp/SponsorUserSyncService.php | 30 +++ .../SponsorUserPermissionTrackingTest.php | 239 +++++++++++++++++- 2 files changed, 255 insertions(+), 14 deletions(-) diff --git a/app/Services/Model/Imp/SponsorUserSyncService.php b/app/Services/Model/Imp/SponsorUserSyncService.php index 47edee66c..4004b8cf1 100644 --- a/app/Services/Model/Imp/SponsorUserSyncService.php +++ b/app/Services/Model/Imp/SponsorUserSyncService.php @@ -24,6 +24,7 @@ use models\main\IGroupRepository; use models\main\IMemberRepository; use models\main\Member; +use App\Models\Foundation\Summit\Repositories\ISponsorRepository; use models\summit\ISummitRepository; use models\summit\Sponsor; use models\summit\Summit; @@ -50,6 +51,8 @@ final class SponsorUserSyncService private IExternalUserApi $external_user_api; + private ISponsorRepository $sponsor_repository; + /** * SponsorUserSyncService constructor. * @param ISummitRepository $summit_repository @@ -58,6 +61,7 @@ final class SponsorUserSyncService * @param ISummitSponsorService $summit_sponsor_service * @param IMemberService $member_service * @param IExternalUserApi $external_user_api + * @param ISponsorRepository $sponsor_repository * @param ITransactionService $tx_service */ public function __construct @@ -68,6 +72,7 @@ public function __construct ISummitSponsorService $summit_sponsor_service, IMemberService $member_service, IExternalUserApi $external_user_api, + ISponsorRepository $sponsor_repository, ITransactionService $tx_service ) { @@ -78,6 +83,7 @@ public function __construct $this->summit_sponsor_service = $summit_sponsor_service; $this->member_service = $member_service; $this->external_user_api = $external_user_api; + $this->sponsor_repository = $sponsor_repository; } /** @@ -301,6 +307,17 @@ public function addSponsorUserToGroup(int $user_id, string $group_slug, int $spo // from the IDP as a side effect. $this->assertAllowedSponsorGroup($group_slug); + // The producer derives sponsor_id and summit_id from the same AccessRight, + // so a mismatched pair is a forged or buggy event - and a deleted sponsor + // leaves nothing to grant onto. Fail here, BEFORE resolveMember (same + // no-side-effect rule as above) and loudly (failed_jobs), rather than ever + // writing onto another summit's sponsor row. + $summit = $this->resolveSummit($summit_id); + if (is_null($summit->getSummitSponsorById($sponsor_id))) { + throw new ValidationException( + "Sponsor {$sponsor_id} does not belong to summit {$summit_id}."); + } + // Resolve (and, if needed, register from the IDP) OUTSIDE the transaction below. // registerExternalUserById opens its own transaction and dispatches NewMember / // MemberDataUpdatedExternally right after it. Those jobs are pushed immediately: @@ -374,6 +391,19 @@ public function removeSponsorUserFromGroup(int $user_id, string $group_slug, int $this->assertAllowedSponsorGroup($group_slug); + // Reject only a sponsor that exists on a DIFFERENT summit (forged or buggy + // event - the producer derives both ids from the same AccessRight). A + // sponsor deleted ENTIRELY must NOT skip: its Sponsor_Users rows are gone, + // and running the removal is exactly what recomputes the remaining + // permission count and strips the global sponsors group when this was the + // member's last sponsor - skipping would leave residual show-admin access. + $sponsor = $this->sponsor_repository->getById($sponsor_id); + if ($sponsor instanceof Sponsor && $sponsor->getSummitId() !== $summit_id) { + Log::warning( + "SponsorUserSyncService::removeSponsorUserFromGroup sponsor {$sponsor_id} belongs to summit {$sponsor->getSummitId()}, not event summit {$summit_id} - skipping"); + return; + } + // Revocation must not provision (see findMember): a member that was never // synced holds no permission entry and no group membership to remove. $member = $this->findMember($user_id); diff --git a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php index 516b24b23..915f54dfa 100644 --- a/tests/Unit/Services/SponsorUserPermissionTrackingTest.php +++ b/tests/Unit/Services/SponsorUserPermissionTrackingTest.php @@ -238,6 +238,213 @@ public function testRemoveSponsorUserFromGroupRejectsNonSponsorGroup(): void ); } + /** + * Builds a second sponsor belonging to summit2 (a DIFFERENT summit than the + * event summit used by these tests). Caller must clean it up with + * deleteCrossSummitSponsor() in a finally block. + */ + private function createCrossSummitSponsor(): \models\summit\Sponsor + { + $summit2 = self::$summit_repository->getById(self::$summit2->getId()); + $company = self::$em->find(\models\main\Company::class, self::$companies[1]->getId()); + + $other_sponsor = new \models\summit\Sponsor(); + $other_sponsor->setCompany($company); + $summit2->addSummitSponsor($other_sponsor); + self::$em->persist($other_sponsor); + self::$em->flush(); + + return $other_sponsor; + } + + /** + * Removes the cross-summit sponsor THROUGH the EntityManager. A raw-SQL + * delete would leave the managed entity dangling in the unit of work, + * referencing a Summit the teardown is about to remove - the next flush + * then fails with "new entity found through relationship" and the + * member/group fixtures leak into the shared test database. + */ + private function deleteCrossSummitSponsor(\models\summit\Sponsor $sponsor): void + { + $managed = self::$em->find(\models\summit\Sponsor::class, $sponsor->getId()); + if (!is_null($managed)) { + self::$em->remove($managed); // owning side: Sponsor_Users rows go with it + self::$em->flush(); + } + } + + /** + * The producer derives sponsor_id and summit_id from the same AccessRight, + * so a mismatched pair is a forged or buggy event. When the member already + * holds a Sponsor_Users row on the foreign sponsor, the permission must NOT + * be written onto another summit's sponsor row. + */ + public function testAddSponsorUserToGroupRejectsSponsorFromAnotherSummit(): void + { + $member = self::$member_repository->find(self::$member->getId()); + $other_sponsor = $this->createCrossSummitSponsor(); + $other_sponsor->addUser($member); // row on the FOREIGN sponsor exists + self::$em->flush(); + + $member_id = $member->getId(); + $external_id = $member->getUserExternalId(); + $other_sponsor_id = $other_sponsor->getId(); + + try { + $thrown = null; + try { + // Event claims the FIRST summit but carries summit2's sponsor. + $this->getService()->addSponsorUserToGroup( + $external_id, + IGroup::Sponsors, + $other_sponsor_id, + self::$summit->getId() + ); + } catch (\models\exceptions\ValidationException $ex) { + $thrown = $ex; + } + + $this->assertNotNull($thrown, 'a sponsor from another summit must be rejected'); + $this->assertEmpty( + $this->getPermissions($other_sponsor_id, $member_id), + 'no permission may be written onto another summit\'s sponsor row' + ); + } finally { + $this->deleteCrossSummitSponsor($other_sponsor); + } + } + + /** + * The ownership gate must run BEFORE resolveMember: a rejected event must + * not provision a member from the IDP as a side effect. + */ + public function testAddSponsorUserToGroupRejectedCrossSummitEventDoesNotProvisionMember(): void + { + $other_sponsor = $this->createCrossSummitSponsor(); + $other_sponsor_id = $other_sponsor->getId(); + $external_id = mt_rand(1500000000, 2000000000); // no local Member row + $email = sprintf("smarcet+xsummit_%s@gmail.com", str_random(8)); + + // The user DOES exist at the IDP - on-demand registration would succeed. + $this->mockExternalUserApi([ + 'id' => $external_id, + 'email' => $email, + 'first_name' => 'Cross', + 'last_name' => 'Summit', + 'bio' => '', + 'active' => true, + 'email_verified' => true, + 'groups' => [], + 'public_profile_show_photo' => false, + 'public_profile_show_fullname' => false, + 'public_profile_show_email' => false, + 'public_profile_show_telephone_number' => false, + 'public_profile_show_bio' => false, + 'public_profile_show_social_media_info' => false, + 'public_profile_allow_chat_with_me' => false, + ]); + + try { + $thrown = null; + try { + $this->getService()->addSponsorUserToGroup( + $external_id, + IGroup::Sponsors, + $other_sponsor_id, + self::$summit->getId() + ); + } catch (\models\exceptions\ValidationException $ex) { + $thrown = $ex; + } + + $this->assertNotNull($thrown, 'a sponsor from another summit must be rejected'); + $this->assertNull( + self::$member_repository->getByExternalId($external_id), + 'a rejected event must not provision a member from the IDP' + ); + } finally { + $this->deleteCrossSummitSponsor($other_sponsor); + $leftover = self::$member_repository->getByExternalId($external_id); + if (!is_null($leftover)) { + self::$em->remove($leftover); + self::$em->flush(); + } + } + } + + /** + * A removal event carrying a sponsor of ANOTHER summit must be a no-op: it + * must neither touch the foreign sponsor's permission entry nor strip the + * member's global group. + */ + public function testRemoveSponsorUserFromGroupSkipsSponsorFromAnotherSummit(): void + { + $member = self::$member_repository->find(self::$member->getId()); + $other_sponsor = $this->createCrossSummitSponsor(); + $other_sponsor->addUser($member); + self::$em->flush(); + + $member_id = $member->getId(); + $external_id = $member->getUserExternalId(); + $other_sponsor_id = $other_sponsor->getId(); + + try { + // Legit grant on summit2 writes the permission on the foreign row. + $this->getService()->addSponsorUserToGroup( + $external_id, IGroup::Sponsors, $other_sponsor_id, self::$summit2->getId()); + $this->assertContains(IGroup::Sponsors, $this->getPermissions($other_sponsor_id, $member_id)); + + // Forged/buggy removal: summit1's event carrying summit2's sponsor. + $this->getService()->removeSponsorUserFromGroup( + $external_id, IGroup::Sponsors, $other_sponsor_id, self::$summit->getId()); + + self::$em->clear(); + $this->assertContains( + IGroup::Sponsors, + $this->getPermissions($other_sponsor_id, $member_id), + 'a cross-summit removal must not touch the foreign sponsor\'s permission entry' + ); + $this->assertTrue( + self::$member_repository->find($member_id)->belongsToGroup(IGroup::Sponsors), + 'a cross-summit removal must not strip the global group' + ); + } finally { + $this->deleteCrossSummitSponsor($other_sponsor); + } + } + + /** + * A sponsor deleted ENTIRELY must not skip the removal: its Sponsor_Users + * rows are gone, and running the removal is exactly what recomputes the + * remaining permission count and strips the global sponsors group when this + * was the member's last sponsor. Skipping would leave the member with + * residual show-admin access forever. + */ + public function testRemoveSponsorUserFromGroupStillCleansUpWhenSponsorWasDeleted(): void + { + $member_id = self::$member->getId(); + $external_id = self::$member->getUserExternalId(); + + // Pre-condition: member holds the global group and NO remaining + // permission entries (the fixture row has a NULL Permissions column). + $this->assertTrue( + self::$member_repository->find($member_id)->belongsToGroup(IGroup::Sponsors) + ); + + $this->getService()->removeSponsorUserFromGroup( + $external_id, + IGroup::Sponsors, + PHP_INT_MAX, // sponsor no longer exists anywhere + self::$summit->getId() + ); + + self::$em->clear(); + $this->assertFalse( + self::$member_repository->find($member_id)->belongsToGroup(IGroup::Sponsors), + 'the last-sponsor cleanup must still strip the global group when the sponsor row is gone' + ); + } + /** * The group slug must be written into the Sponsor_Users.Permissions JSON * column for the correct (SponsorID, MemberID) row. @@ -375,17 +582,28 @@ public function testAddSponsorUserToGroupRegistersMemberOnDemandWhenMissing(): v * transaction and the transaction later rolled back, the Member row would vanish while * the already-queued jobs kept pointing at its id - they would fail forever. * - * Here the sponsor does not exist, so the eager-create path throws inside the - * transaction AFTER the member was resolved. The member must still be present - * afterwards. + * Here the group row for an ALLOWED slug does not exist, so the transaction + * throws AFTER the member was resolved (the allowlist and sponsor-ownership + * gates both pass, and the missing Group row is only discovered inside the + * transaction). The member must still be present afterwards. */ public function testAddSponsorUserToGroupKeepsOnDemandMemberWhenTransactionFails(): void { $external_id = mt_rand(1500000000, 2000000000); // no local Member row - $sponsor_id = PHP_INT_MAX; // no such sponsor: eager-create throws inside the tx + $sponsor_id = self::$sponsors[1]->getId(); // real sponsor: passes the ownership gate $summit_id = self::$summit->getId(); $email = sprintf("smarcet+rollback_%s@gmail.com", str_random(8)); + // Ensure no Group row exists for the allowed slug used below, so the + // in-transaction getBySlug lookup fails AFTER resolveMember succeeded. + // (Fixtures create their own groups per test, so deleting is safe.) + $conn = self::$em->getConnection(); + $conn->executeStatement( + 'DELETE gm FROM Group_Members gm INNER JOIN `Group` g ON g.ID = gm.GroupID WHERE g.Code = ?', + [IGroup::SponsorExternalUsers] + ); + $conn->executeStatement('DELETE FROM `Group` WHERE Code = ?', [IGroup::SponsorExternalUsers]); + $this->mockExternalUserApi([ 'id' => $external_id, 'email' => $email, @@ -409,15 +627,15 @@ public function testAddSponsorUserToGroupKeepsOnDemandMemberWhenTransactionFails try { $this->getService()->addSponsorUserToGroup( $external_id, - IGroup::Sponsors, - $sponsor_id, // unknown sponsor: the eager-create makes the transaction throw + IGroup::SponsorExternalUsers, // allowed slug with no Group row: throws inside the tx + $sponsor_id, $summit_id ); } catch (\models\exceptions\EntityNotFoundException $ex) { $thrown = $ex; } - $this->assertNotNull($thrown, 'The unknown sponsor should have failed the transaction'); + $this->assertNotNull($thrown, 'The missing group row should have failed the transaction'); // The on-demand member was committed by its own transaction, so the jobs // already dispatched for it reference a row that exists. @@ -753,13 +971,6 @@ public function testRemoveSponsorUserFromGroupRemovesGlobalGroupWhenLastSponsor( $service->addSponsorUserToGroup($external_id, IGroup::Sponsors, $sponsor_id, $summit_id); $this->assertContains(IGroup::Sponsors, $this->getPermissions($sponsor_id, $member_id)); - // Doctrine ORM 3 EXTRA_LAZY PersistentCollection::removeElement() delegates to - // parent::removeElement() on the in-memory ArrayCollection first. If the collection - // is still uninitialized (addSponsorUserToGroup leaves it that way), that call - // returns false and changed() is never called, so the flush issues no DELETE. - // Force-initialize through the same model EM so removeFromGroup works correctly. - self::$em->find(\models\main\Member::class, $member_id)->getGroups()->toArray(); - // Remove — no other sponsor holds the permission. $service->removeSponsorUserFromGroup($external_id, IGroup::Sponsors, $sponsor_id, $summit_id); From 4a0f4a64c0f69f748c5b2c38334bff2c5fedacf0 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 18:40:37 -0300 Subject: [PATCH 16/20] fix(tests): stop fixture teardown from leaking rows after an EM reset 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. --- tests/InsertMemberTestData.php | 15 ++++++++++++++- tests/InsertSummitTestData.php | 6 ++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/tests/InsertMemberTestData.php b/tests/InsertMemberTestData.php index a329f9b40..aad6bedf8 100644 --- a/tests/InsertMemberTestData.php +++ b/tests/InsertMemberTestData.php @@ -160,6 +160,14 @@ protected static function clearMemberTestData() try { if (!self::$em->isOpen()) { self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + // The repositories captured at setup are bound to the CLOSED + // manager (a failed tx_service transaction closes and resets it). + // Re-resolve them from the fresh manager or the finds below throw + // "EntityManager is closed" and the fixtures LEAK - leaked + // duplicate Group rows make getBySlug() resolve a stale row and + // turn group removals into silent no-ops on every later run. + self::$group_repository = self::$em->getRepository(Group::class); + self::$member_repository = self::$em->getRepository(Member::class); } self::$member = self::$member_repository->find(self::$member->getId()); @@ -185,7 +193,12 @@ protected static function clearMemberTestData() self::$em->flush(); } catch (\Exception $ex) { - + // Do NOT let a cleanup failure stay invisible: leaked fixtures + // poison the shared test database for every subsequent run. + fwrite(STDERR, sprintf( + "clearMemberTestData failed - fixtures may have leaked: %s\n", + $ex->getMessage() + )); } } } \ No newline at end of file diff --git a/tests/InsertSummitTestData.php b/tests/InsertSummitTestData.php index 9a7def1e0..2a4e8c062 100644 --- a/tests/InsertSummitTestData.php +++ b/tests/InsertSummitTestData.php @@ -1051,6 +1051,12 @@ protected static function insertSummitTestData(){ protected static function clearSummitTestData(){ if (!self::$em ->isOpen()) { self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + // Same staleness trap as clearMemberTestData: repositories captured + // at setup are bound to the closed manager - re-resolve them or the + // finds below throw and the whole summit fixture set leaks. + self::$summit_repository = self::$em->getRepository(Summit::class); + self::$summit_permission_group_repository = self::$em->getRepository(SummitAdministratorPermissionGroup::class); + self::$media_file_type_repository = self::$em->getRepository(SummitMediaFileType::class); } self::$summit = self::$summit_repository->find(self::$summit->getId()); self::$summit2 = self::$summit_repository->find(self::$summit2->getId()); From a48e6e8bb36edc2b42a39d0686a063a79664c344 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 19:08:19 -0300 Subject: [PATCH 17/20] fix: overwrite any smuggled x_event_type with the resolved event type 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. --- .../SponsorServices/SponsorServicesMQJob.php | 9 ++-- .../Jobs/SponsorServicesMQJobRetryTest.php | 45 +++++++++++++++++++ 2 files changed, 51 insertions(+), 3 deletions(-) diff --git a/app/Jobs/SponsorServices/SponsorServicesMQJob.php b/app/Jobs/SponsorServices/SponsorServicesMQJob.php index 5a45252b9..9b945eb6f 100644 --- a/app/Jobs/SponsorServices/SponsorServicesMQJob.php +++ b/app/Jobs/SponsorServices/SponsorServicesMQJob.php @@ -155,10 +155,13 @@ public function release($delay = 0): void 'x-expires' => $ttl * 2, ])); - // Preserve the original event type across the redelivery (idempotent: - // a second release keeps the value written by the first one). + // Preserve the original event type across the redelivery. ALWAYS write + // the resolved value: on a first delivery the routing key is + // authoritative, so this overwrites any forged/buggy x_event_type the + // producer body may have carried; on a redelivery getEventType() already + // resolves from the body, so rewriting it is a no-op (idempotent). $body = json_decode($this->getRawBody(), true) ?? []; - $body[self::EventTypeKey] = $body[self::EventTypeKey] ?? $this->getEventType(); + $body[self::EventTypeKey] = $this->getEventType(); $channel->basic_publish( new AMQPMessage(json_encode($body), [ diff --git a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php index 1106b2a15..dae3277d1 100644 --- a/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php +++ b/tests/Unit/Jobs/SponsorServicesMQJobRetryTest.php @@ -205,6 +205,51 @@ public function testReleaseDeadLettersBackThroughTheDefaultExchange(): void } } + /** + * A forged/buggy x_event_type in a FIRST-delivery body must not survive the + * release: on first delivery the routing key is authoritative (getEventType + * ignores the body), so release() must write the RESOLVED event type over + * whatever the body carried - otherwise the retry would run a different + * handler than the original delivery did. + */ + public function testReleaseOverwritesAForgedEventTypeWithTheResolvedOne(): void + { + $queue_name = 'sponsor-users-api-summit-api-badge-scans-queue'; + + // Routing key says ADDED_TO_GROUP; the body smuggles a conflicting type. + $message = new AMQPMessage(json_encode([ + 'user_external_id' => 1, + SponsorServicesMQJob::EventTypeKey => EventTypes::AUTH_USER_REMOVED_FROM_SUMMIT, + ])); + $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(); + + $republished = null; + $channel->shouldReceive('basic_publish')->once()->with( + Mockery::on(function ($msg) use (&$republished) { + $republished = $msg instanceof AMQPMessage ? $msg->getBody() : null; + return $msg instanceof AMQPMessage; + }), + '', + Mockery::any() + ); + $rabbitmq->shouldReceive('ack')->once(); + + $job = new SponsorServicesMQJob(app(), $rabbitmq, $message, 'rabbitmq', $queue_name); + $job->release(30); + + $body = json_decode($republished, true); + $this->assertSame( + EventTypes::AUTH_USER_ADDED_TO_GROUP, + $body[SponsorServicesMQJob::EventTypeKey] ?? null, + 'release must overwrite a smuggled x_event_type with the routing-key-resolved event type' + ); + } + /** * A second release (the message already carries the event type from the * first one) must keep the ORIGINAL event type, not overwrite it with the From 30d02eacb479a32f8157772f8dd8ea49180f912f Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 20:15:22 -0300 Subject: [PATCH 18/20] fix(tests): refresh cleanup repositories unconditionally 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. --- tests/InsertMemberTestData.php | 23 ++++++++++++++--------- tests/InsertSummitTestData.php | 14 ++++++++------ 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/tests/InsertMemberTestData.php b/tests/InsertMemberTestData.php index aad6bedf8..ecbd63c2b 100644 --- a/tests/InsertMemberTestData.php +++ b/tests/InsertMemberTestData.php @@ -160,16 +160,19 @@ protected static function clearMemberTestData() try { if (!self::$em->isOpen()) { self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); - // The repositories captured at setup are bound to the CLOSED - // manager (a failed tx_service transaction closes and resets it). - // Re-resolve them from the fresh manager or the finds below throw - // "EntityManager is closed" and the fixtures LEAK - leaked - // duplicate Group rows make getBySlug() resolve a stale row and - // turn group removals into silent no-ops on every later run. - self::$group_repository = self::$em->getRepository(Group::class); - self::$member_repository = self::$em->getRepository(Member::class); } + // Re-resolve the repositories UNCONDITIONALLY: a failed tx_service + // transaction closes and resets the manager mid-test, and a test + // finally that already reopened it leaves self::$em fresh and OPEN - + // an isOpen() check here would then skip the refresh while the + // repositories captured at setup still point at the closed manager, + // making the finds below fail and the fixtures LEAK. Leaked duplicate + // Group rows make getBySlug() resolve a stale row and turn group + // removals into silent no-ops on every later run. + self::$group_repository = self::$em->getRepository(Group::class); + self::$member_repository = self::$em->getRepository(Member::class); + self::$member = self::$member_repository->find(self::$member->getId()); self::$group = self::$group_repository->find(self::$group->getId()); @@ -194,7 +197,9 @@ protected static function clearMemberTestData() } catch (\Exception $ex) { // Do NOT let a cleanup failure stay invisible: leaked fixtures - // poison the shared test database for every subsequent run. + // poison the shared test database for every subsequent run (leaked + // duplicate Group rows once turned group removals into silent + // no-ops suite-wide). Log AND rethrow so the test fails loudly. fwrite(STDERR, sprintf( "clearMemberTestData failed - fixtures may have leaked: %s\n", $ex->getMessage() diff --git a/tests/InsertSummitTestData.php b/tests/InsertSummitTestData.php index 2a4e8c062..02cbe4fda 100644 --- a/tests/InsertSummitTestData.php +++ b/tests/InsertSummitTestData.php @@ -1051,13 +1051,15 @@ protected static function insertSummitTestData(){ protected static function clearSummitTestData(){ if (!self::$em ->isOpen()) { self::$em = Registry::resetManager(SilverstripeBaseModel::EntityManager); - // Same staleness trap as clearMemberTestData: repositories captured - // at setup are bound to the closed manager - re-resolve them or the - // finds below throw and the whole summit fixture set leaks. - self::$summit_repository = self::$em->getRepository(Summit::class); - self::$summit_permission_group_repository = self::$em->getRepository(SummitAdministratorPermissionGroup::class); - self::$media_file_type_repository = self::$em->getRepository(SummitMediaFileType::class); } + // Unconditional refresh - same staleness trap as clearMemberTestData + // (see the note there): the manager may have been reset mid-test while + // self::$em was already reopened, so isOpen() alone cannot tell whether + // the repositories captured at setup are still usable. + self::$summit_repository = self::$em->getRepository(Summit::class); + self::$summit_permission_group_repository = self::$em->getRepository(SummitAdministratorPermissionGroup::class); + self::$media_file_type_repository = self::$em->getRepository(SummitMediaFileType::class); + self::$summit = self::$summit_repository->find(self::$summit->getId()); self::$summit2 = self::$summit_repository->find(self::$summit2->getId()); self::$default_media_file_type = self::$media_file_type_repository->find(self::$default_media_file_type->getId()); From 491a88580972f81d9a9c2e8987a73db9ab5c1ef1 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 20:15:36 -0300 Subject: [PATCH 19/20] fix(tests): rethrow fixture cleanup failures instead of swallowing them 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. --- tests/InsertMemberTestData.php | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/InsertMemberTestData.php b/tests/InsertMemberTestData.php index ecbd63c2b..8af731ff3 100644 --- a/tests/InsertMemberTestData.php +++ b/tests/InsertMemberTestData.php @@ -204,6 +204,7 @@ protected static function clearMemberTestData() "clearMemberTestData failed - fixtures may have leaked: %s\n", $ex->getMessage() )); + throw $ex; } } } \ No newline at end of file From 2cbc1da8850c670ef579f3589647a7ad6327b0b4 Mon Sep 17 00:00:00 2001 From: smarcet Date: Mon, 10 Aug 2026 20:36:20 -0300 Subject: [PATCH 20/20] fix(tests): detach the unit of work before fixture cleanup flushes The rethrow in 491a88580 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. --- tests/InsertMemberTestData.php | 6 ++++++ tests/InsertSummitTestData.php | 4 ++++ 2 files changed, 10 insertions(+) diff --git a/tests/InsertMemberTestData.php b/tests/InsertMemberTestData.php index 8af731ff3..d916dc546 100644 --- a/tests/InsertMemberTestData.php +++ b/tests/InsertMemberTestData.php @@ -173,6 +173,12 @@ protected static function clearMemberTestData() self::$group_repository = self::$em->getRepository(Group::class); self::$member_repository = self::$em->getRepository(Member::class); + // Detach whatever the test left in the unit of work: entity unit + // tests build unpersisted object graphs hanging off managed + // fixtures, and flushing them here throws "non-persisted new + // entities found" - the cleanup must only flush its own removals. + self::$em->clear(); + self::$member = self::$member_repository->find(self::$member->getId()); self::$group = self::$group_repository->find(self::$group->getId()); diff --git a/tests/InsertSummitTestData.php b/tests/InsertSummitTestData.php index 02cbe4fda..ff76a40f3 100644 --- a/tests/InsertSummitTestData.php +++ b/tests/InsertSummitTestData.php @@ -1060,6 +1060,10 @@ protected static function clearSummitTestData(){ self::$summit_permission_group_repository = self::$em->getRepository(SummitAdministratorPermissionGroup::class); self::$media_file_type_repository = self::$em->getRepository(SummitMediaFileType::class); + // Detach whatever the test left in the unit of work (see the note in + // clearMemberTestData) - the cleanup must only flush its own removals. + self::$em->clear(); + self::$summit = self::$summit_repository->find(self::$summit->getId()); self::$summit2 = self::$summit_repository->find(self::$summit2->getId()); self::$default_media_file_type = self::$media_file_type_repository->find(self::$default_media_file_type->getId());