diff --git a/CLAUDE.md b/CLAUDE.md index 283cbf0..8b6bac1 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -23,7 +23,7 @@ PHP 8.1+ is required (CI runs 8.1 and 8.2). Use the `8.1`/`8.2` shell switchers - Dependency analysis: `vendor/bin/composer-dependency-analyser` (no composer script; see `composer-dependency-analyser.php`) - Rector (dry-run): `vendor/bin/rector process --dry-run` (config in `rector.php`, `withPhpSets(php81: true)`; not run in CI) -CI (`.github/workflows/build.yaml`) additionally runs `composer validate --strict` and `composer normalize --dry-run` across the PHP 8.1/8.2 × Symfony 6.4/7.0 matrix. +CI (`.github/workflows/build.yaml`) additionally runs `composer validate --strict` and `composer normalize --dry-run` across the PHP 8.1/8.2 × Symfony 6.4/7.0 matrix. Pull requests also run Roave's BackwardCompatibilityCheck against their base branch (`.github/workflows/backwards-compatibility-check.yaml`). > Local note (PHP 8.4): the transitive dev dep `thecodingmachine/safe` emits a flood of implicit-nullable `E_DEPRECATED` notices on PHP 8.4 (they don't exist on CI's 8.1/8.2). This is harmless for PHPStan/PHPUnit but breaks Infection's initial test run — run it with `vendor/bin/infection --initial-tests-php-options="-d error_reporting=24575"` locally on 8.4. @@ -35,8 +35,9 @@ The core abstraction is `ClientContextInterface::getClient(): Client`. Controlle Three concerns are each built as a **Symfony service-decoration chain** in `config/services.xml`. Higher `decoration-priority` is applied first and ends up *innermost*; the public alias resolves to the outermost decorator. Understanding the resolution order is essential before changing any service wiring: -- **Client context** — runtime order outermost→innermost: `CachedClientContext` (prio 32) → `CookieBasedClientContext` (prio 64) → `DefaultClientContext`. +- **Client context** — runtime order outermost→innermost: `CachedClientContext` (prio 32) → `PendingMetadataClientContext` (prio 48) → `CookieBasedClientContext` (prio 64) → `DefaultClientContext`. - `Cached` memoizes the `Client` for the request. It implements `ResetInterface` and is tagged `kernel.reset`, so long-running runtimes (FrankenPHP worker mode, RoadRunner) don't hand one visitor's client to the next request; `tests/Functional/LongRunningProcessTest.php` covers this. Any new service that holds per-request state needs the same, unless it keys the state by the request object like `CachedCookieProvider`. + - `PendingMetadata`: if the main request carries pending metadata signed for the client's id and it has keys (not expired) that the client's metadata lacks (`toArray()` initializes the lazy ghost), returns a new `Client` whose `ChangeAwareMetadata` holds the client's metadata plus those keys with their expiry timestamps, marked dirty so the persister writes it. Keys the client has win, because a stored row was written from metadata that already held the pending metadata (e.g. by a concurrent request) or by the app outside the client's requests. - `CookieBased` reads the cookie; if present, builds `Client(cookieId, metadataProvider->getMetadata(...))`; otherwise delegates. - `Default` returns an anonymous `new Client(null, new ChangeAwareMetadata())`. - **Cookie provider** — `CachedCookieProvider` (prio 32, memoizes per `Request` in a `WeakMap` keyed by the request object, so it needs no reset; not `spl_object_hash()`, which PHP hands to a new request once the old one is freed) → `RequestBasedCookieProvider` (parses the cookie off the request). The test kernel keeps its requests alive, so only `CachedCookieProviderTest` can reproduce that handle reuse. @@ -44,13 +45,23 @@ Three concerns are each built as a **Symfony service-decoration chain** in `conf ### Request lifecycle -- `EventSubscriber/StoreCookieSubscriber` (on `kernel.response`): dispatches `Event/PreStoreCookieEvent` (an app can set `$store = false` to veto), then writes/refreshes the cookie with an updated `lastSeenAt`. The cookie is set `HttpOnly(false)` on purpose so client-side JS can read it. +- `EventSubscriber/StoreCookieSubscriber` (on `kernel.response`, priority -64 so metadata set by default-priority response listeners is included, and before the profiler at -100): dispatches `Event/PreStoreCookieEvent` (an app can set `$store = false` to veto), then writes/refreshes the cookie with an updated `lastSeenAt`. The cookie is set `HttpOnly(false)` on purpose so client-side JS can read it. It also handles pending metadata (below). - `EventSubscriber/StoreMetadataSubscriber` (on `kernel.finish_request`): calls the metadata persister for the current client. ### Lazy metadata (why writes are cheap) `Client/ChangeAwareMetadata` extends `setono/client`'s `Metadata` and flips a `dirty` flag on `set`/`remove`. `Client/LazyChangeAwareMetadata` adds Symfony VarExporter's `LazyGhostTrait`, so `DoctrineOrmBasedMetadataProvider` returns a lazy ghost whose DB query only fires on first access. `DoctrineOrmBasedMetadataPersister::persist()` then **short-circuits** in two cases: the metadata is a lazy ghost that was never initialized (never read), or it is not dirty (never mutated). This is what makes "lazy loaded metadata" in the README real — no SELECT and no flush unless the app touched metadata. +### Pending metadata (new clients) + +A *new* client is one whose request has no id cookie. Its metadata is not persisted until it comes back, so clients that never return (crawlers, cookie blockers) never produce a row (issue #8): + +- `StoreCookieSubscriber`, for a new client: dirty `ChangeAwareMetadata` goes to `PendingMetadataStorage/PendingMetadataStorageInterface::store()` and is then `markClean()`ed, so `StoreMetadataSubscriber` skips it. If `store()` returns `false` (too big / not JSON encodable) it stays dirty and is persisted right away; anything set after the subscriber ran makes it dirty again and is persisted right away too. On veto, a new client's metadata is `markClean()`ed (discarded), because the client never receives its id. +- `StoreCookieSubscriber`, when the cookie is stored: always calls `remove()`, which clears the pending cookie if the request carried one (a `store()` in the same response replaces the clearing cookie). +- `PendingMetadataClientContext` adopts the pending metadata on the return request (see above). +- `CookieBasedPendingMetadataStorage` (default, alias `setono_client.pending_metadata_storage.default`): cookie `_pending_metadata`, value `.` signed with `%kernel.secret%` over a context string + client id + JSON, `HttpOnly`, `Secure` on HTTPS, same expiration as the id cookie, max 2048 bytes URL-encoded. JSON flags mirror Doctrine's json type (`JSON_PRESERVE_ZERO_FRACTION`). +- `tests/Functional/NewClientMetadataTest.php` shows the whole flow in the test app, whose `GoogleClickIdSubscriber` sets metadata on `kernel.response` before (`gclid`) and after (`late_gclid`) `StoreCookieSubscriber`. Functional tests that check what a request stored call `FunctionalTestCase::becomeReturningVisitor()` first, because a new visitor's metadata only reaches the database when it comes back. + ### Entity mapping `Entity/Metadata` (table `setono_client__metadata`, string id `clientId`, nullable `json` `metadata`) is mapped in `config/doctrine-mapping/Metadata.orm.xml` as a **mapped-superclass**. `EventListener/Doctrine/ConvertToEntityListener` listens on Doctrine's `loadClassMetadata` and flips `isMappedSuperclass = false` on the bundle's own `Metadata`, so it becomes a concrete entity. Apps can swap in a custom class via the `metadata_class` config option; `SetonoClientExtension::load()` enforces that it implements `Entity/MetadataInterface`, stores its declared name (e.g. without a leading backslash) in the parameter, and the app is responsible for mapping it. When the custom class extends `Entity/Metadata`, the extension removes the listener: the bundle's class stays a mapped superclass and the app's entity inherits its fields (`tests/Functional/CustomMetadataClassTest.php` runs `MetadataTest` that way). A custom class that only implements the interface leaves the bundle's `Metadata` an entity, as before. @@ -71,4 +82,5 @@ setono_client: - Strict types everywhere (`declare(strict_types=1)`); concrete classes are `final`, except the metadata/entity classes (`ChangeAwareMetadata`, `LazyChangeAwareMetadata`, `Entity\Metadata`) which are intentionally extensible. - New services go in `config/services.xml` using the `setono_client..` id scheme, with a `setono_client..default` alias and an interface alias for autowiring. To insert behavior into a chain, add a decorator with an appropriate `decoration-priority` rather than editing existing classes. +- Public constructors are covered by the BC check, also of `final` classes. Like Symfony, add a new constructor argument at the end as an optional one and `trigger_deprecation('setono/client-bundle', '', 'Not passing an instance of "%s" as "$arg" to "%s()" is deprecated, the argument will be required in .', …)` when it's missing (see `StoreCookieSubscriber`, where a missing pending metadata storage means the metadata of a new client is persisted right away). - Tests use `matthiasnoback/symfony-dependency-injection-test` (e.g. `AbstractExtensionTestCase`), `matthiasnoback/symfony-config-test` (`ConfigurationTestCaseTrait`) and Prophecy (`ProphecyTrait`), with the `@test` annotation and snake_case method-name style. Keep the suite at 100% MSI — when adding logic, add a test that kills the corresponding mutant. diff --git a/README.md b/README.md index 7762aab..49784db 100644 --- a/README.md +++ b/README.md @@ -10,7 +10,9 @@ Recognize returning visitors in your Symfony application and attach your own met The bundle assigns every visitor a stable **client id**, stored in a first‑party cookie (`setono_client_id` by default) that also remembers when the visitor was *first* and *last* seen. On top of that you can persist arbitrary **metadata** per client — for example the first Google click id (`gclid`) a visitor arrived with, an A/B test variant, -or a referrer. Metadata is **lazy**: if you never read or write it, the bundle never touches the database. +or a referrer. Metadata is **lazy**: if you never read or write it, the bundle never touches the database. And the +metadata of a first‑time visitor is only written to the database once the visitor comes back, so crawlers don't fill +your database. Typical use cases: first‑party analytics, marketing attribution, returning‑visitor personalization. @@ -22,6 +24,7 @@ Typical use cases: first‑party analytics, marketing attribution, returning‑v - [Get the current client](#get-the-current-client) - [Read and write metadata](#read-and-write-metadata) - [Set metadata from a request](#set-metadata-from-a-request) + - [Metadata of new visitors](#metadata-of-new-visitors) - [Access the cookie directly](#access-the-cookie-directly) - [Skip storing the cookie (consent)](#skip-storing-the-cookie-consent) - [Configuration](#configuration) @@ -112,7 +115,8 @@ final class SomeService ### Read and write metadata `$client->metadata` is a key/value store. Reading a key for the first time lazily loads the metadata from the -database; writing marks it dirty so it is persisted at the end of the request. +database; writing marks it dirty so it is persisted at the end of the request (for a first‑time visitor, once it comes +back: see [Metadata of new visitors](#metadata-of-new-visitors)). ```php $metadata = $client->metadata; @@ -164,6 +168,33 @@ final class GoogleClickIdSubscriber implements EventSubscriberInterface } ``` +### Metadata of new visitors + +A visitor without the cookie is a *new* client, and many new clients never come back: crawlers, link checkers, link +preview fetchers and browsers that block cookies get a new client id on every request. So that they don't fill the +`setono_client__metadata` table with rows nobody reads, the metadata of a new client is not written to the database +right away. It travels to the browser in a second cookie, `setono_client_id_pending_metadata` (the cookie name plus +`_pending_metadata`), and is persisted on the visitor's next request, i.e. once the visitor has shown that it keeps +its cookies. A visitor that never comes back never produces a row. + +In the request where you set it, the metadata is available as usual, and TTLs carry over unchanged. + +Things to be aware of: + +- The pending metadata cookie is `HttpOnly` and signed with your `kernel.secret`, so JavaScript can't read it and the + visitor can neither forge nor change it. It is not encrypted, so visitors can see their own pending metadata in the + browser's developer tools until it is persisted. Changing `kernel.secret` discards pending metadata that hasn't been + persisted yet. +- Metadata that doesn't fit in about 2 KB, or can't be JSON encoded, is persisted right away. +- If the database already holds metadata for the visitor when it comes back, e.g. because a message handler stored + some, the pending metadata is merged into it. Where both hold a key, the database wins. +- The metadata of a new client is captured when the cookie is stored (on `kernel.response` with priority `-64`). Set it + before that, e.g. on `kernel.request`, in a controller, or in a `kernel.response` listener with the default priority. + Metadata set later is persisted right away. +- If you need the metadata of a new client in the database during its first request, replace the + `setono_client.pending_metadata_storage.default` alias with a `PendingMetadataStorageInterface` implementation whose + `store()` returns `false`. + ### Access the cookie directly The cookie stores the client id plus the first/last seen timestamps. Read it through `CookieProviderInterface`: @@ -222,6 +253,9 @@ final class CookieConsentSubscriber implements EventSubscriberInterface } ``` +If you skip the cookie for a new visitor, its metadata is not persisted either: without the cookie the visitor can't be +recognized again, so nothing could ever read that metadata. + ## Configuration All options are optional; the defaults are shown below: @@ -230,6 +264,7 @@ All options are optional; the defaults are shown below: setono_client: cookie: # Name of the cookie that holds the client id. + # The pending metadata cookie is named after it: _pending_metadata. # NOTE: changing this makes every visitor with the old cookie name look like a new client. name: setono_client_id @@ -251,9 +286,14 @@ setono_client: A new client id is generated only when no valid cookie is present. - **Lazy metadata** — `$client->metadata` is a lazy ghost object. The database is queried only the first time you read a value, and a row is written only at the end of the request, and only if the metadata was actually changed. +- **Pending metadata** — for a new client, `StoreCookieSubscriber` stores changed metadata in the pending metadata + cookie instead of the database. When the client comes back with it, the metadata is handed to the client and + persisted at the end of that request, and the pending metadata cookie is removed. - **Resolution chain** — `ClientContextInterface` is built from a chain of decorating services - (`CachedClientContext` → `CookieBasedClientContext` → `DefaultClientContext`). The cached layer memoizes the client - per request; the cookie‑based layer builds it from the cookie; the default layer creates a fresh, anonymous client. + (`CachedClientContext` → `PendingMetadataClientContext` → `CookieBasedClientContext` → `DefaultClientContext`). The + cached layer memoizes the client per request; the pending metadata layer hands a returning client the metadata it + was given as a new client; the cookie‑based layer builds the client from the cookie; the default layer creates a + fresh, anonymous client. ## Extending the bundle @@ -265,6 +305,7 @@ Every moving part is a service behind an interface, so you can swap or decorate | Reading the cookie | `Setono\ClientBundle\CookieProvider\CookieProviderInterface` | | Loading metadata | `Setono\ClientBundle\MetadataProvider\MetadataProviderInterface` | | Persisting metadata | `Setono\ClientBundle\MetadataPersister\MetadataPersisterInterface` | +| Holding the metadata of new clients | `Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface` | | Metadata entity | `Setono\ClientBundle\Entity\MetadataInterface` | To add behaviour, [decorate](https://symfony.com/doc/current/service_container/service_decoration.html) the relevant diff --git a/composer.json b/composer.json index 733e184..24abd29 100644 --- a/composer.json +++ b/composer.json @@ -20,6 +20,7 @@ "setono/doctrine-orm-trait": "^1.1", "symfony/config": "^6.4 || ^7.0", "symfony/dependency-injection": "^6.4 || ^7.0", + "symfony/deprecation-contracts": "^2.5 || ^3.0", "symfony/event-dispatcher": "^6.4 || ^7.0", "symfony/http-foundation": "^6.4 || ^7.0", "symfony/http-kernel": "^6.4 || ^7.0", diff --git a/config/services.xml b/config/services.xml index 78c7ed7..6a13c58 100644 --- a/config/services.xml +++ b/config/services.xml @@ -17,6 +17,14 @@ + + + + + + @@ -64,6 +72,7 @@ %setono_client.cookie.name% %setono_client.cookie.expiration% + @@ -86,6 +95,20 @@ %setono_client.metadata_class% + + + + + + + %setono_client.cookie.name%_pending_metadata + %setono_client.cookie.expiration% + %kernel.secret% + + diff --git a/src/Client/ChangeAwareMetadata.php b/src/Client/ChangeAwareMetadata.php index 84c393a..3f7430f 100644 --- a/src/Client/ChangeAwareMetadata.php +++ b/src/Client/ChangeAwareMetadata.php @@ -15,6 +15,22 @@ public function isDirty(): bool return $this->dirty; } + /** + * Use this when the metadata holds changes that were not made through this object, e.g. pending metadata of a new client + */ + public function markDirty(): void + { + $this->dirty = true; + } + + /** + * Use this when the changes must not be persisted, e.g. because they were stored as pending metadata of a new client + */ + public function markClean(): void + { + $this->dirty = false; + } + public function set(string $key, mixed $value, ?int $ttl = null): void { parent::set($key, $value, $ttl); diff --git a/src/Context/PendingMetadataClientContext.php b/src/Context/PendingMetadataClientContext.php new file mode 100644 index 0000000..fcf76c4 --- /dev/null +++ b/src/Context/PendingMetadataClientContext.php @@ -0,0 +1,72 @@ +decorated->getClient(); + + $request = $this->requestStack->getMainRequest(); + if (null === $request) { + return $client; + } + + $pendingMetadata = $this->pendingMetadataStorage->get($request, $client->id); + if (null === $pendingMetadata) { + return $client; + } + + [$data, $expires] = self::split($client->metadata); + [$pendingData, $pendingExpires] = self::split(new Metadata($pendingMetadata)); + + // A key the client already has wins, because it was persisted from metadata that held the pending metadata + // (e.g. by a concurrent request), so it's at least as new. Other keys, e.g. persisted by the application + // outside the client's requests, are kept + $missingData = array_diff_key($pendingData, $data); + if ([] === $missingData) { + return $client; + } + + $metadata = new ChangeAwareMetadata($data + $missingData + [ + Metadata::EXPIRES_KEY => $expires + array_intersect_key($pendingExpires, $missingData), + ]); + $metadata->markDirty(); // the pending metadata isn't persisted yet + + return new Client($client->id, $metadata); + } + + /** + * @return array{array, array} the data and the expiry timestamps of the metadata + */ + private static function split(Metadata $metadata): array + { + $data = $metadata->toArray(); + + /** @var array $expires */ + $expires = $data[Metadata::EXPIRES_KEY] ?? []; + unset($data[Metadata::EXPIRES_KEY]); + + return [$data, $expires]; + } +} diff --git a/src/EventSubscriber/StoreCookieSubscriber.php b/src/EventSubscriber/StoreCookieSubscriber.php index 6ac8f61..e0ff4ed 100644 --- a/src/EventSubscriber/StoreCookieSubscriber.php +++ b/src/EventSubscriber/StoreCookieSubscriber.php @@ -5,30 +5,51 @@ namespace Setono\ClientBundle\EventSubscriber; use Psr\EventDispatcher\EventDispatcherInterface; +use Setono\Client\Client; use Setono\Client\Cookie; +use Setono\Client\Metadata; +use Setono\ClientBundle\Client\ChangeAwareMetadata; use Setono\ClientBundle\Context\ClientContextInterface; use Setono\ClientBundle\CookieProvider\CookieProviderInterface; use Setono\ClientBundle\Event\PreStoreCookieEvent; +use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Symfony\Component\EventDispatcher\EventSubscriberInterface; use Symfony\Component\HttpFoundation\Cookie as HttpCookie; +use Symfony\Component\HttpFoundation\Request; +use Symfony\Component\HttpFoundation\Response; use Symfony\Component\HttpKernel\Event\ResponseEvent; use Symfony\Component\HttpKernel\KernelEvents; final class StoreCookieSubscriber implements EventSubscriberInterface { + /** + * @param PendingMetadataStorageInterface|null $pendingMetadataStorage without it, the metadata of a new client is persisted right away + */ public function __construct( private readonly ClientContextInterface $clientContext, private readonly CookieProviderInterface $cookieProvider, private readonly EventDispatcherInterface $eventDispatcher, private readonly string $cookieName, private readonly string $cookieExpiration, + private readonly ?PendingMetadataStorageInterface $pendingMetadataStorage = null, ) { + if (null === $pendingMetadataStorage) { + trigger_deprecation( + 'setono/client-bundle', + '1.3', + 'Not passing an instance of "%s" as "$pendingMetadataStorage" to "%s()" is deprecated, the argument will be required in 2.0.', + PendingMetadataStorageInterface::class, + __METHOD__, + ); + } } public static function getSubscribedEvents(): array { return [ - KernelEvents::RESPONSE => 'store', + // Runs after listeners with the default priority, so the metadata they set on a new client is included + // in its pending metadata, and before the profiler (-100) collects the response + KernelEvents::RESPONSE => ['store', -64], ]; } @@ -38,22 +59,62 @@ public function store(ResponseEvent $event): void return; } - $preStoreEvent = new PreStoreCookieEvent($event->getRequest(), $event->getResponse()); + $request = $event->getRequest(); + $response = $event->getResponse(); + + $preStoreEvent = new PreStoreCookieEvent($request, $response); $this->eventDispatcher->dispatch($preStoreEvent); + $cookie = $this->cookieProvider->getCookie($request); + if (!$preStoreEvent->store) { + // A new client that doesn't get a cookie can't be recognized again, so its metadata would never be read + if (null === $cookie) { + self::markClean($this->clientContext->getClient()->metadata); + } + return; } - $cookie = $this->cookieProvider->getCookie($event->getRequest()); + // Pending metadata sent by the client is handed to the client when it's resolved, at the latest when its metadata + // is persisted on kernel.finish_request (see PendingMetadataClientContext), so the client can forget it + $this->pendingMetadataStorage?->remove($request, $response); if (null === $cookie) { - $cookie = new Cookie($this->clientContext->getClient()->id); + $client = $this->clientContext->getClient(); + $cookie = new Cookie($client->id); + + $this->storePendingMetadata($request, $response, $client); } - $event->getResponse()->headers->setCookie( + $response->headers->setCookie( HttpCookie::create($this->cookieName, $cookie->withLastSeenAt(time())->toString(), $this->cookieExpiration) ->withHttpOnly(false), // this allows javascript to read the cookie ); } + + /** + * The metadata of a new client is not persisted until the client comes back with its cookie + */ + private function storePendingMetadata(Request $request, Response $response, Client $client): void + { + $metadata = $client->metadata; + if (null === $this->pendingMetadataStorage || !$metadata instanceof ChangeAwareMetadata || !$metadata->isDirty()) { + return; + } + + $data = $metadata->toArray(); + + // If the metadata can't be stored as pending metadata, it stays dirty and is persisted right away + if ([] === $data || $this->pendingMetadataStorage->store($request, $response, $client->id, $data)) { + $metadata->markClean(); + } + } + + private static function markClean(Metadata $metadata): void + { + if ($metadata instanceof ChangeAwareMetadata) { + $metadata->markClean(); + } + } } diff --git a/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php new file mode 100644 index 0000000..997a926 --- /dev/null +++ b/src/PendingMetadataStorage/CookieBasedPendingMetadataStorage.php @@ -0,0 +1,102 @@ +cookies->get($this->cookieName); + if (!is_string($value)) { + return null; + } + + $position = strrpos($value, '.'); + if (false === $position) { + return null; + } + + $json = substr($value, 0, $position); + if (!hash_equals($this->sign($clientId, $json), substr($value, $position + 1))) { + return null; + } + + /** @var array|scalar|null $metadata */ + $metadata = json_decode($json, true); + + // The signature proves that store() encoded the metadata, so this only fails for someone who knows the secret. + // Invalid JSON decodes to null too + if (!is_array($metadata)) { + return null; + } + + return $metadata; + } + + public function store(Request $request, Response $response, string $clientId, array $metadata): bool + { + try { + // Like Doctrine's json type, this preserves the zero fraction of floats, so the metadata is the same whether it + // comes from the cookie or the database. Unescaped slashes and unicode keep the URL encoded cookie small + $json = json_encode( + $metadata, + \JSON_THROW_ON_ERROR | \JSON_PRESERVE_ZERO_FRACTION | \JSON_UNESCAPED_SLASHES | \JSON_UNESCAPED_UNICODE, + ); + } catch (\JsonException) { + return false; + } + + $value = $json . '.' . $this->sign($clientId, $json); + + // the cookie value is URL encoded when sent + if (strlen(rawurlencode($value)) > self::MAX_VALUE_LENGTH) { + return false; + } + + $response->headers->setCookie(HttpCookie::create( + $this->cookieName, + $value, + $this->cookieExpiration, + secure: $request->isSecure(), + )); + + return true; + } + + public function remove(Request $request, Response $response): void + { + if ($request->cookies->has($this->cookieName)) { + $response->headers->clearCookie($this->cookieName); + } + } + + private function sign(string $clientId, string $json): string + { + return hash_hmac('sha256', self::SIGNATURE_CONTEXT . '.' . $clientId . '.' . $json, $this->secret); + } +} diff --git a/src/PendingMetadataStorage/PendingMetadataStorageInterface.php b/src/PendingMetadataStorage/PendingMetadataStorageInterface.php new file mode 100644 index 0000000..b7798e4 --- /dev/null +++ b/src/PendingMetadataStorage/PendingMetadataStorageInterface.php @@ -0,0 +1,34 @@ +|null + */ + public function get(Request $request, string $clientId): ?array; + + /** + * Returns false if the metadata cannot be stored (e.g. because it is too big), in which case it must be persisted right away + * + * @param array $metadata the metadata as returned by Metadata::toArray() + */ + public function store(Request $request, Response $response, string $clientId, array $metadata): bool; + + /** + * Removes the pending metadata the request carries, if any + */ + public function remove(Request $request, Response $response): void; +} diff --git a/tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php b/tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php new file mode 100644 index 0000000..a0326fd --- /dev/null +++ b/tests/Functional/App/EventSubscriber/GoogleClickIdSubscriber.php @@ -0,0 +1,49 @@ + [ + ['storeGoogleClickId'], + ['storeLateGoogleClickId', -128], + ], + ]; + } + + public function storeGoogleClickId(ResponseEvent $event): void + { + $this->store($event, 'gclid'); + } + + public function storeLateGoogleClickId(ResponseEvent $event): void + { + $this->store($event, 'late_gclid'); + } + + private function store(ResponseEvent $event, string $parameter): void + { + $gclid = $event->getRequest()->query->get($parameter); + if (null !== $gclid) { + $this->clientContext->getClient()->metadata->set('gclid', $gclid, 7_776_000); + } + } +} diff --git a/tests/Functional/CustomMetadataClassTest.php b/tests/Functional/CustomMetadataClassTest.php index cd1ba8a..690dec0 100644 --- a/tests/Functional/CustomMetadataClassTest.php +++ b/tests/Functional/CustomMetadataClassTest.php @@ -23,6 +23,7 @@ protected static function getEnvironment(): string */ public function it_stores_the_metadata_in_the_configured_entity(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); self::assertResponseIsSuccessful(); diff --git a/tests/Functional/FunctionalTestCase.php b/tests/Functional/FunctionalTestCase.php index 936e684..de1016d 100644 --- a/tests/Functional/FunctionalTestCase.php +++ b/tests/Functional/FunctionalTestCase.php @@ -10,6 +10,7 @@ use Doctrine\Persistence\ManagerRegistry; use Setono\Client\Cookie; use Setono\ClientBundle\Tests\Functional\App\Kernel; +use Symfony\Bridge\Doctrine\Middleware\Debug\DebugDataHolder; use Symfony\Bundle\FrameworkBundle\KernelBrowser; use Symfony\Bundle\FrameworkBundle\Test\WebTestCase; @@ -42,6 +43,15 @@ protected static function getEnvironment(): string return 'test'; } + /** + * Gives the browser a client id, so from the next request on it's a returning visitor, whose metadata is persisted + * right away. The metadata of a new visitor isn't persisted until it comes back (see NewClientMetadataTest) + */ + protected function becomeReturningVisitor(): void + { + $this->browser->request('GET', '/'); + } + /** * Returns the cookie stored in the browser */ @@ -114,6 +124,46 @@ protected static function storeMetadata(string $clientId, array $metadata): void ]); } + /** + * Returns the SQL of the queries that the last request executed, as recorded by DoctrineBundle in debug mode + * + * @return list + */ + protected static function getExecutedQueries(): array + { + $debugDataHolder = self::getContainer()->get('test.doctrine.debug_data_holder'); + self::assertInstanceOf(DebugDataHolder::class, $debugDataHolder); + + $queries = []; + foreach ($debugDataHolder->getData() as $connectionQueries) { + self::assertIsArray($connectionQueries); + + foreach ($connectionQueries as $query) { + self::assertIsArray($query); + self::assertIsString($query['sql']); + + $queries[] = $query['sql']; + } + } + + return $queries; + } + + /** + * Inserts the client's row, like an application does when it persists metadata outside the client's requests + * + * @param array $metadata + */ + protected static function insertMetadata(string $clientId, array $metadata): void + { + $classMetadata = self::getMetadataClassMetadata(); + + self::getConnection()->insert($classMetadata->getTableName(), [ + $classMetadata->getColumnName('clientId') => $clientId, + $classMetadata->getColumnName('metadata') => json_encode($metadata, \JSON_THROW_ON_ERROR), + ]); + } + protected static function getConnection(): Connection { return self::getEntityManager()->getConnection(); diff --git a/tests/Functional/LongRunningProcessTest.php b/tests/Functional/LongRunningProcessTest.php index f0bf992..6fcdd50 100644 --- a/tests/Functional/LongRunningProcessTest.php +++ b/tests/Functional/LongRunningProcessTest.php @@ -38,10 +38,12 @@ public function it_gives_each_new_visitor_its_own_client(): void */ public function it_does_not_give_the_metadata_of_a_visitor_to_the_next_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $firstClientId = $this->getClientId(); $this->browser->getCookieJar()->clear(); + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=baz&value=qux'); $this->browser->request('GET', '/metadata'); diff --git a/tests/Functional/MetadataTest.php b/tests/Functional/MetadataTest.php index d574052..fb77850 100644 --- a/tests/Functional/MetadataTest.php +++ b/tests/Functional/MetadataTest.php @@ -6,7 +6,6 @@ use Doctrine\ORM\Tools\SchemaValidator; use Setono\Client\Metadata; -use Symfony\Bridge\Doctrine\Middleware\Debug\DebugDataHolder; /** * Not final, because CustomMetadataClassTest runs these tests with an application's own metadata entity @@ -27,19 +26,33 @@ public function it_does_not_store_anything_for_a_visitor_that_does_not_use_metad /** * @test */ - public function it_stores_the_metadata_of_a_new_visitor(): void + public function it_stores_the_metadata_of_a_returning_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); self::assertResponseIsSuccessful(); self::assertSame(['foo' => 'bar'], self::fetchStoredMetadata($this->getClientId())); } + /** + * @test + */ + public function it_stores_the_metadata_of_a_new_visitor_when_it_comes_back(): void + { + $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); + self::assertSame(0, self::countStoredMetadata()); + + $this->browser->request('GET', '/'); + self::assertSame(['foo' => 'bar'], self::fetchStoredMetadata($this->getClientId())); + } + /** * @test */ public function it_loads_the_stored_metadata_of_a_returning_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/metadata'); @@ -53,6 +66,7 @@ public function it_loads_the_stored_metadata_of_a_returning_visitor(): void */ public function it_updates_the_stored_metadata_of_a_returning_visitor(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/metadata/set?key=baz&value=qux'); $this->browser->request('GET', '/metadata/set?key=foo&value=changed'); @@ -67,6 +81,7 @@ public function it_updates_the_stored_metadata_of_a_returning_visitor(): void */ public function it_removes_metadata(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/metadata/set?key=baz&value=qux'); @@ -82,10 +97,12 @@ public function it_removes_metadata(): void */ public function it_keeps_the_metadata_of_each_visitor_apart(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=first'); $firstClientId = $this->getClientId(); $this->browser->getCookieJar()->clear(); + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=second'); $secondClientId = $this->getClientId(); @@ -99,6 +116,7 @@ public function it_keeps_the_metadata_of_each_visitor_apart(): void */ public function it_does_not_query_the_database_when_the_metadata_is_not_used(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $this->browser->request('GET', '/'); @@ -115,6 +133,8 @@ public function it_does_not_query_the_database_when_the_metadata_is_not_used(): */ public function it_keeps_the_expiry_of_metadata_between_requests(): void { + $this->becomeReturningVisitor(); + $before = time(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar&ttl=3600'); @@ -137,6 +157,7 @@ public function it_keeps_the_expiry_of_metadata_between_requests(): void */ public function it_removes_expired_metadata(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set?key=foo&value=bar'); $clientId = $this->getClientId(); @@ -157,6 +178,7 @@ public function it_removes_expired_metadata(): void */ public function it_keeps_the_row_of_a_concurrent_request_that_stored_it_first(): void { + $this->becomeReturningVisitor(); $this->browser->request('GET', '/metadata/set-concurrently?key=foo&value=bar'); self::assertResponseIsSuccessful(); @@ -188,29 +210,4 @@ private function getResponseData(): array return $data; } - - /** - * Returns the SQL of the queries that the last request executed, as recorded by DoctrineBundle in debug mode - * - * @return list - */ - private static function getExecutedQueries(): array - { - $debugDataHolder = self::getContainer()->get('test.doctrine.debug_data_holder'); - self::assertInstanceOf(DebugDataHolder::class, $debugDataHolder); - - $queries = []; - foreach ($debugDataHolder->getData() as $connectionQueries) { - self::assertIsArray($connectionQueries); - - foreach ($connectionQueries as $query) { - self::assertIsArray($query); - self::assertIsString($query['sql']); - - $queries[] = $query['sql']; - } - } - - return $queries; - } } diff --git a/tests/Functional/NewClientMetadataTest.php b/tests/Functional/NewClientMetadataTest.php new file mode 100644 index 0000000..390e144 --- /dev/null +++ b/tests/Functional/NewClientMetadataTest.php @@ -0,0 +1,121 @@ +browser->request('GET', '/?gclid=abc'); + + self::assertResponseIsSuccessful(); + self::assertEqualsCanonicalizing( + [self::COOKIE_NAME, self::PENDING_METADATA_COOKIE_NAME], + array_map(static fn (Cookie $cookie): string => $cookie->getName(), $this->browser->getResponse()->headers->getCookies()), + ); + self::assertSame([], self::getExecutedQueries()); + self::assertSame(0, self::countStoredMetadata()); + } + + /** + * @test + */ + public function it_persists_the_metadata_when_the_new_client_comes_back(): void + { + $this->browser->request('GET', '/?gclid=abc'); + self::assertSame(0, self::countStoredMetadata()); + + $this->browser->request('GET', '/'); + + $metadata = self::fetchStoredMetadata($this->getClientId()); + self::assertIsArray($metadata); + self::assertSame('abc', $metadata['gclid']); + self::assertArrayHasKey(Metadata::EXPIRES_KEY, $metadata); + + $pendingMetadataCookies = array_values(array_filter( + $this->browser->getResponse()->headers->getCookies(), + static fn (Cookie $cookie): bool => self::PENDING_METADATA_COOKIE_NAME === $cookie->getName(), + )); + self::assertCount(1, $pendingMetadataCookies); + self::assertTrue($pendingMetadataCookies[0]->isCleared()); + self::assertNull($this->browser->getCookieJar()->get(self::PENDING_METADATA_COOKIE_NAME)); + + // from now on, the metadata is only loaded when it's used + $this->browser->request('GET', '/'); + self::assertSame([], self::getExecutedQueries()); + } + + /** + * @test + */ + public function it_merges_the_metadata_into_metadata_persisted_before_the_new_client_came_back(): void + { + $this->browser->request('GET', '/?gclid=abc'); + + // e.g. by a message handler that the first request dispatched + self::insertMetadata($this->getClientId(), ['country' => 'DK']); + + $this->browser->request('GET', '/'); + + $metadata = self::fetchStoredMetadata($this->getClientId()); + self::assertIsArray($metadata); + self::assertSame('DK', $metadata['country']); + self::assertSame('abc', $metadata['gclid']); + self::assertIsArray($metadata[Metadata::EXPIRES_KEY]); + self::assertSame(['gclid'], array_keys($metadata[Metadata::EXPIRES_KEY])); + } + + /** + * @test + */ + public function it_persists_the_metadata_right_away_when_it_does_not_fit_in_the_pending_metadata_cookie(): void + { + $value = str_repeat('a', 2048); + + $this->browser->request('GET', '/metadata/set?key=foo&value=' . $value); + + self::assertResponseNotHasCookie(self::PENDING_METADATA_COOKIE_NAME); + self::assertSame(['foo' => $value], self::fetchStoredMetadata($this->getClientId())); + } + + /** + * @test + */ + public function it_persists_metadata_set_after_the_cookie_was_stored_right_away(): void + { + $this->browser->request('GET', '/?late_gclid=abc'); + + self::assertResponseNotHasCookie(self::PENDING_METADATA_COOKIE_NAME); + + $metadata = self::fetchStoredMetadata($this->getClientId()); + self::assertIsArray($metadata); + self::assertSame('abc', $metadata['gclid']); + } + + /** + * @test + */ + public function it_does_not_persist_the_metadata_of_a_new_client_that_does_not_get_a_cookie(): void + { + $this->browser->request('GET', '/?gclid=abc&skip_cookie=1'); + + self::assertResponseIsSuccessful(); + self::assertCount(0, $this->browser->getResponse()->headers->getCookies()); + self::assertSame([], self::getExecutedQueries()); + self::assertSame(0, self::countStoredMetadata()); + } +} diff --git a/tests/Unit/Client/ChangeAwareMetadataTest.php b/tests/Unit/Client/ChangeAwareMetadataTest.php index a4b1389..ec02117 100644 --- a/tests/Unit/Client/ChangeAwareMetadataTest.php +++ b/tests/Unit/Client/ChangeAwareMetadataTest.php @@ -43,4 +43,19 @@ public function it_becomes_dirty_when_removing_a_value(): void self::assertTrue($metadata->isDirty()); self::assertFalse($metadata->has('foo')); } + + /** + * @test + */ + public function it_can_be_marked_dirty_and_clean(): void + { + $metadata = new ChangeAwareMetadata(['foo' => 'bar']); + + $metadata->markDirty(); + self::assertTrue($metadata->isDirty()); + + $metadata->markClean(); + self::assertFalse($metadata->isDirty()); + self::assertSame('bar', $metadata->get('foo')); + } } diff --git a/tests/Unit/Context/PendingMetadataClientContextTest.php b/tests/Unit/Context/PendingMetadataClientContextTest.php new file mode 100644 index 0000000..58dc828 --- /dev/null +++ b/tests/Unit/Context/PendingMetadataClientContextTest.php @@ -0,0 +1,163 @@ +prophesize(PendingMetadataStorageInterface::class); + $storage->get(Argument::cetera())->shouldNotBeCalled(); + + $context = $this->createContext($decoratedClient, $storage->reveal(), new RequestStack()); + + self::assertSame($decoratedClient, $context->getClient()); + } + + /** + * @test + */ + public function it_returns_the_decorated_client_when_there_is_no_pending_metadata(): void + { + $decoratedClient = new Client('client-id'); + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn(null); + + $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); + + self::assertSame($decoratedClient, $context->getClient()); + } + + /** + * @test + */ + public function it_hands_the_pending_metadata_to_a_client_without_metadata(): void + { + $request = new Request(); + $pendingMetadata = ['foo' => 'bar', '__expires' => ['foo' => 1893456000]]; + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn($pendingMetadata); + + $context = $this->createContext( + new Client('client-id', new ChangeAwareMetadata()), + $storage->reveal(), + self::createRequestStack($request), + ); + + $client = $context->getClient(); + + self::assertSame('client-id', $client->id); + self::assertInstanceOf(ChangeAwareMetadata::class, $client->metadata); + self::assertTrue($client->metadata->isDirty()); + self::assertSame($pendingMetadata, $client->metadata->toArray()); + } + + /** + * @test + */ + public function it_returns_the_decorated_client_when_its_metadata_has_all_pending_keys(): void + { + $decoratedClient = new Client('client-id', new Metadata(['foo' => 'persisted', 'bar' => 'baz'])); + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn(['foo' => 'pending', '__expires' => ['foo' => 1893456000]]); + + $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); + + self::assertSame($decoratedClient, $context->getClient()); + self::assertSame(['foo' => 'persisted', 'bar' => 'baz'], $decoratedClient->metadata->toArray()); + } + + /** + * @test + */ + public function it_does_not_add_expired_pending_metadata(): void + { + $decoratedClient = new Client('client-id', new Metadata(['bar' => 'baz'])); + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn(['foo' => 'pending', '__expires' => ['foo' => time() - 1]]); + + $context = $this->createContext($decoratedClient, $storage->reveal(), self::createRequestStack($request)); + + self::assertSame($decoratedClient, $context->getClient()); + } + + /** + * @test + */ + public function it_adds_the_pending_metadata_that_the_client_does_not_have(): void + { + $request = new Request(); + + $storage = $this->prophesize(PendingMetadataStorageInterface::class); + $storage->get($request, 'client-id')->willReturn([ + 'foo' => 'pending', + 'qux' => 'pending', + '__expires' => ['foo' => 1893456000, 'qux' => 1893456001], + ]); + + $context = $this->createContext( + new Client('client-id', new Metadata(['foo' => 'persisted', 'bar' => 'baz', '__expires' => ['bar' => 1893456002]])), + $storage->reveal(), + self::createRequestStack($request), + ); + + $client = $context->getClient(); + + self::assertSame('client-id', $client->id); + self::assertInstanceOf(ChangeAwareMetadata::class, $client->metadata); + self::assertTrue($client->metadata->isDirty()); + self::assertSame([ + 'foo' => 'persisted', + 'bar' => 'baz', + 'qux' => 'pending', + '__expires' => ['bar' => 1893456002, 'qux' => 1893456001], + ], $client->metadata->toArray()); + } + + private function createContext( + Client $decoratedClient, + PendingMetadataStorageInterface $storage, + RequestStack $requestStack, + ): PendingMetadataClientContext { + $decorated = $this->prophesize(ClientContextInterface::class); + $decorated->getClient()->willReturn($decoratedClient); + + return new PendingMetadataClientContext($decorated->reveal(), $storage, $requestStack); + } + + private static function createRequestStack(Request $request): RequestStack + { + $requestStack = new RequestStack(); + $requestStack->push($request); + + return $requestStack; + } +} diff --git a/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php b/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php index 88e1e6d..14bbf2b 100644 --- a/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php +++ b/tests/Unit/DependencyInjection/SetonoClientExtensionTest.php @@ -7,10 +7,13 @@ use Matthias\SymfonyDependencyInjectionTest\PhpUnit\AbstractExtensionTestCase; use Setono\ClientBundle\Context\ClientContextInterface; use Setono\ClientBundle\Context\DefaultClientContext; +use Setono\ClientBundle\Context\PendingMetadataClientContext; use Setono\ClientBundle\CookieProvider\RequestBasedCookieProvider; use Setono\ClientBundle\DependencyInjection\SetonoClientExtension; use Setono\ClientBundle\Entity\Metadata; use Setono\ClientBundle\EventListener\Doctrine\ConvertToEntityListener; +use Setono\ClientBundle\PendingMetadataStorage\CookieBasedPendingMetadataStorage; +use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Setono\ClientBundle\Tests\Unit\DependencyInjection\Fixture\CustomMetadata; use Setono\ClientBundle\Tests\Unit\DependencyInjection\Fixture\InterfaceOnlyMetadata; @@ -65,6 +68,47 @@ public function after_loading_the_services_are_registered(): void $this->assertContainerBuilderHasAlias(ClientContextInterface::class, 'setono_client.client_context.default'); } + /** + * @test + */ + public function after_loading_the_pending_metadata_services_are_registered(): void + { + $this->load(); + + $this->assertContainerBuilderHasService( + 'setono_client.pending_metadata_storage.cookie_based', + CookieBasedPendingMetadataStorage::class, + ); + $this->assertContainerBuilderHasServiceDefinitionWithArgument( + 'setono_client.pending_metadata_storage.cookie_based', + 0, + '%setono_client.cookie.name%_pending_metadata', + ); + $this->assertContainerBuilderHasServiceDefinitionWithArgument( + 'setono_client.pending_metadata_storage.cookie_based', + 2, + '%kernel.secret%', + ); + $this->assertContainerBuilderHasAlias( + 'setono_client.pending_metadata_storage.default', + 'setono_client.pending_metadata_storage.cookie_based', + ); + $this->assertContainerBuilderHasAlias( + PendingMetadataStorageInterface::class, + 'setono_client.pending_metadata_storage.default', + ); + + $this->assertContainerBuilderHasService( + 'setono_client.client_context.pending_metadata', + PendingMetadataClientContext::class, + ); + // between the cached (32) and the cookie based (64) client context + self::assertSame( + ['setono_client.client_context.default', null, 48], + $this->container->getDefinition('setono_client.client_context.pending_metadata')->getDecoratedService(), + ); + } + /** * @test */ diff --git a/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php b/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php index cc9ef5d..2a626cb 100644 --- a/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php +++ b/tests/Unit/EventSubscriber/StoreCookieSubscriberTest.php @@ -7,13 +7,17 @@ use PHPUnit\Framework\TestCase; use Prophecy\Argument; use Prophecy\PhpUnit\ProphecyTrait; +use Prophecy\Prophecy\ObjectProphecy; use Psr\EventDispatcher\EventDispatcherInterface; use Setono\Client\Client; use Setono\Client\Cookie; +use Setono\Client\Metadata; +use Setono\ClientBundle\Client\ChangeAwareMetadata; use Setono\ClientBundle\Context\ClientContextInterface; use Setono\ClientBundle\CookieProvider\CookieProviderInterface; use Setono\ClientBundle\Event\PreStoreCookieEvent; use Setono\ClientBundle\EventSubscriber\StoreCookieSubscriber; +use Setono\ClientBundle\PendingMetadataStorage\PendingMetadataStorageInterface; use Symfony\Component\EventDispatcher\EventDispatcher; use Symfony\Component\HttpFoundation\Request; use Symfony\Component\HttpFoundation\Response; @@ -27,12 +31,20 @@ final class StoreCookieSubscriberTest extends TestCase private const COOKIE_NAME = 'setono_client_id'; + /** @var ObjectProphecy */ + private ObjectProphecy $pendingMetadataStorage; + + protected function setUp(): void + { + $this->pendingMetadataStorage = $this->prophesize(PendingMetadataStorageInterface::class); + } + /** * @test */ public function it_subscribes_to_the_response_event(): void { - self::assertSame([KernelEvents::RESPONSE => 'store'], StoreCookieSubscriber::getSubscribedEvents()); + self::assertSame([KernelEvents::RESPONSE => ['store', -64]], StoreCookieSubscriber::getSubscribedEvents()); } /** @@ -41,15 +53,12 @@ public function it_subscribes_to_the_response_event(): void public function it_stores_a_new_cookie_when_none_exists(): void { $clientContext = $this->prophesize(ClientContextInterface::class); - $clientContext->getClient()->willReturn(new Client('client-id')); + $clientContext->getClient()->willReturn(new Client('client-id', new ChangeAwareMetadata(['foo' => 'bar']))); - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(null); + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($clientContext, null); $cookies = $response->headers->getCookies(); self::assertCount(1, $cookies); @@ -66,13 +75,7 @@ public function it_reuses_the_client_id_from_an_existing_cookie(): void $clientContext = $this->prophesize(ClientContextInterface::class); $clientContext->getClient()->shouldNotBeCalled(); - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(new Cookie('existing-id')); - - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($clientContext, new Cookie('existing-id')); $cookies = $response->headers->getCookies(); self::assertCount(1, $cookies); @@ -84,16 +87,8 @@ public function it_reuses_the_client_id_from_an_existing_cookie(): void */ public function it_refreshes_the_last_seen_timestamp_and_keeps_the_first_seen_timestamp(): void { - $clientContext = $this->prophesize(ClientContextInterface::class); - - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(new Cookie('existing-id', 2, 1_000, 2_000)); - $before = time(); - $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( - $this->createResponseEvent(new Request(), $response), - ); + $response = $this->store($this->prophesize(ClientContextInterface::class), new Cookie('existing-id', 2, 1_000, 2_000)); $cookie = Cookie::fromString((string) $response->headers->getCookies()[0]->getValue()); self::assertSame(1_000, $cookie->firstSeenAt); @@ -115,8 +110,8 @@ public function it_uses_the_configured_cookie_name_and_expiration(): void $before = time(); $response = new Response(); $this->createSubscriber( - $clientContext->reveal(), - $cookieProvider->reveal(), + $clientContext, + $cookieProvider, cookieName: 'custom_name', cookieExpiration: '+30 days', )->store($this->createResponseEvent(new Request(), $response)); @@ -131,14 +126,105 @@ public function it_uses_the_configured_cookie_name_and_expiration(): void /** * @test */ - public function it_does_not_store_a_cookie_for_sub_requests(): void + public function it_removes_the_pending_metadata(): void + { + $request = new Request(); + $response = new Response(); + + $this->pendingMetadataStorage->remove($request, $response)->shouldBeCalledOnce(); + + $this->store($this->prophesize(ClientContextInterface::class), new Cookie('existing-id'), $request, $response); + } + + /** + * @test + */ + public function it_stores_the_changed_metadata_of_a_new_client_as_pending_metadata(): void + { + $request = new Request(); + $response = new Response(); + + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->remove($request, $response)->shouldBeCalled(); + $this->pendingMetadataStorage->store($request, $response, 'client-id', ['foo' => 'bar'])->willReturn(true)->shouldBeCalledOnce(); + + $this->store($clientContext, null, $request, $response); + + self::assertFalse($metadata->isDirty()); + } + + /** + * @test + */ + public function it_keeps_the_metadata_dirty_when_it_cannot_be_stored_as_pending_metadata(): void { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->willReturn(false)->shouldBeCalledOnce(); + + $this->store($clientContext, null); + + self::assertTrue($metadata->isDirty()); + } + + /** + * @test + */ + public function it_does_not_store_empty_pending_metadata(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + $metadata->remove('foo'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); + + $this->store($clientContext, null); + + self::assertFalse($metadata->isDirty()); + } + + /** + * @test + */ + public function it_does_not_store_metadata_that_is_not_change_aware_as_pending_metadata(): void + { + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', new Metadata(['foo' => 'bar']))); + + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldBeCalled(); + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); + + $response = $this->store($clientContext, null); + + self::assertCount(1, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_does_not_store_a_cookie_for_sub_requests(): void + { $cookieProvider = $this->prophesize(CookieProviderInterface::class); $cookieProvider->getCookie(Argument::cetera())->shouldNotBeCalled(); + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldNotBeCalled(); + $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal())->store( + $this->createSubscriber($this->prophesize(ClientContextInterface::class), $cookieProvider)->store( $this->createResponseEvent(new Request(), $response, HttpKernelInterface::SUB_REQUEST), ); @@ -151,30 +237,154 @@ public function it_does_not_store_a_cookie_for_sub_requests(): void public function it_does_not_store_a_cookie_when_the_pre_store_event_vetoes_it(): void { $clientContext = $this->prophesize(ClientContextInterface::class); - $cookieProvider = $this->prophesize(CookieProviderInterface::class); - $cookieProvider->getCookie(Argument::cetera())->shouldNotBeCalled(); + $clientContext->getClient()->shouldNotBeCalled(); - $eventDispatcher = new EventDispatcher(); - $eventDispatcher->addListener(PreStoreCookieEvent::class, static function (PreStoreCookieEvent $event): void { - $event->store = false; - }); + $this->pendingMetadataStorage->remove(Argument::cetera())->shouldNotBeCalled(); + + $response = $this->store($clientContext, new Cookie('existing-id'), eventDispatcher: self::createVetoingEventDispatcher()); + + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_discards_the_metadata_of_a_new_client_when_the_pre_store_event_vetoes_the_cookie(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $this->pendingMetadataStorage->store(Argument::cetera())->shouldNotBeCalled(); + + $response = $this->store($clientContext, null, eventDispatcher: self::createVetoingEventDispatcher()); + + self::assertCount(0, $response->headers->getCookies()); + self::assertFalse($metadata->isDirty()); + } + + /** + * @test + */ + public function it_ignores_metadata_that_is_not_change_aware_when_the_pre_store_event_vetoes_the_cookie(): void + { + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', new Metadata(['foo' => 'bar'])))->shouldBeCalled(); + + $response = $this->store($clientContext, null, eventDispatcher: self::createVetoingEventDispatcher()); + + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_triggers_a_deprecation_when_it_does_not_get_a_pending_metadata_storage(): void + { + $deprecations = []; + set_error_handler(static function (int $level, string $message) use (&$deprecations): bool { + $deprecations[] = $message; + + return true; + }, \E_USER_DEPRECATED); + + try { + self::createSubscriberWithoutPendingMetadataStorage( + $this->prophesize(ClientContextInterface::class), + $this->prophesize(CookieProviderInterface::class), + ); + } finally { + restore_error_handler(); + } + + self::assertSame([sprintf( + 'Since setono/client-bundle 1.3: Not passing an instance of "%s" as "$pendingMetadataStorage" to "%s::__construct()" is deprecated, the argument will be required in 2.0.', + PendingMetadataStorageInterface::class, + StoreCookieSubscriber::class, + )], $deprecations); + } + + /** + * @test + */ + public function it_leaves_the_metadata_of_a_new_client_to_be_persisted_right_away_without_a_pending_metadata_storage(): void + { + $metadata = new ChangeAwareMetadata(); + $metadata->set('foo', 'bar'); + + $clientContext = $this->prophesize(ClientContextInterface::class); + $clientContext->getClient()->willReturn(new Client('client-id', $metadata)); + + $cookieProvider = $this->prophesize(CookieProviderInterface::class); + $cookieProvider->getCookie(Argument::type(Request::class))->willReturn(null); $response = new Response(); - $this->createSubscriber($clientContext->reveal(), $cookieProvider->reveal(), $eventDispatcher)->store( + self::createSubscriberWithoutPendingMetadataStorage($clientContext, $cookieProvider)->store( $this->createResponseEvent(new Request(), $response), ); - self::assertCount(0, $response->headers->getCookies()); + self::assertCount(1, $response->headers->getCookies()); + self::assertTrue($metadata->isDirty()); } + /** + * @param ObjectProphecy $clientContext + */ + private function store( + ObjectProphecy $clientContext, + ?Cookie $cookie, + Request $request = new Request(), + Response $response = new Response(), + EventDispatcherInterface $eventDispatcher = new EventDispatcher(), + ): Response { + $cookieProvider = $this->prophesize(CookieProviderInterface::class); + $cookieProvider->getCookie($request)->willReturn($cookie); + + $this->createSubscriber($clientContext, $cookieProvider, $eventDispatcher)->store( + $this->createResponseEvent($request, $response), + ); + + return $response; + } + + /** + * @param ObjectProphecy $clientContext + * @param ObjectProphecy $cookieProvider + */ private function createSubscriber( - ClientContextInterface $clientContext, - CookieProviderInterface $cookieProvider, + ObjectProphecy $clientContext, + ObjectProphecy $cookieProvider, EventDispatcherInterface $eventDispatcher = new EventDispatcher(), string $cookieName = self::COOKIE_NAME, string $cookieExpiration = '+365 days', ): StoreCookieSubscriber { - return new StoreCookieSubscriber($clientContext, $cookieProvider, $eventDispatcher, $cookieName, $cookieExpiration); + return new StoreCookieSubscriber( + $clientContext->reveal(), + $cookieProvider->reveal(), + $eventDispatcher, + $cookieName, + $cookieExpiration, + $this->pendingMetadataStorage->reveal(), + ); + } + + /** + * @param ObjectProphecy $clientContext + * @param ObjectProphecy $cookieProvider + */ + private static function createSubscriberWithoutPendingMetadataStorage( + ObjectProphecy $clientContext, + ObjectProphecy $cookieProvider, + ): StoreCookieSubscriber { + return new StoreCookieSubscriber( + $clientContext->reveal(), + $cookieProvider->reveal(), + new EventDispatcher(), + self::COOKIE_NAME, + '+365 days', + ); } private function createResponseEvent( @@ -189,4 +399,14 @@ private function createResponseEvent( $response, ); } + + private static function createVetoingEventDispatcher(): EventDispatcher + { + $eventDispatcher = new EventDispatcher(); + $eventDispatcher->addListener(PreStoreCookieEvent::class, static function (PreStoreCookieEvent $event): void { + $event->store = false; + }); + + return $eventDispatcher; + } } diff --git a/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php b/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php new file mode 100644 index 0000000..309f9cd --- /dev/null +++ b/tests/Unit/PendingMetadataStorage/CookieBasedPendingMetadataStorageTest.php @@ -0,0 +1,257 @@ +createStorage()->store(new Request(), $response, 'client-id', [ + 'url' => 'https://example.com/æ', + 'amount' => 1.0, + ])); + + $json = '{"url":"https://example.com/æ","amount":1.0}'; + + $cookie = self::getOnlyCookie($response); + self::assertSame(self::COOKIE_NAME, $cookie->getName()); + self::assertSame($json . '.' . self::sign('client-id', $json), $cookie->getValue()); + self::assertSame('/', $cookie->getPath()); + self::assertTrue($cookie->isHttpOnly()); + self::assertFalse($cookie->isSecure()); + self::assertSame(Cookie::SAMESITE_LAX, $cookie->getSameSite()); + self::assertEqualsWithDelta(strtotime('+365 days'), $cookie->getExpiresTime(), 5); + } + + /** + * @test + */ + public function it_stores_a_secure_cookie_on_secure_requests(): void + { + $response = new Response(); + + $this->createStorage()->store(Request::create('https://example.com/'), $response, 'client-id', ['foo' => 'bar']); + + self::assertTrue(self::getOnlyCookie($response)->isSecure()); + } + + /** + * @test + */ + public function it_returns_the_stored_metadata(): void + { + $metadata = ['foo' => 'bar', 'amount' => 1.0, '__expires' => ['foo' => 1893456000]]; + + $storage = $this->createStorage(); + $response = new Response(); + $storage->store(new Request(), $response, 'client-id', $metadata); + + self::assertSame($metadata, $storage->get(self::createRequestFromResponse($response), 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_request_has_no_cookie(): void + { + self::assertNull($this->createStorage()->get(new Request(), 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_cookie_is_not_signed(): void + { + $request = new Request(cookies: [self::COOKIE_NAME => '{"foo":"bar"}']); + + self::assertNull($this->createStorage()->get($request, 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_metadata_has_been_tampered_with(): void + { + $response = new Response(); + $this->createStorage()->store(new Request(), $response, 'client-id', ['foo' => 'bar']); + + $value = str_replace('"bar"', '"baz"', (string) self::getOnlyCookie($response)->getValue()); + $request = new Request(cookies: [self::COOKIE_NAME => $value]); + + self::assertNull($this->createStorage()->get($request, 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_metadata_belongs_to_another_client(): void + { + $storage = $this->createStorage(); + $response = new Response(); + $storage->store(new Request(), $response, 'another-client-id', ['foo' => 'bar']); + + self::assertNull($storage->get(self::createRequestFromResponse($response), 'client-id')); + } + + /** + * @test + */ + public function it_returns_null_when_the_metadata_was_signed_with_another_secret(): void + { + $response = new Response(); + (new CookieBasedPendingMetadataStorage(self::COOKIE_NAME, '+365 days', 'another-secret')) + ->store(new Request(), $response, 'client-id', ['foo' => 'bar']); + + self::assertNull($this->createStorage()->get(self::createRequestFromResponse($response), 'client-id')); + } + + /** + * Only someone who knows the secret can sign such a value + * + * @test + * + * @dataProvider provideSignedValuesThatAreNotMetadata + */ + public function it_returns_null_when_the_signed_value_does_not_decode_to_an_array(string $json): void + { + $request = new Request(cookies: [self::COOKIE_NAME => $json . '.' . self::sign('client-id', $json)]); + + self::assertNull($this->createStorage()->get($request, 'client-id')); + } + + /** + * @return iterable + */ + public static function provideSignedValuesThatAreNotMetadata(): iterable + { + yield 'string' => ['"foo"']; + yield 'integer' => ['1']; + yield 'null' => ['null']; + yield 'invalid JSON' => ['{"foo":']; + } + + /** + * @test + */ + public function it_does_not_store_metadata_that_cannot_be_encoded(): void + { + $response = new Response(); + + self::assertFalse($this->createStorage()->store(new Request(), $response, 'client-id', ['foo' => "\xB1\x31"])); + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_stores_metadata_up_to_the_size_limit(): void + { + $response = new Response(); + + self::assertTrue($this->createStorage()->store(new Request(), $response, 'client-id', self::createMetadataOfLength(2048))); + self::assertSame(2048, strlen(rawurlencode((string) self::getOnlyCookie($response)->getValue()))); + } + + /** + * @test + */ + public function it_does_not_store_metadata_exceeding_the_size_limit(): void + { + $response = new Response(); + + self::assertFalse($this->createStorage()->store(new Request(), $response, 'client-id', self::createMetadataOfLength(2049))); + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function it_removes_the_cookie(): void + { + $response = new Response(); + + $this->createStorage()->remove(new Request(cookies: [self::COOKIE_NAME => 'value']), $response); + + $cookie = self::getOnlyCookie($response); + self::assertSame(self::COOKIE_NAME, $cookie->getName()); + self::assertTrue($cookie->isCleared()); + } + + /** + * @test + */ + public function it_does_not_remove_the_cookie_when_the_request_has_none(): void + { + $response = new Response(); + + $this->createStorage()->remove(new Request(), $response); + + self::assertCount(0, $response->headers->getCookies()); + } + + /** + * @test + */ + public function storing_after_removing_replaces_the_removal(): void + { + $storage = $this->createStorage(); + $response = new Response(); + + $storage->remove(new Request(cookies: [self::COOKIE_NAME => 'value']), $response); + $storage->store(new Request(), $response, 'client-id', ['foo' => 'bar']); + + self::assertFalse(self::getOnlyCookie($response)->isCleared()); + } + + private function createStorage(): CookieBasedPendingMetadataStorage + { + return new CookieBasedPendingMetadataStorage(self::COOKIE_NAME, '+365 days', self::SECRET); + } + + /** + * Returns metadata whose URL encoded cookie value has the given length + * + * @return array + */ + private static function createMetadataOfLength(int $length): array + { + $signatureLength = 64; + + return ['k' => str_repeat('a', $length - strlen(rawurlencode('{"k":""}.')) - $signatureLength)]; + } + + private static function sign(string $clientId, string $json): string + { + return hash_hmac('sha256', 'setono_client_pending_metadata.' . $clientId . '.' . $json, self::SECRET); + } + + private static function getOnlyCookie(Response $response): Cookie + { + $cookies = $response->headers->getCookies(); + self::assertCount(1, $cookies); + + return $cookies[0]; + } + + private static function createRequestFromResponse(Response $response): Request + { + return new Request(cookies: [self::COOKIE_NAME => self::getOnlyCookie($response)->getValue()]); + } +}