Skip to content

LG-13014: Implement password compromised#10861

Merged
mdiarra3 merged 21 commits intomainfrom
LG-13014-implement-password-check
Jul 29, 2024
Merged

LG-13014: Implement password compromised#10861
mdiarra3 merged 21 commits intomainfrom
LG-13014-implement-password-check

Conversation

@mdiarra3
Copy link
Contributor

🎫 Ticket

Link to the relevant ticket:
LG-13014

🛠 Summary of changes

This will Allow users to be notified that their password has been compromised and prompted to change their password.

📜 Testing Plan

Provide a checklist of steps to confirm the changes.
Steps to test

  1. Set compromised_password_randomizer_value and compromised_password_randomizer_threshold to 1 and check_user_password_compromised_enabled to true in the environment you are testing on.
  2. Create An account with Bad password (Will need to go into rails console and update it to be a bad password after creating account)
  3. Sign out of account and sign in with Bad password
  4. Be expected to be taken to a page lettign user know to change password
  5. If you sign out and attempt to do it again you will be directed to after_sign_in_path for user instead of password compromised page.

👀 Screenshots

Screenshot 2024-06-25 at 9 19 30 AM

@mdiarra3 mdiarra3 requested a review from a team June 25, 2024 13:29
@mdiarra3 mdiarra3 marked this pull request as ready for review June 25, 2024 17:27
@aduth
Copy link
Contributor

aduth commented Jun 25, 2024

I was able to trigger the forced password change when signing in to http://localhost:3000 directly, but it did not show when signing in from the sample application. Can you check to make sure that works, since we should assume most users will be signing in to access a partner application?

@aduth
Copy link
Contributor

aduth commented Jun 26, 2024

I was able to trigger the forced password change when signing in to http://localhost:3000 directly, but it did not show when signing in from the sample application. Can you check to make sure that works, since we should assume most users will be signing in to access a partner application?

Follow-up: I paired with @mdiarra3 and this was an issue with how I was testing, and it does actually work as expected.

The issue was due to how I was trying to reset the password_compromised_checked_at value on my user account through the Rails console, and in how it wasn't always resetting correctly due to how Rails caches and avoids database queries.

In case others have similar issues in testing, fixing it was a matter of changing from:

user.update(password_compromised_checked_at: nil)

To:

user.reload.update(password_compromised_checked_at: nil)
# Or maybe?
# User.find_with_email('email@example.com').update(password_compromised_checked_at: nil)

@mdiarra3 mdiarra3 requested a review from aduth July 2, 2024 13:33
Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

I'm liking the new approach reusing the edit password controller 👍

Copy link
Contributor

@aduth aduth left a comment

Choose a reason for hiding this comment

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

A few minor remarks and a question, but LGTM overall 👍

@mdiarra3 mdiarra3 merged commit ec4fb18 into main Jul 29, 2024
@mdiarra3 mdiarra3 deleted the LG-13014-implement-password-check branch July 29, 2024 14:22
Comment on lines +18 to +19
def aria_described_by_if_eligible
return {} if required_password_change?
Copy link
Contributor

Choose a reason for hiding this comment

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

Since we render the description for all views, do we need this condition anymore, or should we just always apply the aria-describedby ?

mitchellhenke pushed a commit that referenced this pull request Jul 31, 2024
* changelog: Upcoming Features, Authentication, let users who's passwords are compromised to change their password

* add password compromised spec and edit objects

* sign in spec updated

* change to use user_password_params

* address comments

* change up to use password controller

* password compromised updates

* fix lint and use global variable

* update edit spec and global variable

* fix the sign in spec

* fix form spec

* address comments, update test to better redirect

* remove former check

* make sure invalid password method is correct

* Update to make aria described conditional

* update specs and use hash format for attributes instead of positional arguments

* fix reset password form spec

* remove conditional for password strength description
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.

4 participants