Skip to content

Commit

Permalink
fix: have the bulk_revoke endpoint deal gracefully with multiple lice…
Browse files Browse the repository at this point in the history
…nses for the same email
  • Loading branch information
iloveagent57 committed May 10, 2024
1 parent 4b1e847 commit 8d4beac
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 5 deletions.
51 changes: 48 additions & 3 deletions license_manager/apps/api/v1/tests/test_views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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='[email protected]', status=constants.ACTIVATED)
alice_assigned_license = LicenseFactory.create(user_email='[email protected]', status=constants.ASSIGNED)
bob_license = LicenseFactory.create(user_email='[email protected]', 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': [
Expand All @@ -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),
])

Expand All @@ -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='[email protected]', status=constants.ACTIVATED,
)
alice_license_2 = alice_license_1 = LicenseFactory.create(
uuid='00000000-0000-0000-0000-000000000001',
user_email='[email protected]', status=constants.ACTIVATED,
)
self.subscription_plan.licenses.set([alice_license_1, alice_license_2])

request_payload = {
'user_emails': [
'[email protected]',
],
}

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'}], ['[email protected]']),
([{'name': 'status_in', 'filter_value': [constants.ACTIVATED]}], ['[email protected]']),
Expand Down
21 changes: 19 additions & 2 deletions license_manager/apps/api/v1/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down

0 comments on commit 8d4beac

Please sign in to comment.