Skip to content

feat: add option to skip rbac - #1879

Merged
arnaud-moncel merged 2 commits into
mainfrom
feat/skip-rbac-option
Sep 7, 2026
Merged

feat: add option to skip rbac#1879
arnaud-moncel merged 2 commits into
mainfrom
feat/skip-rbac-option

Conversation

@arnaud-moncel

@arnaud-moncel arnaud-moncel commented Sep 4, 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 skipRelationReadPermissions option to bypass relation-path read checks in AuthorizationService

  • Adds the optional skipRelationReadPermissions boolean to AgentOptions (default false). When true, the agent skips read-permission checks on fields reached through relation paths — projections, filters, sorts, searches, and chart groupings — while authorization on the queried collection still runs.
  • AuthorizationService gains the constructor parameter and early-returns in redactProjection, assertCanReadQueryFields, and assertCanReadUsages when the option is enabled. The chart route in chart.ts also skips browse-permission checks on the counted collection for Count leaderboards.
  • The capabilities endpoint advertises checksRelationReadPermissions (the inverse of the option) so internal consumers know whether relation-path checks are active.
  • OptionsValidator.withDefaults logs a warning at startup when the option is enabled.
  • Behavioral Change: enabling skipRelationReadPermissions means clients can read, filter, sort, and search through relations they lack browse/read permission on. Root collection authorization (list, get-one, export) is unaffected and still enforced.

Macroscope summarized e3684fd.

@qltysh

qltysh Bot commented Sep 4, 2026

Copy link
Copy Markdown

1 new issue

Tool Category Rule Count
qlty Structure Function with many returns (count = 4): fetchCapabilities 1

@qltysh

qltysh Bot commented Sep 4, 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
packages/agent/src/services/authorization/authorization.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/services/authorization/index.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/routes/access/chart.ts100.0%
Coverage rating: A Coverage rating: A
packages/agent/src/utils/options-validator.ts100.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.

@matthv
matthv self-requested a review September 4, 2026 12:36
@matthv

matthv commented Sep 4, 2026

Copy link
Copy Markdown
Member

packages/agent/src/routes/access/chart.ts:262-263 — the leaderboard chart's count-only path (assertCanBrowse on field.foreignCollection, no aggregation.field) isn't gated by skipRelationReadPermissions, unlike assertCanReadAggregatedFields right above it. So a Count leaderboard over a relation still 403s with the flag on.

The option's JSDoc ("The route's own browse/read/export check on the collection being queried is unaffected") doesn't quite cover this either — the surviving check here is on the related collection, not the one being queried.

Intentional (mirroring the pre-#1840 behavior for this one shape) or worth gating too?

@matthv

matthv commented Sep 4, 2026

Copy link
Copy Markdown
Member

A couple more things from a review pass:

  • Nothing in related-read-permissions.test.ts proves the primary collection's browse/read/export check still holds with skipRelationReadPermissions: true — that boundary is the whole security claim of the option. buildServices's canOnCollection mock always allows cards, so a denied-primary-collection case is never exercised. Worth a test asserting List/CsvRelated still reject when the root collection is denied, flag on.
  • The new default (skipRelationReadPermissions: false) isn't asserted in http-driver-options.test.ts's 'should add default values when they are missing', unlike maxRecordsForApproval/auditTrail when they landed.

Minor: no Warn log on boot when the option is on (existing pattern for permissionsCacheDurationInSeconds/skipSchemaUpdate) — would help an auditor spot the weakened posture from the boot log.

@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.

LGTM

@arnaud-moncel
arnaud-moncel merged commit 9b51ebc into main Sep 7, 2026
33 checks passed
@arnaud-moncel
arnaud-moncel deleted the feat/skip-rbac-option branch September 7, 2026 13:22
forest-bot added a commit that referenced this pull request Sep 7, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants