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

Login-as using WWW-Authenticate #109

Merged
merged 3 commits into from Mar 23, 2020
Merged

Login-as using WWW-Authenticate #109

merged 3 commits into from Mar 23, 2020

Conversation

benbosman
Copy link
Member

The goal of the underlying implementation is:

  • Login-as is migrated to an explicit AuthenticationMethod
  • This AuthenticationMethod implementation will only act if the current user is an admin, and the login method is enabled (replacing the old webui.user.assumelogin boolean)
  • It won't need a password, but still needs an email address or UUID

As part of the implementation, it would also be best to improve the EPersonRestAuthenticationProvider implementation, this will have an impact on the Angular UI as well:

  • The Angular UI currently reads the WWW-Authenticate header to identify the AuthenticationMethod based on the getName() method
  • If the Angular UI specifies which authentication method it attempts (posting to /server/api/authn/login with parameters method=password, user and password), this parameter can used by the EPersonRestAuthenticationProvider to only use the relevant AuthenticationMethod
  • If one would configure password, LDAP, and login-as authentication, the system shouldn't randomly test them until one returns success, but should rather use this applicably method immediate

@abollini
Copy link
Member

I'm incline to think to login as as an additional header (similar to how it is managed by Sword) instead than a different authentication method.
Anyway, we will need at least some additional details about how to manage the authentication flow in the case of the login as. The list of call done to discover the feature, login as admin and login as as another user would be helpful to better understand.

@benbosman
Copy link
Member Author

Summary of what was discussed in the meeting:

  • /api/auth/status can be used to identify whether x-on-behalf-of headers are allowed for the current user
  • Any request to REST (e.g. a request to /api/core/items/<:uuid>) can receive an x-on-behalf-of header.
  • In REST, a filter would be required to verify whether the x-on-behalf-of header is present. If it's present, a verification whether the current user is an admin is required. If that passes, the context's current user will be switched to the other user. The JWT token doesn't change
  • In Angular, when the user starts a "Login as", the cache should be cleared, the x-on-behalf-of header should be included in every REST request, and the UI should display who they're impersonating
  • In Angular, when the user logs out, nothing is sent to REST, but the cache should be cleared, the x-on-behalf-of header should no longer be included in every REST request, and the UI should display the regular user again

Please update this summary if you see anything the differs from what we discussed

I will verify how much work this would require in REST and Angular.

@benbosman
Copy link
Member Author

I've verified the impact on REST and Angular, and REST shouldn't pose any problems. For Angular, the estimated 12 hours might not suffice, but it shouldn't be a huge increase in that work.

I've updated the contract based on the discussed alternative

@benbosman
Copy link
Member Author

One of the side-effects of the given solution:
During the implementation, all occurrences of ePerson = ePersonService.findByEmail(context, (String) authentication.getPrincipal()); will need to be replaced with ePerson = context.getCurrentUser();
The authentication.getPrincipal() should never be used to verify the current user apart from the actual authentication code

Copy link
Member

@abollini abollini left a comment

Choose a reason for hiding this comment

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

I have just a different suggestion about how to discover if the user can or cannot access the loginAs feature.
The other suggestion about the use of the camel case format of the header can be freely ignored if not liked.

## Log in as

For any request, an `x-on-behalf-of` header can be included.
Copy link
Member

Choose a reason for hiding this comment

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

just a style note I would prefer the use of X-On-Behalf-Of as used in the SWORD protocol spec but this is just "layout" as header by spec are case insensitive

Sample request:
```
curl -v "http://{dspace-server.url}/server/api/core/items/1911e8a4-6939-490c-b58b-a5d70f8d91fb" -H "Authorization: Bearer eyJhbG...COdbo" -H "x-on-behalf-of: 028dcbb8-0da2-4122-a0ea-254be49ca107"
Copy link
Member

Choose a reason for hiding this comment

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

again if possible use X-On-Behalf-Of in camel case

@@ -91,6 +91,7 @@ This will return the authentication status, E.G.:
{
"okay" : true,
"authenticated" : true,
"allowOnBehalfOf" : false,
Copy link
Member

Choose a reason for hiding this comment

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

sorry to haven't got the comment immediately where you have anticipated such change. I want to suggest to use the new authorization endpoint to check for the possibility to use the loginAs feature. You can implement a loginAs feature that support the Site object to know if the current user is entitled or not to access such feature.

@benbosman
Copy link
Member Author

@abollini I've processed your feedback.
It's indeed possible to use the "features" endpoint (it's still a very confusing name for determining permissions though)

@abollini
Copy link
Member

thanks @benbosman all my feedback have been processed

@tdonohue tdonohue merged commit 1645d02 into DSpace:master Mar 23, 2020
@KevinVdV KevinVdV mentioned this pull request Apr 7, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants