-
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
Add support for event specific password protection #1244
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
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
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.
Very good idea adding audioOnly
to the API. Only two remarks.
Since this PR has been around for a while and many discussions took place, let me summarize a bit: Thumbnails are now treated as not-protected by the password. This is in line with the current video portal of the ETH (which is the main reason we are developing this feature). There are use cases and arguments for both options, making the thumbnail protected or not protected, so we might make this more flexible in the future. Having the thumbnail not-protected solved one of the major technical hurdles for us. The current PR lacks some goodies compared to Ole's initial version of this PR (as mentioned here). This was my decision to ignore this for now to be able to move on with this and also tackle other, more important issues.
But with that, after the last minor remarks above are fixed, this is finally ready. |
e373b7f
to
80d42af
Compare
80d42af
to
eee097c
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.
Praise the lord
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