feat(webhooks): do not require basic auth password to be re-submitted when a webhook is updated - maintain existing value if not present

Author: bethesque

lib/pact_broker/api/resources/webhook.rb

@@ -33,7 +33,7 @@ def malformed_request?
 
         def from_json
           if webhook
-            @webhook = webhook_service.update_by_uuid uuid, new_webhook
+            @webhook = webhook_service.update_by_uuid uuid, params_with_string_keys
             response.body = to_json
           else
             404

lib/pact_broker/test/test_data_builder.rb

@@ -228,8 +228,8 @@ def create_webhook parameters = {}
           params[:events] || [{ name: PactBroker::Webhooks::WebhookEvent::DEFAULT_EVENT_NAME }]
         end
         events = event_params.collect{ |e| PactBroker::Webhooks::WebhookEvent.new(e) }
-        default_params = { method: 'POST', url: 'http://example.org', headers: {'Content-Type' => 'application/json'}, username: params[:username], password: params[:password]}
-        request = PactBroker::Webhooks::WebhookRequestTemplate.new(default_params.merge(params))
+        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), consumer, provider
         self
       end

lib/pact_broker/webhooks/service.rb

@@ -9,6 +9,7 @@
 require 'pact_broker/webhooks/webhook_event'
 require 'pact_broker/verifications/placeholder_verification'
 require 'pact_broker/pacts/placeholder_pact'
+require 'pact_broker/api/decorators/webhook_decorator'
 
 module PactBroker
 
@@ -40,7 +41,16 @@ def self.find_by_uuid uuid
         webhook_repository.find_by_uuid uuid
       end
 
-      def self.update_by_uuid uuid, webhook
+      def self.update_by_uuid uuid, params
+        webhook = webhook_repository.find_by_uuid(uuid)
+        # Dirty hack to maintain existing password if it is not submitted
+        # This is required because the password is not returned in the API response
+        # for security purposes, so it needs to be re-entered with every response.
+        # TODO implement proper 'secrets' management.
+        if webhook.request.password && !params['request'].key?('password')
+          params['request']['password'] = webhook.request.password
+        end
+        PactBroker::Api::Decorators::WebhookDecorator.new(webhook).from_hash(params)
         webhook_repository.update_by_uuid uuid, webhook
       end
 

spec/lib/pact_broker/webhooks/service_spec.rb

@@ -33,6 +33,56 @@ module Webhooks
         end
       end
 
+      describe ".update_by_uuid" do
+        before do
+          allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:find_by_uuid).and_return(existing_webhook)
+        end
+
+        let(:request) { PactBroker::Webhooks::WebhookRequestTemplate.new(password: 'password')}
+        let(:existing_webhook) { PactBroker::Domain::Webhook.new(request: request) }
+        let(:params) do
+          {
+            'request' => {
+              'url' => "http://url"
+            }
+          }
+        end
+
+        subject { Service.update_by_uuid("1234", params) }
+
+        context "when the webhook has a password and the incoming parameters do not contain a password" do
+          it "does not overwite the password" do
+            updated_webhook = nil
+            allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:update_by_uuid) do | instance, uuid, webhook |
+              updated_webhook = webhook
+              true
+            end
+            subject
+            expect(updated_webhook.request.password).to eq 'password'
+          end
+        end
+
+        context "the incoming parameters contain a password" do
+          let(:params) do
+            {
+              'request' => {
+                'password' => "updated"
+              }
+            }
+          end
+
+          it "updates the password" do
+            updated_webhook = nil
+            allow_any_instance_of(PactBroker::Webhooks::Repository).to receive(:update_by_uuid) do | instance, uuid, webhook |
+              updated_webhook = webhook
+              true
+            end
+            subject
+            expect(updated_webhook.request.password).to eq 'updated'
+          end
+        end
+      end
+
       describe ".trigger_webhooks" do
 
         let(:verification) { instance_double(PactBroker::Domain::Verification)}