Skip to content

Conversation

shaidar
Copy link
Contributor

@shaidar shaidar commented Jul 15, 2025

What are the relevant tickets?

https://github.com/mitodl/hq/issues/7342

Description (What does it do?)

Create a custom email verification flow

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Summary of Changes

Hello @shaidar, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!

This pull request introduces a custom email verification flow for Keycloak, transitioning from a simple click-to-verify link to a more robust and secure code-based system. This change enhances account security by requiring users to input a unique, time-sensitive code sent to their email, while also improving the user experience with updated UI elements and clear instructions.

Highlights

  • Custom Keycloak Email Verification: I've implemented a new RequiredActionProvider in Keycloak, CustomVerifyEmailRequiredAction, to manage a custom email verification process. This replaces the standard link-based verification with a more secure and interactive code-based flow.
  • Code-Based Verification: The new flow generates and sends a 6-digit, time-limited (15-minute expiry) verification code to the user's registered email. Users must enter this code on the login page to complete their account verification.
  • Enhanced User Interface: The login verification page (login-verify-email.ftl) has been updated to include a dedicated input field for the verification code and a 'Resend Code' option, improving the user experience during the verification process.
  • Email Template Update: The email template (email-verification.mjml) now dynamically includes the generated verification code, along with clear instructions and an expiry notice, ensuring users have all necessary information to complete verification.
Using Gemini Code Assist

The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.

Invoking Gemini

You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

Customization

To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a custom email verification flow using a one-time code. I've identified a bug that causes duplicate emails to be sent, a mismatch between the generated code type and the UI description. I've also identified some areas where the user experience can be improved, particularly around code expiration.

Comment on lines 32 to 38
String resend = context.getHttpRequest().getDecodedFormParameters().getFirst("resend");
if ("true".equals(resend)) {
sendVerificationCode(context);
} else if (context.getUser().getFirstAttribute("email_verification_code") == null) {
// First time - send code
sendVerificationCode(context);
}

Choose a reason for hiding this comment

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

high

There is a logic issue that causes two verification emails to be sent when the user clicks "Resend Code". The processAction method handles the resend request by calling sendVerificationCode and then requiredActionChallenge. However, requiredActionChallenge also checks for the "resend" parameter (which is still present in the request) and calls sendVerificationCode again.

The logic in requiredActionChallenge should be simplified to only send a code if one doesn't already exist. The resend action is already correctly handled in processAction.

if (context.getUser().getFirstAttribute("email_verification_code") == null) {
    // This handles the first time the form is shown, or if the code has expired
    // and been cleared. The resend action is handled in processAction.
    sendVerificationCode(context);
}

}

private void sendVerificationCode(RequiredActionContext context) {
String code = RandomString.randomCode(6);

Choose a reason for hiding this comment

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

high

The login form (login-verify-email.ftl) and email template specify a "6-digit code". However, RandomString.randomCode(6) generates an alphanumeric string (e.g., aB1dE2), which could confuse users and lead to validation failures. To generate a digits-only code, you should use RandomString.randomString(6, RandomString.DIGITS).

String code = RandomString.randomString(6, RandomString.DIGITS);

Comment on lines 89 to 111
private boolean isValidVerificationCode(RequiredActionContext context, String code) {
String storedCode = context.getUser().getFirstAttribute("email_verification_code");
String timestampStr = context.getUser().getFirstAttribute("email_code_timestamp");

if (storedCode == null || timestampStr == null) {
return false;
}

// Check if code is expired (15 minutes)
try {
long timestamp = Long.parseLong(timestampStr);
long now = System.currentTimeMillis();
if (now - timestamp > 15 * 60 * 1000) {
context.getUser().removeAttribute("email_verification_code");
context.getUser().removeAttribute("email_code_timestamp");
return false;
}
} catch (NumberFormatException e) {
return false;
}

return Objects.equals(code, storedCode);
}

Choose a reason for hiding this comment

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

high

This method has a side effect: it removes user attributes when the code expires (lines 102-103). A method named isValid... should ideally be a pure function without side effects. This also leads to a confusing user experience, as the user gets a generic "Invalid verification code" message when their code expires, without being told a new one has been sent.

I recommend separating the expiration check from the code validation and handling these cases explicitly in processAction to provide better feedback to the user. For example, you could have separate methods for checking expiration and correctness, and then processAction can provide specific error messages like "Your code has expired. A new one has been sent."

@shaidar
Copy link
Contributor Author

shaidar commented Jul 15, 2025

/gemini review

Copy link

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request introduces a custom email verification flow using a 6-digit code. The implementation is good, but I've provided feedback on a few points. The most critical one is to use SecureRandom for code generation to enhance security. I've also pointed out a potential security issue in the email template where a default verification link could bypass the custom flow. Other suggestions focus on improving maintainability by using constants and following theming best practices like using message bundles for text.

Comment on lines 15 to 45
<#if code??>
<!-- Verification Code Section -->
<mj-text align="center" padding="20px 0 10px 0">
<h2 style="color: #333333; margin: 0;">Your Verification Code</h2>
</mj-text>

<mj-text align="center" padding="10px 20px">
<div style="background-color: #f8f9fa; border: 2px solid #e9ecef; border-radius: 8px; padding: 20px; margin: 20px 0;">
<span style="font-size: 36px; font-weight: bold; color: #A31F34; letter-spacing: 4px;">
${code}
</span>
</div>
</mj-text>

<mj-text align="center" padding="0 20px 20px 20px">
<p style="font-size: 14px; color: #666666; margin: 0;">
Enter this code on the verification page to complete your email verification.
</p>
<p style="font-size: 12px; color: #999999; margin: 10px 0 0 0;">
This code expires in 15 minutes.
</p>
</mj-text>

<mj-divider border-color="#e9ecef" border-width="1px" padding="20px 0" />

<mj-text align="center" padding="0 20px">
<p style="font-size: 14px; color: #666666;">
Or click the button below to verify automatically:
</p>
</mj-text>
</#if>

Choose a reason for hiding this comment

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

critical

The template provides two ways to verify: entering a code or clicking a button. However, the custom RequiredActionProvider only implements the code-based flow. The ${link} variable used in the <mj-button> is for Keycloak's default link-based email verification. Clicking it will either fail or bypass your custom code-based flow, which is a potential security issue and creates a confusing user experience. To fix this, you should remove the link-based verification option from this template, including the text and the button.

          <#if code??>
          <!-- Verification Code Section -->
          <mj-text align="center" padding="20px 0 10px 0">
            <h2 style="color: #333333; margin: 0;">Your Verification Code</h2>
          </mj-text>

          <mj-text align="center" padding="10px 20px">
            <div style="background-color: #f8f9fa; border: 2px solid #e9ecef; border-radius: 8px; padding: 20px; margin: 20px 0;">
              <span style="font-size: 36px; font-weight: bold; color: #A31F34; letter-spacing: 4px;">
                ${code}
              </span>
            </div>
          </mj-text>

          <mj-text align="center" padding="0 20px 20px 20px">
            <p style="font-size: 14px; color: #666666; margin: 0;">
              Enter this code on the verification page to complete your email verification.
            </p>
            <p style="font-size: 12px; color: #999999; margin: 10px 0 0 0;">
              This code expires in 15 minutes.
            </p>
          </mj-text>

          <mj-divider border-color="#e9ecef" border-width="1px" padding="20px 0" />
          </#if>

Comment on lines 125 to 129
java.util.Random random = new java.util.Random();
for (int i = 0; i < length; i++) {
sb.append(random.nextInt(10));
}
return sb.toString();

Choose a reason for hiding this comment

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

high

For security-sensitive operations like generating verification codes, java.util.Random should not be used as its output can be predictable. Please use java.security.SecureRandom instead.

    private static final java.security.SecureRandom random = new java.security.SecureRandom();

    private String generateNumericCode(int length) {
        StringBuilder sb = new StringBuilder(length);
        for (int i = 0; i < length; i++) {
            sb.append(random.nextInt(10));
        }
        return sb.toString();
    }

try {
long timestamp = Long.parseLong(timestampStr);
long now = System.currentTimeMillis();
if (now - timestamp > 15 * 60 * 1000) {

Choose a reason for hiding this comment

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

medium

The code expiration time is a magic number. It's better to define it as a constant to improve readability and maintainability.

private static final long CODE_EXPIRATION_MS = 15 * 60 * 1000L; // 15 minutes

if (now - timestamp > CODE_EXPIRATION_MS) {

}

private void sendVerificationCode(RequiredActionContext context) {
String code = generateNumericCode(6); // 6-digit numeric code

Choose a reason for hiding this comment

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

medium

The code length is a magic number. It's better to define it as a constant to improve readability and maintainability.

private static final int CODE_LENGTH = 6;
String code = generateNumericCode(CODE_LENGTH); // 6-digit numeric code

Comment on lines +43 to 54
<p class="pf-v5-u-my-md">A 6-digit verification code has been sent to your email address.</p>
<hr />
<p class="pf-v5-u-my-md">${msg("emailVerifyInstruction3")}</p>
<p class="pf-v5-u-my-md">Enter the code from your email to verify your account.</p>
<p class="pf-v5-u-my-md">
<b>${msg("emailVerifyInstruction4Bold")}</b>
<b>Code not working?</b>
${msg("emailVerifyInstruction4")}
</p>
<p class="pf-v5-u-my-md">
<b>Didn't receive the code?</b>
Check your spam folder or click "Resend Code" above.
<a href="#">${msg("emailVerifySupportLinkTitle")}</a>.
</p>

Choose a reason for hiding this comment

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

medium

This template contains several hardcoded, user-facing strings (e.g., "A 6-digit verification code...", "Code not working?", etc.). For better maintainability and to support internationalization (i18n), it's a best practice in Keycloak theming to move these strings to a message properties file (e.g., theme/ol/login/messages/messages_en.properties) and reference them with the msg() function, like ${msg("verifyEmail.infoText1")}.

            <p class="pf-v5-u-my-md">${msg("verifyEmailInstruction2")}</p>
            <hr />
            <p class="pf-v5-u-my-md">${msg("verifyEmailInstruction3")}</p>
            <p class="pf-v5-u-my-md">
                <b>${msg("emailVerifyInstruction4Bold")}</b>
                ${msg("emailVerifyInstruction4")}
            </p>
            <p class="pf-v5-u-my-md">
                <b>${msg("emailVerifyInstruction5Bold")}</b>
                ${msg("emailVerifyInstruction5")}
                <a href="#">${msg("emailVerifySupportLinkTitle")}</a>.
            </p>

@blarghmatey blarghmatey requested a review from Copilot July 15, 2025 20:29
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements a custom email verification flow for Keycloak that replaces the default link-based verification with a 6-digit code-based system. The implementation provides a more secure and user-friendly verification process with code expiration and resend functionality.

Key changes:

  • Custom Keycloak Required Action Provider for code-based email verification
  • Updated login template with verification code input form and resend functionality
  • Modified email template structure with whitespace cleanup

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
CustomVerifyEmailRequiredAction.java Core implementation of the custom verification provider with code generation, validation, and email sending
login-verify-email.ftl Updated UI template to include code input form and resend button instead of simple email instructions
email-verification.mjml Minor whitespace cleanup in email template structure
RequiredActionFactory Service registration file for the custom provider
pom.xml Added Keycloak dependencies required for custom provider development
Comments suppressed due to low confidence (1)

ol-keycloak/oltheme/src/main/resources/theme/ol/login/login-verify-email.ftl:40

  • The template references 'verify-email.ftl' in the Java code but this file is named 'login-verify-email.ftl'. Ensure the template name in the Java code matches this filename.
    <#elseif section = "info">

attributes.put("code", code);
emailProvider.setRealm(context.getRealm())
.setUser(context.getUser())
.send("emailVerificationSubject", "email-verification.ftl", attributes);
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

The email template reference uses 'email-verification.ftl' but the actual file is 'email-verification.mjml'. This will cause template resolution to fail.

Suggested change
.send("emailVerificationSubject", "email-verification.ftl", attributes);
.send("emailVerificationSubject", "email-verification.mjml", attributes);

Copilot uses AI. Check for mistakes.

try {
long timestamp = Long.parseLong(timestampStr);
long now = System.currentTimeMillis();
if (now - timestamp > 15 * 60 * 1000) {
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

The expiration time (15 minutes) is hardcoded as a magic number. Consider extracting this to a configurable constant for better maintainability.

Suggested change
if (now - timestamp > 15 * 60 * 1000) {
if (now - timestamp > EMAIL_VERIFICATION_EXPIRATION_MS) {

Copilot uses AI. Check for mistakes.


Response challenge = context.form()
.setAttribute("user", context.getUser())
.createForm("verify-email.ftl");
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Template name 'verify-email.ftl' doesn't match the actual template file 'login-verify-email.ftl'. This will cause template resolution to fail.

Suggested change
.createForm("verify-email.ftl");
.createForm("login-verify-email.ftl");

Copilot uses AI. Check for mistakes.

Comment on lines +94 to +100
.createForm("verify-email.ftl");
context.challenge(challenge);
} else {
Response challenge = context.form()
.setError("Invalid verification code. Please check your email and try again.")
.setAttribute("user", context.getUser())
.createForm("verify-email.ftl");
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Template name 'verify-email.ftl' doesn't match the actual template file 'login-verify-email.ftl'. This will cause template resolution to fail.

Suggested change
.createForm("verify-email.ftl");
context.challenge(challenge);
} else {
Response challenge = context.form()
.setError("Invalid verification code. Please check your email and try again.")
.setAttribute("user", context.getUser())
.createForm("verify-email.ftl");
.createForm("login-verify-email.ftl");
context.challenge(challenge);
} else {
Response challenge = context.form()
.setError("Invalid verification code. Please check your email and try again.")
.setAttribute("user", context.getUser())
.createForm("login-verify-email.ftl");

Copilot uses AI. Check for mistakes.

Comment on lines +94 to +100
.createForm("verify-email.ftl");
context.challenge(challenge);
} else {
Response challenge = context.form()
.setError("Invalid verification code. Please check your email and try again.")
.setAttribute("user", context.getUser())
.createForm("verify-email.ftl");
Copy link

Copilot AI Jul 15, 2025

Choose a reason for hiding this comment

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

Template name 'verify-email.ftl' doesn't match the actual template file 'login-verify-email.ftl'. This will cause template resolution to fail.

Suggested change
.createForm("verify-email.ftl");
context.challenge(challenge);
} else {
Response challenge = context.form()
.setError("Invalid verification code. Please check your email and try again.")
.setAttribute("user", context.getUser())
.createForm("verify-email.ftl");
.createForm("login-verify-email.ftl");
context.challenge(challenge);
} else {
Response challenge = context.form()
.setError("Invalid verification code. Please check your email and try again.")
.setAttribute("user", context.getUser())
.createForm("login-verify-email.ftl");

Copilot uses AI. Check for mistakes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant