feat(analytics): presentation_ro read-only ClickHouse role (#1963) - #2019
Conversation
Second barrier behind the single-SELECT gate (#1962): contract reads run under a role that makes writing the source impossible by construction. - presentation-role.sql: SELECT on the contract (silver, identity/person, legacy gold in `insight`); SELECT/INSERT/CREATE only in `presentation`; no DROP/ALTER/TRUNCATE anywhere. Idempotent. - apply-ch-migrations.sh provisions it, guarded + non-fatal: creating a role needs access_management on the admin, so an admin without it is skipped with a warning instead of aborting the deploy. - clickhouse-access-management.xml + README: enable access_management on the local/throwaway ClickHouse so bootstrap provisions the role. - Opt-in integration test asserting the contract-read-only / presentation- create-insert-only grant matrix against a real ClickHouse. - PRD/DESIGN: mark cpt-presentation-fr-read-only-role implemented and add the read-only-role component. Part of #1803. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Warning Review limit reached
Next review available in: 2 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📥 CommitsReviewing files that changed from the base of the PR and between c0af56f293df6919dfe626c87820756c7494e181 and 9997894. 📒 Files selected for processing (5)
📝 WalkthroughWalkthroughAdds the ChangesPresentation role
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant clickhouse_migrate as apply-ch-migrations.sh
participant clickhouse as ClickHouse
participant role_sql as presentation-role.sql
participant integration_test as test_presentation_role.py
clickhouse_migrate->>clickhouse: Attempt role creation
clickhouse_migrate->>role_sql: Apply grants when permitted
role_sql->>clickhouse: Grant presentation_ro permissions
integration_test->>clickhouse: Create probe user and test queries
clickhouse-->>integration_test: Allow permitted operations and deny destructive operations
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/domain/presentation-layer/specs/PRD.md`:
- Around line 166-168: Do not mark runtime role enforcement as shipped before
analytics executes queries as presentation_ro. In
docs/domain/presentation-layer/specs/PRD.md lines 166-168, keep
cpt-presentation-fr-read-only-role unchecked or limit it to role provisioning.
In docs/domain/presentation-layer/specs/DESIGN.md lines 238-248, remove the
shipped enforcement claim and state that presentation_ro is provisioned but not
active on the analytics query path.
- Around line 166-168: Align the documented and implemented access for the
presentation_ro role: in docs/domain/presentation-layer/specs/PRD.md lines
166-168, add person to the listed contract databases, and in
src/ingestion/scripts/bootstrap-db/presentation-role.sql lines 9-13, remove the
person.* grant if person is not intended to be contract access. Update the PRD
and SQL consistently based on the intended access scope.
In `@src/ingestion/scripts/tests/test_presentation_role.py`:
- Around line 79-82: Expand the contract-namespace permission test beyond silver
by creating probe tables in silver, person, identity, and insight. Parameterize
the allowed-read and denied-write assertions across all four namespaces,
preserving the existing database setup and admin execution through _admin.
- Around line 85-88: Reorder the setup in the test fixture around _admin so
CREATE USER no longer assigns DEFAULT ROLE presentation_ro before the role
grant. Create the probe user first, grant presentation_ro to PROBE_USER, then
assign presentation_ro as its default role using the appropriate admin
statement, preserving the existing assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 727e8471-0c01-4381-9c2c-ff24e964e08d
📥 Commits
Reviewing files that changed from the base of the PR and between a5314a4 and c0af56f293df6919dfe626c87820756c7494e181.
📒 Files selected for processing (7)
deploy/gitops/system/clickhouse/values.yamldocs/domain/presentation-layer/specs/DESIGN.mddocs/domain/presentation-layer/specs/PRD.mdsrc/ingestion/scripts/apply-ch-migrations.shsrc/ingestion/scripts/bootstrap-db/README.mdsrc/ingestion/scripts/bootstrap-db/presentation-role.sqlsrc/ingestion/scripts/tests/test_presentation_role.py
…espoke XML Access-management is already enabled on every real ClickHouse surface via the official image's CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT env var — root docker-compose.yml and tests/e2e/compose both set it, and the bitnami prod admin has it by default. The bootstrap-only users.d XML I added was reinventing that mechanism. - Delete clickhouse-access-management.xml; the bootstrap README/test now use `-e CLICKHOUSE_DEFAULT_ACCESS_MANAGEMENT=1` on the throwaway container, matching the compose stacks. - Note the bitnami access-management dependency in gitops clickhouse values. Part of #1803. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
c0af56f to
cf0ea46
Compare
#1963) - test: pin CH_URL scheme to http(s) before urlopen (Semgrep dynamic-urllib) and suppress the audited rule with justification. - test: cover every contract namespace (silver/person/identity/insight), not just silver, so a widened/narrowed grant regresses loudly. - test: grant presentation_ro before setting it as the user's default role (ClickHouse rejects a default role that is not yet granted). - PRD/DESIGN: role is provisioned by #1963 but not yet the active query-path identity (analytics still connects as admin), so leave the FR/component unchecked and say so; add the granted `person` database to the PRD list. Part of #1803. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Anton Zelenov <antonz@constructor.tech>
|
Addressed the review in Semgrep ( CodeRabbit — don't mark enforcement shipped — correct: the role is provisioned by #1963 but not yet the active query-path identity (analytics still connects as the admin until the connection wiring lands). Left CodeRabbit — document CodeRabbit — cover every contract namespace — the read-only test is now parameterized over CodeRabbit — grant before default role — fixed: Also, per maintainer feedback, reverted the earlier The connectors-ddl regen reminder is a false positive here: this PR adds a role + provisioning, no bronze/silver/gold schema change. |
| ) | ||
| try: | ||
| # nosemgrep: python.lang.security.audit.dynamic-urllib-use-detected.dynamic-urllib-use-detected | ||
| with urllib.request.urlopen(req) as resp: # noqa: S310 (scheme pinned to http(s) above) |
|
The failing |
3 participants
What
Adds the
presentation_roClickHouse role — the second barrier behind the single-SELECT gate (#1962) in the presentation-layer split. Contract reads run under a role that makes writing the source impossible by construction, not by convention.Grant matrix (matches the governed PRD/DESIGN —
SELECTon the contract,CREATE/INSERTonly inpresentation, noDROP/ALTER/TRUNCATEanywhere):silver.*,person.*,identity.*,insight.*(contract, incl. legacy gold)SELECTpresentation.*(writable namespace)SELECT,INSERT,CREATEChanges
bootstrap-db/presentation-role.sql— canonical, idempotent role DDL.apply-ch-migrations.sh— provisions the role, guarded + non-fatal: creating a role needsaccess_managementon the applying admin, so an admin without it is skipped with a warning instead of aborting the deploy. Bootstrap picks this up via its existing step-4 call.clickhouse-access-management.xml+ README — enableaccess_managementon the local/throwaway (official-image) ClickHouse so bootstrap provisions the role; the official image ships it disabled.tests/test_presentation_role.py— opt-in integration test (repo's skip-unless-*_CH_URLpattern) asserting the full allow/deny grant matrix against a real ClickHouse. This is the adversarial-write half of NFRcpt-presentation-nfr-source-immutability.cpt-presentation-fr-read-only-roleimplemented; add thecpt-presentation-component-read-only-rolecomponent.cfsper-artifact validate +check-languagepass clean.Verification
25.7.5; the exact allow/deny matrix (contract read-only;presentationcreate/insert-only; DROP/ALTER/TRUNCATE denied everywhere) was confirmed with a probe user carrying only the role.set -eabort) on a CH whose admin lacksaccess_management.Scope / follow-ups
presentation_ro(needs the runtime user + sealed secret) and creating the emptypresentationdatabase are the next slices ([pres] Create presentation DB + wire analytics to presentation_ro #1964 + connection wiring), per the DESIGN implementation plan.access_management. If it does not, the guard silently skips the role (WARN). This is harmless today (no consumer yet), but the analytics-wiring PR must confirm/enable it before relying on prod enforcement.Closes #1963
Part of #1803
Summary by CodeRabbit
New Features
presentation_roClickHouse role.Documentation
Tests