Commit f6b450b9 authored by charlie ablett's avatar charlie ablett

Merge branch 'clean-up-captcha-controller-logic' into 'master'

Clean up CAPTCHA controller logic and modules

See merge request gitlab-org/gitlab!66999
parents b1e7f2b5 b4e82ec5
......@@ -4,6 +4,9 @@ module IssuableActions
extend ActiveSupport::Concern
include Gitlab::Utils::StrongMemoize
include Gitlab::Cache::Helpers
include SpammableActions::AkismetMarkAsSpamAction
include SpammableActions::CaptchaCheck::HtmlFormatActionsSupport
include SpammableActions::CaptchaCheck::JsonFormatActionsSupport
included do
before_action :authorize_destroy_issuable!, only: :destroy
......@@ -25,17 +28,42 @@ module IssuableActions
end
def update
@issuable = update_service.execute(issuable) # rubocop:disable Gitlab/ModuleWithInstanceVariables
respond_to do |format|
format.html do
recaptcha_check_if_spammable { render :edit }
updated_issuable = update_service.execute(issuable)
# NOTE: We only assign the instance variable on this line, and use the local variable
# everywhere else in the method, to avoid having to add multiple `rubocop:disable` comments.
@issuable = updated_issuable # rubocop:disable Gitlab/ModuleWithInstanceVariables
# NOTE: This check for `is_a?(Spammable)` is necessary because not all
# possible `issuable` types implement Spammable. Once they all implement Spammable,
# this check can be removed.
if updated_issuable.is_a?(Spammable)
respond_to do |format|
format.html do
# NOTE: This redirect is intentionally only performed in the case where the updated
# issuable is a spammable, and intentionally is not performed in the non-spammable case.
# This preserves the legacy behavior of this action.
if updated_issuable.valid?
redirect_to spammable_path
else
with_captcha_check_html_format { render :edit }
end
end
format.json do
with_captcha_check_json_format { render_entity_json }
end
end
format.json do
recaptcha_check_if_spammable(false) { render_entity_json }
else
respond_to do |format|
format.html do
render :edit
end
format.json do
render_entity_json
end
end
end
rescue ActiveRecord::StaleObjectError
render_conflict_response
end
......@@ -171,12 +199,6 @@ module IssuableActions
DiscussionSerializer.new(project: project, noteable: issuable, current_user: current_user, note_entity: ProjectNoteEntity)
end
def recaptcha_check_if_spammable(should_redirect = true, &block)
return yield unless issuable.is_a? Spammable
recaptcha_check_with_fallback(should_redirect, &block)
end
def render_conflict_response
respond_to do |format|
format.html do
......
# frozen_string_literal: true
module SpammableActions
extend ActiveSupport::Concern
include Spam::Concerns::HasSpamActionResponseFields
included do
before_action :authorize_submit_spammable!, only: :mark_as_spam
end
def mark_as_spam
if Spam::MarkAsSpamService.new(target: spammable).execute
redirect_to spammable_path, notice: _("%{spammable_titlecase} was submitted to Akismet successfully.") % { spammable_titlecase: spammable.spammable_entity_type.titlecase }
else
redirect_to spammable_path, alert: _('Error with Akismet. Please check the logs for more info.')
end
end
private
def recaptcha_check_with_fallback(should_redirect = true, &fallback)
if should_redirect && spammable.valid?
redirect_to spammable_path
elsif spammable.render_recaptcha?
Gitlab::Recaptcha.load_configurations!
respond_to do |format|
format.html do
# NOTE: format.html is still used by issue create, and uses the legacy HAML
# `_recaptcha_form.html.haml` rendered via the `projects/issues/verify` template.
render :verify
end
format.json do
# format.json is used by all new Vue-based CAPTCHA implementations, which
# handle all of the CAPTCHA form rendering on the client via the Pajamas-based
# app/assets/javascripts/captcha/captcha_modal.vue
# NOTE: "409 - Conflict" seems to be the most appropriate HTTP status code for a response
# which requires a CAPTCHA to be solved in order for the request to be resubmitted.
# See https://stackoverflow.com/q/26547466/25192
render json: spam_action_response_fields(spammable), status: :conflict
end
end
else
yield
end
end
# TODO: This method is currently only needed for issue create, to convert spam/CAPTCHA values from
# params, and instead be passed as headers, as the spam services now all expect. It can be removed
# when issue create is is converted to a client/JS based approach instead of the legacy HAML
# `_recaptcha_form.html.haml` which is rendered via the `projects/issues/verify` template.
# In that case, which is based on the legacy reCAPTCHA implementation using the HTML/HAML form,
# the 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the
# recaptcha gem, which is called from the HAML `_recaptcha_form.html.haml` form.
def extract_legacy_spam_params_to_headers
request.headers['X-GitLab-Captcha-Response'] = params['g-recaptcha-response'] || params[:captcha_response]
request.headers['X-GitLab-Spam-Log-Id'] = params[:spam_log_id]
end
def spammable
raise NotImplementedError, "#{self.class} does not implement #{__method__}"
end
def spammable_path
raise NotImplementedError, "#{self.class} does not implement #{__method__}"
end
def authorize_submit_spammable!
access_denied! unless current_user.admin?
end
end
# frozen_string_literal: true
module SpammableActions::AkismetMarkAsSpamAction
extend ActiveSupport::Concern
include SpammableActions::Attributes
included do
before_action :authorize_submit_spammable!, only: :mark_as_spam
end
def mark_as_spam
if Spam::AkismetMarkAsSpamService.new(target: spammable).execute
redirect_to spammable_path, notice: _("%{spammable_titlecase} was submitted to Akismet successfully.") % { spammable_titlecase: spammable.spammable_entity_type.titlecase }
else
redirect_to spammable_path, alert: _('Error with Akismet. Please check the logs for more info.')
end
end
private
def authorize_submit_spammable!
access_denied! unless current_user.can_admin_all_resources?
end
def spammable_path
raise NotImplementedError, "#{self.class} does not implement #{__method__}"
end
end
# frozen_string_literal: true
module SpammableActions
module Attributes
extend ActiveSupport::Concern
private
def spammable
raise NotImplementedError, "#{self.class} does not implement #{__method__}"
end
end
end
# frozen_string_literal: true
module SpammableActions::CaptchaCheck
module Common
extend ActiveSupport::Concern
private
def with_captcha_check_common(captcha_render_lambda:, &block)
# If the Spammable indicates that CAPTCHA is not necessary (either due to it not being flagged
# as spam, or if spam/captcha is disabled for some reason), then we will go ahead and
# yield to the block containing the action's original behavior, then return.
return yield unless spammable.render_recaptcha?
# If we got here, we need to render the CAPTCHA instead of yielding to action's original
# behavior. We will present a CAPTCHA to be solved by executing the lambda which was passed
# as the `captcha_render_lambda:` argument. This lambda contains either the HTML-specific or
# JSON-specific behavior to cause the CAPTCHA modal to be rendered.
Gitlab::Recaptcha.load_configurations!
captcha_render_lambda.call
end
end
end
# frozen_string_literal: true
# This module should *ONLY* be included if needed to support forms submits with HTML MIME type.
# In other words, forms handled by actions which use a `respond_to` of `format.html`.
#
# If the request is handled by actions via `format.json`, for example, for all Javascript based form
# submissions and Vue components which use Apollo and Axios, then the corresponding module
# which supports JSON format should be used instead.
module SpammableActions::CaptchaCheck::HtmlFormatActionsSupport
extend ActiveSupport::Concern
include SpammableActions::Attributes
include SpammableActions::CaptchaCheck::Common
included do
before_action :convert_html_spam_params_to_headers, only: [:create, :update]
end
private
def with_captcha_check_html_format(&block)
captcha_render_lambda = -> { render :verify }
with_captcha_check_common(captcha_render_lambda: captcha_render_lambda, &block)
end
# Convert spam/CAPTCHA values from form field params to headers, because all spam-related services
# expect these values to be passed as headers.
#
# The 'g-recaptcha-response' field name comes from `Recaptcha::ClientHelper#recaptcha_tags` in the
# recaptcha gem. This is a field which is automatically included by calling the
# `#recaptcha_tags` method within a HAML template's form.
def convert_html_spam_params_to_headers
request.headers['X-GitLab-Captcha-Response'] = params['g-recaptcha-response'] if params['g-recaptcha-response']
request.headers['X-GitLab-Spam-Log-Id'] = params[:spam_log_id] if params[:spam_log_id]
end
end
# frozen_string_literal: true
# This module should be included to support forms submits with a 'js' or 'json' type of MIME type.
# In other words, forms handled by actions which use a `respond_to` of `format.js` or `format.json`.
#
# For example, for all Javascript based form submissions and Vue components which use Apollo and Axios
#
# If the request is handled by actions via `format.html`, then the corresponding module which
# supports HTML format should be used instead.
module SpammableActions::CaptchaCheck::JsonFormatActionsSupport
extend ActiveSupport::Concern
include SpammableActions::Attributes
include SpammableActions::CaptchaCheck::Common
include Spam::Concerns::HasSpamActionResponseFields
private
def with_captcha_check_json_format(&block)
# NOTE: "409 - Conflict" seems to be the most appropriate HTTP status code for a response
# which requires a CAPTCHA to be solved in order for the request to be resubmitted.
# See https://stackoverflow.com/q/26547466/25192
captcha_render_lambda = -> { render json: spam_action_response_fields(spammable), status: :conflict }
with_captcha_check_common(captcha_render_lambda: captcha_render_lambda, &block)
end
end
......@@ -7,7 +7,6 @@ class Projects::IssuesController < Projects::ApplicationController
include ToggleAwardEmoji
include IssuableCollections
include IssuesCalendar
include SpammableActions
include RecordUserLastActivity
ISSUES_EXCEPT_ACTIONS = %i[index calendar new create bulk_update import_csv export_csv service_desk].freeze
......@@ -129,7 +128,6 @@ class Projects::IssuesController < Projects::ApplicationController
end
def create
extract_legacy_spam_params_to_headers
create_params = issue_params.merge(
merge_request_to_resolve_discussions_of: params[:merge_request_to_resolve_discussions_of],
discussion_to_resolve: params[:discussion_to_resolve]
......@@ -149,10 +147,11 @@ class Projects::IssuesController < Projects::ApplicationController
end
end
respond_to do |format|
format.html do
recaptcha_check_with_fallback { render :new }
end
if @issue.valid?
redirect_to project_issue_path(@project, @issue)
else
# NOTE: this CAPTCHA support method is indirectly included via IssuableActions
with_captcha_check_html_format { render :new }
end
end
......
......@@ -4,7 +4,7 @@ class Projects::SnippetsController < Projects::Snippets::ApplicationController
extend ::Gitlab::Utils::Override
include SnippetsActions
include ToggleAwardEmoji
include SpammableActions
include SpammableActions::AkismetMarkAsSpamAction
before_action :check_snippets_available!
......
......@@ -4,7 +4,7 @@ class SnippetsController < Snippets::ApplicationController
include SnippetsActions
include PreviewMarkdown
include ToggleAwardEmoji
include SpammableActions
include SpammableActions::AkismetMarkAsSpamAction
before_action :snippet, only: [:show, :edit, :raw, :toggle_award_emoji, :mark_as_spam]
......
......@@ -23,7 +23,7 @@ module Mutations
private
def mark_as_spam(snippet)
Spam::MarkAsSpamService.new(target: snippet).execute
Spam::AkismetMarkAsSpamService.new(target: snippet).execute
end
def authorized_resource?(snippet)
......
# frozen_string_literal: true
module Spam
class MarkAsSpamService
class AkismetMarkAsSpamService
include ::AkismetMethods
attr_accessor :target, :options
......@@ -9,12 +9,12 @@ module Spam
def initialize(target:)
@target = target
@options = {}
end
def execute
@options[:ip_address] = @target.ip_address
@options[:user_agent] = @target.user_agent
end
def execute
return unless target.submittable_as_spam?
return unless akismet.submit_spam
......
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe SpammableActions::AkismetMarkAsSpamAction do
include AfterNextHelpers
controller(ActionController::Base) do
include SpammableActions::AkismetMarkAsSpamAction
private
def spammable_path
'/fake_spammable_path'
end
end
let(:spammable_type) { 'SpammableType' }
let(:spammable) { double(:spammable, spammable_entity_type: double(:spammable_entity_type, titlecase: spammable_type)) }
let(:current_user) { create(:admin) }
before do
allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
routes.draw { get 'mark_as_spam' => 'anonymous#mark_as_spam' }
allow(controller).to receive(:spammable) { spammable }
allow(controller).to receive(:current_user) { double(:current_user, admin?: admin) }
allow(controller).to receive(:current_user).and_return(current_user)
end
describe '#mark_as_spam' do
subject { post :mark_as_spam }
before do
expect_next(Spam::AkismetMarkAsSpamService, target: spammable)
.to receive(:execute).and_return(execute_result)
end
context 'when user is admin', :enable_admin_mode do
let(:admin) { true }
context 'when service returns truthy' do
let(:execute_result) { true }
it 'redirects with notice' do
expect(subject).to redirect_to('/fake_spammable_path')
expect(subject.request.flash[:notice]).to match(/#{spammable_type}.*submitted.*successfully/)
end
end
context 'when service returns falsey' do
let(:execute_result) { false }
it 'redirects with notice' do
expect(subject).to redirect_to('/fake_spammable_path')
expect(subject.request.flash[:alert]).to match(/Error/)
end
end
end
context 'when user is not admin' do
let(:admin) { false }
let(:execute_result) { true }
it 'calls #access_denied!' do
expect(controller).to receive(:access_denied!) { false }
subject
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe SpammableActions::CaptchaCheck::HtmlFormatActionsSupport do
controller(ActionController::Base) do
include SpammableActions::CaptchaCheck::HtmlFormatActionsSupport
def create
with_captcha_check_html_format { render :some_rendered_view }
end
end
let(:spammable) { double(:spammable) }
before do
allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
routes.draw { get 'create' => 'anonymous#create' }
allow(controller).to receive(:spammable) { spammable }
expect(spammable).to receive(:render_recaptcha?).at_least(:once) { render_recaptcha }
end
describe '#convert_html_spam_params_to_headers' do
let(:render_recaptcha) { false }
let(:g_recaptcha_response) { 'abc123' }
let(:spam_log_id) { 42 }
let(:params) do
{
'g-recaptcha-response' => g_recaptcha_response,
spam_log_id: spam_log_id
}
end
# NOTE: `:update` has an identical `before_action` behavior to ``:create``, but `before_action` is
# declarative via the ``:only`` attribute, so there's little value in re-testing the behavior.
subject { post :create, params: params }
before do
allow(controller).to receive(:render).with(:some_rendered_view)
end
it 'converts params to headers' do
subject
expect(controller.request.headers['X-GitLab-Captcha-Response']).to eq(g_recaptcha_response)
expect(controller.request.headers['X-GitLab-Spam-Log-Id']).to eq(spam_log_id.to_s)
end
end
describe '#with_captcha_check_html_format' do
subject { post :create }
context 'when spammable.render_recaptcha? is true' do
let(:render_recaptcha) { true }
it 'renders :verify' do
expect(controller).to receive(:render).with(:verify)
subject
end
end
context 'when spammable.render_recaptcha? is false' do
let(:render_recaptcha) { false }
it 'yields to block' do
expect(controller).to receive(:render).with(:some_rendered_view)
subject
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe SpammableActions::CaptchaCheck::JsonFormatActionsSupport do
controller(ActionController::Base) do
include SpammableActions::CaptchaCheck::JsonFormatActionsSupport
def some_action
with_captcha_check_json_format { render :some_rendered_view }
end
end
before do
allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
end
describe '#with_captcha_check_json_format' do
subject { post :some_action }
let(:spammable) { double(:spammable) }
before do
routes.draw { get 'some_action' => 'anonymous#some_action' }
allow(controller).to receive(:spammable) { spammable }
expect(spammable).to receive(:render_recaptcha?).at_least(:once) { render_recaptcha }
end
context 'when spammable.render_recaptcha? is true' do
let(:render_recaptcha) { true }
let(:spam_log) { double(:spam_log, id: 1) }
let(:spammable) { double(:spammable, spam?: true, render_recaptcha?: render_recaptcha, spam_log: spam_log) }
let(:recaptcha_site_key) { 'abc123' }
let(:spam_action_response_fields) do
{
spam: true,
needs_captcha_response: render_recaptcha,
spam_log_id: 1,
captcha_site_key: recaptcha_site_key
}
end
it 'renders json containing spam_action_response_fields' do
expect(controller).to receive(:render).with(json: spam_action_response_fields, status: :conflict)
allow(Gitlab::CurrentSettings).to receive(:recaptcha_site_key) { recaptcha_site_key }
subject
end
end
context 'when spammable.render_recaptcha? is false' do
let(:render_recaptcha) { false }
it 'yields to block' do
expect(controller).to receive(:render).with(:some_rendered_view)
subject
end
end
end
end
# frozen_string_literal: true
require 'spec_helper'
RSpec.describe SpammableActions do
controller(ActionController::Base) do
include SpammableActions
# #update is used here to test #recaptcha_check_with_fallback, but it could be invoked
# from #create or any other action which mutates a spammable via a controller.
def update
should_redirect = params[:should_redirect] == 'true'
recaptcha_check_with_fallback(should_redirect) { render json: :ok }
end
private
def spammable_path
'/fake_spammable_path'
end
end
before do
allow(Gitlab::Recaptcha).to receive(:load_configurations!) { true }
end
describe '#recaptcha_check_with_fallback' do
shared_examples 'yields to block' do
it do
subject
expect(json_response).to eq({ json: 'ok' })
end
end
let(:format) { :html }
subject { post :update, format: format, params: params }
let(:spammable) { double(:spammable) }
let(:should_redirect) { nil }
let(:params) do
{
should_redirect: should_redirect
}
end
before do
routes.draw { get 'update' => 'anonymous#update' }
allow(controller).to receive(:spammable) { spammable }
end
context 'when should_redirect is true and spammable is valid' do
let(:should_redirect) { true }
before do
allow(spammable).to receive(:valid?) { true }
end
it 'redirects to spammable_path' do
expect(subject).to redirect_to('/fake_spammable_path')
end
end
context 'when should_redirect is false or spammable is not valid' do
before do
allow(spammable).to receive(:valid?) { false }
end
context 'when spammable.render_recaptcha? is true' do
let(:spam_log) { instance_double(SpamLog, id: 123) }
let(:captcha_site_key) { 'abc123' }
before do
expect(spammable).to receive(:render_recaptcha?).at_least(:once) { true }
end
context 'when format is :html' do
it 'renders :verify' do
expect(controller).to receive(:render).with(:verify)
subject
end
end
context 'when format is :json' do
let(:format) { :json }
before do
expect(spammable).to receive(:spam?) { false }
expect(spammable).to receive(:spam_log) { spam_log }
expect(Gitlab::CurrentSettings).to receive(:recaptcha_site_key) { captcha_site_key }
end
it 'renders json with spam_action_response_fields' do
subject
expected_json_response = HashWithIndifferentAccess.new(
{
spam: false,
needs_captcha_response: true,
spam_log_id: spam_log.id,
captcha_site_key: captcha_site_key
})
expect(json_response).to eq(expected_json_response)
end
end
end
end
end
end
......@@ -1464,7 +1464,7 @@ RSpec.describe Projects::IssuesController do
}
end
it 'updates issue' do
it 'updates issue', :enable_admin_mode do
post_spam
expect(issue.submittable_as_spam?).to be_falsey
end
......
......@@ -110,7 +110,7 @@ RSpec.describe Projects::SnippetsController do
}
end
it 'updates the snippet' do
it 'updates the snippet', :enable_admin_mode do
mark_as_spam
expect(snippet.reload).not_to be_submittable_as_spam
......
......@@ -231,7 +231,7 @@ RSpec.describe SnippetsController do
post :mark_as_spam, params: { id: public_snippet.id }
end
it 'updates the snippet' do
it 'updates the snippet', :enable_admin_mode do
mark_as_spam
expect(public_snippet.reload).not_to be_submittable_as_spam
......
......@@ -58,7 +58,7 @@ RSpec.describe 'Mark snippet as spam' do
end
it 'marks snippet as spam' do
expect_next(Spam::MarkAsSpamService, target: snippet)
expect_next(Spam::AkismetMarkAsSpamService, target: snippet)
.to receive(:execute).and_return(true)
post_graphql_mutation(mutation, current_user: current_user)
......
......@@ -2,7 +2,7 @@
require 'spec_helper'
RSpec.describe Spam::MarkAsSpamService do
RSpec.describe Spam::AkismetMarkAsSpamService do
let(:user_agent_detail) { build(:user_agent_detail) }
let(:spammable) { build(:issue, user_agent_detail: user_agent_detail) }
let(:fake_akismet_service) { double(:akismet_service, submit_spam: true) }
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment