Implement forceAuthentication for firefox accounts

RESOLVED FIXED in mozilla30

Status

()

Core
Identity
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: jedp, Assigned: ferjm)

Tracking

(Depends on: 1 bug)

unspecified
mozilla30
All
Gonk (Firefox OS)
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(5 attachments, 6 obsolete attachments)

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.

Updated

4 years ago
Depends on: 938741
Whiteboard: [qa-]

Comment 1

4 years ago
Created attachment 8333860 [details]
ForceAuthentication.pdf

Comment 2

4 years ago
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

4 years ago
Created attachment 8334090 [details]
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

4 years ago
Created attachment 8334096 [details]
FxA_FFOS_Force_Authentication_v.1.pdf

Updated

4 years ago
Attachment #8334090 - Attachment is obsolete: true

Updated

4 years ago
Blocks: 949093

Updated

4 years ago
Depends on: 952347
(Assignee)

Updated

4 years ago
No longer blocks: 929386
Depends on: 929386
(Assignee)

Updated

4 years ago
Blocks: 945278
(Assignee)

Updated

4 years ago
Assignee: nobody → ferjmoreno
(Assignee)

Updated

4 years ago
No longer blocks: 945278
Depends on: 945278
(Assignee)

Updated

4 years ago
Blocks: 949097
(Assignee)

Updated

4 years ago
Depends on: 943521
(Assignee)

Updated

4 years ago
Depends on: 967431
(Assignee)

Updated

4 years ago
Depends on: 967503
(Assignee)

Comment 6

4 years ago
Created attachment 8370842 [details] [diff] [review]
Part 0: Mock client

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

Comment 7

4 years ago
Created attachment 8370843 [details] [diff] [review]
Part 1: DOM API (WIP)
(Assignee)

Comment 8

4 years ago
Created attachment 8370844 [details] [diff] [review]
Part 2: FxAccountsManager (WIP)
(Assignee)

Comment 9

4 years ago
Created attachment 8370845 [details] [diff] [review]
Part 3: UI Glue (WIP)
(Assignee)

Updated

4 years ago
Depends on: 968294
(Assignee)

Updated

4 years ago
Depends on: 970434
(Assignee)

Updated

4 years ago
Attachment #8370842 - Attachment is obsolete: true
(Assignee)

Comment 10

4 years ago
Created attachment 8376239 [details] [diff] [review]
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
Attachment #8370843 - Attachment is obsolete: true
Attachment #8376239 - Flags: review?(jparsons)
(Assignee)

Comment 11

4 years ago
Created attachment 8376257 [details] [diff] [review]
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.
Attachment #8370844 - Attachment is obsolete: true
Attachment #8376257 - Flags: feedback?(mhammond)
(Assignee)

Comment 12

4 years ago
Created attachment 8376259 [details] [diff] [review]
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.
Attachment #8370845 - Attachment is obsolete: true
Attachment #8376259 - Flags: review?(fabrice)
Comment on attachment 8376239 [details] [diff] [review]
Part 1: DOM API

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

LGTM! thanks, Fernando
Attachment #8376239 - Flags: review?(jparsons) → review+
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)
Attachment #8376257 - Flags: feedback?(mhammond) → feedback+
(Assignee)

Comment 15

4 years ago
Created attachment 8377170 [details] [diff] [review]
Part 2: FxAccountsManager

Thanks Mark! Now with unit tests
Attachment #8376257 - Attachment is obsolete: true
Attachment #8377170 - Flags: review?(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!
Attachment #8377170 - Flags: review?(mhammond) → review+
Attachment #8376259 - Flags: review?(fabrice) → review+

Updated

4 years ago
Blocks: 955951, 955952
https://hg.mozilla.org/mozilla-central/rev/fb1e20021f2d
https://hg.mozilla.org/mozilla-central/rev/a8cdafcb11f0
https://hg.mozilla.org/mozilla-central/rev/2cf9d97e2684
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
You need to log in before you can comment on or make changes to this bug.