fix(agent): align action approval requests with the signed payload

packages/forest_admin_agent/lib/forest_admin_agent/routes/action/actions.rb

@@ -1,5 +1,7 @@
 require 'jsonapi-serializers'
 require 'active_support/inflector'
+require 'active_support/core_ext/hash/indifferent_access'
+require 'active_support/core_ext/object/deep_dup'
 require 'jwt'
 
 module ForestAdminAgent
@@ -50,7 +52,7 @@ def setup_routes
         def handle_request(args = {})
           context = build(args)
           ensure_served_action(context)
-          args = middleware_custom_action_approval_request_data(args)
+          args = middleware_custom_action_approval_request_data(args, context)
           filter_for_caller = get_record_selection(args, context)
 
           context.permissions.can_smart_action?(args, context.collection, filter_for_caller, action_name: @action_name)
@@ -210,23 +212,38 @@ def audited_record_ids(context, filter)
           []
         end
 
-        def middleware_custom_action_approval_request_data(args)
+        def middleware_custom_action_approval_request_data(args, context)
           raise Http::Exceptions::UnprocessableError if args.dig(:params, :data, :attributes, :requester_id)
 
-          if (signed_request = args.dig(:params, :data, :attributes, :signed_approval_request))
-            args[:params][:data][:attributes][:signed_approval_request] = decode_signed_approval_request(signed_request)
+          signed_request = args.dig(:params, :data, :attributes, :signed_approval_request)
+          return args if signed_request.nil?
+
+          payload = decode_signed_approval_request(signed_request)
+          data = payload[:data]
+          attributes = data[:attributes] if data.is_a?(ActiveSupport::HashWithIndifferentAccess)
+
+          unless attributes.is_a?(ActiveSupport::HashWithIndifferentAccess) &&
+                 attributes[:collection_name] == context.collection.name
+            raise Http::Exceptions::ForbiddenError, 'This approval request was not signed for this collection.'
           end
+          raise Http::Exceptions::ForbiddenError, 'Approval must name its records.' if attributes[:all_records]
 
+          attributes[:signed_approval_request] = payload.deep_dup
+          args[:params][:data] = data
           args
         end
 
         def decode_signed_approval_request(signed_request)
-          ForestAdminDatasourceToolkit::Utils::HashHelper.convert_keys(JWT.decode(
+          raise Http::Exceptions::ForbiddenError, 'Invalid approval request.' unless signed_request.is_a?(String)
+
+          JWT.decode(
             signed_request,
             Facades::Container.cache(:env_secret),
             true,
             { algorithm: 'HS256' }
-          )[0])
+          )[0].with_indifferent_access
+        rescue JWT::DecodeError
+          raise Http::Exceptions::ForbiddenError, 'Invalid approval request.'
         end
 
         def get_record_selection(args, context)

packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/action/actions_spec.rb

@@ -193,7 +193,10 @@ def request(attributes)
           end
 
           it 'refuses the approval of a request naming two ids and runs nothing' do
-            signed = JWT.encode({ data: { attributes: { ids: %w[4 7] } } }, Facades::Container.cache(:env_secret), 'HS256')
+            signed = JWT.encode(
+              { data: { attributes: { ids: %w[4 7], collection_name: 'orders' } } },
+              Facades::Container.cache(:env_secret), 'HS256'
+            )
 
             expect { route.handle_request(request(ids: %w[4 7], signed_approval_request: signed)) }
               .to raise_error(Http::Exceptions::UnprocessableError, message)

packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/action/actions_spec.rb

@@ -155,20 +155,191 @@ def declare_actions(actions)
           end
 
           context 'when the request is an approval' do
-            it 'get the signed parameters and change body' do
-              unsigned_request = { foo: 'value' }
+            it 'hands the checker the decoded request and uses the signed payload' do
+              unsigned_request = {
+                data: {
+                  attributes: {
+                    requester_id: 20,
+                    ids: ['123e4567-e89b-12d3-a456-426614174999'],
+                    collection_name: 'book',
+                    values: {}
+                  }
+                }
+              }
               signed_request = JWT.encode(
                 unsigned_request,
                 Facades::Container.cache(:env_secret),
                 'HS256'
               )
               args[:params][:data][:attributes][:signed_approval_request] = signed_request
+              args[:params][:data][:attributes][:values] = { 'firstname' => 'John' }
               allow(@action_collection).to receive(:execute).and_return({})
               action.handle_request(args)
 
               expect(permissions).to have_received(:can_smart_action?) do |args, _collection, _filter_for_caller|
-                expect(args[:params][:data][:attributes][:signed_approval_request]).to eq(unsigned_request)
+                expect(args[:params][:data][:attributes][:signed_approval_request][:data][:attributes][:requester_id]).to eq(20)
+                expect(args[:params][:data][:attributes][:ids]).to eq(['123e4567-e89b-12d3-a456-426614174999'])
               end
+              expect(@action_collection).to have_received(:execute) do |_caller, _action, data, _filter|
+                expect(data).to eq({})
+              end
+            end
+          end
+        end
+
+        context 'when the request carries a signed approval request' do
+          let(:book_id) { '123e4567-e89b-12d3-a456-426614174000' }
+          let(:signed_id) { '123e4567-e89b-12d3-a456-426614174999' }
+          let(:other_id) { '123e4567-e89b-12d3-a456-426614174888' }
+
+          def approval_token(attributes = {}, requester_id: 20, exp: Time.now.to_i + 600)
+            JWT.encode(
+              {
+                data: {
+                  type: 'custom-action-requests',
+                  id: 42,
+                  attributes: {
+                    requester_id: requester_id,
+                    ids: [signed_id],
+                    collection_name: 'book',
+                    smart_action_id: 'smartActionId',
+                    values: {},
+                    parent_collection_name: nil,
+                    parent_collection_id: nil,
+                    parent_association_name: nil,
+                    all_records: false,
+                    all_records_subset_query: nil
+                  }.merge(attributes)
+                },
+                approver: 1,
+                iat: Time.now.to_i,
+                exp: exp
+              },
+              Facades::Container.cache(:env_secret),
+              'HS256'
+            )
+          end
+
+          def approval_request(token, body_attributes = {})
+            args[:params][:data][:attributes][:signed_approval_request] = token
+            args[:params][:data][:attributes].merge!(body_attributes)
+            args
+          end
+
+          before do
+            @action_collection.add_action(
+              'foo',
+              ForestAdminDatasourceCustomizer::Decorators::Action::BaseAction.new(
+                scope: ForestAdminDatasourceCustomizer::Decorators::Action::Types::ActionScope::BULK,
+                form: [
+                  { type: ForestAdminDatasourceCustomizer::Decorators::Action::Types::FieldType::NUMBER,
+                    label: 'amount' },
+                  { type: ForestAdminDatasourceCustomizer::Decorators::Action::Types::FieldType::COLLECTION,
+                    label: 'customer', collection_name: 'book' }
+                ]
+              ) { |_context, result_builder| result_builder.success }
+            )
+            allow(@action_collection).to receive(:execute).and_return({})
+          end
+
+          it 'runs the action on the signed ids and hands the checker the signed requester' do
+            action.handle_request(approval_request(approval_token, ids: [other_id]))
+
+            expect(@action_collection).to have_received(:execute) do |_caller, _name, _data, filter|
+              expect(filter.condition_tree.to_h).to eq({ field: 'id', operator: Operators::EQUAL, value: signed_id })
+            end
+            expect(permissions).to have_received(:can_smart_action?) do |a, _collection, _filter|
+              expect(a.dig(:params, :data, :attributes, :signed_approval_request, :data, :attributes, :requester_id)).to eq(20)
+            end
+          end
+
+          it 'runs the action with the signed values' do
+            action.handle_request(approval_request(approval_token({ 'values' => { 'amount' => 10 } }), values: { 'amount' => 999_999 }))
+
+            expect(@action_collection).to have_received(:execute) do |_caller, _name, data, _filter|
+              expect(data).to eq('amount' => 10)
+            end
+          end
+
+          it 'parses the signed values like the frontend ones: a collection field is unpacked, not passed through' do
+            action.handle_request(approval_request(approval_token({ 'values' => { 'customer' => book_id } })))
+
+            expect(@action_collection).to have_received(:execute) do |_caller, _name, data, _filter|
+              expect(data).to eq('customer' => [book_id])
+            end
+          end
+
+          it 'refuses a token signed for another collection before any permission check' do
+            expect do
+              action.handle_request(approval_request(approval_token({ 'collection_name' => 'other_collection' })))
+            end.to raise_error(ForestAdminAgent::Http::Exceptions::ForbiddenError)
+
+            expect(permissions).not_to have_received(:can_smart_action?)
+            expect(@action_collection).not_to have_received(:execute)
+          end
+
+          it 'refuses a token that does not say which collection it was signed for' do
+            expect do
+              action.handle_request(approval_request(approval_token({ 'collection_name' => nil })))
+            end.to raise_error(ForestAdminAgent::Http::Exceptions::ForbiddenError)
+
+            expect(@action_collection).not_to have_received(:execute)
+          end
+
+          it 'refuses a token whose data does not carry server-shaped attributes' do
+            [JWT.encode({ data: { type: 'custom-action-requests' } }, Facades::Container.cache(:env_secret), 'HS256'),
+             JWT.encode({ data: [] }, Facades::Container.cache(:env_secret), 'HS256')].each do |token|
+              expect { action.handle_request(approval_request(token)) }
+                .to raise_error(ForestAdminAgent::Http::Exceptions::ForbiddenError)
+            end
+
+            expect(@action_collection).not_to have_received(:execute)
+          end
+
+          it 'refuses a token that selects all records instead of naming them' do
+            token = approval_token({ 'all_records' => true, 'all_records_subset_query' => { 'search' => 'needle' } })
+
+            expect { action.handle_request(approval_request(token)) }
+              .to raise_error(ForestAdminAgent::Http::Exceptions::ForbiddenError)
+
+            expect(permissions).not_to have_received(:can_smart_action?)
+            expect(@action_collection).not_to have_received(:execute)
+          end
+
+          it 'refuses an expired token with a 403-class error, not a 500' do
+            expect do
+              action.handle_request(approval_request(approval_token(exp: Time.now.to_i - 10)))
+            end.to raise_error(ForestAdminAgent::Http::Exceptions::ForbiddenError)
+
+            expect(@action_collection).not_to have_received(:execute)
+          end
+
+          it 'refuses a token signed with another secret, or not a token at all' do
+            other_secret_token = JWT.encode({ data: { attributes: { ids: [signed_id], collection_name: 'book' } } }, 'not-the-env-secret', 'HS256')
+
+            [other_secret_token, 'abc', true, 123, [], {}, false].each do |token|
+              expect { action.handle_request(approval_request(token)) }
+                .to raise_error(ForestAdminAgent::Http::Exceptions::ForbiddenError)
+            end
+
+            expect(@action_collection).not_to have_received(:execute)
+          end
+
+          it 'applies the outer query filters within the signed selection' do
+            args[:params][:filters] = { field: 'id', operator: Operators::EQUAL, value: book_id }
+
+            action.handle_request(approval_request(approval_token({ 'ids' => [book_id, signed_id] }), ids: [book_id]))
+
+            expect(@action_collection).to have_received(:execute) do |_caller, _name, _data, filter|
+              expect(filter.condition_tree.to_h).to eq(
+                {
+                  aggregator: 'And',
+                  conditions: [
+                    { field: 'id', operator: Operators::EQUAL, value: book_id },
+                    { field: 'id', operator: Operators::IN, value: [book_id, signed_id] }
+                  ]
+                }
+              )
             end
           end
         end

packages/forest_admin_agent/spec/lib/forest_admin_agent/services/smart_action_checker_spec.rb

@@ -468,6 +468,37 @@ module Services
         expect(smart_action_checker).to be_can_execute
       end
 
+      it 'refuses to approve the caller own request on the signed payload shape' do
+        parameters[:data][:attributes][:signed_approval_request] = {
+          data: {
+            attributes: {
+              requester_id: 1
+            }
+          }
+        }.with_indifferent_access
+        smart_action[:userApprovalEnabled] = [1]
+
+        smart_action_checker = described_class.new(parameters, @datasource.get_collection('Book'), smart_action,
+                                                   build_caller, 1, Filter.new)
+        expect { smart_action_checker.can_execute? }
+          .to raise_error(ForbiddenError, 'You don\'t have the permission to trigger this action.')
+      end
+
+      it 'approves another requester request on the same shape' do
+        parameters[:data][:attributes][:signed_approval_request] = {
+          data: {
+            attributes: {
+              requester_id: 20
+            }
+          }
+        }.with_indifferent_access
+        smart_action[:userApprovalEnabled] = [1]
+
+        smart_action_checker = described_class.new(parameters, @datasource.get_collection('Book'), smart_action,
+                                                   build_caller, 1, Filter.new)
+        expect(smart_action_checker).to be_can_execute
+      end
+
       it 'returns true when the user can approve and there is no userApprovalConditions and user roleId is present into selfApprovalEnabled' do
         parameters[:data][:attributes][:signed_approval_request] = {
           data: {