From 87945153189764545afabaffd613055f37e82807 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?V=C3=ADctor=20Fern=C3=A1ndez=20Poyatos?= Date: Fri, 10 Oct 2025 12:35:27 +0200 Subject: [PATCH] fix(api-keys): make name required and unique (#8891) --- .../backend/api/migrations/0048_api_key.py | 15 +++- api/src/backend/api/models.py | 5 ++ api/src/backend/api/specs/v1.yaml | 30 +++++--- api/src/backend/api/tests/test_views.py | 70 ++++++++++++++++++- api/src/backend/api/v1/serializers.py | 18 +++++ 5 files changed, 124 insertions(+), 14 deletions(-) diff --git a/api/src/backend/api/migrations/0048_api_key.py b/api/src/backend/api/migrations/0048_api_key.py index 9bc68af8e6..abcbe212ef 100644 --- a/api/src/backend/api/migrations/0048_api_key.py +++ b/api/src/backend/api/migrations/0048_api_key.py @@ -2,6 +2,7 @@ import uuid +import django.core.validators import django.db.models.deletion import drf_simple_apikey.models from django.conf import settings @@ -20,7 +21,13 @@ class Migration(migrations.Migration): migrations.CreateModel( name="TenantAPIKey", fields=[ - ("name", models.CharField(blank=True, max_length=255, null=True)), + ( + "name", + models.CharField( + max_length=255, + validators=[django.core.validators.MinLengthValidator(3)], + ), + ), ( "expiry_date", models.DateTimeField( @@ -110,7 +117,11 @@ class Migration(migrations.Migration): "constraints": [ models.UniqueConstraint( fields=("tenant_id", "prefix"), name="unique_api_key_prefixes" - ) + ), + models.UniqueConstraint( + fields=("tenant_id", "name"), + name="unique_api_key_name_per_tenant", + ), ], }, ), diff --git a/api/src/backend/api/models.py b/api/src/backend/api/models.py index 048f1ca7db..c3cd30d932 100644 --- a/api/src/backend/api/models.py +++ b/api/src/backend/api/models.py @@ -220,6 +220,7 @@ class Membership(models.Model): class TenantAPIKey(AbstractAPIKey, RowLevelSecurityProtectedModel): id = models.UUIDField(primary_key=True, default=uuid4, editable=False) + name = models.CharField(max_length=100, validators=[MinLengthValidator(3)]) prefix = models.CharField( max_length=11, unique=True, @@ -255,6 +256,10 @@ class TenantAPIKey(AbstractAPIKey, RowLevelSecurityProtectedModel): fields=("tenant_id", "prefix"), name="unique_api_key_prefixes", ), + models.UniqueConstraint( + fields=("tenant_id", "name"), + name="unique_api_key_name_per_tenant", + ), ] indexes = [ diff --git a/api/src/backend/api/specs/v1.yaml b/api/src/backend/api/specs/v1.yaml index d282021d49..cd5e9d4af1 100644 --- a/api/src/backend/api/specs/v1.yaml +++ b/api/src/backend/api/specs/v1.yaml @@ -12460,8 +12460,8 @@ components: properties: name: type: string - nullable: true - maxLength: 255 + minLength: 3 + maxLength: 100 prefix: type: string readOnly: true @@ -12481,6 +12481,8 @@ components: readOnly: true nullable: true description: Last time this API key was used for authentication + required: + - name relationships: type: object properties: @@ -15767,8 +15769,8 @@ components: properties: name: type: string - nullable: true - maxLength: 255 + maxLength: 100 + minLength: 3 prefix: type: string readOnly: true @@ -15790,6 +15792,8 @@ components: format: date-time nullable: true description: Last time this API key was used for authentication + required: + - name relationships: type: object properties: @@ -15836,8 +15840,8 @@ components: properties: name: type: string - nullable: true - maxLength: 255 + maxLength: 100 + minLength: 3 prefix: type: string readOnly: true @@ -15863,6 +15867,8 @@ components: api_key: type: string readOnly: true + required: + - name relationships: type: object properties: @@ -15913,8 +15919,8 @@ components: properties: name: type: string - nullable: true - maxLength: 255 + minLength: 3 + maxLength: 100 prefix: type: string readOnly: true @@ -15941,6 +15947,8 @@ components: api_key: type: string readOnly: true + required: + - name relationships: type: object properties: @@ -16008,8 +16016,8 @@ components: properties: name: type: string - nullable: true - maxLength: 255 + maxLength: 100 + minLength: 3 prefix: type: string readOnly: true @@ -16028,6 +16036,8 @@ components: readOnly: true nullable: true description: Last time this API key was used for authentication + required: + - name relationships: type: object properties: diff --git a/api/src/backend/api/tests/test_views.py b/api/src/backend/api/tests/test_views.py index 71f946116c..11dc1c8b33 100644 --- a/api/src/backend/api/tests/test_views.py +++ b/api/src/backend/api/tests/test_views.py @@ -7739,8 +7739,6 @@ class TestTenantApiKeyViewSet: ( [ {"name": "New API Key"}, - {"name": ""}, - {}, ] ), ) @@ -7781,6 +7779,18 @@ class TestTenantApiKeyViewSet: {"name": "Invalid Expiry", "expires_at": "not-a-date"}, "expires_at", ), + ( + {"name": ""}, + "name", + ), + ( + {}, + "name", + ), + ( + {"name": "AB"}, # Too short (min length is 3) + "name", + ), ] ), ) @@ -7809,6 +7819,58 @@ class TestTenantApiKeyViewSet: == f"/data/attributes/{error_pointer}" ) + def test_api_keys_create_duplicate_name( + self, authenticated_client, api_keys_fixture + ): + """Test creating an API key with a duplicate name fails.""" + # Use the name of an existing API key + existing_name = api_keys_fixture[0].name + data = { + "data": { + "type": "api-keys", + "attributes": { + "name": existing_name, + }, + } + } + response = authenticated_client.post( + reverse("api-key-list"), + data=json.dumps(data), + content_type="application/vnd.api+json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "errors" in response.json() + error_detail = response.json()["errors"][0]["detail"] + assert "already exists" in error_detail.lower() + + def test_api_keys_update_duplicate_name( + self, authenticated_client, api_keys_fixture + ): + """Test updating an API key with a duplicate name fails.""" + # Get two different API keys + first_api_key = api_keys_fixture[0] + second_api_key = api_keys_fixture[1] + + # Try to update the second API key to have the same name as the first one + data = { + "data": { + "type": "api-keys", + "id": str(second_api_key.id), + "attributes": { + "name": first_api_key.name, + }, + } + } + response = authenticated_client.patch( + reverse("api-key-detail", kwargs={"pk": second_api_key.id}), + data=json.dumps(data), + content_type="application/vnd.api+json", + ) + assert response.status_code == status.HTTP_400_BAD_REQUEST + assert "errors" in response.json() + error_detail = response.json()["errors"][0]["detail"] + assert "already exists" in error_detail.lower() + def test_api_keys_create_multiple_unique_prefixes( self, authenticated_client, api_keys_fixture ): @@ -8260,6 +8322,10 @@ class TestTenantApiKeyViewSet: assert included_user["type"] == "users" assert included_user["id"] == str(api_key.entity.id) + # Refresh entity from database to get current state + # (in case other tests modified the shared session-scoped user fixture) + api_key.entity.refresh_from_db() + # Verify UserIncludeSerializer fields are present user_attrs = included_user["attributes"] assert "name" in user_attrs diff --git a/api/src/backend/api/v1/serializers.py b/api/src/backend/api/v1/serializers.py index 70e002cd10..8456c5a081 100644 --- a/api/src/backend/api/v1/serializers.py +++ b/api/src/backend/api/v1/serializers.py @@ -2872,6 +2872,13 @@ class TenantApiKeyCreateSerializer(RLSSerializer, BaseWriteSerializer): "api_key": {"read_only": True}, } + def validate_name(self, value): + """Validate that the name is unique within the tenant.""" + tenant_id = self.context.get("tenant_id") + if TenantAPIKey.objects.filter(tenant_id=tenant_id, name=value).exists(): + raise ValidationError("An API key with this name already exists.") + return value + def get_api_key(self, obj): """Return the raw API key if it was stored during creation.""" return getattr(obj, "_raw_api_key", None) @@ -2913,3 +2920,14 @@ class TenantApiKeyUpdateSerializer(RLSSerializer, BaseWriteSerializer): "inserted_at": {"read_only": True}, "last_used_at": {"read_only": True}, } + + def validate_name(self, value): + """Validate that the name is unique within the tenant, excluding current instance.""" + tenant_id = self.context.get("tenant_id") + if ( + TenantAPIKey.objects.filter(tenant_id=tenant_id, name=value) + .exclude(id=self.instance.id) + .exists() + ): + raise ValidationError("An API key with this name already exists.") + return value