Add ApiKey Auth support #943

cfbarbero · 2019-05-31T20:50:34Z

Issue #, if available:
Fixes #867

Description of changes:
Update the Auth property to include a new sub-property ApiKeyRequired: <boolean>. This will be supported on both the Api Auth Method and the Function Auth Method. The function level specification will override the API level.

For example:

AWSTemplateFormatVersion: '2010-09-09'
Transform: AWS::Serverless-2016-10-31
Description: API Gateway with ApiKey Auth
Resources:
  MyApi:
    Type: AWS::Serverless::Api
    Properties:
      StageName: Prod
      Auth:
        ApiKeyRequired: true

  MyFunction:
    Type: AWS::Serverless::Function
    Properties:
      CodeUri: .
      Handler: index.handler
      Runtime: nodejs8.10
      Events:
        ApiKeyFalse:
          Type: Api
          Properties:
            RestApiId: !Ref MyApi
            Path: /
            Method: get
            # This is a public endpoint
            Auth:
              ApiKeyRequired: false
        ApiKeyTrue:
          Type: Api
          Properties:
            RestApiId: !Ref MyApi
            Path: /
            Method: get
            Auth:
              ApiKeyRequired: true
        ApiKeyDefault:
          Type: Api
          Properties:
            RestApiId: !Ref MyApi
            Path: /other
            Method: get

This is roughly based upon #444. It implements part of #547.

In implementing this change, I refactored the support for the Authorizers in the swagger.py file. ~~By doing this, it changed the order in which some of the generated swagger properties are translated and there by required updates to the following translator test output files:~~

~~tests/translator/output/api_with_aws_iam_auth_overrides.json~~
~~tests/translator/output/api_with_default_aws_iam_auth.json~~

Description of how you validated changes:
Added additional translator tests:

tests/translator/input/api_with_apikey_default_override.yaml
tests/translator/input/api_with_apikey_required.yaml
Updated existing translator tests:
tests/translator/input/globals_for_api.yaml
tests/translator/input/implicit_api_with_auth_and_conditions_max.yaml

Checklist:

Write/update tests
make pr passes
Update documentation

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

brettstack · 2019-06-04T17:58:31Z

it changed the order in which some of the generated swagger properties

Please try to maintain the Swagger integrity as SAM will create an APIGW Deployment when it detects Swagger has changed and we strongly try not to do an APIGW Deployment unless the user actually made a change to their API.

cfbarbero · 2019-06-05T14:36:23Z

@brettstack
I have fixed the bug that caused the AWS_IAM security definition to get reordered. I have reverted the test output files for the 2 tests (across all 3 regions) that I had changed to accomodate the updated ordering and they are passing.

Let me know if there's anything else you'd like me to address.

codecov-io · 2019-06-05T14:43:05Z

Codecov Report

Merging #943 into develop will increase coverage by 0.08%.
The diff coverage is 91.66%.

@@            Coverage Diff             @@
##           develop    #943      +/-   ##
==========================================
+ Coverage    94.71%   94.8%   +0.09%     
==========================================
  Files           69      69              
  Lines         3160    3234      +74     
  Branches       606     629      +23     
==========================================
+ Hits          2993    3066      +73     
- Misses          85      87       +2     
+ Partials        82      81       -1

Impacted Files	Coverage Δ
samtranslator/model/api/api_generator.py	`94.85% <100%> (+1.26%)`	⬆️
samtranslator/model/eventsources/push.py	`88.38% <50%> (-0.47%)`	⬇️
samtranslator/swagger/swagger.py	`97.49% <93.26%> (-0.18%)`	⬇️

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 88df20c...36cf808. Read the comment docs.

praneetap · 2019-06-07T17:22:40Z

samtranslator/model/eventsources/push.py


+            apikey_required_setting = self.Auth.get('ApiKeyRequired')
+            apikey_required_setting_is_false = apikey_required_setting is not None and not apikey_required_setting
+            if apikey_required_setting_is_false and not api_auth.get('ApiKeyRequired'):


Throwing an exception here is not required, as this would be a no-op on api gw.

Like the Authorizers implementation, I am storing the explicit ApiKeyRequired: False for a function in the security block for that function as api_key_false.

"security": { "api_key_false": [] }

This security option gets cleaned up by the swagger.set_path_default_apikey_required method, however, this is only called when there is a default ApiKeyRequired setting on the Serverless API. If there is no default setting (or no explicit Serverless API) then we never loop back and cleanup the api_key_false security setting.

I assumed we didn't want to spit out this security setting, however, I'll defer to your judgement. I'm also open to other ideas on this.

praneetap · 2019-06-07T17:33:34Z

samtranslator/model/api/api_generator.py

+            self._set_default_apikey_required(swagger_editor)
+
        # Assign the Swagger back to template
        self.definition_body = swagger_editor.swagger


Could you please rebase and add/update a test to ensure this works with openapi 3 as well.

@praneetap , I have rebased. I am happy to add a test for openapi 3. Can you provide a little direction on that? Are there other tests I can refer to as an example?

@praneetap , I've added a couple of openapi3 translator tests. Please let me know if there's anything else you'd like to see on this.

New Test:

api_with_apikey_required_openapi_3

Updated Tests:

api_with_auth_all_maximum_openapi_3

api_with_auth_all_maximum

Yes, will be happy to! OpenApi tests are run here.
You can find more examples of input templates here if you search for "openapi".

- Implement new translator tests - Add swagger.py unit tests - Update documentation - refactor Auth handling in swagger.py

…ed just at a Function level, we need to ensure the OpenApi Auth post processing happens even if the API has no explicit Auth Settings

cfbarbero · 2019-06-21T19:46:54Z

@praneetap I have addressed your PR comments, this is ready for review again.

cfbarbero · 2019-06-21T19:47:28Z

samtranslator/model/api/api_generator.py

            rest_api.BodyS3Location = self._construct_body_s3_dict()
        elif self.definition_body:
+            # # Post Process OpenApi Auth Settings
+            self.definition_body = self._openapi_auth_postprocess(self.definition_body)


@praneetap In order to support the ApiKeyRequired setting at the Function level (without requiring an explicit setting on the API), I had to move the _openapi_auth_postprocess method call up a level to ensure it was called on every template reqardless of if there's an explicit API Auth setting. If you have another suggestion I'm happy to change.

emonhaider · 2019-08-26T19:59:56Z

Is there an ETA when these changes are getting merged to master?

jlhood · 2019-08-28T21:38:28Z

@emonhaider We are working on the v1.14.0 release now. We can't give exact dates, but it's coming soon.

cfbarbero force-pushed the apikey-refactor branch from 0a08669 to a751d75 Compare May 31, 2019 21:13

cfbarbero force-pushed the apikey-refactor branch 2 times, most recently from 8d692a1 to 0ff2b41 Compare June 5, 2019 14:35

praneetap reviewed Jun 7, 2019

View reviewed changes

cfbarbero added 3 commits June 20, 2019 09:09

Initial work to add ApiKeyRequired support

beef4cf

- Implement new translator tests - Add swagger.py unit tests - Update documentation - refactor Auth handling in swagger.py

Fix spacing

1b03e59

undo space changes

f8ed469

cfbarbero force-pushed the apikey-refactor branch from 0ff2b41 to f8ed469 Compare June 20, 2019 19:42

cfbarbero added 4 commits June 20, 2019 16:34

Add openapi3 tests

ed7b7a1

Fix linting errors

1875fb3

Update apikey openapi3 tests with expected output.

57c1efe

Move OpenApi Auth processing- Since the ApiKey setting can be specifi…

36cf808

…ed just at a Function level, we need to ensure the OpenApi Auth post processing happens even if the API has no explicit Auth Settings

cfbarbero force-pushed the apikey-refactor branch from 922b0dc to 36cf808 Compare June 21, 2019 17:29

cfbarbero commented Jun 21, 2019

View reviewed changes

praneetap approved these changes Jul 2, 2019

View reviewed changes

brettstack approved these changes Jul 3, 2019

View reviewed changes

brettstack merged commit 0d12466 into aws:develop Jul 3, 2019

cfbarbero deleted the apikey-refactor branch July 8, 2019 17:02

Uh oh!

Add ApiKey Auth support #943

Add ApiKey Auth support #943

Uh oh!

Conversation

cfbarbero commented May 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brettstack commented Jun 4, 2019

Uh oh!

cfbarbero commented Jun 5, 2019

Uh oh!

codecov-io commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

praneetap Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

cfbarbero Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

praneetap Jun 7, 2019

Choose a reason for hiding this comment

Uh oh!

cfbarbero Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

cfbarbero Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

praneetap Jun 20, 2019

Choose a reason for hiding this comment

Uh oh!

cfbarbero commented Jun 21, 2019

Uh oh!

cfbarbero Jun 21, 2019

Choose a reason for hiding this comment

Uh oh!

emonhaider commented Aug 26, 2019

Uh oh!

jlhood commented Aug 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

cfbarbero commented May 31, 2019 •

edited

Loading

codecov-io commented Jun 5, 2019 •

edited

Loading