From ff4d9e4360675d55caceece9ff587f140e7ff443 Mon Sep 17 00:00:00 2001 From: Clinton Blackburn Date: Sun, 23 Apr 2017 12:10:15 -0400 Subject: [PATCH] Using shared secret for JWTs sent to Credentials API This change brings the Credentials API calls in line with those of other services. The change also makes it easier for the future switch to an asymmetric signing key. LEARNER-629 --- lms/djangoapps/student_account/views.py | 2 +- .../djangoapps/catalog/tests/test_utils.py | 2 +- openedx/core/djangoapps/catalog/utils.py | 22 +----- openedx/core/djangoapps/credentials/utils.py | 18 ++++- openedx/core/lib/edx_api_utils.py | 26 +------ openedx/core/lib/tests/test_edx_api_utils.py | 74 ++++--------------- 6 files changed, 38 insertions(+), 106 deletions(-) diff --git a/lms/djangoapps/student_account/views.py b/lms/djangoapps/student_account/views.py index a24f482755..aadbf9fee6 100644 --- a/lms/djangoapps/student_account/views.py +++ b/lms/djangoapps/student_account/views.py @@ -338,7 +338,7 @@ def get_user_orders(user): cache_key = commerce_configuration.CACHE_KEY + '.' + str(user.id) if use_cache else None api = ecommerce_api_client(user) commerce_user_orders = get_edx_api_data( - commerce_configuration, user, 'orders', api=api, querystring=user_query, cache_key=cache_key + commerce_configuration, 'orders', api=api, querystring=user_query, cache_key=cache_key ) for order in commerce_user_orders: diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index 8c28dfd9ae..791d2aae71 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -198,7 +198,7 @@ class TestGetCourseRuns(CatalogIntegrationMixin, TestCase): """ args, kwargs = call_args - for arg in (self.catalog_integration, self.user, 'course_runs'): + for arg in (self.catalog_integration, 'course_runs'): self.assertIn(arg, args) self.assertEqual(kwargs['api']._store['base_url'], self.catalog_integration.internal_api_url) # pylint: disable=protected-access diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index f77ad42165..dc7d6c94aa 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -2,7 +2,6 @@ import copy import logging -import waffle from django.conf import settings from django.contrib.auth import get_user_model from edx_rest_api_client.client import EdxRestApiClient @@ -11,7 +10,6 @@ from openedx.core.djangoapps.catalog.models import CatalogIntegration from openedx.core.lib.edx_api_utils import get_edx_api_data from openedx.core.lib.token_utils import JwtBuilder - log = logging.getLogger(__name__) User = get_user_model() # pylint: disable=invalid-name @@ -67,11 +65,10 @@ def get_programs(uuid=None, types=None): # pylint: disable=redefined-builtin return get_edx_api_data( catalog_integration, - user, 'programs', + api=api, resource_id=uuid, cache_key=cache_key if catalog_integration.is_cache_enabled else None, - api=api, querystring=querystring, ) else: @@ -98,13 +95,8 @@ def get_program_types(name=None): api = create_catalog_api_client(user, catalog_integration) cache_key = '{base}.program_types'.format(base=catalog_integration.CACHE_KEY) - data = get_edx_api_data( - catalog_integration, - user, - 'program_types', - cache_key=cache_key if catalog_integration.is_cache_enabled else None, - api=api - ) + data = get_edx_api_data(catalog_integration, 'program_types', api=api, + cache_key=cache_key if catalog_integration.is_cache_enabled else None) # Filter by name if a name was provided if name: @@ -169,12 +161,6 @@ def get_course_runs(): 'exclude_utm': 1, } - course_runs = get_edx_api_data( - catalog_integration, - user, - 'course_runs', - api=api, - querystring=querystring, - ) + course_runs = get_edx_api_data(catalog_integration, 'course_runs', api=api, querystring=querystring) return course_runs diff --git a/openedx/core/djangoapps/credentials/utils.py b/openedx/core/djangoapps/credentials/utils.py index 9e381492d6..38eced303c 100644 --- a/openedx/core/djangoapps/credentials/utils.py +++ b/openedx/core/djangoapps/credentials/utils.py @@ -1,15 +1,28 @@ """Helper functions for working with Credentials.""" from __future__ import unicode_literals + import logging +from django.conf import settings +from edx_rest_api_client.client import EdxRestApiClient + from openedx.core.djangoapps.catalog.utils import get_programs from openedx.core.djangoapps.credentials.models import CredentialsApiConfig from openedx.core.lib.edx_api_utils import get_edx_api_data - +from openedx.core.lib.token_utils import JwtBuilder log = logging.getLogger(__name__) +def get_credentials_api_client(user): + """ Returns an authenticated Credentials API client. """ + + scopes = ['email', 'profile'] + expires_in = settings.OAUTH_ID_TOKEN_EXPIRATION + jwt = JwtBuilder(user).build_token(scopes, expires_in) + return EdxRestApiClient(CredentialsApiConfig.current().internal_api_url, jwt=jwt) + + def get_credentials(user, program_uuid=None): """ Given a user, get credentials earned from the credentials service. @@ -35,9 +48,10 @@ def get_credentials(user, program_uuid=None): # want to see them displayed immediately. use_cache = credential_configuration.is_cache_enabled and not user.is_staff cache_key = credential_configuration.CACHE_KEY + '.' + user.username if use_cache else None + api = get_credentials_api_client(user) return get_edx_api_data( - credential_configuration, user, 'credentials', querystring=querystring, cache_key=cache_key + credential_configuration, 'credentials', api=api, querystring=querystring, cache_key=cache_key ) diff --git a/openedx/core/lib/edx_api_utils.py b/openedx/core/lib/edx_api_utils.py index 172cf8e6d2..fc330ee622 100644 --- a/openedx/core/lib/edx_api_utils.py +++ b/openedx/core/lib/edx_api_utils.py @@ -14,15 +14,14 @@ from openedx.core.lib.token_utils import JwtBuilder log = logging.getLogger(__name__) -def get_edx_api_data(api_config, user, resource, api=None, resource_id=None, - querystring=None, cache_key=None, many=True, traverse_pagination=True): +def get_edx_api_data(api_config, resource, api, resource_id=None, querystring=None, cache_key=None, many=True, + traverse_pagination=True): """GET data from an edX REST API. DRY utility for handling caching and pagination. Arguments: api_config (ConfigurationModel): The configuration model governing interaction with the API. - user (User): The user to authenticate as when requesting data. resource (str): Name of the API resource being requested. Keyword Arguments: @@ -52,27 +51,6 @@ def get_edx_api_data(api_config, user, resource, api=None, resource_id=None, if cached: return cached - try: - if not api: - # TODO: Use the system's JWT_AUDIENCE and JWT_SECRET_KEY instead of client ID and name. - client_name = api_config.OAUTH2_CLIENT_NAME - - try: - client = Client.objects.get(name=client_name) - except Client.DoesNotExist: - raise ImproperlyConfigured( - 'OAuth2 Client with name [{}] does not exist.'.format(client_name) - ) - - scopes = ['email', 'profile'] - expires_in = settings.OAUTH_ID_TOKEN_EXPIRATION - jwt = JwtBuilder(user, secret=client.client_secret).build_token(scopes, expires_in, aud=client.client_id) - - api = EdxRestApiClient(api_config.internal_api_url, jwt=jwt) - except: # pylint: disable=bare-except - log.exception('Failed to initialize the %s API client.', api_config.API_NAME) - return no_data - try: endpoint = getattr(api, resource) querystring = querystring if querystring else {} diff --git a/openedx/core/lib/tests/test_edx_api_utils.py b/openedx/core/lib/tests/test_edx_api_utils.py index 9031cb2c4e..de7093de55 100644 --- a/openedx/core/lib/tests/test_edx_api_utils.py +++ b/openedx/core/lib/tests/test_edx_api_utils.py @@ -65,7 +65,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach ) with mock.patch('openedx.core.lib.edx_api_utils.EdxRestApiClient.__init__') as mock_init: - actual_collection = get_edx_api_data(catalog_integration, self.user, 'programs', api=api) + actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api) # Verify that the helper function didn't initialize its own client. self.assertFalse(mock_init.called) @@ -96,7 +96,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach self._mock_catalog_api(responses) - actual_collection = get_edx_api_data(catalog_integration, self.user, 'programs', api=api) + actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api) self.assertEqual(actual_collection, expected_collection) self._assert_num_requests(len(expected_collection)) @@ -125,9 +125,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach [httpretty.Response(body=json.dumps(body), content_type='application/json') for body in responses] ) - actual_collection = get_edx_api_data( - catalog_integration, self.user, 'programs', api=api, traverse_pagination=False, - ) + actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api, traverse_pagination=False) self.assertEqual(actual_collection, expected_response) self._assert_num_requests(1) @@ -149,7 +147,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach url=url ) - actual_resource = get_edx_api_data(catalog_integration, self.user, 'programs', api=api, resource_id=resource_id) + actual_resource = get_edx_api_data(catalog_integration, 'programs', api=api, resource_id=resource_id) self.assertEqual(actual_resource, expected_resource) self._assert_num_requests(1) @@ -178,7 +176,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach url=url ) - actual_resource = get_edx_api_data(catalog_integration, self.user, 'programs', api=api, resource_id=resource_id) + actual_resource = get_edx_api_data(catalog_integration, 'programs', api=api, resource_id=resource_id) self.assertEqual(actual_resource, expected_resource) self._assert_num_requests(1) @@ -214,17 +212,15 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach cache_key = CatalogIntegration.current().CACHE_KEY # Warm up the cache. - get_edx_api_data(catalog_integration, self.user, 'programs', api=api, cache_key=cache_key) - get_edx_api_data( - catalog_integration, self.user, 'programs', api=api, resource_id=resource_id, cache_key=cache_key - ) + get_edx_api_data(catalog_integration, 'programs', api=api, cache_key=cache_key) + get_edx_api_data(catalog_integration, 'programs', api=api, resource_id=resource_id, cache_key=cache_key) # Hit the cache. - actual_collection = get_edx_api_data(catalog_integration, self.user, 'programs', api=api, cache_key=cache_key) + actual_collection = get_edx_api_data(catalog_integration, 'programs', api=api, cache_key=cache_key) self.assertEqual(actual_collection, expected_collection) actual_resource = get_edx_api_data( - catalog_integration, self.user, 'programs', api=api, resource_id=resource_id, cache_key=cache_key + catalog_integration, 'programs', api=api, resource_id=resource_id, cache_key=cache_key ) self.assertEqual(actual_resource, expected_resource) @@ -236,24 +232,11 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach """Verify that no data is retrieved if the provided config model is disabled.""" catalog_integration = self.create_catalog_integration(enabled=False) - actual = get_edx_api_data(catalog_integration, self.user, 'programs') + actual = get_edx_api_data(catalog_integration, 'programs', api=None) self.assertTrue(mock_warning.called) self.assertEqual(actual, []) - @mock.patch('edx_rest_api_client.client.EdxRestApiClient.__init__') - @mock.patch(UTILITY_MODULE + '.log.exception') - def test_client_initialization_failure(self, mock_exception, mock_init): - """Verify that an exception is logged when the API client fails to initialize.""" - mock_init.side_effect = Exception - - catalog_integration = self.create_catalog_integration() - - actual = get_edx_api_data(catalog_integration, self.user, 'programs') - - self.assertTrue(mock_exception.called) - self.assertEqual(actual, []) - @mock.patch(UTILITY_MODULE + '.log.exception') def test_data_retrieval_failure(self, mock_exception): """Verify that an exception is logged when data can't be retrieved.""" @@ -264,7 +247,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] ) - actual = get_edx_api_data(catalog_integration, self.user, 'programs', api=api) + actual = get_edx_api_data(catalog_integration, 'programs', api=api) self.assertTrue(mock_exception.called) self.assertEqual(actual, []) @@ -276,33 +259,15 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach actual = get_edx_api_data( catalog_integration, - self.user, 'programs', + api=None, resource_id=100, - many=False, + many=False ) self.assertTrue(mock_warning.called) self.assertEqual(actual, {}) - @mock.patch('edx_rest_api_client.client.EdxRestApiClient.__init__') - @mock.patch(UTILITY_MODULE + '.log.exception') - def test_client_initialization_failure_with_id(self, mock_exception, mock_init): - """Verify that an exception is logged when the API client fails to initialize.""" - mock_init.side_effect = Exception - - catalog_integration = self.create_catalog_integration() - - actual = get_edx_api_data( - catalog_integration, - self.user, - 'programs', - resource_id=100, - many=False, - ) - self.assertTrue(mock_exception.called) - self.assertEqual(actual, {}) - @mock.patch(UTILITY_MODULE + '.log.exception') def test_data_retrieval_failure_with_id(self, mock_exception): """Verify that an exception is logged when data can't be retrieved.""" @@ -315,21 +280,10 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach actual = get_edx_api_data( catalog_integration, - self.user, 'programs', api=api, resource_id=100, - many=False, + many=False ) self.assertTrue(mock_exception.called) self.assertEqual(actual, {}) - - def test_api_client_not_provided(self): - """Verify that an API client doesn't need to be provided.""" - ClientFactory(name=CredentialsApiConfig.OAUTH2_CLIENT_NAME, client_type=CONFIDENTIAL) - - credentials_api_config = self.create_credentials_config() - - with mock.patch('openedx.core.lib.edx_api_utils.EdxRestApiClient.__init__') as mock_init: - get_edx_api_data(credentials_api_config, self.user, 'credentials') - self.assertTrue(mock_init.called)