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.

3 files changed, 115 insertions, 0 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);
+    });
+  }
+}