feat: sync app user profile attributes as grant metadata#146
feat: sync app user profile attributes as grant metadata#146c1-dev-bot[bot] wants to merge 3 commits into
Conversation
Include app-specific user profile attributes (assigned apps, scopes, custom fields) in grant metadata when syncing app user and group assignments. This surfaces Okta app assignment profile data in ConductorOne, enabling visibility into app-specific attributes like assigned scopes that are set on user profiles within Okta apps. Changes: - Add appUserProfileToMetadata() to convert AppUser profile to grant metadata, including scope, status, and externalId fields - Add appGroupAssignmentProfileToMetadata() for group assignment profiles - Attach profile metadata to user app grants via WithGrantMetadata - Attach profile metadata to group app grants via WithGrantMetadata
|
Claude finished @c1-dev-bot[bot]'s task in 1m 38s —— View job PR Review: Sync app user profile attributes as grant metadata
SummaryThis is a clean, well-structured PR that adds two helper functions ( What looks good
Issues foundPanic risk with Missing Minor observations (non-blocking)
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| if len(profile) == 0 { | ||
| return nil | ||
| } | ||
|
|
||
| metadata := make(map[string]interface{}) | ||
| for k, v := range profile { | ||
| metadata[k] = v | ||
| } | ||
|
|
||
| // Include scope and status from the app user assignment itself | ||
| if appUser.Scope != "" { | ||
| metadata["_scope"] = appUser.Scope | ||
| } | ||
| if appUser.Status != "" { | ||
| metadata["_status"] = appUser.Status | ||
| } | ||
| if appUser.ExternalId != "" { | ||
| metadata["_externalId"] = appUser.ExternalId | ||
| } | ||
|
|
||
| return metadata |
There was a problem hiding this comment.
Potential panic risk: WithGrantMetadata calls structpb.NewStruct(metadata), and if that returns an error, NewGrant will panic (see grant.go:90). While JSON-deserialized profiles should contain only structpb-compatible types, Okta app profiles are arbitrary and could theoretically contain values that structpb can't handle (e.g., deeply nested structures or unexpected types from custom schema extensions).
Consider either:
- Pre-validating the metadata with
structpb.NewStruct()before passing it, logging and skipping on error, or - Converting all values to strings (safest for arbitrary profile data)
This is a low-probability issue since JSON-unmarshaled maps typically contain only compatible types, but since the failure mode is a panic (not a returned error), it's worth hardening.
| if len(profile) == 0 { | |
| return nil | |
| } | |
| metadata := make(map[string]interface{}) | |
| for k, v := range profile { | |
| metadata[k] = v | |
| } | |
| // Include scope and status from the app user assignment itself | |
| if appUser.Scope != "" { | |
| metadata["_scope"] = appUser.Scope | |
| } | |
| if appUser.Status != "" { | |
| metadata["_status"] = appUser.Status | |
| } | |
| if appUser.ExternalId != "" { | |
| metadata["_externalId"] = appUser.ExternalId | |
| } | |
| return metadata | |
| metadata := make(map[string]interface{}) | |
| for k, v := range profile { | |
| metadata[k] = v | |
| } | |
| // Include scope and status from the app user assignment itself | |
| if appUser.Scope != "" { | |
| metadata["_scope"] = appUser.Scope | |
| } | |
| if appUser.Status != "" { | |
| metadata["_status"] = appUser.Status | |
| } | |
| if appUser.ExternalId != "" { | |
| metadata["_externalId"] = appUser.ExternalId | |
| } | |
| // Validate that metadata is structpb-compatible to avoid panic in NewGrant | |
| if _, err := structpb.NewStruct(metadata); err != nil { | |
| return nil | |
| } | |
| return metadata |
| return nil | ||
| } | ||
|
|
||
| metadata := make(map[string]interface{}) | ||
| for k, v := range profile { | ||
| metadata[k] = v | ||
| } | ||
|
|
||
| return metadata |
There was a problem hiding this comment.
Same panic risk as appUserProfileToMetadata — if the profile contains structpb-incompatible values, NewGrant will panic. Consider adding the same validation here.
| return nil | |
| } | |
| metadata := make(map[string]interface{}) | |
| for k, v := range profile { | |
| metadata[k] = v | |
| } | |
| return metadata | |
| metadata := make(map[string]interface{}) | |
| for k, v := range profile { | |
| metadata[k] = v | |
| } | |
| // Validate that metadata is structpb-compatible to avoid panic in NewGrant | |
| if _, err := structpb.NewStruct(metadata); err != nil { | |
| return nil | |
| } | |
| return metadata |
Address review feedback: ensure profile metadata values are compatible with structpb.NewStruct before passing to WithGrantMetadata. Adds toStructpbCompatibleValue() to coerce non-primitive types to strings, and validates the full metadata map with structpb.NewStruct, returning nil if conversion would fail.
|
Claude finished @c1-dev-bot[bot]'s task in 1m 26s —— View job PR Review: Sync app user profile attributes as grant metadata
SummaryThis PR adds two helper functions to extract Okta app user and group assignment profile attributes and attach them as grant metadata via What looks good
Issues1. Silent data loss on validation failure (low severity, non-blocking) At if _, err := structpb.NewStruct(metadata); err != nil {
// Consider: logger.Debug("skipping invalid profile metadata", zap.Error(err))
return nil
}This is non-blocking since the functions don't currently have access to a logger, and adding one would change the function signatures. But it's worth considering for future observability. 2. Precision loss for large integer IDs (low severity, non-blocking) In case int64:
if val > 1<<53 || val < -(1<<53) {
return strconv.FormatInt(val, 10)
}
return float64(val)This is unlikely to be an issue in practice with Okta profiles, so non-blocking. 3. Double conversion overhead (observation, non-blocking) The code first normalizes all values via Minor observations
VerdictThe PR is well-implemented and the second commit properly addresses the panic risk from the initial review. The remaining observations are all non-blocking. This looks good to merge after manual testing with an Okta instance confirms the metadata appears correctly. |
The gosec linter no longer flags the ApiToken field name, so the //nolint:gosec directive is unused and triggers a nolintlint error.
|
Claude finished @c1-dev-bot[bot]'s task in 1m 34s —— View job PR Review: Sync app user profile attributes as grant metadata
SummaryThis PR adds grant metadata for app user and group assignment profiles across 3 commits: the initial feature ( What looks good
Issues1. Silent data loss on validation failure (low, non-blocking) At 2. Precision loss for large integer IDs (low, non-blocking) In 3. Double allocation in validation (observation, non-blocking) The code first normalizes values via Minor observations
VerdictThis is a solid, well-implemented PR. The panic prevention is correctly placed, the code is consistent between user and group paths, and the helper functions are well-guarded. The remaining observations are all non-blocking. Looks good to merge after manual testing with an Okta instance confirms metadata appears correctly. |
Summary
GrantMetadataannotationsDetails
When syncing app grants, the connector now attaches the app user's profile attributes as grant metadata. This enables ConductorOne to surface app-specific attributes that are set on the user's assignment in Okta, such as:
Key Changes in
pkg/connector/app.go:appUserProfileToMetadata(): Extracts AppUser profile fields plus assignment-level fields (scope, status, externalId) prefixed with_appGroupAssignmentProfileToMetadata(): Extracts ApplicationGroupAssignment profile fieldslistAppUsersGrants()andlistAppGroupGrants()to attach profile metadata to grantsThis follows the same pattern used in AWS mode (
aws_account.go) for managing app user profile attributes likesamlRoles.Test Plan
go build ./...passesgo test ./...passesgo vet ./...passesFixes: CE-526
Automated PR Notice
This PR was automatically created by c1-dev-bot as a potential implementation.
This code requires: