From d801dd177483a8375f1656654ca3638c18550204 Mon Sep 17 00:00:00 2001
From: Bob Van Landuyt <bob@vanlanduyt.co>
Date: Wed, 9 May 2018 12:55:31 +0200
Subject: [PATCH] Allows `access_(git|api)` to anonymous users

The `access_git` and `access_api` were currently never checked for
anonymous users. And they would also be allowed access:

  An anonymous user can clone and pull from a public repo

  An anonymous user can request public information from the API

So the policy didn't actually reflect what we were enforcing.
---
 app/policies/global_policy.rb       | 10 +++---
 spec/policies/global_policy_spec.rb | 52 +++++++++++++++++++++--------
 2 files changed, 43 insertions(+), 19 deletions(-)

diff --git a/app/policies/global_policy.rb b/app/policies/global_policy.rb
index e2736d3bb61..1cf5515d9d7 100644
--- a/app/policies/global_policy.rb
+++ b/app/policies/global_policy.rb
@@ -1,17 +1,17 @@
 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?
@@ -19,8 +19,6 @@ class GlobalPolicy < BasePolicy
 
   rule { anonymous }.policy do
     prevent :log_in
-    prevent :access_api
-    prevent :access_git
     prevent :receive_notifications
     prevent :use_quick_actions
     prevent :create_group
diff --git a/spec/policies/global_policy_spec.rb b/spec/policies/global_policy_spec.rb
index 91d37db035a..873673b50ef 100644
--- a/spec/policies/global_policy_spec.rb
+++ b/spec/policies/global_policy_spec.rb
@@ -91,21 +91,31 @@ describe GlobalPolicy do
     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
-    describe 'regular user' do
+    context 'regular user' do
       it { is_expected.to be_allowed(:access_api) }
     end
 
-    describe 'admin' do
+    context 'admin' do
       let(:current_user) { create(:admin) }
 
       it { is_expected.to be_allowed(:access_api) }
     end
 
-    describe 'anonymous' do
+    context 'anonymous' do
       let(:current_user) { nil }
 
-      it { is_expected.not_to be_allowed(:access_api) }
+      it { is_expected.to be_allowed(:access_api) }
     end
 
     context 'when terms are enforced' do
@@ -113,12 +123,20 @@ describe GlobalPolicy do
         enforce_terms
       end
 
-      it { is_expected.not_to be_allowed(:access_api) }
+      context 'regular user' do
+        it_behaves_like 'access allowed when terms accepted', :access_api
+      end
 
-      it 'allows access to the API when the user accepted the terms' do
-        accept_terms(current_user)
+      context 'admin' do
+        let(:current_user) { create(:admin) }
 
-        is_expected.to be_allowed(:access_api)
+        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
@@ -137,7 +155,7 @@ describe GlobalPolicy do
     describe 'anonymous' do
       let(:current_user) { nil }
 
-      it { is_expected.not_to be_allowed(:access_git) }
+      it { is_expected.to be_allowed(:access_git) }
     end
 
     context 'when terms are enforced' do
@@ -145,12 +163,20 @@ describe GlobalPolicy do
         enforce_terms
       end
 
-      it { is_expected.not_to be_allowed(:access_git) }
+      context 'regular user' do
+        it_behaves_like 'access allowed when terms accepted', :access_git
+      end
 
-      it 'allows access to git when terms are accepted' do
-        accept_terms(current_user)
+      context 'admin' do
+        let(:current_user) { create(:admin) }
 
-        is_expected.to be_allowed(:access_git)
+        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