From 633793cf47b8b02bffc65976cd97c21601661504 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 7 Jun 2017 08:45:34 +0000 Subject: [PATCH 01/14] 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 e936db963e2adb549533cfedcac6f342d7e5e32e Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Wed, 14 Jun 2017 04:30:07 +0000 Subject: [PATCH 02/14] 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 43337c120de9f88b8141b0f8073bfa04a4e23776 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Jun 2017 04:40:47 +0000 Subject: [PATCH 03/14] 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 d705a2548ceebe0fc63356e3e5b0afc54a109d9f Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 15 Jun 2017 12:13:30 +0000 Subject: [PATCH 04/14] 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 fd94855893b96ccab2227330ffd3134a92f4cb45 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 04:40:24 +0000 Subject: [PATCH 05/14] 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 15dba34c9a469c95ea6112419dca33c2c63c6247 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 07:55:09 +0000 Subject: [PATCH 06/14] 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 738789d2cbe04453f96fa86b6f46d9d8213a271b Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 13:10:34 +0000 Subject: [PATCH 07/14] 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 1e9dfb73992e7586c4ee0503357df31925be593f Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 19 Jun 2017 13:15:01 +0000 Subject: [PATCH 08/14] 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 56754848dddb5460500ab056e5ac1b9a61c7ff89 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 20 Jun 2017 03:11:16 +0000 Subject: [PATCH 09/14] 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 8fa08ea3cd81e906c4f4951c70e3571defeab7c7 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 30 Jun 2017 16:36:36 +0000 Subject: [PATCH 10/14] 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 f1caa0b316c0be7c957e34a4bcc9f392023379d3 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 16:23:28 +0000 Subject: [PATCH 11/14] 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 7c2f5bb48d98426b8458782216311f24aa705209 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Mon, 3 Jul 2017 19:37:37 +0000 Subject: [PATCH 12/14] 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 8d49a9fd580212b12bd719e6b310646e285c36b7 Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Tue, 4 Jul 2017 11:59:41 +0000 Subject: [PATCH 13/14] 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/'); + }); +}); From 89b0c987fcba5692842f83cfaba90a9004ac91de Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Thu, 6 Jul 2017 06:23:20 +0000 Subject: [PATCH 14/14] Remove Authentiq from the OAuth login integration tests. - This is causing autoload-related errors in the `migration:path` builds. We need to find a better way of testing this provider. --- config/gitlab.yml.example | 4 ---- spec/features/oauth_login_spec.rb | 2 +- 2 files changed, 1 insertion(+), 5 deletions(-) diff --git a/config/gitlab.yml.example b/config/gitlab.yml.example index 950a58bb0dd..bdf8bcf4931 100644 --- a/config/gitlab.yml.example +++ b/config/gitlab.yml.example @@ -629,10 +629,6 @@ test: 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', diff --git a/spec/features/oauth_login_spec.rb b/spec/features/oauth_login_spec.rb index 452b920307c..1b6d1f3415f 100644 --- a/spec/features/oauth_login_spec.rb +++ b/spec/features/oauth_login_spec.rb @@ -13,7 +13,7 @@ feature 'OAuth Login', js: true do end providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, - :facebook, :authentiq, :cas3, :auth0] + :facebook, :cas3, :auth0] before(:all) do # The OmniAuth `full_host` parameter doesn't get set correctly (it gets set to something like `http://localhost`