Commit 4dcaa4df authored by Scott Escue's avatar Scott Escue Committed by Mike Greiling

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.
parent 6540a946
...@@ -180,6 +180,87 @@ export const urlParamsToObject = (path = '') => ...@@ -180,6 +180,87 @@ export const urlParamsToObject = (path = '') =>
return data; 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; export const isMetaKey = e => e.metaKey || e.ctrlKey || e.altKey || e.shiftKey;
// Identify following special clicks // Identify following special clicks
......
...@@ -2,6 +2,7 @@ import $ from 'jquery'; ...@@ -2,6 +2,7 @@ import $ from 'jquery';
import UsernameValidator from './username_validator'; import UsernameValidator from './username_validator';
import SigninTabsMemoizer from './signin_tabs_memoizer'; import SigninTabsMemoizer from './signin_tabs_memoizer';
import OAuthRememberMe from './oauth_remember_me'; import OAuthRememberMe from './oauth_remember_me';
import preserveUrlFragment from './preserve_url_fragment';
document.addEventListener('DOMContentLoaded', () => { document.addEventListener('DOMContentLoaded', () => {
new UsernameValidator(); // eslint-disable-line no-new new UsernameValidator(); // eslint-disable-line no-new
...@@ -10,4 +11,8 @@ document.addEventListener('DOMContentLoaded', () => { ...@@ -10,4 +11,8 @@ document.addEventListener('DOMContentLoaded', () => {
new OAuthRememberMe({ new OAuthRememberMe({
container: $('.omniauth-container'), container: $('.omniauth-container'),
}).bindEvents(); }).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);
}); });
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);
});
}
}
...@@ -75,10 +75,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -75,10 +75,8 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
private private
def omniauth_flow(auth_module, identity_linker: nil) def omniauth_flow(auth_module, identity_linker: nil)
omniauth_params = request.env['omniauth.params'] if fragment = request.env.dig('omniauth.params', 'redirect_fragment').presence
store_redirect_fragment(fragment)
if omniauth_params.present? && omniauth_params['redirect_fragment'].present?
store_redirect_fragment(omniauth_params['redirect_fragment'])
end end
if current_user if current_user
...@@ -199,8 +197,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController ...@@ -199,8 +197,7 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
def store_redirect_fragment(redirect_fragment) def store_redirect_fragment(redirect_fragment)
key = stored_location_key_for(:user) key = stored_location_key_for(:user)
location = session[key] location = session[key]
unless location.to_s.strip.empty? if uri = parse_uri(location)
uri = URI.parse(location)
uri.fragment = redirect_fragment uri.fragment = redirect_fragment
store_location_for(:user, uri.to_s) store_location_for(:user, uri.to_s)
end end
......
- page_title "Sign in" - 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 #signin-container
- if form_based_providers.any? - if form_based_providers.any?
= render 'devise/shared/tabs_ldap' = render 'devise/shared/tabs_ldap'
......
#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
...@@ -65,6 +65,67 @@ describe('common_utils', () => { ...@@ -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', () => { describe('handleLocationHash', () => {
beforeEach(() => { beforeEach(() => {
spyOn(window.document, 'getElementById').and.callThrough(); spyOn(window.document, 'getElementById').and.callThrough();
......
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');
});
});
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment