-
Notifications
You must be signed in to change notification settings - Fork 984
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
feature!: add maximum character limit to password length #21593
Conversation
9edc2fe
to
cefd06e
Compare
Jenkins BuildsClick to see older builds (22)
|
cefd06e
to
e4953a1
Compare
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.
Self-Review 📑
[password-with-hint/view | ||
{:hint (if (not password-short-enough?) | ||
{:text (i18n/label | ||
:t/password-creation-max-length-hint) | ||
:status :error | ||
:shown? true} | ||
{:text (i18n/label :t/password-creation-hint) | ||
:status hint-1-status | ||
:shown? true}) |
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.
Here we're showing the "maximum 100 characters" error for the Create Password form.
[password-with-hint/view | ||
{:hint (if (not short-enough?) | ||
{:text (i18n/label :t/password-creation-max-length-hint) | ||
:status :error | ||
:shown? true} | ||
{:text (i18n/label :t/password-creation-hint) | ||
:status (if long-enough? :success :default) | ||
:shown? true}) |
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.
Here we're showing the "maximum 100 characters" error for the Change Password form.
(ns status-im.common.password-with-hint.view | ||
(:require | ||
[quo.core :as quo] | ||
[quo.foundations.colors :as colors] | ||
[react-native.core :as rn] | ||
[status-im.common.password-with-hint.style :as style])) | ||
|
||
(defn view | ||
[{{:keys [text status shown?]} :hint :as input-props}] | ||
[rn/view | ||
[quo/input | ||
(-> input-props | ||
(dissoc :hint) | ||
(assoc :type :password | ||
:blur? true))] | ||
[rn/view {:style style/info-message} | ||
(when shown? | ||
[quo/info-message | ||
(cond-> {:status status | ||
:size :default} | ||
(not= :success status) (assoc :icon :i/info) | ||
(= :success status) (assoc :icon :i/check-circle) | ||
(= :default status) (assoc :color colors/white-70-blur)) | ||
text])]]) |
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.
Here we're creating a common component for both the Create Password and Change Password screens.
e4953a1
to
817531a
Compare
88% of end-end tests have passed
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (7)Click to expandClass TestWalletOneDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Class TestWalletMultipleDevice:
|
hi @seanstrom, thank you for PR. take a look found issues: ISSUE 1: "Very weak" validation is shown on the 'create profile password' page during onboarding in case when the password field is emptySteps:Go to onboarding password creation Actual result:
Expected result:
|
PR_ISSUE 2: "Passwords do not match" validation is not shown on the 'create profile password' page during onboardingSteps:
Actual result:Expected result:"Passwords do not match" validation is shown Devices:
|
|
||
(defn view | ||
[{{:keys [text status shown?]} :hint :as input-props}] | ||
[rn/view |
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.
Could it be a fragment instead of view
?
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.
Yeah good idea! I'll change it to a fragment 👍
817531a
to
0660fdf
Compare
@VolodLytvynenko thanks for finding these issues 🙌 For issue 2 and issue 3 , I've pushed up some changes to fix those issues. Let me know if issue 1 should be addressed in this PR please 🙏 |
62% of end-end tests have passed
Failed tests (2)Click to expandClass TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestWalletOneDevice:
Class TestCommunityOneDeviceMerged:
|
Hi @seanstrom, thank you for fixes. And sorry for the PR's back-and-forth. I think it's best to fix Issue 1 within this PR. We don’t know when the validation simplification will be done, and merging as-is would mean showing 'Very Weak' by default on the create profile password page, which seems worse than the current issue on the latest develop where 'Very Strong' is not shown at all Also, the current PR has different validation logic for 'create profile password' and 'change password': For now:
While on
Neither setup is ideal, but aligning them with a consistent logic would be better. Expected result: |
Hey @VolodLytvynenko @seanstrom, I think this password strength validator leaves a lot to be desired. For example, a password I would remove this feature completely from the onboarding and only keep the basic length validation, but to align this point would take approvals from other folks. If you would like to see this discussion move forward please give a thumbs up and I'll gladly try to help. Edit: this separate discussion shouldn't block this PR 🚀 |
@VolodLytvynenko Sounds good, about making them consistent and not showing "Very weak" by default 👍 I've pushed a change that should make them consistent for both change-password and create-password screens. Now both of them will not show "Very weak" by default. Let me know what you think 🙏 |
@ilmotta we may be able to create a more nuanced calculation for password strength. Currently we only count how many validations have passed, but we could try to create something that only follows some of the guidelines mentioned here. Overall I'm okay with removing a misleading feature, and if we can prioritise a better feature that would be nice too 🙌 |
The following is an interesting discussion from Bitwarden. Even more sophisticated calculations can be very misleading. Do you use password strength indicators? Complete this survey https://community.bitwarden.com/t/do-you-use-password-strength-indicators-complete-this-survey/62738/2 Next week I'll try to take the discussion somewhere else and hear other people's opinions. Similar to how we discussed about identity rings and eventually descoped it completely from the product. Now, better than ever is a good chance to understand why things were built the way they were and improve. |
@seanstrom @VolodLytvynenko In the desktop app, the strength validator is somewhat useful, but in mobile it's quite misleading/wrong. For example, in desktop, the password I'll create a new issue about this feature parity problem 👍🏼 It's going to be a low priority feature to work on for 2.32, even though it's related to the onboarding. |
25% of end-end tests have passed
Failed tests (5)Click to expandClass TestWalletMultipleDevice:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestCommunityOneDeviceMerged:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (2)Click to expandClass TestWalletOneDevice:
|
0% of end-end tests have passed
Failed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletMultipleDevice:
|
bfd56ae
to
de0b073
Compare
hi @seanstrom Thank you for the fixes. Unfortunately, additional issue is found PR_ISSUE 4: "Maximum 100 chars" validation is shown by default on create "password password" and "change password" pagesSteps:
Actual result:"Maximum 100 chars" validation is shown by default Expected result:The "Maximum 100 chars" validation message should only be displayed after the user enters more than 100 characters. Devices:
|
de0b073
to
9a82770
Compare
Hey @VolodLytvynenko thank you for catching that issue, I've pushed another fix that should show the correct password hint before typing in the password. |
62% of end-end tests have passed
Failed tests (2)Click to expandClass TestWalletMultipleDevice:
Expected to fail tests (1)Click to expandClass TestCommunityMultipleDeviceMerged:
Passed tests (5)Click to expandClass TestCommunityOneDeviceMerged:
Class TestOneToOneChatMultipleSharedDevicesNewUi:
Class TestWalletOneDevice:
|
100% of end-end tests have passed
Passed tests (2)Click to expandClass TestWalletMultipleDevice:
|
hi @seanstrom thank you for PR. No issues from my side. PR is ready to be merged |
9a82770
to
17b87a5
Compare
fixes #21433
Summary
Platforms
Areas that maybe impacted
Functional
Steps to test
Before and after screenshots comparison
After changes
Create Profile Password
Screen.Recording.2024-11-06.at.09.35.51.mov
Change Profile Password
Screen.Recording.2024-11-06.at.09.35.07.mov
status: ready