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

Fix extension WebUSB permission issue #1079

Merged
merged 7 commits into from Nov 8, 2022
Merged

Conversation

buberdds
Copy link
Contributor

@buberdds buberdds commented Oct 12, 2022

Fixes #1045
During sync meeting we decided to go with a popup like in a current ext.
Screenshot 2022-10-12 at 18 27 51
Screenshot 2022-10-12 at 18 28 06
Screenshot 2022-10-12 at 18 28 56
Screenshot 2022-10-12 at 18 29 04
Screenshot 2022-10-12 at 18 29 13

error:
Screenshot 2022-11-08 at 13 14 15

@github-actions
Copy link

github-actions bot commented Oct 12, 2022

🦙 MegaLinter status: ✅ SUCCESS

Descriptor Linter Files Fixed Errors Elapsed time
✅ EDITORCONFIG editorconfig-checker 26 0 0.05s
✅ JAVASCRIPT eslint 1 0 0 5.11s
✅ JSON eslint-plugin-jsonc 2 0 0 1.86s
✅ JSON jsonlint 2 0 0.28s
✅ JSON prettier 2 0 0 0.77s
✅ JSON v8r 2 0 7.64s
✅ REPOSITORY checkov yes no 17.05s
✅ REPOSITORY git_diff yes no 0.07s
✅ TSX eslint 15 0 0 8.42s
✅ TYPESCRIPT eslint 4 0 0 7.45s

See errors details in artifact MegaLinter reports on CI Job page
Set VALIDATE_ALL_CODEBASE: true in mega-linter.yml to validate all sources, not only the diff

MegaLinter is graciously provided by OX Security

@buberdds buberdds requested a review from lukaw3d October 12, 2022 16:31
@codecov
Copy link

codecov bot commented Oct 12, 2022

Codecov Report

Merging #1079 (e07f288) into master (ab36d1e) will increase coverage by 0.17%.
The diff coverage is 92.18%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1079      +/-   ##
==========================================
+ Coverage   84.88%   85.05%   +0.17%     
==========================================
  Files         121      126       +5     
  Lines        2249     2309      +60     
  Branches      539      559      +20     
==========================================
+ Hits         1909     1964      +55     
- Misses        340      345       +5     
Flag Coverage Δ
cypress 52.05% <80.76%> (+0.24%) ⬆️
jest 78.47% <82.53%> (+0.38%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/app/index.tsx 100.00% <ø> (ø)
src/app/pages/AccountPage/index.tsx 95.16% <ø> (ø)
src/app/pages/OpenWalletPage/webextension.tsx 0.00% <0.00%> (ø)
src/utils/webextension.ts 77.77% <77.77%> (ø)
src/app/components/Header/index.tsx 100.00% <100.00%> (ø)
src/app/lib/ledger.ts 100.00% <100.00%> (ø)
src/app/pages/ConnectDevicePage/index.tsx 100.00% <100.00%> (ø)
src/app/pages/OpenWalletPage/index.tsx 100.00% <100.00%> (ø)
src/commonRoutes.tsx 100.00% <100.00%> (ø)
src/index.tsx 100.00% <100.00%> (ø)
... and 1 more

src/commonRoutes.tsx Outdated Show resolved Hide resolved
src/utils/webextension.ts Outdated Show resolved Hide resolved
src/index.tsx Show resolved Hide resolved
src/app/pages/OpenWalletPage/webextension.tsx Outdated Show resolved Hide resolved
@buberdds buberdds force-pushed the buberdds/extLedgerAccess branch 7 times, most recently from b3369fc to 0430931 Compare October 25, 2022 11:45
src/utils/webextension.ts Outdated Show resolved Hide resolved
src/app/lib/ledger.ts Outdated Show resolved Hide resolved
src/app/pages/ConnectDevicePage/index.tsx Outdated Show resolved Hide resolved
</Box>
)}
{connection === 'connected' && (
<ConnectionStatusIcon label={t('ledger.extension.succeed', 'Device connected')} />
Copy link
Member

Choose a reason for hiding this comment

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

Extra popup should probably automatically close after this

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if you changed this in an odd way: popup1 closes after returning to it, after success
popup1-disappears

Copy link
Contributor Author

Choose a reason for hiding this comment

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

checked both dev and prod builds and I cannot repro this

Comment on lines 47 to 53
{webExtensionLedgerAccess ? (
<Button
style={{ width: 'fit-content' }}
onClick={webExtensionLedgerAccess}
label={t('ledger.extension.grantAccess', 'Grant access to your Ledger')}
primary
/>
Copy link
Member

Choose a reason for hiding this comment

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

This flow should only be needed the first time for each unpaired ledger device, not every time. Background page already has the needed permissions.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The only problem is that permission can be revoked at any time, right after it was granted, and we have no knowledge about it.

Copy link
Member

Choose a reason for hiding this comment

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

How about on click: attempt to get a device and if it fails openLedgerAccessPopup

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you share more details? Do you want to dispatch saga action first and handle "LedgerNoDeviceSelected" differently or there is some way to get a device and checked if it's paired?

Copy link
Member

Choose a reason for hiding this comment

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

I think just try { await TransportWebUSB.create(); to="ledger" } catch (e) { openLedgerAccessPopup() }

Copy link
Contributor Author

@buberdds buberdds Nov 2, 2022

Choose a reason for hiding this comment

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

If we want to create usb transport here we will have to close it in React component as well to avoid USB Transport error: Failed to execute 'claimInterface' on 'USBDevice': Unable to claim interface. At this point everything looks fine, but when I will revoke usb access and go through process again it will end up with USB Transport error: Failed to execute 'open' on 'USBDevice': The device was disconnected..

Copy link
Member

Choose a reason for hiding this comment

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

Alright I am able to reproduce this

  try {
    await (await TransportWebUSB.create()).close()
    navigate('/open-wallet/ledger')
  } catch (e) {
    webExtensionLedgerAccess()
  }

what's very odd is that it works after reloading background page, as if close() doesn't fully reset device communication

  • start with extension having USB permission
  • enumerate accounts works, cancel
  • remove USB permission
  • connect ledger flow, enumerate accounts does not work, cancel
  • reload background page
  • enumerate accounts works, cancel
  • remove USB permission
  • reload background page
  • connect ledger flow, enumerate accounts works, cancel

@buberdds buberdds force-pushed the buberdds/extLedgerAccess branch 3 times, most recently from 9dbc6fa to e79968f Compare November 2, 2022 10:00
extension/src/popup/routes.tsx Outdated Show resolved Hide resolved
src/app/lib/ledger.ts Outdated Show resolved Hide resolved
src/app/lib/ledger.test.ts Outdated Show resolved Hide resolved
src/app/pages/ConnectDevicePage/__tests__/index.test.tsx Outdated Show resolved Hide resolved
src/utils/webextension.ts Show resolved Hide resolved
src/utils/webextension.ts Outdated Show resolved Hide resolved
src/app/pages/OpenWalletPage/webextension.tsx Show resolved Hide resolved
src/app/pages/OpenWalletPage/__tests__/index.test.tsx Outdated Show resolved Hide resolved
src/app/pages/OpenWalletPage/__tests__/index.test.tsx Outdated Show resolved Hide resolved
@buberdds buberdds force-pushed the buberdds/extLedgerAccess branch 2 times, most recently from cb8e38e to 5b56312 Compare November 7, 2022 14:34
@buberdds buberdds requested a review from lukaw3d November 7, 2022 15:57
@buberdds buberdds force-pushed the buberdds/extLedgerAccess branch 2 times, most recently from 4e3c61f to 68eb66e Compare November 8, 2022 14:31
@buberdds buberdds merged commit 401a13b into master Nov 8, 2022
@buberdds buberdds deleted the buberdds/extLedgerAccess branch November 8, 2022 18:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extension throws LedgerNoDeviceSelected
2 participants