aws-samples/cloudfront-authorization-at-edge

Unexpected key 'ClientSecret' found in params

mikereinhold opened this issue · 2 comments

When deploying this solution recently, the CloudFormation stack rolls back

Rolls back with the UserPoolClientUpdate custom resource having status CREATE-FAILED:
Received response status [FAILED] from custom resource. Message returned: UnexpectedParameter: Unexpected key 'ClientSecret' found in params

Looking at the Lambda logs for the custom resource, found the following stack trace:

ERROR	UnexpectedParameter: Unexpected key 'ClientSecret' found in params
    at ParamValidator.fail (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:50:37)
    at ParamValidator.validateStructure (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:77:14)
    at ParamValidator.validateMember (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:88:21)
    at ParamValidator.validate (/var/runtime/node_modules/aws-sdk/lib/param_validator.js:34:10)
    at Request.VALIDATE_PARAMETERS (/var/runtime/node_modules/aws-sdk/lib/event_listeners.js:132:42)
    at Request.callListeners (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:106:20)
    at callNextListener (/var/runtime/node_modules/aws-sdk/lib/sequential_executor.js:96:12)
    at /var/runtime/node_modules/aws-sdk/lib/event_listeners.js:86:9
    at finish (/var/runtime/node_modules/aws-sdk/lib/config.js:386:7)
    at /var/runtime/node_modules/aws-sdk/lib/config.js:404:9 {
  code: 'UnexpectedParameter',
  time: 2021-12-10T17:52:56.497Z

The "Updating User Pool Client" log entry does indeed include a ClientSecret property in the object. It appears as though the AWS SDK is choking on this value in the request.

Looking at both the Cognito API documentation and the Javascript SDK documentation for the updateUserPoolClient operation, it looks like ClientSecret is not a valid input parameter.

Looking at the user pool client update code, the response from the DescribeUserPoolClient call is fed back into the UpdateUserPoolClient call to avoid defaulting certain inputs as per #144. Unfortunately, it looks like this approach has it's own issues as certain properties returned by the Describe operation are not valid when running an Update operation.

My thought is that the most maintainable approach would be to create a deny-list of properties from the Describe operation which are not valid for Update. Those deny-list properties would be removed prior to the Update call. I'm thinking a deny-list for two reasons:

  1. An approve list is likely harder to keep up with and defeats the purpose of #144 in the first place
  2. It may be a little less likely for the Describe operation to add a property not supported in Update
  3. In the event there is a new property that needs to be deny-listed, the code would fail-fast rather than transparently use a default value that might break behavior (back to #144).
  4. This particular API response could be caught with a detailed error message provided to encourage error reporting and corrective action

A quick diff of the Describe operation properties vs the Update operation properties, highlights three properties:

  • ClientSecret
  • CreationDate
  • LastModifiedDate

Hey mate thanks for the very clear issue description and quick PR! Fully agree with your reasoning.
Thanks for the effort