Add support for local custom authorizers #467

stealthycoin · 2017-08-10T01:56:39Z

BuiltinAuthorizer will now work in local mode. Authorizer types that
cannot be run locally will now simply allow the requests to proceed as
if there were no authorizer to allow for local testing.

closes #404

codecov-io · 2017-08-10T02:00:18Z

Codecov Report

Merging #467 into master will increase coverage by 0.25%.
The diff coverage is 98.65%.

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
+ Coverage   94.01%   94.26%   +0.25%     
==========================================
  Files          18       18              
  Lines        2939     3103     +164     
  Branches      380      396      +16     
==========================================
+ Hits         2763     2925     +162     
  Misses        129      129              
- Partials       47       49       +2

Impacted Files	Coverage Δ
chalice/cli/__init__.py	`81.86% <100%> (ø)`	⬆️
chalice/cli/factory.py	`89.52% <50%> (ø)`	⬆️
chalice/local.py	`98.26% <99.09%> (+0.66%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 07968a6...7305052. Read the comment docs.

JordonPhillips

Code looks good! Only had a few minor comments.

JordonPhillips · 2017-08-10T20:57:35Z

chalice/cli/factory.py

-    def create_local_server(self, app_obj, port):
-        # type: (Chalice, int) -> local.LocalDevServer
-        return local.create_local_server(app_obj, port)
+    def create_local_server(self, app_obj, config, port):


You don't appear to be testing this function

JordonPhillips · 2017-08-10T21:03:42Z

chalice/local.py

+        self._initialize_lambda_context_attributes(
+            function_name, memory_size)
+
+    def _initialize_lambda_context_attributes(self, function_name,


It's a bit bizzare to define instance attributes outside of init, especially since you're not doing any complicated initialization or logic to set their initial values.

I separated the logic that is specific to the actual lambda context initialization vs the initializations I have to do for my fake object to run.

Yeah I'm surprised pylint didn't yell at you for this. Even if you want it extracted out to a separate function, I think they should also be "declared" in the __init__.

JordonPhillips · 2017-08-10T21:13:58Z

chalice/local.py

+    def _prepare_authorizer_event(self, lambda_event):
+        # type: (EventType) -> EventType
+        """Translate event for an authorizer input."""
+        authorizer_event = lambda_event.copy()


How nested is this structure? if it's more than one level then maybe you should deepcopy.

The event is a dict of entirely [str, Union[str, Dict[str, str]] so I don't think it needs to be deepcopied unless I am mistaken about something.

JordonPhillips · 2017-08-10T21:17:27Z

chalice/local.py

+        try:
+            route_entry = self._app_object.routes[resource_path][http_method]
+        except KeyError:
+            # If there a key error is raised when trying to get the route entry


s/If there/If/

JordonPhillips · 2017-08-10T21:19:42Z

chalice/local.py

+            # here by returning no authorizer to avoid duplicating the logic.
+            return None, None
+        authorizer = route_entry.authorizer
+        return authorizer, route_entry


If authorizer is an attribute of route_entry why are you not just returning route_entry?

Yes I think this made more sense before a refactor and I didn't think to update it.

JordonPhillips · 2017-08-10T21:21:52Z

chalice/local.py

+        # Technically we only need to check for authorizer being `None` here
+        # since the route_entry will always match authorizer but mypy needs us
+        # to explicitly check both before we can use them. In any event, if
+        # either of these are not defined then there was no authroizer or


You seem to have trailed off here.

JordonPhillips · 2017-08-10T21:39:26Z

chalice/local.py

+        return response
+
+
+class ChaliceRequestHandler(BaseHTTPRequestHandler):


A class doc string for this would be nice

jamesls

Some initial feedback, still going through it.

jamesls · 2017-08-10T22:35:13Z

chalice/local.py

+        self._initialize_lambda_context_attributes(
+            function_name, memory_size)
+
+    def _initialize_lambda_context_attributes(self, function_name,


Yeah I'm surprised pylint didn't yell at you for this. Even if you want it extracted out to a separate function, I think they should also be "declared" in the __init__.

jamesls · 2017-08-10T22:36:17Z

chalice/local.py

+            # automatically generate our CORS headers.
+            options_headers = self._autogen_options_headers(lambda_event)
+            raise NoOptionsRouteDefined(options_headers)
+        # The authroizer call will be a noop if there is no authroizer method


%s/authroizer/authorizer/g

jamesls · 2017-08-10T22:36:59Z

chalice/local.py

+            # BuiltinAuthConfig type. Anything else we will err on the side of
+            # allowing local testing by simply admiting the request. Otherwise
+            # there is no way for users to test their code in local mode
+            warnings.warn((


You shouldn't need the extra set of parens here.

jamesls · 2017-08-11T19:54:04Z

chalice/local.py

+        # particular view function.
+        for statement in statements:
+            if statement.get('Effect') == 'Allow' and \
+               statement.get('Action') == 'execute-api:Invoke':


These technically support wildcards as well. If you're not going to support them, you should probably make a note about eventually supporting them.

jamesls · 2017-08-11T20:15:21Z

chalice/local.py

+            pattern_suffix = '/'
+        method_pattern = r'(?:\*|%s)' % route_entry.method
+        route_entry_pattern = r'^.*?/%s%s%s$' % (method_pattern,
+                                                 route_entry.uri_pattern,


I don't think this is right. The uri pattern will include the placeholder values so something like this won't work:

@app.route('/foo/{name}', authorizer=demo_auth) def index(name): return {'context': app.current_request.context}

If I return AuthResponse(routes=['/foo/bar'], principal_id='user') from my authorizer.

What if we mirrored things more closely to real IAM?

Generate the resource ARN for the specific method/view

Check if any of the resources in the returned policy match the ARN (accounting for wildcards).

I also would suggest extracting this policy matching out to a separate class so we can add a lot more tests for the edge cases (using pytest.paramtrize).

JordonPhillips

Implementation looks good! Just one little nitpick before I go over the tests.

JordonPhillips · 2017-08-23T17:46:40Z

chalice/local.py

+        escaped_resource = re.escape(resource)
+        resource_regex = escaped_resource.replace(r'\?', '.').replace(
+            r'\*', '.*?')
+        matcher = re.compile(resource_regex)


nitpick, but you don't need to compile if you're just immediately using it anyway

jamesls

Just had some minor feedback.

jamesls · 2017-08-24T00:20:53Z

chalice/app.pyi

@@ -1,3 +1,4 @@
+from chalice.local import LambdaContext


Import orders, chalice after 3rd party libs.

jamesls · 2017-08-24T00:21:48Z

chalice/local.py

+    return LocalDevServer(app_obj, config, port)
+
+
+class LocalArnBuilder(object):


jamesls · 2017-08-24T00:22:51Z

chalice/local.py

+        )
+
+
+class ArnMatcher(object):


jamesls · 2017-08-24T00:27:03Z

chalice/local.py

+                % authorizer.__class__.__name__
+            )
+            return lambda_event, lambda_context
+        auth_result = authorizer(auth_event, lambda_context)


I don't think this is quite right. If you don't have the appropriate auth header (Authorization) in the request, you get a 401 and the authorizer isn't called at all.

jamesls · 2017-08-24T00:28:31Z

chalice/local.py

+
+    def _send_not_authorized_response(self, headers):
+        # type: (HeaderType) -> None
+        self.send_response(401)


An authorized error where we generate a policy that forbids access to a resource gives a 403, not a 401.

$ http https://rest-api-id.execute-api.us-west-2.amazonaws.com/api/ 'Authorization: deny' HTTP/1.1 403 Forbidden Connection: keep-alive Content-Length: 60 Content-Type: application/json Date: Thu, 24 Aug 2017 00:27:57 GMT Via: 1.1 7f3f42df8af148df1f9f1ee7175987ad.cloudfront.net (CloudFront) X-Amz-Cf-Id: 4ZhC3GokHeXr68EKakpF6xQWxVvTDZtQiGK253psOcWbPW3M980rGw== X-Cache: Error from cloudfront x-amzn-ErrorType: AccessDeniedException x-amzn-RequestId: 13ff2f6d-8863-11e7-af78-e1cc9bcdaa69 { "Message": "User is not authorized to access this resource" }

jamesls · 2017-08-24T00:39:45Z

chalice/local.py

+            time_source = Clock()
+        self._time_source = time_source
+        self._start_time = self._current_time_millis()
+        self._max_runtime = runtime_millis


runtime_millis is pretty vague, but max_runtime_ms is a lot clearer. Is there a reason to not map to call the arg something different than it's instance attribute name?

I wanted to give the unit in the name is why its not a direct mirror. I agree with max_runtime_ms so I'll do that.

jamesls · 2017-08-24T00:41:59Z

tests/unit/test_local.py

    assert dev_server.server.server_port == 23456
+
+
+class FakeTimeSource(object):


We usually put all the non test classes/functions at the top of the file before the actual tests.

I was keeping it nearby for easier reference. I'll move it.

jamesls · 2017-08-24T00:45:03Z

tests/unit/test_local.py

+        event, context = authorizer.authorize(event, context)
+        assert event['requestContext']['authorizer']['principalId'] == 'user'
+
+    def test_can_authorize_with_lower_case_headers(self, lambda_context_args,


Isn't this identical to the previous test test_can_authorize_empty_path?

jamesls · 2017-08-24T00:45:52Z

tests/unit/test_local.py

+        event, context = authorizer.authorize(event, context)
+        # Assert that when the authorizer.authorize is called and there is no
+        # authorizer defined for a particular route that it is a noop.
+        assert event == event


Fixed... still passes.

jamesls · 2017-08-24T00:46:07Z

tests/unit/test_local.py

+        # Assert that when the authorizer.authorize is called and there is no
+        # authorizer defined for a particular route that it is a noop.
+        assert event == event
+        assert context == context


jamesls · 2017-08-30T22:13:10Z

I'm just going through this now, but it looks like there's still some feedback that hasn't been incorporated: #467 (comment)

The ARN generated for a route that has a captured param is still: 'arn:aws:execute-api:mars-west-1:123456789012:ymy8tbxw7b/api/GET/foo/{name}' which isn't right. I tested the example in the linked comment and it still fails in local mode but passes on actual API gateway/lambda.

BuiltinAuthorizer will now work in local mode. Authorizer types that cannot be run locally will now simply allow the requests to proceed as if there were no authorizer to allow for local testing.

stealthycoin requested review from jamesls and kyleknap August 10, 2017 01:56

JordonPhillips reviewed Aug 10, 2017

View reviewed changes

jamesls reviewed Aug 11, 2017

View reviewed changes

stealthycoin force-pushed the local-custom-auth branch 2 times, most recently from bd86e3b to a47eb8c Compare August 23, 2017 17:04

JordonPhillips reviewed Aug 23, 2017

View reviewed changes

JordonPhillips approved these changes Aug 23, 2017

View reviewed changes

jamesls reviewed Aug 24, 2017

View reviewed changes

stealthycoin force-pushed the local-custom-auth branch 4 times, most recently from e006290 to 3712b8b Compare August 28, 2017 20:21

stealthycoin force-pushed the local-custom-auth branch 2 times, most recently from 0e58247 to 2030304 Compare August 31, 2017 06:49

jamesls approved these changes Sep 1, 2017

View reviewed changes

stealthycoin force-pushed the local-custom-auth branch 5 times, most recently from 517261f to 8403c5e Compare September 5, 2017 07:31

Add support for local custom authorizers

7305052

BuiltinAuthorizer will now work in local mode. Authorizer types that cannot be run locally will now simply allow the requests to proceed as if there were no authorizer to allow for local testing.

stealthycoin force-pushed the local-custom-auth branch from 8403c5e to 7305052 Compare September 5, 2017 07:35

stealthycoin merged commit c09ed07 into aws:master Sep 5, 2017

stealthycoin deleted the local-custom-auth branch September 5, 2017 07:37

		return response


		class ChaliceRequestHandler(BaseHTTPRequestHandler):

		return LocalDevServer(app_obj, config, port)


		class LocalArnBuilder(object):

		assert dev_server.server.server_port == 23456


		class FakeTimeSource(object):

Add support for local custom authorizers #467

Add support for local custom authorizers #467

Conversation

stealthycoin commented Aug 10, 2017 • edited Loading

codecov-io commented Aug 10, 2017 • edited Loading

Codecov Report

JordonPhillips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JordonPhillips left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jamesls commented Aug 30, 2017

stealthycoin commented Aug 10, 2017 •

edited

Loading

codecov-io commented Aug 10, 2017 •

edited

Loading