diff options
author | Oswaldo Ferreira <oswaldo@gitlab.com> | 2017-02-14 22:07:11 +0300 |
---|---|---|
committer | Oswaldo Ferreira <oswluizf@gmail.com> | 2017-02-21 19:32:49 +0300 |
commit | 2ace39f2420abf018ceef6aaad52e4917bcbab7d (patch) | |
tree | cae709a6381c80c70af5da459c3ffa992593843d /app | |
parent | 881529495379505542033bf7fb0d91cdc5b51e8d (diff) |
Spam check and reCAPTCHA improvements
Diffstat (limited to 'app')
-rw-r--r-- | app/controllers/concerns/spammable_actions.rb | 30 | ||||
-rw-r--r-- | app/controllers/projects/issues_controller.rb | 34 | ||||
-rw-r--r-- | app/controllers/projects/snippets_controller.rb | 21 | ||||
-rw-r--r-- | app/controllers/snippets_controller.rb | 13 | ||||
-rw-r--r-- | app/models/concerns/spammable.rb | 2 | ||||
-rw-r--r-- | app/models/project_snippet.rb | 4 | ||||
-rw-r--r-- | app/services/create_snippet_service.rb | 10 | ||||
-rw-r--r-- | app/services/issuable_base_service.rb | 32 | ||||
-rw-r--r-- | app/services/issues/create_service.rb | 21 | ||||
-rw-r--r-- | app/services/issues/update_service.rb | 8 | ||||
-rw-r--r-- | app/services/spam_check_service.rb | 24 | ||||
-rw-r--r-- | app/services/spam_service.rb | 31 | ||||
-rw-r--r-- | app/services/update_snippet_service.rb | 10 | ||||
-rw-r--r-- | app/views/layouts/_recaptcha_verification.html.haml | 23 | ||||
-rw-r--r-- | app/views/projects/issues/verify.html.haml | 22 | ||||
-rw-r--r-- | app/views/projects/snippets/verify.html.haml | 4 | ||||
-rw-r--r-- | app/views/snippets/verify.html.haml | 4 |
17 files changed, 175 insertions, 118 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 366804ab17e..a632c36cfb8 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 + |