diff --git a/app/Models/Foundation/Summit/ScheduleEntity.php b/app/Models/Foundation/Summit/ScheduleEntity.php index 7d735a848..867b9578e 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 bfc46a25d..a853fe400 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 68641c68a..862c6bacd 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 4c3955973..34b2a92d9 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()); @@ -342,11 +350,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 000000000..d92773688 --- /dev/null +++ b/tests/MemberServiceTest.php @@ -0,0 +1,207 @@ +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, 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(); + $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, $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); + } + } + $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" + ); + } + + /** + * 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 [ + '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, + ]; + } +}