You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
By default the value set above is used to reject passwords and warn users.
Optionally, you can add the following snippet to config/initializers/devise.rb
if you want to use different thresholds for rejecting the password and warning
the user (for example you may only want to reject passwords that are common but
warn if the password occurs at all in the list):
# Minimum number of times a pwned password must exist in the data set in order# to warn the user.config.min_password_matches_warn=1
But if you look at that changeset, it seems like mostly a documentation change. It doesn't appear to actually change any behavior, except that it adds an error, when the docs only talk about it adding a warning, so when I saw that in the code I was pretty confused and started to dig deeper.
To be fair, this isn't #16's fault. There was already this in the docs, which says that it's an optional feature but doesn't say how to enable this feature:
You can optionally warn existing users when they sign in if they are using a password from the PwnedPasswords dataset. The default message is:
Both of these should probably link to and/or make it very clear that you need to add this snippet for it to actually show the warning that it's talking about!:
Can we make the whole warn-on-log-in feature automatic, controlled by a config option, instead of requiring copy and paste of a method override?
If possible we should simply make it as simple to enable the the warn-on-log-in behavior as setting a config option.
There's already
config.pwned_password_check_on_sign_in
I would suggest renaming that to config.pwned_password_warn_on_sign_in, since the check isn't really useful for anything unless you do something with it, right?
But I guess we could add pwned_password_warn_on_sign_in as a new separate option to keep backward compatibility, and have pwned_password_warn_on_sign_in (which would also require pwned_password_check_on_sign_in to be true) only control whether to add the warning or not.
How?
Is there any reason we couldn't just automatically prepend a module that adds the after_sign_in_path_for method override mentioned in the Readme??
Otherwise, maybe we could look into doing this with a Warden::Manager.after_authentication callback, but that doesn't have access to the controller, so I'm not sure if that option is even possible.
Should be pretty easy to add. The hardest part would be adding the tests, because it doesn't look there are any tests that actually test the integration with Devise and actually test signing in. What would be the best way to add those tests? I guess check what other devise plugins do for integration tests?
The text was updated successfully, but these errors were encountered:
TylerRick
added a commit
to TylerRick/devise-pwned_password
that referenced
this issue
Apr 25, 2020
#16 added this to docs:
But if you look at that changeset, it seems like mostly a documentation change. It doesn't appear to actually change any behavior, except that it adds an error, when the docs only talk about it adding a warning, so when I saw that in the code I was pretty confused and started to dig deeper.
To be fair, this isn't #16's fault. There was already this in the docs, which says that it's an optional feature but doesn't say how to enable this feature:
Both of these should probably link to and/or make it very clear that you need to add this snippet for it to actually show the warning that it's talking about!:
But better yet...
Can we make the whole warn-on-log-in feature automatic, controlled by a config option, instead of requiring copy and paste of a method override?
If possible we should simply make it as simple to enable the the warn-on-log-in behavior as setting a config option.
There's already
I would suggest renaming that to
config.pwned_password_warn_on_sign_in
, since the check isn't really useful for anything unless you do something with it, right?But I guess we could add
pwned_password_warn_on_sign_in
as a new separate option to keep backward compatibility, and havepwned_password_warn_on_sign_in
(which would also requirepwned_password_check_on_sign_in
to be true) only control whether to add the warning or not.How?
Is there any reason we couldn't just automatically prepend a module that adds the
after_sign_in_path_for
method override mentioned in the Readme??Otherwise, maybe we could look into doing this with a
Warden::Manager.after_authentication
callback, but that doesn't have access to the controller, so I'm not sure if that option is even possible.Should be pretty easy to add. The hardest part would be adding the tests, because it doesn't look there are any tests that actually test the integration with Devise and actually test signing in. What would be the best way to add those tests? I guess check what other devise plugins do for integration tests?
The text was updated successfully, but these errors were encountered: