Skip to content

Give ro_strict kibana access real e2e coverage - #130

Merged
sscarduzio merged 1 commit into
masterfrom
fix/ro-strict-fixture-uses-ro-strict-access
Sep 13, 2026
Merged

sscarduzio merged 1 commit into
masterfrom
fix/ro-strict-fixture-uses-ro-strict-access

Conversation

@sscarduzio

@sscarduzio sscarduzio commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

The bug

e2e-tests/cypress/fixtures/roStrictSettings.yaml was a copy of roSettings.yaml. The two files were identical apart from trailing whitespace on two lines:

$ diff -w e2e-tests/cypress/fixtures/roSettings.yaml e2e-tests/cypress/fixtures/roStrictSettings.yaml
$ grep -n kibana_access e2e-tests/cypress/fixtures/roStrictSettings.yaml
3:    kibana_access: ro
54:      kibana_access: admin

Sanity-check-ro_strict-kibana-access.cy.ts:13 gives that fixture to the same RoAndRoStrictKibanaAccessAssertions.runAssertions that Sanity-check-ro-kibana-access.cy.ts:13 calls with roSettings.yaml. The spec therefore ran the ro test a second time. It cost about 62 seconds per leg and it gave ro_strict zero end-to-end coverage, while the suite reported ro_strict as covered.

The change

One line. kibana_access: ro_strict in the &common-rules anchor. The ADMIN_GRP block keeps its own kibana_access: admin override, so the admin group is unchanged.

What ro_strict is, and how it differs from ro

ro_strict is a first-class access level. The ES plugin decodes it at core/src/main/scala/tech/beshu/ror/accesscontrol/factory/decoders/common.scala:295 and emits it in the user metadata at core/src/main/scala/tech/beshu/ror/accesscontrol/blocks/metadata/MetadataResponse.scala:160.

In the ES plugin, ro and ro_strict differ in exactly one clause of BaseKibanaRule. Both are "Kibana cannot be modified" (BaseKibanaRule.scala:259-267, where RO | ROStrict | ApiOnly map to false). The single asymmetry is isRoNonStrictCase (BaseKibanaRule.scala:108-114):

private lazy val isRoNonStrictCase = {
  isTargetingKibana &&
  isAccessOtherThanRoStrictConfigured &&   // <- the only ro / ro_strict difference
  kibanaCannotBeModified &&
  isNonStrictAllowedPath &&
  isNonStrictAction
}

isAccessOtherThanRoStrictConfigured is settings.access =!= ROStrict (BaseKibanaRule.scala:116-121). So ro gets an extra allowance that ro_strict does not get: a small set of writes into the user's own Kibana index. The allowed paths are at BaseKibanaRule.scala:278-279, and the source comment on isNonStrictAllowedPath (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/* and indices: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_strict is ro minus 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:19 applies the identical capability downgrade to both:

if (AccessLevel.RO !== metadata.kibanaAccess && AccessLevel.RO_STRICT !== metadata.kibanaAccess) {
  return cap;
}

test/CapabilityTransformers.test.ts:210-223 asserts the same thing with it.each([AccessLevel.RO, AccessLevel.RO_STRICT]).

Does ro_strict block 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.

  • Kibana-internal writes needed to render read-heavy pages (POST /_bulk, POST /_update/core-usage-stats) are re-signed with the Kibana tech user before they leave the pre-ES proxy. See SpecialCaseDetector.shouldUseSystemCredentials (proxy/preElasticsearchProxy/SpecialCaseDetector.ts:73-80), applied at preElasticsearchProxy.ts:183-198. The whitelist at SpecialCaseDetector.ts:158-174 covers 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 protects ro_strict the same as ro. docs/0018-ro-bulk-requests-as-tech-user.md in the Kibana plugin repo records this decision, and lines 53-54 of that ADR name the ro and ro_strict e2e suites as its regression suite. Until this PR, half of that regression suite did not exist.
  • Session documents are written with the tech user, not the end user. IndexSessionRepository calls postAsKibana / putAsKibana / getAsKibana / deleteAsKibana only (proxy/core/sessionManager/indexBasedSessionManager/indexSessionRepository.ts:55,75,82,105,117,142). Session storage is therefore independent of kibana_access.
  • Tenancy index bootstrap (PUT .../_create/config with a Kibana user agent) also uses the tech user — SpecialCaseDetector.isTenantIndexInitRequest (SpecialCaseDetector.ts:82-89).
  • UI settings updates and telemetry on Kibana 8/9 go through _update/config:<version> and _update/telemetry:telemetry. Neither form matches the non-strict regex, which only carries the 7.x config/*/_create and doc/telemetry* shapes. So these are already forbidden under plain ro today, and ro_strict adds no new failure there.
  • Short URLs are not exercised. RoAndRoStrictKibanaAccessAssertions.ts:72 calls Discover.clickCopyLinkButton('ro'), and Discover.ts:116-126 only intercepts POST /api/short_url for admin and rw. For ro it copies the snapshot URL. The assertion that follows checks the clipboard against a DISCOVER_APP_LOCATOR snapshot URL, not a short URL. No short URL is created, so the largest ro_strict restriction is not touched.

The write ordering still holds

Confirmed. RoAndRoStrictKibanaAccessAssertions.ts:19-20 loads the sample data first and applies the fixture second:

kbnApiClient.loadSampleData('ecommerce', credentials, 'template_group');
Settings.setSettingsData(fixtureYamlFileName);

At the moment of loadSampleData, defaultSettings.yaml is still in force, and its Template Tenancy block gives template_group access: rw (fixtures/defaultSettings.yaml:82-85). The sample data indices and the sample saved objects are written under rw. The test body needs read access only. The afterEach restores defaultSettings.yaml before it deletes the sample data, so the teardown write is also under rw. Applying the fixture itself goes through POST _readonlyrest/admin/config with Cypress.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, per docs/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 at master, because ro_strict is released behaviour, so master must 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 to master, and the merge back to develop carries 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.

Failing assertion Blocked request to look for Reading
IndexPattern.openItem(0) / SubHeader.breadcrumbsLastItem on the Data Views page (RoAndRoStrictKibanaAccessAssertions.ts:130-136) POST .../_update/index-pattern:* or .../doc/index-pattern* Kibana refreshes the data view field cache on open. Expected under ro_strict. The assertion, not the product, needs a ro_strict branch.
The Discover share clipboard check (:70-83) POST .../_create/url:* or .../_update/url* A short URL was created despite the 'ro' argument. Check Discover.ts:119 — the version gate may have drifted. Test-side fix.
A Discover UI-state assertion after a page reload any <kbn_index>/url/* write Discover UI state persistence. Expected under ro_strict. Test-side fix.

In all of these the correct fix is a ro_strict branch in the assertions, or a separate ro_strict-specific assertion file. Do not "fix" them by relaxing ro_strict in the ES plugin — the whole point of the level is that these writes are refused.

B. "ro_strict has a real product problem" — the failure is in an assertion that has nothing to do with the non-strict path list.

Failing assertion Reading
Dashboard fails to load, or embeddableError appears, or lnsVisualizationContainer is missing (:38-60) The tech-user substitution in SpecialCaseDetector.shouldUseSystemCredentials did not fire. A Kibana-internal _bulk or core-usage-stats write leaked out under the user's credentials. This is ADR 0018 regressing, and it is a product bug.
Canvas fails at cy.wait('@canvasResolve') (:108) Same as above, for api/canvas/workpad/resolve or internal/canvas/workpad/resolve. Product bug.
The Stack Management side-nav has the wrong item count (:117), or a readonly badge or hidden-button assertion fails (:50-51, :64-66, :103-107, :131, :137-139) These come from ROUserTransformer, which treats RO and RO_STRICT identically (ROUserTransformer.ts:19). A difference here means the access level is not reaching the capability transformer. Product bug.
Login or tenancy switching fails (:21-22) Session or user metadata handling. Sessions use tech-user credentials (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.yaml has the identical defect — line 3 reads kibana_access: ro, and diff -w against its roSettings.yaml is empty. The two repos sync these page objects and fixtures (see the sync note at RoAndRoStrictKibanaAccessAssertions.ts:31-32), so the bug shipped through the sync and a future sync could overwrite this fix. A matching PR on readonlyrest_kbn should follow. I have not opened it — this PR stays to one repo and one line.

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Access Control
    • Updated applicable access rules to use strict read-only Kibana access.
    • Affected rules now prevent write actions while continuing to allow permitted read-only access.

`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>
@sscarduzio
sscarduzio requested a review from Dzuming September 12, 2026 15:41
@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Advanced

Run ID: 17766520-b903-486a-8955-19a3e47a98d8

📥 Commits

Reviewing files that changed from the base of the PR and between fb9ecce and 9a8e5ae.

📒 Files selected for processing (1)
  • e2e-tests/cypress/fixtures/roStrictSettings.yaml

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The shared common-rules YAML anchor now sets kibana_access to ro_strict. The PERSONAL_GRP, infosec, and Template Tenancy rules inherit this setting.

Changes

Kibana access settings

Layer / File(s) Summary
Apply strict read-only access
e2e-tests/cypress/fixtures/roStrictSettings.yaml
The shared common-rules anchor changes kibana_access from ro to ro_strict.

Priority: ⬇️ Low

Estimated code review effort: 1 (Trivial) | ~3 minutes

Change: Bug fix

Merge Risk: ⚪ Minimal · up to 9a8e5

The fixture now independently validates strict read-only access without changing administrative access.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: adding real end-to-end coverage for ro_strict Kibana access.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 0…
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/ro-strict-fixture-uses-ro-strict-access

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sscarduzio

Copy link
Copy Markdown
Contributor Author

First CI run: ro_strict works. The spec passed with the real access level.

✔  Sanity-check-ro_strict-kibana-acces        00:50  tests=1 pass=1
✔  Sanity-check-ro-kibana-access.cy.ts        00:50  tests=1 pass=1

Every assertion in RoAndRoStrictKibanaAccessAssertions holds under ro_strict, including the Data Views page that was the predicted risk — opening a data view does not write the index-pattern saved object as the user, because SpecialCaseDetector.shouldUseSystemCredentials re-signs it with the tech user first.

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: Tenancy.cy.ts, 12 of 17 tests passing, failing with

CypressError: Timed out after waiting `20000ms` for your remote page to load.
Your page did not fire its `load` event within `20000ms`.

That is the 9.4.6, docker proxy hang that #128 fixes — the same leg failed on #129 and on the ES plugin's e2e_es94x in the same hour. Re-running it.

@Dzuming Dzuming left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

LGTM

@sscarduzio
sscarduzio merged commit 6b0814e into master Sep 13, 2026
98 of 99 checks passed
@sscarduzio
sscarduzio deleted the fix/ro-strict-fixture-uses-ro-strict-access branch September 13, 2026 13:14
@sscarduzio

Copy link
Copy Markdown
Contributor Author

/review

@10hexdev

10hexdev Bot commented Sep 13, 2026

Copy link
Copy Markdown

Review posted: #130 (review)

@10hexdev 10hexdev Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 -w against roSettings.yaml now shows a single roro_strict difference.
  • ADMIN_GRP keeps its kibana_access: admin override (roStrictSettings.yaml:51-53), so admin access is unchanged.
  • The write-ordering claim holds: defaultSettings.yaml:82-85 gives template_group access: rw via *common-kibana-rules (defaultSettings.yaml:5), so loadSampleData (RoAndRoStrictKibanaAccessAssertions.ts:19) runs under rw before the fixture applies, and the afterEach restores defaultSettings.yaml before the delete.
  • CI: End-to-end test workflow success; all 30 bootstrap legs green.

Index of inline findings

  1. roStrictSettings.yaml:3 — the ro_strict spec is read-only; the write-refusal that distinguishes ro_strict from ro is never asserted (decide).
  2. roStrictSettings.yaml:3 — the twin readonlyrest_kbn fixture 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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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.

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