From f7f13f9db0da92c7b43481dfe5559f317711e533 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Tue, 8 May 2018 15:07:55 +0200 Subject: 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. --- .../gitlab/auth/user_access_denied_reason_spec.rb | 34 ++++++++++ spec/lib/gitlab/git_access_spec.rb | 78 +++++++++++++++++++++- spec/models/user_spec.rb | 27 ++++++++ spec/policies/global_policy_spec.rb | 64 ++++++++++++++++++ spec/requests/api/helpers_spec.rb | 18 +++++ 5 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 spec/lib/gitlab/auth/user_access_denied_reason_spec.rb (limited to 'spec') diff --git a/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb b/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb new file mode 100644 index 00000000000..fa209bed74e --- /dev/null +++ b/spec/lib/gitlab/auth/user_access_denied_reason_spec.rb @@ -0,0 +1,34 @@ +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 diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb index 6c625596605..f421a7135f3 100644 --- a/spec/lib/gitlab/git_access_spec.rb +++ b/spec/lib/gitlab/git_access_spec.rb @@ -1,7 +1,9 @@ require 'spec_helper' describe Gitlab::GitAccess do - set(:user) { create(:user) } + include TermsHelper + + let(:user) { create(:user) } let(:actor) { user } let(:project) { create(:project, :repository) } @@ -1040,6 +1042,80 @@ describe Gitlab::GitAccess do 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 def raise_unauthorized(message) diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 3f2eb58f009..ad094b3ed48 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2,6 +2,7 @@ require 'spec_helper' describe User do include ProjectForksHelper + include TermsHelper describe 'modules' do subject { described_class } @@ -2728,4 +2729,30 @@ describe User do .to change { RedirectRoute.where(path: 'foo').count }.by(-1) 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 diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb index ec26810e371..91d37db035a 100644 --- a/spec/policies/global_policy_spec.rb +++ b/spec/policies/global_policy_spec.rb @@ -90,4 +90,68 @@ describe GlobalPolicy do it { is_expected.to be_allowed(:update_custom_attribute) } 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 diff --git a/spec/requests/api/helpers_spec.rb b/spec/requests/api/helpers_spec.rb index 837389451e8..d3ab44c0d7e 100644 --- a/spec/requests/api/helpers_spec.rb +++ b/spec/requests/api/helpers_spec.rb @@ -6,6 +6,7 @@ describe API::Helpers do include API::APIGuard::HelperMethods include described_class include SentryHelper + include TermsHelper let(:user) { create(:user) } let(:admin) { create(:admin) } @@ -163,6 +164,23 @@ describe API::Helpers do expect { current_user }.to raise_error /403/ 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 env[Gitlab::Auth::UserAuthFinders::PRIVATE_TOKEN_HEADER] = personal_access_token.token expect(current_user).to eq(user) -- cgit v1.2.1