BA-1808 [BE] Add basic profile information to JWT claims #191
Conversation
|
Warning Rate limit exceeded@tsl-ps2 has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 24 minutes and 52 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. WalkthroughThe changes in this pull request focus on enhancing user profile handling and serialization within the application. Key modifications include the addition of a Changes
Possibly related PRs
Suggested reviewers
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 (5)
baseapp-profiles/baseapp_profiles/managers.py (1)
6-6: Consider caching profile results for JWT claims.Since this method will be used for JWT claim generation, consider implementing caching for the profile results to improve performance, especially if tokens are frequently generated or verified. The caching strategy should account for profile updates to maintain data consistency.
baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py (2)
33-34: LGTM: Good addition of context to claim serializerAdding the context to the claim serializer is a good practice. However, we could make the code more readable by extracting the serializer instantiation to a separate line.
Consider this minor refactoring for better readability:
if self._claim_serializer_class: - data = self.get_claim_serializer_class()(user, context=self.context).data + serializer_class = self.get_claim_serializer_class() + data = serializer_class(user, context=self.context).data for key, value in data.items(): token[key] = value
29-37: Consider JWT size limitationsSince we're adding profile information to the JWT claims, we should be mindful of the token size. Large tokens can impact performance and may exceed size limits in some systems (e.g., HTTP headers, cookies).
Consider:
- Limiting the profile data to essential fields only
- Adding a maximum size check for the serialized claims
- Implementing claim compression if needed
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (2)
26-46: Well-structured serializer implementation with room for improvementThe serializer effectively handles the basic profile information needed for JWT claims. However, there are a few suggestions for improvement:
- Consider making the image size configurable rather than hardcoded
- Add validation for the url_path field to ensure consistent formatting
Consider applying these improvements:
class JWTProfileSerializer(serializers.ModelSerializer): url_path = serializers.SlugField(required=False) id = serializers.SerializerMethodField() - image = ThumbnailImageField(required=False, sizes={"small": (100,100)}) + # Use settings for image size configuration + image = ThumbnailImageField( + required=False, + sizes={"small": (settings.JWT_PROFILE_IMAGE_SIZE, settings.JWT_PROFILE_IMAGE_SIZE)} + ) + + def validate_url_path(self, value): + if value and not value.islower(): + raise serializers.ValidationError("URL path must be lowercase") + return value
34-36: Review security implications of JWT payload sizeWhile the fields included are appropriate, consider the following security aspects:
- JWT payload size increases with profile data, which could impact performance
- Ensure the
namefield doesn't expose sensitive information for users with privacy concernsConsider implementing a configuration flag to make profile fields in JWT claims optional based on application needs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 1d303f8 and 5011060787d58e3bb97af92e653ef8e9b9bc2576.
📒 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-profiles/baseapp_profiles/managers.py (1)
6-6: LGTM! Adding distinct() is a good practice here.
The addition of .distinct() is appropriate as it prevents duplicate profiles when a user has multiple relationships (both as member and owner) with the same profile. This aligns well with the PR's objective of adding profile information to JWT claims, where duplicate profiles would be problematic.
Let's verify the query optimization:
✅ Verification successful
The distinct() operation is well-supported and consistently used across profile queries
The verification shows that .distinct() is already being used consistently in similar profile-related queries across the codebase:
- In
baseapp_auth/graphql/object_types.pyfor profile filtering - In
baseapp_profiles/models.pyfor user filtering - All using similar patterns combining
ownerandmembersrelationships
The addition of .distinct() in filter_user_profiles aligns with this established pattern and is necessary for correct behavior when users have multiple relationships with the same profile.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if there are any existing DISTINCT operations in related profile queries
# and verify the database indexes that would support this operation efficiently
# Look for similar distinct operations in profile-related queries
rg -l "distinct\(\)" --type py | while read -r file; do
echo "=== $file ==="
rg "distinct\(\)" -C 3 "$file"
done
# Check for model definitions to verify indexes
ast-grep --pattern 'class Meta:
$$$
indexes = [
$$$
]
$$$'
Length of output: 5034
baseapp-auth/baseapp_auth/graphql/object_types.py (3)
Line range hint 39-42: LGTM! Well-structured class definition.
The UserProfiles class is properly defined with relay.Node interface and the profiles field is correctly typed using the dynamic model object type.
Line range hint 44-51: Consider optimizing the authorization check and query performance.
A few suggestions for improvement:
- The current authorization check might be too restrictive. Consider extracting it to a reusable permission class for consistency across resolvers.
- The query could benefit from optimization by adding appropriate indexes.
- The
.distinct()call might impact performance on large datasets.
Consider refactoring to:
def resolve_profiles(self, info):
- if not info.context.user.is_authenticated or self.id != info.context.user.id:
+ user = info.context.user
+ if not user.is_authenticated or self.id != user.id:
return Profile.objects.none()
- return Profile.objects.filter(
- Q(owner_id=info.context.user.id) | Q(members__user_id=info.context.user.id)
- ).distinct()
+ return Profile.objects.filter(
+ Q(owner_id=user.id) | Q(members__user_id=user.id)
+ ).select_related('owner').distinct()Let's verify the database indexes:
✅ Verification successful
The query optimization concerns are valid but require careful consideration.
-
The authorization check is actually appropriate as it ensures users can only access their own profiles and those they are members of.
-
The database schema shows that appropriate indexes exist:
ownerfield has an index as it's a foreign keymembers__userrelationship has an index through theProfileUserRolemodel's unique constraint on(user, profile)- The query uses these indexed fields effectively in the
Qobjects
-
The
.distinct()call is necessary here because themembers__user_idjoin could produce duplicates, and its performance impact is mitigated by the existing indexes.
Suggested improvements:
- Add
select_related('owner')to reduce subsequent queries if owner data is needed - Consider caching frequently accessed profiles if performance becomes an issue
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if appropriate indexes exist for the query
rg -A 10 "class Migration.*Profile"
Length of output: 37
Script:
#!/bin/bash
# Let's try to find Profile model definition and its indexes
ast-grep --pattern 'class Profile($$$)'
# Also search for any Profile-related migrations
fd -e py migrations | xargs rg -l "Profile"
# Check for any model indexes or meta configurations
rg -A 5 "class Meta.*Profile"
Length of output: 6798
Script:
#!/bin/bash
# Let's check the Profile model definition to understand its structure and indexes
cat baseapp-profiles/baseapp_profiles/models.py
# Let's also check if there are any existing indexes in migrations
cat baseapp-profiles/baseapp_profiles/migrations/0001_initial.py
Length of output: 11324
53-53: Verify JWT claims integration with profiles.
Since the main PR objective is to add profile information to JWT claims, we should verify that this GraphQL type properly supports the JWT serialization.
Let's check the JWT serializer implementation:
✅ Verification successful
Let's gather more information about the JWT serializer implementations to verify the profile integration.
Let's verify how the JWT claims are actually used in the token generation by checking the settings and token implementation.
JWT claims integration is properly implemented
The profile integration with JWT claims is correctly implemented through a well-structured chain:
- The
UserProfilesinterface is properly connected to JWT claims viaJWTProfileSerializerwhich is nested withinUserBaseSerializer UserBaseSerializeris configured as the JWT claim serializer throughJWT_CLAIM_SERIALIZER_CLASSsetting- The claims are injected into tokens through:
BaseJwtLoginSerializer.get_token()for loginCustomClaimRefreshToken.access_tokenfor refresh
- Profile data is properly serialized with essential fields (id, name, image, url_path)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Look for JWT serializer implementation
rg -l "class.*JWTProfileSerializer|class.*BaseJwtLoginSerializer"
# Check if profile fields are included in JWT claims
ast-grep --pattern 'class $_(JWTProfileSerializer|BaseJwtLoginSerializer) {
$$$
def to_representation($$$) {
$$$
}
}'
Length of output: 386
Script:
#!/bin/bash
# Check the content of JWT serializer files to see profile-related implementations
echo "=== Content of baseapp_auth/rest_framework/jwt/serializers.py ==="
cat baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py
echo -e "\n=== Content of baseapp_auth/rest_framework/users/serializers.py ==="
cat baseapp-auth/baseapp_auth/rest_framework/users/serializers.py
# Also check if there are any profile-related fields or methods in these files
rg -A 5 "profile" baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py baseapp-auth/baseapp_auth/rest_framework/users/serializers.py
Length of output: 14016
Script:
#!/bin/bash
# Check settings for JWT claim serializer configuration
rg "JWT_CLAIM_SERIALIZER_CLASS" -A 3 -B 3
# Check the CustomClaimRefreshToken implementation
echo "=== Content of baseapp_auth/rest_framework/jwt/tokens.py ==="
cat baseapp-auth/baseapp_auth/rest_framework/jwt/tokens.py
Length of output: 7112
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (2)
1-2: LGTM! Clean import organization
The new imports and model loading are well-organized and appropriate for the added functionality.
Also applies to: 6-8, 21-22
47-53: Verify the impact of avatar field removal
The changes look good, but we should verify the impact of removing the avatar field since it might affect existing code that depends on it.
Let's check for any remaining references to the avatar field:
Also applies to: 57-58
✅ Verification successful
Avatar field removal is safe as it's been migrated to profile.image
The removal of the avatar field from UserBaseSerializer is safe because:
- The avatar functionality is now handled through the profile.image field (as seen in the code where avatar data is being assigned to instance.profile.image)
- The JWTProfileSerializer (which is now included in UserBaseSerializer) presumably handles the profile image
- No direct references to UserBaseSerializer.avatar were found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for avatar field usage in the codebase
rg -i "avatar.*field|field.*avatar" --type py
# Search for avatar in serializer usage
rg -i "UserBaseSerializer.*avatar|avatar.*UserBaseSerializer" --type py
Length of output: 406
Script:
#!/bin/bash
# Let's check for any direct references to 'avatar' in the codebase
rg -i "\.avatar" --type py -A 2
# Let's also check the history of the file to understand the avatar field usage
git log -p baseapp-auth/baseapp_auth/rest_framework/users/serializers.py | grep -A 5 -B 5 "avatar"
# Check for any imports of AvatarField
rg "from.*AvatarField|import.*AvatarField" --type py
Length of output: 182
Script:
#!/bin/bash
# Let's check where AvatarField is used in serializers.py
ast-grep --pattern 'class $_ {
$$$
avatar = AvatarField($$$)
$$$
}' baseapp-auth/baseapp_auth/rest_framework/users/serializers.py
# Check if avatar is used in profile related code
rg -i "avatar" baseapp-auth/baseapp_auth/rest_framework/users/serializers.py -A 2 -B 2
# Check if JWTProfileSerializer includes avatar field
ast-grep --pattern 'class JWTProfileSerializer {
$$$
}'
Length of output: 704
tsl-ps2
force-pushed
the
BA-1808-add-profile-key-to-jwt-claims
branch
from
5011060 to
d6e5244
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
baseapp-auth/baseapp_auth/tests/integration/test_users.py (1)
158-158: Consider using direct assertion for clarityThe boolean conversion might be less explicit than directly checking for image presence.
-assert bool(user_client.user.profile.image) is True +assert user_client.user.profile.image is not None
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between 5011060787d58e3bb97af92e653ef8e9b9bc2576 and d6e524495d066a208cf57f3d05b0e4b0ee9c7f67.
📒 Files selected for processing (8)
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-auth/baseapp_auth/tests/integration/test_login.py(1 hunks)baseapp-auth/baseapp_auth/tests/integration/test_users.py(3 hunks)baseapp-auth/setup.cfg(1 hunks)baseapp-profiles/baseapp_profiles/managers.py(1 hunks)baseapp-profiles/setup.cfg(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- baseapp-profiles/setup.cfg
- baseapp-auth/setup.cfg
🚧 Files skipped from review as they are similar to previous changes (4)
- baseapp-profiles/baseapp_profiles/managers.py
- baseapp-auth/baseapp_auth/graphql/object_types.py
- baseapp-auth/baseapp_auth/rest_framework/jwt/serializers.py
- baseapp-auth/baseapp_auth/rest_framework/users/serializers.py
🔇 Additional comments (4)
baseapp-auth/baseapp_auth/tests/integration/test_login.py (1)
137-137: Verify JWT token profile structure consistency
The test has been updated to check for user data within a nested "profile" structure, which aligns with the PR objective of enhancing JWT claims. However, we should ensure all JWT token validations are consistent.
baseapp-auth/baseapp_auth/tests/integration/test_users.py (3)
45-45: LGTM: Own user profile structure is well-defined
The test correctly validates the new profile structure for own user, including all required fields (id, name, image, url_path).
Also applies to: 58-66
71-71: LGTM: Other user profile structure maintains privacy
The test appropriately validates that other users' data is limited while maintaining the same profile structure.
Also applies to: 75-83
187-187: LGTM: Search test updated for profile structure
The test correctly reflects the new nested profile structure in search results.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (3)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (3)
32-32: Consider making the image size configurable.The thumbnail size is hardcoded to 100x100. Consider making this configurable through settings or constants for better flexibility.
38-39: Add docstrings to methods.The methods
get_id,get_url_path, andto_representationlack docstrings explaining their purpose and behavior.Also applies to: 41-43, 45-49
53-54: Document the avatar/image field relationship.The relationship between the write-only
avatarfield and the read-onlyprofile.imagefield might be confusing for API consumers. Consider adding API documentation to clarify this relationship.Also applies to: 63-64
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between e2cd7965544fa4b339b37f45ad520d3de775e3c8 and ad674703a6374fe44d8446a0b56aaf3a1d4362b0.
📒 Files selected for processing (2)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py(3 hunks)baseapp-auth/baseapp_auth/tests/integration/test_users.py(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- baseapp-auth/baseapp_auth/tests/integration/test_users.py
🔇 Additional comments (3)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (3)
3-6: LGTM! Clean import organization and model loading.
The added imports and model loading are well-organized and follow Django's best practices for model swapping.
Also applies to: 20-21
25-49: Verify JWT size impact.
The addition of profile data to JWT claims will increase the token size. We should verify that this doesn't cause issues with header size limits in your infrastructure.
✅ Verification successful
Profile data in JWT is optimized for size
The implementation shows careful consideration of token size:
- Only essential fields are included: id, name, image, url_path
- Image is specifically downsized to small thumbnail (100x100)
- The to_representation method further optimizes by only including the small image URL instead of all image variations
Since the serializer is already designed to minimize the payload size and there are no explicit header size limits in the infrastructure configuration that would affect JWT tokens, the current implementation should be safe to use.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Estimate the size impact of adding profile data to JWT claims
# Search for JWT token generation/encoding
rg -A 5 "encode.*jwt|jwt.*encode"
# Search for any header size configurations in deployment files
fd -e yaml -e yml | xargs rg -l "header.*size|size.*header"
Length of output: 97
Script:
#!/bin/bash
# Let's try a different approach to find JWT-related code and configurations
# Search for JWT-related imports and configurations
rg -i "jwt" -A 5
# Search for token generation or authentication settings
rg -i "token|auth.*settings" -A 5
# Look for any nginx or web server configurations that might have header size limits
fd "(nginx|apache|web)" -e conf -e config
Length of output: 73390
Script:
#!/bin/bash
# Let's check for JWT token size configuration and any size-related settings
# Look for JWT token configuration and settings
rg -i "token.*size|size.*token|token.*limit|limit.*token" -A 3
# Search for any SIMPLE_JWT configuration that might affect token size
ast-grep --pattern 'SIMPLE_JWT = {
$$$
}'
# Look for any serializer fields that contribute to token size
ast-grep --pattern 'class JWTProfileSerializer($$$):
$$$
class Meta:
$$$
fields = $$$
'
Length of output: 2676
51-54: Verify test coverage for profile serialization.
The addition of the profile serializer requires comprehensive test coverage, especially for the JWT claims integration.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (2)
45-49: Add defensive programming for image handling.The image transformation could fail if the 'small' size is not available. Consider adding error handling:
def to_representation(self, profile): data = super().to_representation(profile) - if data["image"] is not None: - data["image"] = data["image"]["small"] + if data.get("image"): + try: + data["image"] = data["image"]["small"] + except (KeyError, TypeError): + data["image"] = None return data
26-28: Enhance JWT claims documentation.While the docstring indicates the purpose, it would be helpful to add:
- Example of the JWT payload structure
- Size limitations for the included fields
- Impact on token size and any performance considerations
""" Serializes minimal profile data that will be attached to the JWT token as claim + + Example JWT payload: + { + "profile": { + "id": "UHJvZmlsZTox", + "name": "John Doe", + "image": "https://example.com/image_small.jpg", + "url_path": "john-doe" + } + } + + Note: Image URLs are transformed to use the 'small' thumbnail size (100x100) + to minimize token size. """
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Reviewing files that changed from the base of the PR and between ad674703a6374fe44d8446a0b56aaf3a1d4362b0 and 5aed94d976cad80d66850ff7540b3f978382dcd2.
📒 Files selected for processing (1)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py(3 hunks)
🔇 Additional comments (3)
baseapp-auth/baseapp_auth/rest_framework/users/serializers.py (3)
3-6: LGTM: Import statements and model loading are appropriate.
The new imports support the JWT profile serialization functionality, and the Profile model is correctly loaded using swapper.
Also applies to: 20-21
41-43: Improve url_path handling and add tests.
The url_path is a ForeignKey that can contain '/' characters. The current implementation needs more robust error handling and tests.
Let's verify the current usage of url_path in tests:
#!/bin/bash
# Search for url_path test cases
rg -A 5 "url_path" --type py53-54: Verify API compatibility for existing consumers.
The addition of the profile field and making avatar write-only represents a significant API change. Please ensure:
- All API consumers are prepared for this change
- Documentation is updated to reflect the new structure
- Consider if a migration period is needed where both old and new formats are supported
Let's check for API consumers:
Also applies to: 63-64
This allows the claim serializer to convert the profile image urls into absolute urls.
tsl-ps2
force-pushed
the
BA-1808-add-profile-key-to-jwt-claims
branch
from
5aed94d to
0ad92c7
Compare
|
|
This adds basic profile information (id, name, image, urlPath) to the JWT claims
Summary by CodeRabbit
Release Notes
New Features
JWTProfileSerializerfor improved user data serialization in JWT tokens.Bug Fixes
Tests
Chores