Conversation
Summary by CodeRabbit
Summary by CodeRabbit
WalkthroughThis change introduces a comprehensive API key management system across both backend and frontend. It adds Django models, admin, authentication classes, API endpoints, GraphQL queries/mutations, and permissions for API keys. The frontend provides React components, hooks, and types for API key management, including creation, revocation, and listing, along with associated tests and utility functions. Changes
Assessment against linked issues
Assessment against linked issues: Out-of-scope changesNo out-of-scope changes found. Suggested reviewers
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (4)
backend/apps/core/api/ninja.py (2)
21-23: Fix documentation inconsistency.The docstring states that the method returns "APIKey object if valid, otherwise None" but the implementation raises
HttpErrorexceptions instead of returningNonefor invalid cases.Update the docstring to accurately reflect the behavior:
- Returns: - APIKey: The APIKey object if the key is valid, otherwise None. + Returns: + APIKey: The APIKey object if the key is valid. + + Raises: + HttpError: If the key is missing, invalid, revoked, or expired.
29-36: Simplify error handling logic.The current flow can be simplified since line 36 is only reachable when the API key exists but
is_valid()returns False, making the error message more specific.Consider this cleaner approach:
try: api_key = APIKey.objects.get(key_hash=key_hash) - if api_key.is_valid(): - return api_key + if not api_key.is_valid(): + raise HttpError(401, "API key revoked or expired") + return api_key except APIKey.DoesNotExist as err: raise HttpError(401, "Invalid or expired API key") from err - - raise HttpError(401, "API key revoked or expired")backend/apps/nest/graphql/utils.py (1)
9-32: Consider using more specific exception types for better error handling.The function correctly implements the authentication flow but uses generic
Exceptiontypes. Consider using more specific exceptions likeAuthenticationErrororPermissionDeniedfrom Django's core exceptions for better error categorization and handling.+from django.core.exceptions import AuthenticationError, PermissionDenied + def get_authenticated_user(request) -> NestUser: """Get authenticated user from request.""" auth_header = request.headers.get("Authorization") if not auth_header or not auth_header.startswith("Bearer "): - raise Exception("Missing or malformed Authorization header") + raise AuthenticationError("Missing or malformed Authorization header") access_token = auth_header.removeprefix("Bearer ").strip() try: github = Github(access_token) gh_user = github.get_user() except Exception as err: - raise Exception("GitHub token is invalid or expired") from err + raise AuthenticationError("GitHub token is invalid or expired") from err try: github_user = GithubUser.objects.get(login=gh_user.login) except GithubUser.DoesNotExist as err: - raise Exception("GitHub user not found in local database") from err + raise PermissionDenied("GitHub user not found in local database") from err try: user = NestUser.objects.get(github_user=github_user) except NestUser.DoesNotExist as err: - raise Exception("Local user not linked to this GitHub account") from err + raise PermissionDenied("Local user not linked to this GitHub account") from errbackend/apps/nest/models/apikey.py (1)
49-58: Consider adding timing attack protection.The
authenticatemethod is functionally correct but could be vulnerable to timing attacks. The database query time might vary based on whether the key exists, potentially leaking information about valid key hashes.Consider adding constant-time comparison or implementing rate limiting at the application level to mitigate timing attacks:
@classmethod def authenticate(cls, raw_key: str): """Authenticate an API key using the raw key.""" key_hash = cls.generate_hash_key(raw_key) try: api_key = cls.objects.get(key_hash=key_hash) if api_key.is_valid(): return api_key except cls.DoesNotExist: pass return None
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
backend/apps/core/api/ninja.py(1 hunks)backend/apps/nest/admin.py(2 hunks)backend/apps/nest/graphql/mutations/__init__.py(1 hunks)backend/apps/nest/graphql/mutations/apikey.py(1 hunks)backend/apps/nest/graphql/nodes/apikey.py(1 hunks)backend/apps/nest/graphql/queries/__init__.py(1 hunks)backend/apps/nest/graphql/queries/apikey.py(1 hunks)backend/apps/nest/graphql/utils.py(1 hunks)backend/apps/nest/migrations/0002_apikey.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/apikey.py(1 hunks)backend/pyproject.toml(2 hunks)backend/settings/api_v1.py(1 hunks)backend/settings/graphql.py(1 hunks)frontend/src/app/api/auth/[...nextauth]/route.ts(1 hunks)frontend/src/server/apolloClient.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
backend/pyproject.toml (1)
Learnt from: yashgoyal0110
PR: OWASP/Nest#0
File: :0-0
Timestamp: 2025-04-06T06:57:42.144Z
Learning: The OWASP/Nest project uses mypy for static type checking, configured in .pre-commit-config.yaml with settings to exclude tests and use backend/pyproject.toml as the config file.
backend/settings/graphql.py (1)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.169Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
🧬 Code Graph Analysis (12)
backend/apps/nest/models/__init__.py (1)
backend/apps/nest/models/apikey.py (1)
APIKey(11-66)
backend/apps/nest/graphql/queries/__init__.py (1)
backend/apps/nest/graphql/queries/apikey.py (1)
APIKeyQueries(11-31)
frontend/src/server/apolloClient.ts (1)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
session(63-68)
backend/settings/api_v1.py (1)
backend/apps/core/api/ninja.py (1)
APIKeyAuth(9-36)
backend/apps/nest/graphql/nodes/apikey.py (1)
backend/apps/nest/models/apikey.py (1)
APIKey(11-66)
backend/apps/nest/graphql/queries/apikey.py (3)
backend/apps/nest/graphql/nodes/apikey.py (1)
APIKeyNode(11-12)backend/apps/nest/graphql/utils.py (1)
get_authenticated_user(9-32)backend/apps/nest/models/apikey.py (1)
APIKey(11-66)
backend/apps/nest/graphql/utils.py (2)
backend/apps/github/api/v1/user.py (1)
get_user(46-51)backend/apps/github/graphql/queries/user.py (1)
user(40-56)
backend/settings/graphql.py (4)
backend/apps/nest/graphql/mutations/__init__.py (1)
NestMutations(10-11)backend/apps/nest/graphql/queries/__init__.py (1)
NestQuery(7-8)backend/apps/owasp/graphql/queries/__init__.py (1)
OwaspQuery(13-23)backend/apps/github/graphql/queries/__init__.py (1)
GithubQuery(13-23)
backend/apps/core/api/ninja.py (1)
backend/apps/nest/models/apikey.py (4)
APIKey(11-66)authenticate(50-58)generate_hash_key(35-37)is_valid(60-62)
backend/apps/nest/admin.py (1)
backend/apps/nest/models/apikey.py (1)
APIKey(11-66)
backend/apps/nest/graphql/mutations/apikey.py (3)
backend/apps/nest/graphql/nodes/apikey.py (1)
APIKeyNode(11-12)backend/apps/nest/graphql/utils.py (1)
get_authenticated_user(9-32)backend/apps/nest/models/apikey.py (2)
APIKey(11-66)create(40-47)
backend/apps/nest/graphql/mutations/__init__.py (2)
backend/apps/nest/graphql/mutations/apikey.py (1)
APIKeyMutations(19-42)backend/apps/nest/graphql/mutations/user.py (1)
UserMutations(24-56)
🪛 Flake8 (7.2.0)
backend/apps/nest/models/__init__.py
[error] 1-1: '.apikey.APIKey' imported but unused
(F401)
🪛 GitHub Actions: Run CI/CD
frontend/src/server/apolloClient.ts
[error] 23-23: ESLint: Object Literal Property name Authorization must match one of the following formats: camelCase, UPPER_CASE (@typescript-eslint/naming-convention)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (21)
frontend/src/server/apolloClient.ts (3)
4-4: LGTM: Correct import for session management.The import of
getSessionfromnext-auth/reactis appropriate for accessing the session data in the Apollo client context.
7-9: LGTM: Well-defined interface for extended session.The
ExtendedSessioninterface correctly extends the session object to include the optionalaccessTokenproperty, which aligns with the NextAuth configuration in the auth route.
15-16: LGTM: Proper session retrieval and token extraction.The implementation correctly retrieves the session and extracts the access token using type casting to the
ExtendedSessioninterface.frontend/src/app/api/auth/[...nextauth]/route.ts (1)
50-50: LGTM: Improved error handling with original error context.The enhanced error handling now includes the original error message, which will significantly improve debugging capabilities when GitHub authentication fails.
backend/pyproject.toml (1)
130-130: LGTM: Appropriate linting rule adjustments for new API key functionality.The added error codes (EM101, TRY002, TRY003) are commonly ignored in Django projects for pragmatic exception handling. These changes support the new API key management code without compromising code quality.
Also applies to: 144-145
backend/apps/nest/models/__init__.py (1)
1-1: LGTM: Package-level model import for API key functionality.The import of
APIKeyfrom the.apikeymodule correctly exposes the model at the package level, following Django's standard pattern for model organization. The static analysis "unused import" warning is a false positive - this import is used by other modules that import from this package.backend/apps/nest/graphql/queries/__init__.py (1)
1-8: LGTM: Well-structured GraphQL query class for API key management.The
NestQueryclass correctly inherits fromAPIKeyQueriesand uses the appropriate Strawberry decorators. This structure properly integrates API key queries into the broader GraphQL schema while maintaining clean separation of concerns.backend/settings/api_v1.py (1)
6-6: LGTM! Clean integration of API key authentication.The import and integration of
APIKeyAuthinto the Ninja API configuration is correctly implemented and follows Django Ninja best practices.Also applies to: 14-14
backend/apps/nest/graphql/nodes/apikey.py (1)
8-12: LGTM! Secure field exposure for GraphQL API.The GraphQL node correctly exposes only safe, non-sensitive fields from the APIKey model. Excluding
key_hashanduserfrom the exposed fields is a good security practice.backend/apps/nest/graphql/mutations/__init__.py (1)
9-11: LGTM! Clean consolidation of GraphQL mutations.The multiple inheritance approach correctly combines API key and user mutations into a single GraphQL type, following established patterns in the codebase.
backend/apps/core/api/ninja.py (1)
25-27: LGTM! Proper input validation and security.The key validation and hashing approach using SHA-256 is secure and follows best practices for API key authentication.
backend/apps/nest/admin.py (1)
14-18: LGTM! Well-designed admin interface configuration.The admin configuration appropriately exposes relevant fields while maintaining security by not displaying the sensitive
key_hashfield. The search, filter, and ordering options provide good usability for administrators.backend/apps/nest/migrations/0002_apikey.py (1)
1-44: LGTM! Well-structured Django migration.The migration properly creates the APIKey model with appropriate field types, constraints, and indexes. The foreign key relationship with CASCADE deletion and the database indexes are correctly configured for performance and data integrity.
backend/settings/graphql.py (1)
6-17: LGTM! Proper GraphQL schema integration.The schema updates correctly integrate the new Nest GraphQL components following the established patterns. The imports, inheritance structure, and decorators are properly configured.
backend/apps/nest/graphql/mutations/apikey.py (1)
30-42: LGTM! Proper error handling for API key revocation.The revoke_api_key mutation correctly handles the case where the API key doesn't exist or doesn't belong to the user, returning False appropriately. The use of
update_fieldsin the save operation is also a good practice for performance.backend/apps/nest/models/apikey.py (6)
1-9: LGTM! Clean and appropriate imports.The imports are well-organized and include all necessary modules for secure API key management:
hashlibfor hashing,secretsfor secure random generation, and proper Django utilities.
18-20: LGTM! Proper foreign key configuration.The user foreign key is correctly configured with CASCADE deletion, appropriate related_name, and proper indexing for performance.
29-37: LGTM! Secure key generation and hashing implementation.The implementation uses cryptographically secure methods:
secrets.token_urlsafe(32)provides 256 bits of entropy- SHA-256 hashing is appropriate for this use case
- The methods are properly structured as static methods
39-47: LGTM! Well-designed key creation method.The
createmethod properly generates a secure key, stores the hash, and returns both the instance and raw key. The pattern of returning the raw key only during creation is a good security practice.
60-62: LGTM! Proper validation logic.The
is_validmethod correctly checks both revocation status and expiration. The timezone-aware comparison is appropriate for Django applications.
64-66: LGTM! Clear string representation.The
__str__method provides a clear, human-readable representation showing the name and status of the API key.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
frontend/src/utils/helpers/apolloClient.ts (1)
24-27: Consider type safety for session casting.The type casting
(session as ExtendedSession)may be unsafe if the session object doesn't match the expected structure.- const session = await getSession() - const accessToken = (session as ExtendedSession)?.accessToken + const session = await getSession() + const accessToken = session?.accessToken as string | undefinedfrontend/src/components/ApiKeyPageContent.tsx (3)
260-260: Simplify unnecessary type conversion.The
parseInt(key.id.toString())conversion is unnecessary ifkey.idis already a number type.- onPress={() => handleRevokeKey(parseInt(key.id.toString()))} + onPress={() => handleRevokeKey(key.id)}
27-403: Consider breaking down this large component.This component handles many responsibilities and could benefit from being split into smaller, more focused components for better maintainability and reusability.
Consider extracting the following into separate components:
ApiKeyTable- for the table display logicCreateApiKeyModal- for the modal functionalityApiKeyUsageCard- for the usage instructionsApiKeyStatusChip- for the status displayThis would make the component more modular, easier to test, and more maintainable.
96-118: Extract hardcoded validation messages to constants.The validation messages and toast configurations are hardcoded throughout the component. Consider extracting them to constants for better maintainability.
+const VALIDATION_MESSAGES = { + EMPTY_KEY_NAME: 'Please provide a name for your API key.', + KEY_CREATED_SUCCESS: 'API key created successfully. Make sure to copy it now as you won\'t be able to see it again.', + KEY_REVOKED_SUCCESS: 'API key revoked successfully.', + KEY_COPIED: 'API key copied to clipboard.' +} const handleCreateKey = () => { if (!newKeyName.trim()) { addToast({ - description: 'Please provide a name for your API key.', + description: VALIDATION_MESSAGES.EMPTY_KEY_NAME, title: 'Validation Error', timeout: 3000, shouldShowTimeoutProgress: true, color: 'danger', variant: 'solid', }) return }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
frontend/src/app/auth/apikeys/page.tsx(1 hunks)frontend/src/components/ApiKeyPageContent.tsx(1 hunks)frontend/src/server/queries/apiKeyQueries.ts(1 hunks)frontend/src/types/apikey.ts(1 hunks)frontend/src/utils/helpers/apolloClient.ts(3 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/types/apikey.ts
- frontend/src/server/queries/apiKeyQueries.ts
🧰 Additional context used
🧬 Code Graph Analysis (2)
frontend/src/app/auth/apikeys/page.tsx (2)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
session(63-68)frontend/src/utils/credentials.ts (1)
IS_GITHUB_AUTH_ENABLED(9-11)
frontend/src/utils/helpers/apolloClient.ts (2)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
session(63-68)frontend/src/utils/utility.ts (1)
getCsrfToken(51-65)
🪛 GitHub Actions: Run CI/CD
frontend/src/utils/helpers/apolloClient.ts
[error] 32-32: ESLint: Object Literal Property name Authorization must match one of the following formats: camelCase, UPPER_CASE (@typescript-eslint/naming-convention)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/src/app/auth/apikeys/page.tsx (1)
6-14: Well-structured authentication flow.The server component correctly handles authentication using
getServerSession()and implements proper redirect logic for unauthenticated users. The component structure is clean and follows Next.js best practices.
abhayymishraa
force-pushed
the
feat/api_management
branch
from
08e819e to
313a5b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
frontend/src/components/ApiKeyPageContent.tsx (2)
24-24: Apollo client import issue still needs to be addressed.This is a duplicate of the previous review comment. The Apollo client is exported as a promise but used as a synchronous client instance, which will cause runtime errors.
45-45: Apollo client usage will cause runtime errors.The Apollo client is being used directly in the
useQueryhook, but according to the import issue, it should be awaited since it's exported as a promise. This will cause the query to fail.
🧹 Nitpick comments (2)
frontend/src/components/ApiKeyPageContent.tsx (2)
269-269: Unnecessary type conversion in key ID handling.The code converts
key.idto string then back to number, which is redundant and potentially error-prone.- onPress={() => handleRevokeKey(parseInt(key.id.toString()))} + onPress={() => handleRevokeKey(key.id)}
248-248: Consider security implications of key suffix display.Displaying the key suffix might provide information that could be used in attack scenarios. Consider if this is necessary for the user experience.
The current implementation shows
*******{key.keySuffix}which reveals the last few characters of the API key. Consider whether this is necessary for user identification or if it could be replaced with a more secure identifier like creation timestamp or a non-sensitive ID.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/components/ApiKeyPageContent.tsx(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: Run Code Scan
- GitHub Check: Run CI Denendencies Scan
- GitHub Check: CodeQL (python)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (2)
frontend/src/components/ApiKeyPageContent.tsx (2)
427-435: Good implementation of confirmation modal.The confirmation modal properly replaces the native
confirm()function with a modern React-based dialog, addressing the previous review concern about blocking UI.
52-77: Excellent error handling and user feedback.The mutation setup includes comprehensive error handling with user-friendly toast notifications for both success and error scenarios.
arkid15r
left a comment
arkid15r
left a comment
There was a problem hiding this comment.
Please address these. It's also a good idea to request a security perspective review from bot
backend/apps/nest/models/apikey.py
Outdated
| key_hash = models.CharField(max_length=64, unique=True, db_index=True) | ||
| key_suffix = models.CharField(max_length=4, blank=True) | ||
| name = models.CharField(max_length=100, null=False, blank=False) | ||
| revoked = models.BooleanField(default=False) |
There was a problem hiding this comment.
| revoked = models.BooleanField(default=False) | |
| is_revoked = models.BooleanField(default=False) |
There was a problem hiding this comment.
Or better replace w/ revoked_at DateTime field
There was a problem hiding this comment.
i followed this
https://github.com/florimondmanca/djangorestframework-api-key/blob/master/src/rest_framework_api_key/models.py
they follow a similar approach, so whatever you suggest i can further work on that!!
backend/apps/nest/models/apikey.py
Outdated
| class APIKey(models.Model): | ||
| """API key model.""" | ||
|
|
||
| key_hash = models.CharField(max_length=64, unique=True, db_index=True) |
There was a problem hiding this comment.
You already have an index for unique=True
backend/apps/nest/models/apikey.py
Outdated
| class APIKey(models.Model): | ||
| """API key model.""" | ||
|
|
||
| key_hash = models.CharField(max_length=64, unique=True, db_index=True) |
There was a problem hiding this comment.
It has key in the model name -- remove key_ from names
backend/pyproject.toml
Outdated
| "COM812", | ||
| "D407", | ||
| "DJ012", | ||
| "EM101", |
There was a problem hiding this comment.
Please stop suppressing warnings on the entire backend level. The tool is telling you're doing it wrong and you reply "shut up" instead of improving your code.
There was a problem hiding this comment.
It's just for the checks for now i was going to fix it after the initial review
| instance, raw_key = APIKey.create(user=user, name=name, expires_at=expires_at) | ||
| return CreateAPIKeyResult(api_key=instance, raw_key=raw_key) | ||
| except Exception as err: | ||
| raise Exception("API key name already exists") from err |
There was a problem hiding this comment.
You suppressed ruff instead of introducing a custom exception here.
| try: | ||
| instance, raw_key = APIKey.create(user=user, name=name, expires_at=expires_at) | ||
| return CreateAPIKeyResult(api_key=instance, raw_key=raw_key) | ||
| except Exception as err: |
There was a problem hiding this comment.
This exception is too wide, you probably need just IntegrityError or similar.
|
|
||
| const { loading, error, data, refetch } = useQuery(GET_API_KEYS, { | ||
| variables: { includeRevoked }, | ||
| skip: status !== userAuthStatus.AUTHENTICATED, |
There was a problem hiding this comment.
Why should we take care of it on this level? If user is not authenticated there should be no content rendered at all.
| </div> | ||
| ) : error ? ( | ||
| <div className="rounded-md bg-red-50 p-4 text-red-700 dark:bg-red-900/20 dark:text-red-400"> | ||
| Error loading API keys: {error.message} |
There was a problem hiding this comment.
We don't want to expose naked error messages to users.
|
@coderabbitai review this PR from API keys management security perspective. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain@arkid15r, I'll conduct a comprehensive security review of the API key management implementation. Based on my analysis, here are the security strengths and critical concerns: ✅ Security Strengths
🚨 Critical Security Concerns1. Missing Rate LimitingThe API key authentication lacks rate limiting, enabling brute force attacks: # In ninja.py - no rate limiting on key validation
def authenticate(self, request, key: str) -> APIKey:
# Vulnerable to brute force attacks2. No Audit LoggingNo logging of API key usage, creation, or authentication attempts:
3. Information Disclosure in Error MessagesError messages may leak information about key existence: # In apikey.py
raise ValueError("API key not found or invalid")
# Should be generic: "Invalid API key"4. Missing Permission Scoping
5. No IP RestrictionsKeys can be used from any IP address without restrictions 6. Missing HTTPS EnforcementNo explicit HTTPS requirement for API key transmission 🔧 Security RecommendationsHigh Priority
Medium Priority
Code Examples for Fixes# Rate limiting example
from django_ratelimit.decorators import ratelimit
@ratelimit(key='header:x-api-key', rate='100/h', method='ALL')
def authenticate(self, request, key: str) -> APIKey:
# Implementation with rate limiting# Audit logging example
import logging
api_logger = logging.getLogger('api_keys')
def authenticate(self, request, key: str) -> APIKey:
try:
api_key = APIKey.authenticate(key)
api_logger.info(f"API key {api_key.key_suffix} used", extra={
'key_id': api_key.id, 'user_id': api_key.user.id, 'ip': request.META.get('REMOTE_ADDR')
})
return api_key
except ValueError:
api_logger.warning(f"Invalid API key attempted", extra={
'ip': request.META.get('REMOTE_ADDR'), 'key_suffix': key[-4:] if len(key) >= 4 else 'short'
})
raise AuthenticationError("Invalid API key")Overall Security Rating: Medium RiskWhile the basic cryptographic implementation is sound, the lack of rate limiting, audit logging, and permission scoping presents significant security risks for a production environment. 🏁 Script executed: Length of output: 5511 After examining the actual implementation, I need to update my security assessment. Here's the corrected security review: ✅ Security Strengths (Updated)
🚨 Remaining Security Concerns1. Information Disclosure in Error MessagesThe Ninja authentication layer provides specific error messages: # In ninja.py - these messages leak information
raise HttpError(401, "Invalid or expired API key") # vs DoesNotExist
raise HttpError(401, "API key revoked or expired") # vs invalid keyFix: Use generic "Invalid API key" for all failure cases. 2. No Audit LoggingCritical operations lack logging:
3. Missing Permission Scoping
4. GraphQL vs REST Authentication Inconsistency
5. No IP Restrictions or GeofencingKeys can be used from any location without restrictions. 🔧 Priority RecommendationsHigh Priority
def authenticate(self, request, key: str) -> APIKey:
if not key:
raise HttpError(401, "Invalid API key")
api_key = APIKey.authenticate(key)
if not api_key:
raise HttpError(401, "Invalid API key") # Generic message
return api_key
import logging
api_audit = logging.getLogger('api_keys.audit')
def authenticate(self, request, key: str) -> APIKey:
client_ip = request.META.get('REMOTE_ADDR', 'unknown')
if api_key := APIKey.authenticate(key):
api_audit.info("API key authenticated", extra={
'key_suffix': api_key.key_suffix,
'user_id': api_key.user_id,
'ip': client_ip
})
return api_key
else:
api_audit.warning("Invalid API key attempt", extra={
'key_suffix': key[-4:] if len(key) >= 4 else 'short',
'ip': client_ip
})
raise HttpError(401, "Invalid API key")Medium Priority
Updated Security Rating: Low-Medium RiskWith rate limiting in place, the implementation has significantly better security posture. The main remaining concerns are audit logging and permission granularity, which are important for production environments but not critical vulnerabilities. Recommendation: This implementation is suitable for production deployment with the suggested audit logging improvements. 🧠 Learnings used |
There was a problem hiding this comment.
Actionable comments posted: 18
♻️ Duplicate comments (1)
frontend/src/utils/helpers/apolloClient.ts (1)
35-37: Breaking change: Export is now a Promise instead of ApolloClient instance.This change breaks consuming code that expects a synchronous ApolloClient instance. Components using
<ApolloProvider client={apolloClient}>will fail at runtime.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (25)
backend/apps/core/api/ninja.py(1 hunks)backend/apps/nest/admin.py(2 hunks)backend/apps/nest/graphql/mutations/__init__.py(1 hunks)backend/apps/nest/graphql/mutations/api_key.py(1 hunks)backend/apps/nest/graphql/mutations/user.py(3 hunks)backend/apps/nest/graphql/nodes/api_key.py(1 hunks)backend/apps/nest/graphql/permissions.py(1 hunks)backend/apps/nest/graphql/queries/__init__.py(1 hunks)backend/apps/nest/graphql/queries/api_key.py(1 hunks)backend/apps/nest/migrations/0002_apikey.py(1 hunks)backend/apps/nest/models/__init__.py(1 hunks)backend/apps/nest/models/api_key.py(1 hunks)backend/settings/api_v1.py(1 hunks)frontend/src/app/api/auth/[...nextauth]/route.ts(2 hunks)frontend/src/app/settings/api-keys/layout.tsx(1 hunks)frontend/src/app/settings/api-keys/page.tsx(1 hunks)frontend/src/components/UserMenu.tsx(5 hunks)frontend/src/components/skeletons/ApiKeySkelton.tsx(1 hunks)frontend/src/hooks/useDjangoSession.ts(1 hunks)frontend/src/hooks/useLogout.ts(1 hunks)frontend/src/server/queries/apiKeyQueries.ts(1 hunks)frontend/src/server/queries/authQueries.ts(1 hunks)frontend/src/types/apikey.ts(1 hunks)frontend/src/utils/helpers/apolloClient.ts(2 hunks)frontend/src/wrappers/provider.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
frontend/src/components/UserMenu.tsx (2)
Learnt from: codic-yeeshu
PR: OWASP/Nest#1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
Learnt from: codic-yeeshu
PR: OWASP/Nest#1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: When implementing dropdown menus or similar interactive components, always include proper accessibility features: ARIA attributes (aria-expanded, aria-haspopup, aria-controls), keyboard navigation support (Enter, Space, Escape keys), and mechanisms to close dropdowns when clicking outside.
frontend/src/hooks/useDjangoSession.ts (1)
Learnt from: codic-yeeshu
PR: OWASP/Nest#1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
🧬 Code Graph Analysis (17)
backend/apps/nest/models/__init__.py (1)
backend/apps/nest/models/api_key.py (1)
APIKey(11-70)
backend/apps/nest/graphql/queries/__init__.py (1)
backend/apps/nest/graphql/queries/api_key.py (1)
APIKeyQueries(11-28)
backend/settings/api_v1.py (1)
backend/apps/core/api/ninja.py (1)
ApiKeyAuth(9-36)
frontend/src/components/UserMenu.tsx (1)
frontend/src/hooks/useLogout.ts (1)
useLogout(7-26)
frontend/src/wrappers/provider.tsx (2)
frontend/src/hooks/useDjangoSession.ts (1)
useDjangoSession(16-42)frontend/src/server/apolloClient.ts (1)
apolloClient(37-38)
backend/apps/nest/admin.py (1)
backend/apps/nest/models/api_key.py (1)
APIKey(11-70)
backend/apps/nest/graphql/mutations/__init__.py (2)
backend/apps/nest/graphql/mutations/api_key.py (1)
APIKeyMutations(21-48)backend/apps/nest/graphql/mutations/user.py (1)
UserMutations(26-74)
frontend/src/utils/helpers/apolloClient.ts (1)
frontend/src/utils/utility.ts (1)
getCsrfToken(51-65)
backend/apps/nest/graphql/queries/api_key.py (3)
backend/apps/nest/graphql/nodes/api_key.py (1)
APIKeyNode(11-12)backend/apps/nest/graphql/permissions.py (1)
IsAuthenticated(7-16)backend/apps/nest/models/api_key.py (1)
APIKey(11-70)
backend/apps/core/api/ninja.py (1)
backend/apps/nest/models/api_key.py (4)
APIKey(11-70)authenticate(54-62)generate_hash_key(35-37)is_valid(64-66)
frontend/src/app/settings/api-keys/layout.tsx (2)
frontend/src/utils/credentials.ts (1)
IS_GITHUB_AUTH_ENABLED(9-11)frontend/src/app/api/auth/[...nextauth]/route.ts (1)
session(44-49)
frontend/src/hooks/useLogout.ts (1)
frontend/src/server/queries/authQueries.ts (1)
LOGOUT_DJANGO_MUTATION(3-7)
backend/apps/nest/graphql/nodes/api_key.py (1)
backend/apps/nest/models/api_key.py (1)
APIKey(11-70)
backend/apps/nest/graphql/mutations/user.py (3)
backend/apps/nest/models/user.py (1)
User(9-35)backend/apps/nest/graphql/nodes/user.py (1)
AuthUserNode(9-10)backend/apps/nest/graphql/permissions.py (1)
IsAuthenticated(7-16)
backend/apps/nest/models/api_key.py (2)
backend/apps/github/graphql/queries/user.py (1)
user(40-56)backend/apps/core/api/ninja.py (1)
authenticate(14-36)
frontend/src/hooks/useDjangoSession.ts (2)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
session(44-49)frontend/src/server/queries/authQueries.ts (1)
SYNC_DJANGO_SESSION_MUTATION(9-17)
backend/apps/nest/graphql/mutations/api_key.py (3)
backend/apps/nest/graphql/nodes/api_key.py (1)
APIKeyNode(11-12)backend/apps/nest/graphql/permissions.py (1)
IsAuthenticated(7-16)backend/apps/nest/models/api_key.py (2)
APIKey(11-70)create(40-51)
🪛 Biome (1.9.4)
frontend/src/hooks/useLogout.ts
[error] 19-19: Template literals are preferred over string concatenation.
Unsafe fix: Use a template literal.
(lint/style/useTemplate)
frontend/src/components/skeletons/ApiKeySkelton.tsx
[error] 20-20: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 23-23: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 26-26: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 29-29: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 32-32: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
[error] 35-35: JSX elements without children should be marked as self-closing. In JSX, it is valid for any element to be self-closing.
Unsafe fix: Use a SelfClosingElement instead
(lint/style/useSelfClosingElements)
🪛 GitHub Actions: Run CI/CD
frontend/src/hooks/useLogout.ts
[error] 4-4: Unstaged changes detected. Extra blank line found. Run make check and use git add to address it.
backend/apps/nest/graphql/mutations/user.py
[error] 57-57: Ruff: G004 Logging statement uses f-string.
[error] 67-67: Ruff: D401 First line of docstring should be in imperative mood: "Logs the current user out of their Django session."
[error] 69-69: Ruff: G004 Logging statement uses f-string.
[error] 71-71: Ruff: TRY300 Consider moving this statement to an else block.
[error] 72-72: Ruff: BLE001 Do not catch blind exception: Exception.
[error] 73-73: Ruff: TRY400 Use logging.exception instead of logging.error. G004 Logging statement uses f-string.
frontend/src/components/skeletons/ApiKeySkelton.tsx
[error] 2-2: Unstaged changes detected. Missing semicolon at the end of the statement 'const totalRows = 3'. Run make check and use git add to address it.
backend/apps/nest/graphql/permissions.py
[error] 1-1: Ruff: D100 Missing docstring in public module.
[error] 12-12: Ruff: D102 Missing docstring in public method.
[error] 15-15: Ruff: D102 Missing docstring in public method.
backend/apps/nest/graphql/mutations/api_key.py
[error] 34-35: Ruff: BLE001 Do not catch blind exception: Exception. B904 Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling.
🪛 Ruff (0.11.9)
backend/apps/nest/graphql/mutations/user.py
57-57: Logging statement uses f-string
(G004)
67-67: First line of docstring should be in imperative mood: "Logs the current user out of their Django session."
(D401)
69-69: Logging statement uses f-string
(G004)
71-71: Consider moving this statement to an else block
(TRY300)
72-72: Do not catch blind exception: Exception
(BLE001)
73-73: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
73-73: Logging statement uses f-string
(G004)
backend/apps/nest/graphql/permissions.py
1-1: Missing docstring in public module
(D100)
12-12: Missing docstring in public method
(D102)
15-15: Missing docstring in public method
(D102)
backend/apps/nest/graphql/mutations/api_key.py
34-34: Do not catch blind exception: Exception
(BLE001)
35-35: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: CodeQL (javascript-typescript)
- GitHub Check: CodeQL (python)
🔇 Additional comments (24)
backend/apps/nest/models/__init__.py (1)
1-1: LGTM: Standard model import patternThe import addition follows Django's standard practice of exposing models at the package level for convenient access across the application.
backend/apps/nest/graphql/queries/__init__.py (1)
6-8: LGTM: Clean GraphQL query composition patternThe
NestQueryclass provides a clean composition point for nest-related GraphQL queries by inheriting fromAPIKeyQueries. This pattern allows for future extension with additional query methods while maintaining a clear separation of concerns.backend/settings/api_v1.py (1)
6-6: LGTM: Proper authentication importThe import statement correctly brings in the API key authentication class.
frontend/src/wrappers/provider.tsx (2)
11-14: LGTM: Clean initialization patternThe
AppInitializercomponent follows the correct pattern for side-effect-only components that need to run during app initialization. Returningnullis appropriate for components that don't render anything.
23-26: LGTM: Proper component placementThe
AppInitializeris correctly placed inside theApolloProvidersince theuseDjangoSessionhook likely uses GraphQL mutations for session synchronization. The placement beforechildrenensures initialization runs before the main app content.backend/apps/nest/graphql/nodes/api_key.py (1)
8-12: LGTM: Security-conscious field selectionThe
APIKeyNodecorrectly exposes only non-sensitive fields from theAPIKeymodel. Notable security considerations:
- ✅
hashfield is excluded to prevent key reconstruction- ✅
userfield is excluded to prevent unauthorized access to user data- ✅
key_suffixinclusion is appropriate for user identification (standard practice)- ✅ Exposed fields provide necessary information for UI without security risks
This field selection aligns with security best practices for API key management.
frontend/src/app/api/auth/[...nextauth]/route.ts (2)
1-4: LGTM! Good separation of concerns.Moving the GraphQL mutation handling out of the NextAuth callback to dedicated hooks like
useDjangoSessionimproves code organization and maintainability.
31-34: Simplified callback logic looks good.The streamlined signIn callback appropriately delegates session synchronization to the
useDjangoSessionhook, following the single responsibility principle.backend/apps/nest/graphql/mutations/__init__.py (1)
9-11: Clean consolidation of mutation classes.The multiple inheritance approach effectively combines API key and user mutations under a single GraphQL type, providing a unified interface for the frontend.
frontend/src/utils/helpers/apolloClient.ts (1)
20-25: Good optimization of CSRF token retrieval.Storing the CSRF token in a variable instead of calling
getCsrfToken()twice improves performance and reduces redundant async calls.frontend/src/components/skeletons/ApiKeySkelton.tsx (2)
1-43: Well-structured skeleton component.The component provides good loading UX with appropriate skeleton placeholders for each table column and proper responsive design.
2-2: Add missing semicolon.The pipeline failure indicates a missing semicolon at the end of the statement.
Apply this fix:
- const totalRows = 3; + const totalRows = 3;Likely an incorrect or invalid review comment.
frontend/src/app/settings/api-keys/layout.tsx (1)
6-16: LGTM! Well-implemented authentication guard.The layout component properly enforces access control by checking GitHub authentication availability and user session before allowing access to API key management routes. The logic flow is correct and follows Next.js best practices.
frontend/src/components/UserMenu.tsx (1)
5-7: LGTM! Excellent integration with the new logout hook.The component has been properly updated to use the centralized logout functionality. The changes provide good user experience with:
- Visual loading states during logout
- Disabled buttons to prevent multiple clicks
- Clear feedback with "Signing out..." text
The integration preserves all existing functionality while adding the new logout flow.
Also applies to: 17-17, 56-59, 69-69, 88-92
backend/apps/nest/admin.py (1)
14-21: LGTM! Well-configured admin interface for API keys.The admin configuration provides excellent management capabilities with:
- Comprehensive display of key information
- Useful search and filtering options
- Logical ordering by creation date
- Proper registration with the admin site
The configuration will make API key management efficient for administrators.
backend/apps/nest/graphql/mutations/user.py (1)
7-7: LGTM: Authentication imports properly added.The Django authentication imports and permissions are correctly imported for the new session management functionality.
Also applies to: 12-12
backend/apps/nest/migrations/0002_apikey.py (1)
14-43: LGTM: Well-designed API key model with security considerations.The migration properly implements the APIKey model with several security best practices:
✅ Security Strengths:
hashfield (64 chars) for storing SHA-256 hashes instead of raw keyskey_suffixfield (4 chars) for user identification without exposing full keysis_revokedboolean flag for key lifecycle managementexpires_attimestamp for key expiration- Proper foreign key relationship with CASCADE delete
- Unique constraint on hash field to prevent duplicates
✅ Database Design:
- Custom table name
nest_api_keysfor clear organization- Proper ordering by
-created_atfor recent-first display- Appropriate field lengths and constraints
This aligns well with the security requirements mentioned in the PR objectives.
frontend/src/server/queries/authQueries.ts (1)
1-17: LGTM: Well-structured GraphQL mutations for authentication.The mutations are properly defined and align with the backend implementation:
✅ LOGOUT_DJANGO_MUTATION:
- Simple mutation calling
logoutUserwithout parameters- Matches the backend
logout_usermutation signature✅ SYNC_DJANGO_SESSION_MUTATION:
- Properly typed
$accessToken: String!variable- Correctly calls
githubAuthwith the access token- Appropriately selects
authUser.usernamefield for session syncThe structure integrates well with the backend mutations defined in
backend/apps/nest/graphql/mutations/user.py.frontend/src/types/apikey.ts (1)
1-17: LGTM: Well-defined TypeScript types for API key management.The type definitions are well-structured and align with the backend schema:
✅ CreateApiKeyResult:
- Properly represents the mutation result with both
apiKeyandrawKey- Aligns with security practice of showing raw key only once
✅ ApiKey:
- Complete field coverage matching the backend model
- Proper typing with
numberfor ID,stringfor text fields,booleanfor flags- Consistent camelCase naming convention (converting snake_case from backend)
✅ ApiKeyPageContentProps:
- Proper readonly modifier for immutable prop
- Clear naming for GitHub authentication flag
The types provide good type safety for the frontend API key management features.
backend/apps/nest/graphql/mutations/api_key.py (2)
12-18: Good security design for one-time key display.The
CreateAPIKeyResulttype correctly separates the API key metadata from the raw key, supporting the security best practice of showing the raw key only once at creation time.
37-48: Well-implemented revocation logic with proper authorization.The mutation correctly verifies key ownership before allowing revocation and uses
update_fieldsfor efficient database updates.backend/apps/nest/models/api_key.py (2)
11-28: Secure model design with proper key storage.The model correctly stores API keys as SHA-256 hashes and includes all necessary fields for lifecycle management and user identification.
29-37: Cryptographically strong key generation and hashing.Using
secrets.token_urlsafe(32)provides 256 bits of entropy, and SHA-256 hashing ensures secure storage.frontend/src/server/queries/apiKeyQueries.ts (1)
1-36: Well-structured GraphQL operations.The queries and mutations are properly defined and match the backend GraphQL schema.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
backend/apps/core/api/ninja.py (1)
34-36: Standardize error messages to prevent information disclosure.Different error messages can leak information about whether an API key exists or its status, which was flagged as a security concern in the PR objectives.
backend/apps/nest/graphql/mutations/user.py (1)
65-74: Address multiple code quality issues in logout_user mutation.The logout functionality is implemented correctly, but several code quality issues need to be addressed based on static analysis.
frontend/src/hooks/useDjangoSession.ts (1)
29-31: Fix error handling to prevent unhandled promise rejection.Throwing an error inside a catch block within
useEffectwill cause an unhandled promise rejection. The error won't be caught by React error boundaries.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (5)
backend/apps/core/api/ninja.py(1 hunks)backend/apps/nest/graphql/mutations/user.py(3 hunks)backend/apps/nest/graphql/permissions.py(1 hunks)backend/apps/nest/models/api_key.py(1 hunks)frontend/src/hooks/useDjangoSession.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
backend/apps/nest/graphql/permissions.py (1)
Learnt from: ahmedxgouda
PR: OWASP/Nest#1676
File: backend/apps/owasp/graphql/filters/project_health_metrics.py:17-22
Timestamp: 2025-06-29T00:41:32.198Z
Learning: In the OWASP Nest codebase, when implementing GraphQL filters that convert string values to enums (like ProjectLevel), do not catch ValueError exceptions for invalid values. Let the errors propagate to provide proper error responses to GraphQL clients rather than silently ignoring invalid input.
frontend/src/hooks/useDjangoSession.ts (2)
Learnt from: codic-yeeshu
PR: OWASP/Nest#1444
File: frontend/src/components/NavDropDown.tsx:0-0
Timestamp: 2025-04-30T13:41:20.846Z
Learning: Use React's useId() hook rather than manually generating random strings when creating accessibility identifiers for UI components. This creates stable, unique IDs without causing hydration mismatches.
Learnt from: kasya
PR: OWASP/Nest#1680
File: frontend/src/components/SponsorCard.tsx:3-3
Timestamp: 2025-06-30T00:55:03.133Z
Learning: In the OWASP/Nest project, interfaces or types are not created for component props that are used only once. Inline prop type definitions are preferred for single-use cases.
🧬 Code Graph Analysis (4)
backend/apps/core/api/ninja.py (1)
backend/apps/nest/models/api_key.py (4)
APIKey(11-70)authenticate(54-62)generate_hash_key(35-37)is_valid(64-66)
backend/apps/nest/graphql/mutations/user.py (3)
backend/apps/nest/models/user.py (1)
User(9-35)backend/apps/nest/graphql/nodes/user.py (1)
AuthUserNode(9-10)backend/apps/nest/graphql/permissions.py (1)
IsAuthenticated(9-20)
backend/apps/nest/models/api_key.py (2)
backend/apps/github/graphql/queries/user.py (1)
user(40-56)backend/apps/core/api/ninja.py (1)
authenticate(14-36)
frontend/src/hooks/useDjangoSession.ts (2)
frontend/src/app/api/auth/[...nextauth]/route.ts (1)
session(44-49)frontend/src/server/queries/authQueries.ts (1)
SYNC_DJANGO_SESSION_MUTATION(9-17)
🪛 Ruff (0.11.9)
backend/apps/nest/graphql/permissions.py
20-20: Create your own exception
(TRY002)
backend/apps/nest/graphql/mutations/user.py
67-67: First line of docstring should be in imperative mood: "Logs the current user out of their Django session."
(D401)
69-69: Logging statement uses f-string
(G004)
71-71: Consider moving this statement to an else block
(TRY300)
72-72: Do not catch blind exception: Exception
(BLE001)
73-73: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
73-73: Logging statement uses f-string
(G004)
🪛 GitHub Actions: Run CI/CD
backend/apps/nest/graphql/permissions.py
[error] 20-20: Ruff TRY002: Create your own exception instead of raising generic Exception.
backend/apps/nest/graphql/mutations/user.py
[error] 67-67: Ruff D401: First line of docstring should be in imperative mood: "Logs the current user out of their Django session."
[error] 69-69: Ruff G004: Logging statement uses f-string; consider using logging format style.
[error] 71-71: Ruff TRY300: Consider moving this statement to an else block.
[error] 72-72: Ruff BLE001: Do not catch blind exception: Exception.
[error] 73-73: Ruff TRY400: Use logging.exception instead of logging.error for exception logging.
[error] 73-73: Ruff G004: Logging statement uses f-string; consider using logging format style.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/apps/nest/models/api_key.py (1)
11-70: Excellent API key model implementation with strong security practices.The model demonstrates excellent security practices:
- Uses
secrets.token_urlsafe(32)for cryptographically strong key generation (256 bits entropy)- Stores SHA-256 hashes instead of raw keys
- Implements proper lifecycle management with expiration and revocation
- Separates concerns with well-designed static and class methods
- Includes comprehensive validation logic
The implementation aligns with security best practices mentioned in the PR objectives.
| return ApiKey.objects.filter( | ||
| user=info.context.request.user, | ||
| is_revoked=False, | ||
| ).order_by("-created_at") |
There was a problem hiding this comment.
| return ApiKey.objects.filter( | |
| user=info.context.request.user, | |
| is_revoked=False, | |
| ).order_by("-created_at") | |
| return info.context.request.user.active_api_keys.order_by("-created_at") |
| user=info.context.request.user, | ||
| ) | ||
| api_key.is_revoked = True | ||
| api_key.save(update_fields=["is_revoked"]) |
There was a problem hiding this comment.
You should update updated_at too.
backend/apps/nest/models/api_key.py
Outdated
| @property | ||
| def is_expired(self): | ||
| """Check if the API key has expired.""" | ||
| return self.expires_at <= timezone.now() |
There was a problem hiding this comment.
| return self.expires_at <= timezone.now() | |
| return self.expires_at < timezone.now() |
backend/apps/nest/models/api_key.py
Outdated
|
|
||
| @classmethod | ||
| @transaction.atomic | ||
| def create(cls, user, name, expires_at=None): |
There was a problem hiding this comment.
I guess it's mandatory now?
| def create(cls, user, name, expires_at=None): | |
| def create(cls, user, name, expires_at): |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
frontend/src/app/settings/api-keys/page.tsx (1)
28-387: Component size issue persists - break down for maintainability.This 386-line component still handles multiple responsibilities (data fetching, form management, modal states, table rendering). The previous suggestion to extract separate components remains valid.
🧹 Nitpick comments (3)
frontend/src/app/settings/api-keys/page.tsx (2)
101-107: Improve modal state cleanup consistency.The modal cleanup should reset the expiration to the default value rather than empty string to maintain consistency with the open behavior.
const closeCreateModal = () => { setIsCreateModalOpen(false) setNewKeyName('') - setNewKeyExpiry('') + setNewKeyExpiry(defaultExpiryDate) setNewlyCreatedKey(null) setShowNewKey(false) }
293-298: Add frontend input validation for API key names.Based on the backend model constraints, add validation for name length and invalid characters to provide immediate feedback.
<Input id="api-key-name" value={newKeyName} onChange={(e) => setNewKeyName(e.target.value)} placeholder="e.g., Development, Production, CI/CD" + maxLength={100} + pattern="[a-zA-Z0-9\s\-_]+" + title="Name should contain only letters, numbers, spaces, hyphens, and underscores" />backend/tests/apps/nest/graphql/mutations/api_key_test.py (1)
122-141: Consider adding edge case for already revoked keys.The current tests cover key not found, but consider adding a test case for attempting to revoke an already revoked key to ensure idempotent behavior.
@patch("apps.nest.graphql.mutations.api_key.ApiKey.objects.get") def test_revoke_already_revoked_key(self, mock_objects_get, api_key_mutations): """Test revoking a key that is already revoked.""" info = fake_info() user = info.context.request.user uuid_to_revoke = uuid4() mock_api_key = MagicMock(spec=ApiKey) mock_api_key.is_revoked = True # Already revoked mock_objects_get.return_value = mock_api_key result = api_key_mutations.revoke_api_key(info, uuid=uuid_to_revoke) # Should still succeed (idempotent operation) assert isinstance(result, RevokeApiKeyResult) assert result.ok is True
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
backend/apps/nest/graphql/mutations/api_key.py(1 hunks)backend/apps/nest/graphql/queries/api_key.py(1 hunks)backend/apps/nest/models/api_key.py(1 hunks)backend/tests/apps/nest/graphql/mutations/api_key_test.py(1 hunks)backend/tests/apps/nest/graphql/queries/api_keys_test.py(1 hunks)frontend/src/app/settings/api-keys/page.tsx(1 hunks)frontend/src/types/apiKey.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- frontend/src/types/apiKey.ts
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/apps/nest/graphql/queries/api_key.py
- backend/tests/apps/nest/graphql/queries/api_keys_test.py
- backend/apps/nest/graphql/mutations/api_key.py
- backend/apps/nest/models/api_key.py
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/settings/api-keys/page.tsx (2)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.202Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.516Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
🧬 Code Graph Analysis (1)
frontend/src/app/settings/api-keys/page.tsx (4)
backend/apps/nest/models/api_key.py (1)
ApiKey(16-94)frontend/src/types/apiKey.ts (1)
ApiKey(9-15)frontend/src/server/queries/apiKeyQueries.ts (3)
GET_API_KEYS(3-14)CREATE_API_KEY(16-32)REVOKE_API_KEY(34-42)frontend/src/components/skeletons/ApiKeySkelton.tsx (1)
ApiKeysSkeleton(1-95)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend unit tests
🔇 Additional comments (5)
frontend/src/app/settings/api-keys/page.tsx (1)
96-99: Initialize expiration date to prevent validation issues.The modal opens with an empty expiration date, but the default should be set immediately to ensure the form is always valid.
const openCreateModal = () => { setIsCreateModalOpen(true) - setNewKeyExpiry(defaultExpiryDate) + setNewKeyExpiry(defaultExpiryDate) + setNewKeyName('') }Likely an incorrect or invalid review comment.
backend/tests/apps/nest/graphql/mutations/api_key_test.py (4)
17-25: Well-designed helper function for test mocking.The
fake_info()helper cleanly mocks the GraphQL resolver context with proper authentication setup. This follows good testing practices for isolating dependencies.
36-57: Comprehensive success case testing.The test thoroughly validates the successful API key creation flow, including proper method calls, return values, and result structure verification.
78-98: Good error handling test coverage.The integrity error test properly mocks the exception, verifies logging behavior, and validates the error response structure. This ensures robustness in database failure scenarios.
100-120: Thorough revocation flow testing.The test validates both the database query parameters and the mutation's state changes, ensuring the revocation logic works correctly with proper field updates.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
frontend/src/app/settings/api-keys/page.tsx (1)
28-394: Large component requires decomposition for maintainabilityThis 390+ line component handles multiple responsibilities including data fetching, table rendering, modal management, and form handling. The complexity makes it difficult to maintain and test effectively.
Consider extracting these separate components:
ApiKeyTablefor the keys listing and revocationCreateApiKeyModalfor the creation flowRevokeApiKeyModalfor the confirmation dialogApiKeyUsageInstructionsfor the documentation sectionThis would improve code organization, enable better testing of individual features, and make the codebase more maintainable.
🧹 Nitpick comments (3)
frontend/src/app/settings/api-keys/page.tsx (3)
36-39: Optimize GraphQL query with proper caching strategyThe current query uses
notifyOnNetworkStatusChange: trueanderrorPolicy: 'all'which may cause unnecessary re-renders and network requests.const { loading, error, data, refetch } = useQuery(GET_API_KEYS, { - notifyOnNetworkStatusChange: true, - errorPolicy: 'all', + fetchPolicy: 'cache-and-network', + errorPolicy: 'all', })This change will serve cached data immediately while fetching fresh data in the background, improving perceived performance.
207-210: Handle timezone inconsistencies in date displayThe date formatting uses the user's local timezone, which may cause confusion when API keys expire at different times than expected.
- <td className="py-3">{format(new Date(key.createdAt), 'PP')}</td> + <td className="py-3">{format(new Date(key.createdAt), 'PP p')}</td> <td className="py-3"> - {key.expiresAt ? format(new Date(key.expiresAt), 'PP') : 'Never'} + {key.expiresAt ? format(new Date(key.expiresAt), 'PP p') : 'Never'} </td>Adding time information (
pformat) provides better clarity about when keys expire, especially for users in different timezones.
26-26: Make configuration values environment-dependentThe hardcoded
MAX_ACTIVE_KEYSconstant should be configurable for different environments (development, staging, production).-const MAX_ACTIVE_KEYS = 3 +const MAX_ACTIVE_KEYS = Number(process.env.NEXT_PUBLIC_MAX_ACTIVE_KEYS) || 3This allows flexibility in different deployment environments without code changes.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/settings/api-keys/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/settings/api-keys/page.tsx (4)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.202Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.516Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/components/MetricsCard.tsx:25-25
Timestamp: 2025-07-10T10:04:36.224Z
Learning: In the OWASP Nest project, createdAt fields are auto-created in the backend, so they will always exist and not be null. This makes null checks unnecessary when displaying these dates in the frontend.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
frontend/src/app/settings/api-keys/page.tsx (3)
291-327: Security: Implement auto-clear for sensitive data.The newly created API key remains in component state and DOM until manually dismissed, increasing exposure risk. Consider implementing automatic cleanup after a reasonable timeout.
import { useState } from 'react' +import { useState, useEffect } from 'react'Then add this effect after line 34:
+ useEffect(() => { + if (newlyCreatedKey) { + const timer = setTimeout(() => { + setNewlyCreatedKey(null) + setShowNewKey(false) + }, 5 * 60 * 1000) // 5 minutes + + return () => clearTimeout(timer) + } + }, [newlyCreatedKey])
28-431: Refactor: Break down large component for maintainability.This 400+ line component handles multiple responsibilities and would benefit from extraction into smaller, focused components for better maintainability and testing.
Consider extracting:
ApiKeyTablecomponent (lines 226-261)CreateApiKeyModalcomponent (lines 287-402)RevokeApiKeyModalcomponent (lines 404-428)ApiKeyUsageInstructionscomponent (lines 264-284)This would reduce the main component size by ~60% and improve code organization.
101-108: Add future date validation for expiration.The validation checks for expiry date presence but doesn't verify it's in the future, which could allow creating keys that are immediately expired.
if (!newKeyExpiry) { addToast({ title: 'Error', description: 'Please select an expiration date', color: 'danger' }) return } + if (new Date(newKeyExpiry) <= new Date()) { + addToast({ title: 'Error', description: 'Expiration date must be in the future', color: 'danger' }) + return + }
🧹 Nitpick comments (3)
frontend/src/app/settings/api-keys/page.tsx (3)
92-99: Inconsistent validation pattern documentation.The regex pattern
/[^a-zA-Z0-9\s-]/is more restrictive than the description suggests. It only allows letters, numbers, spaces, and hyphens, but the description mentions "spaces, and hyphens" without listing the full allowed character set.if (newKeyName.match(/[^a-zA-Z0-9\s-]/)) { addToast({ title: 'Error', - description: 'Name can only contain letters, numbers, spaces, and hyphens', + description: 'Name can only contain letters, numbers, spaces, and hyphens', color: 'danger', }) return }Consider also updating the helper text on line 341 to match:
- Use letters, numbers, spaces, and hyphens only. Avoid special characters. + Use letters, numbers, spaces, and hyphens only.
244-244: Enhance null safety for date formatting.While the ternary operator handles null
expiresAt, it's worth ensuring the date parsing is robust for edge cases.- {key.expiresAt ? format(new Date(key.expiresAt), 'PP') : 'Never'} + {key.expiresAt ? format(new Date(key.expiresAt), 'PP') : 'Never'}The current implementation is correct. However, consider adding error handling if date parsing could fail:
{key.expiresAt ? ( (() => { try { return format(new Date(key.expiresAt), 'PP') } catch { return 'Invalid date' } })() ) : 'Never'}
26-26: Configuration: Consider making MAX_ACTIVE_KEYS configurable.The hardcoded limit should ideally be configurable through environment variables or a settings file to avoid duplicating this constant across frontend and backend.
-const MAX_ACTIVE_KEYS = 3 +const MAX_ACTIVE_KEYS = process.env.NEXT_PUBLIC_MAX_ACTIVE_KEYS ? parseInt(process.env.NEXT_PUBLIC_MAX_ACTIVE_KEYS) : 3This would allow runtime configuration while maintaining the fallback value.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/settings/api-keys/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/settings/api-keys/page.tsx (3)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.516Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/components/MetricsCard.tsx:25-25
Timestamp: 2025-07-10T10:04:36.224Z
Learning: In the OWASP Nest project, createdAt fields are auto-created in the backend, so they will always exist and not be null. This makes null checks unnecessary when displaying these dates in the frontend.
🔇 Additional comments (2)
frontend/src/app/settings/api-keys/page.tsx (2)
1-432: Overall assessment: Well-implemented with room for improvement.The component successfully implements API key management with proper validation, error handling, and user experience considerations. The code addresses most security and usability concerns from previous reviews.
Strengths:
- Comprehensive input validation
- Proper error handling and user feedback
- Secure handling of sensitive data
- Good UX with loading states and confirmations
- Consistent styling and responsive design
Areas for improvement:
- Component size and single responsibility
- Configuration management
- Enhanced security with auto-clear functionality
- Minor validation edge cases
149-149: GraphQL variable name and type are consistentThe
RevokeApiKeymutation is defined asmutation RevokeApiKey($uuid: UUID!)and the call atfrontend/src/app/settings/api-keys/page.tsx:149usesvariables: { uuid: keyToRevoke.uuid }. No mismatches between ID and Int types were found. No changes required.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
frontend/src/app/settings/api-keys/page.tsx (2)
28-432: Component remains too large - refactoring still neededThis component is still 432 lines and handles multiple responsibilities (data fetching, UI rendering, form management, modals). The structure violates the single responsibility principle and makes the code harder to maintain and test.
Consider extracting these into separate components:
ApiKeyTable- handles the table display and revoke actionsCreateApiKeyModal- handles key creation flowApiKeyUsageInstructions- displays usage informationApiKeyLimitsInfo- displays limit informationThis will improve maintainability, testability, and code organization.
292-328: Security: Implement auto-clear for displayed API keyThe newly created API key remains in component state and DOM indefinitely, which poses a security risk through memory dumps or browser inspection tools.
Add auto-clear functionality to remove the key from memory after a reasonable timeout:
const [createApiKey, { loading: createLoading }] = useMutation(CREATE_API_KEY, { onCompleted: (data) => { const result = data.createApiKey if (!result?.ok) { addToast({ title: 'Error', description: result.message || 'Failed to create API key', color: 'danger', }) return } setNewlyCreatedKey(result.rawKey) + // Auto-clear the key after 5 minutes for security + setTimeout(() => { + setNewlyCreatedKey(null) + setShowNewKey(false) + }, 5 * 60 * 1000) addToast({ title: 'API Key Created', description: "Copy it now - you won't see it again!", color: 'success', }) refetch() },
🧹 Nitpick comments (2)
frontend/src/app/settings/api-keys/page.tsx (2)
102-109: Add future date validation for extra securityWhile the HTML input has
minattribute to prevent past dates, adding explicit validation provides additional protection against manually crafted requests or JavaScript manipulation.if (!newKeyExpiry) { addToast({ title: 'Error', description: 'Please select an expiration date', color: 'danger' }) return } + if (new Date(newKeyExpiry) <= new Date()) { + addToast({ title: 'Error', description: 'Expiration date must be in the future', color: 'danger' }) + return + } const variables: { name: string; expiresAt: Date } = { name: newKeyName.trim(), expiresAt: new Date(newKeyExpiry), }
29-34: Consider consolidating related state with useReducerMultiple related state variables could be better organized using
useReducerto improve maintainability and prevent state synchronization issues.+ import { useReducer } from 'react' + + interface ApiKeyState { + isCreateModalOpen: boolean + newKeyName: string + newKeyExpiry: string + showNewKey: boolean + newlyCreatedKey: string | null + keyToRevoke: ApiKey | null + } + + const initialState: ApiKeyState = { + isCreateModalOpen: false, + newKeyName: '', + newKeyExpiry: '', + showNewKey: false, + newlyCreatedKey: null, + keyToRevoke: null, + } - const [isCreateModalOpen, setIsCreateModalOpen] = useState(false) - const [newKeyName, setNewKeyName] = useState('') - const [newKeyExpiry, setNewKeyExpiry] = useState('') - const [showNewKey, setShowNewKey] = useState(false) - const [newlyCreatedKey, setNewlyCreatedKey] = useState<string | null>(null) - const [keyToRevoke, setKeyToRevoke] = useState<ApiKey | null>(null) + const [state, dispatch] = useReducer(apiKeyReducer, initialState)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
frontend/src/app/settings/api-keys/page.tsx(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
frontend/src/app/settings/api-keys/page.tsx (4)
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/__tests__/unit/pages/createProgram.test.tsx:70-86
Timestamp: 2025-07-12T17:36:57.202Z
Learning: When testing React page components that use mocked form components, validation logic should be tested at the form component level, not the page level. Page-level tests should focus on authentication, role checking, submission handling, and navigation logic.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:41-44
Timestamp: 2025-07-08T17:24:36.501Z
Learning: In the mentorship program GraphQL mutations, date validation is handled at the GraphQL schema/node level in the input types (CreateProgramInput, UpdateProgramInput), preventing null values from reaching the mutation logic where date comparisons are performed.
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: frontend/src/app/mentorship/programs/[programKey]/edit/page.tsx:90-128
Timestamp: 2025-07-12T17:14:28.516Z
Learning: Both ProgramForm (programCard.tsx) and ModuleForm (mainmoduleCard.tsx) components already implement HTML validation using the `required` attribute on form fields. The browser's native validation prevents form submission and displays error messages when required fields are empty, eliminating the need for additional JavaScript validation before GraphQL mutations.
Learnt from: ahmedxgouda
PR: OWASP/Nest#1718
File: frontend/src/components/MetricsCard.tsx:25-25
Timestamp: 2025-07-10T10:04:36.224Z
Learning: In the OWASP Nest project, createdAt fields are auto-created in the backend, so they will always exist and not be null. This makes null checks unnecessary when displaying these dates in the frontend.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: Run backend tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
frontend/src/app/settings/api-keys/page.tsx (1)
75-77: Good practices implementedThe component correctly implements the maximum active keys limit and proper default expiry date calculation. The validation and error handling have been significantly improved from previous iterations.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
backend/tests/apps/nest/graphql/mutations/api_key_test.py (2)
28-142: Add test coverage for the critical race condition identified in PR objectives.The current tests don't cover the race condition in API key creation where multiple concurrent requests can bypass the maximum active keys limit.
Based on the PR objectives, there's a critical race condition that needs test coverage. Add this test method:
@patch("apps.nest.graphql.mutations.api_key.ApiKey.objects.filter") @patch("apps.nest.graphql.mutations.api_key.ApiKey.objects.create") def test_create_api_key_race_condition(self, mock_create, mock_filter, api_key_mutations): """Test that concurrent API key creation doesn't bypass the limit.""" info = mock_info() user = info.context.request.user # Simulate race condition: count shows under limit but creation should fail mock_filter.return_value.count.return_value = MAX_ACTIVE_KEYS - 1 mock_create.side_effect = IntegrityError("Key limit constraint violated") result = api_key_mutations.create_api_key( info, name="Concurrent Key", expires_at=timezone.now() + timedelta(days=30) ) assert not result.ok assert result.code == "ERROR"This test would verify the fix mentioned in PR objectives using database constraints.
28-142: Add missing edge case tests for input validation.The current test suite doesn't cover input validation scenarios that were identified as missing in the PR objectives.
Add these test methods to cover input validation gaps:
def test_create_api_key_empty_name(self, api_key_mutations): """Test creating an API key with empty name.""" info = mock_info() result = api_key_mutations.create_api_key( info, name="", expires_at=timezone.now() + timedelta(days=30) ) assert not result.ok assert result.code == "VALIDATION_ERROR" def test_create_api_key_past_expiry(self, api_key_mutations): """Test creating an API key with past expiry date.""" info = mock_info() result = api_key_mutations.create_api_key( info, name="Valid Name", expires_at=timezone.now() - timedelta(days=1) ) assert not result.ok assert result.code == "VALIDATION_ERROR" def test_mutations_require_authentication(self, api_key_mutations): """Test that mutations fail for unauthenticated users.""" info = mock_info(is_authenticated=False) # Both mutations should fail for unauthenticated users create_result = api_key_mutations.create_api_key( info, name="Test", expires_at=timezone.now() + timedelta(days=30) ) revoke_result = api_key_mutations.revoke_api_key(info, uuid=uuid4()) assert not create_result.ok assert not revoke_result.okThese tests address the missing input validation mentioned in the PR objectives.
🧹 Nitpick comments (2)
backend/tests/apps/nest/graphql/mutations/api_key_test.py (2)
17-25: Enhance the mock_info() helper function for better test coverage.The current implementation is functional but could be more robust for testing authentication scenarios.
Consider this enhanced version that allows testing different authentication states:
-def mock_info() -> MagicMock: - """Return a mocked Info object.""" - info = MagicMock() - info.context = MagicMock() - info.context.request = MagicMock() - info.context.request.user = MagicMock() - info.context.request.user.is_authenticated = True - info.context.request.user.pk = 1 - return info +def mock_info(user_id: int = 1, is_authenticated: bool = True) -> MagicMock: + """Return a mocked Info object with configurable user state.""" + info = MagicMock() + info.context = MagicMock() + info.context.request = MagicMock() + info.context.request.user = MagicMock() + info.context.request.user.is_authenticated = is_authenticated + info.context.request.user.pk = user_id + return infoThis would enable testing unauthenticated scenarios and different user contexts.
59-76: Improve assertion pattern for limit reached scenario.The test correctly verifies the limit reached behavior but could be more explicit about testing the actual limit logic.
Consider adding verification that the mutation checked the limit before failing:
@patch("apps.nest.graphql.mutations.api_key.ApiKey.create", return_value=None) +@patch("apps.nest.graphql.mutations.api_key.ApiKey.objects.filter") -def test_create_api_key_limit_reached(self, mock_api_key_create, api_key_mutations): +def test_create_api_key_limit_reached(self, mock_filter, mock_api_key_create, api_key_mutations): """Test creating an API key when the user has reached their active key limit.""" + # Setup: user already has maximum active keys + mock_filter.return_value.count.return_value = MAX_ACTIVE_KEYS + info = mock_info() user = info.context.request.user name = "This key should not be created" expires_at = timezone.now() + timedelta(days=30) result = api_key_mutations.create_api_key(info, name=name, expires_at=expires_at) mock_api_key_create.assert_called_once_with(user=user, name=name, expires_at=expires_at) + mock_filter.assert_called_with(user=user, is_revoked=False)
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
backend/apps/nest/admin.py(2 hunks)backend/apps/nest/graphql/mutations/api_key.py(1 hunks)backend/apps/nest/models/user.py(2 hunks)backend/tests/apps/nest/graphql/mutations/api_key_test.py(1 hunks)backend/tests/apps/nest/graphql/nodes/api_keys_test.py(1 hunks)backend/tests/apps/nest/graphql/queries/api_keys_test.py(1 hunks)backend/tests/apps/nest/models/api_key_test.py(1 hunks)frontend/src/server/queries/apiKeyQueries.ts(1 hunks)frontend/src/server/queries/authQueries.ts(1 hunks)frontend/src/types/apiKey.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (9)
- backend/apps/nest/models/user.py
- backend/tests/apps/nest/graphql/nodes/api_keys_test.py
- backend/tests/apps/nest/models/api_key_test.py
- backend/apps/nest/admin.py
- frontend/src/server/queries/authQueries.ts
- backend/tests/apps/nest/graphql/queries/api_keys_test.py
- frontend/src/server/queries/apiKeyQueries.ts
- frontend/src/types/apiKey.ts
- backend/apps/nest/graphql/mutations/api_key.py
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (1)
backend/tests/apps/nest/graphql/mutations/api_key_test.py (1)
114-115: Fix test assertion for API key revocation.The test directly sets the
is_revokedattribute instead of verifying that the mutation sets it correctly.This assertion pattern is incorrect - it sets the value instead of testing that the mutation sets it:
- assert mock_api_key.is_revoked is True + # Verify the mutation set is_revoked to True + assert mock_api_key.is_revoked == TrueOr better yet, verify the mutation called the setter:
- assert mock_api_key.is_revoked is True + # The mutation should have set is_revoked to True + mock_api_key.__setattr__.assert_any_call('is_revoked', True)Likely an incorrect or invalid review comment.
* API Key management backend optimization * fix lint * Added code rabbit suggestions * Initial frontend * fix check * Added model * Added new logic for authentication * code rabbit suggestion * fix checks * backend test fix * frontend test fix * added tests fe * cleanup * fixes * Added skeleton for laoding and better gql responses * fixed cases * fixed the hook * fix code * added testcases * Update code * fixed warnings * Added comments on auth * Updated code * updated types and comments on the backend * Update code (w/o test fixes) * updated code new suggestions * fix fe tests * Added the remaining suggestions * code rabbit optimizations * code rabbit optimizations -2 * bug * Update code --------- Co-authored-by: Kate Golovanova <kate@kgthreads.com> Co-authored-by: Arkadii Yakovets <arkadii.yakovets@owasp.org>
abhayymishraa
added
gssoc2025
3 participants
Resolves #1616
Add the PR description here.