Commit 0842ab87 authored by Douwe Maan's avatar Douwe Maan

Merge branch 'bvl-restrict-api-git-for-terms' into 'master'

Block access to API & git when terms are enforced

Closes #45849

See merge request gitlab-org/gitlab-ce!18816
parents 26b1ba12 e0768a9b
...@@ -42,22 +42,11 @@ module UsersHelper ...@@ -42,22 +42,11 @@ module UsersHelper
items << :sign_out if current_user items << :sign_out if current_user
# TODO: Remove these conditions when the permissions are prevented in return items if current_user&.required_terms_not_accepted?
# https://gitlab.com/gitlab-org/gitlab-ce/issues/45849
terms_not_enforced = !Gitlab::CurrentSettings
.current_application_settings
.enforce_terms?
required_terms_accepted = terms_not_enforced || current_user.terms_accepted?
items << :help if required_terms_accepted items << :help
items << :profile if can?(current_user, :read_user, current_user)
if can?(current_user, :read_user, current_user) && required_terms_accepted items << :settings if can?(current_user, :update_user, current_user)
items << :profile
end
if can?(current_user, :update_user, current_user) && required_terms_accepted
items << :settings
end
items items
end end
......
...@@ -1197,6 +1197,11 @@ class User < ActiveRecord::Base ...@@ -1197,6 +1197,11 @@ class User < ActiveRecord::Base
accepted_term_id.present? accepted_term_id.present?
end end
def required_terms_not_accepted?
Gitlab::CurrentSettings.current_application_settings.enforce_terms? &&
!terms_accepted?
end
protected protected
# override, from Devise::Validatable # override, from Devise::Validatable
......
class GlobalPolicy < BasePolicy class GlobalPolicy < BasePolicy
desc "User is blocked" desc "User is blocked"
with_options scope: :user, score: 0 with_options scope: :user, score: 0
condition(:blocked) { @user.blocked? } condition(:blocked) { @user&.blocked? }
desc "User is an internal user" desc "User is an internal user"
with_options scope: :user, score: 0 with_options scope: :user, score: 0
condition(:internal) { @user.internal? } condition(:internal) { @user&.internal? }
desc "User's access has been locked" desc "User's access has been locked"
with_options scope: :user, score: 0 with_options scope: :user, score: 0
condition(:access_locked) { @user.access_locked? } condition(:access_locked) { @user&.access_locked? }
condition(:can_create_fork, scope: :user) { @user.manageable_namespaces.any? { |namespace| @user.can?(:create_projects, namespace) } } condition(:can_create_fork, scope: :user) { @user && @user.manageable_namespaces.any? { |namespace| @user.can?(:create_projects, namespace) } }
condition(:required_terms_not_accepted, scope: :user, score: 0) do
@user&.required_terms_not_accepted?
end
rule { anonymous }.policy do rule { anonymous }.policy do
prevent :log_in prevent :log_in
prevent :access_api
prevent :access_git
prevent :receive_notifications prevent :receive_notifications
prevent :use_quick_actions prevent :use_quick_actions
prevent :create_group prevent :create_group
...@@ -38,6 +40,11 @@ class GlobalPolicy < BasePolicy ...@@ -38,6 +40,11 @@ class GlobalPolicy < BasePolicy
prevent :use_quick_actions prevent :use_quick_actions
end end
rule { required_terms_not_accepted }.policy do
prevent :access_api
prevent :access_git
end
rule { can_create_group }.policy do rule { can_create_group }.policy do
enable :create_group enable :create_group
end end
......
---
title: Block access to the API & git for users that did not accept enforced Terms
of Service
merge_request: 18816
author:
type: other
...@@ -45,7 +45,9 @@ module API ...@@ -45,7 +45,9 @@ module API
user = find_user_from_sources user = find_user_from_sources
return unless user return unless user
forbidden!('User is blocked') unless Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api) unless api_access_allowed?(user)
forbidden!(api_access_denied_message(user))
end
user user
end end
...@@ -72,6 +74,14 @@ module API ...@@ -72,6 +74,14 @@ module API
end end
end end
end end
def api_access_allowed?(user)
Gitlab::UserAccess.new(user).allowed? && user.can?(:access_api)
end
def api_access_denied_message(user)
Gitlab::Auth::UserAccessDeniedReason.new(user).rejection_message
end
end end
module ClassMethods module ClassMethods
......
module Gitlab
module Auth
class UserAccessDeniedReason
def initialize(user)
@user = user
end
def rejection_message
case rejection_type
when :internal
'This action cannot be performed by internal users'
when :terms_not_accepted
'You must accept the Terms of Service in order to perform this action. '\
'Please access GitLab from a web browser to accept these terms.'
else
'Your account has been blocked.'
end
end
private
def rejection_type
if @user.internal?
:internal
elsif @user.required_terms_not_accepted?
:terms_not_accepted
else
:blocked
end
end
end
end
end
module Gitlab
class BuildAccess < UserAccess
attr_accessor :user, :project
# This bypasses the `can?(:access_git)`-check we normally do in `UserAccess`
# for CI. That way if a user was able to trigger a pipeline, then the
# build is allowed to clone the project.
def can_access_git?
true
end
end
end
...@@ -2,8 +2,6 @@ ...@@ -2,8 +2,6 @@
# class return an instance of `GitlabAccessStatus` # class return an instance of `GitlabAccessStatus`
module Gitlab module Gitlab
class GitAccess class GitAccess
include Gitlab::Utils::StrongMemoize
UnauthorizedError = Class.new(StandardError) UnauthorizedError = Class.new(StandardError)
NotFoundError = Class.new(StandardError) NotFoundError = Class.new(StandardError)
ProjectCreationError = Class.new(StandardError) ProjectCreationError = Class.new(StandardError)
...@@ -17,7 +15,6 @@ module Gitlab ...@@ -17,7 +15,6 @@ module Gitlab
deploy_key_upload: 'This deploy key does not have write access to this project.', deploy_key_upload: 'This deploy key does not have write access to this project.',
no_repo: 'A repository for this project does not exist yet.', no_repo: 'A repository for this project does not exist yet.',
project_not_found: 'The project you were looking for could not be found.', project_not_found: 'The project you were looking for could not be found.',
account_blocked: 'Your account has been blocked.',
command_not_allowed: "The command you're trying to execute is not allowed.", command_not_allowed: "The command you're trying to execute is not allowed.",
upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.', upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.', receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.',
...@@ -108,8 +105,11 @@ module Gitlab ...@@ -108,8 +105,11 @@ module Gitlab
end end
def check_active_user! def check_active_user!
if user && !user_access.allowed? return unless user
raise UnauthorizedError, ERROR_MESSAGES[:account_blocked]
unless user_access.allowed?
message = Gitlab::Auth::UserAccessDeniedReason.new(user).rejection_message
raise UnauthorizedError, message
end end
end end
...@@ -340,6 +340,8 @@ module Gitlab ...@@ -340,6 +340,8 @@ module Gitlab
def user_access def user_access
@user_access ||= if ci? @user_access ||= if ci?
CiAccess.new CiAccess.new
elsif user && request_from_ci_build?
BuildAccess.new(user, project: project)
else else
UserAccess.new(user, project: project) UserAccess.new(user, project: project)
end end
......
require 'spec_helper'
describe Gitlab::Auth::UserAccessDeniedReason do
include TermsHelper
let(:user) { build(:user) }
let(:reason) { described_class.new(user) }
describe '#rejection_message' do
subject { reason.rejection_message }
context 'when a user is blocked' do
before do
user.block!
end
it { is_expected.to match /blocked/ }
end
context 'a user did not accept the enforced terms' do
before do
enforce_terms
end
it { is_expected.to match /You must accept the Terms of Service/ }
end
context 'when the user is internal' do
let(:user) { User.ghost }
it { is_expected.to match /This action cannot be performed by internal users/ }
end
end
end
require 'spec_helper'
describe Gitlab::BuildAccess do
let(:user) { create(:user) }
let(:project) { create(:project) }
describe '#can_do_action' do
subject { described_class.new(user, project: project).can_do_action?(:download_code) }
context 'when the user can do an action on the project but cannot access git' do
before do
user.block!
project.add_developer(user)
end
it { is_expected.to be(true) }
end
context 'when the user cannot do an action on the project' do
it { is_expected.to be(false) }
end
end
end
require 'spec_helper' require 'spec_helper'
describe Gitlab::GitAccess do describe Gitlab::GitAccess do
set(:user) { create(:user) } include TermsHelper
let(:user) { create(:user) }
let(:actor) { user } let(:actor) { user }
let(:project) { create(:project, :repository) } let(:project) { create(:project, :repository) }
...@@ -1040,6 +1042,96 @@ describe Gitlab::GitAccess do ...@@ -1040,6 +1042,96 @@ describe Gitlab::GitAccess do
end end
end end
context 'terms are enforced' do
before do
enforce_terms
end
shared_examples 'access after accepting terms' do
let(:actions) do
[-> { pull_access_check },
-> { push_access_check }]
end
it 'blocks access when the user did not accept terms', :aggregate_failures do
actions.each do |action|
expect { action.call }.to raise_unauthorized(/You must accept the Terms of Service in order to perform this action/)
end
end
it 'allows access when the user accepted the terms', :aggregate_failures do
accept_terms(user)
actions.each do |action|
expect { action.call }.not_to raise_error
end
end
end
describe 'as an anonymous user to a public project' do
let(:actor) { nil }
let(:project) { create(:project, :public, :repository) }
it { expect { pull_access_check }.not_to raise_error }
end
describe 'as a guest to a public project' do
let(:project) { create(:project, :public, :repository) }
it_behaves_like 'access after accepting terms' do
let(:actions) { [-> { pull_access_check }] }
end
end
describe 'as a reporter to the project' do
before do
project.add_reporter(user)
end
it_behaves_like 'access after accepting terms' do
let(:actions) { [-> { pull_access_check }] }
end
end
describe 'as a developer of the project' do
before do
project.add_developer(user)
end
it_behaves_like 'access after accepting terms'
end
describe 'as a master of the project' do
before do
project.add_master(user)
end
it_behaves_like 'access after accepting terms'
end
describe 'as an owner of the project' do
let(:project) { create(:project, :repository, namespace: user.namespace) }
it_behaves_like 'access after accepting terms'
end
describe 'when a ci build clones the project' do
let(:protocol) { 'http' }
let(:authentication_abilities) { [:build_download_code] }
let(:auth_result_type) { :build }
before do
project.add_developer(user)
end
it "doesn't block http pull" do
aggregate_failures do
expect { pull_access_check }.not_to raise_error
end
end
end
end
private private
def raise_unauthorized(message) def raise_unauthorized(message)
......
...@@ -2,6 +2,7 @@ require 'spec_helper' ...@@ -2,6 +2,7 @@ require 'spec_helper'
describe User do describe User do
include ProjectForksHelper include ProjectForksHelper
include TermsHelper
describe 'modules' do describe 'modules' do
subject { described_class } subject { described_class }
...@@ -2728,4 +2729,30 @@ describe User do ...@@ -2728,4 +2729,30 @@ describe User do
.to change { RedirectRoute.where(path: 'foo').count }.by(-1) .to change { RedirectRoute.where(path: 'foo').count }.by(-1)
end end
end end
describe '#required_terms_not_accepted?' do
let(:user) { build(:user) }
subject { user.required_terms_not_accepted? }
context "when terms are not enforced" do
it { is_expected.to be_falsy }
end
context "when terms are enforced and accepted by the user" do
before do
enforce_terms
accept_terms(user)
end
it { is_expected.to be_falsy }
end
context "when terms are enforced but the user has not accepted" do
before do
enforce_terms
end
it { is_expected.to be_truthy }
end
end
end end
...@@ -90,4 +90,94 @@ describe GlobalPolicy do ...@@ -90,4 +90,94 @@ describe GlobalPolicy do
it { is_expected.to be_allowed(:update_custom_attribute) } it { is_expected.to be_allowed(:update_custom_attribute) }
end end
end end
shared_examples 'access allowed when terms accepted' do |ability|
it { is_expected.not_to be_allowed(ability) }
it "allows #{ability} when the user accepted the terms" do
accept_terms(current_user)
is_expected.to be_allowed(ability)
end
end
describe 'API access' do
context 'regular user' do
it { is_expected.to be_allowed(:access_api) }
end
context 'admin' do
let(:current_user) { create(:admin) }
it { is_expected.to be_allowed(:access_api) }
end
context 'anonymous' do
let(:current_user) { nil }
it { is_expected.to be_allowed(:access_api) }
end
context 'when terms are enforced' do
before do
enforce_terms
end
context 'regular user' do
it_behaves_like 'access allowed when terms accepted', :access_api
end
context 'admin' do
let(:current_user) { create(:admin) }
it_behaves_like 'access allowed when terms accepted', :access_api
end
context 'anonymous' do
let(:current_user) { nil }
it { is_expected.to be_allowed(:access_api) }
end
end
end
describe 'git access' do
describe 'regular user' do
it { is_expected.to be_allowed(:access_git) }
end
describe 'admin' do
let(:current_user) { create(:admin) }
it { is_expected.to be_allowed(:access_git) }
end
describe 'anonymous' do
let(:current_user) { nil }
it { is_expected.to be_allowed(:access_git) }
end
context 'when terms are enforced' do
before do
enforce_terms
end
context 'regular user' do
it_behaves_like 'access allowed when terms accepted', :access_git
end
context 'admin' do
let(:current_user) { create(:admin) }
it_behaves_like 'access allowed when terms accepted', :access_git
end
context 'anonymous' do
let(:current_user) { nil }
it { is_expected.to be_allowed(:access_git) }
end
end
end
end end
...@@ -6,6 +6,7 @@ describe API::Helpers do ...@@ -6,6 +6,7 @@ describe API::Helpers do
include API::APIGuard::HelperMethods include API::APIGuard::HelperMethods
include described_class include described_class
include SentryHelper include SentryHelper
include TermsHelper
let(:user) { create(:user) } let(:user) { create(:user) }
let(:admin) { create(:admin) } let(:admin) { create(:admin) }
...@@ -163,6 +164,23 @@ describe API::Helpers do ...@@ -163,6 +164,23 @@ describe API::Helpers do
expect { current_user }.to raise_error /403/ expect { current_user }.to raise_error /403/
end end
context 'when terms are enforced' do
before do
enforce_terms
env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token
end
it 'returns a 403 when a user has not accepted the terms' do
expect { current_user }.to raise_error /You must accept the Terms of Service/
end
it 'sets the current user when the user accepted the terms' do
accept_terms(user)
expect(current_user).to eq(user)
end
end
it "sets current_user" do it "sets current_user" do
env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token
expect(current_user).to eq(user) expect(current_user).to eq(user)
......
require "spec_helper" require "spec_helper"
describe 'Git HTTP requests' do describe 'Git HTTP requests' do
include TermsHelper
include GitHttpHelpers include GitHttpHelpers
include WorkhorseHelpers include WorkhorseHelpers
include UserActivitiesHelpers include UserActivitiesHelpers
...@@ -824,4 +825,56 @@ describe 'Git HTTP requests' do ...@@ -824,4 +825,56 @@ describe 'Git HTTP requests' do
end end
end end
end end
context 'when terms are enforced' do
let(:project) { create(:project, :repository) }
let(:user) { create(:user) }
let(:path) { "#{project.full_path}.git" }
let(:env) { { user: user.username, password: user.password } }
before do
project.add_master(user)
enforce_terms
end
it 'blocks git access when the user did not accept terms', :aggregate_failures do
clone_get(path, env) do |response|
expect(response).to have_gitlab_http_status(:forbidden)
end
download(path, env) do |response|
expect(response).to have_gitlab_http_status(:forbidden)
end
upload(path, env) do |response|
expect(response).to have_gitlab_http_status(:forbidden)
end
end
context 'when the user accepted the terms' do
before do
accept_terms(user)
end
it 'allows clones' do
clone_get(path, env) do |response|
expect(response).to have_gitlab_http_status(:ok)
end
end
it_behaves_like 'pulls are allowed'
it_behaves_like 'pushes are allowed'
end
context 'from CI' do
let(:build) { create(:ci_build, :running) }
let(:env) { { user: 'gitlab-ci-token', password: build.token } }
before do
build.update!(user: user, project: project)
end
it_behaves_like 'pulls are allowed'
end
end
end end
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment