Merge branch '18000-remember-me-for-oauth-login' into 'master'

Honor the "Remember me" parameter for OAuth-based login

Closes #18000

See merge request !11963

12 files changed, 258 insertions, 4 deletions

diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js
index 4247540de22..e924fde60bf 100644
--- a/app/assets/javascripts/dispatcher.js
+++ b/app/assets/javascripts/dispatcher.js
@@ -56,6 +56,7 @@ import GfmAutoComplete from './gfm_auto_complete';
 import ShortcutsBlob from './shortcuts_blob';
 import initSettingsPanels from './settings_panels';
 import initExperimentalFlags from './experimental_flags';
+import OAuthRememberMe from './oauth_remember_me';
 
 (function() {
   var Dispatcher;
@@ -127,6 +128,7 @@ import initExperimentalFlags from './experimental_flags';
         case 'sessions:new':
           new UsernameValidator();
           new ActiveTabMemoizer();
+          new OAuthRememberMe({ container: $(".omniauth-container") }).bindEvents();
           break;
         case 'projects:boards:show':
         case 'projects:boards:index':
diff --git a/app/assets/javascripts/oauth_remember_me.js b/app/assets/javascripts/oauth_remember_me.js
new file mode 100644
index 00000000000..ffc2dd6bbca
--- /dev/null
+++ b/app/assets/javascripts/oauth_remember_me.js
@@ -0,0 +1,32 @@
+/**
+ * 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.
+ **/
+
+export default class OAuthRememberMe {
+  constructor(opts = {}) {
+    this.container = opts.container || '';
+    this.loginLinkSelector = '.oauth-login';
+  }
+
+  bindEvents() {
+    $('#remember_me', this.container).on('click', this.toggleRememberMe);
+  }
+
+  // eslint-disable-next-line class-methods-use-this
+  toggleRememberMe(event) {
+    const rememberMe = $(event.target).is(':checked');
+
+    $('.oauth-login', this.container).each((i, element) => {
+      const href = $(element).attr('href');
+
+      if (rememberMe) {
+        $(element).attr('href', `${href}?remember_me=1`);
+      } else {
+        $(element).attr('href', href.replace('?remember_me=1', ''));
+      }
+    });
+  }
+}
diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb
index b82681b197e..323d5d26eb6 100644
--- a/app/controllers/omniauth_callbacks_controller.rb
+++ b/app/controllers/omniauth_callbacks_controller.rb
@@ -1,5 +1,6 @@
 class OmniauthCallbacksController < Devise::OmniauthCallbacksController
   include AuthenticatesWithTwoFactor
+  include Devise::Controllers::Rememberable
 
   protect_from_forgery except: [:kerberos, :saml, :cas3]
 
@@ -115,8 +116,10 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     if @user.persisted? && @user.valid?
       log_audit_event(@user, with: oauth['provider'])
       if @user.two_factor_enabled?
+        params[:remember_me] = '1' if remember_me?
         prompt_for_two_factor(@user)
       else
+        remember_me(@user) if remember_me?
         sign_in_and_redirect(@user)
       end
     else
@@ -147,4 +150,9 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController
     AuditEventService.new(user, user, options)
       .for_authentication.security_event
   end
+
+  def remember_me?
+    request_params = request.env['omniauth.params']
+    (request_params['remember_me'] == '1') if request_params.present?
+  end
 end
diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml
index f92f89e73ff..e80d10dc8f1 100644
--- a/app/views/devise/shared/_omniauth_box.html.haml
+++ b/app/views/devise/shared/_omniauth_box.html.haml
@@ -6,4 +6,7 @@
     - providers.each do |provider|
       %span.light
         - has_icon = provider_has_icon?(provider)
-        = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: (has_icon ? 'oauth-image-link' : 'btn')
+        = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: 'oauth-login' + (has_icon ? ' oauth-image-link' : ' btn'), id: "oauth-login-#{provider}"
+    %fieldset
+      = check_box_tag :remember_me
+      = label_tag :remember_me, 'Remember Me'
diff --git a/changelogs/unreleased/18000-remember-me-for-oauth-login.yml b/changelogs/unreleased/18000-remember-me-for-oauth-login.yml
new file mode 100644
index 00000000000..1ef92756a76
--- /dev/null
+++ b/changelogs/unreleased/18000-remember-me-for-oauth-login.yml
@@ -0,0 +1,4 @@
+---
+title: Honor the "Remember me" parameter for OAuth-based login
+merge_request: 11963
+author:
diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example
index 4b81fd90f59..fdc2b24e110 100644
--- a/config/gitlab.yml.example
+++ b/config/gitlab.yml.example
@@ -619,6 +619,53 @@ test:
       title: "JIRA"
       url: https://sample_company.atlassian.net
       project_key: PROJECT
+
+  omniauth:
+    enabled: true
+    allow_single_sign_on: true
+    external_providers: []
+
+    providers:
+      - { name: 'cas3',
+          label: 'cas3',
+          args: { url: 'https://sso.example.com',
+                  disable_ssl_verification: false,
+                  login_url: '/cas/login',
+                  service_validate_url: '/cas/p3/serviceValidate',
+                  logout_url: '/cas/logout'} }
+      - { name: 'authentiq',
+          app_id: 'YOUR_CLIENT_ID',
+          app_secret: 'YOUR_CLIENT_SECRET',
+          args: { scope: 'aq:name email~rs address aq:push' } }
+      - { name: 'github',
+          app_id: 'YOUR_APP_ID',
+          app_secret: 'YOUR_APP_SECRET',
+          url: "https://github.com/",
+          verify_ssl: false,
+          args: { scope: 'user:email' } }
+      - { name: 'bitbucket',
+          app_id: 'YOUR_APP_ID',
+          app_secret: 'YOUR_APP_SECRET' }
+      - { name: 'gitlab',
+          app_id: 'YOUR_APP_ID',
+          app_secret: 'YOUR_APP_SECRET',
+          args: { scope: 'api' } }
+      - { name: 'google_oauth2',
+          app_id: 'YOUR_APP_ID',
+          app_secret: 'YOUR_APP_SECRET',
+          args: { access_type: 'offline', approval_prompt: '' } }
+      - { name: 'facebook',
+          app_id: 'YOUR_APP_ID',
+          app_secret: 'YOUR_APP_SECRET' }
+      - { name: 'twitter',
+          app_id: 'YOUR_APP_ID',
+          app_secret: 'YOUR_APP_SECRET' }
+      - { name: 'auth0',
+          args: {
+            client_id: 'YOUR_AUTH0_CLIENT_ID',
+            client_secret: 'YOUR_AUTH0_CLIENT_SECRET',
+            namespace: 'YOUR_AUTH0_DOMAIN' } }
+
   ldap:
     enabled: false
     servers:
diff --git a/lib/tasks/gitlab/info.rake b/lib/tasks/gitlab/info.rake
index e3883278886..e9fb6a008b0 100644
--- a/lib/tasks/gitlab/info.rake
+++ b/lib/tasks/gitlab/info.rake
@@ -42,8 +42,7 @@ namespace :gitlab do
       http_clone_url = project.http_url_to_repo
       ssh_clone_url  = project.ssh_url_to_repo
 
-      omniauth_providers = Gitlab.config.omniauth.providers
-      omniauth_providers.map! { |provider| provider['name'] }
+      omniauth_providers = Gitlab.config.omniauth.providers.map { |provider| provider['name'] }
 
       puts ""
       puts "GitLab information".color(:yellow)
diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb
new file mode 100644
index 00000000000..452b920307c
--- /dev/null
+++ b/spec/features/oauth_login_spec.rb
@@ -0,0 +1,112 @@
+require 'spec_helper'
+
+feature 'OAuth Login', js: true do
+  def enter_code(code)
+    fill_in 'user_otp_attempt', with: code
+    click_button 'Verify code'
+  end
+
+  def stub_omniauth_config(provider)
+    OmniAuth.config.add_mock(provider, OmniAuth::AuthHash.new(provider: provider.to_s, uid: "12345"))
+    Rails.application.env_config['devise.mapping'] = Devise.mappings[:user]
+    Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[provider]
+  end
+
+  providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2,
+               :facebook, :authentiq, :cas3, :auth0]
+
+  before(:all) do
+    # The OmniAuth `full_host` parameter doesn't get set correctly (it gets set to something like `http://localhost`
+    # here), and causes integration tests to fail with 404s. We set the `full_host` by removing the request path (and
+    # anything after it) from the request URI.
+    @omniauth_config_full_host = OmniAuth.config.full_host
+    OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') }
+  end
+
+  after(:all) do
+    OmniAuth.config.full_host = @omniauth_config_full_host
+  end
+
+  providers.each do |provider|
+    context "when the user logs in using the #{provider} provider" do
+      context 'when two-factor authentication is disabled' do
+        it 'logs the user in' do
+          stub_omniauth_config(provider)
+          user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
+          login_via(provider.to_s, user, 'my-uid')
+
+          expect(current_path).to eq root_path
+        end
+      end
+
+      context 'when two-factor authentication is enabled' do
+        it 'logs the user in' do
+          stub_omniauth_config(provider)
+          user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s)
+          login_via(provider.to_s, user, 'my-uid')
+
+          enter_code(user.current_otp)
+          expect(current_path).to eq root_path
+        end
+      end
+
+      context 'when "remember me" is checked' do
+        context 'when two-factor authentication is disabled' do
+          it 'remembers the user after a browser restart' do
+            stub_omniauth_config(provider)
+            user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
+            login_via(provider.to_s, user, 'my-uid', remember_me: true)
+
+            clear_browser_session
+
+            visit(root_path)
+            expect(current_path).to eq root_path
+          end
+        end
+
+        context 'when two-factor authentication is enabled' do
+          it 'remembers the user after a browser restart' do
+            stub_omniauth_config(provider)
+            user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s)
+            login_via(provider.to_s, user, 'my-uid', remember_me: true)
+            enter_code(user.current_otp)
+
+            clear_browser_session
+
+            visit(root_path)
+            expect(current_path).to eq root_path
+          end
+        end
+      end
+
+      context 'when "remember me" is not checked' do
+        context 'when two-factor authentication is disabled' do
+          it 'does not remember the user after a browser restart' do
+            stub_omniauth_config(provider)
+            user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
+            login_via(provider.to_s, user, 'my-uid', remember_me: false)
+
+            clear_browser_session
+
+            visit(root_path)
+            expect(current_path).to eq new_user_session_path
+          end
+        end
+
+        context 'when two-factor authentication is enabled' do
+          it 'does not remember the user after a browser restart' do
+            stub_omniauth_config(provider)
+            user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s)
+            login_via(provider.to_s, user, 'my-uid', remember_me: false)
+            enter_code(user.current_otp)
+
+            clear_browser_session
+
+            visit(root_path)
+            expect(current_path).to eq new_user_session_path
+          end
+        end
+      end
+    end
+  end
+end
diff --git a/spec/javascripts/fixtures/oauth_remember_me.html.haml b/spec/javascripts/fixtures/oauth_remember_me.html.haml
new file mode 100644
index 00000000000..7886e995e57
--- /dev/null
+++ b/spec/javascripts/fixtures/oauth_remember_me.html.haml
@@ -0,0 +1,5 @@
+#oauth-container
+  %input#remember_me{ type: "checkbox" }
+
+  %a.oauth-login.twitter{ href: "http://example.com/" }
+  %a.oauth-login.github{ href: "http://example.com/" }
diff --git a/spec/javascripts/oauth_remember_me_spec.js b/spec/javascripts/oauth_remember_me_spec.js
new file mode 100644
index 00000000000..f90e0093d25
--- /dev/null
+++ b/spec/javascripts/oauth_remember_me_spec.js
@@ -0,0 +1,26 @@
+import OAuthRememberMe from '~/oauth_remember_me';
+
+describe('OAuthRememberMe', () => {
+  preloadFixtures('static/oauth_remember_me.html.raw');
+
+  beforeEach(() => {
+    loadFixtures('static/oauth_remember_me.html.raw');
+
+    new OAuthRememberMe({ container: $('#oauth-container') }).bindEvents();
+  });
+
+  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');
+  });
+
+  it('removes the "remember_me" query parameter from all OAuth login buttons', () => {
+    $('#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/');
+  });
+});
diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb
index b57a3493aff..3eb7bea3227 100644
--- a/spec/support/capybara_helpers.rb
+++ b/spec/support/capybara_helpers.rb
@@ -35,6 +35,11 @@ module CapybaraHelpers
     visit 'about:blank'
     visit url
   end
+
+  # Simulate a browser restart by clearing the session cookie.
+  def clear_browser_session
+    page.driver.remove_cookie('_gitlab_session')
+  end
 end
 
 RSpec.configure do |config|
diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb
index 4c88958264b..99e7806353d 100644
--- a/spec/support/login_helpers.rb
+++ b/spec/support/login_helpers.rb
@@ -62,6 +62,16 @@ module LoginHelpers
     Thread.current[:current_user] = user
   end
 
+  def login_via(provider, user, uid, remember_me: false)
+    mock_auth_hash(provider, uid, user.email)
+    visit new_user_session_path
+    expect(page).to have_content('Sign in with')
+
+    check 'Remember Me' if remember_me
+
+    click_link "oauth-login-#{provider}"
+  end
+
   def mock_auth_hash(provider, uid, email)
     # The mock_auth configuration allows you to set per-provider (or default)
     # authentication hashes to return during integration testing.
@@ -108,6 +118,7 @@ module LoginHelpers
     end
     allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: mock_saml_config)
     stub_omniauth_setting(messages)
-    expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml')
+    allow_any_instance_of(Object).to receive(:user_saml_omniauth_authorize_path).and_return('/users/auth/saml')
+    allow_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml')
   end
 end