handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor (#2263)

* handle 'Invalid Swagger Document' and refactor some validation into Swagger Editor constructor

* missed removing a comment

* move path item validation into method

* update new function to use swagger editor instance

* small change, changing test to use swagger instead of openapi

* update deployment hashes in some updated tests

* add docstring :raises

Author: torresxb1

samtranslator/model/api/api_generator.py

@@ -236,6 +236,8 @@ def __init__(
         self.template_conditions = template_conditions
         self.mode = mode
 
+        self.swagger_editor = SwaggerEditor(self.definition_body) if self.definition_body else None
+
     def _construct_rest_api(self):
         """Constructs and returns the ApiGateway RestApi.
 
@@ -282,7 +284,7 @@ def _construct_rest_api(self):
             rest_api.BodyS3Location = self._construct_body_s3_dict()
         elif self.definition_body:
             # # Post Process OpenApi Auth Settings
-            self.definition_body = self._openapi_postprocess(self.definition_body)
+            self.definition_body = self._openapi_postprocess(self.swagger_editor.swagger)
             rest_api.Body = self.definition_body
 
         if self.name:
@@ -308,9 +310,7 @@ def _add_endpoint_extension(self):
             raise InvalidResourceException(
                 self.logical_id, "DisableExecuteApiEndpoint works only within 'DefinitionBody' property."
             )
-        editor = SwaggerEditor(self.definition_body)
-        editor.add_disable_execute_api_endpoint_extension(self.disable_execute_api_endpoint)
-        self.definition_body = editor.swagger
+        self.swagger_editor.add_disable_execute_api_endpoint_extension(self.disable_execute_api_endpoint)
 
     def _construct_body_s3_dict(self):
         """Constructs the RestApi's `BodyS3Location property`_, from the SAM Api's DefinitionUri property.
@@ -620,13 +620,6 @@ def _add_cors(self):
         else:
             raise InvalidResourceException(self.logical_id, INVALID_ERROR)
 
-        if not SwaggerEditor.is_valid(self.definition_body):
-            raise InvalidResourceException(
-                self.logical_id,
-                "Unable to add Cors configuration because "
-                "'DefinitionBody' does not contain a valid Swagger definition.",
-            )
-
         if properties.AllowCredentials is True and properties.AllowOrigin == _CORS_WILDCARD:
             raise InvalidResourceException(
                 self.logical_id,
@@ -635,10 +628,9 @@ def _add_cors(self):
                 "'AllowOrigin' is \"'*'\" or not set",
             )
 
-        editor = SwaggerEditor(self.definition_body)
-        for path in editor.iter_on_path():
+        for path in self.swagger_editor.iter_on_path():
             try:
-                editor.add_cors(
+                self.swagger_editor.add_cors(
                     path,
                     properties.AllowOrigin,
                     properties.AllowHeaders,
@@ -649,9 +641,6 @@ def _add_cors(self):
             except InvalidTemplateException as ex:
                 raise InvalidResourceException(self.logical_id, ex.message)
 
-        # Assign the Swagger back to template
-        self.definition_body = editor.swagger
-
     def _add_binary_media_types(self):
         """
         Add binary media types to Swagger
@@ -664,11 +653,7 @@ def _add_binary_media_types(self):
         if self.binary_media and not self.definition_body:
             return
 
-        editor = SwaggerEditor(self.definition_body)
-        editor.add_binary_media_types(self.binary_media)
-
-        # Assign the Swagger back to template
-        self.definition_body = editor.swagger
+        self.swagger_editor.add_binary_media_types(self.binary_media)
 
     def _add_auth(self):
         """
@@ -687,40 +672,31 @@ def _add_auth(self):
         if not all(key in AuthProperties._fields for key in self.auth.keys()):
             raise InvalidResourceException(self.logical_id, "Invalid value for 'Auth' property")
 
-        if not SwaggerEditor.is_valid(self.definition_body):
-            raise InvalidResourceException(
-                self.logical_id,
-                "Unable to add Auth configuration because "
-                "'DefinitionBody' does not contain a valid Swagger definition.",
-            )
-        swagger_editor = SwaggerEditor(self.definition_body)
         auth_properties = AuthProperties(**self.auth)
         authorizers = self._get_authorizers(auth_properties.Authorizers, auth_properties.DefaultAuthorizer)
 
         if authorizers:
-            swagger_editor.add_authorizers_security_definitions(authorizers)
+            self.swagger_editor.add_authorizers_security_definitions(authorizers)
             self._set_default_authorizer(
-                swagger_editor,
+                self.swagger_editor,
                 authorizers,
                 auth_properties.DefaultAuthorizer,
                 auth_properties.AddDefaultAuthorizerToCorsPreflight,
                 auth_properties.Authorizers,
             )
 
         if auth_properties.ApiKeyRequired:
-            swagger_editor.add_apikey_security_definition()
-            self._set_default_apikey_required(swagger_editor)
+            self.swagger_editor.add_apikey_security_definition()
+            self._set_default_apikey_required(self.swagger_editor)
 
         if auth_properties.ResourcePolicy:
             SwaggerEditor.validate_is_dict(
                 auth_properties.ResourcePolicy, "ResourcePolicy must be a map (ResourcePolicyStatement)."
             )
-            for path in swagger_editor.iter_on_path():
-                swagger_editor.add_resource_policy(auth_properties.ResourcePolicy, path, self.stage_name)
+            for path in self.swagger_editor.iter_on_path():
+                self.swagger_editor.add_resource_policy(auth_properties.ResourcePolicy, path, self.stage_name)
             if auth_properties.ResourcePolicy.get("CustomStatements"):
-                swagger_editor.add_custom_statements(auth_properties.ResourcePolicy.get("CustomStatements"))
-
-        self.definition_body = self._openapi_postprocess(swagger_editor.swagger)
+                self.swagger_editor.add_custom_statements(auth_properties.ResourcePolicy.get("CustomStatements"))
 
     def _construct_usage_plan(self, rest_api_stage=None):
         """Constructs and returns the ApiGateway UsagePlan, ApiGateway UsagePlanKey, ApiGateway ApiKey for Auth.
@@ -923,15 +899,6 @@ def _add_gateway_responses(self):
                         ),
                     )
 
-        if not SwaggerEditor.is_valid(self.definition_body):
-            raise InvalidResourceException(
-                self.logical_id,
-                "Unable to add Auth configuration because "
-                "'DefinitionBody' does not contain a valid Swagger definition.",
-            )
-
-        swagger_editor = SwaggerEditor(self.definition_body)
-
         # The dicts below will eventually become part of swagger/openapi definition, thus requires using Py27Dict()
         gateway_responses = Py27Dict()
         for response_type, response in self.gateway_responses.items():
@@ -943,10 +910,7 @@ def _add_gateway_responses(self):
             )
 
         if gateway_responses:
-            swagger_editor.add_gateway_responses(gateway_responses)
-
-        # Assign the Swagger back to template
-        self.definition_body = swagger_editor.swagger
+            self.swagger_editor.add_gateway_responses(gateway_responses)
 
     def _add_models(self):
         """
@@ -962,22 +926,10 @@ def _add_models(self):
                 self.logical_id, "Models works only with inline Swagger specified in " "'DefinitionBody' property."
             )
 
-        if not SwaggerEditor.is_valid(self.definition_body):
-            raise InvalidResourceException(
-                self.logical_id,
-                "Unable to add Models definitions because "
-                "'DefinitionBody' does not contain a valid Swagger definition.",
-            )
-
         if not all(isinstance(model, dict) for model in self.models.values()):
             raise InvalidResourceException(self.logical_id, "Invalid value for 'Models' property")
 
-        swagger_editor = SwaggerEditor(self.definition_body)
-        swagger_editor.add_models(self.models)
-
-        # Assign the Swagger back to template
-
-        self.definition_body = self._openapi_postprocess(swagger_editor.swagger)
+        self.swagger_editor.add_models(self.models)
 
     def _openapi_postprocess(self, definition_body):
         """

samtranslator/swagger/swagger.py

@@ -46,19 +46,13 @@ def __init__(self, doc):
         modifications on this copy.
 
         :param dict doc: Swagger document as a dictionary
-        :raises ValueError: If the input Swagger document does not meet the basic Swagger requirements.
+        :raises InvalidDocumentException if doc is invalid
         """
 
-        if not SwaggerEditor.is_valid(doc):
-            raise ValueError("Invalid Swagger document")
-
         self._doc = copy.deepcopy(doc)
-        self.paths = self._doc["paths"]
-        self.security_definitions = self._doc.get("securityDefinitions", Py27Dict())
-        self.gateway_responses = self._doc.get(self._X_APIGW_GATEWAY_RESPONSES, Py27Dict())
-        self.resource_policy = self._doc.get(self._X_APIGW_POLICY, Py27Dict())
-        self.definitions = self._doc.get("definitions", Py27Dict())
+        self.validate_definition_body(doc)
 
+        self.paths = self._doc.get("paths")
         # https://swagger.io/specification/#path-item-object
         # According to swagger spec,
         # each path item object must be a dict (even it is empty).
@@ -67,6 +61,11 @@ def __init__(self, doc):
         for path in self.iter_on_path():
             SwaggerEditor.validate_path_item_is_dict(self.get_path(path), path)
 
+        self.security_definitions = self._doc.get("securityDefinitions", Py27Dict())
+        self.gateway_responses = self._doc.get(self._X_APIGW_GATEWAY_RESPONSES, Py27Dict())
+        self.resource_policy = self._doc.get(self._X_APIGW_POLICY, Py27Dict())
+        self.definitions = self._doc.get("definitions", Py27Dict())
+
     def get_path(self, path):
         path_dict = self.paths.get(path)
         if isinstance(path_dict, dict) and self._CONDITIONAL_IF in path_dict:
@@ -162,7 +161,6 @@ def add_path(self, path, method=None):
 
         :param string path: Path name
         :param string method: HTTP method
-        :raises ValueError: If the value of `path` in Swagger is not a dictionary
         """
         method = self._normalize_method_name(method)
 
@@ -1310,6 +1308,32 @@ def is_valid(data):
                 )
         return False
 
+    def validate_definition_body(self, definition_body):
+        """
+        Checks if definition_body is a valid Swagger document
+
+        :param dict definition_body: Data to be validated
+        :raises InvalidDocumentException if definition_body is invalid
+        """
+
+        SwaggerEditor.validate_is_dict(definition_body, "DefinitionBody must be a dictionary.")
+        SwaggerEditor.validate_is_dict(
+            definition_body.get("paths"), "The 'paths' property of DefinitionBody must be present and be a dictionary."
+        )
+
+        has_swagger = definition_body.get("swagger")
+        has_openapi3 = definition_body.get("openapi") and SwaggerEditor.safe_compare_regex_with_string(
+            SwaggerEditor.get_openapi_version_3_regex(), definition_body["openapi"]
+        )
+        if not (has_swagger) and not (has_openapi3):
+            raise InvalidDocumentException(
+                [
+                    InvalidTemplateException(
+                        "DefinitionBody must have either: (1) a 'swagger' property or (2) an 'openapi' property with version 3.x or 3.x.x"
+                    )
+                ]
+            )
+
     @staticmethod
     def validate_is_dict(obj, exception_message):
         """

tests/model/api/test_api_generator.py

@@ -18,7 +18,7 @@ def test_construct_usage_plan_with_invalid_usage_plan_type(self, invalid_usage_p
             Mock(),
             Mock(),
             Mock(),
-            Mock(),
+            {"paths": {}, "openapi": "3.0.1"},
             Mock(),
             Mock(),
             Mock(),
@@ -39,7 +39,7 @@ def test_construct_usage_plan_with_invalid_usage_plan_fields(self, AuthPropertie
             Mock(),
             Mock(),
             Mock(),
-            Mock(),
+            {"paths": {}, "openapi": "3.0.1"},
             Mock(),
             Mock(),
             Mock(),

tests/swagger/test_swagger.py

@@ -18,7 +18,7 @@ class TestSwaggerEditor_init(TestCase):
     def test_must_raise_on_invalid_swagger(self):
 
         invalid_swagger = {"paths": {}}  # Missing "Swagger" keyword
-        with self.assertRaises(ValueError):
+        with self.assertRaises(InvalidDocumentException):
             SwaggerEditor(invalid_swagger)
 
     def test_must_succeed_on_valid_swagger(self):
@@ -32,13 +32,13 @@ def test_must_succeed_on_valid_swagger(self):
     def test_must_fail_on_invalid_openapi_version(self):
         invalid_swagger = {"openapi": "2.3.0", "paths": {"/foo": {}, "/bar": {}}}
 
-        with self.assertRaises(ValueError):
+        with self.assertRaises(InvalidDocumentException):
             SwaggerEditor(invalid_swagger)
 
     def test_must_fail_on_invalid_openapi_version_2(self):
         invalid_swagger = {"openapi": "3.1.1.1", "paths": {"/foo": {}, "/bar": {}}}
 
-        with self.assertRaises(ValueError):
+        with self.assertRaises(InvalidDocumentException):
             SwaggerEditor(invalid_swagger)
 
     def test_must_succeed_on_valid_openapi3(self):
@@ -53,7 +53,7 @@ def test_must_succeed_on_valid_openapi3(self):
     def test_must_fail_with_bad_values_for_path(self, invalid_path_item):
         invalid_swagger = {"openapi": "3.1.1.1", "paths": {"/foo": {}, "/bad": invalid_path_item}}
 
-        with self.assertRaises(ValueError):
+        with self.assertRaises(InvalidDocumentException):
             SwaggerEditor(invalid_swagger)
 
 

tests/translator/input/api_with_resource_refs.yaml

@@ -6,8 +6,8 @@ Resources:
     Properties:
       StageName: foo
       DefinitionBody:
-        "this": "is"
-        "a": "swagger"
+        paths: {}
+        openapi: "3.0.1"
 
   MyFunction:
     Type: "AWS::Serverless::Function"

tests/translator/input/error_api_definition_body_invalid_openapi_version.yaml

@@ -0,0 +1,13 @@
+Resources:
+  MyApi:
+    Type: AWS::Serverless::Api
+    Properties:
+      StageName: Prod
+      DefinitionBody:
+        info:
+          version: '1.0'
+          title: 'title'
+        paths:
+          "/some/path": {}
+          "/other": {}
+        openapi: '2.0'

tests/translator/input/error_api_definition_body_invalid_paths.yaml

@@ -0,0 +1,11 @@
+Resources:
+  MyApi:
+    Type: AWS::Serverless::Api
+    Properties:
+      StageName: Prod
+      DefinitionBody:
+        info:
+          version: '1.0'
+          title: 'title'
+        openapi: 3.0.1
+        paths: 'invalid'

tests/translator/input/error_api_definition_body_missing_openapi_or_swagger.yaml

@@ -0,0 +1,12 @@
+Resources:
+  MyApi:
+    Type: AWS::Serverless::Api
+    Properties:
+      StageName: Prod
+      DefinitionBody:
+        info:
+          version: '1.0'
+          title: 'title'
+        paths:
+          "/some/path": {}
+          "/other": {}

tests/translator/input/error_api_definition_body_missing_paths.yaml

@@ -0,0 +1,10 @@
+Resources:
+  MyApi:
+    Type: AWS::Serverless::Api
+    Properties:
+      StageName: Prod
+      DefinitionBody:
+        info:
+          version: '1.0'
+          title: 'title'
+        openapi: 3.0.1

tests/translator/input/error_api_invalid_auth.yaml

@@ -121,14 +121,6 @@ Resources:
       Auth:
         MyBad: Foo
 
-  AuthWithInvalidDefinitionBodyApi:
-    Type: AWS::Serverless::Api
-    Properties:
-      StageName: Prod
-      DefinitionBody: { invalid: true }
-      Auth:
-        DefaultAuthorizer: Foo
-
   AuthWithMissingDefaultAuthorizerApi:
     Type: AWS::Serverless::Api
     Properties: