fix(auth-oidc): fail fast at synth when cognitoFederated() is configured (#447) - #516
Open
osama-rizk wants to merge 1 commit into
Open
fix(auth-oidc): fail fast at synth when cognitoFederated() is configured (#447)#516osama-rizk wants to merge 1 commit into
osama-rizk wants to merge 1 commit into
Conversation
cognitoFederated() emitted a CloudFormation template that always failed to
deploy: the CDK layer wrote the IdP client_id/client_secret into
AWS::Cognito::UserPoolIdentityProvider.ProviderDetails as {{resolve:ssm-secure}}
dynamic references, which CloudFormation does not permit on that property. synth
succeeded; deploy failed at change-set creation (stack left REVIEW_IN_PROGRESS).
Surface the limitation at synth via Annotations.addError with an actionable
message pointing at the self-hosted runtime providers (google/github/customOidc/
customOauth2), which resolve credentials at runtime and deploy cleanly. Docs +
regression test added.
Fixes #447
🦋 Changeset detectedLatest commit: 165ccb3 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
cognitoFederated()produced a CloudFormation template that always failed to deploy (#447). The CDK layer registered the federated IdP by writing the clientclient_id/client_secretintoAWS::Cognito::UserPoolIdentityProvider.ProviderDetailsas{{resolve:ssm-secure:...}}dynamic references, but CloudFormation only allowsssm-securereferences on a small allowlist of properties that excludesProviderDetails.cdk synthsucceeded, so the problem was invisible untilcdk deploy, which failed at change-set creation — before any resource was created — leaving the stack inREVIEW_IN_PROGRESS:This affected every
cognitoFederated()user — the credentials are always sourced fromsecretAppSettings, sossm-securewas the only code path.Fix (scoped: surface at synth)
Per the maintainer-sanctioned direction in the issue ("surface the limitation at synth time and document"), AuthOIDC now registers a synth error (via
cdk.Annotations.of(this).addError(...)) when acognitoFederated()provider is configured. This abortscdk deployearly with an actionable message naming the offending provider(s) and pointing at the self-hosted runtime providers —google(),github(),customOidc(),customOauth2()— which resolve IdP credentials at runtime viaAppSetting.get()(not through CloudFormation) and deploy cleanly.Strict DX improvement: the path was 100% undeployable before, so no working configuration is affected — a cryptic deploy-time change-set failure becomes a clear synth-time error with the workaround inline.
The provisioning scaffolding is left intact (it's built but never synthesized, since the error aborts synth) so the eventual proper fix has a starting point — see below.
Follow-up (the real fix, out of scope here)
Register the IdP through a deploy-time custom resource (
AwsCustomResourcecallingCreateIdentityProvider/UpdateIdentityProvider) that reads the SecureString at deploy time instead of via a CFN dynamic reference. Documented inDESIGN.md; that work isLand needs a sandbox, so it's deliberately not attempted here.Tests
packages/bb-auth-oidc/src/index.cdk.test.ts(new):cognitoFederated()→ asserts the synth error is present (verified as a true red: fails without the guard).google()provider → asserts no such error (control).Docs
cognitoFederated()JSDoc —@remarkslimitation + workaround.> [!WARNING]callout.DESIGN.md— "Known limitation" section + the custom-resource fix path.Checklist
npm run build, biome lint,check:apiclean@aws-blocks/bb-auth-oidcpatch +@aws-blocks/blockspatch)Fixes #447