Updated permissions model for Secure Files

app/controllers/projects/ci/secure_files_controller.rb

@@ -11,6 +11,6 @@ class Projects::Ci::SecureFilesController < Projects::ApplicationController
   private
 
   def check_can_collaborate!
-    render_404 unless can_collaborate_with_project?(project)
+    render_404 unless can?(current_user, :read_secure_files, project)
   end
 end

app/policies/project_policy.rb

@@ -413,6 +413,7 @@ class ProjectPolicy < BasePolicy
     enable :admin_feature_flag
     enable :admin_feature_flags_user_lists
     enable :update_escalation_status
+    enable :read_secure_files
   end
 
   rule { can?(:developer_access) & user_confirmed? }.policy do
@@ -462,6 +463,7 @@ class ProjectPolicy < BasePolicy
     enable :register_project_runners
     enable :update_runners_registration_token
     enable :admin_project_google_cloud
+    enable :read_secure_files
     enable :admin_secure_files
   end
 

lib/api/ci/secure_files.rb

@@ -7,8 +7,8 @@ module API
 
       before do
         authenticate!
-        authorize! :admin_secure_files, user_project
         feature_flag_enabled?
+        authorize! :read_secure_files, user_project
       end
 
       feature_category :pipeline_authoring
@@ -59,6 +59,10 @@ module API
           optional :permissions, type: String, desc: 'The file permissions', default: 'read_only', values: %w[read_only read_write execute]
         end
 
+        before do
+          authorize! :admin_secure_files, user_project
+        end
+
         route_setting :authentication, basic_auth_personal_access_token: true, job_token_allowed: true
         post ':id/secure_files' do
           secure_file = user_project.secure_files.new(
@@ -78,6 +82,11 @@ module API
         end
 
         desc 'Delete an individual Secure File'
+
+        before do
+          authorize! :admin_secure_files, user_project
+        end
+
         route_setting :authentication, basic_auth_personal_access_token: true, job_token_allowed: true
         delete ':id/secure_files/:secure_file_id' do
           secure_file = user_project.secure_files.find(params[:secure_file_id])

spec/requests/api/ci/secure_files_spec.rb

@@ -10,9 +10,12 @@ RSpec.describe API::Ci::SecureFiles do
 
   let_it_be(:user) { create(:user) }
   let_it_be(:user2) { create(:user) }
+  let_it_be(:user3) { create(:user) }
+  let_it_be(:user4) { create(:user) }
   let_it_be(:project) { create(:project, creator_id: user.id) }
   let_it_be(:maintainer) { create(:project_member, :maintainer, user: user, project: project) }
   let_it_be(:developer) { create(:project_member, :developer, user: user2, project: project) }
+  let_it_be(:guest) { create(:project_member, :guest, user: user4, project: project) }
   let_it_be(:secure_file) { create(:ci_secure_file, project: project) }
 
   describe 'GET /projects/:id/secure_files' do
@@ -33,7 +36,7 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
-    context 'authorized user with proper permissions' do
+    context 'authorized user with admin permissions' do
       it 'returns project secure files' do
         get api("/projects/#{project.id}/secure_files", user)
 
@@ -42,14 +45,31 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authorized user with read permissions' do
       it 'does not return project secure files' do
         get api("/projects/#{project.id}/secure_files", user2)
 
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response).to be_a(Array)
+      end
+    end
+
+    context 'authorized user with guest permissions' do
+      it 'does not return project secure files' do
+        get api("/projects/#{project.id}/secure_files", user4)
+
         expect(response).to have_gitlab_http_status(:forbidden)
       end
     end
 
+    context 'authorized user with no permissions' do
+      it 'does not return project secure files' do
+        get api("/projects/#{project.id}/secure_files", user3)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
     context 'unauthorized user' do
       it 'does not return project secure files' do
         get api("/projects/#{project.id}/secure_files")
@@ -60,7 +80,7 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'GET /projects/:id/secure_files/:secure_file_id' do
-    context 'authorized user with proper permissions' do
+    context 'authorized user with admin permissions' do
       it 'returns project secure file details' do
         get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user)
 
@@ -76,11 +96,27 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
-    context 'authorized user with invalid permissions' do
-      it 'does not return project secure file details' do
+    context 'authorized user with read permissions' do
+      it 'returns project secure file details' do
         get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user2)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(json_response['name']).to eq(secure_file.name)
+        expect(json_response['permissions']).to eq(secure_file.permissions)
+      end
+
+      it 'responds with 404 Not Found if requesting non-existing secure file' do
+        get api("/projects/#{project.id}/secure_files/99999", user2)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'authorized user with no permissions' do
+      it 'does not return project secure file details' do
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user3)
+
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
@@ -94,7 +130,7 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'GET /projects/:id/secure_files/:secure_file_id/download' do
-    context 'authorized user with proper permissions' do
+    context 'authorized user with admin permissions' do
       it 'returns a secure file' do
         sample_file = fixture_file('ci_secure_files/upload-keystore.jks')
         secure_file.file = CarrierWaveStringFile.new(sample_file)
@@ -113,11 +149,30 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
-    context 'authorized user with invalid permissions' do
-      it 'does not return project secure file details' do
+    context 'authorized user with read permissions' do
+      it 'returns a secure file' do
+        sample_file = fixture_file('ci_secure_files/upload-keystore.jks')
+        secure_file.file = CarrierWaveStringFile.new(sample_file)
+        secure_file.save!
+
         get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user2)
 
-        expect(response).to have_gitlab_http_status(:forbidden)
+        expect(response).to have_gitlab_http_status(:ok)
+        expect(Base64.encode64(response.body)).to eq(Base64.encode64(sample_file))
+      end
+
+      it 'responds with 404 Not Found if requesting non-existing secure file' do
+        get api("/projects/#{project.id}/secure_files/99999/download", user2)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
+    context 'authorized user with no permissions' do
+      it 'does not return project secure file details' do
+        get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user3)
+
+        expect(response).to have_gitlab_http_status(:not_found)
       end
     end
 
@@ -131,7 +186,7 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'POST /projects/:id/secure_files' do
-    context 'authorized user with proper permissions' do
+    context 'authorized user with admin permissions' do
       it 'creates a secure file' do
         params = {
           file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks'),
@@ -262,7 +317,7 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authorized user with read permissions' do
       it 'does not create a secure file' do
         post api("/projects/#{project.id}/secure_files", user2)
 
@@ -270,6 +325,14 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
+    context 'authorized user with no permissions' do
+      it 'does not create a secure file' do
+        post api("/projects/#{project.id}/secure_files", user3)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
     context 'unauthorized user' do
       it 'does not create a secure file' do
         post api("/projects/#{project.id}/secure_files")
@@ -280,7 +343,7 @@ RSpec.describe API::Ci::SecureFiles do
   end
 
   describe 'DELETE /projects/:id/secure_files/:secure_file_id' do
-    context 'authorized user with proper permissions' do
+    context 'authorized user with admin permissions' do
       it 'deletes the secure file' do
         expect do
           delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user)
@@ -296,7 +359,7 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
-    context 'authorized user with invalid permissions' do
+    context 'authorized user with read permissions' do
       it 'does not delete the secure_file' do
         delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user2)
 
@@ -304,6 +367,14 @@ RSpec.describe API::Ci::SecureFiles do
       end
     end
 
+    context 'authorized user with no permissions' do
+      it 'does not delete the secure_file' do
+        delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user3)
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
+
     context 'unauthorized user' do
       it 'does not delete the secure_file' do
         delete api("/projects/#{project.id}/secure_files/#{secure_file.id}")

spec/services/ci/destroy_secure_file_service_spec.rb

@@ -4,9 +4,11 @@ require 'spec_helper'
 
 RSpec.describe ::Ci::DestroySecureFileService do
   let_it_be(:maintainer_user) { create(:user) }
+  let_it_be(:developer_user) { create(:user) }
   let_it_be(:project) { create(:project) }
   let_it_be(:secure_file) { create(:ci_secure_file, project: project) }
   let_it_be(:project_member) { create(:project_member, :maintainer, user: maintainer_user, project: project) }
+  let_it_be(:project_member2) { create(:project_member, :developer, user: developer_user, project: project) }
 
   subject { described_class.new(project, user).execute(secure_file) }
 
@@ -20,8 +22,8 @@ RSpec.describe ::Ci::DestroySecureFileService do
     end
   end
 
-  context 'user is not owner' do
-    let(:user) { create(:user) }
+  context 'user is a developer' do
+    let(:user) { developer_user }
 
     it 'raises an exception' do
       expect { subject }.to raise_error(Gitlab::Access::AccessDeniedError)