OTP form issues #7

kotnikd3 · 2024-04-24T18:15:21Z

Fix #6

The number of tries is incorrectly displayed as "{0}" in case user submits incorrect code.
User can submit an incorrect code infinite number of times regardless of the value set for "Max Retries".
The label "<< Back to Login" should be revised to something like "Resend Email", as it triggers the email resend action instead of restarting the login process (like pressing on the icon "Restart login" above on the right side of user's email).

Tested on Keycloak version 22.0.5 and 24.0.3. Screenshot of the new form:

- The number of tries is incorrectly displayed as "{0}" in case user submits incorrect code. - User can submit an incorrect code more times than the specified value set for "Max Retries".

…"Resend Email"

jspizziri · 2024-04-24T20:57:11Z

pom.xml

@@ -13,9 +13,9 @@
 		<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
 		<maven.compiler.source>${java.version}</maven.compiler.source>
 		<maven.compiler.target>${java.version}</maven.compiler.target>
-		<keycloak.version>22.0.5</keycloak.version>
+		<keycloak.version>24.0.3</keycloak.version>


I'm pretty sure this is going to introduce a backwards compatibility breaking change. We still need to target Keycloak 22.

Due to java.version? Changed back to 22.0.5.

pratibha3011 · 2024-04-30T06:59:34Z

When is this PR targeted to merge @kotnikd3 ? anything blocking this?

kotnikd3 · 2024-04-30T07:11:05Z

When is this PR targeted to merge @kotnikd3 ? anything blocking this?

@pratibha3011, it is up to @jspizziri to approve and merge it.

jspizziri · 2024-04-30T10:16:17Z

@pratibha3011 @kotnikd3 I need to have time to test it prior to merging it. It's on my radar and I'll get to it asap.

mohitcse22 · 2024-05-02T07:38:13Z

@kotnikd3 i have some issues with it
resend email button gives me expired code error

kotnikd3 · 2024-05-02T07:46:38Z

@kotnikd3 i have some issues with it resend email button gives me expired code error

@mohitcse22 I can not reproduce the issue you are describing. Can you provide Keycloak version and a screenshot of Authentication/<your otp flow>/Flow details screen?

Ken-Michalak · 2024-07-31T19:01:14Z

@kotnikd3
I see why you had an issue when it's your only login option and you're using "Required". Removing that condition makes sense. (That probably shouldn't have been there.)

The remaining attempt logic was correct originally, but maybe the wording is confusing. The configuration form says, "Max Retries", not attempts. So if it's configured with "1", and you enter a wrong code once, you have 1 chance to retry and you can get it on the 2nd attempt. If it's 0, then it immediately fails.
(It also has no message on the last failure, and just goes back to login, which probably should be fixed, but I guess it was like that before.)

The "Resend email" (or "Back to Login") seems to be acting like a back button, so it does neither of those things with the auth flow and user I just tried. I have it set up as an alternative, and the first thing I see is a password form, so if I hit "Resend email", it actually just goes back to the password form. I feel like that should just be removed, or at least left as is. In the base theme, it looks like there already is another button to "Restart login". If you need to resend the code, you can always just restart the login.

If that button does work for you though, you can just override that file in your theme and add those messages, instead of in here. It would just go in themes/your-theme/login/totp-form.ftl.

kotnikd3 · 2024-08-01T11:38:18Z

@kotnikd3 I see why you had an issue when it's your only login option and you're using "Required". Removing that condition makes sense. (That probably shouldn't have been there.)

The remaining attempt logic was correct originally, but maybe the wording is confusing. The configuration form says, "Max Retries", not attempts. So if it's configured with "1", and you enter a wrong code once, you have 1 chance to retry and you can get it on the 2nd attempt. If it's 0, then it immediately fails. (It also has no message on the last failure, and just goes back to login, which probably should be fixed, but I guess it was like that before.)

The "Resend email" (or "Back to Login") seems to be acting like a back button, so it does neither of those things with the auth flow and user I just tried. I have it set up as an alternative, and the first thing I see is a password form, so if I hit "Resend email", it actually just goes back to the password form. I feel like that should just be removed, or at least left as is. In the base theme, it looks like there already is another button to "Restart login". If you need to resend the code, you can always just restart the login.

If that button does work for you though, you can just override that file in your theme and add those messages, instead of in here. It would just go in themes/your-theme/login/totp-form.ftl.

@Ken-Michalak
With the original version, regardless of the value set for "Max Retries", the user can submit incorrect code an infinite number of times and it never redirects user back to the initial login form (with input for email).

Ken-Michalak

I can get this merged in if you can restrict it to the fix.

Ken-Michalak · 2024-08-01T14:57:39Z

src/main/java/com/weare5stones/keycloak/authenticators/emailotp/EmailOTPAuthenticator.java

+      remainingAttempts--;
+      authSession.setAuthNote(AUTH_NOTE_REMAINING_RETRIES, Integer.toString(remainingAttempts));


Move this inside the the condition.

Ken-Michalak · 2024-08-01T14:58:34Z

src/main/resources/theme-resources/templates/totp-form.ftl

Revert these changes. (The button probably should just be removed, but that could be a breaking change for some.)

We could add a resend button, but to get that working properly for everyone, it would still be removing the "back to login", and adding a new button with some logic in src/main/java/com/weare5stones/keycloak/authenticators/emailotp/EmailOTPAuthenticator.java

Ken-Michalak · 2024-08-01T14:59:32Z

src/main/java/com/weare5stones/keycloak/authenticators/emailotp/EmailOTPAuthenticator.java

+                          .setError("emailTOTPCodeInvalid", Integer.toString(remainingAttempts)).createForm(TOTP_FORM));
+      } else {
+          // Reset login
+          context.resetFlow();


Add an error here if you can. If you can't, I may take a look at that later.

Ken-Michalak · 2024-08-01T15:01:14Z

pom.xml

@@ -15,7 +15,7 @@
 		<maven.compiler.target>${java.version}</maven.compiler.target>
 		<keycloak.version>22.0.5</keycloak.version>
 		<maven-shade.version>3.2.4</maven-shade.version>
-		<package.version>2.0.0</package.version>
+		<package.version>2.1.0</package.version>


This can just be 2.0.1, since it's just fixing the attempt logic when it's set as required.

kotnikd3 · 2024-10-15T11:25:08Z

I can get this merged in if you can restrict it to the fix.

Hi @jspizziri @Ken-Michalak, If you would be willing to continue with what I have started, I would be grateful.

kotnikd3 added 3 commits April 24, 2024 20:10

Fix:

e4433f4

- The number of tries is incorrectly displayed as "{0}" in case user submits incorrect code. - User can submit an incorrect code more times than the specified value set for "Max Retries".

Fix The label "<< Back to Login" should be revised to something like …

b0e114a

…"Resend Email"

Change versions in pom.xml

6d871d2

kotnikd3 changed the title ~~Fix displaying attempts~~ OTP form issues Apr 24, 2024

kotnikd3 mentioned this pull request Apr 24, 2024

OTP form issues #6

Open

kotnikd3 changed the title ~~OTP form issues~~ OTP form issues #6 Apr 24, 2024

kotnikd3 changed the title ~~OTP form issues #6~~ OTP form issues Apr 24, 2024

jspizziri reviewed Apr 24, 2024

View reviewed changes

Downgrade keycloak.version in pom.xml

bac015f

kotnikd3 requested a review from jspizziri April 26, 2024 06:59

jspizziri assigned jhaltercmu2020 May 21, 2024

jspizziri assigned Ken-Michalak Jul 31, 2024

Ken-Michalak requested changes Aug 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OTP form issues #7

OTP form issues #7

kotnikd3 commented Apr 24, 2024 •

edited

Loading

jspizziri Apr 24, 2024

kotnikd3 Apr 25, 2024

pratibha3011 commented Apr 30, 2024

kotnikd3 commented Apr 30, 2024

jspizziri commented Apr 30, 2024

mohitcse22 commented May 2, 2024

kotnikd3 commented May 2, 2024 •

edited

Loading

Ken-Michalak commented Jul 31, 2024

kotnikd3 commented Aug 1, 2024 •

edited

Loading

Ken-Michalak left a comment

Ken-Michalak Aug 1, 2024

Ken-Michalak Aug 1, 2024

Ken-Michalak Aug 1, 2024

Ken-Michalak Aug 1, 2024

kotnikd3 commented Oct 15, 2024 •

edited

Loading

		remainingAttempts--;
		authSession.setAuthNote(AUTH_NOTE_REMAINING_RETRIES, Integer.toString(remainingAttempts));

OTP form issues #7

Are you sure you want to change the base?

OTP form issues #7

Conversation

kotnikd3 commented Apr 24, 2024 • edited Loading

jspizziri Apr 24, 2024

Choose a reason for hiding this comment

kotnikd3 Apr 25, 2024

Choose a reason for hiding this comment

pratibha3011 commented Apr 30, 2024

kotnikd3 commented Apr 30, 2024

jspizziri commented Apr 30, 2024

mohitcse22 commented May 2, 2024

kotnikd3 commented May 2, 2024 • edited Loading

Ken-Michalak commented Jul 31, 2024

kotnikd3 commented Aug 1, 2024 • edited Loading

Ken-Michalak left a comment

Choose a reason for hiding this comment

Ken-Michalak Aug 1, 2024

Choose a reason for hiding this comment

Ken-Michalak Aug 1, 2024

Choose a reason for hiding this comment

Ken-Michalak Aug 1, 2024

Choose a reason for hiding this comment

Ken-Michalak Aug 1, 2024

Choose a reason for hiding this comment

kotnikd3 commented Oct 15, 2024 • edited Loading

kotnikd3 commented Apr 24, 2024 •

edited

Loading

kotnikd3 commented May 2, 2024 •

edited

Loading

kotnikd3 commented Aug 1, 2024 •

edited

Loading

kotnikd3 commented Oct 15, 2024 •

edited

Loading