Give ro_strict kibana access real e2e coverage - #130
Conversation
`roStrictSettings.yaml` sets `kibana_access: ro_strict` in the `&common-rules` anchor, so the `ro_strict` sanity spec exercises the access level it names. The `ADMIN_GRP` block keeps its `kibana_access: admin` override. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe shared ChangesKibana access settings
Priority: ⬇️ Low Estimated code review effort: 1 (Trivial) | ~3 minutes Change: Bug fix Merge Risk: ⚪ Minimal · up to The fixture now independently validates strict read-only access without changing administrative access. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
First CI run: Every assertion in Worth noting the two specs now take the same 50 seconds. Before this change they were 134s and 62s for identical work, which was an invisible retry rather than a real difference. The leg is red for an unrelated reason: That is the |
|
/review |
|
Review posted: #130 (review) |
There was a problem hiding this comment.
Verdict: the one-line change is correct and CI is green. Two follow-up decisions remain.
The fixture now really configures kibana_access: ro_strict, so Sanity-check-ro_strict-kibana-access.cy.ts exercises the level it names instead of running the ro test a second time.
Verified sound
- The diff is exactly one line (
roStrictSettings.yaml:3);diff -wagainstroSettings.yamlnow shows a singlero→ro_strictdifference. ADMIN_GRPkeeps itskibana_access: adminoverride (roStrictSettings.yaml:51-53), so admin access is unchanged.- The write-ordering claim holds:
defaultSettings.yaml:82-85givestemplate_groupaccess: rwvia*common-kibana-rules(defaultSettings.yaml:5), soloadSampleData(RoAndRoStrictKibanaAccessAssertions.ts:19) runs underrwbefore the fixture applies, and theafterEachrestoresdefaultSettings.yamlbefore the delete. - CI: End-to-end test workflow
success; all 30 bootstrap legs green.
Index of inline findings
roStrictSettings.yaml:3— thero_strictspec is read-only; the write-refusal that distinguishesro_strictfromrois never asserted (decide).roStrictSettings.yaml:3— the twinreadonlyrest_kbnfixture has the same bug and nothing tracks the companion fix against a silent sync revert (decide).
| helpers: | ||
| cr: &common-rules | ||
| kibana_access: ro | ||
| kibana_access: ro_strict |
There was a problem hiding this comment.
The ro_strict spec runs the same read-only assertions as ro, so the one behavior that separates the two levels is never exercised.
$ grep -rn "clickCopyLinkButton" e2e-tests/cypress/e2e e2e-tests/cypress/support
RoAndRoStrictKibanaAccessAssertions.ts:72: Discover.clickCopyLinkButton('ro')
Discover.clickCopyLinkButton (Discover.ts:116-126) creates a short URL only for admin and rw. For 'ro' (and for 'ro_strict') it clicks the copy button and does nothing else. No POST /api/short_url, no _update/index-pattern*, no config/*/_create, no doc/telemetry* — none of the writes in BaseKibanaRule.scala:278-279 that ro_strict refuses.
The two specs are the same test with a different fixture name; after substituting the fixture name they are byte-identical. So this spec passes under ro_strict and under ro, and it would also pass if the plugin silently downgraded ro_strict to ro. It proves "read-heavy pages render under ro_strict" — fail-closed is covered. It cannot prove "ro_strict refuses writes" — fail-open is not covered.
Action: add one assertion that performs a write the non-strict list allows and asserts it is refused — drive Discover.clickCopyLinkButton('ro_strict') through a branch that intercepts POST /api/short_url and asserts 403, or a Direct-kibana-request.cy.ts-style call under the ro_strict fixture asserting the ROR forbidden envelope for _update/index-pattern:*. If that write is intentionally out of scope, rename the spec so it no longer claims ro_strict coverage as a whole, and state which behavior it does cover.
Severity: decide.
| helpers: | ||
| cr: &common-rules | ||
| kibana_access: ro | ||
| kibana_access: ro_strict |
There was a problem hiding this comment.
The same one-line bug is live in the Kibana plugin repo, and nothing tracks the companion fix.
readonlyrest_kbn/automatic-tests/cypress/fixtures/roStrictSettings.yaml has kibana_access: ro on line 3 — the PR body confirms diff -w against its roSettings.yaml is empty. The two repos sync these fixtures (RoAndRoStrictKibanaAccessAssertions.ts:31-32). This PR is now merged, so the next sync can silently restore ro here or never deliver ro_strict there.
Action: open the matching readonlyrest_kbn PR now and link it in this thread, or file a tracking issue pointing at this PR — so the fix does not depend on memory.
Severity: decide.
The bug
e2e-tests/cypress/fixtures/roStrictSettings.yamlwas a copy ofroSettings.yaml. The two files were identical apart from trailing whitespace on two lines:Sanity-check-ro_strict-kibana-access.cy.ts:13gives that fixture to the sameRoAndRoStrictKibanaAccessAssertions.runAssertionsthatSanity-check-ro-kibana-access.cy.ts:13calls withroSettings.yaml. The spec therefore ran therotest a second time. It cost about 62 seconds per leg and it gavero_strictzero end-to-end coverage, while the suite reportedro_strictas covered.The change
One line.
kibana_access: ro_strictin the&common-rulesanchor. TheADMIN_GRPblock keeps its ownkibana_access: adminoverride, so the admin group is unchanged.What
ro_strictis, and how it differs fromroro_strictis a first-class access level. The ES plugin decodes it atcore/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/common.scala:295and emits it in the user metadata atcore/src/main/scala/tech/beshu/ror/accesscontrol/blocks/metadata/MetadataResponse.scala:160.In the ES plugin,
roandro_strictdiffer in exactly one clause ofBaseKibanaRule. Both are "Kibana cannot be modified" (BaseKibanaRule.scala:259-267, whereRO | ROStrict | ApiOnlymap tofalse). The single asymmetry isisRoNonStrictCase(BaseKibanaRule.scala:108-114):isAccessOtherThanRoStrictConfiguredissettings.access =!= ROStrict(BaseKibanaRule.scala:116-121). Sorogets an extra allowance thatro_strictdoes not get: a small set of writes into the user's own Kibana index. The allowed paths are atBaseKibanaRule.scala:278-279, and the source comment onisNonStrictAllowedPath(BaseKibanaRule.scala:134) states the intent: "Save UI state in discover & Short urls".The allowance covers these Kibana-index paths:
<kbn_index>/url/*,<kbn_index>/_create/url:*,<kbn_index>/_update/url*,<kbn_index>/doc/url*— short URLs<kbn_index>/index-pattern/*,<kbn_index>/doc/index-pattern*,<kbn_index>/_update/index-pattern*— index pattern / data view saved object writes<kbn_index>/config/*/_create— UI settings document creation<kbn_index>/doc/telemetry*— telemetry (the 7.x mapping-type path form)/_template/*and only for the actions
indices:data/write/*andindices:admin/template/put(KibanaActionMatchers.scala:113-118).Everything else is identical. Every other clause of
shouldMatch(BaseKibanaRule.scala:49-60) treats the two levels the same.In short:
ro_strictisrominus the ability to persist Discover UI state, short URLs, index-pattern saved object updates and telemetry into the tenancy Kibana index.On the Kibana plugin side the two levels are not distinguished at all.
ROUserTransformer.ts:19applies the identical capability downgrade to both:test/CapabilityTransformers.test.ts:210-223asserts the same thing withit.each([AccessLevel.RO, AccessLevel.RO_STRICT]).Does
ro_strictblock writes that Kibana itself needs to render a page?This was the main risk. The answer is no, for the writes this spec triggers, and the reason is that those writes never reach ROR as the end user.
POST /_bulk,POST /_update/core-usage-stats) are re-signed with the Kibana tech user before they leave the pre-ES proxy. SeeSpecialCaseDetector.shouldUseSystemCredentials(proxy/preElasticsearchProxy/SpecialCaseDetector.ts:73-80), applied atpreElasticsearchProxy.ts:183-198. The whitelist atSpecialCaseDetector.ts:158-174covers exactly the pages this spec opens:api/canvas/workpad/resolve,api/content_management/rpc/get,api/dashboards,internal/dashboards/app,api/lens/existing_fields,internal/lens/visualizations. The substitution is unconditional in the access level, so it protectsro_strictthe same asro.docs/0018-ro-bulk-requests-as-tech-user.mdin the Kibana plugin repo records this decision, and lines 53-54 of that ADR name theroandro_stricte2e suites as its regression suite. Until this PR, half of that regression suite did not exist.IndexSessionRepositorycallspostAsKibana/putAsKibana/getAsKibana/deleteAsKibanaonly (proxy/core/sessionManager/indexBasedSessionManager/indexSessionRepository.ts:55,75,82,105,117,142). Session storage is therefore independent ofkibana_access.PUT .../_create/configwith aKibanauser agent) also uses the tech user —SpecialCaseDetector.isTenantIndexInitRequest(SpecialCaseDetector.ts:82-89)._update/config:<version>and_update/telemetry:telemetry. Neither form matches the non-strict regex, which only carries the 7.xconfig/*/_createanddoc/telemetry*shapes. So these are already forbidden under plainrotoday, andro_strictadds no new failure there.RoAndRoStrictKibanaAccessAssertions.ts:72callsDiscover.clickCopyLinkButton('ro'), andDiscover.ts:116-126only interceptsPOST /api/short_urlforadminandrw. Forroit copies the snapshot URL. The assertion that follows checks the clipboard against aDISCOVER_APP_LOCATORsnapshot URL, not a short URL. No short URL is created, so the largestro_strictrestriction is not touched.The write ordering still holds
Confirmed.
RoAndRoStrictKibanaAccessAssertions.ts:19-20loads the sample data first and applies the fixture second:At the moment of
loadSampleData,defaultSettings.yamlis still in force, and itsTemplate Tenancyblock givestemplate_groupaccess: rw(fixtures/defaultSettings.yaml:82-85). The sample data indices and the sample saved objects are written underrw. The test body needs read access only. TheafterEachrestoresdefaultSettings.yamlbefore it deletes the sample data, so the teardown write is also underrw. Applying the fixture itself goes throughPOST _readonlyrest/admin/configwithCypress.env().kibanaUserCredentials(RorApiClient.ts:4-10), not the test user, so it is unaffected as well.Why this PR is separate
This PR is deliberately separate from #129 so that a red leg here is a finding, not a blocker on unrelated work. If this leg fails, it fails alone and it tells us something we did not know. Nothing else depends on it.
I cannot run the suite locally. It needs
ROR_ACTIVATION_KEY, which is a CI secret. CI is the verifier.Branch target
master, perdocs/dev/branching.md. Case 3 is the closest fit: a fix for a test that does not do its job against the released plugins. Case 2 also points atmaster, becausero_strictis released behaviour, somastermust run a real test against it. The nuance worth stating: the test does not go red today, it goes green for the wrong reason, which is worse. Both cases send the fix tomaster, and the merge back todevelopcarries it over.Decision tree if CI goes red
Read the failing assertion first, then the ES plugin log for the forbidden request path.
A. "The test needs adjusting for
ro_strict" — the failure is in an assertion that depends on a write the non-strict allowance used to cover.IndexPattern.openItem(0)/SubHeader.breadcrumbsLastItemon the Data Views page (RoAndRoStrictKibanaAccessAssertions.ts:130-136)POST .../_update/index-pattern:*or.../doc/index-pattern*ro_strict. The assertion, not the product, needs aro_strictbranch.:70-83)POST .../_create/url:*or.../_update/url*'ro'argument. CheckDiscover.ts:119— the version gate may have drifted. Test-side fix.<kbn_index>/url/*writero_strict. Test-side fix.In all of these the correct fix is a
ro_strictbranch in the assertions, or a separatero_strict-specific assertion file. Do not "fix" them by relaxingro_strictin the ES plugin — the whole point of the level is that these writes are refused.B. "
ro_stricthas a real product problem" — the failure is in an assertion that has nothing to do with the non-strict path list.embeddableErrorappears, orlnsVisualizationContaineris missing (:38-60)SpecialCaseDetector.shouldUseSystemCredentialsdid not fire. A Kibana-internal_bulkorcore-usage-statswrite leaked out under the user's credentials. This is ADR 0018 regressing, and it is a product bug.cy.wait('@canvasResolve')(:108)api/canvas/workpad/resolveorinternal/canvas/workpad/resolve. Product bug.:117), or a readonly badge or hidden-button assertion fails (:50-51,:64-66,:103-107,:131,:137-139)ROUserTransformer, which treatsROandRO_STRICTidentically (ROUserTransformer.ts:19). A difference here means the access level is not reaching the capability transformer. Product bug.:21-22)indexSessionRepository.ts), so a failure here is a product bug, not a test problem.The short version: if the blocked path matches the list in
BaseKibanaRule.scala:278-279, the test is wrong. If it does not, the product is wrong.The same bug exists in the Kibana plugin repo
readonlyrest_kbn/automatic-tests/cypress/fixtures/roStrictSettings.yamlhas the identical defect — line 3 readskibana_access: ro, anddiff -wagainst itsroSettings.yamlis empty. The two repos sync these page objects and fixtures (see the sync note atRoAndRoStrictKibanaAccessAssertions.ts:31-32), so the bug shipped through the sync and a future sync could overwrite this fix. A matching PR onreadonlyrest_kbnshould follow. I have not opened it — this PR stays to one repo and one line.🤖 Generated with Claude Code
Summary by CodeRabbit