From a1b7007190e5c54e43fc3d6fbcfa68671a95c62a Mon Sep 17 00:00:00 2001
From: Jan Provaznik <jprovaznik@gitlab.com>
Date: Thu, 28 Jul 2022 12:09:49 +0000
Subject: [PATCH] Add graphql mutation for deleting file uploads
Allows to delete file upload in a project or group.
Changelog: added
---
app/graphql/mutations/uploads/delete.rb | 37 +++++++
app/graphql/types/mutation_type.rb | 1 +
app/graphql/types/upload_type.rb | 16 +++
app/policies/group_policy.rb | 2 +
app/policies/project_policy.rb | 2 +
app/policies/upload_policy.rb | 5 +
app/services/uploads/destroy_service.rb | 51 +++++++++
doc/api/graphql/reference/index.md | 40 +++++++
locale/gitlab.pot | 6 +
spec/graphql/types/upload_type_spec.rb | 13 +++
spec/policies/upload_policy_spec.rb | 76 +++++++++++++
.../graphql/mutations/uploads/delete_spec.rb | 74 +++++++++++++
spec/services/uploads/destroy_service_spec.rb | 103 ++++++++++++++++++
.../policies/group_policy_shared_context.rb | 1 +
.../policies/project_policy_shared_context.rb | 1 +
15 files changed, 428 insertions(+)
create mode 100644 app/graphql/mutations/uploads/delete.rb
create mode 100644 app/graphql/types/upload_type.rb
create mode 100644 app/policies/upload_policy.rb
create mode 100644 app/services/uploads/destroy_service.rb
create mode 100644 spec/graphql/types/upload_type_spec.rb
create mode 100644 spec/policies/upload_policy_spec.rb
create mode 100644 spec/requests/api/graphql/mutations/uploads/delete_spec.rb
create mode 100644 spec/services/uploads/destroy_service_spec.rb
diff --git a/app/graphql/mutations/uploads/delete.rb b/app/graphql/mutations/uploads/delete.rb
new file mode 100644
index 0000000000000..e2fb967cd2c60
--- /dev/null
+++ b/app/graphql/mutations/uploads/delete.rb
@@ -0,0 +1,37 @@
+# frozen_string_literal: true
+
+module Mutations
+ module Uploads
+ class Delete < BaseMutation
+ graphql_name 'UploadDelete'
+ description 'Deletes an upload.'
+
+ include Mutations::ResolvesResourceParent
+
+ authorize :destroy_upload
+
+ argument :secret, GraphQL::Types::String,
+ required: true,
+ description: 'Secret part of upload path.'
+
+ argument :filename, GraphQL::Types::String,
+ required: true,
+ description: 'Upload filename.'
+
+ field :upload, Types::UploadType,
+ null: true,
+ description: 'Deleted upload.'
+
+ def resolve(args)
+ parent = authorized_resource_parent_find!(args)
+
+ result = ::Uploads::DestroyService.new(parent, current_user).execute(args[:secret], args[:filename])
+
+ {
+ upload: result[:status] == :success ? result[:upload] : nil,
+ errors: Array(result[:message])
+ }
+ end
+ end
+ end
+end
diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb
index 3907b096c2c9b..dc9eb369dc890 100644
--- a/app/graphql/types/mutation_type.rb
+++ b/app/graphql/types/mutation_type.rb
@@ -151,6 +151,7 @@ class MutationType < BaseObject
mount_mutation Mutations::SavedReplies::Update
mount_mutation Mutations::Pages::MarkOnboardingComplete
mount_mutation Mutations::SavedReplies::Destroy
+ mount_mutation Mutations::Uploads::Delete
end
end
diff --git a/app/graphql/types/upload_type.rb b/app/graphql/types/upload_type.rb
new file mode 100644
index 0000000000000..68792fa526f64
--- /dev/null
+++ b/app/graphql/types/upload_type.rb
@@ -0,0 +1,16 @@
+# frozen_string_literal: true
+
+module Types
+ class UploadType < BaseObject
+ graphql_name 'FileUpload'
+
+ authorize :read_upload
+
+ field :id, Types::GlobalIDType[::Upload], null: false,
+ description: 'Global ID of the upload.'
+ field :path, GraphQL::Types::String, null: false,
+ description: 'Path of the upload.'
+ field :size, GraphQL::Types::Int, null: false,
+ description: 'Size of the upload in bytes.'
+ end
+end
diff --git a/app/policies/group_policy.rb b/app/policies/group_policy.rb
index 50b6f4bbe1547..0ee759a6ad60d 100644
--- a/app/policies/group_policy.rb
+++ b/app/policies/group_policy.rb
@@ -181,6 +181,8 @@ class GroupPolicy < Namespaces::GroupProjectNamespaceSharedPolicy
enable :create_jira_connect_subscription
enable :maintainer_access
enable :maintain_namespace
+ enable :read_upload
+ enable :destroy_upload
end
rule { owner }.policy do
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 70e8542c44a6a..9d121715d2cf7 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -498,6 +498,8 @@ class ProjectPolicy < BasePolicy
enable :admin_project_google_cloud
enable :admin_secure_files
enable :read_web_hooks
+ enable :read_upload
+ enable :destroy_upload
end
rule { public_project & metrics_dashboard_allowed }.policy do
diff --git a/app/policies/upload_policy.rb b/app/policies/upload_policy.rb
new file mode 100644
index 0000000000000..c7fde5d9df419
--- /dev/null
+++ b/app/policies/upload_policy.rb
@@ -0,0 +1,5 @@
+# frozen_string_literal: true
+
+class UploadPolicy < BasePolicy # rubocop:disable Gitlab/NamespacedClass
+ delegate { @subject.model }
+end
diff --git a/app/services/uploads/destroy_service.rb b/app/services/uploads/destroy_service.rb
new file mode 100644
index 0000000000000..1f0d99ff7bbb2
--- /dev/null
+++ b/app/services/uploads/destroy_service.rb
@@ -0,0 +1,51 @@
+# frozen_string_literal: true
+
+module Uploads
+ class DestroyService < BaseService
+ attr_accessor :model, :current_user
+
+ def initialize(model, user = nil)
+ @model = model
+ @current_user = user
+ end
+
+ def execute(secret, filename)
+ upload = find_upload(secret, filename)
+
+ unless current_user && upload && current_user.can?(:destroy_upload, upload)
+ return error(_("The resource that you are attempting to access does not "\
+ "exist or you don't have permission to perform this action."))
+ end
+
+ if upload.destroy
+ success(upload: upload)
+ else
+ error(_('Upload could not be deleted.'))
+ end
+ end
+
+ private
+
+ # rubocop: disable CodeReuse/ActiveRecord
+ def find_upload(secret, filename)
+ uploader = uploader_class.new(model, secret: secret)
+ upload_paths = uploader.upload_paths(filename)
+
+ Upload.find_by(model: model, uploader: uploader_class.to_s, path: upload_paths)
+ rescue FileUploader::InvalidSecret
+ nil
+ end
+ # rubocop: enable CodeReuse/ActiveRecord
+
+ def uploader_class
+ case model
+ when Group
+ NamespaceFileUploader
+ when Project
+ FileUploader
+ else
+ raise ArgumentError, "unknown uploader for #{model.class.name}"
+ end
+ end
+ end
+end
diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md
index e04ef88b277cc..7ee604207a035 100644
--- a/doc/api/graphql/reference/index.md
+++ b/doc/api/graphql/reference/index.md
@@ -5385,6 +5385,30 @@ Input type: `UpdateSnippetInput`
| <a id="mutationupdatesnippeterrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
| <a id="mutationupdatesnippetsnippet"></a>`snippet` | [`Snippet`](#snippet) | Snippet after mutation. |
+### `Mutation.uploadDelete`
+
+Deletes an upload.
+
+Input type: `UploadDeleteInput`
+
+#### Arguments
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationuploaddeleteclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationuploaddeletefilename"></a>`filename` | [`String!`](#string) | Upload filename. |
+| <a id="mutationuploaddeletegrouppath"></a>`groupPath` | [`ID`](#id) | Full path of the group with which the resource is associated. |
+| <a id="mutationuploaddeleteprojectpath"></a>`projectPath` | [`ID`](#id) | Full path of the project with which the resource is associated. |
+| <a id="mutationuploaddeletesecret"></a>`secret` | [`String!`](#string) | Secret part of upload path. |
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="mutationuploaddeleteclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. |
+| <a id="mutationuploaddeleteerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. |
+| <a id="mutationuploaddeleteupload"></a>`upload` | [`FileUpload`](#fileupload) | Deleted upload. |
+
### `Mutation.userCalloutCreate`
Input type: `UserCalloutCreateInput`
@@ -11845,6 +11869,16 @@ Represents an external issue.
| <a id="externalissueupdatedat"></a>`updatedAt` | [`Time`](#time) | Timestamp of when the issue was updated. |
| <a id="externalissueweburl"></a>`webUrl` | [`String`](#string) | URL to the issue in the external tracker. |
+### `FileUpload`
+
+#### Fields
+
+| Name | Type | Description |
+| ---- | ---- | ----------- |
+| <a id="fileuploadid"></a>`id` | [`UploadID!`](#uploadid) | Global ID of the upload. |
+| <a id="fileuploadpath"></a>`path` | [`String!`](#string) | Path of the upload. |
+| <a id="fileuploadsize"></a>`size` | [`Int!`](#int) | Size of the upload in bytes. |
+
### `GeoNode`
#### Fields
@@ -21247,6 +21281,12 @@ A regexp containing patterns sourced from user input.
### `Upload`
+### `UploadID`
+
+A `UploadID` is a global ID. It is encoded as a string.
+
+An example `UploadID` is: `"gid://gitlab/Upload/1"`.
+
### `UserID`
A `UserID` is a global ID. It is encoded as a string.
diff --git a/locale/gitlab.pot b/locale/gitlab.pot
index 5f0058380c008..d7644247f4b9d 100644
--- a/locale/gitlab.pot
+++ b/locale/gitlab.pot
@@ -39170,6 +39170,9 @@ msgstr ""
msgid "The repository must be accessible over %{code_open}http://%{code_close}, %{code_open}https://%{code_close}, %{code_open}ssh://%{code_close} or %{code_open}git://%{code_close}."
msgstr ""
+msgid "The resource that you are attempting to access does not exist or you don't have permission to perform this action."
+msgstr ""
+
msgid "The same shared runner executes code from multiple projects, unless you configure autoscaling with %{link} set to 1 (which it is on GitLab.com)."
msgstr ""
@@ -41685,6 +41688,9 @@ msgstr ""
msgid "Upload a private key for your certificate"
msgstr ""
+msgid "Upload could not be deleted."
+msgstr ""
+
msgid "Upload file"
msgstr ""
diff --git a/spec/graphql/types/upload_type_spec.rb b/spec/graphql/types/upload_type_spec.rb
new file mode 100644
index 0000000000000..2b959fbf105f2
--- /dev/null
+++ b/spec/graphql/types/upload_type_spec.rb
@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe GitlabSchema.types['FileUpload'] do
+ it { expect(described_class).to require_graphql_authorizations(:read_upload) }
+
+ it 'has the expected fields' do
+ expected_fields = %w[id size path]
+
+ expect(described_class).to include_graphql_fields(*expected_fields)
+ end
+end
diff --git a/spec/policies/upload_policy_spec.rb b/spec/policies/upload_policy_spec.rb
new file mode 100644
index 0000000000000..1169df0b30068
--- /dev/null
+++ b/spec/policies/upload_policy_spec.rb
@@ -0,0 +1,76 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe UploadPolicy do
+ let_it_be(:group) { create(:group) }
+ let_it_be(:project) { create(:project, group: group) }
+ let_it_be(:guest) { create(:user).tap { |user| group.add_guest(user) } }
+ let_it_be(:developer) { create(:user).tap { |user| group.add_developer(user) } }
+ let_it_be(:maintainer) { create(:user).tap { |user| group.add_maintainer(user) } }
+ let_it_be(:owner) { create(:user).tap { |user| group.add_owner(user) } }
+ let_it_be(:admin) { create(:admin) }
+ let_it_be(:non_member_user) { create(:user) }
+
+ let(:upload_permissions) { [:read_upload, :destroy_upload] }
+
+ shared_examples_for 'uploads policy' do
+ subject { described_class.new(current_user, upload) }
+
+ context 'when user is guest' do
+ let(:current_user) { guest }
+
+ it { is_expected.to be_disallowed(*upload_permissions) }
+ end
+
+ context 'when user is developer' do
+ let(:current_user) { developer }
+
+ it { is_expected.to be_disallowed(*upload_permissions) }
+ end
+
+ context 'when user is maintainer' do
+ let(:current_user) { maintainer }
+
+ it { is_expected.to be_allowed(*upload_permissions) }
+ end
+
+ context 'when user is owner' do
+ let(:current_user) { owner }
+
+ it { is_expected.to be_allowed(*upload_permissions) }
+ end
+
+ context 'when user is admin' do
+ let(:current_user) { admin }
+
+ it { is_expected.to be_disallowed(*upload_permissions) }
+
+ context 'with admin mode', :enable_admin_mode do
+ it { is_expected.to be_allowed(*upload_permissions) }
+ end
+ end
+ end
+
+ describe 'destroy_upload' do
+ context 'when deleting project upload' do
+ let_it_be(:upload) { create(:upload, model: project) }
+
+ it_behaves_like 'uploads policy'
+ end
+
+ context 'when deleting group upload' do
+ let_it_be(:upload) { create(:upload, model: group) }
+
+ it_behaves_like 'uploads policy'
+ end
+
+ context 'when deleting upload associated with other model' do
+ let_it_be(:upload) { create(:upload, model: maintainer) }
+
+ subject { described_class.new(maintainer, upload) }
+
+ it { is_expected.to be_disallowed(*upload_permissions) }
+ end
+ end
+end
diff --git a/spec/requests/api/graphql/mutations/uploads/delete_spec.rb b/spec/requests/api/graphql/mutations/uploads/delete_spec.rb
new file mode 100644
index 0000000000000..f44bf179397c5
--- /dev/null
+++ b/spec/requests/api/graphql/mutations/uploads/delete_spec.rb
@@ -0,0 +1,74 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'Delete an upload' do
+ include GraphqlHelpers
+
+ let_it_be(:group) { create(:group) }
+ let_it_be(:project) { create(:project, group: group) }
+ let_it_be(:developer) { create(:user).tap { |user| group.add_developer(user) } }
+ let_it_be(:maintainer) { create(:user).tap { |user| group.add_maintainer(user) } }
+
+ let(:extra_params) { {} }
+ let(:params) { { filename: File.basename(upload.path), secret: upload.secret }.merge(extra_params) }
+ let(:mutation) { graphql_mutation(:uploadDelete, params) }
+ let(:mutation_response) { graphql_mutation_response(:upload_delete) }
+
+ shared_examples_for 'upload deletion' do
+ context 'when the user is not allowed to delete uploads' do
+ let(:current_user) { developer }
+
+ it_behaves_like 'a mutation that returns a top-level access error'
+ end
+
+ context 'when the user is anonymous' do
+ let(:current_user) { nil }
+
+ it_behaves_like 'a mutation that returns a top-level access error'
+ end
+
+ context 'when user has permissions to delete uploads' do
+ let(:current_user) { maintainer }
+
+ it 'deletes the upload' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(mutation_response['upload']).to include('id' => upload.to_global_id.to_s)
+ expect(mutation_response['errors']).to be_empty
+ end
+
+ context 'when upload does not exist' do
+ let(:params) { { filename: 'invalid', secret: upload.secret }.merge(extra_params) }
+
+ it 'returns an error' do
+ post_graphql_mutation(mutation, current_user: current_user)
+
+ expect(response).to have_gitlab_http_status(:success)
+ expect(mutation_response['upload']).to be_nil
+ expect(mutation_response['errors']).to match_array([
+ "The resource that you are attempting to access does not "\
+ "exist or you don't have permission to perform this action."
+ ])
+ end
+ end
+ end
+ end
+
+ context 'when deleting project upload' do
+ let_it_be_with_reload(:upload) { create(:upload, :issuable_upload, model: project) }
+
+ let(:extra_params) { { project_path: project.full_path } }
+
+ it_behaves_like 'upload deletion'
+ end
+
+ context 'when deleting group upload' do
+ let_it_be_with_reload(:upload) { create(:upload, :namespace_upload, model: group) }
+
+ let(:extra_params) { { group_path: group.full_path } }
+
+ it_behaves_like 'upload deletion'
+ end
+end
diff --git a/spec/services/uploads/destroy_service_spec.rb b/spec/services/uploads/destroy_service_spec.rb
new file mode 100644
index 0000000000000..bb58da231b6e3
--- /dev/null
+++ b/spec/services/uploads/destroy_service_spec.rb
@@ -0,0 +1,103 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe Uploads::DestroyService do
+ let_it_be(:project) { create(:project) }
+ let_it_be(:user) { create(:user) }
+ let_it_be_with_reload(:upload) { create(:upload, :issuable_upload, model: project) }
+
+ let(:filename) { File.basename(upload.path) }
+ let(:secret) { upload.secret }
+ let(:model) { project }
+ let(:service) { described_class.new(model, user) }
+
+ describe '#execute' do
+ subject { service.execute(secret, filename) }
+
+ shared_examples_for 'upload not found' do
+ it 'does not delete any upload' do
+ expect { subject }.not_to change { Upload.count }
+ end
+
+ it 'returns an error' do
+ expect(subject[:status]).to eq(:error)
+ expect(subject[:message]).to eq("The resource that you are attempting to access does not "\
+ "exist or you don't have permission to perform this action.")
+ end
+ end
+
+ context 'when user is nil' do
+ let(:user) { nil }
+
+ it_behaves_like 'upload not found'
+ end
+
+ context 'when user cannot destroy upload' do
+ before do
+ project.add_developer(user)
+ end
+
+ it_behaves_like 'upload not found'
+ end
+
+ context 'when user can destroy upload' do
+ before do
+ project.add_maintainer(user)
+ end
+
+ it 'deletes the upload' do
+ expect { subject }.to change { Upload.count }.by(-1)
+ end
+
+ it 'returns success response' do
+ expect(subject[:status]).to eq(:success)
+ expect(subject[:upload]).to eq(upload)
+ end
+
+ context 'when upload is not found' do
+ let(:filename) { 'not existing filename' }
+
+ it_behaves_like 'upload not found'
+ end
+
+ context 'when upload secret is not found' do
+ let(:secret) { 'aaaaaaaaaa' }
+
+ it_behaves_like 'upload not found'
+ end
+
+ context 'when upload secret has invalid format' do
+ let(:secret) { 'invalid' }
+
+ it_behaves_like 'upload not found'
+ end
+
+ context 'when unknown model is used' do
+ let(:model) { user }
+
+ it 'raises an error' do
+ expect { subject }.to raise_exception(ArgumentError)
+ end
+ end
+
+ context 'when upload belongs to other model' do
+ let_it_be(:upload) { create(:upload, :namespace_upload) }
+
+ it_behaves_like 'upload not found'
+ end
+
+ context 'when upload destroy fails' do
+ before do
+ allow(service).to receive(:find_upload).and_return(upload)
+ allow(upload).to receive(:destroy).and_return(false)
+ end
+
+ it 'returns error' do
+ expect(subject[:status]).to eq(:error)
+ expect(subject[:message]).to eq('Upload could not be deleted.')
+ end
+ end
+ end
+ end
+end
diff --git a/spec/support/shared_contexts/policies/group_policy_shared_context.rb b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
index eec6e92c5fe09..893d370240793 100644
--- a/spec/support/shared_contexts/policies/group_policy_shared_context.rb
+++ b/spec/support/shared_contexts/policies/group_policy_shared_context.rb
@@ -56,6 +56,7 @@
admin_package
create_projects
create_cluster update_cluster admin_cluster add_cluster
+ destroy_upload
]
end
diff --git a/spec/support/shared_contexts/policies/project_policy_shared_context.rb b/spec/support/shared_contexts/policies/project_policy_shared_context.rb
index 789b385c43571..1d4731d9b390f 100644
--- a/spec/support/shared_contexts/policies/project_policy_shared_context.rb
+++ b/spec/support/shared_contexts/policies/project_policy_shared_context.rb
@@ -62,6 +62,7 @@
admin_project admin_project_member admin_snippet admin_terraform_state
admin_wiki create_deploy_token destroy_deploy_token
push_to_delete_protected_branch read_deploy_token update_snippet
+ destroy_upload
]
end
--
GitLab