Assume login feature #2740

KevinVdV · 2020-04-07T10:02:36Z

References

Add references/links to any related tickets or PRs. These may include:

Link to REST Contract

Description

Rest api implementation for the assume login feature. Included are the actual "login as" implementation by making use of the "X-On-Behalf-Of" header & code to support "loginOnBehalfOf" on the "features" feature.

Instructions for Reviewers

Please add a more detailed description of the changes made by your PR. At a minimum, providing a bulleted list of changes in your PR is helpful to reviewers.

List of changes in this PR:

StatelessAuthenticationFilter: contains the code for the actual "login as"
LoginOnBehalfOfFeature: contains the code for the feature

Checklist

This checklist provides a reminder of what we are going to look for when reviewing your PR. You need not complete this checklist prior to creating your PR (draft PRs are always welcome). If you are unsure about an item in the checklist, don't hesitate to ask. We're here to help!

My PR is small in size (e.g. less than 1,000 lines of code, not including comments & integration tests). Exceptions may be made if previously agreed upon.
My PR passes Checkstyle validation based on the Code Style Guide
My PR includes Javadoc for all new (or modified) public methods and classes. It also includes Javadoc for large or complex private methods.
My PR passes all tests and includes new/updated Unit or Integration Tests for any bug fixes, improvements or new features. A few reminders about what constitutes good tests:
- Include tests for different user types, including: (1) Anonymous user, (2) Logged in user (non-admin), and (3) Administrator.
- Include tests for known error scenarios and error codes (e.g. 400 Bad Request, 401 Unauthorized, 403 Forbidden, 404 Not Found, etc)
- For bug fixes, include a test that reproduces the bug and proves it is fixed. For clarity, it may be useful to provide the test in a separate commit from the bug fix.
If my PR includes new, third-party dependencies (in any pom.xml), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.
If my PR modifies the REST API, I've linked to the REST Contract page (or open PR) related to this change.

…test

… unneeded code in StatelessAuthenticationFilter

abollini

I have added just minor suggestions about the IT. The only coding issue is related to the use of an instance attribute in the Spring Security Filter that should be removed (maybe refactoring the code to use a custom exception)

abollini · 2020-04-14T15:48:30Z

...rver-webapp/src/main/java/org/dspace/app/rest/authorization/impl/LoginOnBehalfOfFeature.java

+
+    @Override
+    public String[] getSupportedTypes() {
+        return new String[] { SiteRest.CATEGORY + "." + SiteRest.NAME };


I suggest to support also EPersonRest object as we will need a way to inform the UI that the loginAs can or cannot be used on a specific user (You cannot assume the login of another admin in the current version)

abollini · 2020-04-14T15:52:20Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

@@ -1325,4 +1341,113 @@ private String getAuthorizationID(String epersonUuid, String featureName, String
                + id.toString();
    }

+    @Test
+    public void loginOnBehalfOfTest() throws Exception {


please move this test in a dedicated class. This class should contains only general tests to validate the authorization framework and not specific features. See for instance https://github.com/DSpace/DSpace/blob/0039b309d11e3d55a2a92699b25458fe9bfc4740/dspace-server-webapp/src/test/java/org/dspace/app/rest/authorization/WithdrawFeatureRestIT.java

abollini · 2020-04-14T15:55:03Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

+                                     .param("eperson", String.valueOf(eperson.getID()))
+                                     .param("feature", loginOnBehalfOf.getName()))
+                        .andExpect(status().isOk())
+                        .andExpect(jsonPath("$._embedded.authorizations", Matchers.not(


I would suggest to check for an empty list here instead than exclude the authorization object

@abollini We can't test on an empty list here as some features are always returned by default. For example the "AlwaysTrueFeature". As we can't anticipate which features will be returned it would be best to check for an exclusion.

abollini · 2020-04-14T15:58:01Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

+                        .andExpect(status().isOk())
+                        .andExpect(jsonPath("$._embedded.authorizations", Matchers.not(
+                            Matchers.hasItem(AuthorizationMatcher.matchAuthorization(loginOnBehalfOfAuthorization)))));
+    }
 }


we should have test to verify that we cannot loginas another admin

abollini · 2020-04-14T16:07:22Z

...-server-webapp/src/main/java/org/dspace/app/rest/security/StatelessAuthenticationFilter.java

+
+    private ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();
+
+    private boolean inErrorOnBehalfOf = false;


this cannot be an instance attribute otherwise there are issue when multiple login request are processed at the same time

abollini · 2020-04-14T16:08:24Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/LoginAsEPersonIT.java

+
+    @Before
+    public void setup() {
+        context.turnOffAuthorisationSystem();


it is not really needed, we can simplify the code here

abollini · 2020-04-14T16:08:52Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/LoginAsEPersonIT.java

+
+    @Test
+    public void loggedInUserPropertyFalseTest() throws Exception {
+        context.turnOffAuthorisationSystem();


the turnOff is not really needed

abollini · 2020-04-14T16:10:11Z

dspace-server-webapp/src/test/java/org/dspace/app/rest/LoginAsEPersonIT.java

+                                     .header("X-On-Behalf-Of", eperson.getID()))
+                        .andExpect(status().isBadRequest());
+
+        context.turnOffAuthorisationSystem();


I would suggest to don't revert the configuration manually here the change as this is already done in a Before method (just to avoid in future confusion in other developers)

dspace-server-webapp/src/test/java/org/dspace/app/rest/LoginAsEPersonIT.java

tdonohue

@KevinVdV : Besides what @abollini has already noted (which I agree with), I added one other request about simply refactoring the filter to remove the isErrorOnBehalfOf flag altogether (as I agree it's fragile, and it's not really necessary anyways).

...-server-webapp/src/main/java/org/dspace/app/rest/security/StatelessAuthenticationFilter.java

…nd tests

KevinVdV · 2020-04-22T12:03:58Z

@tdonohue @abollini We have processed your test feedback, let us know if there is anything else or if we missed something.

benbosman

I've deployed and tested this branch, and I couldn't see any problems.
I was able to verify:

Without webui.user.assumelogin = true the X-On-Behalf-Of header is denied
With webui.user.assumelogin = true the pages return the content based on the user's permissions, including e.g. the workflow tasks, or claiming a workflow task
With webui.user.assumelogin = true the pages return forbidden for pages the user is not authorized to use

tdonohue

@KevinVdV : I gave this another review today. While I think it's looking better, the Pair logic seems really odd to me. I'd like to either better understand why you decided on this route (which means better code commenting here), or replace it with normal error handling. See more inline below.

...rver-webapp/src/main/java/org/dspace/app/rest/authorization/impl/LoginOnBehalfOfFeature.java

...-server-webapp/src/main/java/org/dspace/app/rest/security/StatelessAuthenticationFilter.java

KevinVdV · 2020-05-07T11:27:32Z

@tdonohue We have processed your feedback, can you review again ?

abollini

thanks @Raf-atmire to have addressed my feedback. I have just added a small cleanup comment inline

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java

...-server-webapp/src/main/java/org/dspace/app/rest/security/StatelessAuthenticationFilter.java

abollini · 2020-05-10T11:54:14Z

all my feedback have been processed. Can be merged once @tdonohue is satisfied

tdonohue

👍 Looks good to me now. Thanks @KevinVdV and @Raf-atmire

Raf-atmire and others added 6 commits March 31, 2020 15:55

[Task 70131] added support for the login as feature and added simple …

4698ebc

…test

[Task 70131] intermediate fixes error handling

c12d83f

[Task 70131] cleanup and futher implement login as feature

9124228

[Task 70140] implemented loginOnBehalfOfFeature and tests, cleaned up…

1f6e1e0

… unneeded code in StatelessAuthenticationFilter

[Task 70178] applied feedback to the LoginAs feature

3389a2c

Removing enter in a file that we didn't need to modify

fb1a90b

KevinVdV added the interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) label Apr 7, 2020

KevinVdV added this to the 7.0 milestone Apr 7, 2020

[Task 70131] added X-On-Behalf-Of to allowedHeaders

153786c

abollini requested changes Apr 14, 2020

View reviewed changes

tdonohue reviewed Apr 15, 2020

View reviewed changes

...-server-webapp/src/main/java/org/dspace/app/rest/security/StatelessAuthenticationFilter.java Outdated Show resolved Hide resolved

Atmire-Kristof mentioned this pull request Apr 16, 2020

Login as EPerson DSpace/dspace-angular#653

Merged

Raf-atmire added 2 commits April 22, 2020 09:11

[Task 70478] applied feedback to the login as eperson functionality a…

cf61ae9

…nd tests

[Task 70478] applied feedback to the login as feature

3c28c65

benbosman approved these changes Apr 23, 2020

View reviewed changes

tdonohue reviewed May 4, 2020

View reviewed changes

[Task 70763] applied community feedback to the LoginOnBehalfOf feature

cb6c5cc

abollini reviewed May 7, 2020

View reviewed changes

dspace-server-webapp/src/test/java/org/dspace/app/rest/AuthorizationRestRepositoryIT.java Outdated Show resolved Hide resolved

Removed unused AuthorizationFeature variable

ece341e

tdonohue reviewed May 8, 2020

View reviewed changes

...-server-webapp/src/main/java/org/dspace/app/rest/security/StatelessAuthenticationFilter.java Show resolved Hide resolved

abollini approved these changes May 10, 2020

View reviewed changes

Updated javadoc on StatelessAuthenticationFilter#getAuthentication

d7cbe8c

tdonohue approved these changes May 11, 2020

View reviewed changes

tdonohue merged commit aaa5f09 into DSpace:master May 11, 2020

tdonohue modified the milestones: 7.0, 7.0beta3 Jun 30, 2020

tdonohue added this to Needs Reviewers Assigned in DSpace 7 Beta 3 via automation Jun 30, 2020

tdonohue moved this from Needs Reviewers Assigned to Done in DSpace 7 Beta 3 Jun 30, 2020

benbosman deleted the w2p-70131_login-as-eperson branch September 11, 2020 07:36

tdonohue mentioned this pull request Oct 8, 2020

Offer sword-like "on-Behalf-of" authentication or equivalent functionality DSpace/RestContract#24

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Assume login feature #2740

Assume login feature #2740

KevinVdV commented Apr 7, 2020

abollini left a comment

abollini Apr 14, 2020

abollini Apr 14, 2020

abollini Apr 14, 2020

KevinVdV Apr 22, 2020

abollini Apr 14, 2020

abollini Apr 14, 2020

abollini Apr 14, 2020

abollini Apr 14, 2020

abollini Apr 14, 2020

tdonohue left a comment

KevinVdV commented Apr 22, 2020

benbosman left a comment

tdonohue left a comment

KevinVdV commented May 7, 2020

abollini left a comment

abollini commented May 10, 2020

tdonohue left a comment


		private ConfigurationService configurationService = DSpaceServicesFactory.getInstance().getConfigurationService();

		private boolean inErrorOnBehalfOf = false;

Assume login feature #2740

Assume login feature #2740

Conversation

KevinVdV commented Apr 7, 2020

References

Description

Instructions for Reviewers

Checklist

abollini left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

KevinVdV commented Apr 22, 2020

benbosman left a comment

Choose a reason for hiding this comment

tdonohue left a comment

Choose a reason for hiding this comment

KevinVdV commented May 7, 2020

abollini left a comment

Choose a reason for hiding this comment

abollini commented May 10, 2020

tdonohue left a comment

Choose a reason for hiding this comment