Conversation
Owner
Author
|
@coderabbitai check this PR |
|
🧠 Learnings used✅ Actions performedReview triggered.
|
📝 WalkthroughWalkthroughAdds token-based API authentication: a new Changes
Sequence Diagram(s)sequenceDiagram
actor Client
participant API as API Request Handler
participant AuthBackend as TokenAuthentication
participant DB as Auth Model (Database)
Client->>API: Request with Authorization: Token <token>
API->>AuthBackend: authenticate(request)
AuthBackend->>AuthBackend: parse header -> keyword + token
AuthBackend->>DB: query Auth.objects.select_related("user").get(token=...)
alt found
DB-->>AuthBackend: Auth instance (with user)
AuthBackend-->>API: return (user, auth)
API-->>Client: proceed -> 200 OK
else not found / bad header
AuthBackend-->>API: raise AuthenticationFailed
API-->>Client: 401 Unauthorized
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 (1)
odin/settings/base.py (1)
220-225:⚠️ Potential issue | 🟠 MajorAuthentication enforcement is implicit and incompatible with public views.
The current config unintentionally blocks all API access via exception in
TokenAuthentication.authenticate(). Since DRF runs authentication before permission checks, theAllowAnypermission is never evaluated—requests without valid tokens fail at the authentication stage instead. This breaksApplicationServerKeyViewandDeviceView, which should be publicly accessible.To fix this cleanly, either:
- Switch
DEFAULT_PERMISSION_CLASSEStoIsAuthenticatedand explicitly override public views withpermission_classes = [AllowAny], or- Change authentication to not raise on missing tokens (return
Noneinstead) and use permission classes to enforce authentication where needed.Option 1 is clearer and recommended: make authentication policy explicit in permission classes rather than hidden in exception handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@odin/settings/base.py` around lines 220 - 225, The REST_FRAMEWORK config currently causes TokenAuthentication.authenticate() to raise and block public endpoints; change the policy to make authentication a permission concern by setting DEFAULT_PERMISSION_CLASSES to IsAuthenticated and keep DEFAULT_AUTHENTICATION_CLASSES as-is, then explicitly mark public views (e.g., ApplicationServerKeyView and DeviceView) with permission_classes = [rest_framework.permissions.AllowAny]; update any references to TokenAuthentication.authenticate() only if it still raises on missing tokens to ensure authenticated-only access is controlled via IsAuthenticated permission classes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@odin/api/authentication.py`:
- Around line 10-19: The authenticate method currently raises
AuthenticationFailed when the Authorization header is missing and compares the
auth scheme case-sensitively; change authenticate(self, request) to return None
if request.headers.get("Authorization") is falsy (so DRF treats it as "no
credentials provided") and perform the scheme check using a case-insensitive
comparison (e.g., compare parts[0].lower() to self.keyword.lower()), only
raising AuthenticationFailed when the header is present but malformed or the
token is invalid; also add appropriate type hints to the method signature and
any local variables per project guidelines (e.g., typing.Optional and
rest_framework.request.Request) while keeping references to authenticate,
self.keyword, and AuthenticationFailed.
- Line 28: The Auth instance being returned as request.user from your
authentication backend (the return of (auth_instance, auth_instance)) lacks the
is_authenticated attribute DRF expects; update the Auth model/class to define
is_authenticated = True (or implement an is_authenticated property/method) so
permission checks like IsAuthenticated work, or alternatively change the
authentication backend to return a Django/DRF-compatible user-like principal
(e.g., a wrapper object implementing is_authenticated) instead of raw Auth.
In `@odin/apps/core/models.py`:
- Around line 88-97: Replace storing the raw API token in the Auth model with a
one-way digest and add an updated_at timestamp: change the token CharField to
store a token_digest (or rename token to token_digest) and keep it unique, add
an updated_at = models.DateTimeField(auto_now=True), and update any usage of
Auth.token to compute and compare the digest instead of comparing cleartext
(e.g., in your authenticator function/method where tokens are validated, compute
the same secure hash/salted HMAC of the presented token and compare to
Auth.token_digest). Ensure you pick a strong, salted hash scheme (e.g.,
HMAC-SHA256 with per-token salt or Django's salted password hashing utilities)
and update migrations accordingly so existing code references (Auth model and
any authenticator function/class) use the new digest field.
In `@odin/tests/conftest.py`:
- Around line 27-46: The autouse fixture setup_api_client mutates
request.cls.setup_method without restoring it and forces a db dependency for all
tests; either remove autouse and require tests to use the api_client fixture
explicitly, or if keeping this wrapper change setup_api_client to not depend on
db, make the wrapper accept *args, **kwargs and forward them (def
new_setup(self, *args, **kwargs): original_setup(self, *args, **kwargs); if
hasattr(self, "client") and isinstance(self.client, APIClient): token =
AuthFactory().token; self.client.credentials(HTTP_AUTHORIZATION=f"Token
{token}")), and convert the fixture to a yield fixture that saves
original_setup, assigns new_setup, yields, then restores
request.cls.setup_method to original_setup to avoid stacking wrappers and
leaking state.
---
Outside diff comments:
In `@odin/settings/base.py`:
- Around line 220-225: The REST_FRAMEWORK config currently causes
TokenAuthentication.authenticate() to raise and block public endpoints; change
the policy to make authentication a permission concern by setting
DEFAULT_PERMISSION_CLASSES to IsAuthenticated and keep
DEFAULT_AUTHENTICATION_CLASSES as-is, then explicitly mark public views (e.g.,
ApplicationServerKeyView and DeviceView) with permission_classes =
[rest_framework.permissions.AllowAny]; update any references to
TokenAuthentication.authenticate() only if it still raises on missing tokens to
ensure authenticated-only access is controlled via IsAuthenticated permission
classes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a6b97a33-946a-4cac-93f6-ea884f0cca6a
📒 Files selected for processing (7)
odin/api/authentication.pyodin/apps/core/migrations/0005_auth.pyodin/apps/core/models.pyodin/settings/base.pyodin/tests/api/test_auth.pyodin/tests/conftest.pyodin/tests/factories.py
…ntication-for-odin-project-api
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
odin/api/authentication.py (1)
12-15:⚠️ Potential issue | 🔴 CriticalReturn
Nonewhen no auth header is present instead of raising.Per the DRF authentication contract,
authenticate()should returnNonewhen credentials are not provided, allowing DRF to try other authenticators or fall back toAnonymousUser. RaisingAuthenticationFailedhere causes 401 responses even forAllowAnyendpoints.Also update the return type annotation to reflect that
Noneis a valid return value.🐛 Proposed fix
- def authenticate(self, request: Request) -> tuple[User, Auth]: + def authenticate(self, request: Request) -> tuple[User, Auth] | None: auth_header = request.headers.get("Authorization") if not auth_header: - raise AuthenticationFailed("Token not provided") + return None🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@odin/api/authentication.py` around lines 12 - 15, The authenticate function currently raises AuthenticationFailed when the Authorization header is missing; change it to return None instead so DRF can try other authenticators or allow anonymous access, and update the function signature/return type annotation from tuple[User, Auth] to Optional[tuple[User, Auth]] (or tuple[User, Auth] | None) to reflect that None is a valid return value; update any related usages or imports if necessary to satisfy typing.
🧹 Nitpick comments (1)
odin/tests/conftest.py (1)
27-37: Autouse fixture may mask unauthenticated endpoint testing.This fixture auto-injects credentials into all API tests except
test_auth.py. However, endpoints like healthcheck and push (app-server-key) should likely work without authentication. Auto-injecting tokens prevents verifying that these public endpoints remain accessible to unauthenticated clients.Consider an opt-in pattern instead (explicit
@pytest.mark.authenticatedmarker or direct use ofapi_clientfixture) to ensure each test explicitly declares its authentication requirements.♻️ Example: marker-based opt-in approach
-@pytest.fixture(autouse=True) -def setup_api_client(db, request): - """Auto-authenticates all API tests with a valid token. - - Skips test_auth.py since it specifically tests authentication failures. - Use the api_client fixture directly if you need an authenticated client - without this auto-injection. - """ - if "test_auth.py" in str(request.fspath): - yield - return +@pytest.fixture(autouse=True) +def setup_api_client(db, request): + """Injects auth token for tests marked with `@pytest.mark.authenticated`.""" + if not request.node.get_closest_marker("authenticated"): + yield + return if hasattr(request.cls, "setup_method"): # ... rest unchangedThen decorate test classes that need auth:
`@pytest.mark.authenticated` `@pytest.mark.django_db` class TestSensorsAPI: ...🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@odin/tests/conftest.py` around lines 27 - 37, The autouse fixture setup_api_client is masking unauthenticated endpoint tests; change it from autouse=True to an explicit opt-in pattern: remove autouse and have setup_api_client check for a pytest marker (e.g., request.node.get_closest_marker("authenticated")) or require tests to use the api_client fixture directly, and only inject auth token when that marker is present; update tests that rely on implicit auth to add `@pytest.mark.authenticated` or to use api_client, and ensure test_auth.py and public endpoints (healthcheck/push) remain unauthenticated by default.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@odin/api/authentication.py`:
- Around line 25-28: The current auth lookup returns (auth.user, auth) without
guarding for auth.user being None
(Auth.objects.filter(...).select_related("user").first()), so update the
authentication flow to explicitly handle a missing user: after obtaining auth,
check if auth.user is None and either raise AuthenticationFailed("Token not
associated with a user") to require a user association, or alternatively return
the Auth instance as the principal (e.g., return (auth, auth)) if you want the
token to represent the Auth record itself; implement one of these two fixes in
the block that currently returns (auth.user, auth).
In `@odin/apps/core/admin.py`:
- Around line 38-43: The admin currently exposes full tokens via
AuthAdmin.list_display ("token"), which is insecure; replace that by either
removing "token" from list_display or add a masking accessor on the admin (e.g.,
define a method masked_token(self, obj) on AuthAdmin that returns a safely
truncated/masked string like first 8 chars + "..." or None) and update
list_display to use "masked_token" instead of "token"; ensure masked_token is
marked short_description="token" (and optionally admin_order_field if needed) so
the column name remains friendly while preventing full token disclosure.
---
Duplicate comments:
In `@odin/api/authentication.py`:
- Around line 12-15: The authenticate function currently raises
AuthenticationFailed when the Authorization header is missing; change it to
return None instead so DRF can try other authenticators or allow anonymous
access, and update the function signature/return type annotation from
tuple[User, Auth] to Optional[tuple[User, Auth]] (or tuple[User, Auth] | None)
to reflect that None is a valid return value; update any related usages or
imports if necessary to satisfy typing.
---
Nitpick comments:
In `@odin/tests/conftest.py`:
- Around line 27-37: The autouse fixture setup_api_client is masking
unauthenticated endpoint tests; change it from autouse=True to an explicit
opt-in pattern: remove autouse and have setup_api_client check for a pytest
marker (e.g., request.node.get_closest_marker("authenticated")) or require tests
to use the api_client fixture directly, and only inject auth token when that
marker is present; update tests that rely on implicit auth to add
`@pytest.mark.authenticated` or to use api_client, and ensure test_auth.py and
public endpoints (healthcheck/push) remain unauthenticated by default.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: adf7f05e-4d6f-4fc2-87b4-a53ca292462a
📒 Files selected for processing (7)
odin/api/authentication.pyodin/api/v1/sensors/views.pyodin/apps/core/admin.pyodin/apps/core/migrations/0006_auth_user_fk.pyodin/apps/core/models.pyodin/tests/conftest.pyodin/tests/factories.py
✅ Files skipped from review due to trivial changes (1)
- odin/api/v1/sensors/views.py
🚧 Files skipped from review as they are similar to previous changes (2)
- odin/apps/core/models.py
- odin/tests/factories.py
Comment on lines
+25
to
+28
| if not (auth := Auth.objects.filter(token=token).select_related("user").first()): | ||
| raise AuthenticationFailed("Invalid token") | ||
|
|
||
| return (auth.user, auth) |
There was a problem hiding this comment.
Handle case where auth.user is None.
The user foreign key on Auth is nullable (null=True), so auth.user can be None. Returning (None, auth) sets request.user = None, which will raise AttributeError if any permission class checks request.user.is_authenticated.
Consider either requiring a user association or handling the None case explicitly:
🛡️ Option 1: Require user association
if not (auth := Auth.objects.filter(token=token).select_related("user").first()):
raise AuthenticationFailed("Invalid token")
+ if auth.user is None:
+ raise AuthenticationFailed("Token not associated with a user")
+
return (auth.user, auth)🛡️ Option 2: Return Auth as the principal when no user
- return (auth.user, auth)
+ # Auth model has is_authenticated = True, so it can serve as principal
+ return (auth.user if auth.user else auth, auth)📝 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.
Suggested change
| if not (auth := Auth.objects.filter(token=token).select_related("user").first()): | |
| raise AuthenticationFailed("Invalid token") | |
| return (auth.user, auth) | |
| if not (auth := Auth.objects.filter(token=token).select_related("user").first()): | |
| raise AuthenticationFailed("Invalid token") | |
| if auth.user is None: | |
| raise AuthenticationFailed("Token not associated with a user") | |
| return (auth.user, auth) |
Suggested change
| if not (auth := Auth.objects.filter(token=token).select_related("user").first()): | |
| raise AuthenticationFailed("Invalid token") | |
| return (auth.user, auth) | |
| if not (auth := Auth.objects.filter(token=token).select_related("user").first()): | |
| raise AuthenticationFailed("Invalid token") | |
| # Auth model has is_authenticated = True, so it can serve as principal | |
| return (auth.user if auth.user else auth, auth) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@odin/api/authentication.py` around lines 25 - 28, The current auth lookup
returns (auth.user, auth) without guarding for auth.user being None
(Auth.objects.filter(...).select_related("user").first()), so update the
authentication flow to explicitly handle a missing user: after obtaining auth,
check if auth.user is None and either raise AuthenticationFailed("Token not
associated with a user") to require a user association, or alternatively return
the Auth instance as the principal (e.g., return (auth, auth)) if you want the
token to represent the Auth record itself; implement one of these two fixes in
the block that currently returns (auth.user, auth).
Comment on lines
+38
to
+43
| @admin.register(Auth) | ||
| class AuthAdmin(admin.ModelAdmin): | ||
| list_display = ("id", "user", "token", "created_at") | ||
| list_filter = ("created_at",) | ||
| search_fields = ("token", "user__username", "user__email") | ||
| readonly_fields = ("created_at",) |
There was a problem hiding this comment.
Exposing full plaintext token in admin list view is a security risk.
The token field in list_display shows the complete bearer token to any admin user. This allows token theft via the admin interface. The model's __str__ method already truncates to 8 characters for safer display.
Consider either removing token from list_display or creating a custom method that masks most of the token:
🔒 Proposed fix to mask token display
`@admin.register`(Auth)
class AuthAdmin(admin.ModelAdmin):
- list_display = ("id", "user", "token", "created_at")
+ list_display = ("id", "user", "masked_token", "created_at")
list_filter = ("created_at",)
search_fields = ("token", "user__username", "user__email")
readonly_fields = ("created_at",)
+
+ `@admin.display`(description="Token")
+ def masked_token(self, obj: Auth) -> str:
+ return f"{obj.token[:8]}..." if obj.token else ""🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@odin/apps/core/admin.py` around lines 38 - 43, The admin currently exposes
full tokens via AuthAdmin.list_display ("token"), which is insecure; replace
that by either removing "token" from list_display or add a masking accessor on
the admin (e.g., define a method masked_token(self, obj) on AuthAdmin that
returns a safely truncated/masked string like first 8 chars + "..." or None) and
update list_display to use "masked_token" instead of "token"; ensure
masked_token is marked short_description="token" (and optionally
admin_order_field if needed) so the column name remains friendly while
preventing full token disclosure.
manti-by
deleted the
demetra/mnt-46-implement-token-based-authentication-for-odin-project-api
branch
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit