From 8d4beac44a7e9168c0133f512521664e9eb64de7 Mon Sep 17 00:00:00 2001 From: Alex Dusenbery Date: Fri, 10 May 2024 10:21:23 -0400 Subject: [PATCH] fix: have the bulk_revoke endpoint deal gracefully with multiple licenses for the same email --- .../apps/api/v1/tests/test_views.py | 51 +++++++++++++++++-- license_manager/apps/api/v1/views.py | 21 +++++++- 2 files changed, 67 insertions(+), 5 deletions(-) diff --git a/license_manager/apps/api/v1/tests/test_views.py b/license_manager/apps/api/v1/tests/test_views.py index bb7fd732..929a7f37 100644 --- a/license_manager/apps/api/v1/tests/test_views.py +++ b/license_manager/apps/api/v1/tests/test_views.py @@ -2164,8 +2164,9 @@ def test_bulk_revoke_happy_path(self, mock_revoke_license, mock_execute_post_rev """ self._setup_request_jwt(user=self.user) alice_license = LicenseFactory.create(user_email='alice@example.com', status=constants.ACTIVATED) + alice_assigned_license = LicenseFactory.create(user_email='alice@example.com', status=constants.ASSIGNED) bob_license = LicenseFactory.create(user_email='bob@example.com', status=constants.ACTIVATED) - self.subscription_plan.licenses.set([alice_license, bob_license]) + self.subscription_plan.licenses.set([alice_license, bob_license, alice_assigned_license]) request_payload = { 'user_emails': [ @@ -2174,15 +2175,18 @@ def test_bulk_revoke_happy_path(self, mock_revoke_license, mock_execute_post_rev ], } - revoke_alice_license_result = {'revoked_license': alice_license, 'original_status': alice_license.status} + revoke_alice_license_result = { + 'revoked_license': alice_assigned_license, 'original_status': alice_assigned_license.status, + } revoke_bob_license_result = {'revoked_license': bob_license, 'original_status': bob_license.status} mock_revoke_license.side_effect = [revoke_alice_license_result, revoke_bob_license_result] response = self.api_client.post(self.bulk_revoke_license_url, request_payload) assert response.status_code == status.HTTP_204_NO_CONTENT + # Since alice has multiple licenses, we should only revoke her assigned one. mock_revoke_license.assert_has_calls([ - mock.call(alice_license), + mock.call(alice_assigned_license), mock.call(bob_license), ]) @@ -2191,6 +2195,47 @@ def test_bulk_revoke_happy_path(self, mock_revoke_license, mock_execute_post_rev mock.call(**revoke_bob_license_result), ]) + @mock.patch('license_manager.apps.api.v1.views.execute_post_revocation_tasks') + @mock.patch('license_manager.apps.api.v1.views.revoke_license') + def test_bulk_revoke_multiple_activated_same_email(self, mock_revoke_license, mock_execute_post_revocation_tasks): + """ + Test the edge condition where one email in a single plan has multiple activated licenses. + """ + self._setup_request_jwt(user=self.user) + alice_license_1 = LicenseFactory.create( + uuid='00000000-0000-0000-0000-000000000000', + user_email='alice@example.com', status=constants.ACTIVATED, + ) + alice_license_2 = alice_license_1 = LicenseFactory.create( + uuid='00000000-0000-0000-0000-000000000001', + user_email='alice@example.com', status=constants.ACTIVATED, + ) + self.subscription_plan.licenses.set([alice_license_1, alice_license_2]) + + request_payload = { + 'user_emails': [ + 'alice@example.com', + ], + } + + revoke_alice_license_result = { + 'revoked_license': alice_license_1, 'original_status': alice_license_1.status, + } + mock_revoke_license.side_effect = [revoke_alice_license_result] + + response = self.api_client.post(self.bulk_revoke_license_url, request_payload) + + assert response.status_code == status.HTTP_204_NO_CONTENT + # Since alice has multiple licenses, we should only revoke the first one (which is arbitrarily + # the one with the smallest uuid). + mock_revoke_license.assert_has_calls([ + mock.call(alice_license_1), + ]) + + mock_execute_post_revocation_tasks.assert_has_calls([ + mock.call(**revoke_alice_license_result), + ]) + @ddt.data( ([{'name': 'user_email', 'filter_value': 'al'}], ['alice@example.com']), ([{'name': 'status_in', 'filter_value': [constants.ACTIVATED]}], ['alice@example.com']), diff --git a/license_manager/apps/api/v1/views.py b/license_manager/apps/api/v1/views.py index 149c480d..a263a3d5 100644 --- a/license_manager/apps/api/v1/views.py +++ b/license_manager/apps/api/v1/views.py @@ -1025,11 +1025,28 @@ def _revoke_by_email_and_plan(self, user_email, subscription_plan): """ Helper method to revoke a single license. """ + user_license = None try: - user_license = subscription_plan.licenses.get( + user_licenses = subscription_plan.licenses.filter( user_email=user_email, status__in=constants.REVOCABLE_LICENSE_STATUSES, - ) + ).order_by('uuid') + if len(user_licenses) == 0: + raise LicenseNotFoundError(user_email, subscription_plan, constants.REVOCABLE_LICENSE_STATUSES) + if len(user_licenses) == 1: + user_license = user_licenses[0] + else: + # if this email address has multiple licenses, prefer to revoke the assigned one. + assigned_licenses = [record for record in user_licenses if record.status == constants.ASSIGNED] + if assigned_licenses: + user_license = assigned_licenses[0] + else: + # Otherwise, we're in a super weird, maybe-impossible edge case where this email + # address is associated with multiple activated licenses in the same plan. + # So we'll just revoke the first one in the result set, which + # *must* be an activated one, because the revocable states are only (assigned, activated), + # and we know from the `if` conditional that there are no assigned licenses. + user_license = user_licenses[0] except License.DoesNotExist as exc: logger.error(exc) raise LicenseNotFoundError(user_email, subscription_plan, constants.REVOCABLE_LICENSE_STATUSES) from exc