From c31e8186a19c7b10aa2d6fa53f56ebd8a57382b8 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 27 May 2026 09:56:26 -0300 Subject: [PATCH 1/2] fix(external-user-sync): guard email collisions on IDP delete/reassign When the IDP deletes a user and reassigns its email to a different external id, registration failed with a Member.Email unique-constraint violation. Free the former member's email (the IDP is the source of truth) before assigning it. - MemberService::registerExternalUserByPayload: invalidate the former member's email when the email moved to a new external id - ResourceServerContext::getCurrentUser: same collision guard before setEmail - ScheduleEntity: wrap lifecycle event/cache hooks so a queue/cache failure cannot roll back the Doctrine delete/update - EventServiceProvider: dispatch ScheduleEntityLifeCycleEvent via JobDispatcher::withDbFallback - add reproducing functional test (tests/MemberServiceTest.php) --- .../Foundation/Summit/ScheduleEntity.php | 69 ++++++-- app/Models/OAuth2/ResourceServerContext.php | 10 ++ app/Providers/EventServiceProvider.php | 16 +- app/Services/Model/Imp/MemberService.php | 11 +- tests/MemberServiceTest.php | 165 ++++++++++++++++++ 5 files changed, 247 insertions(+), 24 deletions(-) create mode 100644 tests/MemberServiceTest.php diff --git a/app/Models/Foundation/Summit/ScheduleEntity.php b/app/Models/Foundation/Summit/ScheduleEntity.php index 7d735a8484..867b9578ea 100644 --- a/app/Models/Foundation/Summit/ScheduleEntity.php +++ b/app/Models/Foundation/Summit/ScheduleEntity.php @@ -70,23 +70,39 @@ private function _getSummitId(): int #[ORM\PreRemove] public function deleting($args) { - Event::dispatch(new ScheduleEntityLifeCycleEvent(ScheduleEntityLifeCycleEvent::Operation_Delete, - $this->_getSummitId(), - $this->id, - $this->_getClassName())); + try { + Event::dispatch(new ScheduleEntityLifeCycleEvent(ScheduleEntityLifeCycleEvent::Operation_Delete, + $this->_getSummitId(), + $this->id, + $this->_getClassName())); + } catch (\Exception $ex) { + // Lifecycle notifications must not abort Doctrine transactions. + // A queue/cache failure here should never roll back the delete. + Log::warning(sprintf( + "ScheduleEntity::deleting failed to dispatch lifecycle event for %s id %s: %s", + $this->_getClassName(), $this->id, $ex->getMessage() + )); + } } #[ORM\PostRemove] public function deleted($args) { - $this->cachedDeleted($args); + try { + $this->cachedDeleted($args); + } catch (\Exception $ex) { + Log::warning(sprintf( + "ScheduleEntity::deleted failed cache cleanup for %s id %s: %s", + $this->_getClassName(), $this->id, $ex->getMessage() + )); + } } #[ORM\PreUpdate] public function updating(PreUpdateEventArgs $args) { parent::updating($args); - } + } #[ORM\PostUpdate] public function updated($args) @@ -94,22 +110,43 @@ public function updated($args) if($this->shouldSkipDataUpdate()){ Log::debug(sprintf("ScheduleEntity::updated skipping data update for id %s type %s ...", $this->id, $this->_getClassName())); return; - }; + } Log::debug(sprintf("ScheduleEntity::updated id %s", $this->id)); - $this->cachedUpdated($args); - Event::dispatch(new ScheduleEntityLifeCycleEvent(ScheduleEntityLifeCycleEvent::Operation_Update, - $this->_getSummitId(), - $this->id, - $this->_getClassName())); + try { + $this->cachedUpdated($args); + } catch (\Exception $ex) { + Log::warning(sprintf( + "ScheduleEntity::updated failed cache update for %s id %s: %s", + $this->_getClassName(), $this->id, $ex->getMessage() + )); + } + try { + Event::dispatch(new ScheduleEntityLifeCycleEvent(ScheduleEntityLifeCycleEvent::Operation_Update, + $this->_getSummitId(), + $this->id, + $this->_getClassName())); + } catch (\Exception $ex) { + Log::warning(sprintf( + "ScheduleEntity::updated failed to dispatch lifecycle event for %s id %s: %s", + $this->_getClassName(), $this->id, $ex->getMessage() + )); + } } // events #[ORM\PostPersist] public function inserted($args) { - Event::dispatch(new ScheduleEntityLifeCycleEvent(ScheduleEntityLifeCycleEvent::Operation_Insert, - $this->_getSummitId(), - $this->id, - $this->_getClassName())); + try { + Event::dispatch(new ScheduleEntityLifeCycleEvent(ScheduleEntityLifeCycleEvent::Operation_Insert, + $this->_getSummitId(), + $this->id, + $this->_getClassName())); + } catch (\Exception $ex) { + Log::warning(sprintf( + "ScheduleEntity::inserted failed to dispatch lifecycle event for %s id %s: %s", + $this->_getClassName(), $this->id, $ex->getMessage() + )); + } } } diff --git a/app/Models/OAuth2/ResourceServerContext.php b/app/Models/OAuth2/ResourceServerContext.php index bfc46a25df..a853fe4004 100644 --- a/app/Models/OAuth2/ResourceServerContext.php +++ b/app/Models/OAuth2/ResourceServerContext.php @@ -249,6 +249,16 @@ public function getCurrentUser(bool $synch_groups = true, bool $update_member_fi // update member fields if (!empty($user_email)) { Log::debug(sprintf("ResourceServerContext::getCurrentUser setting email for member %s", $member->getId())); + // guard against email collision: another member may already hold this email + $member_by_email = $this->member_repository->getByEmail($user_email); + if (!is_null($member_by_email) && $member_by_email->getId() !== $member->getId()) { + Log::warning(sprintf( + "ResourceServerContext::getCurrentUser email %s already owned by member %s, invalidating it", + $user_email, + $member_by_email->getId() + )); + $member_by_email->setEmail(sprintf("%s-invalid@invalid", $member_by_email->getId())); + } $member->setEmail($user_email); } diff --git a/app/Providers/EventServiceProvider.php b/app/Providers/EventServiceProvider.php index 68641c68a3..862c6bacd2 100644 --- a/app/Providers/EventServiceProvider.php +++ b/app/Providers/EventServiceProvider.php @@ -348,13 +348,15 @@ public function boot() Log::debug(sprintf("ScheduleEntityLifeCycleEvent event %s", $event)); - ProcessScheduleEntityLifeCycleEvent::dispatch - ( - $event->entity_operator, - $event->summit_id, - $event->entity_id, - $event->entity_type, - $event->params + JobDispatcher::withDbFallback( + new ProcessScheduleEntityLifeCycleEvent( + $event->entity_operator, + $event->summit_id, + $event->entity_id, + $event->entity_type, + $event->params + ), + ['entity_type' => $event->entity_type, 'entity_id' => $event->entity_id] ); }); diff --git a/app/Services/Model/Imp/MemberService.php b/app/Services/Model/Imp/MemberService.php index 4c39559739..172a911f6c 100644 --- a/app/Services/Model/Imp/MemberService.php +++ b/app/Services/Model/Imp/MemberService.php @@ -342,11 +342,20 @@ public function registerExternalUserByPayload(array $user_data):Member{ // first by external id due email could be updated Log::debug(sprintf("MemberService::registerExternalUserByPayload trying to get user by external id %s", $user_external_id)); $member = $this->member_repository->getByExternalIdExclusiveLock(intval($user_external_id)); + $member_by_email = $this->member_repository->getByEmail($email); + // if there are 2 users and the email was transferred to the new external id + if(!is_null($member) && !is_null($member_by_email) && $member_by_email->getUserExternalId() != $user_external_id) { + // the idp is the source of truth + Log::warning(sprintf("MemberService::registerExternalUserByPayload there is a former user with same email %s former id %s invalidating email", $email, $member_by_email->getId())); + $member_by_email->setEmail(sprintf("%s-invalid@invalid", $member_by_email->getUserExternalId())); + $this->member_repository->add($member_by_email, true); + } // if we dont registered yet a member with that external id try to get by email if(is_null($member)) { Log::debug(sprintf("MemberService::registerExternalUserByPayload trying to get user by email %s", $email)); - $member = $this->member_repository->getByEmail($email); + $member = $member_by_email; } + $is_new = false; if(is_null($member)) { Log::debug(sprintf("MemberService::registerExternalUserByPayload %s does not exists , creating it ...", $email)); diff --git a/tests/MemberServiceTest.php b/tests/MemberServiceTest.php new file mode 100644 index 0000000000..c0926fa5ef --- /dev/null +++ b/tests/MemberServiceTest.php @@ -0,0 +1,165 @@ +em = Registry::getManager(SilverstripeBaseModel::EntityManager); + if (!$this->em->isOpen()) { + $this->em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + } + $this->member_repository = EntityManager::getRepository(Member::class); + + $prefix = str_random(10); + // emails are normalized to lowercase by the repository lookups + $this->reassigned_email = strtolower("reassign+{$prefix}@test.com"); + // ExternalUserId is a signed INT column (max 2147483647); keep ids in range and non-overlapping + $this->old_external_id = mt_rand(1000000000, 1499999999); + $this->new_external_id = mt_rand(1500000000, 1999999999); + + // member that currently owns the email, tied to the old (deleted) external id + $old_member = new Member(); + $old_member->setEmail($this->reassigned_email); + $old_member->setActive(true); + $old_member->setEmailVerified(true); + $old_member->setFirstName("Old"); + $old_member->setLastName("Owner"); + $old_member->setUserExternalId($this->old_external_id); + + // member already provisioned for the new external id, with a different email + $new_member = new Member(); + $new_member->setEmail(strtolower("new+{$prefix}@test.com")); + $new_member->setActive(true); + $new_member->setEmailVerified(true); + $new_member->setFirstName("New"); + $new_member->setLastName("Owner"); + $new_member->setUserExternalId($this->new_external_id); + + $this->em->persist($old_member); + $this->em->persist($new_member); + $this->em->flush(); + } + + protected function tearDown(): void + { + try { + if (!$this->em->isOpen()) { + $this->em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + } + foreach ([$this->old_external_id, $this->new_external_id] as $external_id) { + $member = $this->member_repository->findOneBy(['user_external_id' => $external_id]); + if (!is_null($member)) { + $this->em->remove($member); + } + } + $this->em->flush(); + } catch (\Exception $ex) { + // best-effort cleanup + } + parent::tearDown(); + } + + /** + * The IDP moved $reassigned_email from $old_external_id to $new_external_id. + * Registering the new external id with that email must: + * - invalidate the old member's email (so the unique constraint is freed), and + * - assign the reassigned email to the member of the new external id. + * Before the fix this throws a unique-constraint violation on flush. + */ + public function testRegisterExternalUserByPayloadReassignsEmailFromDeletedExternalId(): void + { + $payload = $this->buildExternalProfilePayload($this->new_external_id, $this->reassigned_email); + + $tx_service = App::make(ITransactionService::class); + // mirror registerExternalUserById(): the pessimistic lock requires a transaction + $tx_service->transaction(function () use ($payload) { + return App::make(IMemberService::class)->registerExternalUserByPayload($payload); + }); + + $this->em->clear(); + + $owner_of_email = $this->member_repository->findOneBy(['email' => $this->reassigned_email]); + $this->assertNotNull($owner_of_email, "reassigned email should still belong to a member"); + $this->assertEquals( + $this->new_external_id, + $owner_of_email->getUserExternalId(), + "reassigned email must now belong to the new external id" + ); + + $old_member = $this->member_repository->findOneBy(['user_external_id' => $this->old_external_id]); + $this->assertNotNull($old_member, "old member must still exist"); + $this->assertEquals( + sprintf("%s-invalid@invalid", $this->old_external_id), + $old_member->getEmail(), + "old member's email must be invalidated" + ); + } + + private function buildExternalProfilePayload(int $external_id, string $email): array + { + return [ + 'id' => $external_id, + 'email' => $email, + 'active' => true, + 'email_verified' => true, + 'first_name' => 'Reassigned', + 'last_name' => 'User', + 'bio' => '', + '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, + ]; + } +} From fafcf59e7c1954b2c6374920a4f8534769a3f2e3 Mon Sep 17 00:00:00 2001 From: smarcet Date: Wed, 27 May 2026 10:32:20 -0300 Subject: [PATCH 2/2] fix(external-user-sync): guard registerExternalUser create branch against email collision registerExternalUser created a Member with the IDP email without checking whether a former member (e.g. one whose delete failed) still held it, hitting the Member.Email unique constraint. Apply the same invalidate-former-email guard already used in registerExternalUserByPayload. Add a reproducing test for the create-branch collision (tests/MemberServiceTest.php). --- app/Services/Model/Imp/MemberService.php | 8 ++++ tests/MemberServiceTest.php | 48 ++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 3 deletions(-) diff --git a/app/Services/Model/Imp/MemberService.php b/app/Services/Model/Imp/MemberService.php index 172a911f6c..34b2a92d99 100644 --- a/app/Services/Model/Imp/MemberService.php +++ b/app/Services/Model/Imp/MemberService.php @@ -316,6 +316,14 @@ public function registerExternalUser(ExternalUserDTO $userDTO): Member ); $member = $this->member_repository->getByExternalIdExclusiveLock($userDTO->getId()); if(is_null($member)) { + // guard against email collision: a former member (e.g. a failed delete) may still hold this email + $member_by_email = $this->member_repository->getByEmail($userDTO->getEmail()); + if(!is_null($member_by_email) && $member_by_email->getUserExternalId() != $userDTO->getId()) { + // the idp is the source of truth + Log::warning(sprintf("MemberService::registerExternalUser there is a former user with same email %s former id %s invalidating email", $userDTO->getEmail(), $member_by_email->getId())); + $member_by_email->setEmail(sprintf("%s-invalid@invalid", $member_by_email->getUserExternalId())); + $this->member_repository->add($member_by_email, true); + } $member = new Member(); $member->setUserExternalId($userDTO->getId()); $member->setActive($userDTO->isActive()); diff --git a/tests/MemberServiceTest.php b/tests/MemberServiceTest.php index c0926fa5ef..d927736883 100644 --- a/tests/MemberServiceTest.php +++ b/tests/MemberServiceTest.php @@ -11,6 +11,7 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Services\Model\dto\ExternalUserDTO; use App\Services\Model\IMemberService; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\DB; @@ -42,6 +43,9 @@ final class MemberServiceTest extends TestCase /** @var int */ private $new_external_id; + /** @var int */ + private $create_external_id; + /** @var string */ private $reassigned_email; @@ -61,8 +65,9 @@ protected function setUp(): void // emails are normalized to lowercase by the repository lookups $this->reassigned_email = strtolower("reassign+{$prefix}@test.com"); // ExternalUserId is a signed INT column (max 2147483647); keep ids in range and non-overlapping - $this->old_external_id = mt_rand(1000000000, 1499999999); - $this->new_external_id = mt_rand(1500000000, 1999999999); + $this->old_external_id = mt_rand(1000000000, 1299999999); + $this->new_external_id = mt_rand(1300000000, 1599999999); + $this->create_external_id = mt_rand(1600000000, 1999999999); // member that currently owns the email, tied to the old (deleted) external id $old_member = new Member(); @@ -93,7 +98,7 @@ protected function tearDown(): void if (!$this->em->isOpen()) { $this->em = Registry::resetManager(SilverstripeBaseModel::EntityManager); } - foreach ([$this->old_external_id, $this->new_external_id] as $external_id) { + foreach ([$this->old_external_id, $this->new_external_id, $this->create_external_id] as $external_id) { $member = $this->member_repository->findOneBy(['user_external_id' => $external_id]); if (!is_null($member)) { $this->em->remove($member); @@ -142,6 +147,43 @@ public function testRegisterExternalUserByPayloadReassignsEmailFromDeletedExtern ); } + /** + * Creating a brand-new external user whose email is still held by a former member + * (e.g. a member whose delete failed) must invalidate the former member's email + * instead of failing on the Member.Email unique constraint. + */ + public function testRegisterExternalUserInvalidatesFormerMemberEmailOnCreate(): void + { + $dto = new ExternalUserDTO( + $this->create_external_id, + $this->reassigned_email, + 'Created', + 'User', + true, + true + ); + + App::make(IMemberService::class)->registerExternalUser($dto); + + $this->em->clear(); + + $owner_of_email = $this->member_repository->findOneBy(['email' => $this->reassigned_email]); + $this->assertNotNull($owner_of_email, "reassigned email should belong to a member"); + $this->assertEquals( + $this->create_external_id, + $owner_of_email->getUserExternalId(), + "reassigned email must now belong to the newly created external id" + ); + + $former_member = $this->member_repository->findOneBy(['user_external_id' => $this->old_external_id]); + $this->assertNotNull($former_member, "former member must still exist"); + $this->assertEquals( + sprintf("%s-invalid@invalid", $this->old_external_id), + $former_member->getEmail(), + "former member's email must be invalidated" + ); + } + private function buildExternalProfilePayload(int $external_id, string $email): array { return [