diff --git a/app/Services/Model/Imp/MemberService.php b/app/Services/Model/Imp/MemberService.php index 60a57cff0..6995271fb 100644 --- a/app/Services/Model/Imp/MemberService.php +++ b/app/Services/Model/Imp/MemberService.php @@ -21,6 +21,7 @@ use App\Services\Apis\IExternalUserApi; use App\Services\Model\dto\ExternalUserDTO; use DateTime; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Event; use Illuminate\Support\Facades\Log; @@ -454,15 +455,35 @@ public function registerExternalUserByPayload(array $user_data):Member{ */ public function registerExternalUserById($user_external_id): Member { - $member = $this->tx_service->transaction(function () use ($user_external_id) { - // get external user from IDP - $user_data = $this->user_ext_api->getUserById($user_external_id); - if(is_null($user_data) || !isset($user_data['email'])){ - Log::warning(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id)); - throw new EntityNotFoundException(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id)); - } - return $this->registerExternalUserByPayload($user_data); - }); + $register = function () use ($user_external_id) { + return $this->tx_service->transaction(function () use ($user_external_id) { + // get external user from IDP + $user_data = $this->user_ext_api->getUserById($user_external_id); + if(is_null($user_data) || !isset($user_data['email'])){ + Log::warning(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id)); + throw new EntityNotFoundException(sprintf("MemberService::registerExternalUserById user_external_id %s does not exists.", $user_external_id)); + } + return $this->registerExternalUserByPayload($user_data); + }); + }; + + try { + $member = $register(); + } catch (UniqueConstraintViolationException $ex) { + // another process (login or a concurrent IDP message) created the same member between + // our lookups and our insert (READ COMMITTED takes no gap lock), the retry finds it and updates it. + // Both Email and ExternalUserId are unique keys, so either one can be the reported one + Log::warning + ( + sprintf + ( + "MemberService::registerExternalUserById user_external_id %s concurrent member creation, retrying once: %s", + $user_external_id, + $ex->getMessage() + ) + ); + $member = $register(); + } Event::dispatch(new MemberDataUpdatedExternally($member->getId())); diff --git a/database/migrations/model/Version20261021170000.php b/database/migrations/model/Version20261021170000.php new file mode 100644 index 000000000..c0b557add --- /dev/null +++ b/database/migrations/model/Version20261021170000.php @@ -0,0 +1,28 @@ +addSql($sql); + } + + public function down(Schema $schema): void + { + + } +} diff --git a/tests/MemberServiceTest.php b/tests/MemberServiceTest.php index c457f4612..7a7250cbf 100644 --- a/tests/MemberServiceTest.php +++ b/tests/MemberServiceTest.php @@ -11,14 +11,18 @@ * See the License for the specific language governing permissions and * limitations under the License. **/ +use App\Services\Apis\IExternalUserApi; use App\Services\Model\dto\ExternalUserDTO; use App\Services\Model\IMemberService; +use Doctrine\DBAL\Exception\UniqueConstraintViolationException; use Illuminate\Support\Facades\App; use Illuminate\Support\Facades\DB; use LaravelDoctrine\ORM\Facades\EntityManager; use LaravelDoctrine\ORM\Facades\Registry; use libs\utils\ITransactionService; +use Mockery; use models\main\Group; +use models\main\IMemberRepository; use models\main\Member; use models\utils\SilverstripeBaseModel; @@ -50,6 +54,25 @@ final class MemberServiceTest extends TestCase /** @var string */ private $reassigned_email; + /** @var string */ + private $new_email; + + /** @var \Mockery\MockInterface */ + private $external_user_api; + + public function createApplication() + { + $app = parent::createApplication(); + + // mock the IDP user API before any service singleton is resolved + $this->external_user_api = Mockery::mock(IExternalUserApi::class)->shouldIgnoreMissing(); + App::singleton(IExternalUserApi::class, function () { + return $this->external_user_api; + }); + + return $app; + } + protected function setUp(): void { parent::setUp(); @@ -81,7 +104,8 @@ protected function setUp(): void // member already provisioned for the new external id, with a different email $new_member = new Member(); - $new_member->setEmail(strtolower("new+{$prefix}@test.com")); + $this->new_email = strtolower("new+{$prefix}@test.com"); + $new_member->setEmail($this->new_email); $new_member->setActive(true); $new_member->setEmailVerified(true); $new_member->setFirstName("New"); @@ -268,6 +292,90 @@ public function testSynchronizeGroupsAdditiveModePreservesOutOfBandGroups(): voi } } + /** + * Company is stored in a column that must hold any character the IDP sends, such as the + * full-width parentheses. Production runs MySQL in strict mode and rejects the write (1366), + * CI is non-strict and silently stores "?" in place of the characters latin1 lacks, so the + * test asserts the value read back from the database. + * + * @dataProvider companyOutsideLatin1Provider + */ + public function testRegisterExternalUserByPayloadStoresCompanyOutsideLatin1(string $company): void + { + $payload = $this->buildExternalProfilePayload($this->new_external_id, $this->new_email); + $payload['company'] = $company; + $payload['first_name'] = 'Renamed'; + + $tx_service = App::make(ITransactionService::class); + $tx_service->transaction(function () use ($payload) { + return App::make(IMemberService::class)->registerExternalUserByPayload($payload); + }); + + $this->em->clear(); + + $member = $this->member_repository->findOneBy(['user_external_id' => $this->new_external_id]); + $this->assertNotNull($member, "member must still exist"); + $this->assertEquals($company, $member->getCompany(), "company must be stored as sent by the IDP"); + $this->assertEquals('Renamed', $member->getFirstName(), "first name must be updated"); + } + + public static function companyOutsideLatin1Provider(): array + { + return [ + 'full-width parentheses' => ['(ACME)'], + 'latin extended' => ['Łódź Inc'], + ]; + } + + /** + * Race: another process created the member of this IDP user between our lookups and our + * insert, so both lookups miss (READ COMMITTED takes no gap lock) and the insert collides + * with the unique ExternalUserId / Email keys. The sync must retry once and update the + * member the other process created, as the login path does. + */ + public function testRegisterExternalUserByIdRetriesWhenMemberWasCreatedConcurrently(): void + { + $payload = $this->buildExternalProfilePayload($this->new_external_id, $this->new_email); + $payload['first_name'] = 'Raced'; + + $this->external_user_api->shouldReceive('getUserById') + ->with($this->new_external_id) + ->andReturn($payload); + + // double of the member repository: the first lookups by external id and by email miss + // (the member is not visible yet), every other call is forwarded to the real repository + $real_repository = EntityManager::getRepository(Member::class); + $repository = Mockery::mock(IMemberRepository::class); + $repository->shouldReceive('getByExternalIdExclusiveLock')->once()->andReturn(null); + $repository->shouldReceive('getByEmail')->once()->andReturn(null); + foreach ((new \ReflectionClass(IMemberRepository::class))->getMethods() as $method) { + $name = $method->getName(); + $repository->shouldReceive($name)->andReturnUsing( + function (...$args) use ($real_repository, $name) { + return $real_repository->$name(...$args); + } + ); + } + App::singleton(IMemberRepository::class, function () use ($repository) { + return $repository; + }); + // the service singleton may already be resolved with the real repository + App::forgetInstance(IMemberService::class); + + try { + App::make(IMemberService::class)->registerExternalUserById($this->new_external_id); + } catch (UniqueConstraintViolationException $ex) { + $this->fail(sprintf("the sync must retry on a concurrent member creation: %s", $ex->getMessage())); + } + + $this->em = Registry::resetManager(SilverstripeBaseModel::EntityManager); + $this->member_repository = EntityManager::getRepository(Member::class); + + $members = $this->member_repository->findBy(['user_external_id' => $this->new_external_id]); + $this->assertCount(1, $members, "the existing member must be reused, not duplicated"); + $this->assertEquals('Raced', $members[0]->getFirstName(), "the existing member must be updated"); + } + private function buildExternalProfilePayload(int $external_id, string $email): array { return [