fix(signin): add oauth query strings to sign in and sign up views

[vladikoff]

Fixes #4547

@shane-tomlinson thoughts on this approach?

[vladikoff]

the use-different is a messed up case, should we convert this to a button instead of a link, so it cannot be opened in a new tab, or we add /{{queryString}}&useDifferent=1 ?

[shane-tomlinson]

use-different re-renders the signin page, so we should change the href to be /signin with the query string. We already have equivalent functionality to useDifferent=1, it's email=blank - see

fxa-content-server/app/scripts/models/reliers/relier.js

Line 38 in 9cbf5d9

email: Vat.email().allow(Constants.DISALLOW_CACHED_CREDENTIALS),

and

fxa-content-server/app/scripts/lib/constants.js

Line 85 in 9cbf5d9

DISALLOW_CACHED_CREDENTIALS: 'blank',

, no need for a new query parameter.

[shane-tomlinson]

Can you also add tests to ensure the queryString makes it into the templates where expected?

[vladikoff]

the problem is useDifferent currently does:

      this.user.removeAllAccounts();
      Session.clear();
      this._formPrefill.clear();
      this.logViewEvent('use-different-account');

So we either need to add another query param or do the same with email=blank

[shane-tomlinson]

I don't think email=blank needs to do anything extra, "email=blank" is from a user's P.O.V. equivalent to "Use another account"; the sign_in view will see target=blank and ignore the cached credentials.

As an aside, I have no problems re-visiting this code and why it's being done. I have a lot of grumbles about this section of code and cached accounts in general, it's so complex and hard to think about. First, it seems to me we should do away with storing multiple accounts locally. It's complicated. It's yucky. Nobody fully understands its behavior. Next, if we must store multiple accounts, getChooserAccount seems like it's misplaced - what does the model care about what account should be displayed to the user? That seems like it should be in the signin view. Next, if we must store multiple accounts and the the user chooses Use a different account, clearing all accounts seems equally erroneous. It seems we should keep all the account data and either not call getChooseAccount or pass in an override that returns an empty user. None of this for this PR of course.

[philbooth]

...we should do away with storing multiple accounts locally.

This, this, a thousand times this.

[vladikoff]

After talking to Shane, we decided to leave use-different and add email=blank in this PR.
We can work on removing the removeAllAcounts and other jazz in followup #4635

[vladikoff]

Also Fixes #4400

[shane-tomlinson]

This PR also brings up how we should handle the "back" link which always has an href of "#", which is also not new tab friendly.

[shane-tomlinson]

We call Query Search in other functions, can you use it here too? And remove the Current. You might as well just move this function to views/base.js with the others so it's available everywhere.

Pedantic as it is, this function also needs tests wherever it lands.

[shane-tomlinson]

If we call them Search parameters elsewhere, should we call it a searchString here and in the templates?

[shane-tomlinson]

Can you also add tests to ensure the queryString makes it into the templates where expected?

[vladikoff]

@shane-tomlinson the PR should be good to go except of this ^ I keep getting logger.js [sm]:73 String contains variables that are not escaped: Looking for your Add-ons data? <a href="%(signUpWithSearchString)s">Sign up</a> for a Firefox Account with your old Add-ons account email address.

[shane-tomlinson]

Two comments about this line.

1. In the auth-mailer, anchors that must be translated don't even include the href, we use the form of %(linkNameAttribute)s - see https://github.com/mozilla/fxa-auth-mailer/blob/d0cf24960cbc19278f42679da42842c822a20862/templates/verify_login.html#L65. Might be nice to keep a similar convention here where even the href portion is passed in.
2. For unsafeTranslate, all variables must be prefixed with escaped as a reminder to the reviewer to look for escaping. This is to keep us from unintentionally introducing XSS vulnerabilities via HTML. See

   fxa-content-server/app/scripts/views/base.js

   Line 394 in af6cdd7

   if (Strings.hasUnsafeVariables(text)) {

   .

[vladikoff]

this is not used anymore AFAIK

made changes

[vladikoff]

Circle CI fail due to Docker, cc @jbuck https://circleci.com/gh/mozilla/fxa-content-server/5875?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

[shane-tomlinson]

@vladikoff - While I was looking at this PR, I noticed the service-mixin and realized we can use that to do all the extra stuff we need it to, with only minimal updates to the templates.

Here's what I came up with:

diff --git a/app/scripts/models/auth_brokers/oauth.js b/app/scripts/models/auth_brokers/oauth.js
index 5e40ec3d..19697cb3 100644
--- a/app/scripts/models/auth_brokers/oauth.js
+++ b/app/scripts/models/auth_brokers/oauth.js
@@ -177,7 +177,12 @@ define(function (require, exports, module) {
         link = '/' + link;
       }
 
-      return '/oauth' + link;
+      if (/^\/(signin|signup)/.test(link)) {
+        link = '/oauth' + link;
+      }
+
+      const windowSearchParams = Url.searchParams(this.window.location.search);
+      return Url.updateSearchString(link, windowSearchParams);
     }
   });
 
diff --git a/app/scripts/templates/sign_in.mustache b/app/scripts/templates/sign_in.mustache
index b26b34d0..58ca7283 100644
--- a/app/scripts/templates/sign_in.mustache
+++ b/app/scripts/templates/sign_in.mustache
@@ -46,7 +46,7 @@
 
            <div class="links">
              <a href="/reset_password" class="left reset-password" data-flow-event="forgot-password">{{#t}}Forgot password?{{/t}}</a>
-             <a href="/#" class="right use-different" data-flow-event="use-different-account">{{#t}}Use a different account{{/t}}</a><br/>
+             <a href="/signin?email=blank" class="right use-different" data-flow-event="use-different-account">{{#t}}Use a different account{{/t}}</a><br/>
            </div>
         {{/chooserAskForPassword}}
 
@@ -56,7 +56,7 @@
           </div>
 
           <div class="links">
-            <a href="/#" class="use-different" data-flow-event="use-different-account">{{#t}}Use a different account{{/t}}</a><br/>
+            <a href="/signin?email=blank" class="use-different" data-flow-event="use-different-account">{{#t}}Use a different account{{/t}}</a><br/>
           </div>
         {{/chooserAskForPassword}}
       {{/suggestedAccount}}
diff --git a/app/scripts/views/mixins/service-mixin.js b/app/scripts/views/mixins/service-mixin.js
index 591472d7..56a0c128 100644
--- a/app/scripts/views/mixins/service-mixin.js
+++ b/app/scripts/views/mixins/service-mixin.js
@@ -9,13 +9,15 @@ define(function (require, exports, module) {
   'use strict';
 
   const BaseView = require('views/base');
+  const $ = require('jquery');
 
   module.exports = {
     transformLinks () {
-      this.$('a[href~="/signin"]').attr('href',
-          this.broker.transformLink('/signin'));
-      this.$('a[href~="/signup"]').attr('href',
-          this.broker.transformLink('/signup'));
+      const $linkEls = this.$('a[href^="/signin"],a[href^="/signup"],a[href^="/reset_password"]');
+      $linkEls.each((index, el) => {
+        const $linkEl = $(el);
+        $linkEl.attr('href', this.broker.transformLink($linkEl.attr('href')));
+      });
     },
 
     // override this method so we can fix signup/signin links in errors

[vladikoff]

wat

[shane-tomlinson]

wat indeed.

[shane-tomlinson]

Other than the one small request to remove base.js->getSearchString, looks great!

[shane-tomlinson]

Two comments about this line.

1. In the auth-mailer, anchors that must be translated don't even include the href, we use the form of %(linkNameAttribute)s - see https://github.com/mozilla/fxa-auth-mailer/blob/d0cf24960cbc19278f42679da42842c822a20862/templates/verify_login.html#L65. Might be nice to keep a similar convention here where even the href portion is passed in.
2. For unsafeTranslate, all variables must be prefixed with escaped as a reminder to the reviewer to look for escaping. This is to keep us from unintentionally introducing XSS vulnerabilities via HTML. See

   fxa-content-server/app/scripts/views/base.js

   Line 394 in af6cdd7

   if (Strings.hasUnsafeVariables(text)) {

   .

[shane-tomlinson]

wat indeed.

[shane-tomlinson]

I don't see this function referenced anymore.