diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 16:02:17 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-09-29 16:02:17 +0300 |
commit | 6f10ecdeb6d8636ce7c9fb6cf7930f1a543f58df (patch) | |
tree | 959df42c10bab01d1bc81c87ea1ed8f9d3e4e98f | |
parent | 003d8b5eac3aa173a7061b82d84ffaf28e8024f6 (diff) |
Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee
-rw-r--r-- | app/controllers/admin/users_controller.rb | 6 | ||||
-rw-r--r-- | app/controllers/concerns/impersonation.rb | 6 | ||||
-rw-r--r-- | app/controllers/profiles/passwords_controller.rb | 8 | ||||
-rw-r--r-- | app/controllers/projects_controller.rb | 5 | ||||
-rw-r--r-- | app/controllers/uploads_controller.rb | 6 | ||||
-rw-r--r-- | lib/api/projects.rb | 5 | ||||
-rw-r--r-- | locale/gitlab.pot | 3 | ||||
-rw-r--r-- | spec/controllers/admin/users_controller_spec.rb | 15 | ||||
-rw-r--r-- | spec/controllers/projects_controller_spec.rb | 41 | ||||
-rw-r--r-- | spec/controllers/uploads_controller_spec.rb | 2 | ||||
-rw-r--r-- | spec/features/profiles/password_spec.rb | 78 | ||||
-rw-r--r-- | spec/requests/api/projects_spec.rb | 10 | ||||
-rw-r--r-- | spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb | 10 |
13 files changed, 166 insertions, 29 deletions
diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index dfc1434d909..cdfb3a32f4c 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -45,7 +45,7 @@ class Admin::UsersController < Admin::ApplicationController end def impersonate - if can?(user, :log_in) + if can?(user, :log_in) && !impersonation_in_progress? session[:impersonator_id] = current_user.id warden.set_user(user, scope: :user) @@ -58,7 +58,9 @@ class Admin::UsersController < Admin::ApplicationController redirect_to root_path else flash[:alert] = - if user.blocked? + if impersonation_in_progress? + _("You are already impersonating another user") + elsif user.blocked? _("You cannot impersonate a blocked user") elsif user.internal? _("You cannot impersonate an internal user") diff --git a/app/controllers/concerns/impersonation.rb b/app/controllers/concerns/impersonation.rb index a8788e7f8bd..539dd9ad69d 100644 --- a/app/controllers/concerns/impersonation.rb +++ b/app/controllers/concerns/impersonation.rb @@ -20,7 +20,7 @@ module Impersonation protected def check_impersonation_availability - return unless session[:impersonator_id] + return unless impersonation_in_progress? unless Gitlab.config.gitlab.impersonation_enabled stop_impersonation @@ -38,6 +38,10 @@ module Impersonation current_user end + def impersonation_in_progress? + session[:impersonator_id].present? + end + def log_impersonation_event Gitlab::AppLogger.info("User #{impersonator.username} has stopped impersonating #{current_user.username}") end diff --git a/app/controllers/profiles/passwords_controller.rb b/app/controllers/profiles/passwords_controller.rb index 85e901eb3eb..c8c2dd1c7d6 100644 --- a/app/controllers/profiles/passwords_controller.rb +++ b/app/controllers/profiles/passwords_controller.rb @@ -47,6 +47,8 @@ class Profiles::PasswordsController < Profiles::ApplicationController password_attributes[:password_automatically_set] = false unless @user.password_automatically_set || @user.valid_password?(user_params[:current_password]) + handle_invalid_current_password_attempt! + redirect_to edit_profile_password_path, alert: _('You must provide a valid current password') return end @@ -85,6 +87,12 @@ class Profiles::PasswordsController < Profiles::ApplicationController render_404 unless @user.allow_password_authentication? end + def handle_invalid_current_password_attempt! + Gitlab::AppLogger.info(message: 'Invalid current password when attempting to update user password', username: @user.username, ip: request.remote_ip) + + @user.increment_failed_attempts! + end + def user_params params.require(:user).permit(:current_password, :password, :password_confirmation) end diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb index d7c1d87ae4b..de4e51a3a2f 100644 --- a/app/controllers/projects_controller.rb +++ b/app/controllers/projects_controller.rb @@ -19,6 +19,7 @@ class ProjectsController < Projects::ApplicationController before_action :redirect_git_extension, only: [:show] before_action :project, except: [:index, :new, :create, :resolve] before_action :repository, except: [:index, :new, :create, :resolve] + before_action :verify_git_import_enabled, only: [:create] before_action :project_export_enabled, only: [:export, :download_export, :remove_export, :generate_new_export] before_action :present_project, only: [:edit] before_action :authorize_download_code!, only: [:refs] @@ -495,6 +496,10 @@ class ProjectsController < Projects::ApplicationController url_for(safe_params) end + def verify_git_import_enabled + render_404 if project_params[:import_url] && !git_import_enabled? + end + def project_export_enabled render_404 unless Gitlab::CurrentSettings.project_export_enabled? end diff --git a/app/controllers/uploads_controller.rb b/app/controllers/uploads_controller.rb index 4077a3d3dac..d040ac7f76c 100644 --- a/app/controllers/uploads_controller.rb +++ b/app/controllers/uploads_controller.rb @@ -36,14 +36,10 @@ class UploadsController < ApplicationController end def find_model - return unless params[:id] - upload_model_class.find(params[:id]) end def authorize_access! - return unless model - authorized = case model when Note @@ -68,8 +64,6 @@ class UploadsController < ApplicationController end def authorize_create_access! - return unless model - authorized = case model when User diff --git a/lib/api/projects.rb b/lib/api/projects.rb index a92d904be84..34e0b528ced 100644 --- a/lib/api/projects.rb +++ b/lib/api/projects.rb @@ -89,6 +89,10 @@ module API Gitlab::AppLogger.info({ message: "File exceeds maximum size", file_bytes: file.size, project_id: user_project.id, project_path: user_project.full_path, upload_allowed: allowed }) end end + + def check_import_by_url_is_enabled + forbidden! unless Gitlab::CurrentSettings.import_sources&.include?('git') + end end helpers do @@ -267,6 +271,7 @@ module API attrs = declared_params(include_missing: false) attrs = translate_params_for_compatibility(attrs) filter_attributes_using_license!(attrs) + check_import_by_url_is_enabled if params[:import_url].present? project = ::Projects::CreateService.new(current_user, attrs).execute if project.saved? diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 6c0b33911ca..03a26c01cd5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -38380,6 +38380,9 @@ msgstr "" msgid "You are already a member of this %{member_source}." msgstr "" +msgid "You are already impersonating another user" +msgstr "" + msgid "You are an admin, which means granting access to %{client_name} will allow them to interact with GitLab as an admin as well. Proceed with caution." msgstr "" diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index 4d2c311c9a4..3a2b5dcb99d 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/spec/controllers/admin/users_controller_spec.rb @@ -815,5 +815,20 @@ RSpec.describe Admin::UsersController do expect(response).to have_gitlab_http_status(:not_found) end end + + context 'when impersonating an admin and attempting to impersonate again' do + let(:admin2) { create(:admin) } + + before do + post :impersonate, params: { id: admin2.username } + end + + it 'does not allow double impersonation', :aggregate_failures do + post :impersonate, params: { id: user.username } + + expect(flash[:alert]).to eq(_('You are already impersonating another user')) + expect(warden.user).to eq(admin2) + end + end end end diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb index 8afb80d9cc5..9d070061850 100644 --- a/spec/controllers/projects_controller_spec.rb +++ b/spec/controllers/projects_controller_spec.rb @@ -408,6 +408,47 @@ RSpec.describe ProjectsController do end end + describe 'POST create' do + let!(:params) do + { + path: 'foo', + description: 'bar', + import_url: project.http_url_to_repo, + namespace_id: user.namespace.id + } + end + + subject { post :create, params: { project: params } } + + before do + sign_in(user) + end + + context 'when import by url is disabled' do + before do + stub_application_setting(import_sources: []) + end + + it 'does not create project and reports an error' do + expect { subject }.not_to change { Project.count } + + expect(response).to have_gitlab_http_status(:not_found) + end + end + + context 'when import by url is enabled' do + before do + stub_application_setting(import_sources: ['git']) + end + + it 'creates project' do + expect { subject }.to change { Project.count } + + expect(response).to have_gitlab_http_status(:redirect) + end + end + end + describe 'GET edit' do it 'allows an admin user to access the page', :enable_admin_mode do sign_in(create(:user, :admin)) diff --git a/spec/controllers/uploads_controller_spec.rb b/spec/controllers/uploads_controller_spec.rb index 043fd97f1ad..2aa9b86b20e 100644 --- a/spec/controllers/uploads_controller_spec.rb +++ b/spec/controllers/uploads_controller_spec.rb @@ -666,6 +666,6 @@ RSpec.describe UploadsController do def post_authorize(verified: true) request.headers.merge!(workhorse_internal_api_request_header) if verified - post :authorize, params: { model: 'personal_snippet', id: model.id }, format: :json + post :authorize, params: params, format: :json end end diff --git a/spec/features/profiles/password_spec.rb b/spec/features/profiles/password_spec.rb index c9059395377..893dd2c76e0 100644 --- a/spec/features/profiles/password_spec.rb +++ b/spec/features/profiles/password_spec.rb @@ -78,40 +78,80 @@ RSpec.describe 'Profile > Password' do end end - context 'Change passowrd' do + context 'Change password' do + let(:new_password) { '22233344' } + before do sign_in(user) visit(edit_profile_password_path) end - it 'does not change user passowrd without old one' do - page.within '.update-password' do - fill_passwords('22233344', '22233344') + shared_examples 'user enters an incorrect current password' do + subject do + page.within '.update-password' do + fill_in 'user_current_password', with: user_current_password + fill_passwords(new_password, new_password) + end end - page.within '.flash-container' do - expect(page).to have_content 'You must provide a valid current password' - end - end + it 'handles the invalid password attempt, and prompts the user to try again', :aggregate_failures do + expect(Gitlab::AppLogger).to receive(:info) + .with(message: 'Invalid current password when attempting to update user password', username: user.username, ip: user.current_sign_in_ip) + + subject + + user.reload - it 'does not change password with invalid old password' do - page.within '.update-password' do - fill_in 'user_current_password', with: 'invalid' - fill_passwords('password', 'confirmation') + expect(user.failed_attempts).to eq(1) + expect(user.valid_password?(new_password)).to eq(false) + expect(current_path).to eq(edit_profile_password_path) + + page.within '.flash-container' do + expect(page).to have_content('You must provide a valid current password') + end end - page.within '.flash-container' do - expect(page).to have_content 'You must provide a valid current password' + it 'locks the user account when user passes the maximum attempts threshold', :aggregate_failures do + user.update!(failed_attempts: User.maximum_attempts.pred) + + subject + + expect(current_path).to eq(new_user_session_path) + + page.within '.flash-container' do + expect(page).to have_content('Your account is locked.') + end end end - it 'changes user password' do - page.within '.update-password' do - fill_in "user_current_password", with: user.password - fill_passwords('22233344', '22233344') + context 'when current password is blank' do + let(:user_current_password) { nil } + + it_behaves_like 'user enters an incorrect current password' + end + + context 'when current password is incorrect' do + let(:user_current_password) {'invalid' } + + it_behaves_like 'user enters an incorrect current password' + end + + context 'when the password reset is successful' do + subject do + page.within '.update-password' do + fill_in "user_current_password", with: user.password + fill_passwords(new_password, new_password) + end end - expect(current_path).to eq new_user_session_path + it 'changes the password, logs the user out and prompts them to sign in again', :aggregate_failures do + expect { subject }.to change { user.reload.valid_password?(new_password) }.to(true) + expect(current_path).to eq new_user_session_path + + page.within '.flash-container' do + expect(page).to have_content('Password was successfully updated. Please sign in again.') + end + end end end diff --git a/spec/requests/api/projects_spec.rb b/spec/requests/api/projects_spec.rb index 80bccdfee0c..be8a6c7bdcf 100644 --- a/spec/requests/api/projects_spec.rb +++ b/spec/requests/api/projects_spec.rb @@ -1149,6 +1149,16 @@ RSpec.describe API::Projects do expect(response).to have_gitlab_http_status(:bad_request) end + it 'disallows creating a project with an import_url when git import source is disabled' do + stub_application_setting(import_sources: nil) + + project_params = { import_url: 'http://example.com', path: 'path-project-Foo', name: 'Foo Project' } + expect { post api('/projects', user), params: project_params } + .not_to change { Project.count } + + expect(response).to have_gitlab_http_status(:forbidden) + end + it 'sets a project as public' do project = attributes_for(:project, visibility: 'public') diff --git a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb index a20c1d78912..62b35923bcd 100644 --- a/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb +++ b/spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb @@ -323,6 +323,16 @@ RSpec.shared_examples 'handle uploads authorize' do end end + context 'when id is not passed as a param' do + let(:params) { super().without(:id) } + + it 'returns 404 status' do + post_authorize + + expect(response).to have_gitlab_http_status(:not_found) + end + end + context 'when a user can upload a file' do before do sign_in(user) |