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

[aws-apigateway] impossible to remove default method authorization #8827

Closed
erik-sab opened this issue Jul 1, 2020 · 21 comments · Fixed by #30415 · 4 remaining pull requests
Closed

[aws-apigateway] impossible to remove default method authorization #8827

erik-sab opened this issue Jul 1, 2020 · 21 comments · Fixed by #30415 · 4 remaining pull requests
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 p1

Comments

@erik-sab
Copy link

erik-sab commented Jul 1, 2020

It seems not possible to remove authorization for API Gateway methods if it is defined in defaultMethodOptions on RestApi level.

Reproduction Steps

First I create RestApi Gateway (https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-apigateway.RestApi.html) with custom authorizer set by default for all methods:

        var apiGw = RestApi.Builder.create(scope, "Stack-RestApi")
                .defaultMethodOptions(MethodOptions.builder()
                        .apiKeyRequired(Boolean.FALSE)
                        .authorizationType(AuthorizationType.CUSTOM)
                        .authorizer(authorizer)
                        .build())
...

And then in resources stack I try to create documentation Method (https://docs.aws.amazon.com/cdk/api/latest/docs/@aws-cdk_aws-apigateway.Method.html) with security disabled:

        Method.Builder builder = Method.Builder.create(scope, "Stack-ApiInfoMethodGET")
                .options(MethodOptions.builder()
                        .apiKeyRequired(Boolean.FALSE)
                        .authorizationType(AuthorizationType.NONE)
                        .authorizer(null) // tried to reset authorizer also
                        .build())
...

Error Log

[ERROR] Failed to execute goal org.codehaus.mojo:exec-maven-plugin:1.6.0:java (default-cli) on project cdk-stacks: An exception occured while executing the Java class. Stack-ApiGwResource/Stack-ApiInfoResource/GET - Authorization type is set to NONE which is different from what is required by the authorizer [CUSTOM]
[ERROR] Error: Stack-ApiGwResource/Stack-ApiInfoResource/GET - Authorization type is set to NONE which is different from what is required by the authorizer [CUSTOM]
[ERROR]     at new Method (/tmp/jsii-kernel-TH8eSk/node_modules/@aws-cdk/aws-apigateway/lib/method.js:27:19)
[ERROR]     at /tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:7906:49
[ERROR]     at Kernel._wrapSandboxCode (/tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:8382:19)
[ERROR]     at Kernel._create (/tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:7906:26)
[ERROR]     at Kernel.create (/tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:7650:21)
[ERROR]     at KernelHost.processRequest (/tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:7439:28)
[ERROR]     at KernelHost.run (/tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:7377:14)
[ERROR]     at Immediate._onImmediate (/tmp/jsii-java-runtime16647868641746308035/jsii-runtime.js:7380:37)
[ERROR]     at processImmediate (internal/timers.js:456:21)

Environment

  • CLI Version : 1.47.0
  • Framework Version: 1.47.0
  • Node.js Version: v12.18.1
  • OS : Ubuntu Linux
  • Language (Version): Java 11

Other

It is still possible to override these setting as described in #8615

        var cfnMethod = (CfnMethod) method.getNode().getDefaultChild();
        cfnMethod.addPropertyOverride("ApiKeyRequired", false);
        cfnMethod.addPropertyOverride("AuthorizationType", "NONE");
        cfnMethod.addPropertyDeletionOverride("AuthorizerId");

and then stack is created with correct Method-level security settings.


This is 🐛 Bug Report

@erik-sab erik-sab added bug This issue is a bug. needs-triage This issue or PR still needs to be triaged. labels Jul 1, 2020
@github-actions github-actions bot added the @aws-cdk/aws-apigateway Related to Amazon API Gateway label Jul 1, 2020
@nija-at nija-at added p2 and removed needs-triage This issue or PR still needs to be triaged. labels Jul 13, 2020
@nija-at nija-at changed the title [aws-apigateway] impossible to remove dafault method authorization [aws-apigateway] impossible to remove default method authorization Jul 13, 2020
@nija-at nija-at removed the p2 label Jul 13, 2020
@nija-at
Copy link
Contributor

nija-at commented Jul 13, 2020

It does seem like we don't handle the NONE case correctly here -

const authorizationTypeOption = options.authorizationType || defaultMethodOptions.authorizationType;
const authorizationType = authorizer?.authorizationType || authorizationTypeOption || AuthorizationType.NONE;
// if the authorizer defines an authorization type and we also have an explicit option set, check that they are the same
if (authorizer?.authorizationType && authorizationTypeOption && authorizer?.authorizationType !== authorizationTypeOption) {

@nija-at nija-at added the p2 label Jul 13, 2020
@nija-at nija-at added the effort/small Small work item – less than a day of effort label Aug 10, 2020
@mattsenior
Copy link

I’ve also noticed a similar issue trying to set apiKeyRequired: false when I’ve got apiKeyRequired: true in defaultMethodOptions:

apiKeyRequired: options.apiKeyRequired || defaultMethodOptions.apiKeyRequired,

I think || should be ??

@erik-sab
Copy link
Author

erik-sab commented Oct 2, 2020

apiKeyRequired: options.apiKeyRequired || defaultMethodOptions.apiKeyRequired,

I think || should be ??

IMHO it is not so simple, as we need to distinguish when options.apiKeyRequired is not set.

Probably should be something like:

apiKeyRequired = (options.apiKeyRequired !== undefined && options.apiKeyRequired !== null) ?
    options.apiKeyRequired :
    defaultMethodOptions.apiKeyRequired

Only not sure what is current default for options.apiKeyRequired

@mattsenior
Copy link

@erik-telesoftas Hey I’m not sure I follow, that’s what ?? does, right?

@erik-sab
Copy link
Author

erik-sab commented Oct 5, 2020

@mattsenior absolutely right, probably fool moon on top of my JS syntax ignorance ;)

@shellscape
Copy link

Of possibly related trouble is the inability to instruct a particular method not to use an authorizer, once defaultMethodOptions is set @nija-at

@bmacher
Copy link
Contributor

bmacher commented Nov 9, 2021

Hi @nija-at, will there be any progress on this issue soon? As a workaround one could write an aspect that removes the authorizer from certain methods. Not beautiful, but would work.

const excludedMethodIds: Method[] = [];

const aspect: IAspect = {
  visit(node: IConstruct): void {
    if (
      node instanceof CfnMethod
      && !excludedMethodIds.find((rid) => rid === node.resourceId)
    ) {
      delete node.authorizerId;
      node.authorizationType = AuthorizationType.NONE;
    }
  },
};

Aspects.of(myStack).add(aspect);

@markcarroll
Copy link

apiKeyRequired: options.apiKeyRequired || defaultMethodOptions.apiKeyRequired,

This issue (the || needing to be ??) is what is causing #8615 to still be broken if the default is to have API key required since the fix added in #22402 seems to get overridden by the default setting - options.apiKeyRequired is false which leads the other side of the boolean operator to get evaluated which then returns whatever defaultMethodOptions.apiKeyRequired is set to.

@mirgj
Copy link
Contributor

mirgj commented Nov 25, 2022

facing the same issue as well with IAM:

Authorization type is set to AWS_IAM which is different from what is required by the authorizer [COGNITO_USER_POOLS]

this is the part of code that does the check:
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-apigateway/lib/method.ts#L187-L198

and this is the test that covers it:
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-apigateway/test/method.test.ts#L700-L716

I'm trying to understand the reason of enforcing all route to use the "default authorizer" while API Gateway does allow to have a route that uses Cognito and another one that uses IAM (eg. for internal services for example).

I think the only check that makes sense to do is the one covered by this other test:
https://github.com/aws/aws-cdk/blob/main/packages/%40aws-cdk/aws-apigateway/test/method.test.ts#L685-L698

@stleon
Copy link

stleon commented Mar 19, 2023

Hello from 2023, I have the same problem :)

@shellscape
Copy link

@rix0rrr given this one is coming up on 3 years old it could probably use some eyeballs if just for confirmation.

@devpascoe
Copy link

same

iRoachie added a commit to iRoachie/aws-cdk that referenced this issue May 23, 2023
RestApi has the ability to set the apiKeyRequired option for all methods via defaultMethodOptions.

Setting this option on a method should override the value set in defaultMethodOptions, but it doesn't work.

This commit fixes the behaviour and adds a test.

Mentioned in aws#8827
@iRoachie
Copy link
Contributor

@mattsenior @markcarroll submitted a PR for the apiKeyRequired here #25682.

?? works - made a unit test for it as well - https://github.com/aws/aws-cdk/pull/25682/files#diff-2a75ad420d0c98981d7269b81d6496388ba7399a933e05b43d6d13686b181833R1334

mergify bot pushed a commit that referenced this issue Jun 5, 2023
RestApi has the ability to set the apiKeyRequired option for all methods via defaultMethodOptions.

Setting this option on a method should override the value set in defaultMethodOptions, but it doesn't work.

This commit fixes the behaviour and adds a test.

Mentioned in #8827

----

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

Hi @nija-at, will there be any progress on this issue soon? As a workaround one could write an aspect that removes the authorizer from certain methods. Not beautiful, but would work.

const excludedMethodIds: Method[] = [];

const aspect: IAspect = {
  visit(node: IConstruct): void {
    if (
      node instanceof CfnMethod
      && !excludedMethodIds.find((rid) => rid === node.resourceId)
    ) {
      delete node.authorizerId;
      node.authorizationType = AuthorizationType.NONE;
    }
  },
};

Aspects.of(myStack).add(aspect);

This didn't work for me.

In the end, I had to set my API root's default authorization type to NONE and then set the authorizer at the top of every tree in my API that consists entirely of endpoints requiring authorization. So overriding a default of NONE with an authorizer works, but overriding a default authorizer with NONE does not. Not ideal.

@nija-at: is there any plan to fix this?

@nija-at
Copy link
Contributor

nija-at commented Jun 22, 2023

Hi @cupid-dev - I no longer work for the AWS CDK. Someone from the team will respond to you in due time.

@github-actions github-actions bot removed the p2 label Jul 30, 2023
@github-actions github-actions bot added the p1 label Jul 30, 2023
@github-actions
Copy link

This issue has received a significant amount of attention so we are automatically upgrading its priority. A member of the community will see the re-prioritization and provide an update on the issue.

@charlierharper
Copy link

Any updates on this issue?
Authorization type is set to NONE which is different from what is required by the authorizer [CUSTOM]

@kcp-chewie
Copy link
Contributor

As a workaround, you can use the CDK -> CloudFormation escape hatch.

In Python, that looks like this:
noauth_method.node.default_child.add_property_override("AuthorizationType", "NONE")

More details at #29658

@HansFalkenberg-Visma
Copy link

+1

@moelasmar moelasmar self-assigned this May 8, 2024
@typefox09
Copy link

#29658

Doing this removes the authorizer for every route though. AWS CDK is the worst, very little documentation, very little support. This issue has been activer since July 2020????

@mergify mergify bot closed this as completed in #30415 Jun 1, 2024
mergify bot pushed a commit that referenced this issue Jun 1, 2024
### Issue # (if applicable)

Closes #8827.

### Reason for this change

Customers could not override the authorizer defined in the default method configuration if they want to set the authorization type to None.

### Description of changes

If the customer set the authorization type to None while creating a new method, we will not use the authorizer value defined in the default configuration and instead we will set it to undefined.

### Description of how you validated changes

added unit, and integration test cases.

### Checklist
- [X] 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*
Copy link

github-actions bot commented Jun 1, 2024

⚠️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.

atanaspam pushed a commit to atanaspam/aws-cdk that referenced this issue Jun 3, 2024
### Issue # (if applicable)

Closes aws#8827.

### Reason for this change

Customers could not override the authorizer defined in the default method configuration if they want to set the authorization type to None.

### Description of changes

If the customer set the authorization type to None while creating a new method, we will not use the authorizer value defined in the default configuration and instead we will set it to undefined.

### Description of how you validated changes

added unit, and integration test cases.

### Checklist
- [X] 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*
vdahlberg pushed a commit to vdahlberg/aws-cdk that referenced this issue Jun 10, 2024
### Issue # (if applicable)

Closes aws#8827.

### Reason for this change

Customers could not override the authorizer defined in the default method configuration if they want to set the authorization type to None.

### Description of changes

If the customer set the authorization type to None while creating a new method, we will not use the authorizer value defined in the default configuration and instead we will set it to undefined.

### Description of how you validated changes

added unit, and integration test cases.

### Checklist
- [X] 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*
Leo10Gama pushed a commit to Leo10Gama/aws-cdk that referenced this issue Jun 11, 2024
### Issue # (if applicable)

Closes aws#8827.

### Reason for this change

Customers could not override the authorizer defined in the default method configuration if they want to set the authorization type to None.

### Description of changes

If the customer set the authorization type to None while creating a new method, we will not use the authorizer value defined in the default configuration and instead we will set it to undefined.

### Description of how you validated changes

added unit, and integration test cases.

### Checklist
- [X] 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*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment