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.

8 files changed, 226 insertions, 34 deletions

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');
+  });
+});