x-pack/filebeat/input/entityanalytics/provider/okta: read permissions for custom roles#49805
Conversation
chemamartinez
added
Filebeat
botelastic
bot
added
the
needs_team
|
Pinging @elastic/security-service-integrations (Team:Security-Service Integrations) |
botelastic
bot
removed
the
needs_team
🤖 GitHub commentsJust comment with:
|
|
This pull request does not have a backport label.
To fixup this pull request, you need to add the backport labels for the needed
|
📝 WalkthroughWalkthroughAdds a new "perms" value to the Okta 🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 the current code and only fix it if needed.
Inline comments:
In `@x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go`:
- Around line 312-339: GetRolePermissions is being called with role assignment
IDs (from GetUserRoles) but the permissions endpoint requires the role
definition ID; fix by changing the flow so callers pass the definition ID
(role.RoleID or equivalent) into GetRolePermissions instead of role.ID, or if
GetUserRoles doesn't currently expose the definition id, update GetUserRoles to
return the definition id field and update all callers to use that, otherwise
perform a short translation lookup (call the roles API to map assignment ID →
definition ID) before invoking GetRolePermissions; update tests so they use
assignment IDs where appropriate and assert the translation path works.
In `@x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go`:
- Line 61: The roles raw string constant (roles) in the Okta unit test contains
malformed JSON keys (e.g., `created\"`, `lastUpdated\"`, `assignmentType\"`)
caused by stray backslashes inside the backtick literal; update the `roles`
constant in okta_test.go so the fourth role object uses valid JSON key names
(remove the backslashes so keys are `"created"`, `"lastUpdated"`,
`"assignmentType"`), verify the JSON is syntactically correct (matching quotes
and commas) and then run the tests to confirm unmarshaling succeeds.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 8c334a50-a67b-449e-9e8e-a05347a7ce06
📒 Files selected for processing (6)
changelog/fragments/1774954711-okta-entity-analytics-role-permissions.yamldocs/reference/filebeat/filebeat-input-entity-analytics.mdx-pack/filebeat/input/entityanalytics/provider/okta/conf.gox-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.gox-pack/filebeat/input/entityanalytics/provider/okta/okta.gox-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go
| // GetRolePermissions returns the permissions for an Okta role using the IAM roles API endpoint. | ||
| // host is the Okta user domain and key is the API token to use for the query. roleID must not be empty. | ||
| // | ||
| // This call requires the okta.roles.read OAuth2 scope and only applies to custom roles (type CUSTOM). | ||
| // | ||
| // See https://developer.okta.com/docs/api/openapi/okta-management/management/tags/roleecustompermission. | ||
| func GetRolePermissions(ctx context.Context, cli *http.Client, host, key, roleID string, lim *RateLimiter, log *logp.Logger) ([]Permission, http.Header, error) { | ||
| if roleID == "" { | ||
| return nil, nil, errors.New("no role ID specified") | ||
| } | ||
|
|
||
| const endpoint = "/api/v1/iam/roles/{roleId}/permissions" | ||
| path := strings.Replace(endpoint, "{roleId}", roleID, 1) | ||
|
|
||
| u := &url.URL{ | ||
| Scheme: "https", | ||
| Host: host, | ||
| Path: path, | ||
| } | ||
| // The permissions endpoint returns {"permissions":[...]} not a plain JSON array, | ||
| // so we use permissionsWrapper with all=false to let getDetails unmarshal it as a | ||
| // single object, then unwrap the slice. | ||
| result, h, err := getDetails[permissionsWrapper](ctx, cli, u, endpoint, key, false, OmitNone, lim, log) | ||
| if err != nil || len(result) == 0 { | ||
| return nil, h, err | ||
| } | ||
| return result[0].Permissions, h, nil | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
Okta API /api/v1/users/{userId}/roles response format for custom roles - does the id field contain the role definition ID or assignment ID?
💡 Result:
The id field in the response from Okta API GET /api/v1/users/{userId}/roles contains the role assignment ID (also called roleAssignmentId), not the role definition ID. This applies to both standard and custom roles. For custom roles, the preferred assignment method is creating a role resource set binding, and the id in the list response and single retrieval is the unique identifier of that binding/assignment object. The role definition ID (e.g., starting with "cr..." for custom roles from /api/v1/iam/roles) is not directly included in the response; the response includes metadata like label and type instead. Examples from docs: - List roles: [{"id": "IFIFAX2BIRGUSTQ", "label": "Application administrator", "type": "APP_ADMIN", ...}] - Single assignment: {"id": "ra1b8anIk7rx7em7L0g4", "label": "Super Organization administrator", ...} - Path for single: /api/v1/users/{userId}/roles/{roleAssignmentId}, where roleAssignmentId is the id from the list. Official docs confirm: "The roleAssignmentId parameter is the unique identifier for either a standard role assignment object or a custom role resource set binding object."
Citations:
- 1: https://developer.okta.com/docs/api/openapi/okta-management/management/tag/RoleAssignmentAUser
- 2: https://developer.okta.com/docs/api/openapi/okta-management/management/tags/roleassignmentauser
- 3: https://preview.redoc.ly/oktadev/okta-1035720-ai-agent-beta/openapi/okta-management/management/tag/RoleAssignmentAUser/
🏁 Script executed:
# Search for calls to GetRolePermissions
rg "GetRolePermissions" x-pack/filebeat/input/entityanalytics/provider/okta/
# Also look for how roles are retrieved and what ID is used
rg "roles" x-pack/filebeat/input/entityanalytics/provider/okta/ -A 3 -B 3 | head -100Repository: elastic/beats
Length of output: 13266
🏁 Script executed:
# Look at the permissions endpoint test and how it's called
rg -A 10 "perms" x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go | grep -A 10 "permissions"
# Also check what data["permissions"] returns
rg "permissions" x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go -B 5 -A 5Repository: elastic/beats
Length of output: 5972
GetRolePermissions will fail with real Okta API; role ID mismatch.
The code passes role.ID from GetUserRoles() to the permissions endpoint, but /api/v1/users/{user}/roles returns role assignment IDs (like ra1b8anIk7rx7em7L0g4), not role definition IDs (like cr0...). The permissions endpoint /api/v1/iam/roles/{roleId}/permissions requires the role definition ID. This will fail against the real Okta API for custom roles. The test data masks this by unrealistically using definition ID format; it should be updated to use assignment IDs to catch this bug.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go`
around lines 312 - 339, GetRolePermissions is being called with role assignment
IDs (from GetUserRoles) but the permissions endpoint requires the role
definition ID; fix by changing the flow so callers pass the definition ID
(role.RoleID or equivalent) into GetRolePermissions instead of role.ID, or if
GetUserRoles doesn't currently expose the definition id, update GetUserRoles to
return the definition id field and update all callers to use that, otherwise
perform a short translation lookup (call the roles API to map assignment ID →
definition ID) before invoking GetRolePermissions; update tests so they use
assignment IDs where appropriate and assert the translation path works.
There was a problem hiding this comment.
When calling the roles endpoint, the assignment ID is the only ID returned for the role to be identified.
It should be the same that is referred to in the permissions endpoint.
There was a problem hiding this comment.
@chemamartinez, thanks for the clarification. I looked more carefully at both endpoints you linked.
Looking at the Okta docs, the picture is actually nuanced:
-
The
roleAssignmentIdis the unique identifier for either a standard role group assignment object or a custom role resource set binding object. This is theidfield returned by/api/v1/users/{userId}/roles. -
However, the permissions endpoint
/api/v1/iam/roles/{roleId}/permissionsuses IDs of the formcr0Yq6IJxGIr0ouum0g3— these are custom role definition IDs from the IAM Custom Roles API (/api/v1/iam/roles), not the assignment-level IDs returned from the user roles endpoint.
An assignment of a role to an admin is called a Binding, which you can identify by its unique ID. A binding represents a single unique combination of principal, resource set, and custom role. These are structurally different objects with different IDs.
If you have directly verified against a real Okta tenant that passing the assignment id from /api/v1/users/{userId}/roles to /api/v1/iam/roles/{roleId}/permissions works correctly, that would resolve the concern. Otherwise, the risk remains that the two IDs are not interchangeable for custom roles, which would cause silent failures (a 404 from Okta) logged only as a warning. Since permissions fetch failures are only warned (not errored), this would silently produce empty permissions without surfacing the bug.
Could you confirm this has been tested against a real Okta tenant with custom roles?
🐇 curious about the exact ID mapping in Okta's API...
🧠 Learnings used
Learnt from: ShourieG
Repo: elastic/beats PR: 48846
File: x-pack/filebeat/input/akamai/config.go:231-297
Timestamp: 2026-03-16T11:05:51.163Z
Learning: When reviewing Go code that uses cfg.Unpack(&dst) from the elastic-agent-libs config wrapper, understand that Unpack will call Validate() on dst if it implements the Validator interface and will enforce validate struct tags (e.g., validate:"required"). Do not require or flag an explicit dst.Validate() call after Unpack in factory methods like InputManager.Create(); rely on the upstream validation behavior. If a destination type should be validated, ensure it implements Validator or uses appropriate validate tags.
There was a problem hiding this comment.
Ok, I see now that when clicking on CUSTOM roles type, more fields are exposed. Among them it is the role field which is the one that must be the identifier for the custom role in the new endpoint.
There was a problem hiding this comment.
@chemamartinez, thanks for digging into the docs — that's the key finding. The CUSTOM role response includes a role field that holds the custom role definition ID (e.g., cr0...), distinct from the id which is the binding/assignment ID.
The bug is confirmed: the current Role struct doesn't capture this role field, so the code silently passes the assignment ID to the permissions endpoint instead. Since failures are only logged as warnings, this would produce empty permissions without any visible error.
Two changes are needed:
1. Add Role field to the Role struct in x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go:
type Role struct {
ID string `json:"id"`
Label string `json:"label"`
Type string `json:"type"`
Status string `json:"status"`
Created time.Time `json:"created"`
LastUpdated time.Time `json:"lastUpdated"`
AssignmentType string `json:"assignmentType"`
Role string `json:"role,omitempty"` // custom role definition ID, present only for CUSTOM type
Links HAL `json:"_links"`
Permissions []Permission `json:"permissions,omitempty"`
}2. Use role.Role in the permissions call in x-pack/filebeat/input/entityanalytics/provider/okta/okta.go:
if contains(p.cfg.EnrichWith, "perms") {
for i, role := range roles {
if role.Type == "CUSTOM" {
roleDefID := role.Role // use definition ID, not assignment ID
if roleDefID == "" {
p.logger.Warnw("custom role missing definition ID, skipping permissions fetch", "role_id", role.ID)
continue
}
perms, _, err := okta.GetRolePermissions(ctx, p.client, p.cfg.OktaDomain, p.cfg.OktaToken, roleDefID, p.lim, p.logger)
if err != nil {
p.logger.Warnw("failed to get permissions for role", "role_id", role.ID, "error", err)
continue
}
roles[i].Permissions = perms
}
}
}🧠 Learnings used
Learnt from: ShourieG
Repo: elastic/beats PR: 48846
File: x-pack/filebeat/input/akamai/config.go:231-297
Timestamp: 2026-03-16T11:05:51.163Z
Learning: When reviewing Go code that uses cfg.Unpack(&dst) from the elastic-agent-libs config wrapper, understand that Unpack will call Validate() on dst if it implements the Validator interface and will enforce validate struct tags (e.g., validate:"required"). Do not require or flag an explicit dst.Validate() call after Unpack in factory methods like InputManager.Create(); rely on the upstream validation behavior. If a destination type should be validated, ensure it implements Validator or uses appropriate validate tags.
x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go`:
- Around line 148-151: The permissions mock in okta_test.go is too permissive:
update the HTTP handler registered for "/api/v1/iam/roles/{roleId}/permissions"
to validate the roleId requested (inspect r.URL.Path or parse variables) and
only return data["permissions"] when the roleId matches the expected test
custom-role ID; return a 404 or empty response for mismatches. Also tighten the
test assertions near the permission-count checks (the block around the current
assertions at lines ~257-267) to verify the handler received the correct roleId
(e.g., assert the request path or a captured variable) in addition to permission
counts so the test fails if the implementation calls the endpoint with the wrong
ID.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 900a877c-bac4-4097-a402-75e10a875b15
📒 Files selected for processing (1)
x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go
| mux.Handle("/api/v1/iam/roles/{roleId}/permissions", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| setHeaders(w) | ||
| fmt.Fprintln(w, data["permissions"]) | ||
| })) |
There was a problem hiding this comment.
Permissions mock is too permissive and can hide broken role-ID behavior.
Line 148 accepts any {roleId} and Line 257 only checks permission counts. This test can still pass if implementation calls /api/v1/iam/roles/{roleId}/permissions with the wrong ID (e.g., assignment id instead of custom-role role) or hits non-custom roles, which can fail in real Okta tenants.
Proposed hardening
+ permCalls := map[string]int{}
mux.Handle("/api/v1/iam/roles/{roleId}/permissions", http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
setHeaders(w)
- fmt.Fprintln(w, data["permissions"])
+ roleID := r.PathValue("roleId")
+ permCalls[roleID]++
+ if roleID != "cr0Yq6IJxGIr0ouum0g3" {
+ w.WriteHeader(http.StatusNotFound)
+ return
+ }
+ fmt.Fprintln(w, data["permissions"])
}))
...
+ if slices.Contains(test.enrichWith, "perms") {
+ if permCalls["cr0Yq6IJxGIr0ouum0g3"] != len(got) {
+ t.Errorf("unexpected permissions call count for custom role: got:%d want:%d", permCalls["cr0Yq6IJxGIr0ouum0g3"], len(got))
+ }
+ if len(permCalls) != 1 {
+ t.Errorf("permissions endpoint called for non-custom role IDs: %+v", permCalls)
+ }
+ }As per coding guidelines: “Highlight only issues that could cause runtime errors” and “focus strictly on merge-blocking concerns.”
Also applies to: 257-267
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x-pack/filebeat/input/entityanalytics/provider/okta/okta_test.go` around
lines 148 - 151, The permissions mock in okta_test.go is too permissive: update
the HTTP handler registered for "/api/v1/iam/roles/{roleId}/permissions" to
validate the roleId requested (inspect r.URL.Path or parse variables) and only
return data["permissions"] when the roleId matches the expected test custom-role
ID; return a 404 or empty response for mismatches. Also tighten the test
assertions near the permission-count checks (the block around the current
assertions at lines ~257-267) to verify the handler received the correct roleId
(e.g., assert the request path or a captured variable) in addition to permission
counts so the test fails if the implementation calls the endpoint with the wrong
ID.
| // standard built-in roles have no associated permissions endpoint. | ||
| continue | ||
| } | ||
| perms, _, err := okta.GetRolePermissions(ctx, p.client, p.cfg.OktaDomain, p.getAuthToken(), role.ID, p.lim, p.logger) |
There was a problem hiding this comment.
I'm worried about the API pattern effectively being O(users * custom_roles_per_user), and this triggering rate-limits in certain orgs.
Can we cache permissions by role ID so each custom role is fetched once per run and reused for all users that carry that role?
| Fetching and shipping updates occurs in one of two processes: **full synchronizations** and **incremental updates**. Full synchronizations will send the entire list of users and devices in state, along with write markers to indicate the start and end of the synchronization event. Incremental updates will only send data for changed users and devices during that event. Changes on a user or device can come in many forms, whether it be a change to the user’s metadata, or a user was added or deleted. | ||
|
|
||
|
|
||
| #### API Interactions [_api_interactions_3] |
There was a problem hiding this comment.
The API Interactions section should be updated if any new APIs are being used.
efd6
left a comment
efd6
left a comment
There was a problem hiding this comment.
nit only, then LGTM, but please wait for @andrewkroh.
| // Permission is an Okta role permission. | ||
| // | ||
| // See https://developer.okta.com/docs/api/openapi/okta-management/management/tags/roleecustompermission. | ||
| type Permission struct { | ||
| Label string `json:"label"` | ||
| Created time.Time `json:"created"` | ||
| LastUpdated time.Time `json:"lastUpdated"` | ||
| Links HAL `json:"_links,omitempty"` | ||
| } | ||
|
|
| #### `enrich_with` [_enrich_with] | ||
|
|
||
| The metadata to enrich users with. This is an array of values that may contain "groups", "roles" and "factors", or "none". If the array only contains "none", no metadata is collected for users. The default behavior is to collect "groups". | ||
| The metadata to enrich users with. This is an array of values that may contain "groups", "roles", "factors", "perms", or "none". If the array only contains "none", no metadata is collected for users. The default behavior is to collect "groups". |
There was a problem hiding this comment.
Note regarding Vale rules:
- punctuation outside quotes in natural English is conventional, but in this case it is not helpful; the strings are intended to be a verbatim representation of a string that can be included in the configuration, and using the English convention would confuse that.
- the use of "may" here has the sense of permission, not capacity ("can") or possibility ("might"), and so the suggestions are incorrect.
cc @theletterf
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go`:
- Around line 335-339: The getDetails call path that returns a single object
(used by the caller expecting permissions via permissionsWrapper) is
unmarshalling the response body without checking resp.StatusCode, so non-2xx
Okta errors decode into zero-value permissionsWrapper and return empty
permissions; update getDetails to check resp.StatusCode (e.g., accept 2xx)
before attempting to json.Unmarshal into the target type, and on non-2xx read
the response body and return a descriptive error (or decode into the error
payload) instead of the zero-value result so callers of getDetails (and the
caller that relies on result[0].Permissions) receive a non-nil error; ensure
this behavior is applied for the single-object path that uses permissionsWrapper
and that getDetails still returns the original headers (h) and error
consistently.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 087abec1-63dc-40e6-ae7e-5409e802e1b3
📒 Files selected for processing (2)
docs/reference/filebeat/filebeat-input-entity-analytics.mdx-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go
| result, h, err := getDetails[permissionsWrapper](ctx, cli, u, endpoint, key, false, OmitNone, lim, log) | ||
| if err != nil || len(result) == 0 { | ||
| return nil, h, err | ||
| } | ||
| return result[0].Permissions, h, nil |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Go's encoding/json package, if a JSON object only contains fields unknown to the destination struct, does json.Unmarshal return nil error and leave the struct at its zero value?
💡 Result:
No, json.Unmarshal does not leave the struct at its zero value when the JSON object only contains fields unknown to the destination struct. Instead, it returns nil error and ignores the unknown fields, resulting in a struct where all fields remain at their zero values since no matching fields were populated.
Citations:
- 1: https://oneuptime.com/blog/post/2026-01-23-go-json-unknown-fields/view
- 2: https://go.dev/src/encoding/json/decode.go?s=4043:4091
- 3: https://stackoverflow.com/questions/75318601/golang-unmarshalling-behaviour-excess-fields
- 4: https://pkg.go.dev/encoding/json/
- 5: https://pkg.go.dev/encoding/json@go1.25.6
🏁 Script executed:
cd x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta && \
sed -n '330,345p' okta.go && \
echo "---" && \
sed -n '480,510p' okta.goRepository: elastic/beats
Length of output: 1462
Check HTTP status before unmarshalling in getDetails single-object path.
Lines 489–495 unmarshal the response body directly into the target struct without validating resp.StatusCode. When a non-2xx Okta error response (e.g., 403 for missing okta.roles.read scope) contains unknown JSON fields, it decodes into permissionsWrapper{} as a zero-value struct with nil error. This causes line 339 to return empty permissions silently instead of propagating the error.
The error-detection strategy (relying on type mismatch between error objects and array responses) fails for single-object endpoints. Add status code validation before unmarshalling to catch non-2xx responses uniformly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@x-pack/filebeat/input/entityanalytics/provider/okta/internal/okta/okta.go`
around lines 335 - 339, The getDetails call path that returns a single object
(used by the caller expecting permissions via permissionsWrapper) is
unmarshalling the response body without checking resp.StatusCode, so non-2xx
Okta errors decode into zero-value permissionsWrapper and return empty
permissions; update getDetails to check resp.StatusCode (e.g., accept 2xx)
before attempting to json.Unmarshal into the target type, and on non-2xx read
the response body and return a descriptive error (or decode into the error
payload) instead of the zero-value result so callers of getDetails (and the
caller that relies on result[0].Permissions) receive a non-nil error; ensure
this behavior is applied for the single-object path that uses permissionsWrapper
and that getDetails still returns the original headers (h) and error
consistently.
5 participants
Proposed commit message
Checklist
stresstest.shscript to run them under stress conditions and race detector to verify their stability../changelog/fragmentsusing the changelog tool.Related issues