Commit 69833f36 authored by James Lopez's avatar James Lopez

Merge branch '217722-fj-add-spam-mechanism-to-snippet-mutations' into 'master'

Add spam field to snippet mutations

See merge request gitlab-org/gitlab!44010
parents 6f5bf6b8 26edc5c2
...@@ -81,7 +81,7 @@ class GraphqlController < ApplicationController ...@@ -81,7 +81,7 @@ class GraphqlController < ApplicationController
end end
def context def context
@context ||= { current_user: current_user, is_sessionless_user: !!sessionless_user? } @context ||= { current_user: current_user, is_sessionless_user: !!sessionless_user?, request: request }
end end
def build_variables(variable_info) def build_variables(variable_info)
......
# frozen_string_literal: true
module Mutations
module SpammableMutationFields
extend ActiveSupport::Concern
included do
field :spam,
GraphQL::BOOLEAN_TYPE,
null: true,
description: 'Indicates whether the operation returns a record detected as spam'
end
def with_spam_params(&block)
request = Feature.enabled?(:snippet_spam) ? context[:request] : nil
yield.merge({ api: true, request: request })
end
def with_spam_fields(spammable, &block)
{ spam: spammable.spam? }.merge!(yield)
end
end
end
...@@ -3,6 +3,7 @@ ...@@ -3,6 +3,7 @@
module Mutations module Mutations
module Snippets module Snippets
class Create < BaseMutation class Create < BaseMutation
include SpammableMutationFields
include ResolvesProject include ResolvesProject
graphql_name 'CreateSnippet' graphql_name 'CreateSnippet'
...@@ -56,11 +57,13 @@ module Mutations ...@@ -56,11 +57,13 @@ module Mutations
::Gitlab::UsageDataCounters::EditorUniqueCounter.track_snippet_editor_edit_action(author: current_user) ::Gitlab::UsageDataCounters::EditorUniqueCounter.track_snippet_editor_edit_action(author: current_user)
end end
with_spam_fields(snippet) do
{ {
snippet: service_response.success? ? snippet : nil, snippet: service_response.success? ? snippet : nil,
errors: errors_on_object(snippet) errors: errors_on_object(snippet)
} }
end end
end
private private
...@@ -81,6 +84,7 @@ module Mutations ...@@ -81,6 +84,7 @@ module Mutations
end end
def create_params(args) def create_params(args)
with_spam_params do
args.tap do |create_args| args.tap do |create_args|
# We need to rename `blob_actions` into `snippet_actions` because # We need to rename `blob_actions` into `snippet_actions` because
# it's the expected key param # it's the expected key param
...@@ -93,4 +97,5 @@ module Mutations ...@@ -93,4 +97,5 @@ module Mutations
end end
end end
end end
end
end end
...@@ -3,6 +3,8 @@ ...@@ -3,6 +3,8 @@
module Mutations module Mutations
module Snippets module Snippets
class Update < Base class Update < Base
include SpammableMutationFields
graphql_name 'UpdateSnippet' graphql_name 'UpdateSnippet'
argument :id, argument :id,
...@@ -39,11 +41,13 @@ module Mutations ...@@ -39,11 +41,13 @@ module Mutations
::Gitlab::UsageDataCounters::EditorUniqueCounter.track_snippet_editor_edit_action(author: current_user) ::Gitlab::UsageDataCounters::EditorUniqueCounter.track_snippet_editor_edit_action(author: current_user)
end end
with_spam_fields(snippet) do
{ {
snippet: result.success? ? snippet : snippet.reset, snippet: result.success? ? snippet : snippet.reset,
errors: errors_on_object(snippet) errors: errors_on_object(snippet)
} }
end end
end
private private
...@@ -52,6 +56,7 @@ module Mutations ...@@ -52,6 +56,7 @@ module Mutations
end end
def update_params(args) def update_params(args)
with_spam_params do
args.tap do |update_args| args.tap do |update_args|
# We need to rename `blob_actions` into `snippet_actions` because # We need to rename `blob_actions` into `snippet_actions` because
# it's the expected key param # it's the expected key param
...@@ -60,4 +65,5 @@ module Mutations ...@@ -60,4 +65,5 @@ module Mutations
end end
end end
end end
end
end end
---
title: Add spam flag to snippet create/update mutations
merge_request: 44010
author:
type: changed
---
name: snippet_spam
introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/44010
rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/262013
type: development
group: group::editor
default_enabled: false
...@@ -3504,6 +3504,11 @@ type CreateSnippetPayload { ...@@ -3504,6 +3504,11 @@ type CreateSnippetPayload {
The snippet after mutation The snippet after mutation
""" """
snippet: Snippet snippet: Snippet
"""
Indicates whether the operation returns a record detected as spam
"""
spam: Boolean
} }
""" """
...@@ -19276,6 +19281,11 @@ type UpdateSnippetPayload { ...@@ -19276,6 +19281,11 @@ type UpdateSnippetPayload {
The snippet after mutation The snippet after mutation
""" """
snippet: Snippet snippet: Snippet
"""
Indicates whether the operation returns a record detected as spam
"""
spam: Boolean
} }
scalar Upload scalar Upload
......
...@@ -9438,6 +9438,20 @@ ...@@ -9438,6 +9438,20 @@
}, },
"isDeprecated": false, "isDeprecated": false,
"deprecationReason": null "deprecationReason": null
},
{
"name": "spam",
"description": "Indicates whether the operation returns a record detected as spam",
"args": [
],
"type": {
"kind": "SCALAR",
"name": "Boolean",
"ofType": null
},
"isDeprecated": false,
"deprecationReason": null
} }
], ],
"inputFields": null, "inputFields": null,
...@@ -56106,6 +56120,20 @@ ...@@ -56106,6 +56120,20 @@
}, },
"isDeprecated": false, "isDeprecated": false,
"deprecationReason": null "deprecationReason": null
},
{
"name": "spam",
"description": "Indicates whether the operation returns a record detected as spam",
"args": [
],
"type": {
"kind": "SCALAR",
"name": "Boolean",
"ofType": null
},
"isDeprecated": false,
"deprecationReason": null
} }
], ],
"inputFields": null, "inputFields": null,
...@@ -561,6 +561,7 @@ Autogenerated return type of CreateSnippet. ...@@ -561,6 +561,7 @@ Autogenerated return type of CreateSnippet.
| `clientMutationId` | String | A unique identifier for the client performing the mutation. | | `clientMutationId` | String | A unique identifier for the client performing the mutation. |
| `errors` | String! => Array | Errors encountered during execution of the mutation. | | `errors` | String! => Array | Errors encountered during execution of the mutation. |
| `snippet` | Snippet | The snippet after mutation | | `snippet` | Snippet | The snippet after mutation |
| `spam` | Boolean | Indicates whether the operation returns a record detected as spam |
### CreateTestCasePayload ### CreateTestCasePayload
...@@ -2732,6 +2733,7 @@ Autogenerated return type of UpdateSnippet. ...@@ -2732,6 +2733,7 @@ Autogenerated return type of UpdateSnippet.
| `clientMutationId` | String | A unique identifier for the client performing the mutation. | | `clientMutationId` | String | A unique identifier for the client performing the mutation. |
| `errors` | String! => Array | Errors encountered during execution of the mutation. | | `errors` | String! => Array | Errors encountered during execution of the mutation. |
| `snippet` | Snippet | The snippet after mutation | | `snippet` | Snippet | The snippet after mutation |
| `spam` | Boolean | Indicates whether the operation returns a record detected as spam |
### User ### User
......
...@@ -98,6 +98,12 @@ RSpec.describe GraphqlController do ...@@ -98,6 +98,12 @@ RSpec.describe GraphqlController do
expect(assigns(:context)[:is_sessionless_user]).to be false expect(assigns(:context)[:is_sessionless_user]).to be false
end end
end end
it 'includes request object in context' do
post :execute
expect(assigns(:context)[:request]).to eq request
end
end end
describe 'Admin Mode' do describe 'Admin Mode' do
......
...@@ -76,6 +76,8 @@ RSpec.describe 'Creating a Snippet' do ...@@ -76,6 +76,8 @@ RSpec.describe 'Creating a Snippet' do
expect(mutation_response['snippet']).to be_nil expect(mutation_response['snippet']).to be_nil
end end
it_behaves_like 'spam flag is present'
end end
shared_examples 'creates snippet' do shared_examples 'creates snippet' do
...@@ -103,6 +105,10 @@ RSpec.describe 'Creating a Snippet' do ...@@ -103,6 +105,10 @@ RSpec.describe 'Creating a Snippet' do
end end
it_behaves_like 'snippet edit usage data counters' it_behaves_like 'snippet edit usage data counters'
it_behaves_like 'spam flag is present'
it_behaves_like 'can raise spam flag' do
let(:service) { Snippets::CreateService }
end
end end
context 'with PersonalSnippet' do context 'with PersonalSnippet' do
...@@ -142,6 +148,9 @@ RSpec.describe 'Creating a Snippet' do ...@@ -142,6 +148,9 @@ RSpec.describe 'Creating a Snippet' do
it_behaves_like 'a mutation that returns errors in the response', errors: ["Title can't be blank"] it_behaves_like 'a mutation that returns errors in the response', errors: ["Title can't be blank"]
it_behaves_like 'does not create snippet' it_behaves_like 'does not create snippet'
it_behaves_like 'can raise spam flag' do
let(:service) { Snippets::CreateService }
end
end end
context 'when there non ActiveRecord errors' do context 'when there non ActiveRecord errors' do
......
...@@ -37,6 +37,8 @@ RSpec.describe 'Updating a Snippet' do ...@@ -37,6 +37,8 @@ RSpec.describe 'Updating a Snippet' do
graphql_mutation_response(:update_snippet) graphql_mutation_response(:update_snippet)
end end
subject { post_graphql_mutation(mutation, current_user: current_user) }
shared_examples 'graphql update actions' do shared_examples 'graphql update actions' do
context 'when the user does not have permission' do context 'when the user does not have permission' do
let(:current_user) { create(:user) } let(:current_user) { create(:user) }
...@@ -46,14 +48,14 @@ RSpec.describe 'Updating a Snippet' do ...@@ -46,14 +48,14 @@ RSpec.describe 'Updating a Snippet' do
it 'does not update the Snippet' do it 'does not update the Snippet' do
expect do expect do
post_graphql_mutation(mutation, current_user: current_user) subject
end.not_to change { snippet.reload } end.not_to change { snippet.reload }
end end
end end
context 'when the user has permission' do context 'when the user has permission' do
it 'updates the snippet record' do it 'updates the snippet record' do
post_graphql_mutation(mutation, current_user: current_user) subject
expect(snippet.reload.title).to eq(updated_title) expect(snippet.reload.title).to eq(updated_title)
end end
...@@ -65,7 +67,7 @@ RSpec.describe 'Updating a Snippet' do ...@@ -65,7 +67,7 @@ RSpec.describe 'Updating a Snippet' do
expect(blob_to_update.data).not_to eq updated_content expect(blob_to_update.data).not_to eq updated_content
expect(blob_to_delete).to be_present expect(blob_to_delete).to be_present
post_graphql_mutation(mutation, current_user: current_user) subject
blob_to_update = blob_at(updated_file) blob_to_update = blob_at(updated_file)
blob_to_delete = blob_at(deleted_file) blob_to_delete = blob_at(deleted_file)
...@@ -79,13 +81,19 @@ RSpec.describe 'Updating a Snippet' do ...@@ -79,13 +81,19 @@ RSpec.describe 'Updating a Snippet' do
end end
end end
it_behaves_like 'can raise spam flag' do
let(:service) { Snippets::UpdateService }
end
it_behaves_like 'spam flag is present'
context 'when there are ActiveRecord validation errors' do context 'when there are ActiveRecord validation errors' do
let(:updated_title) { '' } let(:updated_title) { '' }
it_behaves_like 'a mutation that returns errors in the response', errors: ["Title can't be blank"] it_behaves_like 'a mutation that returns errors in the response', errors: ["Title can't be blank"]
it 'does not update the Snippet' do it 'does not update the Snippet' do
post_graphql_mutation(mutation, current_user: current_user) subject
expect(snippet.reload.title).to eq(original_title) expect(snippet.reload.title).to eq(original_title)
end end
...@@ -94,7 +102,7 @@ RSpec.describe 'Updating a Snippet' do ...@@ -94,7 +102,7 @@ RSpec.describe 'Updating a Snippet' do
blob_to_update = blob_at(updated_file) blob_to_update = blob_at(updated_file)
blob_to_delete = blob_at(deleted_file) blob_to_delete = blob_at(deleted_file)
post_graphql_mutation(mutation, current_user: current_user) subject
aggregate_failures do aggregate_failures do
expect(blob_at(updated_file).data).to eq blob_to_update.data expect(blob_at(updated_file).data).to eq blob_to_update.data
...@@ -104,6 +112,11 @@ RSpec.describe 'Updating a Snippet' do ...@@ -104,6 +112,11 @@ RSpec.describe 'Updating a Snippet' do
expect(mutation_response['snippet']['visibilityLevel']).to eq('private') expect(mutation_response['snippet']['visibilityLevel']).to eq('private')
end end
end end
it_behaves_like 'spam flag is present'
it_behaves_like 'can raise spam flag' do
let(:service) { Snippets::UpdateService }
end
end end
def blob_at(filename) def blob_at(filename)
...@@ -144,7 +157,7 @@ RSpec.describe 'Updating a Snippet' do ...@@ -144,7 +157,7 @@ RSpec.describe 'Updating a Snippet' do
context 'when the author is not a member of the project' do context 'when the author is not a member of the project' do
it 'returns an an error' do it 'returns an an error' do
post_graphql_mutation(mutation, current_user: current_user) subject
errors = json_response['errors'] errors = json_response['errors']
expect(errors.first['message']).to eq(Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR) expect(errors.first['message']).to eq(Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR)
...@@ -162,7 +175,7 @@ RSpec.describe 'Updating a Snippet' do ...@@ -162,7 +175,7 @@ RSpec.describe 'Updating a Snippet' do
it 'returns an an error' do it 'returns an an error' do
project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::DISABLED) project.project_feature.update_attribute(:snippets_access_level, ProjectFeature::DISABLED)
post_graphql_mutation(mutation, current_user: current_user) subject
errors = json_response['errors'] errors = json_response['errors']
expect(errors.first['message']).to eq(Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR) expect(errors.first['message']).to eq(Gitlab::Graphql::Authorize::AuthorizeResource::RESOURCE_ACCESS_ERROR)
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.shared_examples 'spam flag is present' do
specify :aggregate_failures do
subject
expect(mutation_response).to have_key('spam')
expect(mutation_response['spam']).to be_falsey
end
end
RSpec.shared_examples 'can raise spam flag' do
it 'spam parameters are passed to the service' do
expect(service).to receive(:new).with(anything, anything, hash_including(api: true, request: instance_of(ActionDispatch::Request)))
subject
end
context 'when the snippet is detected as spam' do
it 'raises spam flag' do
allow_next_instance_of(service) do |instance|
allow(instance).to receive(:spam_check) do |snippet, user, _|
snippet.spam!
end
end
subject
expect(mutation_response['spam']).to be true
expect(mutation_response['errors']).to include("Your snippet has been recognized as spam and has been discarded.")
end
end
context 'when :snippet_spam flag is disabled' do
before do
stub_feature_flags(snippet_spam: false)
end
it 'request parameter is not passed to the service' do
expect(service).to receive(:new).with(anything, anything, hash_not_including(request: instance_of(ActionDispatch::Request)))
subject
end
end
end
Markdown is supported
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