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

[PM-3343] Capture TOTP QR codes from websites in the browser extension #5985

Merged
merged 17 commits into from
Jan 3, 2024

Conversation

quexten
Copy link
Contributor

@quexten quexten commented Aug 8, 2023

Type of change

- [ ] Bug fix
- [x] New feature development
- [ ] Tech debt (refactoring, code cleanup, dependency upgrades, etc)
- [ ] Build/deploy pipeline (DevOps)
- [ ] Other

Objective

Some sites don't provide the TOTP secret directly, but only a QR code. Additionally, copying and pasting is an additional step for the user. Finally, copying the TOTP secret temporarily exposes it to the clipboard, which might be snooped on by other applications / browser extensions.

This PR adds a button next to the TOTP secret field, which when pressed captures a screenshot of the current webpage, extracts the TOTP QR code and reads it.

Closes https://community.bitwarden.com/t/totp-screenshot-feature/7043/2

Code changes

  • messages.json: Add the i18n messages for English
  • manifest.json / manifestv3: Add <all_urls> permission for chrome.tabs.captureVisibleTab to work (on chrome, just giving "activeTab" works, but firefox requires <all_urls>)
  • browser-api.ts: Add a function to capture the visible tab
  • add-edit.component.ts: Add the code to capture the tab, decode the QR code and - if parsed correctly - set the cipher's TOTP secret entry
  • add-edit.component.html: Add the button to capture the TOTP code from the webpage
  • package.json: Add the qrcode-parser library for QR code parsing

Screenshots

totp-capture.webm

Before you submit

  • Please add unit tests where it makes sense to do so (encouraged but not required)
  • If this change requires a documentation update - notify the documentation team
  • If this change has particular deployment requirements - notify the DevOps team
  • Ensure that all UI additions follow WCAG AA requirements

@quexten quexten requested review from a team as code owners August 8, 2023 01:14
@bitwarden-bot
Copy link

Thank you for your contribution! We've added this to our internal Community PR board for review.
ID: PM-3343

@bitwarden-bot bitwarden-bot changed the title Feature/browser totp capture [PM-3343] Feature/browser totp capture Aug 8, 2023
@quexten quexten changed the title [PM-3343] Feature/browser totp capture [PM-3343] Capture TOTP QR codes from websites in the browser extension Aug 8, 2023
@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

Right now, the PR is parsing the QR code as a URI such as:
otpauth://totp/NodeJSTest:testuser?secret=GZ4FORKTNBVFGQTFJJGEIRDOKY&issuer=NodeJSTest
and extracting just the secret GZ4FORKTNBVFGQTFJJGEIRDOKY, as on browser the default approach is copy pasting just the secret anyways. We could also store the entire URI in the TOTP secret field (this is what happens on mobile). I'm happy to change it if required, but don't want to sway this choice either way as I'm OK with either.

@fabriziobagala
Copy link

fabriziobagala commented Aug 8, 2023

Right now, the PR is parsing the QR code as a URI such as: otpauth://totp/NodeJSTest:testuser?secret=GZ4FORKTNBVFGQTFJJGEIRDOKY&issuer=NodeJSTest and extracting just the secret GZ4FORKTNBVFGQTFJJGEIRDOKY, as on browser the default approach is copy pasting just the secret anyways. We could also store the entire URI in the TOTP secret field (this is what happens on mobile). I'm happy to change it if required, but don't want to sway this choice either way as I'm OK with either.

Thank you very much for this interesting and desired feature!

For consistency with the mobile app, the best choice would be to store the entire URI. Moreover, in analyzing other password managers, I have noticed that the entire URI is always saved.

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

As a note for QA: I tested manifest v2 developer builds of Firefox/Chrome. I don't have a testing setup for Safari at the moment. It could be that we need a separate way to capture the webpage screenshot on Safari, reference https://stackoverflow.com/questions/3329117/what-does-visiblecontentsasdataurl-exactly-do

@sukhleenb
Copy link

Hello! The designers are requesting a few UI/UX tweaks to be consistent with mobile.

Could we look into

  • updating the toast copy to "Authenticator key added" when the QR code successfully scans
  • updating the toast copy to "Item saved" when the user selects Save
  • on hover, add/update the camera icon tooltip to say “scan QR code”
  • once the authenticator key is added, add the copy icon next to the camera icon
  • double-check that when a user has "can-view" permissions, that the camera icon and the copy icon are in a disabled state

Once these changes are implemented, this will be a great win for the extension. Appreciate your work on this.

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

on hover, add/update the camera icon tooltip to say “scan QR code”

Thanks for the feedback. Minor nit-pick, "scan QR code" could mean that the camera is used (in case of a laptop, or a desktop with a webcam is used). How about "Scan QR code from webpage"?

Also:

updating the toast copy to "Item saved" when the user selects Save

When saving, it does pop the notification (visible in the video). Does the point refer to replacing the active toast instead of pushing a new one?

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

image

Added the copy button; changed the messages to be:

  • "Unable to scan QR code from the current webpage"
  • "Authenticator key added"
  • "Scan authenticator QR code from current webpage"
  • "Copy TOTP secret"

double-check that when a user has "can-view" permissions, that the camera icon and the copy icon are in a disabled state

Question about this; I can see why with "can-view" permissions, the capture would be disabled as the user should not modify the entry. But as the code is visible to the user, they could just mark & copy it, or manually copy it by copying it letter by letter. So I'm not sure what the purpose is for disabling the copy button?

@sukhleenb
Copy link

  • "Scan QR code from webpage" works great for the tooltip
  • For the "Item saved" toast, looks like the copy is correct. I must have misread it when viewing the video.
  • For "can-view", the main purpose is to prevent edits to the item. We are looking into disabling all quick-action buttons for this permission. If you'd like to be consistent with the existing UI, we can disable capture for now and look at disabling the copy icon with the other initiative.
  • Is the "Copy TOTP secret" the tooltip message for the copy icon? If so, can you please rename it to "Copy verification code" as seen below
Screenshot 2023-08-08 at 2 21 49 PM

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

Is the "Copy TOTP secret" the tooltip message for the copy icon? If so, can you please rename it to "Copy verification code" as seen below

When editing the entry, it shows the TOTP secret (otpauth://totp/NodeJSTest:testuser?secret=GZ4FORKTNBVFGQTFJJGEIRDOKY&issuer=NodeJSTest)
When regularly viewing the entry, it shows the generated TOTP codes (143234,...)

The copy button is left as "Copy TOTP code" in view mode (when actually copying TOTP codes).
In edit mode, since it shows the TOTP secret, and upon click it copies the secret, the copy button says "Copy TOTP secret" (since it is a different action from copying the generated TOTP code)

For "can-view", the main purpose is to prevent edits to the item. We are looking into disabling all quick-action buttons for this permission. If you'd like to be consistent with the existing UI, we can disable capture for now and look at disabling the copy icon with the other initiative.

That's fine. For now I completely hid the actions (both copy the secret and capture the qr code) when in view only mode (this is the same behaviour as is currently applied for the "generate username" / "generate password buttons").

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

Actually, thinking about it, since the text field is labeled "Authenticator key (TOTP)", it might make sense to rename the copy message to "Copy Authenticator key" or "Copy Authenticator key (TOTP)".

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

image

Like this?

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

In view only mode:

image

@sukhleenb
Copy link

I realized I misspoke. With "can view", you can currently copy all fields, regardless of if you are on the edit or view page. We should hide the copy icon for the "Can-view, except password" permission in the edit page, along with hiding the field value. We would keep the copy icon for the user to copy the verification code in the view page. Screenshots of existing UI with "Can-view, except password" permissions.

Screenshot 2023-08-08 at 3 31 55 PM Screenshot 2023-08-08 at 3 31 49 PM

We'll still want to disable to capture icon for both permissions.

@quexten
Copy link
Contributor Author

quexten commented Aug 8, 2023

We should hide the copy icon for the "Can-view, except password" permission in the edit page, along with hiding the field value

image

That behaviour is unchanged in this PR :)

Should the "Copy Authenticator secret (TOTP)" button be enabled when the permission is "Can-view"? (Right now both actions are disabled for both permissions).

@sukhleenb
Copy link

Should the "Copy Authenticator secret (TOTP)" button be enabled when the permission is "Can-view"? (Right now both actions are disabled for both permissions).

Yup, exactly! Other than that, I think this should be good to go :)

MGibson1
MGibson1 previously approved these changes Dec 19, 2023
@@ -642,4 +643,28 @@ export class AddEditComponent implements OnInit, OnDestroy {

return loadedSavedInfo;
}

async copy(value: string, typeI18nKey: string, aType: string): Promise<boolean> {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the contribution. Everything looks great, just one more requested change from our end. the copy method here has conflicts with the copy inside web apps/web/src/app/vault/individual-vault/add-edit.component.ts

@Jingo88 Jingo88 enabled auto-merge (squash) December 22, 2023 15:19
Copy link
Contributor

@andrebispo5 andrebispo5 left a comment

Choose a reason for hiding this comment

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

Looks good!

@Jingo88 Jingo88 enabled auto-merge (squash) December 26, 2023 14:18
@Jingo88 Jingo88 merged commit 1b4717a into bitwarden:main Jan 3, 2024
39 of 52 checks passed
@Jingo88
Copy link
Contributor

Jingo88 commented Jan 3, 2024

Thank you for your contribution!

MGibson1 added a commit that referenced this pull request Apr 19, 2024
Discussions on this permission here: #5985
MGibson1 added a commit that referenced this pull request Apr 19, 2024
Discussions on this permission here: #5985
MGibson1 added a commit that referenced this pull request Apr 22, 2024
Discussions on this permission here: #5985
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants