-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix enableOTP
handling to match docs: prioritize store, then extension config
#308
Conversation
it will have use in `detailsInterface.js`, not just `background.js` (next patch).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for catching an inconsistency! 👍 You are on a right track, and I think the code change can be even slightly simpler, please see the comment below.
e97748d
to
a5f5e27
Compare
fixed, thanks for linking to the code path! tested just with and without |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! I'll let @erayd have a look as well, but looks good to me
@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. |
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? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks - much appreciated 👍
@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 🙂. |
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 👍 |
the README.md says:
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:
.browserpass.json
-> shows token"enableOTP": false
in store -> no token"enableOTP": true
in the store level -> shows tokenenableOTP
from the store config, setenableOTP: true
in a secret -> no tokeni'm a novice JS dev, so if i'm breaking idioms with my approach here don't hold back in setting me straight 😉