Commit f7f13f9d authored by Bob Van Landuyt's avatar Bob Van Landuyt

Block access to API & git when terms are enforced

When terms are enforced, but the user has not accepted the terms
access to the API & git is rejected with a message directing the user
to the web app to accept the terms.
parent f667bbce
...@@ -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
......
...@@ -1193,6 +1193,11 @@ class User < ActiveRecord::Base ...@@ -1193,6 +1193,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
......
...@@ -13,6 +13,10 @@ class GlobalPolicy < BasePolicy ...@@ -13,6 +13,10 @@ class GlobalPolicy < BasePolicy
condition(:can_create_fork, scope: :user) { @user.manageable_namespaces.any? { |namespace| @user.can?(:create_projects, namespace) } } condition(:can_create_fork, scope: :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_api
...@@ -38,6 +42,11 @@ class GlobalPolicy < BasePolicy ...@@ -38,6 +42,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
...@@ -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.',
...@@ -109,7 +106,8 @@ module Gitlab ...@@ -109,7 +106,8 @@ module Gitlab
def check_active_user! def check_active_user!
if user && !user_access.allowed? if user && !user_access.allowed?
raise UnauthorizedError, ERROR_MESSAGES[:account_blocked] message = Gitlab::Auth::UserAccessDeniedReason.new(user).rejection_message
raise UnauthorizedError, message
end end
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' 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,80 @@ describe Gitlab::GitAccess do ...@@ -1040,6 +1042,80 @@ 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
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,68 @@ describe GlobalPolicy do ...@@ -90,4 +90,68 @@ 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
describe 'API access' do
describe 'regular user' do
it { is_expected.to be_allowed(:access_api) }
end
describe 'admin' do
let(:current_user) { create(:admin) }
it { is_expected.to be_allowed(:access_api) }
end
describe 'anonymous' do
let(:current_user) { nil }
it { is_expected.not_to be_allowed(:access_api) }
end
context 'when terms are enforced' do
before do
enforce_terms
end
it { is_expected.not_to be_allowed(:access_api) }
it 'allows access to the API when the user accepted the terms' do
accept_terms(current_user)
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.not_to be_allowed(:access_git) }
end
context 'when terms are enforced' do
before do
enforce_terms
end
it { is_expected.not_to be_allowed(:access_git) }
it 'allows access to git when terms are accepted' do
accept_terms(current_user)
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)
......
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