1023463 - [FxA] leave user logged in after failed RP refresh authentication

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect, P2)

Description

[No-Jun Park [:njpark]]

STR:
- Open UI Tests App
- Go to API tab, Firefox Accounts mozId
- click refreshAuth button, and enter the correct password.
- click refreshAuth button again, and when the password field shows, enter a wrong password.  close the error message, and click X on top left corner to close the dialog.
- click refreshAuth again.

Expected:
Password field shows up.
Actual:
User is asked to enter password.

Version info:
Gaia      34cdda626d6bb7b0c74a82b4607162c2e967b80a
Gecko     9a6a74ab706d271815593f0c91b5e0a3878c560d
BuildID   20140610130808
Version   33.0a1
ro.build.version.incremental=eng.mozilla.20140610.125223
ro.build.date=Tue Jun 10 12:52:39 EDT 2014

Comment 1

[Sam Penrose]

Attachment: 1023463-refreshAuth-fail.patch

This patch fixes a silly mistake on my part. I did a big rewrite for bug 1004319 in the context of a server-side password change, forgetting to implement the common case of an RP-requested refreshAuth. I added a comment to job my future self's memory of the state machine.

Comment 2

[Jed Parsons (use needinfo, please) [:jedp, :jparsons]]

Comment on attachment 8438009 [details] [diff] [review]
1023463-refreshAuth-fail.patch

Review of attachment 8438009 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for the fix, Sam.  Looks good to me!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +216,5 @@
> +   * There are two very different scenarios:
> +   *   1) The password has changed on the server. Failure should log
> +   *      the current account OUT.
> +   *   2) The person typing can't prove knowledge of the password used
> +   *      to log in. Failure should do nothing.

comment is helpful

@@ +218,5 @@
> +   *      the current account OUT.
> +   *   2) The person typing can't prove knowledge of the password used
> +   *      to log in. Failure should do nothing.
> +   */
> +  _refreshAuthentication: function(aAudience, aEmail, logoutOnFailure) {

Since logoutOnFailure is optional, you could give it a default value?  (logoutOnFailure=false)

Comment 3

[Sam Penrose]

Attachment: 1023463-refreshAuth-fail.patch

Thanks Jed! Default parameter added.

https://tbpl.mozilla.org/?tree=Try&rev=a2f23d979763

Comment 4

[Sam Penrose]

This patch fixes an FxA bug which will impact Find My Device in 2.0.

Comment 5

[Sam Penrose]

[User impact] User will be logged out of entire device if friend they lend phone to fails a password check. Low occurrence, medium impact.
[Risk] Adds a default parameter. Tested manually and via try server.
[Strings] No string changes.

Comment 6

[bhavana bajaj [:bajaj]]

(In reply to Sam Penrose from comment #5)
> [User impact] User will be logged out of entire device if friend they lend
> phone to fails a password check. Low occurrence, medium impact.
> [Risk] Adds a default parameter. Tested manually and via try server.
> [Strings] No string changes.
Looks like this is a server side issue to us so would not block.

Comment 7

[Sam Penrose]

Bhavana -- It's an FxOS user experience issue that can only be fixed by uplifting this code; I am not sure how it is a server-side issue. This is not the most important uplift issue we face, and I am OK with deciding it is not important enough to uplift, but can you very that we agree that failing to uplift may have client-side impact?

Comment 8

[bhavana bajaj [:bajaj]]

(In reply to Sam Penrose from comment #7)
> Bhavana -- It's an FxOS user experience issue that can only be fixed by
> uplifting this code; I am not sure how it is a server-side issue. This is
> not the most important uplift issue we face, and I am OK with deciding it is
> not important enough to uplift, but can you very that we agree that failing
> to uplift may have client-side impact?

Apologies, may be I mixed up the bugs here :-/ and thanks for clarifying. But I remember considering vishy's opinion on this and he said this might be edge-casey and not something we would block on. Can you seek approval on this once its ready ? , that way this can get uplifted to 2.0 as this looks low risk(Refer guidelines here: https://wiki.mozilla.org/Release_Management/Uplift_rules). I would wait till its landed on central..

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/3632c562fec1

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/3632c562fec1

Comment 11

[Sam Penrose]

Requesting approval, thanks!

(In reply to bhavana bajaj [:bajaj] from comment #8)
> Can you seek approval on
> this once its ready ? , that way this can get uplifted to 2.0 as this looks
> low risk(Refer guidelines here:
> https://wiki.mozilla.org/Release_Management/Uplift_rules). I would wait till
> its landed on central..

Comment 12

[bhavana bajaj [:bajaj]]

(In reply to Sam Penrose from comment #11)
> Requesting approval, thanks!
> 
> (In reply to bhavana bajaj [:bajaj] from comment #8)
> > Can you seek approval on
> > this once its ready ? , that way this can get uplifted to 2.0 as this looks
> > low risk(Refer guidelines here:
> > https://wiki.mozilla.org/Release_Management/Uplift_rules). I would wait till
> > its landed on central..

Hey sam,

Blocking and approval are two different things. Blocking is used on bugs that are regressions, functional issues etc typically anything that you will stop-ship the release for. Whereas approvals can be used on nice-to-have's, polish issues,papercuts etc.. To seek approval, please click on "details" on the patch attachment, set approval-gaia-2.0:? and make sure to answer the attachment questionnaire. Once you do that I will go through the details you've filled and a+ it(after weighing risk/reward) after which sheriff does the uplift for you.

Comment 13

[bhavana bajaj [:bajaj]]

(In reply to bhavana bajaj [:bajaj] from comment #12)
> (In reply to Sam Penrose from comment #11)
> > Requesting approval, thanks!
> > 
> > (In reply to bhavana bajaj [:bajaj] from comment #8)
> > > Can you seek approval on
> > > this once its ready ? , that way this can get uplifted to 2.0 as this looks
> > > low risk(Refer guidelines here:
> > > https://wiki.mozilla.org/Release_Management/Uplift_rules). I would wait till
> > > its landed on central..
> 
> Hey sam,
> 
> Blocking and approval are two different things. Blocking is used on bugs
> that are regressions, functional issues etc typically anything that you will
> stop-ship the release for. Whereas approvals can be used on nice-to-have's,
> polish issues,papercuts etc.. To seek approval, please click on "details" on
> the patch attachment, set approval-gaia-2.0:? and make sure to answer the
or approval-mozilla-aurora in this case as this is a gecko patch :)
> attachment questionnaire. Once you do that I will go through the details
> you've filled and a+ it(after weighing risk/reward) after which sheriff does
> the uplift for you.

Comment 14

[Sam Penrose]

Comment on attachment 8438043 [details] [diff] [review]
1023463-refreshAuth-fail.patch

[Approval Request Comment]
Bug caused by (feature/regressing bug #): 949093 (Comment 0 "A failure of a 3rd party initiated Forced authentication should not log out the user.")
User impact if declined: User will be logged out on failure.
Testing completed (on m-c, etc.): https://tbpl.mozilla.org/?tree=Try&rev=a2f23d979763
Risk to taking this patch (and alternatives if risky): some risk to User Story if there is a lurking error.
String or IDL/UUID changes made by this patch: None

Comment 15

[Lawrence Mandel [:lmandel] (use needinfo)]

Comment on attachment 8438043 [details] [diff] [review]
1023463-refreshAuth-fail.patch

Aurora approval granted.

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f4c13377f1fe