Re-enable webusb feature

[simonhong]

This api was disabled by #114

Fix brave/brave-browser#4669

Submitter Checklist:

• Submitted a ticket for my issue if one did not already exist.
• Used Github auto-closing keywords in the commit message.
• Added/updated tests for this change (for new code or code which already has tests).
• Verified that these changes build without errors on
  • Windows
  • macOS
  • Linux
• Verified that these changes pass automated tests (npm test brave_unit_tests && npm test brave_browser_tests && npm run test-security) on
  • Windows
  • macOS
  • Linux
• Verified that all lint errors/warnings are resolved (npm run lint)
• Ran git rebase master (if needed).
• Ran git rebase -i to squash commits (if needed).
• Tagged reviewers and labelled the pull request as needed.
• Request a security/privacy review as needed.
• Add appropriate QA labels (QA/Yes or QA/No) to include the closed issue in milestone
• Public documentation has been updated as necessary. For instance:
  • https://github.com/brave/brave-browser/wiki/Deviations-from-Chromium-(features-we-disable-or-remove)
  • https://github.com/brave/brave-browser/wiki/Proxy-redirected-URLs
  • https://github.com/brave/brave-browser/wiki/Fingerprinting-Protection-Mode
  • https://github.com/brave/brave-browser/wiki/Brave%E2%80%99s-Use-of-Referral-Codes
  • https://github.com/brave/brave-browser/wiki/Custom-Headers
  • https://github.com/brave/brave-browser/wiki/Web-compatibility-issues-with-tracking-protection
  • https://github.com/brave/brave-browser/wiki/QA-Guide

Test Plan:

Check navigator.usb doesn't return undefined in console.

Reviewer Checklist:

• New files have MPL-2.0 license header.
• Request a security/privacy review as needed.
• Adequate test coverage exists to prevent regressions
• Verify test plan is specified in PR before merging to source

After-merge Checklist:

• The associated issue milestone is set to the smallest version that the
  changes has landed on.
• All relevant documentation has been updated.

[keepkeyjon]

Thanks for considering adding WebUSB back! We (and our users) would love to have it enabled in Brave.

What was the rationale for removing it in the first place? From the thread in #13, it seems it was done out of privacy concerns. Does the need to pair devices in order for websites to talk to them mitigate that?

[diracdeltas]

@keepkeyjon it was disabled due to a security issue in brave/browser-laptop#13374.

[diracdeltas]

according to https://www.yubico.com/2018/06/webusb-and-responsible-disclosure/, the original security issue was fixed in chromium 67 so we should be ok to re-enable webusb now.

@simonhong please make sure all necessary wiki pages (linked in the PR template) that mention webusb are updated once this is merged. thanks!

[simonhong]

@diracdeltas two document are updated.

[bbondy]

Not your issue, but shouldn't we be calling the base class here?

void ChromeContentRendererClient::
    SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() {
  // The performance manager service interfaces are provided by the chrome
  // embedder only.
  blink::WebRuntimeFeatures::EnablePerformanceManagerInstrumentation(true);

// Web Share is shipped on Android, experimental otherwise. It is enabled here,
// in chrome/, to avoid it being made available in other clients of content/
// that do not have a Web Share Mojo implementation.
#if defined(OS_ANDROID)
  blink::WebRuntimeFeatures::EnableWebShare(true);
  blink::WebRuntimeFeatures::EnableWebShareV2(true);
#endif

  if (base::FeatureList::IsEnabled(subresource_filter::kAdTagging))
    blink::WebRuntimeFeatures::EnableAdTagging(true);
}

[simonhong]

@bbondy Yup, we should call base class' method also if we don't want to prevent all.
I assume that when @jumde pushed #114, base class methods would be like below.(Only it had android stuff)

void ChromeContentRendererClient::
    SetRuntimeFeaturesDefaultsBeforeBlinkInitialization() {
// Web Share is shipped on Android, experimental otherwise. It is enabled here,
// in chrome/, to avoid it being made available in other clients of content/
// that do not have a Web Share Mojo implementation.
#if defined(OS_ANDROID)
  blink::WebRuntimeFeatures::EnableWebShare(true);
#endif
}

So, @jumde might skip calling it.
Added.

[bbondy]

I think maybe it is safest to always call it and just undo anything we don't want to do. But probably there is nothing else we don't want to do. We'll have android support soon btw.

[pes10k]

Just to surface something that came up in #privacy today

Clearing site data does not currently clear suite permissions to access USB devices. Whether thats part of this issue, or its own issue, that seems like something important to work out before enabling WebUSB

[bbondy]

Just to surface something that came up in #privacy today

Clearing site data does not currently clear suite permissions to access USB devices. Whether thats part of this issue, or its own issue, that seems like something important to work out before enabling WebUSB

Please post a different issue for that @snyderp , I think we want to have better webcompat sooner than later.

[simonhong]

Only unrelated one step in CI was failed - Copy file to S3 - (68 ms in self)

[simonhong]

Passed in CI. Any more comments on this?

[kjozwiak]

Went through the following verifications:

• ensured that navigator.usb isn't returning undefined in DevTools
• connect to a Ledger Nano S using https://mycrypto.com/
• went through a Trezor setup using https://connect.trezor.io without any issues
• connected to Trezor using https://mycrypto.com/
• @mrose17 confirmed that he can connect to his KeepKey using 0.68.36 Chromium: 75.0.3770.87

Examples:

[kjozwiak]

Just to surface something that came up in #privacy today

Clearing site data does not currently clear suite permissions to access USB devices. Whether thats part of this issue, or its own issue, that seems like something important to work out before enabling WebUSB

@snyderp did you end up creating an issue for the above? Just double checking to make sure it doesn't end up falling through the cracks.

[pes10k]

@kjozwiak the issue was that I'm a dingbat and was confused by the "Clear data" dialog. I didn't realize the tabs on the "clear data" dialog control the behavior of the "clear" button at the bottom (instead of just showing more information about the same action).

TL;DR: i'm a dumb-dumb and no issue needed. I appreciate you double checking though :)

[kjozwiak]

@snyderp haha no worries! Appreciate the update 👍