Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Closed
wants to merge 2 commits into from
Closed

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5dc506
fix(aws-cognito): make callbackUrls required when auth flow is explic…
markmansur Dec 3, 2023
e31d617
Add integ tests
markmansur Dec 3, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions packages/aws-cdk-lib/aws-cognito/lib/user-pool-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,12 +45,13 @@ export interface OAuthSettings {
* OAuth flows that are allowed with this client.
* @see - the 'Allowed OAuth Flows' section at https://docs.aws.amazon.com/cognito/latest/developerguide/cognito-user-pools-app-idp-settings.html
* @default {authorizationCodeGrant:true,implicitCodeGrant:true}
* @requires at least one callback if explicity set.
*/
readonly flows?: OAuthFlows;

/**
* List of allowed redirect URLs for the identity providers.
* @default - ['https://example.com'] if either authorizationCodeGrant or implicitCodeGrant flows are enabled, no callback URLs otherwise.
* @default - ['https://example.com'] if `flows` isn't set.
*/
readonly callbackUrls?: string[];

Expand Down Expand Up @@ -391,11 +392,11 @@ export class UserPoolClient extends Resource implements IUserPoolClient {
};

let callbackUrls: string[] | undefined = props.oAuth?.callbackUrls;
if (this.oAuthFlows.authorizationCodeGrant || this.oAuthFlows.implicitCodeGrant) {
if (callbackUrls === undefined) {
callbackUrls = ['https://example.com'];
} else if (callbackUrls.length === 0) {
throw new Error('callbackUrl must not be empty when codeGrant or implicitGrant OAuth flows are enabled.');
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.');
Comment on lines +395 to +399
Copy link
Contributor

Choose a reason for hiding this comment

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

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.');
}

Copy link
Contributor Author

@markmansur markmansur Dec 3, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

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

}
}

Expand Down
66 changes: 59 additions & 7 deletions packages/aws-cdk-lib/aws-cognito/test/user-pool-client.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,6 +338,7 @@ describe('User Pool Client', () => {
authorizationCodeGrant: true,
implicitCodeGrant: true,
},
callbackUrls: ['https://example.com'],
scopes: [OAuthScope.PHONE],
},
});
Expand Down Expand Up @@ -377,16 +378,35 @@ describe('User Pool Client', () => {
pool.addClient('Client2', {
oAuth: {
flows: {
authorizationCodeGrant: true,
clientCredentials: true,
},
callbackUrls: ['https://aws.com'],
},
});

pool.addClient('Client3', {
oAuth: {
flows: {
authorizationCodeGrant: true,
},
callbackUrls: ['https://aws.com'],
},
});

pool.addClient('Client4', {
oAuth: {
flows: {
implicitCodeGrant: true,
},
callbackUrls: ['https://aws.com'],
},
});

pool.addClient('Client5');

pool.addClient('Client6', {
oAuth: {
callbackUrls: ['https://aws.com'],
},
});

Expand All @@ -396,14 +416,29 @@ describe('User Pool Client', () => {
});

Template.fromStack(stack).hasResourceProperties('AWS::Cognito::UserPoolClient', {
AllowedOAuthFlows: ['implicit'],
CallbackURLs: ['https://example.com'],
AllowedOAuthFlows: ['client_credentials'],
CallbackURLs: ['https://aws.com'],
});

Template.fromStack(stack).hasResourceProperties('AWS::Cognito::UserPoolClient', {
AllowedOAuthFlows: ['code'],
CallbackURLs: ['https://aws.com'],
});

Template.fromStack(stack).hasResourceProperties('AWS::Cognito::UserPoolClient', {
AllowedOAuthFlows: ['implicit'],
CallbackURLs: ['https://aws.com'],
});

Template.fromStack(stack).hasResourceProperties('AWS::Cognito::UserPoolClient', {
AllowedOAuthFlows: ['implicit', 'code'],
CallbackURLs: ['https://example.com'],
});

Template.fromStack(stack).hasResourceProperties('AWS::Cognito::UserPoolClient', {
AllowedOAuthFlows: ['implicit', 'code'],
CallbackURLs: ['https://aws.com'],
});
});

test('callbackUrls are not rendered if OAuth is disabled ', () => {
Expand All @@ -423,7 +458,7 @@ describe('User Pool Client', () => {
});
});

test('fails when callbackUrls is empty for codeGrant or implicitGrant', () => {
test('fails when callbackUrls is empty/undefined for codeGrant or implicitGrant', () => {
const stack = new Stack();
const pool = new UserPool(stack, 'Pool');

Expand All @@ -432,21 +467,36 @@ describe('User Pool Client', () => {
flows: { implicitCodeGrant: true },
callbackUrls: [],
},
})).toThrow(/callbackUrl must not be empty/);
})).toThrow(/callbackUrls must not be empty/);

expect(() => pool.addClient('Client3', {
expect(() => pool.addClient('Client2', {
oAuth: {
flows: { authorizationCodeGrant: true },
callbackUrls: [],
},
})).toThrow(/callbackUrl must not be empty/);
})).toThrow(/callbackUrls must not be empty/);

expect(() => pool.addClient('Client3', {
oAuth: {
flows: { implicitCodeGrant: true },
},
})).toThrow(/callbackUrls must not be empty/);

expect(() => pool.addClient('Client4', {
oAuth: {
flows: { authorizationCodeGrant: true },
},
})).toThrow(/callbackUrls must not be empty/);

expect(() => pool.addClient('Client5', {
oAuth: {
flows: { clientCredentials: true },
callbackUrls: [],
},
})).not.toThrow();

expect(() => pool.addClient('Client6', {
})).not.toThrow();
});

test('logoutUrls can be set', () => {
Expand Down Expand Up @@ -474,6 +524,7 @@ describe('User Pool Client', () => {
authorizationCodeGrant: true,
clientCredentials: true,
},
callbackUrls: ['https://example.com'],
scopes: [OAuthScope.PHONE],
},
})).toThrow(/clientCredentials OAuth flow cannot be selected/);
Expand All @@ -484,6 +535,7 @@ describe('User Pool Client', () => {
implicitCodeGrant: true,
clientCredentials: true,
},
callbackUrls: ['https://example.com'],
scopes: [OAuthScope.PHONE],
},
})).toThrow(/clientCredentials OAuth flow cannot be selected/);
Expand Down
Loading