Make syncing secrets an opt in resource type. - #203
Conversation
…ets config option. This change is backwards compatible. Existing connectors with sync-secrets enabled will still sync API tokens. This fixes a bug in incremental sync when sync secrets is disabled, but changes to api tokens still show up in the event feed.
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| func (o *Okta) shouldFetchApiTokens() bool { | ||
| if o.SyncSecrets { | ||
| return true | ||
| } | ||
|
|
||
| if shouldSyncResourceType(c.opts, resourceTypeDevice.Id) { | ||
| resourceTypes = append(resourceTypes, resourceTypeDevice) | ||
| if o.opts == nil { | ||
| return true | ||
| } | ||
|
|
||
| return &v2.ResourceTypesServiceListResourceTypesResponse{ | ||
| List: resourceTypes, | ||
| }, nil | ||
| return o.opts.SyncFilterIsExplicit() && o.opts.WillSyncResourceType(resourceTypeApiToken.Id) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: This predicate and the scope decision in New() (line 442) encode the same policy but disagree on opts == nil: here nil opts returns true (fetch tokens), while line 442 requires opts != nil before adding okta.apiTokens.read. On the private-key auth path, a nil-opts sync with SyncSecrets=false would therefore call ListApiTokens without ever having requested the scope, and the 403 fails the whole sync. Consider extracting one shared helper (e.g. shouldFetchApiTokens(syncSecrets bool, opts *cli.ConnectorOpts) bool) and calling it from both places so the two can't drift.
| if !o.connector.shouldFetchApiTokens() { | ||
| return nil, nil, nil | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Returning (nil, nil, nil) from Get gives the caller a nil resource with a nil error, which is ambiguous — the api-token type advertises CAPABILITY_TARGETED_SYNC, so this path is reachable from a targeted sync/expand and the SDK has no way to distinguish "not opted in" from "found nothing." Prefer an explicit status.Error(codes.NotFound, "okta-connector-v5: api token sync is not enabled") (or PermissionDenied) so the failure is legible rather than a silent nil.
| apiTokens, resp, err = o.connector.clientV5.ApiTokenAPI.ListApiTokens(ctx).Execute() | ||
| if err != nil { | ||
| return nil, nil, fmt.Errorf("okta-connector-v5: failed to list api tokens: %w", err) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: Now that api-token is advertised to every install as an opt-in type, any customer can enable it from the C1 UI without having granted okta.apiTokens.read. Per the note in docs/docs-info.md, an ungranted scope silently drops from the issued token and only surfaces as a 403 here — which this hard error turns into a full sync failure rather than a missing resource type. Consider degrading gracefully on 403/404 (log at Warn, return an empty page) as R7 suggests.
6370230 to
12e3b69
Compare
| func (o *Okta) shouldFetchApiTokens() bool { | ||
| if o.SyncSecrets { | ||
| return true | ||
| } | ||
|
|
||
| if shouldSyncResourceType(c.opts, resourceTypeDevice.Id) { | ||
| resourceTypes = append(resourceTypes, resourceTypeDevice) | ||
| if o.opts == nil { | ||
| return true | ||
| } | ||
|
|
||
| return &v2.ResourceTypesServiceListResourceTypesResponse{ | ||
| List: resourceTypes, | ||
| }, nil | ||
| return o.opts.SyncFilterIsExplicit() && o.opts.WillSyncResourceType(resourceTypeApiToken.Id) | ||
| } |
There was a problem hiding this comment.
🟡 Suggestion: this gate reads ConnectorOpts.SyncResourceTypeIDs, which the SDK populates only from the sync-resource-types config value (pkg/config/config.go:28, pkg/cli/lambda_server__added.go:503). The C1 UI's resource-type selection is also delivered per sync task via SyncFull.SyncResourceTypeIds, and tasks/c1api/full_sync.go:243-248 calls that "the authoritative source when set", falling back to local config. If C1 ships an api-token opt-in only on the task, shouldFetchApiTokens() stays false and the opted-in customer gets an advertised api-token type with zero resources (and, on private-key auth, no okta.apiTokens.read scope) — silently, with no error. Worth confirming C1 also writes sync-resource-types into the connector config for opt-in types before relying on this alone. (Confidence: medium.)
| scopes = append(scopes, provisioningScopes...) | ||
|
|
||
| if cc.SyncSecrets { | ||
| if cc.SyncSecrets || (opts != nil && opts.SyncFilterIsExplicit() && opts.WillSyncResourceType(resourceTypeApiToken.Id)) { |
There was a problem hiding this comment.
🟡 Suggestion: this predicate duplicates shouldFetchApiTokens() minus its opts == nil branch, so the two can drift — with nil opts the connector would fetch tokens but this line would not request okta.apiTokens.read (a silent 403 on private-key auth). Not reachable today since New() is always called with non-nil opts, but extracting one shared helper (e.g. apiTokenSyncRequested(syncSecrets bool, opts *cli.ConnectorOpts) bool) used by both New() and shouldFetchApiTokens() would keep the fetch decision and the scope decision from diverging. The new unit test covers only the method, not this line.
| - `okta.roles.read` and `okta.apps.read` (required for sync) | ||
| - `okta.users.manage`, `okta.groups.manage`, `okta.roles.manage`, `okta.apps.manage` (required for provisioning) | ||
| - `okta.apiTokens.read` (required when **Sync secrets** is enabled) | ||
| - `okta.apiTokens.read` (required when the **API Token** resource type is enabled) |
There was a problem hiding this comment.
🟡 Suggestion: the Capabilities section (lines 15-29) still presents Secrets - API tokens as an unconditional sync row, and only Devices carries the **Devices is opt-in.** callout. Since API tokens are now opt-in too, add a matching note next to the table (and consider line 95, "This connector can sync secrets…") so the capabilities section isn't stale relative to this change.
Connector PR Review: Make syncing secrets an opt in resource type.Blocking Issues: 0 | Suggestions: 3 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness: the api-token syncer is now registered unconditionally and gated at Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
1 participant
And deprecate the sync secrets config option.
This change is backwards compatible. Existing connectors with sync-secrets enabled will still sync API tokens.
This fixes a bug in incremental sync when sync secrets is disabled, but changes to api tokens still show up in the event feed.