Restrict manifest diffs to fields in the request body manifest

[#173802711]

Co-authored-by: Nick Webb <nwebb@pivotal.io>
Co-authored-by: Sarah Weinstein <sweinstein@pivotal.io>

Author: nickjameswebb

app/actions/space_diff_manifest.rb

@@ -0,0 +1,47 @@
+require 'presenters/v3/app_manifest_presenter'
+require 'json-diff'
+
+module VCAP::CloudController
+  class SpaceDiffManifest
+    def self.generate_diff(app_manifests, space)
+      json_diff = []
+
+      app_manifests.each_with_index do |manifest_app_hash, index|
+        existing_app = space.app_models.find { |app| app.name == manifest_app_hash['name'] }
+
+        if existing_app.nil?
+          existing_app_hash = {}
+        else
+          manifest_presenter = Presenters::V3::AppManifestPresenter.new(
+            existing_app,
+            existing_app.service_bindings,
+            existing_app.routes,
+          )
+          existing_app_hash = manifest_presenter.to_hash.deep_stringify_keys['applications'][0]
+        end
+
+        manifest_app_hash.each do |key, value|
+          existing_value = existing_app_hash[key]
+
+          key_diffs = JsonDiff.diff(
+            existing_value,
+            value,
+            include_was: true,
+          )
+
+          key_diffs.each do |diff|
+            diff['path'] = "/applications/#{index}/#{key}" + diff['path']
+
+            if diff['op'] == 'replace' && diff['was'].nil?
+              diff['op'] = 'add'
+            end
+
+            json_diff << diff
+          end
+        end
+      end
+
+      json_diff
+    end
+  end
+end

app/controllers/v3/space_manifests_controller.rb

@@ -3,7 +3,7 @@
 require 'messages/named_app_manifest_message'
 require 'actions/app_find_or_create_skeleton'
 require 'actions/app_create'
-require 'json-diff'
+require 'actions/space_diff_manifest'
 
 class SpaceManifestsController < ApplicationController
   wrap_parameters :body, format: [:yaml]
@@ -15,7 +15,7 @@ def apply_manifest
     space_not_found! unless space && permission_queryer.can_read_from_space?(space.guid, space.organization.guid)
     unauthorized! unless permission_queryer.can_write_to_space?(space.guid)
 
-    messages = parsed_app_manifests.map { |app_manifest| NamedAppManifestMessage.create_from_yml(app_manifest) }
+    messages = parsed_app_manifests.map(&:to_unsafe_h).map { |app_manifest| NamedAppManifestMessage.create_from_yml(app_manifest) }
     errors = messages.each_with_index.flat_map { |message, i| errors_for_message(message, i) }
     compound_error!(errors) unless errors.empty?
 
@@ -47,36 +47,11 @@ def diff_manifest
     space_not_found! unless space && permission_queryer.can_read_from_space?(space.guid, space.organization.guid)
     unauthorized! unless permission_queryer.can_write_to_space?(space.guid)
 
-    json_diff = []
+    parsed_manifests = parsed_app_manifests.map(&:to_hash)
 
-    parsed_app_manifests.each_with_index do |manifest_app_hash, index|
-      existing_app = space.app_models.find { |app| app.name == manifest_app_hash['name'] } || {}
+    diff = SpaceDiffManifest.generate_diff(parsed_manifests, space)
 
-      if existing_app == {}
-        existing_app_hash = {}
-      else
-        manifest_presenter = Presenters::V3::AppManifestPresenter.new(
-          existing_app,
-          existing_app.service_bindings,
-          existing_app.routes,
-        )
-        existing_app_hash = manifest_presenter.to_hash.deep_stringify_keys['applications'][0]
-      end
-
-      curr_diff = JsonDiff.diff(
-        existing_app_hash,
-        manifest_app_hash.to_hash,
-        include_was: true,
-      )
-
-      curr_diff.each do |diff|
-        diff['path'] = "/applications/#{index}" + diff['path']
-      end
-
-      json_diff += curr_diff
-    end
-
-    render status: :created, json: { diff: json_diff }
+    render status: :created, json: { diff: diff }
   end
 
   private
@@ -119,10 +94,10 @@ def check_version_is_supported!
 
   def parsed_app_manifests
     check_version_is_supported!
-    parsed_applications = params[:body]['applications']
+    parsed_applications = params[:body].permit!['applications']
     raise unprocessable!("Cannot parse manifest with no 'applications' field.") unless parsed_applications.present?
 
-    parsed_applications.map(&:to_unsafe_h)
+    parsed_applications
   end
 
   def space_not_found!

spec/request/space_manifests_spec.rb

@@ -109,7 +109,6 @@
     it 'applies the manifest' do
       web_process = app1_model.web_processes.first
       expect(web_process.instances).to eq(1)
-
       post "/v3/spaces/#{space.guid}/actions/apply_manifest", yml_manifest, yml_headers(user_header)
 
       expect(last_response.status).to eq(202)
@@ -462,11 +461,36 @@
   end
 
   describe 'POST /v3/spaces/:guid/manifest_diff' do
+    let(:api_call) { lambda { |user_headers| post "/v3/spaces/#{space.guid}/manifest_diff", yml_manifest, yml_headers(user_headers) } }
+
     let(:org) { space.organization }
     let(:app1_model) { VCAP::CloudController::AppModel.make(name: 'app-1', space: space) }
     let!(:process1) { VCAP::CloudController::ProcessModel.make(app: app1_model) }
-
-    let(:api_call) { lambda { |user_headers| post "/v3/spaces/#{space.guid}/manifest_diff", yml_manifest, yml_headers(user_headers) } }
+    let!(:route_mapping) { VCAP::CloudController::RouteMappingModel.make(app: app1_model, process_type: process1.type, route: route) }
+    let(:default_manifest) do
+      {
+        'applications' => [
+          {
+            'name' => app1_model.name,
+            'stack' => process1.stack.name,
+            'routes' => [
+              {
+                'route' => "a_host.#{shared_domain.name}"
+              }
+            ],
+            'processes' => [
+              {
+                'type' => process1.type,
+                'instances' => process1.instances,
+                'memory' => '1024M',
+                'disk_quota' => '1024M',
+                'health-check-type' =>  process1.health_check_type
+              }
+            ]
+          },
+        ]
+      }
+    end
 
     let(:expected_codes_and_responses) do
       h = Hash.new(code: 403)
@@ -487,92 +511,25 @@
       h.freeze
     end
 
-    context 'when there are no changes in the manifest' do
-      let(:diff_json) do
-        {
-          diff: []
-        }
-      end
-
-      let(:yml_manifest) do
-        {
-          'applications' => [
-            {
-              'name' => app1_model.name,
-              'stack' => process1.stack.name,
-              'processes' => [
-                {
-                  'type' => process1.type,
-                  'instances' => process1.instances,
-                  'memory' => '1024M',
-                  'disk_quota' => '1024M',
-                  'health-check-type' =>  process1.health_check_type
-                }
-              ]
-            },
-          ]
-        }.to_yaml
-      end
-      it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
-    end
-
     context 'when there are changes in the manifest' do
       let(:diff_json) do
         {
-          diff: [
-            { op: 'add', path: '/applications/0/comp', value: 'hoh' },
+          diff: a_collection_containing_exactly(
+            { op: 'add', path: '/applications/0/new-key', value: 'hoh' },
             { op: 'replace', path: '/applications/0/stack', was: process1.stack.name, value: 'big brother' },
-            { op: 'remove', path: '/applications/0/processes/0/memory', was: '1024M' },
-          ]
+          )
         }
       end
 
       let(:yml_manifest) do
-        {
-          'version' => 1,
-          'applications' => [
-            {
-              'name' => app1_model.name,
-              'stack' => 'big brother',
-              'comp' => 'hoh',
-              'processes' => [
-                {
-                  'type' => process1.type,
-                  'instances' => process1.instances,
-                  'disk_quota' => '1024M',
-                  'health-check-type' =>  process1.health_check_type
-                }
-              ]
-            },
-          ]
-        }.to_yaml
-      end
-      it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
-    end
-    context 'when there is a new app' do
-      let(:diff_json) do
-        {
-          diff: [
-            { op: 'add', path: '/applications/0/name', value: 'new-app' },
-            { op: 'add', path: '/applications/1/name', value: 'newer-app' },
-          ]
-        }
+        default_manifest['applications'][0]['new-key'] = 'hoh'
+        default_manifest['applications'][0]['stack'] = 'big brother'
+        default_manifest.to_yaml
       end
 
-      let(:yml_manifest) do
-        {
-          'applications' => [
-            {
-              'name' => 'new-app',
-            },
-            {
-              'name' => 'newer-app',
-            }
-          ]
-        }.to_yaml
-      end
       it_behaves_like 'permissions for single object endpoint', ALL_PERMISSIONS
     end
+
     context 'when the request is invalid' do
       before do
         space.organization.add_user(user)

spec/unit/actions/space_diff_manifest_spec.rb

@@ -0,0 +1,119 @@
+require 'spec_helper'
+require 'actions/space_diff_manifest'
+
+module VCAP::CloudController
+  RSpec.describe SpaceDiffManifest do
+    describe 'generate_diff' do
+      let(:default_manifest) {
+        {
+          'applications' => [
+            {
+              'name' => app1_model.name,
+              'stack' => process1.stack.name,
+              'routes' => [
+                {
+                  'route' => "a_host.#{shared_domain.name}"
+                }
+              ],
+              'processes' => [
+                {
+                  'type' => process1.type,
+                  'instances' => process1.instances,
+                  'memory' => '1024M',
+                  'disk_quota' => '1024M',
+                  'health-check-type' =>  process1.health_check_type
+                }
+              ]
+            },
+          ]
+        }
+      }
+
+      let(:app_manifests) { default_manifest['applications'] }
+      let(:space) { Space.make }
+      let(:app1_model) { AppModel.make(name: 'app-1', space: space) }
+      let!(:process1) { ProcessModel.make(app: app1_model) }
+      let(:shared_domain) { VCAP::CloudController::SharedDomain.make }
+      let(:route) { VCAP::CloudController::Route.make(domain: shared_domain, space: space, host: 'a_host') }
+      let!(:route_mapping) { VCAP::CloudController::RouteMappingModel.make(app: app1_model, process_type: process1.type, route: route) }
+
+      subject { SpaceDiffManifest.generate_diff(app_manifests, space) }
+
+      context 'when a top-level field is omitted' do
+        before do
+          default_manifest['applications'][0].delete 'stack'
+        end
+
+        it 'returns an empty diff' do
+          expect(subject).to eq([])
+        end
+      end
+
+      context 'when there are changes in the manifest' do
+        before do
+          default_manifest['applications'][0]['new-key'] = 'hoh'
+          default_manifest['applications'][0]['stack'] = 'big brother'
+        end
+
+        it 'returns the correct diff' do
+          expect(subject).to match_array([
+            { 'op' => 'add', 'path' => '/applications/0/new-key', 'value' => 'hoh' },
+            { 'op' => 'replace', 'path' => '/applications/0/stack', 'was' => process1.stack.name, 'value' => 'big brother' },
+          ])
+        end
+      end
+
+      context 'when there is a change inside of a nested hash' do
+        before do
+          default_manifest['applications'][0]['processes'][0].delete('memory')
+        end
+
+        it 'returns the correct diff' do
+          expect(subject).to eq([
+            { 'op' => 'remove', 'path' => '/applications/0/processes/0/memory', 'was' => '1024M' },
+          ])
+        end
+      end
+
+      context 'when there is a difference in a nested array' do
+        before do
+          default_manifest['applications'][0]['sidecars'] = [
+            { 'name' => 'sidecar1', 'command' => 'bundle exec rake lol', 'process_types' => ['web', 'worker'] }
+          ]
+        end
+
+        it 'returns the correct diff' do
+          expect(subject).to eq([
+            {
+              'op' => 'add',
+              'path' => '/applications/0/sidecars',
+              'value' => [
+                { 'name' => 'sidecar1', 'command' => 'bundle exec rake lol', 'process_types' => ['web', 'worker'] }
+              ]
+            },
+          ])
+        end
+      end
+
+      context 'when there is a new app' do
+        let(:app_manifests) do
+          [
+            {
+              'name' => 'new-app',
+            },
+            {
+              'name' => 'newer-app',
+            }
+          ]
+        end
+
+        it 'returns the correct diff' do
+          expect(subject).to eq([
+            { 'op' => 'add', 'path' => '/applications/0/name', 'value' => 'new-app' },
+            { 'op' => 'add', 'path' => '/applications/1/name', 'value' => 'newer-app' },
+          ])
+        end
+      end
+    end
+  end
+end