From 7eb26c7ff7a78df9cb8fe5b30d48c80ce4eb8a99 Mon Sep 17 00:00:00 2001 From: Robin Bobbitt Date: Mon, 12 Jun 2017 12:13:22 -0400 Subject: Provide hint to create a personal access token for Git over HTTP If internal auth is disabled and user is not an LDAP user, present the user with an alert to create a personal access token if he does not have one already. --- app/helpers/button_helper.rb | 11 +++- app/helpers/projects_helper.rb | 17 +++++ app/models/user.rb | 8 ++- app/views/shared/_no_password.html.haml | 7 +- app/views/shared/_no_ssh.html.haml | 4 +- .../unreleased/pat-alert-when-signin-disabled.yml | 4 ++ spec/features/login_spec.rb | 23 +------ spec/features/projects/no_password_spec.rb | 69 ++++++++++++++++++++ spec/helpers/button_helper_spec.rb | 65 ++++++++++++++++++ spec/helpers/projects_helper_spec.rb | 76 ++++++++++++++++++++++ spec/support/login_helpers.rb | 21 ++++++ 11 files changed, 274 insertions(+), 31 deletions(-) create mode 100644 changelogs/unreleased/pat-alert-when-signin-disabled.yml create mode 100644 spec/features/projects/no_password_spec.rb create mode 100644 spec/helpers/button_helper_spec.rb diff --git a/app/helpers/button_helper.rb b/app/helpers/button_helper.rb index 00464810054..ba84dbe4a7a 100644 --- a/app/helpers/button_helper.rb +++ b/app/helpers/button_helper.rb @@ -50,10 +50,17 @@ module ButtonHelper def http_clone_button(project, placement = 'right', append_link: true) klass = 'http-selector' - klass << ' has-tooltip' if current_user.try(:require_password?) + klass << ' has-tooltip' if current_user.try(:require_password?) || current_user.try(:require_personal_access_token?) protocol = gitlab_config.protocol.upcase + tooltip_title = + if current_user.try(:require_password?) + _("Set a password on your account to pull or push via %{protocol}.") % { protocol: protocol } + else + _("Create a personal access token on your account to pull or push via %{protocol}.") % { protocol: protocol } + end + content_tag (append_link ? :a : :span), protocol, class: klass, href: (project.http_url_to_repo if append_link), @@ -61,7 +68,7 @@ module ButtonHelper html: true, placement: placement, container: 'body', - title: _("Set a password on your account to pull or push via %{protocol}") % { protocol: protocol } + title: tooltip_title } end diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index d10e0bd45b0..c04b1419a19 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -198,6 +198,23 @@ module ProjectsHelper .load_in_batch_for_projects(projects) end + def show_no_ssh_key_message? + cookies[:hide_no_ssh_message].blank? && !current_user.hide_no_ssh_key && current_user.require_ssh_key? + end + + def show_no_password_message? + cookies[:hide_no_password_message].blank? && !current_user.hide_no_password && + ( current_user.require_password? || current_user.require_personal_access_token? ) + end + + def link_to_set_password + if current_user.require_password? + link_to s_('SetPasswordToCloneLink|set a password'), edit_profile_password_path + else + link_to s_('CreateTokenToCloneLink|create a personal access token'), profile_personal_access_tokens_path + end + end + private def repo_children_classes(field) diff --git a/app/models/user.rb b/app/models/user.rb index 650b64e7551..6dd1b1415d6 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -570,7 +570,13 @@ class User < ActiveRecord::Base end def require_password? - password_automatically_set? && !ldap_user? + password_automatically_set? && !ldap_user? && current_application_settings.signin_enabled? + end + + def require_personal_access_token? + return false if current_application_settings.signin_enabled? || ldap_user? + + PersonalAccessTokensFinder.new(user: self, impersonation: false, state: 'active').execute.none? end def can_change_username? diff --git a/app/views/shared/_no_password.html.haml b/app/views/shared/_no_password.html.haml index b561e6dc248..9b1a467df6b 100644 --- a/app/views/shared/_no_password.html.haml +++ b/app/views/shared/_no_password.html.haml @@ -1,9 +1,8 @@ -- if cookies[:hide_no_password_message].blank? && !current_user.hide_no_password && current_user.require_password? +- if show_no_password_message? .no-password-message.alert.alert-warning - - set_password_link = link_to s_('SetPasswordToCloneLink|set a password'), edit_profile_password_path - - translation_params = { protocol: gitlab_config.protocol.upcase, set_password_link: set_password_link } + - translation_params = { protocol: gitlab_config.protocol.upcase, set_password_link: link_to_set_password } - set_password_message = _("You won't be able to pull or push project code via %{protocol} until you %{set_password_link} on your account") % translation_params - + = set_password_message.html_safe .alert-link-group = link_to _("Don't show again"), profile_path(user: {hide_no_password: true}), method: :put | diff --git a/app/views/shared/_no_ssh.html.haml b/app/views/shared/_no_ssh.html.haml index e7815e28017..17ef5327341 100644 --- a/app/views/shared/_no_ssh.html.haml +++ b/app/views/shared/_no_ssh.html.haml @@ -1,8 +1,8 @@ -- if cookies[:hide_no_ssh_message].blank? && !current_user.hide_no_ssh_key && current_user.require_ssh_key? +- if show_no_ssh_key_message? .no-ssh-key-message.alert.alert-warning - add_ssh_key_link = link_to s_('MissingSSHKeyWarningLink|add an SSH key'), profile_keys_path, class: 'alert-link' - ssh_message = _("You won't be able to pull or push project code via SSH until you %{add_ssh_key_link} to your profile") % { add_ssh_key_link: add_ssh_key_link } - #{ ssh_message.html_safe } + = ssh_message.html_safe .alert-link-group = link_to _("Don't show again"), profile_path(user: {hide_no_ssh_key: true}), method: :put, class: 'alert-link' | diff --git a/changelogs/unreleased/pat-alert-when-signin-disabled.yml b/changelogs/unreleased/pat-alert-when-signin-disabled.yml new file mode 100644 index 00000000000..dca3670aeb7 --- /dev/null +++ b/changelogs/unreleased/pat-alert-when-signin-disabled.yml @@ -0,0 +1,4 @@ +--- +title: Provide hint to create a personal access token for Git over HTTP +merge_request: 12105 +author: Robin Bobbitt diff --git a/spec/features/login_spec.rb b/spec/features/login_spec.rb index 53b8ba5b0f7..a8055b21cee 100644 --- a/spec/features/login_spec.rb +++ b/spec/features/login_spec.rb @@ -143,29 +143,8 @@ feature 'Login', feature: true do end context 'logging in via OAuth' do - def saml_config - 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' - }) - 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]) + stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config]) user = create(:omniauth_user, :two_factor, extern_uid: 'my-uid', provider: 'saml') gitlab_sign_in_via('saml', user, 'my-uid') diff --git a/spec/features/projects/no_password_spec.rb b/spec/features/projects/no_password_spec.rb new file mode 100644 index 00000000000..30a16e38e3c --- /dev/null +++ b/spec/features/projects/no_password_spec.rb @@ -0,0 +1,69 @@ +require 'spec_helper' + +feature 'No Password Alert' do + let(:project) { create(:project, namespace: user.namespace) } + + context 'with internal auth enabled' do + before do + sign_in(user) + visit namespace_project_path(project.namespace, project) + end + + context 'when user has a password' do + let(:user) { create(:user) } + + it 'shows no alert' do + expect(page).not_to have_content "You won't be able to pull or push project code via HTTP until you set a password on your account" + end + end + + context 'when user has password automatically set' do + let(:user) { create(:user, password_automatically_set: true) } + + it 'shows a password alert' do + expect(page).to have_content "You won't be able to pull or push project code via HTTP until you set a password on your account" + end + end + end + + context 'with internal auth disabled' do + let(:user) { create(:omniauth_user, extern_uid: 'my-uid', provider: 'saml') } + + before do + stub_application_setting(signin_enabled?: false) + stub_omniauth_saml_config(enabled: true, auto_link_saml_user: true, allow_single_sign_on: ['saml'], providers: [mock_saml_config]) + end + + context 'when user has no personal access tokens' do + it 'has a personal access token alert' do + gitlab_sign_in_via('saml', user, 'my-uid') + visit namespace_project_path(project.namespace, project) + + expect(page).to have_content "You won't be able to pull or push project code via HTTP until you create a personal access token on your account" + end + end + + context 'when user has a personal access token' do + it 'shows no alert' do + create(:personal_access_token, user: user) + gitlab_sign_in_via('saml', user, 'my-uid') + visit namespace_project_path(project.namespace, project) + + expect(page).not_to have_content "You won't be able to pull or push project code via HTTP until you create a personal access token on your account" + end + end + end + + context 'when user is ldap user' do + let(:user) { create(:omniauth_user, password_automatically_set: true) } + + before do + sign_in(user) + visit namespace_project_path(project.namespace, project) + end + + it 'shows no alert' do + expect(page).not_to have_content "You won't be able to pull or push project code via HTTP until you" + end + end +end diff --git a/spec/helpers/button_helper_spec.rb b/spec/helpers/button_helper_spec.rb new file mode 100644 index 00000000000..661327d4432 --- /dev/null +++ b/spec/helpers/button_helper_spec.rb @@ -0,0 +1,65 @@ +require 'spec_helper' + +describe ButtonHelper do + describe 'http_clone_button' do + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:has_tooltip_class) { 'has-tooltip' } + + def element + element = helper.http_clone_button(project) + + Nokogiri::HTML::DocumentFragment.parse(element).first_element_child + end + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'with internal auth enabled' do + context 'when user has a password' do + it 'shows no tooltip' do + expect(element.attr('class')).not_to include(has_tooltip_class) + end + end + + context 'when user has password automatically set' do + let(:user) { create(:user, password_automatically_set: true) } + + it 'shows a password tooltip' do + expect(element.attr('class')).to include(has_tooltip_class) + expect(element.attr('data-title')).to eq('Set a password on your account to pull or push via HTTP.') + end + end + end + + context 'with internal auth disabled' do + before do + stub_application_setting(signin_enabled?: false) + end + + context 'when user has no personal access tokens' do + it 'has a personal access token tooltip ' do + expect(element.attr('class')).to include(has_tooltip_class) + expect(element.attr('data-title')).to eq('Create a personal access token on your account to pull or push via HTTP.') + end + end + + context 'when user has a personal access token' do + it 'shows no tooltip' do + create(:personal_access_token, user: user) + + expect(element.attr('class')).not_to include(has_tooltip_class) + end + end + end + + context 'when user is ldap user' do + let(:user) { create(:omniauth_user, password_automatically_set: true) } + + it 'shows no tooltip' do + expect(element.attr('class')).not_to include(has_tooltip_class) + end + end + end +end diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb index 9a4086725d2..487d9800707 100644 --- a/spec/helpers/projects_helper_spec.rb +++ b/spec/helpers/projects_helper_spec.rb @@ -115,6 +115,82 @@ describe ProjectsHelper do end end + describe '#show_no_ssh_key_message?' do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'user has no keys' do + it 'returns true' do + expect(helper.show_no_ssh_key_message?).to be_truthy + end + end + + context 'user has an ssh key' do + it 'returns false' do + create(:personal_key, user: user) + + expect(helper.show_no_ssh_key_message?).to be_falsey + end + end + end + + describe '#show_no_password_message?' do + let(:user) { create(:user) } + + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'user has password set' do + it 'returns false' do + expect(helper.show_no_password_message?).to be_falsey + end + end + + context 'user requires a password' do + let(:user) { create(:user, password_automatically_set: true) } + + it 'returns true' do + expect(helper.show_no_password_message?).to be_truthy + end + end + + context 'user requires a personal access token' do + it 'returns true' do + stub_application_setting(signin_enabled?: false) + + expect(helper.show_no_password_message?).to be_truthy + end + end + end + + describe '#link_to_set_password' do + before do + allow(helper).to receive(:current_user).and_return(user) + end + + context 'user requires a password' do + let(:user) { create(:user, password_automatically_set: true) } + + it 'returns link to set a password' do + expect(helper.link_to_set_password).to match %r{set a password} + end + end + + context 'user requires a personal access token' do + let(:user) { create(:user) } + + it 'returns link to create a personal access token' do + stub_application_setting(signin_enabled?: false) + + expect(helper.link_to_set_password).to match %r{create a personal access token} + end + end + end + describe 'link_to_member' do let(:group) { create(:group) } let(:project) { create(:empty_project, group: group) } diff --git a/spec/support/login_helpers.rb b/spec/support/login_helpers.rb index 879386b5437..4c88958264b 100644 --- a/spec/support/login_helpers.rb +++ b/spec/support/login_helpers.rb @@ -89,4 +89,25 @@ module LoginHelpers }) Rails.application.env_config['omniauth.auth'] = OmniAuth.config.mock_auth[:saml] end + + def mock_saml_config + 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' + }) + end + + def stub_omniauth_saml_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: 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') + end end -- cgit v1.2.1