Andal.LND/edx-platform
6
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Merge pull request #17920 from edx/bmedx/fix_retirement_apis

Browse Source 
Update retirement APIs to match spec
This commit is contained in:
Brian Mesick
2018-04-25 10:39:26 -04:00
committed by GitHub
parent 321a1acb04 98742c1c50
commit 1828bf7d6b
9 changed files with 201 additions and 177 deletions
Download Patch File Download Diff File Expand all files Collapse all files

17
common/djangoapps/enrollment/tests/test_views.py
View File

@@ -1282,11 +1282,19 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
self._assert_active()
def test_deactivate_enrollments_no_username(self):
self._assert_active()
response = self._submit_unenroll(self.superuser, None)
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
data = json.loads(response.content)
self.assertEqual(data, u"Username not specified.")
self._assert_active()
def test_deactivate_enrollments_empty_username(self):
self._assert_active()
response = self._submit_unenroll(self.superuser, "")
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
data = json.loads(response.content)
self.assertEqual(data['message'], 'The user was not specified.')
self.assertEqual(data, u'The user "" does not exist.')
self._assert_active()
def test_deactivate_enrollments_invalid_username(self):
@@ -1294,7 +1302,7 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
response = self._submit_unenroll(self.superuser, "a made up username")
self.assertEqual(response.status_code, status.HTTP_404_NOT_FOUND)
data = json.loads(response.content)
self.assertEqual(data['message'], 'The user "a made up username" does not exist.')
self.assertEqual(data, u'The user "a made up username" does not exist.')
self._assert_active()
def test_deactivate_enrollments_called_twice(self):
@@ -1318,7 +1326,10 @@ class UnenrollmentTest(EnrollmentTestMixin, ModuleStoreTestCase):
self.assertFalse(is_active)
def _submit_unenroll(self, submitting_user, unenrolling_username):
data = {'user': unenrolling_username}
data = {}
if unenrolling_username is not None:
data['username'] = unenrolling_username
url = reverse('unenrollment')
headers = self.build_jwt_headers(submitting_user)
return self.client.post(url, json.dumps(data), content_type='application/json', **headers)

36
common/djangoapps/enrollment/views.py
View File

@@ -5,6 +5,7 @@ consist primarily of authentication, request validation, and serialization.
"""
import logging
from django.contrib.auth import get_user_model
from django.core.exceptions import ObjectDoesNotExist
from django.utils.decorators import method_decorator
from edx_rest_framework_extensions.authentication import JwtAuthentication
@@ -23,6 +24,7 @@ from openedx.core.djangoapps.cors_csrf.authentication import SessionAuthenticati
from openedx.core.djangoapps.cors_csrf.decorators import ensure_csrf_cookie_cross_domain
from openedx.core.djangoapps.embargo import api as embargo_api
from openedx.core.djangoapps.user_api.accounts.permissions import CanRetireUser
from openedx.core.djangoapps.user_api.models import UserRetirementStatus
from openedx.core.djangoapps.user_api.preferences.api import update_email_opt_in
from openedx.core.lib.api.authentication import (
OAuth2AuthenticationAllowInactiveUser,
@@ -313,14 +315,14 @@ class UnenrollmentView(APIView):
**Example Requests**
POST /api/enrollment/v1/enrollment {
"user": "username12345"
"username": "username12345"
}
**POST Parameters**
A POST request must include the following parameter.
* user: The username of the user being unenrolled.
* username: The username of the user being unenrolled.
This will never match the username from the request,
since the request is issued as a privileged service user.
@@ -342,31 +344,23 @@ class UnenrollmentView(APIView):
"""
Unenrolls the specified user from all courses.
"""
# Get the User from the request.
username = request.data.get('user', None)
if not username:
return Response(
status=status.HTTP_404_NOT_FOUND,
data={
'message': u'The user was not specified.'
}
)
try:
# make sure the specified user exists
User.objects.get(username=username)
except ObjectDoesNotExist:
return Response(
status=status.HTTP_404_NOT_FOUND,
data={
'message': u'The user "{}" does not exist.'.format(username)
}
)
username = None
user_model = get_user_model()
try:
# Get the username from the request and check that it exists
username = request.data['username']
user_model.objects.get(username=username)
enrollments = api.get_enrollments(username)
active_enrollments = [enrollment for enrollment in enrollments if enrollment['is_active']]
if len(active_enrollments) < 1:
return Response(status=status.HTTP_200_OK)
return Response(api.unenroll_user_from_all_courses(username))
except KeyError:
return Response(u'Username not specified.', status=status.HTTP_404_NOT_FOUND)
except user_model.DoesNotExist:
return Response(u'The user "{}" does not exist.'.format(username), status=status.HTTP_404_NOT_FOUND)
except Exception as exc: # pylint: disable=broad-except
return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR)

37
lms/djangoapps/discussion_api/tests/test_views.py
View File

@@ -28,6 +28,7 @@ from discussion_api.tests.utils import (
)
from django_comment_client.tests.utils import ForumsEnableMixin
from openedx.core.djangoapps.user_api.accounts.image_helpers import get_profile_image_storage
from openedx.core.djangoapps.user_api.models import RetirementState, UserRetirementStatus
from openedx.core.lib.token_utils import JwtBuilder
from student.models import get_retired_username_by_username
from student.tests.factories import CourseEnrollmentFactory, UserFactory, SuperuserFactory
@@ -163,12 +164,16 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
"""Tests for CourseView"""
def setUp(self):
super(RetireViewTest, self).setUp()
RetirementState.objects.create(state_name='PENDING', state_execution_order=1)
self.retire_forums_state = RetirementState.objects.create(state_name='RETIRE_FORUMS', state_execution_order=11)
self.retirement = UserRetirementStatus.create_retirement(self.user)
self.retirement.current_state = self.retire_forums_state
self.retirement.save()
self.superuser = SuperuserFactory()
self.retired_username = get_retired_username_by_username(self.user.username)
self.url = reverse(
"retire_discussion_user",
kwargs={"username": text_type(self.user.username)}
)
self.url = reverse("retire_discussion_user")
def assert_response_correct(self, response, expected_status, expected_content):
"""
@@ -193,24 +198,18 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
"""
self.register_get_user_retire_response(self.user)
headers = self.build_jwt_headers(self.superuser)
response = self.client.post(self.url, {'retired_username': self.retired_username}, **headers)
data = {'username': self.user.username}
response = self.client.post(self.url, data, **headers)
self.assert_response_correct(response, 204, "")
def test_bad_hash(self):
"""
Check that we fail on a hash mismatch with an appropriate error
"""
headers = self.build_jwt_headers(self.superuser)
response = self.client.post(self.url, {'retired_username': "this will never match"}, **headers)
self.assert_response_correct(response, 500, '"Mismatched hashed_username, bad salt?"')
def test_downstream_forums_error(self):
"""
Check that we bubble up errors from the comments service
"""
self.register_get_user_retire_response(self.user, status=500, body="Server error")
headers = self.build_jwt_headers(self.superuser)
response = self.client.post(self.url, {'retired_username': self.retired_username}, **headers)
data = {'username': self.user.username}
response = self.client.post(self.url, data, **headers)
self.assert_response_correct(response, 500, '"Server error"')
def test_nonexistent_user(self):
@@ -218,19 +217,15 @@ class RetireViewTest(DiscussionAPIViewTestMixin, ModuleStoreTestCase):
Check that we handle unknown users appropriately
"""
nonexistent_username = "nonexistent user"
self.url = reverse(
"retire_discussion_user",
kwargs={"username": nonexistent_username}
)
self.retired_username = get_retired_username_by_username(nonexistent_username)
data = {'username': nonexistent_username}
headers = self.build_jwt_headers(self.superuser)
response = self.client.post(self.url, {'retired_username': self.retired_username}, **headers)
response = self.client.post(self.url, data, **headers)
self.assert_response_correct(response, 404, None)
def test_not_authenticated(self):
"""
Override the parent implementation of this, we auth differently
Override the parent implementation of this, we JWT auth for this API
"""
pass

2
lms/djangoapps/discussion_api/urls.py
View File

@@ -17,7 +17,7 @@ urlpatterns = [
CourseView.as_view(),
name="discussion_course"
),
url(r"^v1/users/{}".format(settings.USERNAME_PATTERN), RetireUserView.as_view(), name="retire_discussion_user"),
url(r"^v1/accounts/retire_forum", RetireUserView.as_view(), name="retire_discussion_user"),
url(
r"^v1/course_topics/{}".format(settings.COURSE_ID_PATTERN),
CourseTopicsView.as_view(),

13
lms/djangoapps/discussion_api/views.py
View File

@@ -33,7 +33,7 @@ from discussion_api.forms import CommentGetForm, CommentListGetForm, ThreadListG
from openedx.core.lib.api.parsers import MergePatchParser
from openedx.core.lib.api.view_utils import DeveloperErrorViewMixin, view_auth_classes
from openedx.core.djangoapps.user_api.accounts.permissions import CanRetireUser
from student.models import get_potentially_retired_user_by_username_and_hash
from openedx.core.djangoapps.user_api.models import UserRetirementStatus
from xmodule.modulestore.django import modulestore
@@ -543,23 +543,22 @@ class RetireUserView(APIView):
authentication_classes = (JwtAuthentication,)
permission_classes = (permissions.IsAuthenticated, CanRetireUser)
def post(self, request, username):
def post(self, request):
"""
Implements the retirement endpoint.
"""
user_model = get_user_model()
retired_username = request.data['retired_username']
username = request.data['username']
try:
user = get_potentially_retired_user_by_username_and_hash(username, retired_username)
cc_user = comment_client.User.from_django_user(user)
retirement = UserRetirementStatus.get_retirement_for_retirement_action(username)
cc_user = comment_client.User.from_django_user(retirement.user)
# We can't count on the LMS username being un-retired at this point,
# so we pass the old username as a parameter to describe which
# user to retire. This will either succeed or throw an error which
# should be good to raise from here.
cc_user.retire(username)
except user_model.DoesNotExist:
except UserRetirementStatus.DoesNotExist:
return Response(status=status.HTTP_404_NOT_FOUND)
except Exception as exc: # pylint: disable=broad-except
return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR)

207
openedx/core/djangoapps/user_api/accounts/tests/test_views.py
View File

@@ -2,6 +2,8 @@
"""
Test cases to cover Accounts-related behaviors of the User API application
"""
from __future__ import print_function
import datetime
import hashlib
import json
@@ -924,99 +926,6 @@ class TestAccountDeactivation(TestCase):
)
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
class TestAccountRetireMailings(TestCase):
"""
Tests the account retire mailings endpoint.
"""
def setUp(self):
super(TestAccountRetireMailings, self).setUp()
self.test_user = UserFactory()
self.test_superuser = SuperuserFactory()
self.test_service_user = UserFactory()
UserOrgTag.objects.create(user=self.test_user, key='email-optin', org="foo", value="True")
UserOrgTag.objects.create(user=self.test_user, key='email-optin', org="bar", value="True")
self.url = reverse('accounts_retire_mailings', kwargs={'username': self.test_user.username})
def build_post(self, user):
retired_username = get_retired_username_by_username(user.username)
return {'retired_username': retired_username}
def build_jwt_headers(self, user):
"""
Helper function for creating headers for the JWT authentication.
"""
token = JwtBuilder(user).build_token([])
headers = {
'HTTP_AUTHORIZATION': 'JWT ' + token
}
return headers
def assert_status_and_tag_count(self, headers, expected_status=status.HTTP_204_NO_CONTENT, expected_tag_count=2,
expected_tag_value="False", expected_content=None):
"""
Helper function for making a request to the retire subscriptions endpoint, and asserting the status.
"""
response = self.client.post(self.url, self.build_post(self.test_user), **headers)
self.assertEqual(response.status_code, expected_status)
# Check that the expected number of tags with the correct value exist
tag_count = UserOrgTag.objects.filter(user=self.test_user, value=expected_tag_value).count()
self.assertEqual(tag_count, expected_tag_count)
if expected_content:
self.assertEqual(response.content.strip('"'), expected_content)
def test_superuser_retires_user_subscriptions(self):
"""
Verify a user's subscriptions are retired when a superuser posts to the retire subscriptions endpoint.
"""
headers = self.build_jwt_headers(self.test_superuser)
self.assert_status_and_tag_count(headers)
def test_superuser_retires_user_subscriptions_no_orgtags(self):
"""
Verify the call succeeds when the user doesn't have any org tags.
"""
UserOrgTag.objects.all().delete()
headers = self.build_jwt_headers(self.test_superuser)
self.assert_status_and_tag_count(headers, expected_tag_count=0)
def test_unauthorized_rejection(self):
"""
Verify unauthorized users cannot retire subscriptions.
"""
headers = self.build_jwt_headers(self.test_user)
# User should still have 2 "True" subscriptions.
self.assert_status_and_tag_count(headers, expected_status=status.HTTP_403_FORBIDDEN, expected_tag_value="True")
def test_signal_failure(self):
"""
Verify that if a signal fails the transaction is rolled back and a proper error message is returned.
"""
headers = self.build_jwt_headers(self.test_superuser)
mock_handler = MagicMock()
mock_handler.side_effect = Exception("Tango")
try:
USER_RETIRE_MAILINGS.connect(mock_handler)
# User should still have 2 "True" subscriptions.
self.assert_status_and_tag_count(
headers,
expected_status=status.HTTP_500_INTERNAL_SERVER_ERROR,
expected_tag_value="True",
expected_content="Tango"
)
finally:
USER_RETIRE_MAILINGS.disconnect(mock_handler)
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
class TestDeactivateLogout(TestCase):
"""
@@ -1052,7 +961,7 @@ class TestDeactivateLogout(TestCase):
return headers
def build_post(self, username):
return {'user': username}
return {'username': username}
def test_superuser_deactivates_user(self):
"""
@@ -1095,8 +1004,13 @@ class RetirementTestCase(TestCase):
"""
Test case with a helper methods for retirement
"""
@classmethod
def setUpClass(cls):
super(RetirementTestCase, cls).setUpClass()
cls.setup_states()
def setup_states(self):
@staticmethod
def setup_states():
"""
Create basic states that mimic our current understanding of the retirement process
"""
@@ -1215,6 +1129,106 @@ class RetirementTestCase(TestCase):
return [state for state in RetirementState.objects.filter(is_dead_end_state=True)]
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
class TestAccountRetireMailings(RetirementTestCase):
"""
Tests the account retire mailings endpoint.
"""
def setUp(self):
super(TestAccountRetireMailings, self).setUp()
self.test_superuser = SuperuserFactory()
self.test_service_user = UserFactory()
# Should be created in parent setUpClass
retiring_email_lists = RetirementState.objects.get(state_name='RETIRING_EMAIL_LISTS')
self.retirement = self._create_retirement(retiring_email_lists)
self.test_user = self.retirement.user
UserOrgTag.objects.create(user=self.test_user, key='email-optin', org="foo", value="True")
UserOrgTag.objects.create(user=self.test_user, key='email-optin', org="bar", value="True")
self.url = reverse('accounts_retire_mailings')
def build_jwt_headers(self, user):
"""
Helper function for creating headers for the JWT authentication.
"""
token = JwtBuilder(user).build_token([])
headers = {
'HTTP_AUTHORIZATION': 'JWT ' + token
}
return headers
def build_post(self, user):
return {'username': user.username}
def assert_status_and_tag_count(self, headers, expected_status=status.HTTP_204_NO_CONTENT, expected_tag_count=2,
expected_tag_value="False", expected_content=None):
"""
Helper function for making a request to the retire subscriptions endpoint, and asserting the status.
"""
response = self.client.post(self.url, self.build_post(self.test_user), **headers)
if response.status_code != expected_status:
print(response)
self.assertEqual(response.status_code, expected_status)
# Check that the expected number of tags with the correct value exist
tag_count = UserOrgTag.objects.filter(user=self.test_user, value=expected_tag_value).count()
self.assertEqual(tag_count, expected_tag_count)
if expected_content:
self.assertEqual(response.content.strip('"'), expected_content)
def test_superuser_retires_user_subscriptions(self):
"""
Verify a user's subscriptions are retired when a superuser posts to the retire subscriptions endpoint.
"""
headers = self.build_jwt_headers(self.test_superuser)
self.assert_status_and_tag_count(headers)
def test_superuser_retires_user_subscriptions_no_orgtags(self):
"""
Verify the call succeeds when the user doesn't have any org tags.
"""
UserOrgTag.objects.all().delete()
headers = self.build_jwt_headers(self.test_superuser)
self.assert_status_and_tag_count(headers, expected_tag_count=0)
def test_unauthorized_rejection(self):
"""
Verify unauthorized users cannot retire subscriptions.
"""
headers = self.build_jwt_headers(self.test_user)
# User should still have 2 "True" subscriptions.
self.assert_status_and_tag_count(headers, expected_status=status.HTTP_403_FORBIDDEN, expected_tag_value="True")
def test_signal_failure(self):
"""
Verify that if a signal fails the transaction is rolled back and a proper error message is returned.
"""
headers = self.build_jwt_headers(self.test_superuser)
mock_handler = MagicMock()
mock_handler.side_effect = Exception("Tango")
try:
USER_RETIRE_MAILINGS.connect(mock_handler)
# User should still have 2 "True" subscriptions.
self.assert_status_and_tag_count(
headers,
expected_status=status.HTTP_500_INTERNAL_SERVER_ERROR,
expected_tag_value="True",
expected_content="Tango"
)
finally:
USER_RETIRE_MAILINGS.disconnect(mock_handler)
@unittest.skipUnless(settings.ROOT_URLCONF == 'lms.urls', 'Account APIs are only supported in LMS')
class TestAccountRetirementList(RetirementTestCase):
"""
@@ -1227,7 +1241,6 @@ class TestAccountRetirementList(RetirementTestCase):
self.headers = self.build_jwt_headers(self.test_superuser)
self.url = reverse('accounts_retirement_queue')
self.maxDiff = None
self.setup_states()
def assert_status_and_user_list(
self,
@@ -1371,7 +1384,6 @@ class TestAccountRetirementRetrieve(RetirementTestCase):
self.url = reverse('accounts_retirement_retrieve', kwargs={'username': self.test_user.username})
self.headers = self.build_jwt_headers(self.test_superuser)
self.maxDiff = None
self.setup_states()
def assert_status_and_user_data(self, expected_data, expected_status=status.HTTP_200_OK, username_to_find=None):
"""
@@ -1438,7 +1450,6 @@ class TestAccountRetirementUpdate(RetirementTestCase):
"""
def setUp(self):
super(TestAccountRetirementUpdate, self).setUp()
self.setup_states()
self.pending_state = RetirementState.objects.get(state_name='PENDING')
self.locking_state = RetirementState.objects.get(state_name='LOCKING_ACCOUNT')

43
openedx/core/djangoapps/user_api/accounts/views.py
View File

@@ -271,7 +271,7 @@ class AccountRetireMailingsView(APIView):
authentication_classes = (JwtAuthentication, )
permission_classes = (permissions.IsAuthenticated, CanRetireUser)
def post(self, request, username):
def post(self, request):
"""
POST /api/user/v1/accounts/{username}/retire_mailings/
@@ -280,40 +280,39 @@ class AccountRetireMailingsView(APIView):
- Update UserOrgTags to opt the user out of org emails
- Call Sailthru API to force opt-out the user from all email lists
"""
user_model = get_user_model()
retired_username = request.data['retired_username']
username = request.data['username']
try:
user = get_potentially_retired_user_by_username_and_hash(username, retired_username)
retirement = UserRetirementStatus.get_retirement_for_retirement_action(username)
with transaction.atomic():
# Take care of org emails first, using the existing API for consistency
for preference in UserOrgTag.objects.filter(user=user, key='email-optin'):
update_email_opt_in(user, preference.org, False)
for preference in UserOrgTag.objects.filter(user=retirement.user, key='email-optin'):
update_email_opt_in(retirement.user, preference.org, False)
# This signal allows lms' email_marketing and other 3rd party email
# providers to unsubscribe the user as well
USER_RETIRE_MAILINGS.send(sender=self.__class__, user=user)
except user_model.DoesNotExist:
USER_RETIRE_MAILINGS.send(sender=self.__class__, user=retirement.user)
return Response(status=status.HTTP_204_NO_CONTENT)
except UserRetirementStatus.DoesNotExist:
return Response(status=status.HTTP_404_NOT_FOUND)
except Exception as exc: # pylint: disable=broad-except
return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR)
return Response(status=status.HTTP_204_NO_CONTENT)
class DeactivateLogoutView(APIView):
"""
POST /api/user/v1/accounts/deactivate_logout/
{
"user": "example_username",
"username": "example_username",
}
**POST Parameters**
A POST request must include the following parameter.
* user: Required. The username of the user being deactivated.
* username: Required. The username of the user being deactivated.
**POST Response Values**
@@ -345,18 +344,11 @@ class DeactivateLogoutView(APIView):
Marks the user as having no password set for deactivation purposes,
and logs the user out.
"""
username = request.data.get('user', None)
if not username:
return Response(
status=status.HTTP_404_NOT_FOUND,
data={
'message': u'The user was not specified.'
}
)
username = None
user_model = get_user_model()
try:
# make sure the specified user exists
# Get the username from the request and check that it exists
username = request.data['username']
user = user_model.objects.get(username=username)
with transaction.atomic():
@@ -367,13 +359,14 @@ class DeactivateLogoutView(APIView):
user.save()
_set_unusable_password(user)
# 3. Unlink social accounts & change password on each IDA, still to be implemented
return Response(status=status.HTTP_204_NO_CONTENT)
except KeyError:
return Response(u'Username not specified.', status=status.HTTP_404_NOT_FOUND)
except user_model.DoesNotExist:
return Response(status=status.HTTP_404_NOT_FOUND)
return Response(u'The user "{}" does not exist.'.format(username), status=status.HTTP_404_NOT_FOUND)
except Exception as exc: # pylint: disable=broad-except
return Response(text_type(exc), status=status.HTTP_500_INTERNAL_SERVER_ERROR)
return Response(status=status.HTTP_204_NO_CONTENT)
def _set_unusable_password(user):
"""

21
openedx/core/djangoapps/user_api/models.py
View File

@@ -269,5 +269,26 @@ class UserRetirementStatus(TimeStampedModel):
self.responses += "\n Moved from {} to {}:\n{}\n".format(old_state, self.current_state, update['response'])
self.save()
@classmethod
def get_retirement_for_retirement_action(cls, username):
"""
Convenience method to get a UseRetirementStatus for a particular user with some checking
to make sure they're in a state that is acceptable to be acted upon. The user should be
in a "working state" (not a dead end state, PENDING, or *_COMPLETE). This should help
a bit with situations like the retirement driver accidentally trying to act upon the
same user twice at the same time, or trying to take action on an errored user.
Can raise UserRetirementStatus.DoesNotExist or RetirementStateError, otherwise should
return a UserRetirementStatus
"""
retirement = cls.objects.get(original_username=username)
state = retirement.current_state
if state.required or state.state_name.endswith('_COMPLETE'):
raise RetirementStateError('{} is in {}, not a valid state to perform retirement '
'actions on.'.format(retirement, state.state_name))
return retirement
def __unicode__(self):
return u'User: {} State: {} Last Updated: {}'.format(self.user.id, self.current_state, self.modified)

2
openedx/core/djangoapps/user_api/urls.py
View File

@@ -70,7 +70,7 @@ urlpatterns = [
name='accounts_deactivation'
),
url(
r'^v1/accounts/{}/retire_mailings/$'.format(settings.USERNAME_PATTERN),
r'^v1/accounts/retire_mailings/$',
AccountRetireMailingsView.as_view(),
name='accounts_retire_mailings'
),