From a4f2de796411236bfda81b7fa281cfa8199c5acf Mon Sep 17 00:00:00 2001 From: Brett Walker Date: Fri, 28 Dec 2018 11:53:39 -0700 Subject: [PATCH] Refactoring and review comments including verifying the project_slug --- .../email/handler/create_issue_handler.rb | 21 +++++++------------ .../handler/create_merge_request_handler.rb | 21 +++++++------------ lib/gitlab/email/handler/reply_processing.rb | 21 ++++++++++++++++++- .../email/handler/unsubscribe_handler.rb | 2 +- .../handler/create_issue_handler_spec.rb | 5 +++-- .../create_merge_request_handler_spec.rb | 5 +++-- spec/lib/gitlab/email/handler_spec.rb | 2 +- 7 files changed, 42 insertions(+), 35 deletions(-) diff --git a/lib/gitlab/email/handler/create_issue_handler.rb b/lib/gitlab/email/handler/create_issue_handler.rb index 7e5fa5f5cd2..50928f0d59e 100644 --- a/lib/gitlab/email/handler/create_issue_handler.rb +++ b/lib/gitlab/email/handler/create_issue_handler.rb @@ -11,16 +11,19 @@ module Gitlab class CreateIssueHandler < BaseHandler include ReplyProcessing - HANDLER_REGEX = /\A.+-(?.+)-(?.+)-issue\z/.freeze + HANDLER_REGEX = /\A#{HANDLER_ACTION_BASE_REGEX}-issue\z/.freeze HANDLER_REGEX_LEGACY = /\A(?[^\+]*)\+(?.*)\z/.freeze def initialize(mail, mail_key) super(mail, mail_key) - if matched = HANDLER_REGEX.match(mail_key.to_s) - @project_id, @incoming_email_token = matched.captures + if !mail_key&.include?('/') && (matched = HANDLER_REGEX.match(mail_key.to_s)) + @project_slug = matched[:project_slug] + @project_id = matched[:project_id]&.to_i + @incoming_email_token = matched[:incoming_email_token] elsif matched = HANDLER_REGEX_LEGACY.match(mail_key.to_s) - @project_path, @incoming_email_token = matched.captures + @project_path = matched[:project_path] + @incoming_email_token = matched[:incoming_email_token] end end @@ -45,18 +48,8 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord - def project - @project ||= if project_id - Project.find_by_id(project_id) - else - Project.find_by_full_path(project_path) - end - end - private - attr_reader :project_id, :project_path, :incoming_email_token - def create_issue Issues::CreateService.new( project, diff --git a/lib/gitlab/email/handler/create_merge_request_handler.rb b/lib/gitlab/email/handler/create_merge_request_handler.rb index d065da70931..21bb09fa4a1 100644 --- a/lib/gitlab/email/handler/create_merge_request_handler.rb +++ b/lib/gitlab/email/handler/create_merge_request_handler.rb @@ -12,16 +12,19 @@ module Gitlab class CreateMergeRequestHandler < BaseHandler include ReplyProcessing - HANDLER_REGEX = /\A.+-(?.+)-(?.+)-merge-request\z/.freeze + HANDLER_REGEX = /\A#{HANDLER_ACTION_BASE_REGEX}-merge-request\z/.freeze HANDLER_REGEX_LEGACY = /\A(?[^\+]*)\+merge-request\+(?.*)/.freeze def initialize(mail, mail_key) super(mail, mail_key) - if matched = HANDLER_REGEX.match(mail_key.to_s) - @project_id, @incoming_email_token = matched.captures + if !mail_key&.include?('/') && (matched = HANDLER_REGEX.match(mail_key.to_s)) + @project_slug = matched[:project_slug] + @project_id = matched[:project_id]&.to_i + @incoming_email_token = matched[:incoming_email_token] elsif matched = HANDLER_REGEX_LEGACY.match(mail_key.to_s) - @project_path, @incoming_email_token = matched.captures + @project_path = matched[:project_path] + @incoming_email_token = matched[:incoming_email_token] end end @@ -47,22 +50,12 @@ module Gitlab end # rubocop: enable CodeReuse/ActiveRecord - def project - @project ||= if project_id - Project.find_by_id(project_id) - else - Project.find_by_full_path(project_path) - end - end - def metrics_params super.merge(includes_patches: patch_attachments.any?) end private - attr_reader :project_id, :project_path, :incoming_email_token - def build_merge_request MergeRequests::BuildService.new(project, author, merge_request_params).execute end diff --git a/lib/gitlab/email/handler/reply_processing.rb b/lib/gitlab/email/handler/reply_processing.rb index ff6b2c729b2..76b393f3259 100644 --- a/lib/gitlab/email/handler/reply_processing.rb +++ b/lib/gitlab/email/handler/reply_processing.rb @@ -6,13 +6,28 @@ module Gitlab module ReplyProcessing private + HANDLER_ACTION_BASE_REGEX = /(?.+)-(?\d+)-(?.+)/.freeze + + attr_reader :project_id, :project_slug, :project_path, :incoming_email_token + def author raise NotImplementedError end + # rubocop:disable Gitlab/ModuleWithInstanceVariables def project - raise NotImplementedError + return @project if instance_variable_defined?(:@project) + + if project_id + @project = Project.find_by_id(project_id) + @project = nil unless valid_project_slug?(@project) + else + @project = Project.find_by_full_path(project_path) + end + + @project end + # rubocop:enable Gitlab/ModuleWithInstanceVariables def message @message ||= process_message @@ -58,6 +73,10 @@ module Gitlab raise invalid_exception, msg end + + def valid_project_slug?(found_project) + project_slug == found_project.full_path_slug + end end end end diff --git a/lib/gitlab/email/handler/unsubscribe_handler.rb b/lib/gitlab/email/handler/unsubscribe_handler.rb index 7589658d2fe..20e4c125626 100644 --- a/lib/gitlab/email/handler/unsubscribe_handler.rb +++ b/lib/gitlab/email/handler/unsubscribe_handler.rb @@ -23,7 +23,7 @@ module Gitlab end def can_handle? - @reply_token.present? + reply_token.present? end def execute diff --git a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb index 583e73751d7..48139c2f9dc 100644 --- a/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_issue_handler_spec.rb @@ -27,9 +27,10 @@ describe Gitlab::Email::Handler::CreateIssueHandler do let(:mail) { Mail::Message.new(email_raw) } it "matches the new format" do - handler = described_class.new(mail, "h5bp-html5-boilerplate-#{project.project_id}-#{user.incoming_email_token}-issue") + handler = described_class.new(mail, "gitlabhq-gitlabhq-#{project.project_id}-#{user.incoming_email_token}-issue") - expect(handler.instance_variable_get(:@project_id).to_i).to eq project.project_id + expect(handler.instance_variable_get(:@project_id)).to eq project.project_id + expect(handler.instance_variable_get(:@project_slug)).to eq project.full_path_slug expect(handler.instance_variable_get(:@incoming_email_token)).to eq user.incoming_email_token expect(handler.can_handle?).to be_truthy end diff --git a/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb b/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb index 0cf15adc35c..2fa86b2b46f 100644 --- a/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb +++ b/spec/lib/gitlab/email/handler/create_merge_request_handler_spec.rb @@ -31,9 +31,10 @@ describe Gitlab::Email::Handler::CreateMergeRequestHandler do let(:mail) { Mail::Message.new(email_raw) } it "matches the new format" do - handler = described_class.new(mail, "h5bp-html5-boilerplate-#{project.project_id}-#{user.incoming_email_token}-merge-request") + handler = described_class.new(mail, "gitlabhq-gitlabhq-#{project.project_id}-#{user.incoming_email_token}-merge-request") - expect(handler.instance_variable_get(:@project_id).to_i).to eq project.project_id + expect(handler.instance_variable_get(:@project_id)).to eq project.project_id + expect(handler.instance_variable_get(:@project_slug)).to eq project.full_path_slug expect(handler.instance_variable_get(:@incoming_email_token)).to eq user.incoming_email_token expect(handler.can_handle?).to be_truthy end diff --git a/spec/lib/gitlab/email/handler_spec.rb b/spec/lib/gitlab/email/handler_spec.rb index 97c5f693c53..d2920b08956 100644 --- a/spec/lib/gitlab/email/handler_spec.rb +++ b/spec/lib/gitlab/email/handler_spec.rb @@ -19,7 +19,7 @@ describe Gitlab::Email::Handler do describe 'regexps are set properly' do let(:addresses) do - %W(sent_notification_key#{Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX} sent_notification_key path-to-project-project_id-user_email_token-merge-request path-to-project-user_email_token-issue) + + %W(sent_notification_key#{Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX} sent_notification_key path-to-project-123-user_email_token-merge-request path-to-project-123-user_email_token-issue) + %W(sent_notification_key#{Gitlab::IncomingEmail::UNSUBSCRIBE_SUFFIX_LEGACY} sent_notification_key path/to/project+merge-request+user_email_token path/to/project+user_email_token) end