BA-1808 Add basic profile information to JWT claims #189
Conversation
This allows the claim serializer to convert the profile image urls into absolute urls.
WalkthroughThe changes in this pull request involve modifications to the GraphQL and REST framework components of the application. A new Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
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: 1
🧹 Outside diff range and nitpick comments (7)
baseapp-profiles/baseapp_profiles/managers.py (1)
6-6: Consider adding an index for query optimization.Since this query is used in JWT token generation, consider adding a composite index on (user_id, owner_id) to optimize the distinct operation, especially if the profiles table grows large.
baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py (2)
33-36: Add error handling and data validation for custom claims.While the implementation is correct, consider these improvements for robustness:
- Add error handling for invalid serializer instantiation
- Validate claim values for JWT compatibility (primitive types only)
if self._claim_serializer_class: - data = self.get_claim_serializer_class()(user, context=self.context).data - for key, value in data.items(): - token[key] = value + try: + serializer_class = self.get_claim_serializer_class() + data = serializer_class(user, context=self.context).data + for key, value in data.items(): + if not isinstance(value, (str, int, float, bool, list, dict, type(None))): + raise ValueError(f"Invalid claim type for {key}: {type(value)}") + token[key] = value + except Exception as e: + raise serializers.ValidationError(f"Failed to serialize custom claims: {str(e)}")
33-36: Document required context keys for claim serialization.Since the code now passes context to the claim serializer, it would be helpful to document the expected context keys for proper profile serialization.
Add a docstring like:
def get_token(self, user): """ Generate JWT token with custom claims for the user. Required context keys for profile serialization: - 'request': The current request object - ... (list other required keys) """baseapp-auth/baseapp_auth/graphql/object_types.py (2)
51-51: Optimize the query by moving distinct() earlier in the chain.The addition of
.distinct()is good to prevent duplicate profiles. However, consider optimizing the query by moving the distinct operation before the filter to reduce the number of records processed:- ).distinct() + return ( + Profile.objects.distinct() + .filter( + Q(owner_id=info.context.user.id) | + Q(members__user_id=info.context.user.id) + ) + )
48-50: Consider adding explicit error handling for unauthorized access.The security checks are well implemented, ensuring proper authentication and authorization. However, returning an empty queryset silently might not provide the best user experience.
Consider raising a GraphQL error for unauthorized access:
def resolve_profiles(self, info): - if not info.context.user.is_authenticated or self.id != info.context.user.id: - return Profile.objects.none() + if not info.context.user.is_authenticated: + raise graphene.GraphQLError("Authentication required to access profiles") + if self.id != info.context.user.id: + raise graphene.GraphQLError("Unauthorized access to profiles")baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (2)
37-38: Consider adding URL path validation.The url_path structure should be validated to prevent potential security issues with malformed paths.
def get_url_path(self, profile): + if not profile.url_path or not profile.url_path.path: + return {'path': ''} + if not profile.url_path.path.startswith('/'): + return {'path': f'/{profile.url_path.path}'} return { 'path': profile.url_path.path}
25-46: Verify JWT token size and caching strategy.The implementation successfully adds profile information to JWT claims. However, we should verify:
- The impact on token size
- Token invalidation strategy when profile data changes
- Caching implications
Consider implementing a caching strategy for profile data to prevent frequent token regeneration, especially for the image URLs. This could involve:
- Using a separate endpoint for image updates
- Implementing a versioning system for profile data
- Setting appropriate cache headers for image URLs
Also applies to: 50-50, 59-59
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
baseapp-auth/baseapp_auth/graphql/object_types.py(1 hunks)baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py(1 hunks)baseapp-auth/baseapp_auth/rest_framework/users/serializers.py(2 hunks)baseapp-profiles/baseapp_profiles/managers.py(1 hunks)
🔇 Additional comments (6)
baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py (1)
29-29: Verify subclass compatibility with the method signature change.
The change from classmethod to instance method is necessary for context access but could affect subclasses.
✅ Verification successful
Method signature change is safe to proceed
The verification shows that:
- Only one subclass exists (
MyTokenObtainPairSerializer) and it doesn't override theget_tokenmethod - The subclass only customizes the
_claim_serializer_classattribute - No other usages of
get_tokenas a classmethod were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Find any subclasses of BaseJwtLoginSerializer that might be affected
# by the method signature change from classmethod to instance method
# Search for classes inheriting from BaseJwtLoginSerializer
ast-grep --pattern 'class $NAME($$$BaseJwtLoginSerializer$$$)'
# Search for any overrides of get_token that might still use @classmethod
rg -A 5 '@classmethod.*get_token'
Length of output: 100
Script:
#!/bin/bash
# Let's try a different approach to find potential subclasses and usages
# Search for any imports of BaseJwtLoginSerializer
rg "from.*BaseJwtLoginSerializer" -l
# Search for any class definitions that might inherit from it (without AST)
rg "class.*\(.*BaseJwtLoginSerializer.*\)"
# Search for any get_token method definitions
rg "def get_token"
# Look at the actual serializer file to understand the inheritance chain
cat baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py
Length of output: 2226
Script:
#!/bin/bash
# Let's examine the test project's serializer to see how it uses get_token
cat baseapp-auth/testproject/testapp/rest_framework/jwt/serializers.py
Length of output: 329
baseapp-auth/baseapp_auth/graphql/object_types.py (1)
Line range hint 42-52: Verify profile fields match JWT claim requirements.
The PR aims to add user ID, name, image, and URL path to JWT claims. Please ensure that all required fields are accessible through this GraphQL type.
✅ Verification successful
Profile fields correctly match JWT claim requirements
The verification confirms that all required JWT claim fields are properly accessible:
- The
JWTProfileSerializerexplicitly includes all needed fields: "id", "name", "image", "url_path" - The GraphQL type
UserProfilesprovides access to profiles through the Profile model - The
AbstractUserObjectTypeincludes avatar resolution through profile.image and name through profile.name
The implementation aligns perfectly with the PR's objective of adding user ID, name, image, and URL path to JWT claims.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if all required JWT claim fields are defined in the codebase
# Search for JWT serializer implementation
rg -l "JWTProfileSerializer|get_token.*profile" --type py
# Search for profile-related fields in GraphQL types
ast-grep --pattern 'class $_(DjangoObjectType) {
$$$
profile = $$$
$$$
}'
Length of output: 203
Script:
#!/bin/bash
# Let's examine the JWT serializer implementation and profile fields
rg -A 10 "JWTProfileSerializer" --type py
# Also check the Profile model definition to understand available fields
ast-grep --pattern 'class Profile(models.Model):
$$$'
# Check if there are any other profile-related fields in object_types.py
cat baseapp-auth/baseapp_auth/graphql/object_types.py
Length of output: 7606
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (4)
1-2: LGTM! Imports and model loading are properly structured.
The imports support the new JWT profile functionality while maintaining model swappability through swapper.
Also applies to: 6-8, 21-21
25-28: LGTM! Well-documented serializer purpose.
The serializer's scope is clearly defined for JWT token claims.
50-50: LGTM! Profile integration looks correct.
The profile field is properly added with read-only access, and the Meta fields are correctly updated.
Also applies to: 59-59
29-31: Verify the security implications of including profile data in JWT.
Consider the following security aspects:
- JWT token size with the included profile data
- Sensitive information exposure
- Image URL accessibility
✅ Verification successful
Let me gather more specific information about the JWT token implementation and profile serialization.
Let me check the claim serializer implementation and profile data that gets included in the token.
Let me check one final thing about the profile model and its serialization.
Based on the gathered information, I can now provide a final response about the security implications.
Profile data in JWT appears to be properly scoped and controlled
The implementation shows that:
- JWT claims are serialized using
UserBaseSerializer, not the full profile serializer, which limits the data exposure - The profile model contains sensitive fields (phone_number, image, banner_image), but these are not directly included in the JWT as the claim serializer is specifically set to
UserBaseSerializer - The image field in the review is using
ThumbnailImageFieldwith size constraints (100x100), which helps control the token size - No sensitive profile information is being exposed through the JWT claims
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential sensitive information in Profile model
ast-grep --pattern 'class Profile:
$$$
class Meta:
$$$
'
# Check for other JWT token usages
rg -l "jwt" .
Length of output: 1525
Script:
#!/bin/bash
# Check JWT token implementation
ast-grep --pattern 'class $_(serializers.Serializer):
$$$
def get_token($$$):
$$$
'
# Check profile serializer implementation
ast-grep --pattern 'class ProfileSerializer($$$):
$$$'
# Check what fields are included in the token
rg -A 10 "def get_token" ./baseapp-auth/baseapp_auth/rest_framework/jwt/
# Check if there's any size limitation configuration for JWT
rg "JWT.*SIZE|TOKEN.*SIZE" .
Length of output: 1269
Script:
#!/bin/bash
# Check claim serializer implementation
rg -A 10 "claim_serializer_class" ./baseapp-auth/
# Check profile fields that might be included
ast-grep --pattern 'class Profile($$$):
$$$'
# Check user serializer implementation
rg -A 20 "class UserSerializer" ./baseapp-auth/
# Check if there are any token size tests
rg -l "test.*token.*size|test.*jwt.*size" .
Length of output: 9023
Script:
#!/bin/bash
# Check the AbstractProfile implementation
rg -A 20 "class AbstractProfile" ./baseapp-profiles/
# Check the profile serialization in JWT claims
rg -A 10 "JWT_CLAIM_SERIALIZER_CLASS" ./baseapp-auth/baseapp_auth/settings.py
# Check if there are any profile-related serializers that might be used in JWT
rg -A 10 "class.*ProfileSerializer" ./baseapp-profiles/
Length of output: 7729
| class ProfileManager(models.Manager): | ||
| def filter_user_profiles(self, user): | ||
| return self.filter(models.Q(members__user=user) | models.Q(owner=user)) | ||
| return self.filter(models.Q(members__user=user) | models.Q(owner=user)).distinct() |
There was a problem hiding this comment.
💡 Codebase verification
Similar queries found in the codebase - one missing .distinct()
The codebase scan reveals an inconsistency in the usage of .distinct() with similar Q-object queries:
- In
baseapp_profiles/baseapp_profiles/models.py: A similar query uses.distinct()forprofiles_ownerandprofile_members__profile - However, in the same file, another query
filter(models.Q(members__user=user) | models.Q(owner=user))is missing.distinct()in the.exists()check
🔗 Analysis chain
LGTM! The addition of .distinct() prevents duplicate profiles.
The change correctly ensures unique profiles when a user is both a member and owner of a profile, which is essential for accurate JWT claims.
Let's verify the SQL query generation and check for similar patterns in the codebase:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for similar query patterns and potential inconsistencies
# Look for similar Q-object queries that might need .distinct()
rg -A 2 "models\.Q\(.*\)\s*\|.*models\.Q\(" --type py
# Check for other profile-related queries that might need similar treatment
ast-grep --pattern 'filter($$$).distinct()'
Length of output: 1004
| return Profile.objects.filter( | ||
| Q(owner_id=info.context.user.id) | Q(members__user_id=info.context.user.id) | ||
| ) | ||
| ).distinct() |
There was a problem hiding this comment.
This ensures that profiles are only displayed once when switching profiles (calling .filter with a condition that involves multiple databases might produce duplicate entries)
| if cls._claim_serializer_class: | ||
| data = cls.get_claim_serializer_class()(user).data | ||
| if self._claim_serializer_class: | ||
| data = self.get_claim_serializer_class()(user, context=self.context).data |
There was a problem hiding this comment.
This allows relative urls to be converted to absolute urls when serializing the profile image
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.0)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py
43-43: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
🔇 Additional comments (4)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (4)
26-36: Implementation aligns well with PR objectives
The new JWTProfileSerializer effectively adds basic profile information to JWT claims while maintaining a minimal payload size. The implementation is clean and follows good practices by:
- Using relay IDs instead of database IDs
- Including only essential profile fields
- Optimizing image size for JWT context
Consider addressing the noted security and compatibility considerations, but otherwise, the changes look good.
49-49: Verify data migration requirements
The addition of the profile field and removal of direct avatar access might require data migration for existing applications. Please ensure:
- Existing JWT tokens remain valid
- Frontend applications are updated to use the new profile structure
26-46: Consider security implications of image URLs in JWT claims
The current implementation includes image URLs in JWT claims. This could potentially expose internal storage paths or increase token size. Consider:
- Using relative paths instead of full URLs
- Implementing a separate endpoint for image resolution
- Adding URL sanitization before including in claims
✅ Verification successful
JWT claims contain only the small thumbnail URL, mitigating security concerns
After analyzing the implementation, the security concerns are minimal because:
- The
JWTProfileSerializeronly includes the 'small' (100x100) thumbnail URL in the JWT claim, not the original image path - The
ThumbnailImageFieldimplementation properly handles URL generation through Django's URL system and request context - Image URLs are processed through Django's thumbnailer, which abstracts the actual storage location
The current implementation follows security best practices by:
- Using Django's built-in URL handling
- Only exposing processed thumbnails, not original files
- Using proper URL abstraction through the thumbnailer
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if image URLs might expose sensitive information
rg -g '*.py' 'ThumbnailImageField.*get_url'
Length of output: 45
Script:
#!/bin/bash
# Let's check the ThumbnailImageField implementation and related files
rg -g '*.py' 'class ThumbnailImageField'
# Also check for any image URL processing or storage configuration
rg -g '*.py' -B 2 -A 2 'image.*url|MEDIA_URL|storage.*url'
# Check the actual serialization of image data
ast-grep --pattern 'data["image"] = $_'
Length of output: 8421
Script:
#!/bin/bash
# Let's check the ThumbnailImageField implementation to understand how it handles URLs
cat baseapp-core/baseapp_core/rest_framework/fields.py
# Check how the image field is used in the Profile model
rg -g '*.py' 'class Profile.*image'
Length of output: 1996
🧰 Tools
🪛 Ruff (0.8.0)
43-43: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
58-58: Verify profile field visibility requirements
The profile field is included in the base fields but not explicitly listed in private_fields. Confirm if this is intentional and that it doesn't expose sensitive user information in public endpoints.
✅ Verification successful
Based on the code analysis, I can now provide a definitive response about the profile field visibility:
Profile field visibility is correctly configured
The profile field is intentionally excluded from private_fields in UserBaseSerializer because:
- The profile field uses JWTProfileSerializer which only exposes minimal public data (id, name, image, url_path)
- UserPublicSerializer correctly filters out private fields while keeping profile accessible
- The profile system has proper permission controls through
baseapp_profiles.view_profilepermission which is checked in multiple places - The profile data exposed through the serializer is read-only and contains only public-facing information
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check profile field usage in public endpoints
rg -g '*.py' 'UserPublicSerializer.*profile'
Length of output: 46
Script:
#!/bin/bash
# Let's first check the implementation of UserPublicSerializer and its fields
ast-grep --pattern 'class UserPublicSerializer($$$)'
# Also check the base serializer implementation
ast-grep --pattern 'class UserBaseSerializer($$$)'
# And check for any profile-related models or fields
rg -g '*.py' 'profile' -B 2 -A 2
Length of output: 104189
Script:
#!/bin/bash
# Let's check the UserPublicSerializer implementation and its fields
rg -g '*.py' 'class UserPublicSerializer.*?}' -U
# Also check for any private fields definition
rg -g '*.py' 'private_fields.*=.*profile'
# And check if there are any permissions or visibility controls for profile
rg -g '*.py' 'profile.*private|private.*profile'
Length of output: 145
Script:
#!/bin/bash
# Let's check the entire serializer file content to understand the visibility setup
cat baseapp-auth/baseapp_auth/rest_framework/users/serializers.py
# Also check for any profile-related permissions in the codebase
rg -g '*.py' 'has_perm.*profile'
Length of output: 10428
| if (data['image'] != None): | ||
| data['image'] = data['image']['small'] | ||
| return data |
There was a problem hiding this comment.
Fix comparison to None
Use is not None instead of != None for Python identity comparison.
- if (data['image'] != None):
+ if data['image'] is not None:📝 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.
| if (data['image'] != None): | |
| data['image'] = data['image']['small'] | |
| return data | |
| if data['image'] is not None: | |
| data['image'] = data['image']['small'] | |
| return data |
🧰 Tools
🪛 Ruff (0.8.0)
43-43: Comparison to None should be cond is not None
Replace with cond is not None
(E711)
2 participants
This adds basic profile information (id, name, image, urlPath) to the JWT claims
Summary by CodeRabbit
Release Notes
New Features
UserProfilesclass for enhanced user profile management in the GraphQL API.JWTProfileSerializerto streamline profile data serialization in JWT tokens.Improvements
Bug Fixes
These changes enhance user experience by providing distinct profiles and improving the security of token-based authentication.