Refactor API to a separate app, add v0 #2245
Conversation
Summary by CodeRabbit
WalkthroughCreates a new apps.api Django app (AppConfig, model, migration), migrates ApiKey-related admin/GraphQL/REST code from nest/owasp/github into apps.api, introduces REST v0 with unified routers and ApiKeyHeader auth, updates settings/urls to use /api/v0/, adjusts lint config, and updates tests accordingly. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. ✨ 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
backend/tests/apps/api/rest/v0/member_test.py (1)
47-48: Fix parsing of ISO datetimes with 'Z'.datetime.fromisoformat does not accept a 'Z' suffix in many Python versions. Replace 'Z' with '+00:00' in expectations to avoid failures.
Apply:
- assert member.created_at == datetime.fromisoformat(member_data["created_at"]) - assert member.updated_at == datetime.fromisoformat(member_data["updated_at"]) + assert member.created_at == datetime.fromisoformat(member_data["created_at"].replace("Z", "+00:00")) + assert member.updated_at == datetime.fromisoformat(member_data["updated_at"].replace("Z", "+00:00"))backend/tests/apps/api/rest/v0/issue_test.py (1)
38-39: Handle 'Z' timezone in expectations.Same issue: fromisoformat won't parse 'Z'. Normalize to '+00:00'.
Apply:
- assert schema.created_at == datetime.fromisoformat(issue_data["created_at"]) - assert schema.updated_at == datetime.fromisoformat(issue_data["updated_at"]) + assert schema.created_at == datetime.fromisoformat(issue_data["created_at"].replace("Z", "+00:00")) + assert schema.updated_at == datetime.fromisoformat(issue_data["updated_at"].replace("Z", "+00:00"))backend/tests/apps/api/rest/v0/organization_test.py (1)
32-33: Normalize 'Z' timezone in expectations.Avoid fromisoformat('...Z') failures by replacing with '+00:00'.
Apply:
- assert schema.created_at == datetime.fromisoformat(organization_data["created_at"]) - assert schema.updated_at == datetime.fromisoformat(organization_data["updated_at"]) + assert schema.created_at == datetime.fromisoformat(organization_data["created_at"].replace("Z", "+00:00")) + assert schema.updated_at == datetime.fromisoformat(organization_data["updated_at"].replace("Z", "+00:00"))backend/apps/api/internal/mutations/api_key.py (1)
52-56: Handle naive datetimes to avoid TypeError in expiry check.Comparing a naive expires_at to timezone-aware now() raises at runtime. Normalize before comparison.
Apply this diff:
- if expires_at <= timezone.now(): + # Normalize to aware datetime before comparison. + now = timezone.now() + if timezone.is_naive(expires_at): + expires_at = timezone.make_aware(expires_at, timezone.get_current_timezone()) + if expires_at <= now: return CreateApiKeyResult( ok=False, code="INVALID_DATE", message="Expiry date must be in future" )backend/apps/api/rest/v0/__init__.py (1)
83-89: API root says v1 but this is v0.User-facing mismatch; fix the welcome banner.
Apply this diff:
- "message": "Welcome to the OWASP Nest API v1", + "message": "Welcome to the OWASP Nest API v0",
🧹 Nitpick comments (23)
backend/apps/api/rest/auth/__init__.py (1)
1-1: Re-export ApiKeyHeader for a stable import path.Enable
from apps.api.rest.auth import ApiKeyHeaderto avoid deep imports.-"""OWASP REST API.""" +"""OWASP REST API auth package.""" + +# Public re-exports for stable import paths. +from .api_key import ApiKeyHeader + +__all__ = ["ApiKeyHeader"]backend/apps/api/rest/__init__.py (1)
1-1: Expose default REST version alias from the package.Handy for callers to discover the current version without importing
v0directly.-"""OWASP REST API.""" +"""OWASP REST API package.""" + +# Convenience re-export of the current default version. +from .v0 import ROUTERS as V0_ROUTERS + +__all__ = ["V0_ROUTERS"]backend/pyproject.toml (1)
119-123: Narrow the Ruff ignore scope to reduce blanket suppressions.Limiting to
v0helps surface issues in future versions while keeping current ergonomics.-lint.per-file-ignores."apps/api/rest/**/*.py" = [ +lint.per-file-ignores."apps/api/rest/v0/**/*.py" = [ "ARG001", # https://docs.astral.sh/ruff/rules/unused-function-argument/ "B008", # https://docs.astral.sh/ruff/rules/function-call-in-default-argument/ ]If only a few endpoints trigger B008/ARG001, consider local
# noqainstead.backend/apps/api/apps.py (1)
6-9: Optional: set a human-friendly verbose name for the app.Purely cosmetic in admin.
class ApiConfig(AppConfig): """API app config.""" name = "apps.api" + verbose_name = "API"backend/apps/api/migrations/0001_initial.py (1)
21-43: Consider adding pragmatic indexes for common queries.If you frequently list active keys or clean up expired/revoked keys, add indexes to keep scans cheap.
Example follow-up migration:
+class Migration(migrations.Migration): + dependencies = [("api", "0001_initial")] + operations = [ + migrations.AddIndex( + model_name="apikey", + index=models.Index(fields=["user", "is_revoked"], name="ak_user_revoked_idx"), + ), + migrations.AddIndex( + model_name="apikey", + index=models.Index(fields=["expires_at"], name="ak_expires_at_idx"), + ), + ]If your hot paths differ (e.g., filter by hash prefix), tailor indexes accordingly.
backend/apps/api/internal/queries/api_key.py (1)
20-30: Optional: return a concrete list or optimize queryset usage.If downstream expects a list (not a lazy queryset), wrap with list(); if staying queryset, consider selecting only required fields to reduce payload.
Example:
- return info.context.request.user.active_api_keys.order_by("-created_at") + return list( + info.context.request.user.active_api_keys + .only("uuid", "name", "created_at", "expires_at", "is_revoked") + .order_by("-created_at") + )backend/tests/apps/api/rest/v0/committee_test.py (1)
29-30: Parse 'Z' timestamps portably
datetime.fromisoformatrejects 'Z' in some Python versions; normalize via.replace("Z", "+00:00"). Apply:- assert committee.created_at == datetime.fromisoformat(committee_data["created_at"]) - assert committee.updated_at == datetime.fromisoformat(committee_data["updated_at"]) + assert committee.created_at == datetime.fromisoformat(committee_data["created_at"].replace("Z", "+00:00")) + assert committee.updated_at == datetime.fromisoformat(committee_data["updated_at"].replace("Z", "+00:00"))No other
fromisoformat(... "Z" ...)occurrences found inbackend/tests/.backend/apps/api/internal/nodes/api_key.py (1)
12-17: Consider exposing last_used_at.
If helpful to clients, include last_used_at in the node fields.Apply this diff:
fields=[ "created_at", "is_revoked", "expires_at", "name", "uuid", + "last_used_at", ],backend/tests/apps/api/rest/v0/project_test.py (1)
31-33: Update test assertions to compare enum values and handle ‘Z’ timestamps (backend/tests/apps/api/rest/v0/project_test.py:31–33)- assert project.level == project_data["level"] - assert project.created_at == datetime.fromisoformat(project_data["created_at"]) - assert project.updated_at == datetime.fromisoformat(project_data["updated_at"]) + assert getattr(project.level, "value", project.level) == project_data["level"] + assert project.created_at == datetime.fromisoformat(project_data["created_at"].replace("Z", "+00:00")) + assert project.updated_at == datetime.fromisoformat(project_data["updated_at"].replace("Z", "+00:00"))backend/tests/apps/api/rest/v0/chapter_test.py (1)
32-33: Parse 'Z' timestamps portably.
Normalize 'Z' to '+00:00' for stable parsing.Apply this diff:
- assert chapter.created_at == datetime.fromisoformat(chapter_data["created_at"]) - assert chapter.updated_at == datetime.fromisoformat(chapter_data["updated_at"]) + assert chapter.created_at == datetime.fromisoformat(chapter_data["created_at"].replace("Z", "+00:00")) + assert chapter.updated_at == datetime.fromisoformat(chapter_data["updated_at"].replace("Z", "+00:00"))backend/apps/api/admin/api_key.py (1)
21-27: Make sensitive fields read-only in admin.
Prevents accidental edits to identifiers/timestamps.Apply this diff:
list_filter = ("is_revoked",) ordering = ("-created_at",) + readonly_fields = ("uuid", "hash", "created_at", "updated_at", "last_used_at") search_fields = (backend/apps/api/models/__init__.py (1)
1-1: Add all for explicit export.
Small packaging nicety.Apply this diff:
from .api_key import ApiKey + +__all__ = ["ApiKey"]backend/apps/api/internal/queries/__init__.py (1)
6-9: Class name may be confusing here.
Inside apps.api, NestQuery reads oddly; consider ApiQuery in a follow-up (with aliases if needed).backend/apps/api/admin/__init__.py (1)
1-1: Re-export looks good; add all for clarity.Apply this diff:
from .api_key import ApiKeyAdmin + +__all__ = ["ApiKeyAdmin"]backend/tests/apps/api/rest/v0/release_test.py (1)
1-1: Use parse_datetime for ISO-8601 with Z.
Prevents cross-version flakiness and keeps tz-aware behavior.-from datetime import datetime +from datetime import datetime +from django.utils.dateparse import parse_datetime @@ - assert schema.created_at == datetime.fromisoformat(release_data["created_at"]) - assert schema.published_at == datetime.fromisoformat(release_data["published_at"]) + assert schema.created_at == parse_datetime(release_data["created_at"]) + assert schema.published_at == parse_datetime(release_data["published_at"])Also applies to: 30-31
backend/apps/api/internal/mutations/__init__.py (1)
1-10: Export the public symbol explicitly.
Helps static tools and importers."""API app mutations.""" import strawberry from .api_key import ApiKeyMutations @strawberry.type class ApiMutations(ApiKeyMutations): """API mutations.""" + +__all__ = ["ApiMutations"]backend/tests/apps/api/models/api_key_test.py (1)
25-31: Remove flaky randomness assertion.
Asserting two secrets.token_urlsafe() calls differ tests the stdlib, not your method; theoretically flaky.def test_generate_raw_key(self): """Test that generate_raw_key creates a sufficiently long, random string.""" raw_key = ApiKey.generate_raw_key() assert isinstance(raw_key, str) assert len(raw_key) > 40 - assert secrets.token_urlsafe(32) != secrets.token_urlsafe(32)backend/apps/api/rest/auth/api_key.py (1)
11-34: Docstring return contract mismatches behavior.
Method returns an ApiKey or raises 401; it never returns None. Align the docstring, and optionally document the 401.class ApiKeyHeader(APIKeyHeader): """Custom API key authentication class for Ninja.""" @@ def authenticate(self, request, key: str) -> ApiKey: - """Authenticate the API key from the request header. + """Authenticate the API key from the request header. @@ - Returns: - APIKey: The APIKey object if the key is valid, otherwise None. + Returns: + ApiKey: The ApiKey object if the key is valid. + + Raises: + HttpError: 401 if the header is missing or the key is invalid.Confirm downstream views expect request.auth (ApiKey) and not request.user. If they need request.user, consider returning api_key.user or attaching it to request for convenience.
backend/tests/apps/api/rest/v0/urls_test.py (1)
3-13: Avoid coupling to private attributes and object identity in router tests.
Accessing main_router._routers and asserting identity with "is" makes tests brittle. Prefer public APIs (e.g., inspecting main_router.urls or registered paths) or compare prefixes only.- registered_router_map = dict(main_router._routers) + registered_router_map = dict(main_router._routers) # consider a helper that uses public API @@ - assert registered_router_map[prefix] is expected_router_instance + assert prefix in registered_router_map # reduce reliance on object identityIf a public accessor exists in apps.api.rest.v0 (e.g., ROUTERS or a getter), switch to that.
backend/settings/urls.py (1)
14-14: Dropping /api/v1/ is a breaking change—consider a temporary alias/redirect.
Minimize client disruption while refactoring.-from apps.api.rest.v0 import api as api_v0 +from apps.api.rest.v0 import api as api_v0 +# Optional temporary alias to keep v1 working during migration: +# from apps.api.rest.v0 import api as api_v1 @@ - path("api/v0/", api_v0.urls), + path("api/v0/", api_v0.urls), + # Optional: keep legacy path for one release cycle + # path("api/v1/", api_v1.urls),Confirm external consumers and docs; if v1 is intentionally removed now, add a deprecation note in CHANGELOG and update API docs.
Also applies to: 26-26
backend/tests/apps/api/internal/nodes/api_key_test.py (1)
27-34: Make UUID type assertion resilient to Strawberry’s UUID scalar.Across Strawberry versions, UUID may surface as strawberry.scalars.UUID. Broaden the assertion to accept both.
Apply this diff:
+import strawberry @@ - assert fields_map["uuid"].type is uuid.UUID + # Accept both built-in and Strawberry UUID scalar. + assert fields_map["uuid"].type in {uuid.UUID, getattr(strawberry.scalars, "UUID", uuid.UUID)}backend/tests/apps/api/internal/mutations/api_key_test.py (1)
36-77: Add negative-path tests for validation.Cover input validation branches to lock behavior before/after refactors: blank name, overlong name, and past/naive expiry.
Example tests to add:
def test_create_api_key_invalid_name_blank(api_key_mutations): info = mock_info() result = api_key_mutations.create_api_key(info, name=" ", expires_at=timezone.now() + timedelta(days=1)) assert not result.ok and result.code == "INVALID_NAME" def test_create_api_key_name_too_long(api_key_mutations): info = mock_info() long_name = "x" * (MAX_ACTIVE_KEYS + 512) # or MAX_WORD_LENGTH + 1 if available here result = api_key_mutations.create_api_key(info, name=long_name, expires_at=timezone.now() + timedelta(days=1)) assert not result.ok and result.code == "INVALID_NAME" @patch("apps.api.internal.mutations.api_key.ApiKey.create") def test_create_api_key_invalid_expiry_in_past(mock_create, api_key_mutations): info = mock_info() past = timezone.now() - timedelta(days=1) result = api_key_mutations.create_api_key(info, name="n", expires_at=past) mock_create.assert_not_called() assert not result.ok and result.code == "INVALID_DATE"Also applies to: 59-77, 78-99
backend/apps/api/internal/mutations/api_key.py (1)
14-14: Cross-app permission import—confirm intended dependency.If the goal is to fully decouple into apps.api, consider relocating or re-exporting IsAuthenticated under apps.api to avoid reaching into apps.nest.
Would you like me to open a follow-up PR to move/re-export this permission?
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (48)
backend/apps/api/__init__.py(1 hunks)backend/apps/api/admin/__init__.py(1 hunks)backend/apps/api/admin/api_key.py(1 hunks)backend/apps/api/apps.py(1 hunks)backend/apps/api/internal/mutations/__init__.py(1 hunks)backend/apps/api/internal/mutations/api_key.py(3 hunks)backend/apps/api/internal/nodes/api_key.py(1 hunks)backend/apps/api/internal/queries/__init__.py(1 hunks)backend/apps/api/internal/queries/api_key.py(1 hunks)backend/apps/api/migrations/0001_initial.py(1 hunks)backend/apps/api/models/__init__.py(1 hunks)backend/apps/api/models/api_key.py(2 hunks)backend/apps/api/rest/__init__.py(1 hunks)backend/apps/api/rest/auth/__init__.py(1 hunks)backend/apps/api/rest/auth/api_key.py(1 hunks)backend/apps/api/rest/v0/__init__.py(1 hunks)backend/apps/github/api/rest/__init__.py(0 hunks)backend/apps/github/api/rest/v1/__init__.py(0 hunks)backend/apps/nest/admin/__init__.py(0 hunks)backend/apps/nest/api/internal/mutations/__init__.py(1 hunks)backend/apps/nest/migrations/0006_delete_apikey.py(1 hunks)backend/apps/nest/models/__init__.py(0 hunks)backend/apps/nest/models/user.py(1 hunks)backend/apps/owasp/api/rest/__init__.py(0 hunks)backend/apps/owasp/api/rest/v1/__init__.py(0 hunks)backend/pyproject.toml(1 hunks)backend/settings/api/__init__.py(0 hunks)backend/settings/base.py(1 hunks)backend/settings/graphql.py(2 hunks)backend/settings/urls.py(1 hunks)backend/tests/apps/api/internal/mutations/api_key_test.py(6 hunks)backend/tests/apps/api/internal/nodes/api_key_test.py(1 hunks)backend/tests/apps/api/internal/queries/api_key_test.py(1 hunks)backend/tests/apps/api/models/api_key_test.py(4 hunks)backend/tests/apps/api/rest/auth/api_key_test.py(2 hunks)backend/tests/apps/api/rest/v0/chapter_test.py(1 hunks)backend/tests/apps/api/rest/v0/committee_test.py(1 hunks)backend/tests/apps/api/rest/v0/event_test.py(1 hunks)backend/tests/apps/api/rest/v0/issue_test.py(1 hunks)backend/tests/apps/api/rest/v0/label_test.py(1 hunks)backend/tests/apps/api/rest/v0/member_test.py(1 hunks)backend/tests/apps/api/rest/v0/organization_test.py(1 hunks)backend/tests/apps/api/rest/v0/project_test.py(1 hunks)backend/tests/apps/api/rest/v0/release_test.py(1 hunks)backend/tests/apps/api/rest/v0/repository_test.py(1 hunks)backend/tests/apps/api/rest/v0/urls_test.py(1 hunks)backend/tests/apps/nest/api/internal/nodes/api_keys_test.py(1 hunks)backend/tests/apps/nest/api/internal/queries/api_keys_test.py(1 hunks)
💤 Files with no reviewable changes (7)
- backend/settings/api/init.py
- backend/apps/nest/models/init.py
- backend/apps/nest/admin/init.py
- backend/apps/github/api/rest/init.py
- backend/apps/owasp/api/rest/init.py
- backend/apps/github/api/rest/v1/init.py
- backend/apps/owasp/api/rest/v1/init.py
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-13T05:55:46.436Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/mutations/program.py:166-166
Timestamp: 2025-07-13T05:55:46.436Z
Learning: In the OWASP Nest mentorship GraphQL mutations, Strawberry GraphQL automatically converts between Django Program instances and ProgramNode types, so mutations can return Program instances directly without manual conversion even when typed as ProgramNode, similar to the Module/ModuleNode pattern.
Applied to files:
backend/settings/graphql.py
📚 Learning: 2025-07-11T15:57:56.648Z
Learnt from: Rajgupta36
PR: OWASP/Nest#1717
File: backend/apps/mentorship/graphql/queries/module.py:39-50
Timestamp: 2025-07-11T15:57:56.648Z
Learning: In the OWASP Nest mentorship GraphQL queries, Strawberry GraphQL automatically converts between Django Module instances and ModuleNode types, so methods can return Module instances directly without manual conversion even when typed as ModuleNode.
Applied to files:
backend/settings/graphql.py
🧬 Code graph analysis (33)
backend/apps/api/internal/nodes/api_key.py (1)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/apps/api/internal/queries/__init__.py (1)
backend/apps/api/internal/queries/api_key.py (1)
ApiKeyQueries(11-30)
backend/tests/apps/api/internal/mutations/api_key_test.py (2)
backend/apps/api/internal/mutations/api_key.py (3)
ApiKeyMutations(40-107)CreateApiKeyResult(29-36)RevokeApiKeyResult(20-25)backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/apps/api/admin/__init__.py (1)
backend/apps/api/admin/api_key.py (1)
ApiKeyAdmin(8-27)
backend/tests/apps/api/rest/v0/event_test.py (1)
backend/apps/api/rest/v0/event.py (1)
EventSchema(18-25)
backend/tests/apps/api/rest/v0/urls_test.py (1)
backend/apps/api/rest/v0/__init__.py (1)
api_root(84-89)
backend/apps/api/models/__init__.py (1)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/tests/apps/api/rest/v0/committee_test.py (1)
backend/apps/api/rest/v0/committee.py (1)
CommitteeSchema(18-24)
backend/tests/apps/api/rest/v0/chapter_test.py (1)
backend/apps/api/rest/v0/chapter.py (1)
ChapterSchema(33-40)
backend/tests/apps/api/internal/nodes/api_key_test.py (1)
backend/apps/api/internal/nodes/api_key.py (1)
ApiKeyNode(19-20)
backend/tests/apps/nest/api/internal/nodes/api_keys_test.py (1)
backend/apps/api/internal/nodes/api_key.py (1)
ApiKeyNode(19-20)
backend/tests/apps/api/internal/queries/api_key_test.py (3)
backend/apps/api/internal/queries/api_key.py (3)
ApiKeyQueries(11-30)active_api_key_count(15-17)api_keys(20-30)backend/tests/apps/nest/api/internal/queries/api_keys_test.py (1)
mock_info(8-16)backend/apps/nest/models/user.py (1)
active_api_keys(46-51)
backend/tests/apps/api/rest/v0/repository_test.py (1)
backend/apps/api/rest/v0/repository.py (1)
RepositorySchema(18-24)
backend/tests/apps/api/rest/v0/label_test.py (1)
backend/apps/api/rest/v0/label.py (1)
LabelSchema(27-32)
backend/tests/apps/nest/api/internal/queries/api_keys_test.py (1)
backend/apps/api/internal/queries/api_key.py (1)
ApiKeyQueries(11-30)
backend/tests/apps/api/rest/v0/organization_test.py (1)
backend/apps/api/rest/v0/organization.py (1)
OrganizationSchema(28-36)
backend/apps/api/internal/mutations/__init__.py (1)
backend/apps/api/internal/mutations/api_key.py (1)
ApiKeyMutations(40-107)
backend/apps/nest/models/user.py (1)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/tests/apps/api/rest/v0/issue_test.py (1)
backend/apps/api/rest/v0/issue.py (1)
IssueSchema(28-36)
backend/tests/apps/api/rest/v0/member_test.py (1)
backend/apps/api/rest/v0/member.py (1)
MemberSchema(30-47)
backend/tests/apps/api/rest/v0/project_test.py (1)
backend/apps/api/rest/v0/project.py (1)
ProjectSchema(28-35)
backend/apps/nest/migrations/0006_delete_apikey.py (1)
backend/apps/api/migrations/0001_initial.py (1)
Migration(10-50)
backend/apps/nest/api/internal/mutations/__init__.py (1)
backend/apps/nest/api/internal/mutations/user.py (1)
UserMutations(38-114)
backend/tests/apps/api/rest/v0/release_test.py (1)
backend/apps/api/rest/v0/release.py (1)
ReleaseSchema(24-31)
backend/apps/api/internal/queries/api_key.py (1)
backend/apps/api/internal/nodes/api_key.py (1)
ApiKeyNode(19-20)
backend/tests/apps/api/models/api_key_test.py (1)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/apps/api/internal/mutations/api_key.py (2)
backend/apps/api/internal/nodes/api_key.py (1)
ApiKeyNode(19-20)backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/apps/api/rest/auth/api_key.py (1)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/apps/api/migrations/0001_initial.py (2)
backend/apps/nest/migrations/0006_delete_apikey.py (1)
Migration(6-15)backend/apps/nest/migrations/0002_apikey.py (1)
Migration(10-48)
backend/apps/api/admin/api_key.py (1)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)
backend/tests/apps/api/rest/auth/api_key_test.py (2)
backend/apps/api/models/api_key.py (1)
ApiKey(16-94)backend/apps/api/rest/auth/api_key.py (1)
ApiKeyHeader(11-33)
backend/settings/graphql.py (2)
backend/apps/api/internal/mutations/__init__.py (1)
ApiMutations(9-10)backend/apps/api/internal/queries/api_key.py (1)
ApiKeyQueries(11-30)
backend/apps/api/rest/v0/__init__.py (1)
backend/apps/api/rest/auth/api_key.py (1)
ApiKeyHeader(11-33)
⏰ 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 unit tests
- GitHub Check: Run backend tests
- GitHub Check: Run frontend e2e tests
🔇 Additional comments (28)
backend/apps/api/__init__.py (1)
1-1: LGTM.Docstring-only package init is fine.
backend/settings/base.py (1)
55-55: LGTM.Adding
apps.apito LOCAL_APPS aligns with the refactor.backend/apps/nest/models/user.py (1)
14-14: LGTM.TYPE_CHECKING import updated to the new
apps.apipath;active_api_keysstill matchesrelated_name="api_keys".backend/apps/nest/api/internal/mutations/__init__.py (1)
9-10: Decoupling Nest mutations from ApiKey looks right.Nice separation: NestMutations now focuses on user-only concerns.
Confirm the new ApiMutations root in apps.api is wired into the GraphQL schema (schema assembly/urls) and that any client-facing changes are documented/changelogged.
backend/tests/apps/nest/api/internal/queries/api_keys_test.py (1)
5-5: Import path update looks correct. No remaining references to the oldapps.nest.api.internal.queries.api_keyimport were found.backend/tests/apps/nest/api/internal/nodes/api_keys_test.py (1)
4-4: Import path aligned with new namespace.Good move to apps.api.internal.nodes.api_key.
backend/tests/apps/api/rest/v0/label_test.py (1)
3-3: LGTM on v0 import.Import path now points to apps.api.rest.v0.label as intended.
backend/tests/apps/api/rest/v0/event_test.py (1)
5-5: LGTM on v0 import.No functional changes to the test.
backend/tests/apps/api/rest/v0/member_test.py (1)
5-5: LGTM on v0 import.backend/tests/apps/api/rest/v0/issue_test.py (1)
5-5: LGTM on v0 import.backend/apps/api/internal/queries/api_key.py (2)
6-6: Import path for ApiKeyNode updated — OK.Consistent with the API move.
7-7: KeepIsAuthenticatedimport unchanged
Thepermissions.pyclass lives only underapps/nest/api/internal, so the existing import path is correct.backend/tests/apps/api/rest/v0/organization_test.py (1)
5-5: LGTM on v0 import.backend/tests/apps/api/rest/v0/committee_test.py (1)
5-5: Import path update looks good.
Aligns with the v0 move.backend/apps/api/internal/nodes/api_key.py (1)
6-6: LGTM on model import path change.
Matches the model relocation into apps.api.backend/tests/apps/api/rest/v0/project_test.py (1)
5-5: Import path update looks good.
Consistent with v0.backend/tests/apps/api/rest/v0/chapter_test.py (1)
5-5: Import path update looks good.
Matches the v0 namespace.backend/apps/api/admin/api_key.py (1)
1-6: Admin import/docstring updates look good.
Path now points to apps.api.models; docstring clarified.backend/apps/api/internal/queries/__init__.py (1)
3-3: Import path update looks good.
ApiKeyQueries now sourced from apps.api.backend/tests/apps/api/rest/v0/repository_test.py (1)
5-5: Import path update looks correct.
Matches the new v0 module location.backend/tests/apps/api/rest/v0/release_test.py (1)
5-5: Import path update looks correct.
Aligned with apps.api.rest.v0.release.backend/apps/api/internal/mutations/__init__.py (1)
8-10: GraphQL mutation root wiring LGTM.
Subclassing ApiKeyMutations under @strawberry.type is standard.backend/tests/apps/api/models/api_key_test.py (1)
9-9: Imports and patch targets updated correctly.
Targets match the relocated ApiKey model; decorator argument order is correct.Also applies to: 41-43, 59-59, 71-71
backend/apps/api/rest/auth/api_key.py (1)
8-8: Import path change is correct.
Now points to apps.api.models.api_key.backend/tests/apps/api/internal/queries/api_key_test.py (1)
1-48: Solid unit tests for resolvers.
Mocks are minimal and assertions target the correct queryset methods.backend/tests/apps/api/internal/nodes/api_key_test.py (1)
10-26: Good whitelist to prevent leaking sensitive fields.Explicit equality on the field set helps ensure fields like "hash" never slip into the schema.
backend/settings/graphql.py (1)
21-27: GraphQL root composition looks consistent with the refactor.ApiMutations/ApiKeyQueries are correctly wired; NestQuery removal is reflected. Keep an eye on name collisions across inherited roots.
Also applies to: 31-39
backend/tests/apps/api/rest/auth/api_key_test.py (1)
6-17: Auth tests updated cleanly to ApiKeyHeader.Patch targets and behaviors (success, invalid, missing) are correct and focused.
Also applies to: 22-51, 52-67
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (5)
proxy/staging.conf (1)
31-36: Optionally pass X-Forwarded-Host for correct absolute URL building behind proxies.Django may rely on X-Forwarded-Host when USE_X_FORWARDED_HOST=True. This is harmless even if unset.
Apply this diff to both backend proxy locations (and optionally the frontend one):
proxy_set_header Host $host; + proxy_set_header X-Forwarded-Host $host; proxy_set_header X-Forwarded-For $proxy_add_x_forwarded_for; proxy_set_header X-Forwarded-Proto $scheme; proxy_set_header X-Real-IP $remote_addr;backend/apps/api/rest/v0/__init__.py (4)
19-37: Normalize router prefixes (drop leading “/”) to avoid accidental double slashes.Ninja joins prefixes; leading slashes aren’t required and can produce // in some setups/logging.
- "/chapters": chapter_router, - "/committees": committee_router, - "/members": member_router, - "/organizations": organization_router, - "/events": event_router, - "/issues": issue_router, - "/projects": project_router, - "/releases": release_router, - "/repositories": repository_router, + "chapters": chapter_router, + "committees": committee_router, + "members": member_router, + "organizations": organization_router, + "events": event_router, + "issues": issue_router, + "projects": project_router, + "releases": release_router, + "repositories": repository_router,
44-45: Externalize throttle rate into settings.Hardcoding “10/s” makes tuning per-environment harder; read from settings with a sane default.
- "throttle": [AuthRateThrottle("10/s")], + "throttle": [AuthRateThrottle(getattr(settings, "API_THROTTLE_RATE", "10/s"))],
46-46: Single source of truth for API version.Consider moving the version to settings (e.g., API_VERSION) or package init to avoid drift.
Would you like a quick patch to read it from settings with a default?
87-87: Use dynamic version in the welcome message.Keeps the message in sync with api.version automatically.
- "message": "Welcome to the OWASP Nest API v0", + "message": f"Welcome to the OWASP Nest API v{api.version}",
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/apps/api/rest/v0/__init__.py(2 hunks)proxy/staging.conf(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/api/rest/v0/__init__.py (1)
backend/apps/api/rest/auth/api_key.py (1)
ApiKeyHeader(11-33)
⏰ 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 frontend unit tests
- GitHub Check: Run backend tests
🔇 Additional comments (2)
backend/apps/api/rest/v0/__init__.py (2)
41-41: Confirm docs/openapi auth posture when setting global ApiKeyHeader.With global auth, your /api/v0/openapi.json (and thus Swagger UI) may require the API key. If public docs are intended, explicitly exempt docs/openapi or override auth per-route.
Would you like me to propose a minimal pattern to keep routes authenticated while leaving docs/openapi public?
7-17: Router consolidation/imports look good.The relative import for chapter_router and unified ROUTERS map simplify wiring. No issues spotted.
|
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
backend/apps/api/rest/v0/__init__.py (1)
19-37: Docs cleanup echo: replace stale /api/v1 links with /api/v0.Reiterating earlier review: update any remaining doc references to match these prefixes (chapters, committees, members, organizations, events, issues, projects, releases, repositories, and labels if added).
🧹 Nitpick comments (2)
backend/apps/api/rest/v0/__init__.py (2)
7-15: Use a consistent import style (avoid mixing absolute and relative).Prefer absolute imports for all v0 modules for clarity and grep-ability.
Apply:
-from .chapter import router as chapter_router +from apps.api.rest.v0.chapter import router as chapter_routerAlso applies to: 17-17
41-41: Avoid auth error info leakage (generic 401s).Current ApiKeyHeader differentiates missing vs invalid keys. Prefer a single 401 message to reduce enumeration signals.
Change in backend/apps/api/rest/auth/api_key.py:
- if not key: - raise HttpError(HTTPStatus.UNAUTHORIZED, "Missing API key in 'X-API-Key' header") + if not key: + raise HttpError(HTTPStatus.UNAUTHORIZED, "Invalid or missing API key") @@ - raise HttpError(HTTPStatus.UNAUTHORIZED, "Invalid API key") + raise HttpError(HTTPStatus.UNAUTHORIZED, "Invalid or missing API key")
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/apps/api/rest/v0/__init__.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
backend/apps/api/rest/v0/__init__.py (1)
backend/apps/api/rest/auth/api_key.py (1)
ApiKeyHeader(11-33)
⏰ 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 backend tests
- GitHub Check: Run frontend unit tests
- GitHub Check: CodeQL (javascript-typescript)
🔇 Additional comments (3)
backend/apps/api/rest/v0/__init__.py (3)
1-1: LGTM: clear module docstring.
87-87: LGTM: root banner matches v0 path.
39-47: Harden Swagger docs persistence
- In backend/apps/api/rest/v0/init.py, change
docs: Swagger(settings={"persistAuthorization": True})
→
docs: Swagger(settings={"persistAuthorization": settings.IS_LOCAL_ENVIRONMENT})- Confirm any tests or code references expecting an API version are updated to
"version": "0.2.0".
* Refactor API to a separate app, add v0 * Add v0 migration files * Set v0 version
Proposed change
Refactor API.
Checklist
make check-testlocally; all checks and tests passed.