browserpass / browserpass-extension

Browserpass web extension
ISC License
825 stars 50 forks source link

fix `enableOTP` handling to match docs: prioritize store, then extension config #308

Closed uninsane closed 1 year ago

uninsane commented 1 year ago

the README.md says:

Browserpass allows configuring certain settings in different places places using the following priority, highest first:

  1. Options defined in specific *.gpg files, only apply to these password entries:
    • autoSubmit
  2. Options defined in .browserpass.json file located in the root of a password store:
    • autoSubmit
    • enableOTP ...
  3. Options defined in browser extension options:
    • Automatically submit forms after filling (aka autoSubmit)
    • Enable support for OTP tokens (aka enableOTP)

this makes it sound like one should be able to set enableOTP = true inside a store's config and leave it unset at the extension level, which sounds intuitive. but when i try that on master i'm not shown any token. unless i've missed something, the code only attempts to read this from the extension-level settings. this PR fixes that to match the behavior of the readme.

tested manually:

i'm a novice JS dev, so if i'm breaking idioms with my approach here don't hold back in setting me straight 😉

uninsane commented 1 year ago

fixed, thanks for linking to the code path! tested just with and without enableOTP in the store settings this time, assuming if that works it should all work.

uninsane commented 1 year ago

@maximbaz any movement on this? i'd like to move onto other browserpass work before life gets busy, but i'm not the type to maintain a dev branch that diverges from upstream in more than one direction at a time.

maximbaz commented 1 year ago

Sorry for the delay and that it blocks the further development, @erayd what do you think about this, would you have time to have a look here?

erayd commented 1 year ago

@maximbaz I missed this one sorry - I've taken a look, and I'm happy for it to be merged from a code standpoint. I haven't actually tested it - but if you are happy with the behavior, then feel free to merge 🙂.

maximbaz commented 1 year ago

With a reservation of me not being the typical user of OTP functionality in Browserpass, I think the behavior did make sense to me, and I'm happy that we fixed an inconsistency and gained the more intuitive behavior 🙂 Let's merge, and if you spot a regression later, we can always revisit 👍