diff --git a/lms/envs/common.py b/lms/envs/common.py index 122143deeb..549ea11d62 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -2882,3 +2882,5 @@ THEME_CACHE_TIMEOUT = 30 * 60 # API access management API_ACCESS_MANAGER_EMAIL = 'api-access@example.com' API_ACCESS_FROM_EMAIL = 'api-requests@example.com' +API_DOCUMENTATION_URL = 'http://edx.readthedocs.org/projects/edx-platform-api/en/latest/overview.html' +AUTH_DOCUMENTATION_URL = 'http://edx.readthedocs.org/projects/edx-platform-api/en/latest/authentication.html' diff --git a/lms/templates/api_admin/api_access_request_email_approved.txt b/lms/templates/api_admin/api_access_request_email_approved.txt new file mode 100644 index 0000000000..3781c08d3c --- /dev/null +++ b/lms/templates/api_admin/api_access_request_email_approved.txt @@ -0,0 +1,12 @@ +## mako +Dear ${name}, + +Your request for access to the ${platform_name} Course Catalog API has been approved. Sign in at ${api_management_url} using the same account you used to request API access to generate your API client credentials. + +Notes: +- ${platform_name} recommends that you bookmark the ${api_management_url} page for easy access. +- For more information about how to use the API client credentials to authenticate your API request, see the OAuth 2.0 documentation at ${authentication_docs_url}. +- For questions about using this API, see the documentation at ${api_docs_url} or contact ${support_email_address}. + +Thanks! +The ${platform_name} API team diff --git a/lms/templates/api_admin/api_access_request_email_denied.txt b/lms/templates/api_admin/api_access_request_email_denied.txt new file mode 100644 index 0000000000..caaf887ec4 --- /dev/null +++ b/lms/templates/api_admin/api_access_request_email_denied.txt @@ -0,0 +1,6 @@ +## mako +Dear ${name}, + +Your request for access to the ${platform_name} Course Catalog API has been denied. If you think this is an error, or for other questions about using this API, contact ${support_email_address}. + +- The ${platform_name} API team diff --git a/lms/templates/api_admin/api_access_request_email_new_request.txt b/lms/templates/api_admin/api_access_request_email_new_request.txt new file mode 100644 index 0000000000..ade841a7f3 --- /dev/null +++ b/lms/templates/api_admin/api_access_request_email_new_request.txt @@ -0,0 +1,8 @@ +## mako +We have received the following request to use the Course Catalog API. Go to ${approval_url} to approve the user. + +Company name: ${api_request.company_name} +Company contact: ${api_request.user.username} +Company URL: ${api_request.website} +Address: ${api_request.company_address} +Reason for API usage: ${api_request.reason} diff --git a/lms/templates/api_admin/email.txt b/lms/templates/api_admin/email.txt deleted file mode 100644 index 1d665edd82..0000000000 --- a/lms/templates/api_admin/email.txt +++ /dev/null @@ -1,8 +0,0 @@ -## mako -We have received the following request to use the Course Discovery API. Please go to ${approval_url} to approve the user. - -Company name: ${company_name} -Company contact: ${username} -Company URL: ${url} -Address: ${company_address} -Reason for API usage: ${reason} diff --git a/lms/templates/api_admin/terms_of_service.html b/lms/templates/api_admin/terms_of_service.html index f15e9d922a..2054d6ad7e 100644 --- a/lms/templates/api_admin/terms_of_service.html +++ b/lms/templates/api_admin/terms_of_service.html @@ -14,7 +14,7 @@ from django.utils.translation import ugettext as _

${_("API Access")}

-

${_("To access the APIs, you will need to create an {platform_name} user account for your application (not for personal use). This account will provide you with access to our API request page at {request_url}. On that page, you must complete the API request form including a description of your proposed uses for the APIs. Any account and registration information that you provide to {platform_name} must be accurate and up to date, and you agree to inform us promptly of any changes. {platform_name} will review your API request form and, upon approval in {platform_name}'s sole discretion, will provide you with instructions for obtaining your API shared secret and client ID.").format(platform_name=settings.PLATFORM_NAME, request_url=reverse('api-request'))}

+

${_("To access the APIs, you will need to create an {platform_name} user account for your application (not for personal use). This account will provide you with access to our API request page at {request_url}. On that page, you must complete the API request form including a description of your proposed uses for the APIs. Any account and registration information that you provide to {platform_name} must be accurate and up to date, and you agree to inform us promptly of any changes. {platform_name} will review your API request form and, upon approval in {platform_name}'s sole discretion, will provide you with instructions for obtaining your API shared secret and client ID.").format(platform_name=settings.PLATFORM_NAME, request_url=reverse('api_admin:api-request'))}

${_("Permissible Use")}

diff --git a/lms/urls.py b/lms/urls.py index 2befc3f94f..a4e18e3433 100644 --- a/lms/urls.py +++ b/lms/urls.py @@ -110,6 +110,9 @@ urlpatterns = ( # TODO Namespace these! url(r'^course_modes/', include('course_modes.urls')), url(r'^verify_student/', include('verify_student.urls')), + + # URLs for API access management + url(r'^api-admin/', include('openedx.core.djangoapps.api_admin.urls', namespace='api_admin')), ) urlpatterns += ( @@ -1000,8 +1003,3 @@ if settings.FEATURES.get('ENABLE_FINANCIAL_ASSISTANCE_FORM'): name='submit_financial_assistance_request' ) ) - -# URLs for API access management -urlpatterns += ( - url(r'^api-admin/', include('openedx.core.djangoapps.api_admin.urls')), -) diff --git a/openedx/core/djangoapps/api_admin/admin.py b/openedx/core/djangoapps/api_admin/admin.py index b961613331..d6018f7bd8 100644 --- a/openedx/core/djangoapps/api_admin/admin.py +++ b/openedx/core/djangoapps/api_admin/admin.py @@ -12,6 +12,8 @@ class ApiAccessRequestAdmin(admin.ModelAdmin): list_filter = ('status',) search_fields = ('user__email',) raw_id_fields = ('user',) + readonly_fields = ('user', 'website', 'reason', 'company_name', 'company_address', 'contacted', ) + exclude = ('site',) admin.site.register(ApiAccessConfig, ConfigurationModelAdmin) diff --git a/openedx/core/djangoapps/api_admin/migrations/0004_auto_20160412_1506.py b/openedx/core/djangoapps/api_admin/migrations/0004_auto_20160412_1506.py new file mode 100644 index 0000000000..6a9365fa48 --- /dev/null +++ b/openedx/core/djangoapps/api_admin/migrations/0004_auto_20160412_1506.py @@ -0,0 +1,37 @@ +# -*- coding: utf-8 -*- +from __future__ import unicode_literals + +from django.db import migrations, models +import django.db.models.deletion + + +class Migration(migrations.Migration): + + dependencies = [ + ('sites', '0001_initial'), + ('api_admin', '0003_auto_20160404_1618'), + ] + + operations = [ + migrations.AddField( + model_name='apiaccessrequest', + name='contacted', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='apiaccessrequest', + name='site', + field=models.ForeignKey(default=1, to='sites.Site'), + preserve_default=False, + ), + migrations.AddField( + model_name='historicalapiaccessrequest', + name='contacted', + field=models.BooleanField(default=False), + ), + migrations.AddField( + model_name='historicalapiaccessrequest', + name='site', + field=models.ForeignKey(related_name='+', on_delete=django.db.models.deletion.DO_NOTHING, db_constraint=False, blank=True, to='sites.Site', null=True), + ), + ] diff --git a/openedx/core/djangoapps/api_admin/models.py b/openedx/core/djangoapps/api_admin/models.py index ac2454e80e..5133c4f5b5 100644 --- a/openedx/core/djangoapps/api_admin/models.py +++ b/openedx/core/djangoapps/api_admin/models.py @@ -1,10 +1,19 @@ """Models for API management.""" import logging +from smtplib import SMTPException +from urlparse import urlunsplit +from django.conf import settings from django.contrib.auth.models import User +from django.contrib.sites.models import Site +from django.core.mail import send_mail +from django.core.urlresolvers import reverse from django.db import models +from django.db.models.signals import post_save, pre_save +from django.dispatch import receiver from django.utils.translation import ugettext as _ from django_extensions.db.models import TimeStampedModel +from edxmako.shortcuts import render_to_string from simple_history.models import HistoricalRecords from config_models.models import ConfigurationModel @@ -35,6 +44,8 @@ class ApiAccessRequest(TimeStampedModel): reason = models.TextField(help_text=_('The reason this user wants to access the API.')) company_name = models.CharField(max_length=255, default='') company_address = models.CharField(max_length=255, default='') + site = models.ForeignKey(Site) + contacted = models.BooleanField(default=False) history = HistoricalRecords() @@ -88,3 +99,83 @@ class ApiAccessConfig(ConfigurationModel): def __unicode__(self): return u'ApiAccessConfig [enabled={}]'.format(self.enabled) + + +@receiver(post_save, sender=ApiAccessRequest, dispatch_uid="api_access_request_post_save_email") +def send_request_email(sender, instance, created, **kwargs): # pylint: disable=unused-argument + """ Send request email after new record created. """ + if created: + _send_new_pending_email(instance) + + +@receiver(pre_save, sender=ApiAccessRequest, dispatch_uid="api_access_request_pre_save_email") +def send_decision_email(sender, instance, **kwargs): # pylint: disable=unused-argument + """ Send decision email after status changed. """ + if instance.id and not instance.contacted: + old_instance = ApiAccessRequest.objects.get(pk=instance.id) + if instance.status != old_instance.status: + _send_decision_email(instance) + + +def _send_new_pending_email(instance): + """ Send an email to settings.API_ACCESS_MANAGER_EMAIL with the contents of this API access request. """ + context = { + 'approval_url': urlunsplit( + ( + 'https' if settings.HTTPS == 'on' else 'http', + instance.site.domain, + reverse('admin:api_admin_apiaccessrequest_change', args=(instance.id,)), + '', + '', + ) + ), + 'api_request': instance + } + + message = render_to_string('api_admin/api_access_request_email_new_request.txt', context) + try: + send_mail( + _('API access request from {company}').format(company=instance.company_name), + message, + settings.API_ACCESS_FROM_EMAIL, + [settings.API_ACCESS_MANAGER_EMAIL], + fail_silently=False + ) + except SMTPException: + log.exception('Error sending API user notification email for request [%s].', instance.id) + + +def _send_decision_email(instance): + """ Send an email to requesting user with the decision made about their request. """ + context = { + 'name': instance.user.username, + 'api_management_url': urlunsplit( + ( + 'https' if settings.HTTPS == 'on' else 'http', + instance.site.domain, + reverse('api_admin:api-status'), + '', + '', + ) + ), + 'authentication_docs_url': settings.AUTH_DOCUMENTATION_URL, + 'api_docs_url': settings.API_DOCUMENTATION_URL, + 'support_email_address': settings.API_ACCESS_FROM_EMAIL, + 'platform_name': settings.PLATFORM_NAME + } + + message = render_to_string( + 'api_admin/api_access_request_email_{status}.txt'.format(status=instance.status), + context + ) + try: + send_mail( + _('API access request'), + message, + settings.API_ACCESS_FROM_EMAIL, + [instance.user.email], + fail_silently=False + ) + instance.contacted = True + except SMTPException: + log.exception('Error sending API user notification email for request [%s].', instance.id) diff --git a/openedx/core/djangoapps/api_admin/tests/factories.py b/openedx/core/djangoapps/api_admin/tests/factories.py index 942424a91b..d421b6de91 100644 --- a/openedx/core/djangoapps/api_admin/tests/factories.py +++ b/openedx/core/djangoapps/api_admin/tests/factories.py @@ -2,6 +2,7 @@ import factory from factory.django import DjangoModelFactory +from microsite_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.api_admin.models import ApiAccessRequest from student.tests.factories import UserFactory @@ -12,3 +13,4 @@ class ApiAccessRequestFactory(DjangoModelFactory): model = ApiAccessRequest user = factory.SubFactory(UserFactory) + site = factory.SubFactory(SiteFactory) diff --git a/openedx/core/djangoapps/api_admin/tests/test_models.py b/openedx/core/djangoapps/api_admin/tests/test_models.py index 67d2a9c607..9337d26c0e 100644 --- a/openedx/core/djangoapps/api_admin/tests/test_models.py +++ b/openedx/core/djangoapps/api_admin/tests/test_models.py @@ -1,14 +1,22 @@ # pylint: disable=missing-docstring +from smtplib import SMTPException + import ddt +from django.conf import settings from django.db import IntegrityError from django.test import TestCase +import mock +import unittest +from microsite_configuration.tests.factories import SiteFactory from openedx.core.djangoapps.api_admin.models import ApiAccessRequest, ApiAccessConfig +from openedx.core.djangoapps.api_admin.models import log as model_log from openedx.core.djangoapps.api_admin.tests.factories import ApiAccessRequestFactory from student.tests.factories import UserFactory @ddt.ddt +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') class ApiAccessRequestTests(TestCase): def setUp(self): @@ -69,3 +77,71 @@ class ApiAccessConfigTests(TestCase): unicode(ApiAccessConfig(enabled=False)), u'ApiAccessConfig [enabled=False]' ) + + +@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Test only valid in lms') +class ApiAccessRequestSignalTests(TestCase): + def setUp(self): + super(ApiAccessRequestSignalTests, self).setUp() + self.user = UserFactory() + self.api_access_request = ApiAccessRequest(user=self.user, site=SiteFactory()) + self.send_new_pending_email_function = 'openedx.core.djangoapps.api_admin.models._send_new_pending_email' + self.send_decision_email_function = 'openedx.core.djangoapps.api_admin.models._send_decision_email' + + def test_save_signal_success_new_email(self): + """ Verify that initial save sends new email and no decision email. """ + with mock.patch(self.send_new_pending_email_function) as mock_new_email: + with mock.patch(self.send_decision_email_function) as mock_decision_email: + self.api_access_request.save() + + mock_new_email.assert_called_once_with(self.api_access_request) + self.assertFalse(mock_decision_email.called) + + def test_save_signal_success_decision_email(self): + """ Verify that updating request status sends decision email and no new email. """ + self.api_access_request.save() + + with mock.patch(self.send_new_pending_email_function) as mock_new_email: + with mock.patch(self.send_decision_email_function) as mock_decision_email: + self.api_access_request.approve() + + mock_decision_email.assert_called_once_with(self.api_access_request) + self.assertFalse(mock_new_email.called) + + def test_save_signal_success_no_emails(self): + """ Verify that updating request status again sends no emails. """ + self.api_access_request.save() + self.api_access_request.approve() + + with mock.patch(self.send_new_pending_email_function) as mock_new_email: + with mock.patch(self.send_decision_email_function) as mock_decision_email: + self.api_access_request.deny() + + self.assertFalse(mock_decision_email.called) + self.assertFalse(mock_new_email.called) + + def test_save_signal_failure_email(self): + """ Verify that saving still functions even on email errors. """ + self.assertIsNone(self.api_access_request.id) + + mail_function = 'openedx.core.djangoapps.api_admin.models.send_mail' + with mock.patch(mail_function, side_effect=SMTPException): + with mock.patch.object(model_log, 'exception') as mock_model_log_exception: + self.api_access_request.save() + + # Verify that initial save logs email errors properly + mock_model_log_exception.assert_called_once_with( + 'Error sending API user notification email for request [%s].', self.api_access_request.id + ) + # Verify object saved + self.assertIsNotNone(self.api_access_request.id) + + with mock.patch(mail_function, side_effect=SMTPException): + with mock.patch.object(model_log, 'exception') as mock_model_log_exception: + self.api_access_request.approve() + # Verify that updating request status logs email errors properly + mock_model_log_exception.assert_called_once_with( + 'Error sending API user notification email for request [%s].', self.api_access_request.id + ) + # Verify object saved + self.assertEqual(self.api_access_request.status, ApiAccessRequest.APPROVED) diff --git a/openedx/core/djangoapps/api_admin/tests/test_views.py b/openedx/core/djangoapps/api_admin/tests/test_views.py index ca8c1665ab..d85ce4fca4 100644 --- a/openedx/core/djangoapps/api_admin/tests/test_views.py +++ b/openedx/core/djangoapps/api_admin/tests/test_views.py @@ -1,16 +1,13 @@ #pylint: disable=missing-docstring -from smtplib import SMTPException import unittest from django.conf import settings from django.core.urlresolvers import reverse from django.test import TestCase -import mock from openedx.core.djangoapps.api_admin.models import ApiAccessRequest, ApiAccessConfig from openedx.core.djangoapps.api_admin.tests.factories import ApiAccessRequestFactory from openedx.core.djangoapps.api_admin.tests.utils import VALID_DATA -from openedx.core.djangoapps.api_admin.views import log as view_log from student.tests.factories import UserFactory @@ -26,7 +23,7 @@ class ApiRequestViewTest(ApiAdminTest): def setUp(self): super(ApiRequestViewTest, self).setUp() - self.url = reverse('api-request') + self.url = reverse('api_admin:api-request') password = 'abc123' self.user = UserFactory(password=password) self.client.login(username=self.user.username, password=password) @@ -49,14 +46,14 @@ class ApiRequestViewTest(ApiAdminTest): """ ApiAccessRequestFactory(user=self.user) response = self.client.get(self.url) - self.assertRedirects(response, reverse('api-status')) + self.assertRedirects(response, reverse('api_admin:api-status')) def _assert_post_success(self, response): """ Assert that a successful POST has been made, that the response redirects correctly, and that the correct object has been created. """ - self.assertRedirects(response, reverse('api-status')) + self.assertRedirects(response, reverse('api_admin:api-status')) api_request = ApiAccessRequest.objects.get(user=self.user) self.assertEqual(api_request.status, ApiAccessRequest.PENDING) return api_request @@ -64,38 +61,14 @@ class ApiRequestViewTest(ApiAdminTest): def test_post_valid(self): """Verify that a logged-in user can create an API request.""" self.assertFalse(ApiAccessRequest.objects.all().exists()) - with mock.patch('openedx.core.djangoapps.api_admin.views.send_mail') as mock_send_mail: - response = self.client.post(self.url, VALID_DATA) - mock_send_mail.assert_called_once_with( - 'API access request from ' + VALID_DATA['company_name'], - mock.ANY, - settings.API_ACCESS_FROM_EMAIL, - [settings.API_ACCESS_MANAGER_EMAIL], - fail_silently=False - ) + response = self.client.post(self.url, VALID_DATA) self._assert_post_success(response) - def test_failed_email(self): - """ - Verify that an access request is still created if sending email - fails for some reason, and that the necessary information is - logged. - """ - mail_function = 'openedx.core.djangoapps.api_admin.views.send_mail' - with mock.patch(mail_function, side_effect=SMTPException): - with mock.patch.object(view_log, 'exception') as mock_view_log_exception: - response = self.client.post(self.url, VALID_DATA) - api_request = self._assert_post_success(response) - mock_view_log_exception.assert_called_once_with( - 'Error sending API request email for request [%s].', api_request.id # pylint: disable=no-member - ) - def test_post_anonymous(self): """Verify that users must be logged in to create an access request.""" self.client.logout() - with mock.patch('openedx.core.djangoapps.api_admin.views.send_mail') as mock_send_mail: - response = self.client.post(self.url, VALID_DATA) - mock_send_mail.assert_not_called() + response = self.client.post(self.url, VALID_DATA) + self.assertEqual(response.status_code, 302) self.assertFalse(ApiAccessRequest.objects.all().exists()) @@ -120,7 +93,7 @@ class ApiRequestStatusViewTest(ApiAdminTest): password = 'abc123' self.user = UserFactory(password=password) self.client.login(username=self.user.username, password=password) - self.url = reverse('api-status') + self.url = reverse('api_admin:api-status') def test_get_without_request(self): """ @@ -128,7 +101,7 @@ class ApiRequestStatusViewTest(ApiAdminTest): redirected to the API request form. """ response = self.client.get(self.url) - self.assertRedirects(response, reverse('api-request')) + self.assertRedirects(response, reverse('api_admin:api-request')) def test_get_with_request(self): """ @@ -157,7 +130,7 @@ class ApiTosViewTest(ApiAdminTest): def test_get_api_tos(self): """Verify that the terms of service can be read.""" - url = reverse('api-tos') + url = reverse('api_admin:api-tos') response = self.client.get(url) self.assertEqual(response.status_code, 200) self.assertIn('Terms of Service', response.content) diff --git a/openedx/core/djangoapps/api_admin/views.py b/openedx/core/djangoapps/api_admin/views.py index 4c3d051eb7..b99a81cba4 100644 --- a/openedx/core/djangoapps/api_admin/views.py +++ b/openedx/core/djangoapps/api_admin/views.py @@ -1,16 +1,15 @@ """Views for API management.""" import logging -from smtplib import SMTPException from django.conf import settings -from django.core.mail import send_mail +from django.contrib.sites.shortcuts import get_current_site from django.core.urlresolvers import reverse_lazy, reverse from django.shortcuts import redirect from django.utils.translation import ugettext as _ from django.views.generic import View from django.views.generic.base import TemplateView from django.views.generic.edit import CreateView -from edxmako.shortcuts import render_to_response, render_to_string +from edxmako.shortcuts import render_to_response from openedx.core.djangoapps.api_admin.forms import ApiAccessRequestForm from openedx.core.djangoapps.api_admin.models import ApiAccessRequest @@ -22,7 +21,7 @@ class ApiRequestView(CreateView): """Form view for requesting API access.""" form_class = ApiAccessRequestForm template_name = 'api_admin/api_access_request_form.html' - success_url = reverse_lazy('api-status') + success_url = reverse_lazy('api_admin:api-status') def get(self, request): """ @@ -30,41 +29,13 @@ class ApiRequestView(CreateView): them to the client creation page. """ if ApiAccessRequest.api_access_status(request.user) is not None: - return redirect(reverse('api-status')) + return redirect(reverse('api_admin:api-status')) return super(ApiRequestView, self).get(request) - def send_email(self, api_request): - """ - Send an email to settings.API_ACCESS_MANAGER_EMAIL with the - contents of this API access request. - """ - context = { - 'approval_url': self.request.build_absolute_uri( - reverse('admin:api_admin_apiaccessrequest_change', args=(api_request.id,)) - ), - 'company_name': api_request.company_name, - 'username': api_request.user.username, - 'url': api_request.website, - 'company_address': api_request.company_address, - 'reason': api_request.reason, - } - message = render_to_string('api_admin/email.txt', context) - try: - send_mail( - _('API access request from {company}').format(company=api_request.company_name), - message, - settings.API_ACCESS_FROM_EMAIL, - [settings.API_ACCESS_MANAGER_EMAIL], - fail_silently=False - ) - except SMTPException: - log.exception('Error sending API request email for request [%s].', api_request.id) - def form_valid(self, form): form.instance.user = self.request.user - result = super(ApiRequestView, self).form_valid(form) - self.send_email(form.instance) - return result + form.instance.site = get_current_site(self.request) + return super(ApiRequestView, self).form_valid(form) class ApiRequestStatusView(View): @@ -77,7 +48,7 @@ class ApiRequestStatusView(View): """ status = ApiAccessRequest.api_access_status(request.user) if status is None: - return redirect(reverse('api-request')) + return redirect(reverse('api_admin:api-request')) return render_to_response('api_admin/status.html', { 'status': status, 'api_support_link': _('TODO'), diff --git a/openedx/core/djangoapps/api_admin/widgets.py b/openedx/core/djangoapps/api_admin/widgets.py index 95486fd96e..0b1e5f4600 100644 --- a/openedx/core/djangoapps/api_admin/widgets.py +++ b/openedx/core/djangoapps/api_admin/widgets.py @@ -24,7 +24,7 @@ class TermsOfServiceCheckboxInput(CheckboxInput): # platform_name is the name of this Open edX installation. label = _('I, and my company, accept the {link_start}{platform_name} API Terms of Service{link_end}.').format( platform_name=settings.PLATFORM_NAME, - link_start=''.format(url=reverse('api-tos')), + link_start=''.format(url=reverse('api_admin:api-tos')), link_end='', )