Skip to content

fix: gate public presentation serialization on display_on_site for media uploads #577

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
JpMaxMan wants to merge 1 commit into
base: main
Choose a base branch
from
+23 −3
Open

fix: gate public presentation serialization on display_on_site for media uploads #577

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 23 additions & 3 deletions app/ModelSerializers/Summit/Presentation/PresentationSerializer.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JpMaxMan This filter is an off switch rather than a draft gate, because display_on_site is false on essentially every existing row.

Nothing in the submission flow ever sets it true. PresentationMaterial::__construct (PresentationMaterial.php:190) defaults it false, PresentationMediaUploadFactory::populate (:39-40) only calls setDisplayOnSite when the key is present in the payload, and call-for-presentations - the app speakers actually upload through - has zero occurrences of display_on_site in src/. The only writer is summit-admin's material form checkbox, which itself starts unchecked (event-material-reducer.js:30).

Concrete failure: pub-api's snapshot job requests expand=media_uploads with a client_credentials token, which resolves Public here, so every media_uploads array in events.json and presentations.json comes back empty. pub-api does not validate response shape - it only raises on transport errors - so the snapshot publishes normally and SnapshotCompleted fires. dropbox-materializer then iterates sess["media_uploads"] with no filter of its own and stages nothing, for every session, silently.

Suggested: get a production count first -

SELECT DisplayOnSite, COUNT(*) FROM PresentationMaterial
WHERE ClassName = 'PresentationMediaUpload' GROUP BY DisplayOnSite;

and gate this behind a backfill plus a defined write path for the flag. Also worth a test pinning both branches - the PR's own test plan is still unchecked, and this is exactly the case where behaviour depends entirely on data.

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update: the counts are in, and they change this recommendation rather than just confirming it.

Global split for PresentationMediaUpload: 707 rows at DisplayOnSite = 1, 4514 at 0. Per summit, every show since 63 is at exactly zero approved - summit 71, the one in the report and currently live, is 0 visible of 303 uploads across 166 published presentations. The 707 are concentrated in 63 (316), 31 (189) and 12 (169), where the flag was genuinely maintained: summit 63 has 282 hidden uploads but only 18 presentations that lose everything, which is what a working approval flow looks like. So the filter behaves exactly as designed where the practice was kept, and the practice stopped after 63.

I previously suggested gating this behind a backfill. Withdrawing that. There is nothing to preserve: DocumentsComponent.js:23 filters the same flag client-side, so event-site already renders zero media uploads for summit 71 and every recent show. Those files have always been in the payload and have never been displayed. This is by design, and documented - the internal JSON-contract doc lists media_uploads shipping display_on_site and public_url together, with the filtering deliberately placed in the client.

A blanket backfill is also wrong in both directions: on 12/31/63 it would publish material somebody deliberately left unapproved, and on the shows at zero the flag means "never triaged" rather than "reviewed and rejected", so approval cannot be inferred from it.

Net effect on this PR: no backfill needed, and no user-visible change on event-site. The remaining blockers are the serializer-type alignment and the dropbox-materializer feed, both noted in the other threads.

{
$presentation = $this->object;
$mediaUploads = $presentation->getMediaUploads();
if ($this->getMediaUploadsSerializerType() === SerializerRegistry::SerializerType_Private) {

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JpMaxMan Delegating to getMediaUploadsSerializerType() inherits a privilege gap this PR does not close: that method checks only isAdmin() || memberCanEdit(), so summit admins and service accounts both land on Public.

Compare OAuth2SummitEventsApiController::getSerializerType() (:127), which already resolves Private for isSummitAdmin() and for ApplicationType_Service. The presentation is served Private to those callers while its media uploads are served Public - the two decisions disagree.

Concrete failure, and it is circular: an operator in summit-front-end-administrators loads the summit-admin event grid, which requests media_uploads.display_on_site among its fields (event-actions.js:288) against GET /api/v1/summits/{id}/events. With this filter in place those rows disappear, so the operator cannot see the upload in order to tick the very checkbox that would make it visible again. The same gap is why pub-api's snapshot client gets Public output despite that controller granting service accounts Private.

Suggested: align getMediaUploadsSerializerType() with the check the controller already performs - Private for isSummitAdmin() as well as isAdmin(), and for service accounts. The application-type pattern has serializer-layer precedent at PresentationSpeakerBaseSerializer.php:104-109. For the service-account half I would require a dedicated snapshot scope rather than ApplicationType_Service alone, so this does not hand drafts to every service client.

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concrete patch for this, since it is two files and easy to get subtly wrong.

1) app/Security/SummitScopes.php - new scope, following the existing pattern:

    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';

2) app/ModelSerializers/Summit/Presentation/PresentationSerializer.php - imports:

+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;

3) Same file - replace getMediaUploadsSerializerType():

    protected function getMediaUploadsSerializerType(): string
    {
        $currentUser  = $this->resource_server_context->getCurrentUser();
        $presentation = $this->object;

        // Service account (client_credentials => no current user) holding the dedicated
        // snapshot scope. The content pipeline needs unapproved uploads in order to stage
        // files pre-event. Narrowed by scope on purpose: ApplicationType_Service on its own
        // would hand drafts to every service client.
        $isSnapshotClient =
            $this->resource_server_context->getApplicationType() === IResourceServerContext::ApplicationType_Service
            && in_array(
                SummitScopes::ReadAllPresentationMediaUploads,
                $this->resource_server_context->getCurrentScope()
            );

        if ($isSnapshotClient)
            return SerializerRegistry::SerializerType_Private;

        if (!is_null($currentUser)
            && ($currentUser->isAdmin()
                || $currentUser->isSummitAdmin()      // <- show admin operators, currently missing
                || $presentation->memberCanEdit($currentUser))) {
            return SerializerRegistry::SerializerType_Private;
        }

        return SerializerRegistry::SerializerType_Public;
    }

Notes on the shape:

  • getVisibleMediaUploads() needs no change. It already keys off this method, which is why that hook was the right choice.
  • TrackChairPresentationSerializer.php:59 overrides this and returns Private unconditionally, so track chairs are unaffected either way.
  • The in_array scope check follows the precedent already in the serializer layer at SummitSerializer.php:372-377, and getCurrentScope() returns an array (see OAuth2SummitEventsApiController.php:125). The && short-circuits, so getCurrentScope() is only reached for service accounts.
  • isSummitAdmin() resolves summit-front-end-administrators (Member::isSummitAdmin), which is the group the show-admin operators are in and the reason the grid goes blank today.

Out-of-code step, easy to forget: the new scope has to be provisioned on the content-snapshot client in the IDP, and added to pub-api's CONTENT_SNAPSHOT_OAUTH2_SCOPES. Until both are done pub-api keeps resolving Public and the snapshot still empties.

Worth a test per branch on this method: anonymous, attendee, speaker on the presentation, summit admin, service account with the scope, service account without it. Six cases, all cheap, and they pin the exact behaviour this PR depends on.

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Follow-up on the scope: it needs a seeder entry and a migration, and the answer to "which endpoint does it attach to" is none. Worth spelling out because the obvious precedent points the wrong way.

Why no endpoint association. OAuth2BearerAccessTokenRequestValidator.php:193 rejects with count(array_intersect($endpoint_scopes, $token_scopes)) == 0, so endpoint scopes are any-of, not all-of. Associating this scope with /summits/{id}/events/published would mean a token holding only this scope is admitted to that endpoint - widening access rather than narrowing it. And it is unnecessary: getMediaUploadsSerializerType() reads getCurrentScope() off the token directly and never consults endpoint_api_scopes.

That is the difference from ReadOverflowEvents (Version20260715120000.php), which was minted alongside a brand-new endpoint it gates. This one is a privilege modifier over endpoints that already exist.

Two registries, only one of which is load-bearing.

  • summit-api config DB (apis / api_scopes / endpoint_api_scopes): validates that an incoming token intersects the endpoint's scopes. Registering the scope here is convention and discoverability (Swagger security annotations), not a functional requirement for this check.
  • openstackid (oauth2_api_scope): issues the token. This is the one that matters. If the scope does not exist there and is not granted to the content-snapshot client, the token never carries it and the serializer never sees it. openstackid/database/seeds/ApiScopeSeeder.php only seeds the IDP's own APIs - no summit scopes - so this is administered through the IDP admin UI, an ops step rather than a code change.

1) database/seeders/ApiScopesSeeder.php, inside seedSummitScopes(), next to ReadOverflowEvents:

            [
                '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',
            ],

2) database/migrations/config/Version20260804120000.php (latest currently is Version20260715120000):

<?php namespace Database\Migrations\Config;

use App\Security\SummitScopes;
use Doctrine\DBAL\Schema\Schema;
use Doctrine\Migrations\AbstractMigration;

/**
 * Register the ReadAllPresentationMediaUploads scope.
 *
 * Adds 1 api_scopes row and NO endpoint association, on purpose: endpoint scopes are
 * validated with array_intersect (any-of, see OAuth2BearerAccessTokenRequestValidator:193),
 * so associating this scope with an existing endpoint would admit a token holding only
 * this scope. The scope is read directly off the token by
 * PresentationSerializer::getMediaUploadsSerializerType(), which never consults
 * endpoint_api_scopes.
 *
 * Idempotent via WHERE NOT EXISTS.
 */
final class Version20260804120000 extends AbstractMigration
{
    use APIEndpointsMigrationHelper;

    private const API_NAME = 'summits';

    public function getDescription(): string
    {
        return 'Register ReadAllPresentationMediaUploads scope (no endpoint association).';
    }

    public function up(Schema $schema): void
    {
        $this->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]));
    }
}

insertApiScope() is independent of registerEndpoint() in APIEndpointsMigrationHelper, so a scope with no endpoint association is a supported shape rather than a workaround.

Rollout order matters: IDP scope + client grant first, then pub-api's CONTENT_SNAPSHOT_OAUTH2_SCOPES, then this PR. Land the serializer filter before the client actually holds the scope and the snapshot empties in between.

return $mediaUploads;
}
return $mediaUploads->filter(function ($mediaUpload) {
return $mediaUpload->getDisplayOnSite();
});
}


/**
* @param null $expand
Expand Down Expand Up @@ -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) {

@coderabbitai coderabbitai Bot Aug 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Expect private/editor callers to bypass this cache, or cache keys to include serializer type.
rg -n -C 12 'PresentationSerializer|public_presentation_|use_cache' .

Repository: OpenStackweb/summit-api

Length of output: 50380

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Inspect the relevant PresentationSerializer paths and all passing of use_cache.
printf '--- PresentationSerializer relevant lines ---\n'
sed -n '1,240p;400,430p' app/ModelSerializers/Summit/Presentation/PresentationSerializer.php | cat -n

printf '\n--- use_cache usages (concise) ---\n'
rg -n --glob '!adr/**' 'use_cache|SerializerRegistry::getInstance\(\)->getSerializer\(|serialize\(' app tests tests-api || true

Repository: OpenStackweb/summit-api

Length of output: 252

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

# Inspect the relevant PresentationSerializer paths and all passing of use_cache.
printf '%s\n' '--- PresentationSerializer relevant lines ---'
sed -n '1,240p;400,430p' app/ModelSerializers/Summit/Presentation/PresentationSerializer.php | cat -n

printf '%s\n' ''
printf '%s\n' '--- use_cache usages (concise) ---'
rg -n --glob '!adr/**' 'use_cache|SerializerRegistry::getInstance\(\)->getSerializer\(|serialize\(' app tests tests-api || true

Repository: OpenStackweb/summit-api

Length of output: 50380

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- exact use_cache references outside tests ---'
rg -n --glob '!adr/**' --glob '!tests/**' 'use_cache' app

printf '%s\n' ''
printf '%s\n' '--- exact use_cache references in tests ---'
rg -n 'use_cache' tests || true

printf '%s\n' ''
printf '%s\n' '--- parent cache key helpers/usages ---'
rg -n --glob '!adr/**' 'build.*[Cc]ache[Kk]ey|get.*Cache.*Key|Cache::put|Cache::has' app/ModelSerializers | head -n 120

Repository: OpenStackweb/summit-api

Length of output: 1247

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- OAuth2SummitEventsApiController cache calls context ---'
sed -n '520,700p' app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php | cat -n

printf '%s\n' ''
printf '%s\n' '--- route/controller declarations around protected events ---'
rg -n --glob '!adr/**' --glob '!tests/**' 'OAuth2SummitEventsApiController|Protected/Summit|summit/events|/presentations' app/routes app/Http/Controllers/Apis app/Http/Controllers -g '*.php' | head -n 200

Repository: OpenStackweb/summit-api

Length of output: 40762

🏁 Script executed:

#!/usr/bin/env bash
set -euo pipefail

printf '%s\n' '--- SerializerRegistry resolved serializer and toArray ---'
sed -n '750,835p' app/ModelSerializers/SerializerRegistry.php | cat -n

printf '%s\n' ''
printf '%s\n' '--- OAuth2SummitEventsApiController serializer type logic ---'
sed -n '86,150p' app/Http/Controllers/Apis/Protected/Summit/OAuth2SummitEventsApiController.php | cat -n

Repository: OpenStackweb/summit-api

Length of output: 7021

Keep private presentation cache keys isolated.

Private/admin endpoints pass use_cache => true and are resolved to AdminPresentationSerializer, while cached PresentationSerializer output key is only "public_presentation_...". Store private data under a private-serializer cache key and apply visibility rules to cached relation IDs.

Also applies to: 215-219

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@app/ModelSerializers/Summit/Presentation/PresentationSerializer.php` at line
153, Update PresentationSerializer cache-key generation to distinguish public
output from private/admin output resolved through AdminPresentationSerializer,
using a private serializer-specific key for private data. Ensure cached relation
IDs still pass through the appropriate visibility rules before being returned,
including the logic around getVisibleMediaUploads and the related cache handling
at the referenced later section.

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JpMaxMan Pre-existing rather than introduced here, but it undercuts this PR's premise, so flagging it on the branch you are touching: the cache key is not privilege-scoped.

It is built from presentation id, last_edited, expand, fields and relations with no serializer-type or user component, while getAllVoteablePresentations passes $this->getSerializerType(), which resolves Private or Public per caller. For one combination of params, a payload built by a Private caller can be served to a Public one, and vice versa.

Recomputing media_uploads here covers this PR's field, but every other field the Admin serializer contributes still comes straight off the shared key. So "Public callers only ever see approved uploads" is not guaranteed by construction while this stands.

Suggested: add the resolved serializer type to the key. Worth its own ticket rather than expanding this PR's scope.

$media_uploads[] = SerializerRegistry::getInstance()->getSerializer
(
$mediaUpload, $this->getMediaUploadsSerializerType()
Expand Down Expand Up @@ -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) {

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JpMaxMan This call site never executes when caching is on, so the relations id-list path stays unfiltered.

The cached branch above returns at line 170, and on a cache hit it only recomputes media_uploads for the expand path. A request passing relations=media_uploads without expand=media_uploads gets the cached id array verbatim - built by whichever caller populated the key, filtered or not.

use_cache => true is set on the voteable-presentations endpoints (OAuth2SummitEventsApiController.php:554, :616, :685), which is where pub-api's presentations.json feed goes, so this is a live path rather than a theoretical one.

Suggested: handle the relations case inside the cache-hit branch the same way expand is handled, or drop media_uploads from the cached payload entirely and always recompute it after the cache lookup.

@smarcet smarcet Aug 4, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting evidence: track-chairs exercises this path in production. presentation-actions.js:145-173 sends both expand=media_uploads,media_uploads.media_upload_type and relations=media_uploads on the same request. It resolves Private via TrackChairPresentationSerializer.php:59, so nothing breaks there today, but it confirms the relations branch is a live code path rather than a theoretical one.

$media_uploads[] = $mediaUpload->getId();
}

Expand Down Expand Up @@ -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()
Expand Down
Loading