Commit f3281e12 authored by Diego Louzán's avatar Diego Louzán

fix: avoid javascript for omniauth logins

When a user clicks on an Omniauth login icon (e.g. Google), Rails will
translate a `link_to` URL from a GET to a POST form submission via
JavaScript. However, if JavaScript is disabled or not loaded before the
page loads, this will cause a GET request to go to the login provider
instead of POST, resulting in a 404.

To avoid this, we use `button_to` instead of `link_to`. `button_to` will
set a form submission with a POST request without JavaScript.

Closes #28904
parent 3986aa16
......@@ -5,13 +5,12 @@ import { mergeUrlParams, removeParams } from '~/lib/utils/url_utility';
* OAuth-based login buttons have a separate "remember me" checkbox.
*
* Toggling this checkbox adds/removes a `remember_me` parameter to the
* login buttons' href, which is passed on to the omniauth callback.
* login buttons' parent form action, which is passed on to the omniauth callback.
*/
export default class OAuthRememberMe {
constructor(opts = {}) {
this.container = opts.container || '';
this.loginLinkSelector = '.oauth-login';
}
bindEvents() {
......@@ -22,12 +21,13 @@ export default class OAuthRememberMe {
const rememberMe = $(event.target).is(':checked');
$('.oauth-login', this.container).each((i, element) => {
const href = $(element).attr('href');
const $form = $(element).parent('form');
const href = $form.attr('action');
if (rememberMe) {
$(element).attr('href', mergeUrlParams({ remember_me: 1 }, href));
$form.attr('action', mergeUrlParams({ remember_me: 1 }, href));
} else {
$(element).attr('href', removeParams(['remember_me'], href));
$form.attr('action', removeParams(['remember_me'], href));
}
});
}
......
......@@ -12,7 +12,7 @@ export default function preserveUrlFragment(fragment = '') {
// 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');
const forms = document.querySelectorAll('#signin-container .tab-content form');
Array.prototype.forEach.call(forms, form => {
const actionWithFragment = setUrlFragment(form.getAttribute('action'), `#${normalFragment}`);
form.setAttribute('action', actionWithFragment);
......@@ -20,13 +20,13 @@ export default function preserveUrlFragment(fragment = '') {
// 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 oauthForms = document.querySelectorAll('#signin-container .omniauth-container form');
Array.prototype.forEach.call(oauthForms, oauthForm => {
const newHref = mergeUrlParams(
{ redirect_fragment: normalFragment },
anchor.getAttribute('href'),
oauthForm.getAttribute('action'),
);
anchor.setAttribute('href', newHref);
oauthForm.setAttribute('action', newHref);
});
}
}
......@@ -96,14 +96,21 @@
margin: 0;
}
.omniauth-btn {
margin-bottom: $gl-padding;
form {
width: 48%;
padding: $gl-padding-8;
padding: 0;
border: 0;
background: none;
margin-bottom: $gl-padding;
@include media-breakpoint-down(md) {
width: 100%;
}
}
.omniauth-btn {
width: 100%;
padding: $gl-padding-8;
img {
width: $default-icon-size;
......
......@@ -7,7 +7,7 @@
.d-flex.justify-content-between.flex-wrap
- providers.each do |provider|
- has_icon = provider_has_icon?(provider)
= link_to omniauth_authorize_path(:user, provider), method: :post, class: "btn d-flex align-items-center omniauth-btn text-left oauth-login #{qa_class_for_provider(provider)}", id: "oauth-login-#{provider}" do
= button_to omniauth_authorize_path(:user, provider), id: "oauth-login-#{provider}", class: "btn d-flex align-items-center omniauth-btn text-left oauth-login #{qa_class_for_provider(provider)}" do
- if has_icon
= provider_image_tag(provider)
%span
......
---
title: Avoid javascript for omniauth logins
merge_request: 33459
author: Diego Louzán
type: other
<div id="oauth-container">
<input id="remember_me" type="checkbox">
<a class="oauth-login twitter" href="http://example.com/"></a>
<a class="oauth-login github" href="http://example.com/"></a>
<a class="oauth-login facebook" href="http://example.com/?redirect_fragment=L1"></a>
<form method="post" action="http://example.com/">
<button class="oauth-login twitter" type="submit">
<span>Twitter</span>
</button>
</form>
<form method="post" action="http://example.com/">
<button class="oauth-login github" type="submit">
<span>GitHub</span>
</button>
</form>
<form method="post" action="http://example.com/?redirect_fragment=L1">
<button class="oauth-login facebook" type="submit">
<span>Facebook</span>
</button>
</form>
</div>
......@@ -2,6 +2,12 @@ import $ from 'jquery';
import OAuthRememberMe from '~/pages/sessions/new/oauth_remember_me';
describe('OAuthRememberMe', () => {
const findFormAction = selector => {
return $(`#oauth-container .oauth-login${selector}`)
.parent('form')
.attr('action');
};
preloadFixtures('static/oauth_remember_me.html');
beforeEach(() => {
......@@ -13,15 +19,9 @@ describe('OAuthRememberMe', () => {
it('adds the "remember_me" query parameter to all OAuth login buttons', () => {
$('#oauth-container #remember_me').click();
expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe(
'http://example.com/?remember_me=1',
);
expect($('#oauth-container .oauth-login.github').attr('href')).toBe(
'http://example.com/?remember_me=1',
);
expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
expect(findFormAction('.twitter')).toBe('http://example.com/?remember_me=1');
expect(findFormAction('.github')).toBe('http://example.com/?remember_me=1');
expect(findFormAction('.facebook')).toBe(
'http://example.com/?redirect_fragment=L1&remember_me=1',
);
});
......@@ -30,10 +30,8 @@ describe('OAuthRememberMe', () => {
$('#oauth-container #remember_me').click();
$('#oauth-container #remember_me').click();
expect($('#oauth-container .oauth-login.twitter').attr('href')).toBe('http://example.com/');
expect($('#oauth-container .oauth-login.github').attr('href')).toBe('http://example.com/');
expect($('#oauth-container .oauth-login.facebook').attr('href')).toBe(
'http://example.com/?redirect_fragment=L1',
);
expect(findFormAction('.twitter')).toBe('http://example.com/');
expect(findFormAction('.github')).toBe('http://example.com/');
expect(findFormAction('.facebook')).toBe('http://example.com/?redirect_fragment=L1');
});
});
......@@ -2,6 +2,12 @@ import $ from 'jquery';
import preserveUrlFragment from '~/pages/sessions/new/preserve_url_fragment';
describe('preserve_url_fragment', () => {
const findFormAction = selector => {
return $(`.omniauth-container ${selector}`)
.parent('form')
.attr('action');
};
preloadFixtures('sessions/new.html');
beforeEach(() => {
......@@ -25,35 +31,36 @@ describe('preserve_url_fragment', () => {
it('does not add an empty query parameter to OmniAuth login buttons', () => {
preserveUrlFragment();
expect($('#oauth-login-cas3').attr('href')).toBe('http://test.host/users/auth/cas3');
expect(findFormAction('#oauth-login-cas3')).toBe('http://test.host/users/auth/cas3');
expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
'http://test.host/users/auth/auth0',
);
expect(findFormAction('#oauth-login-auth0')).toBe('http://test.host/users/auth/auth0');
});
describe('adds "redirect_fragment" query parameter to OmniAuth login buttons', () => {
it('when "remember_me" is not present', () => {
preserveUrlFragment('#L65');
expect($('#oauth-login-cas3').attr('href')).toBe(
expect(findFormAction('#oauth-login-cas3')).toBe(
'http://test.host/users/auth/cas3?redirect_fragment=L65',
);
expect($('.omniauth-container #oauth-login-auth0').attr('href')).toBe(
expect(findFormAction('#oauth-login-auth0')).toBe(
'http://test.host/users/auth/auth0?redirect_fragment=L65',
);
});
it('when "remember-me" is present', () => {
$('a.omniauth-btn').attr('href', (i, href) => `${href}?remember_me=1`);
$('.omniauth-btn')
.parent('form')
.attr('action', (i, href) => `${href}?remember_me=1`);
preserveUrlFragment('#L65');
expect($('#oauth-login-cas3').attr('href')).toBe(
expect(findFormAction('#oauth-login-cas3')).toBe(
'http://test.host/users/auth/cas3?remember_me=1&redirect_fragment=L65',
);
expect($('#oauth-login-auth0').attr('href')).toBe(
expect(findFormAction('#oauth-login-auth0')).toBe(
'http://test.host/users/auth/auth0?remember_me=1&redirect_fragment=L65',
);
});
......
......@@ -57,13 +57,13 @@ module LoginHelpers
def gitlab_sign_in_via(provider, user, uid, saml_response = nil)
mock_auth_hash_with_saml_xml(provider, uid, user.email, saml_response)
visit new_user_session_path
click_link provider
click_button provider
end
def gitlab_enable_admin_mode_sign_in_via(provider, user, uid, saml_response = nil)
mock_auth_hash_with_saml_xml(provider, uid, user.email, saml_response)
visit new_admin_session_path
click_link provider
click_button provider
end
# Requires Javascript driver.
......@@ -103,7 +103,7 @@ module LoginHelpers
check 'remember_me' if remember_me
click_link "oauth-login-#{provider}"
click_button "oauth-login-#{provider}"
end
def fake_successful_u2f_authentication
......
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