DS-4027: New user registration & forgot password #2763

benbosman · 2020-05-20T07:19:56Z

References

Link to JIRA ticket
Link to REST Contract

Description

This is the backend to:

Request a new user registration (which sends an email)
Complete a new user registration (creating the user with the token)
Requests a new password (which sends an email)
Complete a password update (patching the user with the token)

Instructions for Reviewers

List of changes in this PR:

Request a new user registration and Requests a new password are handled using POST /api/eperson/registrations
Complete a new user registration is handled using POST /api/eperson/epersons?token=<:token>
Complete a password update is handled using PATCH /api/eperson/epersons/<:id-eperson>?token=<:token>
An authorization feature to verify whether you can register as a new user is created
The details of the token can be retrieved using the token (but not using the email address)
None of these requests require authentication

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.

…d added ITs

…ITs for the functionality

…ests

…ost with token and added its to verify functionality

… in RegistrationRestControllerIT

…l instead of as a parameter

…thorizeUpdatePassword method

…w-registration Conflicts: dspace-api/src/main/java/org/dspace/app/util/AuthorizeUtil.java

tdonohue

@benbosman : I gave this a (partial) code review today. Unfortunately I ran into a lot of minor mistakes/cleanup. Please see my inline comments. There are areas of this code that don't make sense to me...so either they need fixing or need inline code comments to explain the logic.

I stopped reviewing part way through as there was too much to comment on (especially in the ITs, which I found very messy overall & not implemented based on current best practices). So, I'd recommend working with the developer on cleaning up this code based on our best practices & then let me know when you feel it's ready for a public review again.

dspace-api/src/main/java/org/dspace/app/util/AuthorizeUtil.java

dspace-api/src/main/java/org/dspace/eperson/AccountServiceImpl.java

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

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

…onality

…w-registration Conflicts: dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java

… to the new registration endpoints

benbosman · 2020-06-08T16:08:29Z

@tdonohue All feedback has been resolved and/or answered
The master branch has also been merged in this branch

…n to now be available to anonymous calls

… and rewrote EPersonRestRepositoryIT tests to use this

tdonohue

@benbosman : This is looking great overall. That said, in my re-review, I stumbled on a few tiny mistakes. Some look like simple copy & paste errors that were overlooked. Once these tiny things are cleaned up, I think this is ready to go. Thanks!

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

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

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

...ver-webapp/src/test/java/org/dspace/app/rest/authorization/EPersonRegistrationFeatureIT.java

...ce-server-webapp/src/test/java/org/dspace/app/rest/jackson/IgnoreJacksonWriteOnlyAccess.java

…w-registration Conflicts: dspace-api/src/main/java/org/dspace/app/util/AuthorizeUtil.java dspace-server-webapp/src/main/java/org/dspace/app/rest/security/EPersonRestPermissionEvaluatorPlugin.java dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java

benbosman · 2020-06-19T16:04:28Z

@tdonohue all feedback has been processed, and the branch has been updated with the latest master

This should be ready to merge now

tdonohue

@benbosman : Thanks for the updates! I've found two more minor things that can be cleaned up quickly (a recent commit to add in an odd context.complete() in a test made me realize these additional minor flaws). Then it all looks good to me.

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

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

…RestRepositoryIT and added extra cleanup

abollini

I have added some minor comments and questions inline

abollini · 2020-06-25T13:39:46Z

dspace-api/src/main/java/org/dspace/app/util/AuthorizeUtil.java

+            // This allowSetPassword is currently the only mthod that would return true only when it's
+            // actually expected to be returning true.
+            // For example the LDAP canSelfRegister will return true due to auto-register, while that
+            // does not imply a new user can register explicitly
+            return AuthenticateServiceFactory.getInstance().getAuthenticationService()
+                                             .allowSetPassword(context, request, null);


I have read the previous discussion but I still don't understand why we cannot simply rely only on the dspace.cfg property. Why should I put user.registration to true if I wan't allow user registration?

As discussed with Tim in https://github.com/DSpace/DSpace/pull/2763/files/891ab3f3e261e8a78321091b8835fab413937899#r428844401, there is no method in the authentication services which correctly verifies whether you can register a new account. A ticket can be created for that problem, but it was considered out of scope for this PR to fix that issue in the API

I've created #2793

I'm fine with moving this to a separate discussion

abollini · 2020-06-25T13:41:44Z

dspace-api/src/main/java/org/dspace/eperson/AccountServiceImpl.java

+        if (!configurationService.getBooleanProperty("user.registration", true)) {
+            throw new IllegalStateException("The user.registration parameter was set to false");
+        }


this seems me to confirm that we only need to check the dspace.cfg parameter (see previous comment)

A ticket can be created to deal with the lack of this detail in the API

I've created #2793

abollini · 2020-06-25T13:49:41Z

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java

+        RegistrationData registrationData = registrationDataService.findByToken(context, token);
+        if (registrationData == null) {
+            throw new DSpaceBadRequestException("The token given as parameter: " + token + " does not exist" +
+                                                " in the database");
+        }
+        if (es.findByEmail(context, registrationData.getEmail()) != null) {
+            throw new DSpaceBadRequestException("The token given already contains an email address that resolves" +
+                                                "to an eperson");


this meet the current contract, I'm just thinking that a 422 would be more appropriate here as there is a semantic / status issue more than a formal error. But if you don't agree, ignore it and we can eventually open an issue to discuss it in future and apply later the changes (if agreed)

The contract explicitly mentions status 400 for this use case, that would contradict the contract

abollini · 2020-06-25T13:51:52Z

dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java

+                throw new DSpaceBadRequestException("The eperson.firstname and eperson.lastname values need to be " +
+                                                    "filled in");


also in this case, as the issue is inside the json I would prefer a 422. It would be useful in any case note in the contract that this error is throw when the metadata are missing (it is noted that they are mandatory)

This is not defined in the contract, 422 is fine for me. We'll update this in the code

abollini · 2020-06-25T13:59:01Z

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

+        getClient().perform(post("/api/eperson/registrations")
+                                .content(mapper.writeValueAsBytes(registrationRest))
+                                .contentType(contentType))
+                   .andExpect(status().is(401));


small thing but I would appreciate if we can use the constant here

This code has been moved, but can indeed be adjusted

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

abollini · 2020-06-25T17:48:33Z

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

+        registrationDataList = registrationDataDAO.findAll(context, RegistrationData.class);
+        assertEquals(1, registrationDataList.size());
+        assertTrue(StringUtils.equalsIgnoreCase(registrationDataList.get(0).getEmail(), eperson.getEmail()));
+        Iterator<RegistrationData> iterator = registrationDataList.iterator();
+        while (iterator.hasNext()) {
+            RegistrationData registrationData = iterator.next();
+            registrationDataDAO.delete(context, registrationData);
+        }


put in a finally block

We'll move this to a finally

...ver-webapp/src/test/java/org/dspace/app/rest/authorization/EPersonRegistrationFeatureIT.java

tdonohue

👍 Thanks @benbosman! This looks good to me now. So, once @abollini 's minor feedback is addressed, this can be merged.
(Also, as noted on Slack, if you rebase on current master then LGTM should build properly and stop putting a ❌ here. It also will analyze your PR and provide feedback as well.)

lgtm-com · 2020-06-30T06:56:33Z

This pull request introduces 1 alert when merging 0555644 into 000b5c7 - view on LGTM.com

new alerts:

1 for Missing space in string literal

abollini · 2020-06-30T15:25:02Z

it is ready to go for me now. Thanks @benbosman

Raf-atmire added 15 commits April 14, 2020 11:58

[Task 70273] added POST endpoint for new registration creation and IT

44d3d2b

[Task 70382] added the search findByToken method for Registrations an…

f07c382

…d added ITs

[Task 70397] added EPersonRegistrationFeature and added ITs

7da7ff0

[Task 70398] implementing change password with token patch and added …

fee24e3

…ITs for the functionality

[Task 70399] created the POST EPerson endpoint with token and added t…

cb8b5c8

…ests

[Task 70399] added context authorizations modifications for eperson p…

94d9f62

…ost with token and added its to verify functionality

Added extra cleanup to the EPersonRestRepositoryIT and optimized test…

b59aeec

… in RegistrationRestControllerIT

Fixed checkstyle in the EPersonRestRepositoryIT class

fc91b0b

[Task 70808] changed the link the mail to include the token in the ur…

3dc9f51

…l instead of as a parameter

[Task 70808] added javadocs

b18b52a

[Task 70808] applied feedback to the registration implementation

ea47ef0

[Task 70808] added javadoc

8ff77a9

[Task 70937] removed the canLogin parameter from the AuthorizeUtil#au…

d3cc506

…thorizeUpdatePassword method

Merge remote-tracking branch 'dspace/master' into w2p-70273_create-ne…

d75c9d3

…w-registration Conflicts: dspace-api/src/main/java/org/dspace/app/util/AuthorizeUtil.java

[Task 70937] fixes after merge

2a0971d

tdonohue requested changes May 21, 2020

View reviewed changes

[Task 71213] applied feedback to the new registration creation functi…

4ea4f91

…onality

atarix83 mentioned this pull request Jun 4, 2020

New User Registration DSpace/dspace-angular#687

Merged

5 tasks

Raf-atmire added 2 commits June 8, 2020 15:21

Merge remote-tracking branch 'dspace/master' into w2p-70273_create-ne…

9ccdff5

…w-registration Conflicts: dspace-server-webapp/src/main/java/org/dspace/app/rest/repository/EPersonRestRepository.java dspace-server-webapp/src/test/java/org/dspace/app/rest/EPersonRestRepositoryIT.java

[Task 71271] applied fixes after merge and applied community feedback…

fdaddd9

… to the new registration endpoints

Raf-atmire added 2 commits June 11, 2020 10:23

[Task 71348] fixed the patch replace password for eperson with a toke…

80f7bad

…n to now be available to anonymous calls

Added a way to ignore write only properties in REST objects for tests…

3aba1b7

… and rewrote EPersonRestRepositoryIT tests to use this

tdonohue reviewed Jun 11, 2020

View reviewed changes

heathergreerklein added this to Ready to review in DSpace 7 Beta 3 Jun 15, 2020

heathergreerklein requested a review from abollini June 18, 2020 20:42

Raf-atmire added 4 commits June 19, 2020 09:56

[Task 71442] applied feedback to the new registration endpoint

d212a84

[Task 71442] fixed compile error after merge

de6bc7d

[Task 71442] fixed the tests without the additional cleanup required

891ab3f

tdonohue reviewed Jun 22, 2020

View reviewed changes

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

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

tdonohue added authentication: password Related to default password-based authentication authentication: EPerson Related to how the EPerson object works during authentication new feature interface: REST API v7+ REST API for v7 and later (dspace-server-webapp module) labels Jun 22, 2020

tdonohue moved this from Needs Reviewers Assigned to Under Review in DSpace 7 Beta 3 Jun 23, 2020

tdonohue added this to the 7.0beta3 milestone Jun 23, 2020

tdonohue changed the title ~~beta3: New user registration & forgot password~~ DS-4027: New user registration & forgot password Jun 24, 2020

tdonohue assigned benbosman Jun 24, 2020

[Task 71604] moved RegistrationRestControllerIT tests to Registration…

32a03ee

…RestRepositoryIT and added extra cleanup

benbosman requested a review from tdonohue June 25, 2020 14:26

tdonohue added the high priority label Jun 25, 2020

YanaDePauw mentioned this pull request Jun 25, 2020

Forgot password DSpace/dspace-angular#707

Merged

5 tasks

abollini requested changes Jun 25, 2020

View reviewed changes

tdonohue mentioned this pull request Jun 25, 2020

Refactoring authority support / value pairs / controlled vocabularies DSpace/RestContract#128

Merged

tdonohue approved these changes Jun 26, 2020

View reviewed changes

[Task 71627] addressed feedback on the new registration functionaliity

0555644

benbosman requested a review from abollini June 30, 2020 08:22

Fixed LGTM issue with an error String

2a67312

tdonohue moved this from Under Review to Reviewer approved in DSpace 7 Beta 3 Jun 30, 2020

abollini approved these changes Jun 30, 2020

View reviewed changes

tdonohue merged commit cc362d8 into DSpace:master Jun 30, 2020

DSpace 7 Beta 3 automation moved this from Reviewer approved to Done Jun 30, 2020

benbosman mentioned this pull request Jul 15, 2020

No API support to verify whether you can self-register exists #2793

Closed

benbosman deleted the w2p-70273_create-new-registration branch September 11, 2020 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DS-4027: New user registration & forgot password #2763

DS-4027: New user registration & forgot password #2763

benbosman commented May 20, 2020 •

edited

Loading

tdonohue left a comment

benbosman commented Jun 8, 2020

tdonohue left a comment

benbosman commented Jun 19, 2020

tdonohue left a comment

abollini left a comment

abollini Jun 25, 2020

benbosman Jun 26, 2020

benbosman Jun 26, 2020

abollini Jun 30, 2020

abollini Jun 25, 2020

benbosman Jun 26, 2020

benbosman Jun 26, 2020

abollini Jun 25, 2020

benbosman Jun 26, 2020 •

edited

Loading

abollini Jun 25, 2020

benbosman Jun 26, 2020

abollini Jun 25, 2020

benbosman Jun 26, 2020

abollini Jun 25, 2020

benbosman Jun 26, 2020

tdonohue left a comment •

edited

Loading

lgtm-com bot commented Jun 30, 2020

abollini commented Jun 30, 2020

		throw new DSpaceBadRequestException("The eperson.firstname and eperson.lastname values need to be " +
		"filled in");

DS-4027: New user registration & forgot password #2763

DS-4027: New user registration & forgot password #2763

Conversation

benbosman commented May 20, 2020 • edited Loading

References

Description

Instructions for Reviewers

Checklist

tdonohue left a comment

Choose a reason for hiding this comment

benbosman commented Jun 8, 2020

tdonohue left a comment

Choose a reason for hiding this comment

benbosman commented Jun 19, 2020

tdonohue left a comment

Choose a reason for hiding this comment

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

benbosman Jun 26, 2020 • edited Loading

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 • edited Loading

Choose a reason for hiding this comment

lgtm-com bot commented Jun 30, 2020

abollini commented Jun 30, 2020

benbosman commented May 20, 2020 •

edited

Loading

benbosman Jun 26, 2020 •

edited

Loading

tdonohue left a comment •

edited

Loading