Skip to content

feat: add option to skip rbac - #388

Merged
arnaud-moncel merged 4 commits into
mainfrom
feat/rbac-capa
Sep 10, 2026
Merged

arnaud-moncel merged 4 commits into
mainfrom
feat/rbac-capa

Conversation

@arnaud-moncel

@arnaud-moncel arnaud-moncel commented Sep 9, 2026

Copy link
Copy Markdown
Member

Definition of Done

General

  • Write an explicit title for the Pull Request, following Conventional Commits specification
  • Test manually the implemented changes
  • Validate the code quality (indentation, syntax, style, simplicity, readability)

Security

  • Consider the security impact of the changes made

Note

Add skip_relation_read_permissions option to bypass relation-read checks

  • Adds a new skip_relation_read_permissions config setting (default false) to ForestAdminRails, ForestAdminRpcAgent, and the agent setup options.
  • When true, Permissions#read_permissions returns all non-root collections as readable without fetching permission data, and assert_extended_search_checkable no longer refuses extended searches whose reached fields cannot be determined.
  • Charts relation count leaderboards skip the foreign collection browse permission check, and the capabilities payload reports checksRelationReadPermissions: false.
  • AgentFactory logs a Warn-level message at boot when the option is enabled.
  • Risk: enabling this option serves unread related collection columns through relation paths, weakening the read-permission posture. Root-collection browse checks in permission_allowed? are unaffected.

Changes since #388 opened

  • Reordered private methods within ForestAdminAgent::Routes::Capabilities::Collections class [ef511d4]

Macroscope summarized 4db058b.

@qltysh

qltysh Bot commented Sep 9, 2026

Copy link
Copy Markdown

2 new issues

Tool Category Rule Count
qlty Structure Function with high complexity (count = 5): read_permissions 1
qlty Structure Function with many returns (count = 4): assert_extended_search_checkable 1

@qltysh

qltysh Bot commented Sep 9, 2026

Copy link
Copy Markdown

Qlty


Coverage Impact

This PR will not change total coverage.

Modified Files with Diff Coverage (4)

RatingFile% DiffUncovered Line #s
Coverage rating: A Coverage rating: A
...st_admin_agent/lib/forest_admin_agent/builder/agent_factory.rb100.0%
Coverage rating: A Coverage rating: A
...gent/lib/forest_admin_agent/routes/capabilities/collections.rb100.0%
Coverage rating: A Coverage rating: A
...est_admin_agent/lib/forest_admin_agent/services/permissions.rb100.0%
Coverage rating: A Coverage rating: A
...est_admin_agent/lib/forest_admin_agent/routes/charts/charts.rb100.0%
Total100.0%
🚦 See full report on Qlty Cloud »

🛟 Help
  • Diff Coverage: Coverage for added or modified lines of code (excludes deleted files). Learn more.

  • Total Coverage: Coverage for the whole repository, calculated as the sum of all File Coverage. Learn more.

  • File Coverage: Covered Lines divided by Covered Lines plus Missed Lines. (Excludes non-executable lines including blank lines and comments.)

    • Indirect Changes: Changes to File Coverage for files that were not modified in this PR. Learn more.

Comment thread packages/forest_admin_agent/lib/forest_admin_agent/services/permissions.rb Outdated
@matthv
matthv self-requested a review September 9, 2026 14:40
@matthv

matthv commented Sep 9, 2026

Copy link
Copy Markdown
Member

Reviewed against the agent-nodejs#1879 port — solid overall (direct-collection auth stays enforced, boot-safe default, frontend contract matches). One point worth a look before merge:

  • skip_relation_read_permissions is a class-level attribute on Services::Permissions, shared process-wide. packages/_examples/demo/Gemfile already loads both forest_admin_rails and forest_admin_rpc_agent in one process — each is its own singleton, but both write into the same class attribute on #setup. Whichever boots last wins for both, silently (no conflict detection/warning). An operator setting false on the main agent can end up overridden to true by a co-loaded RPC agent, or vice versa.
  • assert_search_target_exposed isn't gated by the flag (introduced by the same fix(agent): serve only the columns of collections the caller may read #365-equivalent work this option is meant to opt back out of) — a search reaching an unpublished collection still 403s even with the flag on. Might be intentional (schema exposure vs. permission), but it's not stated anywhere.
  • Exposing the setting on forest_admin_rpc_agent fires the boot warning even though no RPC route actually calls Services::Permissions — on a standalone RPC process it only matters via the shared-state point above.

Minor: the subclass-singleton rationale comment is duplicated verbatim across agent_factory_spec.rb, charts_spec.rb and related_read_permissions_spec.rb — could live once at the source.

@matthv

matthv commented Sep 9, 2026

Copy link
Copy Markdown
Member

Test proving the shared-flag issue above — currently red (expected false, got true), same warning log fires for the wrong agent:

    describe '#setup' do
      # Agent < AgentFactory: Ruby's Singleton gives each class its own #instance, so the main
      # agent and this one never share an object — but #setup writes skip_relation_read_permissions
      # onto Services::Permissions, a single class shared by the whole process. A host running
      # both in one process (e.g. this gem alongside forest_admin_rails, as the demo example app
      # does) has whichever #setup ran last decide the setting for both agents, silently.
      after { ForestAdminAgent::Services::Permissions.skip_relation_read_permissions = false }

      let(:options) do
        {
          auth_secret: 'cba803d01a4d43b55010cab41fa1ea1f1f51a95e',
          env_secret: '89719c6d8e2e2de2694c2f220fe2dbf02d5289487364daf1e4c6b13733ed0cdb',
          is_production: false
        }
      end

      it 'does not let this agent override the main agent skip_relation_read_permissions setting' do
        ForestAdminAgent::Builder::AgentFactory.instance.setup(options.merge(skip_relation_read_permissions: false))
        expect(ForestAdminAgent::Services::Permissions.skip_relation_read_permissions?).to be false

        instance.setup(options.merge(skip_relation_read_permissions: true))

        expect(ForestAdminAgent::Services::Permissions.skip_relation_read_permissions?).to be false
      end
    end

Drops into packages/forest_admin_rpc_agent/spec/lib/forest_admin_rpc_agent/agent_spec.rb, inside the top-level describe Agent do. Rest of the file (27 examples) stays green — only this one fails.

@matthv matthv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok for me.
You have to move the method skip_relation_read_permissions before or after audit_trail_enabled?

@matthv matthv left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@arnaud-moncel
arnaud-moncel merged commit 3894104 into main Sep 10, 2026
33 checks passed
@arnaud-moncel
arnaud-moncel deleted the feat/rbac-capa branch September 10, 2026 08:11
forest-bot added a commit that referenced this pull request Sep 10, 2026
# [1.43.0](v1.42.0...v1.43.0) (2026-09-10)

### Features

* add option to skip rbac ([#388](#388)) ([3894104](3894104))
@forest-bot

Copy link
Copy Markdown
Member

🎉 This PR is included in version 1.43.0 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants