[RORDEV-2053] analysis#99
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a shell helper to create Kibana data views, integrates it into the demo init flow, pins ES/Kibana/ROR versions, wires KIBANA_ADDRESS into compose files, and refactors ReadonlyREST access-control and user mappings to a group-based tenancy model. ChangesKibana Tenancy and Data View Integration
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@ror-demo-cluster/conf/es/readonlyrest.yml`:
- Around line 21-33: The Group1 users and Group2 users rules only set
kibana.index and don’t restrict Elasticsearch data indices, so add per-tenant
index restrictions that use the selected tenancy/current group; update the
"Group1 users" and "Group2 users" entries (the rules that contain groups_or
["g1"] and ["g2"] and kibana.index .kibana_@{acl:current_group}) to also include
data index permission blocks that reference @{acl:current_group} (e.g. restrict
allowed index patterns like logs-@{acl:current_group}-* or a generic pattern
using @{acl:current_group}) so queries and data-view creation are limited to the
current tenant, and ensure the permission uses acl:current_group rather than
relying on the first-matching group rule.
In `@ror-demo-cluster/docker-compose.yml`:
- Line 43: The docker-compose environment string sets
com.readonlyrest.settings.loading.delay without a duration unit which
ReadonlyREST requires; update the ES_JAVA_OPTS value that contains
"com.readonlyrest.settings.loading.delay=1" to use an explicit unit such as
"com.readonlyrest.settings.loading.delay=1s" so the setting is parseable (locate
the ES_JAVA_OPTS entry in the compose service, modify the delay token
accordingly).
🪄 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
Run ID: 1d82bbab-8552-4ccd-bfed-f0968c18b6e3
📒 Files selected for processing (7)
examples/ror-with-kibana-reverse-proxy-demo/docker-compose.ymlror-demo-cluster/.env-showcaseror-demo-cluster/conf/es/readonlyrest.ymlror-demo-cluster/conf/kbn/enterprise-ror-newplatform-kibana.ymlror-demo-cluster/docker-compose.ymlshared/init-scripts/init.shshared/init-scripts/utils/lib.sh
| - name: "Group1 users" | ||
| type: allow | ||
| groups_or: ["g1"] | ||
| kibana: | ||
| access: rw | ||
| index: .kibana_@{acl:current_group} | ||
|
|
||
| - name: "End users" | ||
| groups: ["EndUsers"] | ||
| indices: ["*-frontend-*", "kibana_sample_data_*"] | ||
| - name: "Group2 users" | ||
| type: allow | ||
| groups_or: ["g2"] | ||
| kibana: | ||
| index: .kibana_end_@{user} | ||
| access: rw | ||
| hide_apps: ["Security", "Observability"] | ||
| index: .kibana_@{acl:current_group} |
There was a problem hiding this comment.
Add per-tenant index restrictions to the g1/g2 rules.
These blocks only switch the Kibana saved-object index. They do not restrict Elasticsearch data indices, so a g1 user can still create a data view for logs-system-* and query another tenant's data. That breaks the group isolation this PR is introducing. Because user1 belongs to both groups, the restriction also needs to follow the selected tenancy/current group rather than just the first matching group rule.
🤖 Prompt for 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.
In `@ror-demo-cluster/conf/es/readonlyrest.yml` around lines 21 - 33, The Group1
users and Group2 users rules only set kibana.index and don’t restrict
Elasticsearch data indices, so add per-tenant index restrictions that use the
selected tenancy/current group; update the "Group1 users" and "Group2 users"
entries (the rules that contain groups_or ["g1"] and ["g2"] and kibana.index
.kibana_@{acl:current_group}) to also include data index permission blocks that
reference @{acl:current_group} (e.g. restrict allowed index patterns like
logs-@{acl:current_group}-* or a generic pattern using @{acl:current_group}) so
queries and data-view creation are limited to the current tenant, and ensure the
permission uses acl:current_group rather than relying on the first-matching
group rule.
| - discovery.type=single-node | ||
| - bootstrap.memory_lock=true | ||
| - "ES_JAVA_OPTS=-Xms512m -Xmx512m -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005 -Dcom.readonlyrest.settings.loading.attempts.count=1 -Dcom.readonlyrest.settings.loading.delay=0s" | ||
| - "ES_JAVA_OPTS=-Xms512m -Xmx512m -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005 -Dcom.readonlyrest.settings.loading.attempts.count=1 -Dcom.readonlyrest.settings.loading.delay=1" |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What is the accepted value format for ReadonlyREST JVM property com.readonlyrest.settings.loading.delay? Is 1valid, or must it include a time unit like1s?
💡 Result:
Accepted value format for ReadonlyREST JVM property com.readonlyrest.settings.loading.delay is a Java-style duration that includes a time unit (e.g., 1s), not a bare integer like 1. [1][2] Evidence: ReadonlyREST documents duration-valued configuration parameters using explicit unit suffixes (table shows s, m, h, d with examples like "1s"). [2] A community discussion also references com.readonlyrest.settings.loading.delay specifically as a setting to adjust delay, implying it’s handled as a duration-like value. [1] So: use -Dcom.readonlyrest.settings.loading.delay=1s (seconds) rather than =1. [2][1]
Citations:
- 1: Option to disable loading settings from .readonlyrest readonlyrest-docs#78
- 2: https://docs.readonlyrest.com/kibana
Use an explicit duration unit for com.readonlyrest.settings.loading.delay (ror-demo-cluster/docker-compose.yml line 43)
ReadonlyREST expects com.readonlyrest.settings.loading.delay to be a duration value with a unit suffix (e.g., 1s), so =1 is not a valid/explicitly parseable form. Update to 1s.
Proposed fix
- - "ES_JAVA_OPTS=-Xms512m -Xmx512m -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005 -Dcom.readonlyrest.settings.attempts.count=1 -Dcom.readonlyrest.settings.loading.delay=1"
+ - "ES_JAVA_OPTS=-Xms512m -Xmx512m -agentlib:jdwp=transport=dt_socket,server=y,suspend=n,address=*:5005 -Dcom.readonlyrest.settings.attempts.count=1 -Dcom.readonlyrest.settings.loading.delay=1s"🤖 Prompt for 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.
In `@ror-demo-cluster/docker-compose.yml` at line 43, The docker-compose
environment string sets com.readonlyrest.settings.loading.delay without a
duration unit which ReadonlyREST requires; update the ES_JAVA_OPTS value that
contains "com.readonlyrest.settings.loading.delay=1" to use an explicit unit
such as "com.readonlyrest.settings.loading.delay=1s" so the setting is parseable
(locate the ES_JAVA_OPTS entry in the compose service, modify the delay token
accordingly).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
shared/init-scripts/utils/lib.sh (1)
133-152: 💤 Low valueUnescaped interpolation into JSON body and
x-ror-tenancy-idheader.
$INDEX_PATTERN,$DATA_VIEW_NAME,$TIME_FIELD_NAME, and$TENANCYare inlined verbatim. A value containing",\, or a newline will break the JSON document or, in the tenancy case, allow CRLF header smuggling. Inputs are demo-controlled today, so this is robustness-only, but a quickjq -nc --arg ...(or aprintf %q-style sanitizer) eliminates the foot-gun for free.♻️ Sketch using `jq` to build the body
- data_view_fields="\"title\": \"$INDEX_PATTERN\", \"name\": \"$DATA_VIEW_NAME\"" - if [ -n "$TIME_FIELD_NAME" ]; then - data_view_fields="$data_view_fields, \"timeFieldName\": \"$TIME_FIELD_NAME\"" - fi + body=$(jq -nc \ + --arg title "$INDEX_PATTERN" \ + --arg name "$DATA_VIEW_NAME" \ + --arg tf "$TIME_FIELD_NAME" \ + '{data_view: ({title:$title, name:$name} + (if $tf == "" then {} else {timeFieldName:$tf} end))}') @@ - "${tenancy_header[@]}" -d "{ - \"data_view\": { $data_view_fields } - }" + "${tenancy_header[@]}" -d "$body"🤖 Prompt for 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. In `@shared/init-scripts/utils/lib.sh` around lines 133 - 152, The JSON body and tenancy header are built by directly interpolating INDEX_PATTERN, DATA_VIEW_NAME, TIME_FIELD_NAME and TENANCY into data_view_fields and tenancy_header before the curl POST to /api/data_views/data_view, which can break JSON or allow header smuggling; instead, build the request body using a safe serializer (e.g. jq -n --arg INDEX_PATTERN "$INDEX_PATTERN" --arg DATA_VIEW_NAME "$DATA_VIEW_NAME" --arg TIME_FIELD_NAME "$TIME_FIELD_NAME" '...') to produce a properly escaped JSON object for the -d argument, and sanitize/escape TENANCY before adding it to tenancy_header (or only add the header when the sanitized value is non-empty) so you never inline raw unescaped values into data_view_fields, tenancy_header or the curl -d payload.
🤖 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 `@shared/init-scripts/init.sh`:
- Around line 10-17: The script only registers the frontend Kibana data view
because the other lines are commented out; either re-enable the corresponding
data stream creation and data view calls (uncomment the createDataStream
"logs-business-dev" / "logs-system-dev" + generate_log_documents | putDocument
lines and uncomment createKibanaDataView "logs-business-*" and "logs-system-*"
so groups g1/g2 get views), or remove those commented stubs entirely to avoid
misleading the PR summary; update the same symbols createDataStream,
generate_log_documents, putDocument, and createKibanaDataView accordingly so
running ./run.sh produces the expected indices and data views for g1 and g2.
---
Nitpick comments:
In `@shared/init-scripts/utils/lib.sh`:
- Around line 133-152: The JSON body and tenancy header are built by directly
interpolating INDEX_PATTERN, DATA_VIEW_NAME, TIME_FIELD_NAME and TENANCY into
data_view_fields and tenancy_header before the curl POST to
/api/data_views/data_view, which can break JSON or allow header smuggling;
instead, build the request body using a safe serializer (e.g. jq -n --arg
INDEX_PATTERN "$INDEX_PATTERN" --arg DATA_VIEW_NAME "$DATA_VIEW_NAME" --arg
TIME_FIELD_NAME "$TIME_FIELD_NAME" '...') to produce a properly escaped JSON
object for the -d argument, and sanitize/escape TENANCY before adding it to
tenancy_header (or only add the header when the sanitized value is non-empty) so
you never inline raw unescaped values into data_view_fields, tenancy_header or
the curl -d payload.
🪄 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
Run ID: e997b7c6-43a4-4880-acd4-05fa02e7551a
⛔ Files ignored due to path filters (1)
ror-demo-cluster/readonlyrest-1.70.0-pre7_es8.19.11.zipis excluded by!**/*.zip
📒 Files selected for processing (3)
ror-demo-cluster/.env-showcaseshared/init-scripts/init.shshared/init-scripts/utils/lib.sh
There was a problem hiding this comment.
♻️ Duplicate comments (1)
ror-demo-cluster/conf/es/readonlyrest.yml (1)
21-33:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRe-enable tenant isolation in g1/g2 rules before merge.
kibana.indexis still commented (Line 26, Line 33), and these rules still do not constrain Elasticsearch data indices. That leaves tenancy separation incomplete for both saved objects and data access.Suggested minimal config direction
- name: "Group1 users" type: allow groups_or: ["g1"] kibana: access: rw - `#index`: .kibana_@{acl:current_group} + index: .kibana_@{acl:current_group} + indices: ["logs-frontend-*", "logs-business-*"] - name: "Group2 users" type: allow groups_or: ["g2"] kibana: access: rw - `#index`: .kibana_@{acl:current_group} + index: .kibana_@{acl:current_group} + indices: ["logs-system-*"]🤖 Prompt for 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. In `@ror-demo-cluster/conf/es/readonlyrest.yml` around lines 21 - 33, Re-enable tenant isolation by uncommenting and restoring the kibana.index entries in the "Group1 users" and "Group2 users" rules so saved objects and Kibana access are scoped to each group's tenant (restore the kibana.index setting to use the .kibana_@{acl:current_group} placeholder for the rules named "Group1 users" and "Group2 users"); ensure the kibana.access remains rw and verify the index pattern exactly matches .kibana_@{acl:current_group} so both saved objects and data access are constrained to each group's tenant.
🤖 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.
Duplicate comments:
In `@ror-demo-cluster/conf/es/readonlyrest.yml`:
- Around line 21-33: Re-enable tenant isolation by uncommenting and restoring
the kibana.index entries in the "Group1 users" and "Group2 users" rules so saved
objects and Kibana access are scoped to each group's tenant (restore the
kibana.index setting to use the .kibana_@{acl:current_group} placeholder for the
rules named "Group1 users" and "Group2 users"); ensure the kibana.access remains
rw and verify the index pattern exactly matches .kibana_@{acl:current_group} so
both saved objects and data access are constrained to each group's tenant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 691463a9-19f9-40e6-bded-cbc15b44e31d
⛔ Files ignored due to path filters (1)
ror-demo-cluster/readonlyrest_kbn_universal-1.70.0-pre11_es8.19.11.zipis excluded by!**/*.zip
📒 Files selected for processing (3)
ror-demo-cluster/.env-showcaseror-demo-cluster/conf/es/readonlyrest.ymlshared/init-scripts/init.sh
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@shared/init-scripts/init.sh`:
- Line 18: The createKibanaDataView invocation in init.sh passes "kibana"
"kibana" without a tenancy which may lack saved-object privileges under the
"KIBANA" readonlyrest rule; either change that call to use the admin user like
the other data view calls (use admin:admin) or update the "KIBANA" rule in
ror-demo-cluster/conf/es/readonlyrest.yml to include a kibana block granting the
saved-objects/data-views rights (e.g., kibana: { access: rw, index:
<appropriate-kibana-index> }) so the createKibanaDataView function can
successfully POST to "$KIBANA_ADDRESS/api/data_views/data_view".
🪄 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
Run ID: 7db0a2d2-1899-4607-bfe4-25c14bf9d8a0
📒 Files selected for processing (2)
ror-demo-cluster/conf/es/readonlyrest.ymlshared/init-scripts/init.sh
2 participants
cd ror-demo-cluster && ./clean.sh && ./run.shSummary by CodeRabbit
New Features
Configuration