Closed Bug 938635 Opened 9 years ago Closed 9 years ago

Implement forceAuthentication for firefox accounts

Categories

(Core Graveyard :: Identity, defect)

All
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla30

People

(Reporter: jedp, Assigned: ferjm)

References

Details

(Whiteboard: [qa-])

Attachments

(5 files, 6 obsolete files)

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.
Depends on: 938741
Whiteboard: [qa-]
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.
Attached file FxA_FFOS_Force_Authentication_V.1.pdf (obsolete) —
First pass at high fidelity mocks for forced authentication with FxA. Needs user stories to match UX.
Attachment #8334090 - Attachment is obsolete: true
Blocks: 949093
Depends on: 952347
No longer blocks: 929386
Depends on: 929386
Blocks: 945278
Assignee: nobody → ferjmoreno
No longer blocks: 945278
Depends on: 945278
Blocks: 949097
Depends on: 943521
Depends on: 967431
Depends on: 967503
Attached patch Part 0: Mock client (obsolete) — Splinter Review
This mock is required until bug 943521 and bug 938741 are done.
Attached patch Part 1: DOM API (WIP) (obsolete) — Splinter Review
Attached patch Part 3: UI Glue (WIP) (obsolete) — Splinter Review
Depends on: 968294
Depends on: 970434
Attachment #8370842 - Attachment is obsolete: true
Attached patch Part 1: DOM APISplinter Review
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)
Attached patch Part 2: FxAccountsManager (obsolete) — Splinter Review
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)
Attached patch Part 3: UI GlueSplinter Review
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+
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+
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
Closed: 9 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Product: Core → Core Graveyard
You need to log in before you can comment on or make changes to this bug.