diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2021-08-11 09:10:02 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2021-08-11 09:10:02 +0300 |
commit | 2000704b7a6cc9eb37dd597ed03567265eda9308 (patch) | |
tree | bf52d4b9b4d5420a6e2b2974f34326e0eda8a25f /app/controllers | |
parent | f6dad4cfc60d81a00c5ef33b7680d3cca4393e29 (diff) |
Add latest changes from gitlab-org/gitlab@master
Diffstat (limited to 'app/controllers')
10 files changed, 167 insertions, 95 deletions
diff --git a/app/controllers/concerns/issuable_actions.rb b/app/controllers/concerns/issuable_actions.rb index 2664a7b7151..7ee680db7f9 100644 --- a/app/controllers/concerns/issuable_actions.rb +++ b/app/controllers/concerns/issuable_actions.rb @@ -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 diff --git a/app/controllers/concerns/spammable_actions.rb b/app/controllers/concerns/spammable_actions.rb deleted file mode 100644 index eb1223f22a9..00000000000 --- a/app/controllers/concerns/spammable_actions.rb +++ /dev/null @@ -1,73 +0,0 @@ -# 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 diff --git a/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb b/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb new file mode 100644 index 00000000000..234c591ffb7 --- /dev/null +++ b/app/controllers/concerns/spammable_actions/akismet_mark_as_spam_action.rb @@ -0,0 +1,28 @@ +# 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 diff --git a/app/controllers/concerns/spammable_actions/attributes.rb b/app/controllers/concerns/spammable_actions/attributes.rb new file mode 100644 index 00000000000..d7060e47c07 --- /dev/null +++ b/app/controllers/concerns/spammable_actions/attributes.rb @@ -0,0 +1,13 @@ +# 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 diff --git a/app/controllers/concerns/spammable_actions/captcha_check/common.rb b/app/controllers/concerns/spammable_actions/captcha_check/common.rb new file mode 100644 index 00000000000..7c047e02a1d --- /dev/null +++ b/app/controllers/concerns/spammable_actions/captcha_check/common.rb @@ -0,0 +1,23 @@ +# 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 diff --git a/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb b/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb new file mode 100644 index 00000000000..70b4c046128 --- /dev/null +++ b/app/controllers/concerns/spammable_actions/captcha_check/html_format_actions_support.rb @@ -0,0 +1,35 @@ +# 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 diff --git a/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb b/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb new file mode 100644 index 00000000000..9fa448a9abb --- /dev/null +++ b/app/controllers/concerns/spammable_actions/captcha_check/json_format_actions_support.rb @@ -0,0 +1,25 @@ +# 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 diff --git a/app/controllers/projects/issues_controller.rb b/app/controllers/projects/issues_controller.rb index 5d38e431c8a..e3b2d452151 100644 --- a/app/controllers/projects/issues_controller.rb +++ b/app/controllers/projects/issues_controller.rb @@ -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 diff --git a/app/controllers/projects/snippets_controller.rb b/app/controllers/projects/snippets_controller.rb index 7d9c416e8f0..97f9c5814e2 100644 --- a/app/controllers/projects/snippets_controller.rb +++ b/app/controllers/projects/snippets_controller.rb @@ -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! diff --git a/app/controllers/snippets_controller.rb b/app/controllers/snippets_controller.rb index 1c6168dbc2c..e81868faa6e 100644 --- a/app/controllers/snippets_controller.rb +++ b/app/controllers/snippets_controller.rb @@ -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] |