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

FxA webchannel support and account manager integration #4221

Merged
merged 3 commits into from
Sep 4, 2019

Conversation

grigoryk
Copy link
Contributor

@grigoryk grigoryk commented Aug 27, 2019

Part 1:

Preparatory work for allowing users to choose what to sync. We are about to
tie SyncConfig with FxA's "capabilities" webchannel API, which takes a set of
"engines" as strings. To simplify our integration work, this patch replaces an
untyped set of "stores" with a typed set of "engines" at the API level.

Part 2:

(should have been split up further for easier reviewing, but things got a bit tangled)
This patch does a few things:

  • adds a webextension for communicating with fxa web content
  • provides a backend for said webextension, which understands webchannel communication protocol
    and knows how to relay messages to the account manager, and query its state
  • expands account manager a bit to support the webchannel authentication protocol
  • adds a (rather detailed!) AuthType parameter to AccountObserver:onAuthenticated, instead of the very vague 'newAccount' flag

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.
  • Breaking Changes: If this is a breaking change, please push a draft PR on Reference Browser to address the breaking issues.

@grigoryk grigoryk force-pushed the webchannel-support branch 5 times, most recently from 9d13484 to 140b251 Compare August 27, 2019 20:49
@codecov
Copy link

codecov bot commented Aug 29, 2019

Codecov Report

Merging #4221 into master will increase coverage by 1.28%.
The diff coverage is 91.78%.

Impacted file tree graph

@@             Coverage Diff              @@
##             master    #4221      +/-   ##
============================================
+ Coverage     81.14%   82.42%   +1.28%     
+ Complexity     3981     3835     -146     
============================================
  Files           514      495      -19     
  Lines         17399    16328    -1071     
  Branches       2577     2412     -165     
============================================
- Hits          14118    13459     -659     
+ Misses         2215     1854     -361     
+ Partials       1066     1015      -51
Impacted Files Coverage Δ Complexity Δ
...a/components/feature/accounts/WebChannelFeature.kt 91.78% <91.78%> (ø) 7 <7> (?)
...va/mozilla/components/browser/icons/IconRequest.kt 100% <0%> (ø) 5% <0%> (-1%) ⬇️
...ozilla/components/service/fxa/SyncAuthInfoCache.kt
...a/mozilla/components/service/fxa/AccountStorage.kt
...a/mozilla/components/browser/storage/sync/Types.kt
...telemetry/measurement/ExperimentsMapMeasurement.kt
...a/components/service/fxa/FxaDeviceConstellation.kt
...nents/browser/storage/sync/PlacesHistoryStorage.kt
...mozilla/components/service/fxa/sync/StorageSync.kt
...a/components/browser/storage/sync/PlacesStorage.kt
... and 6 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 e1ea6e3...01ff631. Read the comment docs.

@grigoryk grigoryk force-pushed the webchannel-support branch 3 times, most recently from c132a96 to 34ac69e Compare August 30, 2019 02:08
@grigoryk grigoryk requested a review from csadilek August 30, 2019 02:10
@grigoryk grigoryk marked this pull request as ready for review August 30, 2019 02:15
@grigoryk grigoryk requested a review from a team as a code owner August 30, 2019 02:15
@grigoryk
Copy link
Contributor Author

Fenix side of this: mozilla-mobile/fenix#4931 (note that fxa functionality is currently running on a dev box 2).

Copy link
Contributor

@csadilek csadilek left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for adding so many tests and comments. I've had a few comments and two small things to change (just naming), but otherwise I think this is ready.

* @param engine a reference to the application's browser engine.
*/
fun install(engine: Engine) {
engine.installWebExtension(WEB_CHANNEL_EXTENSION_ID, WEB_CHANNEL_EXTENSION_URL,
Copy link
Contributor

Choose a reason for hiding this comment

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

This had to bring in a lot of code from the reader view feature. I've filed #4297 to address the code duplication and come up with an abstraction.

@grigoryk
Copy link
Contributor Author

grigoryk commented Sep 3, 2019

@csadilek addressed your feedback.

@grigoryk
Copy link
Contributor Author

grigoryk commented Sep 4, 2019

reference-browser PR: mozilla-mobile/reference-browser#895

Copy link
Contributor

@csadilek csadilek 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!

We've also just started upgrading Fenix to 12.0.0, so we need to update the PR there as well and orchestrate the merge.

Grisha Kruglov and others added 2 commits September 4, 2019 11:41
Preparatory work for allowing users to choose what to sync. We are about to
tie SyncConfig with FxA's "capabilities" webchannel API, which takes a set of
"engines" as strings. To simplify our integration work, this patch replaces an
untyped set of "stores" with a typed set of "engines" at the API level.
This patch does a few things:
- adds a webextension for communicating with fxa web content
- provides a backend for said webextension, which understands webchannel communication protocol
and knows how to relay messages to the account manager, and query its state
- expands account manager a bit to support the webchannel authentication protocol
- adds a (rather detailed!) AuthType parameter to AccountObserver:onAuthenticated, instead of the very vague 'newAccount' flag

Co-authored-by: Arturo Mejia <arturomejiamarmol@gmail.com>
Co-authored-by: vladikoff <vlad.filippov@gmail.com>
@grigoryk
Copy link
Contributor Author

grigoryk commented Sep 4, 2019

bors r=csadilek

bors bot pushed a commit that referenced this pull request Sep 4, 2019
4221: FxA webchannel support and account manager integration r=csadilek a=grigoryk

## Part 1:
Preparatory work for allowing users to choose what to sync. We are about to
tie SyncConfig with FxA's "capabilities" webchannel API, which takes a set of
"engines" as strings. To simplify our integration work, this patch replaces an
untyped set of "stores" with a typed set of "engines" at the API level.

## Part 2:
(should have been split up further for easier reviewing, but things got a bit tangled)
This patch does a few things:
- adds a webextension for communicating with fxa web content
- provides a backend for said webextension, which understands webchannel communication protocol
and knows how to relay messages to the account manager, and query its state
- expands account manager a bit to support the webchannel authentication protocol
- adds a (rather detailed!) AuthType parameter to AccountObserver:onAuthenticated, instead of the very vague 'newAccount' flag
  - Closes #4238 



Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
"version": "1.0",
"content_scripts": [
{
"matches": ["https://acounts.firefox.com/*"],
Copy link
Contributor

Choose a reason for hiding this comment

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

this is missing lcip domains, we should probably include them still

* loaded ------> | fxa web content loaded
* fxa-status ------> | web content requests account status & device capabilities
* | <------ fxa-status-response this class responds, based on state of [accountManager]
* can-link-account ------> | user submitted credentials, web content verifying if account linking is allowed
Copy link
Contributor

Choose a reason for hiding this comment

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

can link account should ago away?

* @property accountManager a reference to application's [FxaAccountManager].
*/
@Suppress("TooManyFunctions")
class FxaWebChannelFeature(
Copy link
Contributor

Choose a reason for hiding this comment

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

the other class is called Firefox Accounts Auth Feature , this one just says Fxa...., does that matter?

@bors
Copy link

bors bot commented Sep 4, 2019

Build succeeded

@bors bors bot merged commit f186993 into master Sep 4, 2019
@bors bors bot deleted the webchannel-support branch September 4, 2019 19:06
bors bot pushed a commit that referenced this pull request Sep 11, 2019
4375: Proper 'matches' permission for webchannel extension manifest r=vladikoff a=grigoryk

FxA URL was mistyped in #4221 :(

### After merge
- [ ] **Milestone**: Make sure issues closed by this pull request are added to the [milestone](https://github.com/mozilla-mobile/android-components/milestones) of the version currently in development.
- [ ] **Breaking Changes**: If this is a breaking change, please push a draft PR on [Reference Browser](https://github.com/mozilla-mobile/reference-browser) to address the breaking issues.


Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
bors bot pushed a commit that referenced this pull request Sep 11, 2019
4375: Proper 'matches' permission for webchannel extension manifest r=jonalmeida a=grigoryk

FxA URL was mistyped in #4221 :(

### After merge
- [ ] **Milestone**: Make sure issues closed by this pull request are added to the [milestone](https://github.com/mozilla-mobile/android-components/milestones) of the version currently in development.
- [ ] **Breaking Changes**: If this is a breaking change, please push a draft PR on [Reference Browser](https://github.com/mozilla-mobile/reference-browser) to address the breaking issues.


Co-authored-by: Grisha Kruglov <gkruglov@mozilla.com>
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.

AccountObserver: differentiate between sign-in and sign-up
5 participants