Skip to content
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

Honor store-specific enableOTP setting #261

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

shinenelson
Copy link

The Options allow for the enableOTP feature to be specified within
the .browserpass.json, but the feature is not enabled when the
option is specified within .browserpass.json and not globally.

This was because the check for the setting was only looking for the
global setting - settings.enableOTP and did not check for the
.browserpass.json setting ( settings.stores[*].settings.enableOTP ).

This changeset fixes that issue and properly honors the enableOTP
setting specified within .browserpass.json with or without the
global enableOTP setting.

Copy link
Member

@maximbaz maximbaz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, looks good to me, @erayd what do you think?

Copy link
Collaborator

@erayd erayd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch, thanks 🙂.

src/background.js Outdated Show resolved Hide resolved
src/background.js Outdated Show resolved Hide resolved
src/popup/detailsInterface.js Outdated Show resolved Hide resolved
also, catch `enableOTP` in `fill` action to copy OTP token after filling login
information

reference : browserpass#261 (review)
@shinenelson shinenelson requested a review from erayd April 10, 2021 09:23
@shinenelson
Copy link
Author

Ping @erayd if the review request was missed

@erayd
Copy link
Collaborator

erayd commented Apr 24, 2021

Ping @erayd if the review request was missed

Not missed, just short on time. I'm expecting to have some time to spend working on Browserpass during the coming week, which will include this PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants