From 5b649ac64dc2a987e2bede544dd13c1fab2d55a4 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 7 Jun 2017 08:45:34 +0000 Subject: [PATCH 01/13] Implement "remember me" for OAuth-based login. - Pass a `remember_me` query parameter along with the initial OAuth request, and pick this parameter up during the omniauth callback from request.env['omniauth.params']` - For 2FA-based login, copy the `remember_me` param from `omniauth.params` to `params`, which the 2FA process will pick up. - For non-2FA-based login, simply call the `remember_me` devise method to set the session cookie. --- .../omniauth_callbacks_controller.rb | 8 ++++++++ .../devise/shared/_omniauth_box.html.haml | 19 ++++++++++++++++++- 2 files changed, 26 insertions(+), 1 deletion(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index b82681b197e..c5adadfa529 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' + end end diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index f92f89e73ff..acb38c300b9 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -6,4 +6,21 @@ - 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') + %fieldset + = check_box_tag :remember_me + = label_tag :remember_me, "Remember Me" + +:javascript + $("#remember_me").click(function(event){ + var rememberMe = $(event.target).is(":checked"); + $(".oauth-login").each(function(i, element) { + var href = $(element).attr('href'); + + if (rememberMe) { + $(element).attr('href', href + '?remember_me=1'); + } else { + $(element).attr('href', href.replace('?remember_me=1', '')); + } + }); + }); From dd9264011bf554567b3e7f860c2acbf53dfa3f77 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 14 Jun 2017 04:30:07 +0000 Subject: [PATCH 02/13] Add integration tests around OAuth login. - There was previously a test for `saml` login in `login_spec`, but this didn't seem to be passing. A lot of things didn't seem right here, and I suspect that this test hasn't been running. I'll investigate this further. - It took almost a whole working day to figure out this line: OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(request['REQUEST_PATH'], '') } As always, it's obvious in retrospect, but it took some digging to figure out tests were failing and returning 404s during the callback phase. - Test all OAuth providers - github, twitter, bitbucket, gitlab, google, and facebook --- .../devise/shared/_omniauth_box.html.haml | 2 +- spec/features/oauth_login_spec.rb | 58 +++++++++++++++++++ spec/support/login_helpers.rb | 7 +++ 3 files changed, 66 insertions(+), 1 deletion(-) create mode 100644 spec/features/oauth_login_spec.rb diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index acb38c300b9..e06b804e349 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -6,7 +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: 'oauth-login' + (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/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb new file mode 100644 index 00000000000..f960dacdcac --- /dev/null +++ b/spec/features/oauth_login_spec.rb @@ -0,0 +1,58 @@ +require 'spec_helper' + +feature 'OAuth Login', feature: true, js: true do + def enter_code(code) + fill_in 'user_otp_attempt', with: code + click_button 'Verify code' + end + + def provider_config(provider) + OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') + 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] + + before do + OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(request['REQUEST_PATH'], '') } + + messages = { + enabled: true, + allow_single_sign_on: providers.map(&:to_s), + providers: providers.map { |provider| provider_config(provider) } + } + + allow(Gitlab.config.omniauth).to receive_messages(messages) + 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 + save_screenshot + 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 + end + end +end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 4c88958264b..27f12cacc62 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -62,6 +62,13 @@ module LoginHelpers Thread.current[:current_user] = user end + def login_via(provider, user, uid) + mock_auth_hash(provider, uid, user.email) + visit new_user_session_path + expect(page).to have_content('Sign in with') + 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. From 6bfc355e17ef2a431940b6f697b7cd6b743fb8ad Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Jun 2017 04:40:47 +0000 Subject: [PATCH 03/13] Test the "Remember Me" flow for OAuth-based login. --- spec/features/oauth_login_spec.rb | 61 ++++++++++++++++++++++++++++++- spec/support/capybara_helpers.rb | 5 +++ spec/support/login_helpers.rb | 5 ++- 3 files changed, 68 insertions(+), 3 deletions(-) diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index f960dacdcac..2d51abd0e97 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -19,7 +19,7 @@ feature 'OAuth Login', feature: true, js: true do providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook] before do - OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(request['REQUEST_PATH'], '') } + OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } messages = { enabled: true, @@ -39,7 +39,6 @@ feature 'OAuth Login', feature: true, js: true do login_via(provider.to_s, user, 'my-uid') expect(current_path).to eq root_path - save_screenshot end end @@ -53,6 +52,64 @@ feature 'OAuth Login', feature: true, js: true do 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) + + restart_browser + + 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) + + restart_browser + + 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) + + restart_browser + + visit(root_path) + expect(current_path).to eq new_user_session_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: false) + enter_code(user.current_otp) + + restart_browser + + visit(root_path) + expect(current_path).to eq new_user_session_path + end + end + end end end end diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb index b57a3493aff..1037e9def8c 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 restart_browser + 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 27f12cacc62..789cf9baae2 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -62,10 +62,13 @@ module LoginHelpers Thread.current[:current_user] = user end - def login_via(provider, user, uid) + 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 From 26295001407caf3e6ccdc71d05662ecdb49de5f3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Jun 2017 12:13:30 +0000 Subject: [PATCH 04/13] Move OAuth "remember me" javascript logic into a class. --- app/assets/javascripts/dispatcher.js | 2 ++ app/assets/javascripts/oauth_remember_me.js | 31 +++++++++++++++++++ .../devise/shared/_omniauth_box.html.haml | 14 --------- 3 files changed, 33 insertions(+), 14 deletions(-) create mode 100644 app/assets/javascripts/oauth_remember_me.js diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index 4247540de22..a58d1be68b5 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: $("#remember_me") }).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..3048f6fec90 --- /dev/null +++ b/app/assets/javascripts/oauth_remember_me.js @@ -0,0 +1,31 @@ +/** + * 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() { + this.container.on('click', this.toggleRememberMe); + } + + toggleRememberMe(event) { + var rememberMe = $(event.target).is(":checked"); + + $('.oauth-login').each(function(i, element) { + var 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/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index e06b804e349..493e18565c0 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -10,17 +10,3 @@ %fieldset = check_box_tag :remember_me = label_tag :remember_me, "Remember Me" - -:javascript - $("#remember_me").click(function(event){ - var rememberMe = $(event.target).is(":checked"); - $(".oauth-login").each(function(i, element) { - var href = $(element).attr('href'); - - if (rememberMe) { - $(element).attr('href', href + '?remember_me=1'); - } else { - $(element).attr('href', href.replace('?remember_me=1', '')); - } - }); - }); From de0dcfe577f7e5a849e081734eb4a396bc70d3dc Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 04:40:24 +0000 Subject: [PATCH 05/13] Add more providers to the OAuth login integration tests. - Added saml, authentiq, cas3, and auth0 - Crowd seems to be a special case that will be handled separately. --- spec/features/oauth_login_spec.rb | 43 +++++++++++++++++++++++++++++-- 1 file changed, 41 insertions(+), 2 deletions(-) diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 2d51abd0e97..b37c14bd638 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -7,7 +7,20 @@ feature 'OAuth Login', feature: true, js: true do end def provider_config(provider) - OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') + if provider == :saml + OpenStruct.new( + name: 'saml', label: 'saml', + args: { + assertion_consumer_service_url: 'https://localhost:3443/users/auth/saml/callback', + idp_cert_fingerprint: '26:43:2C:47:AF:F0:6B:D0:07:9C:AD:A3:74:FE:5D:94:5F:4E:9E:52', + idp_sso_target_url: 'https://idp.example.com/sso/saml', + issuer: 'https://localhost:3443/', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' + } + ) + else + OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') + end end def stub_omniauth_config(provider) @@ -16,7 +29,8 @@ feature 'OAuth Login', feature: true, js: true do Rails.application.env_config["omniauth.auth"] = OmniAuth.config.mock_auth[provider] end - providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook] + providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, + :facebook, :authentiq, :cas3, :auth0] before do OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } @@ -24,12 +38,37 @@ feature 'OAuth Login', feature: true, js: true do messages = { enabled: true, allow_single_sign_on: providers.map(&:to_s), + auto_link_saml_user: true, providers: providers.map { |provider| provider_config(provider) } } allow(Gitlab.config.omniauth).to receive_messages(messages) end + # context 'logging in via OAuth' do + # def saml_config + + # end + # def stub_omniauth_config(messages) + # Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] + # Rails.application.routes.disable_clear_and_finalize = true + # Rails.application.routes.draw do + # post '/users/auth/saml' => 'omniauth_callbacks#saml' + # end + # allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: saml_config) + # allow(Gitlab.config.omniauth).to receive_messages(messages) + # expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') + # end + # it 'shows 2FA prompt after OAuth login' do + # stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config]) + # user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') + # login_via('saml', user, 'my-uid') + # expect(page).to have_content('Two-Factor Authentication') + # enter_code(user.current_otp) + # expect(current_path).to eq root_path + # end + # end + providers.each do |provider| context "when the user logs in using the #{provider} provider" do context "when two-factor authentication is disabled" do From a931ead00cfa25752b9d66fd2c34f8463fdc54fc Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 07:55:09 +0000 Subject: [PATCH 06/13] Add Omniauth OAuth config to the test section of `gitlab.yml` - I tried to get this to work by stubbing out portions of the config within the test. This didn't work as expected because Devise/Omniauth loaded before the stub could run, and the stubbed config was ignored. - I attempted to fix this by reloading Devise/Omniauth after stubbing the config. This successfully got Devise to load the stubbed providers, but failed while trying to access a route such as `user_gitlab_omniauth_authorize_path`. - I spent a while trying to figure this out (even trying `Rails.application.reload_routes!`), but nothing seemed to work. - I settled for adding this config directly to `gitlab.yml` rather than go down this path any further. --- config/gitlab.yml.example | 66 +++++++++++++++++++++++++++++++ spec/features/oauth_login_spec.rb | 52 +----------------------- 2 files changed, 67 insertions(+), 51 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 43a8c0078ca..b58a173bccb 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -615,6 +615,72 @@ test: title: "JIRA" url: https://sample_company.atlassian.net project_key: PROJECT + + omniauth: + enabled: true + allow_single_sign_on: true + block_auto_created_users: false + auto_link_saml_user: 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: 'saml', + label: 'Our SAML Provider', + groups_attribute: 'Groups', + external_groups: ['Contractors', 'Freelancers'], + args: { + assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback', + idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8', + idp_sso_target_url: 'https://login.example.com/idp', + issuer: 'https://gitlab.example.com', + name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' + } } + + - { 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/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index b37c14bd638..8e02bc88fad 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -6,23 +6,6 @@ feature 'OAuth Login', feature: true, js: true do click_button 'Verify code' end - def provider_config(provider) - if provider == :saml - OpenStruct.new( - name: 'saml', label: 'saml', - args: { - assertion_consumer_service_url: 'https://localhost:3443/users/auth/saml/callback', - idp_cert_fingerprint: '26:43:2C:47:AF:F0:6B:D0:07:9C:AD:A3:74:FE:5D:94:5F:4E:9E:52', - idp_sso_target_url: 'https://idp.example.com/sso/saml', - issuer: 'https://localhost:3443/', - name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' - } - ) - else - OpenStruct.new(name: provider.to_s, app_id: 'app_id', app_secret: 'app_secret') - end - 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] @@ -32,43 +15,10 @@ feature 'OAuth Login', feature: true, js: true do providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, :facebook, :authentiq, :cas3, :auth0] - before do + before(:all) do OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') } - - messages = { - enabled: true, - allow_single_sign_on: providers.map(&:to_s), - auto_link_saml_user: true, - providers: providers.map { |provider| provider_config(provider) } - } - - allow(Gitlab.config.omniauth).to receive_messages(messages) end - # context 'logging in via OAuth' do - # def saml_config - - # end - # def stub_omniauth_config(messages) - # Rails.application.env_config['devise.mapping'] = Devise.mappings[:user] - # Rails.application.routes.disable_clear_and_finalize = true - # Rails.application.routes.draw do - # post '/users/auth/saml' => 'omniauth_callbacks#saml' - # end - # allow(Gitlab::OAuth::Provider).to receive_messages(providers: [:saml], config_for: saml_config) - # allow(Gitlab.config.omniauth).to receive_messages(messages) - # expect_any_instance_of(Object).to receive(:omniauth_authorize_path).with(:user, "saml").and_return('/users/auth/saml') - # end - # it 'shows 2FA prompt after OAuth login' do - # stub_omniauth_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [saml_config]) - # user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') - # login_via('saml', user, 'my-uid') - # expect(page).to have_content('Two-Factor Authentication') - # enter_code(user.current_otp) - # expect(current_path).to eq root_path - # end - # end - providers.each do |provider| context "when the user logs in using the #{provider} provider" do context "when two-factor authentication is disabled" do From 5050bfd9c98b8072067a265c121dc6de1dee42be Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 13:10:34 +0000 Subject: [PATCH 07/13] Get ESLint spec passing for the `OAuthRememberMe` class. --- app/assets/javascripts/oauth_remember_me.js | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/oauth_remember_me.js b/app/assets/javascripts/oauth_remember_me.js index 3048f6fec90..8f4796f2ede 100644 --- a/app/assets/javascripts/oauth_remember_me.js +++ b/app/assets/javascripts/oauth_remember_me.js @@ -12,17 +12,17 @@ export default class OAuthRememberMe { } bindEvents() { - this.container.on('click', this.toggleRememberMe); + this.container.on('click', this.constructor.toggleRememberMe); } - toggleRememberMe(event) { - var rememberMe = $(event.target).is(":checked"); + static toggleRememberMe(event) { + const rememberMe = $(event.target).is(':checked'); - $('.oauth-login').each(function(i, element) { - var href = $(element).attr('href'); + $('.oauth-login').each((i, element) => { + const href = $(element).attr('href'); if (rememberMe) { - $(element).attr('href', href + '?remember_me=1'); + $(element).attr('href', `${href}?remember_me=1`); } else { $(element).attr('href', href.replace('?remember_me=1', '')); } From c9da377ded1bd189a706e72c69ee05f2fd6a4523 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 13:15:01 +0000 Subject: [PATCH 08/13] Add CHANGELOG entry for CE MR 11963 --- changelogs/unreleased/18000-remember-me-for-oauth-login.yml | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 changelogs/unreleased/18000-remember-me-for-oauth-login.yml 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: From 1652e68a6d18bb7a44e73e713aae5c581c980b2c Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 20 Jun 2017 03:11:16 +0000 Subject: [PATCH 09/13] Don't allow the `gitlab:env:info` rake task to mutate the list of omniauth providers. - The test for `rake gitlab:env:info` executed the rake task, which mutated the list of omniauth providers, breaking subsequent tests relying on this list. - I've changed the rake task to duplicate the providers list before modifying it. --- lib/tasks/gitlab/info.rake | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/tasks/gitlab/info.rake b/lib/tasks/gitlab/info.rake index e3883278886..2245dfb7828 100644 --- a/lib/tasks/gitlab/info.rake +++ b/lib/tasks/gitlab/info.rake @@ -42,7 +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 = Gitlab.config.omniauth.providers.dup omniauth_providers.map! { |provider| provider['name'] } puts "" From 4c34374d16411e728300be9f709bb3a7d10fbbde Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 30 Jun 2017 16:36:36 +0000 Subject: [PATCH 10/13] Implement review comments for !11963 from @adamniedzielski. - Change double quotes to single quotes. - Why is `OmniAuth.config.full_host` being reassigned in the integration test? - Use `map` over `map!` to avoid `dup` in the `gitlab:info` rake task - Other minor changes --- .../devise/shared/_omniauth_box.html.haml | 2 +- lib/tasks/gitlab/info.rake | 3 +- spec/features/oauth_login_spec.rb | 36 +++++++++++-------- spec/support/capybara_helpers.rb | 2 +- spec/support/login_helpers.rb | 2 +- 5 files changed, 26 insertions(+), 19 deletions(-) diff --git a/app/views/devise/shared/_omniauth_box.html.haml b/app/views/devise/shared/_omniauth_box.html.haml index 493e18565c0..e80d10dc8f1 100644 --- a/app/views/devise/shared/_omniauth_box.html.haml +++ b/app/views/devise/shared/_omniauth_box.html.haml @@ -9,4 +9,4 @@ = 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" + = label_tag :remember_me, 'Remember Me' diff --git a/lib/tasks/gitlab/info.rake b/lib/tasks/gitlab/info.rake index 2245dfb7828..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.dup - 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 index 8e02bc88fad..452b920307c 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -1,27 +1,35 @@ require 'spec_helper' -feature 'OAuth Login', feature: true, js: true do +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" })) + 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] + 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 + 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) @@ -31,7 +39,7 @@ feature 'OAuth Login', feature: true, js: true do end end - context "when two-factor authentication is enabled" do + 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) @@ -43,27 +51,27 @@ feature 'OAuth Login', feature: true, js: true do end context 'when "remember me" is checked' do - context "when two-factor authentication is disabled" 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) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq root_path end end - context "when two-factor authentication is enabled" do + 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) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq root_path @@ -72,27 +80,27 @@ feature 'OAuth Login', feature: true, js: true do end context 'when "remember me" is not checked' do - context "when two-factor authentication is disabled" 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) - restart_browser + 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 'remembers the user after a browser restart' do + 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) - restart_browser + clear_browser_session visit(root_path) expect(current_path).to eq new_user_session_path diff --git a/spec/support/capybara_helpers.rb b/spec/support/capybara_helpers.rb index 1037e9def8c..3eb7bea3227 100644 --- a/spec/support/capybara_helpers.rb +++ b/spec/support/capybara_helpers.rb @@ -37,7 +37,7 @@ module CapybaraHelpers end # Simulate a browser restart by clearing the session cookie. - def restart_browser + def clear_browser_session page.driver.remove_cookie('_gitlab_session') end end diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 789cf9baae2..184c7b5125a 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -67,7 +67,7 @@ module LoginHelpers visit new_user_session_path expect(page).to have_content('Sign in with') - check "Remember Me" if remember_me + check 'Remember Me' if remember_me click_link "oauth-login-#{provider}" end From 60cb3e910a5781b6afeb4edcbda425d604581153 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 16:23:28 +0000 Subject: [PATCH 11/13] Implement review comments for !11963 from @filipa. - Disable an ESLint check rather than work around it (by converting `OAuthRememberMe` from a regular class to a static class. - Scope `$` calls inside `OAuthRememberMe` --- app/assets/javascripts/dispatcher.js | 2 +- app/assets/javascripts/oauth_remember_me.js | 7 ++++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/assets/javascripts/dispatcher.js b/app/assets/javascripts/dispatcher.js index a58d1be68b5..e924fde60bf 100644 --- a/app/assets/javascripts/dispatcher.js +++ b/app/assets/javascripts/dispatcher.js @@ -128,7 +128,7 @@ import OAuthRememberMe from './oauth_remember_me'; case 'sessions:new': new UsernameValidator(); new ActiveTabMemoizer(); - new OAuthRememberMe({ container: $("#remember_me") }).bindEvents(); + 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 index 8f4796f2ede..ffc2dd6bbca 100644 --- a/app/assets/javascripts/oauth_remember_me.js +++ b/app/assets/javascripts/oauth_remember_me.js @@ -12,13 +12,14 @@ export default class OAuthRememberMe { } bindEvents() { - this.container.on('click', this.constructor.toggleRememberMe); + $('#remember_me', this.container).on('click', this.toggleRememberMe); } - static toggleRememberMe(event) { + // eslint-disable-next-line class-methods-use-this + toggleRememberMe(event) { const rememberMe = $(event.target).is(':checked'); - $('.oauth-login').each((i, element) => { + $('.oauth-login', this.container).each((i, element) => { const href = $(element).attr('href'); if (rememberMe) { From 9a0f5bd55bc9f1514909a59283fbc0651e11c127 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 19:37:37 +0000 Subject: [PATCH 12/13] Fix build for !11963. - Don't use `request.env['omniauth.params']` if it isn't present. - Remove the `saml` section from the `gitlab.yml` test section. Some tests depend on this section not being initially present, so it can be overridden in the test. This MR doesn't add any tests for SAML, so we didn't really need this in the first place anyway. - Clean up the test -> omniauth section of `gitlab.yml` --- .../omniauth_callbacks_controller.rb | 2 +- config/gitlab.yml.example | 25 +++---------------- spec/support/login_helpers.rb | 3 ++- 3 files changed, 6 insertions(+), 24 deletions(-) diff --git a/app/controllers/omniauth_callbacks_controller.rb b/app/controllers/omniauth_callbacks_controller.rb index c5adadfa529..323d5d26eb6 100644 --- a/app/controllers/omniauth_callbacks_controller.rb +++ b/app/controllers/omniauth_callbacks_controller.rb @@ -153,6 +153,6 @@ class OmniauthCallbacksController < Devise::OmniauthCallbacksController def remember_me? request_params = request.env['omniauth.params'] - request_params['remember_me'] == '1' + (request_params['remember_me'] == '1') if request_params.present? end end diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index b58a173bccb..950a58bb0dd 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -619,15 +619,12 @@ test: omniauth: enabled: true allow_single_sign_on: true - block_auto_created_users: false - auto_link_saml_user: true external_providers: [] providers: - { name: 'cas3', label: 'cas3', - args: { - url: 'https://sso.example.com', + args: { url: 'https://sso.example.com', disable_ssl_verification: false, login_url: '/cas/login', service_validate_url: '/cas/p3/serviceValidate', @@ -635,11 +632,7 @@ test: - { name: 'authentiq', app_id: 'YOUR_CLIENT_ID', app_secret: 'YOUR_CLIENT_SECRET', - args: { - scope: 'aq:name email~rs address aq:push' - } - } - + args: { scope: 'aq:name email~rs address aq:push' } } - { name: 'github', app_id: 'YOUR_APP_ID', app_secret: 'YOUR_APP_SECRET', @@ -663,24 +656,12 @@ test: - { name: 'twitter', app_id: 'YOUR_APP_ID', app_secret: 'YOUR_APP_SECRET' } - - - { name: 'saml', - label: 'Our SAML Provider', - groups_attribute: 'Groups', - external_groups: ['Contractors', 'Freelancers'], - args: { - assertion_consumer_service_url: 'https://gitlab.example.com/users/auth/saml/callback', - idp_cert_fingerprint: '43:51:43:a1:b5:fc:8b:b7:0a:3a:a9:b1:0f:66:73:a8', - idp_sso_target_url: 'https://login.example.com/idp', - issuer: 'https://gitlab.example.com', - name_identifier_format: 'urn:oasis:names:tc:SAML:2.0:nameid-format:transient' - } } - - { 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/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 184c7b5125a..99e7806353d 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -118,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 From 2ae8158c8c1ac6add71b4f3ef8738c5919bdca4e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 4 Jul 2017 11:59:41 +0000 Subject: [PATCH 13/13] Add Jasmine tests for `OAuthRememberMe` --- .../fixtures/oauth_remember_me.html.haml | 5 ++++ spec/javascripts/oauth_remember_me_spec.js | 26 +++++++++++++++++++ 2 files changed, 31 insertions(+) create mode 100644 spec/javascripts/fixtures/oauth_remember_me.html.haml create mode 100644 spec/javascripts/oauth_remember_me_spec.js 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/'); + }); +});