Merge pull request #2529 from alphagov/add-expiry-date-to-constraint

Allow api_keys names to be reused

app/models.py

@@ -748,7 +748,7 @@ class ApiKey(db.Model, Versioned):
     created_by_id = db.Column(UUID(as_uuid=True), db.ForeignKey('users.id'), index=True, nullable=False)
 
     __table_args__ = (
-        UniqueConstraint('service_id', 'name', name='uix_service_to_key_name'),
+        Index('uix_service_to_key_name', 'service_id', 'name', unique=True, postgresql_where=expiry_date.is_(None)),
     )
 
     @property

migrations/versions/0295_api_key_constraint.py

@@ -0,0 +1,23 @@
+"""
+
+Revision ID: 0295_api_key_constraint
+Revises: 0294_add_verify_reply_to
+Create Date: 2019-06-04 13:49:50.685493
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+revision = '0295_api_key_constraint'
+down_revision = '0294_add_verify_reply_to'
+
+
+def upgrade():
+    op.drop_constraint('uix_service_to_key_name', 'api_keys', type_='unique')
+    op.create_index('uix_service_to_key_name', 'api_keys', ['service_id', 'name'], unique=True,
+                    postgresql_where=sa.text('expiry_date IS NULL'))
+
+
+def downgrade():
+    op.drop_index('uix_service_to_key_name', table_name='api_keys')
+    op.create_unique_constraint('uix_service_to_key_name', 'api_keys', ['service_id', 'name'])

tests/app/dao/test_api_key_dao.py

@@ -4,11 +4,13 @@ import pytest
 from sqlalchemy.exc import IntegrityError
 from sqlalchemy.orm.exc import NoResultFound
 
-from app.dao.api_key_dao import (save_model_api_key,
+from app.dao.api_key_dao import (
-                                 get_model_api_keys,
+    save_model_api_key,
-                                 get_unsigned_secrets,
+    get_model_api_keys,
-                                 get_unsigned_secret,
+    get_unsigned_secrets,
-                                 expire_api_key)
+    get_unsigned_secret,
+    expire_api_key
+)
 from app.models import ApiKey, KEY_TYPE_NORMAL
 
 
@@ -82,6 +84,22 @@ def test_should_not_allow_duplicate_key_names_per_service(sample_api_key, fake_u
         save_model_api_key(api_key)
 
 
+def test_save_api_key_can_create_key_with_same_name_if_other_is_expired(sample_service):
+    expired_api_key = ApiKey(**{'service': sample_service,
+                                'name': "normal api key",
+                                'created_by': sample_service.created_by,
+                                'key_type': KEY_TYPE_NORMAL,
+                                'expiry_date': datetime.utcnow()})
+    save_model_api_key(expired_api_key)
+    api_key = ApiKey(**{'service': sample_service,
+                        'name': "normal api key",
+                        'created_by': sample_service.created_by,
+                        'key_type': KEY_TYPE_NORMAL})
+    save_model_api_key(api_key)
+    keys = ApiKey.query.all()
+    assert len(keys) == 2
+
+
 def test_save_api_key_should_not_create_new_service_history(sample_service):
     from app.models import Service
 
@@ -99,9 +117,9 @@ def test_save_api_key_should_not_create_new_service_history(sample_service):
 
 @pytest.mark.parametrize('days_old, expected_length', [(5, 1), (8, 0)])
 def test_should_not_return_revoked_api_keys_older_than_7_days(
-    sample_service,
+        sample_service,
-    days_old,
+        days_old,
-    expected_length
+        expected_length
 ):
     expired_api_key = ApiKey(**{'service': sample_service,
                                 'name': sample_service.name,