issue-1931 nameid lookup api by kayjoosten · Pull Request #1959 · OpenConext/OpenConext-engineblock

kayjoosten · 2026-03-31T06:34:00Z

No description provided.

Two new endpoints on the internal API: - POST /info/users/nameid — forward lookup (sho + uid + sp_entityid → nameid) - POST /info/users/id — reverse lookup (nameid → sho + uid + sp_entityid) Both require ROLE_API_USER_NAMEID_LOOKUP and are feature-flag gated.

johanib · 2026-04-08T07:23:37Z

config/packages/ci/parameters.yml

+    api.users.nameIdLookup.username: nameid
+    api.users.nameIdLookup.password: secret
+    feature_api_users_nameid: true
+    feature_api_users_id: true


The original ticket does not mention feature flags. I see the deprovision controller does also use a feature flag. Did you check with Bas if we want feature flags?

I think it fits, but would be good to doublecheck.
Also, one or two feature flags?

I have discussed with bas and we have decided that 1 feature flag will be enough so ill change that

config/services/services.yml

migrations/DoctrineMigrations/Version20260331000000.php

johanib · 2026-04-08T07:47:22Z

src/OpenConext/EngineBlock/Service/NameIdLookupService.php

+
+    public function resolveNameId(string $schacHomeOrganization, string $uid, string $spEntityId): ?array
+    {
+        $collabPersonId = new CollabPersonId(sprintf(


Use \OpenConext\EngineBlock\Authentication\Value\CollabPersonId::generateWithReplacedAtSignFrom instead?

johanib · 2026-04-08T07:56:04Z

src/OpenConext/EngineBlock/Service/NameIdLookupService.php

+
+        $user = $this->userRepository->findByCollabPersonId($collabPersonId);
+        if ($user === null) {
+            $this->logger->debug('NameIdLookupService: user not found', [


debating whether a service should log.

Also, because you return an array, I think it would be nicer to return a VO, that can also return a not-found reason or something.

Because the service in this context does not know what is happening. In this context (the api lookup), you might want this level of logging.
But in other theoretical context, it might be totally expected to not find the actual user, and the logs are just noise.

Otoh, this is how other services log.. So for consistency, OK to leave it in.

johanib · 2026-04-08T08:49:47Z

src/OpenConext/EngineBlock/Service/NameIdLookupService.php

+
+        $parts = explode(':', $user->collabPersonId->getCollabPersonId(), 5);
+        $schacHomeOrganization = $parts[3] ?? '';
+        $uid = $parts[4] ?? '';


Should it fallback to ''? It should fail if something fails that should not happen.

johanib · 2026-04-08T08:52:06Z

src/OpenConext/EngineBlockBundle/Authentication/Repository/SamlPersistentIdRepository.php

+    {
+        return $this->findOneBy([
+            'userUuid' => $userUuid,
+            'serviceProviderUuid' => $serviceProviderUuid,


Nitpick: Not sure if theoretical, but what if two matching records exist? It would return a random of the existing records.

You could argue that db constraints should prevent that from occuring if that is the constraint, so ok to leave it as is.

Duplicates for the same user+SP combination are impossible by design: persistentId is the primary key and is computed as sha1('COIN:' + userUuid + serviceProviderUuid). The same pair always produces the same SHA1, so the DB would reject a duplicate insert. findOneBy is safe here.

johanib · 2026-04-08T09:01:32Z

src/OpenConext/EngineBlockBundle/Authentication/Repository/UserRepository.php

+    {
+        return $this
+            ->createQueryBuilder('u')
+            ->where('u.collabPersonUuid = :uuid')


This is indexed, but not a unique key in the db 🤔

You're right, idx_user_uuid on the user table is a regular index, not a UNIQUE constraint.

However, UUIDs are generated via CollabPersonUuid::generate() which uses Uuid::uuid4(). Collisions are astronomically unlikely, and the getOneOrNullResult() call would throw a NonUniqueResultException if it ever happened, making the problem visible rather than silent.

This is an application-level guarantee, same as the existing findByCollabPersonId method in the same repository which follows the same pattern.

Adding a UNIQUE constraint is a separate concern outside this PR's scope i think. What do you think?

johanib · 2026-04-08T09:11:00Z

src/OpenConext/EngineBlockBundle/Controller/Api/UserController.php

+
+        $this->assertAuthorized();
+
+        $entries = $this->decodeJsonArray($request);


I see the supplied python example supplies multiple ids, and returns multiple ids. And is a POST.

Since this is a lookup, it would be much nicer to:

Make it GET

Accept three named route parameters.

It's not just the the code that would become better, but in general, you search for one thing at a time.
But I'm not sure what the context is. If this is a 'batch' endpoint, we can rename it as such.

@baszoetekouw Is that an option, from a consumer perspective?
In summary: Should this route accept multiple lookups per request?

I have discussed this with bas as well the batch endpoints are intended

johanib · 2026-04-08T09:23:34Z

tests/unit/OpenConext/EngineBlock/Service/NameIdLookupServiceTest.php

+use Psr\Log\LoggerInterface;
+use Ramsey\Uuid\Uuid;
+
+final class NameIdLookupServiceTest extends TestCase


The service doesn't really do anything, except tie together other parts. Especially after the relevant business logic is extracted to the correct places.
So, this test just tests the implementation details.

I'd be ok with dropping it.

kayjoosten force-pushed the feature/1931-nameid-lookup-api branch from 71be884 to 2ed58fd Compare March 31, 2026 06:34

kayjoosten requested a review from johanib March 31, 2026 06:34

kayjoosten linked an issue Mar 31, 2026 that may be closed by this pull request

Add internal API to look up NameID for specific user/SP #1931

Open

kayjoosten changed the title ~~Feature/1931 nameid lookup api~~ issue-1931 nameid lookup api Mar 31, 2026

test: add unit, functional and integration tests for NameID lookup API

6367c74

kayjoosten force-pushed the feature/1931-nameid-lookup-api branch from 2ed58fd to 6367c74 Compare March 31, 2026 08:41

johanib requested changes Apr 8, 2026

View reviewed changes

Comments

c074c69

kayjoosten force-pushed the feature/1931-nameid-lookup-api branch from 9b34a40 to c074c69 Compare April 9, 2026 10:01

kayjoosten requested a review from johanib April 10, 2026 07:47


		$this->assertAuthorized();

		$entries = $this->decodeJsonArray($request);

Conversation

kayjoosten commented Mar 31, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants