Skip to content
Snippets Groups Projects
Commit 494c6dde authored by Darby Frey's avatar Darby Frey
Browse files

Refactoring and test clean up

parent a2eba097
No related branches found
No related tags found
1 merge request!82443Remove Secure Files when a project is destroyed
Hide whitespace changes
Inline Side-by-side
app/controllers/projects/ci/secure_files_controller.rb
+ 1
− 7
View file @ 494c6dde
# frozen_string_literal: true
class Projects::Ci::SecureFilesController < Projects::ApplicationController
before_action :check_can_collaborate!
before_action :authorize_read_secure_files!
feature_category :pipeline_authoring
def show
end
private
def check_can_collaborate!
render_404 unless can?(current_user, :read_secure_files, project)
end
end
app/policies/project_policy.rb
+ 0
− 1
View file @ 494c6dde
......@@ -463,7 +463,6 @@ 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
+ 6
− 10
View file @ 494c6dde
......@@ -52,17 +52,18 @@ class SecureFiles < ::API::Base
body secure_file.file.read
end
# Additional authorization check for admin endpoints
# All APIs defined below this block will require admin level permissions
before do
authorize! :admin_secure_files, user_project
end
desc 'Upload a Secure File'
params do
requires :name, type: String, desc: 'The name of the file'
requires :file, types: [Rack::Multipart::UploadedFile, ::API::Validations::Types::WorkhorseFile], desc: 'The secure file to be uploaded'
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(
......@@ -82,11 +83,6 @@ class SecureFiles < ::API::Base
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
+ 63
− 72
View file @ 494c6dde
......@@ -8,69 +8,72 @@
stub_feature_flags(ci_secure_files: true)
end
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(:maintainer) { create(:user) }
let_it_be(:developer) { create(:user) }
let_it_be(:guest) { create(:user) }
let_it_be(:anonymous) { create(:user) }
let_it_be(:project) { create(:project, creator_id: maintainer.id) }
let_it_be(:secure_file) { create(:ci_secure_file, project: project) }
before_all do
project.add_maintainer(maintainer)
project.add_developer(developer)
project.add_guest(guest)
end
describe 'GET /projects/:id/secure_files' do
context 'feature flag' do
it 'returns a 503 when the feature flag is disabled' do
stub_feature_flags(ci_secure_files: false)
get api("/projects/#{project.id}/secure_files", user)
get api("/projects/#{project.id}/secure_files", maintainer)
expect(response).to have_gitlab_http_status(:service_unavailable)
end
it 'returns a 200 when the feature flag is enabled' do
get api("/projects/#{project.id}/secure_files", user)
get api("/projects/#{project.id}/secure_files", maintainer)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_a(Array)
end
end
context 'authorized user with admin permissions' do
context 'authenticated user with admin permissions' do
it 'returns project secure files' do
get api("/projects/#{project.id}/secure_files", user)
get api("/projects/#{project.id}/secure_files", maintainer)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_a(Array)
end
end
context 'authorized user with read permissions' do
it 'does not return project secure files' do
get api("/projects/#{project.id}/secure_files", user2)
context 'authenticated user with read permissions' do
it 'returns project secure files' do
get api("/projects/#{project.id}/secure_files", developer)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response).to be_a(Array)
end
end
context 'authorized user with guest permissions' do
context 'authenticated user with guest permissions' do
it 'does not return project secure files' do
get api("/projects/#{project.id}/secure_files", user4)
get api("/projects/#{project.id}/secure_files", guest)
expect(response).to have_gitlab_http_status(:forbidden)
end
end
context 'authorized user with no permissions' do
context 'authenticated user with no permissions' do
it 'does not return project secure files' do
get api("/projects/#{project.id}/secure_files", user3)
get api("/projects/#{project.id}/secure_files", anonymous)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'unauthenticated user' do
it 'does not return project secure files' do
get api("/projects/#{project.id}/secure_files")
......@@ -80,9 +83,9 @@
end
describe 'GET /projects/:id/secure_files/:secure_file_id' do
context 'authorized user with admin permissions' do
context 'authenticated user with admin permissions' do
it 'returns project secure file details' do
get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}", maintainer)
expect(response).to have_gitlab_http_status(:ok)
expect(json_response['name']).to eq(secure_file.name)
......@@ -90,37 +93,31 @@
end
it 'responds with 404 Not Found if requesting non-existing secure file' do
get api("/projects/#{project.id}/secure_files/99999", user)
get api("/projects/#{project.id}/secure_files/#{non_existing_record_id}", maintainer)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'authorized user with read permissions' do
context 'authenticated user with read permissions' do
it 'returns project secure file details' do
get api("/projects/#{project.id}/secure_files/#{secure_file.id}", user2)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}", developer)
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
context 'authenticated 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)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}", anonymous)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'unauthenticated user' do
it 'does not return project secure file details' do
get api("/projects/#{project.id}/secure_files/#{secure_file.id}")
......@@ -130,53 +127,47 @@
end
describe 'GET /projects/:id/secure_files/:secure_file_id/download' do
context 'authorized user with admin permissions' do
context 'authenticated 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)
secure_file.save!
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", maintainer)
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", user)
get api("/projects/#{project.id}/secure_files/#{non_existing_record_id}/download", maintainer)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'authorized user with read permissions' do
context 'authenticated 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)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", developer)
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
context 'authenticated 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)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", anonymous)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'unauthenticated user' do
it 'does not return project secure file details' do
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download")
......@@ -186,7 +177,7 @@
end
describe 'POST /projects/:id/secure_files' do
context 'authorized user with admin permissions' do
context 'authenticated user with admin permissions' do
it 'creates a secure file' do
params = {
file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks'),
......@@ -195,7 +186,7 @@
}
expect do
post api("/projects/#{project.id}/secure_files", user), params: params
post api("/projects/#{project.id}/secure_files", maintainer), params: params
end.to change {project.secure_files.count}.by(1)
expect(response).to have_gitlab_http_status(:created)
......@@ -219,7 +210,7 @@
}
expect do
post api("/projects/#{project.id}/secure_files", user), params: params
post api("/projects/#{project.id}/secure_files", maintainer), params: params
end.to change {project.secure_files.count}.by(1)
expect(json_response['permissions']).to eq('read_only')
......@@ -232,11 +223,11 @@
permissions: 'read_write'
}
post api("/projects/#{project.id}/secure_files", user), params: post_params
post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
secure_file_id = json_response['id']
get api("/projects/#{project.id}/secure_files/#{secure_file_id}/download", user)
get api("/projects/#{project.id}/secure_files/#{secure_file_id}/download", maintainer)
expect(Base64.encode64(response.body)).to eq(Base64.encode64(fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks').read))
end
......@@ -244,7 +235,7 @@
it 'returns an error when the file checksum fails to validate' do
secure_file.update!(checksum: 'foo')
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", user)
get api("/projects/#{project.id}/secure_files/#{secure_file.id}/download", maintainer)
expect(response.code).to eq("500")
end
......@@ -254,7 +245,7 @@
name: 'upload-keystore.jks'
}
post api("/projects/#{project.id}/secure_files", user), params: post_params
post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq('file is missing')
......@@ -265,7 +256,7 @@
file: fixture_file_upload('spec/fixtures/ci_secure_files/upload-keystore.jks')
}
post api("/projects/#{project.id}/secure_files", user), params: post_params
post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq('name is missing')
......@@ -278,7 +269,7 @@
permissions: 'foo'
}
post api("/projects/#{project.id}/secure_files", user), params: post_params
post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
expect(response).to have_gitlab_http_status(:bad_request)
expect(json_response['error']).to eq('permissions does not have a valid value')
......@@ -296,7 +287,7 @@
name: 'upload-keystore.jks'
}
post api("/projects/#{project.id}/secure_files", user), params: post_params
post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
expect(response).to have_gitlab_http_status(:bad_request)
end
......@@ -311,29 +302,29 @@
name: 'upload-keystore.jks'
}
post api("/projects/#{project.id}/secure_files", user), params: post_params
post api("/projects/#{project.id}/secure_files", maintainer), params: post_params
expect(response).to have_gitlab_http_status(:payload_too_large)
end
end
context 'authorized user with read permissions' do
context 'authenticated user with read permissions' do
it 'does not create a secure file' do
post api("/projects/#{project.id}/secure_files", user2)
post api("/projects/#{project.id}/secure_files", developer)
expect(response).to have_gitlab_http_status(:forbidden)
end
end
context 'authorized user with no permissions' do
context 'authenticated user with no permissions' do
it 'does not create a secure file' do
post api("/projects/#{project.id}/secure_files", user3)
post api("/projects/#{project.id}/secure_files", anonymous)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'unauthenticated user' do
it 'does not create a secure file' do
post api("/projects/#{project.id}/secure_files")
......@@ -343,39 +334,39 @@
end
describe 'DELETE /projects/:id/secure_files/:secure_file_id' do
context 'authorized user with admin permissions' do
context 'authenticated user with admin permissions' do
it 'deletes the secure file' do
expect do
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user)
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", maintainer)
expect(response).to have_gitlab_http_status(:no_content)
end.to change {project.secure_files.count}.by(-1)
end
it 'responds with 404 Not Found if requesting non-existing secure_file' do
delete api("/projects/#{project.id}/secure_files/99999", user)
delete api("/projects/#{project.id}/secure_files/#{non_existing_record_id}", maintainer)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'authorized user with read permissions' do
context 'authenticated user with read permissions' do
it 'does not delete the secure_file' do
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user2)
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", developer)
expect(response).to have_gitlab_http_status(:forbidden)
end
end
context 'authorized user with no permissions' do
context 'authenticated user with no permissions' do
it 'does not delete the secure_file' do
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", user3)
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}", anonymous)
expect(response).to have_gitlab_http_status(:not_found)
end
end
context 'unauthorized user' do
context 'unauthenticated user' do
it 'does not delete the secure_file' do
delete api("/projects/#{project.id}/secure_files/#{secure_file.id}")
......
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment