DRY the identity pick and validation #76

Glutexo · 2018-12-21T12:00:28Z

Introduced a new custom Exception to DRY the identity pick and validation. If the header is missing, undecodeable or invalid, this new Exception is raised. Aborting the request with a Forbidden status code is now unified in one place.

This will help with counting invalid logins, which now must accompany every abort call.

Asking @dehort for a review.

Glutexo · 2019-01-04T10:23:43Z

Created an alternative pull request to this: #84 The code itself is the same, but there is no mocking in the tests. Pick whichever you like better.

Glutexo · 2019-01-04T10:26:47Z

Fixed the unit tests – there was a test case copied from elsewhere that should not have been there at all.

Introduced a new custom Exception to DRY the identity pick and validation. If the header is missing, undecodable or invalid, this new Exception is raised. Aborting the request with a Forbidden status code is not unified in one place.

Refactor the tests a bit: * Moved the test descriptions from the noisy comments to the test names. * Renamed unused variables. * Removed a Flask function dependency.

Get rid of the helper method. Use unittest.mock’s side_effect instead.

Glutexo · 2019-01-15T13:28:53Z

Refactored tests for this PR too:

Moved the test descriptions from the noisy comments to the test names.
Got rid of the Flask function dependency.
Renamed unused variables.

Also rebased on current master.

dehort · 2019-01-18T17:15:34Z

app/auth/__init__.py

+            _validate(identity)
+        except InvalidIdentityError:
+            abort(Forbidden.code)
+


I think this needs to catch the top level Exception so that any exceptions are caught and cause an abort.

We also need to make sure we are logging why the authentication request failed.

Any uncaught exception causes an implicit abort with a 500 Internal Server Error. If we want to make sure these aborts returns a valid JSON response (which we should!), it’s a matter of a generic error handling. Thus it belongs neither to the identity module nor to this pull request. Created a separate issue for that #110.

dehort · 2019-01-18T17:25:29Z

app/auth/__init__.py

@@ -21,26 +31,30 @@ def _pick_identity():
        try:
            payload = request.headers[_IDENTITY_HEADER]
        except KeyError:
-            abort(Forbidden.code)
+            raise InvalidIdentityError("The identity header is missing.")


We need to log why the authentication request failed.

Looks like there are a couple of approaches:

Catch the top level Exception here (and in the other places the custom exception is used), log the reason ("unable to retrieve auth header...", "header cannot be decoded", "invalid header", etc), re-throw the exception. We could possibly remove the custom exception classes that we aren't being used that much.

Keep the custom exception classes. Modify the exception handling at line 55 to log the reason why the authn request failed.

Since the failure reason is the exception message, we can log it in the common except block. Good catch as exception implicitly causing an abort is logged automatically, but our manual abort does not log it. 👍

Made the error messages more concise yet more verbose and added logging for them. Maybe these changes should go to their own PR, but added them anyway. I didn’t add tests for the logging.

dehort · 2019-01-18T18:10:13Z

test_unit.py

+
+    @patch("app.auth.from_encoded")
+    @patch("app.auth.request", headers={_IDENTITY_HEADER: Mock()})
+    def test_identity_is_decoded(self, request, from_encoded):


I'm not sure I see much value in this test case. Isn't this same area of the code going to be hit in the valid/invalid decode testing?

I removed all the test cases for this _pick_identity method and use it directly in the requires_identity decorator tests instead of patching them. Thanks to that those tests are covering all the success and failure cases of the _pick_identity method.

dehort · 2019-01-18T18:11:46Z

test_unit.py

+        from_encoded.assert_called_once_with(request.headers[_IDENTITY_HEADER])
+
+    @patch("app.auth.request", headers={_IDENTITY_HEADER: Mock()})
+    def test_identity_is_not_valid_if_decode_fails(self, _):


Maybe we should make the exception handling more generic (catch the top level Exception class) in the _pick_identity method and remove or simplify this test case.

The problem lies in the fact that the identity can actually be bad in different ways, rather than in the number of possible exception types. I simplified this by removing the _pick_identity method tests. Now, I test only the actual abort, which is the same for any failure. The sub-tests are there for the various cases of a bad header, replacing the brittle _pick_identity tests with more durable ones.

dehort · 2019-01-18T18:31:02Z

test_unit.py

+
+
+@patch("app.auth._request_ctx_stack")
+class AuthRequiresIdentityTestCase(TestCase):


This test case seems very complicated. This seems like a lot of (somewhat brittle) code. For example, if a method name changes in the impl, we have to chase down all of those name changes in here. Repeating the internal method names throughout these types of tests concerns me. Maybe I am misunderstanding things.

At the least, it looks like a few of the tests could be combined.

In the end, I think we're really just testing for the case where:

the auth header is not set

the auth header cannot be decoded

the auth header is invalid

I agree that the tests are rather brittle, focusing on internal workings more than on the outcome. Even though I wouldn’t say that exactly a method rename would necessarily be a problem, many other light refactorings would.

I simplified the cases by removing the separate tests for the _pick_identity method and by using the _pick_identity and _validate methods without patching. Now, the decorator is tested as a whole unit. Only the Flask stuff remains mocked and that makes it different from the API tests.

Make the authentication error messages more verbose yet more concise. Added logging of the identity validation errors as warnings.

Made tests simpler and more explicit. The requires_identity decorator is now tested as a unit including the behavior of the _pick_identity and _get_identity helper methods. Those helpers need not to be patched and the tests of the right header format can be more readable.

Glutexo

Thanks for the review, @dehort! I incorporated some changes:

Improved the error messages and added logging.
Simplified the tests.

More information in the inline comments. Is it better now, or even mergeable?

Glutexo · 2019-01-21T13:52:53Z

app/auth/__init__.py

@@ -21,26 +31,30 @@ def _pick_identity():
        try:
            payload = request.headers[_IDENTITY_HEADER]
        except KeyError:
-            abort(Forbidden.code)
+            raise InvalidIdentityError("The identity header is missing.")


Since the failure reason is the exception message, we can log it in the common except block. Good catch as exception implicitly causing an abort is logged automatically, but our manual abort does not log it. 👍

Made the error messages more concise yet more verbose and added logging for them. Maybe these changes should go to their own PR, but added them anyway. I didn’t add tests for the logging.

Glutexo · 2019-01-21T13:53:02Z

app/auth/__init__.py

+            _validate(identity)
+        except InvalidIdentityError:
+            abort(Forbidden.code)
+


Any uncaught exception causes an implicit abort with a 500 Internal Server Error. If we want to make sure these aborts returns a valid JSON response (which we should!), it’s a matter of a generic error handling. Thus it belongs neither to the identity module nor to this pull request. Created a separate issue for that #110.

Glutexo · 2019-01-22T10:59:27Z

test_unit.py

+
+
+@patch("app.auth._request_ctx_stack")
+class AuthRequiresIdentityTestCase(TestCase):


I agree that the tests are rather brittle, focusing on internal workings more than on the outcome. Even though I wouldn’t say that exactly a method rename would necessarily be a problem, many other light refactorings would.

I simplified the cases by removing the separate tests for the _pick_identity method and by using the _pick_identity and _validate methods without patching. Now, the decorator is tested as a whole unit. Only the Flask stuff remains mocked and that makes it different from the API tests.

Glutexo · 2019-01-22T11:09:56Z

test_unit.py

+        from_encoded.assert_called_once_with(request.headers[_IDENTITY_HEADER])
+
+    @patch("app.auth.request", headers={_IDENTITY_HEADER: Mock()})
+    def test_identity_is_not_valid_if_decode_fails(self, _):


The problem lies in the fact that the identity can actually be bad in different ways, rather than in the number of possible exception types. I simplified this by removing the _pick_identity method tests. Now, I test only the actual abort, which is the same for any failure. The sub-tests are there for the various cases of a bad header, replacing the brittle _pick_identity tests with more durable ones.

Glutexo · 2019-01-22T12:41:00Z

test_unit.py

+
+    @patch("app.auth.from_encoded")
+    @patch("app.auth.request", headers={_IDENTITY_HEADER: Mock()})
+    def test_identity_is_decoded(self, request, from_encoded):


I removed all the test cases for this _pick_identity method and use it directly in the requires_identity decorator tests instead of patching them. Thanks to that those tests are covering all the success and failure cases of the _pick_identity method.

dehort · 2019-01-22T15:58:58Z

test_unit.py

+                self._dummy_view_func()
+            with self.assertRaises(NoIdentityError):
+                _get_identity()
+


It looks like test_request_is_aborted_on_bad_identity and test_bad_identity_is_not_stored are essentially testing the same thing. Is there a reason these tests are not combined?

dehort · 2019-01-22T16:36:38Z

test_unit.py

+        with self._patch_request_valid():
+            self._dummy_view_func()
+        self.assertEqual(Identity(account_number="some number"), _get_identity())
+


It looks like test_request_is_not_aborted_on_valid_identity and test_valid_identity_is_stored are essentially testing the same thing. Is there a reason these tests are not combined?

Glutexo · 2019-04-16T11:43:23Z

This is now outdated. Replaced by #203.

Glutexo requested a review from dehort December 21, 2018 12:00

Glutexo force-pushed the dry_identity branch from 2811bb7 to f9cc735 Compare December 21, 2018 15:54

Glutexo self-assigned this Jan 2, 2019

This was referenced Jan 2, 2019

[merge] Login failure count with dry identity #81

Closed

DRY the identity pick and validation – now without mocking #84

Closed

Glutexo force-pushed the dry_identity branch from f9cc735 to 9218054 Compare January 4, 2019 10:26

Glutexo added 3 commits January 15, 2019 13:37

DRY the identity pick and validation

e947d5f

Introduced a new custom Exception to DRY the identity pick and validation. If the header is missing, undecodable or invalid, this new Exception is raised. Aborting the request with a Forbidden status code is not unified in one place.

Refactor tests

ee3a9fe

Refactor the tests a bit: * Moved the test descriptions from the noisy comments to the test names. * Renamed unused variables. * Removed a Flask function dependency.

Fix tests that use abort

6f1f9bd

Get rid of the helper method. Use unittest.mock’s side_effect instead.

Glutexo force-pushed the dry_identity branch from 888b63f to 6f1f9bd Compare January 15, 2019 13:22

dehort reviewed Jan 18, 2019

View reviewed changes

Glutexo added 2 commits January 22, 2019 14:01

Log authentication failures

781d4ea

Make the authentication error messages more verbose yet more concise. Added logging of the identity validation errors as warnings.

Glutexo commented Jan 22, 2019

View reviewed changes

dehort reviewed Jan 22, 2019

View reviewed changes

Glutexo closed this Apr 16, 2019

Glutexo deleted the dry_identity branch April 16, 2019 11:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DRY the identity pick and validation #76

DRY the identity pick and validation #76

Glutexo commented Dec 21, 2018

Glutexo commented Jan 4, 2019

Glutexo commented Jan 4, 2019 •

edited

Loading

Glutexo commented Jan 15, 2019

dehort Jan 18, 2019

Glutexo Jan 21, 2019

dehort Jan 18, 2019

Glutexo Jan 21, 2019

dehort Jan 18, 2019

Glutexo Jan 22, 2019

dehort Jan 18, 2019

Glutexo Jan 22, 2019

dehort Jan 18, 2019 •

edited

Loading

Glutexo Jan 22, 2019

Glutexo left a comment

Glutexo Jan 21, 2019

Glutexo Jan 21, 2019

Glutexo Jan 22, 2019

Glutexo Jan 22, 2019

Glutexo Jan 22, 2019

dehort Jan 22, 2019

dehort Jan 22, 2019

Glutexo commented Apr 16, 2019



		@patch("app.auth._request_ctx_stack")
		class AuthRequiresIdentityTestCase(TestCase):

DRY the identity pick and validation #76

DRY the identity pick and validation #76

Conversation

Glutexo commented Dec 21, 2018

Glutexo commented Jan 4, 2019

Glutexo commented Jan 4, 2019 • edited Loading

Glutexo commented Jan 15, 2019

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

dehort Jan 18, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Glutexo 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

Glutexo commented Apr 16, 2019

Glutexo commented Jan 4, 2019 •

edited

Loading

dehort Jan 18, 2019 •

edited

Loading