Add authorizationscopes #917

klmz · 2019-05-14T06:41:06Z

Issue #, if available:
#652

Description of changes:
This adds support for authorisation scopes to the SAM spec. It is a small hanges to the swagger translator that add authorization scopes per method.

Description of how you validated changes:
I have added an e2e test in the from of an example yaml file (api_with_auth_with_scopes.yaml) with all usage options. I also added swagger specific tests to test_swagger.py.

Checklist:

Write/update tests
make pr passes
Update documentation
Add swagger test

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

klmz · 2019-05-14T08:33:18Z

Ok. I must admit that I don't know how. But I'm guessing it had something to do with linting. Once I made sure make pr passed the test worked on python 2 as well. Sorry for the noise, still have to write some test though. I will do this somewhere this week!

codecov-io · 2019-05-14T18:00:40Z

Codecov Report

Merging #917 into develop will increase coverage by 0.08%.
The diff coverage is 90%.

@@             Coverage Diff             @@
##           develop     #917      +/-   ##
===========================================
+ Coverage     94.5%   94.58%   +0.08%     
===========================================
  Files           72       78       +6     
  Lines         3746     4269     +523     
  Branches       742      850     +108     
===========================================
+ Hits          3540     4038     +498     
- Misses         104      111       +7     
- Partials       102      120      +18

Impacted Files	Coverage Δ
samtranslator/model/api/api_generator.py	`95.22% <ø> (ø)`	⬆️
samtranslator/model/eventsources/push.py	`90.51% <0%> (+0.67%)`	⬆️
samtranslator/model/apigateway.py	`97.9% <100%> (+0.01%)`	⬆️
samtranslator/swagger/swagger.py	`92.77% <100%> (+0.2%)`	⬆️
samtranslator/model/sam_resources.py	`95.08% <0%> (-0.57%)`	⬇️
samtranslator/plugins/api/implicit_api_plugin.py	`98.23% <0%> (-0.39%)`	⬇️
samtranslator/model/naming.py	`100% <0%> (ø)`	⬆️
samtranslator/sdk/resource.py	`100% <0%> (ø)`	⬆️
samtranslator/translator/verify_logical_id.py	`100% <0%> (ø)`	⬆️
...ator/plugins/api/default_definition_body_plugin.py	`100% <0%> (ø)`	⬆️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b56a09d...b8943c6. Read the comment docs.

fyayc-ale · 2019-05-21T08:03:12Z

@keetonian, sorry to ping you, would be possible to merge this? We are using a lambda to achieve the same and this will reduce errors on our side by a lot!

klmz · 2019-05-21T15:51:12Z

@keetonian, sorry to ping you, would be possible to merge this? We are using a lambda to achieve the same and this will reduce errors on our side by a lot!

Depends on how much you need this but you can pull this branch and run the transformation manually as described here: Dev guide

If you ever worked with python and virtualenv you can have it done in minutes.

keetonian · 2019-05-21T22:43:26Z

@fyayc-ale @klmz Will review today. Thank you for pinging me.

tests/translator/input/api_with_auth_with_scopes.yaml

brettstack · 2019-05-21T22:51:05Z

@klmz thanks! Looks like it's missing AuthorizationScopes at the Api level? This should set scopes on all methods which use this authorizer (and can be overridden at the method level)

klmz · 2019-05-22T06:59:10Z

@brettstack I didn't do that because I tried to stay as close to the cloudformation spec. A default scope doesn't make a whole lot of sense to me. If it is default you can just as well leave the scope out I guess? Although I guess there are use cases where it could make sense. If you want most endpoints to be with a scope and just a couple without scopes. Or if you use the same token with different api's or something like that? Would like to hear you thoughts on this.
Actually, would also like your thoughts on my assumption of staying close to the CF spec. Is that something you strive for? Maybe this is more of general discussion for somewhere else 😅.

That being said, I guess there is some use for it so I expect to have some time to add the api level scopes this week.

brettstack · 2019-05-22T13:43:37Z

Yes it's the scenario where you want most endpoints to have a certain configuration and you override the outliers.

eterjoost · 2019-05-28T07:17:51Z

Lol. That was much more complex than I anticipated. I think I got the behavior down, but I need to write tests still and then check if I can refactor it a bit to make it more clear. I expect to have some time to finish this on Thursday!

brettstack · 2019-05-28T17:53:55Z

Thanks @eterjoost we'll review when it's all ready. 👍

brettstack

Looking good! Could you submit an example or update the existing Cognito example?

tests/translator/input/error_api_with_auth_with_default_scopes.yaml

tests/translator/input/api_with_auth_with_default_scopes.yaml

klmz · 2019-06-11T05:33:43Z

I need some help with fixing the tests. Some tests are now failing, but it is very hard to see why in the output. Am I doing something wrong? Or is this how it works? Maybe there is a better way of using the tests instead of just running them all?

klmz · 2019-06-11T14:53:44Z

It is an issue with the china and us-gov region. The output doesn't match the expected output, but I can't see what is different.

keetonian · 2019-06-13T17:47:57Z

@klmz ok, we can look at it, thanks for bringing it up!

ghost · 2019-08-09T06:56:56Z

Hi All,

This is a hugely important pull request and is great work by @klmz. Having the ability to use scopes within a SAM template is the only way to implement access tokens in an enterprise scenario.

Can someone have a look, help @klmz to get to the end goal or provide some input to how we could fix this?

Thanks in advance,

Mike

leantorres73 · 2019-08-09T17:50:29Z

+1, This is a very important feature for implementing access_token and client_credentials grant type.

louism517 · 2019-08-13T14:03:25Z

+1 - the client_credentials flow actually requires a scope for authentication.

MohammedAlSafwanOld · 2019-09-03T16:48:55Z

Any progress on this issue?!

keetonian · 2019-09-10T20:58:28Z

@leantorres73 @louism517 I'm not familiar with those features, could you provide some more information about them?

@Michael1d I resolved the merge conflicts locally, but had a couple of questions regarding this feature and the overall scope. Could you give some more information about access tokens and how you use/would like to use them?

ghost · 2019-09-19T23:52:35Z

Hi @keetonian,

Apologies for not get back to you sooner. In my scenario, I have API's that have many potential "clients" - think an SPA, an analytics ETL process, a B2B customer etc. I have created Cognito Application Clients that represent those use cases, assigning a single scope to each one. Therefore, I have the requirement to add one or more scopes selectively to serverless functions deployed by SAM. I hope that makes sense.

On the standard CF AWS::ApiGateway::Method, there is a property for AuthorizationScopes and the same functionality is required here with a AWS::Serverless::Function in SAM. I also like the idea of being able to set default scopes on a AWS::Serverless::Api as discussed in here as well.

Please let me know if you have any other questions or clarifications.

keetonian · 2019-12-05T00:38:46Z

Just pushed up a few changes to fully support this feature.

praneetap · 2019-12-05T19:53:16Z

tests/translator/input/api_with_auth_with_default_scopes.yaml

+            Auth:
+              Authorizer: MyDefaultCognitoAuth
+              AuthorizationScopes: []
+        CognitoDefaultAuythDefaultScopesNone:


praneetap

Can you add a test with openapi to make sure the auth scopes get transformed into openapi correctly? Also try deploying it end to end.

keetonian · 2019-12-05T20:26:04Z

Scopes match the Swagger/OpenApi spec: https://swagger.io/docs/specification/authentication/

keetonian · 2019-12-05T20:28:24Z

No APIGW extensions for scopes, they use the same implementation: https://docs.aws.amazon.com/apigateway/latest/developerguide/api-gateway-swagger-extensions-authorizer.html

keetonian · 2019-12-06T01:37:14Z

Implementation/deployment verified for scopes changes.

klmz force-pushed the add-authorizationscopes branch from 117f266 to 274470e Compare May 14, 2019 07:44

klmz force-pushed the add-authorizationscopes branch from a72d3d7 to f02b74d Compare May 14, 2019 17:53

klmz marked this pull request as ready for review May 14, 2019 17:57

matthiasotto approved these changes May 21, 2019

View reviewed changes

brettstack reviewed May 21, 2019

View reviewed changes

tests/translator/input/api_with_auth_with_scopes.yaml Outdated Show resolved Hide resolved

brettstack suggested changes May 30, 2019

View reviewed changes

tests/translator/input/error_api_with_auth_with_default_scopes.yaml Outdated Show resolved Hide resolved

tests/translator/input/api_with_auth_with_default_scopes.yaml Outdated Show resolved Hide resolved

praneetap assigned keetonian Aug 16, 2019

MohammedAlSafwanOld mentioned this pull request Aug 20, 2019

Authorizers: Support AuthorizationScopes #652

Closed

Add support for Auth Scopes

0c0523e

keetonian force-pushed the add-authorizationscopes branch from 3d40bd8 to 0c0523e Compare December 5, 2019 00:37

keetonian assigned praneetap Dec 5, 2019

keetonian removed their assignment Dec 5, 2019

Add auth scopes to github docs

ec25005

keetonian changed the title ~~[WIP] Add authorizationscopes~~ Add authorizationscopes Dec 5, 2019

praneetap reviewed Dec 5, 2019

View reviewed changes

keetonian added 2 commits December 5, 2019 12:01

Fix typo

4e8153d

Update tests, add openapi 3.0.1 test

b8943c6

ShreyaGangishetty approved these changes Dec 5, 2019

View reviewed changes

praneetap approved these changes Dec 6, 2019

View reviewed changes

praneetap merged commit d50b959 into aws:develop Dec 6, 2019

ShreyaGangishetty mentioned this pull request Dec 10, 2019

V1.20.0 Release Tracker #1212

Closed

praneetap added stage/waiting-for-release labels Dec 13, 2019

jamesiri unassigned praneetap Aug 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add authorizationscopes #917

Add authorizationscopes #917

klmz commented May 14, 2019 •

edited

Loading

klmz commented May 14, 2019 •

edited

Loading

codecov-io commented May 14, 2019 •

edited

Loading

fyayc-ale commented May 21, 2019

klmz commented May 21, 2019 •

edited

Loading

keetonian commented May 21, 2019

brettstack commented May 21, 2019

klmz commented May 22, 2019

brettstack commented May 22, 2019

eterjoost commented May 28, 2019 •

edited

Loading

brettstack commented May 28, 2019

brettstack left a comment

klmz commented Jun 11, 2019

klmz commented Jun 11, 2019

keetonian commented Jun 13, 2019

ghost commented Aug 9, 2019 •

edited by ghost

Loading

leantorres73 commented Aug 9, 2019

louism517 commented Aug 13, 2019

MohammedAlSafwanOld commented Sep 3, 2019

keetonian commented Sep 10, 2019

ghost commented Sep 19, 2019

keetonian commented Dec 5, 2019

praneetap Dec 5, 2019

praneetap left a comment

keetonian commented Dec 5, 2019

keetonian commented Dec 5, 2019

keetonian commented Dec 6, 2019

Add authorizationscopes #917

Add authorizationscopes #917

Conversation

klmz commented May 14, 2019 • edited Loading

klmz commented May 14, 2019 • edited Loading

codecov-io commented May 14, 2019 • edited Loading

Codecov Report

fyayc-ale commented May 21, 2019

klmz commented May 21, 2019 • edited Loading

keetonian commented May 21, 2019

brettstack commented May 21, 2019

klmz commented May 22, 2019

brettstack commented May 22, 2019

eterjoost commented May 28, 2019 • edited Loading

brettstack commented May 28, 2019

brettstack left a comment

Choose a reason for hiding this comment

klmz commented Jun 11, 2019

klmz commented Jun 11, 2019

keetonian commented Jun 13, 2019

ghost commented Aug 9, 2019 • edited by ghost Loading

leantorres73 commented Aug 9, 2019

louism517 commented Aug 13, 2019

MohammedAlSafwanOld commented Sep 3, 2019

keetonian commented Sep 10, 2019

ghost commented Sep 19, 2019

keetonian commented Dec 5, 2019

praneetap Dec 5, 2019

Choose a reason for hiding this comment

praneetap left a comment

Choose a reason for hiding this comment

keetonian commented Dec 5, 2019

keetonian commented Dec 5, 2019

keetonian commented Dec 6, 2019

klmz commented May 14, 2019 •

edited

Loading

klmz commented May 14, 2019 •

edited

Loading

codecov-io commented May 14, 2019 •

edited

Loading

klmz commented May 21, 2019 •

edited

Loading

eterjoost commented May 28, 2019 •

edited

Loading

ghost commented Aug 9, 2019 •

edited by ghost

Loading