summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2021-09-29 13:02:17 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2021-09-29 13:02:17 +0000
commit6f10ecdeb6d8636ce7c9fb6cf7930f1a543f58df (patch)
tree959df42c10bab01d1bc81c87ea1ed8f9d3e4e98f
parent003d8b5eac3aa173a7061b82d84ffaf28e8024f6 (diff)
downloadgitlab-ce-6f10ecdeb6d8636ce7c9fb6cf7930f1a543f58df.tar.gz
Add latest changes from gitlab-org/security/gitlab@14-3-stable-ee
-rw-r--r--app/controllers/admin/users_controller.rb6
-rw-r--r--app/controllers/concerns/impersonation.rb6
-rw-r--r--app/controllers/profiles/passwords_controller.rb8
-rw-r--r--app/controllers/projects_controller.rb5
-rw-r--r--app/controllers/uploads_controller.rb6
-rw-r--r--lib/api/projects.rb5
-rw-r--r--locale/gitlab.pot3
-rw-r--r--spec/controllers/admin/users_controller_spec.rb15
-rw-r--r--spec/controllers/projects_controller_spec.rb41
-rw-r--r--spec/controllers/uploads_controller_spec.rb2
-rw-r--r--spec/features/profiles/password_spec.rb78
-rw-r--r--spec/requests/api/projects_spec.rb10
-rw-r--r--spec/support/shared_examples/controllers/uploads_actions_shared_examples.rb10
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)