feat: personal access tokens (PATs) #1924
Conversation
WalkthroughThis update introduces comprehensive support for Personal Access Tokens (PATs) across backend and frontend components. It adds new models, serializers, API endpoints, and test coverage for PAT management, along with corresponding UI elements, localization strings, and form validation schemas. Supporting settings, migrations, and documentation are also updated. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Knox
participant DB
User->>Frontend: Request to create PAT
Frontend->>Backend: POST /iam/auth-tokens/ (name, expiry)
Backend->>Knox: Create AuthToken (with expiry)
Knox-->>Backend: AuthToken instance
Backend->>DB: Create PersonalAccessToken (links AuthToken)
DB-->>Backend: PAT record
Backend-->>Frontend: Return token string and metadata
Frontend-->>User: Display token (once)
sequenceDiagram
participant User
participant Frontend
participant Backend
participant DB
User->>Frontend: Request to delete PAT
Frontend->>Backend: DELETE /iam/auth-tokens/{digest}/
Backend->>DB: Verify PAT ownership and existence
DB-->>Backend: PAT record (if exists)
Backend->>DB: Delete PAT and linked AuthToken
DB-->>Backend: Success
Backend-->>Frontend: Success response
Frontend-->>User: Show confirmation
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (10)
✨ Finishing Touches
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
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (1)
backend/iam/migrations/0012_personalaccesstoken.py (1)
1-25:⚠️ Potential issuePipeline failing – run
ruff format(and lint-driven clean-ups) before mergingCI is red because Ruff’s formatter would rewrite this file.
Runningruff format backend/iam/migrations/0012_personalaccesstoken.pylocally (or adding it to
pre-commit) will fix whitespace/blank-line issues and unblock the build.🧰 Tools
🪛 GitHub Actions: Backend Linters
[error] 1-1: Ruff formatting check failed. File would be reformatted. Run 'ruff format' to fix code style issues.
🧹 Nitpick comments (14)
backend/iam/migrations/0012_personalaccesstoken.py (1)
19-22: Optionally add a uniqueness safeguard for token namesRight now multiple rows can share the same
namefor the same user because the uniqueness is only guaranteed by theauth_tokenFK.
If you want to avoid name clashes per user, you could add a DB/ORM-level constraint:+ migrations.AddConstraint( + model_name="personalaccesstoken", + constraint=models.UniqueConstraint( + fields=["auth_token", "name"], name="unique_pat_name_per_user" + ), + ),That would prevent accidental duplicates without extra app logic.
frontend/src/routes/(app)/(internal)/my-profile/settings/+page.svelte (2)
17-18: Consistent casing for the imported component
CreatePatModal(camel-case Pat) is imported fromCreatePATModal.svelte(upper-case PAT).
While Svelte is case-sensitive at the file-system level, the mixed spelling can be confusing for grep/refactoring.-import CreatePatModal from './pat/components/CreatePATModal.svelte'; +import CreatePATModal from './pat/components/CreatePATModal.svelte';…and update the two
refusages accordingly.
94-104: Rename deletion payload key fromid→digestfor clarityYou’re passing the token digest, not the DB id:
- _form: defaults({ id }, zod(z.object({ id: z.string() }))), - schema: zod(z.object({ id: z.string() })), + _form: defaults({ digest: id }, zod(z.object({ digest: z.string() }))), + schema: zod(z.object({ digest: z.string() })),Renaming keeps the semantic intent crystal-clear and prevents accidental overlap with other objects that do use numeric IDs.
backend/iam/views.py (1)
117-120: Use the token model helper for the quota check
request.user.auth_token_setassumes the default Knox model.
For swappable models keep it generic:-from request.user.auth_token_set.filter(expiry__gt=now) +from get_token_model().objects.filter(user=request.user, expiry__gt=now)This aligns with
create_token()and avoids subtle bugs whenKNOX_TOKEN_MODELis overridden.frontend/src/routes/(app)/(internal)/my-profile/settings/+page.server.ts (4)
50-52: Consider moving schema definition to a shared location.The deletion schema is defined inline in the load function and again in the deletePAT action. Consider moving this to the schemas.ts file alongside AuthTokenCreateSchema for better reusability.
// In $lib/utils/schemas.ts + export const AuthTokenDeleteSchema = z.object({ id: z.string() }); // Then in this file - const personalAccessTokenDeleteForm = await superValidate(zod(z.object({ id: z.string() }))); + const personalAccessTokenDeleteForm = await superValidate(zod(AuthTokenDeleteSchema));
173-173: Remove debug logging statement.This appears to be a development trace that should be removed before production.
- console.debug('deletePAT 1');
174-197: PAT deletion implementation is secure and follows existing patterns.The implementation properly handles form validation, API requests, and user feedback. Consider using the same pattern as the createPAT action for consistency in the return statement.
For consistency with the createPAT action, consider:
- return message(form, { status: response.status }); + console.debug('Deleted PAT', { status: response.status }); + setFlash({ type: 'success', message: m.successfullyDeletedPersonalAccessToken() }, event); + return message(form, { status: response.status });
177-177: Use shared schema for deletePAT validation.For consistency and maintainability, consider extracting this schema definition to your schemas.ts file as suggested earlier.
- const form = await superValidate(formData, zod(z.object({ id: z.string() }))); + const form = await superValidate(formData, zod(AuthTokenDeleteSchema));frontend/src/routes/(app)/(internal)/my-profile/settings/pat/components/CreatePATModal.svelte (6)
1-5: Improve typing for parent prop.The parent prop is typed as
any, which could lead to potential issues. Consider creating a proper interface for the expected parent properties.- export let parent: any; + interface ParentProps { + onConfirm: () => void; + buttonNeutral: string; + regionFooter: string; + } + export let parent: ParentProps;
49-49: Remove development console.log.This appears to be a development debugging statement that should be removed before production.
- $: console.log('toto', $page.form?.form?.message?.data?.token);
79-81: Update test ID to match PAT functionality.The test ID refers to TOTP (Time-based One-Time Password) instead of PAT (Personal Access Token).
- data-testid="activate-totp-confirm-button" + data-testid="generate-pat-button"
84-97: Token display with copy functionality is well implemented.Good use of a warning message to inform the user that the token will only be displayed once. The copy button integration is implemented correctly.
Consider adding visual feedback when copying succeeds:
<button type="button" class="btn px-2 py-1 {parent.buttonNeutral} rounded-l-none" - use:copy={{ text: $page?.form?.form?.message?.data?.token }} + use:copy={{ + text: $page?.form?.form?.message?.data?.token, + onCopied: () => { + // Add toast notification or brief visual feedback + } + }} ><i class="fa-solid fa-copy mr-2"></i>{m.copy()}</button >
90-90: Extract token path to a derived store for cleaner code.The nested path to access the token is repeated and could be simplified.
+ $: token = $page?.form?.form?.message?.data?.token || ''; - <pre>{$page?.form?.form?.message?.data?.token}</pre> + <pre>{token}</pre> // And update the copy directive: - use:copy={{ text: $page?.form?.form?.message?.data?.token }} + use:copy={{ text: token }}
98-106: Update test ID in confirmation button.Similar to the previous suggestion, update the test ID to match PAT functionality.
- data-testid="activate-totp-confirm-button" + data-testid="pat-done-button"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
backend/ciso_assistant/settings.py(1 hunks)backend/iam/migrations/0012_personalaccesstoken.py(1 hunks)backend/iam/models.py(2 hunks)backend/iam/serializers.py(2 hunks)backend/iam/urls.py(2 hunks)backend/iam/views.py(4 hunks)dispatcher/README.md(1 hunks)frontend/messages/en.json(1 hunks)frontend/src/lib/components/Modals/ConfirmModal.svelte(2 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)frontend/src/routes/(app)/(internal)/my-profile/settings/+page.server.ts(3 hunks)frontend/src/routes/(app)/(internal)/my-profile/settings/+page.svelte(3 hunks)frontend/src/routes/(app)/(internal)/my-profile/settings/pat/components/CreatePATModal.svelte(1 hunks)frontend/src/routes/(app)/(third-party)/evidences/[id=uuid]/+page.svelte(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
backend/iam/serializers.py (2)
backend/iam/models.py (3)
PersonalAccessToken(867-888)User(389-595)expiry(880-881)backend/core/serializer_fields.py (1)
FieldsRelatedField(24-86)
frontend/src/routes/(app)/(internal)/my-profile/settings/+page.server.ts (2)
frontend/src/lib/utils/constants.ts (1)
BASE_API_URL(3-7)frontend/src/lib/utils/schemas.ts (1)
AuthTokenCreateSchema(687-690)
backend/iam/urls.py (1)
backend/iam/views.py (2)
AuthTokenDetailViewSet(139-155)AuthTokenListViewSet(68-136)
🪛 GitHub Actions: Backend Linters
backend/iam/migrations/0012_personalaccesstoken.py
[error] 1-1: Ruff formatting check failed. File would be reformatted. Run 'ruff format' to fix code style issues.
🪛 Ruff (0.8.2)
backend/iam/serializers.py
2-2: django.utils.timezone imported but unused
Remove unused import: django.utils.timezone
(F401)
backend/iam/views.py
14-14: knox.views.user_logged_in imported but unused
Remove unused import: knox.views.user_logged_in
(F401)
15-15: rest_framework.viewsets imported but unused
Remove unused import: rest_framework.viewsets
(F401)
29-29: .serializers.PersonalAccessTokenCreateSerializer imported but unused
Remove unused import: .serializers.PersonalAccessTokenCreateSerializer
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
🔇 Additional comments (22)
backend/ciso_assistant/settings.py (2)
275-275: Token limit configuration added for PATs.Setting
TOKEN_LIMIT_PER_USERto 5 enforces the requirement mentioned in the PR objectives to limit personal access tokens to a maximum of five per user.
282-282: Knox token model explicitly defined.The new
KNOX_TOKEN_MODELsetting specifies the model to be used for token authentication, which is necessary for the new Personal Access Token functionality.dispatcher/README.md (1)
140-140: Documentation updated to reflect PAT feature availability.The "soon to be released" phrase has been removed from the token-based authentication section heading, correctly reflecting that the Personal Access Tokens feature is now implemented.
frontend/src/routes/(app)/(third-party)/evidences/[id=uuid]/+page.svelte (1)
36-36: Validation schema added to ConfirmModal.A schema property was added to the ConfirmModal component props, using the same Zod validation schema already used in
_form. This enhances form validation consistency.frontend/src/lib/components/Modals/ConfirmModal.svelte (2)
22-22: Schema prop added to ConfirmModal component.A new exportable property
schemahas been added to accept validation schemas from parent components.
53-59: SuperForm updated to use the schema for validation.The SuperForm component now includes a
validatorsattribute that uses the provided schema, enabling proper form validation.frontend/src/lib/utils/schemas.ts (1)
687-690: LGTM: Clean and concise schema for PAT creation.The schema correctly validates the required fields for creating a personal access token. The
namefield requires a non-empty string, and theexpiryfield is an optional positive number (presumably days until expiration).frontend/messages/en.json (1)
1474-1477: LGTM: Comprehensive localization strings for PAT management.The localization strings provide clear and informative messages for all PAT operations, including helpful warnings and instructions about token security and one-time display.
Also applies to: 1479-1485
backend/iam/urls.py (2)
5-7: LGTM: Clean imports for new view classes.The imports are correctly added for the new AuthToken view classes.
35-40: LGTM: Properly defined REST API endpoints for PAT management.The URL patterns follow RESTful conventions with appropriate naming and structure:
/auth-tokens/for listing and creating tokens/auth-tokens/<str:pk>/for operations on specific tokensThese endpoints align with the implementation in the view classes.
backend/iam/models.py (2)
39-39: LGTM: Better import practice for settings.Using
django.conf.settingsinstead of a direct import is a better practice for Django applications.
867-889: LGTM: Well-structured PersonalAccessToken model.The PersonalAccessToken model is well-designed:
- Links to the Knox token model with a ForeignKey
- Provides convenient property accessors for token metadata
- Includes a clear string representation
- Aligns with the PR objective of implementing personal access tokens
The model correctly delegates to the underlying auth_token for properties like creation time, expiry, and digest, avoiding data duplication.
frontend/src/routes/(app)/(internal)/my-profile/settings/+page.svelte (1)
203-210: Date parsing may break with custom backend format
new Date(pat.expiry)works only ifexpiryis ISO-8601 or RFC-2822.
The backend usesknox_settings.EXPIRY_DATETIME_FORMAT, which can be customised (e.g."%d/%m/%Y %H:%M"). JS’sDateconstructor would then returnInvalid Date.Two lightweight fixes:
- {m.expiresOn({ date: new Date(pat.expiry).toLocaleDateString() })} + {m.expiresOn({ date: dayjs(pat.expiry, 'YYYY-MM-DD HH:mm:ss').format('LL') })}(or) ask the backend to send ISO timestamps (
DateTimeField(format=None)), keeping the existing line intact.Choose whichever side is easier to change but make them consistent.
frontend/src/routes/(app)/(internal)/my-profile/settings/+page.server.ts (4)
1-11: Imports look good with appropriate organization.The new imports for PAT functionality are properly organized, with schema imports for form validation and necessary utilities from the superforms library.
43-50: PAT data fetching implementation is well structured.The implementation follows existing patterns in the codebase for API requests with proper error handling. Good job using standard fetch patterns consistent with the rest of the file.
53-62: Return object properly extended with PAT data.The return object has been cleanly extended to include the personal access tokens and related forms.
145-171: PAT creation implementation is thorough and follows best practices.The createPAT action properly validates form data, handles API responses, and provides appropriate user feedback. Good implementation.
frontend/src/routes/(app)/(internal)/my-profile/settings/pat/components/CreatePATModal.svelte (5)
6-22: Imports and store setup follow best practices.Good job organizing imports by category and using the appropriate stores and components for the functionality.
23-38: Form configuration is well implemented.The superForm setup with appropriate validation, data types, and event handling shows good attention to detail and follows best practices.
39-48: Base styles and imports are organized well.The component organization maintains a clear separation between imports, configuration, and styling.
52-66: Modal structure is well organized.The modal implementation with appropriate header and layout structure follows UI patterns effectively.
67-83: Form implementation for token creation looks good.The form fields are well structured with appropriate labels and help text. The validation integration works correctly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
backend/iam/views.py (2)
15-16: Remove unused imports to keep the codebase clean.-from knox.views import DateTimeField, LoginView as KnoxLoginView, user_logged_in -from rest_framework import permissions, serializers, status, views, viewsets +from knox.views import DateTimeField, LoginView as KnoxLoginView +from rest_framework import permissions, serializers, status, views🧰 Tools
🪛 Ruff (0.8.2)
15-15:
knox.views.user_logged_inimported but unusedRemove unused import:
knox.views.user_logged_in(F401)
16-16:
rest_framework.viewsetsimported but unusedRemove unused import:
rest_framework.viewsets(F401)
121-134:⚠️ Potential issueHarden input validation in token creation.
The current implementation doesn't properly validate the
nameandexpiryinputs, which could lead to unexpected behavior or server errors.def post(self, request, format=None): token_limit_per_user = self.get_token_limit_per_user() name = request.data.get("name") - expiry = request.data.get("expiry", "30") + + # Validate name is not empty + if not name or not name.strip(): + return Response( + {"error": "Name is required and cannot be empty."}, + status=status.HTTP_400_BAD_REQUEST, + ) + + # Validate expiry is a positive integer + try: + expiry_days = int(request.data.get("expiry", 30)) + if expiry_days <= 0: + raise ValueError("Expiry must be positive") + except (TypeError, ValueError): + return Response( + {"error": "Expiry must be a positive integer (days)."}, + status=status.HTTP_400_BAD_REQUEST, + ) + if token_limit_per_user is not None: now = timezone.now() token = request.user.auth_token_set.filter(expiry__gt=now) if token.count() >= token_limit_per_user: return Response( {"error": "Maximum amount of tokens allowed per user exceeded."}, status=status.HTTP_403_FORBIDDEN, ) - instance, token = self.create_token(timedelta(days=int(expiry))) + instance, token = self.create_token(timedelta(days=expiry_days)) pat = PersonalAccessToken.objects.create(auth_token=instance, name=name) return self.get_post_response(request, token, pat.name, pat.auth_token)
🧹 Nitpick comments (3)
backend/iam/views.py (3)
30-31: Remove unused import.The
PersonalAccessTokenCreateSerializeris imported but not used anywhere in this file.- PersonalAccessTokenCreateSerializer, PersonalAccessTokenReadSerializer,🧰 Tools
🪛 Ruff (0.8.2)
30-30:
.serializers.PersonalAccessTokenCreateSerializerimported but unusedRemove unused import:
.serializers.PersonalAccessTokenCreateSerializer(F401)
76-91: Consider implementing pagination for token listing.For users with many tokens, the
getmethod should implement pagination to improve performance and reduce response size. Additionally, consider adding sorting options.class AuthTokenListViewSet(views.APIView): def get_queryset(self): - return PersonalAccessToken.objects.filter(auth_token__user=self.request.user) + queryset = PersonalAccessToken.objects.filter(auth_token__user=self.request.user) + # Optional: Add ordering + return queryset.order_by('-auth_token__created')You might also consider using Django REST framework's pagination classes for the GET endpoint if you expect users to have many tokens in the future.
124-131: Consider caching token count to improve performance.The current implementation queries the database to count active tokens on every token creation request. For frequently used APIs, consider caching this value.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/iam/views.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/iam/views.py (2)
backend/iam/models.py (4)
PersonalAccessToken(867-888)digest(884-885)delete(440-442)expiry(880-881)backend/iam/serializers.py (3)
PersonalAccessTokenCreateSerializer(134-154)PersonalAccessTokenReadSerializer(122-131)create(139-150)
🪛 Ruff (0.8.2)
backend/iam/views.py
15-15: knox.views.user_logged_in imported but unused
Remove unused import: knox.views.user_logged_in
(F401)
16-16: rest_framework.viewsets imported but unused
Remove unused import: rest_framework.viewsets
(F401)
30-30: .serializers.PersonalAccessTokenCreateSerializer imported but unused
Remove unused import: .serializers.PersonalAccessTokenCreateSerializer
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/iam/views.py (1)
136-144: LGTM! Good implementation of the GET endpoint.The implementation correctly filters tokens by the authenticated user and uses the appropriate serializer.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
backend/app_tests/api/test_api_personal_access_tokens.py (3)
9-9: Remove the unusedTokenAuthenticationimport
TokenAuthenticationis never used in this module and Ruff (F401) is already flagging it. Eliminating dead imports keeps the test file tidy and prevents--fail-on-errorpipelines from breaking.-from knox.auth import TokenAuthentication🧰 Tools
🪛 Ruff (0.8.2)
9-9:
knox.auth.TokenAuthenticationimported but unusedRemove unused import:
knox.auth.TokenAuthentication(F401)
56-63: Hard-coding endpoint paths makes tests brittleEvery POST/GET in this suite uses the literal
"/api/iam/auth-tokens/".
If the router prefix changes (e.g., versioning/v2/), all these tests break simultaneously.DRF’s
reverse()(ordjango.urls.reverse) keeps the tests resilient to URL refactors:-from_path = "/api/iam/auth-tokens/" +from django.urls import reverse + +from_path = reverse("iam:personal-access-token-list")Consider introducing a fixture that returns this value so you can reuse it across all tests.
Same applies to the"/api/risk-assessments/"URL in the authentication tests.
234-244:expiryexpressed in “days” is implicit – document or validate
"expiry": 1is assumed to mean “1 day” in the API, yet nothing in the payload conveys units.
For clarity and future-proofing:
- Add an explicit comment or docstring in the test explaining that the backend interprets this as days, or
- Change the payload to use an ISO-8601 duration string or seconds field if the API supports it.
This prevents misinterpretation if the parameter semantics evolve.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app_tests/api/test_api_personal_access_tokens.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/app_tests/api/test_api_personal_access_tokens.py
9-9: knox.auth.TokenAuthentication imported but unused
Remove unused import: knox.auth.TokenAuthentication
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/app_tests/api/test_api_personal_access_tokens.py (1)
61-63: Creation should return HTTP 201, not 200A successful resource creation conventionally answers with
201 Created.
If the view is deliberately returning200, confirm this is intentional; otherwise, update either the view or the assertion:-assert response.status_code == status.HTTP_200_OK +assert response.status_code == status.HTTP_201_CREATED
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app_tests/api/test_api_personal_access_tokens.py (2)
9-9: Remove unused import.The
TokenAuthenticationclass is imported but not used anywhere in this test file.-from knox.auth import TokenAuthentication🧰 Tools
🪛 Ruff (0.8.2)
9-9:
knox.auth.TokenAuthenticationimported but unusedRemove unused import:
knox.auth.TokenAuthentication(F401)
100-103: Add explicit error message assertion for non-integer expiry test.The test for negative expiry (above) explicitly checks for a specific error message, but this test for non-integer expiry doesn't. For consistency and completeness, add an assertion for the error message content.
data["expiry"] = "not-a-number" response = authenticated_client.post(url, data) assert response.status_code == status.HTTP_400_BAD_REQUEST +assert "error" in response.data +assert "Expiry must be a positive integer" in response.data["error"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app_tests/api/test_api_personal_access_tokens.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
backend/app_tests/api/test_api_personal_access_tokens.py
9-9: knox.auth.TokenAuthentication imported but unused
Remove unused import: knox.auth.TokenAuthentication
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (3)
backend/app_tests/api/test_api_personal_access_tokens.py (3)
125-125: LGTM! User-scoped token count check is correct.This line correctly filters tokens by user, which prevents test failures if other fixtures or tests create tokens for different users.
207-225: LGTM! Comprehensive token authentication test.This test properly verifies that a personal access token can be used for authentication by:
- Creating a token
- Confirming access is denied without authentication
- Using the token to authenticate
- Verifying successful access to protected resources
The error messages in assertions also provide helpful debugging context if the test fails.
226-269: LGTM! Well-designed token expiration test.This test thoroughly validates the token expiration functionality by:
- Creating a token and verifying it works initially
- Manually setting the expiry time in the past
- Confirming the expired token is properly rejected
The assertion error messages include detailed information for easier debugging.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
backend/iam/views.py (2)
97-99: Consider making token limit configurable.The token limit is currently hardcoded to 5. For better flexibility, consider making this configurable through settings, similar to how Knox settings are used elsewhere in the code.
- def get_token_limit_per_user(self): - return 5 + def get_token_limit_per_user(self): + return getattr(settings, 'KNOX_TOKEN_LIMIT_PER_USER', 5)This would allow administrators to adjust the limit without code changes.
143-146: Consider internationalizing the error message limit.The error message "errorMaxPatAmountExceeded" appears to be a translation key, but it's not wrapped in the gettext function for translation, unlike other messages in the codebase.
- {"error": "errorMaxPatAmountExceeded"}, + {"error": _("errorMaxPatAmountExceeded")},
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
backend/ciso_assistant/settings.py(1 hunks)backend/iam/views.py(3 hunks)enterprise/backend/enterprise_core/settings.py(1 hunks)frontend/messages/en.json(2 hunks)frontend/messages/fr.json(1 hunks)frontend/src/lib/components/Toast/Toast.svelte(1 hunks)frontend/src/lib/utils/schemas.ts(1 hunks)frontend/src/routes/(app)/(internal)/my-profile/settings/+page.server.ts(3 hunks)frontend/src/routes/(app)/(internal)/my-profile/settings/pat/components/CreatePATModal.svelte(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- frontend/src/lib/components/Toast/Toast.svelte
- enterprise/backend/enterprise_core/settings.py
🚧 Files skipped from review as they are similar to previous changes (6)
- backend/ciso_assistant/settings.py
- frontend/messages/fr.json
- frontend/src/lib/utils/schemas.ts
- frontend/src/routes/(app)/(internal)/my-profile/settings/pat/components/CreatePATModal.svelte
- frontend/messages/en.json
- frontend/src/routes/(app)/(internal)/my-profile/settings/+page.server.ts
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/iam/views.py (3)
backend/iam/models.py (7)
Folder(58-229)PersonalAccessToken(907-928)Role(638-651)RoleAssignment(654-904)digest(924-925)delete(455-457)expiry(920-921)backend/iam/serializers.py (1)
PersonalAccessTokenReadSerializer(120-129)backend/app_tests/api/test_api_personal_access_tokens.py (2)
auth_token(33-35)user(17-18)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (1)
backend/iam/views.py (1)
62-79: Great implementation of token deletion in LogoutView.The implementation follows best practices by:
- Checking for valid authorization header format
- Properly handling token extraction and deletion
- Adding detailed logging for debugging
This addresses the previous suggestion to improve error handling in the logout process.
| def post(self, request, format=None): | ||
| token_limit_per_user = self.get_token_limit_per_user() | ||
| name = request.data.get("name") | ||
| try: | ||
| expiry_days = int(request.data.get("expiry", 30)) | ||
| if expiry_days <= 0: | ||
| raise ValueError | ||
| except (TypeError, ValueError): | ||
| return Response( | ||
| {"error": "Expiry must be a positive integer (days)."}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| if token_limit_per_user is not None: | ||
| now = timezone.now() | ||
| token = request.user.auth_token_set.filter(expiry__gt=now).filter( | ||
| personalaccesstoken__isnull=False | ||
| ) | ||
| if token.count() >= token_limit_per_user: | ||
| return Response( | ||
| {"error": "errorMaxPatAmountExceeded"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| instance, token = self.create_token(timedelta(days=int(expiry_days))) | ||
| pat = PersonalAccessToken.objects.create(auth_token=instance, name=name) | ||
| return self.get_post_response(request, token, pat.name, pat.auth_token) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Input validation could be strengthened for token creation.
The post method validates expiry_days thoroughly but doesn't validate that name is provided and non-empty. This could lead to anonymous entries in the token list.
Also, consider adding validation for maximum name length to prevent very long token names.
🤖 Prompt for AI Agents
In backend/iam/views.py around lines 125 to 149, the post method lacks
validation for the 'name' field, allowing empty or missing names which can cause
anonymous token entries. Add a check to ensure 'name' is provided and not empty,
returning a 400 error if invalid. Also, enforce a maximum length constraint on
'name' to prevent excessively long token names, returning an appropriate error
response if exceeded.
| class AuthTokenDetailView(views.APIView): | ||
| def delete(self, request, *args, **kwargs): | ||
| try: | ||
| token = AuthToken.objects.get(digest=kwargs["pk"]) | ||
| if token.user != request.user: | ||
| return Response( | ||
| {"error": "You do not have permission to delete this token."}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| token.delete() | ||
| return Response(status=status.HTTP_204_NO_CONTENT) | ||
| except AuthToken.DoesNotExist: | ||
| logger.info( | ||
| "Attempt to delete non-existent token", | ||
| digest=kwargs["pk"], | ||
| user=request.user.id, | ||
| ) | ||
| return Response( | ||
| {"error": "Token not found or already deleted."}, | ||
| status=status.HTTP_404_NOT_FOUND, | ||
| ) | ||
| except Exception as e: | ||
| logger.error( | ||
| "Error deleting token", | ||
| error=str(e), | ||
| digest=kwargs["pk"], | ||
| user=request.user.id, | ||
| ) | ||
| return Response( | ||
| {"error": "Failed to delete token due to an internal error."}, | ||
| status=status.HTTP_500_INTERNAL_SERVER_ERROR, | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Excellent error handling in AuthTokenDetailView.
The implementation follows best practices by:
- Using specific exception handlers for different error cases
- Providing clear error messages
- Including detailed logging with contextual information
- Returning appropriate HTTP status codes
However, there's a potential issue: the view doesn't verify if the token is a personal access token. It operates directly on the AuthToken model, which could allow deletion of session tokens.
🏁 Script executed:
#!/bin/bash
# Check if the view filters tokens to only delete PersonalAccessTokens
# Look for PersonalAccessToken references in the deletion logic
rg -A 5 "AuthToken.objects.get\(digest=kwargs\[\"pk\"\]\)" backend/iam/views.py
# Check if there's filtering on personalaccesstoken relation
rg "personalaccesstoken" backend/iam/views.pyLength of output: 469
Ensure deletion is scoped to personal access tokens only
The current delete method in backend/iam/views.py (lines 162–193) fetches any AuthToken by digest and allows the owner to delete it—even session tokens. To prevent accidental removal of non-personal tokens, scope the lookup to only tokens with an associated PersonalAccessToken.
• File: backend/iam/views.py, AuthTokenDetailView.delete
• Replace:
token = AuthToken.objects.get(digest=kwargs["pk"])with something like:
token = AuthToken.objects.filter(
personalaccesstoken__isnull=False,
digest=kwargs["pk"]
).first()
if not token:
return Response(
{"error": "Personal access token not found."},
status=status.HTTP_404_NOT_FOUND,
)This ensures only personal access tokens can be deleted.
🤖 Prompt for AI Agents
In backend/iam/views.py within AuthTokenDetailView.delete method (lines
162-193), the code fetches any AuthToken by digest, allowing deletion of session
tokens. Modify the token lookup to filter only tokens linked to a
PersonalAccessToken by using a filter with personalaccesstoken__isnull=False and
digest=kwargs["pk"], then check if the token exists before proceeding. If no
token is found, return a 404 response indicating the personal access token was
not found. This change restricts deletion to personal access tokens only.
| class PersonalAccessTokenViewSet(views.APIView): | ||
| def get_queryset(self): | ||
| return PersonalAccessToken.objects.filter(auth_token__user=self.request.user) | ||
|
|
||
| def get_context(self): | ||
| return {"request": self.request, "format": self.format_kwarg, "view": self} | ||
|
|
||
| def get_token_prefix(self): | ||
| return knox_settings.TOKEN_PREFIX | ||
|
|
||
| def get_token_limit_per_user(self): | ||
| return 5 | ||
|
|
||
| def get_expiry_datetime_format(self): | ||
| return knox_settings.EXPIRY_DATETIME_FORMAT | ||
|
|
||
| def format_expiry_datetime(self, expiry): | ||
| datetime_format = self.get_expiry_datetime_format() | ||
| return DateTimeField(format=datetime_format).to_representation(expiry) | ||
|
|
||
| def create_token(self, expiry): | ||
| token_prefix = self.get_token_prefix() | ||
| return get_token_model().objects.create( | ||
| user=self.request.user, expiry=expiry, prefix=token_prefix | ||
| ) | ||
|
|
||
| def get_post_response_data(self, request, token, name, instance): | ||
| data = { | ||
| "name": name, | ||
| "expiry": self.format_expiry_datetime(instance.expiry), | ||
| "token": token, | ||
| } | ||
| return data | ||
|
|
||
| def get_post_response(self, request, token, name, instance): | ||
| data = self.get_post_response_data(request, token, name, instance) | ||
| return Response(data) | ||
|
|
||
| def post(self, request, format=None): | ||
| token_limit_per_user = self.get_token_limit_per_user() | ||
| name = request.data.get("name") | ||
| try: | ||
| expiry_days = int(request.data.get("expiry", 30)) | ||
| if expiry_days <= 0: | ||
| raise ValueError | ||
| except (TypeError, ValueError): | ||
| return Response( | ||
| {"error": "Expiry must be a positive integer (days)."}, | ||
| status=status.HTTP_400_BAD_REQUEST, | ||
| ) | ||
| if token_limit_per_user is not None: | ||
| now = timezone.now() | ||
| token = request.user.auth_token_set.filter(expiry__gt=now).filter( | ||
| personalaccesstoken__isnull=False | ||
| ) | ||
| if token.count() >= token_limit_per_user: | ||
| return Response( | ||
| {"error": "errorMaxPatAmountExceeded"}, | ||
| status=status.HTTP_403_FORBIDDEN, | ||
| ) | ||
| instance, token = self.create_token(timedelta(days=int(expiry_days))) | ||
| pat = PersonalAccessToken.objects.create(auth_token=instance, name=name) | ||
| return self.get_post_response(request, token, pat.name, pat.auth_token) | ||
|
|
||
| def get(self, request, *args, **kwargs): | ||
| """ | ||
| Get all personal access tokens for the user. | ||
| """ | ||
| queryset = self.get_queryset() | ||
| serializer = PersonalAccessTokenReadSerializer( | ||
| queryset, many=True, context=self.get_context() | ||
| ) | ||
| return Response(serializer.data) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Well-structured implementation of PersonalAccessTokenViewSet.
The implementation provides a robust API for creating and listing personal access tokens with proper validation for expiry and enforcing token limits.
However, there's a potential issue with the name parameter validation:
- name = request.data.get("name")
+ name = request.data.get("name")
+ if not name:
+ return Response(
+ {"error": "Name is required for personal access tokens."},
+ status=status.HTTP_400_BAD_REQUEST,
+ )This would prevent anonymous token creation and align with input validation practices applied to the expiry parameter.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| class PersonalAccessTokenViewSet(views.APIView): | |
| def get_queryset(self): | |
| return PersonalAccessToken.objects.filter(auth_token__user=self.request.user) | |
| def get_context(self): | |
| return {"request": self.request, "format": self.format_kwarg, "view": self} | |
| def get_token_prefix(self): | |
| return knox_settings.TOKEN_PREFIX | |
| def get_token_limit_per_user(self): | |
| return 5 | |
| def get_expiry_datetime_format(self): | |
| return knox_settings.EXPIRY_DATETIME_FORMAT | |
| def format_expiry_datetime(self, expiry): | |
| datetime_format = self.get_expiry_datetime_format() | |
| return DateTimeField(format=datetime_format).to_representation(expiry) | |
| def create_token(self, expiry): | |
| token_prefix = self.get_token_prefix() | |
| return get_token_model().objects.create( | |
| user=self.request.user, expiry=expiry, prefix=token_prefix | |
| ) | |
| def get_post_response_data(self, request, token, name, instance): | |
| data = { | |
| "name": name, | |
| "expiry": self.format_expiry_datetime(instance.expiry), | |
| "token": token, | |
| } | |
| return data | |
| def get_post_response(self, request, token, name, instance): | |
| data = self.get_post_response_data(request, token, name, instance) | |
| return Response(data) | |
| def post(self, request, format=None): | |
| token_limit_per_user = self.get_token_limit_per_user() | |
| name = request.data.get("name") | |
| try: | |
| expiry_days = int(request.data.get("expiry", 30)) | |
| if expiry_days <= 0: | |
| raise ValueError | |
| except (TypeError, ValueError): | |
| return Response( | |
| {"error": "Expiry must be a positive integer (days)."}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| if token_limit_per_user is not None: | |
| now = timezone.now() | |
| token = request.user.auth_token_set.filter(expiry__gt=now).filter( | |
| personalaccesstoken__isnull=False | |
| ) | |
| if token.count() >= token_limit_per_user: | |
| return Response( | |
| {"error": "errorMaxPatAmountExceeded"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| instance, token = self.create_token(timedelta(days=int(expiry_days))) | |
| pat = PersonalAccessToken.objects.create(auth_token=instance, name=name) | |
| return self.get_post_response(request, token, pat.name, pat.auth_token) | |
| def get(self, request, *args, **kwargs): | |
| """ | |
| Get all personal access tokens for the user. | |
| """ | |
| queryset = self.get_queryset() | |
| serializer = PersonalAccessTokenReadSerializer( | |
| queryset, many=True, context=self.get_context() | |
| ) | |
| return Response(serializer.data) | |
| def post(self, request, format=None): | |
| token_limit_per_user = self.get_token_limit_per_user() | |
| - name = request.data.get("name") | |
| + name = request.data.get("name") | |
| + if not name: | |
| + return Response( | |
| + {"error": "Name is required for personal access tokens."}, | |
| + status=status.HTTP_400_BAD_REQUEST, | |
| + ) | |
| try: | |
| expiry_days = int(request.data.get("expiry", 30)) | |
| if expiry_days <= 0: | |
| raise ValueError | |
| except (TypeError, ValueError): | |
| return Response( | |
| {"error": "Expiry must be a positive integer (days)."}, | |
| status=status.HTTP_400_BAD_REQUEST, | |
| ) | |
| if token_limit_per_user is not None: | |
| now = timezone.now() | |
| token = request.user.auth_token_set.filter(expiry__gt=now).filter( | |
| personalaccesstoken__isnull=False | |
| ) | |
| if token.count() >= token_limit_per_user: | |
| return Response( | |
| {"error": "errorMaxPatAmountExceeded"}, | |
| status=status.HTTP_403_FORBIDDEN, | |
| ) | |
| instance, token = self.create_token(timedelta(days=int(expiry_days))) | |
| pat = PersonalAccessToken.objects.create(auth_token=instance, name=name) | |
| return self.get_post_response(request, token, pat.name, pat.auth_token) |
🤖 Prompt for AI Agents
In backend/iam/views.py around lines 87 to 160, the 'name' parameter from the
request data is used without validation, which could lead to issues if it is
missing or invalid. Add validation to ensure 'name' is provided and is a
non-empty string before proceeding with token creation. If 'name' is invalid,
return a 400 Bad Request response with an appropriate error message, similar to
the expiry parameter validation.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
backend/app_tests/api/test_api_personal_access_tokens.py (2)
8-9: Remove unused importsThese imports are not being used in the test file:
-from knox.settings import knox_settings -from knox.auth import TokenAuthentication🧰 Tools
🪛 Ruff (0.11.9)
8-8:
knox.settings.knox_settingsimported but unusedRemove unused import:
knox.settings.knox_settings(F401)
9-9:
knox.auth.TokenAuthenticationimported but unusedRemove unused import:
knox.auth.TokenAuthentication(F401)
233-233: Consider parameterizing expiry testsYou've written good tests for token expiry, but consider using pytest's parameterize feature to test multiple expiry values in a single test to make the test suite more maintainable.
@pytest.mark.parametrize("expiry_days,expected_status", [ (1, status.HTTP_200_OK), (30, status.HTTP_200_OK), (365, status.HTTP_200_OK), (-5, status.HTTP_400_BAD_REQUEST), ("not-a-number", status.HTTP_400_BAD_REQUEST), ]) def test_create_token_with_various_expiry_values(self, authenticated_client, expiry_days, expected_status): url = "/api/iam/auth-tokens/" data = {"name": f"Token with expiry {expiry_days}", "expiry": expiry_days} response = authenticated_client.post(url, data) assert response.status_code == expected_status
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app_tests/api/test_api_personal_access_tokens.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app_tests/api/test_api_personal_access_tokens.py
8-8: knox.settings.knox_settings imported but unused
Remove unused import: knox.settings.knox_settings
(F401)
9-9: knox.auth.TokenAuthentication imported but unused
Remove unused import: knox.auth.TokenAuthentication
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: Analyze (python)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (2)
backend/app_tests/api/test_api_personal_access_tokens.py (2)
52-264: Test suite looks comprehensive - nice work!The test coverage for Personal Access Tokens is thorough, covering token creation, retrieval, authentication, expiration, and authorization. You've effectively tested all the key user scenarios and edge cases.
120-120: Good fix for user-scoped token assertionI see you've properly implemented the token-limit assertion to be scoped to the specific user. This is the correct approach to avoid test failures when other users have tokens in the database.
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
backend/app_tests/api/test_api_personal_access_tokens.py (1)
52-265: 🛠️ Refactor suggestionMissing test for token deletion functionality.
The test suite covers creation, retrieval, authentication, and expiry scenarios for Personal Access Tokens, but it's missing tests for the token deletion functionality mentioned in the PR objectives.
Consider adding a test case to verify that:
- Users can delete their own tokens
- The deleted tokens can no longer be used for authentication
- Users cannot delete tokens belonging to other users
Example implementation:
@pytest.mark.django_db def test_delete_token(self, authenticated_client, user): """Test deleting a personal access token.""" # Create a token first url = "/api/iam/auth-tokens/" response = authenticated_client.post(url, {"name": "Delete Test Token", "expiry": 30}) assert response.status_code == status.HTTP_200_OK token_value = response.data["token"] # Get the token ID get_response = authenticated_client.get(url) assert get_response.status_code == status.HTTP_200_OK token_id = get_response.data[0]["id"] # Delete the token delete_url = f"{url}{token_id}/" delete_response = authenticated_client.delete(delete_url) assert delete_response.status_code == status.HTTP_204_NO_CONTENT # Verify token is deleted get_response_after = authenticated_client.get(url) assert len(get_response_after.data) == 0 # Verify token can't be used for authentication test_client = APIClient() test_client.credentials(HTTP_AUTHORIZATION=f"Token {token_value}") protected_url = "/api/risk-assessments/" auth_response = test_client.get(protected_url) assert auth_response.status_code == status.HTTP_401_UNAUTHORIZED
🧹 Nitpick comments (2)
backend/app_tests/api/test_api_personal_access_tokens.py (2)
8-9: Remove unused imports to improve code cleanliness.The following imports are not used anywhere in the test file:
knox_settingsfromknox.settingsTokenAuthenticationfromknox.auth-from knox.settings import knox_settings -from knox.auth import TokenAuthentication🧰 Tools
🪛 Ruff (0.11.9)
8-8:
knox.settings.knox_settingsimported but unusedRemove unused import:
knox.settings.knox_settings(F401)
9-9:
knox.auth.TokenAuthenticationimported but unusedRemove unused import:
knox.auth.TokenAuthentication(F401)
100-102: Incomplete assertion for non-integer expiry validation.The test checks that non-integer expiry values are rejected with a 400 status code, but doesn't verify the specific error message like it does for negative values.
Add an assertion to verify the error message for non-integer expiry:
data["expiry"] = "not-a-number" response = authenticated_client.post(url, data) assert response.status_code == status.HTTP_400_BAD_REQUEST +assert "error" in response.data +assert "Expiry must be a valid integer" in response.data["error"]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
backend/app_tests/api/test_api_personal_access_tokens.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.11.9)
backend/app_tests/api/test_api_personal_access_tokens.py
8-8: knox.settings.knox_settings imported but unused
Remove unused import: knox.settings.knox_settings
(F401)
9-9: knox.auth.TokenAuthentication imported but unused
Remove unused import: knox.auth.TokenAuthentication
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: build (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (4)
backend/app_tests/api/test_api_personal_access_tokens.py (4)
104-121: Token limit assertion is correctly user-scoped.Great improvement! The assertion now correctly filters by user to properly test the per-user token limit constraint, which aligns with the PR objective of limiting tokens to 5 per user.
222-265: Comprehensive test for expired token authentication.This test thoroughly verifies that expired tokens are properly rejected for authentication, which is crucial for security. The test includes:
- Creating a token with a short expiry
- Verifying it works initially
- Manually expiring the token
- Verifying it no longer works
The detailed assertions and error messages make debugging failures easier.
95-97: Good validation for negative expiry values.The test properly verifies that the API rejects negative expiry values with an appropriate error message.
145-180: Well-structured test for user isolation.This test effectively ensures that the PAT implementation maintains proper user isolation, which is a critical security requirement. The test:
- Creates two distinct users
- Creates a token for each user
- Verifies each user can only see their own tokens
This validates that the API properly scopes token access to the authenticated user.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
backend/core/views.py(3 hunks)backend/fixtures/__init__.py(1 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
backend/core/views.py (1)
backend/fixtures/__init__.py (1)
get_resource_path(7-17)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: startup-functional-test (3.12)
- GitHub Check: startup-docker-compose-test
- GitHub Check: enterprise-startup-functional-test (3.12)
- GitHub Check: functional-tests (3.12, chromium)
- GitHub Check: test (3.12)
- GitHub Check: build (3.12)
- GitHub Check: enterprise-functional-tests (3.12, chromium)
- GitHub Check: enterprise-startup-docker-compose-test
- GitHub Check: Analyze (python)
- GitHub Check: Analyze (javascript-typescript)
🔇 Additional comments (5)
backend/fixtures/__init__.py (2)
1-2: LGTM: Appropriate imports for resource handling.The imports are correct for modern Python resource handling using
importlib.resources, which is the recommended approach for accessing package resources in a cross-platform manner.
4-4: LGTM: Proper package resource initialization.The package resource reference is correctly initialized to point to the fixtures package using the modern
importlib.resources.files()API.backend/core/views.py (3)
34-34: LGTM: Clean import additionThe import of
get_resource_pathfrom the fixtures module is correctly placed and properly used in theimport_dummy_domainmethod below.
135-135: LGTM: Modern resource handling importThe
importlib.resourcesimport is the correct choice for modern package resource access and is properly utilized in the refactored code.
2698-2745: Excellent refactoring with improved resource handling and error managementThis refactoring significantly improves the
import_dummy_domainmethod with several key enhancements:Improvements:
- Modern Resource Access: Uses
importlib.resources.as_file()context manager for safe resource handling- Robust Error Handling: Comprehensive exception handling for different failure scenarios
- Resource Validation: Proper checking with
is_file()before processing- Enhanced Logging: Detailed logging for both success and error cases
- Clean Resource Management: Context manager ensures proper resource cleanup
Technical Benefits:
- Follows modern Python best practices for package resource access
- Eliminates potential resource leaks through proper context management
- Provides clear error messages for different failure modes
- Maintains backward compatibility while improving reliability
The implementation correctly handles the transition from a
Traversableresource path to a concrete filesystem path, which is necessary for the existing file processing logic.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests