From 4c34374d16411e728300be9f709bb3a7d10fbbde Mon Sep 17 00:00:00 2001 From: Timothy Andrew Date: Fri, 30 Jun 2017 16:36:36 +0000 Subject: 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 --- app/views/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 -- cgit v1.2.1