Feature/forgot password #421

ivanmrsulja · 2023-10-02T13:22:28Z

VIVO PR
This PR solves "I forgot my pasword" issue.

What does this pull request do?

This PR allows the user to request password change from login widget, instead of having to ask an admin to do it for him.

What's new?

Password reset functionality with built-in anti-spam measures. I have added a new servlet (ForgotPassword) that handles the logic. I have also updated i18n files with required frontend labels. Have in mind that I only speak English and Serbian (and a tiny bit of German :D) so I am not well suited for translation. For majority of languages I have used Google Translate so if you notice any translation errors please leave a comment on this PR.

How should this be tested?

Try to change your password through login widget. Try to spam and see what happens.

Additional Notes:

Removed old captcha implementation files which was replaced in PR
jquery.realperson.css
jquery.realperson.js

Admin notification email translations:

English

German

Spanish

Russian

Portuguese

French-Canadian

Interested parties

@chenejac @litvinovg @brianjlowe

…PAM messages. Fixed dao saving bug.

…ogging in.

chenejac

@ivanmrsulja is it possible to use infoM or errorMessage at widget-login.ftl instead of specifying html output (at the next blank page) in the ForgotPassword Servlet? If you try to type wrong message in the login widget, you will see the errorMessage. Can that property be used for informing user about sent email (infoMessage) and warning when the next email can be sent (errorMessage) in the case of "I forgot my password" was clicked?

Moreover, if you type wrong password and try to login, you will get the error message, and after that the option "I forgot my password" is not working in my case. Can you please check this scenario?

ivanmrsulja · 2023-10-24T14:23:58Z

@ivanmrsulja is it possible to use infoM or errorMessage at widget-login.ftl instead of specifying html output (at the next blank page) in the ForgotPassword Servlet? If you try to type wrong message in the login widget, you will see the errorMessage. Can that property be used for informing user about sent email (infoMessage) and warning when the next email can be sent (errorMessage) in the case of "I forgot my password" was clicked?

Moreover, if you type wrong password and try to login, you will get the error message, and after that the option "I forgot my password" is not working in my case. Can you please check this scenario?

I have discussed whether the message should be displayed in a toast-like way that you are suggesting or on the next blank page with Brian and Georgy on a dynapi meeting about 1 month ago. After a brief discussion we went with the blank page approach but I am willing to change it if that is the better way to do it, never the less the change is not that big so it could be implemented in a short time. In regards of the bug you are mentioning, it has to do with the JS logic, i will look at it as soon as possible.

…s with required labels.

litvinovg · 2023-11-01T08:06:19Z

I think there is a need for configuration option to turn this feature on/off.
Also should requestHistory and requestFrequency implementations be thread safe?

ivanmrsulja · 2023-11-03T10:30:35Z

I think there is a need for configuration option to turn this feature on/off. Also should requestHistory and requestFrequency implementations be thread safe?

I am allready working on feature toggle but I didn't think of thread safety for this scenario so thanks for reminding me 🙂 . Do you know if just switching to ConcurrentHashMap will suffice or will I have to implement mutual exclusion?

litvinovg · 2023-11-03T12:41:34Z

I am allready working on feature toggle but I didn't think of thread safety for this scenario so thanks for reminding me 🙂 . Do you know if just switching to ConcurrentHashMap will suffice or will I have to implement mutual exclusion?
I think ConcurrentHashMap should be fine.

ivanmrsulja · 2023-11-03T16:25:07Z

I am allready working on feature toggle but I didn't think of thread safety for this scenario so thanks for reminding me 🙂 . Do you know if just switching to ConcurrentHashMap will suffice or will I have to implement mutual exclusion?
I think ConcurrentHashMap should be fine.

In addition, I used atomic operations for addition and increment so as not to cause a potential race condition. With regards to mutual exclusion, I have not thought of a scenario where I would need to synchronize a part of the code.

…ration. Added java docs.

… toggle.

webapp/src/main/webapp/templates/freemarker/body/captcha/captcha-clientExecutionLogic.ftl

bkampe

German translations are fine.

litvinovg

@ivanmrsulja I added some comments, please have a look.
Russian translations look good to me.
Link to restore password works for me.
Styles might need to be improved.

home/src/main/resources/rdf/i18n/sr_Latn_RS/interface-i18n/firsttime/vitro_UiLabel.ttl

litvinovg · 2024-03-12T10:43:50Z

webapp/src/main/webapp/templates/freemarker/body/captcha/captcha-clientExecutionLogic.ftl

+<#assign isEnabled = isEnabled!true>
+
+${stylesheets.add('<link rel="stylesheet" href="${urls.base}/templates/freemarker/edit/forms/css/customForm.css" />')}
+${scripts.add('<script type="text/javascript" src="${urls.base}/js/commentForm.js"></script>',


is js/commentForm.js used on password reset page?

It is not needed, I have removed it. Jquery import is also not needed.

litvinovg · 2024-03-12T10:45:32Z

webapp/src/main/webapp/templates/freemarker/body/captcha/captcha-clientExecutionLogic.ftl

@@ -0,0 +1,39 @@
+<#assign isEnabled = isEnabled!true>
+
+${stylesheets.add('<link rel="stylesheet" href="${urls.base}/templates/freemarker/edit/forms/css/customForm.css" />')}


is ./webapp/src/main/webapp/templates/freemarker/edit/forms/css/customForm.css used on password reset page?

litvinovg · 2024-03-12T14:42:28Z

.../java/edu/cornell/mannlib/vitro/webapp/controller/authenticate/ForgotPasswordController.java

+        UserAccountsDao userAccountsDao = constructUserAccountsDao();
+        I18nBundle i18n = I18n.bundle(vreq);
+
+        String email = getNonNullTrimmedParameterValue(vreq, "email");


Maybe first check if string is shorter than 320, and trim only in this case?

litvinovg

I think this component should reuse styles from existing themes.

.../java/edu/cornell/mannlib/vitro/webapp/controller/authenticate/ForgotPasswordController.java

litvinovg

@ivanmrsulja Thank you very much. Logic looks good to me. Could you please clean up some minor issues I described in the comments below.
@chenejac are we going to apply theme specific styling in this PR or it should be done separately? If it is out of scope, then we can create a ticket for theme styling and merge this PR after small clean-up.

api/src/main/java/edu/cornell/mannlib/vitro/webapp/web/widgets/LoginWidget.java

.../java/edu/cornell/mannlib/vitro/webapp/controller/authenticate/ForgotPasswordController.java

webapp/src/main/webapp/templates/freemarker/body/captcha/captcha-clientExecutionLogic.ftl

webapp/src/main/webapp/templates/freemarker/body/accounts/userAccounts-resetPasswordRequest.ftl

litvinovg · 2024-04-04T10:29:43Z

webapp/src/main/webapp/templates/freemarker/edit/forms/css/customForm.css

@@ -164,6 +164,12 @@ section#pubsContainer input {
 img#indicator {
    padding-left:60px;
 }
+


This file doesn't contain modifications related to this PR. Please remove new empty line.

litvinovg

Tested with themes: WIlma, Ternderfoot, Nemo, Vitro.
With turned on nano captcha and with turned off captcha.
Successfully restored forgotten password by following link to reset password.
Russian translations look good to me.
Code looks good to me.

chenejac

@ivanmrsulja looks nice. Thanks

bkampe

Just a minor optimization, the rest of the german translations looks good.

bkampe · 2024-04-18T09:11:36Z

home/src/main/resources/rdf/i18n/de_DE/interface-i18n/firsttime/vitro_UiLabel.ttl

+uil-data:password_reset_email_sent.Vitro
+        rdf:type         owl:NamedIndividual ;
+        rdf:type         uil:UILabel ;
+        rdfs:label       "Eine E-Mail zur Wiederherstellung des Passworts wird an {0} gesendet, wenn sie mit einem bestehenden Konto verknüpft ist."@de-DE ;


Suggested change

rdfs:label "Eine E-Mail zur Wiederherstellung des Passworts wird an {0} gesendet, wenn sie mit einem bestehenden Konto verknüpft ist."@de-DE ;

rdfs:label "Eine E-Mail zur Wiederherstellung des Passworts wird an {0} gesendet, wenn die Adresse mit einem bestehenden Konto verknüpft ist."@de-DE ;

chenejac

Looks ok for me and works well. Serbian labels are fine.

michel-heon · 2024-05-07T14:14:18Z

home/src/main/resources/rdf/i18n/fr_CA/interface-i18n/firsttime/vitro_UiLabel.ttl

michel-heon · 2024-05-07T14:16:36Z

home/src/main/resources/rdf/i18n/fr_CA/interface-i18n/firsttime/vitro_UiLabel.ttl

@@ -3185,7 +3241,7 @@ uil-data:javascript_instructions.Vitro
 uil-data:error_invalid_email.Vitro
        rdf:type         owl:NamedIndividual ;
        rdf:type         uil:UILabel ;
-        rdfs:label       "''{0}'' n'est pas une adresse courriel valide."@fr-CA ;
+        rdfs:label       "L'adresse e-mail que vous avez fournie n'est pas une adresse e-mail valide."@fr-CA ;


Replace by:
L'adresse électronique que vous avez fournie n'est pas valide.

ivanmrsulja added 6 commits September 29, 2023 16:20

Added forgot password servlet as well as some UI and i18n files changes.

051cb5e

Moved labels in the correct language file.

4926978

Implemented localisation and delayed successive requests to prevent S…

65f629c

…PAM messages. Fixed dao saving bug.

Refactored logic in order to not force user to change password upon l…

5f2bdf3

…ogging in.

Added generic app name loading in url.

f3e4c90

Fixed wait time bug.

74528fb

chenejac requested review from litvinovg, brianjlowe and chenejac October 17, 2023 09:33

chenejac linked an issue Oct 17, 2023 that may be closed by this pull request

I forgot my vivo password vivo-project/VIVO#3770

Open

chenejac requested changes Oct 24, 2023

View reviewed changes

ivanmrsulja added 5 commits October 24, 2023 17:05

Link unavaliable after unsuccessfull login bug fix.

fab4304

Remade message notifications to be inside VIVO context.

62bebcc

Moved password change form to separate page. Updated translation file…

8ec6dcb

…s with required labels.

Removed contact us section. Cleaned up mitigation code.

d7852d1

Added proper support page link, fixed information disclosure issue.

618c169

ivanmrsulja added 3 commits November 1, 2023 15:12

Fixed codestyle violations.

81f14d8

Added captcha.

054afd5

Fixed localization bug regarding Serbian labels.

767e8fe

Added feature toggle. Fixed concurrency issues in SPAM mitigation class.

3a96b35

ivanmrsulja requested a review from chenejac November 3, 2023 16:26

Switched to use enabled/disabled instead of boolean value for configu…

fcbb7bc

…ration. Added java docs.

ivanmrsulja mentioned this pull request Nov 7, 2023

Added configuration example for feature toggle. vivo-project/VIVO#3916

Open

ivanmrsulja added 2 commits November 7, 2023 13:43

Added example configuration for forgot password functionality feature…

459a690

… toggle.

Renamed showFormIfEnabled to showForm. Updated java docs.

d797bfe

ivanmrsulja requested review from litvinovg and michel-heon March 1, 2024 17:07

Moved styles to separate css files, refactored selectors.

1b70450

wwelling reviewed Mar 7, 2024

View reviewed changes

webapp/src/main/webapp/templates/freemarker/body/captcha/captcha-clientExecutionLogic.ftl Outdated Show resolved Hide resolved

bkampe approved these changes Mar 12, 2024

View reviewed changes

litvinovg reviewed Mar 12, 2024

View reviewed changes

Code cleanup and slight email check refactoring.

2831dfa

ivanmrsulja requested review from litvinovg and wwelling March 14, 2024 09:49

michel-heon approved these changes Mar 19, 2024

View reviewed changes

litvinovg reviewed Mar 19, 2024

View reviewed changes

.../java/edu/cornell/mannlib/vitro/webapp/controller/authenticate/ForgotPasswordController.java Outdated Show resolved Hide resolved

.../java/edu/cornell/mannlib/vitro/webapp/controller/authenticate/ForgotPasswordController.java Outdated Show resolved Hide resolved

Fixed potential NPE and improved error message localization.

a4f9209

ivanmrsulja requested a review from litvinovg March 25, 2024 14:32

litvinovg requested changes Apr 4, 2024

View reviewed changes

Fixed minor licencing and tag usage errors.

a5ab108

ivanmrsulja requested a review from litvinovg April 8, 2024 07:24

ivanmrsulja added 2 commits April 12, 2024 09:15

Removed useless blank line in css file.

a7bf2ac

Added newline at the end of file.

f7de431

litvinovg previously approved these changes Apr 12, 2024

View reviewed changes

Updated example properties.

d2bc008

ivanmrsulja dismissed litvinovg’s stale review via d2bc008 April 12, 2024 13:28

chenejac previously approved these changes Apr 15, 2024

View reviewed changes

Updated spanish localization.

b7a5339

ivanmrsulja dismissed chenejac’s stale review via b7a5339 April 15, 2024 15:45

bkampe suggested changes Apr 18, 2024

View reviewed changes

chenejac approved these changes Apr 28, 2024

View reviewed changes

litvinovg self-requested a review May 2, 2024 14:41

litvinovg approved these changes May 2, 2024

View reviewed changes

michel-heon reviewed May 7, 2024

View reviewed changes

home/src/main/resources/rdf/i18n/fr_CA/interface-i18n/firsttime/vitro_UiLabel.ttl

Copy link

Member

michel-heon May 7, 2024

Choose a reason for hiding this comment

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

It is good

michel-heon reviewed May 7, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/forgot password #421

Feature/forgot password #421

ivanmrsulja commented Oct 2, 2023 •

edited

chenejac left a comment

ivanmrsulja commented Oct 24, 2023

litvinovg commented Nov 1, 2023

ivanmrsulja commented Nov 3, 2023

litvinovg commented Nov 3, 2023

ivanmrsulja commented Nov 3, 2023

bkampe left a comment

litvinovg left a comment

litvinovg Mar 12, 2024

ivanmrsulja Mar 14, 2024 •

edited

litvinovg Mar 12, 2024

ivanmrsulja Mar 14, 2024

litvinovg Mar 12, 2024

litvinovg left a comment

litvinovg left a comment

litvinovg Apr 4, 2024

litvinovg left a comment

chenejac left a comment

bkampe left a comment

bkampe Apr 18, 2024

chenejac left a comment

michel-heon May 7, 2024

michel-heon May 7, 2024

		@@ -0,0 +1,39 @@
		<#assign isEnabled = isEnabled!true>

		${stylesheets.add('<link rel="stylesheet" href="${urls.base}/templates/freemarker/edit/forms/css/customForm.css" />')}

	rdfs:label "Eine E-Mail zur Wiederherstellung des Passworts wird an {0} gesendet, wenn sie mit einem bestehenden Konto verknüpft ist."@de-DE ;
	rdfs:label "Eine E-Mail zur Wiederherstellung des Passworts wird an {0} gesendet, wenn die Adresse mit einem bestehenden Konto verknüpft ist."@de-DE ;

Feature/forgot password #421

Are you sure you want to change the base?

Feature/forgot password #421

Conversation

ivanmrsulja commented Oct 2, 2023 • edited

What does this pull request do?

What's new?

How should this be tested?

Additional Notes:

Admin notification email translations:

English

German

Spanish

Russian

Portuguese

French-Canadian

Interested parties

chenejac left a comment

Choose a reason for hiding this comment

ivanmrsulja commented Oct 24, 2023

litvinovg commented Nov 1, 2023

ivanmrsulja commented Nov 3, 2023

litvinovg commented Nov 3, 2023

ivanmrsulja commented Nov 3, 2023

bkampe left a comment

Choose a reason for hiding this comment

litvinovg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanmrsulja Mar 14, 2024 • edited

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

litvinovg left a comment

Choose a reason for hiding this comment

litvinovg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

litvinovg left a comment

Choose a reason for hiding this comment

chenejac left a comment

Choose a reason for hiding this comment

bkampe left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenejac left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ivanmrsulja commented Oct 2, 2023 •

edited

ivanmrsulja Mar 14, 2024 •

edited