From ef87db6ba17f673afc5a34c2664d32549d97f824 Mon Sep 17 00:00:00 2001 From: Vincent Chalamon <407859+vincentchalamon@users.noreply.github.com> Date: Tue, 19 Mar 2024 21:07:21 +0100 Subject: [PATCH] fix: remove useless User.sub --- api/migrations/Version20230906094949.php | 4 +--- api/src/DataFixtures/Factory/ReviewFactory.php | 6 +++++- api/src/DataFixtures/Factory/UserFactory.php | 2 -- api/src/Entity/User.php | 8 -------- api/src/Security/Core/UserProvider.php | 10 ---------- api/tests/Api/Admin/BookTest.php | 2 +- api/tests/Api/Admin/UserTest.php | 7 +------ .../Api/Admin/schemas/Review/collection.json | 7 ------- api/tests/Api/Admin/schemas/Review/item.json | 7 ------- .../Api/Admin/schemas/User/collection.json | 7 ------- api/tests/Api/Admin/schemas/User/item.json | 7 ------- api/tests/Api/schemas/Review/collection.json | 7 ------- api/tests/Api/schemas/Review/item.json | 7 ------- api/tests/Security/Core/UserProviderTest.php | 10 +--------- .../Processor/ReviewPersistProcessorTest.php | 18 +++++++++++++++++- .../Processor/ReviewRemoveProcessorTest.php | 16 +++++++++++++++- 16 files changed, 41 insertions(+), 84 deletions(-) diff --git a/api/migrations/Version20230906094949.php b/api/migrations/Version20230906094949.php index 4c37dbb6d..de2268c6d 100644 --- a/api/migrations/Version20230906094949.php +++ b/api/migrations/Version20230906094949.php @@ -41,11 +41,9 @@ public function up(Schema $schema): void $this->addSql('COMMENT ON COLUMN review.user_id IS \'(DC2Type:uuid)\''); $this->addSql('COMMENT ON COLUMN review.book_id IS \'(DC2Type:uuid)\''); $this->addSql('COMMENT ON COLUMN review.published_at IS \'(DC2Type:datetime_immutable)\''); - $this->addSql('CREATE TABLE "user" (id UUID NOT NULL, sub UUID NOT NULL, email VARCHAR(255) NOT NULL, first_name VARCHAR(255) NOT NULL, last_name VARCHAR(255) NOT NULL, roles JSON NOT NULL, PRIMARY KEY(id))'); - $this->addSql('CREATE UNIQUE INDEX UNIQ_8D93D649580282DC ON "user" (sub)'); + $this->addSql('CREATE TABLE "user" (id UUID NOT NULL, email VARCHAR(255) NOT NULL, first_name VARCHAR(255) NOT NULL, last_name VARCHAR(255) NOT NULL, roles JSON NOT NULL, PRIMARY KEY(id))'); $this->addSql('CREATE UNIQUE INDEX UNIQ_8D93D649E7927C74 ON "user" (email)'); $this->addSql('COMMENT ON COLUMN "user".id IS \'(DC2Type:uuid)\''); - $this->addSql('COMMENT ON COLUMN "user".sub IS \'(DC2Type:uuid)\''); $this->addSql('ALTER TABLE bookmark ADD CONSTRAINT FK_DA62921DA76ED395 FOREIGN KEY (user_id) REFERENCES "user" (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); $this->addSql('ALTER TABLE bookmark ADD CONSTRAINT FK_DA62921D16A2B381 FOREIGN KEY (book_id) REFERENCES book (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); $this->addSql('ALTER TABLE review ADD CONSTRAINT FK_794381C6A76ED395 FOREIGN KEY (user_id) REFERENCES "user" (id) NOT DEFERRABLE INITIALLY IMMEDIATE'); diff --git a/api/src/DataFixtures/Factory/ReviewFactory.php b/api/src/DataFixtures/Factory/ReviewFactory.php index dca75a65e..4ee375c5b 100644 --- a/api/src/DataFixtures/Factory/ReviewFactory.php +++ b/api/src/DataFixtures/Factory/ReviewFactory.php @@ -54,7 +54,7 @@ final class ReviewFactory extends ModelFactory * @see https://symfony.com/bundles/ZenstruckFoundryBundle/current/index.html#factories-as-services */ public function __construct( - private readonly ResourceHandlerInterface $resourceHandler, + private readonly ?ResourceHandlerInterface $resourceHandler = null, ) { parent::__construct(); } @@ -81,6 +81,10 @@ protected function initialize(): self return $this // create the resource on the OIDC server ->afterPersist(function (Review $object): void { + if (!$this->resourceHandler) { + return; + } + // project specification: only create resource on OIDC server for known users (john.doe and chuck.norris) if (\in_array($object->user?->email, ['john.doe@example.com', 'chuck.norris@example.com'], true)) { $this->resourceHandler->create($object, $object->user, [ diff --git a/api/src/DataFixtures/Factory/UserFactory.php b/api/src/DataFixtures/Factory/UserFactory.php index b87c824e3..641114a55 100644 --- a/api/src/DataFixtures/Factory/UserFactory.php +++ b/api/src/DataFixtures/Factory/UserFactory.php @@ -6,7 +6,6 @@ use App\Entity\User; use Doctrine\ORM\EntityRepository; -use Symfony\Component\Uid\Uuid; use Zenstruck\Foundry\ModelFactory; use Zenstruck\Foundry\Proxy; use Zenstruck\Foundry\RepositoryProxy; @@ -76,7 +75,6 @@ public function withAdmin(): self protected function getDefaults(): array { return [ - 'sub' => Uuid::v7(), 'email' => self::faker()->unique()->email(), 'firstName' => self::faker()->firstName(), 'lastName' => self::faker()->lastName(), diff --git a/api/src/Entity/User.php b/api/src/Entity/User.php index 4535adc29..1543914f9 100644 --- a/api/src/Entity/User.php +++ b/api/src/Entity/User.php @@ -63,14 +63,6 @@ class User implements UserInterface #[ORM\Id] private ?Uuid $id = null; - /** - * @see https://schema.org/identifier - */ - #[ApiProperty(types: ['https://schema.org/identifier'])] - #[Groups(groups: ['User:read'])] - #[ORM\Column(type: UuidType::NAME, unique: true)] - public ?Uuid $sub = null; - /** * @see https://schema.org/email */ diff --git a/api/src/Security/Core/UserProvider.php b/api/src/Security/Core/UserProvider.php index 55b27e36e..c0c641649 100644 --- a/api/src/Security/Core/UserProvider.php +++ b/api/src/Security/Core/UserProvider.php @@ -10,7 +10,6 @@ use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\User\AttributesBasedUserProviderInterface; use Symfony\Component\Security\Core\User\UserInterface; -use Symfony\Component\Uid\Uuid; /** * @implements AttributesBasedUserProviderInterface @@ -46,15 +45,6 @@ public function loadUserByIdentifier(string $identifier, array $attributes = []) $user = $this->repository->findOneBy(['email' => $identifier]) ?: new User(); $user->email = $identifier; - if (!isset($attributes['sub'])) { - throw new UnsupportedUserException('Property "sub" is missing in token attributes.'); - } - try { - $user->sub = Uuid::fromString($attributes['sub']); - } catch (\Throwable $e) { - throw new UnsupportedUserException($e->getMessage(), $e->getCode(), $e); - } - if (!isset($attributes['given_name'])) { throw new UnsupportedUserException('Property "given_name" is missing in token attributes.'); } diff --git a/api/tests/Api/Admin/BookTest.php b/api/tests/Api/Admin/BookTest.php index 3773c2aaa..e2b0f6785 100644 --- a/api/tests/Api/Admin/BookTest.php +++ b/api/tests/Api/Admin/BookTest.php @@ -33,7 +33,7 @@ final class BookTest extends ApiTestCase protected function setup(): void { - $this->client = self::createClient(['debug' => true]); + $this->client = self::createClient(); } #[Test] diff --git a/api/tests/Api/Admin/UserTest.php b/api/tests/Api/Admin/UserTest.php index 067e6abd1..4aaf80eb6 100644 --- a/api/tests/Api/Admin/UserTest.php +++ b/api/tests/Api/Admin/UserTest.php @@ -12,7 +12,6 @@ use App\Tests\Api\Security\TokenGenerator; use PHPUnit\Framework\Attributes\DataProvider; use PHPUnit\Framework\Attributes\Test; -use Symfony\Component\Uid\Uuid; use Zenstruck\Foundry\FactoryCollection; use Zenstruck\Foundry\Test\Factories; use Zenstruck\Foundry\Test\ResetDatabase; @@ -27,7 +26,7 @@ final class UserTest extends ApiTestCase protected function setup(): void { - $this->client = self::createClient(); + $this->client = self::createClient(['debug' => true]); } #[Test] @@ -156,12 +155,9 @@ public function asAUserIAmUpdatedOnLogin(): void $user = UserFactory::createOne([ 'firstName' => 'John', 'lastName' => 'DOE', - 'sub' => Uuid::fromString('b5c5bff1-5b5f-4a73-8fc8-4ea8f18586a9'), ])->disableAutoRefresh(); - $sub = Uuid::v7()->__toString(); $token = self::getContainer()->get(TokenGenerator::class)->generateToken([ - 'sub' => $sub, 'email' => $user->email, 'given_name' => 'Chuck', 'family_name' => 'NORRIS', @@ -176,6 +172,5 @@ public function asAUserIAmUpdatedOnLogin(): void self::assertEquals('Chuck', $user->firstName); self::assertEquals('NORRIS', $user->lastName); self::assertEquals('Chuck NORRIS', $user->getName()); - self::assertEquals($sub, $user->sub); } } diff --git a/api/tests/Api/Admin/schemas/Review/collection.json b/api/tests/Api/Admin/schemas/Review/collection.json index 181231e03..6a1f327d4 100644 --- a/api/tests/Api/Admin/schemas/Review/collection.json +++ b/api/tests/Api/Admin/schemas/Review/collection.json @@ -131,12 +131,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -162,7 +156,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/Admin/schemas/Review/item.json b/api/tests/Api/Admin/schemas/Review/item.json index f2e6902e6..3f81816f5 100644 --- a/api/tests/Api/Admin/schemas/Review/item.json +++ b/api/tests/Api/Admin/schemas/Review/item.json @@ -38,12 +38,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -69,7 +63,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/Admin/schemas/User/collection.json b/api/tests/Api/Admin/schemas/User/collection.json index 915885e01..f94e49ad0 100644 --- a/api/tests/Api/Admin/schemas/User/collection.json +++ b/api/tests/Api/Admin/schemas/User/collection.json @@ -17,12 +17,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -48,7 +42,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/Admin/schemas/User/item.json b/api/tests/Api/Admin/schemas/User/item.json index 0c24d77f0..945d5b051 100644 --- a/api/tests/Api/Admin/schemas/User/item.json +++ b/api/tests/Api/Admin/schemas/User/item.json @@ -18,12 +18,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -50,7 +44,6 @@ "@context", "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/schemas/Review/collection.json b/api/tests/Api/schemas/Review/collection.json index 1779749f2..cfe83e86c 100644 --- a/api/tests/Api/schemas/Review/collection.json +++ b/api/tests/Api/schemas/Review/collection.json @@ -88,12 +88,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -119,7 +113,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Api/schemas/Review/item.json b/api/tests/Api/schemas/Review/item.json index 4578faf03..fb4c5716f 100644 --- a/api/tests/Api/schemas/Review/item.json +++ b/api/tests/Api/schemas/Review/item.json @@ -38,12 +38,6 @@ "type": "string", "pattern": "^https://schema.org/Person$" }, - "sub": { - "externalDocs": { - "url": "https:\/\/schema.org\/identifier" - }, - "type": "string" - }, "firstName": { "description": "The givenName of the person", "externalDocs": { @@ -69,7 +63,6 @@ "required": [ "@id", "@type", - "sub", "firstName", "lastName", "name" diff --git a/api/tests/Security/Core/UserProviderTest.php b/api/tests/Security/Core/UserProviderTest.php index e4eb1a69b..6e26887ab 100644 --- a/api/tests/Security/Core/UserProviderTest.php +++ b/api/tests/Security/Core/UserProviderTest.php @@ -15,7 +15,6 @@ use PHPUnit\Framework\TestCase; use Symfony\Component\Security\Core\Exception\UnsupportedUserException; use Symfony\Component\Security\Core\User\UserInterface; -use Symfony\Component\Uid\Uuid; final class UserProviderTest extends TestCase { @@ -102,12 +101,8 @@ public function itCannotLoadUserIfAttributeIsMissing(array $attributes): void public static function getInvalidAttributes(): iterable { - yield 'missing sub' => [[]]; - yield 'missing given_name' => [[ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', - ]]; + yield 'missing given_name' => [[]]; yield 'missing family_name' => [[ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', 'given_name' => 'John', ]]; } @@ -128,7 +123,6 @@ public function itLoadsUserFromAttributes(): void ; $this->assertSame($this->userMock, $this->provider->loadUserByIdentifier('john.doe@example.com', [ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', 'given_name' => 'John', 'family_name' => 'DOE', ])); @@ -140,7 +134,6 @@ public function itCreatesAUserFromAttributes(): void $expectedUser = new User(); $expectedUser->firstName = 'John'; $expectedUser->lastName = 'DOE'; - $expectedUser->sub = Uuid::fromString('ba86c94b-efeb-4452-a0b4-93ed3c889156'); $expectedUser->email = 'john.doe@example.com'; $this->repositoryMock @@ -156,7 +149,6 @@ public function itCreatesAUserFromAttributes(): void ; $this->assertEquals($expectedUser, $this->provider->loadUserByIdentifier('john.doe@example.com', [ - 'sub' => 'ba86c94b-efeb-4452-a0b4-93ed3c889156', 'given_name' => 'John', 'family_name' => 'DOE', ])); diff --git a/api/tests/State/Processor/ReviewPersistProcessorTest.php b/api/tests/State/Processor/ReviewPersistProcessorTest.php index 56d71c892..05dbfb46c 100644 --- a/api/tests/State/Processor/ReviewPersistProcessorTest.php +++ b/api/tests/State/Processor/ReviewPersistProcessorTest.php @@ -9,6 +9,7 @@ use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; use App\Entity\User; +use App\Security\Http\Protection\ResourceHandlerInterface; use App\State\Processor\ReviewPersistProcessor; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -25,6 +26,7 @@ final class ReviewPersistProcessorTest extends TestCase private MockObject|User $userMock; private MockObject|Review $objectMock; private ClockInterface|MockObject $clockMock; + private ResourceHandlerInterface|MockObject $resourceHandlerMock; private ReviewPersistProcessor $processor; protected function setUp(): void @@ -35,12 +37,14 @@ protected function setUp(): void $this->userMock = $this->createMock(User::class); $this->objectMock = $this->createMock(Review::class); $this->clockMock = new MockClock(); + $this->resourceHandlerMock = $this->createMock(ResourceHandlerInterface::class); $this->processor = new ReviewPersistProcessor( $this->persistProcessorMock, $this->mercureProcessorMock, $this->securityMock, - $this->clockMock + $this->clockMock, + $this->resourceHandlerMock ); } @@ -53,6 +57,7 @@ public function itUpdatesReviewDataFromOperationBeforeSaveAndSendMercureUpdates( $expectedData->user = $this->userMock; $expectedData->publishedAt = $this->clockMock->now(); + $this->userMock->email = 'john.doe@example.com'; $this->securityMock ->expects($this->once()) ->method('getUser') @@ -64,6 +69,13 @@ public function itUpdatesReviewDataFromOperationBeforeSaveAndSendMercureUpdates( ->with($expectedData, $operation, [], []) ->willReturn($expectedData) ; + $this->resourceHandlerMock + ->expects($this->once()) + ->method('create') + ->with($expectedData, $this->userMock, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]) + ; $this->mercureProcessorMock ->expects($this->exactly(2)) ->method('process') @@ -105,6 +117,10 @@ public function itUpdatesReviewDataFromContextBeforeSaveAndSendMercureUpdates(): ->with($expectedData, $operation, [], $context) ->willReturn($expectedData) ; + $this->resourceHandlerMock + ->expects($this->never()) + ->method('create') + ; $this->mercureProcessorMock ->expects($this->exactly(2)) ->method('process') diff --git a/api/tests/State/Processor/ReviewRemoveProcessorTest.php b/api/tests/State/Processor/ReviewRemoveProcessorTest.php index cfd60bc76..acedcd2dd 100644 --- a/api/tests/State/Processor/ReviewRemoveProcessorTest.php +++ b/api/tests/State/Processor/ReviewRemoveProcessorTest.php @@ -12,6 +12,8 @@ use ApiPlatform\Metadata\Resource\ResourceMetadataCollection; use ApiPlatform\State\ProcessorInterface; use App\Entity\Review; +use App\Entity\User; +use App\Security\Http\Protection\ResourceHandlerInterface; use App\State\Processor\ReviewRemoveProcessor; use PHPUnit\Framework\Attributes\Test; use PHPUnit\Framework\MockObject\MockObject; @@ -26,6 +28,7 @@ final class ReviewRemoveProcessorTest extends TestCase private IriConverterInterface|MockObject $iriConverterMock; private MockObject|Review $objectMock; private MockObject|Operation $operationMock; + private ResourceHandlerInterface|MockObject $resourceHandlerMock; private ReviewRemoveProcessor $processor; protected function setUp(): void @@ -33,6 +36,7 @@ protected function setUp(): void $this->removeProcessorMock = $this->createMock(ProcessorInterface::class); $this->mercureProcessorMock = $this->createMock(ProcessorInterface::class); $this->resourceMetadataCollectionFactoryMock = $this->createMock(ResourceMetadataCollectionFactoryInterface::class); + $this->resourceHandlerMock = $this->createMock(ResourceHandlerInterface::class); $this->resourceMetadataCollection = new ResourceMetadataCollection(Review::class, [ new ApiResource(operations: [new Get('/admin/reviews/{id}{._format}')]), new ApiResource(operations: [new Get('/books/{bookId}/reviews/{id}{._format}')]), @@ -45,7 +49,8 @@ protected function setUp(): void $this->removeProcessorMock, $this->mercureProcessorMock, $this->resourceMetadataCollectionFactoryMock, - $this->iriConverterMock + $this->iriConverterMock, + $this->resourceHandlerMock ); } @@ -81,6 +86,15 @@ public function itRemovesBookAndSendMercureUpdates(): void '/books/8ad70d36-abaf-4c9b-aeaa-7ec63e6ca6f3/reviews/9aff4b91-31cf-4e91-94b0-1d52bbe23fe6', ) ; + $this->objectMock->user = $this->createMock(User::class); + $this->objectMock->user->email = 'john.doe@example.com'; + $this->resourceHandlerMock + ->expects($this->once()) + ->method('delete') + ->with($this->objectMock, $this->objectMock->user, [ + 'operation_name' => '/books/{bookId}/reviews/{id}{._format}', + ]) + ; $this->mercureProcessorMock ->expects($this->exactly(2)) ->method('process')