[FOLIO-1332] SPIKE: Design/Discuss Overrides Created: 09/Jul/18 Updated: 03/Jul/19 Resolved: 04/Oct/18 |
|
| Status: | Closed |
| Project: | FOLIO |
| Components: | None |
| Affects versions: | None |
| Fix versions: | None |
| Type: | Umbrella | Priority: | P2 |
| Reporter: | Cate Boerema (Inactive) | Assignee: | Kurt Nordstrom |
| Resolution: | Done | Votes: | 0 |
| Labels: | RFC, core, sprint42, sprint43, sprint44, sprint45, sprint46, sprint47, sprint48, ui-core | ||
| Remaining Estimate: | Not Specified | ||
| Time Spent: | Not Specified | ||
| Original estimate: | Not Specified | ||
| Issue links: |
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Sprint: | |||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Development Team: | Prokopovych | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| Description |
|
We held a meeting yesterday and have come to the following:
|
| Comments |
| Comment by Cate Boerema (Inactive) [ 16/Jul/18 ] |
|
Per Jakub Skoczen, I am assigning this to Kurt Nordstrom |
| Comment by Cate Boerema (Inactive) [ 20/Jul/18 ] |
|
Hi Kurt Nordstrom, do you have an update on this? Did you determine if this should stay assigned to you for analysis or if it's a frontend-only thing? |
| Comment by Kurt Nordstrom [ 20/Jul/18 ] |
|
Cate Boerema Here are my thoughts on the matter: Yes, we currently could implement an override functionality directly in the front end. However, this is slightly problematic, as it will mean that an access token for the superuser will be exposed in the user's browser, and a mischievous user could use their browser's dev tools to extract it, and then re-use the token for gaining access to endpoints. This raises the topic of token expiration and refresh tokens, which is something that we have discussed in the past, but haven't formally implemented yet. What this feature would allow us to do would be to have the superuser token be created with a short time-to-live, so that the window of opportunity for mischief was very small. It would also improve security overall by reducing the usefulness of token leakage. See https://folio-org.atlassian.net/browse/FOLIO-1233 My recommendation would be to raise the priority of implementing refresh tokens and token expiration and make it a blocker for this story. |
| Comment by Jakub Skoczen [ 24/Jul/18 ] |
|
Kurt Nordstrom Expiration/TTL makes sense to me. I am not clear we need the full capability of "refresh tokens" for this? |
| Comment by Kurt Nordstrom [ 25/Jul/18 ] |
|
Okay, here is how I currently see things:
|
| Comment by Cate Boerema (Inactive) [ 27/Jul/18 ] |
|
Thanks Kurt Nordstrom. Are we almost ready to get started on development? Emma Boettcher has created some stories we can work from, in case the Fee/fine-related stories are still blocked (Jakub Skoczen asked that she create these):
|
| Comment by Kurt Nordstrom [ 27/Jul/18 ] |
|
Cate Boerema If we want to proceed with the short-term solution, all we'd need for the backend would be an optional flag to the login module to tell it to request a short-lived token as opposed to a long-lived one. |
| Comment by Cate Boerema (Inactive) [ 30/Jul/18 ] |
|
Excellent! We had a chat about this in today's roundup and I think everyone understands what needs to be done, as Emma talked us through her stories (
I guess we can close this spike if the solution is designed and documented (at least at a high level) in this ticket (I think that's what you have done in the comments above). Please close this ticket if you agree. |
| Comment by Jakub Skoczen [ 20/Aug/18 ] |
|
Kurt Nordstrom let's decided how would we utilize the refresh token functionality to provide this. |
| Comment by Cate Boerema (Inactive) [ 03/Sep/18 ] |
|
Kurt Nordstrom and Jakub Skoczen, can we call this done already? It's sprint review time and, while we won't have anything to show for this (I assume), it is a significant piece of design work we did and I'd like it to display in the list of completed work. Please mark as Closed > Done if you think this is complete. Thanks! |
| Comment by Heikki Levanto [ 05/Sep/18 ] |
|
In my humble opinion, messing about tokens is a wrong approach. It is tricky to get right, and may lead to security problems. I think a safer, saner, and simpler solution would be to let everyone keep using their own tokens, but add a facility in mod-permissions, where a supervisor has the ability to grant some of his own permissions to a given user, for a limited time. UI-wise, this could be done in a new tab or popup on the users screen, where the superuser logs in, makes the permission-giving call, and closes the tab. Or it could be a function the supervisor can invoke on his own screen. In any case, the supervisor could select the (top-level) permissions to grant, and the time they should be available. In mod-permissions, we would need to add a timeout field to the permission structure, and when checking permissions, see if the user has some that have timed out, and delete them. There may be some issues with caching the permissions, but I am sure we can find a way around those. It could even be possible to restrict the temporary permission to a single request, by noting the first time it is being used, attaching the request-id to that, and later verifying the request-id. But this could get tricky when expanding permissions, etc. Probably not needed for the first version(s). Of course the supervisor would have to have a permission to delegate his permissions to another user. This could be a general one, or restricted to some top-level permissions. |
| Comment by Kurt Nordstrom [ 05/Sep/18 ] |
|
Heikki Levanto It would seem tricky, with the 'temporary permissions' approach, to attach the needed permissions upgrade to a particular request, though. In essence the supervisor is boosting the user's privileges (albeit temporarily), but not restricting what the user might do with them. Even if we attach a request id, it seems like it would be tricky to peg that to one particular request that the override is granting the ability to do. |
| Comment by VBar [ 07/Sep/18 ] |
|
I'm liking Heikki's "temporary permissions" approach, but I do have concerns about privilege escalation be it through a superuser context or a supervisor impersonation. Have we considered supporting the notion of allowing multiple tokens for a user?
|
| Comment by Cate Boerema (Inactive) [ 19/Sep/18 ] |
|
Guys, this spike has been open for almost an entire quarter. Can we document a decision by the end of this week (September 21)? |
| Comment by Kurt Nordstrom [ 19/Sep/18 ] |
|
We held a meeting yesterday and have come to the following:
|
| Comment by Mike Taylor [ 19/Sep/18 ] |
|
That seems like a much more complicated scheme than the original. How is the client (Stripes or a module) able to know which specific temporary permissions to request?
But the supervisor exposes their credentials to the user's browser, which seems like a much worse security hole (since in general credentials last much longer than tokens generated from them). |
| Comment by Marc Johnson [ 19/Sep/18 ] |
|
Mike Taylor Good question about the awareness (and coupling) around the specific permissions. Could it initially be all of the permissions of the supervisor (which admittedly is another potential security challenge)?
As I understand it, we don't have an approach where the supervisor's credentials aren't exposed to the original user's browser. At least not where the user experience is where the supervisor performs their login in the original user's browser, which is what I believe is described in scenario 2 of
|
| Comment by Mike Taylor [ 19/Sep/18 ] |
It could – but only at the expense of sacrificing all the flexibility that (presumably) this more complex approach is intended to get us. If that's all we wanted to do, then the simpler `sudo`-like approach would seem the better way to do it.
I agree. All I'm saying is that, given that this seems to be a hard requirement, it seems a bit pointless to worry that the supervisor's much shorter-lived session token is also available to the user's browser. |
| Comment by Heikki Levanto [ 19/Sep/18 ] |
|
We discussed this briefly in yesterdays call. Although the use case here is that the supervisor will walk over to the assistants browser and type in credentials, this design leaves open the possibility that in some future version the supervisor may do this on his own browser, get some opaque token, and pass it to the assistant via a Folio notification, email, or other channel. As to the permissions, yes, we can start with all permissions, but most likely the UI module that handles the situation will have a good idea of what is needed here - the operation to forgive a fine clearly needs some fine-related permissions, and not purchase-order permissions... |
| Comment by Heikki Levanto [ 19/Sep/18 ] |
|
I just came to think that the override endpoint should be in a separate interface than the regular login service. We already have alternative login methods, and maybe not all of them are suitable for authenticating a supervisor that happens to walk by the browser, without his session cookies from some other system present... Keeping the service in a separate interface will make it possible for the module to declare that it does not support the override service, and for some other module to step in and do it. |
| Comment by Marc Johnson [ 19/Sep/18 ] |
|
Mike Taylor Agreed, thanks for clarifying my understanding of your thoughts. |
| Comment by Marc Johnson [ 19/Sep/18 ] |
|
The topic of which permissions apply to a given operation may well overlap a little with
|
| Comment by Marc Johnson [ 19/Sep/18 ] |
|
Heikki Levanto I think a separate interface is worth exploring (we may need to figure out endpoints, as RAML Module Builder does not support overlapping paths in interface definitions AFAIK). There is a note in
|
| Comment by Kurt Nordstrom [ 19/Sep/18 ] |
|
Heikki Levanto I like the idea of being able to perform an override via multiple authentication services. Need to think through how to implement that in a way that leaves the door open for future expandability. |
| Comment by Mike Taylor [ 19/Sep/18 ] |
|
Heikki Levanto Your point about separating interfaces is well taken. |