Implement Devise Pwned Password To Restrict Breached Passwords #1935
Conversation
- uses google translate
- recommends reenable if stubbing with Webmock
|
|
||
| def after_sign_in_path_for(resource) | ||
| if resource.respond_to?(:pwned?) && resource.pwned? | ||
| cookies[:pwned] = { value: true, expires: Time.now + 10.minutes } |
There was a problem hiding this comment.
Since pwned status is only called in the resource during an after_sign_in_path_for called, I can't use user.pwned? in the _update_password_modal.html.erb view as well since that state can no longer be tested and will just return nil because the password is only checked with haveIBeenpwned upon login for security reasons.
I stored this as a temporary cookie to check it right after the user login to make the change password notification pop up if their password was compromised.
If anyone have a better idea or suggestion, let me know :)
| devise :invitable, :database_authenticatable, :registerable, :uid, :lockable, | ||
| :recoverable, :rememberable, :trackable, :validatable, :omniauthable, | ||
| omniauth_providers: %i[google_oauth2 facebook] | ||
| # https://github.com/michaelbanfield/devise-pwned_password#disabling-in-test-environments | ||
| # TODO: might be best to reenable if we disable real network requests & stub them with https://github.com/bblimke/webmock | ||
| devise :pwned_password unless Rails.env.test? |
There was a problem hiding this comment.
Disabling testing for this based on the disabling in test environments section of Devise Pwned Password an external API calls. I will reenable this if we implement Webmock then create tests that checks that users can still register if haveIBeenPwned service was down for any reason and pwned passwords to be invalid.
There was a problem hiding this comment.
Sounds good, thanks for explaining this. It sounds like we can safely disable this since we don't' need to test for this in our tests.
| @@ -93,3 +93,4 @@ de: | |||
| not_saved: | |||
| one: '1 Fehler verhinderte das Speichern von %{resource}' | |||
| other: '%{count} Fehler verhinderten das Speichern von %{resource}' | |||
| pwned_password: "ist zuvor in einem Datenverstoß aufgetreten und sollte niemals verwendet werden. Wenn Sie es schon einmal benutzt haben, ändern Sie es sofort!" | |||
There was a problem hiding this comment.
I pretty much used Google Translate for all of these messages so if any native speakers want to give feedback on any or fix some in the future, that would help 👍 (I can speak and read Vietnamese, but my grammar and spelling are bad).
bennpham
force-pushed
the
devise-pwned
branch
from
a18da98 to
49ae965
Compare
| devise :invitable, :database_authenticatable, :registerable, :uid, :lockable, | ||
| :recoverable, :rememberable, :trackable, :validatable, :omniauthable, | ||
| omniauth_providers: %i[google_oauth2 facebook] | ||
| # https://github.com/michaelbanfield/devise-pwned_password#disabling-in-test-environments | ||
| # TODO: might be best to reenable if we disable real network requests & stub them with https://github.com/bblimke/webmock | ||
| devise :pwned_password unless Rails.env.test? |
There was a problem hiding this comment.
Sounds good, thanks for explaining this. It sounds like we can safely disable this since we don't' need to test for this in our tests.
Description
This implements the Pwned Password Gem which uses https://haveibeenpwned.com/ API to make sure that user does not used a password that has been found in a known data breach and reenables the password pop up if users password were found in a data breach.
More Details
I was going to implement tests for this, but based on the diabling in test environments section, I disabled Pwned Passwords for test environment because it'd call an external API every time you create a new user in the tests. There is a workaround for this by using gems such as webmock (I use this one at work) or VCR to stub/mock external service request calls.
In regards to using Webmock, you'll need to add
WebMock.disable_net_connect!(allow_localhost: true)torails_helper.rb. This would probably cause all of the Service specs to fail since they'll no longer make external API calls directly (which would also speed up the tests) since you'll have to mock the expected output of the service class (happy case and unhappy case) instead of rely on the service to be up to send you back the result in the tests. I think this might make it easier to setup the project as well since everyone won't need to setup Cloudinary API env and other ENV keys to pass their tests since it'll be stubbed.You can read more about that on: https://www.betterspecs.org/#stubbing and https://marnen.github.io/webmock-presentation/webmock.html
Corresponding Issue
#1917
Screenshots
Sorry for low qual gifs. Going higher resolution was looking a bit costly on the filesize.
Signing up with Breached Password
Logging in with existing Breached Password
Trying to change password to breached password
Reviewing this pull request? Check out our Code Review Practices guide if you haven't already!