Skip to content

feat(payments): add RotatePaymentConnectorCredentials support - #676

Open
walunjv wants to merge 2 commits into
aws:mainfrom
walunjv:feat/payments-rotate-connector-credentials
Open

walunjv wants to merge 2 commits into
aws:mainfrom
walunjv:feat/payments-rotate-connector-credentials

Conversation

@walunjv

@walunjv walunjv commented Sep 22, 2026

Copy link
Copy Markdown

What

Expose on-demand rotation of a payment connector's service-managed credentials, and surface the two connector fields that operation relies on.

  • PaymentClient.rotate_payment_connector_credentials() wraps the control plane operation. It builds the credentialsToRotate union and validates it against the model's constraints — non-empty secrets, @uniqueItems de-duplication, known secret names — so bad input fails locally instead of round-tripping.
  • New CoinbaseCdpSecret constant (API_KEY, WALLET_SECRET) names the rotatable secrets. An enum rather than a bool, so new secret kinds can be added without a breaking change.
  • The operation is allowlisted in _ALLOWED_PAYMENTS_CP_METHODS, so it is also reachable through direct boto3 forwarding.
  • get_payment_connector() and list_payment_connectors() now return provisionMode, and get_payment_connector() returns credentialsUpdatedAt when the service supplies it. Both were previously dropped by the hand-built result dicts, leaving callers no way to tell whether a connector's credentials are service-managed or how old they are.

Rotation applies only to connectors with a provisionMode of QUICK_CREATE, whose credentials the service issued and stores. MANUAL connectors are unaffected — the caller owns those credentials and rotates them with the payment provider directly. StripePrivy is rejected with a clear error, since it has no rotatable service-managed secrets today.

The rotation completes before the response returns, so the wrapper has no wait_for_ready parameter and nothing to poll. Only one rotation runs at a time per connector, so a concurrent call surfaces ConflictException.

Tests

  • 16 new cases in tests/bedrock_agentcore/payments/test_client.py across three classes: the union builder, the client method, and the new response fields. Covers enum and string inputs, de-duplication, each validation error, generated vs. caller-supplied client tokens, validation happening before any request is issued, ConflictException propagation, and credentialsUpdatedAt being omitted for MANUAL connectors.
  • Full payments suite: 917 passed, 9 skipped. Coverage 91% overall, constants.py at 100%.
  • ruff check and ruff format --check clean.

Expose on-demand rotation of a connector's service-managed credentials, and
surface the two connector fields the operation relies on.

- `PaymentClient.rotate_payment_connector_credentials()` wraps the control plane
  operation, building the `credentialsToRotate` union and validating it against
  the model's constraints (non-empty, unique, known secret names) before the
  request goes out.
- New `CoinbaseCdpSecret` constant (`API_KEY`, `WALLET_SECRET`) names the
  rotatable secrets; an enum rather than a bool so new kinds can be added
  without a breaking change.
- Allowlist the operation for boto3 forwarding, so it is also reachable directly.
- `get_payment_connector()` and `list_payment_connectors()` now return
  `provisionMode`, and `get_payment_connector()` returns `credentialsUpdatedAt`
  when the service supplies it. Both were previously dropped by the hand-built
  result dicts, so callers had no way to tell whether a connector's credentials
  are service-managed or how old they are.

Rotation applies only to QUICK_CREATE connectors. MANUAL connectors are
unaffected: the caller owns those credentials and rotates them with the payment
provider directly.

Tests
- 16 new cases in tests/bedrock_agentcore/payments/test_client.py covering the
  union builder, enum/string inputs, de-duplication, each validation error,
  client token handling, ConflictException propagation, and the new response
  fields.
- Full payments suite: 917 passed, 9 skipped; coverage 91%. Ruff lint and
  format clean.

if normalized_type != PaymentConnectorType.COINBASE_CDP.value:
raise ValueError(
f"Credential rotation is not supported for connector type: '{normalized_type}'. "

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to add this check here? Backend level check is good enough, no?

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed, removed in f3e7621. The service is the single source of truth now — all three checks were duplicating constraints the model already enforces (CoinbaseCdpSecret enum, @length(min: 1), @uniqueItems), so a secret name the service adds would have been rejected client-side until the SDK cut a release. Invalid input now surfaces as a ValidationException from the service.

One thing beyond your comment: I removed the connector_type parameter too, not just the check. CredentialRotationConfig models exactly one member (coinbaseCDP), so the parameter was selecting from a set of one — dropping only the if would have silently built a coinbaseCDP payload for anyone passing StripePrivy. The README example never passed it, so nothing user-facing changes.

Left _build_provider_config_input's vendor check alone — that one picks between two genuinely different payload shapes, so an unknown vendor has no correct output.

Addresses review feedback: the SDK was re-implementing constraints the
control plane model already enforces, so a secret name the service adds
would be rejected client-side until the SDK shipped a new release.

Drops from `_build_rotation_config_input`:
- the `CoinbaseCdpSecret` allowlist check (service enforces the enum)
- the empty-list check (`@length(min: 1)`)
- de-duplication (`@uniqueItems`)

The builder now only normalizes enum members to their string values and
wraps them in the union member. Invalid input surfaces as a
`ValidationException` from the service rather than a local `ValueError`.

Also removes the `connector_type` parameter rather than just its check.
`CredentialRotationConfig` models exactly one member (`coinbaseCDP`), so
the parameter selected from a set of one; keeping it without the check
would silently build a `coinbaseCDP` payload for a caller who passed
`StripePrivy`. The README example never passed it, so this is not a
user-facing change.

`_build_provider_config_input` keeps its vendor check: that one picks
between two genuinely different payload shapes, so an unknown vendor has
no correct output.

Tests
- `TestBuildRotationConfigInput`: 7 cases down to 4, with the two new
  ones pinning the pass-through contract (duplicates and empty lists go
  out on the wire unchanged).
- `test_rotate_validates_before_calling_service` becomes
  `test_rotate_defers_validation_to_service`, asserting the service's
  ClientError propagates and the unvalidated payload was forwarded.
- Payments + init + utils suites: 921 passed, 9 skipped. Ruff lint and
  format clean.
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
⚠️ Please upload report for BASE (main@889615f). Learn more about missing BASE report.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #676   +/-   ##
=======================================
  Coverage        ?   89.55%           
=======================================
  Files           ?      123           
  Lines           ?    10733           
  Branches        ?     1682           
=======================================
  Hits            ?     9612           
  Misses          ?      733           
  Partials        ?      388           
Flag Coverage Δ
unittests 89.55% <100.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

This branch is waiting to be deployed

1 waiting deployment
manual-approval f3e76215 Waiting Sep 23, 2026 by walunjv via Test (services) #1596
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/m PR size: M

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants