diff --git a/openedx/core/djangoapps/api_admin/utils.py b/openedx/core/djangoapps/api_admin/utils.py deleted file mode 100644 index 850200a8ca..0000000000 --- a/openedx/core/djangoapps/api_admin/utils.py +++ /dev/null @@ -1,14 +0,0 @@ -""" Course Discovery API Service. """ -from django.conf import settings -from edx_rest_api_client.client import EdxRestApiClient - -from openedx.core.lib.token_utils import JwtBuilder - - -def course_discovery_api_client(user): - """ Returns a Course Discovery API client setup with authentication for the specified user. """ - scopes = ['email', 'profile'] - expires_in = settings.OAUTH_ID_TOKEN_EXPIRATION - jwt = JwtBuilder(user).build_token(scopes, expires_in) - - return EdxRestApiClient(settings.COURSE_CATALOG_API_URL, jwt=jwt) diff --git a/openedx/core/djangoapps/api_admin/views.py b/openedx/core/djangoapps/api_admin/views.py index d03f670417..1dfafeb65d 100644 --- a/openedx/core/djangoapps/api_admin/views.py +++ b/openedx/core/djangoapps/api_admin/views.py @@ -18,7 +18,7 @@ from edxmako.shortcuts import render_to_response from openedx.core.djangoapps.api_admin.decorators import require_api_access from openedx.core.djangoapps.api_admin.forms import ApiAccessRequestForm, CatalogForm from openedx.core.djangoapps.api_admin.models import ApiAccessRequest, Catalog -from openedx.core.djangoapps.api_admin.utils import course_discovery_api_client +from openedx.core.djangoapps.catalog.utils import create_catalog_api_client log = logging.getLogger(__name__) @@ -119,6 +119,11 @@ class ApiTosView(TemplateView): template_name = 'api_admin/terms_of_service.html' +class CatalogApiMixin(object): + def get_catalog_api_client(self, user): + return create_catalog_api_client(user) + + class CatalogSearchView(View): """View to search for catalogs belonging to a user.""" @@ -135,7 +140,7 @@ class CatalogSearchView(View): return redirect(reverse('api_admin:catalog-list', kwargs={'username': username})) -class CatalogListView(View): +class CatalogListView(CatalogApiMixin, View): """View to list existing catalogs and create new ones.""" template = 'api_admin/catalogs/list.html' @@ -162,14 +167,14 @@ class CatalogListView(View): def get(self, request, username): """Display a list of a user's catalogs.""" - client = course_discovery_api_client(request.user) + client = self.get_catalog_api_client(request.user) form = CatalogForm(initial={'viewers': [username]}) return render_to_response(self.template, self.get_context_data(client, username, form)) def post(self, request, username): """Create a new catalog for a user.""" form = CatalogForm(request.POST) - client = course_discovery_api_client(request.user) + client = self.get_catalog_api_client(request.user) if not form.is_valid(): return render_to_response(self.template, self.get_context_data(client, username, form), status=400) @@ -178,7 +183,7 @@ class CatalogListView(View): return redirect(reverse('api_admin:catalog-edit', kwargs={'catalog_id': catalog['id']})) -class CatalogEditView(View): +class CatalogEditView(CatalogApiMixin, View): """View to edit an individual catalog.""" template_name = 'api_admin/catalogs/edit.html' @@ -196,7 +201,7 @@ class CatalogEditView(View): def get(self, request, catalog_id): """Display a form to edit this catalog.""" - client = course_discovery_api_client(request.user) + client = self.get_catalog_api_client(request.user) response = client.catalogs(catalog_id).get() catalog = Catalog(attributes=response) form = CatalogForm(instance=catalog) @@ -204,7 +209,7 @@ class CatalogEditView(View): def post(self, request, catalog_id): """Update or delete this catalog.""" - client = course_discovery_api_client(request.user) + client = self.get_catalog_api_client(request.user) if request.POST.get('delete-catalog') == 'on': client.catalogs(catalog_id).delete() return redirect(reverse('api_admin:catalog-search')) @@ -217,7 +222,7 @@ class CatalogEditView(View): return redirect(reverse('api_admin:catalog-edit', kwargs={'catalog_id': catalog['id']})) -class CatalogPreviewView(View): +class CatalogPreviewView(CatalogApiMixin, View): """Endpoint to preview courses for a query.""" def get(self, request): @@ -225,7 +230,7 @@ class CatalogPreviewView(View): Return the results of a query against the course catalog API. If no query parameter is given, returns an empty result set. """ - client = course_discovery_api_client(request.user) + client = self.get_catalog_api_client(request.user) # Just pass along the request params including limit/offset pagination if 'q' in request.GET: results = client.courses.get(**request.GET) diff --git a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py index 74e457d816..0a25d3054b 100644 --- a/openedx/core/djangoapps/catalog/management/commands/cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/cache_programs.py @@ -30,7 +30,7 @@ class Command(BaseCommand): try: user = User.objects.get(username=username) - client = create_catalog_api_client(user, catalog_integration) + client = create_catalog_api_client(user) except User.DoesNotExist: logger.error( 'Failed to create API client. Service user {username} does not exist.'.format(username) diff --git a/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py b/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py index de8c15149d..e67b902627 100644 --- a/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py +++ b/openedx/core/djangoapps/catalog/management/commands/tests/test_cache_programs.py @@ -21,7 +21,7 @@ class TestCachePrograms(CatalogIntegrationMixin, CacheIsolationTestCase): self.catalog_integration = self.create_catalog_integration() - self.list_url = self.catalog_integration.internal_api_url.rstrip('/') + '/programs/' + self.list_url = self.catalog_integration.get_internal_api_url().rstrip('/') + '/programs/' self.detail_tpl = self.list_url.rstrip('/') + '/{uuid}/' self.programs = ProgramFactory.create_batch(3) diff --git a/openedx/core/djangoapps/catalog/migrations/0004_auto_20170616_0618.py b/openedx/core/djangoapps/catalog/migrations/0004_auto_20170616_0618.py new file mode 100644 index 0000000000..762ca77f8b --- /dev/null +++ b/openedx/core/djangoapps/catalog/migrations/0004_auto_20170616_0618.py @@ -0,0 +1,19 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ('catalog', '0003_catalogintegration_page_size'), + ] + + operations = [ + migrations.AlterField( + model_name='catalogintegration', + name='internal_api_url', + field=models.URLField(help_text='DEPRECATED: Use the setting COURSE_CATALOG_API_URL.', verbose_name='Internal API URL'), + ), + ] diff --git a/openedx/core/djangoapps/catalog/models.py b/openedx/core/djangoapps/catalog/models.py index d916f41188..05d2e50e1f 100644 --- a/openedx/core/djangoapps/catalog/models.py +++ b/openedx/core/djangoapps/catalog/models.py @@ -1,18 +1,23 @@ """Models governing integration with the catalog service.""" from config_models.models import ConfigurationModel +from django.conf import settings +from django.contrib.auth import get_user_model from django.db import models from django.utils.translation import ugettext_lazy as _ +from openedx.core.djangoapps.site_configuration import helpers + class CatalogIntegration(ConfigurationModel): """Manages configuration for connecting to the catalog service and using its API.""" API_NAME = 'catalog' CACHE_KEY = 'catalog.api.data' + # TODO Replace all usages of this field with a call to get_internal_api_url(). internal_api_url = models.URLField( verbose_name=_('Internal API URL'), help_text=_( - 'API root to be used for server-to-server requests (e.g., https://catalog-internal.example.com/api/v1/).' + 'DEPRECATED: Use the setting COURSE_CATALOG_API_URL.' ) ) @@ -47,5 +52,12 @@ class CatalogIntegration(ConfigurationModel): """Whether responses from the catalog API will be cached.""" return self.cache_ttl > 0 - def __unicode__(self): - return self.internal_api_url + def get_internal_api_url(self): + """ Returns the internal Catalog API URL associated with the request's site. """ + return helpers.get_value('COURSE_CATALOG_API_URL', settings.COURSE_CATALOG_API_URL) + + def get_service_user(self): + # NOTE: We load the user model here to avoid issues at startup time that result from the hacks + # in lms/startup.py. + User = get_user_model() # pylint: disable=invalid-name + return User.objects.get(username=self.service_username) diff --git a/openedx/core/djangoapps/catalog/tests/test_models.py b/openedx/core/djangoapps/catalog/tests/test_models.py index 1854e1d5f8..32b19cf1db 100644 --- a/openedx/core/djangoapps/catalog/tests/test_models.py +++ b/openedx/core/djangoapps/catalog/tests/test_models.py @@ -1,9 +1,12 @@ """Catalog model tests.""" import ddt -from django.test import TestCase import mock +from django.test import TestCase, override_settings from openedx.core.djangoapps.catalog.tests import mixins +from openedx.core.djangoapps.site_configuration.tests.test_util import with_site_configuration + +COURSE_CATALOG_API_URL = 'https://api.example.com/v1/' @ddt.ddt @@ -12,6 +15,11 @@ from openedx.core.djangoapps.catalog.tests import mixins class TestCatalogIntegration(mixins.CatalogIntegrationMixin, TestCase): """Tests covering the CatalogIntegration model.""" + def assert_get_internal_api_url_value(self, expected): + """ Asserts the value of get_internal_api_url matches the expected value. """ + catalog_integration = self.create_catalog_integration() + self.assertEqual(catalog_integration.get_internal_api_url(), expected) + @ddt.data( (0, False), (1, True), @@ -21,3 +29,24 @@ class TestCatalogIntegration(mixins.CatalogIntegrationMixin, TestCase): """Test the behavior of the property controlling whether API responses are cached.""" catalog_integration = self.create_catalog_integration(cache_ttl=cache_ttl) self.assertEqual(catalog_integration.is_cache_enabled, is_cache_enabled) + + @override_settings(COURSE_CATALOG_API_URL=COURSE_CATALOG_API_URL) + def test_get_internal_api_url(self, _mock_cache): + """ Requests made without a microsite should return the value from settings. """ + self.assert_get_internal_api_url_value(COURSE_CATALOG_API_URL) + catalog_integration = self.create_catalog_integration() + self.assertEqual(catalog_integration.get_internal_api_url(), COURSE_CATALOG_API_URL) + + @override_settings(COURSE_CATALOG_API_URL=COURSE_CATALOG_API_URL) + @with_site_configuration(configuration={}) + def test_get_internal_api_url_without_microsite_override(self, _mock_cache): + """ Requests made to microsites that do not have COURSE_CATALOG_API_URL overridden should + return the default value from settings. """ + self.assert_get_internal_api_url_value(COURSE_CATALOG_API_URL) + + @override_settings(COURSE_CATALOG_API_URL=COURSE_CATALOG_API_URL) + @with_site_configuration(configuration={'COURSE_CATALOG_API_URL': 'foo'}) + def test_get_internal_api_url_with_microsite_override(self, _mock_cache): + """ If a microsite has overridden the value of COURSE_CATALOG_API_URL, the overridden + value should be returned. """ + self.assert_get_internal_api_url_value('foo') diff --git a/openedx/core/djangoapps/catalog/tests/test_utils.py b/openedx/core/djangoapps/catalog/tests/test_utils.py index a1864a72db..a27bca56e2 100644 --- a/openedx/core/djangoapps/catalog/tests/test_utils.py +++ b/openedx/core/djangoapps/catalog/tests/test_utils.py @@ -7,7 +7,7 @@ import ddt import mock from django.contrib.auth import get_user_model from django.core.cache import cache -from django.test import TestCase +from django.test import TestCase, override_settings from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL, PROGRAM_UUIDS_CACHE_KEY from openedx.core.djangoapps.catalog.models import CatalogIntegration @@ -209,6 +209,7 @@ class TestGetProgramsWithType(TestCase): @mock.patch(UTILS_MODULE + '.get_edx_api_data') class TestGetProgramTypes(CatalogIntegrationMixin, TestCase): """Tests covering retrieval of program types from the catalog service.""" + @override_settings(COURSE_CATALOG_API_URL='https://api.example.com/v1/') def test_get_program_types(self, mock_get_edx_api_data): """Verify get_program_types returns the expected list of program types.""" program_types = ProgramTypeFactory.create_batch(3) @@ -249,7 +250,7 @@ class TestGetCourseRuns(CatalogIntegrationMixin, TestCase): 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 + self.assertEqual(kwargs['api']._store['base_url'], self.catalog_integration.get_internal_api_url()) # pylint: disable=protected-access querystring = { 'page_size': 20, diff --git a/openedx/core/djangoapps/catalog/utils.py b/openedx/core/djangoapps/catalog/utils.py index 07d6720f89..32c015c541 100644 --- a/openedx/core/djangoapps/catalog/utils.py +++ b/openedx/core/djangoapps/catalog/utils.py @@ -3,8 +3,8 @@ import copy import logging from django.conf import settings -from django.contrib.auth import get_user_model from django.core.cache import cache +from django.core.exceptions import ObjectDoesNotExist from edx_rest_api_client.client import EdxRestApiClient from openedx.core.djangoapps.catalog.cache import PROGRAM_CACHE_KEY_TPL, PROGRAM_UUIDS_CACHE_KEY @@ -13,16 +13,16 @@ from openedx.core.lib.edx_api_utils import get_edx_api_data from openedx.core.lib.token_utils import JwtBuilder logger = logging.getLogger(__name__) -User = get_user_model() # pylint: disable=invalid-name -def create_catalog_api_client(user, catalog_integration): - """Returns an API client which can be used to make catalog API requests.""" +def create_catalog_api_client(user): + """Returns an API client which can be used to make Catalog API requests.""" scopes = ['email', 'profile'] expires_in = settings.OAUTH_ID_TOKEN_EXPIRATION jwt = JwtBuilder(user).build_token(scopes, expires_in) - return EdxRestApiClient(catalog_integration.internal_api_url, jwt=jwt) + url = CatalogIntegration.current().get_internal_api_url() + return EdxRestApiClient(url, jwt=jwt) def get_programs(uuid=None): @@ -90,11 +90,11 @@ def get_program_types(name=None): catalog_integration = CatalogIntegration.current() if catalog_integration.enabled: try: - user = User.objects.get(username=catalog_integration.service_username) - except User.DoesNotExist: + user = catalog_integration.get_service_user() + except ObjectDoesNotExist: return [] - api = create_catalog_api_client(user, catalog_integration) + api = create_catalog_api_client(user) cache_key = '{base}.program_types'.format(base=catalog_integration.CACHE_KEY) data = get_edx_api_data(catalog_integration, 'program_types', api=api, @@ -161,15 +161,15 @@ def get_course_runs(): course_runs = [] if catalog_integration.enabled: try: - user = User.objects.get(username=catalog_integration.service_username) - except User.DoesNotExist: + user = catalog_integration.get_service_user() + except ObjectDoesNotExist: logger.error( 'Catalog service user with username [%s] does not exist. Course runs will not be retrieved.', catalog_integration.service_username, ) return course_runs - api = create_catalog_api_client(user, catalog_integration) + api = create_catalog_api_client(user) querystring = { 'page_size': catalog_integration.page_size, diff --git a/openedx/core/lib/tests/test_edx_api_utils.py b/openedx/core/lib/tests/test_edx_api_utils.py index 99fa215ca3..9434fda582 100644 --- a/openedx/core/lib/tests/test_edx_api_utils.py +++ b/openedx/core/lib/tests/test_edx_api_utils.py @@ -40,7 +40,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def _mock_catalog_api(self, responses, url=None): self.assertTrue(httpretty.is_enabled(), msg='httpretty must be enabled to mock Catalog API calls.') - url = url if url else CatalogIntegration.current().internal_api_url.strip('/') + '/programs/' + url = url if url else CatalogIntegration.current().get_internal_api_url().strip('/') + '/programs/' httpretty.register_uri(httpretty.GET, url, responses=responses) @@ -51,7 +51,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def test_get_unpaginated_data(self): """Verify that unpaginated data can be retrieved.""" catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) expected_collection = ['some', 'test', 'data'] data = { @@ -76,10 +76,10 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def test_get_paginated_data(self): """Verify that paginated data can be retrieved.""" catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) expected_collection = ['some', 'test', 'data'] - url = CatalogIntegration.current().internal_api_url.strip('/') + '/programs/?page={}' + url = CatalogIntegration.current().get_internal_api_url().strip('/') + '/programs/?page={}' responses = [] for page, record in enumerate(expected_collection, start=1): @@ -105,9 +105,9 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach Verify that pagination is not traversed if traverse_pagination=False is passed as argument. """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) - url = CatalogIntegration.current().internal_api_url.strip('/') + '/programs/?page={}' + url = CatalogIntegration.current().get_internal_api_url().strip('/') + '/programs/?page={}' responses = [ { 'next': url.format(2), @@ -131,11 +131,11 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def test_get_specific_resource(self): """Verify that a specific resource can be retrieved.""" catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) resource_id = 1 url = '{api_root}/programs/{resource_id}/'.format( - api_root=CatalogIntegration.current().internal_api_url.strip('/'), + api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), resource_id=resource_id, ) @@ -160,11 +160,11 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach return the value of that "results" key. """ catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) resource_id = 0 url = '{api_root}/programs/{resource_id}/'.format( - api_root=CatalogIntegration.current().internal_api_url.strip('/'), + api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), resource_id=resource_id, ) @@ -183,7 +183,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def test_cache_utilization(self): """Verify that when enabled, the cache is used.""" catalog_integration = self.create_catalog_integration(cache_ttl=5) - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) expected_collection = ['some', 'test', 'data'] data = { @@ -197,7 +197,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach resource_id = 1 url = '{api_root}/programs/{resource_id}/'.format( - api_root=CatalogIntegration.current().internal_api_url.strip('/'), + api_root=CatalogIntegration.current().get_internal_api_url().strip('/'), resource_id=resource_id, ) @@ -240,7 +240,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def test_data_retrieval_failure(self, mock_exception): """Verify that an exception is logged when data can't be retrieved.""" catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) self._mock_catalog_api( [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] @@ -271,7 +271,7 @@ class TestGetEdxApiData(CatalogIntegrationMixin, CredentialsApiConfigMixin, Cach def test_data_retrieval_failure_with_id(self, mock_exception): """Verify that an exception is logged when data can't be retrieved.""" catalog_integration = self.create_catalog_integration() - api = create_catalog_api_client(self.user, catalog_integration) + api = create_catalog_api_client(self.user) self._mock_catalog_api( [httpretty.Response(body='clunk', content_type='application/json', status_code=500)] diff --git a/openedx/features/enterprise_support/api.py b/openedx/features/enterprise_support/api.py index 592c1aa6f0..f054656fc1 100644 --- a/openedx/features/enterprise_support/api.py +++ b/openedx/features/enterprise_support/api.py @@ -18,8 +18,8 @@ from edx_rest_api_client.client import EdxRestApiClient from requests.exceptions import ConnectionError, Timeout from slumber.exceptions import HttpClientError, HttpServerError, SlumberBaseException -from openedx.core.djangoapps.api_admin.utils import course_discovery_api_client from openedx.core.djangoapps.catalog.models import CatalogIntegration +from openedx.core.djangoapps.catalog.utils import create_catalog_api_client from openedx.core.djangoapps.site_configuration import helpers as configuration_helpers from openedx.core.lib.token_utils import JwtBuilder @@ -31,7 +31,6 @@ try: except ImportError: pass - CONSENT_FAILED_PARAMETER = 'consent_failed' LOGGER = logging.getLogger("edx.enterprise_helpers") @@ -201,6 +200,7 @@ def data_sharing_consent_required(view_func): After granting consent, the user will be redirected back to the original request.path. """ + @wraps(view_func) def inner(request, course_id, *args, **kwargs): """ @@ -462,7 +462,7 @@ def is_course_in_enterprise_catalog(site, course_id, enterprise_catalog_id): try: # GET: /api/v1/catalogs/{catalog_id}/contains?course_run_id={course_run_ids} - response = course_discovery_api_client(user=user).catalogs(enterprise_catalog_id).contains.get( + response = create_catalog_api_client(user=user).catalogs(enterprise_catalog_id).contains.get( course_run_id=course_id ) cache.set(cache_key, response, settings.COURSES_API_CACHE_TIMEOUT)