Merge branch 'add-recaptcha-fields-to-snippet-mutations' into 'master'

Add reCAPTCHA fields to snippet mutations

See merge request gitlab-org/gitlab!51956

app/graphql/mutations/concerns/mutations/can_mutate_spammable.rb

@@ -2,17 +2,45 @@
 
 module Mutations
   # This concern can be mixed into a mutation to provide support for spam checking,
-  # and optionally support the workflow to allow clients to display and solve recaptchas.
+  # and optionally support the workflow to allow clients to display and solve CAPTCHAs.
   module CanMutateSpammable
     extend ActiveSupport::Concern
 
+    # NOTE: The arguments and fields are intentionally named with 'captcha' instead of 'recaptcha',
+    # so that they can be applied to future alternative CAPTCHA implementations other than
+    # reCAPTCHA (e.g. FriendlyCaptcha) without having to change the names and descriptions in the API.
     included do
+      argument :captcha_response, GraphQL::STRING_TYPE,
+               required: false,
+               description: 'A valid CAPTCHA response value obtained by using the provided captchaSiteKey with a CAPTCHA API to present a challenge to be solved on the client. Required to resubmit if the previous operation returned "NeedsCaptchaResponse: true".'
+
+      argument :spam_log_id, GraphQL::INT_TYPE,
+               required: false,
+               description: 'The spam log ID which must be passed along with a valid CAPTCHA response for the operation to be completed. Required to resubmit if the previous operation returned "NeedsCaptchaResponse: true".'
+
       field :spam,
             GraphQL::BOOLEAN_TYPE,
             null: true,
-            description: 'Indicates whether the operation returns a record detected as spam.'
+            description: 'Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response.'
+
+      field :needs_captcha_response,
+            GraphQL::BOOLEAN_TYPE,
+            null: true,
+            description: 'Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true.'
+
+      field :spam_log_id,
+            GraphQL::INT_TYPE,
+            null: true,
+            description: 'The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true.'
+
+      field :captcha_site_key,
+            GraphQL::STRING_TYPE,
+            null: true,
+            description: 'The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because "NeedsCaptchaResponse" is true.'
     end
 
+    private
+
     # additional_spam_params    -> hash
     #
     # Used from a spammable mutation's #resolve method to generate
@@ -41,7 +69,7 @@ module Mutations
       spam_action_fields = {
         spam: spammable.spam?,
         # NOTE: These fields are intentionally named with 'captcha' instead of 'recaptcha', so
-        # that they can be applied to future alternative captcha implementations other than
+        # that they can be applied to future alternative CAPTCHA implementations other than
         # reCAPTCHA (such as FriendlyCaptcha) without having to change the response field name
         # in the API.
         needs_captcha_response: spammable.render_recaptcha?,

changelogs/unreleased/add-recaptcha-fields-to-snippet-mutations.yml

+---
+title: Add captcha-related fields to snippet GraphQL mutations
+merge_request: 51956
+author:
+type: added

doc/api/graphql/reference/gitlab_schema.graphql

@@ -5314,6 +5314,13 @@ input CreateSnippetInput {
   """
   blobActions: [SnippetBlobActionInputType!]
 
+  """
+  A valid CAPTCHA response value obtained by using the provided captchaSiteKey
+  with a CAPTCHA API to present a challenge to be solved on the client. Required
+  to resubmit if the previous operation returned "NeedsCaptchaResponse: true".
+  """
+  captchaResponse: String
+
   """
   A unique identifier for the client performing the mutation.
   """
@@ -5329,6 +5336,13 @@ input CreateSnippetInput {
   """
   projectPath: ID
 
+  """
+  The spam log ID which must be passed along with a valid CAPTCHA response for
+  the operation to be completed. Required to resubmit if the previous operation
+  returned "NeedsCaptchaResponse: true".
+  """
+  spamLogId: Int
+
   """
   Title of the snippet.
   """
@@ -5349,6 +5363,13 @@ input CreateSnippetInput {
 Autogenerated return type of CreateSnippet
 """
 type CreateSnippetPayload {
+  """
+  The CAPTCHA site key which must be used to render a challenge for the user to
+  solve to obtain a valid captchaResponse value. Included only when an operation
+  was not completed because "NeedsCaptchaResponse" is true.
+  """
+  captchaSiteKey: String
+
   """
   A unique identifier for the client performing the mutation.
   """
@@ -5359,15 +5380,32 @@ type CreateSnippetPayload {
   """
   errors: [String!]!
 
+  """
+  Indicates whether the operation was detected as possible spam and not
+  completed. If CAPTCHA is enabled, the request must be resubmitted with a valid
+  CAPTCHA response and spam_log_id included for the operation to be completed.
+  Included only when an operation was not completed because
+  "NeedsCaptchaResponse" is true.
+  """
+  needsCaptchaResponse: Boolean
+
   """
   The snippet after mutation.
   """
   snippet: Snippet
 
   """
-  Indicates whether the operation returns a record detected as spam.
+  Indicates whether the operation was detected as definite spam. There is no
+  option to resubmit the request with a CAPTCHA response.
   """
   spam: Boolean
+
+  """
+  The spam log ID which must be passed along with a valid CAPTCHA response for
+  an operation to be completed. Included only when an operation was not
+  completed because "NeedsCaptchaResponse" is true.
+  """
+  spamLogId: Int
 }
 
 """
@@ -26234,6 +26272,13 @@ input UpdateSnippetInput {
   """
   blobActions: [SnippetBlobActionInputType!]
 
+  """
+  A valid CAPTCHA response value obtained by using the provided captchaSiteKey
+  with a CAPTCHA API to present a challenge to be solved on the client. Required
+  to resubmit if the previous operation returned "NeedsCaptchaResponse: true".
+  """
+  captchaResponse: String
+
   """
   A unique identifier for the client performing the mutation.
   """
@@ -26249,6 +26294,13 @@ input UpdateSnippetInput {
   """
   id: SnippetID!
 
+  """
+  The spam log ID which must be passed along with a valid CAPTCHA response for
+  the operation to be completed. Required to resubmit if the previous operation
+  returned "NeedsCaptchaResponse: true".
+  """
+  spamLogId: Int
+
   """
   Title of the snippet.
   """
@@ -26264,6 +26316,13 @@ input UpdateSnippetInput {
 Autogenerated return type of UpdateSnippet
 """
 type UpdateSnippetPayload {
+  """
+  The CAPTCHA site key which must be used to render a challenge for the user to
+  solve to obtain a valid captchaResponse value. Included only when an operation
+  was not completed because "NeedsCaptchaResponse" is true.
+  """
+  captchaSiteKey: String
+
   """
   A unique identifier for the client performing the mutation.
   """
@@ -26274,15 +26333,32 @@ type UpdateSnippetPayload {
   """
   errors: [String!]!
 
+  """
+  Indicates whether the operation was detected as possible spam and not
+  completed. If CAPTCHA is enabled, the request must be resubmitted with a valid
+  CAPTCHA response and spam_log_id included for the operation to be completed.
+  Included only when an operation was not completed because
+  "NeedsCaptchaResponse" is true.
+  """
+  needsCaptchaResponse: Boolean
+
   """
   The snippet after mutation.
   """
   snippet: Snippet
 
   """
-  Indicates whether the operation returns a record detected as spam.
+  Indicates whether the operation was detected as definite spam. There is no
+  option to resubmit the request with a CAPTCHA response.
   """
   spam: Boolean
+
+  """
+  The spam log ID which must be passed along with a valid CAPTCHA response for
+  an operation to be completed. Included only when an operation was not
+  completed because "NeedsCaptchaResponse" is true.
+  """
+  spamLogId: Int
 }
 
 scalar Upload

doc/api/graphql/reference/gitlab_schema.json

@@ -14446,6 +14446,26 @@
           "description": "Autogenerated input type of CreateSnippet",
           "fields": null,
           "inputFields": [
+            {
+              "name": "captchaResponse",
+              "description": "A valid CAPTCHA response value obtained by using the provided captchaSiteKey with a CAPTCHA API to present a challenge to be solved on the client. Required to resubmit if the previous operation returned \"NeedsCaptchaResponse: true\".",
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "spamLogId",
+              "description": "The spam log ID which must be passed along with a valid CAPTCHA response for the operation to be completed. Required to resubmit if the previous operation returned \"NeedsCaptchaResponse: true\".",
+              "type": {
+                "kind": "SCALAR",
+                "name": "Int",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
             {
               "name": "title",
               "description": "Title of the snippet.",
@@ -14550,6 +14570,20 @@
           "name": "CreateSnippetPayload",
           "description": "Autogenerated return type of CreateSnippet",
           "fields": [
+            {
+              "name": "captchaSiteKey",
+              "description": "The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because \"NeedsCaptchaResponse\" is true.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "clientMutationId",
               "description": "A unique identifier for the client performing the mutation.",
@@ -14590,6 +14624,20 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "needsCaptchaResponse",
+              "description": "Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because \"NeedsCaptchaResponse\" is true.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "Boolean",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "snippet",
               "description": "The snippet after mutation.",
@@ -14606,7 +14654,7 @@
             },
             {
               "name": "spam",
-              "description": "Indicates whether the operation returns a record detected as spam.",
+              "description": "Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response.",
               "args": [
 
               ],
@@ -14617,6 +14665,20 @@
               },
               "isDeprecated": false,
               "deprecationReason": null
+            },
+            {
+              "name": "spamLogId",
+              "description": "The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because \"NeedsCaptchaResponse\" is true.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "Int",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
             }
           ],
           "inputFields": null,
@@ -76122,6 +76184,26 @@
           "description": "Autogenerated input type of UpdateSnippet",
           "fields": null,
           "inputFields": [
+            {
+              "name": "captchaResponse",
+              "description": "A valid CAPTCHA response value obtained by using the provided captchaSiteKey with a CAPTCHA API to present a challenge to be solved on the client. Required to resubmit if the previous operation returned \"NeedsCaptchaResponse: true\".",
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
+            {
+              "name": "spamLogId",
+              "description": "The spam log ID which must be passed along with a valid CAPTCHA response for the operation to be completed. Required to resubmit if the previous operation returned \"NeedsCaptchaResponse: true\".",
+              "type": {
+                "kind": "SCALAR",
+                "name": "Int",
+                "ofType": null
+              },
+              "defaultValue": null
+            },
             {
               "name": "id",
               "description": "The global ID of the snippet to update.",
@@ -76204,6 +76286,20 @@
           "name": "UpdateSnippetPayload",
           "description": "Autogenerated return type of UpdateSnippet",
           "fields": [
+            {
+              "name": "captchaSiteKey",
+              "description": "The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because \"NeedsCaptchaResponse\" is true.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "String",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "clientMutationId",
               "description": "A unique identifier for the client performing the mutation.",
@@ -76244,6 +76340,20 @@
               "isDeprecated": false,
               "deprecationReason": null
             },
+            {
+              "name": "needsCaptchaResponse",
+              "description": "Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because \"NeedsCaptchaResponse\" is true.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "Boolean",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
+            },
             {
               "name": "snippet",
               "description": "The snippet after mutation.",
@@ -76260,7 +76370,7 @@
             },
             {
               "name": "spam",
-              "description": "Indicates whether the operation returns a record detected as spam.",
+              "description": "Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response.",
               "args": [
 
               ],
@@ -76271,6 +76381,20 @@
               },
               "isDeprecated": false,
               "deprecationReason": null
+            },
+            {
+              "name": "spamLogId",
+              "description": "The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because \"NeedsCaptchaResponse\" is true.",
+              "args": [
+
+              ],
+              "type": {
+                "kind": "SCALAR",
+                "name": "Int",
+                "ofType": null
+              },
+              "isDeprecated": false,
+              "deprecationReason": null
             }
           ],
           "inputFields": null,

doc/api/graphql/reference/index.md

@@ -843,10 +843,13 @@ Autogenerated return type of CreateSnippet.
 
 | Field | Type | Description |
 | ----- | ---- | ----------- |
+| `captchaSiteKey` | String | The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
 | `clientMutationId` | String | A unique identifier for the client performing the mutation. |
 | `errors` | String! => Array | Errors encountered during execution of the mutation. |
+| `needsCaptchaResponse` | Boolean | Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
 | `snippet` | Snippet | The snippet after mutation. |
-| `spam` | Boolean | Indicates whether the operation returns a record detected as spam. |
+| `spam` | Boolean | Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response. |
+| `spamLogId` | Int | The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
 
 ### CreateTestCasePayload
 
@@ -3934,10 +3937,13 @@ Autogenerated return type of UpdateSnippet.
 
 | Field | Type | Description |
 | ----- | ---- | ----------- |
+| `captchaSiteKey` | String | The CAPTCHA site key which must be used to render a challenge for the user to solve to obtain a valid captchaResponse value. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
 | `clientMutationId` | String | A unique identifier for the client performing the mutation. |
 | `errors` | String! => Array | Errors encountered during execution of the mutation. |
+| `needsCaptchaResponse` | Boolean | Indicates whether the operation was detected as possible spam and not completed. If CAPTCHA is enabled, the request must be resubmitted with a valid CAPTCHA response and spam_log_id included for the operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
 | `snippet` | Snippet | The snippet after mutation. |
-| `spam` | Boolean | Indicates whether the operation returns a record detected as spam. |
+| `spam` | Boolean | Indicates whether the operation was detected as definite spam. There is no option to resubmit the request with a CAPTCHA response. |
+| `spamLogId` | Int | The spam log ID which must be passed along with a valid CAPTCHA response for an operation to be completed. Included only when an operation was not completed because "NeedsCaptchaResponse" is true. |
 
 ### User
 

spec/graphql/mutations/concerns/mutations/can_mutate_spammable_spec.rb

@@ -17,7 +17,7 @@ RSpec.describe Mutations::CanMutateSpammable do
 
   describe '#additional_spam_params' do
     it 'returns additional spam-related params' do
-      expect(subject.additional_spam_params).to eq({ api: true, request: request })
+      expect(subject.send(:additional_spam_params)).to eq({ api: true, request: request })
     end
   end
 
@@ -30,7 +30,7 @@ RSpec.describe Mutations::CanMutateSpammable do
     end
 
     it 'merges in spam action fields from spammable' do
-      result = subject.with_spam_action_fields(spammable) do
+      result = subject.send(:with_spam_action_fields, spammable) do
         { other_field: true }
       end
       expect(result)

spec/requests/api/graphql/mutations/snippets/create_spec.rb

@@ -17,6 +17,7 @@ RSpec.describe 'Creating a Snippet' do
   let(:actions) { [{ action: action }.merge(file_1), { action: action }.merge(file_2)] }
   let(:project_path) { nil }
   let(:uploaded_files) { nil }
+  let(:spam_mutation_vars) { {} }
   let(:mutation_vars) do
     {
       description: description,
@@ -25,7 +26,7 @@ RSpec.describe 'Creating a Snippet' do
       project_path: project_path,
       uploaded_files: uploaded_files,
       blob_actions: actions
-    }
+    }.merge(spam_mutation_vars)
   end
 
   let(:mutation) do
@@ -77,8 +78,6 @@ RSpec.describe 'Creating a Snippet' do
         expect(mutation_response['snippet']).to be_nil
       end
 
-      it_behaves_like 'spam flag is present'
-
       context 'when snippet_spam flag is disabled' do
         before do
           stub_feature_flags(snippet_spam: false)
@@ -113,15 +112,24 @@ RSpec.describe 'Creating a Snippet' do
       end
 
       context 'when action is invalid' do
-        let(:file_1) { { filePath: 'example_file1' }}
+        let(:file_1) { { filePath: 'example_file1' } }
 
         it_behaves_like 'a mutation that returns errors in the response', errors: ['Snippet actions have invalid data']
         it_behaves_like 'does not create snippet'
       end
 
       it_behaves_like 'snippet edit usage data counters'
-      it_behaves_like 'spam flag is present'
-      it_behaves_like 'can raise spam flag' do
+
+      it_behaves_like 'a mutation which can mutate a spammable' do
+        let(:captcha_response) { 'abc123' }
+        let(:spam_log_id) { 1234 }
+        let(:spam_mutation_vars) do
+          {
+            captcha_response: captcha_response,
+            spam_log_id: spam_log_id
+          }
+        end
+
         let(:service) { Snippets::CreateService }
       end
     end
@@ -163,9 +171,6 @@ 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 'does not create snippet'
-      it_behaves_like 'can raise spam flag' do
-        let(:service) { Snippets::CreateService }
-      end
     end
 
     context 'when there non ActiveRecord errors' do

spec/requests/api/graphql/mutations/snippets/update_spec.rb

@@ -16,6 +16,7 @@ RSpec.describe 'Updating a Snippet' do
   let(:updated_file) { 'CHANGELOG' }
   let(:deleted_file) { 'README' }
   let(:snippet_gid) { GitlabSchema.id_from_object(snippet).to_s }
+  let(:spam_mutation_vars) { {} }
   let(:mutation_vars) do
     {
       id: snippet_gid,
@@ -26,7 +27,7 @@ RSpec.describe 'Updating a Snippet' do
         { action: :update, filePath: updated_file, content: updated_content },
         { action: :delete, filePath: deleted_file }
       ]
-    }
+    }.merge(spam_mutation_vars)
   end
 
   let(:mutation) do
@@ -81,12 +82,6 @@ RSpec.describe 'Updating a Snippet' do
         end
       end
 
-      it_behaves_like 'can raise spam flag' do
-        let(:service) { Snippets::UpdateService }
-      end
-
-      it_behaves_like 'spam flag is present'
-
       context 'when snippet_spam flag is disabled' do
         before do
           stub_feature_flags(snippet_spam: false)
@@ -127,11 +122,19 @@ RSpec.describe 'Updating a Snippet' do
             expect(mutation_response['snippet']['visibilityLevel']).to eq('private')
           end
         end
+      end
 
-        it_behaves_like 'spam flag is present'
-        it_behaves_like 'can raise spam flag' do
-          let(:service) { Snippets::UpdateService }
+      it_behaves_like 'a mutation which can mutate a spammable' do
+        let(:captcha_response) { 'abc123' }
+        let(:spam_log_id) { 1234 }
+        let(:spam_mutation_vars) do
+          {
+            captcha_response: captcha_response,
+            spam_log_id: spam_log_id
+          }
         end
+
+        let(:service) { Snippets::UpdateService }
       end
 
       def blob_at(filename)

spec/support/shared_examples/graphql/mutations/can_mutate_spammable_examples.rb

@@ -2,35 +2,35 @@
 
 require 'spec_helper'
 
-RSpec.shared_examples 'spam flag is present' do
-  specify :aggregate_failures do
-    subject
+RSpec.shared_examples 'a mutation which can mutate a spammable' do
+  describe "#additional_spam_params" do
+    it 'passes additional spam params to the service' do
+      args = [
+        anything,
+        anything,
+        hash_including(
+          api: true,
+          request: instance_of(ActionDispatch::Request),
+          captcha_response: captcha_response,
+          spam_log_id: spam_log_id
+        )
+      ]
+      expect(service).to receive(:new).with(*args).and_call_original
 
-    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
-    args = [anything, anything, hash_including(api: true, request: instance_of(ActionDispatch::Request))]
-    expect(service).to receive(:new).with(*args).and_call_original
-
-    subject
+      subject
+    end
   end
 
-  context 'when the snippet is detected as spam' do
-    it 'raises spam flag' do
-      allow_next_instance_of(Spam::SpamActionService) do |instance|
-        allow(instance).to receive(:execute) { true }
-        instance.target.spam!
-        instance.target.unrecoverable_spam_error!
-      end
-
+  describe "#with_spam_action_fields" do
+    it 'resolves with spam action fields' do
       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.")
+      # NOTE: We do not need to assert on the specific values of spam action fields here, we only need
+      # to verify that #with_spam_action_fields was invoked and that the fields are present in the
+      # response. The specific behavior of #with_spam_action_fields is covered in the
+      # CanMutateSpammable unit tests.
+      expect(mutation_response.keys)
+        .to include('spam', 'spamLogId', 'needsCaptchaResponse', 'captchaSiteKey')
     end
   end
 end