From 8236be82c7c687a305d16c15fd647b9d5193a1cc Mon Sep 17 00:00:00 2001
From: Douwe Maan <douwe@gitlab.com>
Date: Fri, 11 May 2018 14:37:14 +0000
Subject: 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
---
 app/helpers/users_helper.rb                        | 19 +----
 app/models/user.rb                                 |  5 ++
 app/policies/global_policy.rb                      | 19 +++--
 .../unreleased/bvl-restrict-api-git-for-terms.yml  |  6 ++
 lib/api/api_guard.rb                               | 12 ++-
 lib/gitlab/auth/user_access_denied_reason.rb       | 33 ++++++++
 lib/gitlab/build_access.rb                         | 12 +++
 lib/gitlab/git_access.rb                           | 12 +--
 .../gitlab/auth/user_access_denied_reason_spec.rb  | 34 ++++++++
 spec/lib/gitlab/build_access_spec.rb               | 23 ++++++
 spec/lib/gitlab/git_access_spec.rb                 | 94 +++++++++++++++++++++-
 spec/models/user_spec.rb                           | 27 +++++++
 spec/policies/global_policy_spec.rb                | 90 +++++++++++++++++++++
 spec/requests/api/helpers_spec.rb                  | 18 +++++
 spec/requests/git_http_spec.rb                     | 53 ++++++++++++
 15 files changed, 429 insertions(+), 28 deletions(-)
 create mode 100644 changelogs/unreleased/bvl-restrict-api-git-for-terms.yml
 create mode 100644 lib/gitlab/auth/user_access_denied_reason.rb
 create mode 100644 lib/gitlab/build_access.rb
 create mode 100644 spec/lib/gitlab/auth/user_access_denied_reason_spec.rb
 create mode 100644 spec/lib/gitlab/build_access_spec.rb

diff --git a/app/helpers/users_helper.rb b/app/helpers/users_helper.rb
index e803cd3a8d8..ce9373f5883 100644
--- a/app/helpers/users_helper.rb
+++ b/app/helpers/users_helper.rb
@@ -42,22 +42,11 @@ module UsersHelper
 
     items << :sign_out if current_user
 
-    # TODO: Remove these conditions when the permissions are prevented in
-    # 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?
+    return items if current_user&.required_terms_not_accepted?
 
-    items << :help if required_terms_accepted
-
-    if can?(current_user, :read_user, current_user) && required_terms_accepted
-      items << :profile
-    end
-
-    if can?(current_user, :update_user, current_user) && required_terms_accepted
-      items << :settings
-    end
+    items << :help
+    items << :profile if can?(current_user, :read_user, current_user)
+    items << :settings if can?(current_user, :update_user, current_user)
 
     items
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index a9cfd39f604..884f3bbb364 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -1193,6 +1193,11 @@ class User < ActiveRecord::Base
     accepted_term_id.present?
   end
 
+  def required_terms_not_accepted?
+    Gitlab::CurrentSettings.current_application_settings.enforce_terms? &&
+      !terms_accepted?
+  end
+
   protected
 
   # override, from Devise::Validatable
diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index 64e550d19d0..1cf5515d9d7 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -1,22 +1,24 @@
 class GlobalPolicy < BasePolicy
   desc "User is blocked"
   with_options scope: :user, score: 0
-  condition(:blocked) { @user.blocked? }
+  condition(:blocked) { @user&.blocked? }
 
   desc "User is an internal user"
   with_options scope: :user, score: 0
-  condition(:internal) { @user.internal? }
+  condition(:internal) { @user&.internal? }
 
   desc "User's access has been locked"
   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
     prevent :log_in
-    prevent :access_api
-    prevent :access_git
     prevent :receive_notifications
     prevent :use_quick_actions
     prevent :create_group
@@ -38,6 +40,11 @@ class GlobalPolicy < BasePolicy
     prevent :use_quick_actions
   end
 
+  rule { required_terms_not_accepted }.policy do
+    prevent :access_api
+    prevent :access_git
+  end
+
   rule { can_create_group }.policy do
     enable :create_group
   end
diff --git a/changelogs/unreleased/bvl-restrict-api-git-for-terms.yml b/changelogs/unreleased/bvl-restrict-api-git-for-terms.yml
new file mode 100644
index 00000000000..49cd04b065b
--- /dev/null
+++ b/changelogs/unreleased/bvl-restrict-api-git-for-terms.yml
@@ -0,0 +1,6 @@
+---
+title: Block access to the API & git for users that did not accept enforced Terms
+  of Service
+merge_request: 18816
+author:
+type: other
diff --git a/lib/api/api_guard.rb b/lib/api/api_guard.rb
index c2113551207..c17089759de 100644
--- a/lib/api/api_guard.rb
+++ b/lib/api/api_guard.rb
@@ -45,7 +45,9 @@ module API
         user = find_user_from_sources
         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
       end
@@ -72,6 +74,14 @@ module API
             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
 
     module ClassMethods
diff --git a/lib/gitlab/auth/user_access_denied_reason.rb b/lib/gitlab/auth/user_access_denied_reason.rb
new file mode 100644
index 00000000000..af310aa12fc
--- /dev/null
+++ b/lib/gitlab/auth/user_access_denied_reason.rb
@@ -0,0 +1,33 @@
+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
diff --git a/lib/gitlab/build_access.rb b/lib/gitlab/build_access.rb
new file mode 100644
index 00000000000..08a8f846ca5
--- /dev/null
+++ b/lib/gitlab/build_access.rb
@@ -0,0 +1,12 @@
+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
diff --git a/lib/gitlab/git_access.rb b/lib/gitlab/git_access.rb
index 0d1ee73ca1a..db7c29be94b 100644
--- a/lib/gitlab/git_access.rb
+++ b/lib/gitlab/git_access.rb
@@ -2,8 +2,6 @@
 # class return an instance of `GitlabAccessStatus`
 module Gitlab
   class GitAccess
-    include Gitlab::Utils::StrongMemoize
-
     UnauthorizedError = Class.new(StandardError)
     NotFoundError = Class.new(StandardError)
     ProjectCreationError = Class.new(StandardError)
@@ -17,7 +15,6 @@ module Gitlab
       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.',
       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.",
       upload_pack_disabled_over_http: 'Pulling over HTTP is not allowed.',
       receive_pack_disabled_over_http: 'Pushing over HTTP is not allowed.',
@@ -108,8 +105,11 @@ module Gitlab
     end
 
     def check_active_user!
-      if user && !user_access.allowed?
-        raise UnauthorizedError, ERROR_MESSAGES[:account_blocked]
+      return unless user
+
+      unless user_access.allowed?
+        message = Gitlab::Auth::UserAccessDeniedReason.new(user).rejection_message
+        raise UnauthorizedError, message
       end
     end
 
@@ -340,6 +340,8 @@ module Gitlab
     def user_access
       @user_access ||= if ci?
                          CiAccess.new
+                       elsif user && request_from_ci_build?
+                         BuildAccess.new(user, project: project)
                        else
                          UserAccess.new(user, project: project)
                        end
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/build_access_spec.rb b/spec/lib/gitlab/build_access_spec.rb
new file mode 100644
index 00000000000..08f50bf4fac
--- /dev/null
+++ b/spec/lib/gitlab/build_access_spec.rb
@@ -0,0 +1,23 @@
+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
diff --git a/spec/lib/gitlab/git_access_spec.rb b/spec/lib/gitlab/git_access_spec.rb
index 6c625596605..317a932d5a6 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,96 @@ 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
+
+    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
 
   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..873673b50ef 100644
--- a/spec/policies/global_policy_spec.rb
+++ b/spec/policies/global_policy_spec.rb
@@ -90,4 +90,94 @@ describe GlobalPolicy do
       it { is_expected.to be_allowed(:update_custom_attribute) }
     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
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)
diff --git a/spec/requests/git_http_spec.rb b/spec/requests/git_http_spec.rb
index 494db30e8e0..2514dab1714 100644
--- a/spec/requests/git_http_spec.rb
+++ b/spec/requests/git_http_spec.rb
@@ -1,6 +1,7 @@
 require "spec_helper"
 
 describe 'Git HTTP requests' do
+  include TermsHelper
   include GitHttpHelpers
   include WorkhorseHelpers
   include UserActivitiesHelpers
@@ -824,4 +825,56 @@ describe 'Git HTTP requests' do
       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
-- 
cgit v1.2.3