fix(cognito): make callbackUrls required when auth flow is set

[markmansur]

make callbackUrls required when auth flow is explicitly set.

See comment for explanation: #28204 (comment)

Closes #28204

---

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

[aws-cdk-automation]

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

[aws-cdk-automation]

AWS CodeBuild CI Report

• CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
• Commit ID: e31d617
• Result: SUCCEEDED
• Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

[jolo-dev]

Why don't you define a UserPoolClient? Don't we want to test the callbackUrls

[markmansur]

The existing integ tests already test with callbackUrls,

See here:

aws-cdk/packages/@aws-cdk-testing/framework-integ/test/aws-cognito/test/integ.user-pool-client-explicit-props.ts

Line 37 in 03a5ecd

callbackUrls: ['https://redirect-here.myapp.com'],

There wasn't a test for a basic client with no props, so I added this test.

I also added the appropriate unit tests for all cases around callbackUrls.

[jolo-dev]

This is quite complicated.
What do you think about following?

Suggested change

	if (!props.oAuth && callbackUrls === undefined) {
	callbackUrls = ['https://example.com'];
	} else if (props.oAuth?.flows?.authorizationCodeGrant || props.oAuth?.flows?.implicitCodeGrant) {
	if (callbackUrls === undefined || callbackUrls.length === 0) {
	throw new Error('callbackUrls must not be empty when codeGrant or implicitGrant OAuth flows are enabled.');
	// We set a default when the flow is `clientCredentials`, otherwise we check what is set
	let callbackUrls: string[] | undefined = props.oAuth?.flows?.clientCredentials
	? ['https://example.com']
	: props.oAuth?.callbackUrls;
	// Now we do a type guard check
	if (callbackUrls === undefined || callbackUrls.length === 0) {
	throw new Error('callbackUrl must not be empty when codeGrant or implicitGrant OAuth flows are enabled.');
	}

[markmansur]

I don't think that gives the same result.

let callbackUrls: string[] | undefined = props.oAuth?.flows?.clientCredentials
      ? ['https://example.com']
      : props.oAuth?.callbackUrls;

This will set callbacks to example.com as long as clientCredentials is true. We don't want that. We want to use the callbacks passed in and fallback to example.com if needed.

if only clientCredentials is true, we don't require any callbacks.

[jolo-dev]

Ah true, you're right.
So a callback URL always must be set, right?
When looking at the UI, it says this:

It seems like there is no distinction between each oAuth flow.

[scanlonp]

@markmansur I have concerns that this may be a breaking change. I see that having example.com as a callback url is likely not desirable, but I believe that the reproduction code from the issue will now fail at synth time, where it would have successfully deployed previously, correct?

From my understanding, we add this dummy value to avoid deployment failures. Are there cases where the callback function is un-important? Such that having the dummy does not have an affect on the application? If so, I think a warning may be the best course. If a valid callback is necessary, then looking at ways to tightly couple the props could be best.

In any case, better docs and possibly a warning could work. Making sure the user knows about this behavior is the crux of the issue, right?

[jolo-dev]

@markmansur I have concerns that this may be a breaking change. I see that having example.com as a callback url is likely not desirable, but I believe that the reproduction code from the issue will now fail at synth time, where it would have successfully deployed previously, correct?

From my understanding, we add this dummy value to avoid deployment failures. Are there cases where the callback function is un-important? Such that having the dummy does not have an affect on the application? If so, I think a warning may be the best course. If a valid callback is necessary, then looking at ways to tightly couple the props could be best.

In any case, better docs and possibly a warning could work. Making sure the user knows about this behavior is the crux of the issue, right?

Well, if we can avoid this in code that would be best. I believe throwing an error would be the best solution.

[kaizencc]

@markmansur @jolo-dev unfortunately when it comes to breaking changes we don't have much leeway. We simply cannot change the default as is, since it could have been successfully deployed before. We could do a feature flag, but I don't think that's necessary.

I agree with @scanlonp that the path forward for this PR to be accepted is to emit a warning instead of an error. That should be enough deterrent.

[jolo-dev]

Hey @kaizencc @scanlonp,
Thanks for the reply.
I don't think, this is a breaking change. It should throw an error if authorizationCodeGrant and implicitCodeGrant are true and at the same time callbackUrls is not set. I agree that, we can set a default for implicitCodeGrant=true.

[kaizencc]

@jolo-dev @markmansur

It really hinges on whether or not you dispute this comment: "I see that having example.com as a callback url is likely not desirable, but I believe that the reproduction code from the issue will now fail at synth time, where it would have successfully deployed previously, correct?"

If you do, please explain why more clearly, and we can discuss.

If you don't, CDK simply cannot turn something that successfully deploys into something that fails at synth time. We will have to find an alternate solution (which is to emit a warning only).

[jolo-dev]

It really hinges on whether or not you dispute this comment: "I see that having example.com as a callback url is likely not desirable, but I believe that the reproduction code from the issue will now fail at synth time, where it would have successfully deployed previously, correct?"

@kaizencc @scanlonp
Callback URLs are interchangeable.
Even though someone has successfully deployed with example.com, it should be replaced then.

If you do, please explain why more clearly, and we can discuss.

When I tried to deploy it, I forgot to set the Callback URL and Cognito was simply not working (because it was redirecting to example.com) and in the AWS Console I saw example.com.
By the way, I tested it with the code provided by user @markmansur. I had a UserPoolClient deployed with the unchanged code and it resulted in example.com. Then I deployed it again with the code here to change the callback URL. The code worked successfully, and I received an error message when attempting to use the authorizationCodeGrant or implicitCodeGrant methods without setting the callback URL.

[scanlonp]

@jolo-dev I think we talking about slightly different things here. This discussion of breaking change is somewhat independent of the behavior post-deployment. If the code deploys successfully now (whether or not it functions), and we make a change which then causes that same code to now throw an error, that is a breaking change. And we cannot do that.

I believe we are in agreement that making sure the user understands that example.com will be a poor default value is a good change. However, we cannot do that by erroring out on code that deploys as-is today.

[jolo-dev]

@scanlonp So you mean throwing an error is not the right way here? I don't really get it.

Okay, I give up. If that does not work then feel free to close it.

[scanlonp]

Correct, throwing an error is not an option here.