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

Bumps Application Services to v62.0.0 #7694

Closed
wants to merge 1 commit into from

Conversation

tarikeshaq
Copy link
Contributor

@tarikeshaq tarikeshaq commented Jul 13, 2020

Connects to mozilla/application-services#3337
Bumps Application Services to 62.0.0

v62.0.0 (2020-07-13)

Full Changelog

FxA Client

⚠️ Breaking changes ⚠️

  • Adds support for entrypoint in oauth flow APIs: consumers of beginOAuthFlow and beginPairingFlow (beginAuthentication and beginPairingAuthentication in ios) are now required to pass an entrypoint argument that would be used for metrics. This puts the beginOAuthFlow and beginPairingFlow APIs inline with other existing APIs, like getManageAccountUrl. (#3265)
  • Changes the authorizeOAuthCode API to now accept an AuthorizationParams object instead of the individual parameters. The AuthorizationParams also includes optional AuthorizationPKCEParams that contain the codeChallenge, codeChallengeMethod. AuthorizationParams also includes an optional keysJwk for requesting keys (#3264)

What's new

  • Consumers can now optionally include parameters for metrics in beginOAuthFlow and beginPairingFlow (beginAuthentication and beginPairingAuthentication in ios). Those parameters can be passed in using a MetricsParams struct/class. MetricsParams is defined in both the Kotlin and Swift bindings. The parameters are the following (#3328):
    • flow_id
    • flow_begin_time
    • device_id
    • utm_source
    • utm_content
    • utm_medium
    • utm_term
    • utm_campaign
    • entrypoint_experiment
    • entrypoint_variation

Logins

What's fixed

  • Fixed a bug where attempting to edit a login with an empty form_submit_url would incorrectly
    reject the entry as invalid (#3331).

Tabs

What's new

  • Tab records now have an explicit TTL set when storing on the server, to match the behaviour
    of Firefox Desktop clients (#3322).

val state = Uri.parse(url).getQueryParameter("state")!!
AuthFlowUrl(state, url)
}
}

override fun beginPairingFlowAsync(pairingUrl: String, scopes: Set<String>) = scope.async {
handleFxaExceptions(logger, "begin oauth pairing flow", { null }) {
val url = inner.beginPairingFlow(pairingUrl, scopes.toTypedArray())
val url = inner.beginPairingFlow(pairingUrl, scopes.toTypedArray(), "notset")
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will create an issue for this, probably going to require upstream consumers to set the entrypoint.

Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Adding a bit of context: FxA basically considers any attempt to load the login page without an entrypoint parameter to be a metrics bug, as we have no way of attributing the signin metrics for that flow back to a particular UI touchpoint. Making this a required argument surfaces that requirement to the application, so we probably don't want to paper over it at this level. (Although @grigoryk might prefer to paper over it here for a short period while considering the API implications).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we do decide to temporarily hard-code a default entrypoint here, I suggest the string "none"; this is what FxA uses internally when you don't provide an explicit entrypoint value.

@@ -140,7 +141,8 @@ class FirefoxAccount internal constructor(
accessType: AccessType
) = scope.async {
handleFxaExceptions(logger, "authorizeOAuthCode", { null }) {
inner.authorizeOAuthCode(clientId, scopes, state, accessType.msg)
val authParams = AuthorizationParams(clientId, scopes, state, accessType.msg)
inner.authorizeOAuthCode(authParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

Not necessarily for this PR, but flagging for future consideration - we should consider pushing the AuthorizationParams abstraction up through the API layers here, so that callers of the higher-level API can pass all the new properties in a sensible way.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we can do that in a follow-up.

val state = Uri.parse(url).getQueryParameter("state")!!
AuthFlowUrl(state, url)
}
}

override fun beginPairingFlowAsync(pairingUrl: String, scopes: Set<String>) = scope.async {
handleFxaExceptions(logger, "begin oauth pairing flow", { null }) {
val url = inner.beginPairingFlow(pairingUrl, scopes.toTypedArray())
val url = inner.beginPairingFlow(pairingUrl, scopes.toTypedArray(), "notset")
Copy link
Contributor

Choose a reason for hiding this comment

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

Right. Adding a bit of context: FxA basically considers any attempt to load the login page without an entrypoint parameter to be a metrics bug, as we have no way of attributing the signin metrics for that flow back to a particular UI touchpoint. Making this a required argument surfaces that requirement to the application, so we probably don't want to paper over it at this level. (Although @grigoryk might prefer to paper over it here for a short period while considering the API implications).

@jonalmeida
Copy link
Contributor

bors try

^ To run UI automation.

bors bot pushed a commit that referenced this pull request Jul 13, 2020
@bors
Copy link

bors bot commented Jul 13, 2020

try

Build succeeded:

@eliserichards
Copy link
Contributor

Ping! What's the timeline on getting this merged? 🙏 😉

@grigoryk
Copy link
Contributor

@eliserichards should merge today, hopefully.

Copy link
Contributor

@grigoryk grigoryk left a comment

Choose a reason for hiding this comment

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

Filed #7729 so we're not blocking on the entrypoint stuff, and #7730 for the auth params bit.

I'll take this for a quick spin locally, and if it all looks good will merge.

@@ -140,7 +141,8 @@ class FirefoxAccount internal constructor(
accessType: AccessType
) = scope.async {
handleFxaExceptions(logger, "authorizeOAuthCode", { null }) {
inner.authorizeOAuthCode(clientId, scopes, state, accessType.msg)
val authParams = AuthorizationParams(clientId, scopes, state, accessType.msg)
inner.authorizeOAuthCode(authParams)
Copy link
Contributor

Choose a reason for hiding this comment

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

Agreed, we can do that in a follow-up.

@grigoryk grigoryk added the do not land PRs that requires coordination before landing label Jul 15, 2020
@grigoryk
Copy link
Contributor

New plan: we'll land 61.0.9 first (with a logins crash fix and the metrics extension), and hopefully try to get it onto beta/nightly first.

That gives us time to actually do the API bits for this PR properly.

@grigoryk
Copy link
Contributor

Closing this PR, I'll do a pass on this instead which will make the necessary API changes (#7729 , #7730 ).

@grigoryk grigoryk closed this Jul 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do not land PRs that requires coordination before landing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants