From 405592f4951a5ce6aa7de052cb5ddf48cf9b7c37 Mon Sep 17 00:00:00 2001 From: Robert Raposa Date: Tue, 11 Apr 2023 09:21:28 -0400 Subject: [PATCH] fix: reorder JWT decoders (#3941) Reordered the JWT decoders to first use the standard library version, and then use the custom ecommerce decoder which uses multiple issuers. In this way, we can see if any JWTs cannot be decoded by that standard library version, and when and if we are ready to retire the custom JWT decoding code. See DEPR https://github.com/openedx/public-engineering/issues/83 --- ecommerce/extensions/api/handlers.py | 35 ++++++++++--------- .../extensions/api/tests/test_handlers.py | 14 ++++---- 2 files changed, 25 insertions(+), 24 deletions(-) diff --git a/ecommerce/extensions/api/handlers.py b/ecommerce/extensions/api/handlers.py index 0970e58acb5..f40b252869d 100644 --- a/ecommerce/extensions/api/handlers.py +++ b/ecommerce/extensions/api/handlers.py @@ -5,8 +5,6 @@ custom jwt_decode_handler. """ - - import logging import jwt @@ -18,8 +16,6 @@ logger = logging.getLogger(__name__) -JWT_DECODE_HANDLER_METRIC_KEY = 'ecom_jwt_decode_handler' - def _ecommerce_jwt_decode_handler_multiple_issuers(token): """ @@ -94,26 +90,31 @@ def jwt_decode_handler(token): """ - # First, try ecommerce decoder that handles multiple issuers. - # See ARCH-276 for details of removing additional issuers and retiring this - # custom jwt_decode_handler. - try: - jwt_payload = _ecommerce_jwt_decode_handler_multiple_issuers(token) - monitoring_utils.set_custom_metric(JWT_DECODE_HANDLER_METRIC_KEY, 'ecommerce-multiple-issuers') - return jwt_payload - except Exception: # pylint: disable=broad-except - if waffle.switch_is_active('jwt_decode_handler.log_exception.ecommerce-multiple-issuers'): - logger.info('Failed to use ecommerce multiple issuer jwt_decode_handler.', exc_info=True) - - # Next, try jwt_decode_handler from edx_drf_extensions + # First, try jwt_decode_handler from edx_drf_extensions # Note: this jwt_decode_handler can handle asymmetric keys, but only a # single issuer. Therefore, the LMS must be the first configured issuer. try: jwt_payload = edx_drf_extensions_jwt_decode_handler(token) - monitoring_utils.set_custom_metric(JWT_DECODE_HANDLER_METRIC_KEY, 'edx-drf-extensions') + # Note: use increment in case there are multiple calls for different JWTs in the same transaction. + monitoring_utils.increment("ecom_jwt_decode_standard") return jwt_payload except Exception: # pylint: disable=broad-except # continue and try again if waffle.switch_is_active('jwt_decode_handler.log_exception.edx-drf-extensions'): logger.info('Failed to use edx-drf-extensions jwt_decode_handler.', exc_info=True) + + # Next, try ecommerce decoder that handles multiple issuers. + # See ARCH-276 for details of removing additional issuers and retiring this + # custom jwt_decode_handler. + try: + jwt_payload = _ecommerce_jwt_decode_handler_multiple_issuers(token) + # Ultimately we want there to be no JWTs that aren't handled by the first case. + # This enables monitoring to see if there are still cases where the custom JWT + # decoder is in use. + monitoring_utils.increment("ecom_jwt_decode_custom") + return jwt_payload + except Exception: # pylint: disable=broad-except + if waffle.switch_is_active('jwt_decode_handler.log_exception.ecommerce-multiple-issuers'): + logger.info('Failed to use ecommerce multiple issuer jwt_decode_handler.', exc_info=True) + monitoring_utils.increment("ecom_jwt_decode_failed") raise diff --git a/ecommerce/extensions/api/tests/test_handlers.py b/ecommerce/extensions/api/tests/test_handlers.py index 8c19df501be..8893738ab0f 100644 --- a/ecommerce/extensions/api/tests/test_handlers.py +++ b/ecommerce/extensions/api/tests/test_handlers.py @@ -49,9 +49,9 @@ def setUp(self): 'JWT_VERIFY_AUDIENCE': False, } ) - @mock.patch('edx_django_utils.monitoring.set_custom_metric') + @mock.patch('edx_django_utils.monitoring.increment') @mock.patch('ecommerce.extensions.api.handlers._ecommerce_jwt_decode_handler_multiple_issuers') - def test_decode_success_edx_drf_extensions(self, mock_multiple_issuer_decoder, mock_set_custom_metric): + def test_decode_success_edx_drf_extensions(self, mock_multiple_issuer_decoder, mock_increment): """ Should pass using the edx-drf-extensions jwt_decode_handler. @@ -63,7 +63,7 @@ def test_decode_success_edx_drf_extensions(self, mock_multiple_issuer_decoder, m payload = generate_jwt_payload(self.user, issuer_name=first_issuer['ISSUER']) token = generate_jwt_token(payload, first_issuer['SECRET_KEY']) self.assertDictContainsSubset(payload, jwt_decode_handler(token)) - mock_set_custom_metric.assert_called_with('ecom_jwt_decode_handler', 'edx-drf-extensions') + mock_increment.assert_called_with('ecom_jwt_decode_standard') @override_settings( JWT_AUTH={ @@ -87,9 +87,9 @@ def test_decode_success_edx_drf_extensions(self, mock_multiple_issuer_decoder, m 'JWT_VERIFY_AUDIENCE': False, } ) - @mock.patch('edx_django_utils.monitoring.set_custom_metric') + @mock.patch('edx_django_utils.monitoring.increment') @mock.patch('ecommerce.extensions.api.handlers.logger') - def test_decode_success_multiple_issuers(self, mock_logger, mock_set_custom_metric): + def test_decode_success_multiple_issuers(self, mock_logger, mock_increment): """ Should pass using ``_ecommerce_jwt_decode_handler_multiple_issuers``. @@ -101,7 +101,7 @@ def test_decode_success_multiple_issuers(self, mock_logger, mock_set_custom_metr payload = generate_jwt_payload(self.user, issuer_name=non_first_issuer['ISSUER']) token = generate_jwt_token(payload, non_first_issuer['SECRET_KEY']) self.assertDictContainsSubset(payload, jwt_decode_handler(token)) - mock_set_custom_metric.assert_called_with('ecom_jwt_decode_handler', 'ecommerce-multiple-issuers') + mock_increment.assert_called_with('ecom_jwt_decode_custom') mock_logger.exception.assert_not_called() mock_logger.warning.assert_not_called() mock_logger.error.assert_not_called() @@ -141,6 +141,6 @@ def test_decode_error_invalid_token(self, mock_logger): mock_logger.exception.assert_called_with('Custom config JWT decode failed!') mock_logger.info.assert_has_calls(calls=[ - mock.call('Failed to use ecommerce multiple issuer jwt_decode_handler.', exc_info=True), mock.call('Failed to use edx-drf-extensions jwt_decode_handler.', exc_info=True), + mock.call('Failed to use ecommerce multiple issuer jwt_decode_handler.', exc_info=True), ])