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