From 6540a9468a8bce3f496423179db1862cfb9f5c8c Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Tue, 22 May 2018 15:04:19 -0500
Subject: Preserve URL fragment across sign-in and sign-up redirects If
 window.location contains a URL fragment, append the fragment to all sign-in
 forms, the sign-up form, and all button based providers.

---
 app/controllers/omniauth_callbacks_controller.rb   | 16 ++++++++++
 app/views/devise/sessions/new.html.haml            | 30 ++++++++++++++++++-
 .../omniauth_callbacks_controller_spec.rb          | 34 ++++++++++++++++++++++
 3 files changed, 79 insertions(+), 1 deletion(-)

diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index 30be50d4595..12f11976439 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -75,6 +75,12 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   private
 
   def omniauth_flow(auth_module, identity_linker: nil)
+    omniauth_params = request.env['omniauth.params']
+
+    if omniauth_params.present? && omniauth_params['redirect_fragment'].present?
+      store_redirect_fragment(omniauth_params['redirect_fragment'])
+    end
+
     if current_user
       log_audit_event(current_user, with: oauth['provider'])
 
@@ -189,4 +195,14 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     request_params = request.env['omniauth.params']
     (request_params['remember_me'] == '1') if request_params.present?
   end
+
+  def store_redirect_fragment(redirect_fragment)
+    key = stored_location_key_for(:user)
+    location = session[key]
+    unless location.to_s.strip.empty?
+      uri = URI.parse(location)
+      uri.fragment = redirect_fragment
+      store_location_for(:user, uri.to_s)
+    end
+  end
 end
diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml
index 34d4293bd45..3840cbb0b31 100644
--- a/app/views/devise/sessions/new.html.haml
+++ b/app/views/devise/sessions/new.html.haml
@@ -1,6 +1,34 @@
 - page_title "Sign in"
 
-%div
+- content_for :page_specific_javascripts do
+  -# haml-lint:disable InlineJavaScript
+  :javascript
+    document.addEventListener('DOMContentLoaded', function() {
+      // Determine if the current URL location contains a fragment identifier (aka hash or anchor ref).
+      // This should be present if the user was redirected to sign in after attempting to access a
+      // protected URL that included a fragment identifier.
+      var fragment = window.location.hash;
+      if (fragment && fragment !== '') {
+        // Append the fragment to all signin/signup form actions so it is preserved when
+        // the user is eventually redirected back to the originally requested URL.
+        $('div#signin-container form').attr('action', function (index, action) {
+          return action + fragment;
+        });
+
+        // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment
+        // query param will be passed to the omniauth callback upon successful authentication
+        $('div#signin-container a.oauth-login').attr('href', function (index, href) {
+          const tokens = href.split('?');
+          let url = tokens[0] + '?redirect_fragment=' + encodeURIComponent(fragment.substr(1));
+          if (tokens.length > 1) {
+            url += '&' + tokens[1];
+          }
+          return url;
+        });
+      }
+    });
+
+#signin-container
   - if form_based_providers.any?
     = render 'devise/shared/tabs_ldap'
   - else
diff --git a/spec/controllers/omniauth_callbacks_controller_spec.rb b/spec/controllers/omniauth_callbacks_controller_spec.rb
index d377d69457f..21936491ffc 100644
--- a/spec/controllers/omniauth_callbacks_controller_spec.rb
+++ b/spec/controllers/omniauth_callbacks_controller_spec.rb
@@ -45,6 +45,40 @@ describe OmniauthCallbacksController, type: :controller do
       end
     end
 
+    context 'when a redirect fragment is provided' do
+      let(:provider) { :jwt }
+      let(:extern_uid) { 'my-uid' }
+
+      before do
+        request.env['omniauth.params'] = { 'redirect_fragment' => 'L101' }
+      end
+
+      context 'when a redirect url is stored' do
+        it 'redirects with fragment' do
+          post provider, nil, { user_return_to: '/fake/url' }
+
+          expect(response).to redirect_to('/fake/url#L101')
+        end
+      end
+
+      context 'when a redirect url with a fragment is stored' do
+        it 'redirects with the new fragment' do
+          post provider, nil, { user_return_to: '/fake/url#replaceme' }
+
+          expect(response).to redirect_to('/fake/url#L101')
+        end
+      end
+
+      context 'when no redirect url is stored' do
+        it 'does not redirect with the fragment' do
+          post provider
+
+          expect(response.redirect?).to be true
+          expect(response.location).not_to include('#L101')
+        end
+      end
+    end
+
     context 'strategies' do
       context 'github' do
         let(:extern_uid) { 'my-uid' }
-- 
cgit v1.2.3


From 4dcaa4df3622ae267363fcff184d0929b2102035 Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Mon, 4 Jun 2018 16:28:18 -0500
Subject: Addressing peer review feedback. Replacing inline JS with ES 2015
 functions included in pages/sessions/new. Also applying suggested server-side
 syntax improvements to OmniAuthCallbacksController.

---
 app/assets/javascripts/lib/utils/common_utils.js   | 81 ++++++++++++++++++++++
 app/assets/javascripts/pages/sessions/new/index.js |  5 ++
 .../pages/sessions/new/preserve_url_fragment.js    | 29 ++++++++
 app/controllers/omniauth_callbacks_controller.rb   |  9 +--
 app/views/devise/sessions/new.html.haml            | 28 --------
 .../fixtures/signin_forms_and_buttons.html.haml    | 21 ++++++
 spec/javascripts/lib/utils/common_utils_spec.js    | 61 ++++++++++++++++
 .../sessions/new/preserve_url_fragment_spec.js     | 26 +++++++
 8 files changed, 226 insertions(+), 34 deletions(-)
 create mode 100644 app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
 create mode 100644 spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
 create mode 100644 spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js

diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index fc34d243dd7..ccf1d924ef2 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -180,6 +180,87 @@ export const urlParamsToObject = (path = '') =>
     return data;
   }, {});
 
+/**
+ * Apply the query param and value to the given url by returning a new url string that includes
+ * the param/value pair. If the given url already includes the query param, the query param value
+ * will be updated in the new url string. Otherwise, the query param and value will by added in
+ * the new url string.
+ *
+ * @param url {string} - url to which the query param will be applied
+ * @param param {string} - name of the query param to set
+ * @param value {string|number} - value to give the query param
+ * @returns {string} A copy of the original url with the new or updated query param
+ */
+export const setUrlParam = (url, param, value) => {
+  const [rootAndQuery, fragment] = url.split('#');
+  const [root, query] = rootAndQuery.split('?');
+  const encodedParam = encodeURIComponent(param);
+  const encodedPair = `${encodedParam}=${encodeURIComponent(value)}`;
+
+  let paramExists = false;
+  const paramArray =
+    (query ? query.split('&') : [])
+    .map(paramPair => {
+      const [foundParam] = paramPair.split('=');
+      if (foundParam === encodedParam) {
+        paramExists = true;
+        return encodedPair;
+      }
+      return paramPair;
+    });
+
+  if (paramExists === false) {
+    paramArray.push(encodedPair);
+  }
+
+  const writableFragment = fragment ? `#${fragment}` : '';
+  return `${root}?${paramArray.join('&')}${writableFragment}`;
+};
+
+/**
+ * Remove the query param from the given url by returning a new url string that no longer includes
+ * the param/value pair.
+ *
+ * @param url {string} - url from which the query param will be removed
+ * @param param {string} - the name of the query param to remove
+ * @returns {string} A copy of the original url but without the query param
+ */
+export const removeUrlParam = (url, param) => {
+  const [rootAndQuery, fragment] = url.split('#');
+  const [root, query] = rootAndQuery.split('?');
+
+  if (query === undefined) {
+    return url;
+  }
+
+  const encodedParam = encodeURIComponent(param);
+  const updatedQuery = query
+    .split('&')
+    .filter(paramPair => {
+      const [foundParam] = paramPair.split('=');
+      return foundParam !== encodedParam;
+    })
+    .join('&');
+
+  const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : '';
+  const writableFragment = fragment ? `#${fragment}` : '';
+  return `${root}${writableQuery}${writableFragment}`;
+};
+
+/**
+ * Apply the fragment to the given url by returning a new url string that includes
+ * the fragment. If the given url already contains a fragment, the original fragment
+ * will be removed.
+ *
+ * @param url {string} - url to which the fragment will be applied
+ * @param fragment {string} - fragment to append
+ */
+export const setUrlFragment = (url, fragment) => {
+  const [rootUrl] = url.split('#');
+  const encodedFragment = encodeURIComponent(fragment.replace(/^#/, ''));
+  return `${rootUrl}#${encodedFragment}`;
+};
+
 export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey;
 
 // Identify following special clicks
diff --git a/app/assets/javascripts/pages/sessions/new/index.js b/app/assets/javascripts/pages/sessions/new/index.js
index 07f32210d93..d54bff88f70 100644
--- a/app/assets/javascripts/pages/sessions/new/index.js
+++ b/app/assets/javascripts/pages/sessions/new/index.js
@@ -2,6 +2,7 @@ import $ from 'jquery';
 import UsernameValidator from './username_validator';
 import SigninTabsMemoizer from './signin_tabs_memoizer';
 import OAuthRememberMe from './oauth_remember_me';
+import preserveUrlFragment from './preserve_url_fragment';
 
 document.addEventListener('DOMContentLoaded', () => {
   new UsernameValidator(); // eslint-disable-line no-new
@@ -10,4 +11,8 @@ document.addEventListener('DOMContentLoaded', () => {
   new OAuthRememberMe({
     container: $('.omniauth-container'),
   }).bindEvents();
+
+  // Save the URL fragment from the current window location. This will be present if the user was
+  // redirected to sign-in after attempting to access a protected URL that included a fragment.
+  preserveUrlFragment(window.location.hash);
 });
diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
new file mode 100644
index 00000000000..82ac59224df
--- /dev/null
+++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
@@ -0,0 +1,29 @@
+import { setUrlFragment, setUrlParam } from '../../../lib/utils/common_utils';
+
+/**
+ * Ensure the given URL fragment is preserved by appending it to sign-in/sign-up form actions and
+ * OAuth/SAML login links.
+ *
+ * @param fragment {string} - url fragment to be preserved
+ */
+export default function preserveUrlFragment(fragment) {
+  if (fragment && fragment !== '') {
+    const normalFragment = fragment.replace(/^#/, '');
+
+    // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is
+    // eventually redirected back to the originally requested URL.
+    const forms = document.querySelectorAll('#signin-container form');
+    Array.prototype.forEach.call(forms, (form) => {
+      const actionWithFragment = setUrlFragment(form.getAttribute('action'), `#${normalFragment}`);
+      form.setAttribute('action', actionWithFragment);
+    });
+
+    // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment
+    // query param will be available in the omniauth callback upon successful authentication
+    const anchors = document.querySelectorAll('#signin-container a.oauth-login');
+    Array.prototype.forEach.call(anchors, (anchor) => {
+      const newHref = setUrlParam(anchor.getAttribute('href'), 'redirect_fragment', normalFragment);
+      anchor.setAttribute('href', newHref);
+    });
+  }
+}
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index 12f11976439..f8e482937d5 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -75,10 +75,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   private
 
   def omniauth_flow(auth_module, identity_linker: nil)
-    omniauth_params = request.env['omniauth.params']
-
-    if omniauth_params.present? && omniauth_params['redirect_fragment'].present?
-      store_redirect_fragment(omniauth_params['redirect_fragment'])
+    if fragment = request.env.dig('omniauth.params', 'redirect_fragment').presence
+      store_redirect_fragment(fragment)
     end
 
     if current_user
@@ -199,8 +197,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   def store_redirect_fragment(redirect_fragment)
     key = stored_location_key_for(:user)
     location = session[key]
-    unless location.to_s.strip.empty?
-      uri = URI.parse(location)
+    if uri = parse_uri(location)
       uri.fragment = redirect_fragment
       store_location_for(:user, uri.to_s)
     end
diff --git a/app/views/devise/sessions/new.html.haml b/app/views/devise/sessions/new.html.haml
index 3840cbb0b31..30ed7ed6b29 100644
--- a/app/views/devise/sessions/new.html.haml
+++ b/app/views/devise/sessions/new.html.haml
@@ -1,33 +1,5 @@
 - page_title "Sign in"
 
-- content_for :page_specific_javascripts do
-  -# haml-lint:disable InlineJavaScript
-  :javascript
-    document.addEventListener('DOMContentLoaded', function() {
-      // Determine if the current URL location contains a fragment identifier (aka hash or anchor ref).
-      // This should be present if the user was redirected to sign in after attempting to access a
-      // protected URL that included a fragment identifier.
-      var fragment = window.location.hash;
-      if (fragment && fragment !== '') {
-        // Append the fragment to all signin/signup form actions so it is preserved when
-        // the user is eventually redirected back to the originally requested URL.
-        $('div#signin-container form').attr('action', function (index, action) {
-          return action + fragment;
-        });
-
-        // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment
-        // query param will be passed to the omniauth callback upon successful authentication
-        $('div#signin-container a.oauth-login').attr('href', function (index, href) {
-          const tokens = href.split('?');
-          let url = tokens[0] + '?redirect_fragment=' + encodeURIComponent(fragment.substr(1));
-          if (tokens.length > 1) {
-            url += '&' + tokens[1];
-          }
-          return url;
-        });
-      }
-    });
-
 #signin-container
   - if form_based_providers.any?
     = render 'devise/shared/tabs_ldap'
diff --git a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
new file mode 100644
index 00000000000..32a9becb636
--- /dev/null
+++ b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
@@ -0,0 +1,21 @@
+#signin-container
+  .tab-content
+    .active.login-box.tab-pane
+      .login-body
+        %form#new_ldap_user{ action: '/users/auth/ldapmain/callback', method: 'post' }
+
+    .login-box.tab-pane
+      .login-body
+        %form#new_user.new_user{ action: '/users/sign_in', method: 'post' }
+
+    #register-pane.login-box.tab-pane
+      .login-body
+        %form#new_new_user.new_new_user{ action: '/users', method: 'post' }
+
+  .omniauth-container
+    %span.light
+      %a#oauth-login-auth0.oauth-login.btn{ href: '/users/auth/auth0' } Auth0
+    %span.light
+      %a#oauth-login-facebook.oauth-login.btn{ href:'/users/auth/facebook?remember_me=1' } Facebook
+    %span.light
+      %a#oauth-login-saml.oauth-login.btn{ href:'/users/auth/saml' } SAML
diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js
index f320f232687..3a25be766cb 100644
--- a/spec/javascripts/lib/utils/common_utils_spec.js
+++ b/spec/javascripts/lib/utils/common_utils_spec.js
@@ -65,6 +65,67 @@ describe('common_utils', () => {
     });
   });
 
+  describe('setUrlParam', () => {
+    it('should append param when url has no other params', () => {
+      const url = commonUtils.setUrlParam('/feature/home', 'newParam', 'yes');
+      expect(url).toBe('/feature/home?newParam=yes');
+    });
+
+    it('should append param when url has other params', () => {
+      const url = commonUtils.setUrlParam('/feature/home?showAll=true', 'newParam', 'yes');
+      expect(url).toBe('/feature/home?showAll=true&newParam=yes');
+    });
+
+    it('should replace param when url contains the param', () => {
+      const url = commonUtils.setUrlParam('/feature/home?showAll=true&limit=5', 'limit', '100');
+      expect(url).toBe('/feature/home?showAll=true&limit=100');
+    });
+
+    it('should update param and preserve fragment', () => {
+      const url = commonUtils.setUrlParam('/home?q=no&limit=5&showAll=true#H1', 'limit', '100');
+      expect(url).toBe('/home?q=no&limit=100&showAll=true#H1');
+    });
+  });
+
+  describe('removeUrlParam', () => {
+    it('should remove param when url has no other params', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?size=5', 'size');
+      expect(url).toBe('/feature/home');
+    });
+
+    it('should remove param when url has other params', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'size');
+      expect(url).toBe('/feature/home?q=1&f=html');
+    });
+
+    it('should remove param and preserve fragment', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?size=5#H2', 'size');
+      expect(url).toBe('/feature/home#H2');
+    });
+
+    it('should not modify url if param does not exist', () => {
+      const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'locale');
+      expect(url).toBe('/feature/home?q=1&size=5&f=html');
+    });
+  });
+
+  describe('setUrlFragment', () => {
+    it('should set fragment when url has no fragment', () => {
+      const url = commonUtils.setUrlFragment('/home/feature', 'usage');
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when url has existing fragment', () => {
+      const url = commonUtils.setUrlFragment('/home/feature#overview', 'usage');
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when given fragment includes #', () => {
+      const url = commonUtils.setUrlFragment('/home/feature#overview', '#install');
+      expect(url).toBe('/home/feature#install');
+    });
+  });
+
   describe('handleLocationHash', () => {
     beforeEach(() => {
       spyOn(window.document, 'getElementById').and.callThrough();
diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
new file mode 100644
index 00000000000..c3be06ce6f9
--- /dev/null
+++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
@@ -0,0 +1,26 @@
+import $ from 'jquery';
+import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment';
+
+describe('preserve_url_fragment', () => {
+  preloadFixtures('static/signin_forms_and_buttons.html.raw');
+
+  beforeEach(() => {
+    loadFixtures('static/signin_forms_and_buttons.html.raw');
+  });
+
+  it('adds the url fragment to all login and sign up form actions', () => {
+    preserveUrlFragment('#L65');
+
+    expect($('#new_ldap_user').attr('action')).toBe('/users/auth/ldapmain/callback#L65');
+    expect($('#new_user').attr('action')).toBe('/users/sign_in#L65');
+    expect($('#new_new_user').attr('action')).toBe('/users#L65');
+  });
+
+  it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => {
+    preserveUrlFragment('#L65');
+
+    expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe('/users/auth/auth0?redirect_fragment=L65');
+    expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe('/users/auth/facebook?remember_me=1&redirect_fragment=L65');
+    expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe('/users/auth/saml?redirect_fragment=L65');
+  });
+});
-- 
cgit v1.2.3


From 6b067fe470857a478939a6037280beb07cf9680d Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Mon, 4 Jun 2018 16:30:59 -0500
Subject: Updating OAuthRememberMe to use new common utility functions when
 manipulating query parameters on OAuth buttons. This ensures the
 'remember_me' parameter is safely added and removed when other query
 parameters are present.

---
 app/assets/javascripts/pages/sessions/new/oauth_remember_me.js | 5 +++--
 spec/javascripts/fixtures/oauth_remember_me.html.haml          | 1 +
 spec/javascripts/oauth_remember_me_spec.js                     | 5 +++++
 3 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
index 761618109a4..0c6ccd6e495 100644
--- a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
+++ b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
@@ -1,4 +1,5 @@
 import $ from 'jquery';
+import { setUrlParam, removeUrlParam } from '~/lib/utils/common_utils';
 
 /**
  * OAuth-based login buttons have a separate "remember me" checkbox.
@@ -24,9 +25,9 @@ export default class OAuthRememberMe {
       const href = $(element).attr('href');
 
       if (rememberMe) {
-        $(element).attr('href', `${href}?remember_me=1`);
+        $(element).attr('href', setUrlParam(href, 'remember_me', 1));
       } else {
-        $(element).attr('href', href.replace('?remember_me=1', ''));
+        $(element).attr('href', removeUrlParam(href, 'remember_me'));
       }
     });
   }
diff --git a/spec/javascripts/fixtures/oauth_remember_me.html.haml b/spec/javascripts/fixtures/oauth_remember_me.html.haml
index 7886e995e57..a5d7c4e816a 100644
--- a/spec/javascripts/fixtures/oauth_remember_me.html.haml
+++ b/spec/javascripts/fixtures/oauth_remember_me.html.haml
@@ -3,3 +3,4 @@
 
   %a.oauth-login.twitter{ href: "http://example.com/" }
   %a.oauth-login.github{ href: "http://example.com/" }
+  %a.oauth-login.facebook{ href: "http://example.com/?redirect_fragment=L1" }
diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js
index 2caa266b85f..f363aa7a407 100644
--- a/spec/javascripts/oauth_remember_me_spec.js
+++ b/spec/javascripts/oauth_remember_me_spec.js
@@ -20,6 +20,10 @@ describe('OAuthRememberMe', () => {
     expect($('#oauth-container .oauth-login.github').attr('href')).toBe(
       'http://example.com/?remember_me=1',
     );
+
+    expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
+      'http://example.com/?redirect_fragment=L1&remember_me=1'
+    );
   });
 
   it('removes the "remember_me" query parameter from all OAuth login buttons', () => {
@@ -28,5 +32,6 @@ describe('OAuthRememberMe', () => {
 
     expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/');
     expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/');
+    expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe('http://example.com/?redirect_fragment=L1');
   });
 });
-- 
cgit v1.2.3


From a3541a8d8dd1f4db690b7293d2a7674287020e84 Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Thu, 7 Jun 2018 23:06:44 -0500
Subject: Removing the URL manipulation functions added to 'common_utils.js' in
 favor of the functions that already existed in 'url_utility.js'. Refactoring
 'removeParams' function in 'url_utility.js' to allow url to be passed and to
 preserve the original host and/or path provided in the url.

---
 app/assets/javascripts/lib/utils/common_utils.js   | 89 ++--------------------
 app/assets/javascripts/lib/utils/url_utility.js    | 57 ++++++++++----
 .../pages/sessions/new/oauth_remember_me.js        |  6 +-
 .../pages/sessions/new/preserve_url_fragment.js    |  4 +-
 spec/javascripts/lib/utils/common_utils_spec.js    | 61 ---------------
 spec/javascripts/lib/utils/url_utility_spec.js     | 82 +++++++++++++++++---
 6 files changed, 127 insertions(+), 172 deletions(-)

diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index ccf1d924ef2..afdca012127 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -4,6 +4,14 @@ import { getLocationHash } from './url_utility';
 import { convertToCamelCase } from './text_utility';
 import { isObject } from './type_utility';
 
+/**
+ * Simply returns `window.location`. This function exists to provide a means to spy
+ * `window.location` in unit tests.
+ *
+ * @returns {Location | string | any} The browser's `window.location`
+ */
+export const windowLocation = () => window.location;
+
 export const getPagePath = (index = 0) => {
   const page = $('body').attr('data-page') || '';
 
@@ -180,87 +188,6 @@ export const urlParamsToObject = (path = '') =>
     return data;
   }, {});
 
-/**
- * Apply the query param and value to the given url by returning a new url string that includes
- * the param/value pair. If the given url already includes the query param, the query param value
- * will be updated in the new url string. Otherwise, the query param and value will by added in
- * the new url string.
- *
- * @param url {string} - url to which the query param will be applied
- * @param param {string} - name of the query param to set
- * @param value {string|number} - value to give the query param
- * @returns {string} A copy of the original url with the new or updated query param
- */
-export const setUrlParam = (url, param, value) => {
-  const [rootAndQuery, fragment] = url.split('#');
-  const [root, query] = rootAndQuery.split('?');
-  const encodedParam = encodeURIComponent(param);
-  const encodedPair = `${encodedParam}=${encodeURIComponent(value)}`;
-
-  let paramExists = false;
-  const paramArray =
-    (query ? query.split('&') : [])
-    .map(paramPair => {
-      const [foundParam] = paramPair.split('=');
-      if (foundParam === encodedParam) {
-        paramExists = true;
-        return encodedPair;
-      }
-      return paramPair;
-    });
-
-  if (paramExists === false) {
-    paramArray.push(encodedPair);
-  }
-
-  const writableFragment = fragment ? `#${fragment}` : '';
-  return `${root}?${paramArray.join('&')}${writableFragment}`;
-};
-
-/**
- * Remove the query param from the given url by returning a new url string that no longer includes
- * the param/value pair.
- *
- * @param url {string} - url from which the query param will be removed
- * @param param {string} - the name of the query param to remove
- * @returns {string} A copy of the original url but without the query param
- */
-export const removeUrlParam = (url, param) => {
-  const [rootAndQuery, fragment] = url.split('#');
-  const [root, query] = rootAndQuery.split('?');
-
-  if (query === undefined) {
-    return url;
-  }
-
-  const encodedParam = encodeURIComponent(param);
-  const updatedQuery = query
-    .split('&')
-    .filter(paramPair => {
-      const [foundParam] = paramPair.split('=');
-      return foundParam !== encodedParam;
-    })
-    .join('&');
-
-  const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : '';
-  const writableFragment = fragment ? `#${fragment}` : '';
-  return `${root}${writableQuery}${writableFragment}`;
-};
-
-/**
- * Apply the fragment to the given url by returning a new url string that includes
- * the fragment. If the given url already contains a fragment, the original fragment
- * will be removed.
- *
- * @param url {string} - url to which the fragment will be applied
- * @param fragment {string} - fragment to append
- */
-export const setUrlFragment = (url, fragment) => {
-  const [rootUrl] = url.split('#');
-  const encodedFragment = encodeURIComponent(fragment.replace(/^#/, ''));
-  return `${rootUrl}#${encodedFragment}`;
-};
-
 export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey;
 
 // Identify following special clicks
diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 9850f7ce782..61f53a632b8 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -1,3 +1,5 @@
+import { windowLocation } from './common_utils';
+
 // Returns an array containing the value(s) of the
 // of the key passed as an argument
 export function getParameterValues(sParam) {
@@ -42,22 +44,35 @@ export function mergeUrlParams(params, url) {
   return `${urlparts[1]}?${query}${urlparts[3]}`;
 }
 
-export function removeParamQueryString(url, param) {
-  const decodedUrl = decodeURIComponent(url);
-  const urlVariables = decodedUrl.split('&');
-
-  return urlVariables.filter(variable => variable.indexOf(param) === -1).join('&');
-}
-
-export function removeParams(params, source = window.location.href) {
-  const url = document.createElement('a');
-  url.href = source;
+/**
+ * Removes specified query params from the url by returning a new url string that no longer
+ * includes the param/value pair. If no url is provided, `window.location.href` is used as
+ * the default value.
+ *
+ * @param {string[]} params - the query param names to remove
+ * @param {string} [url=windowLocation().href] - url from which the query param will be removed
+ * @returns {string} A copy of the original url but without the query param
+ */
+export function removeParams(params, url = windowLocation().href) {
+  const [rootAndQuery, fragment] = url.split('#');
+  const [root, query] = rootAndQuery.split('?');
+
+  if (query === undefined) {
+    return url;
+  }
 
-  params.forEach(param => {
-    url.search = removeParamQueryString(url.search, param);
-  });
+  const encodedParams = params.map(param => encodeURIComponent(param));
+  const updatedQuery = query
+    .split('&')
+    .filter(paramPair => {
+      const [foundParam] = paramPair.split('=');
+      return encodedParams.indexOf(foundParam) < 0;
+    })
+    .join('&');
 
-  return url.href;
+  const writableQuery = updatedQuery.length > 0 ? `?${updatedQuery}` : '';
+  const writableFragment = fragment ? `#${fragment}` : '';
+  return `${root}${writableQuery}${writableFragment}`;
 }
 
 export function getLocationHash(url = window.location.href) {
@@ -66,6 +81,20 @@ export function getLocationHash(url = window.location.href) {
   return hashIndex === -1 ? null : url.substring(hashIndex + 1);
 }
 
+/**
+ * Apply the fragment to the given url by returning a new url string that includes
+ * the fragment. If the given url already contains a fragment, the original fragment
+ * will be removed.
+ *
+ * @param {string} url - url to which the fragment will be applied
+ * @param {string} fragment - fragment to append
+ */
+export const setUrlFragment = (url, fragment) => {
+  const [rootUrl] = url.split('#');
+  const encodedFragment = encodeURIComponent(fragment.replace(/^#/, ''));
+  return `${rootUrl}#${encodedFragment}`;
+};
+
 export function visitUrl(url, external = false) {
   if (external) {
     // Simulate `target="blank" rel="noopener noreferrer"`
diff --git a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
index 0c6ccd6e495..191221a48cd 100644
--- a/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
+++ b/app/assets/javascripts/pages/sessions/new/oauth_remember_me.js
@@ -1,5 +1,5 @@
 import $ from 'jquery';
-import { setUrlParam, removeUrlParam } from '~/lib/utils/common_utils';
+import { mergeUrlParams, removeParams } from '~/lib/utils/url_utility';
 
 /**
  * OAuth-based login buttons have a separate "remember me" checkbox.
@@ -25,9 +25,9 @@ export default class OAuthRememberMe {
       const href = $(element).attr('href');
 
       if (rememberMe) {
-        $(element).attr('href', setUrlParam(href, 'remember_me', 1));
+        $(element).attr('href', mergeUrlParams({ remember_me: 1 }, href));
       } else {
-        $(element).attr('href', removeUrlParam(href, 'remember_me'));
+        $(element).attr('href', removeParams(['remember_me'], href));
       }
     });
   }
diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
index 82ac59224df..71b7ca8ec31 100644
--- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
+++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
@@ -1,4 +1,4 @@
-import { setUrlFragment, setUrlParam } from '../../../lib/utils/common_utils';
+import { mergeUrlParams, setUrlFragment } from '~/lib/utils/url_utility';
 
 /**
  * Ensure the given URL fragment is preserved by appending it to sign-in/sign-up form actions and
@@ -22,7 +22,7 @@ export default function preserveUrlFragment(fragment) {
     // query param will be available in the omniauth callback upon successful authentication
     const anchors = document.querySelectorAll('#signin-container a.oauth-login');
     Array.prototype.forEach.call(anchors, (anchor) => {
-      const newHref = setUrlParam(anchor.getAttribute('href'), 'redirect_fragment', normalFragment);
+      const newHref = mergeUrlParams({ redirect_fragment: normalFragment }, anchor.getAttribute('href'));
       anchor.setAttribute('href', newHref);
     });
   }
diff --git a/spec/javascripts/lib/utils/common_utils_spec.js b/spec/javascripts/lib/utils/common_utils_spec.js
index 3a25be766cb..f320f232687 100644
--- a/spec/javascripts/lib/utils/common_utils_spec.js
+++ b/spec/javascripts/lib/utils/common_utils_spec.js
@@ -65,67 +65,6 @@ describe('common_utils', () => {
     });
   });
 
-  describe('setUrlParam', () => {
-    it('should append param when url has no other params', () => {
-      const url = commonUtils.setUrlParam('/feature/home', 'newParam', 'yes');
-      expect(url).toBe('/feature/home?newParam=yes');
-    });
-
-    it('should append param when url has other params', () => {
-      const url = commonUtils.setUrlParam('/feature/home?showAll=true', 'newParam', 'yes');
-      expect(url).toBe('/feature/home?showAll=true&newParam=yes');
-    });
-
-    it('should replace param when url contains the param', () => {
-      const url = commonUtils.setUrlParam('/feature/home?showAll=true&limit=5', 'limit', '100');
-      expect(url).toBe('/feature/home?showAll=true&limit=100');
-    });
-
-    it('should update param and preserve fragment', () => {
-      const url = commonUtils.setUrlParam('/home?q=no&limit=5&showAll=true#H1', 'limit', '100');
-      expect(url).toBe('/home?q=no&limit=100&showAll=true#H1');
-    });
-  });
-
-  describe('removeUrlParam', () => {
-    it('should remove param when url has no other params', () => {
-      const url = commonUtils.removeUrlParam('/feature/home?size=5', 'size');
-      expect(url).toBe('/feature/home');
-    });
-
-    it('should remove param when url has other params', () => {
-      const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'size');
-      expect(url).toBe('/feature/home?q=1&f=html');
-    });
-
-    it('should remove param and preserve fragment', () => {
-      const url = commonUtils.removeUrlParam('/feature/home?size=5#H2', 'size');
-      expect(url).toBe('/feature/home#H2');
-    });
-
-    it('should not modify url if param does not exist', () => {
-      const url = commonUtils.removeUrlParam('/feature/home?q=1&size=5&f=html', 'locale');
-      expect(url).toBe('/feature/home?q=1&size=5&f=html');
-    });
-  });
-
-  describe('setUrlFragment', () => {
-    it('should set fragment when url has no fragment', () => {
-      const url = commonUtils.setUrlFragment('/home/feature', 'usage');
-      expect(url).toBe('/home/feature#usage');
-    });
-
-    it('should set fragment when url has existing fragment', () => {
-      const url = commonUtils.setUrlFragment('/home/feature#overview', 'usage');
-      expect(url).toBe('/home/feature#usage');
-    });
-
-    it('should set fragment when given fragment includes #', () => {
-      const url = commonUtils.setUrlFragment('/home/feature#overview', '#install');
-      expect(url).toBe('/home/feature#install');
-    });
-  });
-
   describe('handleLocationHash', () => {
     beforeEach(() => {
       spyOn(window.document, 'getElementById').and.callThrough();
diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js
index e4df8441793..fe787baf08e 100644
--- a/spec/javascripts/lib/utils/url_utility_spec.js
+++ b/spec/javascripts/lib/utils/url_utility_spec.js
@@ -1,4 +1,4 @@
-import { webIDEUrl, mergeUrlParams } from '~/lib/utils/url_utility';
+import UrlUtility, * as urlUtils from '~/lib/utils/url_utility';
 
 describe('URL utility', () => {
   describe('webIDEUrl', () => {
@@ -8,7 +8,7 @@ describe('URL utility', () => {
 
     describe('without relative_url_root', () => {
       it('returns IDE path with route', () => {
-        expect(webIDEUrl('/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
+        expect(urlUtils.webIDEUrl('/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
           '/-/ide/project/gitlab-org/gitlab-ce/merge_requests/1',
         );
       });
@@ -20,7 +20,7 @@ describe('URL utility', () => {
       });
 
       it('returns IDE path with route', () => {
-        expect(webIDEUrl('/gitlab/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
+        expect(urlUtils.webIDEUrl('/gitlab/gitlab-org/gitlab-ce/merge_requests/1')).toBe(
           '/gitlab/-/ide/project/gitlab-org/gitlab-ce/merge_requests/1',
         );
       });
@@ -29,23 +29,83 @@ describe('URL utility', () => {
 
   describe('mergeUrlParams', () => {
     it('adds w', () => {
-      expect(mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
-      expect(mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
-      expect(mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
-      expect(mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag');
-      expect(mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag');
     });
 
     it('updates w', () => {
-      expect(mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, '?k1=v1&w=0#frag')).toBe('?k1=v1&w=1#frag');
     });
 
     it('adds multiple params', () => {
-      expect(mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag');
+      expect(urlUtils.mergeUrlParams({ a: 1, b: 2, c: 3 }, '#frag')).toBe('?a=1&b=2&c=3#frag');
     });
 
     it('adds and updates encoded params', () => {
-      expect(mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag');
+      expect(urlUtils.mergeUrlParams({ a: '&', q: '?' }, '?a=%23#frag')).toBe('?a=%26&q=%3F#frag');
+    });
+  });
+
+  describe('removeParams', () => {
+    describe('when url is passed', () => {
+      it('removes query param with encoded ampersand', () => {
+        const url = urlUtils.removeParams(['filter'], '/mail?filter=n%3Djoe%26l%3Dhome');
+
+        expect(url).toBe('/mail');
+      });
+
+      it('should remove param when url has no other params', () => {
+        const url = urlUtils.removeParams(['size'], '/feature/home?size=5');
+        expect(url).toBe('/feature/home');
+      });
+
+      it('should remove param when url has other params', () => {
+        const url = urlUtils.removeParams(['size'], '/feature/home?q=1&size=5&f=html');
+        expect(url).toBe('/feature/home?q=1&f=html');
+      });
+
+      it('should remove param and preserve fragment', () => {
+        const url = urlUtils.removeParams(['size'], '/feature/home?size=5#H2');
+        expect(url).toBe('/feature/home#H2');
+      });
+
+      it('should remove multiple params', () => {
+        const url = urlUtils.removeParams(['z', 'a'], '/home?z=11111&l=en_US&a=true#H2');
+        expect(url).toBe('/home?l=en_US#H2');
+      });
+    });
+
+    describe('when no url is passed', () => {
+      it('should remove params from window.location.href', () => {
+        spyOnDependency(UrlUtility, 'windowLocation').and.callFake(() => {
+          const anchor = document.createElement('a');
+          anchor.href = 'https://mysite.com/?zip=11111&locale=en_US&ads=false#privacy';
+          return anchor;
+        });
+
+        const url = urlUtils.removeParams(['locale']);
+        expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy');
+      });
+    });
+  });
+
+  describe('setUrlFragment', () => {
+    it('should set fragment when url has no fragment', () => {
+      const url = urlUtils.setUrlFragment('/home/feature', 'usage');
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when url has existing fragment', () => {
+      const url = urlUtils.setUrlFragment('/home/feature#overview', 'usage');
+      expect(url).toBe('/home/feature#usage');
+    });
+
+    it('should set fragment when given fragment includes #', () => {
+      const url = urlUtils.setUrlFragment('/home/feature#overview', '#install');
+      expect(url).toBe('/home/feature#install');
     });
   });
 });
-- 
cgit v1.2.3


From 2cbc475e5327a860032414561916c7bd725685ac Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Tue, 27 Nov 2018 07:16:24 -0600
Subject: Fixing static analysis issues

---
 .../pages/sessions/new/preserve_url_fragment.js         |  9 ++++++---
 spec/javascripts/lib/utils/url_utility_spec.js          | 17 +++++++++++++++--
 spec/javascripts/oauth_remember_me_spec.js              |  6 ++++--
 .../pages/sessions/new/preserve_url_fragment_spec.js    | 14 +++++++++++---
 4 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
index 71b7ca8ec31..56ea39f1259 100644
--- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
+++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
@@ -13,7 +13,7 @@ export default function preserveUrlFragment(fragment) {
     // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is
     // eventually redirected back to the originally requested URL.
     const forms = document.querySelectorAll('#signin-container form');
-    Array.prototype.forEach.call(forms, (form) => {
+    Array.prototype.forEach.call(forms, form => {
       const actionWithFragment = setUrlFragment(form.getAttribute('action'), `#${normalFragment}`);
       form.setAttribute('action', actionWithFragment);
     });
@@ -21,8 +21,11 @@ export default function preserveUrlFragment(fragment) {
     // Append a redirect_fragment query param to all oauth provider links. The redirect_fragment
     // query param will be available in the omniauth callback upon successful authentication
     const anchors = document.querySelectorAll('#signin-container a.oauth-login');
-    Array.prototype.forEach.call(anchors, (anchor) => {
-      const newHref = mergeUrlParams({ redirect_fragment: normalFragment }, anchor.getAttribute('href'));
+    Array.prototype.forEach.call(anchors, anchor => {
+      const newHref = mergeUrlParams(
+        { redirect_fragment: normalFragment },
+        anchor.getAttribute('href'),
+      );
       anchor.setAttribute('href', newHref);
     });
   }
diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js
index fe787baf08e..1ac1c414df7 100644
--- a/spec/javascripts/lib/utils/url_utility_spec.js
+++ b/spec/javascripts/lib/utils/url_utility_spec.js
@@ -32,8 +32,13 @@ describe('URL utility', () => {
       expect(urlUtils.mergeUrlParams({ w: 1 }, '#frag')).toBe('?w=1#frag');
       expect(urlUtils.mergeUrlParams({ w: 1 }, '/path#frag')).toBe('/path?w=1#frag');
       expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path')).toBe('https://host/path?w=1');
-      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe('https://host/path?w=1#frag');
-      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe('https://h/p?k1=v1&w=1#frag');
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://host/path#frag')).toBe(
+        'https://host/path?w=1#frag',
+      );
+
+      expect(urlUtils.mergeUrlParams({ w: 1 }, 'https://h/p?k1=v1#frag')).toBe(
+        'https://h/p?k1=v1&w=1#frag',
+      );
     });
 
     it('updates w', () => {
@@ -59,21 +64,25 @@ describe('URL utility', () => {
 
       it('should remove param when url has no other params', () => {
         const url = urlUtils.removeParams(['size'], '/feature/home?size=5');
+
         expect(url).toBe('/feature/home');
       });
 
       it('should remove param when url has other params', () => {
         const url = urlUtils.removeParams(['size'], '/feature/home?q=1&size=5&f=html');
+
         expect(url).toBe('/feature/home?q=1&f=html');
       });
 
       it('should remove param and preserve fragment', () => {
         const url = urlUtils.removeParams(['size'], '/feature/home?size=5#H2');
+
         expect(url).toBe('/feature/home#H2');
       });
 
       it('should remove multiple params', () => {
         const url = urlUtils.removeParams(['z', 'a'], '/home?z=11111&l=en_US&a=true#H2');
+
         expect(url).toBe('/home?l=en_US#H2');
       });
     });
@@ -87,6 +96,7 @@ describe('URL utility', () => {
         });
 
         const url = urlUtils.removeParams(['locale']);
+
         expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy');
       });
     });
@@ -95,16 +105,19 @@ describe('URL utility', () => {
   describe('setUrlFragment', () => {
     it('should set fragment when url has no fragment', () => {
       const url = urlUtils.setUrlFragment('/home/feature', 'usage');
+
       expect(url).toBe('/home/feature#usage');
     });
 
     it('should set fragment when url has existing fragment', () => {
       const url = urlUtils.setUrlFragment('/home/feature#overview', 'usage');
+
       expect(url).toBe('/home/feature#usage');
     });
 
     it('should set fragment when given fragment includes #', () => {
       const url = urlUtils.setUrlFragment('/home/feature#overview', '#install');
+
       expect(url).toBe('/home/feature#install');
     });
   });
diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js
index f363aa7a407..4125706a407 100644
--- a/spec/javascripts/oauth_remember_me_spec.js
+++ b/spec/javascripts/oauth_remember_me_spec.js
@@ -22,7 +22,7 @@ describe('OAuthRememberMe', () => {
     );
 
     expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
-      'http://example.com/?redirect_fragment=L1&remember_me=1'
+      'http://example.com/?redirect_fragment=L1&remember_me=1',
     );
   });
 
@@ -32,6 +32,8 @@ describe('OAuthRememberMe', () => {
 
     expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/');
     expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/');
-    expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe('http://example.com/?redirect_fragment=L1');
+    expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
+      'http://example.com/?redirect_fragment=L1',
+    );
   });
 });
diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
index c3be06ce6f9..de308127a0f 100644
--- a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
+++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
@@ -19,8 +19,16 @@ describe('preserve_url_fragment', () => {
   it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => {
     preserveUrlFragment('#L65');
 
-    expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe('/users/auth/auth0?redirect_fragment=L65');
-    expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe('/users/auth/facebook?remember_me=1&redirect_fragment=L65');
-    expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe('/users/auth/saml?redirect_fragment=L65');
+    expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
+      '/users/auth/auth0?redirect_fragment=L65',
+    );
+
+    expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe(
+      '/users/auth/facebook?remember_me=1&redirect_fragment=L65',
+    );
+
+    expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe(
+      '/users/auth/saml?redirect_fragment=L65',
+    );
   });
 });
-- 
cgit v1.2.3


From 87c571f8a3e9af9de0d979dc26f9838bb0fc924d Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Sun, 2 Dec 2018 00:46:11 -0600
Subject: Addressing feedback from most recent reviews.

---
 app/assets/javascripts/lib/utils/common_utils.js   |  8 ----
 app/assets/javascripts/lib/utils/url_utility.js    |  6 +--
 .../pages/sessions/new/preserve_url_fragment.js    |  4 +-
 spec/javascripts/fixtures/sessions.rb              | 26 ++++++++++
 .../fixtures/signin_forms_and_buttons.html.haml    | 21 ---------
 spec/javascripts/lib/utils/url_utility_spec.js     | 16 +------
 .../sessions/new/preserve_url_fragment_spec.js     | 55 ++++++++++++++++------
 7 files changed, 72 insertions(+), 64 deletions(-)
 create mode 100644 spec/javascripts/fixtures/sessions.rb
 delete mode 100644 spec/javascripts/fixtures/signin_forms_and_buttons.html.haml

diff --git a/app/assets/javascripts/lib/utils/common_utils.js b/app/assets/javascripts/lib/utils/common_utils.js
index afdca012127..fc34d243dd7 100644
--- a/app/assets/javascripts/lib/utils/common_utils.js
+++ b/app/assets/javascripts/lib/utils/common_utils.js
@@ -4,14 +4,6 @@ import { getLocationHash } from './url_utility';
 import { convertToCamelCase } from './text_utility';
 import { isObject } from './type_utility';
 
-/**
- * Simply returns `window.location`. This function exists to provide a means to spy
- * `window.location` in unit tests.
- *
- * @returns {Location | string | any} The browser's `window.location`
- */
-export const windowLocation = () => window.location;
-
 export const getPagePath = (index = 0) => {
   const page = $('body').attr('data-page') || '';
 
diff --git a/app/assets/javascripts/lib/utils/url_utility.js b/app/assets/javascripts/lib/utils/url_utility.js
index 61f53a632b8..4ba84589705 100644
--- a/app/assets/javascripts/lib/utils/url_utility.js
+++ b/app/assets/javascripts/lib/utils/url_utility.js
@@ -1,5 +1,3 @@
-import { windowLocation } from './common_utils';
-
 // Returns an array containing the value(s) of the
 // of the key passed as an argument
 export function getParameterValues(sParam) {
@@ -53,11 +51,11 @@ export function mergeUrlParams(params, url) {
  * @param {string} [url=windowLocation().href] - url from which the query param will be removed
  * @returns {string} A copy of the original url but without the query param
  */
-export function removeParams(params, url = windowLocation().href) {
+export function removeParams(params, url = window.location.href) {
   const [rootAndQuery, fragment] = url.split('#');
   const [root, query] = rootAndQuery.split('?');
 
-  if (query === undefined) {
+  if (!query) {
     return url;
   }
 
diff --git a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
index 56ea39f1259..e617fecaa0f 100644
--- a/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
+++ b/app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js
@@ -6,8 +6,8 @@ import { mergeUrlParams, setUrlFragment } from '~/lib/utils/url_utility';
  *
  * @param fragment {string} - url fragment to be preserved
  */
-export default function preserveUrlFragment(fragment) {
-  if (fragment && fragment !== '') {
+export default function preserveUrlFragment(fragment = '') {
+  if (fragment) {
     const normalFragment = fragment.replace(/^#/, '');
 
     // Append the fragment to all sign-in/sign-up form actions so it is preserved when the user is
diff --git a/spec/javascripts/fixtures/sessions.rb b/spec/javascripts/fixtures/sessions.rb
new file mode 100644
index 00000000000..e90a58e8c54
--- /dev/null
+++ b/spec/javascripts/fixtures/sessions.rb
@@ -0,0 +1,26 @@
+require 'spec_helper'
+
+describe 'Sessions (JavaScript fixtures)' do
+  include JavaScriptFixturesHelpers
+
+  before(:all) do
+    clean_frontend_fixtures('sessions/')
+  end
+
+  describe SessionsController, '(JavaScript fixtures)', type: :controller do
+    include DeviseHelpers
+
+    render_views
+
+    before do
+      set_devise_mapping(context: @request)
+    end
+
+    it 'sessions/new.html.raw' do |example|
+      get :new
+
+      expect(response).to be_success
+      store_frontend_fixture(response, example.description)
+    end
+  end
+end
diff --git a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml b/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
deleted file mode 100644
index 32a9becb636..00000000000
--- a/spec/javascripts/fixtures/signin_forms_and_buttons.html.haml
+++ /dev/null
@@ -1,21 +0,0 @@
-#signin-container
-  .tab-content
-    .active.login-box.tab-pane
-      .login-body
-        %form#new_ldap_user{ action: '/users/auth/ldapmain/callback', method: 'post' }
-
-    .login-box.tab-pane
-      .login-body
-        %form#new_user.new_user{ action: '/users/sign_in', method: 'post' }
-
-    #register-pane.login-box.tab-pane
-      .login-body
-        %form#new_new_user.new_new_user{ action: '/users', method: 'post' }
-
-  .omniauth-container
-    %span.light
-      %a#oauth-login-auth0.oauth-login.btn{ href: '/users/auth/auth0' } Auth0
-    %span.light
-      %a#oauth-login-facebook.oauth-login.btn{ href:'/users/auth/facebook?remember_me=1' } Facebook
-    %span.light
-      %a#oauth-login-saml.oauth-login.btn{ href:'/users/auth/saml' } SAML
diff --git a/spec/javascripts/lib/utils/url_utility_spec.js b/spec/javascripts/lib/utils/url_utility_spec.js
index 1ac1c414df7..381c7b2d0a6 100644
--- a/spec/javascripts/lib/utils/url_utility_spec.js
+++ b/spec/javascripts/lib/utils/url_utility_spec.js
@@ -1,4 +1,4 @@
-import UrlUtility, * as urlUtils from '~/lib/utils/url_utility';
+import * as urlUtils from '~/lib/utils/url_utility';
 
 describe('URL utility', () => {
   describe('webIDEUrl', () => {
@@ -86,20 +86,6 @@ describe('URL utility', () => {
         expect(url).toBe('/home?l=en_US#H2');
       });
     });
-
-    describe('when no url is passed', () => {
-      it('should remove params from window.location.href', () => {
-        spyOnDependency(UrlUtility, 'windowLocation').and.callFake(() => {
-          const anchor = document.createElement('a');
-          anchor.href = 'https://mysite.com/?zip=11111&locale=en_US&ads=false#privacy';
-          return anchor;
-        });
-
-        const url = urlUtils.removeParams(['locale']);
-
-        expect(url).toBe('https://mysite.com/?zip=11111&ads=false#privacy');
-      });
-    });
   });
 
   describe('setUrlFragment', () => {
diff --git a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
index de308127a0f..7a8227479d4 100644
--- a/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
+++ b/spec/javascripts/pages/sessions/new/preserve_url_fragment_spec.js
@@ -2,33 +2,60 @@ import $ from 'jquery';
 import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment';
 
 describe('preserve_url_fragment', () => {
-  preloadFixtures('static/signin_forms_and_buttons.html.raw');
+  preloadFixtures('sessions/new.html.raw');
 
   beforeEach(() => {
-    loadFixtures('static/signin_forms_and_buttons.html.raw');
+    loadFixtures('sessions/new.html.raw');
   });
 
   it('adds the url fragment to all login and sign up form actions', () => {
     preserveUrlFragment('#L65');
 
-    expect($('#new_ldap_user').attr('action')).toBe('/users/auth/ldapmain/callback#L65');
-    expect($('#new_user').attr('action')).toBe('/users/sign_in#L65');
-    expect($('#new_new_user').attr('action')).toBe('/users#L65');
+    expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in#L65');
+    expect($('#new_new_user').attr('action')).toBe('http://test.host/users#L65');
   });
 
-  it('adds the "redirect_fragment" query parameter to all OAuth and SAML login buttons', () => {
-    preserveUrlFragment('#L65');
+  it('does not add an empty url fragment to login and sign up form actions', () => {
+    preserveUrlFragment();
+
+    expect($('#new_user').attr('action')).toBe('http://test.host/users/sign_in');
+    expect($('#new_new_user').attr('action')).toBe('http://test.host/users');
+  });
+
+  it('does not add an empty query parameter to OmniAuth login buttons', () => {
+    preserveUrlFragment();
+
+    expect($('#oauth-login-cas3').attr('href')).toBe('http://test.host/users/auth/cas3');
 
     expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
-      '/users/auth/auth0?redirect_fragment=L65',
+      'http://test.host/users/auth/auth0',
     );
+  });
 
-    expect($('.omniauth-container #oauth-login-facebook').attr('href')).toBe(
-      '/users/auth/facebook?remember_me=1&redirect_fragment=L65',
-    );
+  describe('adds "redirect_fragment" query parameter to OmniAuth login buttons', () => {
+    it('when "remember_me" is not present', () => {
+      preserveUrlFragment('#L65');
 
-    expect($('.omniauth-container #oauth-login-saml').attr('href')).toBe(
-      '/users/auth/saml?redirect_fragment=L65',
-    );
+      expect($('#oauth-login-cas3').attr('href')).toBe(
+        'http://test.host/users/auth/cas3?redirect_fragment=L65',
+      );
+
+      expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
+        'http://test.host/users/auth/auth0?redirect_fragment=L65',
+      );
+    });
+
+    it('when "remember-me" is present', () => {
+      $('a.omniauth-btn').attr('href', (i, href) => `${href}?remember_me=1`);
+      preserveUrlFragment('#L65');
+
+      expect($('#oauth-login-cas3').attr('href')).toBe(
+        'http://test.host/users/auth/cas3?remember_me=1&redirect_fragment=L65',
+      );
+
+      expect($('#oauth-login-auth0').attr('href')).toBe(
+        'http://test.host/users/auth/auth0?remember_me=1&redirect_fragment=L65',
+      );
+    });
   });
 });
-- 
cgit v1.2.3


From 9c6f183da0157d08b7990d420a7b71ab69dbb5d7 Mon Sep 17 00:00:00 2001
From: Scott Escue <scott.escue@gmail.com>
Date: Sun, 2 Dec 2018 13:32:03 -0600
Subject: Adding changelog entry.

---
 .../iss-32584-preserve-line-number-fragment-after-redirect.yml      | 6 ++++++
 1 file changed, 6 insertions(+)
 create mode 100644 changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml

diff --git a/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml b/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml
new file mode 100644
index 00000000000..8025cd472bd
--- /dev/null
+++ b/changelogs/unreleased/iss-32584-preserve-line-number-fragment-after-redirect.yml
@@ -0,0 +1,6 @@
+---
+title: Fix lost line number when navigating to a specific line in a protected file
+  before authenticating.
+merge_request: 19165
+author: Scott Escue
+type: fixed
-- 
cgit v1.2.3