-
Notifications
You must be signed in to change notification settings - Fork 18
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
base: next
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
8e8763f
to
30d55f1
Compare
8ed23b8
to
e252587
Compare
e252587
to
a2e4d24
Compare
a2e4d24
to
402869f
Compare
402869f
to
5fd35f1
Compare
5fd35f1
to
aca0209
Compare
aca0209
to
e4630aa
Compare
df91940
to
ac46ed4
Compare
There was a problem hiding this 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.
-- 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. |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
This comment was marked as resolved.
This comment was marked as resolved.
e4630aa
to
2632d55
Compare
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.
8c93f7d
to
ae81723
Compare
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.
ae81723
to
35e0a07
Compare
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.
For the latest changes of this PR please see Lukas's commit messages and my comment in his PR. |
1c9f4bc
to
be9bbaf
Compare
be9bbaf
to
e373b7f
Compare
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:
Of course you can use any other event. This will give the event the password user
pass
and the passwordword
.Test here (credentials are "user" and "password")
Closes #1186