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.

5 files changed, 118 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'