From 3ca0128d969cb179fa4afa31a8c998f50df96e76 Mon Sep 17 00:00:00 2001 From: Asad Ali Date: Thu, 17 Aug 2023 15:06:35 +0500 Subject: [PATCH] feat: add ability to retire users on edX (#1785) * feat: add ability to retire users on edX * fix tests * fix tests * update app-json * added checks for client id and secret * tests: add tests * requirements: updated edx-api-client version * update dependency * update dependency --- app.json | 8 ++++ main/settings.py | 12 +++++ openedx/api.py | 50 ++++++++++++++++++++ openedx/api_test.py | 42 ++++++++++++++++ poetry.lock | 8 ++-- pyproject.toml | 2 +- users/management/commands/retire_users.py | 22 ++++++++- users/management/tests/retire_users_test.py | 45 +++++++++++++++--- users/management/tests/unblock_users_test.py | 27 +++++++---- 9 files changed, 196 insertions(+), 20 deletions(-) diff --git a/app.json b/app.json index 0a4ef9a8d7..15c8998ad8 100644 --- a/app.json +++ b/app.json @@ -489,6 +489,14 @@ "description": "The 'name' value for the Open edX OAuth Application", "required": true }, + "OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID": { + "description": "OAuth2 client id for retirement service worker", + "required": false + }, + "OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET": { + "description": "OAuth2 client secret for retirement service worker", + "required": false + }, "OPENEDX_SERVICE_WORKER_API_TOKEN": { "description": "Active access token with staff level permissions to use with OpenEdX API client for service tasks", "required": false diff --git a/main/settings.py b/main/settings.py index a7d99c85aa..e0a4823e96 100644 --- a/main/settings.py +++ b/main/settings.py @@ -1019,6 +1019,18 @@ default=None, description="Username of the user whose token has been set in OPENEDX_SERVICE_WORKER_API_TOKEN", ) + +OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID = get_string( + name="OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID", + default=None, + description="OAuth2 client id for retirement service worker", +) +OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET = get_string( + name="OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET", + default=None, + description="OAuth2 client secret for retirement service worker", +) + EDX_API_CLIENT_TIMEOUT = get_int( name="EDX_API_CLIENT_TIMEOUT", default=60, diff --git a/openedx/api.py b/openedx/api.py index 50dabfc96e..6ab0b3788c 100644 --- a/openedx/api.py +++ b/openedx/api.py @@ -515,6 +515,40 @@ def get_edx_api_service_client(): return edx_client +def get_edx_retirement_service_client(): + """ + Generates a JWT access token for the retirement service worker and returns the edX api client. + """ + if not settings.OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID: + raise ImproperlyConfigured( + "OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID is not set" + ) + elif not settings.OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET: + raise ImproperlyConfigured( + "OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET is not set" + ) + + data = { + "grant_type": "client_credentials", + "client_id": settings.OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID, + "client_secret": settings.OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET, + "token_type": "jwt", + } + resp = requests.post(edx_url(OPENEDX_OAUTH2_ACCESS_TOKEN_PATH), data=data) + resp.raise_for_status() + access_token = resp.json()["access_token"] + + edx_client = EdxApi( + { + "access_token": access_token, + }, + settings.OPENEDX_API_BASE_URL, + timeout=settings.EDX_API_CLIENT_TIMEOUT, + ) + + return edx_client + + def get_edx_api_course_detail_client(): """ Gets an edx api client instance for use with the grades api @@ -879,3 +913,19 @@ def validate_username_with_edx(username): raise EdxApiRegistrationValidationException(username, resp) result = resp.json() return result["validation_decisions"]["username"] + + +def bulk_retire_edx_users(usernames): + """ + Bulk retires edX users. + + This method calls edX bulk retirement API to initiate the user retirement on edX. + Users will be moved to the pending retirement state and then retirement will be carried out by our + tubular concourse pipeline. + + Args: + usernames (str): Comma separated usernames + """ + edx_client = get_edx_retirement_service_client() + response = edx_client.bulk_user_retirement.retire_users({"usernames": usernames}) + return response diff --git a/openedx/api_test.py b/openedx/api_test.py index 239b21fb90..abff39085f 100644 --- a/openedx/api_test.py +++ b/openedx/api_test.py @@ -22,12 +22,14 @@ OPENEDX_AUTH_DEFAULT_TTL_IN_SECONDS, OPENEDX_REGISTRATION_VALIDATION_PATH, OPENEDX_UPDATE_USER_PATH, + bulk_retire_edx_users, create_edx_auth_token, create_edx_user, create_user, enroll_in_edx_course_runs, existing_edx_enrollment, get_edx_api_client, + get_edx_retirement_service_client, get_valid_edx_api_auth, repair_faulty_edx_user, repair_faulty_openedx_users, @@ -421,6 +423,26 @@ def test_get_edx_api_client(mocker, settings, user): ) +def test_get_edx_retirement_service_client(mocker, settings): + """Tests that get_edx_retirement_service_client returns an EdxApi client""" + + settings.OPENEDX_API_BASE_URL = "http://example.com" + settings.OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID = ( + "OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_ID" + ) + settings.OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET = ( + "OPENEDX_RETIREMENT_SERVICE_WORKER_CLIENT_SECRET" + ) + mock_resp = mocker.Mock() + mock_resp.json.return_value = {"access_token": "an_access_token"} + mock_resp.json.status_code = 200 + mock_resp.raise_for_status.side_effect = None + mocker.patch("openedx.api.requests.post", return_value=mock_resp) + client = get_edx_retirement_service_client() + assert client.credentials["access_token"] == "an_access_token" + assert client.base_url == settings.OPENEDX_API_BASE_URL + + def test_enroll_in_edx_course_runs(settings, mocker, user): """Tests that enroll_in_edx_course_runs uses the EdxApi client to enroll in course runs""" settings.OPENEDX_SERVICE_WORKER_API_TOKEN = "mock_api_token" @@ -890,3 +912,23 @@ def test_existing_edx_enrollment( ) is None ) + + +def test_bulk_retire_edx_users(mocker): + """Tests that bulk_retire_edx_users calls the edX bulk retirement api via the edx api client""" + test_usernames = "test_username1,test_username2" + mock_client = mocker.MagicMock() + mock_get_edx_retirement_service_client = mocker.patch( + "openedx.api.get_edx_retirement_service_client", return_value=mock_client + ) + mock_client.bulk_user_retirement.retire_users = mocker.Mock( + return_value={ + "successful_user_retirements": ["test_username1", "test_username2"] + } + ) + resp = bulk_retire_edx_users(test_usernames) + assert resp["successful_user_retirements"] == ["test_username1", "test_username2"] + mock_get_edx_retirement_service_client.assert_called() + mock_client.bulk_user_retirement.retire_users.assert_called_with( + {"usernames": test_usernames} + ) diff --git a/poetry.lock b/poetry.lock index 24d07ea3e1..0bbd3e91c4 100644 --- a/poetry.lock +++ b/poetry.lock @@ -1524,13 +1524,13 @@ djangorestframework = ">=3.9.3" [[package]] name = "edx-api-client" -version = "1.7.0" +version = "1.8.0" description = "Python interface to the edX REST APIs" optional = false python-versions = "*" files = [ - {file = "edx-api-client-1.7.0.tar.gz", hash = "sha256:9c1bc32e77cdb4a2db9bfbc11fa1f024fd06d9ea7784f7f1f227d27452a32d34"}, - {file = "edx_api_client-1.7.0-py2.py3-none-any.whl", hash = "sha256:2947fb5ecdc1087c3f5396c1f916ffd140cf314ce18975c2c57993981dc88b8c"}, + {file = "edx-api-client-1.8.0.tar.gz", hash = "sha256:cbf9af4d466cbea465054d78688e6f335a3e485a4e2f90dc82e050202af29ba1"}, + {file = "edx_api_client-1.8.0-py2.py3-none-any.whl", hash = "sha256:050f0622c2c3a0d4f24902bf6218b2a554b86a5e84800b71d1fefd0bddf3ad13"}, ] [package.dependencies] @@ -4328,4 +4328,4 @@ testing = ["coverage (>=5.0.3)", "zope.event", "zope.testing"] [metadata] lock-version = "2.0" python-versions = "3.9.6" -content-hash = "9afa30650eace052570089c9326aed13cfad93e9f62f1cfbb30fe99dd52e08c0" +content-hash = "dd17aeb2233fff97a9564baa4cb23fa407939904e15f254b007cad667bdc1475" diff --git a/pyproject.toml b/pyproject.toml index ac4b92c376..c2f26d8708 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -57,7 +57,7 @@ drf-extensions = "^0.7.1" django-reversion = "^4.0.1" django-storages = "^1.11.1" djoser = "^2.1.0" -edx-api-client = "^1.7.0" +edx-api-client = "^1.8.0" hubspot-api-client = "^6.1.0" ipython = "^7.31.1" mitol-django-common = "2.7.0" diff --git a/users/management/commands/retire_users.py b/users/management/commands/retire_users.py index 152d11ec18..a44f682e8f 100644 --- a/users/management/commands/retire_users.py +++ b/users/management/commands/retire_users.py @@ -1,5 +1,5 @@ """ -Retire user(s) from MITx Online +Retire user(s) from MITx Online and user initiate retirement on edX """ import hashlib import sys @@ -13,6 +13,7 @@ from authentication.utils import block_user_email from main import settings +from openedx.api import bulk_retire_edx_users from users.api import fetch_users from users.models import BlockList @@ -106,6 +107,25 @@ def handle(self, *args, **kwargs): ) continue + resp = bulk_retire_edx_users(user.username) + if user.username not in resp["successful_user_retirements"]: + self.stderr.write( + self.style.ERROR( + "Could not initiate retirement request on edX for user {user}".format( + user=user + ) + ) + ) + continue + else: + self.stdout.write( + self.style.SUCCESS( + "Retirement request initiated on edX for User: '{user}'".format( + user=user + ) + ) + ) + user.is_active = False # Change user password & email diff --git a/users/management/tests/retire_users_test.py b/users/management/tests/retire_users_test.py index 1955620266..f18f93634e 100644 --- a/users/management/tests/retire_users_test.py +++ b/users/management/tests/retire_users_test.py @@ -15,10 +15,15 @@ @pytest.mark.django_db -def test_single_success(): +def test_single_success(mocker): """test retire_users command success with one user""" test_username = "test_user" + mock_bulk_retire_edx_users = mocker.patch( + "users.management.commands.retire_users.bulk_retire_edx_users", + return_value={"successful_user_retirements": [test_username]}, + ) + user = UserFactory.create(username=test_username, is_active=True) UserSocialAuthFactory.create(user=user, provider="edX") @@ -32,13 +37,19 @@ def test_single_success(): assert user.is_active is False assert "retired_email" in user.email assert UserSocialAuth.objects.filter(user=user).count() == 0 + mock_bulk_retire_edx_users.assert_called() @pytest.mark.django_db -def test_multiple_success(): +def test_multiple_success(mocker): """test retire_users command success with more than one user""" test_usernames = ["foo", "bar", "baz"] + mock_bulk_retire_edx_users = mocker.patch( + "users.management.commands.retire_users.bulk_retire_edx_users", + return_value={"successful_user_retirements": test_usernames}, + ) + for username in test_usernames: user = UserFactory.create(username=username, is_active=True) UserSocialAuthFactory.create(user=user, provider="not_edx") @@ -54,16 +65,22 @@ def test_multiple_success(): assert user.is_active is False assert "retired_email" in user.email assert UserSocialAuth.objects.filter(user=user).count() == 0 + mock_bulk_retire_edx_users.assert_called() @pytest.mark.django_db -def test_retire_user_with_email(): +def test_retire_user_with_email(mocker): """test retire_users command success with user email""" test_email = "test@email.com" user = UserFactory.create(email=test_email, is_active=True) UserSocialAuthFactory.create(user=user, provider="edX") + mock_bulk_retire_edx_users = mocker.patch( + "users.management.commands.retire_users.bulk_retire_edx_users", + return_value={"successful_user_retirements": [user.username]}, + ) + assert user.is_active is True assert "retired_email" not in user.email assert UserSocialAuth.objects.filter(user=user).count() == 1 @@ -74,10 +91,11 @@ def test_retire_user_with_email(): assert user.is_active is False assert "retired_email" in user.email assert UserSocialAuth.objects.filter(user=user).count() == 0 + mock_bulk_retire_edx_users.assert_called_with(user.username) @pytest.mark.django_db -def test_retire_user_blocking_with_email(): +def test_retire_user_blocking_with_email(mocker): """test retire_users command success with user email""" test_email = "test@email.com" @@ -90,6 +108,10 @@ def test_retire_user_blocking_with_email(): assert UserSocialAuth.objects.filter(user=user).count() == 1 assert BlockList.objects.all().count() == 0 + mock_bulk_retire_edx_users = mocker.patch( + "users.management.commands.retire_users.bulk_retire_edx_users", + return_value={"successful_user_retirements": [user.username]}, + ) COMMAND.handle("retire_users", users=[test_email], block_users=True) user.refresh_from_db() @@ -98,12 +120,17 @@ def test_retire_user_blocking_with_email(): assert UserSocialAuth.objects.filter(user=user).count() == 0 assert BlockList.objects.all().count() == 1 assert BlockList.objects.filter(hashed_email=hashed_email).count() == 1 + mock_bulk_retire_edx_users.assert_called_with(user.username) @pytest.mark.django_db -def test_multiple_success_blocking_user(): +def test_multiple_success_blocking_user(mocker): """test retire_users command blocking emails success with more than one user""" test_usernames = ["foo", "bar", "baz"] + mock_bulk_retire_edx_users = mocker.patch( + "users.management.commands.retire_users.bulk_retire_edx_users", + return_value={"successful_user_retirements": test_usernames}, + ) for username in test_usernames: user = UserFactory.create(username=username, is_active=True) @@ -123,10 +150,11 @@ def test_multiple_success_blocking_user(): assert UserSocialAuth.objects.filter(user=user).count() == 0 assert BlockList.objects.all().count() == 3 + mock_bulk_retire_edx_users.assert_called() @pytest.mark.django_db -def test_user_blocking_if_not_requested(): +def test_user_blocking_if_not_requested(mocker): """test retire_users command success but it should not block user(s) if not requested""" test_email = "test@email.com" @@ -139,6 +167,10 @@ def test_user_blocking_if_not_requested(): assert UserSocialAuth.objects.filter(user=user).count() == 1 assert BlockList.objects.all().count() == 0 + mock_bulk_retire_edx_users = mocker.patch( + "users.management.commands.retire_users.bulk_retire_edx_users", + return_value={"successful_user_retirements": [user.username]}, + ) COMMAND.handle("retire_users", users=[test_email]) user.refresh_from_db() @@ -146,3 +178,4 @@ def test_user_blocking_if_not_requested(): assert "retired_email" in user.email assert UserSocialAuth.objects.filter(user=user).count() == 0 assert BlockList.objects.all().count() == 0 + mock_bulk_retire_edx_users.assert_called_with(user.username) diff --git a/users/management/tests/unblock_users_test.py b/users/management/tests/unblock_users_test.py index e41bf31b0f..2fa00d3be4 100644 --- a/users/management/tests/unblock_users_test.py +++ b/users/management/tests/unblock_users_test.py @@ -1,5 +1,6 @@ """retire user test""" import hashlib +from unittest.mock import patch import pytest from django.contrib.auth import get_user_model @@ -13,6 +14,7 @@ User = get_user_model() +# @patch("users.management.commands.retire_users.bulk_retire_edx_users") class TestUnblockUsers(TestCase): """ Tests unblock users management command. @@ -23,8 +25,9 @@ def setUp(self): self.RETIRE_USER_COMMAND = retire_users.Command() self.UNBLOCK_USER_COMMAND = unblock_users.Command() + @patch("users.management.commands.retire_users.bulk_retire_edx_users") @pytest.mark.django_db - def test_user_unblocking_with_email(self): + def test_user_unblocking_with_email(self, mocked_bulk_retire_edx_users): """test unblock_users command success with user email""" test_email = "test@email.com" @@ -37,6 +40,9 @@ def test_user_unblocking_with_email(self): assert UserSocialAuth.objects.filter(user=user).count() == 1 assert BlockList.objects.all().count() == 0 + mocked_bulk_retire_edx_users.return_value = { + "successful_user_retirements": [user.username] + } self.RETIRE_USER_COMMAND.handle( "retire_users", users=[test_email], block_users=True ) @@ -53,13 +59,18 @@ def test_user_unblocking_with_email(self): assert BlockList.objects.all().count() == 0 assert BlockList.objects.filter(hashed_email=hashed_email).count() == 0 + @patch("users.management.commands.retire_users.bulk_retire_edx_users") @pytest.mark.django_db - def test_multiple_success_unblocking_user(self): + def test_multiple_success_unblocking_user(self, mocked_bulk_retire_edx_users): """test unblock_users command unblocking emails success with more than one user""" - test_users = ["foo@email.com", "bar@email.com", "baz@email.com"] - - for email in test_users: - user = UserFactory.create(email=email, is_active=True) + test_user_emails = ["foo@email.com", "bar@email.com", "baz@email.com"] + test_usernames = ["foo", "bar", "baz"] + mocked_bulk_retire_edx_users.return_value = { + "successful_user_retirements": test_usernames + } + + for email, username in zip(test_user_emails, test_usernames): + user = UserFactory.create(email=email, username=username, is_active=True) UserSocialAuthFactory.create(user=user, provider="not_edx") assert user.is_active is True @@ -68,12 +79,12 @@ def test_multiple_success_unblocking_user(self): assert BlockList.objects.all().count() == 0 self.RETIRE_USER_COMMAND.handle( - "retire_users", users=test_users, block_users=True + "retire_users", users=test_user_emails, block_users=True ) assert BlockList.objects.all().count() == 3 # Now we need to unblock the user from block list. - self.UNBLOCK_USER_COMMAND.handle("unblock_users", users=test_users) + self.UNBLOCK_USER_COMMAND.handle("unblock_users", users=test_user_emails) assert BlockList.objects.all().count() == 0 @pytest.mark.django_db