Skip to content
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

[Login-UI] Password Strength Indicator #23574

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

andreas-blaettlinger
Copy link
Contributor

Closes #23573

Very first draft implemented for the register page only. Want to collect some feedback or other proposals.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Unreported flaky test detected, please review

@ghost
Copy link

ghost commented Sep 28, 2023

Unreported flaky test detected

If the below flaky tests below are affected by the changes, please review and update the changes accordingly. Otherwise, a maintainer should report the flaky tests prior to merging the PR.

org.keycloak.testsuite.ui.account2.WelcomeScreenTest#applicationsTest

Keycloak CI - Account Console IT (firefox)

org.awaitility.core.ConditionTimeoutException: Assertion condition defined as a org.keycloak.testsuite.util.URLAssert URL expected to begin with: https://localhost:8543/auth/realms/test/protocol/openid-connect/auth ; actual URL: https://localhost:8543/auth/realms/test/account/#/applications within 10 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:119)
	at org.awaitility.core.AssertionCondition.await(AssertionCondition.java:31)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:985)
...

Report flaky test

@jonkoops jonkoops requested a review from a team September 28, 2023 10:07
@ghost ghost added the team/ui label Sep 28, 2023
@jonkoops
Copy link
Contributor

Aside from the technical implementation, 'password strength' is a relative term that means different things depending on who you ask. Some users might not even want to have this indicator at all. This likely means that if we were to land this feature it would have to be configurable.

ping @keycloak/core

@andreas-blaettlinger
Copy link
Contributor Author

@jonkoops Yes exactly, I agree, looking forward to discussions. The initial state of the PR was just to provide a small demo on the register page as a basis for discussion; not very nice code-wise but functional. Fixed your remarks anyway :)

@pedroigor
Copy link
Contributor

@andreas-blaettlinger It is a very nice start to start thinking about something more generic.

@jonkoops I would say also configurable in the sense that it should take into account the password policies set to the realm.

@jbman
Copy link

jbman commented Oct 4, 2023

@pedroigor A strength indicator is usually used independently of password rules. Patternfly has a nice definition: "A password strength indicator is displayed to a user after they have entered a password in a field. It allows the user to understand the strength of a password after it has met the password requirements."

I agree with configurability if it should be shown or not. But which mechanism to use? For our custom themes we use localized messages fro configurability, e.g. "passwordStrengthIndicator.show" = false.

@@ -104,6 +104,17 @@
</span>
</#if>
</div>
<div class="${properties.kcInputWrapperClass!}" id="password-entropy" data-password-input="password">
<div class="progress">
<div class="progress-bar" role="status" aria-valuenow="0" aria-valuemin="0" aria-valuemax="100"
Copy link
Contributor

Choose a reason for hiding this comment

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

The status role doesn't understand the value* attributes, so they can be removed. As long as there is a human-readable text that is inside of the element it will be legible to screen readers.

Suggested change
<div class="progress-bar" role="status" aria-valuenow="0" aria-valuemin="0" aria-valuemax="100"
<div class="progress-bar" role="status"

display: none;
}

#password-entropy > .progress {
Copy link
Contributor

Choose a reason for hiding this comment

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

There should be no need to use a child combinator here, using the class name should be more than specific enough.

Suggested change
#password-entropy > .progress {
#password-entropy .progress {

@@ -1,7 +1,7 @@
parent=base
import=common/keycloak

styles=css/login.css
styles=css/login.css css/password-entropy.css
Copy link
Contributor

Choose a reason for hiding this comment

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

Perhaps we can add the styles to login.css instead? That saves us from needing to request another file.

Suggested change
styles=css/login.css css/password-entropy.css
styles=css/login.css

entropyElement.style.display = "block";
}

const entropyElement = document.querySelector('#password-entropy');
Copy link
Contributor

Choose a reason for hiding this comment

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

Rather than using the id of the element I think we should be doing the same as we do for password visibility. Which is to use the data attribute to find the element on the page, and then bootstrapping all of the logic on that element.

Alternatively, since we're already using modern JavaScript, we could go the extra mile and just splurge on making this a full-fledged Web Component.

Thinking about it a bit more, I think Web Components might be a lot better of a harness than the sprinkles of JavaScript we've been doing. Also from a maintainability perspective.

WDYT?

@andreas-blaettlinger andreas-blaettlinger force-pushed the feature/login-theme-password-entropy branch from b39b775 to 4dc219a Compare October 6, 2023 11:57
@andreas-blaettlinger
Copy link
Contributor Author

@jonkoops Provided a new WebComponent version. Also made some other changes, e.g. I removed the data-password-input="password", instead I search for a input[type="password"] within the parent. Let me know what you think about that.

@andreas-blaettlinger
Copy link
Contributor Author

There was this hint from @pedroigor to make this feature configurable. Is there any plan how to handle those fine-grained theme settings and where to persist those yet?

@thomasdarimont
Copy link
Contributor

thomasdarimont commented Oct 18, 2023

How about creating a property showPasswordQuality in themes.properties with default to false?

In register.ftl you could then write:

<#if properties.showPasswordQuality == "true">
                      <!-- insert passwort quality js here -->
                        <password-entropy data-password-poor="${msg('poorPassword')}"
                                          data-password-weak="${msg('weakPassword')}"
                                          data-password-good="${msg('goodPassword')}"
                                          data-password-strong="${msg('strongPassword')}">
                        </password-entropy>
</#if>

in https://github.com/keycloak/keycloak/blob/main/themes/src/main/resources/theme/base/login/register.ftl#L3
You could then also move the loading of the java script into the if block. This avoids loading the js if the feature is not needed.

With that in place themes that inherit from the base theme will get this feature for free and they just have to activate it in their themes.properties

@andreas-blaettlinger
Copy link
Contributor Author

andreas-blaettlinger commented Oct 23, 2023

Updated PR which now includes the configuration via theme.properties as suggested by @thomasdarimont. Just enabled showPasswordQuality by default because this was a security relevant finding as described in #23573. Security by default sounds much better, WDYT?

Also added the Password Strength Indicator to all relevant FTL pages.

Any feedback welcome. :)

@jonkoops jonkoops requested a review from a team October 30, 2023 12:06
@ghost ghost added the team/core label Oct 30, 2023
@jonkoops
Copy link
Contributor

I have no objections against this feature, but it will need to gather some approval from the core team.

@ssilvert ssilvert marked this pull request as draft October 30, 2023 12:08
@ssilvert
Copy link
Contributor

I converted this PR to a draft as it looks like that was the intention all along. And I think this probably needs more discussion.

I agree that the strength indicator should be on by default. But I don't think existing installations will necessarily like having this appear when they upgrade. So we need to decide which way to go on that.

Also, it looks like this doesn't satisfy PatternFly's recommendation that the indicator only comes on after the general password requirements are met? And that leads us into a discussion of the oft-requested feature of displaying those password requirements to the user. The user should be told what the requirements are and also should be told how to create a stronger password. I'm not saying we have to implement that other feature to merge this PR, but the two features are closely related.

Lastly, we will need some documentation on how to customize passwordEntropy.js.

@jbman
Copy link

jbman commented Nov 6, 2023

Hi @ssilvert,

the intention is also to get a strength indicator into Keycloak to fulfill ASVS v4.0 Password recommendation 2.1.8:

2.1.8 | Verify that a password strength meter is provided to help users set a stronger password.

On by-default would be good, on by default at new realms and not at existing ones would be best.

But it would also be fine if it could be activated like the password policies. A policy "Show strength indicator" could be added. It wouldn't be rule which prevents any passwords, but password settings would be in one place.

image

The Patternfly recommendation that "strength indicator is displayed after the password has met all password requirements." could be realized, as soon as the Patternfly component is used and password rules are also displayed to the user beforehand.
In a setup with only one minimum length rule, the strength-indicator is still helpful to encourage strong passwords even if the minimum length is displayed as error after sending the form.

@MarcelSz
Copy link

MarcelSz commented Mar 4, 2024

This idea is nice. However, I'm thinking, how to return password policy from KeyCloak to js. I have many realms with different policies.

As you know, password policies are stored as a single string: "length(8) and maxLength(64) and upperCase(1) and lowerCase(1)". I'd like to have a possibility to validate password in real time - instant feedback. Maybe you have any hints, how to start?

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

Successfully merging this pull request may close these issues.

[Login UI] Password Strength Indicator
8 participants