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

RESOLVED FIXED in Firefox 32

Status

defect
P2
normal
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: njpark, Assigned: spenrose)

Tracking

unspecified
2.0 S4 (20june)
ARM
Gonk (Firefox OS)
Dependency tree / graph

Firefox Tracking Flags

(blocking-b2g:-, firefox31 wontfix, firefox32 fixed, firefox33 fixed, b2g-v2.0 fixed, b2g-v2.1 fixed)

Details

(Whiteboard: [fxa4fxos2.0])

Attachments

(1 attachment, 1 obsolete attachment)

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
Assignee: nobody → spenrose
Status: NEW → ASSIGNED
Priority: -- → P2
Whiteboard: [fxa4fxos2.0]
Summary: [FxA] Failing refresh authentication causes user to login again → [FxA] leave user logged in after failed RP refresh authentication
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.
Attachment #8438009 - Flags: review?(jparsons)
Blocks: 1000323
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)
Attachment #8438009 - Flags: review?(jparsons) → review+
Thanks Jed! Default parameter added.

https://tbpl.mozilla.org/?tree=Try&rev=a2f23d979763
Attachment #8438009 - Attachment is obsolete: true
This patch fixes an FxA bug which will impact Find My Device in 2.0.
blocking-b2g: --- → 2.0?
Keywords: checkin-needed
[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.
(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.
blocking-b2g: 2.0? → -
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?
Flags: needinfo?(bbajaj)
(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..
Flags: needinfo?(bbajaj)
https://hg.mozilla.org/mozilla-central/rev/3632c562fec1
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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..
blocking-b2g: - → 2.0?
(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.
blocking-b2g: 2.0? → -
(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 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
Attachment #8438043 - Flags: approval-mozilla-aurora?
Comment on attachment 8438043 [details] [diff] [review]
1023463-refreshAuth-fail.patch

Aurora approval granted.
Attachment #8438043 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.