diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2017-02-22 10:13:20 +0000 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2017-02-22 10:13:20 +0000 |
commit | 7e8a6a63af2138c5f3f2030ef8c65e8d4815eed3 (patch) | |
tree | e33f0190509c5793c67f5589bc455f53a529efc4 | |
parent | 065a52c8144e621f699762b0206a14a010897254 (diff) | |
parent | 2ace39f2420abf018ceef6aaad52e4917bcbab7d (diff) | |
download | gitlab-ce-7e8a6a63af2138c5f3f2030ef8c65e8d4815eed3.tar.gz |
Merge branch '28093-snippet-and-issue-spam-check-on-edit' into 'master'
Spam check and reCAPTCHA improvements
Closes #28093
See merge request !9248
35 files changed, 935 insertions, 247 deletions
diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb index a6891149bfa..da225d8f1c7 100644 --- a/app/controllers/concerns/spammable_actions.rb +++ b/app/controllers/concerns/spammable_actions.rb @@ -17,13 +17,31 @@ module SpammableActions private - def recaptcha_params - return {} unless params[:recaptcha_verification] && Gitlab::Recaptcha.load_configurations! && verify_recaptcha + def recaptcha_check_with_fallback(&fallback) + if spammable.valid? + redirect_to spammable + elsif render_recaptcha? + if params[:recaptcha_verification] + flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' + end + + render :verify + else + fallback.call + end + end + + def spammable_params + default_params = { request: request } + + recaptcha_check = params[:recaptcha_verification] && + Gitlab::Recaptcha.load_configurations! && + verify_recaptcha + + return default_params unless recaptcha_check - { - recaptcha_verified: true, - spam_log_id: params[:spam_log_id] - } + { recaptcha_verified: true, + spam_log_id: params[:spam_log_id] }.merge(default_params) end def spammable diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 744a4af1c51..6ef36771ac1 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -94,15 +94,15 @@ class Projects::IssuesController < Projects::ApplicationController end def create - extra_params = { request: request, - merge_request_for_resolving_discussions: merge_request_for_resolving_discussions } - extra_params.merge!(recaptcha_params) + create_params = issue_params + .merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) + .merge(spammable_params) - @issue = Issues::CreateService.new(project, current_user, issue_params.merge(extra_params)).execute + @issue = Issues::CreateService.new(project, current_user, create_params).execute respond_to do |format| format.html do - html_response_create + recaptcha_check_with_fallback { render :new } end format.js do @link = @issue.attachment.url.to_js @@ -111,7 +111,9 @@ class Projects::IssuesController < Projects::ApplicationController end def update - @issue = Issues::UpdateService.new(project, current_user, issue_params).execute(issue) + update_params = issue_params.merge(spammable_params) + + @issue = Issues::UpdateService.new(project, current_user, update_params).execute(issue) if params[:move_to_project_id].to_i > 0 new_project = Project.find(params[:move_to_project_id]) @@ -123,11 +125,7 @@ class Projects::IssuesController < Projects::ApplicationController respond_to do |format| format.html do - if @issue.valid? - redirect_to issue_path(@issue) - else - render :edit - end + recaptcha_check_with_fallback { render :edit } end format.json do @@ -179,20 +177,6 @@ class Projects::IssuesController < Projects::ApplicationController protected - def html_response_create - if @issue.valid? - redirect_to issue_path(@issue) - elsif render_recaptcha? - if params[:recaptcha_verification] - flash[:alert] = 'There was an error with the reCAPTCHA. Please solve the reCAPTCHA again.' - end - - render :verify - else - render :new - end - end - def issue # The Sortable default scope causes performance issues when used with find_by @noteable = @issue ||= @project.issues.where(iid: params[:id]).reorder(nil).take || redirect_old diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index ef5d3d242eb..ea1a97b7cf0 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -38,24 +38,19 @@ class Projects::SnippetsController < Projects::ApplicationController end def create - create_params = snippet_params.merge(request: request) + create_params = snippet_params.merge(spammable_params) + @snippet = CreateSnippetService.new(@project, current_user, create_params).execute - if @snippet.valid? - respond_with(@snippet, - location: namespace_project_snippet_path(@project.namespace, - @project, @snippet)) - else - render :new - end + recaptcha_check_with_fallback { render :new } end def update - UpdateSnippetService.new(project, current_user, @snippet, - snippet_params).execute - respond_with(@snippet, - location: namespace_project_snippet_path(@project.namespace, - @project, @snippet)) + update_params = snippet_params.merge(spammable_params) + + UpdateSnippetService.new(project, current_user, @snippet, update_params).execute + + recaptcha_check_with_fallback { render :edit } end def show diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 445898a2e9e..2d26718873f 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -42,16 +42,19 @@ class SnippetsController < ApplicationController end def create - create_params = snippet_params.merge(request: request) + create_params = snippet_params.merge(spammable_params) + @snippet = CreateSnippetService.new(nil, current_user, create_params).execute - respond_with @snippet.becomes(Snippet) + recaptcha_check_with_fallback { render :new } end def update - UpdateSnippetService.new(nil, current_user, @snippet, - snippet_params).execute - respond_with @snippet.becomes(Snippet) + update_params = snippet_params.merge(spammable_params) + + UpdateSnippetService.new(nil, current_user, @snippet, update_params).execute + + recaptcha_check_with_fallback { render :edit } end def show diff --git a/app/models/concerns/spammable.rb b/app/models/concerns/spammable.rb index 79adc77c9e4..107e6764ba2 100644 --- a/app/models/concerns/spammable.rb +++ b/app/models/concerns/spammable.rb @@ -13,7 +13,7 @@ module Spammable attr_accessor :spam attr_accessor :spam_log - after_validation :check_for_spam, on: :create + after_validation :check_for_spam, on: [:create, :update] cattr_accessor :spammable_attrs, instance_accessor: false do [] diff --git a/app/models/project_snippet.rb b/app/models/project_snippet.rb index 9bb456eee24..25b5d777641 100644 --- a/app/models/project_snippet.rb +++ b/app/models/project_snippet.rb @@ -9,8 +9,4 @@ class ProjectSnippet < Snippet participant :author participant :notes_with_associations - - def check_for_spam? - super && project.public? - end end diff --git a/app/services/create_snippet_service.rb b/app/services/create_snippet_service.rb index 14f5ba064ff..40286dbf3bf 100644 --- a/app/services/create_snippet_service.rb +++ b/app/services/create_snippet_service.rb @@ -1,7 +1,8 @@ class CreateSnippetService < BaseService + include SpamCheckService + def execute - request = params.delete(:request) - api = params.delete(:api) + filter_spam_check_params snippet = if project project.snippets.build(params) @@ -15,10 +16,11 @@ class CreateSnippetService < BaseService end snippet.author = current_user - snippet.spam = SpamService.new(snippet, request).check(api) + + spam_check(snippet, current_user) if snippet.save - UserAgentDetailService.new(snippet, request).create + UserAgentDetailService.new(snippet, @request).create end snippet diff --git a/app/services/issuable_base_service.rb b/app/services/issuable_base_service.rb index 5f3ced49665..9500faf2862 100644 --- a/app/services/issuable_base_service.rb +++ b/app/services/issuable_base_service.rb @@ -191,14 +191,12 @@ class IssuableBaseService < BaseService # To be overridden by subclasses end - def after_update(issuable) + def before_update(issuable) # To be overridden by subclasses end - def update_issuable(issuable, attributes) - issuable.with_transaction_returning_status do - issuable.update(attributes.merge(updated_by: current_user)) - end + def after_update(issuable) + # To be overridden by subclasses end def update(issuable) @@ -212,16 +210,22 @@ class IssuableBaseService < BaseService label_ids = process_label_ids(params, existing_label_ids: issuable.label_ids) params[:label_ids] = label_ids if labels_changing?(issuable.label_ids, label_ids) - if params.present? && update_issuable(issuable, params) - # We do not touch as it will affect a update on updated_at field - ActiveRecord::Base.no_touching do - handle_common_system_notes(issuable, old_labels: old_labels) - end + if params.present? + issuable.assign_attributes(params.merge(updated_by: current_user)) + + before_update(issuable) - handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users) - after_update(issuable) - issuable.create_new_cross_references!(current_user) - execute_hooks(issuable, 'update') + if issuable.with_transaction_returning_status { issuable.save } + # We do not touch as it will affect a update on updated_at field + ActiveRecord::Base.no_touching do + handle_common_system_notes(issuable, old_labels: old_labels) + end + + handle_changes(issuable, old_labels: old_labels, old_mentioned_users: old_mentioned_users) + after_update(issuable) + issuable.create_new_cross_references!(current_user) + execute_hooks(issuable, 'update') + end end issuable diff --git a/app/services/issues/create_service.rb b/app/services/issues/create_service.rb index 961605a1005..366b3572738 100644 --- a/app/services/issues/create_service.rb +++ b/app/services/issues/create_service.rb @@ -1,10 +1,9 @@ module Issues class CreateService < Issues::BaseService + include SpamCheckService + def execute - @request = params.delete(:request) - @api = params.delete(:api) - @recaptcha_verified = params.delete(:recaptcha_verified) - @spam_log_id = params.delete(:spam_log_id) + filter_spam_check_params issue_attributes = params.merge(merge_request_for_resolving_discussions: merge_request_for_resolving_discussions) @issue = BuildService.new(project, current_user, issue_attributes).execute @@ -12,14 +11,8 @@ module Issues create(@issue) end - def before_create(issuable) - if @recaptcha_verified - spam_log = current_user.spam_logs.find_by(id: @spam_log_id, title: issuable.title) - spam_log&.update!(recaptcha_verified: true) - else - issuable.spam = spam_service.check(@api) - issuable.spam_log = spam_service.spam_log - end + def before_create(issue) + spam_check(issue, current_user) end def after_create(issuable) @@ -42,10 +35,6 @@ module Issues private - def spam_service - @spam_service ||= SpamService.new(@issue, @request) - end - def user_agent_detail_service UserAgentDetailService.new(@issue, @request) end diff --git a/app/services/issues/update_service.rb b/app/services/issues/update_service.rb index 78cbf94ec69..22e32b13259 100644 --- a/app/services/issues/update_service.rb +++ b/app/services/issues/update_service.rb @@ -1,9 +1,17 @@ module Issues class UpdateService < Issues::BaseService + include SpamCheckService + def execute(issue) + filter_spam_check_params + update(issue) end + def before_update(issue) + spam_check(issue, current_user) + end + def handle_changes(issue, old_labels: [], old_mentioned_users: []) if has_changes?(issue, old_labels: old_labels) todo_service.mark_pending_todos_as_done(issue, current_user) diff --git a/app/services/spam_check_service.rb b/app/services/spam_check_service.rb new file mode 100644 index 00000000000..023e0824e85 --- /dev/null +++ b/app/services/spam_check_service.rb @@ -0,0 +1,24 @@ +# SpamCheckService +# +# Provide helper methods for checking if a given spammable object has +# potential spam data. +# +# Dependencies: +# - params with :request +# +module SpamCheckService + def filter_spam_check_params + @request = params.delete(:request) + @api = params.delete(:api) + @recaptcha_verified = params.delete(:recaptcha_verified) + @spam_log_id = params.delete(:spam_log_id) + end + + def spam_check(spammable, user) + spam_service = SpamService.new(spammable, @request) + + spam_service.when_recaptcha_verified(@recaptcha_verified, @api) do + user.spam_logs.find_by(id: @spam_log_id)&.update!(recaptcha_verified: true) + end + end +end diff --git a/app/services/spam_service.rb b/app/services/spam_service.rb index 024a7c19d33..3e65b7d31a3 100644 --- a/app/services/spam_service.rb +++ b/app/services/spam_service.rb @@ -17,15 +17,6 @@ class SpamService end end - def check(api = false) - return false unless request && check_for_spam? - - return false unless akismet.is_spam? - - create_spam_log(api) - true - end - def mark_as_spam! return false unless spammable.submittable_as_spam? @@ -36,8 +27,30 @@ class SpamService end end + def when_recaptcha_verified(recaptcha_verified, api = false) + # In case it's a request which is already verified through recaptcha, yield + # block. + if recaptcha_verified + yield + else + # Otherwise, it goes to Akismet and check if it's a spam. If that's the + # case, it assigns spammable record as "spam" and create a SpamLog record. + spammable.spam = check(api) + spammable.spam_log = spam_log + end + end + private + def check(api) + return false unless request && check_for_spam? + + return false unless akismet.is_spam? + + create_spam_log(api) + true + end + def akismet @akismet ||= AkismetService.new( spammable_owner, diff --git a/app/services/update_snippet_service.rb b/app/services/update_snippet_service.rb index a6bb36821c3..358bca73aec 100644 --- a/app/services/update_snippet_service.rb +++ b/app/services/update_snippet_service.rb @@ -1,4 +1,6 @@ class UpdateSnippetService < BaseService + include SpamCheckService + attr_accessor :snippet def initialize(project, user, snippet, params) @@ -9,7 +11,7 @@ class UpdateSnippetService < BaseService def execute # check that user is allowed to set specified visibility_level new_visibility = params[:visibility_level] - + if new_visibility && new_visibility.to_i != snippet.visibility_level unless Gitlab::VisibilityLevel.allowed_for?(current_user, new_visibility) deny_visibility_level(snippet, new_visibility) @@ -17,6 +19,10 @@ class UpdateSnippetService < BaseService end end - snippet.update_attributes(params) + filter_spam_check_params + snippet.assign_attributes(params) + spam_check(snippet, current_user) + + snippet.save end end diff --git a/app/views/layouts/_recaptcha_verification.html.haml b/app/views/layouts/_recaptcha_verification.html.haml new file mode 100644 index 00000000000..77c77dc6754 --- /dev/null +++ b/app/views/layouts/_recaptcha_verification.html.haml @@ -0,0 +1,23 @@ +- humanized_resource_name = spammable.class.model_name.human.downcase +- resource_name = spammable.class.model_name.singular + +%h3.page-title + Anti-spam verification +%hr + +%p + #{"We detected potential spam in the #{humanized_resource_name}. Please solve the reCAPTCHA to proceed."} + += form_for form do |f| + .recaptcha + - params[resource_name].each do |field, value| + = hidden_field(resource_name, field, value: value) + = hidden_field_tag(:spam_log_id, spammable.spam_log.id) + = hidden_field_tag(:recaptcha_verification, true) + = recaptcha_tags + + -# Yields a block with given extra params. + = yield + + .row-content-block.footer-block + = f.submit "Submit #{humanized_resource_name}", class: 'btn btn-create' diff --git a/app/views/projects/issues/verify.html.haml b/app/views/projects/issues/verify.html.haml index 1934b18c086..09aa401e44a 100644 --- a/app/views/projects/issues/verify.html.haml +++ b/app/views/projects/issues/verify.html.haml @@ -1,20 +1,4 @@ -- page_title "Anti-spam verification" +- form = [@project.namespace.becomes(Namespace), @project, @issue] -%h3.page-title - Anti-spam verification -%hr - -%p - We detected potential spam in the issue description. Please verify that you are not a robot to submit the issue. - -= form_for [@project.namespace.becomes(Namespace), @project, @issue] do |f| - .recaptcha - - params[:issue].each do |field, value| - = hidden_field(:issue, field, value: value) - = hidden_field_tag(:merge_request_for_resolving_discussions, params[:merge_request_for_resolving_discussions]) - = hidden_field_tag(:spam_log_id, @issue.spam_log.id) - = hidden_field_tag(:recaptcha_verification, true) - = recaptcha_tags - - .row-content-block.footer-block - = f.submit "Submit #{@issue.class.model_name.human.downcase}", class: 'btn btn-create' += render layout: 'layouts/recaptcha_verification', locals: { spammable: @issue, form: form } do + = hidden_field_tag(:merge_request_for_resolving_discussions, params[:merge_request_for_resolving_discussions]) diff --git a/app/views/projects/snippets/verify.html.haml b/app/views/projects/snippets/verify.html.haml new file mode 100644 index 00000000000..eb56f03b3f4 --- /dev/null +++ b/app/views/projects/snippets/verify.html.haml @@ -0,0 +1,4 @@ +- form = [@project.namespace.becomes(Namespace), @project, @snippet.becomes(Snippet)] + += render 'layouts/recaptcha_verification', spammable: @snippet, form: form + diff --git a/app/views/snippets/verify.html.haml b/app/views/snippets/verify.html.haml new file mode 100644 index 00000000000..cb623ccab57 --- /dev/null +++ b/app/views/snippets/verify.html.haml @@ -0,0 +1,4 @@ +- form = [@snippet.becomes(Snippet)] + += render 'layouts/recaptcha_verification', spammable: @snippet, form: form + diff --git a/changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml b/changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml new file mode 100644 index 00000000000..d70b5ef8fd5 --- /dev/null +++ b/changelogs/unreleased/28093-snippet-and-issue-spam-check-on-edit.yml @@ -0,0 +1,4 @@ +--- +title: Spam check and reCAPTCHA improvements +merge_request: +author: diff --git a/lib/api/helpers.rb b/lib/api/helpers.rb index ae62f1f9580..a1db2099693 100644 --- a/lib/api/helpers.rb +++ b/lib/api/helpers.rb @@ -215,6 +215,10 @@ module API end end + def render_spam_error! + render_api_error!({ error: 'Spam detected' }, 400) + end + def render_api_error!(message, status) error!({ 'message' => message }, status, header) end diff --git a/lib/api/issues.rb b/lib/api/issues.rb index 2b946bfd349..6d30c5d81b1 100644 --- a/lib/api/issues.rb +++ b/lib/api/issues.rb @@ -169,9 +169,13 @@ module API params.delete(:updated_at) end + update_params = declared_params(include_missing: false).merge(request: request, api: true) + issue = ::Issues::UpdateService.new(user_project, current_user, - declared_params(include_missing: false)).execute(issue) + update_params).execute(issue) + + render_spam_error! if issue.spam? if issue.valid? present issue, with: Entities::Issue, current_user: current_user, project: user_project diff --git a/lib/api/project_snippets.rb b/lib/api/project_snippets.rb index dcc0c82ee27..2a1cce73f3f 100644 --- a/lib/api/project_snippets.rb +++ b/lib/api/project_snippets.rb @@ -63,6 +63,8 @@ module API snippet = CreateSnippetService.new(user_project, current_user, snippet_params).execute + render_spam_error! if snippet.spam? + if snippet.persisted? present snippet, with: Entities::ProjectSnippet else @@ -92,12 +94,16 @@ module API authorize! :update_project_snippet, snippet snippet_params = declared_params(include_missing: false) + .merge(request: request, api: true) + snippet_params[:content] = snippet_params.delete(:code) if snippet_params[:code].present? UpdateSnippetService.new(user_project, current_user, snippet, snippet_params).execute - if snippet.persisted? + render_spam_error! if snippet.spam? + + if snippet.valid? present snippet, with: Entities::ProjectSnippet else render_validation_error!(snippet) diff --git a/lib/api/snippets.rb b/lib/api/snippets.rb index eb9ece49e7f..ac03fbd2a3d 100644 --- a/lib/api/snippets.rb +++ b/lib/api/snippets.rb @@ -67,6 +67,8 @@ module API attrs = declared_params(include_missing: false).merge(request: request, api: true) snippet = CreateSnippetService.new(nil, current_user, attrs).execute + render_spam_error! if snippet.spam? + if snippet.persisted? present snippet, with: Entities::PersonalSnippet else @@ -93,9 +95,12 @@ module API return not_found!('Snippet') unless snippet authorize! :update_personal_snippet, snippet - attrs = declared_params(include_missing: false) + attrs = declared_params(include_missing: false).merge(request: request, api: true) UpdateSnippetService.new(nil, current_user, snippet, attrs).execute + + render_spam_error! if snippet.spam? + if snippet.persisted? present snippet, with: Entities::PersonalSnippet else diff --git a/lib/api/v3/issues.rb b/lib/api/v3/issues.rb index ba5b6fdbe52..d0af09f0e1e 100644 --- a/lib/api/v3/issues.rb +++ b/lib/api/v3/issues.rb @@ -149,9 +149,7 @@ module API issue = ::Issues::CreateService.new(user_project, current_user, issue_params.merge(request: request, api: true)).execute - if issue.spam? - render_api_error!({ error: 'Spam detected' }, 400) - end + render_spam_error! if issue.spam? if issue.valid? present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project @@ -182,9 +180,13 @@ module API params.delete(:updated_at) end + update_params = declared_params(include_missing: false).merge(request: request, api: true) + issue = ::Issues::UpdateService.new(user_project, current_user, - declared_params(include_missing: false)).execute(issue) + update_params).execute(issue) + + render_spam_error! if issue.spam? if issue.valid? present issue, with: ::API::Entities::Issue, current_user: current_user, project: user_project diff --git a/lib/api/v3/project_snippets.rb b/lib/api/v3/project_snippets.rb index 9f95d4395fa..e03e941d30b 100644 --- a/lib/api/v3/project_snippets.rb +++ b/lib/api/v3/project_snippets.rb @@ -64,6 +64,8 @@ module API snippet = CreateSnippetService.new(user_project, current_user, snippet_params).execute + render_spam_error! if snippet.spam? + if snippet.persisted? present snippet, with: ::API::V3::Entities::ProjectSnippet else @@ -93,12 +95,16 @@ module API authorize! :update_project_snippet, snippet snippet_params = declared_params(include_missing: false) + .merge(request: request, api: true) + snippet_params[:content] = snippet_params.delete(:code) if snippet_params[:code].present? UpdateSnippetService.new(user_project, current_user, snippet, snippet_params).execute - if snippet.persisted? + render_spam_error! if snippet.spam? + + if snippet.valid? present snippet, with: ::API::V3::Entities::ProjectSnippet else render_validation_error!(snippet) diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index e576bf9ef79..7871b6a9e10 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -152,6 +152,113 @@ describe Projects::IssuesController do end end + context 'Akismet is enabled' do + let(:project) { create(:project_empty_repo, :public) } + + before do + stub_application_setting(recaptcha_enabled: true) + allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) + end + + context 'when an issue is not identified as spam' do + before do + allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) + end + + it 'normally updates the issue' do + expect { update_issue(title: 'Foo') }.to change { issue.reload.title }.to('Foo') + end + end + + context 'when an issue is identified as spam' do + before { allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) } + + context 'when captcha is not verified' do + def update_spam_issue + update_issue(title: 'Spam Title', description: 'Spam lives here') + end + + before { allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) } + + it 'rejects an issue recognized as a spam' do + expect { update_spam_issue }.not_to change{ issue.reload.title } + end + + it 'rejects an issue recognized as a spam when recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + expect { update_spam_issue }.not_to change{ issue.reload.title } + end + + it 'creates a spam log' do + update_spam_issue + + spam_logs = SpamLog.all + + expect(spam_logs.count).to eq(1) + expect(spam_logs.first.title).to eq('Spam Title') + expect(spam_logs.first.recaptcha_verified).to be_falsey + end + + it 'renders verify template' do + update_spam_issue + + expect(response).to render_template(:verify) + end + end + + context 'when captcha is verified' do + let(:spammy_title) { 'Whatever' } + let!(:spam_logs) { create_list(:spam_log, 2, user: user, title: spammy_title) } + + def update_verified_issue + update_issue({ title: spammy_title }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + end + + before do + allow_any_instance_of(described_class).to receive(:verify_recaptcha) + .and_return(true) + end + + it 'redirect to issue page' do + update_verified_issue + + expect(response). + to redirect_to(namespace_project_issue_path(project.namespace, project, issue)) + end + + it 'accepts an issue after recaptcha is verified' do + expect{ update_verified_issue }.to change{ issue.reload.title }.to(spammy_title) + end + + it 'marks spam log as recaptcha_verified' do + expect { update_verified_issue }.to change { SpamLog.last.recaptcha_verified }.from(false).to(true) + end + + it 'does not mark spam log as recaptcha_verified when it does not belong to current_user' do + spam_log = create(:spam_log) + + expect { update_issue(spam_log_id: spam_log.id, recaptcha_verification: true) }. + not_to change { SpamLog.last.recaptcha_verified } + end + end + end + end + + def update_issue(issue_params = {}, additional_params = {}) + params = { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: issue.iid, + issue: issue_params + }.merge(additional_params) + + put :update, params + end + def move_issue put :update, namespace_id: project.namespace.to_param, @@ -384,7 +491,7 @@ describe Projects::IssuesController do allow_any_instance_of(SpamService).to receive(:check_for_spam?).and_return(true) end - context 'when an issue is not identified as a spam' do + context 'when an issue is not identified as spam' do before do allow_any_instance_of(described_class).to receive(:verify_recaptcha).and_return(false) allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(false) @@ -395,7 +502,7 @@ describe Projects::IssuesController do end end - context 'when an issue is identified as a spam' do + context 'when an issue is identified as spam' do before { allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) } context 'when captcha is not verified' do diff --git a/spec/controllers/projects/snippets_controller_spec.rb b/spec/controllers/projects/snippets_controller_spec.rb index 77ee10a1e15..8bab094a79e 100644 --- a/spec/controllers/projects/snippets_controller_spec.rb +++ b/spec/controllers/projects/snippets_controller_spec.rb @@ -70,7 +70,7 @@ describe Projects::SnippetsController do end describe 'POST #create' do - def create_snippet(project, snippet_params = {}) + def create_snippet(project, snippet_params = {}, additional_params = {}) sign_in(user) project.add_developer(user) @@ -79,7 +79,7 @@ describe Projects::SnippetsController do namespace_id: project.namespace.to_param, project_id: project.to_param, project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params) - } + }.merge(additional_params) end context 'when the snippet is spam' do @@ -87,35 +87,179 @@ describe Projects::SnippetsController do allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end - context 'when the project is private' do - let(:private_project) { create(:project_empty_repo, :private) } + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) + end + end + + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + expect(response).to render_template(:new) + end + + it 'creates a spam log' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + + it 'renders :new with recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + create_snippet(project, visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:new) + end - context 'when the snippet is public' do - it 'creates the snippet' do - expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }. - to change { Snippet.count }.by(1) + context 'recaptcha enabled' do + before do + stub_application_setting(recaptcha_enabled: true) end + + it 'renders :verify with recaptcha enabled' do + create_snippet(project, visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:verify) + end + + it 'renders snippet page when recaptcha verified' do + spammy_title = 'Whatever' + + spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title) + create_snippet(project, + { visibility_level: Snippet::PUBLIC }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + + expect(response).to redirect_to(Snippet.last) + end + end + end + end + end + + describe 'PUT #update' do + let(:project) { create :project, :public } + let(:snippet) { create :project_snippet, author: user, project: project, visibility_level: visibility_level } + + def update_snippet(snippet_params = {}, additional_params = {}) + sign_in(user) + + project.add_developer(user) + + put :update, { + namespace_id: project.namespace.to_param, + project_id: project.to_param, + id: snippet.id, + project_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params) + }.merge(additional_params) + + snippet.reload + end + + context 'when the snippet is spam' do + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'updates the snippet' do + expect { update_snippet(title: 'Foo') }. + to change { snippet.reload.title }.to('Foo') end end - context 'when the project is public' do - context 'when the snippet is private' do - it 'creates the snippet' do - expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. - to change { Snippet.count }.by(1) + context 'when the snippet is public' do + let(:visibility_level) { Snippet::PUBLIC } + + it 'rejects the shippet' do + expect { update_snippet(title: 'Foo') }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo') }. + to change { SpamLog.count }.by(1) + end + + it 'renders :edit with recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + update_snippet(title: 'Foo') + + expect(response).to render_template(:edit) + end + + context 'recaptcha enabled' do + before do + stub_application_setting(recaptcha_enabled: true) + end + + it 'renders :verify with recaptcha enabled' do + update_snippet(title: 'Foo') + + expect(response).to render_template(:verify) + end + + it 'renders snippet page when recaptcha verified' do + spammy_title = 'Whatever' + + spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title) + snippet = update_snippet({ title: spammy_title }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + + expect(response).to redirect_to(snippet) end end + end + + context 'when the private snippet is made public' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'rejects the shippet' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + + it 'renders :edit with recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) - context 'when the snippet is public' do - it 'rejects the shippet' do - expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. - not_to change { Snippet.count } - expect(response).to render_template(:new) + update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:edit) + end + + context 'recaptcha enabled' do + before do + stub_application_setting(recaptcha_enabled: true) + end + + it 'renders :verify with recaptcha enabled' do + update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:verify) end - it 'creates a spam log' do - expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. - to change { SpamLog.count }.by(1) + it 'renders snippet page when recaptcha verified' do + spammy_title = 'Whatever' + + spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title) + snippet = update_snippet({ title: spammy_title, visibility_level: Snippet::PUBLIC }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + + expect(response).to redirect_to(snippet) end end end diff --git a/spec/controllers/snippets_controller_spec.rb b/spec/controllers/snippets_controller_spec.rb index f90c0d76ceb..5de3b9890ef 100644 --- a/spec/controllers/snippets_controller_spec.rb +++ b/spec/controllers/snippets_controller_spec.rb @@ -139,12 +139,14 @@ describe SnippetsController do end describe 'POST #create' do - def create_snippet(snippet_params = {}) + def create_snippet(snippet_params = {}, additional_params = {}) sign_in(user) post :create, { personal_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params) - } + }.merge(additional_params) + + Snippet.last end context 'when the snippet is spam' do @@ -163,13 +165,164 @@ describe SnippetsController do it 'rejects the shippet' do expect { create_snippet(visibility_level: Snippet::PUBLIC) }. not_to change { Snippet.count } - expect(response).to render_template(:new) end it 'creates a spam log' do expect { create_snippet(visibility_level: Snippet::PUBLIC) }. to change { SpamLog.count }.by(1) end + + it 'renders :new with recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + create_snippet(visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:new) + end + + context 'recaptcha enabled' do + before do + stub_application_setting(recaptcha_enabled: true) + end + + it 'renders :verify with recaptcha enabled' do + create_snippet(visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:verify) + end + + it 'renders snippet page when recaptcha verified' do + spammy_title = 'Whatever' + + spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title) + snippet = create_snippet({ title: spammy_title }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + + expect(response).to redirect_to(snippet_path(snippet)) + end + end + end + end + end + + describe 'PUT #update' do + let(:project) { create :project } + let(:snippet) { create :personal_snippet, author: user, project: project, visibility_level: visibility_level } + + def update_snippet(snippet_params = {}, additional_params = {}) + sign_in(user) + + put :update, { + id: snippet.id, + personal_snippet: { title: 'Title', content: 'Content' }.merge(snippet_params) + }.merge(additional_params) + + snippet.reload + end + + context 'when the snippet is spam' do + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'updates the snippet' do + expect { update_snippet(title: 'Foo') }. + to change { snippet.reload.title }.to('Foo') + end + end + + context 'when a private snippet is made public' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'rejects the snippet' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + + it 'renders :edit with recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:edit) + end + + context 'recaptcha enabled' do + before do + stub_application_setting(recaptcha_enabled: true) + end + + it 'renders :verify with recaptcha enabled' do + update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) + + expect(response).to render_template(:verify) + end + + it 'renders snippet page when recaptcha verified' do + spammy_title = 'Whatever' + + spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title) + snippet = update_snippet({ title: spammy_title, visibility_level: Snippet::PUBLIC }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + + expect(response).to redirect_to(snippet) + end + end + end + + context 'when the snippet is public' do + let(:visibility_level) { Snippet::PUBLIC } + + it 'rejects the shippet' do + expect { update_snippet(title: 'Foo') }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo') }. + to change { SpamLog.count }.by(1) + end + + it 'renders :edit with recaptcha disabled' do + stub_application_setting(recaptcha_enabled: false) + + update_snippet(title: 'Foo') + + expect(response).to render_template(:edit) + end + + context 'recaptcha enabled' do + before do + stub_application_setting(recaptcha_enabled: true) + end + + it 'renders :verify with recaptcha enabled' do + update_snippet(title: 'Foo') + + expect(response).to render_template(:verify) + end + + it 'renders snippet page when recaptcha verified' do + spammy_title = 'Whatever' + + spam_logs = create_list(:spam_log, 2, user: user, title: spammy_title) + snippet = update_snippet({ title: spammy_title }, + { spam_log_id: spam_logs.last.id, + recaptcha_verification: true }) + + expect(response).to redirect_to(snippet_path(snippet)) + end + end end end end diff --git a/spec/models/concerns/spammable_spec.rb b/spec/models/concerns/spammable_spec.rb index b6e5c95d18a..fd3b8307571 100644 --- a/spec/models/concerns/spammable_spec.rb +++ b/spec/models/concerns/spammable_spec.rb @@ -23,6 +23,7 @@ describe Issue, 'Spammable' do describe '#check_for_spam?' do it 'returns true for public project' do issue.project.update_attribute(:visibility_level, Gitlab::VisibilityLevel::PUBLIC) + expect(issue.check_for_spam?).to eq(true) end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index ece1b43567d..7a0bd5f9721 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -1028,6 +1028,33 @@ describe API::Issues, api: true do end end + describe 'PUT /projects/:id/issues/:issue_id with spam filtering' do + let(:params) do + { + title: 'updated title', + description: 'content here', + labels: 'label, label2' + } + end + + it "does not create a new project issue" do + allow_any_instance_of(SpamService).to receive_messages(check_for_spam?: true) + allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) + + put api("/projects/#{project.id}/issues/#{issue.id}", user), params + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) + + spam_logs = SpamLog.all + expect(spam_logs.count).to eq(1) + expect(spam_logs[0].title).to eq('updated title') + expect(spam_logs[0].description).to eq('content here') + expect(spam_logs[0].user).to eq(user) + expect(spam_logs[0].noteable_type).to eq('Issue') + end + end + describe 'PUT /projects/:id/issues/:issue_id to update labels' do let!(:label) { create(:label, title: 'dummy', project: project) } let!(:label_link) { create(:label_link, label: label, target: issue) } diff --git a/spec/requests/api/project_snippets_spec.rb b/spec/requests/api/project_snippets_spec.rb index f56876bcf54..da9df56401b 100644 --- a/spec/requests/api/project_snippets_spec.rb +++ b/spec/requests/api/project_snippets_spec.rb @@ -78,43 +78,33 @@ describe API::ProjectSnippets, api: true do allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end - context 'when the project is private' do - let(:private_project) { create(:project_empty_repo, :private) } - - context 'when the snippet is public' do - it 'creates the snippet' do - expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }. - to change { Snippet.count }.by(1) - end + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) end end - context 'when the project is public' do - context 'when the snippet is private' do - it 'creates the snippet' do - expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. - to change { Snippet.count }.by(1) - end + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) end - context 'when the snippet is public' do - it 'rejects the shippet' do - expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. - not_to change { Snippet.count } - expect(response).to have_http_status(400) - end - - it 'creates a spam log' do - expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. - to change { SpamLog.count }.by(1) - end + it 'creates a spam log' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) end end end end describe 'PUT /projects/:project_id/snippets/:id/' do - let(:snippet) { create(:project_snippet, author: admin) } + let(:visibility_level) { Snippet::PUBLIC } + let(:snippet) { create(:project_snippet, author: admin, visibility_level: visibility_level) } it 'updates snippet' do new_content = 'New content' @@ -138,6 +128,56 @@ describe API::ProjectSnippets, api: true do expect(response).to have_http_status(400) end + + context 'when the snippet is spam' do + def update_snippet(snippet_params = {}) + put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin), snippet_params + end + + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'creates the snippet' do + expect { update_snippet(title: 'Foo') }. + to change { snippet.reload.title }.to('Foo') + end + end + + context 'when the snippet is public' do + let(:visibility_level) { Snippet::PUBLIC } + + it 'rejects the snippet' do + expect { update_snippet(title: 'Foo') }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo') }. + to change { SpamLog.count }.by(1) + end + end + + context 'when the private snippet is made public' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'rejects the snippet' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + not_to change { snippet.reload.title } + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end end describe 'DELETE /projects/:project_id/snippets/:id/' do diff --git a/spec/requests/api/snippets_spec.rb b/spec/requests/api/snippets_spec.rb index 1ef92930b3c..41def7cd1d4 100644 --- a/spec/requests/api/snippets_spec.rb +++ b/spec/requests/api/snippets_spec.rb @@ -129,7 +129,9 @@ describe API::Snippets, api: true do it 'rejects the shippet' do expect { create_snippet(visibility_level: Snippet::PUBLIC) }. not_to change { Snippet.count } + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) end it 'creates a spam log' do @@ -141,16 +143,20 @@ describe API::Snippets, api: true do end describe 'PUT /snippets/:id' do + let(:visibility_level) { Snippet::PUBLIC } let(:other_user) { create(:user) } - let(:public_snippet) { create(:personal_snippet, :public, author: user) } + let(:snippet) do + create(:personal_snippet, author: user, visibility_level: visibility_level) + end + it 'updates snippet' do new_content = 'New content' - put api("/snippets/#{public_snippet.id}", user), content: new_content + put api("/snippets/#{snippet.id}", user), content: new_content expect(response).to have_http_status(200) - public_snippet.reload - expect(public_snippet.content).to eq(new_content) + snippet.reload + expect(snippet.content).to eq(new_content) end it 'returns 404 for invalid snippet id' do @@ -161,7 +167,7 @@ describe API::Snippets, api: true do end it "returns 404 for another user's snippet" do - put api("/snippets/#{public_snippet.id}", other_user), title: 'fubar' + put api("/snippets/#{snippet.id}", other_user), title: 'fubar' expect(response).to have_http_status(404) expect(json_response['message']).to eq('404 Snippet Not Found') @@ -172,6 +178,56 @@ describe API::Snippets, api: true do expect(response).to have_http_status(400) end + + context 'when the snippet is spam' do + def update_snippet(snippet_params = {}) + put api("/snippets/#{snippet.id}", user), snippet_params + end + + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'updates the snippet' do + expect { update_snippet(title: 'Foo') }. + to change { snippet.reload.title }.to('Foo') + end + end + + context 'when the snippet is public' do + let(:visibility_level) { Snippet::PUBLIC } + + it 'rejects the shippet' do + expect { update_snippet(title: 'Foo') }. + not_to change { snippet.reload.title } + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo') }. + to change { SpamLog.count }.by(1) + end + end + + context 'when a private snippet is made public' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'rejects the snippet' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end end describe 'DELETE /snippets/:id' do diff --git a/spec/requests/api/v3/issues_spec.rb b/spec/requests/api/v3/issues_spec.rb index 33a127de98a..8e6732fe23e 100644 --- a/spec/requests/api/v3/issues_spec.rb +++ b/spec/requests/api/v3/issues_spec.rb @@ -986,6 +986,33 @@ describe API::V3::Issues, api: true do end end + describe 'PUT /projects/:id/issues/:issue_id with spam filtering' do + let(:params) do + { + title: 'updated title', + description: 'content here', + labels: 'label, label2' + } + end + + it "does not create a new project issue" do + allow_any_instance_of(SpamService).to receive_messages(check_for_spam?: true) + allow_any_instance_of(AkismetService).to receive_messages(is_spam?: true) + + put v3_api("/projects/#{project.id}/issues/#{issue.id}", user), params + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) + + spam_logs = SpamLog.all + expect(spam_logs.count).to eq(1) + expect(spam_logs[0].title).to eq('updated title') + expect(spam_logs[0].description).to eq('content here') + expect(spam_logs[0].user).to eq(user) + expect(spam_logs[0].noteable_type).to eq('Issue') + end + end + describe 'PUT /projects/:id/issues/:issue_id to update labels' do let!(:label) { create(:label, title: 'dummy', project: project) } let!(:label_link) { create(:label_link, label: label, target: issue) } diff --git a/spec/requests/api/v3/project_snippets_spec.rb b/spec/requests/api/v3/project_snippets_spec.rb index 3700477f0db..957a3bf97ef 100644 --- a/spec/requests/api/v3/project_snippets_spec.rb +++ b/spec/requests/api/v3/project_snippets_spec.rb @@ -85,43 +85,33 @@ describe API::ProjectSnippets, api: true do allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) end - context 'when the project is private' do - let(:private_project) { create(:project_empty_repo, :private) } - - context 'when the snippet is public' do - it 'creates the snippet' do - expect { create_snippet(private_project, visibility_level: Snippet::PUBLIC) }. - to change { Snippet.count }.by(1) - end + context 'when the snippet is private' do + it 'creates the snippet' do + expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. + to change { Snippet.count }.by(1) end end - context 'when the project is public' do - context 'when the snippet is private' do - it 'creates the snippet' do - expect { create_snippet(project, visibility_level: Snippet::PRIVATE) }. - to change { Snippet.count }.by(1) - end + context 'when the snippet is public' do + it 'rejects the shippet' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + not_to change { Snippet.count } + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) end - context 'when the snippet is public' do - it 'rejects the shippet' do - expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. - not_to change { Snippet.count } - expect(response).to have_http_status(400) - end - - it 'creates a spam log' do - expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. - to change { SpamLog.count }.by(1) - end + it 'creates a spam log' do + expect { create_snippet(project, visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) end end end end describe 'PUT /projects/:project_id/snippets/:id/' do - let(:snippet) { create(:project_snippet, author: admin) } + let(:visibility_level) { Snippet::PUBLIC } + let(:snippet) { create(:project_snippet, author: admin, visibility_level: visibility_level) } it 'updates snippet' do new_content = 'New content' @@ -145,6 +135,56 @@ describe API::ProjectSnippets, api: true do expect(response).to have_http_status(400) end + + context 'when the snippet is spam' do + def update_snippet(snippet_params = {}) + put v3_api("/projects/#{snippet.project.id}/snippets/#{snippet.id}", admin), snippet_params + end + + before do + allow_any_instance_of(AkismetService).to receive(:is_spam?).and_return(true) + end + + context 'when the snippet is private' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'creates the snippet' do + expect { update_snippet(title: 'Foo') }. + to change { snippet.reload.title }.to('Foo') + end + end + + context 'when the snippet is public' do + let(:visibility_level) { Snippet::PUBLIC } + + it 'rejects the snippet' do + expect { update_snippet(title: 'Foo') }. + not_to change { snippet.reload.title } + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo') }. + to change { SpamLog.count }.by(1) + end + end + + context 'when the private snippet is made public' do + let(:visibility_level) { Snippet::PRIVATE } + + it 'rejects the snippet' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + not_to change { snippet.reload.title } + + expect(response).to have_http_status(400) + expect(json_response['message']).to eq({ "error" => "Spam detected" }) + end + + it 'creates a spam log' do + expect { update_snippet(title: 'Foo', visibility_level: Snippet::PUBLIC) }. + to change { SpamLog.count }.by(1) + end + end + end end describe 'DELETE /projects/:project_id/snippets/:id/' do diff --git a/spec/services/issues/create_service_spec.rb b/spec/services/issues/create_service_spec.rb index e1feeed8a67..6045d00ff09 100644 --- a/spec/services/issues/create_service_spec.rb +++ b/spec/services/issues/create_service_spec.rb @@ -230,16 +230,6 @@ describe Issues::CreateService, services: true do expect { issue }.not_to change{SpamLog.last.recaptcha_verified} end end - - context 'when spam log title does not match the issue title' do - before do - opts[:title] = 'Another issue' - end - - it 'does not mark spam_log as recaptcha_verified' do - expect { issue }.not_to change{SpamLog.last.recaptcha_verified} - end - end end context 'when recaptcha was not verified' do diff --git a/spec/services/spam_service_spec.rb b/spec/services/spam_service_spec.rb index 271c17dd8c0..4ce3b95aa87 100644 --- a/spec/services/spam_service_spec.rb +++ b/spec/services/spam_service_spec.rb @@ -1,46 +1,61 @@ require 'spec_helper' describe SpamService, services: true do - describe '#check' do - let(:project) { create(:project, :public) } - let(:issue) { create(:issue, project: project) } - let(:request) { double(:request, env: {}) } + describe '#when_recaptcha_verified' do + def check_spam(issue, request, recaptcha_verified) + described_class.new(issue, request).when_recaptcha_verified(recaptcha_verified) do + 'yielded' + end + end + + it 'yields block when recaptcha was already verified' do + issue = build_stubbed(:issue) - def check_spam(issue, request) - described_class.new(issue, request).check + expect(check_spam(issue, nil, true)).to eql('yielded') end - context 'when indicated as spam by akismet' do - before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) } + context 'when recaptcha was not verified' do + let(:project) { create(:project, :public) } + let(:issue) { create(:issue, project: project) } + let(:request) { double(:request, env: {}) } - it 'returns false when request is missing' do - expect(check_spam(issue, nil)).to be_falsey - end + context 'when indicated as spam by akismet' do + before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: true)) } - it 'returns false when issue is not public' do - issue = create(:issue, project: create(:project, :private)) + it 'doesnt check as spam when request is missing' do + check_spam(issue, nil, false) - expect(check_spam(issue, request)).to be_falsey - end + expect(issue.spam).to be_falsey + end - it 'returns true' do - expect(check_spam(issue, request)).to be_truthy - end + it 'checks as spam' do + check_spam(issue, request, false) - it 'creates a spam log' do - expect { check_spam(issue, request) }.to change { SpamLog.count }.from(0).to(1) - end - end + expect(issue.spam).to be_truthy + end - context 'when not indicated as spam by akismet' do - before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) } + it 'creates a spam log' do + expect { check_spam(issue, request, false) } + .to change { SpamLog.count }.from(0).to(1) + end - it 'returns false' do - expect(check_spam(issue, request)).to be_falsey + it 'doesnt yield block' do + expect(check_spam(issue, request, false)) + .to eql(SpamLog.last) + end end - it 'does not create a spam log' do - expect { check_spam(issue, request) }.not_to change { SpamLog.count } + context 'when not indicated as spam by akismet' do + before { allow(AkismetService).to receive(:new).and_return(double(is_spam?: false)) } + + it 'returns false' do + expect(check_spam(issue, request, false)).to be_falsey + end + + it 'does not create a spam log' do + expect { check_spam(issue, request, false) } + .not_to change { SpamLog.count } + end end end end |