From 30cda05616f14b5442c186d094d45454841c797c Mon Sep 17 00:00:00 2001 From: Pierre Merlet Date: Fri, 21 Aug 2026 14:00:45 +0200 Subject: [PATCH 1/4] 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 2/4] 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 3/4] 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 4/4] 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)