Store hashed tokens - Githubissues

mikker / passwordless

🗝 Authentication for your Rails app without the icky-ness of passwords

MIT License

1.26k stars 87 forks source link

Store hashed tokens #81

Closed timwis closed 1 year ago

timwis commented 4 years ago

Stores hashed versions of the tokens in the database per #80. This PR is not quite done yet, but I wanted to share the draft to get feedback in the meantime. I'm not normally a ruby developer so I'm sure there's things we could improve in it.

I used OpenSSL::HMAC like devise does (and basically copied their token generator class) since BCrypt is not idempotent (there's no way to do session = Session.find_by(token_hash: BCrypt::Password.create(token)) as create generates a new salt every time).

I'll leave more comments inline in the code.

I know I still need to:

[x] Get the assertion in navigation_test to pass
[ ] Figure out how to use the rails secret key for generating the key name for HMAC, or whether that's even necessary (devise seems to do it)

Looking forward to your feedback (feel free to edit directly). Hope this is a good start.

mikker commented 4 years ago

Wonderful work so far ❤️

timwis commented 4 years ago

Thanks! I’d love your thoughts on the field rename and migration bit when you have a sec (see my inline comments). And the secret key thing if any ideas come to mind, otherwise I’ll do some more research on it.

timwis commented 4 years ago

Thanks for the feedback! All great notes. I'll work on that over the next week or so as I get time.

Didn't realise there could be breaking changes in a minor version release for this library though! I'll have to update my project's Gemfile to prevent breaking changes coming in 😬

timwis commented 4 years ago

Okay, I think I've incorporated all of your feedback. I dropped the separate TokenGenerator module in order to keep token generation and hashing functionality separate. I moved hashing to a static Passwordless.digest method as you suggested, though the only way I could see to do that was on the Passwordless module - let me know if there's a better way. I also added a couple tests for the configurable bits.

Beyond this, I think it's just a matter of updating the readme to reflect (1) how to upgrade/migrate, and (2) how to customise hashing.

Looking forward to your feedback.

xdmx commented 4 years ago

Any update on this? I'd love to start having hashed values in the db, instead of the real ones.

@timwis were you able to see @mikker's feedback?

timwis commented 4 years ago

Hi @xdmx, I haven't been able to integrate @mikker's feedback — unfortunately I've just started a new job that's had me quite full-on :/ I'd love to, but I'd certainly welcome the help if you or @mikker can pick it up! Only a few things left to do, I think.

mikker commented 4 years ago

I'm quite busy as well. Any help from anyone on bringing this home is very appreciated 😊

rickychilcott commented 2 years ago

It's been a while @timwis or @xdmx - interested in seeing this across the finish line?

mikker commented 2 years ago

AFAIR the work is already done in the big 1.0 PR. It was short sighted of me to have everything be in one PR. If anyone's willing to split it up into separate PRs it would be greatly appreciated.