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

fix: Defining CORS when ApiKeyRequired is true results in an OPTIONS method that requires an API key #2981

Merged
merged 17 commits into from
Mar 10, 2023
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
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
5 changes: 1 addition & 4 deletions integration/combination/test_api_with_cors.py
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,7 @@
@skipIf(current_region_does_not_support([REST_API]), "Rest API is not supported in this testing region")
class TestApiWithCors(BaseTest):
@parameterized.expand(
[
"combination/api_with_cors",
"combination/api_with_cors_openapi",
]
["combination/api_with_cors", "combination/api_with_cors_openapi", "combination/api_with_cors_and_apikey"]
ConnorRobertson marked this conversation as resolved.
Show resolved Hide resolved
)
def test_cors(self, file_name):
self.create_and_verify_stack(file_name)
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
[
{
"LogicalResourceId": "MyApi",
"ResourceType": "AWS::ApiGateway::RestApi"
},
{
"LogicalResourceId": "MyApiDeployment",
"ResourceType": "AWS::ApiGateway::Deployment"
},
{
"LogicalResourceId": "MyApidevStage",
"ResourceType": "AWS::ApiGateway::Stage"
},
{
"LogicalResourceId": "ApiGatewayLambdaRole",
"ResourceType": "AWS::IAM::Role"
},
{
"LogicalResourceId": "MyFunction",
"ResourceType": "AWS::Lambda::Function"
},
{
"LogicalResourceId": "MyFunctionRole",
"ResourceType": "AWS::IAM::Role"
}
]
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
AWSTemplateFormatVersion: '2010-09-09'

Transform:
- AWS::Serverless-2016-10-31

Globals:
Api:
Auth:
ApiKeyRequired: true
AddApiKeyRequiredToCorsPreflight: false

Resources:

MyFunction:
Type: AWS::Serverless::Function
Properties:
Handler: index.handler
InlineCode: |
exports.handler = async function (event) {
return {
statusCode: 200,
body: JSON.stringify({ message: "Hello, SAM!" }),
}
}
Runtime: nodejs16.x

ApiGatewayLambdaRole:
Type: AWS::IAM::Role
Properties:
AssumeRolePolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Principal: {Service: apigateway.amazonaws.com}
Action: sts:AssumeRole
Policies:
- PolicyName: AllowInvokeLambdaFunctions
PolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Action: lambda:InvokeFunction
Resource: '*'

MyApi:
Type: AWS::Serverless::Api
Properties:
Cors:
AllowMethods: "'methods'"
AllowHeaders: "'headers'"
AllowOrigin: "'origins'"
MaxAge: "'600'"
Auth:
ApiKeyRequired: true
StageName: dev
DefinitionBody:
openapi: 3.0.1
paths:
/apione:
get:
x-amazon-apigateway-integration:
credentials:
Fn::Sub: ${ApiGatewayLambdaRole.Arn}
uri:
Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${MyFunction.Arn}/invocations
passthroughBehavior: when_no_match
httpMethod: POST
type: aws_proxy
/apitwo:
get:
x-amazon-apigateway-integration:
credentials:
Fn::Sub: ${ApiGatewayLambdaRole.Arn}
uri:
Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${MyFunction.Arn}/invocations
passthroughBehavior: when_no_match
httpMethod: POST
type: aws_proxy



Outputs:
ApiUrl:
Description: URL of your API endpoint
Value: !Sub "https://${MyApi}.execute-api.${AWS::Region}.amazonaws.com/dev/"
Metadata:
SamTransformTest: true
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,7 @@ class UsagePlan(BaseModel):

class Auth(BaseModel):
AddDefaultAuthorizerToCorsPreflight: Optional[bool] = auth("AddDefaultAuthorizerToCorsPreflight")
AddApiKeyRequiredToCorsPreflight: Optional[bool] # TODO Add Docs
ConnorRobertson marked this conversation as resolved.
Show resolved Hide resolved
ApiKeyRequired: Optional[bool] = auth("ApiKeyRequired")
Authorizers: Optional[
Dict[
Expand Down
11 changes: 7 additions & 4 deletions samtranslator/model/api/api_generator.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,12 +52,13 @@
"DefaultAuthorizer",
"InvokeRole",
"AddDefaultAuthorizerToCorsPreflight",
"AddApiKeyRequiredToCorsPreflight",
"ApiKeyRequired",
"ResourcePolicy",
"UsagePlan",
],
)
AuthProperties.__new__.__defaults__ = (None, None, None, True, None, None, None)
AuthProperties.__new__.__defaults__ = (None, None, None, True, True, None, None, None)
UsagePlanProperties = namedtuple(
"UsagePlanProperties", ["CreateUsagePlan", "Description", "Quota", "Tags", "Throttle", "UsagePlanName"]
)
Expand Down Expand Up @@ -752,7 +753,7 @@ def _add_auth(self) -> None:

if auth_properties.ApiKeyRequired:
swagger_editor.add_apikey_security_definition()
self._set_default_apikey_required(swagger_editor)
self._set_default_apikey_required(swagger_editor, auth_properties.AddApiKeyRequiredToCorsPreflight)

if auth_properties.ResourcePolicy:
SwaggerEditor.validate_is_dict(
Expand Down Expand Up @@ -1224,9 +1225,11 @@ def _set_default_authorizer(
add_default_auth_to_preflight=add_default_auth_to_preflight,
)

def _set_default_apikey_required(self, swagger_editor: SwaggerEditor) -> None:
def _set_default_apikey_required(
self, swagger_editor: SwaggerEditor, AddApiKeyRequiredToCorsPreflight: bool
ConnorRobertson marked this conversation as resolved.
Show resolved Hide resolved
) -> None:
for path in swagger_editor.iter_on_path():
swagger_editor.set_path_default_apikey_required(path)
swagger_editor.set_path_default_apikey_required(path, AddApiKeyRequiredToCorsPreflight)

def _set_endpoint_configuration(self, rest_api: ApiGatewayRestApi, value: Union[str, Dict[str, Any]]) -> None:
"""
Expand Down
4 changes: 4 additions & 0 deletions samtranslator/schema/schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -196613,6 +196613,10 @@
"samtranslator__internal__schema_source__aws_serverless_api__Auth": {
"additionalProperties": false,
"properties": {
"AddApiKeyRequiredToCorsPreflight": {
"title": "Addapikeyrequiredtocorspreflight",
"type": "boolean"
},
"AddDefaultAuthorizerToCorsPreflight": {
"description": "If the `DefaultAuthorizer` and `Cors` properties are set, then setting `AddDefaultAuthorizerToCorsPreflight` will cause the default authorizer to be added to the `Options` property in the OpenAPI section\\. \n*Type*: Boolean \n*Required*: No \n*Default*: True \n*AWS CloudFormation compatibility*: This property is unique to AWS SAM and doesn't have an AWS CloudFormation equivalent\\.",
"markdownDescription": "If the `DefaultAuthorizer` and `Cors` properties are set, then setting `AddDefaultAuthorizerToCorsPreflight` will cause the default authorizer to be added to the `Options` property in the OpenAPI section\\. \n*Type*: Boolean \n*Required*: No \n*Default*: True \n*AWS CloudFormation compatibility*: This property is unique to AWS SAM and doesn't have an AWS CloudFormation equivalent\\.",
Expand Down
11 changes: 10 additions & 1 deletion samtranslator/swagger/swagger.py
Original file line number Diff line number Diff line change
Expand Up @@ -612,14 +612,16 @@ def set_path_default_authorizer( # noqa: too-many-branches
if "AWS_IAM" in method_definition["security"][0]:
self.add_awsiam_security_definition()

def set_path_default_apikey_required(self, path: str) -> None:
def set_path_default_apikey_required(self, path: str, AddApiKeyRequiredToCorsPreflight: bool = True) -> None:
ConnorRobertson marked this conversation as resolved.
Show resolved Hide resolved
"""
Add the ApiKey security as required for each method on this path unless ApiKeyRequired
was defined at the Function/Path/Method level. This is intended to be used to set the
apikey security restriction for all api methods based upon the default configured in the
Serverless API.

:param string path: Path name
:param bool AddApiKeyRequiredToCorsPreflight: Bool of whether to add the ApiKeyRequired
to OPTIONS preflight requests.
"""

for method_name, method_definition in self.iter_on_all_methods_for_path(path): # type: ignore[no-untyped-call]
Expand Down Expand Up @@ -673,6 +675,9 @@ def set_path_default_apikey_required(self, path: str) -> None:

security = existing_non_apikey_security + apikey_security

if method_name == "options" and not AddApiKeyRequiredToCorsPreflight:
security = existing_non_apikey_security
ConnorRobertson marked this conversation as resolved.
Show resolved Hide resolved

if security != existing_security:
method_definition["security"] = security

Expand All @@ -695,6 +700,10 @@ def add_auth_to_method(self, path: str, method_name: str, auth: Dict[str, Any],
self._set_method_authorizer(path, method_name, method_authorizer, authorizers, method_scopes) # type: ignore[no-untyped-call]

method_apikey_required = auth and auth.get("ApiKeyRequired")

if auth.get("AddApiKeyRequiredToCorsPreflight") and method_name == "options":
ConnorRobertson marked this conversation as resolved.
Show resolved Hide resolved
method_apikey_required = False

if method_apikey_required is not None:
self._set_method_apikey_handling(path, method_name, method_apikey_required) # type: ignore[no-untyped-call]

Expand Down
4 changes: 4 additions & 0 deletions schema_source/sam.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -3012,6 +3012,10 @@
"samtranslator__internal__schema_source__aws_serverless_api__Auth": {
"additionalProperties": false,
"properties": {
"AddApiKeyRequiredToCorsPreflight": {
"title": "Addapikeyrequiredtocorspreflight",
"type": "boolean"
},
"AddDefaultAuthorizerToCorsPreflight": {
"description": "If the `DefaultAuthorizer` and `Cors` properties are set, then setting `AddDefaultAuthorizerToCorsPreflight` will cause the default authorizer to be added to the `Options` property in the OpenAPI section\\. \n*Type*: Boolean \n*Required*: No \n*Default*: True \n*AWS CloudFormation compatibility*: This property is unique to AWS SAM and doesn't have an AWS CloudFormation equivalent\\.",
"markdownDescription": "If the `DefaultAuthorizer` and `Cors` properties are set, then setting `AddDefaultAuthorizerToCorsPreflight` will cause the default authorizer to be added to the `Options` property in the OpenAPI section\\. \n*Type*: Boolean \n*Required*: No \n*Default*: True \n*AWS CloudFormation compatibility*: This property is unique to AWS SAM and doesn't have an AWS CloudFormation equivalent\\.",
Expand Down
87 changes: 87 additions & 0 deletions tests/translator/input/api_with_cors_and_apikey.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,87 @@
AWSTemplateFormatVersion: '2010-09-09'

Transform:
- AWS::Serverless-2016-10-31

Globals:
Api:
Auth:
ApiKeyRequired: true
AddApiKeyRequiredToCorsPreflight: false

Resources:

MyFunction:
Type: AWS::Serverless::Function
Properties:
Handler: index.handler
InlineCode: |
exports.handler = async function (event) {
return {
statusCode: 200,
body: JSON.stringify({ message: "Hello, SAM!" }),
}
}
Runtime: nodejs16.x

ApiGatewayLambdaRole:
Type: AWS::IAM::Role
Properties:
AssumeRolePolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Principal: {Service: apigateway.amazonaws.com}
Action: sts:AssumeRole
Policies:
- PolicyName: AllowInvokeLambdaFunctions
PolicyDocument:
Version: '2012-10-17'
Statement:
- Effect: Allow
Action: lambda:InvokeFunction
Resource: '*'

MyApi:
Type: AWS::Serverless::Api
Properties:
Cors:
AllowMethods: "'methods'"
AllowHeaders: "'headers'"
AllowOrigin: "'origins'"
MaxAge: "'600'"
Auth:
ApiKeyRequired: true
StageName: dev
DefinitionBody:
openapi: 3.0.1
paths:
/apione:
get:
x-amazon-apigateway-integration:
credentials:
Fn::Sub: ${ApiGatewayLambdaRole.Arn}
uri:
Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${MyFunction.Arn}/invocations
passthroughBehavior: when_no_match
httpMethod: POST
type: aws_proxy
/apitwo:
get:
x-amazon-apigateway-integration:
credentials:
Fn::Sub: ${ApiGatewayLambdaRole.Arn}
uri:
Fn::Sub: arn:aws:apigateway:${AWS::Region}:lambda:path/2015-03-31/functions/${MyFunction.Arn}/invocations
passthroughBehavior: when_no_match
httpMethod: POST
type: aws_proxy



Outputs:
ApiUrl:
Description: URL of your API endpoint
Value: !Sub "https://${MyApi}.execute-api.${AWS::Region}.amazonaws.com/dev/"
Metadata:
SamTransformTest: true
Loading