diff --git a/.github/workflows/push.yml b/.github/workflows/push.yml index 51960448f..2e2aec304 100644 --- a/.github/workflows/push.yml +++ b/.github/workflows/push.yml @@ -66,6 +66,9 @@ jobs: - { name: "Repositories", filter: "tests/Repositories/" } - { name: "Services", filter: "tests/Unit/Services/" } - { name: "CacheOptimizations", filter: "--filter '(PresentationSpeakerCacheTest|ResourceServerContextTest)'" } + # Named by path because no job in this matrix runs the tests/ root, only its + # subdirectories - a file added there runs nowhere unless it is listed here. + - { name: "PresentationMediaUploads", filter: "tests/PresentationMediaUploadsTest.php tests/PresentationMediaUploadsVisibilityTest.php tests/PresentationSerializerCacheKeyTest.php" } env: OTEL_SERVICE_ENABLED: false APP_ENV: testing diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index f31fbde16..b94d37f3b 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -12,9 +12,11 @@ * limitations under the License. **/ +use App\Security\SummitScopes; use Illuminate\Support\Facades\Cache; use Illuminate\Support\Facades\Log; use Libs\ModelSerializers\AbstractSerializer; +use models\oauth2\IResourceServerContext; use models\summit\Presentation; use models\summit\PresentationType; @@ -26,6 +28,14 @@ class PresentationSerializer extends SummitEventSerializer { const CacheTTL = 1200; + /** + * Memo for getMediaUploadsSerializerType(). Instance-scoped on purpose - see that method. + * Private rather than protected: the subclasses that override the method answer with a + * constant and have nothing to memo. + * @var string|null + */ + private ?string $media_uploads_serializer_type = null; + protected static $array_mappings = [ 'CreatorId' => 'creator_id:json_int', 'ModeratorId' => 'moderator_speaker_id:json_int', @@ -79,16 +89,136 @@ class PresentationSerializer extends SummitEventSerializer ]; /** + * Resolves who is allowed to see every media upload attached to this presentation, approved + * or not. The reference point is OAuth2SummitEventsApiController::getSerializerType(), which + * decides the serializer type of the presentation itself - where this method is narrower than + * that one the presentation is served Private while its uploads are served Public, which is + * the bug it used to have for summit admins and for service accounts. + * + * It is deliberately still narrower in one place. The controller grants Private to any + * ApplicationType_Service caller; here a service account additionally has to hold + * ReadAllPresentationMediaUploads, because these are unpublished files and the application + * type on its own would hand them to every service client. Members are aligned with the + * controller exactly. + * + * Two distinct privileged callers: + * + * - Service accounts (client_credentials, so getCurrentUser() is null by construction) that + * hold the dedicated snapshot scope. The content pipeline stages files pre-event, so it + * needs unapproved uploads. Gated on the scope and not on ApplicationType_Service alone: + * the application type on its own would hand drafts to every service client. + * - Members with an admin-level group, or with edit rights over this presentation + * (creator / moderator / speaker). + * * @return string */ protected function getMediaUploadsSerializerType():string{ + // Memoized per serializer instance, which is the correct scope and not merely the + // convenient one: memberCanEdit() below is answered against THIS presentation, so a + // caller can be a speaker on one and a stranger to the next. A request-wide memo would + // hand every presentation the first one's answer. SerializerRegistry builds a fresh + // serializer per object and none outlive the request, so per-instance already collapses + // the repeated work - this method is called once per media upload plus once per + // getVisibleMediaUploads(), and it reaches the member's speaker and this presentation's + // speaker collection each time. + if (!is_null($this->media_uploads_serializer_type)) + return $this->media_uploads_serializer_type; + + // && short-circuits, so getCurrentScope() is only reached for service accounts + $isSnapshotClient = + $this->resource_server_context->getApplicationType() === IResourceServerContext::ApplicationType_Service + && in_array + ( + SummitScopes::ReadAllPresentationMediaUploads, + $this->resource_server_context->getCurrentScope() + ); + + if ($isSnapshotClient) + return $this->media_uploads_serializer_type = SerializerRegistry::SerializerType_Private; + $serializerType = SerializerRegistry::SerializerType_Public; $currentUser = $this->resource_server_context->getCurrentUser(); $presentation = $this->object; - if(!is_null($currentUser) && ( $currentUser->isAdmin() || $presentation->memberCanEdit($currentUser))){ + if(!is_null($currentUser) && ( $currentUser->isAdmin() || $currentUser->isSummitAdmin() || $presentation->memberCanEdit($currentUser))){ $serializerType = SerializerRegistry::SerializerType_Private; } - return $serializerType; + return $this->media_uploads_serializer_type = $serializerType; + } + + /** + * Media uploads visible to the resolved serializer type. A Public caller (no admin/editor + * privilege on this presentation) only ever sees uploads marked display_on_site=true — an + * uploaded-but-not-yet-approved draft (display_on_site=false, the model's own default) must + * never reach an unauthenticated/public response, even via ?expand=media_uploads on the + * public events/published endpoints. + * @return \Doctrine\Common\Collections\Collection|PresentationMediaUpload[] + */ + protected function getVisibleMediaUploads() + { + $presentation = $this->object; + $mediaUploads = $presentation->getMediaUploads(); + if ($this->getMediaUploadsSerializerType() === SerializerRegistry::SerializerType_Private) { + return $mediaUploads; + } + return $mediaUploads->filter(function ($mediaUpload) { + return $mediaUpload->getDisplayOnSite(); + }); + } + + /** + * Sets media_uploads on an already-built payload for whoever is asking right now, in the + * shape the request asked for: an id list for ?relations=media_uploads, serialized objects + * for ?expand=media_uploads, expand winning when both are present. + * + * This is the only place that decides the value, and it runs on every path - including + * after a cache read - because getMediaUploadsSerializerType() resolves per user and per + * scope, which nothing in the cache key expresses. Two callers can share a key and still + * disagree here: a speaker on the presentation and a plain attendee both serialize through + * PresentationSerializer, and a service account holding ReadAllPresentationMediaUploads and + * one without it both serialize through AdminPresentationSerializer. Adding the serializer + * class to the key would not separate either pair. + * + * @param array $values + * @param null $expand + * @param array $fields + * @param array $relations + * @return array + */ + private function withMediaUploads(array $values, $expand, array $fields, array $relations): array + { + // Nothing asked for it: drop whatever a cached payload may be carrying, so a stale + // entry can never contribute this field to a response that did not request it. + unset($values['media_uploads']); + + if (in_array('media_uploads', $relations)) { + $media_uploads = []; + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { + $media_uploads[] = $mediaUpload->getId(); + } + $values['media_uploads'] = $media_uploads; + } + + if (!empty($expand)) { + foreach (explode(',', $expand) as $relation) { + if (trim($relation) !== 'media_uploads') continue; + + $media_uploads = []; + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { + $media_uploads[] = SerializerRegistry::getInstance()->getSerializer + ( + $mediaUpload, $this->getMediaUploadsSerializerType() + )->serialize + ( + AbstractSerializer::filterExpandByPrefix($expand, 'media_uploads'), + AbstractSerializer::filterFieldsByPrefix($fields, 'media_uploads'), + AbstractSerializer::filterFieldsByPrefix($relations, 'media_uploads'), + ); + } + $values['media_uploads'] = $media_uploads; + } + } + + return $values; } @@ -104,50 +234,74 @@ public function serialize($expand = null, array $fields = [], array $relations = $presentation = $this->object; if(!$presentation instanceof Presentation) return []; - // Include last_edited timestamp so a presentation update naturally busts the cache - // without needing an explicit Cache::forget — the old key just ages out via TTL. + // fields and relations are read with in_array() throughout, so their order cannot change + // the payload and sorting lets two spellings of one request share an entry. $expand is + // deliberately NOT sorted: its relations are dispatched in the order given, and the + // speakers and moderator cases both write $values['moderator'] while disagreeing about + // moderator_speaker_id, so the order is capable of changing the result. + $cache_fields = $fields; + $cache_relations = $relations; + sort($cache_fields); + sort($cache_relations); + + // The digest covers everything that shapes the payload and is not already named in the + // readable part of the key. static::class is in it because this method is inherited: + // AdminPresentationSerializer and the track-chair and CSV serializers all cache through + // here, and their merged $array_mappings add fields the public serializer never emits. + // + // INVARIANT: anything the payload depends on either appears here or stays out of the + // cache. static::class covers audience today only because the remaining differences are + // class-determined - the mappings are static, getSerializerType() returns a constant per + // class, and media_uploads, the one per-user field, is stripped before Cache::put. A + // per-user value added to the mappings, or read before parent::serialize() rather than + // after it the way AdminPresentationCSVSerializer and TrackChairPresentationSerializer + // do, would silently break that. + // + // json_encode rather than concatenation: the parts contain "_" and "," themselves + // (media_uploads, extra_questions, selection_plan), so joining them on those characters + // let distinct requests render one key. sha256 rather than md5 because the parts come + // from the query string and a collision here means serving one audience's payload to + // another - the exact failure the class component is here to prevent. + // + // last_edited stays readable so a presentation update naturally busts every entry it has + // without an explicit Cache::forget, and so an operator can still scan or drop one + // presentation's entries by pattern. + // The parts come raw off the query string, and percent-decoding hands them over as + // bytes: a malformed sequence like %FF makes json_encode() return false, which hash() + // would silently coerce to "" - collapsing class, expand, fields and relations onto one + // shared digest per presentation, the exact cross-audience collision the digest exists + // to prevent. A request that cannot be keyed unambiguously bypasses the cache entirely. + $digest_source = json_encode + ([ + 'serializer' => static::class, + 'expand' => $expand ?? "", + 'fields' => $cache_fields, + 'relations' => $cache_relations, + ]); + $key = sprintf ( - "public_presentation_%s_%s_%s_%s_%s", + "presentation_%s_%s_%s", $presentation->getId(), $presentation->getLastEditedUTC()?->getTimestamp() ?? 0, - $expand ?? "", - implode(",",$fields), - implode(",", $relations) + hash('sha256', (string) $digest_source) ); - $use_cache = $params['use_cache'] ?? false; - - if($use_cache && Cache::has($key)){ - $values = json_decode(Cache::get($key), true); - Log::debug(sprintf("PresentationSerializer::serialize cache hit for presentation %s", $presentation->getId())); - if (!empty($expand)) { - foreach (explode(',', $expand) as $relation) { - $relation = trim($relation); - switch ($relation) { - case 'media_uploads': - { - $media_uploads = []; + $use_cache = ($params['use_cache'] ?? false) && $digest_source !== false; - foreach ($presentation->getMediaUploads() as $mediaUpload) { - $media_uploads[] = SerializerRegistry::getInstance()->getSerializer - ( - $mediaUpload, $this->getMediaUploadsSerializerType() - )->serialize - ( - AbstractSerializer::filterExpandByPrefix($expand, $relation), - AbstractSerializer::filterFieldsByPrefix($fields, $relation), - AbstractSerializer::filterFieldsByPrefix($relations, $relation), - ); - } - - $values['media_uploads'] = $media_uploads; - } - } - } + if($use_cache){ + // One read, not Cache::has() followed by Cache::get(): the entry can expire on its + // own TTL, be evicted, or be flushed in the window between the two, and the second + // call would then hand back null for a key the first call vouched for. Anything that + // does not decode to an array - a miss, that race, a truncated write - falls through + // and is rebuilt. + $cached = Cache::get($key); + $values = is_string($cached) ? json_decode($cached, true) : null; + if(is_array($values)){ + Log::debug(sprintf("PresentationSerializer::serialize cache hit for presentation %s", $presentation->getId())); + return $this->withMediaUploads($values, $expand, $fields, $relations); } - return $values; } $values = parent::serialize($expand, $fields, $relations, $params); @@ -192,16 +346,6 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['videos'] = $videos; } - if(in_array('media_uploads', $relations)) - { - $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { - $media_uploads[] = $mediaUpload->getId(); - } - - $values['media_uploads'] = $media_uploads; - } - if(in_array('extra_questions', $relations)) { $answers = []; @@ -334,24 +478,6 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['videos'] = $videos; } break; - case 'media_uploads':{ - $media_uploads = []; - - foreach ($presentation->getMediaUploads() as $mediaUpload) { - $media_uploads[] = SerializerRegistry::getInstance()->getSerializer - ( - $mediaUpload, $this->getMediaUploadsSerializerType() - )->serialize - ( - AbstractSerializer::filterExpandByPrefix($expand, $relation), - AbstractSerializer::filterFieldsByPrefix($fields, $relation), - AbstractSerializer::filterFieldsByPrefix($relations, $relation), - ); - } - - $values['media_uploads'] = $media_uploads; - } - break; case 'extra_questions':{ $answers = []; foreach ($presentation->getExtraQuestionAnswers() as $answer) { @@ -401,9 +527,16 @@ public function serialize($expand = null, array $fields = [], array $relations = } } - if($use_cache) - Cache::put($key, json_encode($values), self::CacheTTL); + if($use_cache) { + // media_uploads is deliberately kept out of the stored payload: it is the one field + // here whose value depends on who is asking, and the key has no audience component. + // Storing it would make correctness depend on every future reader remembering to + // recompute it. + $cacheable = $values; + unset($cacheable['media_uploads']); + Cache::put($key, json_encode($cacheable), self::CacheTTL); + } - return $values; + return $this->withMediaUploads($values, $expand, $fields, $relations); } } diff --git a/app/Security/SummitScopes.php b/app/Security/SummitScopes.php index 5911424aa..b8ed477e2 100644 --- a/app/Security/SummitScopes.php +++ b/app/Security/SummitScopes.php @@ -22,6 +22,7 @@ final class SummitScopes const ReadSummitData = SCOPE_BASE_REALM.'/summits/read'; const ReadAllSummitData = SCOPE_BASE_REALM.'/summits/read/all'; const ReadOverflowEvents = SCOPE_BASE_REALM.'/summits/events/overflow/read'; + const ReadAllPresentationMediaUploads = SCOPE_BASE_REALM.'/summits/presentations/media-uploads/read/all'; // me const MeRead = SCOPE_BASE_REALM.'/me/read'; diff --git a/database/migrations/config/Version20260804120000.php b/database/migrations/config/Version20260804120000.php new file mode 100644 index 000000000..3f07c297d --- /dev/null +++ b/database/migrations/config/Version20260804120000.php @@ -0,0 +1,61 @@ +addSql($this->insertApiScope( + self::API_NAME, + SummitScopes::ReadAllPresentationMediaUploads, + 'Read All Presentation Media Uploads', + 'Grants read access to presentation media uploads regardless of display_on_site, for trusted service accounts feeding the content pipeline' + )); + } + + public function down(Schema $schema): void + { + $this->addSql($this->deleteApiScopes(self::API_NAME, [SummitScopes::ReadAllPresentationMediaUploads])); + } +} diff --git a/database/seeders/ApiScopesSeeder.php b/database/seeders/ApiScopesSeeder.php index 6eab76a31..de32ab477 100644 --- a/database/seeders/ApiScopesSeeder.php +++ b/database/seeders/ApiScopesSeeder.php @@ -68,6 +68,11 @@ private function seedSummitScopes() 'short_description' => 'Read Summit Overflow Events Data', 'description' => 'Grants read only access to published summit events currently in OVERFLOW occupancy, including overflow streaming URLs and tokens', ], + [ + 'name' => SummitScopes::ReadAllPresentationMediaUploads, + 'short_description' => 'Read All Presentation Media Uploads', + 'description' => 'Grants read access to presentation media uploads regardless of display_on_site, for trusted service accounts feeding the content pipeline', + ], [ 'name' => SummitScopes::MeRead, 'short_description' => 'Get own summit member data', diff --git a/tests/PresentationMediaUploadsTests.php b/tests/PresentationMediaUploadsTest.php similarity index 74% rename from tests/PresentationMediaUploadsTests.php rename to tests/PresentationMediaUploadsTest.php index 7bd1e5d5b..834d076d6 100644 --- a/tests/PresentationMediaUploadsTests.php +++ b/tests/PresentationMediaUploadsTest.php @@ -19,9 +19,9 @@ use models\summit\Presentation; use models\summit\SummitMediaUploadType; /** - * Class PresentationMediaUploadsTests + * Class PresentationMediaUploadsTest */ -class PresentationMediaUploadsTests +class PresentationMediaUploadsTest extends ProtectedApiTestCase { use InsertSummitTestData; @@ -44,17 +44,32 @@ protected function setUp():void { parent::setUp(); self::$media_file_type_repository = EntityManager::getRepository(SummitMediaFileType::class); - $types = self::$media_file_type_repository->findAll(); self::insertSummitTestData(); + + // Built here rather than read from the repository: insertSummitTestData() opens with + // DELETE FROM SummitMediaFileType, so anything fetched before it is a detached row by + // the time we flush. It cannot reuse self::$default_media_file_type either - that one + // carries ".PDF", and SummitMediaUploadType::isValidExtension() compares + // strtoupper($ext) against explode('|', ...), so a leading dot never matches. + $media_file_type = new SummitMediaFileType(); + $media_file_type->setName("PNG_".rand(1, 100)); + $media_file_type->setDescription("PNG"); + $media_file_type->setAllowedExtensions("PNG"); + self::$em->persist($media_file_type); + self::$media_upload_type = new SummitMediaUploadType(); - self::$media_upload_type->setType($types[0]); + self::$media_upload_type->setType($media_file_type); self::$media_upload_type->setName('TEST'); self::$media_upload_type->setDescription("TEST"); self::$media_upload_type->setMaxSize(2048); self::$media_upload_type->setMinUploadsQty(2); self::$media_upload_type->setMaxUploadsQty(4); - self::$media_upload_type->setPrivateStorageType(\App\Models\Utils\IStorageTypesConstants::DropBox); - self::$media_upload_type->setPublicStorageType(\App\Models\Utils\IStorageTypesConstants::Swift); + self::$media_upload_type->setPrivateStorageType(\App\Models\Utils\IStorageTypesConstants::Local); + // Local, not Swift: serializing public_url builds a download strategy for whatever the + // type declares, and the Swift one needs an authUrl that neither this container nor CI + // provides. The assertion is about public_url being serialized at all, not about which + // backend serves it. + self::$media_upload_type->setPublicStorageType(\App\Models\Utils\IStorageTypesConstants::Local); self::$presentation = new Presentation(); $event_types = self::$summit->getEventTypes(); diff --git a/tests/PresentationMediaUploadsVisibilityTest.php b/tests/PresentationMediaUploadsVisibilityTest.php new file mode 100644 index 000000000..d479514cf --- /dev/null +++ b/tests/PresentationMediaUploadsVisibilityTest.php @@ -0,0 +1,273 @@ +shouldReceive('getId')->andReturn(self::ApprovedUploadId); + $approved->shouldReceive('getDisplayOnSite')->andReturn(true); + + $draft = Mockery::mock(PresentationMediaUpload::class); + $draft->shouldReceive('getId')->andReturn(self::DraftUploadId); + $draft->shouldReceive('getDisplayOnSite')->andReturn(false); + + $presentation = Mockery::mock(Presentation::class); + $presentation->shouldReceive('getId')->andReturn($identifier); + $presentation->shouldReceive('getLastEditedUTC')->andReturn(null); + $presentation->shouldReceive('getMediaUploads') + ->andReturn(new ArrayCollection([$approved, $draft])); + $presentation->shouldReceive('memberCanEdit')->andReturn($member_can_edit); + + return $presentation; + } + + /** + * A member-backed caller: a browser client carrying a user token. + * @param bool $is_admin global administrator. + * @param bool $is_summit_admin summit-front-end-administrators, the show-admin operators. + * @return IResourceServerContext + */ + private function buildMemberContext(bool $is_admin = false, bool $is_summit_admin = false): IResourceServerContext + { + $member = Mockery::mock(Member::class); + $member->shouldReceive('isAdmin')->andReturn($is_admin); + $member->shouldReceive('isSummitAdmin')->andReturn($is_summit_admin); + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn($member); + + return $context; + } + + /** + * A client_credentials caller. getCurrentUser() is null by construction - there is no user + * behind the token - which is why the scope, and not the member, is what grants access here. + * @param bool $with_scope whether the token carries ReadAllPresentationMediaUploads. + * @return IResourceServerContext + */ + private function buildServiceContext(bool $with_scope): IResourceServerContext + { + $scopes = ['%s/summits/read']; + if ($with_scope) $scopes[] = SummitScopes::ReadAllPresentationMediaUploads; + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType') + ->andReturn(IResourceServerContext::ApplicationType_Service); + $context->shouldReceive('getCurrentScope')->andReturn($scopes); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + return $context; + } + + /** + * @param Presentation $presentation + * @param IResourceServerContext $context + * @param array $params forwarded to serialize(), which is where use_cache is read. + * @return array the media upload ids this caller receives + */ + private function serializeMediaUploadIds( + Presentation $presentation, + IResourceServerContext $context, + array $params = [] + ): array + { + $serializer = new PresentationSerializer($presentation, $context); + // fields is narrowed to id so the attribute-mapping loop in AbstractSerializer only + // reaches Presentation::getId(); every other mapped getter is irrelevant here and would + // otherwise have to be stubbed for no gain. + $values = $serializer->serialize(null, ['id'], ['media_uploads'], $params); + + $this->assertArrayHasKey('media_uploads', $values); + return $values['media_uploads']; + } + + public function testAnonymousCallerSeesOnlyApprovedUploads() + { + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + $ids = $this->serializeMediaUploadIds($this->buildPresentation(90101), $context); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + public function testPlainAttendeeSeesOnlyApprovedUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90102, false), + $this->buildMemberContext() + ); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + public function testSpeakerOnThePresentationSeesDraftUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90103, true), + $this->buildMemberContext() + ); + + $this->assertSame([self::ApprovedUploadId, self::DraftUploadId], $ids); + } + + /** + * The regression this pins: before the fix a summit admin resolved Public here while + * OAuth2SummitEventsApiController::getSerializerType() resolved Private for the presentation + * itself, so the summit-admin event grid stopped showing the upload whose display_on_site + * checkbox is the only way to approve it. + */ + public function testSummitAdminSeesDraftUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90104, false), + $this->buildMemberContext(false, true) + ); + + $this->assertSame([self::ApprovedUploadId, self::DraftUploadId], $ids); + } + + public function testServiceAccountWithScopeSeesDraftUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90105), + $this->buildServiceContext(true) + ); + + $this->assertSame([self::ApprovedUploadId, self::DraftUploadId], $ids); + } + + /** + * The other half of that gate: the access is granted by the scope, not by the application + * type, so a service client without it stays where every other unprivileged caller is. + */ + public function testServiceAccountWithoutScopeSeesOnlyApprovedUploads() + { + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90106), + $this->buildServiceContext(false) + ); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + /** + * A cached entry that reports as present and then reads back as unavailable must degrade to + * a fresh build, not to an error. The entry can expire on its own TTL, be evicted under + * memory pressure, or be dropped by a cache flush, and none of that is rare enough on the + * voteable-presentation endpoints - the only ones that pass use_cache - to leave unhandled. + * + * The assertion is on the payload rather than on how the cache was consulted, so it holds + * whether the read is one call or two. + */ + public function testUnavailableCachedValueIsTreatedAsAMiss() + { + Cache::shouldReceive('has')->zeroOrMoreTimes()->andReturn(true); + Cache::shouldReceive('get')->zeroOrMoreTimes()->andReturn(null); + Cache::shouldReceive('put')->zeroOrMoreTimes()->andReturn(true); + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + $ids = $this->serializeMediaUploadIds( + $this->buildPresentation(90107), + $context, + ['use_cache' => true] + ); + + $this->assertSame([self::ApprovedUploadId], $ids); + } + + /** + * The other side of that read: a decodable entry is still served from cache, and the + * media_uploads on it are still resolved for the caller asking now rather than taken from + * whoever populated the key. Without this the previous test would pass just as well against + * a serializer that had stopped reading the cache altogether. + */ + public function testCacheHitIsServedButMediaUploadsAreResolvedFresh() + { + // A payload as it is stored: media_uploads is absent by construction, and the stale value + // is one no unprivileged caller may receive. + Cache::shouldReceive('has')->zeroOrMoreTimes()->andReturn(true); + Cache::shouldReceive('get')->zeroOrMoreTimes()->andReturn( + json_encode(['id' => 90108, 'title' => 'from cache', 'media_uploads' => [self::DraftUploadId]]) + ); + Cache::shouldReceive('put')->zeroOrMoreTimes()->andReturn(true); + + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + + $serializer = new PresentationSerializer($this->buildPresentation(90108), $context); + $values = $serializer->serialize(null, ['id'], ['media_uploads'], ['use_cache' => true]); + + // Came off the cached payload: parent::serialize() was never reached, and it does not + // produce this field under fields=['id'] anyway. + $this->assertSame('from cache', $values['title']); + // ...but this one did not. + $this->assertSame([self::ApprovedUploadId], $values['media_uploads']); + } +} diff --git a/tests/PresentationSerializerCacheKeyTest.php b/tests/PresentationSerializerCacheKeyTest.php new file mode 100644 index 000000000..071fd2f26 --- /dev/null +++ b/tests/PresentationSerializerCacheKeyTest.php @@ -0,0 +1,215 @@ +store = []; + // A stateful fake rather than the configured driver: it keeps the test off redis/file, + // and the entry count is the whole point of these assertions. + Cache::shouldReceive('put')->andReturnUsing(function ($key, $value, $ttl = null) { + $this->store[$key] = $value; + return true; + }); + Cache::shouldReceive('get')->andReturnUsing(function ($key, $default = null) { + return $this->store[$key] ?? $default; + }); + Cache::shouldReceive('has')->andReturnUsing(function ($key) { + return array_key_exists($key, $this->store); + }); + } + + public function tearDown(): void + { + Mockery::close(); + parent::tearDown(); + } + + /** + * @param int $identifier + * @return Presentation + */ + private function buildPresentation(int $identifier): Presentation + { + $presentation = Mockery::mock(Presentation::class); + $presentation->shouldReceive('getId')->andReturn($identifier); + $presentation->shouldReceive('getLastEditedUTC')->andReturn(null); + $presentation->shouldReceive('getTitle')->andReturn('a presentation'); + $presentation->shouldReceive('getRank')->andReturn(7); + $presentation->shouldReceive('getMediaUploads')->andReturn(new ArrayCollection([])); + $presentation->shouldReceive('getSlides')->andReturn([]); + $presentation->shouldReceive('memberCanEdit')->andReturn(false); + // Reached only by testExpandOrderIsNotNormalised. getType() answering null short-circuits + // the moderator case before it asks for a moderator, which keeps the fixture to the two + // getters the expand dispatch actually needs. + $presentation->shouldReceive('getSpeakers')->andReturn([]); + $presentation->shouldReceive('getType')->andReturn(null); + return $presentation; + } + + /** + * An unauthenticated caller: whatever ends up in the cache, this is who must not receive the + * admin shape of it. + * @return IResourceServerContext + */ + private function buildPublicContext(): IResourceServerContext + { + $context = Mockery::mock(IResourceServerContext::class); + $context->shouldReceive('getApplicationType')->andReturn('JS_CLIENT'); + $context->shouldReceive('getCurrentUser')->andReturn(null); + return $context; + } + + /** + * The reason this ticket exists. AdminPresentationSerializer does not override serialize(), + * so it writes through the inherited caching path, and its attribute mappings are merged on + * top of the public ones - rank, selection_status, streaming_url, etherpad_link, + * overflow_stream_key, chair scores and vote stats all land in the stored payload. With no + * class component in the key, a public caller repeating the same query params inside the TTL + * reads that payload back verbatim. + */ + public function testAdminPayloadIsNotServedToAPublicCaller() + { + $presentation = $this->buildPresentation(90201); + $context = $this->buildPublicContext(); + $arguments = [null, ['id', 'rank'], [], ['use_cache' => true]]; + + $admin = (new AdminPresentationSerializer($presentation, $context))->serialize(...$arguments); + // Sanity: the field really is admin-only, so the assertion below is about the cache and + // not about a field nobody emits. + $this->assertSame(7, $admin['rank']); + + $public = (new PresentationSerializer($presentation, $context))->serialize(...$arguments); + + $this->assertArrayNotHasKey('rank', $public); + $this->assertCount(2, $this->store); + } + + /** + * The key's parts used to be joined with "_", a character that occurs inside the values it + * joins - media_uploads, extra_questions, selection_plan, public_comments. Two different + * requests could therefore render the same key: expand=media_uploads&fields=x and + * expand=media&fields=uploads_x both flattened to "..._media_uploads_x_". + * + * Here the second request asks for a field that matches no mapping, so its correct payload is + * empty; anything it comes back with was somebody else's. + */ + public function testRequestsThatFlattenAlikeDoNotShareAnEntry() + { + $presentation = $this->buildPresentation(90202); + $context = $this->buildPublicContext(); + + $first = (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id'], ['media_uploads'], ['use_cache' => true]); + $this->assertSame(90202, $first['id']); + + $second = (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id_media'], ['uploads'], ['use_cache' => true]); + + $this->assertArrayNotHasKey('id', $second); + $this->assertCount(2, $this->store); + } + + /** + * fields and relations are both consumed with in_array(), so their order cannot change the + * payload. Leaving them unsorted just spends a second entry on a request already answered. + */ + public function testFieldAndRelationOrderReuseTheSameEntry() + { + $presentation = $this->buildPresentation(90203); + $context = $this->buildPublicContext(); + + (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id', 'title'], ['slides', 'media_uploads'], ['use_cache' => true]); + (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['title', 'id'], ['media_uploads', 'slides'], ['use_cache' => true]); + + $this->assertCount(1, $this->store); + } + + /** + * expand is deliberately left alone. Its relations are dispatched in the order given, and the + * speakers and moderator cases both write $values['moderator'] while disagreeing about + * moderator_speaker_id - speakers reads it, moderator unsets it - so the order is capable of + * changing the result. Normalising it would quietly merge two payloads that are allowed to + * differ, which is the failure this whole ticket is about. + */ + public function testExpandOrderIsNotNormalised() + { + $presentation = $this->buildPresentation(90204); + $context = $this->buildPublicContext(); + + (new PresentationSerializer($presentation, $context)) + ->serialize('speakers,moderator', ['id'], [], ['use_cache' => true]); + (new PresentationSerializer($presentation, $context)) + ->serialize('moderator,speakers', ['id'], [], ['use_cache' => true]); + + $this->assertCount(2, $this->store); + } + + /** + * The digest parts come raw off the query string, and percent-decoding hands them over as + * bytes: expand=%FF arrives as "\xFF", which is not valid UTF-8, so json_encode() returns + * false - and hash() coerces that false to "" without a warning. Every request carrying any + * malformed byte then shares one digest per presentation, serializer class included, which + * is exactly the admin-payload-to-public-caller collision the class component exists to + * prevent. A request that cannot be keyed unambiguously must not touch the cache at all. + */ + public function testMalformedUtf8RequestsDoNotCollideOnOneEntry() + { + $presentation = $this->buildPresentation(90205); + $context = $this->buildPublicContext(); + + $admin = (new AdminPresentationSerializer($presentation, $context)) + ->serialize(null, ['id', 'rank', "\xFF"], [], ['use_cache' => true]); + // Sanity, mirroring testAdminPayloadIsNotServedToAPublicCaller: the admin payload + // really carries the field whose leak is asserted below. + $this->assertSame(7, $admin['rank']); + + $public = (new PresentationSerializer($presentation, $context)) + ->serialize(null, ['id', "\xFE"], [], ['use_cache' => true]); + + // Before the guard both digests collapsed to hash("") and this came back with the + // admin-shaped payload, rank included. + $this->assertArrayNotHasKey('rank', $public); + $this->assertSame(90205, $public['id']); + } +}