diff --git a/docs/api-consumer-guide.md b/docs/api-consumer-guide.md index 953dbc2..63e4d11 100644 --- a/docs/api-consumer-guide.md +++ b/docs/api-consumer-guide.md @@ -380,7 +380,7 @@ Responses: OIDC-style claims for the authenticated user. The response contains only the claims covered by the granted scopes: -- always: `sub` (string), `email` +- always: `sub` (UUID string, see below), `email` - scope `id`: `person_id`, `address_id` - scope `roles`: `roles` (array; only the roles that actually grant something are exported — the implicit baseline role is stripped, and so are the `*_PENDING` markers of roles the BusPro CRM claims but nobody has approved yet, see `docs/buspronet-schema/crm-selection-queries.md#from-claim-to-role`) - scope `profile`: `profile` object: @@ -407,8 +407,10 @@ All of those accounts report the **same** `person_id` and `address_id`, and the `profile.communication.email` — that value is the first contact address on the BusPro record and is **not necessarily** the one signed in with. So: -- **`sub`** — opaque, stable, unique per MyE&P account. The only claim that identifies an account. - Match your local user on it and store it. +- **`sub`** — a **UUID (v4, RFC 4122, 36 characters)**, stable for the life of the account and + unique across accounts. The only claim that identifies an account. Match your local user on it + and store it. Size the column at 36 characters and treat the value as opaque: it is random, it + carries no timestamp and no ordering, and nothing about it should be parsed or derived from. - **`email`** — the address this session actually authenticated with. Distinct per account, but treat it as a display and contact value; it is not the account key. - **`person_id` / `address_id`** — the BusPro person behind the account. Shared between that diff --git a/migrations/Version20260923150000.php b/migrations/Version20260923150000.php new file mode 100644 index 0000000..ec075eb --- /dev/null +++ b/migrations/Version20260923150000.php @@ -0,0 +1,51 @@ +addSql('ALTER TABLE user ADD uuid VARCHAR(36) DEFAULT NULL'); + + foreach ($this->connection->fetchFirstColumn('SELECT id FROM user') as $id) { + $this->addSql('UPDATE user SET uuid = :uuid WHERE id = :id', [ + 'uuid' => Uuid::v4()->toRfc4122(), + 'id' => $id, + ]); + } + + $this->addSql('ALTER TABLE user MODIFY uuid VARCHAR(36) NOT NULL'); + $this->addSql('CREATE UNIQUE INDEX UNIQ_8D93D649D17F50A6 ON user (uuid)'); + } + + public function down(Schema $schema): void + { + $this->addSql('DROP INDEX UNIQ_8D93D649D17F50A6 ON user'); + $this->addSql('ALTER TABLE user DROP uuid'); + } +} diff --git a/src/Controller/Api/UserinfoController.php b/src/Controller/Api/UserinfoController.php index 077ca0e..175d745 100644 --- a/src/Controller/Api/UserinfoController.php +++ b/src/Controller/Api/UserinfoController.php @@ -58,7 +58,11 @@ class UserinfoController extends AbstractController // addresses on a person's record as a login and answers all of them with the same // ids and the same first contact address, so only the local account tells the staff // account and the private one apart — and they hold different roles. - $data->subject = (string) $user->getId(); + // + // The uuid, not the primary key: consumers store this value and match their own + // accounts on it, so it must not disclose how many accounts exist or let one + // account's identifier be guessed from another's. + $data->subject = $user->getUuid(); $data->loginEmail = $user->getEmail(); // Patch current user's roles. The implicit ROLE_USER says nothing about the diff --git a/src/Entity/User.php b/src/Entity/User.php index 794e4b0..b6ff905 100644 --- a/src/Entity/User.php +++ b/src/Entity/User.php @@ -6,6 +6,7 @@ use App\Repository\UserRepository; use Doctrine\ORM\Mapping as ORM; use Symfony\Component\Security\Core\User\PasswordAuthenticatedUserInterface; use Symfony\Component\Security\Core\User\UserInterface; +use Symfony\Component\Uid\Uuid; #[ORM\Entity(repositoryClass: UserRepository::class)] class User implements UserInterface, PasswordAuthenticatedUserInterface @@ -15,6 +16,22 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface #[ORM\Column(type: 'integer')] private ?int $id = null; + /** + * The account's external identifier, exported as the OAuth2 `sub` claim. + * + * Separate from the primary key on purpose: `sub` is handed to other applications, which + * store it and match their own accounts on it, so it has to be stable, unique and free of + * anything the row's position in the table implies. The auto-increment id satisfied the + * first two and neither of the last: it disclosed how many accounts exist and made one + * account's identifier a guess away from the next. + * + * v4 rather than the framework's default v7 (see config/packages/uid.yaml): v7 embeds its + * creation timestamp and sorts, which is useful for a key and is exactly the leak this + * column exists to avoid. Matches Groups\AccommodationBooking::$uuid in shape and version. + */ + #[ORM\Column(type: 'string', length: 36, unique: true)] + private string $uuid; + #[ORM\Column(type: 'string', length: 180, unique: true)] private ?string $email; @@ -54,6 +71,7 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface public function __construct(string $email) { $this->email = $email; + $this->uuid = Uuid::v4()->toRfc4122(); } public function getId(): ?int @@ -61,6 +79,11 @@ class User implements UserInterface, PasswordAuthenticatedUserInterface return $this->id; } + public function getUuid(): string + { + return $this->uuid; + } + public function getEmail(): ?string { return $this->email; diff --git a/tests/Controller/Api/UserinfoControllerTest.php b/tests/Controller/Api/UserinfoControllerTest.php index 082d505..60d41d1 100644 --- a/tests/Controller/Api/UserinfoControllerTest.php +++ b/tests/Controller/Api/UserinfoControllerTest.php @@ -27,13 +27,14 @@ class UserinfoControllerTest extends TestCase { private const LOGIN_EMAIL = 'someone@ep-reisen.de'; private const CONTACT_EMAIL = 'private@example.test'; + private const UUID = '018f3c2a-7b4d-4e91-a3c5-6d2f8b1e4a07'; public function testSubAndEmailAreExportedWithoutAnyOptionalScope(): void { $claims = $this->claims(); self::assertSame(['sub', 'email'], array_keys($claims)); - self::assertSame('42', $claims['sub']); + self::assertSame(self::UUID, $claims['sub']); self::assertSame(self::LOGIN_EMAIL, $claims['email']); } @@ -43,11 +44,27 @@ class UserinfoControllerTest extends TestCase // The ids identify the human, the subject identifies the account. A second account of the // same person reports these same two ids and must still be told apart. - self::assertSame('42', $claims['sub']); + self::assertSame(self::UUID, $claims['sub']); self::assertSame(7, $claims['person_id']); self::assertSame(9, $claims['address_id']); } + /** + * Consumers store `sub` and match their own accounts on it, so it must not disclose how many + * accounts exist or let one account's identifier be guessed from the next. The primary key + * did both. + */ + public function testSubIsTheAccountUuidRatherThanItsPrimaryKey(): void + { + $claims = $this->claims(); + + self::assertNotSame('42', $claims['sub']); + self::assertMatchesRegularExpression( + '/^[0-9a-f]{8}-[0-9a-f]{4}-4[0-9a-f]{3}-[89ab][0-9a-f]{3}-[0-9a-f]{12}$/', + $claims['sub'] + ); + } + public function testEmailIsTheAuthenticatedAddressAndNotTheFirstContactOnTheBusProRecord(): void { $claims = $this->claims(['ROLE_OAUTH2_PROFILE']); @@ -160,10 +177,15 @@ class UserinfoControllerTest extends TestCase ->setRoles($roles) ; - // The id is generated by Doctrine and has no setter, but it is what `sub` exports. + // The id is generated by Doctrine and has no setter. It is deliberately not what `sub` + // exports -- see testSubIsTheAccountUuidRatherThanItsPrimaryKey. $property = new \ReflectionProperty(User::class, 'id'); $property->setValue($user, 42); + // The constructor generates a fresh uuid; pin it so the claim can be asserted literally. + $property = new \ReflectionProperty(User::class, 'uuid'); + $property->setValue($user, self::UUID); + return $user; } diff --git a/tests/Entity/UserTest.php b/tests/Entity/UserTest.php new file mode 100644 index 0000000..45910f5 --- /dev/null +++ b/tests/Entity/UserTest.php @@ -0,0 +1,44 @@ +getUuid() + ); + } + + /** + * v4, not the framework's default v7: a v7 would sort by creation time and carry that + * timestamp in the claim, which is the disclosure the column exists to avoid. + */ + public function testUuidsAreRandomRatherThanOrdered(): void + { + $uuids = []; + for ($i = 0; $i < 50; ++$i) { + $uuids[] = (new User("someone{$i}@ep-reisen.de"))->getUuid(); + } + + self::assertCount(50, array_unique($uuids)); + + $sorted = $uuids; + sort($sorted); + self::assertNotSame($sorted, $uuids, 'v4 uuids must not come out in creation order'); + } +}