Add Deep Linking into FxA and Prefill login Forms

RESOLVED FIXED in Firefox 55

Status

()

enhancement
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: vbudhram, Assigned: Grisha)

Tracking

unspecified
Firefox 55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [fxa-waffle])

Attachments

(1 attachment)

Reporter

Description

2 years ago
This is a feature request to add additional deep linking capabilities into android, in support of Firefox accounts.

Propose deeplinking scheme

firefox://fxa-signin?signin=<token>

After clicking this link, Firefox for android app opens and loads a view containing the FxA sign-in page. The page is loaded with the `signin=<token>` query parameter in the url.

The plan is to have FxA sign-in page read the query paramater and get the correspond user information and prefill the login forms.
Comment hidden (mozreview-request)
Assignee

Comment 2

2 years ago
Comment on attachment 8858186 [details]
Bug 1356386 - Support for FxA sign-in deep links

Sebastian, does this patch look about right for adding deep linking? It works well locally for URLs such as firefox://whatever?signin=token.

I'm curious to know how much do you think we should parse/enforce the specific URL path. Having ability to handle flexible links might be interesting, but it will likely be just something like firefox://fxa-signin?signin=token.

Also, the goal is to use this via adjust (it's supposed to do magic-platform-based redirects and somehow open a deep link after an app is installed).

I'm testing it right now in an emulator, opening https://app.adjust.com/2uo1qc?deep_link=firefox://fxa-signin?signin=token in Chrome redirects to intent://fxa-signin?signin=token&adjust_reftag=trackingcode#Intent;scheme=firefox;package=org.mozilla.firefox;end - which Chrome fails to open due to "unknown url scheme". So I need to look into that a bit more.
Attachment #8858186 - Flags: feedback?(s.kaspari)
Assignee

Comment 3

2 years ago
Huh, this is really weird. Somehow mercurial just lost bunch of code, and I didn't notice when pushing :/ Sebastian, the patch I intended to post actually had proper handling of the incoming intent, not something as hacky :-) So just pretend it's done correctly, and I'll re-post later.
Comment hidden (mozreview-request)
Assignee

Comment 5

2 years ago
(In reply to :Grisha Kruglov from comment #2)
> I'm testing it right now in an emulator, opening
> https://app.adjust.com/2uo1qc?deep_link=firefox://fxa-signin?signin=token in
> Chrome redirects to
> intent://fxa-signin?signin=token&adjust_reftag=trackingcode#Intent;
> scheme=firefox;package=org.mozilla.firefox;end - which Chrome fails to open
> due to "unknown url scheme". So I need to look into that a bit more.

That's just an "intent url" - https://developer.chrome.com/multidevice/android/intents. Not surprisingly, they do seem to work on an actual device (not an emulator). It seems like I will need an Adjust URL configured against a local dev package name - something like org.mozilla.fennec_grisha.

Comment 6

2 years ago
mozreview-review
Comment on attachment 8858186 [details]
Bug 1356386 - Support for FxA sign-in deep links

https://reviewboard.mozilla.org/r/130122/#review133704

Technically this looks good to me.

Note that there are a bunch of other ways to do this on Android:
* Using intent://
* Using android-app:// (I'm not sure whether this supports abitrary parameters)
* Using a more refined filter that catches specific (sub)domains/paths/..
* 

I guess you should talk to Nevin who's also implementing deeplinking to be used via Leanplum (bug 1356517).

> I'm curious to know how much do you think we should parse/enforce the specific URL path. Having ability to handle flexible links might be interesting, but it will likely be just something like firefox://fxa-signin?signin=token.

Ping me about this on IRC again!

::: mobile/android/base/AndroidManifest.xml.in:101
(Diff revision 2)
>                  <category android:name="android.intent.category.BROWSABLE" />
>                  <data android:scheme="http" />
>                  <data android:scheme="https" />
>                  <data android:scheme="about" />
>                  <data android:scheme="javascript" />
> +                <data android:scheme="firefox" />

I would try to not hard-code this so that not all firefox release channels (and forks) respond to "firefox".

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:106
(Diff revision 2)
>          filterFlags(intent);
>  
>          startActivity(intent);
>      }
>  
> +    private void dispatchAccountsDeepLink() {

So technically you could have deep linked to "about:accounts?..." too, right? Although this might be opened by multiple apps too (I think even chrome responds to about://).
(Using the whiteboard tag to link this into the FxA engineering waffleboard for tracking on our side as well; please let me know if this messes up any of your own uses of that field)
Whiteboard: [fxa-waffle]
Assignee

Comment 8

2 years ago
Shane, is the desires schema stabilized? If so, I'll land this on top of Bug 1356517 whenever that's ready.
Flags: needinfo?(stomlinson)
:Thanks Grisha.

The basic firefox://fxa-signin?signin=<token> still applies. What I am asking myself is whether we can pass additional query parameters other than `token`, e.g., adding utm_* params or an entrypoint to the deeplink would allow us to track from where the user came. Is that possible? I imagine so if we are passing the `signin` token. If so, would we want a list of acceptable params to pass, or tunnel arbitrary fields?



:rfkelly, :adavis - thoughts? Is the capability to pass utm_ and entrypoint params necessary? Are there other fields we need? I'm thinking in particular how we are going to start measuring OKR 2.1 [1].


[1] - https://docs.google.com/document/d/1Kg2zTmVipmxM2bqSUUcYTbznAdYIn2gI1etDjf5QVY4/
Flags: needinfo?(stomlinson)
Flags: needinfo?(rfkelly)
Flags: needinfo?(adavis)
Yes, I'd like to be able to pass through utm_* and entrypoint= query parameters through this deep link.  I suppose in theory we could encode these in the signin token and decode them again when we load the app, but it seems cleaner to pass them directly.
Flags: needinfo?(rfkelly)

Comment 11

2 years ago
> Is the capability to pass utm_ and entrypoint params necessary?

We could also lean on the existing flow event data and just pass a flow_id param instead. Those fields are all available in the `flow_metadata` table in redshift.

Comment 12

2 years ago
> We could also lean on the existing flow event data and just pass a flow_id param instead.

Actually, could we even re-use the flow id as the sign-in token? It seems like we'd always have a flow id at that point, would there be any harm in using the same token for both, then we'd automatically get the benefit of being able to look stuff up in `flow_metadata`.
(In reply to Phil Booth [:pb] from comment #11)
> > Is the capability to pass utm_ and entrypoint params necessary?
> 
> We could also lean on the existing flow event data and just pass a flow_id
> param instead. Those fields are all available in the `flow_metadata` table
> in redshift.

Should the flow_id be the same for the originating and new flow? I can make
a case either way:

yes - it's a continuation of the same flow, surely we want to create an aggregate funnel.
no - the user is connecting a new device, we should track that flow independently.

I'm leaning towards "yes, keep the same flow_id" based on that 2 line brief. 

In either case, ISTM the entrypoint for device 1 and device 2 should be distinct
to allow us to create a 2nd, semi-independent funnel for just the 2nd device. For
OKR 2.1, we'd start that funnel from the point the deep link opens FxA. I can
imagine the entrypoint being something like `sms` on the 2nd device.

Can we place the flow_id in the data that is accessible using the signin token?
(In reply to Phil Booth [:pb] from comment #12)
> > We could also lean on the existing flow event data and just pass a flow_id param instead.
> 
> Actually, could we even re-use the flow id as the sign-in token? It seems
> like we'd always have a flow id at that point, would there be any harm in
> using the same token for both, then we'd automatically get the benefit of
> being able to look stuff up in `flow_metadata`.

This seems reasonable to me!
> Should the flow_id be the same for the originating and new flow? 

FWIW I recall :pb talking me around to "no, it should be a new flow" in a meeting some time ago.  One argument in favour of being a new flow is that it keeps a very clear definition of "completed" - if a user signs in on one device, continues the same flow onto a second device, but fails to sign in there, does that count as a completed flow?

> could we even re-use the flow id as the sign-in token?'

I dunno, I recall a lot of talk about adding all sorts of semantics to the sign-in token to e.g. make magic login links work, conflating that with something that appears plaintext in our metrics data might be asking for trouble down the road.
(In reply to Shane Tomlinson [:stomlinson] from comment #9)
> 
> :rfkelly, :adavis - thoughts? Is the capability to pass utm_ and entrypoint
> params necessary? Are there other fields we need? I'm thinking in particular
> how we are going to start measuring OKR 2.1 [1].
> 


IIUC there should be no problem passing more parameters. Seems like we should be able to add any params to the end of our deeplink destination.

https://docs.adjust.com/en/deeplinking/

e.g. https://app.adjust.com/f0ob4r?idfa=[EXAMPLE]&deep_link=example://fxa.login/?utm_source....
Flags: needinfo?(adavis)
:grisha - Would it be acceptable to propagate *all* query parameters in the deep link, with the exception of context and service, to FxA? We would want context and service to always be set by the browser since these parameters control how FxA attempts to communicate with Firefox. Propagating all parameters gives us some room to add new parameters in the future w/o needing to update the browser. If this is unacceptable, we can create a list of parameters currently supported.
Flags: needinfo?(gkruglov)
Assignee

Comment 18

2 years ago
(In reply to Shane Tomlinson [:stomlinson] from comment #17)
> :grisha - Would it be acceptable to propagate *all* query parameters in the
> deep link, with the exception of context and service, to FxA? We would want
> context and service to always be set by the browser since these parameters
> control how FxA attempts to communicate with Firefox. Propagating all
> parameters gives us some room to add new parameters in the future w/o
> needing to update the browser. If this is unacceptable, we can create a list
> of parameters currently supported.

I'd prefer a whitelist of parameters, but being able to move this forward without browser changes is a great point. How are you processing the incoming parameters on the content side? Is there any chance of XSS of sorts, hijacking something useful, like a session token, device list, etc..? I'm vaguely worried that passing through an unfiltered list might broaden an attack surface somehow. On the other hand I'm familiar with the security measures we have around this, so let me know if my worries are unfounded.
Flags: needinfo?(gkruglov)
ni'ing Shane for visibility to Grisha's questions
Flags: needinfo?(stomlinson)
(In reply to :Grisha Kruglov from comment #18)
> (In reply to Shane Tomlinson [:stomlinson] from comment #17)
> 
> I'd prefer a whitelist of parameters, but being able to move this forward
> without browser changes is a great point. How are you processing the
> incoming parameters on the content side? Is there any chance of XSS of
> sorts, hijacking something useful, like a session token, device list, etc..?
> I'm vaguely worried that passing through an unfiltered list might broaden an
> attack surface somehow. On the other hand I'm familiar with the security
> measures we have around this, so let me know if my worries are unfounded.

Thanks for the ni :julie, this slipped past me.

On the content sever, we validate all "expected" query parameters, and ignore
any that we don't. The current list of supported query parameters is in [1].

The backend services validate all query parameters and POST data as well.

Even though we validate all currently expected parameters, where I can imagine 
potential future problems is if new parameters are added that we expect to 
be set by the browser, parameters that control behavior in some way. 
If a browser passes arbitrary query parameters, users on those browsers
may be open to being gamed if we ever add a new parameter.

From that perspective, maybe for this version, we only allow utm_* and entrypoint 
query parameters. If we need to expand the functionality in the future,
we do so, and those browsers will know what to do.

:rfkelly - thoughts? If we go with a strict list of allowed parameters,
are there any others we should add now? :adavis proposed to add
a new query parameter for attribution, I forget what that is.

[1] - https://github.com/mozilla/fxa-content-server/blob/da6a1ef28a7c03c620d72cff7cdaa15500ef1674/docs/query-params.md
Flags: needinfo?(stomlinson)
Flags: needinfo?(rfkelly)
Flags: needinfo?(adavis)
OK, sounds like there are enough raised eyebrows to warrant some caution here, I'm happy to go with an allowlist of `signin` + `entrypoint` + `utm_*` to start with.
Flags: needinfo?(rfkelly)
IIUC we've agreed on using an allowlist of query parameters, so clearing the ni? for Alex here.
Flags: needinfo?(adavis)
master https://github.com/mozilla-mobile/firefox-ios/commit/bbe4956d918b25e58f14a814e19bf6b9ce6723db

Thanks guys for all the hard work!
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Closed the wrong one :/
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment hidden (mozreview-request)
Assignee

Comment 26

2 years ago
mozreview-review
Comment on attachment 8858186 [details]
Bug 1356386 - Support for FxA sign-in deep links

https://reviewboard.mozilla.org/r/130122/#review147726

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:244
(Diff revision 3)
> +
> +        String dispatchUri = AboutPages.ACCOUNTS + "?";
> +
> +        // If token is missing from the deep-link, we'll still open the accounts page.
> +        if (accountsToken != null) {
> +            dispatchUri = dispatchUri.concat(DeepLinkContract.ACCOUNTS_TOKEN_PARAM + "=" + accountsToken + "&");

Uri.Builder doesn't want to play nicely with "about:accounts" style base uris, and I didn't quite feel like making it work.

Nevin, let me know if you feel like this is a bit too hacky.
Assignee

Comment 27

2 years ago
mozreview-review-reply
Comment on attachment 8858186 [details]
Bug 1356386 - Support for FxA sign-in deep links

https://reviewboard.mozilla.org/r/130122/#review133704

> I would try to not hard-code this so that not all firefox release channels (and forks) respond to "firefox".

That ship has sailed along with the other deep linking efforts.

Comment 28

2 years ago
mozreview-review
Comment on attachment 8858186 [details]
Bug 1356386 - Support for FxA sign-in deep links

https://reviewboard.mozilla.org/r/130122/#review147780

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java
(Diff revision 3)
>  
>  /**
>   * Activity that receives incoming Intents and dispatches them to the appropriate activities (e.g. browser, custom tabs, web app).
>   */
>  public class LauncherActivity extends Activity {
> -

nit: a line is missing.

::: mobile/android/base/java/org/mozilla/gecko/LauncherActivity.java:244
(Diff revision 3)
> +
> +        String dispatchUri = AboutPages.ACCOUNTS + "?";
> +
> +        // If token is missing from the deep-link, we'll still open the accounts page.
> +        if (accountsToken != null) {
> +            dispatchUri = dispatchUri.concat(DeepLinkContract.ACCOUNTS_TOKEN_PARAM + "=" + accountsToken + "&");

I don't see much problem here.
Attachment #8858186 - Flags: review?(cnevinchen) → review+

Comment 29

2 years ago
Pushed by gkruglov@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2c04fc5bb9fc
Support for FxA sign-in deep links r=nechen

Comment 30

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2c04fc5bb9fc
Status: REOPENED → RESOLVED
Closed: 2 years ago2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Reporter

Comment 31

2 years ago
Hey :grisha, in your demo you mentioned that you were only able to get `deferred` deeplinking working once. I spoke with Adjust support and they said that makes sense. Once you launch the app, Adjust stores the advertising Id and wont launch the deep link again, ref: https://bugzilla.mozilla.org/show_bug.cgi?id=1368688#c3.

If you tried clearing your advertising Id for android it might launch the deferred deeplink?
Flags: needinfo?(gkruglov)
Assignee

Updated

2 years ago
Depends on: 1376982
Assignee

Comment 32

2 years ago
Circling back here, all seems fine now. Resetting advertising ID coupled with testing this on a build that actually has Adjust SDK enabled (duh!) seems to have done the trick.
Flags: needinfo?(gkruglov)
You need to log in before you can comment on or make changes to this bug.