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

Docs are confusing/misleading regarding min_password_matches_warn #32

Open
TylerRick opened this issue Apr 25, 2020 · 0 comments
Open

Comments

@TylerRick
Copy link
Contributor

#16 added this to docs:

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!:

def after_sign_in_path_for(resource)
  set_flash_message! :alert, :warn_pwned if resource.respond_to?(:pwned?) && resource.pwned?
  super
end

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

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?

TylerRick added a commit to TylerRick/devise-pwned_password that referenced this issue Apr 25, 2020
TylerRick added a commit to TylerRick/devise-pwned_password that referenced this issue Apr 27, 2020
…) clearer

Move all documentation about this warning to its own section so that it's not spread out within the
document.

[michaelbanfield#32]
TylerRick added a commit to TylerRick/devise-pwned_password that referenced this issue Apr 28, 2020
…) clearer

Move all documentation about this warning to its own section so that it's not spread out within the
document.

[michaelbanfield#32]
TylerRick added a commit to TylerRick/devise-pwned_password that referenced this issue Apr 28, 2020
…) clearer

Move all documentation about this warning to its own section so that it's not spread out within the
document.

[michaelbanfield#32]
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

No branches or pull requests

1 participant