Conversation
| @extend_schema( | ||
| responses={200: {"type": "string", "example": "Hello World"}} | ||
| request=EmailLoginSerializer, | ||
| responses={ | ||
| 200: { | ||
| "type": "object", | ||
| "properties": { | ||
| "refresh": { | ||
| "type": "string", | ||
| "description": "JWT refresh token", | ||
| "example": "eyJ0eXAiOiJKV1QiLCJhbGc..." | ||
| }, | ||
| "access": { | ||
| "type": "string", | ||
| "description": "JWT access token", | ||
| "example": "eyJ0eXAiOiJKV1QiLCJhbGc..." | ||
| }, | ||
| "user_info": { | ||
| "type": "object", | ||
| "properties": { | ||
| "full_name": { | ||
| "type": "string", | ||
| "example": "John Doe" | ||
| }, | ||
| "first_name": { | ||
| "type": "string", | ||
| "example": "John" | ||
| }, | ||
| "last_name": { | ||
| "type": "string", | ||
| "example": "Doe" | ||
| }, | ||
| "avatar": { | ||
| "type": ["string", "null"], | ||
| "format": "uri", | ||
| "example": None, | ||
| "description": "User avatar URL, can be null" | ||
| } | ||
| } | ||
| } | ||
| } | ||
| }, |
There was a problem hiding this comment.
Consider adding a response serializer (e.g., LoginResponseSerializer) instead of manually constructing the response dictionary and documenting it inline in @extend_schema.
There was a problem hiding this comment.
Since this is not a serialized model and involves custom fields and calculations, I believe keeping it as is would be better for maintainability and readability.
users/v1/views.py
Outdated
| user = user_model.objects.filter(email=email).first() | ||
| if not user: | ||
| raise ValidationError("Invalid email or password") | ||
| if not user.check_password(password): | ||
| raise ValidationError("Invalid email or password") |
There was a problem hiding this comment.
[question] Why aren't we using Django's authenticate() method here? The current implementation is prone to time attack (The user lookup and password check happen separately which could leak the information about which emails exist)
There was a problem hiding this comment.
I’ve updated the implementation. The check for the password was originally used because Django’s authentication backend relies on the username rather than the email. I’ve now applied a workaround and updated the usage to address the previously mentioned vulnerabilities.
sameeramin
left a comment
sameeramin
left a comment
There was a problem hiding this comment.
Great work! Just a few suggestions, which I added in the comments
Palwisha-18
left a comment
Palwisha-18
left a comment
There was a problem hiding this comment.
Minor Suggestions. Otherwise looks good. 👍
|
|
||
| # Django's default authentication backend uses the `username` field, not `email`. | ||
| # We first fetch the user by email, then authenticate using their username. | ||
| user = user_model.objects.filter(email=email).first() |
There was a problem hiding this comment.
Suggestion: Since email is unique, prefer get() over filter().first() to make the assumption explicit and fail fast if data integrity is violated.
| 'full_name': user.get_full_name(), | ||
| 'first_name': user.first_name, | ||
| 'last_name': user.last_name, | ||
| 'avatar': None |
There was a problem hiding this comment.
Suggestion: avatar is always returned as None. If no avatar is available, consider omitting the field rather than returning a constant null value.
| return Response({ | ||
| 'refresh': str(refresh), | ||
| 'access': str(refresh.access_token), | ||
| 'user_info': { |
There was a problem hiding this comment.
Suggestion: Consider including a stable identifier (e.g. user_id) in user_info to simplify client-side user state management.
3 participants
Description
/api/v1/users/login/emailPOST endpoint.{"email": "[email protected]", "password": "password"}Link to the associated Taiga ticket: https://projects.arbisoft.com/project/arbisoft-sessions-portal-20/us/262
Considerations
Post-review
Combine commits into distinct sets of changes.