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

feat(auth): add support for API Gateway Authorizers #546

Merged
merged 20 commits into from
Sep 21, 2018
Merged

feat(auth): add support for API Gateway Authorizers #546

merged 20 commits into from
Sep 21, 2018

Conversation

brettstack
Copy link
Contributor

@brettstack brettstack commented Aug 9, 2018

This is a WIP. Remaining:

  • Tests
  • Documentation
  • Examples
  • Additional end-to-end manual verification

Issue #, if available:

#512

Description of changes:

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

This is a WIP. Remaining:

1. Tests
1. Documentation
1. Examples
1. Additional end-to-end  manual verification
1. Fixing some known edge cases (e.g. remove hanging NONE authorizers when no DefaultAuthorizer set)
samtranslator/model/apigateway.py Outdated Show resolved Hide resolved
samtranslator/model/apigateway.py Outdated Show resolved Hide resolved
samtranslator/model/apigateway.py Outdated Show resolved Hide resolved
samtranslator/model/api/api_generator.py Show resolved Hide resolved
samtranslator/model/api/api_generator.py Show resolved Hide resolved
samtranslator/model/apigateway.py Outdated Show resolved Hide resolved
samtranslator/model/apigateway.py Show resolved Hide resolved
samtranslator/plugins/api/implicit_api_plugin.py Outdated Show resolved Hide resolved
none_idx = -1
authorizer_security = []

# If this is the Api-level DefaultAuthorizer we need to check for an
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This implementation isn't ideal. The existing order of operations is:

  1. Apply all Function Event configuration
  2. Apply API configuration

This makes it hard to apply a DefaultAuthorizer, especially when 'NONE' is defined on the Function Event. Current logic is to add a 'NONE' entry to the Swagger and then remove it when DefaultAuthorizer comes through. Similarly, DefaultAuthorizer won't be added if it detects an existing entry in security which matches a key defined in Api.Auth.Authorizers.

Ideally we would add DefaultAuthorizer first and then remove/override as needed, but I'd rather not complicate the order of operations (unless there's already precedent of that which I'm not aware of).

samtranslator/model/eventsources/push.py Outdated Show resolved Hide resolved
@brettstack brettstack mentioned this pull request Aug 9, 2018
@hoegertn
Copy link

hoegertn commented Sep 6, 2018

Is there anything new on this topic? Any ETA for authorizers?

@brettstack
Copy link
Contributor Author

Currently working on the Cognito example, followed directly by tests. Then there's just a couple of documentation changes and we can merge this in.

@hoegertn
Copy link

hoegertn commented Sep 6, 2018

So do you have an idea when this will be available in production?

This commit also includes a fix for Authorizers when using the ANY method as well as a fix for InlineCode (which this example uses) when used in combination with aws cloudformation package
@brettstack
Copy link
Contributor Author

As part of our policy, I can't comment on dates. I can say it will be "soon" after it is merged into develop

@brettstack brettstack closed this Sep 10, 2018
@brettstack brettstack reopened this Sep 10, 2018
examples/2016-10-31/api_cognito_auth/src/package.json Outdated Show resolved Hide resolved
versions/2016-10-31.md Outdated Show resolved Hide resolved
versions/2016-10-31.md Outdated Show resolved Hide resolved
samtranslator/translator/arn_generator.py Show resolved Hide resolved
samtranslator/model/eventsources/push.py Show resolved Hide resolved
samtranslator/model/api/api_generator.py Show resolved Hide resolved
Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@ombozek ombozek left a comment

Choose a reason for hiding this comment

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

Left a few comments - not going to block on them.

@brettstack brettstack merged commit c4f3f87 into aws:develop Sep 21, 2018
@ajhool
Copy link

ajhool commented Oct 3, 2018

Very excited for this feature. I'm not entirely sure where translation from SAM to CloudFormation occurs (whether it is sam-cli, client-side, or behind AWS's walls). Once this PR is merged into master, will it be immediately ready to use?

This is my package and deploy script, which is called from CodePipeline/CodeBuild:

aws cloudformation package --template-file ./template.yaml --output-template-file ./build/sam-output.yaml --s3-bucket some-bucket`
sam deploy --template-file ./sam-output.yaml --stack-name some-stack --region us-east-1 --capabilities CAPABILITY_IAM

@keetonian
Copy link
Contributor

It will be available once it is merged into Master. It's possible to use this on the client-side, but the aws managed transform service happens inside our "walls". We'll post an announcement on our #samdev slack channel as well as in other places when this feature is available from the the internal transform.

@keetonian
Copy link
Contributor

This feature has now been released and is available to use

@OndeVai
Copy link

OndeVai commented Nov 27, 2018

@brettstack @keetonian The example template at https://github.com/awslabs/serverless-application-model/blob/master/examples/2016-10-31/api_cognito_auth/template.yaml fails SAM validation via "sam validate -t [filename]".

Error: [InvalidResourceException('MyApi', "Cors works only with inline Swagger specified in 'DefinitionBody' property")] ('MyApi', "Cors works only with inline Swagger specified in 'DefinitionBody' property")

@keetonian
Copy link
Contributor

@OndeVai Which version of SAM CLI are you using? You may need to upgrade your version to make this work.

@OndeVai
Copy link

OndeVai commented Nov 27, 2018

@keetonian I'm on version 0.7.0

@keetonian
Copy link
Contributor

keetonian commented Nov 27, 2018

Ok I looked into it- sam validate doesn't yet support the new auth feature, and errors when neither DefinitionBody nor DefinitionUri is set because it expects one of them to be there (as found here).

I opened an issue on the CLI repo: aws/aws-sam-cli#803

@OndeVai
Copy link

OndeVai commented Nov 28, 2018

Good deal. Thanks. Good news is that the sam deploy works like a champ :)

@OndeVai
Copy link

OndeVai commented Nov 28, 2018

@keetonian @brettstack Sorry, one more issue:
Setting a default authorizer, as in the sample breaks CORS, because it also assigns the authorizer to the options verb for all routes. This seems like an issue because we would only expect that for get,post,patch,put,delete.

MyApi: Type: AWS::Serverless::Api Properties: StageName: Prod Cors: "'*'" Auth: DefaultAuthorizer: MyCognitoAuthorizer #adds to options verb and breaks cors Authorizers: MyCognitoAuthorizer: UserPoolArn: !GetAtt MyCognitoUserPool.Arn

@keetonian
Copy link
Contributor

@OndeVai Could you create a new issue for this use case, detailing how you would like it to work so that we (and others) can more easily track the design and development of this feature?

@OndeVai
Copy link

OndeVai commented Nov 29, 2018

Will do. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants