fix(api): reject API keys whose owning user was deleted (#12210)

api/changelog.d/12210.fixed.md

@@ -0,0 +1 @@
+Authentication with an API key whose owning user was deleted now returns `401` instead of an unhandled `AttributeError`, and user deletion now revokes the user's API keys across all their tenants

api/src/backend/api/authentication.py

@@ -1,3 +1,4 @@
+import logging
 from math import isfinite
 from uuid import UUID
 
@@ -5,6 +6,7 @@ from api.db_router import MainRouter
 from api.models import TenantAPIKey, TenantAPIKeyManager
 from cryptography.fernet import InvalidToken
 from django.core.exceptions import ObjectDoesNotExist
+from django.db import transaction
 from django.utils import timezone
 from drf_simple_apikey.backends import APIKeyAuthentication as BaseAPIKeyAuth
 from drf_simple_apikey.crypto import get_crypto
@@ -14,6 +16,16 @@ from rest_framework.exceptions import AuthenticationFailed
 from rest_framework.request import Request
 from rest_framework_simplejwt.authentication import JWTAuthentication
 
+logger = logging.getLogger(__name__)
+
+
+class OrphanedAPIKeyError(Exception):
+    """Raised when an API key outlived the user that owns it.
+
+    Handled by `authenticate`, which commits the revocation written while detecting it
+    and then rejects the request with `AuthenticationFailed`.
+    """
+
 
 class TenantAPIKeyAuthentication(BaseAPIKeyAuth):
     model = TenantAPIKey
@@ -24,10 +36,13 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth):
     def _authenticate_credentials(self, request, key):
         """
         Override to use admin connection, bypassing RLS during authentication.
+
+        Returns the validated API key row, locked with `select_for_update`, so callers
+        must run inside `transaction.atomic(using=MainRouter.admin_db)`.
         """
         try:
             payload = self.key_crypto.decrypt(key)
-        except ValueError:
+        except (ValueError, InvalidToken):
             raise AuthenticationFailed("Invalid API Key.")
 
         if not isinstance(payload, dict):
@@ -52,13 +67,33 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth):
             raise AuthenticationFailed("API Key has already expired.")
 
         try:
-            api_key = self.model.objects.using(MainRouter.admin_db).get(id=api_key_pk)
+            api_key = (
+                self.model.objects.using(MainRouter.admin_db)
+                .select_for_update()
+                .get(id=api_key_pk)
+            )
         except ObjectDoesNotExist:
             raise AuthenticationFailed("No entity matching this api key.")
 
         if api_key.revoked:
             raise AuthenticationFailed("This API Key has been revoked.")
 
+        # `entity` is nullable and `on_delete=SET_NULL` leaves the key behind when its
+        # owner is deleted, so a key can outlive its user. Reject it here: further down
+        # the authentication would return `None` as the authenticated user, which blows
+        # up while building the auth dict and surfaces as a 500 instead of a 401.
+        # Revoke it as well, so it stops showing up as active and later attempts fail
+        # the `revoked` check above like any other revoked key.
+        if api_key.entity_id is None:
+            api_key.revoked = True
+            api_key.save(update_fields=["revoked"], using=MainRouter.admin_db)
+            logger.warning(
+                "Revoked orphaned API key: prefix=%s tenant=%s",
+                api_key.prefix,
+                api_key.tenant_id,
+            )
+            raise OrphanedAPIKeyError
+
         client_ip = request.META.get(package_settings.IP_ADDRESS_HEADER)
         if api_key.blacklisted_ips and client_ip in api_key.blacklisted_ips:
             raise AuthenticationFailed("Access denied from blacklisted IP.")
@@ -66,7 +101,7 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth):
         if api_key.whitelisted_ips and client_ip not in api_key.whitelisted_ips:
             raise AuthenticationFailed("Access restricted to specific IP addresses.")
 
-        return api_key.entity, key
+        return api_key
 
     def authenticate(self, request: Request):
         prefixed_key = self.get_key(request)
@@ -77,36 +112,34 @@ class TenantAPIKeyAuthentication(BaseAPIKeyAuth):
         except ValueError:
             raise AuthenticationFailed("Invalid API Key.")
 
-        try:
-            entity, _ = self._authenticate_credentials(request, key)
-        except InvalidToken:
-            raise AuthenticationFailed("Invalid API Key.")
+        # Validation, the `last_used_at` update and the auth claims all read the same
+        # row, locked until the transaction ends. Looking the key up a second time to
+        # build the claims used to leave a window where a key revoked or orphaned right
+        # after passing validation still authenticated.
+        with transaction.atomic(using=MainRouter.admin_db):
+            try:
+                api_key = self._authenticate_credentials(request, key)
+            except OrphanedAPIKeyError:
+                # Rejected below instead of here: leaving the block normally commits
+                # the revocation `_authenticate_credentials` wrote, while raising from
+                # inside would roll it back.
+                pass
+            else:
+                # The prefix used to be checked by the second lookup
+                if api_key.prefix != prefix:
+                    raise AuthenticationFailed("Invalid API Key.")
 
-        # Get the API key instance to update last_used_at and retrieve tenant info
-        # We need to decrypt again to get the pk (already validated by _authenticate_credentials)
-        payload = self.key_crypto.decrypt(key)
-        api_key_pk = payload["_pk"]
+                api_key.last_used_at = timezone.now()
+                api_key.save(update_fields=["last_used_at"], using=MainRouter.admin_db)
 
-        # Convert string UUID back to UUID object for lookup
-        if isinstance(api_key_pk, str):
-            api_key_pk = UUID(api_key_pk)
+                entity = api_key.entity
+                return entity, {
+                    "tenant_id": str(api_key.tenant_id),
+                    "sub": str(entity.id),
+                    "api_key_prefix": api_key.prefix,
+                }
 
-        try:
-            api_key_instance = TenantAPIKey.objects.using(MainRouter.admin_db).get(
-                id=api_key_pk, prefix=prefix
-            )
-        except TenantAPIKey.DoesNotExist:
-            raise AuthenticationFailed("Invalid API Key.")
-
-        # Update last_used_at
-        api_key_instance.last_used_at = timezone.now()
-        api_key_instance.save(update_fields=["last_used_at"], using=MainRouter.admin_db)
-
-        return entity, {
-            "tenant_id": str(api_key_instance.tenant_id),
-            "sub": str(api_key_instance.entity.id),
-            "api_key_prefix": prefix,
-        }
+        raise AuthenticationFailed("No entity matching this api key.")
 
 
 class CombinedJWTOrAPIKeyAuthentication(BaseAuthentication):

api/src/backend/api/signals.py

@@ -1,3 +1,4 @@
+from api.db_router import MainRouter
 from api.db_utils import delete_related_daily_task
 from api.models import (
     LighthouseProviderConfiguration,
@@ -47,8 +48,15 @@ def revoke_user_api_keys(sender, instance, **kwargs):  # noqa: F841
 
     The entity field will be set to NULL by on_delete=SET_NULL,
     but we explicitly revoke the keys to prevent further use.
+
+    The update runs on the admin connection because `api_keys` is RLS protected and its
+    policy denies every row when `api.tenant_id` is unset. Users are deleted through the
+    admin connection and may belong to several tenants, so going through the default
+    connection would silently revoke nothing, or only the keys of the active tenant.
     """
-    TenantAPIKey.objects.filter(entity=instance).update(revoked=True)
+    TenantAPIKey.objects.using(MainRouter.admin_db).filter(entity=instance).update(
+        revoked=True
+    )
 
 
 @receiver(post_delete, sender=Membership)
@@ -58,8 +66,12 @@ def revoke_membership_api_keys(sender, instance, **kwargs):  # noqa: F841
 
     When a membership is deleted, all API keys created by that user
     in that tenant should be revoked to prevent further access.
+
+    Uses the admin connection for the same reason as `revoke_user_api_keys`: the RLS
+    policy on `api_keys` denies every row when `api.tenant_id` is unset, which is the
+    case when the membership is removed as a cascade of a user deletion.
     """
-    TenantAPIKey.objects.filter(
+    TenantAPIKey.objects.using(MainRouter.admin_db).filter(
         entity_id=instance.user_id, tenant_id=instance.tenant_id
     ).update(revoked=True)
 

api/src/backend/api/tests/integration/test_authentication.py

@@ -4,8 +4,11 @@ from datetime import UTC, datetime, timedelta
 from uuid import uuid4
 
 import pytest
+from api.db_router import MainRouter
 from api.models import Membership, Role, TenantAPIKey, User, UserRoleRelationship
+from api.signals import revoke_membership_api_keys, revoke_user_api_keys
 from conftest import TEST_PASSWORD, get_api_tokens, get_authorization_header
+from django.db.utils import ConnectionDoesNotExist
 from django.urls import reverse
 from drf_simple_apikey.crypto import get_crypto
 from rest_framework.test import APIClient
@@ -625,6 +628,34 @@ class TestAPIKeyErrors:
         assert response.status_code == 401
         assert "API Key has been revoked." in response.json()["errors"][0]["detail"]
 
+    def test_orphaned_api_key_rejected(
+        self, create_test_user, tenants_fixture, api_keys_fixture
+    ):
+        """Key whose owning user was deleted returns 401 instead of 500."""
+        client = APIClient()
+
+        api_key = api_keys_fixture[0]
+        # `on_delete=SET_NULL` leaves the key behind with no entity when the owner goes
+        TenantAPIKey.objects.filter(id=api_key.id).update(entity=None)
+
+        api_key_headers = get_api_key_header(api_key._raw_key)
+        response = client.get(reverse("provider-list"), headers=api_key_headers)
+
+        assert response.status_code == 401
+        assert (
+            "No entity matching this api key." in response.json()["errors"][0]["detail"]
+        )
+
+        # The orphaned key is revoked on use; retries fail the regular revoked check
+        api_key.refresh_from_db()
+        assert api_key.revoked is True
+
+        retry_response = client.get(reverse("provider-list"), headers=api_key_headers)
+        assert retry_response.status_code == 401
+        assert (
+            "API Key has been revoked." in retry_response.json()["errors"][0]["detail"]
+        )
+
     def test_non_existent_api_key(self, create_test_user, tenants_fixture):
         """Key UUID doesn't exist in database."""
         client = APIClient()
@@ -817,6 +848,93 @@ class TestAPIKeyTenantIsolation:
         error_detail = response_json["errors"][0]["detail"]
         assert "revoked" in error_detail.lower()
 
+    def test_deleting_user_revokes_api_keys_in_every_tenant(self, tenants_fixture):
+        """Deleting a user revokes their keys in all their tenants, not just one."""
+        first_tenant, second_tenant = tenants_fixture[0], tenants_fixture[1]
+
+        test_user = User.objects.create_user(
+            name="multi_tenant_user",
+            email="multi_tenant_user@prowler.com",
+            password=TEST_PASSWORD,
+        )
+        for tenant in (first_tenant, second_tenant):
+            Membership.objects.create(
+                user=test_user, tenant=tenant, role=Membership.RoleChoices.OWNER
+            )
+
+        first_key, _ = TenantAPIKey.objects.create_api_key(
+            name="Key in first tenant", tenant_id=first_tenant.id, entity=test_user
+        )
+        second_key, _ = TenantAPIKey.objects.create_api_key(
+            name="Key in second tenant", tenant_id=second_tenant.id, entity=test_user
+        )
+
+        test_user.delete()
+
+        first_key.refresh_from_db()
+        second_key.refresh_from_db()
+        assert first_key.revoked is True
+        assert second_key.revoked is True
+        # `on_delete=SET_NULL` orphans the keys, so revoking them is what keeps them
+        # from authenticating
+        assert first_key.entity_id is None
+        assert second_key.entity_id is None
+
+    def test_revoke_user_api_keys_uses_the_admin_connection(
+        self, monkeypatch, tenants_fixture
+    ):
+        """The revocation must not go through the default connection.
+
+        `api_keys` is RLS protected and its policy denies every row when `api.tenant_id`
+        is unset, which is the case while a user is deleted through the admin
+        connection: the update would silently revoke nothing and leave usable orphaned
+        keys behind.
+
+        Pointing `admin_db` at a missing alias is the only way to assert the connection
+        here, because the test suite runs on a single superuser database with
+        `MainRouter.admin_db` patched to "default" (see `conftest.py`), so RLS never
+        applies and both connections are otherwise indistinguishable.
+        """
+        test_user = User.objects.create_user(
+            name="admin_connection_user",
+            email="admin_connection_user@prowler.com",
+            password=TEST_PASSWORD,
+        )
+        Membership.objects.create(user=test_user, tenant=tenants_fixture[0])
+        TenantAPIKey.objects.create_api_key(
+            name="Key for admin connection check",
+            tenant_id=tenants_fixture[0].id,
+            entity=test_user,
+        )
+
+        monkeypatch.setattr(MainRouter, "admin_db", "missing_admin_alias")
+
+        with pytest.raises(ConnectionDoesNotExist):
+            revoke_user_api_keys(sender=User, instance=test_user)
+
+    def test_revoke_membership_api_keys_uses_the_admin_connection(
+        self, monkeypatch, tenants_fixture
+    ):
+        """Same as the user deletion case: this receiver also runs as its cascade."""
+        test_user = User.objects.create_user(
+            name="admin_connection_membership_user",
+            email="admin_connection_membership_user@prowler.com",
+            password=TEST_PASSWORD,
+        )
+        membership = Membership.objects.create(
+            user=test_user, tenant=tenants_fixture[0]
+        )
+        TenantAPIKey.objects.create_api_key(
+            name="Key for membership admin connection check",
+            tenant_id=tenants_fixture[0].id,
+            entity=test_user,
+        )
+
+        monkeypatch.setattr(MainRouter, "admin_db", "missing_admin_alias")
+
+        with pytest.raises(ConnectionDoesNotExist):
+            revoke_membership_api_keys(sender=Membership, instance=membership)
+
 
 @pytest.mark.django_db
 class TestAPIKeyLifecycle:

api/src/backend/api/tests/test_authentication.py

@@ -4,11 +4,17 @@ from unittest.mock import MagicMock, patch
 from uuid import uuid4
 
 import pytest
-from api.authentication import SSEAuthentication, TenantAPIKeyAuthentication
+from api.authentication import (
+    OrphanedAPIKeyError,
+    SSEAuthentication,
+    TenantAPIKeyAuthentication,
+)
 from api.db_router import MainRouter
 from api.models import TenantAPIKey
+from django.db import connections
 from django.db.models.query import QuerySet
 from django.test import RequestFactory
+from django.test.utils import CaptureQueriesContext
 from rest_framework.exceptions import AuthenticationFailed
 
 
@@ -38,13 +44,12 @@ class TestTenantAPIKeyAuthentication:
         request = request_factory.get("/")
 
         # Call the method
-        entity, auth_dict = auth_backend._authenticate_credentials(
-            request, encrypted_key
-        )
+        validated_key = auth_backend._authenticate_credentials(request, encrypted_key)
 
         # Verify that the entity is the user associated with the API key
-        assert entity == api_key.entity
-        assert entity.id == api_key.entity.id
+        assert validated_key.id == api_key.id
+        assert validated_key.entity == api_key.entity
+        assert validated_key.entity.id == api_key.entity.id
 
     def test_authenticate_credentials_restores_manager_on_success(
         self, auth_backend, api_keys_fixture, request_factory
@@ -231,6 +236,120 @@ class TestTenantAPIKeyAuthentication:
 
         assert str(exc_info.value.detail) == "This API Key has been revoked."
 
+    def test_authenticate_credentials_orphaned_api_key(
+        self, auth_backend, api_keys_fixture, request_factory
+    ):
+        """Test credential validation fails when the owning user no longer exists."""
+        api_key = api_keys_fixture[0]
+        _, encrypted_key = api_key._raw_key.split(TenantAPIKey.objects.separator, 1)
+
+        # `entity` is what `on_delete=SET_NULL` leaves behind when the owner is deleted
+        TenantAPIKey.objects.filter(id=api_key.id).update(entity=None)
+
+        request = request_factory.get("/")
+
+        with pytest.raises(OrphanedAPIKeyError):
+            auth_backend._authenticate_credentials(request, encrypted_key)
+
+        # The orphaned key is revoked on use, so it stops showing up as active
+        api_key.refresh_from_db()
+        assert api_key.revoked is True
+
+    def test_authenticate_orphaned_api_key(
+        self, auth_backend, api_keys_fixture, request_factory
+    ):
+        """Test authentication fails with a key whose owning user was deleted.
+
+        Regression test: this used to raise `AttributeError: 'NoneType' object has no
+        attribute 'id'` while building the auth dict, which DRF re-raises as
+        `WrappedAttributeError` and turns into a 500 instead of a 401.
+        """
+        api_key = api_keys_fixture[0]
+        raw_key = api_key._raw_key
+
+        TenantAPIKey.objects.filter(id=api_key.id).update(entity=None)
+
+        request = request_factory.get("/")
+        request.META["HTTP_AUTHORIZATION"] = f"Api-Key {raw_key}"
+
+        with pytest.raises(AuthenticationFailed) as exc_info:
+            auth_backend.authenticate(request)
+
+        assert str(exc_info.value.detail) == "No entity matching this api key."
+
+        # The orphaned key is revoked on use; retries fail the regular revoked check
+        api_key.refresh_from_db()
+        assert api_key.revoked is True
+
+        with pytest.raises(AuthenticationFailed) as exc_info:
+            auth_backend.authenticate(request)
+
+        assert str(exc_info.value.detail) == "This API Key has been revoked."
+
+    def test_authenticate_reads_the_api_key_once_under_a_row_lock(
+        self, auth_backend, api_keys_fixture, request_factory
+    ):
+        """Test the API key is read a single time and the row is locked.
+
+        Validation, the `last_used_at` update and the claims must all come from the
+        same authoritative row: a second, unlocked lookup would reopen the window
+        where a key revoked in between still authenticates.
+        """
+        api_key = api_keys_fixture[0]
+
+        request = request_factory.get("/")
+        request.META["HTTP_AUTHORIZATION"] = f"Api-Key {api_key._raw_key}"
+
+        with CaptureQueriesContext(connections[MainRouter.admin_db]) as captured:
+            auth_backend.authenticate(request)
+
+        api_key_selects = [
+            query["sql"]
+            for query in captured.captured_queries
+            if query["sql"].startswith("SELECT") and '"api_keys"' in query["sql"]
+        ]
+
+        assert len(api_key_selects) == 1
+        assert "FOR UPDATE" in api_key_selects[0]
+
+    def test_authenticate_ignores_revocation_after_the_locked_read(
+        self, auth_backend, api_keys_fixture, request_factory
+    ):
+        """Test the claims describe the row that was validated, not a later state.
+
+        Regression test: the key used to be looked up again to build the auth dict,
+        without rechecking `revoked` or `entity`. A key revoked or orphaned between
+        both reads still authenticated, and the claims came from that stale row. With
+        a single locked read the write below cannot land mid-authentication, and the
+        revocation only takes effect on the next request.
+        """
+        api_key = api_keys_fixture[0]
+        entity_at_validation = api_key.entity
+        original_save = TenantAPIKey.save
+
+        def revoke_and_orphan_before_saving(instance, *args, **kwargs):
+            # Runs after validation, right before the claims are built: the exact
+            # window a concurrent revocation or user deletion used to slip into
+            TenantAPIKey.objects.filter(id=api_key.id).update(revoked=True, entity=None)
+            return original_save(instance, *args, **kwargs)
+
+        request = request_factory.get("/")
+        request.META["HTTP_AUTHORIZATION"] = f"Api-Key {api_key._raw_key}"
+
+        with patch.object(TenantAPIKey, "save", revoke_and_orphan_before_saving):
+            entity, auth_dict = auth_backend.authenticate(request)
+
+        assert entity == entity_at_validation
+        assert auth_dict["sub"] == str(entity_at_validation.id)
+        assert auth_dict["tenant_id"] == str(api_key.tenant_id)
+        assert auth_dict["api_key_prefix"] == api_key.prefix
+
+        # The revoked key is rejected from the next request on
+        with pytest.raises(AuthenticationFailed) as exc_info:
+            auth_backend.authenticate(request)
+
+        assert str(exc_info.value.detail) == "This API Key has been revoked."
+
     def test_authenticate_expired_api_key(
         self, auth_backend, create_test_user, tenants_fixture, request_factory
     ):

api/src/backend/api/tests/test_views.py

@@ -15748,6 +15748,23 @@ class TestTenantApiKeyViewSet:
         data = response.json()["data"]
         assert len(data) == len(api_keys_fixture)
 
+    def test_api_keys_list_with_orphaned_key(
+        self, authenticated_client, api_keys_fixture
+    ):
+        """Test listing keys whose owner was deleted: `entity` is serialized as null."""
+        orphaned_key = api_keys_fixture[0]
+        TenantAPIKey.objects.filter(id=orphaned_key.id).update(entity=None)
+
+        response = authenticated_client.get(reverse("api-key-list"))
+
+        assert response.status_code == status.HTTP_200_OK
+        data = response.json()["data"]
+        assert len(data) == len(api_keys_fixture)
+        serialized_key = next(
+            item for item in data if item["id"] == str(orphaned_key.id)
+        )
+        assert serialized_key["relationships"]["entity"]["data"] is None
+
     def test_api_keys_list_empty(self, authenticated_client, tenants_fixture):
         """Test listing API keys when none exist returns empty list."""
         response = authenticated_client.get(reverse("api-key-list"))