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

Create signin context for standalone mobile fxa libs #5029

Closed
mcomella opened this issue May 3, 2017 · 27 comments · Fixed by #5084
Closed

Create signin context for standalone mobile fxa libs #5029

mcomella opened this issue May 3, 2017 · 27 comments · Fixed by #5084

Comments

@mcomella
Copy link

mcomella commented May 3, 2017

We're creating an Android and iOS standalone fxa/Sync library. In the short term, these are to download read-only from Sync.

During the web flow sign in, we're currently using the context "fx_ios_v1" to initiate the postMessage interface (we modeled the login code after FF iOS).

We should create a new sign in context to differentiate these new libraries. I propose:

"mob_[platform]_v1"

e.g. "mob_ios_v1".

fwiw, I'd expect these libraries to be ready for use within the next few weeks (we'll use fx_ios_v1 until this bug is complete).

CC @liuche.

@rfk
Copy link
Contributor

rfk commented May 3, 2017

For context for FxA folk: these libraries will be used for prototyping new standalone mobile apps that want to integrate with the user's browser data.

Our longer-term strategy is to try to move this integration to a more standard OAuth-style flow, which will give us finer control over what data is shared and how. But that's rather open-ended on our side, so the mobile team is moving forward in parallel using the existing sync login flows.

@vladikoff
Copy link
Contributor

cc @shane-tomlinson , let's chat about this when you are back...

@vladikoff
Copy link
Contributor

cc @rfk @shane-tomlinson

@rfk
Copy link
Contributor

rfk commented May 16, 2017

cc @rfk @shane-tomlinson

We discussed this and agreed it was broadly the Right Thing; rfk to follow up on the question of using WebChannels vs older custom events stuff.

@rfk
Copy link
Contributor

rfk commented May 18, 2017

So @shane-tomlinson reminded me that the fx_ios_v1 context uses an older event mechanism for communicating with FxA. If we can do it without too much code churn on your side @mcomella, let's see if we can move you over to using the newer "webchannel" event protocol [1].

My understanding of what this would look like is:

  1. Switch to using context=fx_fennec_v1, which uses the new webchannel-based events
  2. In the code at [2], listen for an event named "WebChannelMessageToChrome" instead of "FirefoxAccountsCommand", and adapt to slightly different fields in the event body as described in [1].
  3. See if it works :-)

@shane-tomlinson can you please sanity-check the above?

[1] https://github.com/mozilla/fxa-content-server/blob/master/docs/relier-communication-protocols/fx-webchannel.md
[2] https://github.com/mcomella/FirefoxAccounts-android/blob/master/accounts/src/main/res/raw/firefox_account_login.js#L17

@mcomella
Copy link
Author

I got the WebChannel implementation working.

However, fx_fennec_v1 has an additional login step where the user selects which engines they want to sync & the copy is not correct for our use case. @rfk How should we go about fixing that?

@rfk
Copy link
Contributor

rfk commented May 19, 2017

Thanks @mcomella, I'm relieved to here there weren't any gotchas!

However, fx_fennec_v1 has an additional login step where the user selects
which engines they want to sync & the copy is not correct for our use case.

I assume you'll just want to skip this screen completely? When we define the new context values, we can add corresponding logic to tweak the flow as appropriate and avoid this screen.

@shane-tomlinson
Copy link

@shane-tomlinson can you please sanity-check the above?

Sounds prefect, and sounds like @mcomella has already done it. 🕺

However, fx_fennec_v1 has an additional login step where the user selects which engines they want to sync & the copy is not correct for our use case. @rfk How should we go about fixing that?

Like @rfk said, we can create the new brokers and have these screens skipped. I'll put that on my plate for first thing next week.

@shane-tomlinson
Copy link

To summarize:

Two new context query parameters are needed, mob_ios_v1 and mob_android_v1. These will both use WebChannels to communicate with the app. Since both access "readonly" versions of Sync, they should skip the "choose what to sync" screen.

Does this sound correct @mcomella?

@shane-tomlinson
Copy link

@mcomella - I was just reviewing the set of WebChannel commands sent for the signin flow. Will the library respond to the fxaccounts:can_link_account command? I assume yes since you already made it to the Choose What to Sync page.

@mcomella
Copy link
Author

mcomella commented May 19, 2017

Two new context query parameters are needed, mob_ios_v1 and mob_android_v1. These will both use WebChannels to communicate with the app. Since both access "readonly" versions of Sync, they should skip the "choose what to sync" screen.

Does this sound correct @mcomella?

Correct.

@mcomella - I was just reviewing the set of WebChannel commands sent for the signin flow. Will the library respond to the fxaccounts:can_link_account command? I assume yes since you already made it to the Choose What to Sync page.

I do currently but I'm returning true every time – my login component isn't aware of the account store so it's up to the user of the library to decide based on the account store state. I can see it being useful if we changed the library API though.

ty!

@shane-tomlinson
Copy link

@mcomella - I'm putting together the new brokers for you and have a question regarding flow.

How do the libraries handle users that must confirm their email, e.g., both sign up and sign in (usually) require an email verification to access Sync data.

On login, we send the fxaccounts:login message with verified: false. Will the app/library poll until verification has completed?

  • If yes, does the app/library take over the UI as soon as it detects the user has verified?
  • If no, will the libraries lean on FxA to poll and send a notification as soon as verification completes? We don't have a WebChannel message for this currently, but it's not a problem to create one.

@mcomella
Copy link
Author

mcomella commented May 23, 2017

@shane-tomlinson Our current flow when receiving fxaccounts:login:

  • If verified, close the login page
  • If not verified, leave the login page open, waiting for the user to complete the verification process. Since we get no indication when the verification process completes, the user has to hit back to close the login page when they see the verification UI appear.

This is modeled after the current Firefox for iOS behavior.


I wasn't aware that we could poll for the verification state but that sounds a lot better than forcing the user to close the login page themselves!

That being said, I think it's the most correct to have FxA notify us when the verification completes. If that's not feasible or is too much work, I think polling will be fine.

How would polling work? What message do we have to send to get the account state?

@rfk
Copy link
Contributor

rfk commented May 23, 2017

Since we get no indication when the verification process completes

ISTM it should be possible for us to send a webchannel message to notify when this completes - @shane-tomlinson?

@shane-tomlinson
Copy link

ISTM it should be possible for us to send a webchannel message to notify when this completes - @shane-tomlinson?

We don't have a WebChannel message for this currently, but it's not a problem to create one.

@shane-tomlinson
Copy link

That being said, I think it's the most correct to have FxA notify us when the verification completes. If that's not feasible or is too much work, I think polling will be fine.

I have implemented polling in the FxA account UI, once the user has verified their email address, we'll send you a new WebChannel message, fxaccounts:email_verified. Sound good?

Another question I had while writing functional tests is during the normal Sync signup, we display a page called Choose what to Sync that allows the users to select which Sync buckets they'd like to sync across their devices. Should this screen be displayed for apps that consume this library?

I'm trying to put PR #5084 on https://stomlinson.dev.lcip.org so that you have an environment you can tests against.

@shane-tomlinson
Copy link

I'm trying to put PR #5084 on https://stomlinson.dev.lcip.org so that you have an environment you can tests against.

@mcomella - The PR is up and ready for you to test against when you get a chance!

@mcomella
Copy link
Author

mcomella commented Jun 7, 2017

I have implemented polling in the FxA account UI, once the user has verified their email address, we'll send you a new WebChannel message, fxaccounts:email_verified. Sound good?

Sounds good!

...we display a page called Choose what to Sync that allows the users to select which Sync buckets they'd like to sync across their devices. Should this screen be displayed for apps that consume this library?

No, this page should not be displayed (because we can't stop the application using the library from taking the sync tokens out of memory and downloading whatever information we want with it so we shouldn't give the user this guarantee).

The PR is up and ready for you to test against when you get a chance!

I went to https://stomlinson.dev.lcip.org/signin?service=sync&context=mob_android_v1 (and tried again with context=mob_ios_v1) and I did not receive any WebChannel events, making me think the context was invalid - any ideas what could be going on, @shane-tomlinson ?

@mcomella
Copy link
Author

mcomella commented Jun 7, 2017

We rebooted the server and I'm received WebChannel events. That being said, my response to the fxaccounts:can_link_account is not being received successfully by the page. There's no log message (in my browser console or from the server) so @shane-tomlinson asked me to paste what I'm injecting into the page in response:

window.dispatchEvent(new CustomEvent('WebChannelMessageToContent', {"detail":{"message":{"command":"fxaccounts:can_link_account","messageId":0,"data":{"ok":true}},"id":"account_updates"}}));

My guesses that I'll look into:

  • messageId should be a string rather than a number
  • id may not match the id we're being given in the original message (iirc, I hard-coded this)

@shane-tomlinson
Copy link

messageId should be a string rather than a number

Yeah, if messageId doesn't match, that'll cause the symptoms you see.
messageId should be a string, it's created using this logic:

return `${Date.now()}${++messageIdSuffix}`;

messageIdSuffix is the # of WebChannel messages that have been sent.


id should be account_updates, so that should be fine.

@mcomella
Copy link
Author

Turns out I was taking putting sending the messageId as a long to Java (thus it was 0) rather than taking a String - I fixed it (I don't understand why this worked with fx_fennec_v1 though...).

The new fxaccounts:email_verified message works well – thanks!

One issue I ran into is that fxaccounts:loaded is no longer fired. I'm not using it at the moment but it could be useful for mozilla-mobile/FirefoxData-android#2. I believe we have some WebView callbacks we can listen for for when the page loads but it's clearer to have the page tell you itself. @shane-tomlinson Is that something you can add back?

mcomella added a commit to mozilla-mobile/FirefoxData-android that referenced this issue Jun 12, 2017
This is mozilla/fxa-content-server#5029.

messageId needed to be a String for this to work. For some reason, it was able
to work before even though we casted it to long when we sent it to Java.
mcomella added a commit to mozilla-mobile/FirefoxData-android that referenced this issue Jun 12, 2017
This is mozilla/fxa-content-server#5029.

messageId needed to be a String for this to work. For some reason, it was able
to work before even though we casted it to long when we sent it to Java.
@shane-tomlinson
Copy link

@shane-tomlinson Is that something you can add back?

Hmm, that's very strange. It's supposed to be sent universally, if that's not the case, I must have a whole in the functional test coverage. :/ I'll try to have that ready for you today.

@shane-tomlinson
Copy link

@mcomella - I just added some functional tests to ensure the fxaccounts:loaded message is sent. It looks like it's being sent, but since it sends no response, it's messageId is set to null:

screen shot 2017-06-13 at 14 18 57

@mcomella
Copy link
Author

Sorry, I think this was my bug again (ty for investigating!).

I inject my JavaScript, which adds the fxa listeners, in the WebView's onPageFinished callback. I'm guessing there's a race condition: sometimes we inject the script before fxaccounts:loaded is fired & sometimes we don't. Sounds like I should just add my own listeners to onPageFinished if I want to know when the page loads (and if that doesn't fit my use case, file a new bug).

This build works for me – thanks @shane-tomlinson ! When do you expect it to merge to production?

(fwiw, iOS may still want to use fxaccounts:loaded)

mcomella added a commit to mozilla-mobile/FirefoxData-android that referenced this issue Jun 14, 2017
This is mozilla/fxa-content-server#5029.

messageId needed to be a String for this to work. For some reason, it was able
to work before even though we casted it to long when we sent it to Java.
@shane-tomlinson
Copy link

shane-tomlinson commented Jun 14, 2017

I inject my JavaScript, which adds the fxa listeners, in the WebView's onPageFinished callback.

The earlier you can bind, the better. We send that message as soon as the first view is rendered, which is usually after DOMContentLoaded, but sometimes not by much.

This build works for me – thanks @shane-tomlinson ! When do you expect it to merge to production?

I'll have the reviews begin today. We just cut a train, it'll unfortunately be 2 more weeks before we cut the next one, and another week before it goes to prod. If having this in prod sooner than that is imperative, let's talk.

Thanks for working through this with me!

@shane-tomlinson
Copy link

@mcomella - if we changed the WebChannel message from fxaccounts:email_verified to fxaccounts:verified, would that cause havoc for you?

@mcomella
Copy link
Author

@shane-tomlinson Nope! Go for it.

shane-tomlinson pushed a commit that referenced this issue Jun 15, 2017
…) r=@philbooth

When the user verifies their email, a `fxaccounts:verified` message is sent over the WebChannel.

fixes #5029
mcomella added a commit to mozilla-mobile/FirefoxData-android that referenced this issue Jun 15, 2017
This is mozilla/fxa-content-server#5029.

messageId needed to be a String for this to work. For some reason, it was able
to work before even though we casted it to long when we sent it to Java.
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 a pull request may close this issue.

4 participants