diff options
| -rw-r--r-- | app/assets/javascripts/dispatcher.js | 2 | ||||
| -rw-r--r-- | app/assets/javascripts/oauth_remember_me.js | 32 | ||||
| -rw-r--r-- | app/controllers/omniauth_callbacks_controller.rb | 8 | ||||
| -rw-r--r-- | app/views/devise/shared/_omniauth_box.html.haml | 5 | ||||
| -rw-r--r-- | changelogs/unreleased/18000-remember-me-for-oauth-login.yml | 4 | ||||
| -rw-r--r-- | config/gitlab.yml.example | 47 | ||||
| -rw-r--r-- | lib/tasks/gitlab/info.rake | 3 | ||||
| -rw-r--r-- | spec/features/oauth_login_spec.rb | 112 | ||||
| -rw-r--r-- | spec/javascripts/fixtures/oauth_remember_me.html.haml | 5 | ||||
| -rw-r--r-- | spec/javascripts/oauth_remember_me_spec.js | 26 | ||||
| -rw-r--r-- | spec/support/capybara_helpers.rb | 5 | ||||
| -rw-r--r-- | spec/support/login_helpers.rb | 13 |
12 files changed, 4 insertions, 258 deletions
diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index e924fde60bf..4247540de22 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -56,7 +56,6 @@ 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; @@ -128,7 +127,6 @@ import OAuthRememberMe from './oauth_remember_me'; 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 deleted file mode 100644 index ffc2dd6bbca..00000000000 --- a/app/assets/javascripts/oauth_remember_me.js +++ /dev/null @@ -1,32 +0,0 @@ -/** - * 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 323d5d26eb6..b82681b197e 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -1,6 +1,5 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController include AuthenticatesWithTwoFactor - include Devise::Controllers::Rememberable protect_from_forgery except: [:kerberos, :saml, :cas3] @@ -116,10 +115,8 @@ 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 @@ -150,9 +147,4 @@ 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 e80d10dc8f1..f92f89e73ff 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -6,7 +6,4 @@ - 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: 'oauth-login' + (has_icon ? ' oauth-image-link' : ' btn'), id: "oauth-login-#{provider}" - %fieldset - = check_box_tag :remember_me - = label_tag :remember_me, 'Remember Me' + = link_to provider_image_tag(provider), omniauth_authorize_path(:user, provider), method: :post, class: (has_icon ? 'oauth-image-link' : 'btn') diff --git a/changelogs/unreleased/18000-remember-me-for-oauth-login.yml b/changelogs/unreleased/18000-remember-me-for-oauth-login.yml deleted file mode 100644 index 1ef92756a76..00000000000 --- a/changelogs/unreleased/18000-remember-me-for-oauth-login.yml +++ /dev/null @@ -1,4 +0,0 @@ ---- -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 fdc2b24e110..4b81fd90f59 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -619,53 +619,6 @@ 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 e9fb6a008b0..e3883278886 100644 --- a/lib/tasks/gitlab/info.rake +++ b/lib/tasks/gitlab/info.rake @@ -42,7 +42,8 @@ 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.map { |provider| provider['name'] } + omniauth_providers = Gitlab.config.omniauth.providers + 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 deleted file mode 100644 index 452b920307c..00000000000 --- a/spec/features/oauth_login_spec.rb +++ /dev/null @@ -1,112 +0,0 @@ -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 deleted file mode 100644 index 7886e995e57..00000000000 --- a/spec/javascripts/fixtures/oauth_remember_me.html.haml +++ /dev/null @@ -1,5 +0,0 @@ -#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 deleted file mode 100644 index f90e0093d25..00000000000 --- a/spec/javascripts/oauth_remember_me_spec.js +++ /dev/null @@ -1,26 +0,0 @@ -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 3eb7bea3227..b57a3493aff 100644 --- a/spec/support/capybara_helpers.rb +++ b/spec/support/capybara_helpers.rb @@ -35,11 +35,6 @@ 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 99e7806353d..4c88958264b 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -62,16 +62,6 @@ 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. @@ -118,7 +108,6 @@ module LoginHelpers end allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: mock_saml_config) stub_omniauth_setting(messages) - 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') + expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') end end |