Merge branch 'fix/omniauth-buttons-js' into 'master'

fix: avoid javascript for omniauth logins

Closes #28904

See merge request gitlab-org/gitlab!33459

app/assets/javascripts/pages/sessions/new/oauth_remember_me.js

@@ -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));
       }
     });
   }

app/assets/javascripts/pages/sessions/new/preserve_url_fragment.js

@@ -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);
     });
   }
 }

app/assets/stylesheets/pages/login.scss

@@ -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;

app/views/devise/shared/_omniauth_box.html.haml

@@ -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

changelogs/unreleased/fix-omniauth-buttons-js.yml

+---
+title: Avoid javascript for omniauth logins
+merge_request: 33459
+author: Diego Louzán
+type: other

spec/frontend/fixtures/static/oauth_remember_me.html

 <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>

spec/frontend/oauth_remember_me_spec.js

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

spec/frontend/pages/sessions/new/preserve_url_fragment_spec.js

@@ -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',
       );
     });

spec/support/helpers/login_helpers.rb

@@ -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