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

Using this asynchronously, but pre-hashing the password #19

Closed
paprikati opened this issue Jul 2, 2020 · 7 comments · Fixed by #20
Closed

Using this asynchronously, but pre-hashing the password #19

paprikati opened this issue Jul 2, 2020 · 7 comments · Fixed by #20

Comments

@paprikati
Copy link
Contributor

I'd like to call the pwned method async (i.e. from a worker) so it doesn't slow down my API call.
But I also would rather not put the password into my queueing db.

Could we add support for this in the library?

I am thinking something like

  1. Route hashes the password
  2. Route enqueues a job with the password-hash as an arg
  3. Worker passes the password-hash into a pwned method which passes it through to the API

I suspect I can already do this by hacking the internals of the library, but was wondering if you think it's something worth explicitly supporting?

Happy to write a PR with an implementation (probably just a standalone 'hash password' method and then a way of initialising a Pwned::Password with an already hashed password)

@philnash
Copy link
Owner

philnash commented Jul 2, 2020

Hey, thanks for opening this issue! This is actually the same request as #17, but with another strong reason to implement.

I can work on this myself, but if you would like to have a go at it, then please do. Your approach to the interface sounds correct too.

Let me know if you're going to take a crack at it. 🙂

@paprikati
Copy link
Contributor Author

I'll take a crack this afternoon

@paprikati
Copy link
Contributor Author

Three options for the interface that I can see:

  1. add hashed: to the possible options, but strip it before we send it to the HTTP client (bit icky)
  2. add a new argument hashed before request_options, which would require a major version bump
  3. add a new function Pwned.pwned_with_hash? or something else (I cannot come up with a good name for this function)

What's your preference?

@paprikati
Copy link
Contributor Author

@philnash any thoughts on the above?

@philnash
Copy link
Owner

philnash commented Jul 7, 2020

I had a hard time thinking about this, because you're right that the 3 options aren't particularly attractive. 1 is icky, 2 seems like it would be unnecessary and I couldn't think of a good name for 3 either.

Then I had an idea.

When you create a Pwned::Password it is a password. So what if we extracted a bunch of the behaviour and shared it with a Pwned::HashedPassword or Pwned::PasswordHash (not decided on the name yet)? Then you could call:

password = Pwned::Password.new('password')
password.pwned?
#=> true
password.pwned_count
#=> 3759315

and also

password = Pwned::PasswordHash.new('5BAA61E4C9B93F3F0682250B6CF8331B7EE68FD8')
password.pwned?
#=> true
password.pwned_count
#=> 3759315

This won't require a new version, doesn't mess with hash options and doesn't introduce a weird method name.

What do you think?

@paprikati
Copy link
Contributor Author

Yeah I think this is the cleanest approach. Let's do it. Will get a PR done this week

@philnash
Copy link
Owner

philnash commented Jul 7, 2020

Awesome, I look forward to it!

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 a pull request may close this issue.

2 participants