From 30cda05616f14b5442c186d094d45454841c797c Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 14:00:45 +0200 Subject: [PATCH 01/13] fix(agent): serve only the columns of collections the caller may read MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A read was permission-checked on the root collection only, so every column a projection, filter or sort reached through a relation was served with no check on the collection it came from — and a starts_with filter answered one guess per request without returning a column at all. The collection a path ends on is now checked. A field the caller named is refused with every offending path in one message; the default expansion is dropped from the projection instead, since refusing there would turn an ordinary listing into a 403. A polymorphic relation resolves to several collections and carries no discriminant, so every target must be readable: one denied is enough to deny the path. The resolver lives in the toolkit so the search layer and the routes cannot disagree about what a path reaches. A Count leaderboard names no path back to the collection it counts, so browse is asserted on it directly. fixes PRD-900 --- .../routes/charts/charts.rb | 43 ++++ .../routes/resources/count.rb | 2 + .../routes/resources/csv.rb | 10 +- .../routes/resources/list.rb | 8 +- .../routes/resources/related/csv_related.rb | 12 +- .../routes/resources/related/list_related.rb | 11 +- .../routes/resources/show.rb | 7 +- .../routes/resources/update.rb | 9 +- .../services/permissions.rb | 113 ++++++++++ .../forest_admin_agent/utils/csv_generator.rb | 23 ++ .../utils/query_string_parser.rb | 16 ++ .../routes/charts/charts_spec.rb | 1 + .../routes/resources/count_spec.rb | 1 + .../routes/resources/csv_spec.rb | 1 + .../routes/resources/list_spec.rb | 2 + .../resources/related/csv_related_spec.rb | 1 + .../resources/related/list_related_spec.rb | 2 + .../routes/resources/show_spec.rb | 1 + .../routes/resources/update_spec.rb | 1 + .../security/related_read_permissions_spec.rb | 202 ++++++++++++++++++ .../forest_admin_agent/spec/spec_helper.rb | 14 ++ .../search/search_collection_decorator.rb | 18 ++ .../decorators/collection_decorator.rb | 8 + .../utils/field_path.rb | 43 ++++ 24 files changed, 540 insertions(+), 9 deletions(-) create mode 100644 packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb create mode 100644 packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb index ef3dfa9d4..fd5eed75e 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb @@ -28,6 +28,7 @@ def setup_routes def handle_request(args = {}) context = build(args) context.permissions.can_chart?(args[:params]) + context.permissions.assert_can_read_query_fields(context.collection, args) type = validate_and_get_type(args[:params][:type]) filter = Filter.new( condition_tree: ConditionTreeFactory.intersect( @@ -89,6 +90,10 @@ def make_objective(context, filter, args) def make_pie(context, filter, args) group_field = args[:params][:groupByFieldName] + assert_can_read_aggregated_fields( + context, context.collection, + [['group a chart by', group_field], ['aggregate a chart on', args[:params][:aggregateFieldName]]] + ) aggregation = Aggregation.new( operation: args[:params][:aggregator], field: args[:params][:aggregateFieldName], @@ -102,6 +107,11 @@ def make_pie(context, filter, args) def make_line(context, filter, args) group_by_field_name = args[:params][:groupByFieldName] + assert_can_read_aggregated_fields( + context, context.collection, + [['group a chart by', group_by_field_name], + ['aggregate a chart on', args[:params][:aggregateFieldName]]] + ) time_range = args[:params][:timeRange] filter_only_with_values = filter.override( condition_tree: ConditionTree::ConditionTreeFactory.intersect( @@ -178,6 +188,18 @@ def make_leaderboard(context, filter, args) end if collection && leaderboard_filter && aggregation + assert_can_read_aggregated_fields( + context, context.datasource.get_collection(collection), + [['group a leaderboard by', aggregation.groups[0][:field]], + ['aggregate a leaderboard on', aggregation.field]] + ) + + # A count exposes the cardinality of the relation, which `/relationships//count` + # puts behind `browse`. No path names it, so nothing above sees it. + if aggregation.field.nil? + context.permissions.can?(:browse, context.datasource.get_collection(field.foreign_collection)) + end + rows = context.datasource.get_collection(collection).aggregate( context.caller, leaderboard_filter, @@ -200,12 +222,33 @@ def make_leaderboard(context, filter, args) end def compute_value(context, filter, args) + assert_can_read_aggregated_fields( + context, context.collection, + [['aggregate a chart on', args[:params][:aggregateFieldName]]] + ) aggregation = Aggregation.new(operation: args[:params][:aggregator], field: args[:params][:aggregateFieldName]) result = context.collection.aggregate(context.caller, filter, aggregation) result[0]['value'] || 0 end + + # +path_collection+ is what the paths resolve against; the permission root stays the chart's + # own collection, which the leaderboard call site does not share. + def assert_can_read_aggregated_fields(context, path_collection, fields) + usages = fields.reject { |_action, path| path.nil? || path.to_s.empty? } + .map do |action, path| + { + action: action, + path: path, + collections: ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names( + path_collection, path + ) + } + end + + context.permissions.assert_can_read_usages(context.collection.name, usages) + end end end end diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb index 1dd8e2ece..cf18cbb25 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb @@ -19,6 +19,8 @@ def handle_request(args = {}) context.permissions.can?(:browse, context.collection) if context.collection.is_countable? + context.permissions.assert_can_read_query_fields(context.collection, args) + filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( [ diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb index 9c028f78a..c727e1d3a 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/csv.rb @@ -22,6 +22,7 @@ def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.collection) context.permissions.can?(:export, context.collection) + context.permissions.assert_can_read_query_fields(context.collection, args) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( [ @@ -37,10 +38,15 @@ def handle_request(args = {}) sort: QueryStringParser.parse_sort(context.collection, args), segment: QueryStringParser.parse_segment(context.collection, args) ) - projection = QueryStringParser.parse_projection_from_request(context.collection, args) + requested = QueryStringParser.parse_requested_projection(context.collection, args) + projection = context.permissions.redact_projection( + context.collection, + requested[:projection], + named_by_caller: requested[:named_by_caller] + ) filename = args[:params][:filename] || args[:params]['collection_name'] filename += '.csv' unless /\.csv$/i.match?(filename) - header = args[:params][:header] + header = Utils::CsvGenerator.filter_header(args[:params][:header], requested[:projection], projection) # Generate timestamp for filename now = Time.now.strftime('%Y%m%d_%H%M%S') diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb index 3a0ab05d0..730f77004 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb @@ -17,6 +17,7 @@ def setup_routes def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.collection) + context.permissions.assert_can_read_query_fields(context.collection, args) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( @@ -33,7 +34,12 @@ def handle_request(args = {}) segment: QueryStringParser.parse_segment(context.collection, args) ) - projection = QueryStringParser.parse_projection_with_pks(context.collection, args) + requested = QueryStringParser.parse_requested_projection(context.collection, args) + projection = context.permissions.redact_projection( + context.collection, + requested[:projection], + named_by_caller: requested[:named_by_caller] + ).with_pks(context.collection) records = context.collection.list(context.caller, filter, projection) { diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb index f653a5bf6..3757bfd06 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb @@ -23,6 +23,7 @@ def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.child_collection) context.permissions.can?(:export, context.child_collection) + context.permissions.assert_can_read_query_fields(context.child_collection, args) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( @@ -32,9 +33,14 @@ def handle_request(args = {}) ] ) ) - projection = ForestAdminAgent::Utils::QueryStringParser.parse_projection_from_request( + requested = ForestAdminAgent::Utils::QueryStringParser.parse_requested_projection( context.child_collection, args ) + projection = context.permissions.redact_projection( + context.child_collection, + requested[:projection], + named_by_caller: requested[:named_by_caller] + ) # Get the parent record primary keys primary_key_values = Utils::Id.unpack_id(context.collection, args[:params]['id'], with_key: true) @@ -43,7 +49,9 @@ def handle_request(args = {}) # Generate timestamp for filename now = Time.now.strftime('%Y%m%d_%H%M%S') collection_name = args.dig(:params, 'collection_name') - header = args.dig(:params, 'header') + header = ForestAdminAgent::Utils::CsvGenerator.filter_header( + args.dig(:params, 'header'), requested[:projection], projection + ) filename_with_timestamp = "#{collection_name}_#{relation_name}_export_#{now}.csv" # Create a callable to fetch related records diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb index 58479d308..8ab1a5bb4 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb @@ -23,6 +23,7 @@ def setup_routes def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.child_collection) + context.permissions.assert_can_read_query_fields(context.child_collection, args) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( @@ -34,8 +35,14 @@ def handle_request(args = {}) page: ForestAdminAgent::Utils::QueryStringParser.parse_pagination(args), sort: ForestAdminAgent::Utils::QueryStringParser.parse_sort(context.child_collection, args) ) - projection = ForestAdminAgent::Utils::QueryStringParser.parse_projection_with_pks(context.child_collection, - args) + requested = ForestAdminAgent::Utils::QueryStringParser.parse_requested_projection( + context.child_collection, args + ) + projection = context.permissions.redact_projection( + context.child_collection, + requested[:projection], + named_by_caller: requested[:named_by_caller] + ).with_pks(context.child_collection) primary_key_values = Utils::Id.unpack_id(context.collection, args[:params]['id'], with_key: true) records = Collection.list_relation( context.collection, diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb index dc3856610..927e119e6 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb @@ -25,7 +25,12 @@ def handle_request(args = {}) condition_tree: ConditionTree::ConditionTreeFactory.intersect([condition_tree, scope]) ) - projection = QueryStringParser.parse_projection_with_pks(context.collection, args) + requested = QueryStringParser.parse_requested_projection(context.collection, args) + projection = context.permissions.redact_projection( + context.collection, + requested[:projection], + named_by_caller: requested[:named_by_caller] + ).with_pks(context.collection) records = context.collection.list(context.caller, filter, projection) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb index 507b0f23e..94f4180c5 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb @@ -26,7 +26,14 @@ def handle_request(args = {}) drop_relationships!(args) data = format_attributes(args, context.collection) context.collection.update(context.caller, filter, data) - records = context.collection.list(context.caller, filter, ProjectionFactory.all(context.collection)) + # The projection is ours, not the caller's, so it is redacted rather than refused: a write + # must not 403 because the row it wrote carries a relation the caller cannot read. + projection = context.permissions.redact_projection( + context.collection, + ProjectionFactory.all(context.collection), + named_by_caller: false + ) + records = context.collection.list(context.caller, filter, projection) { name: args[:params]['collection_name'], diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index ca591cfb5..debbb7f65 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -53,6 +53,91 @@ def can?(action, collection, allow_fetch: false) is_allowed end + # Whether the caller may read each of +collection_names+. + # + # +root_collection_name+ is pinned to readable and never looked up: +browse+ already gates a + # listing, +read+ a get, and the signed hash a chart. + # + # One cached pass for the whole request, and a single refetch only if it denied something — + # unlike +can?+, which refetches on every denial. Denial is the steady state here rather than + # the exception, so refetching per collection would cost one permission fetch per request. + def read_permissions(root_collection_name, collection_names) + to_check = collection_names.uniq.reject { |name| name == root_collection_name } + allowed = { root_collection_name => true } + + return allowed if to_check.empty? || !permission_system? + + user_data = get_user_data(caller.id) + collections_data = get_collections_permissions_data + results = to_check.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } + + unless results.values.all? + collections_data = get_collections_permissions_data(force_fetch: true) + results = to_check.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } + end + + allowed.merge(results) + end + + # An unnamed field is dropped rather than refused: the default expansion covers every column + # of every to-one relation, so refusing would turn an ordinary listing into a 403 for a caller + # that asked for nothing. + def redact_projection(collection, projection, named_by_caller:) + owners = projection.to_h do |path| + [path, ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names(collection, path)] + end + allowed = read_permissions(collection.name, owners.values.flatten) + readable = ->(path) { owners[path].all? { |name| allowed[name] } } + + if named_by_caller + denied = projection.reject { |path| readable.call(path) } + + unless denied.empty? + fields = denied.map { |path| "'#{path}' from the '#{owners[path].join("' or '")}' collection" } + raise ForbiddenError, "You are not allowed to read #{fields.join(", ")}." + end + end + + ForestAdminDatasourceToolkit::Components::Query::Projection.new(projection.select { |path| readable.call(path) }) + end + + # Refused rather than redacted: dropping a condition widens the result set and dropping a sort + # clause silently reorders it, while both leak the value they touch anyway — a `starts_with` + # filter answers one guess per request without returning a column of its own. + def assert_can_read_query_fields(collection, args) + usages = [] + push = lambda do |action, path| + usages << { + action: action, + path: path, + collections: ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names(collection, path) + } + end + + # `for_each_leaf` on a branch replaces each condition with the block's return value, so the + # leaf has to come back out or the tree is rebuilt from whatever `push` returned. + Utils::QueryStringParser.parse_condition_tree(collection, args)&.for_each_leaf do |leaf| + push.call('filter on', leaf.field) + leaf + end + + Utils::QueryStringParser.parse_sort(collection, args).each { |clause| push.call('sort on', clause[:field]) } + + assert_can_read_search(collection, args, usages) + assert_can_read_usages(collection.name, usages) + end + + def assert_can_read_usages(root_collection_name, usages) + allowed = read_permissions(root_collection_name, usages.flat_map { |usage| usage[:collections] }) + denied = usages.find { |usage| !usage[:collections].all? { |name| allowed[name] } } + + return unless denied + + raise ForbiddenError, + "You cannot #{denied[:action]} '#{denied[:path]}': you are not allowed to read the " \ + "'#{denied[:collections].join("' or '")}' collection." + end + def can_chart?(parameters) attributes = sanitize_chart_parameters(parameters.deep_symbolize_keys) hash_request = "#{attributes[:type]}:#{array_hash(attributes)}" @@ -188,6 +273,34 @@ def get_team(rendering_id) private + def read_allowed?(collections_data, collection_name, user_data) + return false unless user_data_valid?(user_data) + + collection_key = collection_name.to_sym + return false unless collection_exists?(collections_data, collection_key, collection_name, user_data) + + role_ids = get_role_ids_for_action(collections_data, collection_key, :read, collection_name, user_data) + return false unless role_ids + + check_user_permission(role_ids, user_data, :read, collection_name) + end + + # Asked of the stack, not derived from the schema: the fields an extended search reaches are + # read below the publication and renaming layers, and only that layer knows whether a replacer + # or a natively searchable datasource has taken the choice out of its hands. + def assert_can_read_search(collection, args, usages) + search = Utils::QueryStringParser.parse_search(collection, args) + + return if search.nil? || !collection.respond_to?(:searched_fields) + + extended = Utils::QueryStringParser.parse_search_extended(args) + searched = collection.searched_fields(search, extended) + + searched&.each do |field| + usages << { action: 'search on', path: field[:path], collections: field[:collections] } + end + end + def permission_allowed?(collections_data, collection, action, user_data) return false unless user_data_valid?(user_data) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb index e351f89f6..bb751c641 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb @@ -3,6 +3,29 @@ module ForestAdminAgent module Utils class CsvGenerator + # Labels are positionally aligned with the requested projection, so dropping a field without + # dropping its label shifts every later value under the wrong heading. + # + # Returns +header+ untouched when nothing was dropped, and when the caller sent none: the + # generator then falls back to the projection, which is already the redacted one. + def self.filter_header(header, requested, kept) + return header if header.nil? || kept.size == requested.size + + labels = header.is_a?(String) ? parse_header_labels(header) : header + + return header unless labels.is_a?(Array) + + labels.each_with_index + .select { |_label, index| kept.include?(requested[index]) } + .map(&:first) + end + + def self.parse_header_labels(header) + JSON.parse(header) + rescue JSON::ParserError + nil + end + def self.generate(records, projection) data = {} projection.each do |schema_field| diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index 97c584165..e967440be 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -66,6 +66,22 @@ def self.parse_projection_from_request(collection, args) parse_projection_from_header(collection, args) || parse_projection(collection, args) end + # The projection, and whether the caller named the fields in it. Both halves read the same + # `fields` param the same way on purpose: an empty `fields[]=` means "every + # column", so it is not named and must take the redaction path rather than a 403. + def self.parse_requested_projection(collection, args) + from_header = parse_projection_from_header(collection, args) + + return { projection: from_header, named_by_caller: true } if from_header + + fields = args.dig(:params, :fields, collection.name) + + { + projection: parse_projection(collection, args), + named_by_caller: !(fields.nil? || fields == '') + } + end + def self.add_polymorphic_type_fields(collection, requested_field_names) polymorphic_relations = collection.schema[:fields].select { |_, field| field.type == 'PolymorphicManyToOne' } diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb index 24c2165f4..032f215bb 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb @@ -15,6 +15,7 @@ module Charts describe Charts do include_context 'with caller' + include_context 'with readable related collections' subject(:chart) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb index 7711a53fd..a06cf9245 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb @@ -11,6 +11,7 @@ module Resources describe Count do include_context 'with caller' + include_context 'with readable related collections' subject(:count) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb index 58390e30c..4fe540fcd 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb @@ -13,6 +13,7 @@ module Resources describe Csv do include_context 'with caller' + include_context 'with readable related collections' subject(:csv) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb index 81dcb1b55..8c55fcac9 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb @@ -14,6 +14,7 @@ module Resources describe List do include_context 'with caller' + include_context 'with readable related collections' subject(:list) { described_class.new } let(:args) do { @@ -168,6 +169,7 @@ module Resources describe List, 'with a projection deeper than one relation' do include_context 'with caller' + include_context 'with readable related collections' subject(:list) { described_class.new } let(:permissions) { instance_double(ForestAdminAgent::Services::Permissions) } diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb index dc160d9f7..cd453934c 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb @@ -14,6 +14,7 @@ module Related describe CsvRelated do include_context 'with caller' + include_context 'with readable related collections' subject(:csv) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb index ab2d270ae..532e65fd4 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb @@ -14,6 +14,7 @@ module Related describe ListRelated do include_context 'with caller' + include_context 'with readable related collections' subject(:list) { described_class.new } let(:args) do { @@ -192,6 +193,7 @@ module Related describe ListRelated, 'with a projection deeper than one relation' do include_context 'with caller' + include_context 'with readable related collections' subject(:list) { described_class.new } let(:permissions) { instance_double(ForestAdminAgent::Services::Permissions) } let(:args) do diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb index cffabf9e2..e51109c26 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb @@ -11,6 +11,7 @@ module Resources describe Show do include_context 'with caller' + include_context 'with readable related collections' subject(:show) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_spec.rb index 47534d9a7..eb1a107a5 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_spec.rb @@ -11,6 +11,7 @@ module Resources describe Update do include_context 'with caller' + include_context 'with readable related collections' subject(:update) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb new file mode 100644 index 000000000..b8d46a32f --- /dev/null +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -0,0 +1,202 @@ +require 'spec_helper' + +# Drives a real Permissions service rather than a double, so the guards themselves are under test +# and not the stubs the route specs install. +module ForestAdminAgent + module Services + include ForestAdminDatasourceToolkit + include ForestAdminDatasourceToolkit::Schema + include ForestAdminDatasourceToolkit::Components::Query + include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + + describe Permissions do + include_context 'with caller' + + let(:datasource) { build_datasource_with_collections(collections) } + let(:cards) { datasource.get_collection('cards') } + + # `cards.holder` is polymorphic: no discriminant travels in the path, so a record may resolve + # to either target and neither can be ruled out. + let(:collections) do + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'pan_last4' => build_column(column_type: 'String'), + 'account_id' => build_column(column_type: 'Number'), + 'holder_id' => build_column(column_type: 'Number'), + 'holder_type' => build_column(column_type: 'String'), + 'account' => build_many_to_one(foreign_collection: 'accounts', foreign_key: 'account_id'), + 'holder' => Relations::PolymorphicManyToOneSchema.new( + foreign_key: 'holder_id', + foreign_key_type_field: 'holder_type', + foreign_collections: %w[persons companies], + foreign_key_targets: { 'persons' => 'id', 'companies' => 'id' } + ) + } + } + ), + build_collection( + name: 'accounts', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'iban' => build_column(column_type: 'String', filter_operators: [Operators::EQUAL]), + 'organization_id' => build_column(column_type: 'Number'), + 'organization' => build_many_to_one( + foreign_collection: 'organizations', foreign_key: 'organization_id' + ) + } + } + ), + build_collection( + name: 'organizations', + schema: { fields: { 'id' => build_numeric_primary_key, 'name' => build_column(column_type: 'String', filter_operators: [Operators::EQUAL]) } } + ), + build_collection( + name: 'persons', + schema: { fields: { 'id' => build_numeric_primary_key, 'national_id' => build_column(column_type: 'String') } } + ), + build_collection( + name: 'companies', + schema: { fields: { 'id' => build_numeric_primary_key, 'siret' => build_column(column_type: 'String') } } + ) + ] + end + + # `cards` is always readable: the route asserts browse or read on it before any of this runs. + def build_permissions(readable) + permissions = described_class.new(caller) + allow(permissions).to receive_messages( + permission_system?: true, + get_user_data: { id: 1, roleId: 7 }, + get_collections_permissions_data: (%w[cards] + readable).to_h { |name| [name.to_sym, { read: [7] }] } + .merge( + (%w[accounts organizations persons companies] - readable) + .to_h { |name| [name.to_sym, { read: [] }] } + ) + ) + + permissions + end + + describe '#redact_projection' do + it 'refuses a field the caller named on a collection it cannot read' do + permissions = build_permissions([]) + + expect do + permissions.redact_projection(cards, Projection.new(%w[id account:iban]), named_by_caller: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You are not allowed to read 'account:iban' from the 'accounts' collection." + ) + end + + it 'names every offending path in one message so a client retries once' do + permissions = build_permissions([]) + + expect do + permissions.redact_projection( + cards, Projection.new(%w[id account:iban account:organization:name]), named_by_caller: true + ) + end.to raise_error(/'account:iban'.+'accounts'.+'account:organization:name'.+'organizations'/) + end + + it 'drops the path instead when the caller never named it' do + permissions = build_permissions([]) + + projection = permissions.redact_projection( + cards, Projection.new(%w[id pan_last4 account:iban]), named_by_caller: false + ) + + expect(projection).to eq(%w[id pan_last4]) + end + + it 'traverses a collection it cannot read to reach a column it can' do + permissions = build_permissions(%w[organizations]) + + projection = permissions.redact_projection( + cards, Projection.new(%w[id account:organization:name]), named_by_caller: true + ) + + expect(projection).to eq(%w[id account:organization:name]) + end + + it 'keeps a polymorphic relation whose every target is readable' do + permissions = build_permissions(%w[persons companies]) + + projection = permissions.redact_projection(cards, Projection.new(%w[id holder:*]), named_by_caller: false) + + expect(projection).to eq(%w[id holder:*]) + end + + it 'refuses a polymorphic relation when a single target is denied' do + permissions = build_permissions(%w[persons]) + + expect do + permissions.redact_projection(cards, Projection.new(%w[id holder:*]), named_by_caller: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You are not allowed to read 'holder:*' from the 'persons' or 'companies' collection." + ) + end + + it 'drops a polymorphic relation with a denied target from the default expansion' do + permissions = build_permissions(%w[persons]) + + projection = permissions.redact_projection( + cards, ProjectionFactory.all(cards), named_by_caller: false + ) + + expect(projection).not_to include('holder:*') + expect(projection).to include('id', 'pan_last4', 'holder_type') + end + end + + describe '#assert_can_read_query_fields' do + def args_with(params) + { headers: { 'HTTP_AUTHORIZATION' => bearer }, params: { 'collection_name' => 'cards' }.merge(params) } + end + + it 'refuses a filter on a collection the caller cannot read' do + permissions = build_permissions([]) + args = args_with(filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json) + + expect { permissions.assert_can_read_query_fields(cards, args) }.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot filter on 'account:iban': you are not allowed to read the 'accounts' collection." + ) + end + + it 'refuses a sort on a collection the caller cannot read' do + permissions = build_permissions([]) + + expect { permissions.assert_can_read_query_fields(cards, args_with(sort: '-account.iban')) } + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot sort on 'account:iban': you are not allowed to read the 'accounts' collection." + ) + end + + it 'leaves the condition tree intact while walking it' do + permissions = build_permissions(%w[accounts]) + args = args_with(filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json) + + permissions.assert_can_read_query_fields(cards, args) + + tree = ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree(cards, args) + expect(tree.field).to eq('account:iban') + end + + it 'accepts a filter once the collection it reaches is readable' do + permissions = build_permissions(%w[accounts]) + args = args_with(filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json) + + expect { permissions.assert_can_read_query_fields(cards, args) }.not_to raise_error + end + end + end + end +end diff --git a/packages/forest_admin_agent/spec/spec_helper.rb b/packages/forest_admin_agent/spec/spec_helper.rb index 91a64768f..da3cd403d 100644 --- a/packages/forest_admin_agent/spec/spec_helper.rb +++ b/packages/forest_admin_agent/spec/spec_helper.rb @@ -32,6 +32,20 @@ let(:caller) { build_caller } end +# Lets a route spec stub the read guards without pinning what they check — the guards themselves are +# exercised against a real Permissions in spec/lib/forest_admin_agent/security. +# +# Worth stubbing rather than leaving unstubbed: RSpec renders an unexpected-message error by +# inspecting the arguments, and a collection reaches its datasource, which reaches every collection, +# so the inspect never finishes and the suite hangs instead of failing. +RSpec.shared_context 'with readable related collections' do + before do + allow(permissions).to receive(:assert_can_read_query_fields) + allow(permissions).to receive(:assert_can_read_usages) + allow(permissions).to receive(:redact_projection) { |_collection, projection, **| projection } + end +end + RSpec.configure do |config| config.include ForestAdminTestToolkit::Factory::Caller config.include ForestAdminTestToolkit::Factory::Collection diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 1f894f18a..68708459e 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -56,6 +56,24 @@ def refine_filter(caller, filter) filter end + # Answers against +@child_collection+, which is what the search actually reads: a field + # hidden by the publication or renaming layers above is still searched. + # + # +nil+ whenever this layer does not choose the fields — a replacer is installed, or the + # child collection searches natively — because then no enumeration made here is true. + def searched_fields(_search, extended) + return nil if @replacer || @child_collection.schema[:searchable] + + get_fields(extended).map do |path, _schema| + { + path: path, + collections: ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names( + @child_collection, path + ) + } + end + end + private def default_replacer(search, extended) diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb index e72573de9..b6ffd92c4 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/decorators/collection_decorator.rb @@ -77,6 +77,14 @@ def render_chart(caller, name, record_id, parameters = {}) @child_collection.render_chart(caller, name, record_id, parameters) end + # Which fields a search will actually reach, and the collection each one ends on. +nil+ means + # the collection cannot say, which a caller must read as "unknown", never as "none". + def searched_fields(search, extended) + return nil unless @child_collection.is_a?(CollectionDecorator) + + @child_collection.searched_fields(search, extended) + end + protected def mark_schema_as_dirty diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb new file mode 100644 index 000000000..43e44c72b --- /dev/null +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb @@ -0,0 +1,43 @@ +module ForestAdminDatasourceToolkit + module Utils + class FieldPath + POLYMORPHIC_MANY_TO_ONE = 'PolymorphicManyToOne'.freeze + + # The collections whose column a path ends on — the ones a read permission applies to. + # Collections crossed on the way are joins, not read targets, so they are not returned. + # + # Several names come back for a path ending on a polymorphic relation: it carries no + # discriminant, so any record may resolve to any of its targets and none can be ruled out. + # + # A prefix naming no relation raises rather than falling back to +collection+. The caller pins + # the collection it asked about to readable, so falling back would turn "this path does not + # resolve" into "this path is allowed". + def self.leaf_collection_names(collection, path) + index = path.index(':') + + return [collection.name] if index.nil? + + relation = relation_at(collection, path[0...index]) + + return relation.foreign_collections if relation.type == POLYMORPHIC_MANY_TO_ONE + + leaf_collection_names( + collection.datasource.get_collection(relation.foreign_collection), + path[(index + 1)..] + ) + end + + def self.relation_at(collection, name) + field = collection.schema[:fields][name] + + if field.nil? || field.type == 'Column' + raise Exceptions::ForestException, "Relation not found: '#{collection.name}.#{name}'" + end + + field + end + + private_class_method :relation_at + end + end +end From 4defd66076ff977e7ca69df58eb1f380ad69fcb1 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 15:21:33 +0200 Subject: [PATCH 02/13] fix(agent): stop treating an absent permission system as a denial MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit read_permissions returned only the root collection, so every related name was missing from the map and read as denied. can? allows everything when no permission system is configured, and this side of the check has to agree: without the fix, any projection through a relation was redacted and any named field 403ed on those deployments. Three more routes serialize a record back with a projection of ours: store, and both sites in update_field. They are redacted like update, through one helper the four of them now share. The search guard no longer parses a search on a collection that has none — parse_search raises there, which turned a parameter the chart routes ignore into a 400. Pins the toolkit path resolver and the search layer's answer, neither of which had a test. --- .../routes/abstract_authenticated_route.rb | 9 ++ .../routes/resources/store.rb | 3 +- .../routes/resources/update.rb | 8 +- .../routes/resources/update_field.rb | 6 +- .../services/permissions.rb | 12 ++- .../routes/resources/store_spec.rb | 1 + .../routes/resources/update_field_spec.rb | 1 + .../security/related_read_permissions_spec.rb | 79 +++++++++++++++ .../decorators/search/searched_fields_spec.rb | 73 ++++++++++++++ .../utils/field_path_spec.rb | 96 +++++++++++++++++++ 10 files changed, 276 insertions(+), 12 deletions(-) create mode 100644 packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb create mode 100644 packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb index abdc1fc55..74af99fe4 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb @@ -12,6 +12,15 @@ def build(args = {}) context end + # For a route that serializes a record back with a projection of its own rather than the + # caller's — a create, an update. Redacted and never refused: a write must not 403 because the + # row it just wrote carries a relation the caller cannot read. + def redacted_full_projection(context) + all = ForestAdminDatasourceToolkit::Components::Query::ProjectionFactory.all(context.collection) + + context.permissions.redact_projection(context.collection, all, named_by_caller: false) + end + def format_attributes(args, collection) record = args[:params][:data][:attributes] || {} diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb index 90dd1a21c..1984a82a1 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/store.rb @@ -25,7 +25,8 @@ def handle_request(args = {}) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTree::ConditionTreeFactory.match_ids(context.collection, [id]) ) - records = context.collection.list(context.caller, filter, ProjectionFactory.all(context.collection)) + projection = redacted_full_projection(context) + records = context.collection.list(context.caller, filter, projection) { name: args[:params]['collection_name'], diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb index 94f4180c5..7b046f685 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update.rb @@ -26,13 +26,7 @@ def handle_request(args = {}) drop_relationships!(args) data = format_attributes(args, context.collection) context.collection.update(context.caller, filter, data) - # The projection is ours, not the caller's, so it is redacted rather than refused: a write - # must not 403 because the row it wrote carries a relation the caller cannot read. - projection = context.permissions.redact_projection( - context.collection, - ProjectionFactory.all(context.collection), - named_by_caller: false - ) + projection = redacted_full_projection(context) records = context.collection.list(context.caller, filter, projection) { diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update_field.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update_field.rb index 443602a4d..1cbaa874f 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update_field.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/update_field.rb @@ -51,7 +51,8 @@ def handle_request(args = {}) ) context.collection.update(context.caller, filter, { field_name => updated_array }) - records = context.collection.list(context.caller, filter, ProjectionFactory.all(context.collection)) + projection = redacted_full_projection(context) + records = context.collection.list(context.caller, filter, projection) { name: args[:params]['collection_name'], @@ -93,7 +94,8 @@ def fetch_record(primary_key_values, context) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTree::ConditionTreeFactory.intersect([condition_tree, scope]) ) - records = context.collection.list(context.caller, filter, ProjectionFactory.all(context.collection)) + projection = redacted_full_projection(context) + records = context.collection.list(context.caller, filter, projection) raise Http::Exceptions::NotFoundError, 'Record not found' unless records&.any? diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index debbb7f65..2b4375d2b 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -65,7 +65,11 @@ def read_permissions(root_collection_name, collection_names) to_check = collection_names.uniq.reject { |name| name == root_collection_name } allowed = { root_collection_name => true } - return allowed if to_check.empty? || !permission_system? + return allowed if to_check.empty? + + # An absent permission system is not a denial: `can?` allows everything there, and answering + # anything else would redact every relation on a deployment that granted nothing to check. + return allowed.merge(to_check.to_h { |name| [name, true] }) unless permission_system? user_data = get_user_data(caller.id) collections_data = get_collections_permissions_data @@ -289,9 +293,13 @@ def read_allowed?(collections_data, collection_name, user_data) # read below the publication and renaming layers, and only that layer knows whether a replacer # or a natively searchable datasource has taken the choice out of its hands. def assert_can_read_search(collection, args, usages) + # Guarded on searchability before parsing: `parse_search` raises for a search on a collection + # that has none, which would turn an ignored parameter into a 400 on the chart routes. + return unless collection.schema[:searchable] && collection.respond_to?(:searched_fields) + search = Utils::QueryStringParser.parse_search(collection, args) - return if search.nil? || !collection.respond_to?(:searched_fields) + return if search.nil? extended = Utils::QueryStringParser.parse_search_extended(args) searched = collection.searched_fields(search, extended) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb index 7038a8255..96202e49f 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb @@ -11,6 +11,7 @@ module Resources describe Store do include_context 'with caller' + include_context 'with readable related collections' subject(:store) { described_class.new } let(:args) do { diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_field_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_field_spec.rb index 1a226d49e..82866d247 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_field_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/update_field_spec.rb @@ -10,6 +10,7 @@ module Resources describe UpdateField do include_context 'with caller' + include_context 'with readable related collections' subject(:update_field) { described_class.new } let(:args) do diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index b8d46a32f..6a9f99b09 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -82,6 +82,35 @@ def build_permissions(readable) permissions end + # `can?` allows everything when no permission system is configured, so this side of the check + # has to agree with it: an absent permission system is not a denial. + describe 'without a permission system' do + it 'keeps every path' do + permissions = described_class.new(caller) + allow(permissions).to receive(:permission_system?).and_return(false) + + projection = permissions.redact_projection( + cards, Projection.new(%w[id account:iban holder:*]), named_by_caller: true + ) + + expect(projection).to eq(%w[id account:iban holder:*]) + end + + it 'refuses no filter' do + permissions = described_class.new(caller) + allow(permissions).to receive(:permission_system?).and_return(false) + args = { + headers: { 'HTTP_AUTHORIZATION' => bearer }, + params: { + 'collection_name' => 'cards', + filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json + } + } + + expect { permissions.assert_can_read_query_fields(cards, args) }.not_to raise_error + end + end + describe '#redact_projection' do it 'refuses a field the caller named on a collection it cannot read' do permissions = build_permissions([]) @@ -190,6 +219,56 @@ def args_with(params) expect(tree.field).to eq('account:iban') end + # The stack is asked what a search reaches, so the check has to be driven by its answer and + # not by anything derived from the schema here. + def searchable_cards(searched) + double = instance_double( + ForestAdminDatasourceToolkit::Decorators::CollectionDecorator, + name: 'cards', + schema: cards.schema.merge(searchable: true), + is_searchable?: true, + datasource: datasource + ) + allow(double).to receive(:searched_fields).and_return(searched) + + double + end + + it 'refuses whatever the stack says the search will reach' do + permissions = build_permissions([]) + collection = searchable_cards([{ path: 'holder:national_id', collections: ['persons'] }]) + + expect { permissions.assert_can_read_query_fields(collection, args_with(search: 'martin')) } + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot search on 'holder:national_id': you are not allowed to read the 'persons' collection." + ) + end + + it 'accepts a search once every collection the stack names is readable' do + permissions = build_permissions(%w[persons]) + collection = searchable_cards([{ path: 'holder:national_id', collections: ['persons'] }]) + + expect { permissions.assert_can_read_query_fields(collection, args_with(search: 'martin')) } + .not_to raise_error + end + + # A replaced search: the handler picks the fields, the caller only supplies the text. + it 'serves the request when the stack cannot say what a search reaches' do + permissions = build_permissions([]) + + expect { permissions.assert_can_read_query_fields(searchable_cards(nil), args_with(search: 'martin')) } + .not_to raise_error + end + + # The chart routes ignore `search`, so parsing it here must not turn it into a 400. + it 'ignores a search on a collection that has none' do + permissions = build_permissions([]) + + expect { permissions.assert_can_read_query_fields(cards, args_with(search: 'martin')) } + .not_to raise_error + end + it 'accepts a filter once the collection it reaches is readable' do permissions = build_permissions(%w[accounts]) args = args_with(filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json) diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb new file mode 100644 index 000000000..6a1835ca5 --- /dev/null +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -0,0 +1,73 @@ +require 'spec_helper' + +module ForestAdminDatasourceCustomizer + module Decorators + module Search + include ForestAdminDatasourceToolkit::Schema + include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + + describe SearchCollectionDecorator do + subject(:decorated) { described_class.new(datasource.get_collection('cards'), datasource) } + + let(:datasource) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'pan_last4' => build_column(column_type: 'String', filter_operators: [Operators::I_CONTAINS]), + 'holder_id' => build_column(column_type: 'Number'), + 'holder' => build_many_to_one(foreign_collection: 'holders', foreign_key: 'holder_id') + } + } + ), + build_collection( + name: 'holders', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'national_id' => build_column(column_type: 'String', filter_operators: [Operators::I_CONTAINS]) + } + } + ) + ] + ) + end + + describe '#searched_fields' do + it 'reports the columns of the collection itself when the search is not extended' do + expect(decorated.searched_fields('martin', false)).to contain_exactly( + { path: 'id', collections: ['cards'] }, + { path: 'pan_last4', collections: ['cards'] } + ) + end + + it 'reports the collection a relation column belongs to when the search is extended' do + expect(decorated.searched_fields('martin', true)).to contain_exactly( + { path: 'id', collections: ['cards'] }, + { path: 'pan_last4', collections: ['cards'] }, + { path: 'holder:id', collections: ['holders'] }, + { path: 'holder:national_id', collections: ['holders'] } + ) + end + + # Reading it as "reaches nothing" would let a replaced search through unchecked. + it 'answers nothing it can be sure of once a replacer chooses the fields' do + decorated.replace_search(->(search, _extended, _context) { { field: 'id', operator: 'equal', value: search } }) + + expect(decorated.searched_fields('martin', true)).to be_nil + end + + it 'answers nothing it can be sure of when the datasource searches natively' do + child = datasource.get_collection('cards') + allow(child).to receive(:schema).and_return(child.schema.merge(searchable: true)) + + expect(described_class.new(child, datasource).searched_fields('martin', true)).to be_nil + end + end + end + end + end +end diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb new file mode 100644 index 000000000..178a412dc --- /dev/null +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb @@ -0,0 +1,96 @@ +require 'spec_helper' + +module ForestAdminDatasourceToolkit + module Utils + include ForestAdminDatasourceToolkit::Schema + + describe FieldPath do + subject(:cards) { datasource.get_collection('cards') } + + let(:datasource) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => ColumnSchema.new(is_primary_key: true, column_type: 'Number'), + 'pan_last4' => ColumnSchema.new(column_type: 'String'), + 'account_id' => ColumnSchema.new(column_type: 'Number'), + 'holder_id' => ColumnSchema.new(column_type: 'Number'), + 'account' => Relations::ManyToOneSchema.new( + foreign_collection: 'accounts', foreign_key: 'account_id', foreign_key_target: 'id' + ), + 'certificate' => Relations::PolymorphicOneToOneSchema.new( + origin_key: 'owner_id', + origin_key_target: 'id', + foreign_collection: 'certificates', + origin_type_field: 'owner_type', + origin_type_value: 'Card' + ), + 'holder' => Relations::PolymorphicManyToOneSchema.new( + foreign_key: 'holder_id', + foreign_key_type_field: 'holder_type', + foreign_collections: %w[persons companies], + foreign_key_targets: { 'persons' => 'id', 'companies' => 'id' } + ) + } + } + ), + build_collection( + name: 'accounts', + schema: { + fields: { + 'id' => ColumnSchema.new(is_primary_key: true, column_type: 'Number'), + 'organization_id' => ColumnSchema.new(column_type: 'Number'), + 'organization' => Relations::ManyToOneSchema.new( + foreign_collection: 'organizations', foreign_key: 'organization_id', foreign_key_target: 'id' + ) + } + } + ), + build_collection(name: 'organizations', schema: { fields: { 'id' => ColumnSchema.new(is_primary_key: true, column_type: 'Number') } }), + build_collection(name: 'certificates', schema: { fields: { 'id' => ColumnSchema.new(is_primary_key: true, column_type: 'Number') } }), + build_collection(name: 'persons', schema: { fields: { 'id' => ColumnSchema.new(is_primary_key: true, column_type: 'Number') } }), + build_collection(name: 'companies', schema: { fields: { 'id' => ColumnSchema.new(is_primary_key: true, column_type: 'Number') } }) + ] + ) + end + + describe '.leaf_collection_names' do + it 'answers the collection itself for one of its own columns' do + expect(described_class.leaf_collection_names(cards, 'pan_last4')).to eq(['cards']) + end + + it 'answers only the collection a path ends on, not the ones it crosses' do + expect(described_class.leaf_collection_names(cards, 'account:organization:id')).to eq(['organizations']) + end + + # No discriminant travels in the path, so a record may resolve to either target. + it 'answers every target of a polymorphic many-to-one' do + expect(described_class.leaf_collection_names(cards, 'holder:*')).to eq(%w[persons companies]) + end + + it 'answers the single target of a polymorphic one-to-one' do + expect(described_class.leaf_collection_names(cards, 'certificate:id')).to eq(['certificates']) + end + + # The caller pins the collection it asked about to readable, so falling back to it would turn + # "this path does not resolve" into "this path is allowed". + it 'raises rather than falling back when the prefix is a column' do + expect { described_class.leaf_collection_names(cards, 'pan_last4:id') }.to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Relation not found: 'cards.pan_last4'" + ) + end + + it 'raises when the prefix names nothing at all' do + expect { described_class.leaf_collection_names(cards, 'unknown:id') }.to raise_error( + ForestAdminDatasourceToolkit::Exceptions::ForestException, + "Relation not found: 'cards.unknown'" + ) + end + end + end + end +end From ea97eff0244b034b46ec7a51a9cfb96bf9f61fdf Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 15:35:28 +0200 Subject: [PATCH 03/13] refactor(agent): drop the comments the code already states Eleven blocks restated a signature, a method name or a doc sitting on the declaration they were calling. The rationale that cannot be read off the code stays. --- .../routes/abstract_authenticated_route.rb | 5 ++--- .../lib/forest_admin_agent/routes/charts/charts.rb | 4 ++-- .../lib/forest_admin_agent/services/permissions.rb | 5 ----- .../lib/forest_admin_agent/utils/csv_generator.rb | 3 --- .../lib/forest_admin_agent/utils/query_string_parser.rb | 6 +++--- .../security/related_read_permissions_spec.rb | 6 ------ .../utils/field_path_spec.rb | 3 --- 7 files changed, 7 insertions(+), 25 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb index 74af99fe4..80f0d052d 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb @@ -12,9 +12,8 @@ def build(args = {}) context end - # For a route that serializes a record back with a projection of its own rather than the - # caller's — a create, an update. Redacted and never refused: a write must not 403 because the - # row it just wrote carries a relation the caller cannot read. + # Never refused: a write must not 403 because the row it just wrote carries a relation the + # caller cannot read. def redacted_full_projection(context) all = ForestAdminDatasourceToolkit::Components::Query::ProjectionFactory.all(context.collection) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb index fd5eed75e..6e188f2fb 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb @@ -233,8 +233,8 @@ def compute_value(context, filter, args) result[0]['value'] || 0 end - # +path_collection+ is what the paths resolve against; the permission root stays the chart's - # own collection, which the leaderboard call site does not share. + # The permission root stays the chart's own collection, which the leaderboard call site does + # not share with the collection its paths resolve against. def assert_can_read_aggregated_fields(context, path_collection, fields) usages = fields.reject { |_action, path| path.nil? || path.to_s.empty? } .map do |action, path| diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index 2b4375d2b..7f7bb5b5f 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -53,8 +53,6 @@ def can?(action, collection, allow_fetch: false) is_allowed end - # Whether the caller may read each of +collection_names+. - # # +root_collection_name+ is pinned to readable and never looked up: +browse+ already gates a # listing, +read+ a get, and the signed hash a chart. # @@ -289,9 +287,6 @@ def read_allowed?(collections_data, collection_name, user_data) check_user_permission(role_ids, user_data, :read, collection_name) end - # Asked of the stack, not derived from the schema: the fields an extended search reaches are - # read below the publication and renaming layers, and only that layer knows whether a replacer - # or a natively searchable datasource has taken the choice out of its hands. def assert_can_read_search(collection, args, usages) # Guarded on searchability before parsing: `parse_search` raises for a search on a collection # that has none, which would turn an ignored parameter into a 400 on the chart routes. diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb index bb751c641..bd05edf77 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb @@ -5,9 +5,6 @@ module Utils class CsvGenerator # Labels are positionally aligned with the requested projection, so dropping a field without # dropping its label shifts every later value under the wrong heading. - # - # Returns +header+ untouched when nothing was dropped, and when the caller sent none: the - # generator then falls back to the projection, which is already the redacted one. def self.filter_header(header, requested, kept) return header if header.nil? || kept.size == requested.size diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb index e967440be..07301f4b0 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/query_string_parser.rb @@ -66,9 +66,9 @@ def self.parse_projection_from_request(collection, args) parse_projection_from_header(collection, args) || parse_projection(collection, args) end - # The projection, and whether the caller named the fields in it. Both halves read the same - # `fields` param the same way on purpose: an empty `fields[]=` means "every - # column", so it is not named and must take the redaction path rather than a 403. + # Both halves read the same `fields` param the same way on purpose: an empty + # `fields[]=` means "every column", so it is not named and must take the redaction + # path rather than a 403. def self.parse_requested_projection(collection, args) from_header = parse_projection_from_header(collection, args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 6a9f99b09..1af8d7269 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -15,8 +15,6 @@ module Services let(:datasource) { build_datasource_with_collections(collections) } let(:cards) { datasource.get_collection('cards') } - # `cards.holder` is polymorphic: no discriminant travels in the path, so a record may resolve - # to either target and neither can be ruled out. let(:collections) do [ build_collection( @@ -82,8 +80,6 @@ def build_permissions(readable) permissions end - # `can?` allows everything when no permission system is configured, so this side of the check - # has to agree with it: an absent permission system is not a denial. describe 'without a permission system' do it 'keeps every path' do permissions = described_class.new(caller) @@ -219,8 +215,6 @@ def args_with(params) expect(tree.field).to eq('account:iban') end - # The stack is asked what a search reaches, so the check has to be driven by its answer and - # not by anything derived from the schema here. def searchable_cards(searched) double = instance_double( ForestAdminDatasourceToolkit::Decorators::CollectionDecorator, diff --git a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb index 178a412dc..d3489718e 100644 --- a/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb +++ b/packages/forest_admin_datasource_toolkit/spec/lib/forest_admin_datasource_toolkit/utils/field_path_spec.rb @@ -66,7 +66,6 @@ module Utils expect(described_class.leaf_collection_names(cards, 'account:organization:id')).to eq(['organizations']) end - # No discriminant travels in the path, so a record may resolve to either target. it 'answers every target of a polymorphic many-to-one' do expect(described_class.leaf_collection_names(cards, 'holder:*')).to eq(%w[persons companies]) end @@ -75,8 +74,6 @@ module Utils expect(described_class.leaf_collection_names(cards, 'certificate:id')).to eq(['certificates']) end - # The caller pins the collection it asked about to readable, so falling back to it would turn - # "this path does not resolve" into "this path is allowed". it 'raises rather than falling back when the prefix is a column' do expect { described_class.leaf_collection_names(cards, 'pan_last4:id') }.to raise_error( ForestAdminDatasourceToolkit::Exceptions::ForestException, From 7873281da741889a1c4b50c217f5700d79a8eb10 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 15:48:54 +0200 Subject: [PATCH 04/13] fix(agent): check only the query components a route applies The guard read filters, sorts and searches on every route, but a count applies no sort and a chart neither sort nor search, so a sort naming a denied collection refused a request that field could never reach. Four routes refused something they drop. Each route now names what it consumes. --- .../routes/charts/charts.rb | 2 +- .../routes/resources/count.rb | 2 +- .../routes/resources/related/csv_related.rb | 2 +- .../routes/resources/related/list_related.rb | 2 +- .../services/permissions.rb | 22 ++++++++++++++----- .../security/related_read_permissions_spec.rb | 12 ++++++++++ 6 files changed, 32 insertions(+), 10 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb index 6e188f2fb..787b78a52 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb @@ -28,7 +28,7 @@ def setup_routes def handle_request(args = {}) context = build(args) context.permissions.can_chart?(args[:params]) - context.permissions.assert_can_read_query_fields(context.collection, args) + context.permissions.assert_can_read_query_fields(context.collection, args, consumes: %i[filter]) type = validate_and_get_type(args[:params][:type]) filter = Filter.new( condition_tree: ConditionTreeFactory.intersect( diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb index cf18cbb25..7729ff6a7 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/count.rb @@ -19,7 +19,7 @@ def handle_request(args = {}) context.permissions.can?(:browse, context.collection) if context.collection.is_countable? - context.permissions.assert_can_read_query_fields(context.collection, args) + context.permissions.assert_can_read_query_fields(context.collection, args, consumes: %i[filter search]) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb index 3757bfd06..0f8ac9bf5 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/csv_related.rb @@ -23,7 +23,7 @@ def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.child_collection) context.permissions.can?(:export, context.child_collection) - context.permissions.assert_can_read_query_fields(context.child_collection, args) + context.permissions.assert_can_read_query_fields(context.child_collection, args, consumes: %i[filter]) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb index 8ab1a5bb4..8b5ae6bc7 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb @@ -23,7 +23,7 @@ def setup_routes def handle_request(args = {}) context = build(args) context.permissions.can?(:browse, context.child_collection) - context.permissions.assert_can_read_query_fields(context.child_collection, args) + context.permissions.assert_can_read_query_fields(context.child_collection, args, consumes: %i[filter sort]) filter = ForestAdminDatasourceToolkit::Components::Query::Filter.new( condition_tree: ConditionTreeFactory.intersect( diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index 7f7bb5b5f..c231dc10d 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -9,6 +9,8 @@ class Permissions include ForestAdminDatasourceToolkit::Exceptions include ForestAdminDatasourceToolkit::Components::Query::ConditionTree + QUERY_COMPONENTS = %i[filter sort search].freeze + attr_reader :caller, :forest_api, :cache def initialize(caller) @@ -106,7 +108,11 @@ def redact_projection(collection, projection, named_by_caller:) # Refused rather than redacted: dropping a condition widens the result set and dropping a sort # clause silently reorders it, while both leak the value they touch anyway — a `starts_with` # filter answers one guess per request without returning a column of its own. - def assert_can_read_query_fields(collection, args) + # + # +consumes+ names the query components the calling route actually applies to its filter. + # Checking one it drops would refuse a request the denied field cannot reach: a count carries + # no sort, a chart neither sort nor search. + def assert_can_read_query_fields(collection, args, consumes: QUERY_COMPONENTS) usages = [] push = lambda do |action, path| usages << { @@ -118,14 +124,18 @@ def assert_can_read_query_fields(collection, args) # `for_each_leaf` on a branch replaces each condition with the block's return value, so the # leaf has to come back out or the tree is rebuilt from whatever `push` returned. - Utils::QueryStringParser.parse_condition_tree(collection, args)&.for_each_leaf do |leaf| - push.call('filter on', leaf.field) - leaf + if consumes.include?(:filter) + Utils::QueryStringParser.parse_condition_tree(collection, args)&.for_each_leaf do |leaf| + push.call('filter on', leaf.field) + leaf + end end - Utils::QueryStringParser.parse_sort(collection, args).each { |clause| push.call('sort on', clause[:field]) } + if consumes.include?(:sort) + Utils::QueryStringParser.parse_sort(collection, args).each { |clause| push.call('sort on', clause[:field]) } + end - assert_can_read_search(collection, args, usages) + assert_can_read_search(collection, args, usages) if consumes.include?(:search) assert_can_read_usages(collection.name, usages) end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 1af8d7269..bbf2b0b87 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -263,6 +263,18 @@ def searchable_cards(searched) .not_to raise_error end + # A count applies no sort, so refusing one would refuse a request the denied field cannot + # reach. Same shape on the chart routes, which apply neither sort nor search. + it 'ignores a query component the route does not apply' do + permissions = build_permissions([]) + + expect do + permissions.assert_can_read_query_fields( + cards, args_with(sort: '-account.iban'), consumes: %i[filter search] + ) + end.not_to raise_error + end + it 'accepts a filter once the collection it reaches is readable' do permissions = build_permissions(%w[accounts]) args = args_with(filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json) From 7daf91516de49fb956f22990ea765f79f3f8dd5e Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:00:32 +0200 Subject: [PATCH 05/13] test(agent): pin how each route wires the read guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The route stubs were installed with `allow` and never inspected, so every wiring decision was unpinned: deleting a guard, passing the route's own collection where the foreign one belongs, widening or narrowing `consumes`, or flipping `named_by_caller` all left the suite green. The shared context now records each call — collection by name, the components the route says it applies, and whether a denial refuses or redacts — and the route specs assert it. `named_by_caller` gets its four cases, and a denied query field is asserted to reach the caller as a 403 rather than a listing. The collection is recorded by name rather than matched on: RSpec renders an argument mismatch by inspecting it, and a collection reaches every collection through its datasource, so the suite hangs instead of failing. Co-Authored-By: Claude Opus 5 --- .../routes/resources/count_spec.rb | 9 +++ .../routes/resources/csv_spec.rb | 10 +++ .../routes/resources/list_spec.rb | 34 +++++++++ .../resources/related/csv_related_spec.rb | 11 +++ .../resources/related/list_related_spec.rb | 15 ++++ .../routes/resources/show_spec.rb | 7 ++ .../routes/resources/store_spec.rb | 3 + .../utils/query_string_parser_spec.rb | 74 +++++++++++++++++++ .../forest_admin_agent/spec/spec_helper.rb | 30 ++++++-- 9 files changed, 186 insertions(+), 7 deletions(-) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb index a06cf9245..fcf2ecc31 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/count_spec.rb @@ -49,6 +49,15 @@ module Resources expect(count.routes.length).to eq 1 end + # A count applies no sort, so checking one would refuse a request the denied field cannot + # reach — while its filter and search are the sharper half and stay checked. + it 'checks only the query components it applies, against its own collection' do + ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count + count.handle_request(args) + + expect(read_guard_calls[:query_fields]).to eq([{ collection: 'user', consumes: %i[filter search] }]) + end + context 'when collection is countable' do it 'return an serialized content' do ForestAdminAgent::Facades::Container.datasource.get_collection('user').enable_count diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb index 4fe540fcd..4e5b24a7e 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/csv_spec.rb @@ -61,6 +61,16 @@ module Resources expect(csv.routes.length).to eq 1 end + it 'checks every query component it applies, against its own collection' do + allow(csv_generator_stream).to receive(:stream).and_return([].to_enum) + csv.handle_request(args) + + expect(read_guard_calls[:query_fields]).to eq( + [{ collection: 'user', consumes: %i[filter sort search] }] + ) + expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: false }]) + end + context 'when call csv' do it 'returns a streaming export csv' do # Create a mock enumerator that yields CSV data diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb index 8c55fcac9..be29ff4ee 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/list_spec.rb @@ -75,6 +75,40 @@ module Resources expect(list.routes.length).to eq 1 end + it 'checks every query component it applies, against its own collection' do + list.handle_request(args) + + expect(read_guard_calls[:query_fields]).to eq( + [{ collection: 'user', consumes: %i[filter sort search] }] + ) + end + + it 'refuses a projection the caller named on its own collection' do + args[:params][:fields] = { 'user' => 'id,first_name' } + list.handle_request(args) + + expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: true }]) + end + + it 'redacts rather than refuses the expansion the caller never asked for' do + list.handle_request(args) + + expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: false }]) + end + + it 'answers a denied query field with a 403 rather than a listing' do + allow(permissions).to receive(:assert_can_read_query_fields).and_raise( + ForestAdminAgent::Http::Exceptions::ForbiddenError.new( + "You cannot filter on 'category:label': you are not allowed to read the 'category' collection." + ) + ) + + expect { list.handle_request(args) }.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + /You cannot filter on 'category:label'/ + ) + end + it 'return an serialized content' do result = list.handle_request(args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb index cd453934c..76bff20eb 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/csv_related_spec.rb @@ -80,6 +80,17 @@ module Related expect(csv.routes.length).to eq 1 end + it 'checks the components it applies against the foreign collection' do + args[:params]['relation_name'] = 'category' + args[:params]['id'] = 1 + allow(ForestAdminDatasourceToolkit::Utils::Collection).to receive(:list_relation).and_return([]) + + csv.handle_request(args) + + expect(read_guard_calls[:query_fields]).to eq([{ collection: 'category', consumes: %i[filter] }]) + expect(read_guard_calls[:projections]).to eq([{ collection: 'category', named_by_caller: false }]) + end + context 'when call csv' do it 'returns a streaming export csv of the related collection' do args[:params]['relation_name'] = 'category' diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb index 532e65fd4..0ee46f8bf 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/related/list_related_spec.rb @@ -80,6 +80,21 @@ module Related expect(list.routes.length).to eq 1 end + # The only routes resolving against the foreign collection rather than their own, and a + # related listing applies no search — so neither is checked against the parent. + it 'checks the components it applies against the foreign collection' do + args[:params]['relation_name'] = 'category' + args[:params]['id'] = 1 + allow(ForestAdminDatasourceToolkit::Utils::Collection).to receive(:list_relation).and_return([]) + + list.handle_request(args) + + expect(read_guard_calls[:query_fields]).to eq( + [{ collection: 'category', consumes: %i[filter sort] }] + ) + expect(read_guard_calls[:projections]).to eq([{ collection: 'category', named_by_caller: false }]) + end + context 'when call without filters' do it 'call list_relation with expected args' do args[:params]['relation_name'] = 'category' diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb index e51109c26..8392e51b8 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/show_spec.rb @@ -74,6 +74,13 @@ def respond_to?(arg) allow(@datasource.get_collection('user')).to receive(:list).and_return([User.new(1, 'foo', 'foo')]) end + it 'redacts the projection it serves against its own collection' do + args[:params]['id'] = 1 + show.handle_request(args) + + expect(read_guard_calls[:projections]).to eq([{ collection: 'user', named_by_caller: false }]) + end + it 'return an serialized content' do args[:params]['id'] = 1 result = show.handle_request(args) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb index 96202e49f..8bb9571d2 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/resources/store_spec.rb @@ -100,6 +100,9 @@ def respond_to?(arg) 'links' => { 'self' => '/forest/book/1' } } ) + # Redacted, never refused: a write must not 403 because the row it just wrote carries a + # relation the caller cannot read. Same helper serves `update` and `update_field`. + expect(read_guard_calls[:projections]).to eq([{ collection: 'book', named_by_caller: false }]) end it 'includes null attributes in the returned result' do diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb index b4eae8dba..cd7ee0144 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/query_string_parser_spec.rb @@ -712,6 +712,80 @@ module Utils end end + describe 'parse_requested_projection' do + let(:collection) do + datasource = Datasource.new + collection_person = Collection.new(datasource, 'Person') + collection_person.add_fields( + { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), + 'name' => ColumnSchema.new(column_type: 'String') + } + ) + collection = Collection.new(datasource, 'Book') + collection.add_fields( + { + 'id' => ColumnSchema.new(column_type: 'Number', is_primary_key: true), + 'title' => ColumnSchema.new(column_type: 'String'), + 'author_id' => ColumnSchema.new(column_type: 'Number'), + 'author' => Relations::ManyToOneSchema.new( + foreign_key: 'author_id', + foreign_key_target: 'id', + foreign_collection: 'Person' + ) + } + ) + + datasource.add_collection(collection) + datasource.add_collection(collection_person) + + return collection + end + + it 'reports a projection header as named by the caller' do + args = { headers: { 'HTTP_FOREST_PROJECTION' => 'title,author:name' }, params: {} } + + expect(described_class.parse_requested_projection(collection, args)).to eq( + { projection: Projection.new(%w[title author:name]), named_by_caller: true } + ) + end + + it 'reports a fields parameter as named by the caller' do + args = { headers: {}, params: { fields: { 'Book' => 'id,title' } } } + + expect(described_class.parse_requested_projection(collection, args)).to eq( + { projection: Projection.new(%w[id title]), named_by_caller: true } + ) + end + + it 'reports an absent fields parameter as not named by the caller' do + args = { headers: {}, params: {} } + + expect(described_class.parse_requested_projection(collection, args)).to eq( + { projection: Projection.new(%w[id title author_id author:id author:name]), named_by_caller: false } + ) + end + + it 'reports an empty fields parameter as not named by the caller' do + args = { headers: {}, params: { fields: { 'Book' => '' } } } + + expect(described_class.parse_requested_projection(collection, args)).to eq( + { projection: Projection.new(%w[id title author_id author:id author:name]), named_by_caller: false } + ) + end + + it 'prefers the header over a fields parameter naming other columns' do + args = { + headers: { 'HTTP_FOREST_PROJECTION' => 'title' }, + params: { fields: { 'Book' => '' } } + } + + expect(described_class.parse_requested_projection(collection, args)).to eq( + { projection: Projection.new(%w[title]), named_by_caller: true } + ) + end + end + describe 'parse_projection_with_pks' do let(:collection) do datasource = Datasource.new diff --git a/packages/forest_admin_agent/spec/spec_helper.rb b/packages/forest_admin_agent/spec/spec_helper.rb index da3cd403d..ff19a71c4 100644 --- a/packages/forest_admin_agent/spec/spec_helper.rb +++ b/packages/forest_admin_agent/spec/spec_helper.rb @@ -32,17 +32,33 @@ let(:caller) { build_caller } end -# Lets a route spec stub the read guards without pinning what they check — the guards themselves are -# exercised against a real Permissions in spec/lib/forest_admin_agent/security. +# Stubs the read guards for a route spec and records how the route wired them, so a spec can pin the +# collection each guard resolves against, the query components the route says it applies, and whether +# a denial is a 403 or a redaction. The guards themselves are exercised against a real Permissions in +# spec/lib/forest_admin_agent/security. # -# Worth stubbing rather than leaving unstubbed: RSpec renders an unexpected-message error by -# inspecting the arguments, and a collection reaches its datasource, which reaches every collection, -# so the inspect never finishes and the suite hangs instead of failing. +# The collection is recorded by name rather than matched on: RSpec renders an argument mismatch by +# inspecting it, and a collection reaches its datasource, which reaches every collection, so the +# inspect never finishes and the suite hangs instead of failing. Leaving a guard unstubbed hangs the +# same way. RSpec.shared_context 'with readable related collections' do + let(:read_guard_calls) { { query_fields: [], projections: [] } } + before do - allow(permissions).to receive(:assert_can_read_query_fields) + allow(permissions).to receive(:assert_can_read_query_fields) do |collection, _args, **options| + read_guard_calls[:query_fields] << { + collection: collection.name, + consumes: options.fetch(:consumes, ForestAdminAgent::Services::Permissions::QUERY_COMPONENTS) + } + end allow(permissions).to receive(:assert_can_read_usages) - allow(permissions).to receive(:redact_projection) { |_collection, projection, **| projection } + allow(permissions).to receive(:redact_projection) do |collection, projection, **options| + read_guard_calls[:projections] << { + collection: collection.name, + named_by_caller: options[:named_by_caller] + } + projection + end end end From 16784e723ad83e91dab4de3c1dcb4b74717f4aa0 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:00:42 +0200 Subject: [PATCH 06/13] fix(agent): gate a count leaderboard on browse whether it names an empty field or none MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `aggregateFieldName=""` walked past both halves of the gate. The aggregate usage was dropped as empty, so no `read` check ran on the foreign collection, while `aggregation.field.nil?` was false for `""`, so the `browse` fallback never ran either — leaving a `Count` leaderboard with no check at all on the collection it counts. `can_chart?` cannot tell the two apart: both hash the same, since `sanitize_chart_parameters` drops an empty value before hashing. The parameter is now normalised where it is read, so the aggregation and the guards that read it agree on what a count is. Co-Authored-By: Claude Opus 5 --- .../routes/charts/charts.rb | 25 ++++-- .../routes/charts/charts_spec.rb | 84 +++++++++++++++++++ 2 files changed, 101 insertions(+), 8 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb index 787b78a52..32d4117c5 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb @@ -48,6 +48,15 @@ def handle_request(args = {}) private + # An empty `aggregateFieldName` is a count, and `can_chart?` cannot tell it from an absent + # one — `sanitize_chart_parameters` drops both before hashing. Normalising here keeps the + # aggregation and the guards that read it from disagreeing on what a count is. + def aggregate_field_name(args) + field = args[:params][:aggregateFieldName] + + field.nil? || field.to_s.empty? ? nil : field + end + def validate_and_get_type(type) chart_types = %w[Value Objective Pie Line Leaderboard] unless chart_types.include?(type) @@ -92,11 +101,11 @@ def make_pie(context, filter, args) group_field = args[:params][:groupByFieldName] assert_can_read_aggregated_fields( context, context.collection, - [['group a chart by', group_field], ['aggregate a chart on', args[:params][:aggregateFieldName]]] + [['group a chart by', group_field], ['aggregate a chart on', aggregate_field_name(args)]] ) aggregation = Aggregation.new( operation: args[:params][:aggregator], - field: args[:params][:aggregateFieldName], + field: aggregate_field_name(args), groups: group_field ? [{ field: group_field }] : [] ) @@ -110,7 +119,7 @@ def make_line(context, filter, args) assert_can_read_aggregated_fields( context, context.collection, [['group a chart by', group_by_field_name], - ['aggregate a chart on', args[:params][:aggregateFieldName]]] + ['aggregate a chart on', aggregate_field_name(args)]] ) time_range = args[:params][:timeRange] filter_only_with_values = filter.override( @@ -126,7 +135,7 @@ def make_line(context, filter, args) filter_only_with_values, Aggregation.new( operation: args[:params][:aggregator], - field: args[:params][:aggregateFieldName], + field: aggregate_field_name(args), groups: [{ field: group_by_field_name, operation: time_range }] ) ) @@ -161,7 +170,7 @@ def make_leaderboard(context, filter, args) leaderboard_filter = filter.nest(inverse) aggregation = Aggregation.new( operation: args[:params][:aggregator], - field: args[:params][:aggregateFieldName], + field: aggregate_field_name(args), groups: [{ field: "#{inverse}:#{args[:params][:labelFieldName]}" }] ) end @@ -181,7 +190,7 @@ def make_leaderboard(context, filter, args) leaderboard_filter = filter.nest(origin) aggregation = Aggregation.new( operation: args[:params][:aggregator], - field: args[:params][:aggregateFieldName] ? "#{target}:#{args[:params][:aggregateFieldName]}" : nil, + field: aggregate_field_name(args) ? "#{target}:#{aggregate_field_name(args)}" : nil, groups: [{ field: "#{origin}:#{args[:params][:labelFieldName]}" }] ) end @@ -224,10 +233,10 @@ def make_leaderboard(context, filter, args) def compute_value(context, filter, args) assert_can_read_aggregated_fields( context, context.collection, - [['aggregate a chart on', args[:params][:aggregateFieldName]]] + [['aggregate a chart on', aggregate_field_name(args)]] ) aggregation = Aggregation.new(operation: args[:params][:aggregator], - field: args[:params][:aggregateFieldName]) + field: aggregate_field_name(args)) result = context.collection.aggregate(context.caller, filter, aggregation) result[0]['value'] || 0 diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb index 032f215bb..62f7d37ec 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/routes/charts/charts_spec.rb @@ -130,6 +130,23 @@ module Charts expect(chart.routes.length).to eq 1 end + # A chart applies neither sort nor search, so checking either would refuse a request the + # denied field cannot reach. + it 'checks only the filter, against its own collection' do + args[:params] = args[:params].merge({ + aggregateFieldName: 'price', + aggregator: 'Sum', + sourceCollectionName: 'book', + type: 'Value', + timezone: 'Europe/Paris' + }) + allow(@datasource.get_collection('book')).to receive(:aggregate).and_return([{ 'value' => 10, 'group' => [] }]) + + chart.handle_request(args) + + expect(read_guard_calls[:query_fields]).to eq([{ collection: 'book', consumes: %i[filter] }]) + end + it 'throw an error when request has a bad chart type' do args[:params][:type] = 'unknown_type' @@ -489,6 +506,73 @@ module Charts ) end + # A count names no path back to the collection it counts, so nothing above sees it. An empty + # `aggregateFieldName` is a count too: `can_chart?` cannot tell it from an absent one. + it 'asserts browse on the counted collection when a OneToMany count names no field' do + args[:params] = args[:params].merge({ + labelFieldName: 'author', + relationshipFieldName: 'bookReviews', + aggregator: 'Count', + aggregateFieldName: '', + sourceCollectionName: 'book', + type: 'Leaderboard', + timezone: 'Europe/Paris' + }) + allow(permissions).to receive(:can?) + allow(@datasource.get_collection('book')).to receive(:datasource).and_return(@datasource) + allow(@datasource.get_collection('review')).to receive(:aggregate).and_return([]) + + chart.handle_request(args) + + expect(permissions).to have_received(:can?) do |action, collection| + expect(action).to eq(:browse) + expect(collection.name).to eq('review') + end + end + + it 'asserts browse on the foreign collection when a ManyToMany count names no field' do + args[:params] = args[:params].merge({ + labelFieldName: 'year', + relationshipFieldName: 'reviews', + aggregator: 'Count', + sourceCollectionName: 'book', + type: 'Leaderboard', + timezone: 'Europe/Paris' + }) + allow(permissions).to receive(:can?) + allow(@datasource.get_collection('book')).to receive(:datasource).and_return(@datasource) + allow(@datasource.get_collection('book_review')).to receive(:aggregate).and_return([]) + + chart.handle_request(args) + + expect(permissions).to have_received(:can?) do |action, collection| + expect(action).to eq(:browse) + expect(collection.name).to eq('review') + end + end + + it 'refuses a count leaderboard the caller cannot browse the counted collection for' do + args[:params] = args[:params].merge({ + labelFieldName: 'author', + relationshipFieldName: 'bookReviews', + aggregator: 'Count', + aggregateFieldName: '', + sourceCollectionName: 'book', + type: 'Leaderboard', + timezone: 'Europe/Paris' + }) + allow(permissions).to receive(:can?).and_raise( + ForestAdminAgent::Http::Exceptions::ForbiddenError.new( + "You don't have permission to browse this collection." + ) + ) + allow(@datasource.get_collection('book')).to receive(:datasource).and_return(@datasource) + + expect { chart.handle_request(args) }.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError + ) + end + it 'throw a ForestException when the request is not filled correctly' do args[:params] = args[:params].merge({ relationshipFieldName: 'unknown_relation', From 5e621db577c021610dd34ffb38c0857460c78f80 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:00:42 +0200 Subject: [PATCH 07/13] fix(customizer): report only the fields a search term will actually reach MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `searched_fields` discarded the term and returned everything `get_fields` yields, while `refine_filter` builds a condition only where `build_condition` does — skipping a number column for a word, a uuid column for anything but a uuid, an enum unless the term matches a value. Extended search therefore 403'd naming a collection the search provably could not have touched, which makes the feature unusable for a restricted role on a common schema shape. The enumeration now runs through the same selection, and a search the stack discards as insignificant reaches nothing rather than everything. Co-Authored-By: Claude Opus 5 --- .../search/search_collection_decorator.rb | 11 +++++++++-- .../decorators/search/searched_fields_spec.rb | 17 ++++++++++++++++- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 68708459e..5e8b15a18 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -59,12 +59,19 @@ def refine_filter(caller, filter) # Answers against +@child_collection+, which is what the search actually reads: a field # hidden by the publication or renaming layers above is still searched. # + # The term is run through the same +build_condition+ selection +refine_filter+ applies, so a + # field the search cannot match — a number column for a word, a uuid column for anything + # else — is left out rather than reported as reached. + # # +nil+ whenever this layer does not choose the fields — a replacer is installed, or the # child collection searches natively — because then no enumeration made here is true. - def searched_fields(_search, extended) + def searched_fields(search, extended) return nil if @replacer || @child_collection.schema[:searchable] + return [] if search.nil? || search.strip.empty? + + get_fields(extended).filter_map do |path, schema| + next unless build_condition(path, schema, search) - get_fields(extended).map do |path, _schema| { path: path, collections: ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names( diff --git a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb index 6a1835ca5..24142426e 100644 --- a/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb +++ b/packages/forest_admin_datasource_customizer/spec/lib/forest_admin_datasource_customizer/decorators/search/searched_fields_spec.rb @@ -39,13 +39,24 @@ module Search describe '#searched_fields' do it 'reports the columns of the collection itself when the search is not extended' do expect(decorated.searched_fields('martin', false)).to contain_exactly( - { path: 'id', collections: ['cards'] }, { path: 'pan_last4', collections: ['cards'] } ) end it 'reports the collection a relation column belongs to when the search is extended' do expect(decorated.searched_fields('martin', true)).to contain_exactly( + { path: 'pan_last4', collections: ['cards'] }, + { path: 'holder:national_id', collections: ['holders'] } + ) + end + + it 'leaves out a column the term cannot match, which the search builds no condition for' do + expect(decorated.searched_fields('martin', true).map { |field| field[:path] }) + .not_to include('id', 'holder:id') + end + + it 'reports a numeric column once the term is a number the search will compare it to' do + expect(decorated.searched_fields('42', true)).to contain_exactly( { path: 'id', collections: ['cards'] }, { path: 'pan_last4', collections: ['cards'] }, { path: 'holder:id', collections: ['holders'] }, @@ -53,6 +64,10 @@ module Search ) end + it 'reaches nothing for a search the stack discards as insignificant' do + expect(decorated.searched_fields(' ', true)).to eq([]) + end + # Reading it as "reaches nothing" would let a replaced search through unchecked. it 'answers nothing it can be sure of once a replacer chooses the fields' do decorated.replace_search(->(search, _extended, _context) { { field: 'id', operator: 'equal', value: search } }) From 2e06e09752760ef8a9a963bf3dfe713d3ab80293 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:01:03 +0200 Subject: [PATCH 08/13] fix(agent): tell an unpublished leaf from a denied one, and deny one that resolves to nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three answers the read guard was giving wrong, all in the same lookup. An extended search 403'd for every role, admins included, on any datasource built with `remove_collection` or `add_datasource(exclude:)`. `searched_fields` answers below publication — deliberately, so a field hidden by renaming is still checked — but a collection the datasource stopped publishing is absent from the permission payload, and `read_allowed?` has no permissionLevel bypass to soften that. It is unpublished, not denied: no route exposes it, and the search targets are now filtered against what the datasource publishes. A `PolymorphicManyToOne` declaring no `foreign_collections` — which Rails does not require alongside `belongs_to polymorphic: true` — resolved to an empty list, and `[].all?` allowed it unconditionally. An empty list is a path that resolves to nothing, not one that needs nothing, so it counts as denied: the default expansion drops it, a caller who names it gets a 403. And a denial no longer refetches the whole environment. Denial is the steady state here, so refetching on each cost a permission fetch and a shared-cache eviction per page load, two to three times over. Read permissions are answered once per collection per request, and refetched only for a collection the payload does not know — the one denial a stale cache explains, with `refresh-roles` on the SSE channel already evicting on a role change. Also fixes the example that claimed to guard the condition tree the guard walks: it asserted a freshly parsed tree, so it could not fail. It now pins the branch that was walked, which is where `for_each_leaf` rebuilds. Co-Authored-By: Claude Opus 5 --- .../services/permissions.rb | 71 +++++++--- .../security/related_read_permissions_spec.rb | 129 +++++++++++++++++- .../utils/field_path.rb | 11 +- 3 files changed, 187 insertions(+), 24 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index c231dc10d..437aca372 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -58,9 +58,8 @@ def can?(action, collection, allow_fetch: false) # +root_collection_name+ is pinned to readable and never looked up: +browse+ already gates a # listing, +read+ a get, and the signed hash a chart. # - # One cached pass for the whole request, and a single refetch only if it denied something — - # unlike +can?+, which refetches on every denial. Denial is the steady state here rather than - # the exception, so refetching per collection would cost one permission fetch per request. + # Answered once per collection for the whole request: +redact_projection+ and + # +assert_can_read_query_fields+ both ask on a listing, and a chart asks three times. def read_permissions(root_collection_name, collection_names) to_check = collection_names.uniq.reject { |name| name == root_collection_name } allowed = { root_collection_name => true } @@ -71,16 +70,11 @@ def read_permissions(root_collection_name, collection_names) # anything else would redact every relation on a deployment that granted nothing to check. return allowed.merge(to_check.to_h { |name| [name, true] }) unless permission_system? - user_data = get_user_data(caller.id) - collections_data = get_collections_permissions_data - results = to_check.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } - - unless results.values.all? - collections_data = get_collections_permissions_data(force_fetch: true) - results = to_check.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } - end + @read_permissions ||= {} + missing = to_check - @read_permissions.keys + @read_permissions.merge!(fetch_read_permissions(missing)) unless missing.empty? - allowed.merge(results) + allowed.merge(@read_permissions.slice(*to_check)) end # An unnamed field is dropped rather than refused: the default expansion covers every column @@ -91,13 +85,13 @@ def redact_projection(collection, projection, named_by_caller:) [path, ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names(collection, path)] end allowed = read_permissions(collection.name, owners.values.flatten) - readable = ->(path) { owners[path].all? { |name| allowed[name] } } + readable = ->(path) { readable_leaves?(owners[path], allowed) } if named_by_caller denied = projection.reject { |path| readable.call(path) } unless denied.empty? - fields = denied.map { |path| "'#{path}' from the '#{owners[path].join("' or '")}' collection" } + fields = denied.map { |path| "'#{path}' from #{leaf_label(owners[path])}" } raise ForbiddenError, "You are not allowed to read #{fields.join(", ")}." end end @@ -141,13 +135,13 @@ def assert_can_read_query_fields(collection, args, consumes: QUERY_COMPONENTS) def assert_can_read_usages(root_collection_name, usages) allowed = read_permissions(root_collection_name, usages.flat_map { |usage| usage[:collections] }) - denied = usages.find { |usage| !usage[:collections].all? { |name| allowed[name] } } + denied = usages.find { |usage| !readable_leaves?(usage[:collections], allowed) } return unless denied raise ForbiddenError, - "You cannot #{denied[:action]} '#{denied[:path]}': you are not allowed to read the " \ - "'#{denied[:collections].join("' or '")}' collection." + "You cannot #{denied[:action]} '#{denied[:path]}': you are not allowed to read " \ + "#{leaf_label(denied[:collections])}." end def can_chart?(parameters) @@ -285,6 +279,35 @@ def get_team(rendering_id) private + # An empty list of leaves resolves to no collection at all — a polymorphic relation declaring + # no `foreign_collections`. `[].all?` would allow it unconditionally, which is the one answer + # this guard must never give by default, so it counts as denied. + def readable_leaves?(names, allowed) + names.any? && names.all? { |name| allowed[name] } + end + + def leaf_label(names) + names.empty? ? 'an unresolved polymorphic relation' : "the '#{names.join("' or '")}' collection" + end + + # A denial is refetched only when the collection is absent from the payload, the one denial a + # stale cache explains. A role that simply lacks `read` is the steady state here, and + # `refresh-roles` on the SSE channel already evicts the cache when a role changes — refetching + # on every denial would cost a permission fetch, and a cache eviction every other in-flight + # request reads through, on each page load. + def fetch_read_permissions(names) + user_data = get_user_data(caller.id) + collections_data = get_collections_permissions_data + results = names.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } + + return results if @read_permissions_refetched || names.all? { |name| collections_data.key?(name.to_sym) } + + @read_permissions_refetched = true + collections_data = get_collections_permissions_data(force_fetch: true) + + names.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } + end + def read_allowed?(collections_data, collection_name, user_data) return false unless user_data_valid?(user_data) @@ -309,8 +332,18 @@ def assert_can_read_search(collection, args, usages) extended = Utils::QueryStringParser.parse_search_extended(args) searched = collection.searched_fields(search, extended) - searched&.each do |field| - usages << { action: 'search on', path: field[:path], collections: field[:collections] } + return if searched.nil? + + published = collection.datasource.collections + + searched.each do |field| + # `searched_fields` answers below the publication layer, so a target the datasource stopped + # publishing is absent from the permission payload. That is unpublished, not denied: no + # route exposes it and looking it up would refuse the search for every role, admins + # included. + targets = field[:collections].select { |name| published.key?(name) } + + usages << { action: 'search on', path: field[:path], collections: targets } unless targets.empty? end end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index bbf2b0b87..45da9b391 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -168,6 +168,74 @@ def build_permissions(readable) ) end + it 'leaves nothing for `with_pks` to re-add once the redaction emptied a relation' do + permissions = build_permissions([]) + + projection = permissions.redact_projection( + cards, Projection.new(%w[id account:iban]), named_by_caller: false + ).with_pks(cards) + + expect(projection).to eq(%w[id]) + end + + # The key of a collection the caller cannot read stays, because the serializer needs it to + # emit the readable column behind it — dropping it would take the permitted path down too. + it 'keeps the key `with_pks` needs to serialize a path the redaction kept' do + permissions = build_permissions(%w[organizations]) + + projection = permissions.redact_projection( + cards, Projection.new(%w[id account:organization:name]), named_by_caller: true + ).with_pks(cards) + + expect(projection).to include('account:id', 'account:organization:id') + end + + context 'when a polymorphic relation declares no target at all' do + let(:orphan_cards) do + build_datasource_with_collections( + [ + build_collection( + name: 'cards', + schema: { + fields: { + 'id' => build_numeric_primary_key, + 'holder_id' => build_column(column_type: 'Number'), + 'holder_type' => build_column(column_type: 'String'), + 'holder' => Relations::PolymorphicManyToOneSchema.new( + foreign_key: 'holder_id', + foreign_key_type_field: 'holder_type', + foreign_collections: [], + foreign_key_targets: {} + ) + } + } + ) + ] + ).get_collection('cards') + end + + it 'refuses the path the caller named rather than allowing what resolves to nothing' do + permissions = build_permissions([]) + + expect do + permissions.redact_projection(orphan_cards, Projection.new(%w[id holder:*]), named_by_caller: true) + end.to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You are not allowed to read 'holder:*' from an unresolved polymorphic relation." + ) + end + + it 'drops the path from the default expansion' do + permissions = build_permissions([]) + + projection = permissions.redact_projection( + orphan_cards, Projection.new(%w[id holder:*]), named_by_caller: false + ) + + expect(projection).to eq(%w[id]) + end + end + it 'drops a polymorphic relation with a denied target from the default expansion' do permissions = build_permissions(%w[persons]) @@ -205,14 +273,25 @@ def args_with(params) ) end - it 'leaves the condition tree intact while walking it' do + # `for_each_leaf` on a branch replaces each condition with the block's return value, so the + # tree the guard walked is the one asserted on — a freshly parsed one could not catch it. + it 'leaves the branch it walked intact instead of rebuilding it from the guard' do permissions = build_permissions(%w[accounts]) - args = args_with(filters: { field: 'account:iban', operator: 'equal', value: 'FR76' }.to_json) + args = args_with( + filters: { + aggregator: 'and', + conditions: [ + { field: 'account:iban', operator: 'equal', value: 'FR76' }, + { field: 'id', operator: 'equal', value: 1 } + ] + }.to_json + ) + tree = ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree(cards, args) + allow(ForestAdminAgent::Utils::QueryStringParser).to receive(:parse_condition_tree).and_return(tree) permissions.assert_can_read_query_fields(cards, args) - tree = ForestAdminAgent::Utils::QueryStringParser.parse_condition_tree(cards, args) - expect(tree.field).to eq('account:iban') + expect(tree.conditions.map(&:field)).to eq(%w[account:iban id]) end def searchable_cards(searched) @@ -247,6 +326,17 @@ def searchable_cards(searched) .not_to raise_error end + # Below publication, so the permission payload has never heard of it: unpublished, not denied. + it 'ignores a search target the datasource stopped publishing' do + permissions = build_permissions([]) + collection = searchable_cards([{ path: 'account:iban', collections: ['accounts'] }]) + published = datasource.collections.except('accounts') + allow(datasource).to receive(:collections).and_return(published) + + expect { permissions.assert_can_read_query_fields(collection, args_with(search: 'martin')) } + .not_to raise_error + end + # A replaced search: the handler picks the fields, the caller only supplies the text. it 'serves the request when the stack cannot say what a search reaches' do permissions = build_permissions([]) @@ -282,6 +372,37 @@ def searchable_cards(searched) expect { permissions.assert_can_read_query_fields(cards, args) }.not_to raise_error end end + + describe '#read_permissions' do + it 'answers a collection once for the whole request' do + permissions = build_permissions(%w[accounts]) + + permissions.read_permissions('cards', %w[accounts]) + permissions.read_permissions('cards', %w[accounts]) + + expect(permissions).to have_received(:get_collections_permissions_data).once + end + + # Denial is the steady state here: a role that simply lacks `read` must not cost a permission + # fetch, and a cache eviction every other in-flight request reads through, on each page load. + it 'does not refetch for a denial the cached payload already accounts for' do + permissions = build_permissions([]) + + expect(permissions.read_permissions('cards', %w[accounts])).to eq( + { 'cards' => true, 'accounts' => false } + ) + expect(permissions).not_to have_received(:get_collections_permissions_data).with(force_fetch: true) + end + + it 'refetches once for the whole request when the payload does not know a collection' do + permissions = build_permissions([]) + + permissions.read_permissions('cards', %w[not_in_payload]) + permissions.read_permissions('cards', %w[another_one_missing]) + + expect(permissions).to have_received(:get_collections_permissions_data).with(force_fetch: true).once + end + end end end end diff --git a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb index 43e44c72b..6fc54bf16 100644 --- a/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb +++ b/packages/forest_admin_datasource_toolkit/lib/forest_admin_datasource_toolkit/utils/field_path.rb @@ -7,11 +7,20 @@ class FieldPath # Collections crossed on the way are joins, not read targets, so they are not returned. # # Several names come back for a path ending on a polymorphic relation: it carries no - # discriminant, so any record may resolve to any of its targets and none can be ruled out. + # discriminant, so any record may resolve to any of its targets and none can be ruled out. The + # list is empty for a polymorphic relation declaring no target at all, which the caller counts + # as denied — an empty list is a path that resolves to nothing, not a path that needs nothing. # # A prefix naming no relation raises rather than falling back to +collection+. The caller pins # the collection it asked about to readable, so falling back would turn "this path does not # resolve" into "this path is allowed". + # + # A path with no prefix at all is a column of +collection+ itself, and so is allowed. Two + # customisations reach a related collection through one deliberately: +import_field+, and an + # +add_field+ whose dependencies cross a relation, both register a root-level column that the + # operator and sorting layers rewrite into a relation path below this guard. Like a scope or a + # +replace_search+, they are the customer's own choice of what a role reaches through a column + # it can read. def self.leaf_collection_names(collection, path) index = path.index(':') From 8387992ecea3d37feadab0093febd286c0452a4b Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:01:04 +0200 Subject: [PATCH 09/13] fix(agent): keep the csv header aligned with the columns actually exported MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `filter_header` only JSON-parsed, and the front sends the labels comma-joined, so it returned the header untouched on the shape that matters — and `CsvGeneratorStream.parse_header` then failed to parse it too and silently replaced the caller's labels with raw field paths. Alignment survived by accident, and the rescue swallowed the only signal. Where it did engage it dropped the wrong labels: indexing into the expanded projection assumes one path per label, which a to-one relation expanded into several paths, or an appended polymorphic type field, breaks. Both halves now read the same shapes and agree: a header carrying one label per exported column is honoured and filtered by position, and one that cannot be mapped onto the columns falls back to the field paths rather than mislabelling them. Co-Authored-By: Claude Opus 5 --- .../forest_admin_agent/utils/csv_generator.rb | 22 +++++++--- .../utils/csv_generator_stream.rb | 21 +++------ .../utils/csv_generator_spec.rb | 43 +++++++++++++++++++ .../utils/csv_generator_stream_spec.rb | 12 ++++++ 4 files changed, 78 insertions(+), 20 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb index bd05edf77..0a64e1b26 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator.rb @@ -5,22 +5,34 @@ module Utils class CsvGenerator # Labels are positionally aligned with the requested projection, so dropping a field without # dropping its label shifts every later value under the wrong heading. + # The header carries one label per exported column, so a label survives exactly when the + # projection path at its index did. A count that does not match the projection cannot be mapped + # onto columns — a to-one relation expanded into several paths, or a polymorphic type field + # appended — so the header is handed back untouched and the stream falls back to field paths. def self.filter_header(header, requested, kept) return header if header.nil? || kept.size == requested.size - labels = header.is_a?(String) ? parse_header_labels(header) : header + labels = parse_header_labels(header) - return header unless labels.is_a?(Array) + return header unless labels&.size == requested.size labels.each_with_index .select { |_label, index| kept.include?(requested[index]) } .map(&:first) end + # The front joins the labels with commas; a JSON array comes from older callers. def self.parse_header_labels(header) - JSON.parse(header) - rescue JSON::ParserError - nil + return header if header.is_a?(Array) + return nil unless header.is_a?(String) + + parsed = begin + JSON.parse(header) + rescue JSON::ParserError + nil + end + + parsed.is_a?(Array) ? parsed : header.split(',', -1) end def self.generate(records, projection) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb index bc01cbab2..b4127145f 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb @@ -58,23 +58,14 @@ def self.stream(header, filter, projection, list_records, limit_export_size = ni end # Parse header parameter into an array - # @param header [String, Array, nil] Header as JSON string, array, or nil - # @param projection [Projection] Field projection (fallback if header is invalid) - # @return [Array] Header fields + # @param header [String, Array, nil] Header as a comma-joined string, JSON array, array or nil + # @param projection [Projection] Field projection + # @return [Array] The labels when there is exactly one per exported column, the field + # paths otherwise — a header that cannot be mapped onto the columns would mislabel them def self.parse_header(header, projection) - case header - when Array - header - when String - return projection.to_a if header.empty? + labels = CsvGenerator.parse_header_labels(header) - JSON.parse(header) - else - projection.to_a - end - rescue JSON::ParserError - # Fallback to projection if JSON parsing fails - projection.to_a + labels&.size == projection.size ? labels : projection.to_a end # Generate CSV row from record data diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb index dd1c038de..57b1d5ace 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb @@ -72,6 +72,49 @@ module Utils "id,last_name,first_name,email,active,created_at,updated_at,address\n1,Skywalker,Luke,luke@sw.com,true,2024-05-21T00:00:00.000Z,2024-05-21T00:00:00.000Z,Tatooine\n2,Solo,Han,han@sw.com,true,2024-05-21T00:00:00.000Z,2024-05-21T00:00:00.000Z,Corellia\n3,Organa,Leia,leia@sw.com,true,2024-05-21T00:00:00.000Z,2024-05-21T00:00:00.000Z,Alderaan\n4,Kenobi,Obi-Wan,obiwan@sw.com,false,2024-05-21T00:00:00.000Z,2024-05-21T00:00:00.000Z,Stewjon\n" end + describe 'filter_header' do + let(:requested) { Projection.new(%w[id last_name address:planet]) } + + it 'hands the header back untouched when the redaction dropped nothing' do + expect(described_class.filter_header('Id,Last name,Planet', requested, requested)) + .to eq('Id,Last name,Planet') + end + + it 'returns nothing when the caller sent no header' do + expect(described_class.filter_header(nil, requested, Projection.new(%w[id]))).to be_nil + end + + it 'drops the label of a path the redaction removed from a comma-joined header' do + kept = Projection.new(%w[id last_name]) + + expect(described_class.filter_header('Id,Last name,Planet', requested, kept)) + .to eq(['Id', 'Last name']) + end + + it 'drops the label of a path the redaction removed from a JSON header' do + kept = Projection.new(%w[id address:planet]) + + expect(described_class.filter_header('["Id","Last name","Planet"]', requested, kept)) + .to eq(%w[Id Planet]) + end + + it 'drops the label of a path the redaction removed from an array of labels' do + kept = Projection.new(%w[address:planet]) + + expect(described_class.filter_header(['Id', 'Last name', 'Planet'], requested, kept)) + .to eq(['Planet']) + end + + it 'hands back a header that carries no label for each exported column' do + # `fields[users]=address,id` expands `address` into two paths, so no label sits at the + # index of a path and dropping one by position would mislabel the rest. + expanded = Projection.new(%w[address:planet address:city id]) + + expect(described_class.filter_header('Address,Id', expanded, Projection.new(%w[id]))) + .to eq('Address,Id') + end + end + describe 'generate' do it 'generates a CSV string' do csv = described_class.generate(records, projection) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb index 4ab550568..55a29cc57 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb @@ -60,6 +60,18 @@ module Utils expect(csv_output).to start_with("id,first_name,last_name\n") end + + it 'labels the columns from a comma-joined header, which is what the front sends' do + enumerator = described_class.stream('Id,First name,Last name', filter, projection, list_records) + + expect(enumerator.to_a.join).to start_with("Id,First name,Last name\n") + end + + it 'uses the projection when the header carries no label for each exported column' do + enumerator = described_class.stream('Id,First name', filter, projection, list_records) + + expect(enumerator.to_a.join).to start_with("id,first_name,last_name\n") + end end it 'streams CSV data with header and records' do From 4b0c231cb138747c8451fefd787ff57a9a12c810 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:01:04 +0200 Subject: [PATCH 10/13] refactor(agent): resolve a redacted projection and its keys in one place `list`, `show` and `list_related` each parsed the requested projection, redacted it and re-added the primary keys. One helper now does it, and carries why `with_pks` runs after the redaction rather than before: it only re-adds keys for relations the redaction kept a path through, and the serializer needs those keys to emit the readable column behind them, so dropping them would take the permitted path down too. A relation the redaction emptied contributes no path, so nothing is re-added for it. Co-Authored-By: Claude Opus 5 --- .../routes/abstract_authenticated_route.rb | 12 ++++++++++++ .../lib/forest_admin_agent/routes/resources/list.rb | 7 +------ .../routes/resources/related/list_related.rb | 9 +-------- .../lib/forest_admin_agent/routes/resources/show.rb | 7 +------ 4 files changed, 15 insertions(+), 20 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb index 80f0d052d..56447849c 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/abstract_authenticated_route.rb @@ -20,6 +20,18 @@ def redacted_full_projection(context) context.permissions.redact_projection(context.collection, all, named_by_caller: false) end + # +with_pks+ runs after the redaction on purpose. It only re-adds keys for relations the + # redaction kept a path through, and the serializer needs those keys to emit the readable + # column behind them — dropping them would take the permitted path down with them. A relation + # the redaction emptied contributes no path, so nothing is re-added for it. + def redacted_projection_with_pks(context, collection, args) + requested = Utils::QueryStringParser.parse_requested_projection(collection, args) + + context.permissions.redact_projection( + collection, requested[:projection], named_by_caller: requested[:named_by_caller] + ).with_pks(collection) + end + def format_attributes(args, collection) record = args[:params][:data][:attributes] || {} diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb index 730f77004..38742c6c0 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/list.rb @@ -34,12 +34,7 @@ def handle_request(args = {}) segment: QueryStringParser.parse_segment(context.collection, args) ) - requested = QueryStringParser.parse_requested_projection(context.collection, args) - projection = context.permissions.redact_projection( - context.collection, - requested[:projection], - named_by_caller: requested[:named_by_caller] - ).with_pks(context.collection) + projection = redacted_projection_with_pks(context, context.collection, args) records = context.collection.list(context.caller, filter, projection) { diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb index 8b5ae6bc7..8cc817602 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/related/list_related.rb @@ -35,14 +35,7 @@ def handle_request(args = {}) page: ForestAdminAgent::Utils::QueryStringParser.parse_pagination(args), sort: ForestAdminAgent::Utils::QueryStringParser.parse_sort(context.child_collection, args) ) - requested = ForestAdminAgent::Utils::QueryStringParser.parse_requested_projection( - context.child_collection, args - ) - projection = context.permissions.redact_projection( - context.child_collection, - requested[:projection], - named_by_caller: requested[:named_by_caller] - ).with_pks(context.child_collection) + projection = redacted_projection_with_pks(context, context.child_collection, args) primary_key_values = Utils::Id.unpack_id(context.collection, args[:params]['id'], with_key: true) records = Collection.list_relation( context.collection, diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb index 927e119e6..00d342cd2 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/routes/resources/show.rb @@ -25,12 +25,7 @@ def handle_request(args = {}) condition_tree: ConditionTree::ConditionTreeFactory.intersect([condition_tree, scope]) ) - requested = QueryStringParser.parse_requested_projection(context.collection, args) - projection = context.permissions.redact_projection( - context.collection, - requested[:projection], - named_by_caller: requested[:named_by_caller] - ).with_pks(context.collection) + projection = redacted_projection_with_pks(context, context.collection, args) records = context.collection.list(context.caller, filter, projection) From a453cf181d89fb71322d1c2c877d2776761d1f5a Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:53:53 +0200 Subject: [PATCH 11/13] fix(agent): keep the read refetch where the cache has no channel keeping it fresh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Narrowing the refetch to a collection missing from the payload dropped the case that needed it most. `instant_cache_refresh` defaults to production only, so outside it `SSECacheInvalidation` never runs and nothing invalidates `forest.collections` — a role just granted `read` would have stayed redacted for up to `permission_expiration`, which is exactly the loop a developer iterating on permissions sits in. The refetch is now gated on that flag, which is the gate node puts its own behind: no channel, so a denial is worth one fetch; channel up, so `refresh-roles` evicts on a role change and only a collection the payload has never heard of is. Both sides pinned, and an absent flag reads as no channel. Also stops the search guard reading two meanings into an empty target list. A list the publication filter emptied has nothing left to check; one that arrived empty is a relation resolving to nothing, and stays denied like everywhere else. Co-Authored-By: Claude Opus 5 --- .../services/permissions.rb | 36 ++++++++++++++----- .../security/related_read_permissions_spec.rb | 35 ++++++++++++++++-- 2 files changed, 59 insertions(+), 12 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index 437aca372..c818325e9 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -290,22 +290,36 @@ def leaf_label(names) names.empty? ? 'an unresolved polymorphic relation' : "the '#{names.join("' or '")}' collection" end - # A denial is refetched only when the collection is absent from the payload, the one denial a - # stale cache explains. A role that simply lacks `read` is the steady state here, and - # `refresh-roles` on the SSE channel already evicts the cache when a role changes — refetching - # on every denial would cost a permission fetch, and a cache eviction every other in-flight - # request reads through, on each page load. def fetch_read_permissions(names) user_data = get_user_data(caller.id) collections_data = get_collections_permissions_data results = names.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } - return results if @read_permissions_refetched || names.all? { |name| collections_data.key?(name.to_sym) } + return results if results.values.all? || !refetch_denied_reads?(names, collections_data) @read_permissions_refetched = true - collections_data = get_collections_permissions_data(force_fetch: true) + refetched = get_collections_permissions_data(force_fetch: true) - names.to_h { |name| [name, read_allowed?(collections_data, name, user_data)] } + names.to_h { |name| [name, read_allowed?(refetched, name, user_data)] } + end + + # `can?` refetches the whole environment on every denial. Denial is the steady state of this + # check rather than the exception, so paying that per denial would cost a permission fetch, and + # a cache eviction every other in-flight request reads through, on each page load. + # + # Two things make a denial worth one fetch. Without `instant_cache_refresh` nothing else keeps + # the cache fresh — this is the gate node puts its own refetch behind — so a role granted `read` + # would otherwise stay redacted until the cache expires. With the channel up, `refresh-roles` + # evicts on a role change, and the only denial staleness still explains is a collection the + # payload has never heard of. + def refetch_denied_reads?(names, collections_data) + return false if @read_permissions_refetched + + !instant_cache_refresh? || names.any? { |name| !collections_data.key?(name.to_sym) } + end + + def instant_cache_refresh? + Facades::Container.config_from_cache[:instant_cache_refresh] == true end def read_allowed?(collections_data, collection_name, user_data) @@ -343,7 +357,11 @@ def assert_can_read_search(collection, args, usages) # included. targets = field[:collections].select { |name| published.key?(name) } - usages << { action: 'search on', path: field[:path], collections: targets } unless targets.empty? + # Only a list the filter emptied has nothing left to check. One that arrived empty is a + # relation that resolves to nothing, and stays denied like everywhere else. + next if targets.empty? && field[:collections].any? + + usages << { action: 'search on', path: field[:path], collections: targets } end end diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 45da9b391..79849ba35 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -374,7 +374,13 @@ def searchable_cards(searched) end describe '#read_permissions' do + def with_instant_cache_refresh(enabled) + config = ForestAdminAgent::Facades::Container.config_from_cache.merge(instant_cache_refresh: enabled) + allow(ForestAdminAgent::Facades::Container).to receive(:config_from_cache).and_return(config) + end + it 'answers a collection once for the whole request' do + with_instant_cache_refresh(true) permissions = build_permissions(%w[accounts]) permissions.read_permissions('cards', %w[accounts]) @@ -383,9 +389,11 @@ def searchable_cards(searched) expect(permissions).to have_received(:get_collections_permissions_data).once end - # Denial is the steady state here: a role that simply lacks `read` must not cost a permission - # fetch, and a cache eviction every other in-flight request reads through, on each page load. - it 'does not refetch for a denial the cached payload already accounts for' do + # Denial is the steady state here, and `refresh-roles` on the SSE channel already evicts on a + # role change: a role that simply lacks `read` must not cost a permission fetch, and a cache + # eviction every other in-flight request reads through, on each page load. + it 'does not refetch for a denial the cached payload accounts for while the channel is up' do + with_instant_cache_refresh(true) permissions = build_permissions([]) expect(permissions.read_permissions('cards', %w[accounts])).to eq( @@ -394,7 +402,19 @@ def searchable_cards(searched) expect(permissions).not_to have_received(:get_collections_permissions_data).with(force_fetch: true) end + # `instant_cache_refresh` defaults to production only, so outside it nothing else keeps the + # cache fresh and a newly granted `read` would stay redacted until the cache expires. + it 'refetches a denial when no channel keeps the cache fresh' do + with_instant_cache_refresh(false) + permissions = build_permissions([]) + + permissions.read_permissions('cards', %w[accounts]) + + expect(permissions).to have_received(:get_collections_permissions_data).with(force_fetch: true).once + end + it 'refetches once for the whole request when the payload does not know a collection' do + with_instant_cache_refresh(true) permissions = build_permissions([]) permissions.read_permissions('cards', %w[not_in_payload]) @@ -402,6 +422,15 @@ def searchable_cards(searched) expect(permissions).to have_received(:get_collections_permissions_data).with(force_fetch: true).once end + + it 'answers without a fetch once the permission system is absent' do + permissions = described_class.new(caller) + allow(permissions).to receive_messages(permission_system?: false) + + expect(permissions.read_permissions('cards', %w[accounts])).to eq( + { 'cards' => true, 'accounts' => true } + ) + end end end end From 01a8e98394a46d7faff0350bef393d53eaea7686 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 11:53:53 +0200 Subject: [PATCH 12/13] fix(agent): escape the csv header row now that the caller's labels reach it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `join(",")` was safe while the header was always field paths, which carry no commas. Honouring the caller's labels makes a label with a comma or a quote emit a header row wider than the data under it, so the header goes through `CSV.generate_line` like every other row. Also pins the branch of `filter_header` that had no example: a JSON header the expanded projection has outgrown. That is the case where filtering by position dropped the wrong label rather than doing nothing, and the comma-joined examples could not catch it — the previous code failed to parse those and returned them untouched by accident. Co-Authored-By: Claude Opus 5 --- .../forest_admin_agent/utils/csv_generator_stream.rb | 5 +++-- .../forest_admin_agent/utils/csv_generator_spec.rb | 11 +++++++++++ .../utils/csv_generator_stream_spec.rb | 6 ++++++ 3 files changed, 20 insertions(+), 2 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb index b4127145f..b3431b3ba 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/utils/csv_generator_stream.rb @@ -16,8 +16,9 @@ class CsvGeneratorStream def self.stream(header, filter, projection, list_records, limit_export_size = nil) Enumerator.new do |yielder| # Yield header row first (client receives immediately) - header_array = parse_header(header, projection) - yielder << "#{header_array.join(",")}\n" + # Escaped like any other row: the labels are the caller's, and one carrying a comma or a + # quote would otherwise emit a header row wider than the data under it. + yielder << CSV.generate_line(parse_header(header, projection)) offset = 0 diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb index 57b1d5ace..ca5607895 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_spec.rb @@ -105,6 +105,17 @@ module Utils .to eq(['Planet']) end + # `fields[books]=author,id,title` with `fields[author]=firstName,lastName` expands into four + # paths against three labels: filtering by position drops "Id" and keeps "Title", leaving one + # label for two data columns, under the wrong name. + it 'hands back a JSON header the expanded projection has outgrown' do + expanded = Projection.new(%w[author:firstName author:lastName id title]) + kept = Projection.new(%w[id title]) + + expect(described_class.filter_header('["Author","Id","Title"]', expanded, kept)) + .to eq('["Author","Id","Title"]') + end + it 'hands back a header that carries no label for each exported column' do # `fields[users]=address,id` expands `address` into two paths, so no label sits at the # index of a path and dropping one by position would mislabel the rest. diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb index 55a29cc57..63ff9ef46 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/utils/csv_generator_stream_spec.rb @@ -67,6 +67,12 @@ module Utils expect(enumerator.to_a.join).to start_with("Id,First name,Last name\n") end + it 'escapes a label carrying a comma instead of widening the header row' do + enumerator = described_class.stream(['Id', 'Last, first', 'Name'], filter, projection, list_records) + + expect(enumerator.to_a.join).to start_with(%(Id,"Last, first",Name\n)) + end + it 'uses the projection when the header carries no label for each exported column' do enumerator = described_class.stream('Id,First name', filter, projection, list_records) From dd1689677f5f074883331b4ae6e65cd19eb18d53 Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Mon, 24 Aug 2026 12:08:29 +0200 Subject: [PATCH 13/13] fix(agent): refuse a search reaching a collection the agent does not expose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Filtering the unpublished targets out of the check removed the only guard on that path, and the path is real: `refine_filter` builds `account:iban i_contains '…'` below publication, which has already passed the filter down and cannot strip it. So an extended search still matches on a column of a collection `remove_collection` took out of the API — verified against a built stack, where the published schema of `cards` no longer carries `account` at all. Discarding the check left that as a presence oracle, which is the shape this whole change exists to close. The requirement it was meant to answer was to tell the two apart, not to serve the search: a collection absent from the permission payload is unexposed, and no grant can lift it, so answering "you are not allowed to read it" points a customer at a permission that does not exist. It is now refused as what it is, naming the collection, which points at `disable_search` or publishing it again. Both messages are pinned, so neither can collapse into the other. The real fix is upstream and does not fit here: search should not traverse a relation whose foreign collection is unpublished. It sits below publication in `init_stack`, so it cannot see what was removed — that needs a stack change. Also splits the two functions qlty flagged at complexity 6. Co-Authored-By: Claude Opus 5 --- .../services/permissions.rb | 33 ++++++++++++------- .../security/related_read_permissions_spec.rb | 24 +++++++++++--- .../search/search_collection_decorator.rb | 30 +++++++++++------ 3 files changed, 61 insertions(+), 26 deletions(-) diff --git a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb index c818325e9..c189aed07 100644 --- a/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb +++ b/packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb @@ -343,26 +343,35 @@ def assert_can_read_search(collection, args, usages) return if search.nil? - extended = Utils::QueryStringParser.parse_search_extended(args) - searched = collection.searched_fields(search, extended) + searched = collection.searched_fields(search, Utils::QueryStringParser.parse_search_extended(args)) return if searched.nil? published = collection.datasource.collections searched.each do |field| - # `searched_fields` answers below the publication layer, so a target the datasource stopped - # publishing is absent from the permission payload. That is unpublished, not denied: no - # route exposes it and looking it up would refuse the search for every role, admins - # included. - targets = field[:collections].select { |name| published.key?(name) } + assert_search_target_exposed(field, published) + usages << { action: 'search on', path: field[:path], collections: field[:collections] } + end + end + + # `searched_fields` answers below the publication layer — deliberately, so a field hidden by + # renaming above it is still checked — so it can name a collection `remove_collection` took out + # of the API. An extended search does reach through to it: the condition is built below + # publication, which has already passed the filter down and cannot strip it. + # + # That collection is absent from the permission payload too, so asking `read_allowed?` answers + # "denied" for every role, admins included, and no grant can lift it. So it is refused as what + # it is — a column the agent does not expose — which points at `disable_search` or publishing + # the collection again rather than at a permission to grant. + def assert_search_target_exposed(field, published) + unexposed = field[:collections].reject { |name| published.key?(name) } - # Only a list the filter emptied has nothing left to check. One that arrived empty is a - # relation that resolves to nothing, and stays denied like everywhere else. - next if targets.empty? && field[:collections].any? + return if unexposed.empty? - usages << { action: 'search on', path: field[:path], collections: targets } - end + raise ForbiddenError, + "You cannot search on '#{field[:path]}': the '#{unexposed.join("' or '")}' collection " \ + 'is not exposed by this agent.' end def permission_allowed?(collections_data, collection, action, user_data) diff --git a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb index 79849ba35..f00329ff4 100644 --- a/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb +++ b/packages/forest_admin_agent/spec/lib/forest_admin_agent/security/related_read_permissions_spec.rb @@ -326,15 +326,31 @@ def searchable_cards(searched) .not_to raise_error end - # Below publication, so the permission payload has never heard of it: unpublished, not denied. - it 'ignores a search target the datasource stopped publishing' do - permissions = build_permissions([]) + # An extended search reaches a removed collection: the condition is built below publication, + # which has already passed the filter down. Refused as unexposed rather than as denied — no + # role can be granted `read` on a collection absent from the permission payload. + it 'refuses a search reaching a collection the agent does not expose' do + permissions = build_permissions(%w[accounts]) collection = searchable_cards([{ path: 'account:iban', collections: ['accounts'] }]) published = datasource.collections.except('accounts') allow(datasource).to receive(:collections).and_return(published) expect { permissions.assert_can_read_query_fields(collection, args_with(search: 'martin')) } - .not_to raise_error + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot search on 'account:iban': the 'accounts' collection is not exposed by this agent." + ) + end + + it 'tells an unexposed collection apart from one the role cannot read' do + permissions = build_permissions([]) + collection = searchable_cards([{ path: 'account:iban', collections: ['accounts'] }]) + + expect { permissions.assert_can_read_query_fields(collection, args_with(search: 'martin')) } + .to raise_error( + ForestAdminAgent::Http::Exceptions::ForbiddenError, + "You cannot search on 'account:iban': you are not allowed to read the 'accounts' collection." + ) end # A replaced search: the handler picks the fields, the caller only supplies the text. diff --git a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb index 5e8b15a18..775d34567 100644 --- a/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb +++ b/packages/forest_admin_datasource_customizer/lib/forest_admin_datasource_customizer/decorators/search/search_collection_decorator.rb @@ -66,23 +66,33 @@ def refine_filter(caller, filter) # +nil+ whenever this layer does not choose the fields — a replacer is installed, or the # child collection searches natively — because then no enumeration made here is true. def searched_fields(search, extended) - return nil if @replacer || @child_collection.schema[:searchable] - return [] if search.nil? || search.strip.empty? + return nil unless enumerable_search? + return [] if insignificant_search?(search) get_fields(extended).filter_map do |path, schema| - next unless build_condition(path, schema, search) - - { - path: path, - collections: ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names( - @child_collection, path - ) - } + searched_field(path) if build_condition(path, schema, search) end end private + def enumerable_search? + @replacer.nil? && !@child_collection.schema[:searchable] + end + + def insignificant_search?(search) + search.nil? || search.strip.empty? + end + + def searched_field(path) + { + path: path, + collections: ForestAdminDatasourceToolkit::Utils::FieldPath.leaf_collection_names( + @child_collection, path + ) + } + end + def default_replacer(search, extended) searchable_fields = get_fields(extended)