Merge branch '22373-reduce-queries-in-api-helpers-find_project' into 'master'

Improve project policy specs

See merge request !6442

2 files changed, 143 insertions, 22 deletions

diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index be25c750d67..a806cf83782 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -98,7 +98,6 @@ class ProjectPolicy < BasePolicy
     can! :admin_milestone
     can! :admin_project_snippet
     can! :admin_project_member
-    can! :admin_merge_request
     can! :admin_note
     can! :admin_wiki
     can! :admin_project
@@ -139,11 +138,18 @@ class ProjectPolicy < BasePolicy
   def team_access!(user)
     access = project.team.max_member_access(user.id)
 
-    guest_access!                if access >= Gitlab::Access::GUEST
-    reporter_access!             if access >= Gitlab::Access::REPORTER
-    team_member_reporter_access! if access >= Gitlab::Access::REPORTER
-    developer_access!            if access >= Gitlab::Access::DEVELOPER
-    master_access!               if access >= Gitlab::Access::MASTER
+    return if access < Gitlab::Access::GUEST
+    guest_access!
+
+    return if access < Gitlab::Access::REPORTER
+    reporter_access!
+    team_member_reporter_access!
+
+    return if access < Gitlab::Access::DEVELOPER
+    developer_access!
+
+    return if access < Gitlab::Access::MASTER
+    master_access!
   end
 
   def archived_access!
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index a7a06744428..43c8d884a47 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -1,20 +1,68 @@
 require 'spec_helper'
 
 describe ProjectPolicy, models: true do
-  let(:project) { create(:empty_project, :public) }
   let(:guest) { create(:user) }
   let(:reporter) { create(:user) }
   let(:dev) { create(:user) }
   let(:master) { create(:user) }
   let(:owner) { create(:user) }
-  let(:admin) { create(:admin) }
+  let(:project) { create(:empty_project, :public, namespace: owner.namespace) }
 
-  let(:users_ordered_by_permissions) do
-    [nil, guest, reporter, dev, master, owner, admin]
+  let(:guest_permissions) do
+    [
+      :read_project, :read_board, :read_list, :read_wiki, :read_issue, :read_label,
+      :read_milestone, :read_project_snippet, :read_project_member,
+      :read_merge_request, :read_note, :create_project, :create_issue, :create_note,
+      :upload_file
+    ]
   end
 
-  let(:users_permissions) do
-    users_ordered_by_permissions.map { |u| Ability.allowed(u, project).size }
+  let(:reporter_permissions) do
+    [
+      :download_code, :fork_project, :create_project_snippet, :update_issue,
+      :admin_issue, :admin_label, :admin_list, :read_commit_status, :read_build,
+      :read_container_image, :read_pipeline, :read_environment, :read_deployment
+    ]
+  end
+
+  let(:team_member_reporter_permissions) do
+    [
+      :build_download_code, :build_read_container_image
+    ]
+  end
+
+  let(:developer_permissions) do
+    [
+      :admin_merge_request, :update_merge_request, :create_commit_status,
+      :update_commit_status, :create_build, :update_build, :create_pipeline,
+      :update_pipeline, :create_merge_request, :create_wiki, :push_code,
+      :resolve_note, :create_container_image, :update_container_image,
+      :create_environment, :create_deployment
+    ]
+  end
+
+  let(:master_permissions) do
+    [
+      :push_code_to_protected_branches, :update_project_snippet, :update_environment,
+      :update_deployment, :admin_milestone, :admin_project_snippet,
+      :admin_project_member, :admin_note, :admin_wiki, :admin_project,
+      :admin_commit_status, :admin_build, :admin_container_image,
+      :admin_pipeline, :admin_environment, :admin_deployment
+    ]
+  end
+
+  let(:public_permissions) do
+    [
+      :download_code, :fork_project, :read_commit_status, :read_pipeline,
+      :read_container_image, :build_download_code, :build_read_container_image
+    ]
+  end
+
+  let(:owner_permissions) do
+    [
+      :change_namespace, :change_visibility_level, :rename_project, :remove_project,
+      :archive_project, :remove_fork_project, :destroy_merge_request, :destroy_issue
+    ]
   end
 
   before do
@@ -22,16 +70,6 @@ describe ProjectPolicy, models: true do
     project.team << [master, :master]
     project.team << [dev, :developer]
     project.team << [reporter, :reporter]
-
-    group = create(:group)
-    project.project_group_links.create(
-      group: group,
-      group_access: Gitlab::Access::MASTER)
-    group.add_owner(owner)
-  end
-
-  it 'returns increasing permissions for each level' do
-    expect(users_permissions).to eq(users_permissions.sort.uniq)
   end
 
   it 'does not include the read_issue permission when the issue author is not a member of the private project' do
@@ -46,4 +84,81 @@ describe ProjectPolicy, models: true do
 
     expect(Ability.allowed?(user, :read_issue, project)).to be_falsy
   end
+
+  context 'abilities for non-public projects' do
+    let(:project) { create(:empty_project, namespace: owner.namespace) }
+
+    subject { described_class.abilities(current_user, project).to_set }
+
+    context 'with no user' do
+      let(:current_user) { nil }
+
+      it { is_expected.to be_empty }
+    end
+
+    context 'guests' do
+      let(:current_user) { guest }
+
+      it do
+        is_expected.to include(*guest_permissions)
+        is_expected.not_to include(*reporter_permissions)
+        is_expected.not_to include(*team_member_reporter_permissions)
+        is_expected.not_to include(*developer_permissions)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'reporter' do
+      let(:current_user) { reporter }
+
+      it do
+        is_expected.to include(*guest_permissions)
+        is_expected.to include(*reporter_permissions)
+        is_expected.to include(*team_member_reporter_permissions)
+        is_expected.not_to include(*developer_permissions)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'developer' do
+      let(:current_user) { dev }
+
+      it do
+        is_expected.to include(*guest_permissions)
+        is_expected.to include(*reporter_permissions)
+        is_expected.to include(*team_member_reporter_permissions)
+        is_expected.to include(*developer_permissions)
+        is_expected.not_to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'master' do
+      let(:current_user) { master }
+
+      it do
+        is_expected.to include(*guest_permissions)
+        is_expected.to include(*reporter_permissions)
+        is_expected.to include(*team_member_reporter_permissions)
+        is_expected.to include(*developer_permissions)
+        is_expected.to include(*master_permissions)
+        is_expected.not_to include(*owner_permissions)
+      end
+    end
+
+    context 'owner' do
+      let(:current_user) { owner }
+
+      it do
+        is_expected.to include(*guest_permissions)
+        is_expected.to include(*reporter_permissions)
+        is_expected.not_to include(*team_member_reporter_permissions)
+        is_expected.to include(*developer_permissions)
+        is_expected.to include(*master_permissions)
+        is_expected.to include(*owner_permissions)
+      end
+    end
+  end
 end