From f8aeaa457da7dee2fd7761580dc46cbc845ef956 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Jun 2025 06:42:49 +0000 Subject: [PATCH 01/12] Initial plan for issue From 1e7fe225f8b5425a505b8763e1eb7f53f6aeb086 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Jun 2025 06:46:29 +0000 Subject: [PATCH 02/12] Initial analysis and plan for custom auth plugins support Co-authored-by: GrantBirki <23362539+GrantBirki@users.noreply.github.com> --- .bundle/config | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.bundle/config b/.bundle/config index 7095f6e9..f9263841 100644 --- a/.bundle/config +++ b/.bundle/config @@ -1,6 +1,6 @@ --- BUNDLE_BIN: "bin" -BUNDLE_PATH: "vendor/gems" +BUNDLE_PATH: "/home/runner/work/hooks/hooks/vendor/bundle" BUNDLE_CACHE_PATH: "vendor/cache" BUNDLE_CACHE_ALL: "true" BUNDLE_SPECIFIC_PLATFORM: "true" From 66a90a7ebe9baf44991191019372363a211b78cc Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Wed, 11 Jun 2025 07:03:07 +0000 Subject: [PATCH 03/12] feat: implement custom auth plugins support Co-authored-by: GrantBirki <23362539+GrantBirki@users.noreply.github.com> --- docs/example-config-with-auth-plugins.yaml | 20 ++ docs/example-endpoint-with-custom-auth.yaml | 7 + docs/example_custom_auth_plugin.rb | 34 +++ lib/hooks/app/api.rb | 4 +- lib/hooks/app/auth/auth.rb | 22 +- lib/hooks/app/helpers.rb | 62 ++++++ lib/hooks/core/config_loader.rb | 22 +- lib/hooks/core/config_validator.rb | 4 +- .../app/auth/custom_auth_integration_spec.rb | 118 ++++++++++ .../hooks/app/auth/custom_auth_plugin_spec.rb | 203 ++++++++++++++++++ .../unit/lib/hooks/core/config_loader_spec.rb | 74 +++++++ 11 files changed, 563 insertions(+), 7 deletions(-) create mode 100644 docs/example-config-with-auth-plugins.yaml create mode 100644 docs/example-endpoint-with-custom-auth.yaml create mode 100644 docs/example_custom_auth_plugin.rb create mode 100644 spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb create mode 100644 spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb diff --git a/docs/example-config-with-auth-plugins.yaml b/docs/example-config-with-auth-plugins.yaml new file mode 100644 index 00000000..6a562e8c --- /dev/null +++ b/docs/example-config-with-auth-plugins.yaml @@ -0,0 +1,20 @@ +# Sample configuration for Hooks webhook server with custom auth plugins +handler_plugin_dir: ./spec/acceptance/handlers +auth_plugin_dir: ./spec/acceptance/plugins/auth # NEW! Directory for custom auth plugins +log_level: debug + +# Request handling +request_limit: 1048576 # 1MB max body size +request_timeout: 15 # 15 seconds timeout + +# Path configuration +root_path: /webhooks +health_path: /health +version_path: /version + +# Runtime behavior +environment: development + +# Available endpoints +# Each endpoint configuration file should be placed in the endpoints directory +endpoints_dir: ./spec/acceptance/config/endpoints \ No newline at end of file diff --git a/docs/example-endpoint-with-custom-auth.yaml b/docs/example-endpoint-with-custom-auth.yaml new file mode 100644 index 00000000..0ba3e0ed --- /dev/null +++ b/docs/example-endpoint-with-custom-auth.yaml @@ -0,0 +1,7 @@ +path: /example +handler: CoolNewHandler + +auth: + type: some_cool_auth_plugin + secret_env_key: SUPER_COOL_SECRET # the name of the environment variable containing the shared secret + header: Authorization \ No newline at end of file diff --git a/docs/example_custom_auth_plugin.rb b/docs/example_custom_auth_plugin.rb new file mode 100644 index 00000000..f48a4537 --- /dev/null +++ b/docs/example_custom_auth_plugin.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true +# Example custom auth plugin implementation +module Hooks + module Plugins + module Auth + class SomeCoolAuthPlugin < Base + def self.valid?(payload:, headers:, config:) + # Get the secret from environment variable + secret = fetch_secret(config) + + # Get the authorization header (case-insensitive) + auth_header = nil + headers.each do |key, value| + if key.downcase == "authorization" + auth_header = value + break + end + end + + # Check if the header matches our expected format + return false unless auth_header + + # Extract the token from "Bearer " format + return false unless auth_header.start_with?("Bearer ") + + token = auth_header[7..-1] # Remove "Bearer " prefix + + # Simple token comparison (in practice, this might be more complex) + token == secret + end + end + end + end +end diff --git a/lib/hooks/app/api.rb b/lib/hooks/app/api.rb index 1691b086..3d34c092 100644 --- a/lib/hooks/app/api.rb +++ b/lib/hooks/app/api.rb @@ -65,11 +65,11 @@ def self.create(config:, endpoints:, log:) if endpoint_config[:auth] log.info "validating request (id: #{request_id}, handler: #{handler_class_name})" - validate_auth!(raw_body, headers, endpoint_config) + validate_auth!(raw_body, headers, endpoint_config, config) end payload = parse_payload(raw_body, headers, symbolize: config[:symbolize_payload]) - handler = load_handler(handler_class_name, config[:handler_dir]) + handler = load_handler(handler_class_name, config[:handler_plugin_dir]) normalized_headers = config[:normalize_headers] ? Hooks::Utils::Normalize.headers(headers) : headers response = handler.call( diff --git a/lib/hooks/app/auth/auth.rb b/lib/hooks/app/auth/auth.rb index 7a552dee..c1f282f6 100644 --- a/lib/hooks/app/auth/auth.rb +++ b/lib/hooks/app/auth/auth.rb @@ -13,10 +13,11 @@ module Auth # @param payload [String, Hash] The request payload to authenticate. # @param headers [Hash] The request headers. # @param endpoint_config [Hash] The endpoint configuration, must include :auth key. + # @param global_config [Hash] The global configuration (optional, needed for custom auth plugins). # @raise [StandardError] Raises error if authentication fails or is misconfigured. # @return [void] # @note This method will halt execution with an error if authentication fails. - def validate_auth!(payload, headers, endpoint_config) + def validate_auth!(payload, headers, endpoint_config, global_config = {}) auth_config = endpoint_config[:auth] # Security: Ensure auth type is present and valid @@ -35,7 +36,24 @@ def validate_auth!(payload, headers, endpoint_config) when "shared_secret" auth_class = Plugins::Auth::SharedSecret else - error!("Custom validators not implemented in POC", 500) + # Try to load custom auth plugin if auth_plugin_dir is configured + if global_config[:auth_plugin_dir] + # Convert auth_type to CamelCase class name + auth_plugin_class_name = auth_type.split("_").map(&:capitalize).join("") + + # Validate the converted class name before attempting to load + unless valid_auth_plugin_class_name?(auth_plugin_class_name) + error!("invalid auth plugin type '#{auth_type}'", 400) + end + + begin + auth_class = load_auth_plugin(auth_plugin_class_name, global_config[:auth_plugin_dir]) + rescue => e + error!("failed to load custom auth plugin '#{auth_type}': #{e.message}", 500) + end + else + error!("Custom validators not implemented in POC", 500) + end end unless auth_class.valid?( diff --git a/lib/hooks/app/helpers.rb b/lib/hooks/app/helpers.rb index 180da285..6b4f6291 100644 --- a/lib/hooks/app/helpers.rb +++ b/lib/hooks/app/helpers.rb @@ -104,6 +104,47 @@ def load_handler(handler_class_name, handler_dir) error!("failed to load handler: #{e.message}", 500) end + # Load auth plugin class + # + # @param auth_plugin_class_name [String] The name of the auth plugin class to load + # @param auth_plugin_dir [String] The directory containing auth plugin files + # @return [Class] The loaded auth plugin class + # @raise [LoadError] If the auth plugin file or class cannot be found + # @raise [StandardError] Halts with error if auth plugin cannot be loaded + def load_auth_plugin(auth_plugin_class_name, auth_plugin_dir) + # Security: Validate auth plugin class name to prevent arbitrary class loading + unless valid_auth_plugin_class_name?(auth_plugin_class_name) + error!("invalid auth plugin class name: #{auth_plugin_class_name}", 400) + end + + # Convert class name to file name (e.g., SomeCoolAuthPlugin -> some_cool_auth_plugin.rb) + file_name = auth_plugin_class_name.gsub(/([A-Z])/, '_\1').downcase.sub(/^_/, "") + ".rb" + file_path = File.join(auth_plugin_dir, file_name) + + # Security: Ensure the file path doesn't escape the auth plugin directory + normalized_auth_plugin_dir = Pathname.new(File.expand_path(auth_plugin_dir)) + normalized_file_path = Pathname.new(File.expand_path(file_path)) + unless normalized_file_path.descend.any? { |path| path == normalized_auth_plugin_dir } + error!("auth plugin path outside of auth plugin directory", 400) + end + + if File.exist?(file_path) + require file_path + auth_plugin_class = Object.const_get("Hooks::Plugins::Auth::#{auth_plugin_class_name}") + + # Security: Ensure the loaded class inherits from the expected base class + unless auth_plugin_class < Hooks::Plugins::Auth::Base + error!("auth plugin class must inherit from Hooks::Plugins::Auth::Base", 400) + end + + auth_plugin_class + else + error!("Auth plugin #{auth_plugin_class_name} not found at #{file_path}", 500) + end + rescue => e + error!("failed to load auth plugin: #{e.message}", 500) + end + private # Validate that a handler class name is safe to load @@ -127,6 +168,27 @@ def valid_handler_class_name?(class_name) true end + # Validate that an auth plugin class name is safe to load + # + # @param class_name [String] The class name to validate + # @return [Boolean] true if the class name is safe, false otherwise + def valid_auth_plugin_class_name?(class_name) + # Must be a string + return false unless class_name.is_a?(String) + + # Must not be empty or only whitespace + return false if class_name.strip.empty? + + # Must match a safe pattern: alphanumeric + underscore, starting with uppercase + # Examples: MyAuthPlugin, SomeCoolAuthPlugin, CustomAuth + return false unless class_name.match?(/\A[A-Z][a-zA-Z0-9_]*\z/) + + # Must not be a system/built-in class name + return false if Hooks::Security::DANGEROUS_CLASSES.include?(class_name) + + true + end + # Determine HTTP error code from exception # # @param exception [Exception] The exception to map to an HTTP status code diff --git a/lib/hooks/core/config_loader.rb b/lib/hooks/core/config_loader.rb index bbd31887..9a43444e 100644 --- a/lib/hooks/core/config_loader.rb +++ b/lib/hooks/core/config_loader.rb @@ -8,7 +8,9 @@ module Core # Loads and merges configuration from files and environment variables class ConfigLoader DEFAULT_CONFIG = { - handler_dir: "./handlers", + handler_dir: "./handlers", # For backward compatibility + handler_plugin_dir: "./handlers", + auth_plugin_dir: nil, log_level: "info", request_limit: 1_048_576, request_timeout: 30, @@ -29,6 +31,7 @@ class ConfigLoader # @return [Hash] Merged configuration def self.load(config_path: nil) config = DEFAULT_CONFIG.dup + default_handler_dir = config[:handler_dir] # Load from file if path provided if config_path.is_a?(String) && File.exist?(config_path) @@ -44,6 +47,19 @@ def self.load(config_path: nil) # Convert string keys to symbols for consistency config = symbolize_keys(config) + # Support backward compatibility for handler_dir <-> handler_plugin_dir + # Check if values changed from default + if config[:handler_plugin_dir] != default_handler_dir && config[:handler_dir] == default_handler_dir + # Only handler_plugin_dir was changed, sync handler_dir + config[:handler_dir] = config[:handler_plugin_dir] + elsif config[:handler_dir] != default_handler_dir && config[:handler_plugin_dir] == default_handler_dir + # Only handler_dir was changed, sync handler_plugin_dir + config[:handler_plugin_dir] = config[:handler_dir] + elsif config[:handler_plugin_dir] != default_handler_dir && config[:handler_dir] != default_handler_dir + # Both changed, handler_plugin_dir takes precedence + config[:handler_dir] = config[:handler_plugin_dir] + end + if config[:environment] == "production" config[:production] = true else @@ -104,7 +120,9 @@ def self.load_env_config env_config = {} env_mappings = { - "HOOKS_HANDLER_DIR" => :handler_dir, + "HOOKS_HANDLER_DIR" => :handler_dir, # For backward compatibility + "HOOKS_HANDLER_PLUGIN_DIR" => :handler_plugin_dir, + "HOOKS_AUTH_PLUGIN_DIR" => :auth_plugin_dir, "HOOKS_LOG_LEVEL" => :log_level, "HOOKS_REQUEST_LIMIT" => :request_limit, "HOOKS_REQUEST_TIMEOUT" => :request_timeout, diff --git a/lib/hooks/core/config_validator.rb b/lib/hooks/core/config_validator.rb index 2a4a4a70..5e1204a1 100644 --- a/lib/hooks/core/config_validator.rb +++ b/lib/hooks/core/config_validator.rb @@ -12,7 +12,9 @@ class ValidationError < StandardError; end # Global configuration schema GLOBAL_CONFIG_SCHEMA = Dry::Schema.Params do - optional(:handler_dir).filled(:string) + optional(:handler_dir).filled(:string) # For backward compatibility + optional(:handler_plugin_dir).filled(:string) + optional(:auth_plugin_dir).maybe(:string) optional(:log_level).filled(:string, included_in?: %w[debug info warn error]) optional(:request_limit).filled(:integer, gt?: 0) optional(:request_timeout).filled(:integer, gt?: 0) diff --git a/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb b/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb new file mode 100644 index 00000000..edf544df --- /dev/null +++ b/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb @@ -0,0 +1,118 @@ +# frozen_string_literal: true + +require_relative "../../../../spec_helper" + +describe "Custom Auth Plugin Integration" do + let(:custom_auth_plugin_dir) { "/tmp/test_auth_plugins" } + let(:plugin_file_content) do + <<~RUBY + module Hooks + module Plugins + module Auth + class SomeCoolAuthPlugin < Base + def self.valid?(payload:, headers:, config:) + # Mock implementation: check for specific header + secret = fetch_secret(config) + bearer_token = headers["authorization"] + bearer_token == "Bearer \#{secret}" + end + end + end + end + end + RUBY + end + + let(:global_config) do + { + auth_plugin_dir: custom_auth_plugin_dir, + handler_plugin_dir: "./spec/acceptance/handlers" + } + end + + let(:endpoint_config) do + { + path: "/example", + handler: "DefaultHandler", + auth: { + type: "some_cool_auth_plugin", + secret_env_key: "SUPER_COOL_SECRET", + header: "Authorization" + } + } + end + + before do + FileUtils.mkdir_p(custom_auth_plugin_dir) + File.write(File.join(custom_auth_plugin_dir, "some_cool_auth_plugin.rb"), plugin_file_content) + ENV["SUPER_COOL_SECRET"] = "test-secret" + end + + after do + FileUtils.rm_rf(custom_auth_plugin_dir) if Dir.exist?(custom_auth_plugin_dir) + ENV.delete("SUPER_COOL_SECRET") + end + + it "successfully validates using a custom auth plugin" do + # Create a test API class using the same pattern as the real API + test_api_class = Class.new do + include Hooks::App::Helpers + include Hooks::App::Auth + + def error!(message, code) + raise StandardError, "#{message} (#{code})" + end + end + + instance = test_api_class.new + payload = '{"test": "data"}' + headers = { "authorization" => "Bearer test-secret" } + + # This should not raise any error + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.not_to raise_error + end + + it "rejects requests with invalid credentials using custom auth plugin" do + test_api_class = Class.new do + include Hooks::App::Helpers + include Hooks::App::Auth + + def error!(message, code) + raise StandardError, "#{message} (#{code})" + end + end + + instance = test_api_class.new + payload = '{"test": "data"}' + headers = { "authorization" => "Bearer wrong-secret" } + + # This should raise authentication failed error + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.to raise_error(StandardError, /authentication failed/) + end + + it "works with the new configuration format" do + # Test the new auth_plugin_dir configuration + config = Hooks::Core::ConfigLoader.load(config_path: { + auth_plugin_dir: "./custom/auth/plugins", + handler_plugin_dir: "./custom/handlers" + }) + + expect(config[:auth_plugin_dir]).to eq("./custom/auth/plugins") + expect(config[:handler_plugin_dir]).to eq("./custom/handlers") + expect(config[:handler_dir]).to eq("./custom/handlers") # backward compatibility + end + + it "maintains backward compatibility with handler_dir" do + # Test that old handler_dir configuration still works + config = Hooks::Core::ConfigLoader.load(config_path: { + handler_dir: "./legacy/handlers" + }) + + expect(config[:handler_dir]).to eq("./legacy/handlers") + expect(config[:handler_plugin_dir]).to eq("./legacy/handlers") + end +end diff --git a/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb b/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb new file mode 100644 index 00000000..32a751f8 --- /dev/null +++ b/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb @@ -0,0 +1,203 @@ +# frozen_string_literal: true + +require_relative "../../../../spec_helper" + +describe Hooks::App::Auth do + let(:log) { instance_double(Logger).as_null_object } + let(:test_class) do + Class.new do + include Hooks::App::Auth + include Hooks::App::Helpers + + def error!(message, code) + raise StandardError, "#{message} (#{code})" + end + end + end + + let(:instance) { test_class.new } + let(:payload) { '{"test": "data"}' } + let(:headers) { { "Content-Type" => "application/json" } } + + before(:each) do + Hooks::Log.instance = log + end + + describe "#validate_auth! with custom auth plugins" do + let(:custom_auth_plugin_dir) { "/tmp/custom_auth_plugins_test" } + let(:global_config) { { auth_plugin_dir: custom_auth_plugin_dir } } + + before do + # Create temporary directory for custom auth plugins + FileUtils.mkdir_p(custom_auth_plugin_dir) + end + + after do + # Clean up + FileUtils.rm_rf(custom_auth_plugin_dir) if Dir.exist?(custom_auth_plugin_dir) + end + + context "when custom auth plugin is configured but directory not set" do + it "falls back to POC error message" do + endpoint_config = { auth: { type: "some_cool_auth_plugin" } } + empty_global_config = {} + + expect do + instance.validate_auth!(payload, headers, endpoint_config, empty_global_config) + end.to raise_error(StandardError, /Custom validators not implemented in POC/) + end + end + + context "when custom auth plugin exists and is valid" do + let(:plugin_file_content) do + <<~RUBY + module Hooks + module Plugins + module Auth + class SomeCoolAuthPlugin < Base + def self.valid?(payload:, headers:, config:) + # Mock validation - always return true + true + end + end + end + end + end + RUBY + end + + before do + File.write(File.join(custom_auth_plugin_dir, "some_cool_auth_plugin.rb"), plugin_file_content) + end + + it "loads and uses the custom auth plugin successfully" do + endpoint_config = { auth: { type: "some_cool_auth_plugin" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.not_to raise_error + end + end + + context "when custom auth plugin fails validation" do + let(:plugin_file_content) do + <<~RUBY + module Hooks + module Plugins + module Auth + class FailingAuthPlugin < Base + def self.valid?(payload:, headers:, config:) + # Mock validation - always return false + false + end + end + end + end + end + RUBY + end + + before do + File.write(File.join(custom_auth_plugin_dir, "failing_auth_plugin.rb"), plugin_file_content) + end + + it "returns authentication failed error" do + endpoint_config = { auth: { type: "failing_auth_plugin" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.to raise_error(StandardError, /authentication failed/) + end + end + + context "when custom auth plugin file does not exist" do + it "returns custom plugin loading error" do + endpoint_config = { auth: { type: "nonexistent_plugin" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.to raise_error(StandardError, /Auth plugin NonexistentPlugin not found/) + end + end + + context "when custom auth plugin has security issues" do + context "with invalid class name" do + it "converts lowercase plugin name and fails to find file" do + endpoint_config = { auth: { type: "lowercase_plugin" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.to raise_error(StandardError, /Auth plugin LowercasePlugin not found/) + end + + it "rejects plugin with special characters" do + endpoint_config = { auth: { type: "plugin$bad" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.to raise_error(StandardError, /invalid auth plugin type/) + end + end + + context "with plugin that doesn't inherit from Base" do + let(:bad_plugin_file_content) do + <<~RUBY + module Hooks + module Plugins + module Auth + class BadPlugin + def self.valid?(payload:, headers:, config:) + true + end + end + end + end + end + RUBY + end + + before do + File.write(File.join(custom_auth_plugin_dir, "bad_plugin.rb"), bad_plugin_file_content) + end + + it "rejects plugin that doesn't inherit from Base" do + endpoint_config = { auth: { type: "bad_plugin" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.to raise_error(StandardError, /auth plugin class must inherit from/) + end + end + end + + context "with complex plugin names" do + let(:plugin_file_content) do + <<~RUBY + module Hooks + module Plugins + module Auth + class GitHubOAuth2Plugin < Base + def self.valid?(payload:, headers:, config:) + true + end + end + end + end + end + RUBY + end + + before do + File.write(File.join(custom_auth_plugin_dir, "git_hub_o_auth2_plugin.rb"), plugin_file_content) + end + + it "handles complex CamelCase names correctly" do + endpoint_config = { auth: { type: "git_hub_o_auth2_plugin" } } + + expect do + instance.validate_auth!(payload, headers, endpoint_config, global_config) + end.not_to raise_error + end + end + end +end diff --git a/spec/unit/lib/hooks/core/config_loader_spec.rb b/spec/unit/lib/hooks/core/config_loader_spec.rb index 740011c1..b1ff51bd 100644 --- a/spec/unit/lib/hooks/core/config_loader_spec.rb +++ b/spec/unit/lib/hooks/core/config_loader_spec.rb @@ -181,6 +181,80 @@ end end + context "with auth plugin directory configuration" do + it "includes auth_plugin_dir in default configuration" do + config = described_class.load + + expect(config).to include(auth_plugin_dir: nil) + end + + it "loads auth_plugin_dir from hash config" do + custom_config = { auth_plugin_dir: "./custom/auth/plugins" } + + config = described_class.load(config_path: custom_config) + + expect(config[:auth_plugin_dir]).to eq("./custom/auth/plugins") + end + + it "loads auth_plugin_dir from environment variable" do + ENV["HOOKS_AUTH_PLUGIN_DIR"] = "/opt/auth/plugins" + + config = described_class.load + + expect(config[:auth_plugin_dir]).to eq("/opt/auth/plugins") + end + end + + context "with handler directory backward compatibility" do + it "uses handler_plugin_dir when both are set" do + custom_config = { + handler_dir: "./old/handlers", + handler_plugin_dir: "./new/handlers" + } + + config = described_class.load(config_path: custom_config) + + expect(config[:handler_dir]).to eq("./new/handlers") # backward compatibility + expect(config[:handler_plugin_dir]).to eq("./new/handlers") + end + + it "sets handler_plugin_dir from handler_dir for backward compatibility" do + custom_config = { handler_dir: "./legacy/handlers" } + + config = described_class.load(config_path: custom_config) + + expect(config[:handler_dir]).to eq("./legacy/handlers") + expect(config[:handler_plugin_dir]).to eq("./legacy/handlers") + end + + it "sets handler_dir from handler_plugin_dir for backward compatibility" do + custom_config = { handler_plugin_dir: "./new/handlers" } + + config = described_class.load(config_path: custom_config) + + expect(config[:handler_dir]).to eq("./new/handlers") # backward compatibility + expect(config[:handler_plugin_dir]).to eq("./new/handlers") + end + + it "supports old HOOKS_HANDLER_DIR environment variable" do + ENV["HOOKS_HANDLER_DIR"] = "/legacy/handlers" + + config = described_class.load + + expect(config[:handler_dir]).to eq("/legacy/handlers") + expect(config[:handler_plugin_dir]).to eq("/legacy/handlers") + end + + it "supports new HOOKS_HANDLER_PLUGIN_DIR environment variable" do + ENV["HOOKS_HANDLER_PLUGIN_DIR"] = "/new/handlers" + + config = described_class.load + + expect(config[:handler_dir]).to eq("/new/handlers") # backward compatibility + expect(config[:handler_plugin_dir]).to eq("/new/handlers") + end + end + context "with production environment detection" do it "sets production to true when environment is production" do config = described_class.load(config_path: { environment: "production" }) From 2d9d52e33ad255b72cfe5d30a9ee4dd171bf0be0 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:22:14 -0700 Subject: [PATCH 04/12] revert --- .bundle/config | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.bundle/config b/.bundle/config index f9263841..7095f6e9 100644 --- a/.bundle/config +++ b/.bundle/config @@ -1,6 +1,6 @@ --- BUNDLE_BIN: "bin" -BUNDLE_PATH: "/home/runner/work/hooks/hooks/vendor/bundle" +BUNDLE_PATH: "vendor/gems" BUNDLE_CACHE_PATH: "vendor/cache" BUNDLE_CACHE_ALL: "true" BUNDLE_SPECIFIC_PLATFORM: "true" From f78a1ef3abdec4bfc53766544e0ac79e670d1697 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:25:22 -0700 Subject: [PATCH 05/12] make the message more clear when no auth_plugin_dir is set for the given auth_type --- lib/hooks/app/auth/auth.rb | 2 +- spec/unit/lib/hooks/app/auth/auth_security_spec.rb | 2 +- spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/lib/hooks/app/auth/auth.rb b/lib/hooks/app/auth/auth.rb index c1f282f6..ced4d2b4 100644 --- a/lib/hooks/app/auth/auth.rb +++ b/lib/hooks/app/auth/auth.rb @@ -52,7 +52,7 @@ def validate_auth!(payload, headers, endpoint_config, global_config = {}) error!("failed to load custom auth plugin '#{auth_type}': #{e.message}", 500) end else - error!("Custom validators not implemented in POC", 500) + error!("unsupported auth type '#{auth_type}' due to auth_plugin_dir not being set", 400) end end diff --git a/spec/unit/lib/hooks/app/auth/auth_security_spec.rb b/spec/unit/lib/hooks/app/auth/auth_security_spec.rb index 7a0b9150..e56435d8 100644 --- a/spec/unit/lib/hooks/app/auth/auth_security_spec.rb +++ b/spec/unit/lib/hooks/app/auth/auth_security_spec.rb @@ -215,7 +215,7 @@ def error!(message, code) expect do instance.validate_auth!(payload, headers, endpoint_config) - end.to raise_error(StandardError, /Custom validators not implemented/) + end.to raise_error(StandardError, /unsupported auth type/) end end end diff --git a/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb b/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb index 32a751f8..eab13554 100644 --- a/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb +++ b/spec/unit/lib/hooks/app/auth/custom_auth_plugin_spec.rb @@ -44,7 +44,7 @@ def error!(message, code) expect do instance.validate_auth!(payload, headers, endpoint_config, empty_global_config) - end.to raise_error(StandardError, /Custom validators not implemented in POC/) + end.to raise_error(StandardError, /unsupported auth type/) end end From 20a69e251fcffc7745a1d57a38190bf8a0bee762 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:33:03 -0700 Subject: [PATCH 06/12] rename `handler_dir` to `handler_plugin_dir` --- lib/hooks/core/config_loader.rb | 16 ------ spec/acceptance/config/hooks.yaml | 2 +- spec/integration/hooks_integration_spec.rb | 2 +- .../app/auth/custom_auth_integration_spec.rb | 10 ++-- .../unit/lib/hooks/core/config_loader_spec.rb | 56 +------------------ 5 files changed, 9 insertions(+), 77 deletions(-) diff --git a/lib/hooks/core/config_loader.rb b/lib/hooks/core/config_loader.rb index 9a43444e..db8bccea 100644 --- a/lib/hooks/core/config_loader.rb +++ b/lib/hooks/core/config_loader.rb @@ -8,7 +8,6 @@ module Core # Loads and merges configuration from files and environment variables class ConfigLoader DEFAULT_CONFIG = { - handler_dir: "./handlers", # For backward compatibility handler_plugin_dir: "./handlers", auth_plugin_dir: nil, log_level: "info", @@ -31,7 +30,6 @@ class ConfigLoader # @return [Hash] Merged configuration def self.load(config_path: nil) config = DEFAULT_CONFIG.dup - default_handler_dir = config[:handler_dir] # Load from file if path provided if config_path.is_a?(String) && File.exist?(config_path) @@ -47,19 +45,6 @@ def self.load(config_path: nil) # Convert string keys to symbols for consistency config = symbolize_keys(config) - # Support backward compatibility for handler_dir <-> handler_plugin_dir - # Check if values changed from default - if config[:handler_plugin_dir] != default_handler_dir && config[:handler_dir] == default_handler_dir - # Only handler_plugin_dir was changed, sync handler_dir - config[:handler_dir] = config[:handler_plugin_dir] - elsif config[:handler_dir] != default_handler_dir && config[:handler_plugin_dir] == default_handler_dir - # Only handler_dir was changed, sync handler_plugin_dir - config[:handler_plugin_dir] = config[:handler_dir] - elsif config[:handler_plugin_dir] != default_handler_dir && config[:handler_dir] != default_handler_dir - # Both changed, handler_plugin_dir takes precedence - config[:handler_dir] = config[:handler_plugin_dir] - end - if config[:environment] == "production" config[:production] = true else @@ -120,7 +105,6 @@ def self.load_env_config env_config = {} env_mappings = { - "HOOKS_HANDLER_DIR" => :handler_dir, # For backward compatibility "HOOKS_HANDLER_PLUGIN_DIR" => :handler_plugin_dir, "HOOKS_AUTH_PLUGIN_DIR" => :auth_plugin_dir, "HOOKS_LOG_LEVEL" => :log_level, diff --git a/spec/acceptance/config/hooks.yaml b/spec/acceptance/config/hooks.yaml index 453cbcdd..eb87f2a5 100644 --- a/spec/acceptance/config/hooks.yaml +++ b/spec/acceptance/config/hooks.yaml @@ -1,5 +1,5 @@ # Sample configuration for Hooks webhook server -handler_dir: ./spec/acceptance/handlers +handler_plugin_dir: ./spec/acceptance/handlers log_level: debug # Request handling diff --git a/spec/integration/hooks_integration_spec.rb b/spec/integration/hooks_integration_spec.rb index 645b8d55..3d5efd65 100644 --- a/spec/integration/hooks_integration_spec.rb +++ b/spec/integration/hooks_integration_spec.rb @@ -12,7 +12,7 @@ def app @app ||= Hooks.build( config: { - handler_dir: "./spec/integration/tmp/handlers", + handler_plugin_dir: "./spec/integration/tmp/handlers", log_level: "error", # Reduce noise in tests request_limit: 1048576, request_timeout: 15, diff --git a/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb b/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb index edf544df..d516c713 100644 --- a/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb +++ b/spec/unit/lib/hooks/app/auth/custom_auth_integration_spec.rb @@ -103,16 +103,14 @@ def error!(message, code) expect(config[:auth_plugin_dir]).to eq("./custom/auth/plugins") expect(config[:handler_plugin_dir]).to eq("./custom/handlers") - expect(config[:handler_dir]).to eq("./custom/handlers") # backward compatibility end - it "maintains backward compatibility with handler_dir" do - # Test that old handler_dir configuration still works + it "uses handler_plugin_dir configuration" do + # Test that handler_plugin_dir configuration works config = Hooks::Core::ConfigLoader.load(config_path: { - handler_dir: "./legacy/handlers" + handler_plugin_dir: "./custom/handlers" }) - expect(config[:handler_dir]).to eq("./legacy/handlers") - expect(config[:handler_plugin_dir]).to eq("./legacy/handlers") + expect(config[:handler_plugin_dir]).to eq("./custom/handlers") end end diff --git a/spec/unit/lib/hooks/core/config_loader_spec.rb b/spec/unit/lib/hooks/core/config_loader_spec.rb index b1ff51bd..efbe1e15 100644 --- a/spec/unit/lib/hooks/core/config_loader_spec.rb +++ b/spec/unit/lib/hooks/core/config_loader_spec.rb @@ -7,7 +7,7 @@ config = described_class.load expect(config).to include( - handler_dir: "./handlers", + handler_plugin_dir: "./handlers", log_level: "info", request_limit: 1_048_576, request_timeout: 30, @@ -33,7 +33,7 @@ expect(config[:log_level]).to eq("debug") expect(config[:environment]).to eq("test") expect(config[:production]).to be false # should be false when environment is test - expect(config[:handler_dir]).to eq("./handlers") # defaults should remain + expect(config[:handler_plugin_dir]).to eq("./handlers") # defaults should remain end end @@ -69,7 +69,7 @@ expect(config[:environment]).to eq("development") expect(config[:request_timeout]).to eq(60) expect(config[:production]).to be false - expect(config[:handler_dir]).to eq("./handlers") # defaults should remain + expect(config[:handler_plugin_dir]).to eq("./handlers") # defaults should remain end end @@ -205,56 +205,6 @@ end end - context "with handler directory backward compatibility" do - it "uses handler_plugin_dir when both are set" do - custom_config = { - handler_dir: "./old/handlers", - handler_plugin_dir: "./new/handlers" - } - - config = described_class.load(config_path: custom_config) - - expect(config[:handler_dir]).to eq("./new/handlers") # backward compatibility - expect(config[:handler_plugin_dir]).to eq("./new/handlers") - end - - it "sets handler_plugin_dir from handler_dir for backward compatibility" do - custom_config = { handler_dir: "./legacy/handlers" } - - config = described_class.load(config_path: custom_config) - - expect(config[:handler_dir]).to eq("./legacy/handlers") - expect(config[:handler_plugin_dir]).to eq("./legacy/handlers") - end - - it "sets handler_dir from handler_plugin_dir for backward compatibility" do - custom_config = { handler_plugin_dir: "./new/handlers" } - - config = described_class.load(config_path: custom_config) - - expect(config[:handler_dir]).to eq("./new/handlers") # backward compatibility - expect(config[:handler_plugin_dir]).to eq("./new/handlers") - end - - it "supports old HOOKS_HANDLER_DIR environment variable" do - ENV["HOOKS_HANDLER_DIR"] = "/legacy/handlers" - - config = described_class.load - - expect(config[:handler_dir]).to eq("/legacy/handlers") - expect(config[:handler_plugin_dir]).to eq("/legacy/handlers") - end - - it "supports new HOOKS_HANDLER_PLUGIN_DIR environment variable" do - ENV["HOOKS_HANDLER_PLUGIN_DIR"] = "/new/handlers" - - config = described_class.load - - expect(config[:handler_dir]).to eq("/new/handlers") # backward compatibility - expect(config[:handler_plugin_dir]).to eq("/new/handlers") - end - end - context "with production environment detection" do it "sets production to true when environment is production" do config = described_class.load(config_path: { environment: "production" }) From 47edef52f6929bbc0b4d1c5f1f8eff7819ca48a0 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:35:44 -0700 Subject: [PATCH 07/12] fix: update default paths for handler and auth plugin directories --- lib/hooks/core/config_loader.rb | 4 ++-- spec/unit/lib/hooks/core/config_loader_spec.rb | 9 +++++---- 2 files changed, 7 insertions(+), 6 deletions(-) diff --git a/lib/hooks/core/config_loader.rb b/lib/hooks/core/config_loader.rb index db8bccea..68a47e38 100644 --- a/lib/hooks/core/config_loader.rb +++ b/lib/hooks/core/config_loader.rb @@ -8,8 +8,8 @@ module Core # Loads and merges configuration from files and environment variables class ConfigLoader DEFAULT_CONFIG = { - handler_plugin_dir: "./handlers", - auth_plugin_dir: nil, + handler_plugin_dir: "./plugins/handlers", + auth_plugin_dir: "./plugins/auth", log_level: "info", request_limit: 1_048_576, request_timeout: 30, diff --git a/spec/unit/lib/hooks/core/config_loader_spec.rb b/spec/unit/lib/hooks/core/config_loader_spec.rb index efbe1e15..95ac3d5a 100644 --- a/spec/unit/lib/hooks/core/config_loader_spec.rb +++ b/spec/unit/lib/hooks/core/config_loader_spec.rb @@ -7,7 +7,8 @@ config = described_class.load expect(config).to include( - handler_plugin_dir: "./handlers", + handler_plugin_dir: "./plugins/handlers", + auth_plugin_dir: "./plugins/auth", log_level: "info", request_limit: 1_048_576, request_timeout: 30, @@ -33,7 +34,7 @@ expect(config[:log_level]).to eq("debug") expect(config[:environment]).to eq("test") expect(config[:production]).to be false # should be false when environment is test - expect(config[:handler_plugin_dir]).to eq("./handlers") # defaults should remain + expect(config[:handler_plugin_dir]).to eq("./plugins/handlers") # defaults should remain end end @@ -69,7 +70,7 @@ expect(config[:environment]).to eq("development") expect(config[:request_timeout]).to eq(60) expect(config[:production]).to be false - expect(config[:handler_plugin_dir]).to eq("./handlers") # defaults should remain + expect(config[:handler_plugin_dir]).to eq("./plugins/handlers") # defaults should remain end end @@ -185,7 +186,7 @@ it "includes auth_plugin_dir in default configuration" do config = described_class.load - expect(config).to include(auth_plugin_dir: nil) + expect(config).to include(auth_plugin_dir: "./plugins/auth") end it "loads auth_plugin_dir from hash config" do From 6826e4e4b1477927d6c43d50a9a7ece2293bcc85 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:37:06 -0700 Subject: [PATCH 08/12] move into plugin dir --- spec/acceptance/config/hooks.yaml | 4 +++- spec/acceptance/{ => plugins}/handlers/github_handler.rb | 0 spec/acceptance/{ => plugins}/handlers/okta_handler.rb | 0 spec/acceptance/{ => plugins}/handlers/slack_handler.rb | 0 spec/acceptance/{ => plugins}/handlers/team1_handler.rb | 0 spec/acceptance/{ => plugins}/handlers/test_handler.rb | 0 6 files changed, 3 insertions(+), 1 deletion(-) rename spec/acceptance/{ => plugins}/handlers/github_handler.rb (100%) rename spec/acceptance/{ => plugins}/handlers/okta_handler.rb (100%) rename spec/acceptance/{ => plugins}/handlers/slack_handler.rb (100%) rename spec/acceptance/{ => plugins}/handlers/team1_handler.rb (100%) rename spec/acceptance/{ => plugins}/handlers/test_handler.rb (100%) diff --git a/spec/acceptance/config/hooks.yaml b/spec/acceptance/config/hooks.yaml index eb87f2a5..0ff9c7f0 100644 --- a/spec/acceptance/config/hooks.yaml +++ b/spec/acceptance/config/hooks.yaml @@ -1,5 +1,7 @@ # Sample configuration for Hooks webhook server -handler_plugin_dir: ./spec/acceptance/handlers +handler_plugin_dir: ./spec/acceptance/plugins/handlers +auth_plugin_dir: ./spec/acceptance/plugins/auth + log_level: debug # Request handling diff --git a/spec/acceptance/handlers/github_handler.rb b/spec/acceptance/plugins/handlers/github_handler.rb similarity index 100% rename from spec/acceptance/handlers/github_handler.rb rename to spec/acceptance/plugins/handlers/github_handler.rb diff --git a/spec/acceptance/handlers/okta_handler.rb b/spec/acceptance/plugins/handlers/okta_handler.rb similarity index 100% rename from spec/acceptance/handlers/okta_handler.rb rename to spec/acceptance/plugins/handlers/okta_handler.rb diff --git a/spec/acceptance/handlers/slack_handler.rb b/spec/acceptance/plugins/handlers/slack_handler.rb similarity index 100% rename from spec/acceptance/handlers/slack_handler.rb rename to spec/acceptance/plugins/handlers/slack_handler.rb diff --git a/spec/acceptance/handlers/team1_handler.rb b/spec/acceptance/plugins/handlers/team1_handler.rb similarity index 100% rename from spec/acceptance/handlers/team1_handler.rb rename to spec/acceptance/plugins/handlers/team1_handler.rb diff --git a/spec/acceptance/handlers/test_handler.rb b/spec/acceptance/plugins/handlers/test_handler.rb similarity index 100% rename from spec/acceptance/handlers/test_handler.rb rename to spec/acceptance/plugins/handlers/test_handler.rb From 652907a047278b7f55c0de27e8137ad88e545362 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:38:05 -0700 Subject: [PATCH 09/12] cleanup --- lib/hooks/core/builder.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/hooks/core/builder.rb b/lib/hooks/core/builder.rb index 5cc8c350..bb2c9066 100644 --- a/lib/hooks/core/builder.rb +++ b/lib/hooks/core/builder.rb @@ -44,8 +44,8 @@ def build # Build and return Grape API class Hooks::App::API.create( - config: config, - endpoints: endpoints, + config:, + endpoints:, log: @log ) end From 97622f498d94a2ed4c5e3b4b1792a1fbd43db597 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:44:57 -0700 Subject: [PATCH 10/12] refactor: migrate handlers to plugins architecture --- docs/design.md | 8 ++--- lib/hooks.rb | 3 +- lib/hooks/app/api.rb | 4 +-- lib/hooks/app/endpoints/catchall.rb | 2 +- lib/hooks/app/helpers.rb | 4 +-- lib/hooks/handlers/base.rb | 33 ----------------- lib/hooks/plugins/handlers/base.rb | 35 +++++++++++++++++++ lib/hooks/{ => plugins}/handlers/default.rb | 2 +- .../plugins/handlers/github_handler.rb | 2 +- .../plugins/handlers/okta_handler.rb | 2 +- .../plugins/handlers/slack_handler.rb | 2 +- .../plugins/handlers/team1_handler.rb | 2 +- .../plugins/handlers/test_handler.rb | 2 +- spec/integration/hooks_integration_spec.rb | 4 +-- .../lib/hooks/app/helpers_security_spec.rb | 8 ++--- spec/unit/lib/hooks/app/helpers_spec.rb | 6 ++-- spec/unit/lib/hooks/handlers/base_spec.rb | 2 +- spec/unit/lib/hooks/handlers/default_spec.rb | 4 +-- 18 files changed, 63 insertions(+), 62 deletions(-) delete mode 100644 lib/hooks/handlers/base.rb create mode 100644 lib/hooks/plugins/handlers/base.rb rename lib/hooks/{ => plugins}/handlers/default.rb (90%) diff --git a/docs/design.md b/docs/design.md index 6cec1ecf..3fdfee27 100644 --- a/docs/design.md +++ b/docs/design.md @@ -32,7 +32,7 @@ Note: The `hooks` gem name is already taken on RubyGems, so this project is name 2. **Plugin Architecture** - * **Team Handlers**: `class MyHandler < Hooks::Handlers::Base` + * **Team Handlers**: `class MyHandler < Hooks::Plugins::Handlers::Base` * Must implement `#call(payload:, headers:, config:)` method * `payload`: parsed request body (JSON Hash or raw String) * `headers`: HTTP headers as Hash with string keys @@ -142,7 +142,7 @@ lib/hooks/ │ ├── logger_factory.rb # Structured JSON logger + context enrichment │ ├── handlers/ -│ └── base.rb # `Hooks::Handlers::Base` interface: defines #call +│ └── base.rb # `Hooks::Plugins::Handlers::Base` interface: defines #call │ ├── plugins/ │ ├── lifecycle.rb # `Hooks::Plugins::Lifecycle` hooks (on_request, response, error) @@ -520,12 +520,12 @@ The health endpoint provides comprehensive status information for load balancers ### Core Classes -#### `Hooks::Handlers::Base` +#### `Hooks::Plugins::Handlers::Base` Base class for all webhook handlers. ```ruby -class MyHandler < Hooks::Handlers::Base +class MyHandler < Hooks::Plugins::Handlers::Base # @param payload [Hash, String] Parsed request body or raw string # @param headers [Hash] HTTP headers # @param config [Hash] Merged endpoint configuration diff --git a/lib/hooks.rb b/lib/hooks.rb index 6d3ee7d4..2105ae8a 100644 --- a/lib/hooks.rb +++ b/lib/hooks.rb @@ -2,9 +2,8 @@ require_relative "hooks/version" require_relative "hooks/core/builder" -require_relative "hooks/handlers/base" -# Load all plugins (request validators, lifecycle hooks, etc.) +# Load all plugins (auth plugins, handler plugins, lifecycle hooks, etc.) Dir[File.join(__dir__, "hooks/plugins/**/*.rb")].sort.each do |file| require file end diff --git a/lib/hooks/app/api.rb b/lib/hooks/app/api.rb index 3d34c092..176a7706 100644 --- a/lib/hooks/app/api.rb +++ b/lib/hooks/app/api.rb @@ -5,8 +5,8 @@ require "securerandom" require_relative "helpers" require_relative "auth/auth" -require_relative "../handlers/base" -require_relative "../handlers/default" +require_relative "../plugins/handlers/base" +require_relative "../plugins/handlers/default" require_relative "../core/logger_factory" require_relative "../core/log" diff --git a/lib/hooks/app/endpoints/catchall.rb b/lib/hooks/app/endpoints/catchall.rb index 3122156f..e26ccc3a 100644 --- a/lib/hooks/app/endpoints/catchall.rb +++ b/lib/hooks/app/endpoints/catchall.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true require "grape" -require_relative "../../handlers/default" +require_relative "../../plugins/handlers/default" require_relative "../helpers" module Hooks diff --git a/lib/hooks/app/helpers.rb b/lib/hooks/app/helpers.rb index 6b4f6291..1444fba6 100644 --- a/lib/hooks/app/helpers.rb +++ b/lib/hooks/app/helpers.rb @@ -92,8 +92,8 @@ def load_handler(handler_class_name, handler_dir) handler_class = Object.const_get(handler_class_name) # Security: Ensure the loaded class inherits from the expected base class - unless handler_class < Hooks::Handlers::Base - error!("handler class must inherit from Hooks::Handlers::Base", 400) + unless handler_class < Hooks::Plugins::Handlers::Base + error!("handler class must inherit from Hooks::Plugins::Handlers::Base", 400) end handler_class.new diff --git a/lib/hooks/handlers/base.rb b/lib/hooks/handlers/base.rb deleted file mode 100644 index 7f803a95..00000000 --- a/lib/hooks/handlers/base.rb +++ /dev/null @@ -1,33 +0,0 @@ -# frozen_string_literal: true - -module Hooks - module Handlers - # Base class for all webhook handlers - # - # All custom handlers must inherit from this class and implement the #call method - class Base - # Process a webhook request - # - # @param payload [Hash, String] Parsed request body (JSON Hash) or raw string - # @param headers [Hash] HTTP headers - # @param config [Hash] Merged endpoint configuration including opts section - # @return [Hash, String, nil] Response body (will be auto-converted to JSON) - # @raise [NotImplementedError] if not implemented by subclass - def call(payload:, headers:, config:) - raise NotImplementedError, "Handler must implement #call method" - end - - # Short logger accessor for all subclasses - # @return [Hooks::Log] Logger instance - # - # Provides a convenient way for handlers to log messages without needing - # to reference the full Hooks::Log namespace. - # - # @example Logging an error in an inherited class - # log.error("oh no an error occured") - def log - Hooks::Log.instance - end - end - end -end diff --git a/lib/hooks/plugins/handlers/base.rb b/lib/hooks/plugins/handlers/base.rb new file mode 100644 index 00000000..e0dffc2a --- /dev/null +++ b/lib/hooks/plugins/handlers/base.rb @@ -0,0 +1,35 @@ +# frozen_string_literal: true + +module Hooks + module Plugins + module Handlers + # Base class for all webhook handlers + # + # All custom handlers must inherit from this class and implement the #call method + class Base + # Process a webhook request + # + # @param payload [Hash, String] Parsed request body (JSON Hash) or raw string + # @param headers [Hash] HTTP headers + # @param config [Hash] Merged endpoint configuration including opts section + # @return [Hash, String, nil] Response body (will be auto-converted to JSON) + # @raise [NotImplementedError] if not implemented by subclass + def call(payload:, headers:, config:) + raise NotImplementedError, "Handler must implement #call method" + end + + # Short logger accessor for all subclasses + # @return [Hooks::Log] Logger instance + # + # Provides a convenient way for handlers to log messages without needing + # to reference the full Hooks::Log namespace. + # + # @example Logging an error in an inherited class + # log.error("oh no an error occured") + def log + Hooks::Log.instance + end + end + end + end +end diff --git a/lib/hooks/handlers/default.rb b/lib/hooks/plugins/handlers/default.rb similarity index 90% rename from lib/hooks/handlers/default.rb rename to lib/hooks/plugins/handlers/default.rb index 72ea718c..1041805a 100644 --- a/lib/hooks/handlers/default.rb +++ b/lib/hooks/plugins/handlers/default.rb @@ -2,7 +2,7 @@ # Default handler when no custom handler is found # This handler simply acknowledges receipt of the webhook and shows a few of the built-in features -class DefaultHandler < Hooks::Handlers::Base +class DefaultHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) log.info("🔔 Default handler invoked for webhook 🔔") diff --git a/spec/acceptance/plugins/handlers/github_handler.rb b/spec/acceptance/plugins/handlers/github_handler.rb index a05a5ee5..3af4ba8e 100644 --- a/spec/acceptance/plugins/handlers/github_handler.rb +++ b/spec/acceptance/plugins/handlers/github_handler.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Example handler for GitHub webhooks -class GithubHandler < Hooks::Handlers::Base +class GithubHandler < Hooks::Plugins::Handlers::Base # Process GitHub webhook # # @param payload [Hash, String] GitHub webhook payload diff --git a/spec/acceptance/plugins/handlers/okta_handler.rb b/spec/acceptance/plugins/handlers/okta_handler.rb index 014f0260..f06c37c6 100644 --- a/spec/acceptance/plugins/handlers/okta_handler.rb +++ b/spec/acceptance/plugins/handlers/okta_handler.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class OktaHandler < Hooks::Handlers::Base +class OktaHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) return { status: "success" diff --git a/spec/acceptance/plugins/handlers/slack_handler.rb b/spec/acceptance/plugins/handlers/slack_handler.rb index 6fe61c41..b2e053a2 100644 --- a/spec/acceptance/plugins/handlers/slack_handler.rb +++ b/spec/acceptance/plugins/handlers/slack_handler.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class SlackHandler < Hooks::Handlers::Base +class SlackHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) return { status: "success" diff --git a/spec/acceptance/plugins/handlers/team1_handler.rb b/spec/acceptance/plugins/handlers/team1_handler.rb index 0d51d6ef..08c9c176 100644 --- a/spec/acceptance/plugins/handlers/team1_handler.rb +++ b/spec/acceptance/plugins/handlers/team1_handler.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true # Example handler for Team 1 webhooks -class Team1Handler < Hooks::Handlers::Base +class Team1Handler < Hooks::Plugins::Handlers::Base # Process Team 1 webhook # # @param payload [Hash, String] Webhook payload diff --git a/spec/acceptance/plugins/handlers/test_handler.rb b/spec/acceptance/plugins/handlers/test_handler.rb index 5c0fd05b..1b072dc0 100644 --- a/spec/acceptance/plugins/handlers/test_handler.rb +++ b/spec/acceptance/plugins/handlers/test_handler.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -class TestHandler < Hooks::Handlers::Base +class TestHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) { status: "test_success", diff --git a/spec/integration/hooks_integration_spec.rb b/spec/integration/hooks_integration_spec.rb index 3d5efd65..74de3224 100644 --- a/spec/integration/hooks_integration_spec.rb +++ b/spec/integration/hooks_integration_spec.rb @@ -38,9 +38,9 @@ def app # Create test handler FileUtils.mkdir_p("./spec/integration/tmp/handlers") File.write("./spec/integration/tmp/handlers/test_handler.rb", <<~RUBY) - require_relative "../../../../lib/hooks/handlers/base" + require_relative "../../../../lib/hooks/plugins/handlers/base" - class TestHandler < Hooks::Handlers::Base + class TestHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) { status: "test_success", diff --git a/spec/unit/lib/hooks/app/helpers_security_spec.rb b/spec/unit/lib/hooks/app/helpers_security_spec.rb index 3d35bee2..36653492 100644 --- a/spec/unit/lib/hooks/app/helpers_security_spec.rb +++ b/spec/unit/lib/hooks/app/helpers_security_spec.rb @@ -158,7 +158,7 @@ def env before do # Create a valid handler file File.write(handler_file, <<~RUBY) - class TestHandler < Hooks::Handlers::Base + class TestHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) { message: "test" } end @@ -169,7 +169,7 @@ def call(payload:, headers:, config:) it "successfully loads valid handlers that inherit from Base" do handler = instance.load_handler(handler_name, handler_dir) expect(handler).to be_a(TestHandler) - expect(handler).to be_a(Hooks::Handlers::Base) + expect(handler).to be_a(Hooks::Plugins::Handlers::Base) end end @@ -188,10 +188,10 @@ def call(payload:, headers:, config:) RUBY end - it "rejects handlers that don't inherit from Hooks::Handlers::Base" do + it "rejects handlers that don't inherit from Hooks::Plugins::Handlers::Base" do expect do instance.load_handler(handler_name, handler_dir) - end.to raise_error(StandardError, /handler class must inherit from Hooks::Handlers::Base/) + end.to raise_error(StandardError, /handler class must inherit from Hooks::Plugins::Handlers::Base/) end end end diff --git a/spec/unit/lib/hooks/app/helpers_spec.rb b/spec/unit/lib/hooks/app/helpers_spec.rb index 329f3a6a..bee899e2 100644 --- a/spec/unit/lib/hooks/app/helpers_spec.rb +++ b/spec/unit/lib/hooks/app/helpers_spec.rb @@ -303,7 +303,7 @@ def error!(message, code) it "loads and instantiates a valid handler" do # Create a test handler file handler_content = <<~RUBY - class TestHandler < Hooks::Handlers::Base + class TestHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) { status: "ok" } end @@ -354,14 +354,14 @@ def call(payload:, headers:, config:) File.write(File.join(temp_dir, "bad_handler.rb"), handler_content) - expect { helper.load_handler("BadHandler", temp_dir) }.to raise_error(StandardError, /400.*must inherit from Hooks::Handlers::Base/) + expect { helper.load_handler("BadHandler", temp_dir) }.to raise_error(StandardError, /400.*must inherit from Hooks::Plugins::Handlers::Base/) end end context "with handler file that has syntax errors" do it "raises SyntaxError when handler file has syntax errors" do # Create a handler with syntax errors - handler_content = "class SyntaxErrorHandler < Hooks::Handlers::Base\n def call\n {invalid syntax\n end\nend" + handler_content = "class SyntaxErrorHandler < Hooks::Plugins::Handlers::Base\n def call\n {invalid syntax\n end\nend" File.write(File.join(temp_dir, "syntax_error_handler.rb"), handler_content) diff --git a/spec/unit/lib/hooks/handlers/base_spec.rb b/spec/unit/lib/hooks/handlers/base_spec.rb index acceca88..717f1356 100644 --- a/spec/unit/lib/hooks/handlers/base_spec.rb +++ b/spec/unit/lib/hooks/handlers/base_spec.rb @@ -1,6 +1,6 @@ # frozen_string_literal: true -describe Hooks::Handlers::Base do +describe Hooks::Plugins::Handlers::Base do describe "#call" do let(:handler) { described_class.new } let(:payload) { { "data" => "test" } } diff --git a/spec/unit/lib/hooks/handlers/default_spec.rb b/spec/unit/lib/hooks/handlers/default_spec.rb index f7066e74..5a96a43e 100644 --- a/spec/unit/lib/hooks/handlers/default_spec.rb +++ b/spec/unit/lib/hooks/handlers/default_spec.rb @@ -92,8 +92,8 @@ end describe "inheritance" do - it "inherits from Hooks::Handlers::Base" do - expect(described_class.superclass).to eq(Hooks::Handlers::Base) + it "inherits from Hooks::Plugins::Handlers::Base" do + expect(described_class.superclass).to eq(Hooks::Plugins::Handlers::Base) end end end From d800e16e1d8f5e2e0edea9724cbecba105645bd5 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 08:51:06 -0700 Subject: [PATCH 11/12] update docs --- docs/auth_plugins.md | 63 +++++++++++++++++++++ docs/example-config-with-auth-plugins.yaml | 20 ------- docs/example-endpoint-with-custom-auth.yaml | 7 --- docs/example_custom_auth_plugin.rb | 34 ----------- 4 files changed, 63 insertions(+), 61 deletions(-) create mode 100644 docs/auth_plugins.md delete mode 100644 docs/example-config-with-auth-plugins.yaml delete mode 100644 docs/example-endpoint-with-custom-auth.yaml delete mode 100644 docs/example_custom_auth_plugin.rb diff --git a/docs/auth_plugins.md b/docs/auth_plugins.md new file mode 100644 index 00000000..c817413a --- /dev/null +++ b/docs/auth_plugins.md @@ -0,0 +1,63 @@ +# Auth Plugins + +This document provides an example of how to implement a custom authentication plugin for a hypothetical system. The plugin checks for a specific authorization header and validates it against a secret stored in an environment variable. + +In your global configuration file (e.g. `hooks.yml`) you would likely set `auth_plugin_dir` to something like `./plugins/auth`. + +Here is an example snippet of how you might configure the global settings in `hooks.yml`: + +```yaml +# hooks.yml +auth_plugin_dir: ./plugins/auth # Directory where custom auth plugins are stored +``` + +Then place your custom auth plugin in the `./plugins/auth` directory, for example `./plugins/auth/some_cool_auth_plugin.rb`. + +```ruby +# frozen_string_literal: true +# Example custom auth plugin implementation +module Hooks + module Plugins + module Auth + class SomeCoolAuthPlugin < Base + def self.valid?(payload:, headers:, config:) + # Get the secret from environment variable + secret = fetch_secret(config) # by default, this will fetch the value of the environment variable specified in the config (e.g. SUPER_COOL_SECRET as defined by `secret_env_key`) + + # Get the authorization header (case-insensitive) + auth_header = nil + headers.each do |key, value| + if key.downcase == "authorization" + auth_header = value + break + end + end + + # Check if the header matches our expected format + return false unless auth_header + + # Extract the token from "Bearer " format + return false unless auth_header.start_with?("Bearer ") + + token = auth_header[7..-1] # Remove "Bearer " prefix + + # Simple token comparison (in practice, this might be more complex) + token == secret + end + end + end + end +end +``` + +Then you could create a new endpoint configuration that references this plugin: + +```yaml +path: /example +handler: CoolNewHandler + +auth: + type: some_cool_auth_plugin # using the newly created auth plugin as seen above + secret_env_key: SUPER_COOL_SECRET # the name of the environment variable containing the shared secret - used by `fetch_secret(config)` in the plugin + header: Authorization +``` diff --git a/docs/example-config-with-auth-plugins.yaml b/docs/example-config-with-auth-plugins.yaml deleted file mode 100644 index 6a562e8c..00000000 --- a/docs/example-config-with-auth-plugins.yaml +++ /dev/null @@ -1,20 +0,0 @@ -# Sample configuration for Hooks webhook server with custom auth plugins -handler_plugin_dir: ./spec/acceptance/handlers -auth_plugin_dir: ./spec/acceptance/plugins/auth # NEW! Directory for custom auth plugins -log_level: debug - -# Request handling -request_limit: 1048576 # 1MB max body size -request_timeout: 15 # 15 seconds timeout - -# Path configuration -root_path: /webhooks -health_path: /health -version_path: /version - -# Runtime behavior -environment: development - -# Available endpoints -# Each endpoint configuration file should be placed in the endpoints directory -endpoints_dir: ./spec/acceptance/config/endpoints \ No newline at end of file diff --git a/docs/example-endpoint-with-custom-auth.yaml b/docs/example-endpoint-with-custom-auth.yaml deleted file mode 100644 index 0ba3e0ed..00000000 --- a/docs/example-endpoint-with-custom-auth.yaml +++ /dev/null @@ -1,7 +0,0 @@ -path: /example -handler: CoolNewHandler - -auth: - type: some_cool_auth_plugin - secret_env_key: SUPER_COOL_SECRET # the name of the environment variable containing the shared secret - header: Authorization \ No newline at end of file diff --git a/docs/example_custom_auth_plugin.rb b/docs/example_custom_auth_plugin.rb deleted file mode 100644 index f48a4537..00000000 --- a/docs/example_custom_auth_plugin.rb +++ /dev/null @@ -1,34 +0,0 @@ -# frozen_string_literal: true -# Example custom auth plugin implementation -module Hooks - module Plugins - module Auth - class SomeCoolAuthPlugin < Base - def self.valid?(payload:, headers:, config:) - # Get the secret from environment variable - secret = fetch_secret(config) - - # Get the authorization header (case-insensitive) - auth_header = nil - headers.each do |key, value| - if key.downcase == "authorization" - auth_header = value - break - end - end - - # Check if the header matches our expected format - return false unless auth_header - - # Extract the token from "Bearer " format - return false unless auth_header.start_with?("Bearer ") - - token = auth_header[7..-1] # Remove "Bearer " prefix - - # Simple token comparison (in practice, this might be more complex) - token == secret - end - end - end - end -end From e7b996c4ff44cd91fdd8369ce4c9239f43f658e3 Mon Sep 17 00:00:00 2001 From: GrantBirki Date: Wed, 11 Jun 2025 09:01:46 -0700 Subject: [PATCH 12/12] feat: add custom auth plugin and related tests --- spec/acceptance/acceptance_tests.rb | 32 +++++++++++++++ .../config/endpoints/with_custom_auth.yml | 7 ++++ .../plugins/auth/example_auth_plugin.rb | 41 +++++++++++++++++++ .../plugins/handlers/test_handler.rb | 1 + 4 files changed, 81 insertions(+) create mode 100644 spec/acceptance/config/endpoints/with_custom_auth.yml create mode 100644 spec/acceptance/plugins/auth/example_auth_plugin.rb diff --git a/spec/acceptance/acceptance_tests.rb b/spec/acceptance/acceptance_tests.rb index 5a137f1d..59311be2 100644 --- a/spec/acceptance/acceptance_tests.rb +++ b/spec/acceptance/acceptance_tests.rb @@ -148,5 +148,37 @@ expect(body["status"]).to eq("success") end end + + describe "custom auth plugin" do + + it "successfully validates using a custom auth plugin" do + payload = {}.to_json + headers = { "Authorization" => "Bearer octoawesome-shared-secret" } + response = http.post("/webhooks/with_custom_auth_plugin", payload, headers) + + expect(response).to be_a(Net::HTTPSuccess) + body = JSON.parse(response.body) + expect(body["status"]).to eq("test_success") + expect(body["handler"]).to eq("TestHandler") + end + + it "rejects requests with invalid credentials using custom auth plugin" do + payload = {}.to_json + headers = { "Authorization" => "Bearer wrong-secret" } + response = http.post("/webhooks/with_custom_auth_plugin", payload, headers) + + expect(response).to be_a(Net::HTTPUnauthorized) + expect(response.body).to include("authentication failed") + end + + it "rejects requests with missing credentials using custom auth plugin" do + payload = {}.to_json + headers = {} + response = http.post("/webhooks/with_custom_auth_plugin", payload, headers) + + expect(response).to be_a(Net::HTTPUnauthorized) + expect(response.body).to include("authentication failed") + end + end end end diff --git a/spec/acceptance/config/endpoints/with_custom_auth.yml b/spec/acceptance/config/endpoints/with_custom_auth.yml new file mode 100644 index 00000000..6b3952d1 --- /dev/null +++ b/spec/acceptance/config/endpoints/with_custom_auth.yml @@ -0,0 +1,7 @@ +path: /with_custom_auth_plugin +handler: TestHandler + +auth: + type: example_auth_plugin + secret_env_key: SHARED_SECRET # the name of the environment variable containing the shared secret + header: Authorization diff --git a/spec/acceptance/plugins/auth/example_auth_plugin.rb b/spec/acceptance/plugins/auth/example_auth_plugin.rb new file mode 100644 index 00000000..15d6ad7c --- /dev/null +++ b/spec/acceptance/plugins/auth/example_auth_plugin.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +# this is just a super simple example of an auth plugin +# it is not secure and should not be used in production +# it is only for demonstration purposes + +module Hooks + module Plugins + module Auth + class ExampleAuthPlugin < Base + def self.valid?(payload:, headers:, config:) + # Get the secret from environment variable as configured with secret_env_key + secret = fetch_secret(config, secret_env_key_name: :secret_env_key) + + # Get the authorization header (case-insensitive) + auth_header = nil + headers.each do |key, value| + if key.downcase == "authorization" + auth_header = value + break + end + end + + # Check if the header matches our expected format + return false unless auth_header + + # Extract the token from "Bearer " format + return false unless auth_header.start_with?("Bearer ") + + token = auth_header[7..-1] # Remove "Bearer " prefix + + # Simple token comparison (in practice, this might be more complex) + Rack::Utils.secure_compare(token, secret) + rescue StandardError => e + log.error("ExampleAuthPlugin failed: #{e.message}") + false + end + end + end + end +end diff --git a/spec/acceptance/plugins/handlers/test_handler.rb b/spec/acceptance/plugins/handlers/test_handler.rb index 1b072dc0..c77f5329 100644 --- a/spec/acceptance/plugins/handlers/test_handler.rb +++ b/spec/acceptance/plugins/handlers/test_handler.rb @@ -4,6 +4,7 @@ class TestHandler < Hooks::Plugins::Handlers::Base def call(payload:, headers:, config:) { status: "test_success", + handler: "TestHandler", payload_received: payload, config_opts: config[:opts], timestamp: Time.now.iso8601