-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
[#9192] Makes forgot-password link removable #9194
Conversation
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.
@vins01-4science : Thanks! Overall, this looks good. I reported most of my feedback in my frontend review.
That said, I did notice some IT cleanup that can be done in this PR. You don't need to reset configurations back to the original values (they do that automatically). There's also a missing IT that we probably should add. See inline comments below.
...r-webapp/src/test/java/org/dspace/app/rest/authorization/EPersonForgotPasswordFeatureIT.java
Outdated
Show resolved
Hide resolved
...r-webapp/src/test/java/org/dspace/app/rest/authorization/EPersonForgotPasswordFeatureIT.java
Outdated
Show resolved
Hide resolved
...r-webapp/src/test/java/org/dspace/app/rest/authorization/EPersonForgotPasswordFeatureIT.java
Outdated
Show resolved
Hide resolved
...r-webapp/src/test/java/org/dspace/app/rest/authorization/EPersonForgotPasswordFeatureIT.java
Outdated
Show resolved
Hide resolved
...e-server-webapp/src/main/java/org/dspace/app/rest/repository/RegistrationRestRepository.java
Show resolved
Hide resolved
@tdonohue Thank you for the review. All the changes have been addressed on this PR. Let me know if you find something else. |
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.
👍 Thank you @vins01-4science ! This looks good to me now.
I will note though that we MUST add documentation for this new user.forgot-password
configuration to the DSpace 8.x docs. It probably should just be added next to user.registration
here: https://wiki.lyrasis.org/display/DSDOC8x/Authentication+Plugins#AuthenticationPlugins-ConfiguringAuthenticationbyPassword
I'll get this merged, but will add a needs documentation
label until the docs have been added. If you can find time to add them, please link them here & we can remove that label.
@tdonohue Thank you! I modified that page, by placing the new property right after the |
Thanks @vins01-4science ! The documentation looks good to me. https://wiki.lyrasis.org/display/DSDOC8x/Authentication+Plugins#AuthenticationPlugins-ConfiguringAuthenticationbyPassword I'll remove the |
References
Description
This PR introduces a new
AuthorizationFeature
that checks for the forgot-password feature.The same feature can also check, whenever is provided, that the user is able to update its password.
Instructions for Reviewers
Try to enable the additional configuration:
user.forgot-password = false
inside the
authentication-password.cfg
and check that it is not shown inside the login-component, in both:Checklist
pom.xml
), I've made sure their licenses align with the DSpace BSD License based on the Licensing of Contributions documentation.