diff options
author | Scott Escue <scott.escue@gmail.com> | 2018-06-05 00:28:18 +0300 |
---|---|---|
committer | Mike Greiling <mike@pixelcog.com> | 2019-01-10 09:00:39 +0300 |
commit | 4dcaa4df3622ae267363fcff184d0929b2102035 (patch) | |
tree | 6135c100e67c14b3359aceea4a36c0d02e2dc9a1 /app | |
parent | 6540a9468a8bce3f496423179db1862cfb9f5c8c (diff) |
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.
Diffstat (limited to 'app')
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' |