1000395 - refreshAuth and forceAuth should not automatically log the user out

Status: RESOLVED FIXED

(Firefox OS Graveyard :: FxA, defect)

Description

[Sam Penrose]

Right now, in contradiction to the user stories in bug 949093 and bug 949095, services/fxaccounts/FxAccountsManager.jsm:getAssertion() implements refreshAuthentication (and therefore forceAuthentication) by calling signOut() and then open flow. We should change the code to conform to the user story.

Comment 1

[Sam Penrose]

"This looks straightforward," he said, as ominous music swelled in the background ... I think I need to move the call to _uiRequest() to where _signOut() is, and have resolve() and reject() branches, the latter which calls _signOut before rejecting.

My question: can I remove the check of aOptions.silent and corresponding ui-less signOut? It makes no sense to silently fail a refreshAuth, and the .silent parameter is described as a deprecated option for the deprecated get() API in nsDOMIdentity.

Comment 2

[Sam Penrose]

Given that bug 989442 adds silent to the System API, we probably don't want to remove it from the DOM API.

Comment 3

[Sam Penrose]

Attachment: 1000395.refreshAuth-no-signout.patch

The existing code assumes that if we're trying to sign in, we must not be signed in, which of course conflicts rather strongly with the concept of refreshing authentication as defined by our product owners. I did the simplest thing that could possibly work and set a flag to indicate when a refresh was in progress. Let me know what you think.

Comment 4

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

Comment on attachment 8413001 [details] [diff] [review]
1000395.refreshAuth-no-signout.patch

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

Thanks, Sam.  This seems like a good approach for the reasons you mention.  It's minimally invasive and gets the job done.

I think we both agree that these fxa modules are in need of some loving refactoring one of these days.  The signInSignUp method is now one screen tall + one line for me, so it's due for some love.  But I also think it makes sense to tackle all these 2.0 bugs first, making minimal changes, and take a long hard look at the whole thing after 2.0 once all these tricky edge cases are understood.

::: services/fxaccounts/FxAccountsManager.jsm
@@ +390,5 @@
> +            // Forcing refreshAuth to silent is a contradiction in terms,
> +            // though it will sometimes succeed silently.
> +            if (aOptions.silent) {
> +              return this._error(ERROR_NO_SILENT_REFRESH_AUTH);
> +            }

Good catch.

@@ +395,1 @@
>              if ((Date.now() / 1000) - this._activeSession.authAt > gracePeriod) {

I know this isn't part of your patch, but it's confusing that we have some temporal values in milliseconds and some in seconds.  In some separate bug, I'd propose we at least add a suffix to all these variables indicating whether they're Msec or Sec.

@@ +405,5 @@
> +                },
> +                (reason) => {
> +                  return this._signOut().then(
> +                    () => {
> +                      this._refreshing = false;

Can this._signOut fail?  If so, perhaps 'this._refreshing = false' ought to go ahead of the call to _signOut.  Alternately, as this promise sets _refreshing to false in either case, maybe _uiRequest could set _refreshing = false?

Comment 5

[Sam Penrose]

Attachment: 1000395.refreshAuth-no-signout.patch

Thanks for catching the flag placement issue. I looked at moving it to _uiRequest, but I couldn't see a clearly better place to put it. The current approach at least keeps setting and unsetting close to each other. Let me know what you think, and thanks as always for your time.

Comment 6

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

Comment on attachment 8413841 [details] [diff] [review]
1000395.refreshAuth-no-signout.patch

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

Looks good.  Thanks, Sam!

::: services/fxaccounts/FxAccountsManager.jsm
@@ +381,5 @@
>            }
>  
>            // RPs might require an authentication refresh.
>            if (aOptions &&
> +              typeof(aOptions.refreshAuthentication) !== "undefined") {

slight inconsistency that you're using a "not identical" test here and a "not string-equal" below (for "number"), though it doesn't matter in this case.

@@ +403,5 @@
> +                  this._refreshing = false;
> +                  return assertion;
> +                },
> +                (reason) => {
> +                  this._refreshing = false;

Thanks for trying to find a single home for the _refreshing flag reset to go in _uiRequest.  This is fine with me if there's no good place for it there.

Comment 7

[Sam Penrose]

Attachment: 1000395.refreshAuth-no-signout.patch

Inconsistent use of typeof fixed -- thanks Jed.

Comment 8

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/b2g-inbound/rev/908cc41657a3

Comment 9

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/908cc41657a3