Skip to content
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.

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

cfp: Add Video download url #1768

Closed
wants to merge 2 commits into from
Closed

cfp: Add Video download url #1768

wants to merge 2 commits into from

Conversation

wlcx
Copy link
Member

@wlcx wlcx commented Aug 27, 2024

This is used to store a canonical url for the video recording of a CFP item, used to automate release of the recording by means of the schedule frab/json.
This is useful in particular when releasing videos via media.ccc.de which can import our frab schedule and automatically download the videos from the listed URLs, but it could also be used to automate youtube uploads, etc.
We also add the ability to set this via the proposal api endpoint.

I'm not entirely convinced this should this be implemented in this way though:

  • Arguably this information is only of use to known automated tools consuming the schedule - perhaps it should require a url flag or something to include it?
  • There are perhaps some minor security concerns around making publicly available the server being used to host the finished recordings
  • These URLs would likely be ephemeral, going away at some unspecified time post-event once video team are done releasing. There's not much use them being included in the schedule export and preserved for all time, for instance.

wlcx added 2 commits August 27, 2024 02:57
This is used to store a canonical url for the video recording of a CFP
item, used to automate release of the recording by means of the schedule
frab/json.
This is useful in particular when releasing videos via media.ccc.de
which can import our frab schedule and automatically download the videos
from the listed URLs, but it could also be used to automate youtube
uploads, etc.
We also add the ability to set this via the proposal api endpoint.
@wlcx wlcx requested a review from lukegb August 27, 2024 02:20
abort(401)

if auth_header.startswith("bearer "):
bearer_token = auth_header.removeprefix("bearer ")
Copy link
Member

@marksteward marksteward Aug 27, 2024

Choose a reason for hiding this comment

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

The contents of the Authorization header is case-sensitive, so "bearer" should be wrong. Can you include a comment to explain why this is needed? (So we can tidy it up when it's no longer the case.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I added it because I wasted 5 minutes scratching my head why my curl request was giving a 401 ;-)

I went looking for consensus online, always a mistake.. honestly, it seems like a bit of a mess: https://sgeb.io/posts/fix-go-oauth2-case-sensitive-bearer-auth-headers/

Given we're not implementing oauth here I figured it was a convenience and didn't worry about it too much.

Copy link
Member

Choose a reason for hiding this comment

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

Ahh, fair, # make it easier with curl would be fine then

@jayaddison
Copy link
Contributor

Same question about the contents of the authorization header, but apart from that: looks good to me 👍

@jayaddison
Copy link
Contributor

There are perhaps some minor security concerns around making publicly available the server being used to host the finished recordings

I don't know whether this would be necessary -- it depends on the workflows and server URL schemes involved -- but we could perhaps assert that the stored values don't contain any authentication details (http://user:[email protected]) and that they don't contain query-strings (sometimes used to embed API tokens - http://example.invalid/api/endoint?key=adsf) - to prevent some easily-avoidable accidental mistakes.

@jayaddison
Copy link
Contributor

Arguably this information is only of use to known automated tools consuming the schedule - perhaps it should require a url flag or something to include it?

Is the schedule format adjusted here only presented through the /schedule/<year>.frab endpoint?

@wlcx
Copy link
Member Author

wlcx commented Aug 27, 2024

From IRC:

@russss : will that end up in the static export? As long as it doesn't then I'm happy with it I think

Currently it will - since AFAICT the static export just dumps the response of the frab/json endpoints to disk. So we probably need to filter that out.

Part of me wonders if this is another reason to have a url param to include video_download_url in the schedule responses...

@wlcx
Copy link
Member Author

wlcx commented Aug 27, 2024

Arguably this information is only of use to known automated tools consuming the schedule - perhaps it should require a url flag or something to include it?

Is the schedule format adjusted here only presented through the /schedule/<year>.frab endpoint?

@jayaddison
Nope, json too

I don't know whether this would be necessary...
I was meaning in so far as we'd expose the location of the recorded videos, not that the url itself would contain tokens/auth

@wlcx
Copy link
Member Author

wlcx commented Aug 27, 2024

Closing - will generate this separately since it's a bit single-purpose and doesn't really need to live in the website.

@wlcx wlcx closed this Aug 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants