Skip to content
This repository was archived by the owner on Nov 1, 2022. It is now read-only.

Closes #3826: FxA WebChannel WebExtension #3881

Closed
wants to merge 2 commits into from

Conversation

vladikoff
Copy link
Contributor

@vladikoff vladikoff commented Jul 23, 2019

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

After merge

  • Milestone: Make sure issues closed by this pull request are added to the milestone of the version currently in development.

Requires mozilla/fxa#2043 to be deployed.

Testing

We need to open in the browser https://oauthchannel.dev.lcip.org/oauth/?action=email&response_type=code&context=oauth_webchannel_v1&client_id=a2270f727f45f648&scope=profile%2Bhttps%3A%2F%2Fidentity.mozilla.com%2Fapps%2Foldsync&state=vHao1p6OizzwReCkQMSpZA&code_challenge_method=S256&code_challenge=W9pbQWxeSWGcwHJUyJjrPQbk3ceMwP3rpCMLGkYlZfw&access_type=offline&keys_jwk=eyJjcnYiOiJQLTI1NiIsImt0eSI6IkVDIiwieCI6IktVenAyZGZSM2wwTVVIblBWMldtbzdmM0NBQlZ4cmMzTFVvb1lfeGFKNjQiLCJ5IjoiR195b0lKdk9QckhUY1lkUjlCVDBWYjdFc01nMXZHVUpEWk8wSVB2VkwyTSJ9 to receive the save oauth_login message.

OR use the https://oauthchannel.dev.lcip.org env plus the r-b client or a2270f727f45f648 and add context=oauth_webchannel_v1 to the URL that is opened

}

private fun recieveLogin(messageObj: JSONObject) {
logger.info(messageObj.toString())
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 result of this here is something like

{"messageId":null,"data":{"verified":false,"keyFetchToken":"7f6ef1a55cc2e629c5b587c5c1fe2a0e8e457ce594e767072e5fbe356b47bba5","sessionToken":"02c354774aa81accae09ada17b21816436dc5461250a60800d078d843c5155e2","unwrapBKey":"cbfabce2b955c06b8057f66b23073efd7fcf3790db5c2967e2291ed1740aafa8","uid":"e82283bfcc4d4e65860e4dc6ea82d378","email":"v@v.com","customizeSync":false,"verifiedCanLinkAccount":true},"command":"fxaccounts:login"}

Copy link
Contributor Author

@vladikoff vladikoff Aug 1, 2019

Choose a reason for hiding this comment

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

@Amejia481 @grigoryk new event is now called oauth_login and has this right now:

{"messageId":null,"data":{"action":"signup","redirect":"http://127.0.0.1:3030/oauth/success/a2270f727f45f648?code=0fdbc2e14ffa3b1d933c3d802aa4753eb6d781c38c0bd67981b6a69fa0623619&state=vHao1p6OizzwReCkQMSpZA","declinedSyncEngines":["history"],"code":"0fdbc2e14ffa3b1d933c3d802aa4753eb6d781c38c0bd67981b6a69fa0623619","offeredSyncEngines":["bookmarks","history","passwords","addons","tabs","prefs"],"state":"vHao1p6OizzwReCkQMSpZA","customizeSync":true},"command":"fxaccounts:oauth_login"}

.buildconfig.yml Outdated
@@ -112,6 +112,10 @@ projects:
path: components/feature/qr
description: 'A feature that provides functionality for scanning QR codes.'
publish: true
feature-webchannel:
path: components/feature/webchannel
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rename this to fxa-webchannel

"version": "1.0",
"content_scripts": [
{
"matches": ["*://*/*"],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

lock this to FxA domains?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds like a good idea, although that might make it harder to use custom FxA environments (stage, China, self-hosted).

Semi-related, I feel like there should also be some sort of origin check logic in here somewhere, where we verify that any messages we receive are in fact coming from the expected FxA content-server domain. Or does the webextension messaging-port magic take care of that for us?

sendContentMessage(status)
}

private fun recieveLogin(messageObj: JSONObject) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grigoryk @Amejia481 this is where give stuff to FxA.kt

Copy link
Contributor

Choose a reason for hiding this comment

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

Cool, here we are going to get notified on different states like:

  • accountSignedIn
  • accountSignedUp
  • passwordChange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grigoryk could you add accountManager.finishAuthenticationAsync(code, state) here? the data is in the messageObj?


private fun sendLinkResponse(messageId: String) {
val statusData = JSONObject()
statusData.put("ok", true)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

here we can also ask FxA.kt for stuff

@@ -59,6 +61,16 @@ class BrowserFragment : BaseBrowserFragment(), BackHandler {
view = layout
)

webChannelFeature.set(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@grigoryk should we work on getting an instance of FxA or FxA Manager passed in here?

@codecov
Copy link

codecov bot commented Aug 8, 2019

Codecov Report

Merging #3881 into master will decrease coverage by 0.1%.
The diff coverage is 54.54%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #3881      +/-   ##
============================================
- Coverage     80.66%   80.56%   -0.11%     
+ Complexity     3882     3706     -176     
============================================
  Files           507      479      -28     
  Lines         17169    16687     -482     
  Branches       2554     2456      -98     
============================================
- Hits          13850    13444     -406     
+ Misses         2268     2224      -44     
+ Partials       1051     1019      -32
Impacted Files Coverage Δ Complexity Δ
...a/components/feature/accounts/WebChannelFeature.kt 54.54% <54.54%> (ø) 8 <8> (?)
...glean/storages/TimingDistributionsStorageEngine.kt 90.82% <0%> (-5.54%) 1% <0%> (ø)
...glean/storages/CustomDistributionsStorageEngine.kt 90.06% <0%> (-5.4%) 1% <0%> (ø)
...zilla/components/feature/media/focus/AudioFocus.kt 88.37% <0%> (-3.3%) 12% <0%> (-1%)
...components/feature/awesomebar/AwesomeBarFeature.kt 81.81% <0%> (-2.4%) 8% <0%> (-2%)
...a/components/browser/search/SearchEngineManager.kt 89.74% <0%> (-0.65%) 18% <0%> (-6%)
...service/glean/storages/StringListsStorageEngine.kt 95.55% <0%> (-0.6%) 1% <0%> (ø)
...ts/feature/media/notification/MediaNotification.kt 95% <0%> (-0.39%) 2% <0%> (ø)
...a/components/feature/media/service/MediaService.kt 89.36% <0%> (-0.23%) 10% <0%> (ø)
...mponents/browser/toolbar/display/DisplayToolbar.kt 95.41% <0%> (-0.06%) 57% <0%> (-1%)
... and 54 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4774272...9d2d596. Read the comment docs.

@grigoryk
Copy link
Contributor

grigoryk commented Aug 9, 2019

@vladikoff what do you think about moving this into feature-accounts? It already houses the interceptor logic and facilitates logging in, so it makes sense for the webchannel stuff to live in the same feature.

@vladikoff
Copy link
Contributor Author

@vladikoff what do you think about moving this into feature-accounts? It already houses the interceptor logic and facilitates logging in, so it makes sense for the webchannel stuff to live in the same feature.

I would be okay with this 👍

@grigoryk
Copy link
Contributor

This will land in #4221.

@grigoryk grigoryk closed this Aug 27, 2019
vladikoff added a commit to mozilla/fxa that referenced this pull request Sep 4, 2019
vladikoff added a commit to mozilla/fxa that referenced this pull request Sep 4, 2019
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