Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

fix path issue preventing u2f support from working on mac #13345

Merged
merged 1 commit into from
Mar 19, 2018
Merged

Conversation

evq
Copy link
Member

@evq evq commented Feb 28, 2018

Fixes #13344

The cryptotoken extension (used for u2f support) is bundled into electron_resources.pak as part of the muon build. There is muon code which attempts to fetch extension resources from the pak if the path passed during extension load is a subdirectory of chrome::DIR_RESOURCES. Currently we use process.resourcesPath on the browser-laptop side to get the path to the chome::DIR_RESOURCES directory.

There's an issue with this on mac though, in that process.resourcesPath returns Brave.app/Contents/Resources while inside muon chome::DIR_RESOURCES actually refers to Brave.app/Contents/Frameworks/Brave Framework.framework/Resources.

This PR adds a helper function to retrieve the correct path for all platforms.

I've tested this PR on mac in dev mode as well as with a packaged build and also on linux to ensure u2f support is still working there.

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).
  • 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. (Ask a Brave employee to help if you cannot access this document.)

Test plan

  1. Go to https://demo.yubico.com/u2f?tab=register
  2. Make up a username and password and click next
  3. You should be prompted to plug in your u2f key
  4. You will be prompted to touch the sensor on your u2f key
  5. On success you can attempt login using the same username and password
  6. You will be prompted to again plug in / touch the sensor on your u2f key

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@evq evq self-assigned this Feb 28, 2018
@evq evq requested a review from bsclifton February 28, 2018 23:29
@codecov-io
Copy link

codecov-io commented Feb 28, 2018

Codecov Report

Merging #13345 into master will decrease coverage by 0.54%.
The diff coverage is 42.85%.

@@            Coverage Diff             @@
##           master   #13345      +/-   ##
==========================================
- Coverage   56.54%      56%   -0.55%     
==========================================
  Files         284      282       -2     
  Lines       28658    28218     -440     
  Branches     4740     4643      -97     
==========================================
- Hits        16206    15804     -402     
+ Misses      12452    12414      -38
Flag Coverage Δ
#unittest 56% <42.85%> (-0.55%) ⬇️
Impacted Files Coverage Δ
js/lib/appUrlUtil.js 84.66% <42.85%> (-1.88%) ⬇️
js/stores/appStore.js 16.57% <0%> (-11.42%) ⬇️
js/lib/request.js 17.64% <0%> (-11.15%) ⬇️
app/common/state/tabUIState.js 30.69% <0%> (-7.19%) ⬇️
...p/renderer/components/tabs/content/closeTabIcon.js 35.71% <0%> (-3.91%) ⬇️
...pp/renderer/components/tabs/content/privateIcon.js 38.09% <0%> (-1.25%) ⬇️
...renderer/components/tabs/content/newSessionIcon.js 36.36% <0%> (-1.14%) ⬇️
app/browser/menu.js 51.64% <0%> (-1.07%) ⬇️
js/stores/windowStore.js 27.66% <0%> (-0.98%) ⬇️
app/common/state/tabContentState/titleState.js 73.46% <0%> (-0.89%) ⬇️
... and 25 more

@NejcZdovc NejcZdovc added this to the 0.22.x (Developer Channel) milestone Mar 1, 2018
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Awesome job with this! Tested this change on both macOS and Windows... both worked great (nice test steps!)

@bsclifton bsclifton merged commit f4c2847 into master Mar 19, 2018
@bsclifton bsclifton deleted the fixMacU2F branch March 19, 2018 06:08
bsclifton added a commit that referenced this pull request Mar 19, 2018
fix path issue preventing u2f support from working on mac
bsclifton added a commit that referenced this pull request Mar 19, 2018
fix path issue preventing u2f support from working on mac
@bsclifton
Copy link
Member

master f4c2847
0.23.x 27f76a7
0.22.x b2f58eb

@evq evq mentioned this pull request Mar 19, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants