Skip to content

Commit

Permalink
feat(webhooks): maintain starred out Authorization header value
Browse files Browse the repository at this point in the history
  • Loading branch information
bethesque committed May 28, 2019
1 parent d865a42 commit cc97858
Show file tree
Hide file tree
Showing 4 changed files with 93 additions and 9 deletions.
1 change: 1 addition & 0 deletions lib/pact_broker/domain/webhook_request.rb
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
require 'pact_broker/build_http_options'
require 'cgi'
require 'delegate'
require 'rack/utils'

module PactBroker

Expand Down
33 changes: 26 additions & 7 deletions lib/pact_broker/webhooks/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -43,13 +43,7 @@ def self.find_by_uuid uuid

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
maintain_redacted_params(webhook, params)
PactBroker::Api::Decorators::WebhookDecorator.new(webhook).from_hash(params)
webhook_repository.update_by_uuid uuid, webhook
end
Expand Down Expand Up @@ -171,6 +165,31 @@ def self.find_triggered_webhooks_for_pact pact
def self.find_triggered_webhooks_for_verification verification
webhook_repository.find_triggered_webhooks_for_verification(verification)
end

private

# Dirty hack to maintain existing password or Authorization header if it is submitted with value ****
# 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)
if webhook.request.password && password_key_does_not_exist_or_is_starred?(params)
params['request']['password'] = webhook.request.password
end

new_headers = params['request']['headers'] ||= {}
existing_headers = webhook.request.headers
starred_new_headers = new_headers.select { |key, value| value =~ /^\**$/ }
starred_new_headers.each do | (key, value) |
new_headers[key] = existing_headers[key]
end
params['request']['headers'] = new_headers
params
end

def self.password_key_does_not_exist_or_is_starred?(params)
!params['request'].key?('password') || params.dig('request','password') =~ /^\**$/
end
end
end
end
6 changes: 5 additions & 1 deletion lib/pact_broker/webhooks/webhook_request_template.rb
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ def initialize attributes = {}
@url = attributes[:url]
@username = attributes[:username]
@password = attributes[:password]
@headers = attributes[:headers] || {}
@headers = Rack::Utils::HeaderHash.new(attributes[:headers] || {})
@body = attributes[:body]
@uuid = attributes[:uuid]
end
Expand Down Expand Up @@ -64,6 +64,10 @@ def redacted_headers
end
end

def headers= headers
@headers = Rack::Utils::HeaderHash.new(headers)
end

private

def to_s
Expand Down
62 changes: 61 additions & 1 deletion spec/lib/pact_broker/webhooks/service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,9 @@ module Webhooks
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(:request) { PactBroker::Webhooks::WebhookRequestTemplate.new(password: existing_password, headers: headers)}
let(:existing_password) { nil }
let(:headers) { {} }
let(:existing_webhook) { PactBroker::Domain::Webhook.new(request: request) }
let(:params) do
{
Expand All @@ -50,7 +52,19 @@ module Webhooks

subject { Service.update_by_uuid("1234", params) }

it "sends through the params to the repository" 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.url).to eq 'http://url'
end

context "when the webhook has a password and the incoming parameters do not contain a password" do
let(:existing_password) { 'password' }

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 |
Expand All @@ -62,6 +76,52 @@ module Webhooks
end
end

context "when the webhook has a password and the incoming parameters contain a *** password" do
let(:existing_password) { 'password' }
let(:params) do
{
'request' => {
'url' => 'http://url',
'password' => '*******'
}
}
end

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 "when the webhook has an authorization header and the incoming parameters contain a *** authorization header" do
let(:headers) { { 'Authorization' => 'existing'} }
let(:params) do
{
'request' => {
'url' => "http://url",
'headers' => {
'authorization' => "***********"
}
}
}
end

it "does not overwite the authorization header" 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.headers['Authorization']).to eq 'existing'
end
end

context "the incoming parameters contain a password" do
let(:params) do
{
Expand Down

0 comments on commit cc97858

Please sign in to comment.