Merge pull request #11570 from dmarcoux/scm_webhook_model+validators

Create ScmWebhook model and reorganize validations for webhook events

src/api/app/models/scm_webhook.rb

@@ -0,0 +1,34 @@
+# Contains the payload extracted from a SCM webhook and provides helper methods to know which webhook event we're dealing with
+class ScmWebhook
+  include ActiveModel::Model
+
+  attr_accessor :payload
+
+  validates_with ScmWebhookEventValidator
+
+  def initialize(attributes = {})
+    super
+    # To safely navigate the hash and compare keys
+    @payload = attributes[:payload].deep_symbolize_keys
+  end
+
+  def new_pull_request?
+    (github_pull_request? && @payload[:action] == 'opened') ||
+      (gitlab_merge_request? && @payload[:action] == 'open')
+  end
+
+  def updated_pull_request?
+    (github_pull_request? && @payload[:action] == 'synchronize') ||
+      (gitlab_merge_request? && @payload[:action] == 'update')
+  end
+
+  private
+
+  def github_pull_request?
+    @payload[:scm] == 'github' && @payload[:event] == 'pull_request'
+  end
+
+  def gitlab_merge_request?
+    @payload[:scm] == 'gitlab' && @payload[:event] == 'Merge Request Hook'
+  end
+end

src/api/app/models/token/workflow.rb

@@ -8,12 +8,11 @@ def self.token_name
   def call(options)
     raise ArgumentError, 'A payload is required' if options[:payload].nil?
 
-    extractor = TriggerControllerService::ScmExtractor.new(options[:scm], options[:event], options[:payload])
-    return unless extractor.allowed_event_and_action?
+    scm_webhook = TriggerControllerService::ScmExtractor.new(options[:scm], options[:event], options[:payload]).call
+    return unless scm_webhook.valid?
 
-    scm_extractor_payload = extractor.call
-    yaml_file = Workflows::YAMLDownloader.new(scm_extractor_payload, token: self).call
-    workflows = Workflows::YAMLToWorkflowsService.new(yaml_file: yaml_file, scm_extractor_payload: scm_extractor_payload, token: self).call
+    yaml_file = Workflows::YAMLDownloader.new(scm_webhook.payload, token: self).call
+    workflows = Workflows::YAMLToWorkflowsService.new(yaml_file: yaml_file, scm_webhook: scm_webhook, token: self).call
     workflows.each do |workflow|
       workflow.steps.each do |step|
         step.call({ workflow_filters: workflow.filters })

src/api/app/models/workflow.rb

@@ -11,17 +11,15 @@ class Workflow
   # The order of the filter types determines their precedence
   SUPPORTED_FILTER_TYPES = [:only, :ignore].freeze
 
-  attr_accessor :workflow_instructions, :scm_extractor_payload, :token
+  attr_accessor :workflow_instructions, :scm_webhook, :token
 
   def initialize(attributes = {})
     super
     @workflow_instructions = attributes[:workflow_instructions].deep_symbolize_keys
-    @scm_extractor_payload = attributes[:scm_extractor_payload].deep_symbolize_keys
   end
 
   validates_with WorkflowStepsValidator
   validates_with WorkflowFiltersValidator
-  validates_with WorkflowEventAndActionValidator
 
   def steps
     return {} if workflow_steps.blank?
@@ -63,7 +61,7 @@ def workflow_steps
 
   def initialize_step(step_name, step_instructions)
     SUPPORTED_STEPS[step_name].new(step_instructions: step_instructions,
-                                   scm_extractor_payload: scm_extractor_payload,
+                                   scm_webhook: scm_webhook,
                                    token: token)
   end
 

src/api/app/models/workflow/step.rb

@@ -4,12 +4,11 @@ class Workflow::Step
 
   validates :source_project_name, presence: true
 
-  attr_reader :scm_extractor_payload, :step_instructions, :token
+  attr_accessor :scm_webhook, :step_instructions, :token
 
   def initialize(attributes = {})
+    super
     @step_instructions = attributes[:step_instructions]&.deep_symbolize_keys || {}
-    @scm_extractor_payload = attributes[:scm_extractor_payload]&.deep_symbolize_keys || {}
-    @token = attributes[:token]
   end
 
   def call(_options)
@@ -23,7 +22,7 @@ def source_project_name
   end
 
   def target_project_name
-    "home:#{@token.user.login}:#{source_project_name}:PR-#{scm_extractor_payload[:pr_number]}"
+    "home:#{@token.user.login}:#{source_project_name}:PR-#{scm_webhook.payload[:pr_number]}"
   end
 
   def source_package_name
@@ -42,16 +41,12 @@ def target_package
     Package.find_by_project_and_name(target_project_name, target_package_name)
   end
 
-  def validator
-    WorkflowEventAndActionValidator.new(scm_extractor_payload: scm_extractor_payload)
-  end
-
   def remote_source?
     Project.find_remote_project(source_project_name).present?
   end
 
   def add_or_update_branch_request_file(package:)
-    branch_request_file = case scm_extractor_payload[:scm]
+    branch_request_file = case scm_webhook.payload[:scm]
                           when 'github'
                             branch_request_content_github
                           when 'gitlab'
@@ -63,23 +58,23 @@ def add_or_update_branch_request_file(package:)
 
   def branch_request_content_github
     {
-      # TODO: change to @scm_extractor_payload[:action]
+      # TODO: change to scm_webhook.payload[:action]
       # when check_for_branch_request method in obs-service-tar_scm accepts other actions than 'opened'
       # https://github.com/openSUSE/obs-service-tar_scm/blob/2319f50e741e058ad599a6890ac5c710112d5e48/TarSCM/tasks.py#L145
       action: 'opened',
       pull_request: {
         head: {
-          repo: { full_name: scm_extractor_payload[:source_repository_full_name] },
-          sha: scm_extractor_payload[:commit_sha]
+          repo: { full_name: scm_webhook.payload[:source_repository_full_name] },
+          sha: scm_webhook.payload[:commit_sha]
         }
       }
     }.to_json
   end
 
   def branch_request_content_gitlab
-    { object_kind: scm_extractor_payload[:object_kind],
-      project: { http_url: scm_extractor_payload[:http_url] },
-      object_attributes: { source: { default_branch: scm_extractor_payload[:commit_sha] } } }.to_json
+    { object_kind: scm_webhook.payload[:object_kind],
+      project: { http_url: scm_webhook.payload[:http_url] },
+      object_attributes: { source: { default_branch: scm_webhook.payload[:commit_sha] } } }.to_json
   end
 
   # FIXME: remove this and use create_subscriptions and update_subscriptions as soon as BranchPackageStep is refactored
@@ -92,7 +87,7 @@ def create_or_update_subscriptions(package, workflow_filters)
                                                           enabled: true,
                                                           token: @token,
                                                           package: package)
-      subscription.update!(payload: scm_extractor_payload.merge({ workflow_filters: workflow_filters }))
+      subscription.update!(payload: scm_webhook.payload.merge({ workflow_filters: workflow_filters }))
     end
   end
 
@@ -105,7 +100,7 @@ def create_subscriptions(package, workflow_filters)
                                 enabled: true,
                                 token: @token,
                                 package: package,
-                                payload: scm_extractor_payload.merge({ workflow_filters: workflow_filters }))
+                                payload: scm_webhook.payload.merge({ workflow_filters: workflow_filters }))
     end
   end
 
@@ -115,7 +110,7 @@ def update_subscriptions(package, workflow_filters)
                                                channel: 'scm',
                                                token: @token,
                                                package: package)
-      subscription.update!(payload: scm_extractor_payload.merge({ workflow_filters: workflow_filters }))
+      subscription.update!(payload: scm_webhook.payload.merge({ workflow_filters: workflow_filters }))
     end
   end
 

src/api/app/models/workflow/step/branch_package_step.rb

@@ -16,7 +16,7 @@ def call(options = {})
       workflow_architectures(repository, workflow_filters).each do |architecture|
         # We cannot report multibuild flavors here... so they will be missing from the initial report
         SCMStatusReporter.new({ project: target_project_name, package: target_package_name, repository: repository.name, arch: architecture.name },
-                              scm_extractor_payload, @token.scm_token).call
+                              scm_webhook.payload, @token.scm_token).call
       end
     end
 
@@ -26,7 +26,7 @@ def call(options = {})
   private
 
   def find_or_create_branched_package
-    return target_package if validator.updated_pull_request? && target_package.present?
+    return target_package if scm_webhook.updated_pull_request? && target_package.present?
 
     branch
   end

src/api/app/models/workflow/step/link_package_step.rb

@@ -7,7 +7,7 @@ def call(options = {})
     workflow_filters = options.fetch(:workflow_filters, {})
 
     # Updated PR
-    if validator.updated_pull_request? && target_package.present?
+    if scm_webhook.updated_pull_request? && target_package.present?
       update_subscriptions(target_package, workflow_filters)
     else # New PR
       create_target_package
@@ -89,7 +89,7 @@ def report_to_scm(workflow_filters)
       workflow_architectures(repository, workflow_filters).each do |architecture|
         # We cannot report multibuild flavors here... so they will be missing from the initial report
         SCMStatusReporter.new({ project: target_project_name, package: target_package_name, repository: repository.name, arch: architecture.name },
-                              scm_extractor_payload, @token.scm_token).call
+                              scm_webhook.payload, @token.scm_token).call
       end
     end
   end

src/api/app/services/trigger_controller_service/scm_extractor.rb

@@ -1,49 +1,26 @@
 module TriggerControllerService
   # NOTE: this class is coupled to GitHub pull requests events and GitLab merge requests events.
   class ScmExtractor
-    ALLOWED_GITHUB_ACTIONS = ['opened', 'synchronize'].freeze
-    ALLOWED_GITLAB_ACTIONS = ['open', 'update'].freeze
-
     def initialize(scm, event, payload)
       # TODO: What should we do when the user sends a wwwurlencoded payload? Raise an exception?
       @payload = payload.deep_symbolize_keys
       @scm = scm
       @event = event
     end
 
-    # TODO: this check seems redundant, but it prevents extracting the payload if the event and action
-    # are not the allowed ones. See BranchPackageStep.
-    def allowed_event_and_action?
-      allowed_github_event_and_action? || allowed_gitlab_event_and_action?
-    end
-
     # TODO: What happens when some of the keys are missing?
     def call
       case @scm
       when 'github'
-        github_extractor_payload
+        ScmWebhook.new(payload: github_extractor_payload)
       when 'gitlab'
-        gitlab_extractor_payload
+        ScmWebhook.new(payload: gitlab_extractor_payload)
       end
     end
 
     private
 
-    def allowed_github_event_and_action?
-      @event == 'pull_request' && @payload[:action].in?(ALLOWED_GITHUB_ACTIONS)
-    end
-
-    def allowed_gitlab_event_and_action?
-      @event == 'Merge Request Hook' && @payload[:object_attributes][:action].in?(ALLOWED_GITLAB_ACTIONS)
-    end
-
     def github_extractor_payload
-      host = URI.parse(@payload.dig(:sender, :url)).host
-      api_endpoint = if host.start_with?('api.github.com')
-                       "https://#{host}"
-                     else
-                       "https://#{host}/api/v3/"
-                     end
       {
         scm: 'github',
         commit_sha: @payload.dig(:pull_request, :head, :sha),
@@ -54,13 +31,12 @@ def github_extractor_payload
         source_repository_full_name: @payload.dig(:pull_request, :head, :repo, :full_name),
         target_repository_full_name: @payload.dig(:pull_request, :base, :repo, :full_name),
         event: @event,
-        api_endpoint: api_endpoint
+        api_endpoint: github_api_endpoint
       }
     end
 
     def gitlab_extractor_payload
       http_url = @payload.dig(:project, :http_url)
-      uri = URI.parse(http_url)
       {
         scm: 'gitlab',
         object_kind: @payload[:object_kind],
@@ -73,8 +49,27 @@ def gitlab_extractor_payload
         project_id: @payload.dig(:object_attributes, :source_project_id),
         path_with_namespace: @payload.dig(:project, :path_with_namespace),
         event: @event,
-        api_endpoint: "#{uri.scheme}://#{uri.host}"
+        api_endpoint: gitlab_api_endpoint(http_url)
       }
     end
+
+    def github_api_endpoint
+      sender_url = @payload.dig(:sender, :url)
+      return unless sender_url
+
+      host = URI.parse(sender_url).host
+      if host.start_with?('api.github.com')
+        "https://#{host}"
+      else
+        "https://#{host}/api/v3/"
+      end
+    end
+
+    def gitlab_api_endpoint(http_url)
+      return unless http_url
+
+      uri = URI.parse(http_url)
+      "#{uri.scheme}://#{uri.host}"
+    end
   end
 end

src/api/app/services/workflows/yaml_to_workflows_service.rb

@@ -1,8 +1,8 @@
 module Workflows
   class YAMLToWorkflowsService
-    def initialize(yaml_file:, scm_extractor_payload:, token:)
+    def initialize(yaml_file:, scm_webhook:, token:)
       @yaml_file = yaml_file
-      @scm_extractor_payload = scm_extractor_payload
+      @scm_webhook = scm_webhook
       @token = token
     end
 
@@ -20,7 +20,7 @@ def create_workflows
       end
 
       parsed_workflows_yaml
-        .map { |_workflow_name, workflow_instructions| Workflow.new(workflow_instructions: workflow_instructions, scm_extractor_payload: @scm_extractor_payload, token: @token) }
+        .map { |_workflow_name, workflow_instructions| Workflow.new(workflow_instructions: workflow_instructions, scm_webhook: @scm_webhook, token: @token) }
         .select(&:valid?)
     end
   end

src/api/app/validators/scm_webhook_event_validator.rb

@@ -0,0 +1,46 @@
+class ScmWebhookEventValidator < ActiveModel::Validator
+  ALLOWED_GITHUB_EVENTS = ['pull_request'].freeze
+  ALLOWED_GITLAB_EVENTS = ['Merge Request Hook'].freeze
+
+  ALLOWED_PULL_REQUEST_ACTIONS = ['opened', 'synchronize'].freeze
+  ALLOWED_MERGE_REQUEST_ACTIONS = ['open', 'update'].freeze
+
+  def validate(record)
+    @record = record
+
+    return if valid_github_event? || valid_gitlab_event?
+
+    # FIXME: This error message is wrong when the SCM isn't supported. This is an edge case, somebody most probably fiddled with the payload.
+    @record.errors.add(:base, 'Event not supported.')
+  end
+
+  private
+
+  def valid_github_event?
+    return false unless @record.payload[:scm] == 'github'
+    return false unless ALLOWED_GITHUB_EVENTS.include?(@record.payload[:event])
+
+    case @record.payload[:event]
+    when 'pull_request'
+      return true if ALLOWED_PULL_REQUEST_ACTIONS.include?(@record.payload[:action])
+
+      @record.errors.add(:base, 'Pull request action not supported.')
+    else
+      true
+    end
+  end
+
+  def valid_gitlab_event?
+    return false unless @record.payload[:scm] == 'gitlab'
+    return false unless ALLOWED_GITLAB_EVENTS.include?(@record.payload[:event])
+
+    case @record.payload[:event]
+    when 'Merge Request Hook'
+      return true if ALLOWED_MERGE_REQUEST_ACTIONS.include?(@record.payload[:action])
+
+      @record.errors.add(:base, 'Merge request action not supported.')
+    else
+      true
+    end
+  end
+end