Added Access-Control-Allow-Credentials for CORS settings fixes #2182 #2736

asprouse · 2016-11-17T19:04:05Z

What did you implement:

How did you implement it:

Added an allowCredentials (defaults to false) setting to the cors config which determines whether or not Access-Control-Allow-Credentials will be set to true or false in the mock OPTIONS request that is configured by the existing cors configuration.

The original fix was implemented by @fruffin then the codebase changed significantly so I applied the same changes to the new code. I don't mean to step on toes, just trying to lend a helping hand @fruffin deserves most of the credit here.

How can we verify it:

Run unit tests.

Toggle settings in the config and verify the changes are reflected in the CloudFormation template.

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
Change ready for review message below

Is this ready for review?: YES

fruffin · 2016-11-18T10:09:52Z

Thanks a lot for picking this one up @asprouse !!!
I have to say I felt a bit demotivated seeing how much the code had changed all of a sudden and I'm glad someone else took the time to port the change to the new code :)

pmuens · 2016-11-22T07:52:40Z

@asprouse and @fruffin thanks for opening this PR! 👍 💯

Looks good from a code design perspective. Haven't tested it thoroughly yet (will do this next up).
@eahefnawy could you also take a look at this one?

FYI: Just pushed some minor updates because some docs were accidentally removed.

pmuens · 2016-11-22T08:04:10Z

docs/providers/aws/events/apigateway.md

@@ -535,6 +535,7 @@ functions:
              - Authorization
              - X-Api-Key
              - X-Amz-Security-Token
+            allowCredentials: true


Do we need more documentation here? Or is this sufficient as it's only a simple config parameter?

/cc @eahefnawy

str3tch · 2016-12-05T02:54:58Z

Shame this missed the 1.3 release, is it on the roadmap for a release soon??

asprouse · 2016-12-05T16:50:09Z

Just brought my fork up to date and fixed merge conflicts. @pmuens I think the docs are sufficient. Let me know if there is anything else missing.

fruffin · 2016-12-23T22:42:57Z

@pmuens it's been over a month since that PR has been opened and over 2 months since my original PR for this. As you can see @asprouse already had to make a few changes to keep it up to date with other serverless changes. Any chance this could be reviewed/merged soon?

pmuens · 2017-01-04T12:04:22Z

@asprouse @fruffin I'm feeling very sorry and bad about that. This PR was in a good state when I reviewed it.

Thank you for all your time you've put into this one 🥇 (and sorry again for the slow response)...

I'll put this PR in the next milestone v1.6 so that we can get it in an upcoming release soon.

dougmoscrop · 2017-01-18T18:59:40Z

It would make reviewing this a lot easier if you would rebase your changes instead of merging. Can you do a git pull --rebase [upstream] master and then git push -f [origin] master

(replace upstream/origin with whatever your remotes are)

…less#2182

asprouse · 2017-01-18T22:18:32Z

@dougmoscrop I just cloned a fresh copy of master and applied the single commit. A lot changes in 2 months 😉 . Hopefully this helps with the review

pmuens

@asprouse thanks for rebasing this (and thanks @dougmoscrop for providing a description on how to do this) 👍 💯

I looked into it today and played around with it. I can see the header in the Method Responses in the API Gateway dashboard and furthermore see it in the CloudFormation output.

Could you maybe provide a quick description how this can be validated more thoroughly (with different settings) with a Serverless service (for both integration types)? This would make reviewing this way easier.

Thanks in advance!

asprouse · 2017-01-19T16:12:28Z

@pmuens The integration type does not matter. Preflight requests are implemented as type MOCK in API Gateway and will be the same regardless of whether you're Lambda Proxy or not. Here are the scenarios that should work properly: (Almost identical to the steps @fruffin provided in his PR)

Scenario 1

Start a project with a function responding to an API Gateway event
Enable CORS with the old configuration
Deploy
Verify that the preflight request has the Access-Control-Allow-Credentials header set to false

Scenario 2

Start a project with a function responding to an API Gateway event
Enable CORS with the old configuration
Add the allowCredentials configuration element to CORS section of the API Gateway event and set it to true
Deploy
Verify that the preflight request has the Access-Control-Allow-Credentials header set to true

Scenario 3

Start a project with a function responding to an API Gateway event
Enable CORS with the old configuration
Add the allowCredentials configuration element to CORS section of the API Gateway event and set it to false
Deploy
Verify that the preflight request has the Access-Control-Allow-Credentials header set to false

The unit tests should verify these cases.

pmuens

@asprouse thanks for the detailed test cases! Really helped me to test this thoroughly! 👍

I just added two minor comments which tackle the default setup of allowCredentials.

Furthermore it would be great if you could add a short section in the CORS docs for API Gateway so that this feature is documented.

After that it's GTM from my side

For everyone else. Here's the serverless.yml file I used to test this feature:

service: service

provider:
  name: aws
  runtime: nodejs4.3
  cfLogs: true

functions:
  hello:
    handler: handler.hello
    events:
      - http:
          method: GET
          path: hello
          integration: LAMBDA
          cors:
            origins:
              - '*'
            headers:
              - Content-Type
              - X-Amz-Date
              - Authorization
              - X-Api-Key
              - X-Amz-Security-Token
            allowCredentials: false
  goodbye:
    handler: handler.goodbye
    events:
      - http: GET goodbye

And here's a screenshot of the integration response.

pmuens · 2017-01-20T10:19:48Z

lib/plugins/aws/deploy/compile/events/apiGateway/lib/cors.test.js

@@ -55,16 +55,19 @@ describe('#compileCors()', () => {
        origins: ['*'],
        headers: ['*'],
        methods: ['OPTIONS', 'PUT'],
+        allowCredentials: false,


Could we remove this setup so that we test that it defaults to false?

Then we have one test w/o any allowCredentials setup, one which tests if it's set to true and one which tests if it's set to false.

The code in validate.js applies the default so there is no default logic to test here. compileCors operates on the validated input (see cors.js). I am just following the convention established in the existing code.

pmuens · 2017-01-20T10:21:34Z

lib/plugins/aws/deploy/compile/events/apiGateway/lib/validate.test.js

@@ -401,6 +401,7 @@ describe('#validate()', () => {
      headers: ['Content-Type', 'X-Amz-Date', 'Authorization', 'X-Api-Key', 'X-Amz-Security-Token'],
      methods: ['OPTIONS', 'POST'],
      origins: ['*'],
+      allowCredentials: false,


Could we do the same like proposed above (for the other test) and remove this configuration so that we test for the default case (which should set allowCredentials to false).

This is correct validate should default to false.

…ders.

asprouse · 2017-01-20T21:15:30Z

@pmuens I just improved the docs. Should be good let me know if there is anything else.

pmuens

Thanks for the updates @asprouse and for the PR in general!

GTM from my side!

eahefnawy

@asprouse Thanks for this PR and the thorough validation/testing steps. Couldn't break it! G2M! 💥

fruffin mentioned this pull request Nov 18, 2016

[FIX #2182] Add Access-Control-Allow-Credentials for CORS settings #2403

Closed

6 tasks

pmuens added status/0-triage and removed status/0-triage labels Nov 20, 2016

pmuens reviewed Nov 22, 2016

View reviewed changes

pmuens added this to the 1.6 milestone Jan 4, 2017

pmuens removed the status/1-design-review label Jan 4, 2017

Added Access Control Allow Credentials for CORS settings fixes server…

d5b64c8

…less#2182

asprouse force-pushed the master branch from 93e73c6 to d5b64c8 Compare January 18, 2017 22:15

pmuens reviewed Jan 19, 2017

View reviewed changes

pmuens suggested changes Jan 20, 2017

View reviewed changes

asprouse added 2 commits January 20, 2017 16:12

Improved CORS documentation with regard to Access-Control-Allow-* hea…

576cc2f

…ders.

Removed duplicate CORS documentation that leads to confusion

3b3ab15

pmuens approved these changes Jan 23, 2017

View reviewed changes

eahefnawy approved these changes Jan 26, 2017

View reviewed changes

eahefnawy merged commit 96139e7 into serverless:master Jan 26, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added Access-Control-Allow-Credentials for CORS settings fixes #2182 #2736

Added Access-Control-Allow-Credentials for CORS settings fixes #2182 #2736

asprouse commented Nov 17, 2016

fruffin commented Nov 18, 2016

pmuens commented Nov 22, 2016

pmuens Nov 22, 2016 •

edited

str3tch commented Dec 5, 2016

asprouse commented Dec 5, 2016

fruffin commented Dec 23, 2016

pmuens commented Jan 4, 2017 •

edited

dougmoscrop commented Jan 18, 2017

asprouse commented Jan 18, 2017

pmuens left a comment •

edited

asprouse commented Jan 19, 2017

pmuens left a comment •

edited

pmuens Jan 20, 2017

asprouse Jan 20, 2017

pmuens Jan 20, 2017

asprouse Jan 20, 2017

asprouse commented Jan 20, 2017

pmuens left a comment

eahefnawy left a comment

Added Access-Control-Allow-Credentials for CORS settings fixes #2182 #2736

Added Access-Control-Allow-Credentials for CORS settings fixes #2182 #2736

Conversation

asprouse commented Nov 17, 2016

What did you implement:

How did you implement it:

How can we verify it:

Todos:

fruffin commented Nov 18, 2016

pmuens commented Nov 22, 2016

pmuens Nov 22, 2016 • edited

Choose a reason for hiding this comment

str3tch commented Dec 5, 2016

asprouse commented Dec 5, 2016

fruffin commented Dec 23, 2016

pmuens commented Jan 4, 2017 • edited

dougmoscrop commented Jan 18, 2017

asprouse commented Jan 18, 2017

pmuens left a comment • edited

Choose a reason for hiding this comment

asprouse commented Jan 19, 2017

Scenario 1

Scenario 2

Scenario 3

pmuens left a comment • edited

Choose a reason for hiding this comment

pmuens Jan 20, 2017

Choose a reason for hiding this comment

asprouse Jan 20, 2017

Choose a reason for hiding this comment

pmuens Jan 20, 2017

Choose a reason for hiding this comment

asprouse Jan 20, 2017

Choose a reason for hiding this comment

asprouse commented Jan 20, 2017

pmuens left a comment

Choose a reason for hiding this comment

eahefnawy left a comment

Choose a reason for hiding this comment

pmuens Nov 22, 2016 •

edited

pmuens commented Jan 4, 2017 •

edited

pmuens left a comment •

edited

pmuens left a comment •

edited