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.

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

❗ NOTICE (aws-apigateway): Authorization Scopes not rendered with CognitoUserPoolsAuthorizer #30444

Closed
t0bst4r opened this issue Jun 4, 2024 · 12 comments · Fixed by #30822 or rwlxxvii/containers#185 · May be fixed by NOUIY/aws-solutions-constructs#112, gitafolabi/kreuzlaker#2 or NOUIY/aws-solutions-constructs#113
Assignees
Labels
@aws-cdk/aws-apigateway Related to Amazon API Gateway bug This issue is a bug. effort/small Small work item – less than a day of effort management/tracking Issues that track a subject or multiple issues p0

Comments

@t0bst4r
Copy link

t0bst4r commented Jun 4, 2024

Please add your +1 👍 to let us know you have encountered this

Status: IN-PROGRESS

Overview:

Describe the bug

When using the CognitoUserPoolsAuthorizer with authorizationScope, the scopes are not rendered to the CloudFormation template, if authorizationType is not set explicitly. This worked before version 2.142.0.

Expected Behavior

Scopes are rendered to the CloudFormation template when using CognitoUserPoolsAuthorizer without setting authorizationType explicitly. The authorizationType of the authorizer should be used implicitly.

Current Behavior

Scopes are not rendered to the CloudFormation template when using CognitoUserPoolsAuthorizer without setting authorizationType explicitly. They are only rendered, when authorizationType is set explicitly.

A warning is printed during CDK synth:

'AuthorizationScopes' can only be set when 'AuthorizationType' sets 'COGNITO_USER_POOLS'. Default to ignore the values set in 'AuthorizationScopes'.

Reproduction Steps

import { Stack } from "aws-cdk-lib";
import { UserPool } from "aws-cdk-lib/aws-cognito";
import { CognitoUserPoolsAuthorizer, RestApi } from "aws-cdk-lib/aws-apigateway";
import { Template } from "aws-cdk-lib/assertions";

const stack = new Stack(undefined, "Stack");
const userPool = UserPool.fromUserPoolId(stack, "UserPool", "userPoolId");
const authorizer = new CognitoUserPoolsAuthorizer(stack, "Authorizer", {
  cognitoUserPools: [userPool],
});
const restApi = new RestApi(stack, "RestApi", {
  deploy: true,
  defaultMethodOptions: {
    authorizer,
    // here we need to add the authorizationType to make it work
  },
});
restApi.root.resourceForPath("/user/profile").addMethod("GET", undefined, {
  authorizationScopes: [OAuthScope.PROFILE.scopeName], // this scope is missing
});
restApi.root.resourceForPath("/any/other").addMethod("POST");

console.log(Template.fromStack(stack).toJSON());

Workaround:

There are 2 workarounds:

  • you can pin the aws-cdk-lib version to 2.141.0.
  • You can update the RestApi definition as following
const restApi = new RestApi(stack, "RestApi", {
 deploy: true,
 defaultMethodOptions: {
   authorizer,
   // setting it explicit:
   authorizationType: authorizer.authorizationType, 
 },
});

Solution:

We are reverting this PR that introduces the breaking change.

@t0bst4r t0bst4r added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jun 4, 2024
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Jun 4, 2024
@khushail khushail added needs-reproduction This issue needs reproduction. and removed needs-triage This issue or PR still needs to be triaged. labels Jun 7, 2024
@khushail khushail self-assigned this Jun 7, 2024
@steamstreetjon
Copy link

Yeah, just upgraded to the latest CDK and all the scopes were blown away. This is pretty bad.

@t0bst4r
Copy link
Author

t0bst4r commented Jun 11, 2024

You can do the following workaround until it’s fixed:

const restApi = new RestApi(stack, "RestApi", {
  deploy: true,
  defaultMethodOptions: {
    authorizer,
    // setting it explicit:
    authorizationType: authorizer.authorizationType, 
  },
});

@khushail khushail added investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-reproduction This issue needs reproduction. labels Jul 3, 2024
@khushail
Copy link
Contributor

khushail commented Jul 3, 2024

Hey @t0bst4r , thanks for reporting this. could you please share the output of cdk diff when the scopes are not rendered and the CDK version which worked successfully ?

@khushail khushail added response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jul 3, 2024
Copy link

github-actions bot commented Jul 5, 2024

This issue has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

@github-actions github-actions bot added the closing-soon This issue will automatically close in 4 days unless further comments are made. label Jul 5, 2024
@t0bst4r
Copy link
Author

t0bst4r commented Jul 5, 2024

Yes, I’ll provide it tomorrow.

@github-actions github-actions bot removed closing-soon This issue will automatically close in 4 days unless further comments are made. response-requested Waiting on additional info and feedback. Will move to "closing-soon" in 7 days. labels Jul 6, 2024
@t0bst4r
Copy link
Author

t0bst4r commented Jul 6, 2024

I created a new AWS account, created a new CDK project using 2.141.0.
I have created the following stack:

import * as cdk from 'aws-cdk-lib';
import { Construct } from 'constructs';
import {CognitoUserPoolsAuthorizer, MockIntegration, RestApi} from "aws-cdk-lib/aws-apigateway";
import {OAuthScope, UserPool} from "aws-cdk-lib/aws-cognito";

export class CdkTestStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const userPool = new UserPool(this, "UserPool");

    const authorizer = new CognitoUserPoolsAuthorizer(this, "Authorizer", {
      cognitoUserPools: [userPool]
    });

    const restApi = new RestApi(this, 'CdkTestStack', {
      defaultMethodOptions: {
        authorizer,
        authorizationScopes: [OAuthScope.PROFILE.scopeName]
      }
    });

    restApi.root
        .resourceForPath("/user")
        .addMethod("GET", new MockIntegration());

    restApi.root
        .resourceForPath("/other")
        .addMethod("POST", new MockIntegration(), {
          authorizationScopes: [OAuthScope.OPENID.scopeName]
        });
  }
}

I bootstrapped the account and deployed the application:

➜  cdk-test git:(main) ✗ npx cdk bootstrap
 ⏳  Bootstrapping environment aws://xxxxxxxxxxxx/eu-central-1...
Trusted accounts for deployment: (none)
Trusted accounts for lookup: (none)
Using default execution policy of 'arn:aws:iam::aws:policy/AdministratorAccess'. Pass '--cloudformation-execution-policies' to customize.
CDKToolkit: creating CloudFormation changeset...
 ✅  Environment aws://xxxxxxxxxxxx/eu-central-1 bootstrapped.

➜  cdk-test git:(main) ✗ npx cdk deploy   

✨  Synthesis time: 2.25s

CdkTestStack:  start: Building 1f3bcbb7f34108d0ff41cec88e179d7b4899a1e72820bd04168af80bc4d977fb:xxxxxxxxxxxx-eu-central-1
CdkTestStack:  success: Built 1f3bcbb7f34108d0ff41cec88e179d7b4899a1e72820bd04168af80bc4d977fb:xxxxxxxxxxxx-eu-central-1
CdkTestStack:  start: Publishing 1f3bcbb7f34108d0ff41cec88e179d7b4899a1e72820bd04168af80bc4d977fb:xxxxxxxxxxxx-eu-central-1
CdkTestStack:  success: Published 1f3bcbb7f34108d0ff41cec88e179d7b4899a1e72820bd04168af80bc4d977fb:xxxxxxxxxxxx-eu-central-1
CdkTestStack: deploying... [1/1]
CdkTestStack: creating CloudFormation changeset...

 ✅  CdkTestStack

✨  Deployment time: 22.07s

Then I updated to 2.148.0, did not change anything in the cdk construct / stack. Run cdk diff:

➜  cdk-test git:(main) ✗ npx cdk diff
[Warning at /CdkTestStack/CdkTestStack/Default/user/GET] 'AuthorizationScopes' can only be set when 'AuthorizationType' sets 'COGNITO_USER_POOLS'. Default to ignore the values set in 'AuthorizationScopes'. [ack: @aws-cdk/aws-apigateway:invalidAuthScope]
[Warning at /CdkTestStack/CdkTestStack/Default/other/POST] 'AuthorizationScopes' can only be set when 'AuthorizationType' sets 'COGNITO_USER_POOLS'. Default to ignore the values set in 'AuthorizationScopes'. [ack: @aws-cdk/aws-apigateway:invalidAuthScope]
Stack CdkTestStack
Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)
Resources
[-] AWS::ApiGateway::Deployment CdkTestStack/Deployment CdkTestStackDeployment201712E9bdb85151af2e300bf835b803e5db9d23 destroy
[+] AWS::ApiGateway::Deployment CdkTestStack/Deployment CdkTestStackDeployment201712E9429aaeea94815003ee36140be839d0b7 
[~] AWS::ApiGateway::Stage CdkTestStack/DeploymentStage.prod CdkTestStackDeploymentStageprod296E6472 
 └─ [~] DeploymentId
     └─ [~] .Ref:
         ├─ [-] CdkTestStackDeployment201712E9bdb85151af2e300bf835b803e5db9d23
         └─ [+] CdkTestStackDeployment201712E9429aaeea94815003ee36140be839d0b7
[~] AWS::ApiGateway::Method CdkTestStack/Default/user/GET CdkTestStackuserGET1317379E 
 └─ [-] AuthorizationScopes
     └─ ["profile"]
[~] AWS::ApiGateway::Method CdkTestStack/Default/other/POST CdkTestStackotherPOST3BA23287 
 └─ [-] AuthorizationScopes
     └─ ["openid"]


✨  Number of stacks with differences: 1

Then I change the cdk code to include an EXPLICIT authorization type:

import * as cdk from 'aws-cdk-lib';
import {Construct} from 'constructs';
import {CognitoUserPoolsAuthorizer, MockIntegration, RestApi} from "aws-cdk-lib/aws-apigateway";
import {OAuthScope, UserPool} from "aws-cdk-lib/aws-cognito";

export class CdkTestStack extends cdk.Stack {
  constructor(scope: Construct, id: string, props?: cdk.StackProps) {
    super(scope, id, props);

    const userPool = new UserPool(this, "UserPool");

    const authorizer = new CognitoUserPoolsAuthorizer(this, "Authorizer", {
      cognitoUserPools: [userPool]
    });

    const restApi = new RestApi(this, 'CdkTestStack', {
      defaultMethodOptions: {
        authorizer,
        authorizationScopes: [OAuthScope.PROFILE.scopeName],
        authorizationType: authorizer.authorizationType // <-- ONLY CHANGED LINE
      }
    });

    restApi.root
        .resourceForPath("/user")
        .addMethod("GET", new MockIntegration());

    restApi.root
        .resourceForPath("/other")
        .addMethod("POST", new MockIntegration(), {
          authorizationScopes: [OAuthScope.OPENID.scopeName]
        });

  }
}

Then running cdk diff:

➜  cdk-test git:(main) ✗ npx cdk diff
Stack CdkTestStack
Hold on while we create a read-only change set to get a diff with accurate replacement information (use --no-change-set to use a less accurate but faster template-only diff)
There were no differences

✨  Number of stacks with differences: 0

In my first post, I linked to the lines that are most likely the cause of this problem.

@khushail khushail added needs-reproduction This issue needs reproduction. investigating This issue is being investigated and/or work is in progress to resolve the issue. and removed needs-reproduction This issue needs reproduction. labels Jul 8, 2024
@khushail
Copy link
Contributor

Thanks @t0bst4r for sharing the code and diff snippet. I can see the difference in the template generated in CDK 2.148.0.

Thanks for reporting this. Marking it as P1 as its a regression and should be fixed.

@khushail khushail added p1 effort/small Small work item – less than a day of effort and removed investigating This issue is being investigated and/or work is in progress to resolve the issue. labels Jul 10, 2024
@khushail khushail removed their assignment Jul 10, 2024
@pahud pahud self-assigned this Jul 10, 2024
@GavinZZ
Copy link
Contributor

GavinZZ commented Jul 10, 2024

Thanks for the report. I can confirm that this is a regression and we will treat it as p0. Created the revert PR to the original change that caused the regression and will do a patch release to get this fixed. Apologize for any inconvenience.

@moelasmar
Copy link
Contributor

moelasmar commented Jul 10, 2024

You can do the following workaround until it’s fixed:

As another workaround you can pin the aws-cdk-lib version to 2.141.0

@moelasmar moelasmar added the management/tracking Issues that track a subject or multiple issues label Jul 10, 2024
@moelasmar moelasmar pinned this issue Jul 10, 2024
moelasmar added a commit to moelasmar/aws-cdk-notices that referenced this issue Jul 10, 2024
mergify bot pushed a commit to cdklabs/aws-cdk-notices that referenced this issue Jul 10, 2024
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

1 similar comment
Copy link

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.

moelasmar pushed a commit that referenced this issue Jul 11, 2024
… defining authorization type in method or root api (#30822)

### Issue # (if applicable)

Closes #30444

### Reason for this change

The original PR caused a breaking change, we can't rollback because it
was released in v2.142.0 and it fixes customers issues (partially).
Simply doing a revert will be breaking for those customers again.

### Description of changes

Identified the root cause and we should use `AuthorizationType` instead
of `AuthorizationTypeOption`. `AuthorizationType` defaults to find the
authorization type from the authorizer, falling back to use the auth
type defined in the `Method` construct's options property and falling
back to `None`.

`AuthorizationTypeOptions` on the other hand tries to find the auth type
from `Method` construct's options property which can be None because
it's optional.

### Description of how you validated changes

New unit tests covering the changes and new integration tests covering
it.

### Checklist
- [ ] My code adheres to the [CONTRIBUTING
GUIDE](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md) and
[DESIGN
GUIDELINES](https://github.com/aws/aws-cdk/blob/main/docs/DESIGN_GUIDELINES.md)

----

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

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
@GavinZZ GavinZZ changed the title aws-apigateway: Authorization Scopes not rendered with CognitoUserPoolsAuthorizer ❗ NOTICE (aws-apigateway): Authorization Scopes not rendered with CognitoUserPoolsAuthorizer Jul 16, 2024
@aws-cdk-automation
Copy link
Collaborator

Comments on closed issues and PRs are hard for our team to see. If you need help, please open a new issue that references this one.

@aws aws locked as resolved and limited conversation to collaborators Jul 25, 2024
@colifran colifran unpinned this issue Aug 19, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.