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
This commit is contained in:
Timothy Andrew 2017-06-30 16:36:36 +00:00
parent 56754848dd
commit 8fa08ea3cd
5 changed files with 26 additions and 19 deletions

View file

@ -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}" = 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 %fieldset
= check_box_tag :remember_me = check_box_tag :remember_me
= label_tag :remember_me, "Remember Me" = label_tag :remember_me, 'Remember Me'

View file

@ -42,8 +42,7 @@ namespace :gitlab do
http_clone_url = project.http_url_to_repo http_clone_url = project.http_url_to_repo
ssh_clone_url = project.ssh_url_to_repo ssh_clone_url = project.ssh_url_to_repo
omniauth_providers = Gitlab.config.omniauth.providers.dup omniauth_providers = Gitlab.config.omniauth.providers.map { |provider| provider['name'] }
omniauth_providers.map! { |provider| provider['name'] }
puts "" puts ""
puts "GitLab information".color(:yellow) puts "GitLab information".color(:yellow)

View file

@ -1,27 +1,35 @@
require 'spec_helper' require 'spec_helper'
feature 'OAuth Login', feature: true, js: true do feature 'OAuth Login', js: true do
def enter_code(code) def enter_code(code)
fill_in 'user_otp_attempt', with: code fill_in 'user_otp_attempt', with: code
click_button 'Verify code' click_button 'Verify code'
end end
def stub_omniauth_config(provider) 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['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 end
providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2, providers = [:github, :twitter, :bitbucket, :gitlab, :google_oauth2,
:facebook, :authentiq, :cas3, :auth0] :facebook, :authentiq, :cas3, :auth0]
before(:all) do 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']}.*/, '') } OmniAuth.config.full_host = ->(request) { request['REQUEST_URI'].sub(/#{request['REQUEST_PATH']}.*/, '') }
end end
after(:all) do
OmniAuth.config.full_host = @omniauth_config_full_host
end
providers.each do |provider| providers.each do |provider|
context "when the user logs in using the #{provider} provider" do 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 it 'logs the user in' do
stub_omniauth_config(provider) stub_omniauth_config(provider)
user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) 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
end end
context "when two-factor authentication is enabled" do context 'when two-factor authentication is enabled' do
it 'logs the user in' do it 'logs the user in' do
stub_omniauth_config(provider) stub_omniauth_config(provider)
user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) 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 end
context 'when "remember me" is checked' do 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 it 'remembers the user after a browser restart' do
stub_omniauth_config(provider) stub_omniauth_config(provider)
user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
login_via(provider.to_s, user, 'my-uid', remember_me: true) login_via(provider.to_s, user, 'my-uid', remember_me: true)
restart_browser clear_browser_session
visit(root_path) visit(root_path)
expect(current_path).to eq root_path expect(current_path).to eq root_path
end end
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 it 'remembers the user after a browser restart' do
stub_omniauth_config(provider) stub_omniauth_config(provider)
user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) 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) login_via(provider.to_s, user, 'my-uid', remember_me: true)
enter_code(user.current_otp) enter_code(user.current_otp)
restart_browser clear_browser_session
visit(root_path) visit(root_path)
expect(current_path).to eq root_path expect(current_path).to eq root_path
@ -72,27 +80,27 @@ feature 'OAuth Login', feature: true, js: true do
end end
context 'when "remember me" is not checked' do 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 it 'does not remember the user after a browser restart' do
stub_omniauth_config(provider) stub_omniauth_config(provider)
user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s) user = create(:omniauth_user, extern_uid: 'my-uid', provider: provider.to_s)
login_via(provider.to_s, user, 'my-uid', remember_me: false) login_via(provider.to_s, user, 'my-uid', remember_me: false)
restart_browser clear_browser_session
visit(root_path) visit(root_path)
expect(current_path).to eq new_user_session_path expect(current_path).to eq new_user_session_path
end end
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 it 'does not remember the user after a browser restart' do
stub_omniauth_config(provider) stub_omniauth_config(provider)
user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: provider.to_s) 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) login_via(provider.to_s, user, 'my-uid', remember_me: false)
enter_code(user.current_otp) enter_code(user.current_otp)
restart_browser clear_browser_session
visit(root_path) visit(root_path)
expect(current_path).to eq new_user_session_path expect(current_path).to eq new_user_session_path

View file

@ -37,7 +37,7 @@ module CapybaraHelpers
end end
# Simulate a browser restart by clearing the session cookie. # Simulate a browser restart by clearing the session cookie.
def restart_browser def clear_browser_session
page.driver.remove_cookie('_gitlab_session') page.driver.remove_cookie('_gitlab_session')
end end
end end

View file

@ -67,7 +67,7 @@ module LoginHelpers
visit new_user_session_path visit new_user_session_path
expect(page).to have_content('Sign in with') 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}" click_link "oauth-login-#{provider}"
end end