938635 - Implement forceAuthentication for firefox accounts

Status: RESOLVED FIXED

(Core Graveyard :: Identity, defect)

Description

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

Marketplace depends on the experimental_forceIssuer flag in persona to force users to sign in again.  We need to support this in Firefox Accounts so we don't regress this feature on marketplace.

Comment 1

[John Gruen [:jgruen]]

Attachment: ForceAuthentication.pdf

Comment 2

[John Gruen [:jgruen]]

Adding flows for Force Auth in Marketplace and WMF, basic idea:

-some user action triggers force auth
-user gets three tries to enter password in Trusted UI (with plain text password option)
-ui in trusted auth changes from submit to reset password
-reset takes user through a standard FxA flow with a minor string modification on the first screen

a few security questions/points: 
1. Is user signed out of FxA on Force Auth, or are they logged out when they hit the password reset flow after 3 failed attempts? The latter seems more accommodating for user error. 
2. If user is signed out of FxA, their WMF status must remain in its current state, a forced sign out event must not disable WMF if the user has opted to turn it on; rather, forced sign out should lock the status of WMF into its enabled state.

Comment 3

[John Gruen [:jgruen]]

Attachment: FxA_FFOS_Force_Authentication_V.1.pdf

First pass at high fidelity mocks for forced authentication with FxA. Needs user stories to match UX.

Comment 4

[John Gruen [:jgruen]]

Attachment: FxA_FFOS_Force_Authentication_v.1.pdf

Comment 5

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

This is a high priority for sync: https://github.com/mozilla/fxa-auth-server/issues/307

Comment 6

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 0: Mock client

This mock is required until bug 943521 and bug 938741 are done.

Comment 7

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API (WIP)

Comment 8

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: FxAccountsManager (WIP)

Comment 9

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: UI Glue (WIP)

Comment 10

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 1: DOM API

Jed, with this patch we simply forward the request options to FxAccountsManager.getAssertion.
I've tested this with a simple tweak added to the UI tests at bug 970438

Comment 11

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: FxAccountsManager

Mark, I still need to write some tests for this, but I would really appreciate some feedback in the mean time.

With this patch FxAccountsManager gets the "refreshAuthentication" parameter from the mozId.request() call, checks if the given grace period has expired and in that case it does a sign out and requests the UI to ask the user for his credentials. If the user successfully logs in again, an assertion is returned. Otherwise, the error given by the UI is returned. You can find the details of what is the UI doing at bug 968294.

Comment 12

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 3: UI Glue

Fabrice, this patch adds the refreshAuthentication method to FxAccountsUIGlue to request Gaia to show the authentication UI for a given account ID when needed.

Comment 13

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

Comment on attachment 8376239 [details] [diff] [review]
Part 1: DOM API

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

LGTM! thanks, Fernando

Comment 14

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8376257 [details] [diff] [review]
Part 2: FxAccountsManager

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

This looks fine to me (and I look forward to when desktop and reuse some of this)

Comment 15

[Fernando Jiménez Moreno [:ferjm]]

Attachment: Part 2: FxAccountsManager

Thanks Mark! Now with unit tests

Comment 16

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8377170 [details] [diff] [review]
Part 2: FxAccountsManager

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

LGTM

::: services/fxaccounts/FxAccounts.jsm
@@ +171,5 @@
>     *          sessionToken: Session for the FxA server
>     *          kA: An encryption key from the FxA server
>     *          kB: An encryption key derived from the user's FxA password
>     *          verified: email verification status
> +   *          authAt: authentication time for the session (seconds since epoch)

I think these comments should be reworded slightly - something like "The time (seconds since epoch) that this record was authenticated" (assuming that's what it means :)

::: services/fxaccounts/FxAccountsManager.jsm
@@ +383,5 @@
>              }
> +
> +            if ((Date.now() / 1000) - this._activeSession.authAt > gracePeriod) {
> +              // Grace period expired, so we sign out and request the user to
> +              // authenticated herself again. If the autentication, we will

Few typos in this line - "authenticate" - no trailing "d", "autentication" is spelt wrong, and I think a word is missing - eg "If the authentication succeeds, we..."

::: services/fxaccounts/tests/xpcshell/test_manager.js
@@ +405,2 @@
>  add_test(function(test_getAccount_existing_verified_session) {
> +  do_print("= Test 10 | getAccount, existing verified session =");

I don't think these test numbers add any value, get out of date (as we have seen), and means many more lines need to be touched if someone wants to add a new test in the middle.  I'd say that instead of touching all these lines just to change the number, remove them instead!

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Thanks Jed, Mark and Fabrice for the reviews.

https://hg.mozilla.org/integration/b2g-inbound/rev/fb1e20021f2d
https://hg.mozilla.org/integration/b2g-inbound/rev/a8cdafcb11f0
https://hg.mozilla.org/integration/b2g-inbound/rev/2cf9d97e2684

Comment 18

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/fb1e20021f2d
https://hg.mozilla.org/mozilla-central/rev/a8cdafcb11f0
https://hg.mozilla.org/mozilla-central/rev/2cf9d97e2684