chore: apply correct scope/no scope to webhook repository queries

lib/pact_broker/contracts/service.rb

@@ -209,7 +209,7 @@ def next_steps_logs(pact)
           logs << LogMessage.warn("    * " + message("messages.next_steps.verifications", provider_name: pact.provider_name))
         end
 
-        if !webhook_service.find_for_pact(pact).any?
+        if !webhook_service.any_webhooks_configured_for_pact?(pact)
           logs << LogMessage.warn("    * " + message("messages.next_steps.webhooks", provider_name: pact.provider_name))
         end
 
@@ -234,7 +234,8 @@ def triggered_webhook_logs(listener, pact)
             LogMessage.debug("  #{text_1}\n    #{text_2}")
           end
         else
-          if webhook_service.find_for_pact(pact).any?
+          if webhook_service.any_webhooks_configured_for_pact?(pact)
+            # There are some webhooks, just not any for this particular event
             [LogMessage.debug("  " + message("messages.webhooks.no_webhooks_enabled_for_event"))]
           else
             []

lib/pact_broker/test/test_data_builder.rb

@@ -286,6 +286,7 @@ def create_webhook parameters = {}
         consumer = params.key?(:consumer) ? params.delete(:consumer) : @consumer
         provider = params.key?(:provider) ? params.delete(:provider) : @provider
         uuid = params[:uuid] || PactBroker::Webhooks::Service.next_uuid
+        enabled = params.key?(:enabled) ? params.delete(:enabled) : true
         event_params = if params[:event_names]
           params[:event_names].collect{ |event_name| {name: event_name} }
         else
@@ -294,7 +295,7 @@ def create_webhook parameters = {}
         events = event_params.collect{ |e| PactBroker::Webhooks::WebhookEvent.new(e) }
         template_params = { method: 'POST', url: 'http://example.org', headers: {'Content-Type' => 'application/json'}, username: params[:username], password: params[:password]}
         request = PactBroker::Webhooks::WebhookRequestTemplate.new(template_params.merge(params))
-        @webhook = PactBroker::Webhooks::Repository.new.create uuid, PactBroker::Domain::Webhook.new(request: request, events: events, description: params[:description]), consumer, provider
+        @webhook = PactBroker::Webhooks::Repository.new.create uuid, PactBroker::Domain::Webhook.new(request: request, events: events, description: params[:description], enabled: enabled), consumer, provider
         self
       end
 

lib/pact_broker/webhooks/repository.rb

@@ -28,12 +28,14 @@ def create uuid, webhook, consumer, provider
         find_by_uuid db_webhook.uuid
       end
 
+      # policy applied at resource level
       def find_by_uuid uuid
-        Webhook.where(uuid: uuid).limit(1).collect(&:to_domain)[0]
+        deliberately_unscoped(Webhook).where(uuid: uuid).limit(1).collect(&:to_domain)[0]
       end
 
+      # policy applied at resource level
       def update_by_uuid uuid, webhook
-        existing_webhook = Webhook.find(uuid: uuid)
+        existing_webhook = deliberately_unscoped(Webhook).find(uuid: uuid)
         existing_webhook.consumer_id = webhook.consumer ? pacticipant_repository.find_by_name(webhook.consumer.name).id : nil
         existing_webhook.provider_id = webhook.provider ? pacticipant_repository.find_by_name(webhook.provider.name).id : nil
         existing_webhook.update_from_domain(webhook).save
@@ -44,65 +46,46 @@ def update_by_uuid uuid, webhook
         find_by_uuid uuid
       end
 
+      # policy applied at resource level
       def delete_by_uuid uuid
-        Webhook.where(uuid: uuid).destroy
+        deliberately_unscoped(Webhook).where(uuid: uuid).destroy
       end
 
+      # policy applied at resource level for pacticipant
       def delete_by_pacticipant pacticipant
-        Webhook.where(consumer_id: pacticipant.id).destroy
-        Webhook.where(provider_id: pacticipant.id).destroy
+        deliberately_unscoped(Webhook).where(consumer_id: pacticipant.id).destroy
+        deliberately_unscoped(Webhook).where(provider_id: pacticipant.id).destroy
       end
 
+      # this needs the scope!
       def find_all
         scope_for(Webhook).all.collect(&:to_domain)
       end
 
-      def find_for_pact pact
-        find_by_consumer_and_or_provider(pact.consumer, pact.provider)
-      end
-
-      def find_by_consumer_and_or_provider consumer, provider
-        find_by_consumer_and_provider(consumer, provider) +
-          find_by_consumer_and_provider(nil, provider) +
-          find_by_consumer_and_provider(consumer, nil) +
-          find_by_consumer_and_provider(nil, nil)
+      # needs to know if there are any at all, regardless of whether or not user can edit them
+      def any_webhooks_configured_for_pact?(pact)
+        deliberately_unscoped(Webhook).find_by_consumer_and_or_provider(pact.consumer, pact.provider).any?
       end
 
       def find_by_consumer_and_provider consumer, provider
-        criteria = {
-          consumer_id: (consumer ? consumer.id : nil),
-          provider_id: (provider ? provider.id : nil)
-        }
-        scope_for(Webhook).where(criteria).collect(&:to_domain)
+        scope_for(Webhook).find_by_consumer_and_provider(consumer, provider).collect(&:to_domain)
       end
 
       def delete_by_consumer_and_provider consumer, provider
         scope_for(Webhook).where(consumer: consumer, provider: provider).destroy
       end
 
-      def find_by_consumer_and_or_provider_and_event_name consumer, provider, event_name
-        find_by_consumer_and_provider_and_event_name(consumer, provider, event_name) +
-          find_by_consumer_and_provider_and_event_name(nil, provider, event_name) +
-          find_by_consumer_and_provider_and_event_name(consumer, nil, event_name) +
-          find_by_consumer_and_provider_and_event_name(nil, nil, event_name)
-      end
-
-      def find_by_consumer_and_provider_and_event_name consumer, provider, event_name
-        criteria = {
-          consumer_id: (consumer ? consumer.id : nil),
-          provider_id: (provider ? provider.id : nil)
-        }
-        scope_for(Webhook)
+      def find_webhooks_to_trigger consumer: , provider: , event_name:
+        deliberately_unscoped(Webhook)
           .select_all_qualified
           .enabled
-          .where(criteria)
-          .join(:webhook_events, { webhook_id: :id })
-          .where(Sequel[:webhook_events][:name] => event_name)
+          .for_event_name(event_name)
+          .find_by_consumer_and_or_provider(consumer, provider)
           .collect(&:to_domain)
       end
 
       def create_triggered_webhook trigger_uuid, webhook, pact, verification, trigger_type, event_name, event_context
-        db_webhook = Webhook.where(uuid: webhook.uuid).single_record
+        db_webhook = deliberately_unscoped(Webhook).where(uuid: webhook.uuid).single_record
         TriggeredWebhook.create(
           status: TriggeredWebhook::STATUS_NOT_RUN,
           pact_publication_id: pact.id,
@@ -124,7 +107,7 @@ def update_triggered_webhook_status triggered_webhook, status
 
       def create_execution triggered_webhook, webhook_execution_result
         # TriggeredWebhook may have been deleted since the webhook execution started
-        if TriggeredWebhook.where(id: triggered_webhook.id).any?
+        if deliberately_unscoped(TriggeredWebhook).where(id: triggered_webhook.id).any?
           Execution.create(
             triggered_webhook: triggered_webhook,
             success: webhook_execution_result.success?,
@@ -134,9 +117,10 @@ def create_execution triggered_webhook, webhook_execution_result
         end
       end
 
+      # policy applied at resource level
       def delete_triggered_webhooks_by_pacticipant pacticipant
-        TriggeredWebhook.where(consumer: pacticipant).delete
-        TriggeredWebhook.where(provider: pacticipant).delete
+        deliberately_unscoped(TriggeredWebhook).where(consumer: pacticipant).delete
+        deliberately_unscoped(TriggeredWebhook).where(provider: pacticipant).delete
       end
 
       def delete_executions_by_pacticipant pacticipants
@@ -177,7 +161,8 @@ def find_latest_triggered_webhooks_for_pact pact
       end
 
       def find_latest_triggered_webhooks consumer, provider
-        scope_for(LatestTriggeredWebhook)
+        # policy already applied to pact
+        deliberately_unscoped(LatestTriggeredWebhook)
           .where(consumer: consumer, provider: provider)
           .order(:id)
           .all
@@ -200,17 +185,21 @@ def find_triggered_webhooks_for_verification verification
       end
 
       def fail_retrying_triggered_webhooks
-        TriggeredWebhook.retrying.update(status: TriggeredWebhook::STATUS_FAILURE)
-      end
-
-      def any_webhooks_exist?
-        scope_for(Webhook).any?
+        deliberately_unscoped(TriggeredWebhook).retrying.update(status: TriggeredWebhook::STATUS_FAILURE)
       end
 
       private
 
+      def deliberately_unscoped(scope)
+        scope
+      end
+
       def scope_for(scope)
-        PactBroker.policy_scope!(scope)
+        if @no_policy
+          scope
+        else
+          PactBroker.policy_scope!(scope)
+        end
       end
 
       def delete_triggered_webhooks_and_executions triggered_webhook_ids

lib/pact_broker/webhooks/service.rb

@@ -1,3 +1,4 @@
+require 'delegate'
 require 'pact_broker/repositories'
 require 'pact_broker/services'
 require 'pact_broker/logging'
@@ -18,24 +19,37 @@
 
 module PactBroker
   module Webhooks
-    class Service
+    module Service
       using PactBroker::HashRefinements
-
+      extend self
+      extend Forwardable
       extend Repositories
       extend Services
       include Logging
       extend PactBroker::Messages
 
+      delegate [
+        :find_by_uuid,
+        :find_all,
+        :update_triggered_webhook_status,
+        :any_webhooks_configured_for_pact?,
+        :find_by_consumer_and_provider,
+        :find_latest_triggered_webhooks_for_pact,
+        :fail_retrying_triggered_webhooks,
+        :find_triggered_webhooks_for_pact,
+        :find_triggered_webhooks_for_verification
+      ] => :webhook_repository
+
       # Not actually a UUID. Ah well.
-      def self.valid_uuid_format?(uuid)
+      def valid_uuid_format?(uuid)
         !!(uuid =~ /^[A-Za-z0-9_\-]{16,}$/)
       end
 
-      def self.next_uuid
+      def next_uuid
         SecureRandom.urlsafe_base64
       end
 
-      def self.errors webhook, uuid = nil
+      def errors webhook, uuid = nil
         contract = PactBroker::Api::Contracts::WebhookContract.new(webhook)
         contract.validate(webhook.attributes)
         messages = contract.errors.messages
@@ -47,81 +61,37 @@ def self.errors webhook, uuid = nil
         OpenStruct.new(messages: messages, empty?: messages.empty?, any?: messages.any?)
       end
 
-      def self.create uuid, webhook, consumer, provider
+      def create uuid, webhook, consumer, provider
         webhook_repository.create uuid, webhook, consumer, provider
       end
 
-      def self.find_by_uuid uuid
-        webhook_repository.find_by_uuid uuid
-      end
-
-      def self.update_by_uuid uuid, params
+      def update_by_uuid uuid, params
         webhook = webhook_repository.find_by_uuid(uuid)
         maintain_redacted_params(webhook, params)
         PactBroker::Api::Decorators::WebhookDecorator.new(webhook).from_hash(params)
         webhook_repository.update_by_uuid uuid, webhook
       end
 
-      def self.delete_by_uuid uuid
+      def delete_by_uuid uuid
         webhook_repository.delete_triggered_webhooks_by_webhook_uuid uuid
         webhook_repository.delete_by_uuid uuid
       end
 
-      def self.delete_all_webhhook_related_objects_by_pacticipant pacticipant
+      def delete_all_webhhook_related_objects_by_pacticipant pacticipant
         webhook_repository.delete_executions_by_pacticipant pacticipant
         webhook_repository.delete_triggered_webhooks_by_pacticipant pacticipant
         webhook_repository.delete_by_pacticipant pacticipant
       end
 
-      def self.delete_all_webhook_related_objects_by_pact_publication_ids pact_publication_ids
+      def delete_all_webhook_related_objects_by_pact_publication_ids pact_publication_ids
         webhook_repository.delete_triggered_webhooks_by_pact_publication_ids pact_publication_ids
       end
 
-      def self.delete_all_webhook_related_objects_by_verification_ids verification_ids
+      def delete_all_webhook_related_objects_by_verification_ids verification_ids
         webhook_repository.delete_triggered_webhooks_by_verification_ids verification_ids
       end
 
-      def self.find_all
-        webhook_repository.find_all
-      end
-
-      def self.update_triggered_webhook_status triggered_webhook, status
-        webhook_repository.update_triggered_webhook_status triggered_webhook, status
-      end
-
-      def self.find_for_pact pact
-        webhook_repository.find_for_pact(pact)
-      end
-
-      def self.find_by_consumer_and_or_provider consumer, provider
-        webhook_repository.find_by_consumer_and_or_provider(consumer, provider)
-      end
-
-      def self.find_by_consumer_and_provider consumer, provider
-        webhook_repository.find_by_consumer_and_provider consumer, provider
-      end
-
-      def self.find_latest_triggered_webhooks_for_pact pact
-        webhook_repository.find_latest_triggered_webhooks_for_pact pact
-      end
-
-      def self.find_latest_triggered_webhooks consumer, provider
-        webhook_repository.find_latest_triggered_webhooks consumer, provider
-      end
-
-      def self.fail_retrying_triggered_webhooks
-        webhook_repository.fail_retrying_triggered_webhooks
-      end
-
-      def self.find_triggered_webhooks_for_pact pact
-        webhook_repository.find_triggered_webhooks_for_pact(pact)
-      end
-
-      def self.find_triggered_webhooks_for_verification verification
-        webhook_repository.find_triggered_webhooks_for_verification(verification)
-      end
-
-      def self.parameters
+      def parameters
         PactAndVerificationParameters::ALL.collect do | parameter |
           OpenStruct.new(
             name: parameter,
@@ -136,7 +106,7 @@ def self.parameters
       # This is required because the password and Authorization header is **** out in the API response
       # for security purposes, so it would need to be re-entered with every response.
       # TODO implement proper 'secrets' management.
-      def self.maintain_redacted_params(webhook, params)
+      def maintain_redacted_params(webhook, params)
         if webhook.request.password && password_key_does_not_exist_or_is_starred?(params)
           params['request']['password'] = webhook.request.password
         end
@@ -151,7 +121,7 @@ def self.maintain_redacted_params(webhook, params)
         params
       end
 
-      def self.password_key_does_not_exist_or_is_starred?(params)
+      def password_key_does_not_exist_or_is_starred?(params)
         !params['request'].key?('password') || params.dig('request','password') =~ /^\**$/
       end
     end

lib/pact_broker/webhooks/trigger_service.rb

@@ -38,7 +38,7 @@ def execute_triggered_webhook_now triggered_webhook, webhook_execution_configura
 
       # the main entry point
       def create_triggered_webhooks_for_event pact, verification, event_name, event_context
-        webhooks = webhook_repository.find_by_consumer_and_or_provider_and_event_name pact.consumer, pact.provider, event_name
+        webhooks = webhook_repository.find_webhooks_to_trigger(consumer: pact.consumer, provider: pact.provider, event_name: event_name)
 
         if webhooks.any?
           create_triggered_webhooks_for_webhooks(webhooks, pact, verification, event_name, event_context.merge(event_name: event_name))

lib/pact_broker/webhooks/webhook.rb

@@ -17,6 +17,30 @@ class Webhook < Sequel::Model
       dataset_module do
         include PactBroker::Repositories::Helpers
 
+        def for_event_name(event_name)
+          join(:webhook_events, { webhook_id: :id })
+            .where(Sequel[:webhook_events][:name] => event_name)
+        end
+
+        def find_by_consumer_and_or_provider consumer, provider
+          where(
+            Sequel.|(
+              { consumer_id: consumer.id, provider_id: provider.id },
+              { consumer_id: nil, provider_id: provider.id },
+              { consumer_id: consumer.id, provider_id: nil },
+              { consumer_id: nil, provider_id: nil}
+            )
+          )
+        end
+
+        def find_by_consumer_and_provider consumer, provider
+          criteria = {
+            consumer_id: (consumer ? consumer.id : nil),
+            provider_id: (provider ? provider.id : nil)
+          }
+          where(criteria)
+        end
+
         def enabled
           where(enabled: true)
         end