From 5c0815365070e4918ab3a2f4938d19c804777258 Mon Sep 17 00:00:00 2001 From: Logan Saso Date: Thu, 11 Jun 2026 23:06:32 +0000 Subject: [PATCH] Fix 16 bugs across provisioning, sync, and panic paths Provisioning: - CreateAccount: use max(8, len) not min(8, len) for random password length; random passwords with no requested length always failed - Role Grant/Revoke: guards rejected group principals, making the implemented group branches unreachable - Role Grant (group): return GrantAlreadyExists on already-assigned instead of an error - App Revoke (group): treat 404 as GrantAlreadyRevoked like the user branch does Sync correctness: - Event filters: fix nonexistent event type "app.lifecycle.create" (-> application.lifecycle.create); app-creation events never synced - Resource set / binding grants: V1 grant IDs used the resource-set ID in place of the principal ID, colliding all grants per resource - Binding Grants: reset principal per loop iteration; unknown member types re-emitted the previous member's grant - listBindings: stop discarding query params so the pagination cursor is actually sent - Custom role Grants: apply the same email-domain user filter as role Grants - Group Grants: emit the completion ETag where the pagination bag actually drains (role branch); the user-branch check was dead code Panics / metadata: - api_token / resource_sets: use nil-safe v5 accessors instead of dereferencing optional pointer fields - Binding Get/Grants: validate composite resource ID before indexing - Group resource: don't pass typed-nil ETagMetadata to Annotations - App Grants: default case wrapped a guaranteed-nil err; report the unexpected resource ID instead - Connector metadata: ExternalLink now a full https URL, not bare host - listUsers: nil-check qp before dereferencing it Co-authored-by: c1-squire-dev[bot] Co-Authored-By: Claude Fable 5 --- pkg/connector/api_token.go | 43 +++++++++++++++++-------- pkg/connector/app.go | 12 +++++-- pkg/connector/connector.go | 2 +- pkg/connector/custom_role.go | 23 ++++++++++--- pkg/connector/event_filter.go | 4 +++ pkg/connector/event_filters.go | 2 +- pkg/connector/group.go | 20 ++++++------ pkg/connector/resource_sets.go | 20 +++++++----- pkg/connector/resource_sets_bindings.go | 18 +++++++---- pkg/connector/role.go | 8 +++-- pkg/connector/user.go | 10 +++--- 11 files changed, 108 insertions(+), 54 deletions(-) diff --git a/pkg/connector/api_token.go b/pkg/connector/api_token.go index 1ea6d35b..c203331b 100644 --- a/pkg/connector/api_token.go +++ b/pkg/connector/api_token.go @@ -128,27 +128,42 @@ func (o *apiTokenResourceType) Get(ctx context.Context, resourceId *v2.ResourceI } func apiTokenResource(apiToken *oktav5.ApiToken) (*v2.Resource, error) { + tokenId, ok := apiToken.GetIdOk() + if !ok { + return nil, fmt.Errorf("okta-connectorv2: api token %q has no id", apiToken.GetName()) + } + options := []resource.SecretTraitOption{ - resource.WithSecretExpiresAt(*apiToken.ExpiresAt), - resource.WithSecretIdentityID(&v2.ResourceId{ - ResourceType: resourceTypeUser.Id, - Resource: *apiToken.UserId, - BatonResource: false, - }), - resource.WithSecretCreatedByID(&v2.ResourceId{ - ResourceType: resourceTypeUser.Id, - Resource: *apiToken.UserId, - BatonResource: false, - }), - resource.WithSecretLastUsedAt(*apiToken.LastUpdated), - resource.WithSecretCreatedAt(*apiToken.Created), resource.WithSecretType(v2.SecretTrait_CREDENTIAL_TYPE_STATIC_SECRET), resource.WithSecretDetail("okta.api_token"), } + if expiresAt, ok := apiToken.GetExpiresAtOk(); ok { + options = append(options, resource.WithSecretExpiresAt(*expiresAt)) + } + if userId, ok := apiToken.GetUserIdOk(); ok { + options = append(options, + resource.WithSecretIdentityID(&v2.ResourceId{ + ResourceType: resourceTypeUser.Id, + Resource: *userId, + BatonResource: false, + }), + resource.WithSecretCreatedByID(&v2.ResourceId{ + ResourceType: resourceTypeUser.Id, + Resource: *userId, + BatonResource: false, + }), + ) + } + if lastUpdated, ok := apiToken.GetLastUpdatedOk(); ok { + options = append(options, resource.WithSecretLastUsedAt(*lastUpdated)) + } + if created, ok := apiToken.GetCreatedOk(); ok { + options = append(options, resource.WithSecretCreatedAt(*created)) + } rv, err := resource.NewSecretResource( apiToken.Name, resourceTypeApiToken, - *apiToken.Id, + *tokenId, options, ) if err != nil { diff --git a/pkg/connector/app.go b/pkg/connector/app.go index 84042321..b035c9a7 100644 --- a/pkg/connector/app.go +++ b/pkg/connector/app.go @@ -151,7 +151,7 @@ func (o *appResourceType) Grants( return nil, nil, fmt.Errorf("okta-connectorv2: failed to list app users grants: %w", err) } default: - return nil, nil, fmt.Errorf("okta-connectorv2: unexpected resource for app: %w", err) + return nil, nil, fmt.Errorf("okta-connectorv2: unexpected resource for app: %s", bag.ResourceID()) } pageToken, err := bag.Marshal() @@ -533,8 +533,16 @@ func (g *appResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotati } case resourceTypeGroup.Id: groupID := principal.Id.Resource - _, _, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil) + _, resp, err := g.client.Application.GetApplicationGroupAssignment(ctx, appID, groupID, nil) if err != nil { + if resp != nil && resp.StatusCode == http.StatusNotFound { + l.Debug( + "okta-connector: revoke: group does not have app membership", + zap.String("principal_id", principal.Id.String()), + zap.String("principal_type", principal.Id.ResourceType), + ) + return annotations.New(&v2.GrantAlreadyRevoked{}), nil + } l.Warn( "okta-connector: group does not have app membership", zap.String("principal_id", principal.Id.String()), diff --git a/pkg/connector/connector.go b/pkg/connector/connector.go index 82001507..343657fb 100644 --- a/pkg/connector/connector.go +++ b/pkg/connector/connector.go @@ -187,7 +187,7 @@ func (c *Okta) Metadata(ctx context.Context) (*v2.ConnectorMetadata, error) { var annos annotations.Annotations annos.Update(&v2.ExternalLink{ - Url: c.domain, + Url: fmt.Sprintf("https://%s", c.domain), }) return &v2.ConnectorMetadata{ diff --git a/pkg/connector/custom_role.go b/pkg/connector/custom_role.go index 975dc837..b90cc983 100644 --- a/pkg/connector/custom_role.go +++ b/pkg/connector/custom_role.go @@ -159,10 +159,25 @@ func (o *customRoleResourceType) Grants( return nil, nil, fmt.Errorf("okta-connectorv2: failed to fetch bag.Next: %w", err) } - // Collect all user IDs upfront, so that we can get all their roles from cache at once. + // Filter users by email domain first to avoid fetching roles for users we'll skip anyway. userIDs := make([]string, 0, len(usersWithRoleAssignments)) for _, user := range usersWithRoleAssignments { - userIDs = append(userIDs, user.Id) + userId := user.Id + + // Check if the user should be included after filtering by email domains. + shouldInclude, ok := o.connector.shouldIncludeUserFromCache(ctx, attrs.Session, userId) + if !ok { + user, _, err := o.connector.client.User.GetUser(ctx, userId) + if err != nil { + return nil, nil, err + } + shouldInclude = o.connector.shouldIncludeUserAndSetCache(ctx, attrs.Session, user) + } + if !shouldInclude { + continue + } + + userIDs = append(userIDs, userId) } // Get all cached roles at once (this will only return roles that were found). @@ -176,9 +191,7 @@ func (o *customRoleResourceType) Grants( // Now, get any remaining user roles (those that aren't in cache) // and keep track of them so that we can try to cache them later. toCache := make(map[string]mapset.Set[string]) - for _, user := range usersWithRoleAssignments { - userId := user.Id - + for _, userId := range userIDs { // If this user's roles are already cached, keep moving. if _, found := userRoles[userId]; found { continue diff --git a/pkg/connector/event_filter.go b/pkg/connector/event_filter.go index eac97a2d..5903258a 100644 --- a/pkg/connector/event_filter.go +++ b/pkg/connector/event_filter.go @@ -98,6 +98,10 @@ func (filter *EventFilter) Handle(l *zap.Logger, event *oktaSDK.LogEvent) (*v2.E targetMap[target.Type] = append(targetMap[target.Type], target) } + if event.Published == nil { + return nil, fmt.Errorf("okta-connectorv2: event %s has no published timestamp", event.Uuid) + } + rv := &v2.Event{ Id: event.Uuid, OccurredAt: timestamppb.New(*event.Published), diff --git a/pkg/connector/event_filters.go b/pkg/connector/event_filters.go index 0c2d1198..944d9da3 100644 --- a/pkg/connector/event_filters.go +++ b/pkg/connector/event_filters.go @@ -132,7 +132,7 @@ var ( }, } ApplicationLifecycleFilter = EventFilter{ - EventTypes: mapset.NewSet[string]("app.lifecycle.create", "application.lifecycle.update"), + EventTypes: mapset.NewSet[string]("application.lifecycle.create", "application.lifecycle.update"), TargetTypes: mapset.NewSet[string]("AppInstance"), EventHandler: func(l *zap.Logger, event *oktaSDK.LogEvent, targetMap map[string][]*oktaSDK.LogTarget, rv *v2.Event) error { if len(targetMap["AppInstance"]) != 1 { diff --git a/pkg/connector/group.go b/pkg/connector/group.go index 7e9c7da1..53db9c04 100644 --- a/pkg/connector/group.go +++ b/pkg/connector/group.go @@ -198,13 +198,6 @@ func (o *groupResourceType) Grants( return nil, nil, err } - if pageToken == "" { - etag := &v2.ETag{ - Value: time.Now().UTC().Format(time.RFC3339Nano), - } - annos.Update(etag) - } - return rv, &sdkResource.SyncOpResults{NextPageToken: pageToken, Annotations: annos}, nil case resourceTypeRole.Id: roles, resp, err := listGroupAssignedRoles(ctx, o.connector.client, groupID, nil) @@ -293,6 +286,13 @@ func (o *groupResourceType) Grants( return nil, nil, err } + if pageToken == "" { + etag := &v2.ETag{ + Value: time.Now().UTC().Format(time.RFC3339Nano), + } + annos.Update(etag) + } + return rv, &sdkResource.SyncOpResults{NextPageToken: pageToken, Annotations: annos}, nil default: return nil, nil, fmt.Errorf("okta-connector: invalid grant resource type: %s", bag.ResourceTypeID()) @@ -376,7 +376,9 @@ func (o *groupResourceType) groupResource(ctx context.Context, group *okta.Group if err != nil { return nil, err } - annos.Update(etagMd) + if etagMd != nil { + annos.Update(etagMd) + } if group.Type == builtInGroupType { annos.Update(&v2.EntitlementImmutable{}) @@ -607,7 +609,7 @@ func (o *groupResourceType) registerModifyGroupAction(ctx context.Context, regis DisplayName: "Description", Description: "The new description for the group.", Field: &config.Field_StringField{}, - IsRequired: false, + IsRequired: false, }, }, ReturnTypes: []*config.Field{ diff --git a/pkg/connector/resource_sets.go b/pkg/connector/resource_sets.go index 54b4b08e..29020b14 100644 --- a/pkg/connector/resource_sets.go +++ b/pkg/connector/resource_sets.go @@ -60,15 +60,15 @@ func resourceSetsResource(ctx context.Context, rs *ResourceSets, parentResourceI func resourceSetResource(ctx context.Context, rs *oktav5.ResourceSet, parentResourceID *v2.ResourceId) (*v2.Resource, error) { profile := map[string]interface{}{ - "id": *rs.Id, - profileFieldLabel: *rs.Label, - profileFieldDescription: *rs.Description, + "id": rs.GetId(), + profileFieldLabel: rs.GetLabel(), + profileFieldDescription: rs.GetDescription(), } return sdkResource.NewResource( - *rs.Label, + rs.GetLabel(), resourceTypeResourceSets, - *rs.Id, + rs.GetId(), sdkResource.WithParentResourceID(parentResourceID), sdkResource.WithAppTrait( sdkResource.WithAppProfile(profile), @@ -157,7 +157,7 @@ func (rs *resourceSetsResourceType) Entitlements(_ context.Context, resource *v2 sdkEntitlement.WithAnnotation(&v2.V1Identifier{ Id: V1MembershipEntitlementID(resource.Id.GetResource()), }), - sdkEntitlement.WithGrantableTo(resourceTypeResourceSets), + sdkEntitlement.WithGrantableTo(resourceTypeCustomRole), sdkEntitlement.WithDisplayName(fmt.Sprintf("%s Resource Set Binding", resource.DisplayName)), sdkEntitlement.WithDescription(fmt.Sprintf("Member of %s resource-set in Okta", resource.DisplayName)), ), @@ -170,13 +170,17 @@ func listBindings( ctx context.Context, client *okta.Client, resourceSetId string, - _ *query.Params, + qp *query.Params, ) ([]Role, *okta.Response, error) { apiPath, err := url.JoinPath(apiPathListIamResourceSets, resourceSetId, "bindings") if err != nil { return nil, nil, err } + if qp != nil { + apiPath += qp.String() + } + var resourceSetsBindings *ResourceSetsBindingsAPIData resp, err := doRequest(ctx, apiPath, http.MethodGet, &resourceSetsBindings, client) if err != nil { @@ -243,7 +247,7 @@ func (rs *resourceSetsResourceType) Grants(ctx context.Context, resource *v2.Res principal := &v2.Resource{Id: &v2.ResourceId{ResourceType: resourceTypeCustomRole.Id, Resource: role.ID}} gr := sdkGrant.NewGrant(resource, bindingEntitlement, principal, sdkGrant.WithAnnotation(&v2.V1Identifier{ - Id: fmtGrantIdV1(V1MembershipEntitlementID(resource.Id.Resource), resource.Id.Resource), + Id: fmtGrantIdV1(V1MembershipEntitlementID(resource.Id.Resource), role.ID), }), ) rv = append(rv, gr) diff --git a/pkg/connector/resource_sets_bindings.go b/pkg/connector/resource_sets_bindings.go index 74ffb9ab..2c1d7fb5 100644 --- a/pkg/connector/resource_sets_bindings.go +++ b/pkg/connector/resource_sets_bindings.go @@ -132,7 +132,7 @@ func (rsb *resourceSetsBindingsResourceType) Entitlements(_ context.Context, res sdkEntitlement.WithAnnotation(&v2.V1Identifier{ Id: V1MembershipEntitlementID(resource.Id.GetResource()), }), - sdkEntitlement.WithGrantableTo(resourceTypeResourceSets), + sdkEntitlement.WithGrantableTo(resourceTypeUser, resourceTypeGroup), sdkEntitlement.WithDisplayName(fmt.Sprintf("%s Resource Set Binding Member", resource.DisplayName)), sdkEntitlement.WithDescription(fmt.Sprintf("Member of %s resource-set-binding member in Okta", resource.DisplayName)), ), @@ -226,16 +226,17 @@ func (rsb *resourceSetsBindingsResourceType) unassignMemberFromBinding(ctx conte func (rsb *resourceSetsBindingsResourceType) Grants(ctx context.Context, resource *v2.Resource, attrs sdkResource.SyncOpAttrs) ([]*v2.Grant, *sdkResource.SyncOpResults, error) { pToken := &attrs.PageToken - var ( - rv []*v2.Grant - principal *v2.Resource - ) + var rv []*v2.Grant bag, _, err := parsePageToken(pToken.Token, resource.Id) if err != nil { return nil, nil, fmt.Errorf("okta-connectorv2: failed to parse page token: %w", err) } resourceIDs := strings.Split(resource.Id.Resource, ":") + if len(resourceIDs) != resourceMaxLength { + return nil, nil, fmt.Errorf("okta-connectorv2: invalid resource set binding id: %s", resource.Id.Resource) + } + resourceSetId := resourceIDs[firstItem] customRoleId := resourceIDs[lastItem] members, _, err := rsb.listMembersOfBinding(ctx, rsb.client, resourceSetId, customRoleId, nil) @@ -244,6 +245,7 @@ func (rsb *resourceSetsBindingsResourceType) Grants(ctx context.Context, resourc } for _, member := range members { + var principal *v2.Resource memberHref := strings.Split(member.Links.Self.Href, "/") resourceType := memberHref[len(memberHref)-resourceMaxLength] resourceId := memberHref[len(memberHref)-lastItem] @@ -260,7 +262,7 @@ func (rsb *resourceSetsBindingsResourceType) Grants(ctx context.Context, resourc gr := sdkGrant.NewGrant(resource, entitlementName, principal, sdkGrant.WithAnnotation(&v2.V1Identifier{ - Id: fmtGrantIdV1(V1MembershipEntitlementID(resource.Id.Resource), resource.Id.Resource), + Id: fmtGrantIdV1(V1MembershipEntitlementID(resource.Id.Resource), resourceId), }), ) rv = append(rv, gr) @@ -389,6 +391,10 @@ func (rsb *resourceSetsBindingsResourceType) Get(ctx context.Context, resourceId l.Debug("getting resource set binding", zap.String("resource_set_binding_id", resourceId.Resource)) resourceIDs := strings.Split(resourceId.Resource, ":") + if len(resourceIDs) != resourceMaxLength { + return nil, nil, fmt.Errorf("okta-connectorv2: invalid resource set binding id: %s", resourceId.Resource) + } + resourceSetId := resourceIDs[firstItem] customRoleId := resourceIDs[lastItem] diff --git a/pkg/connector/role.go b/pkg/connector/role.go index 89c6036b..60657c25 100644 --- a/pkg/connector/role.go +++ b/pkg/connector/role.go @@ -428,13 +428,13 @@ func roleGroupGrant(groupID string, resource *v2.Resource, shouldExpand bool) *v func (g *roleResourceType) Grant(ctx context.Context, principal *v2.Resource, entitlement *v2.Entitlement) (annotations.Annotations, error) { l := ctxzap.Extract(ctx) - if principal.Id.ResourceType != resourceTypeUser.Id { + if principal.Id.ResourceType != resourceTypeUser.Id && principal.Id.ResourceType != resourceTypeGroup.Id { l.Warn( "okta-connector: only users or groups can be granted role membership", zap.String("principal_type", principal.Id.ResourceType), zap.String("principal_id", principal.Id.Resource), ) - return nil, fmt.Errorf("okta-connector: only users or groups can be granted repo membership") + return nil, fmt.Errorf("okta-connector: only users or groups can be granted role membership") } roleId := entitlement.Resource.Id.Resource @@ -506,6 +506,8 @@ func (g *roleResourceType) Grant(ctx context.Context, principal *v2.Resource, en zap.String("ErrorCode", errOkta.ErrorCode), zap.String("ErrorSummary", errOkta.ErrorSummary), ) + + return annotations.New(&v2.GrantAlreadyExists{}), nil } return nil, fmt.Errorf("okta-connector: %v", errOkta) @@ -531,7 +533,7 @@ func (g *roleResourceType) Revoke(ctx context.Context, grant *v2.Grant) (annotat entitlement := grant.Entitlement principal := grant.Principal roleId := "" - if principal.Id.ResourceType != resourceTypeUser.Id { + if principal.Id.ResourceType != resourceTypeUser.Id && principal.Id.ResourceType != resourceTypeGroup.Id { l.Warn( "okta-connector: only users or groups can have role membership revoked", zap.String("principal_type", principal.Id.ResourceType), diff --git a/pkg/connector/user.go b/pkg/connector/user.go index 9e01c2cf..7c8205d7 100644 --- a/pkg/connector/user.go +++ b/pkg/connector/user.go @@ -213,14 +213,14 @@ func userName(user *okta.User) (string, string) { } func listUsers(ctx context.Context, client *okta.Client, token *pagination.Token, qp *query.Params) ([]*okta.User, *responseContext, error) { + if qp == nil { + qp = &query.Params{} + } if qp.Search == "" { qp.Search = "status pr" // ListUsers doesn't get deactivated users by default. this should fetch them all } - uri := usersUrl - if qp != nil { - uri += qp.String() - } + uri := usersUrl + qp.String() reqUrl, err := url.Parse(uri) if err != nil { @@ -420,7 +420,7 @@ func getCredentialOption(credentialOptions *v2.LocalCredentialOptions) (*okta.Us return nil, errors.New("unsupported credential options") } - length := min(8, credentialOptions.GetRandomPassword().GetLength()) + length := max(8, credentialOptions.GetRandomPassword().GetLength()) plaintextPassword, err := crypto.GenerateRandomPassword(&v2.LocalCredentialOptions_RandomPassword{ Length: length, })