Skip to content

fix(amplify-category-auth): clean up OIDC provider on auth stack teardown#14952

Open
ahmedhamouda78 wants to merge 3 commits into
aws-amplify:devfrom
ahmedhamouda78:fix/openid-provider-teardown-leak
Open

fix(amplify-category-auth): clean up OIDC provider on auth stack teardown#14952
ahmedhamouda78 wants to merge 3 commits into
aws-amplify:devfrom
ahmedhamouda78:fix/openid-provider-teardown-leak

Conversation

@ahmedhamouda78

Copy link
Copy Markdown
Member

Description of changes

The auth category's OpenId custom resource Lambda (openIdLambda.js) only handled Create/Update events. On stack deletion CloudFormation sends a Delete event, which fell through to a no-op — so the account-global IAM OIDC provider (accounts.google.com) was never removed. Orphaned providers accumulate in the account on every teardown of an auth resource configured with Google/OpenID federation.

This PR:

  • Adds a Delete handler to openIdLambda.js. Because the provider is account-global and keyed by URL (the create path reuses it and appends client IDs), the handler removes only the client IDs this resource registered and deletes the provider only once no client IDs remain — preserving providers shared by other Amplify apps in the same account. All IAM calls tolerate an already-removed provider/client ID, so the handler is idempotent.
  • Grants the OpenId Lambda role iam:RemoveClientIDFromOpenIDConnectProvider and iam:DeleteOpenIDConnectProvider (scoped to the accounts.google.com provider ARN) in auth-cognito-stack-builder.ts.
  • Adds unit tests for create, sole-owner delete, shared-provider delete, and idempotent delete.

Issue #, if available

N/A

Description of how you validated changes

  • New Jest unit test openIdLambda.test.js passes (4/4), driving the real handler through: create, sole-owner delete (provider removed), shared-provider delete (provider retained, only this resource's client IDs removed), and delete-when-absent (idempotent).
  • Confirmed the IAM policy change does not alter existing stack-builder snapshots.

Checklist

  • PR description included
  • Tests are changed or added
  • New AWS SDK calls or CloudFormation actions have been added to relevant test and service IAM policies

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

…down

The OpenId custom resource Lambda only handled Create/Update requests, so on
stack deletion the account-global IAM OIDC provider (accounts.google.com) was
never removed. Orphaned providers accumulate in the account on every teardown.

Add a Delete handler that removes the client IDs this resource registered and
deletes the provider only once no client IDs remain, so providers shared by
other Amplify apps in the same account are preserved. All IAM calls tolerate
an already-removed provider/client ID, keeping the handler idempotent.

Grant the OpenId Lambda role iam:RemoveClientIDFromOpenIDConnectProvider and
iam:DeleteOpenIDConnectProvider (scoped to the accounts.google.com provider).

Adds unit tests covering create, sole-owner delete, shared-provider delete,
and idempotent delete.
@ahmedhamouda78 ahmedhamouda78 requested a review from a team as a code owner July 1, 2026 17:45
…a deps

cfn-response and @aws-sdk/client-iam are provided by the Lambda runtime and
are not installed in the package, so jest.mock could not resolve them and the
suite failed to run in CI. Mark both mocks as virtual.
Comment thread packages/amplify-category-auth/resources/auth-custom-resource/openIdLambda.js Outdated
- Re-read the provider after removing our client IDs and delete it if a
  concurrently-deleting stack emptied it, closing a race that could leave a
  provider with zero client IDs orphaned in the account.
- Harden provider lookup to match on the URL host / :oidc-provider/<host>
  ARN suffix instead of brittle string splitting.
- Clarify in the stack builder that account-level iam:ListOpenIDConnectProviders
  is granted in the adjacent statement (required by the lookup).
- Tests: guard invoke() when response.send is never called, rename the URL
  constant to avoid shadowing the global, and add Update-appends and
  concurrent-deletion-race cases.
@ahmedhamouda78

Copy link
Copy Markdown
Member Author

Thanks for the thorough review @soberm — all six points addressed in b16780b:

  • [major] Concurrent-deletion race: After removing our client IDs I now re-read the provider and delete it if the list is empty, so two stacks racing on a shared provider can't leave it orphaned with zero client IDs. The existing NoSuchEntityException catch handles the case where the other deleter wins the final delete. Added a deletes the provider when a concurrent stack empties it during our Delete test that simulates the interleaving.
  • [minor] Fragile URL matching: findProviderArn now derives the host via new URL(url).host and matches the provider ARN suffix. One correction to the suggested snippet: real OIDC provider ARNs use :oidc-provider/<host> (colon), e.g. arn:aws:iam::<account>:oidc-provider/accounts.google.com, so I matched on :oidc-provider/${providerHost} rather than /oidc-provider/... — the latter never matches.
  • [minor] ListOpenIDConnectProviders: It's granted in the adjacent statement (account-level, Resource: *) since it can't be scoped to a single provider ARN. Added a comment there noting it's required by findProviderArn.
  • [minor] invoke() guard: Now throws handler never called response.send instead of returning undefined.
  • [nit] URL shadowing: Renamed the test constant to GOOGLE_OIDC_URL.
  • [nit] Update coverage: Added a test that creates, then updates with an additional client ID and asserts it's appended.

All six unit tests pass locally under CI-like conditions (runtime-only deps mocked virtually).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants