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

Add support for event specific password protection #1244

Open
wants to merge 26 commits into
base: next
Choose a base branch
from

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Sep 12, 2024

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 (credentials are "user" and "password")

Closes #1186

@github-actions 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.

@owi92 owi92 added the changelog:user User facing changes label Sep 12, 2024
@owi92 owi92 changed the title Password protected videos Add support for event specific password protection Sep 12, 2024
@owi92 owi92 force-pushed the password-protected-videos branch 4 times, most recently from 8ed23b8 to e252587 Compare September 12, 2024 20:58
@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Sep 12, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:03 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:23 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:32 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 21:38 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 22:26 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 September 12, 2024 22:38 Destroyed
@owi92 owi92 marked this pull request as ready for review September 12, 2024 22:46
@owi92 owi92 changed the base branch from main to next October 2, 2024 14:30
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

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 Outdated Show resolved Hide resolved
Comment on lines 5 to 7
-- 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.
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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 Outdated Show resolved Hide resolved
backend/src/db/migrations/39-event-preview-roles.sql Outdated Show resolved Hide resolved
backend/src/api/model/event.rs Outdated Show resolved Hide resolved
backend/src/sync/harvest/mod.rs Outdated Show resolved Hide resolved
backend/src/sync/harvest/mod.rs Outdated Show resolved Hide resolved
backend/src/sync/harvest/mod.rs Outdated Show resolved Hide resolved
backend/src/sync/mod.rs Outdated Show resolved Hide resolved
util/dev-config/config.toml Show resolved Hide resolved
@github-actions 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.

@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Oct 8, 2024
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 October 8, 2024 21:35 Destroyed
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.
@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Nov 19, 2024
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 github-actions bot temporarily deployed to test-deployment-pr1244 November 19, 2024 10:49 Destroyed
See elan-ev#1244 (comment)

I thought quite a bit about this and I think both options would have
been fine, but I decided to change this for one reason in particular:
currently, `read_roles` and `write_roles` reflect exactly what Opencast
tells us. We of course also have columns with calculated values, but
we should keep it fairly clear what columns are straight from OC and
which ones are "our columns". Most importantly: if we ever would need
to distinguish between read and preview, this would require a resync.
This would be unfortunate, so I'd rather store the exact OC information
so that we have it, just in case.

I then decided to just adjust the few checks to check both columns, but
we could also have added another column that holds the combination of
the two. That's an implementation detail and we can still change it
later. But yes, the more complicated checks don't seem like a problem
to me.
The `perform` method still used lots of manual filter creation, which
is now replaced. I also replaced `Filter::None` with `Filter::True`,
which more precisely describes its role. Before, one could easily run
into a bug where `None`s from `or` rules are just filtered out, but
in all cases of `None` usage, it should rather make the whole `or`
evaluate to `true`. Finally, three more helper functions were added
to avoid repeating `*_roles` a bunch of times.
From a recent meeting, we concluded that we should just do what the
event ACL tell us. The admins need to make sure the effective (i.e.
after applying merge mode) event ACLs contain the password roles, if
that video should be password protected.

Even if that requirement still changes in the future, I think it should
be done in the harvest code, not as a DB trigger. Further, just always
transferring series credentials to events is also wrong in our more
flexible model (that should cover more use cases than the ETH). Finally,
the trigger as written was not complete as it was missing the "read
roles become preview roles" logic, which is already present in code.

I decided to still store series credentials, as that's useful for a few
reasons, e.g. implementing this feature later on without resyncing.
But this tiny SQL command didn't warrant its own migration, so I
combined both.
Instead, the locked icon is shown for all password-protected videos,
regardless of whether they are unlocked.
So, quick recap of what was done before this commit:
- Initial GQL query can include credentials to immediately unlock
  `authorizedData`, but lets just consider the route where it doesn't.
- `useAuthenticatedDataQuery` was called which sent a request based on
  whether the `authorizedData` was already present and on whether
  credentials did exist (by cleverly using `fetchPolicy`). In our case
  it first did nothing as no credentials are there.
- The `authenticatedData` as stored inside a React context, and not in
  `event`.
- The password form, when submitted would send a GQL request. On success
  the `setAuthData` setter from the context was called, meaning that
  all components using the contexts were rerendered.

Generally nothing terrible going on here, but: `authorizedData` is
really a part of the event and having a separate store, instead of just
saving and retrieving it from the Relay store feels weird. There was
also a bug where only one context was used per realm, which lead to
all video blocks showing the same video if one was unlocked.

This commit changes this. The context is completely removed and the
data is stored in the relay store, making this more idiomatic. But oh
boy, it was not easy to get here. Relay docs are just terrible for me
to dig through. So one thing seemed clear fairly quickly: the idiomatic
way is `useRefetchableFragment` for this. With that, one can only
refetch a small part of the initial query, but with potentially
different variables. Perfect! Problem is that the `refetch` function's
callback function does not get the fetched data. So it is hard to
check if the credentials were correct (and to distinguish other network
errors). So my solution at the end is to keep the manual `fetchQuery`
as that allows us to check the result in a callback. Then, on success,
the `refetch` is called with "store-only" to just rerender the
components. One of the big time sinks for me was to understand that the
manual query we use with `fetchQuery` needs to be exactly the same as
what `refetch` will execute. Specifically: use `node()` and not
`eventById`.

Refetching only works with fragments, so another layer of fragments
needed to be introduced. I added a helper function to combine the event
with its `authorizedData` for that.

This commit is best viewed with whitespace-diff disabled.
My previous changes accidentally removed the feature that events are
also unlocked by the credentials associated with their series. For that,
a second request is necessary, unfortunately.

I also reworked how credentials are loaded from localStorage as that was
fishy before: `getCredentials` states it returns `Credentials` which
has `user` and `password` fields. But in practice, the object stored in
local storage always contained fields `eventUser` and `eventPassword`.
And that was actually fine at runtime with all places that read that
data. However, it's obviously not ideal to have types that don't match
the runtime data, so I fixed that. The fields in local storage are now
`user` and `password`.
`hasPassword` was added to events in the previous commits, so we need
to rebuild the index.
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 November 21, 2024 11:31 Destroyed
@owi92
Copy link
Member Author

owi92 commented Nov 26, 2024

For the latest changes of this PR please see Lukas's commit messages and my comment in his PR.

@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 November 26, 2024 16:21 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr1244 November 27, 2024 20:32 Destroyed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Password protected videos (understand existing videos) (ETH)
3 participants