962849 - FxAccounts.jsm - add a method to re-send verification email

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

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

Something like resendVerificationEmail()

Comment 1

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

The endpoint to call is resend_code

https://github.com/mozilla/fxa-auth-server/blob/master/docs/api.md#api-endpoints

Comment 2

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

Attachment: resendVerificationEmail()

Comment 3

[Tim Taubert [:ttaubert] (inactive)]

Comment on attachment 8364721 [details] [diff] [review]
resendVerificationEmail()

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

::: services/fxaccounts/FxAccounts.jsm
@@ +353,5 @@
> +   *         fulfilled: xhr request to fxa server
> +   *         rejected: error
> +   *
> +   * Note that this function does not initiate polling for email verification.
> +   * The public resendVerificationEmail() below does this.

Does it? Looks like this the public function calls this internal one, and this one does start polling.

@@ +357,5 @@
> +   * The public resendVerificationEmail() below does this.
> +   */
> +  resendVerificationEmail: function(data) {
> +    this.abortExistingFlow();
> +    this.pollEmailStatus(data.sessionToken, "start");

Do we need to abort the existing flow? Do we need to restart polling? Couldn't we just keep polling until the user is verified?

::: services/fxaccounts/FxAccountsClient.jsm
@@ +120,5 @@
>      return this._request("/recovery_email/status", "GET",
>        this._deriveHawkCredentials(sessionTokenHex, "sessionToken"));
>    },
>  
> +  /** 

Tiny nit: trailing space

::: services/fxaccounts/tests/xpcshell/test_accounts.js
@@ +63,5 @@
>      });
>      return deferred.promise;
>    };
>  
> +  this.resendVerificationEmail = function(keyFetchToken) {

Nit: although unused, sessionToken?

@@ +66,5 @@
>  
> +  this.resendVerificationEmail = function(keyFetchToken) {
> +    let deferred = Promise.defer();
> +    deferred.resolve("OK");
> +    return deferred.promise;

Nit: return Promise.resolve("OK");

@@ +225,5 @@
>      fxa.internal.getUserAccountData().then(user => {
>        // The user is signing in, but email has not been verified yet
>        do_check_eq(user.verified, false);
>        do_timeout(200, function() {
> +        log.debug("Mocking verification of francine's email"); 

Tiny nit: trailing space.

Comment 4

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

Attachment: resendVerificationEmail()

Addresses feedback from ttaubert - thank you, sir!

Comment 5

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

(In reply to Tim Taubert [:ttaubert] from comment #3)
> Comment on attachment 8364721 [details] [diff] [review]
> resendVerificationEmail()

Thank you for your unsolicited feedback, ttaubert!  Very helpful.

> Review of attachment 8364721 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: services/fxaccounts/FxAccounts.jsm
> @@ +353,5 @@
> > +   *         fulfilled: xhr request to fxa server
> > +   *         rejected: error
> > +   *
> > +   * Note that this function does not initiate polling for email verification.
> > +   * The public resendVerificationEmail() below does this.
> 
> Does it? Looks like this the public function calls this internal one, and
> this one does start polling.

Quite right; I had changed the code and neglected to update the comment, so it was out of sync with reality.

And I agree, we don't need to cancel the current flow.  I think we just need to restart the clock from the start, to give the user time to respond (in case we're, say, one second from giving up).

I've updated the code

> @@ +357,5 @@
> > +   * The public resendVerificationEmail() below does this.
> > +   */
> > +  resendVerificationEmail: function(data) {
> > +    this.abortExistingFlow();
> > +    this.pollEmailStatus(data.sessionToken, "start");
> 
> Do we need to abort the existing flow? Do we need to restart polling?
> Couldn't we just keep polling until the user is verified?

Addressed as above

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +120,5 @@
> >      return this._request("/recovery_email/status", "GET",
> >        this._deriveHawkCredentials(sessionTokenHex, "sessionToken"));
> >    },
> >  
> > +  /** 
> 
> Tiny nit: trailing space

Oops.  Thanks.

> ::: services/fxaccounts/tests/xpcshell/test_accounts.js
> @@ +63,5 @@
> >      });
> >      return deferred.promise;
> >    };
> >  
> > +  this.resendVerificationEmail = function(keyFetchToken) {
> 
> Nit: although unused, sessionToken?

Quite so.  And I've updated the test to fulfill the promise with the value of the session token, to ensure that the right value is sent to the client from FxAccounts.jsm.

> @@ +66,5 @@
> >  
> > +  this.resendVerificationEmail = function(keyFetchToken) {
> > +    let deferred = Promise.defer();
> > +    deferred.resolve("OK");
> > +    return deferred.promise;
> 
> Nit: return Promise.resolve("OK");

Yes indeed.  And updated as described above.

> @@ +225,5 @@
> >      fxa.internal.getUserAccountData().then(user => {
> >        // The user is signing in, but email has not been verified yet
> >        do_check_eq(user.verified, false);
> >        do_timeout(200, function() {
> > +        log.debug("Mocking verification of francine's email"); 
> 
> Tiny nit: trailing space.

Oops.  Fixed.

Thank you again for your feedback,
j

Comment 6

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

Comment on attachment 8364756 [details] [diff] [review]
resendVerificationEmail()

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

Looks OK, except for my concern about the possibility of polling twice in edge cases.

::: services/fxaccounts/FxAccounts.jsm
@@ +349,5 @@
> +  /**
> +   * Resend the verification email to the logged-in user.
> +   *
> +   * @return Promise
> +   *         fulfilled: xhr request to fxa server

If I'm reading the code properly, it looks like it will be resolved with the JSON response from the server rather than the XHR object itself?

@@ +354,5 @@
> +   *         rejected: error
> +   */
> +  resendVerificationEmail: function(data) {
> +    if (this.pollTimeRemaining) {
> +      // If we're already polling, reset the clock to the beginning

it looks like pollEmailStatus is already handling this case, so just call that?

@@ +383,5 @@
>      if (why == "start") {
> +      // If we were already polling, stop and start again.  This could happen
> +      // if the user requested the verification email to be resent while we
> +      // were already polling for receipt of an earlier email.
> +      //this.abortExistingFlow();

nuke the commented lines - but don't we need to ensure there is a whenVerifiedPromise setup.  Eg, if this causes us to start polling when there is no whenVerifiedPromise, then someone else calls .whenVerified() it appears we will start polling a second time?

::: services/fxaccounts/FxAccountsClient.jsm
@@ +120,5 @@
>      return this._request("/recovery_email/status", "GET",
>        this._deriveHawkCredentials(sessionTokenHex, "sessionToken"));
>    },
>  
> +  /** 

trailing space

Comment 7

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

Attachment: resendVerificationEmail()

Addresses questions in comment 6

Comment 8

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

(In reply to Mark Hammond [:markh] from comment #6)
> Comment on attachment 8364756 [details] [diff] [review]
> resendVerificationEmail()

Thanks for your comments, Mark.

> Review of attachment 8364756 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks OK, except for my concern about the possibility of polling twice in
> edge cases.

Thank you for catching this.

> ::: services/fxaccounts/FxAccounts.jsm
> @@ +349,5 @@
> > +  /**
> > +   * Resend the verification email to the logged-in user.
> > +   *
> > +   * @return Promise
> > +   *         fulfilled: xhr request to fxa server
> 
> If I'm reading the code properly, it looks like it will be resolved with the
> JSON response from the server rather than the XHR object itself?

So correct.  Thank you.  Fixed.  (I must say what a pleasure it is to receive comments with such a close reading.)

> @@ +354,5 @@
> > +   *         rejected: error
> > +   */
> > +  resendVerificationEmail: function(data) {
> > +    if (this.pollTimeRemaining) {
> > +      // If we're already polling, reset the clock to the beginning
> 
> it looks like pollEmailStatus is already handling this case, so just call
> that?

Good point.  Updated.

> @@ +383,5 @@
> >      if (why == "start") {
> > +      // If we were already polling, stop and start again.  This could happen
> > +      // if the user requested the verification email to be resent while we
> > +      // were already polling for receipt of an earlier email.
> > +      //this.abortExistingFlow();
> 
> nuke the commented lines - but don't we need to ensure there is a
> whenVerifiedPromise setup.  Eg, if this causes us to start polling when
> there is no whenVerifiedPromise, then someone else calls .whenVerified() it
> appears we will start polling a second time?

That was an excellent catch, thank you.  I believe I've fixed it now.

> ::: services/fxaccounts/FxAccountsClient.jsm
> @@ +120,5 @@
> >      return this._request("/recovery_email/status", "GET",
> >        this._deriveHawkCredentials(sessionTokenHex, "sessionToken"));
> >    },
> >  
> > +  /** 
> 
> trailing space

Sorry.  Thanks.

Comment 9

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

Comment on attachment 8364786 [details] [diff] [review]
resendVerificationEmail()

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

LGTM

Comment 10

[Tim Taubert [:ttaubert] (inactive)]

https://hg.mozilla.org/integration/fx-team/rev/ed8dc28b3ebc

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ed8dc28b3ebc