chore: update the describe routes code and all routes spec (#647)

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
 

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,28 +68,34 @@ 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<Webmachine::DescribeRoutes::Route>]
+    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("/"),
           path_spec: webmachine_route.path_spec,
           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)

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

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

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