Skip to content

Add ApiKey Auth support #943

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.

Sign up for GitHub

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

Merged
merged 7 commits into from
Jul 3, 2019
Merged

Add ApiKey Auth support #943

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
beef4cf
Initial work to add ApiKeyRequired support
cfbarbero May 24, 2019
1b03e59
Fix spacing
cfbarbero Jun 5, 2019
f8ed469
undo space changes
cfbarbero Jun 20, 2019
ed7b7a1
Add openapi3 tests
cfbarbero Jun 20, 2019
1875fb3
Fix linting errors
cfbarbero Jun 21, 2019
57c1efe
Update apikey openapi3 tests with expected output.
cfbarbero Jun 21, 2019
36cf808
Move OpenApi Auth processing- Since the ApiKey setting can be specifi…
cfbarbero Jun 21, 2019
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
21 changes: 16 additions & 5 deletions samtranslator/model/api/api_generator.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,9 @@
CorsProperties.__new__.__defaults__ = (None, None, _CORS_WILDCARD, None, False)

AuthProperties = namedtuple("_AuthProperties",
["Authorizers", "DefaultAuthorizer", "InvokeRole", "AddDefaultAuthorizerToCorsPreflight"])
AuthProperties.__new__.__defaults__ = (None, None, None, True)
["Authorizers", "DefaultAuthorizer", "InvokeRole", "AddDefaultAuthorizerToCorsPreflight",
"ApiKeyRequired"])
AuthProperties.__new__.__defaults__ = (None, None, None, True, None)

GatewayResponseProperties = ["ResponseParameters", "ResponseTemplates", "StatusCode"]

Expand Down Expand Up @@ -115,6 +116,8 @@ def _construct_rest_api(self):
if self.definition_uri:
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)
Copy link
Contributor Author

@cfbarbero cfbarbero Jun 21, 2019

Choose a reason for hiding this comment

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

@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.

rest_api.Body = self.definition_body

if self.name:
Expand Down Expand Up @@ -308,13 +311,17 @@ def _add_auth(self):
authorizers = self._get_authorizers(auth_properties.Authorizers, auth_properties.DefaultAuthorizer)

if authorizers:
swagger_editor.add_authorizers(authorizers)
swagger_editor.add_authorizers_security_definitions(authorizers)
self._set_default_authorizer(swagger_editor, authorizers, auth_properties.DefaultAuthorizer,
auth_properties.AddDefaultAuthorizerToCorsPreflight)

# Assign the Swagger back to template
if auth_properties.ApiKeyRequired:
swagger_editor.add_apikey_security_definition()
self._set_default_apikey_required(swagger_editor)

self.definition_body = self._openapi_auth_postprocess(swagger_editor.swagger)
# Assign the Swagger back to template
# self.definition_body = self._openapi_auth_postprocess(swagger_editor.swagger)
self.definition_body = swagger_editor.swagger
Copy link
Contributor

@praneetap praneetap Jun 7, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cfbarbero cfbarbero Jun 20, 2019

Choose a reason for hiding this comment

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

@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?

Copy link
Contributor Author

@cfbarbero cfbarbero Jun 20, 2019

Choose a reason for hiding this comment

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

@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

Copy link
Contributor

@praneetap praneetap Jun 20, 2019

Choose a reason for hiding this comment

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

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


def _openapi_auth_postprocess(self, definition_body):
"""
Expand Down Expand Up @@ -523,6 +530,10 @@ def _set_default_authorizer(self, swagger_editor, authorizers, default_authorize
swagger_editor.set_path_default_authorizer(path, default_authorizer, authorizers=authorizers,
add_default_auth_to_preflight=add_default_auth_to_preflight)

def _set_default_apikey_required(self, swagger_editor):
for path in swagger_editor.iter_on_path():
swagger_editor.set_path_default_apikey_required(path)

def _set_endpoint_configuration(self, rest_api, value):
"""
Sets endpoint configuration property of AWS::ApiGateway::RestApi resource
Expand Down
18 changes: 14 additions & 4 deletions samtranslator/model/eventsources/push.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -123,9 +123,9 @@ class CloudWatchEvent(PushEventSource):
resource_type = 'CloudWatchEvent'
principal = 'events.amazonaws.com'
property_types = {
'Pattern': PropertyType(False, is_type(dict)),
'Input': PropertyType(False, is_str()),
'InputPath': PropertyType(False, is_str())
'Pattern': PropertyType(False, is_type(dict)),
'Input': PropertyType(False, is_str()),
'InputPath': PropertyType(False, is_str())
}

def to_cloudformation(self, **kwargs):
Expand Down Expand Up @@ -536,9 +536,9 @@ def _add_swagger_integration(self, api, function):

if self.Auth:
method_authorizer = self.Auth.get('Authorizer')
api_auth = api.get('Auth')

if method_authorizer:
api_auth = api.get('Auth')
api_authorizers = api_auth and api_auth.get('Authorizers')

if method_authorizer != 'AWS_IAM':
Expand All @@ -563,6 +563,16 @@ def _add_swagger_integration(self, api, function):
'is only a valid value when a DefaultAuthorizer on the API is specified.'.format(
method=self.Method, path=self.Path))

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'):
Copy link
Contributor

@praneetap praneetap Jun 7, 2019

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@cfbarbero cfbarbero Jun 20, 2019

Choose a reason for hiding this comment

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

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.

raise InvalidEventException(
self.relative_id,
'Unable to set ApiKeyRequired [False] on API method [{method}] for path [{path}] '
'because the related API does not specify any ApiKeyRequired.'.format(
method=self.Method, path=self.Path))

if method_authorizer or apikey_required_setting is not None:
editor.add_auth_to_method(api=api, path=self.Path, method_name=self.Method, auth=self.Auth)

if self.RequestModel:
Expand Down
Loading