From 6fe0d7c6d521be44b14cfe163dd24a1857d50e08 Mon Sep 17 00:00:00 2001 From: agustin-conductor Date: Fri, 28 Aug 2026 16:28:50 -0300 Subject: [PATCH] Distinguish role assignment from unassignment in the privilege filter Okta reports both assignment and unassignment of standard admin roles under user.account.privilege.grant, telling them apart with an extra target typed ROLE_ASSIGNED or ROLE_UNASSIGNED. RoleMembershipFilter read only the ROLE and User targets and emitted a CreateGrantEvent unconditionally, so a revocation reached C1 as an affirmation of the role the user had just lost, and stayed granted until the next full sync. user.account.privilege.revoke fires only when the last role is removed, so the revoke filter could not reach a partial revocation by construction. Branch on the discriminator: ROLE_ASSIGNED emits a grant, ROLE_UNASSIGNED a revoke. The check runs before the ROLE target is read, because custom role bindings reuse the event type and carry no ROLE target. Skip rather than guess when no discriminator is recognised. Two cases reach this: custom role bindings, which belong to the custom-role resource type; and the _GROUP_ROLE_CHANGE / _GROUP_CHANGE variants Okta emits per member when a role is assigned to a group. Those carry no Group target at all, so the group that conferred the role is unknowable from the event, while the sync models the access as a group-principal grant with GrantExpandable. Emitting a user principal would contradict the sync and flap. Defaulting an unrecognised change to a grant would reaffirm access that may have just been removed. An unresolvable role label now skips too, in both role filters, rather than returning an error per revocation -- an unmapped label is an upstream condition, not a connector fault. Skipping is expressed by leaving rv.Event unset. Handle turns that into a nil event, so no handler can hand the feed consumer an Event with no oneof set -- there it is bucketed as unknown, which fails the whole batch without advancing the checkpoint, leaving the feed to re-fetch the same window indefinitely. Adds the first test coverage for RoleMembershipFilter. Co-Authored-By: Claude Opus 5 (1M context) --- pkg/connector/event_filter.go | 8 + pkg/connector/event_filters.go | 76 +++++++-- pkg/connector/event_filters_test.go | 229 ++++++++++++++++++++++++---- pkg/connector/event_log.go | 9 +- pkg/connector/profile_keys.go | 8 + 5 files changed, 292 insertions(+), 38 deletions(-) diff --git a/pkg/connector/event_filter.go b/pkg/connector/event_filter.go index eac97a2d..ecf61c6a 100644 --- a/pkg/connector/event_filter.go +++ b/pkg/connector/event_filter.go @@ -22,6 +22,7 @@ type EventFilter struct { // May contain additional targets. TargetTypes mapset.Set[string] // Required, will be called for each event that matches the filter. + // Leave rv.Event unset to skip the event without emitting anything. EventHandler func(*zap.Logger, *oktaSDK.LogEvent, map[string][]*oktaSDK.LogTarget, *v2.Event) error } @@ -108,5 +109,12 @@ func (filter *EventFilter) Handle(l *zap.Logger, event *oktaSDK.LogEvent) (*v2.E return nil, err } + // A handler that set no event chose to skip it. Returning rv here would hand the + // feed consumer an Event with no oneof set, which it buckets as unknown and which + // fails the whole batch without advancing the checkpoint. + if rv.Event == nil { + return nil, nil + } + return rv, nil } diff --git a/pkg/connector/event_filters.go b/pkg/connector/event_filters.go index 2b7531a4..21d344c0 100644 --- a/pkg/connector/event_filters.go +++ b/pkg/connector/event_filters.go @@ -264,9 +264,35 @@ var ( }, } RoleMembershipFilter = EventFilter{ + // Okta reports both assignment and unassignment of standard admin roles under + // this one event type, distinguishing them with an extra ROLE_ASSIGNED or + // ROLE_UNASSIGNED target, so an unassignment has to emit a revoke. Custom role + // bindings (CUSTOM_ROLE_BINDING_ADDED / _REMOVED) reuse the event type as well + // but carry no ROLE target and belong to the custom-role resource type, so they + // are skipped here. + // + // user.account.privilege.revoke covers only the removal of a user's last role; + // RoleMembershipRevokeFilter handles that case. EventTypes: mapset.NewSet("user.account.privilege.grant"), TargetTypes: mapset.NewSet("ROLE", oktaLogTargetTypeUser), - EventHandler: func(_ *zap.Logger, event *oktaSDK.LogEvent, targetMap map[string][]*oktaSDK.LogTarget, rv *v2.Event) error { + EventHandler: func(l *zap.Logger, event *oktaSDK.LogEvent, targetMap map[string][]*oktaSDK.LogTarget, rv *v2.Event) error { + assigned := len(targetMap[oktaLogTargetRoleAssigned]) > 0 + unassigned := len(targetMap[oktaLogTargetRoleUnassigned]) > 0 + + switch { + case assigned && unassigned: + return fmt.Errorf("okta-connectorv2: event has both %s and %s targets", oktaLogTargetRoleAssigned, oktaLogTargetRoleUnassigned) + case !assigned && !unassigned: + // Custom role bindings land here, as would any discriminator Okta adds + // later. Defaulting to a grant would reaffirm access that may have just + // been removed, so skip rather than guess. + l.Debug("okta-event-feed: RoleMembershipFilter: no role assignment discriminator, skipping", + zap.String("event_type", event.EventType), + zap.String("event_id", event.Uuid), + ) + return nil + } + if len(targetMap["ROLE"]) != 1 { return fmt.Errorf("okta-connectorv2: expected 1 ROLE target, got %d", len(targetMap["ROLE"])) } @@ -277,11 +303,18 @@ var ( } user := targetMap[oktaLogTargetTypeUser][0] - // for some reason we don't get the role ID (or type) formatted properly. - // hack to look it up via DisplayName + // The ROLE target's id is a camelCase name ("UserAdmin") and its alternateId + // is an assignment identifier, so neither is the role type the sync uses as a + // resource ID. Look it up by label instead. roleType := StandardRoleTypeFromLabel(role.DisplayName) if roleType == nil { - return fmt.Errorf("okta-connectorv2: error getting role from label: %s", role.DisplayName) + // Expected for custom roles and for any label missing from + // standardRoleTypes; there is no resource to point an event at. + l.Debug("okta-event-feed: RoleMembershipFilter: no standard role for label, skipping", + zap.String("role_display_name", role.DisplayName), + zap.String("event_id", event.Uuid), + ) + return nil } roleResource, err := sdkResource.NewResource(role.DisplayName, resourceTypeRole, roleType.Type) @@ -300,12 +333,31 @@ var ( } principal.Annotations = annotations.New(userTrait) - rv.Event = &v2.Event_CreateGrantEvent{ - CreateGrantEvent: &v2.CreateGrantEvent{ - Entitlement: sdkEntitlement.NewAssignmentEntitlement(roleResource, "assigned"), - Principal: principal, - }, + entitlement := sdkEntitlement.NewAssignmentEntitlement(roleResource, "assigned") + if assigned { + rv.Event = &v2.Event_CreateGrantEvent{ + CreateGrantEvent: &v2.CreateGrantEvent{ + Entitlement: entitlement, + Principal: principal, + }, + } + } else { + rv.Event = &v2.Event_CreateRevokeEvent{ + CreateRevokeEvent: &v2.CreateRevokeEvent{ + Entitlement: entitlement, + Principal: principal, + }, + } } + + l.Debug("okta-event-feed: RoleMembershipFilter", + zap.String("event_type", event.EventType), + zap.Bool("assigned", assigned), + zap.String("resource_type", resourceTypeRole.Id), + zap.String("resource_id", roleType.Type), + zap.String("role_display_name", role.DisplayName), + zap.String("user_id", user.Id), + ) return nil }, } @@ -327,7 +379,11 @@ var ( // role ID or type, so resolve the standard role by its label. roleType := StandardRoleTypeFromLabel(role.DisplayName) if roleType == nil { - return fmt.Errorf("okta-connectorv2: error getting role from label: %s", role.DisplayName) + l.Debug("okta-event-feed: RoleMembershipRevokeFilter: no standard role for label, skipping", + zap.String("role_display_name", role.DisplayName), + zap.String("event_id", event.Uuid), + ) + return nil } roleResource, err := sdkResource.NewResource(role.DisplayName, resourceTypeRole, roleType.Type) diff --git a/pkg/connector/event_filters_test.go b/pkg/connector/event_filters_test.go index 8b677534..feec9de1 100644 --- a/pkg/connector/event_filters_test.go +++ b/pkg/connector/event_filters_test.go @@ -14,7 +14,7 @@ import ( func logEvent(eventType string, targets ...*oktaSDK.LogTarget) *oktaSDK.LogEvent { published := time.Date(2026, time.August, 25, 12, 0, 0, 0, time.UTC) return &oktaSDK.LogEvent{ - Uuid: "b0f1c2d3-0000-4000-8000-000000000001", + Uuid: "00000000-0000-4000-8000-000000000001", EventType: eventType, Published: &published, Actor: &oktaSDK.LogActor{Type: oktaLogTargetTypeUser, Id: "actor1"}, @@ -78,15 +78,17 @@ func TestRevokeFilters(t *testing.T) { } } -// An unknown role label has no standard type to resolve, so the event is -// rejected rather than emitted against a bogus resource ID. +// An unknown role label has no standard type to resolve, so the event is skipped +// rather than emitted against a bogus resource ID. Skipped, not errored: an +// unmapped label is an upstream condition, not a connector fault. func TestRoleMembershipRevokeFilterUnknownLabel(t *testing.T) { event := logEvent("user.account.privilege.revoke", &oktaSDK.LogTarget{Type: "ROLE", DisplayName: "Some Custom Role"}, &oktaSDK.LogTarget{Type: oktaLogTargetTypeUser, Id: "user1"}, ) - _, err := RoleMembershipRevokeFilter.Handle(zap.NewNop(), event) - require.ErrorContains(t, err, "error getting role from label") + rv, err := RoleMembershipRevokeFilter.Handle(zap.NewNop(), event) + require.NoError(t, err) + require.Nil(t, rv, "a skipped event must not reach the feed") } // A filter that is not in activeFilters is never queried, so registration is the @@ -101,27 +103,27 @@ func TestRevokeFiltersAreRegistered(t *testing.T) { require.True(t, queried.Contains("user.account.privilege.revoke")) } -// Real payload from a test tenant. Okta sends a third target whose type is +// Example payload showing the shape Okta sends. A third target of type // ROLE_UNASSIGNED_ALL_PRIVILEGES_REVOKED, and the ROLE target's id // ("HelpDeskAdmin") is neither the role type the sync uses as a resource ID // nor its alternateId — hence the displayName lookup. const privilegeRevokeFixture = `{ - "actor": {"id":"00ux6rfqpqkPp4A72697","type":"User","alternateId":"admin@example.com","displayName":"Admin"}, + "actor": {"id":"00uEXAMPLEACTOR00001","type":"User","alternateId":"admin@example.com","displayName":"Admin"}, "eventType": "user.account.privilege.revoke", "outcome": {"result":"SUCCESS"}, "published": "2026-08-25T17:41:22.539Z", - "uuid": "30010c48-a0ac-11f1-ae0a-95c6e5af725d", + "uuid": "00000000-0000-4000-8000-000000000002", "target": [ - {"id":"00u12xvaouixjW1y3698","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, {"id":"ROLE_UNASSIGNED_ALL_PRIVILEGES_REVOKED", "type":"ROLE_UNASSIGNED_ALL_PRIVILEGES_REVOKED", "alternateId":"unknown", "displayName":"All Privileges revoked from User. User has no admin privileges"}, - {"id":"HelpDeskAdmin","type":"ROLE","alternateId":"JBCUYUC7IRCVGS27IFCE2SKO","displayName":"Help Desk Administrator"} + {"id":"HelpDeskAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID1","displayName":"Help Desk Administrator"} ] }` -func TestRoleMembershipRevokeFilterRealPayload(t *testing.T) { +func TestRoleMembershipRevokeFilterExtraTargets(t *testing.T) { event := &oktaSDK.LogEvent{} require.NoError(t, json.Unmarshal([]byte(privilegeRevokeFixture), event)) @@ -131,14 +133,14 @@ func TestRoleMembershipRevokeFilterRealPayload(t *testing.T) { rv, err := RoleMembershipRevokeFilter.Handle(zap.NewNop(), event) require.NoError(t, err) - require.Equal(t, "30010c48-a0ac-11f1-ae0a-95c6e5af725d", rv.Id) + require.Equal(t, "00000000-0000-4000-8000-000000000002", rv.Id) revoke := rv.GetCreateRevokeEvent() require.NotNil(t, revoke) // Must equal the entitlement the sync builds in role.go, or c1 cannot // resolve it and silently drops the event. require.Equal(t, "role:HELP_DESK_ADMIN:assigned", revoke.GetEntitlement().GetId()) - require.Equal(t, "00u12xvaouixjW1y3698", revoke.GetPrincipal().GetId().GetResource()) + require.Equal(t, "00uEXAMPLEUSER000001", revoke.GetPrincipal().GetId().GetResource()) } // Okta has no "app.lifecycle.*" namespace; requesting it silently matched nothing. @@ -172,21 +174,21 @@ func TestApplicationLifecycleFilterEventTypes(t *testing.T) { require.Equal(t, resourceTypeApp.Id, change.GetResourceId().GetResourceType()) } -// Real payloads from a test tenant. Okta sends three targets — AppUser, +// Example payloads showing the shape Okta sends: three targets — AppUser, // AppInstance and User — so the extra AppUser bucket must not disturb the // single-AppInstance / single-User expectations. const appMembershipRemoveFixture = `[ - {"eventType":"application.user_membership.remove","published":"2026-08-25T17:41:22.359Z","uuid":"2fe59500-a0ac-11f1-ae0a-95c6e5af725d", - "target":[{"id":"0ua16tpeb9kz6QXVf698","type":"AppUser","alternateId":"unknown","displayName":"Target User"}, - {"id":"0oax6rfqm2b7mnaGs697","type":"AppInstance","alternateId":"Okta Admin Console","displayName":"Okta Admin Console"}, - {"id":"00u12xvaouixjW1y3698","type":"User","alternateId":"user@example.com","displayName":"Target User"}]}, - {"eventType":"application.user_membership.remove","published":"2026-08-25T18:19:21.524Z","uuid":"7e628fc1-a0b1-11f1-bf59-8b5627020a34", - "target":[{"id":"0ua16tp99dorEApPa698","type":"AppUser","alternateId":"user@example.com","displayName":"Target User"}, - {"id":"0oa16scusspsHf5ab698","type":"AppInstance","alternateId":"TestWEbApp01","displayName":"OpenID Connect Client"}, - {"id":"00u12xvaouixjW1y3698","type":"User","alternateId":"user@example.com","displayName":"Target User"}]} + {"eventType":"application.user_membership.remove","published":"2026-08-25T17:41:22.359Z","uuid":"00000000-0000-4000-8000-000000000003", + "target":[{"id":"0uaEXAMPLEAPPUSER001","type":"AppUser","alternateId":"unknown","displayName":"Target User"}, + {"id":"0oaEXAMPLEADMINAPP01","type":"AppInstance","alternateId":"Okta Admin Console","displayName":"Okta Admin Console"}, + {"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}]}, + {"eventType":"application.user_membership.remove","published":"2026-08-25T18:19:21.524Z","uuid":"00000000-0000-4000-8000-000000000004", + "target":[{"id":"0uaEXAMPLEAPPUSER002","type":"AppUser","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"0oaEXAMPLEAPP0000001","type":"AppInstance","alternateId":"ExampleWebApp","displayName":"OpenID Connect Client"}, + {"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}]} ]` -func TestApplicationMembershipRevokeFilterRealPayload(t *testing.T) { +func TestApplicationMembershipRevokeFilterAppUserTarget(t *testing.T) { var events []*oktaSDK.LogEvent require.NoError(t, json.Unmarshal([]byte(appMembershipRemoveFixture), &events)) require.Len(t, events, 2) @@ -194,8 +196,8 @@ func TestApplicationMembershipRevokeFilterRealPayload(t *testing.T) { // Entitlement IDs must equal what app.go builds from app.Id, or c1 cannot // resolve them and silently drops the event. wantEntitlements := []string{ - "app:0oax6rfqm2b7mnaGs697:access", - "app:0oa16scusspsHf5ab698:access", + "app:0oaEXAMPLEADMINAPP01:access", + "app:0oaEXAMPLEAPP0000001:access", } for i, event := range events { @@ -208,6 +210,181 @@ func TestApplicationMembershipRevokeFilterRealPayload(t *testing.T) { revoke := rv.GetCreateRevokeEvent() require.NotNil(t, revoke) require.Equal(t, wantEntitlements[i], revoke.GetEntitlement().GetId()) - require.Equal(t, "00u12xvaouixjW1y3698", revoke.GetPrincipal().GetId().GetResource()) + require.Equal(t, "00uEXAMPLEUSER000001", revoke.GetPrincipal().GetId().GetResource()) + } +} + +// Example payloads showing the shape Okta sends. Assignment and unassignment share +// under the same user.account.privilege.grant type, so the ROLE_ASSIGNED / +// ROLE_UNASSIGNED target is the only thing separating a grant from a revoke. +// Note debugData.privilegeGranted lists the privileges remaining after the +// change, not the change itself, so it cannot serve as the discriminator. +const privilegeGrantFixture = `[ + {"eventType":"user.account.privilege.grant","published":"2026-08-28T16:24:11.960Z", + "uuid":"00000000-0000-4000-8000-000000000005", + "debugContext":{"debugData":{"privilegeGranted":"User administrator (all)"}}, + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED","type":"ROLE_ASSIGNED","alternateId":"unknown","displayName":"Role Assigned"}, + {"id":"UserAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID2","displayName":"Group Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T16:24:39.708Z", + "uuid":"00000000-0000-4000-8000-000000000006", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED","type":"ROLE_ASSIGNED","alternateId":"unknown","displayName":"Role Assigned"}, + {"id":"AppAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID3","displayName":"Application Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T16:30:42.694Z", + "uuid":"00000000-0000-4000-8000-000000000007", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED","type":"ROLE_ASSIGNED","alternateId":"unknown","displayName":"Role Assigned"}, + {"id":"ApiAccessManagementAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID4","displayName":"API Access Management Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T16:32:57.838Z", + "uuid":"00000000-0000-4000-8000-000000000008", + "debugContext":{"debugData":{"privilegeGranted":"User administrator (all), API Access Management administrator"}}, + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_UNASSIGNED","type":"ROLE_UNASSIGNED","alternateId":"unknown","displayName":"Role Unassigned"}, + {"id":"AppAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID3","displayName":"Application Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T17:33:38.480Z", + "uuid":"00000000-0000-4000-8000-000000000009", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED","type":"ROLE_ASSIGNED","alternateId":"unknown","displayName":"Role Assigned"}, + {"id":"ReportAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID5","displayName":"Report Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T17:33:39.022Z", + "uuid":"00000000-0000-4000-8000-000000000010", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED","type":"ROLE_ASSIGNED","alternateId":"unknown","displayName":"Role Assigned"}, + {"id":"MobileAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID6","displayName":"Mobile Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T17:33:39.464Z", + "uuid":"00000000-0000-4000-8000-000000000011", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED","type":"ROLE_ASSIGNED","alternateId":"unknown","displayName":"Role Assigned"}, + {"id":"GroupMembershipAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID7","displayName":"Group Membership Administrator"}]} +]` + +func TestRoleMembershipFilterDiscriminatesAssignment(t *testing.T) { + var events []*oktaSDK.LogEvent + require.NoError(t, json.Unmarshal([]byte(privilegeGrantFixture), &events)) + require.Len(t, events, 7) + + want := []struct { + entitlement string + isGrant bool + }{ + {"role:USER_ADMIN:assigned", true}, + {"role:APP_ADMIN:assigned", true}, + {"role:API_ACCESS_MANAGEMENT_ADMIN:assigned", true}, + // Same event type as the three above, and the same role as the second: + // only the ROLE_UNASSIGNED target makes this a revoke. + {"role:APP_ADMIN:assigned", false}, + // Three roles assigned in a single UI action. Okta logs one event per role + // rather than one event carrying three ROLE targets, so each keeps a single + // ROLE target and the len(...) != 1 guard holds. + {"role:REPORT_ADMIN:assigned", true}, + {"role:MOBILE_ADMIN:assigned", true}, + {"role:GROUP_MEMBERSHIP_ADMIN:assigned", true}, + } + + for i, event := range events { + require.True(t, RoleMembershipFilter.Matches(event), "event %d", i) + + rv, err := RoleMembershipFilter.Handle(zap.NewNop(), event) + require.NoError(t, err, "event %d", i) + require.Equal(t, event.Uuid, rv.Id) + + if want[i].isGrant { + grant := rv.GetCreateGrantEvent() + require.NotNil(t, grant, "event %d must be a grant", i) + require.Nil(t, rv.GetCreateRevokeEvent()) + require.Equal(t, want[i].entitlement, grant.GetEntitlement().GetId()) + require.Equal(t, "00uEXAMPLEUSER000001", grant.GetPrincipal().GetId().GetResource()) + continue + } + + revoke := rv.GetCreateRevokeEvent() + require.NotNil(t, revoke, "event %d must be a revoke, not a grant", i) + require.Nil(t, rv.GetCreateGrantEvent(), "an unassignment must never emit a grant") + require.Equal(t, want[i].entitlement, revoke.GetEntitlement().GetId()) + require.Equal(t, "00uEXAMPLEUSER000001", revoke.GetPrincipal().GetId().GetResource()) + } +} + +// Custom role bindings share the event type but carry no ROLE target, and no +// discriminator at all means Okta changed something we do not understand. +// Either way, emitting a grant would reaffirm possibly-removed access. +func TestRoleMembershipFilterSkipsUnknownDiscriminator(t *testing.T) { + for _, tt := range []struct { + name string + targets []*oktaSDK.LogTarget + }{ + { + name: "custom role binding removed", + targets: []*oktaSDK.LogTarget{ + {Type: oktaLogTargetTypeUser, Id: "user1"}, + {Type: "CUSTOM_ROLE_BINDING_REMOVED", Id: "CUSTOM_ROLE_BINDING_REMOVED"}, + }, + }, + { + name: "no discriminator", + targets: []*oktaSDK.LogTarget{ + {Type: oktaLogTargetTypeUser, Id: "user1"}, + {Type: "ROLE", DisplayName: "Application Administrator"}, + }, + }, + } { + t.Run(tt.name, func(t *testing.T) { + event := logEvent("user.account.privilege.grant", tt.targets...) + rv, err := RoleMembershipFilter.Handle(zap.NewNop(), event) + require.NoError(t, err) + require.Nil(t, rv, "a skipped event must not reach the feed") + }) + } +} + +// An unresolvable label is an expected upstream condition, not a connector +// fault, so it must skip rather than surface as an error per revocation. +func TestRoleMembershipFilterSkipsUnknownLabel(t *testing.T) { + event := logEvent("user.account.privilege.grant", + &oktaSDK.LogTarget{Type: oktaLogTargetTypeUser, Id: "user1"}, + &oktaSDK.LogTarget{Type: oktaLogTargetRoleUnassigned, Id: oktaLogTargetRoleUnassigned}, + &oktaSDK.LogTarget{Type: "ROLE", DisplayName: "Some Custom Role"}, + ) + rv, err := RoleMembershipFilter.Handle(zap.NewNop(), event) + require.NoError(t, err) + require.Nil(t, rv, "a skipped event must not reach the feed") +} + +// Example payloads for the group-derived shape. Assigning a role to a group produces one +// event per group member, each marked with a _GROUP_ suffix on the discriminator +// and carrying no Group target at all. The group that conferred the role is +// therefore unknowable from the event, while the sync models this access as a +// group-principal grant (role.go roleGroupGrant, with GrantExpandable). Emitting a +// user-principal grant here would contradict the sync and flap. Skip instead. +const groupDerivedPrivilegeFixture = `[ + {"eventType":"user.account.privilege.grant","published":"2026-08-28T18:23:44.250Z", + "uuid":"00000000-0000-4000-8000-000000000012", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"ROLE_ASSIGNED_GROUP_ROLE_CHANGE","type":"ROLE_ASSIGNED_GROUP_ROLE_CHANGE","alternateId":"unknown","displayName":"Role Assigned from Group Role Change"}, + {"id":"UserAdmin","type":"ROLE","alternateId":"EXAMPLEASSIGNMENTID2","displayName":"Group Administrator"}]}, + {"eventType":"user.account.privilege.grant","published":"2026-08-28T18:23:44.588Z", + "uuid":"00000000-0000-4000-8000-000000000013", + "target":[{"id":"00uEXAMPLEUSER000001","type":"User","alternateId":"user@example.com","displayName":"Target User"}, + {"id":"CUSTOM_ROLE_BINDING_ADDED_GROUP_CHANGE","type":"CUSTOM_ROLE_BINDING_ADDED_GROUP_CHANGE","alternateId":"unknown","displayName":"Custom role binding added from Group Change"}, + {"id":"cr0EXAMPLEROLE000001","type":"CUSTOM_ROLE","alternateId":"/api/v1/iam/roles/cr0EXAMPLEROLE000001","displayName":"Example Custom Role"}, + {"id":"iamEXAMPLERSET000001","type":"RESOURCE_SET","alternateId":"/api/v1/iam/resource-sets/iamEXAMPLERSET000001","displayName":"Example Resource Set"}]} +]` + +func TestRoleMembershipFilterSkipsGroupDerivedChanges(t *testing.T) { + var events []*oktaSDK.LogEvent + require.NoError(t, json.Unmarshal([]byte(groupDerivedPrivilegeFixture), &events)) + require.Len(t, events, 2) + + for _, event := range events { + // The event still matches: it carries a User target, and the standard-role + // variant carries a ROLE target too. + require.True(t, RoleMembershipFilter.Matches(event)) + + // A _GROUP_ suffix must never be read as the direct discriminator it + // prefixes, or an inherited role reaches C1 as a direct grant. + rv, err := RoleMembershipFilter.Handle(zap.NewNop(), event) + require.NoError(t, err) + require.Nil(t, rv, "an inherited role change must not reach the feed") } } diff --git a/pkg/connector/event_log.go b/pkg/connector/event_log.go index 36e16e4c..8a0da071 100644 --- a/pkg/connector/event_log.go +++ b/pkg/connector/event_log.go @@ -84,10 +84,15 @@ func (connector *Okta) ListEvents( if filter.Matches(log) { event, err := filter.Handle(l, log) // MJP we don't want to stop, we should just log the error and continue - if err != nil { + switch { + case err != nil: l.Error("error handling event", zap.Error(err), zap.String("event_type", log.EventType)) - } else { + case event != nil: rv = append(rv, event) + default: + // The handler matched but had nothing to emit, and logged its reason + // at the skip site. + l.Debug("skipped event", zap.String("event_type", log.EventType)) } } } diff --git a/pkg/connector/profile_keys.go b/pkg/connector/profile_keys.go index 842adff4..bbfb5690 100644 --- a/pkg/connector/profile_keys.go +++ b/pkg/connector/profile_keys.go @@ -34,6 +34,14 @@ const ( // name must not silently change the event-feed lookups. const oktaLogTargetTypeUser = "User" +// Okta reports assignment and unassignment of standard admin roles under the same +// user.account.privilege.grant event type and distinguishes them with an extra +// target carrying one of these types. Wire values, like oktaLogTargetTypeUser. +const ( + oktaLogTargetRoleAssigned = "ROLE_ASSIGNED" + oktaLogTargetRoleUnassigned = "ROLE_UNASSIGNED" +) + const ( profileFieldCreateInactive = "create_inactive" profileFieldAdditionalAttributes = "additionalAttributes"