philnash / pwned

😱 An easy, Ruby way to use the Pwned Passwords API.
https://rubygems.org/gems/pwned/
MIT License
426 stars 22 forks source link

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

Closed paprikati closed 4 years ago

paprikati commented 4 years ago

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 commented 4 years ago

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 commented 4 years ago

I'll take a crack this afternoon

paprikati commented 4 years ago

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 commented 4 years ago

@philnash any thoughts on the above?

philnash commented 4 years ago

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 commented 4 years ago

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

philnash commented 4 years ago

Awesome, I look forward to it!