Add support for event specific password protection #1244

owi92 · 2024-09-12T11:28:54Z

This adds the necessary backend and frontend implementations to allow event specific authentication as done by the ETH.

For technical information, see commit messages. @LukasKalbertodt to test this, you can use this DB command:

update all_events
set credentials = row(
    'sha1:9D4E1E23BD5B727046A9E3B4B7DB57BD8D6EE684',
    'sha1:3CBCD90ADC4B192A87A625850B7F231CADDF0EB3'
)::credentials
where title = 'Beyond Elasticsearch: user-facing search engines';

Of course you can use any other event. This will give the event the password user pass and the password word.

Test here

Closes #1186

LukasKalbertodt

Thanks for tackling this huge feature! The PR was nice to review commit by commit. And while below I have written quite a few comments, I think the general direction is absolutely correct, and all remarks are just "local problems", not really anything that requires rethinking everything.

Apart from the inline comments, while testing it I noticed these things:

The "in flight" circle still spins when invalid credentials are entered on video page. Once the error is shown, the spinner should disappear. This only happens when entering them incorrectly twice... what.
The whole page flashes when submitting credentials. Also, if the credentials were wrong, this means the entered credentials disappear, which is annoying if u only need to correct a typo.

I have to re-test everything again, but I think this is already a long enough review. I can test again after these things are fixed.

backend/src/db/migrations/39-event-preview-roles.sql

LukasKalbertodt · 2024-10-01T15:10:49Z

backend/src/db/migrations/39-event-preview-roles.sql

+-- For convenience, all read roles are also copied over to preview roles.
+-- Removing any roles from read will however _not_ remove them from preview, as they
+-- might also have been added separately and we can't really account for that.


Mhhh this is tricky. So the thing you describe isn't great, right? Because yes, if we mix them together, we have no idea what was originally set. Just so I understand correctly: the only (as in 'singular', not to diminish the importance) reason to mix them together is to have easier queries, right? So we don't have to check both columns everywhere? Or is there more to it?

My gut feeling is to keep them separate. We might have already talked about this, in which case I forgot and would be happy to be reminded :D

Maybe in the future, the ACL selector allows users to select preview, read or write. Well mh no, that could work with either model. I suppose there is no operation that allows users to "just remove a single action permission" from an event, which is what would trigger the thing you mentioned here. The harvest process would also overwrite the whole ACL and wouldn't cause a problem with either model.

So right now I cannot think of a case where your model would break... mhhh. I might think about it a bit more.

is there more to it?

no, I don't think there is.

We might have already talked about this

You suggested doing it like this when we first planned the whole thing. I then said "no lets not, because reasons™". But when implementing it, I realized that I actually find it nicer to have only one filter/check for access things, taking the cue from how read and write roles are handled (whenever something requiring read access is checked, it's not necessary to also check for write, as read also includes all write roles).

backend/src/db/migrations/39-event-preview-roles.sql

backend/src/api/model/event.rs

backend/src/sync/harvest/mod.rs

backend/src/sync/mod.rs

util/dev-config/config.toml

Optional event credentials are now storable in DB, and the are some endpoints to check whether an event is password protected. This also adds a dummy check to `authenticated_data`, to simulate event authentication. Will be extended/replaced in a later commit.

owi92

This is ready for review again. I tried to address everything you commented on and added a few commits to fix some more stuff I noticed myself, and also to allow storing the series id when authenticating for an event, so that each event of that series will be unlocked.

owi92 · 2024-10-09T09:37:01Z

backend/src/db/migrations/39-event-preview-roles.sql

+-- For convenience, all read roles are also copied over to preview roles.
+-- Removing any roles from read will however _not_ remove them from preview, as they
+-- might also have been added separately and we can't really account for that.


is there more to it?

no, I don't think there is.

We might have already talked about this

You suggested doing it like this when we first planned the whole thing. I then said "no lets not, because reasons™". But when implementing it, I realized that I actually find it nicer to have only one filter/check for access things, taking the cue from how read and write roles are handled (whenever something requiring read access is checked, it's not necessary to also check for write, as read also includes all write roles).

backend/src/api/model/event.rs

frontend/src/routes/Video.tsx

frontend/src/i18n/locales/en.yaml

owi92 · 2024-10-10T11:23:46Z

frontend/src/ui/Video.tsx

@@ -43,6 +45,10 @@ export const Thumbnail: React.FC<ThumbnailProps> = ({
 }) => {
    const { t } = useTranslation();
    const isDark = useColorScheme().scheme === "dark";
+    const authenticatedData = useAuthenticatedDataQuery(keyOfId(event.id));


Yeah I don't like that either, would prefer another solution (which I don't have obv).
First I thought this would only send a couple of requests anyway, but when there are one or more blocks of pw-protected series with a lot of videos on a page, there will be a lot of requests.

frontend/src/routes/Video.tsx

owi92 · 2024-10-14T10:11:59Z

frontend/src/ui/Video.tsx

@@ -43,6 +45,10 @@ export const Thumbnail: React.FC<ThumbnailProps> = ({
 }) => {
    const { t } = useTranslation();
    const isDark = useColorScheme().scheme === "dark";
+    const authenticatedData = useAuthenticatedDataQuery(keyOfId(event.id));


I also looked into replacing this specific call in Thumbnail, but that gets somewhat complicated indeed. Not sure if it's worth sinking more time into it for now.

backend/src/api/model/event.rs

owi92 · 2024-10-21T12:41:03Z

@oas777 you can test this (or at least see it in action) with https://pr1244.tobira.opencast.org/test-videos/pw-proc.
User is "user" and password is "password" (...I know, very imaginative).

oas777 · 2024-10-21T15:32:48Z

Thanks, @owi92, access does work as expected. And Bitwarden seems to identify this as access information it can store ... Two remarks on wording:

in German, it shouldn't be "Name" because it usually isn't a name, but an ... "identifier"? We call it "Kennung" in German at ETH, but I'm open to other suggestions.
Similar in English, "Username" is very unlikely to be the username a system provides and not for one user in particular but an ... "identifier"?

owi92 · 2024-10-21T15:39:17Z

Agree 100% on "Name"/"Username" being weird in this context. "Kennung"/"Identifier" is good! Thank you for the suggestion.

LukasKalbertodt

Did a review of your latest changes, resolving almost all inline comments, but adding a few more. Only small ones though.

I am not done with this PR yet, but I thought I'd already send these things out. Reminder to myself what I still want to do:

Think about the remaining unresolved line comments from my previous review
Think about the solution to avoid flashing the page
Think about the React.Context you added
Look at the second new migration
Test the deployment
Check text search

To do that I want to re-review routes/Video.tsx, both SQL migrations and the API docs for previewRoles.

backend/src/sync/harvest/mod.rs

backend/src/auth/mod.rs

frontend/src/util/index.ts

frontend/src/routes/Video.tsx

frontend/src/config.ts

LukasKalbertodt · 2024-10-21T15:50:36Z

frontend/src/routes/Embed.tsx

+    const authorizedData = useAuthenticatedDataQuery(
+        event.id,
+        event.series?.id,
+        { authorizedData: event.authorizedData },
+    );
+
+    return authorizedData
+        ? <Player event={{ ...event, authorizedData }} />
+        : <PreviewPlaceholder embedded {...{ event }}/>;
 };


I think this is broken? Like, entering the credentials on the embed route only shows a spinner but otherwise does nothing.

I forgot to add the context.
That reminds me, since you also wanted to think about the context:
I tried moving that into a custom hook like the one in PlayerContext.tsx, but using that weirdly didn't have the same effect (read: it didn't have any effect) as just defining the state and context individually for each file it's needed in, like I do it now.

If, by any chance, you have some time and motivation to try this out, I'd be interested if you can get that to work. Of course, that only makes sense if we decide to stick with the context solution.

frontend/src/ui/player/Paella.tsx

frontend/src/util/index.ts

LukasKalbertodt · 2024-10-21T16:20:04Z

I agree we switch to "Kennung"/"Identifier". But what about the explanation text then? It's currently this:

Please enter the username and the password you have received to access this series; please note that these are not your login credentials.

Bitte geben Sie die Zugangsdaten ein, die Sie für die Serie dieses Videos erhalten haben. Beachten Sie, dass diese nicht Ihren Logindaten für dieses Portal entsprechen.

Is the second sentence still necessary then? Also the first sentence: it's incorrect in our general Tobira implementation as the password is not necessarily associated with the series. Also: yeah I'm presented with a id/password prompt, obviously I should enter username and password to access the video. So maybe just completely remove the note? @oas777

oas777 · 2024-10-21T19:42:51Z

Thanks for chiming in, @LukasKalbertodt. Obviously, the explanation text should be modified accordingly:

"Please enter the identifier and the password you have received to access this video; please note that these are not your login credentials."
"Bitte geben Sie die Zugangsdaten ein, die Sie für dieses Video erhalten haben. Beachten Sie, dass diese nicht Ihren Logindaten für dieses Portal entsprechen."

As for the second sentence, this is to differentiate the approach from authenticating "the right way" and very important here at ETH for the confusion this causes. But I see how this is not relevant for everyone else. Can we add this "locally"?

owi92

Ready for the next pass.

Regarding the explanation text:

"please note that these are not your login credentials."
Can we add this "locally"?

We could, but I wouldn't mind keeping this for now. Even if we would only prompt for a password, I don't think having this clarification would hurt. Also, having no note at all would just make the authentication mask be really lonely in the vastness of that empty placeholder.

frontend/src/util/index.ts

backend/src/auth/mod.rs

backend/src/sync/harvest/mod.rs

owi92 · 2024-10-22T19:13:16Z

frontend/src/routes/Embed.tsx

+    const authorizedData = useAuthenticatedDataQuery(
+        event.id,
+        event.series?.id,
+        { authorizedData: event.authorizedData },
+    );
+
+    return authorizedData
+        ? <Player event={{ ...event, authorizedData }} />
+        : <PreviewPlaceholder embedded {...{ event }}/>;
 };


I forgot to add the context.
That reminds me, since you also wanted to think about the context:
I tried moving that into a custom hook like the one in PlayerContext.tsx, but using that weirdly didn't have the same effect (read: it didn't have any effect) as just defining the state and context individually for each file it's needed in, like I do it now.

If, by any chance, you have some time and motivation to try this out, I'd be interested if you can get that to work. Of course, that only makes sense if we decide to stick with the context solution.

frontend/src/ui/player/Paella.tsx

This repurposes the UI component that is also used on our login page with some adjustments. The mask is shown on the video route, the embed route and also for video blocks in place of the video player. Entering the correct credentials will unlock the video.

"Sticky" meaning that once the credentials are verified, they won't need to be entered again until the user logs out. Anonymous users will need to re-enter the credentials once their browser session ends. This will also make sure that thumbnails in series blocks and search results are shown after a user has been authenticated for a given event.

The ETH sends their series credentials as SHA1 encoded strings including a username and password. This commit adds a config option that causes these credentials to be separated and stored in Tobira's DB when enabled. To authenticate, users will need to enter these credentials, which will then be hashed and checked against the ones we have from the ETH.

LukasKalbertodt

Just did a pass over your latest changes and resolved my previous comments. I still haven't done the "difficult" things I said I will still do.

frontend/src/routes/Embed.tsx

frontend/src/routes/manage/Realm/Content/Edit/EditMode/Video.tsx

frontend/src/routes/manage/Video/index.tsx

frontend/src/ui/Blocks/Video.tsx

frontend/src/routes/Video.tsx

This is not a good solution as it relies on a frontend check. Ideally this would be done in backend. This same applies for thumbnails in search results.

The ETH passwords are inherited from their series, so it is necessary to keep them in sync.

This replaces some relay logic with relay's `fetchQuery` in order to be able to directly react to the query result or failure. I suspect this should also be possible with `loadQuery` and/or `usePreloadedQuery` but I couldn't figure it out. There was sth going on with the previous implementation that was causing the whole page to reload when submitting correct credentials, while entering the wrong credentials caused the loading indicator to spin indefinetely - huh? I don't know what exactly caused this. Anyway, the new solution also makes the whole video route file a little smaller and saves two middle-man components as well as some props. I think it's an overall improvement in terms of readability and maintainability (and fixes the afore mentioned issues).

Authenticating for an event will now also store that event's series id with its credentials. With that, all events of that particular series will be unlocked. This is a feature only used by the ETH, and as such must be enabled via a configuration option.

Before this change, authenticating a video on video pages or in video blocks would not rerender series blocks, i.e. the thumbnails would not reflect the fact that the videos of these blocks were unlocked. This adds a shared state through context so these blocks are now rerendered and correctly show the videos as unlocked.

github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Sep 12, 2024

This comment was marked as resolved.

Sign in to view

owi92 added the changelog:user User facing changes label Sep 12, 2024

owi92 force-pushed the password-protected-videos branch from 8e8763f to 30d55f1 Compare September 12, 2024 11:35

owi92 changed the title ~~Password protected videos~~ Add support for event specific password protection Sep 12, 2024

owi92 force-pushed the password-protected-videos branch 4 times, most recently from 8ed23b8 to e252587 Compare September 12, 2024 20:58

github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Sep 12, 2024

github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:03 Destroyed

owi92 force-pushed the password-protected-videos branch from e252587 to a2e4d24 Compare September 12, 2024 21:20

github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:23 Destroyed

owi92 force-pushed the password-protected-videos branch from a2e4d24 to 402869f Compare September 12, 2024 21:30

github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:32 Destroyed

owi92 force-pushed the password-protected-videos branch from 402869f to 5fd35f1 Compare September 12, 2024 21:35

github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:38 Destroyed

owi92 force-pushed the password-protected-videos branch from 5fd35f1 to aca0209 Compare September 12, 2024 22:22

github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 22:26 Destroyed

owi92 force-pushed the password-protected-videos branch from aca0209 to e4630aa Compare September 12, 2024 22:35

github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 22:38 Destroyed

owi92 marked this pull request as ready for review September 12, 2024 22:46

LukasKalbertodt force-pushed the master branch from df91940 to ac46ed4 Compare October 2, 2024 10:27

owi92 changed the base branch from main to next October 2, 2024 14:30

LukasKalbertodt requested changes Oct 7, 2024

View reviewed changes

github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Oct 8, 2024

This comment was marked as resolved.

Sign in to view

owi92 force-pushed the password-protected-videos branch from e4630aa to 2632d55 Compare October 8, 2024 21:32

github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Oct 8, 2024

github-actions bot temporarily deployed to test-deployment-pr1244 October 8, 2024 21:35 Destroyed

owi92 added 2 commits October 20, 2024 21:06

Sync preview roles when harvesting

f143373

owi92 force-pushed the password-protected-videos branch from c169436 to c7b710b Compare October 20, 2024 19:07

github-actions bot temporarily deployed to test-deployment-pr1244 October 20, 2024 19:10 Destroyed

owi92 commented Oct 21, 2024

View reviewed changes

LukasKalbertodt requested changes Oct 21, 2024

View reviewed changes

owi92 force-pushed the password-protected-videos branch from c7b710b to d2662a7 Compare October 22, 2024 19:51

github-actions bot temporarily deployed to test-deployment-pr1244 October 22, 2024 19:57 Destroyed

owi92 commented Oct 22, 2024

View reviewed changes

owi92 added 3 commits October 22, 2024 22:01

owi92 force-pushed the password-protected-videos branch from d2662a7 to 5f01c3e Compare October 22, 2024 20:02

github-actions bot temporarily deployed to test-deployment-pr1244 October 22, 2024 20:05 Destroyed

LukasKalbertodt requested changes Oct 23, 2024

View reviewed changes

owi92 force-pushed the password-protected-videos branch 2 times, most recently from 0ca7595 to 121fd02 Compare October 23, 2024 16:22

github-actions bot temporarily deployed to test-deployment-pr1244 October 23, 2024 16:28 Destroyed

owi92 added 5 commits November 7, 2024 12:32

Hide text results for events with passwords

bd2bc66

This is not a good solution as it relies on a frontend check. Ideally this would be done in backend. This same applies for thumbnails in search results.

Add migration and sync for series credentials

f86b02b

The ETH passwords are inherited from their series, so it is necessary to keep them in sync.

owi92 force-pushed the password-protected-videos branch from 121fd02 to 8c93f7d Compare November 7, 2024 11:32

github-actions bot deployed to test-deployment-pr1244 November 7, 2024 11:40 View deployment

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for event specific password protection #1244

Add support for event specific password protection #1244

owi92 commented Sep 12, 2024 •

edited

Loading

This comment was marked as resolved.

LukasKalbertodt left a comment

LukasKalbertodt Oct 1, 2024

owi92 Oct 9, 2024

This comment was marked as resolved.

owi92 left a comment

owi92 Oct 9, 2024

owi92 Oct 10, 2024

owi92 Oct 14, 2024

owi92 commented Oct 21, 2024

oas777 commented Oct 21, 2024

owi92 commented Oct 21, 2024

LukasKalbertodt left a comment •

edited

Loading

LukasKalbertodt Oct 21, 2024

owi92 Oct 22, 2024

LukasKalbertodt commented Oct 21, 2024 •

edited

Loading

oas777 commented Oct 21, 2024

owi92 left a comment

owi92 Oct 22, 2024

LukasKalbertodt left a comment

Add support for event specific password protection #1244

Are you sure you want to change the base?

Add support for event specific password protection #1244

Conversation

owi92 commented Sep 12, 2024 • edited Loading

This comment was marked as resolved.

LukasKalbertodt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as resolved.

owi92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

owi92 commented Oct 21, 2024

oas777 commented Oct 21, 2024

owi92 commented Oct 21, 2024

LukasKalbertodt left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasKalbertodt commented Oct 21, 2024 • edited Loading

oas777 commented Oct 21, 2024

owi92 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasKalbertodt left a comment

Choose a reason for hiding this comment

owi92 commented Sep 12, 2024 •

edited

Loading

LukasKalbertodt left a comment •

edited

Loading

LukasKalbertodt commented Oct 21, 2024 •

edited

Loading