1000323 - Find My Device should call navigator.mozId.request({refreshAuthentication: 0}) on a disable event (can't switch user)

Status: VERIFIED FIXED

(Firefox OS Graveyard :: FindMyDevice, defect)

Description

[Sam Penrose]

My understanding is that disabling FMD on device should require that the user enter their FxA password. The way to do that is to tie the above call to a disable event, and only perform the disabling if the call succeeds.

If I am mistaken about this product requirement, of course feel free to modify or close this bug.

Note that refreshAuthentication has a bug ATM; I have made the corresponding issue a blocker for this one.

Comment 1

[Guilherme Gonçalves [:ggp]]

Makes sense Sam, thanks for reporting. I'll keep an eye on the blocker and fix this as soon as possible.

Comment 2

[Sam Penrose]

Turns out this will be fixed when 994934 lands, which should be some time today. Changing the dependency.

Comment 3

[Guilherme Gonçalves [:ggp]]

I took a shot at this, but it still doesn't seem to work. Here's my attempt: https://github.com/guilherme-pg/gaia/commit/64f7ec01

Even after calling navigator.mozId.request({refreshAuthentication: 0}), I don't get prompted for my password again. Instead, my onlogin callback simply gets called immediately, as I'm already logged in.

I'm trying this on a recently-updated B2G desktop client build that contains the patch in bug 994934.

Comment 4

[Sam Penrose]

You're correct, very sorry about that. For now, this works:

  navigator.mozId.request({refreshAuthentication: 1});

I will fix that. Independent of my fix, you'll need to modify _onCheckboxChanged and the callbacks to mozId.watch() roughly as follows:

  1) _onCheckboxChanged needs to set a flag saying "the user is trying to disable". I think it probably will be easier to get right if you don't immediately disable the checkbox before the user has demonstrated the right to do so.

  2) if the user passes the challenge, the callback passed in as "onlogin" to watch() will fire. I believe you can change the current code at line 27:
    onlogin: self._onChangeLoginState.bind(self, true),
to:
    onlogin: self._onChangeLoginState.bind(self)
That should result in loggedIn becoming an assertion. Then in _onChangeLoginState(), if the "user is trying to disable" flag is set, you disable and clear the flag.

  3) You should also pass onerror and oncancel parameters to mozId.watch(), and make sure they do the right thing. Depending on how you write _onCheckboxChanged, that's probably "nothing."

I'll let you know when 0 works, but for now 1 should get you unblocked.

Comment 5

[Guilherme Gonçalves [:ggp]]

Still not working, sadly. Same code as in the GitHub link in comment 3, but now the console indicates that I'm reaching https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/fxa_manager.js#L150

What is this accountId? Am I missing something in my call to request(), or is this an FxA bug? Note that I am correctly signed in to a verified account while trying this.

Comment 6

[Sam Penrose]

Hey ggp, I missed this case when manually testing a refactor yesterday that removed "accountId" from Gecko. Very sorry about that. We'll have a Gaia fix to you in 12-24 hours, reviewers willing.

Comment 8

[Guilherme Gonçalves [:ggp]]

Sam, I noticed an odd behavior on FxA while trying to fix this.

The first thing I do before calling navigator.mozId.request() when the user changes the "Enable Find My Device" checkbox is to disable the checkbox, and thus I'd like to re-enable it in case the user cancels the FxA flow (I already re-enable it after a succesful login/logout). Hence, I added an oncancel handler [1], which is not called because of bug 982969, which I marked as blocking this one.

However, I also have a small component in the system app that monitors the login state using mozFxAccountsUnsolChromeEvent [2], so that if the user logs out of FxA at any time, FMD is automatically disabled. I have observed that, when I cancel the FxA flow, a mozFxAccountsUnsolChromeEvent with eventName equal to "onlogout" gets triggered, which of course causes FMD to disable itself even though it shouldn't. I'm guessing this is a separate FxA bug, right?

Summarizing the STR (use my bug1000323 branch [3]):
- Log in to FxA.
- Open the FMD panel in the Settings app.
- Enable FMD by toggling the checkbox/slider thingy.
- Try to disable FMD by toggling the checkbox/slider thingy, FxA flow comes up.
- Close the FxA flow.
- Observer how the checkbox is disabled (because oncancel wasn't fired), and FMD has been disabled because of this seemingly spurious mozFxAccountsUnsolChromeEvent.

1- https://github.com/guilherme-pg/gaia/blob/bug1000323/apps/settings/js/findmydevice.js#L85
2- https://github.com/guilherme-pg/gaia/blob/bug1000323/apps/system/js/findmydevice_launcher.js#L24
3- https://github.com/guilherme-pg/gaia/tree/bug1000323

Comment 9

[Sam Penrose]

So I messed up but not giving you good documentation on using the API. (You can see a start here:

  https://id.etherpad.mozilla.org/tracing-mozId-API

I'll create a version focused on your needs in the next couple weeks.)

FMD should not sniff the mozFxAccountsUnsolChromeEvent. Instead, even while you're waiting for oncancel() to work, onerror() should be called instead, and you should be able to work with that. Can you take out the mozFxAccountsUnsolChromeEvent code and let me know what the result is? Sorry to have misled you about the API use.

Comment 10

[Guilherme Gonçalves [:ggp]]

(In reply to Sam Penrose from comment #9)
> FMD should not sniff the mozFxAccountsUnsolChromeEvent. Instead, even while
> you're waiting for oncancel() to work, onerror() should be called instead,
> and you should be able to work with that.

Hmm, I'm not really in a hurry to land a workaround for this in the next few days, so
I think I can just wait until 982969 lands, unless you think it will take more than a
couple of weeks.

It seems to me that the mozFxAccountsUnsolChromeEvent is a separate issue from this one,
so I've filed 1008949 for that, and we can keep the scope of this bug narrow.

Comment 11

[Guilherme Gonçalves [:ggp]]

I expect to remove the mozFxAccountsUnsolChromeEvent listener as part of bug 1014816, so perhaps bug 1013344 doesn't need to block this one (but 1014816 does). I'll remove it from the block list once I'm more confident that this is the case.

The way I see it, then, a solution to this bug would entail storing the email account of the user that logged in to FxA, then upon a disable attempt, check whether the currently logged in user matches the stored email, and only if it does, perform a forced authentication.

We would also like to display the email account of the user who enabled FMD on the Settings app, and :jgruen may provide a mock. It would also be nice to have some UX guidance on what to display when the email accounts don't match while trying to disable.

Comment 12

[Guilherme Gonçalves [:ggp]]

Attachment: gaia pull request

Not handling some error cases, but I think we can figure those out later as needed, and there is some overlap between them and the other bugs we have for handling errors (bug 998489 and bug 1013423).

I'll do a little more testing before flagging this r?, but I think this will work.

Comment 13

[Guilherme Gonçalves [:ggp]]

Also, this still needs tests. Leaving this here as a note for my future self.

Comment 14

[No-Jun Park [:njpark]]

With this bug, following currently happens:
- Log in to firefox accounts, and enable FMD (works)
- disable FMD (this should ask for username/password, but the bug prevents it, and just disables it)

But also, this happens too:
- when FMD is enabled, log out of firefox account (FMD is still enabled)
- Disable FMD (disabled without password prompt, again.)
- Enable FMD (enabled without password prompt!)

So you can enable (in reality, maybe it is lying) FMD without FxA credential, as long as the user was logged in after the phone booted.

Do you think this should be a blocker?  Because once the user decides to sign out for whatever reason and use different credential, this may not work, although it may appear to the user that it works.

Comment 15

[Guilherme Gonçalves [:ggp]]

(In reply to No-Jun Park [:njpark] from comment #14)
> With this bug, following currently happens:
> - Log in to firefox accounts, and enable FMD (works)
> - disable FMD (this should ask for username/password, but the bug prevents
> it, and just disables it)

Are you trying this with the patch in the pull request attached to the bug? It's not complete yet, but it should definitely cause it to ask for the password.

> But also, this happens too:
> - when FMD is enabled, log out of firefox account (FMD is still enabled)

This is intended after behavior after bug 1014816, but my reply to "Enable FMD" below.

> - Disable FMD (disabled without password prompt, again.)

Yup, this is caused by this very bug.

> - Enable FMD (enabled without password prompt!)

Ah, this one is interesting. I would expect FMD's panel in the Settings to be back to the login step after you logged out of FxA. On a quick experiment here, it seems that onlogout is not firing, which looks like a FxA bug (or maybe I just misunderstood something).
 
> Do you think this should be a blocker?  Because once the user decides to
> sign out for whatever reason and use different credential, this may not
> work, although it may appear to the user that it works.

I guess we'll decide that shortly when we triage, but personally, I don't mind making it a blocker. It's really kind of the other half of bug 1014816, which already landed.

Comment 16

[Guilherme Gonçalves [:ggp]]

(In reply to Guilherme Gonçalves [:ggp] from comment #15)
> This is intended after behavior after bug 1014816, but my reply to "Enable
> FMD" below.

"... but please see my reply..."

Comment 17

[No-Jun Park [:njpark]]

Sam, in comment 15, do you think it is the FxA bug?  if so, should we raise a blocker bug for this?

Comment 18

[Guilherme Gonçalves [:ggp]]

I can confirm that onlogout is not firing consistently.

STR:
1) Go to Settings > Firefox Accounts
2) Log in
3) Go back, enter Find My Device, notice how it properly shows the Enable/Disable switch
4) Go back to Firefox Accounts
5) Log out
6) Go back to Find My Device, notice how it still shows the Enable/Disable switch

In step 6, we should see the login button in FMD, which is displayed when onlogout fires. Also, FMD logs a message to the console whenever onlogin/onlogout fires, and that message is not logged for logout -- which further indicates that onlogout is not being fired.

Note, however, that if you close the Settings app, reopen it and go to Find My Device, that will cause a new call to navigator.mozId.request() to be made, which successfully fires onlogout then. Sam, is this a known bug?

Comment 19

[Sam Penrose]

Created bug 1025309 , top of my queue.

Comment 20

[bhavana bajaj [:bajaj]]

Elan, this sounds like a missed feature ? Can you confirm. If so, we should be using feature-b2g on this vs the blocking flag.

Comment 21

[JR Conlin [:jrconlin,:jconlin]]

To validate an assertion (and get a user's UID)call /$VERSION/validate 
$VERSION is the current API version number. 
passing:
{"assert": $ASSERTION }

on success, this call will return 
{"valid": True, "uid": $UID}

if the assertion is not valid (expired, etc.)

{"valid": False}

Comment 22

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #20)
> Elan, this sounds like a missed feature ? Can you confirm. If so, we should
> be using feature-b2g on this vs the blocking flag.

whoops, got my bugs missed up when commenting...

Comment 23

[Guilherme Gonçalves [:ggp]]

Attachment: gaia pull request

This patch completes the FxA integration we were aiming for, and it deserves some explanation:

If Alice logs in to FxA and enables FMD, she will have to retype her password to disable it from the Settings app. If she logs out and Bob logs in, FMD's switch in the Settings app will be disabled until Alice logs back in, bug FMD will keep working in the background and the phone will still be controlled by Alice (bug 1014816).

This was implemented by introducing a couple of new settings: 'findmydevice.current-clientid' is the client id of the current logged in user, as returned by the server's /validate endpoint (which receives a FxA assertion); 'findmydevice.can-disable' is true or false depending on whether the currently logged-in user is allowed to disable FMD. The initial client id is returned by the server when Alice registers for FMD, so 'findmydevice.can-disable' will only be true when 'findmydevice.current-clientid' is equal to Alice's client id.

The disable procedure then works as follows: when Alice attempts to disable FMD from the Settings app, a wake-up message will be posted to FMD via the IAC API, causing 'findmydevice.current-clientid' to be invalidated and re-requested from the server. Upon determining that the currently logged-in user is indeed Alice, 'findmydevice.can-disable' will be set to true, to which the Settings app can react and disable FMD. It would also have been possible to disable FMD from inside FMD itself, but I chose not to because the rest of the code assumes that only the Settings app enables/disables FMD, so I didn't want to risk breaking what is already a quite complicated state machine.

Comment 24

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8447396 [details] [review]
gaia pull request

Etienne, do you mind reviewing this please?

Comment 25

[Edwin Wong [:edwong]]

updated title to emphasis the user issue: can't switch fxa user on device.

Comment 26

[Tony Chung [:tchung]]

Adding "verifyme" keyword, to double check the STR in comment 14 after this gets landed and uplifted to 2.0 branch.

Comment 27

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8447396 [details] [review]
gaia pull request

Apologies for the review flag spam, this should be the last one :)

Comment 28

[:gerard-majax]

Okay, after several (lots of) retries yesterday, I finally got the current FMD feature to work and be able to communicate with my Flame.

Comment 29

[:gerard-majax]

I'm going to leave some general comments there, and keep github for code-specifics remarks.

Comment 30

[:gerard-majax]

A first comment I have to make/ask about is that I don't know if that's expected, but a first feedback I would give is there's a not very good UX the first time I launched the find my device panel: for a couple of seconds, I had an empty panel.

Comment 31

[:gerard-majax]

And it looks like server is down again ...

Comment 32

[:gerard-majax]

Looks like rebooting without data enabled, opening the panel I have lost the information: I get the create account or signin button.

Comment 33

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #32)
> Looks like rebooting without data enabled, opening the panel I have lost the
> information: I get the create account or signin button.

But it is to be noted that I have the "Enabled" text visible on the first page of Settings.

Comment 34

[:gerard-majax]

I fear my recent failures in getting the feature to work could come from bug 1032938 ... It's hard to review with so much breakages in parallel.

Comment 35

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #33)
> (In reply to Alexandre LISSY :gerard-majax from comment #32)
> > Looks like rebooting without data enabled, opening the panel I have lost the
> > information: I get the create account or signin button.
> 
> But it is to be noted that I have the "Enabled" text visible on the first
> page of Settings.

Filed in bug 1034088

Comment 36

[:gerard-majax]

After tons of attempts, I finally have been able to get a window where server accepts to work and I could make sure the very core behavior of this bug is fixed: trying to logout I get the FxAccount authentication flow to validate my password. Entering an invalid password fails, and it properly disables when I enter my password.

Comment 37

[:gerard-majax]

Another spurious UX:
 0. Make sure locate my device is enabled
 1. Open the Find my device settings panel
 2. Uncheck the checkbox to disable the feature
 3. When prompted for your password, tap the "X" in the top left

Expected:
 The checkbox is left unchecked, and disabled, which does not reflect the proper state. I have to kill and restart Settings app to get back the proper state

Actual:
 When getting back to the panel should show that the feature is still enabled.

Comment 38

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #37)
> Another spurious UX:
>  0. Make sure locate my device is enabled
>  1. Open the Find my device settings panel
>  2. Uncheck the checkbox to disable the feature
>  3. When prompted for your password, tap the "X" in the top left
> 
> Expected:
>  The checkbox is left unchecked, and disabled, which does not reflect the
> proper state. I have to kill and restart Settings app to get back the proper
> state
> 
> Actual:
>  When getting back to the panel should show that the feature is still
> enabled.

The only associated logcat I can get is:
> 07-03 17:12:34.965  1274  1274 I GeckoDump: [findmydevice] got wake up request
> 07-03 17:12:34.965  1274  1274 I GeckoDump: [findmydevice] forcing reauth and refreshing client id
> 07-03 17:12:34.995  1274  1274 I GeckoDump: [findmydevice] current id set to: 
> 07-03 17:12:34.995  1274  1274 I GeckoDump: [findmydevice] refreshing client id if registered and logged in: {"registered":true,"loggedIn":true}
> 07-03 17:12:34.995  1274  1274 I GeckoDump: [findmydevice] requesting assertion to refresh client id, forceReauth: true
> 07-03 17:12:35.025   318   318 I GeckoDump: [system] findmydevice received refreshAuthentication FxA event in the System app

Comment 39

[:gerard-majax]

Also, I can disable Find My Device when I'm offline. How is this supposed to be possible if we ask user to authenticate itself on FxAccounts ?

Doing so for now brings me in the same state as documented in comment 38.

Comment 40

[Guilherme Gonçalves [:ggp]]

The issue in comment 37 is blocked by bug 1034229, but I added a fix to the PR in a separate commit for ease of reviewing. I also added a tentative fix for comment 38, but it's pretty much untested.

Comment 41

[:gerard-majax]

And today, again a lot of server issues it seems:

> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] findmydevice alarm!
> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] findmydevice attempting registration.
> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] registering: false
> 07-04 13:26:54.933  1036  1036 I GeckoDump: [findmydevice] refreshing client id if registered and logged in: {"registered":false,"loggedIn":true}
> 07-04 13:26:55.193  1036  1036 I GeckoDump: [findmydevice] findmydevice received push endpoint!
> 07-04 13:26:55.913  1036  1036 I GeckoDump: [findmydevice] findmydevice request failed with status: 401

But the status on the Settings panel seems to be fine. So, how can the user be sure it's enabled and working?

Comment 42

[:gerard-majax]

Correct me if I'm wrong, but you are changing the behavior there, and I don't see any test, either changed or added.

Comment 43

[:gerard-majax]

Guilherme, I commented on the PR. It's mostly questions I have regarding the behavior of your code, or small changes that I feel make it easier to read/understand.

As far as I remember, you told me that you still had some changes to push, so I'll stop having a look at this and continue getting familiar with the rest of the FMD Gaia codebase.

Also, as said in comment 42, I'm suspicious about the lack of tests, I suspect you have them on your other changes to push :)

Comment 44

[:gerard-majax]

Comment on attachment 8447396 [details] [review]
gaia pull request

So mainly for now, I think we lack tests and there are some UX deficiencies that I feel really major before we can get this.

Otherwise, my testing on device showed that the basics were working properly, out of the identified edge cases. Too bad we lost so much time fighting with server issues, landing of stage -> prod changes, etc.

I'm waiting for your next review? on this :)

Comment 45

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8447396 [details] [review]
gaia pull request

Fixed review comments and included tests for both the Settings changes and the FMD app. I rebased on top of bug 1035321 to make it easier to add tests. I think it's time for a new review! :)

Comment 46

[Jared Hirsch [:jhirsch] (he/him) (Needinfo please)]

Looks like the discussion in bug 1034088 has been settled by product peeps, unsetting ni?.

Please feel free to ping me again if there are other questions.

Comment 47

[:gerard-majax]

(In reply to Guilherme Gonçalves [:ggp] from comment #45)
> Comment on attachment 8447396 [details] [review]
> gaia pull request
> 
> Fixed review comments and included tests for both the Settings changes and
> the FMD app. I rebased on top of bug 1035321 to make it easier to add tests.
> I think it's time for a new review! :)

I'm testing this right now. Meanwhile, and even if I think we will land bug 1035321 prior to this one, please make sure to remove this commit at the end.

Comment 48

[:gerard-majax]

Testing your latest PR. My device had FMD enabled prior to applying your patch. When I go in settings, try to disable the "Find my device" checkbox, it stays enabled while the checkbox is still checked but disabled.

Comment 49

[:gerard-majax]

(In reply to Alexandre LISSY :gerard-majax from comment #48)
> Testing your latest PR. My device had FMD enabled prior to applying your
> patch. When I go in settings, try to disable the "Find my device" checkbox,
> it stays enabled while the checkbox is still checked but disabled.

Okay that may be my fault: prior to reflashing, the device was registered on stage. When reflashing it switched to prod. After a factory reset, I can properly disable the feature and I'm getting the Firefox Accounts prompt.

Comment 51

[:gerard-majax]

Comment on attachment 8447396 [details] [review]
gaia pull request

So far of my testing, the feature works pretty well as expected. I've left a bunch of comments, but I have not seens anything serious. Flag me review? again once you have a rebased version that addresses the comments :).

Comment 52

[Guilherme Gonçalves [:ggp]]

Thanks for the feedback! I rebased and fixed some of the simpler comments; I'll dive into the others on Monday.

Comment 53

[Guilherme Gonçalves [:ggp]]

Comment on attachment 8447396 [details] [review]
gaia pull request

PR updated, I think I've addressed all comments (or replied to explain why I couldn't).

I found another issue with the patch while testing: it interacts poorly with our auto-enable behavior on the Settings. The STR were:

1) Log in and enable FMD as usual
2) Disable FMD, re-entering your password.
3) Log out of FxA.
4) From FMD's panel in the Settings, log in as a different account.

This caused FMD to be re-enabled even though the new user is not authorized to do so. One easy fix for this is to only auto-enable when FMD is not already registered, which is how I did it, but I also need to add a test for that. For the record, I consulted with :vishy and :jsavory and they would be OK with just removing this auto-enabling entirely if needed.

I included the two nontrivial changes I made in separate commits for ease of reviewing. Re-setting the review flag.

Comment 54

[:gerard-majax]

Comment on attachment 8447396 [details] [review]
gaia pull request

That all seems to be fine now. I've left a last comment on one of the patch, small nit. Feel free to merge this once try is green. Do not forget to rebase and squash before :)

Comment 55

[:gerard-majax]

Comment on attachment 8447396 [details] [review]
gaia pull request

Hold down. I may have found a case where we can disable without being asked for password.

Comment 56

[:gerard-majax]

Comment on attachment 8447396 [details] [review]
gaia pull request

Looks like I was testing a wrong code when seeing the spurious behavior.

Comment 57

[Carsten Book [:Tomcat]]

master: https://github.com/mozilla-b2g/gaia/commit/0a0f549c6b341b8b14c4188390abbefe52847cb4

Comment 58

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/719f1ebea0fb48c323f40b5638fa60b1c082d769

Comment 59

[Mike Lien[:mlien]]

verify with Flame KK v180 + v2.0 gaia/gecko, it's fine
Gaia      7edd3b0b9f65c3dde235c732d270e43e055a1254
Gecko     https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/f3639e825b3b
BuildID   20140915135336
Version   32.0