Add support for a more granular CORSConfig object. #311
Conversation
stealthycoin
force-pushed
the
cors-config
branch
from
1133702 to
aab1c44
Compare
Codecov Report
@@ Coverage Diff @@
## master #311 +/- ##
==========================================
+ Coverage 88.77% 88.92% +0.15%
==========================================
Files 18 18
Lines 1826 1860 +34
Branches 213 224 +11
==========================================
+ Hits 1621 1654 +33
Misses 152 152
- Partials 53 54 +1
Continue to review full report at Codecov.
|
stealthycoin
force-pushed
the
cors-config
branch
from
aab1c44 to
da7ad10
Compare
chalice/app.py
Outdated
| if allow_headers is None: | ||
| allow_headers = list(self._REQUIRED_HEADERS) | ||
| else: | ||
| allow_headers.extend(self._REQUIRED_HEADERS) |
There was a problem hiding this comment.
What if a passed in allow_header contains one of the _REQUIRED_HEADERS? Seems like we should check for existing values or use a set().
There was a problem hiding this comment.
Good point, I'll fix that.
chalice/app.py
Outdated
| @@ -167,6 +215,10 @@ def __init__(self, view_function, view_name, path, methods, | |||
| #: e.g, '/foo/{bar}/{baz}/qux -> ['bar', 'baz'] | |||
| self.view_args = self._parse_view_args() | |||
| self.content_types = content_types | |||
| if cors is True: | |||
There was a problem hiding this comment.
Can you add a comment about why the identity check is being used instead of just boolean comparison. This is probably gonna catch other dev's attention because it's not a common pattern so it's worth explaining why we need to do this.
There was a problem hiding this comment.
Yea I was considering pulling it out into a _configure_cors and put a comment in there. I'll just do the comment.
chalice/app.py
Outdated
| def _add_cors_headers(self, response): | ||
| response.headers['Access-Control-Allow-Origin'] = '*' | ||
| def _add_cors_headers(self, response, cors): | ||
| response.headers.update(cors.get_access_control_headers()) |
There was a problem hiding this comment.
I realize the old code didn't do this, but I wonder if we should be overriding headers if they provide these values themselves, i.e a custom Response value.
There was a problem hiding this comment.
Hmmm, that seems like the correct behavior to me. I wouldn't want my explicitly set headers getting clobbered by my framework.
|
Looks good, just a few things remaining:
|
stealthycoin
force-pushed
the
cors-config
branch
from
86d1312 to
f4e8fdb
Compare
stealthycoin
force-pushed
the
cors-config
branch
2 times, most recently
from
0cb6245 to
0b94843
Compare
Allows specification of specific access control headers. Docs still need to be written.
I misunderstood the spec slightly, this update corrects that misunderstanding and increases code coverage.
stealthycoin
force-pushed
the
cors-config
branch
from
0b94843 to
8661e1c
Compare
Allows specification of specific access control headers. Docs still
need to be written.
Implements: #70 (comment)