From 4adc657f1323c2c64878e582f08e67c1ec69ddf9 Mon Sep 17 00:00:00 2001 From: Beth Skurrie Date: Wed, 6 Dec 2023 13:45:52 +1100 Subject: [PATCH] chore: update the describe routes code and all routes spec (#647) --- lib/pact_broker/api/resources/all_webhooks.rb | 6 +- lib/webmachine/describe_routes.rb | 92 ++++++++++-------- .../api/resources/all_routes_spec.rb | 43 +++++---- spec/support/all_routes_spec_support.yml | 96 +------------------ tasks/development.rake | 12 ++- 5 files changed, 89 insertions(+), 160 deletions(-) diff --git a/lib/pact_broker/api/resources/all_webhooks.rb b/lib/pact_broker/api/resources/all_webhooks.rb index 95b492bee..d8d4f544e 100644 --- a/lib/pact_broker/api/resources/all_webhooks.rb +++ b/lib/pact_broker/api/resources/all_webhooks.rb @@ -38,7 +38,7 @@ def to_json end def from_json - saved_webhook = webhook_service.create(next_uuid, webhook, consumer, provider) + saved_webhook = webhook_service.create(next_uuid, webhook, webhook_consumer, webhook_provider) response.body = decorator_class(:webhook_decorator).new(saved_webhook).to_json(**decorator_options) end @@ -60,11 +60,11 @@ def schema api_contract_class(:webhook_contract) end - def consumer + def webhook_consumer webhook.consumer&.name ? pacticipant_service.find_pacticipant_by_name(webhook.consumer.name) : nil end - def provider + def webhook_provider webhook.provider&.name ? pacticipant_service.find_pacticipant_by_name(webhook.provider.name) : nil end diff --git a/lib/webmachine/describe_routes.rb b/lib/webmachine/describe_routes.rb index 47c3cb2e5..c75970a89 100644 --- a/lib/webmachine/describe_routes.rb +++ b/lib/webmachine/describe_routes.rb @@ -1,6 +1,10 @@ require "webmachine/adapters/rack_mapped" require "pact_broker/string_refinements" +# Code to describe the routes in a Webmachine API, including +# path, resource class, allowed methods, schemas, policy class. +# Used in tests and in the pact_broker:routes task + module Webmachine class DescribeRoutes using PactBroker::StringRefinements @@ -12,18 +16,11 @@ class DescribeRoutes :resource_name, :resource_class_location, :allowed_methods, - :policy_class, + :policy_names, + :policy_classes, # only used by pactflow :schemas, keyword_init: true) do - def [](key) - if respond_to?(key) - send(key) - else - nil - end - end - def path_include?(component) path.include?(component) end @@ -47,7 +44,7 @@ def build_resource(env, application_context, path_param_values) }.merge(path_params) rack_req = ::Rack::Request.new({ "REQUEST_METHOD" => "GET", "rack.input" => StringIO.new("") }.merge(env) ) - dummy_request = Webmachine::Adapters::Rack::RackRequest.new( + request = Webmachine::Adapters::Rack::RackRequest.new( rack_req.env["REQUEST_METHOD"], path, Webmachine::Headers.from_cgi({"HTTP_HOST" => "example.org"}.merge(env)), @@ -56,13 +53,13 @@ def build_resource(env, application_context, path_param_values) {}, rack_req.env ) - dummy_request.path_info = path_info - resource_class.new(dummy_request, Webmachine::Response.new) + request.path_info = path_info + resource_class.new(request, Webmachine::Response.new) end end def self.call(webmachine_applications, search_term: nil) - path_mappings = webmachine_applications.flat_map { | webmachine_application | paths_to_resource_class_mappings(webmachine_application) } + path_mappings = webmachine_applications.flat_map { | webmachine_application | build_routes(webmachine_application) } if search_term path_mappings = path_mappings.select{ |(route, _)| route[:path].include?(search_term) } @@ -71,8 +68,10 @@ def self.call(webmachine_applications, search_term: nil) path_mappings.sort_by{ | mapping | mapping[:path] } end - def self.paths_to_resource_class_mappings(webmachine_application) - webmachine_application.routes.collect do | webmachine_route | + # Build a Route object to describe every Webmachine route defined in the app.routes block + # @return [Array] + def self.build_routes(webmachine_application) + webmachine_routes_to_describe(webmachine_application).collect do | webmachine_route | resource_path_absolute = Pathname.new(source_location_for(webmachine_route.resource)) Route.new({ path: "/" + webmachine_route.path_spec.collect{ |part| part.is_a?(Symbol) ? ":#{part}" : part }.join("/"), @@ -80,19 +79,23 @@ def self.paths_to_resource_class_mappings(webmachine_application) resource_class: webmachine_route.resource, resource_name: webmachine_route.instance_variable_get(:@bindings)[:resource_name], resource_class_location: resource_path_absolute.relative_path_from(Pathname.pwd).to_s - }.merge(info_from_resource_instance(webmachine_route, webmachine_application.application_context))) - end.reject{ | route | route.resource_class == Webmachine::Trace::TraceResource } + }.merge(properties_for_webmachine_route(webmachine_route, webmachine_application.application_context))) + end end - def self.info_from_resource_instance(webmachine_route, application_context) + def self.webmachine_routes_to_describe(webmachine_application) + webmachine_application.routes.reject{ | route | route.resource == Webmachine::Trace::TraceResource }.collect + end + + def self.properties_for_webmachine_route(webmachine_route, application_context) with_no_logging do path_info = { application_context: application_context, pacticipant_name: "foo", pacticipant_version_number: "1", resource_name: "foo" } path_info.default = "1" - dummy_request = dummy_request(http_method: "GET", path_info: path_info) + request = build_request(http_method: "GET", path_info: path_info) - dummy_resource = webmachine_route.resource.new(dummy_request, Webmachine::Response.new) - if dummy_resource - info_from_dummy_resource(dummy_resource, webmachine_route, path_info) + resource = webmachine_route.resource.new(request, Webmachine::Response.new) + if resource + properties_for_resource(resource.allowed_methods - ["OPTIONS"], webmachine_route, application_context) else {} end @@ -102,29 +105,38 @@ def self.info_from_resource_instance(webmachine_route, application_context) {} end - def self.info_from_dummy_resource(dummy_resource, webmachine_route, path_info) + # Return the properties of the resource that can only be determined by instantiating the resource + # @return [Hash] + def self.properties_for_resource(allowed_methods, webmachine_route, application_context) + schemas = [] + policy_names = [] + allowed_methods.each do | http_method | + resource = build_resource(webmachine_route, http_method, application_context) + if (schema_class = resource.respond_to?(:schema, true) && resource.send(:schema)) + schemas << { http_method: http_method, class: schema_class, location: source_location_for(schema_class)} + end + + policy_names << resource.policy_name + end + { - allowed_methods: dummy_resource.allowed_methods, - schemas: dummy_resource.respond_to?(:schema, true) && dummy_resource.send(:schema) ? schemas(dummy_resource.allowed_methods, webmachine_route.resource, path_info) : nil - }.compact + allowed_methods: allowed_methods, + schemas: schemas, + policy_names: policy_names.uniq + } end - # This is not entirely accurate, because some GET requests have schemas too, but we can't tell that statically at the moment - def self.schemas(allowed_methods, resource, path_info) - (allowed_methods - ["GET", "OPTIONS", "DELETE"]).collect do | http_method | - resource.new(dummy_request(http_method: http_method, path_info: path_info), Webmachine::Response.new).send(:schema) - end.uniq.collect do | schema_class | - { - class: schema_class, - location: source_location_for(schema_class) - } - end + def self.build_resource(webmachine_route, http_method, application_context) + path_info = { application_context: application_context, pacticipant_name: "foo", pacticipant_version_number: "1", resource_name: "foo" } + path_info.default = "1" + request = build_request(http_method: http_method, path_info: path_info) + webmachine_route.resource.new(request, Webmachine::Response.new) end - def self.dummy_request(http_method: "GET", path_info: ) - dummy_request = Webmachine::Adapters::Rack::RackRequest.new(http_method, "/", Webmachine::Headers["host" => "example.org"], nil, {}, {}, { "REQUEST_METHOD" => http_method }) - dummy_request.path_info = path_info - dummy_request + def self.build_request(http_method: "GET", path_info: ) + request = Webmachine::Adapters::Rack::RackRequest.new(http_method, "/", Webmachine::Headers["host" => "example.org"], nil, {}, {}, { "REQUEST_METHOD" => http_method }) + request.path_info = path_info + request end def self.source_location_for(clazz) diff --git a/spec/lib/pact_broker/api/resources/all_routes_spec.rb b/spec/lib/pact_broker/api/resources/all_routes_spec.rb index d1b4a5beb..2f7d861c1 100644 --- a/spec/lib/pact_broker/api/resources/all_routes_spec.rb +++ b/spec/lib/pact_broker/api/resources/all_routes_spec.rb @@ -29,27 +29,22 @@ uuid: UUID } -REQUESTS_WHICH_ARE_EXECTED_TO_HAVE_NO_POLICY_RECORD = YAML.safe_load(File.read("spec/support/all_routes_spec_support.yml"))["requests_which_are_exected_to_have_no_policy_record"] - -RSpec.describe "all the routes" do - it "has a name for every route" do - expect(PactBroker.routes.reject(&:resource_name)).to eq [] - end - - it "has a unique name (except for the ones that don't which we can't change now because it would ruin the PF metrics)" do - dupliates = PactBroker.routes.collect(&:resource_name).group_by(&:itself).select { | _, values | values.size > 1 }.keys - expect(dupliates).to eq(["pact_publication", "verification_results", "verification_result"]) - end -end +REQUESTS_WHICH_ARE_EXECTED_TO_HAVE_NO_POLICY_RECORD = YAML.safe_load(File.read("spec/support/all_routes_spec_support.yml"))["requests_which_are_exected_to_have_no_policy_record_or_pacticipant"] +# Ensure that every resource/http method has a way of determining whether or not a user is allowed to call it. +# The actual permissions logic will be performed in Pactflow as the Pact Broker does not support a permissions model. +# Almost every route should either be associated with a pacticipant, or it should have a method to provide the policy_record. +# Some routes, as listed in spec/support/all_routes_spec_support.yml do not need either. +# This test is here to ensure that every new route added to the Pact Broker API has been considered, and explictly whitelisted if necessary. PactBroker.routes.each do | pact_broker_route | describe "#{pact_broker_route.path} (#{pact_broker_route.resource_name})" do - pact_broker_route.allowed_methods.each do | allowed_method | + (pact_broker_route.allowed_methods - ["OPTIONS"]).each do | allowed_method | - if allowed_method != "OPTIONS" && !REQUESTS_WHICH_ARE_EXECTED_TO_HAVE_NO_POLICY_RECORD.include?("#{pact_broker_route.resource_name} #{allowed_method}") + if !REQUESTS_WHICH_ARE_EXECTED_TO_HAVE_NO_POLICY_RECORD.include?("#{pact_broker_route.resource_name} #{allowed_method}") describe allowed_method do before do + # Create test data that matches the POTENTIAL_PARAMS above so that every route has data present for it td.create_consumer("foo") .create_provider("bar") .create_consumer_version("1", branch: "main", tag_names: ["prod"]) @@ -63,12 +58,26 @@ PactBroker::Pacts::PactVersion.first.update(sha: PACT_VERSION_SHA) end - it "has a policy record object" do - dummy_resource = pact_broker_route.build_resource({ "REQUEST_METHOD" => allowed_method }, PactBroker::ApplicationContext.default_application_context, POTENTIAL_PARAMS) - expect(dummy_resource.policy_record).to_not be nil + it "has a policy record object or a consumer/provider/pacticipant in the route" do + resource = pact_broker_route.build_resource({ "REQUEST_METHOD" => allowed_method }, PactBroker::ApplicationContext.default_application_context, POTENTIAL_PARAMS) + + thing = resource.consumer || resource.provider || resource.pacticipant || resource.respond_to?(:policy_record) + + expect(thing).to_not be_falsy end end end end end end + +RSpec.describe "all the routes" do + it "has a name for every route" do + expect(PactBroker.routes.reject(&:resource_name)).to eq [] + end + + it "has a unique name (except for the ones that don't which we can't change now because it would ruin the PF metrics)" do + duplicates = PactBroker.routes.collect(&:resource_name).group_by(&:itself).select { | _, values | values.size > 1 }.keys + expect(duplicates).to eq(["pact_publication", "verification_results", "verification_result"]) + end +end diff --git a/spec/support/all_routes_spec_support.yml b/spec/support/all_routes_spec_support.yml index f99f47597..cbb370047 100644 --- a/spec/support/all_routes_spec_support.yml +++ b/spec/support/all_routes_spec_support.yml @@ -1,114 +1,20 @@ -# Need to change this to use the path because there are resources with the same resource_name (pact_publications, verification_results and verification_result) -requests_which_are_exected_to_have_no_policy_record: - - publish_contracts POST +requests_which_are_exected_to_have_no_policy_record_or_pacticipant: - index GET - can_i_deploy GET - dashboard GET - - integration_dashboard GET - environments GET - environments POST - environment GET - environment PUT - environment DELETE - - group GET - integrations GET - integrations DELETE - - integration DELETE - matrix GET - - matrix_consumer_provider GET - - matrix_tag_badge GET - metrics GET - - latest_pact_publications GET - - pact_publication GET - - pact_publication PUT - - pact_publication DELETE - - pact_publication PATCH - pacticipants GET - pacticipants POST - - pacticipant GET - - pacticipant PUT - - pacticipant PATCH - - pacticipant DELETE - - can_i_deploy_latest_branch_version_to_environment GET - - can_i_deploy_latest_branch_version_to_environment_badge GET - - branch GET - - branch DELETE - - branch_version GET - - branch_version PUT - - branch_version DELETE - - pacticipant_label GET - - pacticipant_label PUT - - pacticipant_label DELETE - - latest_pacticipant_version GET - - latest_tagged_pacticipant_version GET - - can_i_deploy_latest_tagged_version_to_tag GET - - can_i_deploy_latest_tagged_version_to_tag_badge GET - - pacticipant_versions GET - - pacticipant_version GET - - pacticipant_version PUT - - pacticipant_version PATCH - - pacticipant_version DELETE - - pacticipant_version_tag GET - - pacticipant_version_tag PUT - - pacticipant_version_tag DELETE - - pacticipant_branch_versions GET - pacticipants_for_label GET - latest_pacts GET - - provider_pact_publications GET - - pact_publications_for_branch DELETE - - latest_pact_publication GET - - latest_untagged_pact_publication GET - - latest_untagged_pact_badge GET - - latest_pact_publication_for_branch GET - - latest_tagged_pact_publication GET - - latest_tagged_pact_badge GET - - latest_verification_results_for_latest_tagged_pact_publication GET - - latest_verification_results_for_latest_tagged_pact_publication DELETE - - latest_pact_badge GET - - latest_verification_results_for_latest_pact_publication GET - - latest_verification_results_for_latest_pact_publication DELETE - - pact_publication GET - - pact_version_diff_by_pact_version_sha GET - - pact_publication GET - - verification_results POST - - verification_result GET - - verification_result DELETE - - verification_results POST - - verification_result GET - - verification_result DELETE - - latest_verification_results_for_pact_version GET - - latest_verification_results_for_pact_version DELETE - - tagged_pact_publications GET - - tagged_pact_publications DELETE - - pact_publication GET - - pact_publication PUT - - pact_publication DELETE - - pact_publication PATCH - - previous_distinct_pact_version_diff GET - - pact_version_diff_by_consumer_version GET - - previous_distinct_pact_version GET - - latest_verification_results_for_pact_publication GET - - latest_verification_results_for_pact_publication DELETE - - pact_publications GET - - pact_publications DELETE - - pact_publication GET - - pact_publication PUT - - pact_publication DELETE - - pact_publication PATCH - - pact_webhooks POST - - pact_webhooks GET - - pact_webhooks_status GET - - pacts_for_verification GET - - pacts_for_verification POST - - latest_provider_pact_publications GET - - latest_tagged_provider_pact_publications GET - - tagged_provider_pact_publications GET - relationships GET - error_test GET - error_test POST - - verification_results_for_consumer_version GET - - webhooks GET - - consumer_webhooks GET - - provider_webhooks GET - - pacticipant_branches GET - - pacticipant_webhooks GET diff --git a/tasks/development.rake b/tasks/development.rake index 2bcc3efa1..ef62580a2 100644 --- a/tasks/development.rake +++ b/tasks/development.rake @@ -46,12 +46,14 @@ task :'pact_broker:routes', [:search_term] do | _t, args | puts "#{route.path}" puts " allowed_methods: #{route.allowed_methods.join(", ")}" puts " class: #{route.resource_class}" - puts " location: #{route.resource_class_location}" - if route[:schemas] + puts " location: #{route.resource_class_location}" + puts " policy_name: #{route.policy_names.collect(&:to_s).join(", ")}" + if route.schemas.any? puts " schemas:" - route[:schemas].each do | schema | - puts " class: #{schema[:class]}" - puts " location: #{schema[:location]}" + route.schemas.each do | schema | + puts " - method: #{schema[:http_method]}" + puts " class: #{schema[:class]}" + puts " location: #{schema[:location]}" end end