feat: add OAuth2 provider support to identity - #1856
Conversation
github-actions
Bot
added
the
agentcore-harness-reviewing
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## refactor #1856 +/- ##
============================================
- Coverage 96.01% 95.56% -0.46%
============================================
Files 211 217 +6
Lines 9837 10255 +418
============================================
+ Hits 9445 9800 +355
- Misses 392 455 +63 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
github-actions
Bot
removed
the
agentcore-harness-reviewing
| flag( | ||
| "provider-configuration", | ||
| "complete OAuth2 provider configuration JSON (alternative to guided flags)", | ||
| z.string().optional(), |
There was a problem hiding this comment.
I think --provider-configuration also needs to be marked sensitive in both create and update. This flag accepts the complete SDK config, which can include clientSecret. I tested that form and the plaintext value was written to ~/.agentcore/logs while --client-secret was redacted. Marking both flag definitions sensitive should cover it.
There was a problem hiding this comment.
Nice catch. I've updated to mark as sensitive in both spots
| try { | ||
| parsed = JSON.parse(raw); | ||
| } catch (error) { | ||
| throw new TypeError( |
There was a problem hiding this comment.
This shared parser previously threw InputValidationError, but the move changes it to TypeError. The new OAuth handlers do the same. I reproduced a conflicting-secret invocation and the log classified it as source: "internal". These user-input failures should remain InputValidationError.
| vendorConfig.clientSecretConfig = clientSecretConfig; | ||
| vendorConfig.clientSecretSource = clientSecretSource; | ||
| oauth2ProviderConfigInput = config; | ||
| } else { |
There was a problem hiding this comment.
The two configuration modes are not fully enforced here. Without --provider-configuration, this always builds customOauth2ProviderConfig, even for --vendor GithubOauth2, and guided Custom OAuth also allows the required discovery configuration to be omitted. I reproduced the resulting service validation error. We should require provider configuration for non-Custom vendors and one discovery form for the guided Custom path, including the equivalent update validation.
There was a problem hiding this comment.
Good call. I updated our validations so we we require --provider-configuration for non-custom vendors
| ), | ||
| flag( | ||
| "client-secret-reference", | ||
| 'external secret reference JSON: {"secretId":"<arn>","jsonKey":"<key>"}', |
There was a problem hiding this comment.
does this need to be sensitive or is it strictly pointers to secrets?
There was a problem hiding this comment.
This is just a pointer to keys, so we're okay to leave it non-sensitive
| ], | ||
| handle: async (ctx, flags) => { | ||
| if (!flags.name) { | ||
| throw new TypeError("required option '--name <name>' not specified"); |
There was a problem hiding this comment.
should we throw modeled input validation errors here? (same with below)
There was a problem hiding this comment.
Yep, I went through and updated all instances of TypeError to the correct modeled error.
| "--provider-configuration and guided flags (--client-id, --discovery-url, --authorization-server-metadata) are mutually exclusive", | ||
| ); | ||
| } | ||
| if (flags["discovery-url"] && flags["authorization-server-metadata"]) { |
There was a problem hiding this comment.
same question for errors here.
There was a problem hiding this comment.
Same as above handling of errors
| throw new TypeError(`--${flagName} must be a JSON object with "secretId" and "jsonKey"`); | ||
| } | ||
|
|
||
| const obj = parsed as Record<string, unknown>; |
There was a problem hiding this comment.
could we simplify this with a strict zod schema? Something like:
const secretReferenceSchema = z
.object({
secretId: z.string().min(1),
jsonKey: z.string().min(1),
})
.strict();
b5c46d3 to
8773336
Compare
aidandaly24
left a comment
aidandaly24
left a comment
There was a problem hiding this comment.
Thanks for fixing up the comments. LGTM!
4 participants
Description
Adds command-line CRUDL for Identity OAuth2 credential providers:
identity oauth2-credential-provider createidentity oauth2-credential-provider getidentity oauth2-credential-provider listidentity oauth2-credential-provider updateidentity oauth2-credential-provider deleteSupports two configuration paths:
--client-id,--discovery-url, and--authorization-server-metadatabuild the provider config from scalar flags. Only valid with--vendor CustomOauth2--provider-configuration <json>accepts the complete SDK config union for any of the supported vendors. The CLI merges the resolved secret into the vendor config before sendingSecret handling follows the same pattern established with api-key-credential-provider:
--client-secretis source-aware (inline, file://path, or - for stdin) and marked sensitive for log redaction--client-secret-reference '{"secretId":"...","jsonKey":"..."}'for external Secrets Manager secretsThis PR also generalizes
parseSecretReferenceto a shared identity utility (src/handlers/identity/parser.tsx) parameterized by flag name.Related Issue
Closes #
Documentation PR
Type of Change
Testing
Manual tested cli by invoking all commands, both happy-path and negative validation. Expected results across the board. Additionally ran the full test suite, against latest HEAD after rebasing refactor branch:
How have you tested the change?
npm run test:unitandnpm run test:integnpm run typechecknpm run lintsrc/assets/, I rannpm run test:update-snapshotsand committed the updated snapshotsChecklist
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the
terms of your choice.