passwordless
passwordless copied to clipboard
Store hashed tokens
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_testto 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.
Wonderful work so far ❤️
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.
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 😬
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.
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?
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.
I'm quite busy as well. Any help from anyone on bringing this home is very appreciated 😊
It's been a while @timwis or @xdmx - interested in seeing this across the finish line?
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.