[entityanalytics_okta] Fix permissions enrichment - #20732
Conversation
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
✅ Elastic Docs Style Checker (Vale)No issues found on modified lines! The Vale linter checks documentation changes against the Elastic Docs style guide. To use Vale locally or report issues, refer to Elastic style guide for Vale. |
|
✅ All changelog entries have the correct PR link. |
| - groups | ||
| {{#if enrich_user_roles}} | ||
| - roles | ||
| {{else if enrich_user_perms}} |
There was a problem hiding this comment.
Severity: 🔵 Low confidence: medium path: packages/entityanalytics_okta/data_stream/entity/agent/stream/entity-analytics.yml.hbs:46
The new roles branch is not gated on use_minimal_state even though its comment justifies it purely by minimal-state behaviour, so legacy policies also gain an extra - roles entry; either gate the branch or reword the comment to say the entry is emitted unconditionally.
Details
The comment on lines 47-49 states that the explicit - roles is needed because "the minimal-state path only fetches permissions as part of the 'roles' enrichment", and that "the legacy path infers this from 'perms'". By that reasoning the entry is redundant on the legacy path, yet the branch is not gated on use_minimal_state -- so legacy policies now render groups, roles, perms where they previously rendered groups, perms. That change is visible in the new test-enrich-perms-legacy.expected fixture.
The rendered value is still valid on the legacy path (roles is what enrich_user_roles already emits there), so this is not a functional break -- but it does produce a policy diff for existing legacy configurations, and it leaves the code and its own comment out of step. The adjacent permissions/perms branch on lines 60-64 does gate on use_minimal_state, which makes the inconsistency more noticeable.
Recommendation:
Either gate the implied-roles branch the same way the permissions branch is gated:
or keep the current unconditional form and reword the comment to say that roles is listed explicitly for both paths (required by minimal-state, harmless and already implied on legacy).
🤖 AI-Generated Review | Vera Review Bot - v0.2.6 | 📚 Knowledge base: integration-skills
⚠️ Automated review — verify suggestions before applying.
Review summaryIssues found across the latest commits 4e9a299 — 1 medium, 1 low
Package-level:
🤖 AI-Generated Review | Vera Review Bot - v0.2.6 | 📚 Knowledge base: integration-skills
|
💚 Build Succeeded
|
Proposed commit message
Discussion
Problems
Since 3.5.0 (#20212) this package defaults to
use_minimal_state: true, so it uses Filebeat's minimal-state provider (beats#50685). That exposed two problems with how the template is rendered with the "Enrich User Permissions" option set:Wrong value name. The minimal-state provider strictly validates
enrich_withand names this enrichmentpermissions; the template was using the legacy spellingperms. Enabling the option made the input fail permanently withunable to create minimal-state provider "okta": unknown enrich_with value, and the integration went unhealthy. The legacy path never validatedenrich_with(unknown values are silently ignored), which is why this only started failing once minimal-state became the default.The documented roles implication is unfulfilled. The option description promises "Enabling this option implicitly enables role enrichment". The legacy path implements that in code (roles are fetched when either value is present), but the minimal-state provider only fetches permissions inside the roles enrichment — so even a correctly spelled
permissionswithoutrolesvalidates cleanly and then silently does nothing.Changes
Both changes are in
entity-analytics.yml.hbs; rendered values are otherwise unchanged.1. The permissions spelling depends on the implementation used:
permissionswhenuse_minimal_stateis true,permswhen false. The manifest already requiresagent.version: ^9.5.0since 3.5.0, so every supported agent has both paths and honours the flag. A template comment records the condition for collapsing this to a single value later: the legacy path acceptingpermissions.2. The template now emits
roleswheneverenrich_user_permsis set, restoring the documented implication on the minimal-state path. This is a no-op for the legacy path, which would fetch roles anyway.Policy tests
Three new policy tests assert the rendered
enrich_withlist:groupsonlygroups,roles,permissionsand — critically — nopermsuse_minimal_state: falserendersgroups,roles,permsRecommended beats follow-up
x-pack/filebeat/input/entityanalytics/provider/okta/okta.go): acceptpermissionsas a synonym forperms. This is the condition for removing the template branch added here.x-pack/filebeat/input/entityanalytics/provider/okta/minimal.go, backed byelastic/entcollect): acceptpermsas a synonym, and implement the documented roles implication (or rejectpermissionswithoutrolesinstead of silently no-opping). This unbreaks package versions 3.2.0–3.5.0, which send bareperms, on current and future agents.permissionsspelling rather thanperms.Checklist
changelog.ymlfile.