From 90904aa91ba3c6e1c717374075bcea3a8ae8122d Mon Sep 17 00:00:00 2001 From: JpMaxMan Date: Mon, 3 Aug 2026 23:36:19 -0500 Subject: [PATCH 1/4] fix: gate public presentation serialization on display_on_site for media uploads Public/anonymous callers to the events/published endpoints could pull full PresentationMediaUpload data -- including live public S3 URLs -- for draft (display_on_site=false) uploads via ?expand=media_uploads, since PresentationSerializer read the unfiltered media uploads collection. Reported externally: a third party's calendar-scraping agent recovered pre-event draft slide decks this way. getVisibleMediaUploads() now reuses the existing admin/editor privilege check to filter to display_on_site=true uploads for Public callers, at all three call sites in this file. AdminPresentationCSVSerializer (admin-only) is untouched. --- .../Presentation/PresentationSerializer.php | 26 ++++++++++++++++--- 1 file changed, 23 insertions(+), 3 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index f31fbde16..45747c397 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -91,6 +91,26 @@ protected function getMediaUploadsSerializerType():string{ return $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(); + }); + } + /** * @param null $expand @@ -130,7 +150,7 @@ public function serialize($expand = null, array $fields = [], array $relations = { $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { $media_uploads[] = SerializerRegistry::getInstance()->getSerializer ( $mediaUpload, $this->getMediaUploadsSerializerType() @@ -195,7 +215,7 @@ public function serialize($expand = null, array $fields = [], array $relations = if(in_array('media_uploads', $relations)) { $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { $media_uploads[] = $mediaUpload->getId(); } @@ -337,7 +357,7 @@ public function serialize($expand = null, array $fields = [], array $relations = case 'media_uploads':{ $media_uploads = []; - foreach ($presentation->getMediaUploads() as $mediaUpload) { + foreach ($this->getVisibleMediaUploads() as $mediaUpload) { $media_uploads[] = SerializerRegistry::getInstance()->getSerializer ( $mediaUpload, $this->getMediaUploadsSerializerType() From 7455f60068a4601dedcdb0d7f477aeab1c96952a Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 10:51:51 -0300 Subject: [PATCH 2/4] fix: grant media upload visibility to summit admins and snapshot service accounts getMediaUploadsSerializerType() only recognised isAdmin() and memberCanEdit(), so two callers that OAuth2SummitEventsApiController::getSerializerType() already treats as privileged fell through to Public and lost every media upload once the display_on_site filter landed: - summit admins (summit-front-end-administrators). The event grid requests media_uploads.display_on_site, so the operator could no longer see the upload whose checkbox is the only thing that would make it visible again. - the content-snapshot service account. pub-api reads media_uploads with a client_credentials token, which carries no user_id, so getCurrentUser() is null by construction; the snapshot emptied and dropbox-materializer, which does not filter on the flag itself, staged nothing for every session. Service accounts are gated on a dedicated scope rather than on ApplicationType_Service alone, which would have handed drafts to every service client. The scope is registered with no endpoint association on purpose: endpoint scopes are matched with array_intersect (any-of), so associating it would admit a token holding only this scope to that endpoint. It is read straight off the token and never consulted through endpoint_api_scopes. Rollout order: the scope has to exist in openstackid and be granted to the content-snapshot client, and be added to pub-api's CONTENT_SNAPSHOT_OAUTH2_SCOPES, before this ships - until then that client still resolves Public. --- .../Presentation/PresentationSerializer.php | 31 +++++++++- app/Security/SummitScopes.php | 1 + .../config/Version20260804120000.php | 61 +++++++++++++++++++ database/seeders/ApiScopesSeeder.php | 5 ++ 4 files changed, 97 insertions(+), 1 deletion(-) create mode 100644 database/migrations/config/Version20260804120000.php diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index 45747c397..b4dfe7a03 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; @@ -79,13 +81,40 @@ class PresentationSerializer extends SummitEventSerializer ]; /** + * Resolves who is allowed to see every media upload attached to this presentation, approved + * or not. Kept aligned with OAuth2SummitEventsApiController::getSerializerType(), which is + * what decides the serializer type of the presentation itself - when the two disagree the + * presentation is served Private while its uploads are served Public, which is the bug this + * method used to have for summit admins and for service accounts. + * + * 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{ + // && 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 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; 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', From b9ac1a4afc93cf0b5bd07dd0ff4a0c7a495c2de5 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 10:52:09 -0300 Subject: [PATCH 3/4] test: repair PresentationMediaUploadsTests fixture The file is under tests/ root and no CI job filter covers it (push.yml runs tests/oauth2/, tests/Unit/*, tests/Repositories/), so it rotted unnoticed and failed on any environment. Three separate causes: - setUp() read SummitMediaFileType via findAll() before insertSummitTestData(), which opens with DELETE FROM SummitMediaFileType. The entity it kept pointed at a deleted row, so the flush died on the SummitMediaUploadType.TypeID FK. - self::$default_media_file_type is not a usable substitute: it carries ".PDF", while SummitMediaUploadType::isValidExtension() compares strtoupper($ext) against explode('|', ...), so a leading dot can never match. The test builds its own type declaring PNG, matching the png it uploads and the format the seeder uses (JPG|JPEG|PNG). - the fixture declared Swift public storage, and serializing public_url builds a download strategy for it, which needs an authUrl that neither the local container nor CI provides. Local needs no credentials and the assertion is about public_url being serialized, not about the backend behind it. Green and repeatable across consecutive runs. --- tests/PresentationMediaUploadsTests.php | 23 +++++++++++++++++++---- 1 file changed, 19 insertions(+), 4 deletions(-) diff --git a/tests/PresentationMediaUploadsTests.php b/tests/PresentationMediaUploadsTests.php index 7bd1e5d5b..77d73e338 100644 --- a/tests/PresentationMediaUploadsTests.php +++ b/tests/PresentationMediaUploadsTests.php @@ -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(); From b4079911ec5474929222398c01d2ef4e7cf00478 Mon Sep 17 00:00:00 2001 From: smarcet Date: Tue, 4 Aug 2026 11:21:02 -0300 Subject: [PATCH 4/4] fix: keep media_uploads out of the presentation cache, resolve it per caller getMediaUploadsSerializerType() resolves per user and per OAuth scope, but the cache key is built from id + LastEditedUTC + expand + fields + relations and has no audience component. A payload built for a privileged caller could therefore be served verbatim to an unprivileged one within the 1200s TTL, handing out display_on_site=false uploads the display_on_site filter was added to withhold. Adding the serializer class to the key would not close it: a speaker on the presentation and a plain attendee both serialize through PresentationSerializer, and a service account with ReadAllPresentationMediaUploads and one without it both serialize through AdminPresentationSerializer. Each pair shares a class and disagrees on this field. So the field is never stored. Cache::put receives a copy with media_uploads removed, and a new private withMediaUploads() resolves it fresh on the way out of every path -- cache hit, cache miss, and the non-cached branch alike. It opens by unsetting the field, so a payload written before this change cannot leak one either. Request shape is preserved: an id list for relations=media_uploads, serialized objects for expand=media_uploads, expand winning when both are present. This also removes the three scattered copies of that expansion logic, which were the reason the cache-hit branch could drift from the others in the first place. --- .../Presentation/PresentationSerializer.php | 124 ++++++++++-------- 1 file changed, 67 insertions(+), 57 deletions(-) diff --git a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php index b4dfe7a03..765b6f9ab 100644 --- a/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php +++ b/app/ModelSerializers/Summit/Presentation/PresentationSerializer.php @@ -140,6 +140,62 @@ protected function getVisibleMediaUploads() }); } + /** + * 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; + } + /** * @param null $expand @@ -171,32 +227,7 @@ public function serialize($expand = null, array $fields = [], array $relations = 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 = []; - - foreach ($this->getVisibleMediaUploads() 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; - } - } - } - } - return $values; + return $this->withMediaUploads($values, $expand, $fields, $relations); } $values = parent::serialize($expand, $fields, $relations, $params); @@ -241,16 +272,6 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['videos'] = $videos; } - if(in_array('media_uploads', $relations)) - { - $media_uploads = []; - foreach ($this->getVisibleMediaUploads() as $mediaUpload) { - $media_uploads[] = $mediaUpload->getId(); - } - - $values['media_uploads'] = $media_uploads; - } - if(in_array('extra_questions', $relations)) { $answers = []; @@ -383,24 +404,6 @@ public function serialize($expand = null, array $fields = [], array $relations = $values['videos'] = $videos; } break; - case 'media_uploads':{ - $media_uploads = []; - - foreach ($this->getVisibleMediaUploads() 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) { @@ -450,9 +453,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); } }