From 60fe4b61b11e87dcae3864e791852bffbd204193 Mon Sep 17 00:00:00 2001 From: Tiago Botelho Date: Thu, 31 Aug 2017 11:21:40 +0100 Subject: Rollsback changes made to signing_enabled. --- app/controllers/application_controller.rb | 2 +- app/controllers/passwords_controller.rb | 10 ++++------ app/controllers/profiles/passwords_controller.rb | 2 +- app/helpers/button_helper.rb | 2 +- app/helpers/projects_helper.rb | 2 +- app/models/user.rb | 4 ++-- app/views/admin/application_settings/_form.html.haml | 2 +- app/views/layouts/nav/_profile.html.haml | 2 +- .../unreleased/37202-revert-changes-to-signing-enabled.yml | 5 +++++ lib/gitlab/auth.rb | 4 ---- spec/controllers/application_controller_spec.rb | 13 +++++++++++-- spec/controllers/passwords_controller_spec.rb | 8 ++++---- spec/features/profiles/password_spec.rb | 4 ++-- spec/lib/gitlab/auth_spec.rb | 10 ---------- 14 files changed, 34 insertions(+), 36 deletions(-) create mode 100644 changelogs/unreleased/37202-revert-changes-to-signing-enabled.yml diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 1d92ea11bda..97922e39ba8 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -202,7 +202,7 @@ class ApplicationController < ActionController::Base end def check_password_expiration - if current_user && current_user.password_expires_at && current_user.password_expires_at < Time.now && current_user.allow_password_authentication? + if current_user && current_user.password_expires_at && current_user.password_expires_at < Time.now && !current_user.ldap_user? return redirect_to new_profile_password_path end end diff --git a/app/controllers/passwords_controller.rb b/app/controllers/passwords_controller.rb index aa8cf630032..fda944adecd 100644 --- a/app/controllers/passwords_controller.rb +++ b/app/controllers/passwords_controller.rb @@ -1,8 +1,6 @@ class PasswordsController < Devise::PasswordsController - include Gitlab::CurrentSettings - before_action :resource_from_email, only: [:create] - before_action :check_password_authentication_available, only: [:create] + before_action :prevent_ldap_reset, only: [:create] before_action :throttle_reset, only: [:create] def edit @@ -40,11 +38,11 @@ class PasswordsController < Devise::PasswordsController self.resource = resource_class.find_by_email(email) end - def check_password_authentication_available - return if current_application_settings.password_authentication_enabled? && (resource.nil? || resource.allow_password_authentication?) + def prevent_ldap_reset + return unless resource&.ldap_user? redirect_to after_sending_reset_password_instructions_path_for(resource_name), - alert: "Password authentication is unavailable." + alert: "Cannot reset password for LDAP user." end def throttle_reset diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index c423761ab24..7beb52dd8e8 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -77,7 +77,7 @@ class Profiles::PasswordsController < Profiles::ApplicationController end def authorize_change_password! - render_404 unless @user.allow_password_authentication? + render_404 if @user.ldap_user? end def user_params diff --git a/app/helpers/button_helper.rb b/app/helpers/button_helper.rb index bf9ad95b7c2..2467cd5418f 100644 --- a/app/helpers/button_helper.rb +++ b/app/helpers/button_helper.rb @@ -50,7 +50,7 @@ module ButtonHelper def http_clone_button(project, placement = 'right', append_link: true) klass = 'http-selector' - klass << ' has-tooltip' if current_user.try(:require_password_creation?) || current_user.try(:require_personal_access_token_creation_for_git_auth?) + klass << ' has-tooltip' if current_user.try(:require_password_creation?) || current_user.try(:require_personal_access_token?) protocol = gitlab_config.protocol.upcase diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb index bee4950e414..fbb5723ff3d 100644 --- a/app/helpers/projects_helper.rb +++ b/app/helpers/projects_helper.rb @@ -215,7 +215,7 @@ module ProjectsHelper def show_no_password_message? cookies[:hide_no_password_message].blank? && !current_user.hide_no_password && - ( current_user.require_password_creation? || current_user.require_personal_access_token_creation_for_git_auth? ) + ( current_user.require_password_creation? || current_user.require_personal_access_token? ) end def link_to_set_password diff --git a/app/models/user.rb b/app/models/user.rb index fbd08bc4d0a..3a067f2332e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -600,8 +600,8 @@ class User < ActiveRecord::Base password_automatically_set? && allow_password_authentication? end - def require_personal_access_token_creation_for_git_auth? - return false if allow_password_authentication? || ldap_user? + def require_personal_access_token? + return false if current_application_settings.password_authentication_enabled? || ldap_user? PersonalAccessTokensFinder.new(user: self, impersonation: false, state: 'active').execute.none? end diff --git a/app/views/admin/application_settings/_form.html.haml b/app/views/admin/application_settings/_form.html.haml index 959af5c0d13..8c2a0975a49 100644 --- a/app/views/admin/application_settings/_form.html.haml +++ b/app/views/admin/application_settings/_form.html.haml @@ -153,7 +153,7 @@ .checkbox = f.label :password_authentication_enabled do = f.check_box :password_authentication_enabled - Password authentication enabled + Sign-in enabled - if omniauth_enabled? && button_based_providers.any? .form-group = f.label :enabled_oauth_sign_in_sources, 'Enabled OAuth sign-in sources', class: 'control-label col-sm-2' diff --git a/app/views/layouts/nav/_profile.html.haml b/app/views/layouts/nav/_profile.html.haml index 26d9640e98a..17f85339870 100644 --- a/app/views/layouts/nav/_profile.html.haml +++ b/app/views/layouts/nav/_profile.html.haml @@ -29,7 +29,7 @@ = link_to profile_emails_path, title: 'Emails' do %span Emails - - if current_user.allow_password_authentication? + - if current_user.ldap_user? = nav_link(controller: :passwords) do = link_to edit_profile_password_path, title: 'Password' do %span diff --git a/changelogs/unreleased/37202-revert-changes-to-signing-enabled.yml b/changelogs/unreleased/37202-revert-changes-to-signing-enabled.yml new file mode 100644 index 00000000000..ddbe79cb498 --- /dev/null +++ b/changelogs/unreleased/37202-revert-changes-to-signing-enabled.yml @@ -0,0 +1,5 @@ +--- +title: Reverts changes made to signin_enabled. +merge_request: 13956 +author: +type: fixed diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 8cb4060cd97..e6e06570f13 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -48,10 +48,6 @@ module Gitlab # Avoid resource intensive login checks if password is not provided return unless password.present? - # Nothing to do here if internal auth is disabled and LDAP is - # not configured - return unless current_application_settings.password_authentication_enabled? || Gitlab::LDAP::Config.enabled? - Gitlab::Auth::UniqueIpsLimiter.limit_user! do user = User.by_login(login) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 331903a5543..59a6cfbf4f5 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -8,34 +8,43 @@ describe ApplicationController do it 'redirects if the user is over their password expiry' do user.password_expires_at = Time.new(2002) + expect(user.ldap_user?).to be_falsey allow(controller).to receive(:current_user).and_return(user) expect(controller).to receive(:redirect_to) expect(controller).to receive(:new_profile_password_path) + controller.send(:check_password_expiration) end it 'does not redirect if the user is under their password expiry' do user.password_expires_at = Time.now + 20010101 + expect(user.ldap_user?).to be_falsey allow(controller).to receive(:current_user).and_return(user) expect(controller).not_to receive(:redirect_to) + controller.send(:check_password_expiration) end it 'does not redirect if the user is over their password expiry but they are an ldap user' do user.password_expires_at = Time.new(2002) + allow(user).to receive(:ldap_user?).and_return(true) allow(controller).to receive(:current_user).and_return(user) expect(controller).not_to receive(:redirect_to) + controller.send(:check_password_expiration) end - it 'does not redirect if the user is over their password expiry but sign-in is disabled' do + it 'redirects if the user is over their password expiry and sign-in is disabled' do stub_application_setting(password_authentication_enabled: false) user.password_expires_at = Time.new(2002) + + expect(user.ldap_user?).to be_falsey allow(controller).to receive(:current_user).and_return(user) - expect(controller).not_to receive(:redirect_to) + expect(controller).to receive(:redirect_to) + expect(controller).to receive(:new_profile_password_path) controller.send(:check_password_expiration) end diff --git a/spec/controllers/passwords_controller_spec.rb b/spec/controllers/passwords_controller_spec.rb index 2955d01fad0..cdaa88bbf5d 100644 --- a/spec/controllers/passwords_controller_spec.rb +++ b/spec/controllers/passwords_controller_spec.rb @@ -1,18 +1,18 @@ require 'spec_helper' describe PasswordsController do - describe '#check_password_authentication_available' do + describe '#prevent_ldap_reset' do before do @request.env["devise.mapping"] = Devise.mappings[:user] end context 'when password authentication is disabled' do - it 'prevents a password reset' do + it 'allows password reset' do stub_application_setting(password_authentication_enabled: false) post :create - expect(flash[:alert]).to eq 'Password authentication is unavailable.' + expect(response).to have_http_status(302) end end @@ -22,7 +22,7 @@ describe PasswordsController do it 'prevents a password reset' do post :create, user: { email: user.email } - expect(flash[:alert]).to eq 'Password authentication is unavailable.' + expect(flash[:alert]).to eq('Cannot reset password for LDAP user.') end end end diff --git a/spec/features/profiles/password_spec.rb b/spec/features/profiles/password_spec.rb index 2c757f99a27..225d4c16841 100644 --- a/spec/features/profiles/password_spec.rb +++ b/spec/features/profiles/password_spec.rb @@ -53,12 +53,12 @@ describe 'Profile > Password' do context 'Regular user' do let(:user) { create(:user) } - it 'renders 404 when sign-in is disabled' do + it 'renders 200 when sign-in is disabled' do stub_application_setting(password_authentication_enabled: false) visit edit_profile_password_path - expect(page).to have_http_status(404) + expect(page).to have_http_status(200) end end diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index 4a498e79c87..f685bb83d0d 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -279,16 +279,6 @@ describe Gitlab::Auth do gl_auth.find_with_user_password('ldap_user', 'password') end end - - context "with sign-in disabled" do - before do - stub_application_setting(password_authentication_enabled: false) - end - - it "does not find user by valid login/password" do - expect(gl_auth.find_with_user_password(username, password)).to be_nil - end - end end private -- cgit v1.2.1