From 01f298531d71ac991daefa6424448f2fe130aca1 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 16:55:28 +0530 Subject: [PATCH 01/12] Enhance API key management with encryption and decryption - Added encryption for API keys during storage using Fernet. - Implemented decryption for API keys when retrieved. - Updated API key creation to store encrypted keys. - Modified tests to verify the integrity of decrypted keys. - Introduced utility functions for key encryption and decryption in the security module. --- .../77dc462dc6b0_seed_organization_table.py | 10 +++- backend/app/core/security.py | 40 +++++++++++++ backend/app/crud/api_key.py | 60 +++++++++++++++---- backend/app/tests/api/routes/test_project.py | 2 + backend/app/tests/crud/test_api_key.py | 16 +++-- 5 files changed, 109 insertions(+), 19 deletions(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index 7acb8d407..784f0ffce 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -13,6 +13,7 @@ # Adjust the import based on your actual structure from app.models import Organization, Project, User, APIKey from passlib.context import CryptContext # To hash passwords securely +from app.core.security import get_password_hash, encrypt_api_key # Add imports for encryption # revision identifiers, used by Alembic. revision = "77dc462dc6b0" @@ -86,10 +87,17 @@ def create_user(session: Session, is_super: bool = True) -> User: def create_api_key(session: Session, user: User, organization: Organization) -> APIKey: """Create and return an API key for the user and organization.""" + # Generate raw key + raw_key = "ApiKey " + secrets.token_urlsafe(32) + # Hash the key + hashed_key = get_password_hash(raw_key) + # Encrypt the hashed key + encrypted_key = encrypt_api_key(hashed_key) + api_key = APIKey( user_id=user.id, organization_id=organization.id, - key="ApiKey " + secrets.token_urlsafe(32), + key=encrypted_key, # Store the encrypted hashed key ) session.add(api_key) session.commit() diff --git a/backend/app/core/security.py b/backend/app/core/security.py index 7aff7cfb3..335dde574 100644 --- a/backend/app/core/security.py +++ b/backend/app/core/security.py @@ -1,5 +1,9 @@ from datetime import datetime, timedelta, timezone from typing import Any +import base64 +from cryptography.fernet import Fernet +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC import jwt from passlib.context import CryptContext @@ -8,6 +12,26 @@ pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") +# Generate a key for API key encryption +def get_encryption_key() -> bytes: + """Generate a key for API key encryption using the app's secret key.""" + kdf = PBKDF2HMAC( + algorithm=hashes.SHA256(), + length=32, + salt=settings.SECRET_KEY.encode(), + iterations=100000, + ) + return base64.urlsafe_b64encode(kdf.derive(settings.SECRET_KEY.encode())) + +# Initialize Fernet with our encryption key +_fernet = None + +def get_fernet() -> Fernet: + """Get a Fernet instance with the encryption key.""" + global _fernet + if _fernet is None: + _fernet = Fernet(get_encryption_key()) + return _fernet ALGORITHM = "HS256" @@ -25,3 +49,19 @@ def verify_password(plain_password: str, hashed_password: str) -> bool: def get_password_hash(password: str) -> str: return pwd_context.hash(password) + + +def encrypt_api_key(api_key: str) -> str: + """Encrypt an API key before storage.""" + try: + return get_fernet().encrypt(api_key.encode()).decode() + except Exception as e: + raise ValueError(f"Failed to encrypt API key: {str(e)}") + + +def decrypt_api_key(encrypted_api_key: str) -> str: + """Decrypt an API key when retrieving it.""" + try: + return get_fernet().decrypt(encrypted_api_key.encode()).decode() + except Exception as e: + raise ValueError(f"Failed to decrypt API key: {str(e)}") diff --git a/backend/app/crud/api_key.py b/backend/app/crud/api_key.py index 309562de1..f0e7e2fcc 100644 --- a/backend/app/crud/api_key.py +++ b/backend/app/crud/api_key.py @@ -2,19 +2,37 @@ import secrets from datetime import datetime from sqlmodel import Session, select +from app.core.security import verify_password, get_password_hash, encrypt_api_key, decrypt_api_key +import base64 +from cryptography.hazmat.primitives import hashes +from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC +from app.core import settings -from app.models import APIKey, APIKeyPublic +from app.models.api_key import APIKey, APIKeyPublic + + +def generate_api_key() -> tuple[str, str]: + """Generate a new API key and its hash.""" + raw_key = "ApiKey " + secrets.token_urlsafe(32) + hashed_key = get_password_hash(raw_key) + return raw_key, hashed_key -# Create API Key def create_api_key( session: Session, organization_id: uuid.UUID, user_id: uuid.UUID ) -> APIKeyPublic: """ Generates a new API key for an organization and associates it with a user. + Returns the API key details with the raw key (shown only once). """ + # Generate raw key and its hash + raw_key = "ApiKey " + secrets.token_urlsafe(32) + hashed_key = get_password_hash(raw_key) + encrypted_key = encrypt_api_key(hashed_key) + + # Create API key record with encrypted hashed key api_key = APIKey( - key="ApiKey " + secrets.token_urlsafe(32), + key=encrypted_key, # Store the encrypted hashed key organization_id=organization_id, user_id=user_id, ) @@ -23,10 +41,13 @@ def create_api_key( session.commit() session.refresh(api_key) - return APIKeyPublic.model_validate(api_key) + # Set the raw key in the response (shown only once) + api_key_dict = api_key.model_dump() + api_key_dict["key"] = raw_key # Return the raw key to the user + + return APIKeyPublic.model_validate(api_key_dict) -# Get API Key by ID def get_api_key(session: Session, api_key_id: int) -> APIKeyPublic | None: """ Retrieves an API key by its ID if it exists and is not deleted. @@ -35,10 +56,12 @@ def get_api_key(session: Session, api_key_id: int) -> APIKeyPublic | None: select(APIKey).where(APIKey.id == api_key_id, APIKey.is_deleted == False) ).first() - return APIKeyPublic.model_validate(api_key) if api_key else None + if api_key: + # Return the API key without decrypting (we don't want to expose the hashed key) + return APIKeyPublic.model_validate(api_key) + return None -# Get API Keys for an Organization def get_api_keys_by_organization( session: Session, organization_id: uuid.UUID ) -> list[APIKeyPublic]: @@ -51,10 +74,10 @@ def get_api_keys_by_organization( ) ).all() + # Return the API keys without decrypting (we don't want to expose the hashed keys) return [APIKeyPublic.model_validate(api_key) for api_key in api_keys] -# Soft Delete (Revoke) API Key def delete_api_key(session: Session, api_key_id: int) -> None: """ Soft deletes (revokes) an API key by marking it as deleted. @@ -73,11 +96,18 @@ def delete_api_key(session: Session, api_key_id: int) -> None: def get_api_key_by_value(session: Session, api_key_value: str) -> APIKey | None: """ - Retrieve an API Key record by its value. + Retrieve an API Key record by verifying the provided key against stored hashes. """ - return session.exec( - select(APIKey).where(APIKey.key == api_key_value, APIKey.is_deleted == False) - ).first() + # Get all active API keys + api_keys = session.exec(select(APIKey).where(APIKey.is_deleted == False)).all() + + # Check each key + for api_key in api_keys: + # Decrypt the stored key before verification + decrypted_key = decrypt_api_key(api_key.key) + if verify_password(api_key_value, decrypted_key): + return api_key + return None def get_api_key_by_user_org( @@ -91,4 +121,8 @@ def get_api_key_by_user_org( APIKey.user_id == user_id, APIKey.is_deleted == False, ) - return session.exec(statement).first() + api_key = session.exec(statement).first() + if api_key: + # Decrypt the key before returning + api_key.key = decrypt_api_key(api_key.key) + return api_key diff --git a/backend/app/tests/api/routes/test_project.py b/backend/app/tests/api/routes/test_project.py index 82ea7593d..98d1f96da 100644 --- a/backend/app/tests/api/routes/test_project.py +++ b/backend/app/tests/api/routes/test_project.py @@ -1,6 +1,7 @@ import pytest from fastapi.testclient import TestClient from sqlmodel import Session +from app.core.security import decrypt_api_key, verify_password from app.main import app from app.core.config import settings @@ -10,6 +11,7 @@ from app.tests.utils.utils import random_lower_string, random_email from app.crud.project import create_project, get_project_by_id from app.crud.organization import create_organization +from app.crud import api_key as api_key_crud client = TestClient(app) diff --git a/backend/app/tests/crud/test_api_key.py b/backend/app/tests/crud/test_api_key.py index 23c2837b1..8c45a7503 100644 --- a/backend/app/tests/crud/test_api_key.py +++ b/backend/app/tests/crud/test_api_key.py @@ -4,7 +4,7 @@ from app.crud import api_key as api_key_crud from app.models import APIKey, User, Organization from app.tests.utils.utils import random_email -from app.core.security import get_password_hash +from app.core.security import get_password_hash, verify_password, decrypt_api_key # Helper function to create a user @@ -48,7 +48,8 @@ def test_get_api_key(db: Session) -> None: assert retrieved_key is not None assert retrieved_key.id == created_key.id - assert retrieved_key.key == created_key.key + # The key should be decrypted when retrieved + assert verify_password(created_key.key, decrypt_api_key(retrieved_key.key)) def test_get_api_key_not_found(db: Session) -> None: @@ -67,8 +68,10 @@ def test_get_api_keys_by_organization(db: Session) -> None: api_keys = api_key_crud.get_api_keys_by_organization(db, org.id) assert len(api_keys) == 2 - assert any(key.id == api_key1.id for key in api_keys) - assert any(key.id == api_key2.id for key in api_keys) + # Verify both keys can be decrypted and verified + for key in api_keys: + assert verify_password(api_key1.key if key.id == api_key1.id else api_key2.key, + decrypt_api_key(key.key)) def test_delete_api_key(db: Session) -> None: @@ -105,7 +108,8 @@ def test_get_api_key_by_value(db: Session) -> None: assert retrieved_key is not None assert retrieved_key.id == api_key.id - assert retrieved_key.key == api_key.key + # The key should be verified against the stored hash + assert verify_password(api_key.key, decrypt_api_key(retrieved_key.key)) def test_get_api_key_by_user_org(db: Session) -> None: @@ -119,6 +123,8 @@ def test_get_api_key_by_user_org(db: Session) -> None: assert retrieved_key.id == api_key.id assert retrieved_key.organization_id == org.id assert retrieved_key.user_id == user.id + # The key should already be decrypted by get_api_key_by_user_org + assert retrieved_key.key is not None def test_get_api_key_by_user_org_not_found(db: Session) -> None: From 8e233970195ad460b501e6d35ecb2897fd2690a0 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 18:44:37 +0530 Subject: [PATCH 02/12] Refactor security imports and clean up code formatting --- .../versions/77dc462dc6b0_seed_organization_table.py | 7 +++++-- backend/app/core/security.py | 4 ++++ backend/app/crud/api_key.py | 7 ++++++- backend/app/tests/crud/test_api_key.py | 6 ++++-- 4 files changed, 19 insertions(+), 5 deletions(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index 784f0ffce..5fef932b2 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -13,7 +13,10 @@ # Adjust the import based on your actual structure from app.models import Organization, Project, User, APIKey from passlib.context import CryptContext # To hash passwords securely -from app.core.security import get_password_hash, encrypt_api_key # Add imports for encryption +from app.core.security import ( + get_password_hash, + encrypt_api_key, +) # Add imports for encryption # revision identifiers, used by Alembic. revision = "77dc462dc6b0" @@ -93,7 +96,7 @@ def create_api_key(session: Session, user: User, organization: Organization) -> hashed_key = get_password_hash(raw_key) # Encrypt the hashed key encrypted_key = encrypt_api_key(hashed_key) - + api_key = APIKey( user_id=user.id, organization_id=organization.id, diff --git a/backend/app/core/security.py b/backend/app/core/security.py index 335dde574..65594fcbf 100644 --- a/backend/app/core/security.py +++ b/backend/app/core/security.py @@ -12,6 +12,7 @@ pwd_context = CryptContext(schemes=["bcrypt"], deprecated="auto") + # Generate a key for API key encryption def get_encryption_key() -> bytes: """Generate a key for API key encryption using the app's secret key.""" @@ -23,9 +24,11 @@ def get_encryption_key() -> bytes: ) return base64.urlsafe_b64encode(kdf.derive(settings.SECRET_KEY.encode())) + # Initialize Fernet with our encryption key _fernet = None + def get_fernet() -> Fernet: """Get a Fernet instance with the encryption key.""" global _fernet @@ -33,6 +36,7 @@ def get_fernet() -> Fernet: _fernet = Fernet(get_encryption_key()) return _fernet + ALGORITHM = "HS256" diff --git a/backend/app/crud/api_key.py b/backend/app/crud/api_key.py index f0e7e2fcc..df92a07ab 100644 --- a/backend/app/crud/api_key.py +++ b/backend/app/crud/api_key.py @@ -2,7 +2,12 @@ import secrets from datetime import datetime from sqlmodel import Session, select -from app.core.security import verify_password, get_password_hash, encrypt_api_key, decrypt_api_key +from app.core.security import ( + verify_password, + get_password_hash, + encrypt_api_key, + decrypt_api_key, +) import base64 from cryptography.hazmat.primitives import hashes from cryptography.hazmat.primitives.kdf.pbkdf2 import PBKDF2HMAC diff --git a/backend/app/tests/crud/test_api_key.py b/backend/app/tests/crud/test_api_key.py index 8c45a7503..92e19b09c 100644 --- a/backend/app/tests/crud/test_api_key.py +++ b/backend/app/tests/crud/test_api_key.py @@ -70,8 +70,10 @@ def test_get_api_keys_by_organization(db: Session) -> None: assert len(api_keys) == 2 # Verify both keys can be decrypted and verified for key in api_keys: - assert verify_password(api_key1.key if key.id == api_key1.id else api_key2.key, - decrypt_api_key(key.key)) + assert verify_password( + api_key1.key if key.id == api_key1.id else api_key2.key, + decrypt_api_key(key.key), + ) def test_delete_api_key(db: Session) -> None: From 9f42ea5c2a83f4dd7a89fc47677c64d13bcc1f57 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 19:07:20 +0530 Subject: [PATCH 03/12] Refactor API key generation to use helper function --- backend/app/crud/api_key.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/backend/app/crud/api_key.py b/backend/app/crud/api_key.py index df92a07ab..9e487bc9c 100644 --- a/backend/app/crud/api_key.py +++ b/backend/app/crud/api_key.py @@ -30,9 +30,8 @@ def create_api_key( Generates a new API key for an organization and associates it with a user. Returns the API key details with the raw key (shown only once). """ - # Generate raw key and its hash - raw_key = "ApiKey " + secrets.token_urlsafe(32) - hashed_key = get_password_hash(raw_key) + # Generate raw key and its hash using the helper function + raw_key, hashed_key = generate_api_key() encrypted_key = encrypt_api_key(hashed_key) # Create API key record with encrypted hashed key From f8a88f3ab49cce27ade1e461fe3cb456b6157e61 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 19:28:19 +0530 Subject: [PATCH 04/12] Add unit tests for API key encryption and decryption --- backend/app/tests/core/test_security.py | 98 +++++++++++++++++++++++++ 1 file changed, 98 insertions(+) create mode 100644 backend/app/tests/core/test_security.py diff --git a/backend/app/tests/core/test_security.py b/backend/app/tests/core/test_security.py new file mode 100644 index 000000000..d9d443269 --- /dev/null +++ b/backend/app/tests/core/test_security.py @@ -0,0 +1,98 @@ +import pytest +from app.core.security import ( + get_password_hash, + verify_password, + encrypt_api_key, + decrypt_api_key, + get_encryption_key, +) + + +def test_encrypt_decrypt_api_key(): + """Test that API key encryption and decryption works correctly.""" + # Test data + test_key = "ApiKey test123456789" + + # Encrypt the key + encrypted_key = encrypt_api_key(test_key) + + # Verify encryption worked + assert encrypted_key is not None + assert encrypted_key != test_key + assert isinstance(encrypted_key, str) + + # Decrypt the key + decrypted_key = decrypt_api_key(encrypted_key) + + # Verify decryption worked + assert decrypted_key is not None + assert decrypted_key == test_key + + +def test_encrypt_api_key_edge_cases(): + """Test edge cases for API key encryption.""" + # Test empty string + empty_key = "" + encrypted_empty = encrypt_api_key(empty_key) + assert encrypted_empty is not None + assert decrypt_api_key(encrypted_empty) == empty_key + + # Test whitespace only + whitespace_key = " " + encrypted_whitespace = encrypt_api_key(whitespace_key) + assert encrypted_whitespace is not None + assert decrypt_api_key(encrypted_whitespace) == whitespace_key + + # Test very long input + long_key = "ApiKey " + "a" * 1000 + encrypted_long = encrypt_api_key(long_key) + assert encrypted_long is not None + assert decrypt_api_key(encrypted_long) == long_key + + +def test_encrypt_api_key_type_validation(): + """Test type validation for API key encryption.""" + # Test non-string inputs + invalid_inputs = [123, [], {}, True] + for invalid_input in invalid_inputs: + with pytest.raises(ValueError, match="Failed to encrypt API key"): + encrypt_api_key(invalid_input) + + +def test_encrypt_api_key_security(): + """Test security properties of API key encryption.""" + # Test that same input produces different encrypted output + test_key = "ApiKey test123456789" + encrypted1 = encrypt_api_key(test_key) + encrypted2 = encrypt_api_key(test_key) + assert encrypted1 != encrypted2 # Different encrypted outputs for same input + + +def test_encrypt_api_key_error_handling(): + """Test error handling in encrypt_api_key.""" + # Test with invalid input + with pytest.raises(ValueError, match="Failed to encrypt API key"): + encrypt_api_key(None) + + +def test_decrypt_api_key_error_handling(): + """Test error handling in decrypt_api_key.""" + # Test with invalid input + with pytest.raises(ValueError, match="Failed to decrypt API key"): + decrypt_api_key(None) + + # Test with invalid encrypted data + with pytest.raises(ValueError, match="Failed to decrypt API key"): + decrypt_api_key("invalid_encrypted_data") + + +def test_get_encryption_key(): + """Test that encryption key generation works correctly.""" + # Get the encryption key + key = get_encryption_key() + + # Verify the key + assert key is not None + assert isinstance(key, bytes) + # The key is base64 encoded, so it should be 44 bytes + assert len(key) == 44 # Base64 encoded Fernet key length is 44 bytes \ No newline at end of file From a28c3efb89316f6ccbddd058119319efec6ca229 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 19:30:01 +0530 Subject: [PATCH 05/12] Refactor test cases for API key encryption and decryption --- backend/app/tests/core/test_security.py | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/backend/app/tests/core/test_security.py b/backend/app/tests/core/test_security.py index d9d443269..3bd2bbd4e 100644 --- a/backend/app/tests/core/test_security.py +++ b/backend/app/tests/core/test_security.py @@ -12,18 +12,18 @@ def test_encrypt_decrypt_api_key(): """Test that API key encryption and decryption works correctly.""" # Test data test_key = "ApiKey test123456789" - + # Encrypt the key encrypted_key = encrypt_api_key(test_key) - + # Verify encryption worked assert encrypted_key is not None assert encrypted_key != test_key assert isinstance(encrypted_key, str) - + # Decrypt the key decrypted_key = decrypt_api_key(encrypted_key) - + # Verify decryption worked assert decrypted_key is not None assert decrypted_key == test_key @@ -36,13 +36,13 @@ def test_encrypt_api_key_edge_cases(): encrypted_empty = encrypt_api_key(empty_key) assert encrypted_empty is not None assert decrypt_api_key(encrypted_empty) == empty_key - + # Test whitespace only whitespace_key = " " encrypted_whitespace = encrypt_api_key(whitespace_key) assert encrypted_whitespace is not None assert decrypt_api_key(encrypted_whitespace) == whitespace_key - + # Test very long input long_key = "ApiKey " + "a" * 1000 encrypted_long = encrypt_api_key(long_key) @@ -80,7 +80,7 @@ def test_decrypt_api_key_error_handling(): # Test with invalid input with pytest.raises(ValueError, match="Failed to decrypt API key"): decrypt_api_key(None) - + # Test with invalid encrypted data with pytest.raises(ValueError, match="Failed to decrypt API key"): decrypt_api_key("invalid_encrypted_data") @@ -90,9 +90,9 @@ def test_get_encryption_key(): """Test that encryption key generation works correctly.""" # Get the encryption key key = get_encryption_key() - + # Verify the key assert key is not None assert isinstance(key, bytes) # The key is base64 encoded, so it should be 44 bytes - assert len(key) == 44 # Base64 encoded Fernet key length is 44 bytes \ No newline at end of file + assert len(key) == 44 # Base64 encoded Fernet key length is 44 bytes From d279edb43eceec102fce4dbc9d1d271c199c983d Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 19:38:57 +0530 Subject: [PATCH 06/12] Refactor API key generation and enhance validation tests --- .../77dc462dc6b0_seed_organization_table.py | 18 ++++++----- backend/app/tests/core/test_security.py | 31 ++++++++++++++++--- 2 files changed, 37 insertions(+), 12 deletions(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index 5fef932b2..fe91cd9d6 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -90,17 +90,19 @@ def create_user(session: Session, is_super: bool = True) -> User: def create_api_key(session: Session, user: User, organization: Organization) -> APIKey: """Create and return an API key for the user and organization.""" - # Generate raw key - raw_key = "ApiKey " + secrets.token_urlsafe(32) - # Hash the key - hashed_key = get_password_hash(raw_key) - # Encrypt the hashed key - encrypted_key = encrypt_api_key(hashed_key) - + # Generate token + token = secrets.token_urlsafe(32) + # Hash only the token part + hashed_token = get_password_hash(token) + # Create the full key with prefix + raw_key = "ApiKey " + token + # Encrypt the hashed token + encrypted_key = encrypt_api_key(hashed_token) + api_key = APIKey( user_id=user.id, organization_id=organization.id, - key=encrypted_key, # Store the encrypted hashed key + key=encrypted_key, # Store the encrypted hashed token ) session.add(api_key) session.commit() diff --git a/backend/app/tests/core/test_security.py b/backend/app/tests/core/test_security.py index 3bd2bbd4e..f018d122c 100644 --- a/backend/app/tests/core/test_security.py +++ b/backend/app/tests/core/test_security.py @@ -29,6 +29,21 @@ def test_encrypt_decrypt_api_key(): assert decrypted_key == test_key +def test_api_key_format_validation(): + """Test that API key format is validated correctly.""" + # Test valid API key format + valid_key = "ApiKey test123456789" + encrypted_valid = encrypt_api_key(valid_key) + assert encrypted_valid is not None + assert decrypt_api_key(encrypted_valid) == valid_key + + # Test invalid API key format (missing prefix) + invalid_key = "test123456789" + encrypted_invalid = encrypt_api_key(invalid_key) + assert encrypted_invalid is not None + assert decrypt_api_key(encrypted_invalid) == invalid_key + + def test_encrypt_api_key_edge_cases(): """Test edge cases for API key encryption.""" # Test empty string @@ -80,10 +95,18 @@ def test_decrypt_api_key_error_handling(): # Test with invalid input with pytest.raises(ValueError, match="Failed to decrypt API key"): decrypt_api_key(None) - - # Test with invalid encrypted data - with pytest.raises(ValueError, match="Failed to decrypt API key"): - decrypt_api_key("invalid_encrypted_data") + + # Test with various invalid encrypted data formats + invalid_encrypted_data = [ + "invalid_encrypted_data", # Not base64 + "not_a_base64_string", # Not base64 + "a" * 44, # Wrong length + "!" * 44, # Invalid base64 chars + "aGVsbG8=", # Valid base64 but not encrypted + ] + for invalid_data in invalid_encrypted_data: + with pytest.raises(ValueError, match="Failed to decrypt API key"): + decrypt_api_key(invalid_data) def test_get_encryption_key(): From a4aa7de69caf5f0af49b2958ffd83d744e093e64 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 19:41:18 +0530 Subject: [PATCH 07/12] Clean up whitespace in API key generation and validation tests --- .../versions/77dc462dc6b0_seed_organization_table.py | 2 +- backend/app/tests/core/test_security.py | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index fe91cd9d6..28e08f0e9 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -98,7 +98,7 @@ def create_api_key(session: Session, user: User, organization: Organization) -> raw_key = "ApiKey " + token # Encrypt the hashed token encrypted_key = encrypt_api_key(hashed_token) - + api_key = APIKey( user_id=user.id, organization_id=organization.id, diff --git a/backend/app/tests/core/test_security.py b/backend/app/tests/core/test_security.py index f018d122c..4f7f68610 100644 --- a/backend/app/tests/core/test_security.py +++ b/backend/app/tests/core/test_security.py @@ -36,7 +36,7 @@ def test_api_key_format_validation(): encrypted_valid = encrypt_api_key(valid_key) assert encrypted_valid is not None assert decrypt_api_key(encrypted_valid) == valid_key - + # Test invalid API key format (missing prefix) invalid_key = "test123456789" encrypted_invalid = encrypt_api_key(invalid_key) @@ -95,14 +95,14 @@ def test_decrypt_api_key_error_handling(): # Test with invalid input with pytest.raises(ValueError, match="Failed to decrypt API key"): decrypt_api_key(None) - + # Test with various invalid encrypted data formats invalid_encrypted_data = [ "invalid_encrypted_data", # Not base64 - "not_a_base64_string", # Not base64 - "a" * 44, # Wrong length - "!" * 44, # Invalid base64 chars - "aGVsbG8=", # Valid base64 but not encrypted + "not_a_base64_string", # Not base64 + "a" * 44, # Wrong length + "!" * 44, # Invalid base64 chars + "aGVsbG8=", # Valid base64 but not encrypted ] for invalid_data in invalid_encrypted_data: with pytest.raises(ValueError, match="Failed to decrypt API key"): From 41bcec39c231dc473358096a3502021f7a45ee49 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Thu, 17 Apr 2025 20:27:25 +0530 Subject: [PATCH 08/12] Refactor API key retrieval to use database session and simplify decryption logic --- backend/app/crud/api_key.py | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/backend/app/crud/api_key.py b/backend/app/crud/api_key.py index 9e487bc9c..bb3f66904 100644 --- a/backend/app/crud/api_key.py +++ b/backend/app/crud/api_key.py @@ -115,18 +115,15 @@ def get_api_key_by_value(session: Session, api_key_value: str) -> APIKey | None: def get_api_key_by_user_org( - session: Session, organization_id: int, user_id: str + db: Session, organization_id: int, user_id: int ) -> APIKey | None: - """ - Retrieve an API key for a specific user and organization. - """ + """Get an API key by user and organization ID.""" statement = select(APIKey).where( APIKey.organization_id == organization_id, APIKey.user_id == user_id, APIKey.is_deleted == False, ) - api_key = session.exec(statement).first() - if api_key: - # Decrypt the key before returning - api_key.key = decrypt_api_key(api_key.key) + api_key = db.exec(statement).first() + # Return the API key record as is, without decryption + # Decryption should only happen when verifying the key return api_key From f4643147f695d18f1bb078ccb0483481a700eb61 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Fri, 18 Apr 2025 16:16:39 +0530 Subject: [PATCH 09/12] Refactor API key creation by removing unnecessary comments and cleaning up whitespace for improved readability. --- .../versions/77dc462dc6b0_seed_organization_table.py | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index 28e08f0e9..3d0cb3a3f 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -90,19 +90,16 @@ def create_user(session: Session, is_super: bool = True) -> User: def create_api_key(session: Session, user: User, organization: Organization) -> APIKey: """Create and return an API key for the user and organization.""" - # Generate token + token = secrets.token_urlsafe(32) - # Hash only the token part hashed_token = get_password_hash(token) - # Create the full key with prefix raw_key = "ApiKey " + token - # Encrypt the hashed token encrypted_key = encrypt_api_key(hashed_token) api_key = APIKey( user_id=user.id, organization_id=organization.id, - key=encrypted_key, # Store the encrypted hashed token + key=encrypted_key, ) session.add(api_key) session.commit() From 2fd1b8cd56e77c2a47d657e8a608dd208b4a8b23 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Fri, 18 Apr 2025 16:17:14 +0530 Subject: [PATCH 10/12] Fix whitespace inconsistency in API key creation function for improved code clarity. --- .../alembic/versions/77dc462dc6b0_seed_organization_table.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index 3d0cb3a3f..f036ec62f 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -99,7 +99,7 @@ def create_api_key(session: Session, user: User, organization: Organization) -> api_key = APIKey( user_id=user.id, organization_id=organization.id, - key=encrypted_key, + key=encrypted_key, ) session.add(api_key) session.commit() From d5fbc9e983f57be3baf2491fc6607badef798219 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Sat, 19 Apr 2025 12:53:15 +0530 Subject: [PATCH 11/12] Refactor API key handling to encrypt and return raw keys instead of hashed keys, ensuring keys are stored and retrieved in their original format. Update tests to verify the integrity of the raw keys. --- .../77dc462dc6b0_seed_organization_table.py | 3 +- backend/app/crud/api_key.py | 58 ++++++++++++++----- backend/app/tests/crud/test_api_key.py | 32 ++++++---- 3 files changed, 64 insertions(+), 29 deletions(-) diff --git a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py index f036ec62f..b8f26fdb2 100644 --- a/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py +++ b/backend/app/alembic/versions/77dc462dc6b0_seed_organization_table.py @@ -92,9 +92,8 @@ def create_api_key(session: Session, user: User, organization: Organization) -> """Create and return an API key for the user and organization.""" token = secrets.token_urlsafe(32) - hashed_token = get_password_hash(token) raw_key = "ApiKey " + token - encrypted_key = encrypt_api_key(hashed_token) + encrypted_key = encrypt_api_key(raw_key) # Encrypt the raw key directly api_key = APIKey( user_id=user.id, diff --git a/backend/app/crud/api_key.py b/backend/app/crud/api_key.py index bb3f66904..0de9c5e58 100644 --- a/backend/app/crud/api_key.py +++ b/backend/app/crud/api_key.py @@ -32,11 +32,13 @@ def create_api_key( """ # Generate raw key and its hash using the helper function raw_key, hashed_key = generate_api_key() - encrypted_key = encrypt_api_key(hashed_key) + encrypted_key = encrypt_api_key( + raw_key + ) # Encrypt the raw key instead of hashed key - # Create API key record with encrypted hashed key + # Create API key record with encrypted raw key api_key = APIKey( - key=encrypted_key, # Store the encrypted hashed key + key=encrypted_key, # Store the encrypted raw key organization_id=organization_id, user_id=user_id, ) @@ -55,14 +57,20 @@ def create_api_key( def get_api_key(session: Session, api_key_id: int) -> APIKeyPublic | None: """ Retrieves an API key by its ID if it exists and is not deleted. + Returns the API key in its original format. """ api_key = session.exec( select(APIKey).where(APIKey.id == api_key_id, APIKey.is_deleted == False) ).first() if api_key: - # Return the API key without decrypting (we don't want to expose the hashed key) - return APIKeyPublic.model_validate(api_key) + # Create a copy of the API key data + api_key_dict = api_key.model_dump() + # Decrypt the key + decrypted_key = decrypt_api_key(api_key.key) + api_key_dict["key"] = decrypted_key + + return APIKeyPublic.model_validate(api_key_dict) return None @@ -71,6 +79,7 @@ def get_api_keys_by_organization( ) -> list[APIKeyPublic]: """ Retrieves all active API keys associated with an organization. + Returns the API keys in their original format. """ api_keys = session.exec( select(APIKey).where( @@ -78,8 +87,17 @@ def get_api_keys_by_organization( ) ).all() - # Return the API keys without decrypting (we don't want to expose the hashed keys) - return [APIKeyPublic.model_validate(api_key) for api_key in api_keys] + raw_keys = [] + for api_key in api_keys: + api_key_dict = api_key.model_dump() + + decrypted_key = decrypt_api_key(api_key.key) + + api_key_dict["key"] = decrypted_key + + raw_keys.append(APIKeyPublic.model_validate(api_key_dict)) + + return raw_keys def delete_api_key(session: Session, api_key_id: int) -> None: @@ -98,19 +116,22 @@ def delete_api_key(session: Session, api_key_id: int) -> None: session.commit() -def get_api_key_by_value(session: Session, api_key_value: str) -> APIKey | None: +def get_api_key_by_value(session: Session, api_key_value: str) -> APIKeyPublic | None: """ Retrieve an API Key record by verifying the provided key against stored hashes. + Returns the API key in its original format. """ # Get all active API keys api_keys = session.exec(select(APIKey).where(APIKey.is_deleted == False)).all() - # Check each key for api_key in api_keys: - # Decrypt the stored key before verification decrypted_key = decrypt_api_key(api_key.key) - if verify_password(api_key_value, decrypted_key): - return api_key + if api_key_value == decrypted_key: + api_key_dict = api_key.model_dump() + + api_key_dict["key"] = decrypted_key + + return APIKeyPublic.model_validate(api_key_dict) return None @@ -124,6 +145,13 @@ def get_api_key_by_user_org( APIKey.is_deleted == False, ) api_key = db.exec(statement).first() - # Return the API key record as is, without decryption - # Decryption should only happen when verifying the key - return api_key + + if api_key: + api_key_dict = api_key.model_dump() + + decrypted_key = decrypt_api_key(api_key.key) + + api_key_dict["key"] = decrypted_key + + return APIKey.model_validate(api_key_dict) + return None diff --git a/backend/app/tests/crud/test_api_key.py b/backend/app/tests/crud/test_api_key.py index 92e19b09c..3c23a63ec 100644 --- a/backend/app/tests/crud/test_api_key.py +++ b/backend/app/tests/crud/test_api_key.py @@ -48,8 +48,9 @@ def test_get_api_key(db: Session) -> None: assert retrieved_key is not None assert retrieved_key.id == created_key.id - # The key should be decrypted when retrieved - assert verify_password(created_key.key, decrypt_api_key(retrieved_key.key)) + # The key should be in its original format + assert retrieved_key.key.startswith("ApiKey ") + assert len(retrieved_key.key) > 32 def test_get_api_key_not_found(db: Session) -> None: @@ -68,12 +69,12 @@ def test_get_api_keys_by_organization(db: Session) -> None: api_keys = api_key_crud.get_api_keys_by_organization(db, org.id) assert len(api_keys) == 2 - # Verify both keys can be decrypted and verified + # Verify that the keys are in their original format for key in api_keys: - assert verify_password( - api_key1.key if key.id == api_key1.id else api_key2.key, - decrypt_api_key(key.key), - ) + assert key.key.startswith("ApiKey ") + assert len(key.key) > 32 # Raw key should be longer than 32 characters + assert key.organization_id == org.id + assert key.user_id in [user1.id, user2.id] def test_delete_api_key(db: Session) -> None: @@ -105,13 +106,19 @@ def test_get_api_key_by_value(db: Session) -> None: user = create_test_user(db) org = create_test_organization(db) + # Create an API key api_key = api_key_crud.create_api_key(db, org.id, user.id) - retrieved_key = api_key_crud.get_api_key_by_value(db, api_key.key) + # Get the raw key that was returned during creation + raw_key = api_key.key + + # Test retrieving the API key by its value + retrieved_key = api_key_crud.get_api_key_by_value(db, raw_key) assert retrieved_key is not None assert retrieved_key.id == api_key.id - # The key should be verified against the stored hash - assert verify_password(api_key.key, decrypt_api_key(retrieved_key.key)) + # The key should be in its original format + assert retrieved_key.key.startswith("ApiKey ") + assert len(retrieved_key.key) > 32 def test_get_api_key_by_user_org(db: Session) -> None: @@ -125,8 +132,9 @@ def test_get_api_key_by_user_org(db: Session) -> None: assert retrieved_key.id == api_key.id assert retrieved_key.organization_id == org.id assert retrieved_key.user_id == user.id - # The key should already be decrypted by get_api_key_by_user_org - assert retrieved_key.key is not None + # The key should be in its original format + assert retrieved_key.key.startswith("ApiKey ") + assert len(retrieved_key.key) > 32 def test_get_api_key_by_user_org_not_found(db: Session) -> None: From 30174f3d0991b761bdf9ae7bf1c2c5ad927671e5 Mon Sep 17 00:00:00 2001 From: Priyanshu singh <111607560+PriyanSingh@users.noreply.github.com> Date: Sun, 20 Apr 2025 20:11:00 +0530 Subject: [PATCH 12/12] Enhance test for API key retrieval by adding assertions for organization ID, user ID, and raw key format to ensure comprehensive validation of retrieved keys. --- backend/app/tests/crud/test_api_key.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backend/app/tests/crud/test_api_key.py b/backend/app/tests/crud/test_api_key.py index 3c23a63ec..b6269b700 100644 --- a/backend/app/tests/crud/test_api_key.py +++ b/backend/app/tests/crud/test_api_key.py @@ -116,7 +116,10 @@ def test_get_api_key_by_value(db: Session) -> None: assert retrieved_key is not None assert retrieved_key.id == api_key.id + assert retrieved_key.organization_id == org.id + assert retrieved_key.user_id == user.id # The key should be in its original format + assert retrieved_key.key == raw_key # Should be exactly the same key assert retrieved_key.key.startswith("ApiKey ") assert len(retrieved_key.key) > 32