From 7977d54b31a9f0b5e753fef376858cf972db9747 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 21 Oct 2026 18:30:39 +0200 Subject: [PATCH 1/3] test(members): add regression tests for IDP member sync with non-latin1 company and concurrent creation Company outside latin1 (full-width parentheses, Latin Extended) must be stored as sent by the IDP, and registerExternalUserById() must retry when another process creates the member between the lookups and the insert. All three fail on the current code. --- tests/MemberServiceTest.php | 110 +++++++++++++++++++++++++++++++++++- 1 file changed, 109 insertions(+), 1 deletion(-) 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 [ From a9fff6e1a6515cee536c392a1d4c348431ba8274 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 21 Oct 2026 18:33:36 +0200 Subject: [PATCH 2/3] fix(members): convert Member.Company to utf8mb4 so the IDP sync stores any company name Company was latin1, so a company name with a character outside latin1 made the write fail with 1366 in strict mode and rolled back the whole member sync. latin1 is a subset of utf8mb4, so existing values convert unchanged. The ALTER copies the table and blocks writes to Member while it runs. --- .../model/Version20261021170000.php | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 database/migrations/model/Version20261021170000.php 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 + { + + } +} From 878dbf08cc2c37c1c23510041a66e432436f2e00 Mon Sep 17 00:00:00 2001 From: romanetar Date: Wed, 21 Oct 2026 18:33:36 +0200 Subject: [PATCH 3/3] fix(members): retry the IDP member sync once on a concurrent member creation When another process creates the same member between the lookups and the insert, the unique ExternalUserId or Email key rejects the insert and the message was lost. Run the sync again in a new transaction so it finds and updates that member, as the login path does. --- app/Services/Model/Imp/MemberService.php | 39 ++++++++++++++++++------ 1 file changed, 30 insertions(+), 9 deletions(-) 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()));