Skip to content

Honor store-specific enableOTP setting#261

Open
shinenelson wants to merge 3 commits into
browserpass:masterfrom
shinenelson:honor-store-specific-enable-otp
Open

Honor store-specific enableOTP setting#261
shinenelson wants to merge 3 commits into
browserpass:masterfrom
shinenelson:honor-store-specific-enable-otp

Conversation

@shinenelson

Copy link
Copy Markdown

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.

@max-baz max-baz left a comment

Copy link
Copy Markdown
Member

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?

@erayd erayd left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice catch, thanks 🙂.

Comment thread src/background.js Outdated
Comment thread src/background.js Outdated
Comment thread src/popup/detailsInterface.js Outdated
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
Copy Markdown
Author

Ping @erayd if the review request was missed

@erayd

erayd commented Apr 24, 2021

Copy link
Copy Markdown
Collaborator

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