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

AWS API Gateway request body validation #5956

Merged
merged 9 commits into from Apr 23, 2019
Merged

AWS API Gateway request body validation #5956

merged 9 commits into from Apr 23, 2019

Conversation

@dschep
Copy link
Contributor

dschep commented Mar 26, 2019

What did you implement:

Added support for JSONSchema request body validation with API Gateway, Closing #3464. Reading the various docs for request validation(notably with CloudFormation), I couldn't quite understand how parameter or header validation would work. Any input regarding this implementation or how param/header validation would work is greatly appreciated.

Closes #3464

How did you implement it:

When you add a schema containing JSON schema to an HTTP event, the following are added:

  • AWS::ApiGateway::Model with the schema specified
  • AWS::ApiGateway::RequestValidator with ValidateRequestBody: true and attached to the AWS::ApiGateway::RestApi that serverless generates
  • attach the model above to the AWS::ApiGateway::Method for the event

How can we verify it:

  1. Install serverless from this branch: npm i -g serverless/serverless#sls-3464
  2. Create a serverless.yml containing this:
service: request-schema-test
provider:
  name: aws
  runtime: nodejs8.10
functions:
  hello:
    handler: handler.hello
    events:
      - http:
          path: users/create
          method: post
          request:
            schema:
              application/json: ${file(create_request.json)}
  1. Create a create_request.json containing the following json schema:
{
  "definitions": {},
  "$schema": "http://json-schema.org/draft-07/schema#",
  "type": "object",
  "title": "The Root Schema",
  "required": [
    "username"
  ],
  "properties": {
    "username": {
      "type": "string",
      "title": "The Foo Schema",
      "default": "",
      "pattern": "^[a-zA-Z0-9]+$"
    }
  }
}
  1. deploy! sls deploy
  2. Test a valid body:
$ curl -X POST https://xxxxxxxxxx.execute-api.us-east-1.amazonaws.com/dev/users/create -H 'Content-Type: applica
tion/json' --data-binary '{"username":"fobar"}'
{"message":"Go Serverless v1.0! Your function executed successfully!","input":{"resource":"/users/create","path":"/users/create","httpM
ethod":"POST","headers":{"Accept":"*/*","CloudFront-Forwarded-Proto":"https","CloudFront-Is-Desktop-Viewer":"true","CloudFront-Is-Mobil
e-Viewer":"false","CloudFront-Is-SmartTV-Viewer":"false","CloudFront-Is-Tablet-Viewer":"false","CloudFront-Viewer-Country":"US","conten
t-type":"application/json","Host":"l9g362zco2.execute-api.us-east-1.amazonaws.com","User-Agent":"curl/7.54.0","Via":"2.0 170a9cb5b4951d
3141f3cdf6b50b780c.cloudfront.net (CloudFront)","X-Amz-Cf-Id":"hC0MNvDYecp6TysdX_LbcHU7U4NMdl8AcAmLNQW7fFMWJtLKsW0bLw==","X-Amzn-Trace-
Id":"Root=1-5c9a2fbf-844aa8f755339602ea3fdc6a","X-Forwarded-For":"96.86.1.5, 70.132.59.151","X-Forwarded-Port":"443","X-Forwarded-Proto
":"https"},"multiValueHeaders":{"Accept":["*/*"],"CloudFront-Forwarded-Proto":["https"],"CloudFront-Is-Desktop-Viewer":["true"],"CloudF
ront-Is-Mobile-Viewer":["false"],"CloudFront-Is-SmartTV-Viewer":["false"],"CloudFront-Is-Tablet-Viewer":["false"],"CloudFront-Viewer-Co
untry":["US"],"content-type":["application/json"],"Host":["l9g362zco2.execute-api.us-east-1.amazonaws.com"],"User-Agent":["curl/7.54.0"
],"Via":["2.0 170a9cb5b4951d3141f3cdf6b50b780c.cloudfront.net (CloudFront)"],"X-Amz-Cf-Id":["hC0MNvDYecp6TysdX_LbcHU7U4NMdl8AcAmLNQW7fF
MWJtLKsW0bLw=="],"X-Amzn-Trace-Id":["Root=1-5c9a2fbf-844aa8f755339602ea3fdc6a"],"X-Forwarded-For":["96.86.1.5, 70.132.59.151"],"X-Forwa
rded-Port":["443"],"X-Forwarded-Proto":["https"]},"queryStringParameters":null,"multiValueQueryStringParameters":null,"pathParameters":
null,"stageVariables":null,"requestContext":{"resourceId":"54yhea","resourcePath":"/users/create","httpMethod":"POST","extendedRequestI
d":"XJxl5FjTIAMF3EQ=","requestTime":"26/Mar/2019:13:57:19 +0000","path":"/dev/users/create","accountId":"490103061721","protocol":"HTTP
/1.1","stage":"dev","domainPrefix":"l9g362zco2","requestTimeEpoch":1553608639379,"requestId":"1234deb3-4fcf-11e9-b124-6949ca7fd2f1","id
entity":{"cognitoIdentityPoolId":null,"accountId":null,"cognitoIdentityId":null,"caller":null,"sourceIp":"96.86.1.5","accessKey":null,"
cognitoAuthenticationType":null,"cognitoAuthenticationProvider":null,"userArn":null,"userAgent":"curl/7.54.0","user":null},"domainName"
:"l9g362zco2.execute-api.us-east-1.amazonaws.com","apiId":"l9g362zco2"},"body":"{\"username\":\"fobar\"}","isBase64Encoded":false}}
  1. Test an invalid body:
$ curl -X POST https://l9g362zco2.execute-api.us-east-1.amazonaws.com/dev/users/create -H 'Content-Type: applica
tion/json' --data-binary '{"username":"fo-bar"}'
{"message": "Invalid request body"}

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@dschep dschep requested review from pmuens and eahefnawy Mar 26, 2019
@pmuens pmuens added this to In progress in Serverless via automation Mar 27, 2019
Copy link
Member

pmuens left a comment

Great PR @dschep 👍

I like it. Should we document the resource logical Id names later on in our resource naming table or would this just result into noise and it's usually not common to manipulate them? 🤔

Also I'm thinking about the abstraction right now.

I really like the simple schema property. When we implemnted request templates we made it possible to provide different templates like this (here's the link to the docs):

events:
  - http:
      method: get
      path: whatever
      integration: lambda
      request:
        template:
          text/xhtml: '{ "stage" : "$context.stage" }'
          application/json: '{ "httpMethod" : "$context.httpMethod" }'

I'm wondering if we should support something like that right now or if we should add that later on (not even sure if there's a need / possibility for multiple schema validation templates).

@dschep

This comment has been minimized.

Copy link
Contributor Author

dschep commented Mar 29, 2019

Good idea @pmuens . I've updated so you do this:

events:
  - http:
      path: users/create
      method: post
      request:
        schema:
          application/json: ${file(create_request.json)}
@dschep

This comment has been minimized.

Copy link
Contributor Author

dschep commented Mar 29, 2019

Should we document the resource logical Id names later on in our resource naming table or would this just result into noise and it's usually not common to manipulate them? 🤔

TBH, I don't know. From the look of it, there's really nothing more you can do with them than what is already being done by the framework, so I don't think it's critical.

Copy link
Member

pmuens left a comment

Thanks for adding the tests @dschep 👍 👌

I just added some comments about potential DRYing practices.

Re "Documenting the logical Ids": After working on a couple of API Gateway changes I saw a ton of logical Ids which were not documented so I think it's reasonable to not document this right now (we could add it later on).

Other than that I think we're pretty close to getting this merged.

template.Properties.RequestValidatorId = { Ref: `${methodLogicalId}Validator` };
template.Properties.RequestModels = {
[contentType]: {
Ref: `${methodLogicalId}${_.startCase(contentType).replace(' ', '')}Model`,

This comment has been minimized.

Copy link
@pmuens

pmuens Apr 15, 2019

Member

Same here.

Serverless automation moved this from In progress to Needs review Apr 15, 2019
@dschep dschep marked this pull request as ready for review Apr 17, 2019
@dschep dschep requested a review from pmuens Apr 17, 2019
Serverless automation moved this from Needs review to Reviewer approved Apr 18, 2019
Copy link
Member

pmuens left a comment

This looks good! Thanks for the update @dschep 👍

Just looked through the code and added some nitpicky comments. They're not of uber importance so feel free to merge if just want to see this in master ASAP.

lib/plugins/aws/lib/naming.js Show resolved Hide resolved
lib/plugins/aws/lib/naming.js Outdated Show resolved Hide resolved
lib/plugins/aws/lib/naming.js Outdated Show resolved Hide resolved
@pmuens pmuens added pr/in-review and removed pr/in-progress labels Apr 18, 2019
Serverless automation moved this from Reviewer approved to Needs review Apr 18, 2019
@dschep dschep force-pushed the sls-3464 branch from cec273d to 88189d9 Apr 18, 2019
@dschep dschep requested a review from pmuens Apr 18, 2019
@dschep dschep requested review from pmuens and removed request for pmuens and eahefnawy Apr 22, 2019
@pmuens
pmuens approved these changes Apr 23, 2019
Copy link
Member

pmuens left a comment

Thanks for fixing the PR comments! LGTM :shipit:

Serverless automation moved this from Needs review to Reviewer approved Apr 23, 2019
@dschep dschep merged commit b042ef0 into master Apr 23, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Serverless automation moved this from Reviewer approved to Done Apr 23, 2019
@dschep dschep added this to the 1.42.0 milestone Apr 23, 2019
@dschep dschep deleted the sls-3464 branch Apr 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Serverless
  
Done
2 participants
You can’t perform that action at this time.