feature: add enterprise id into token, remove wrong constraint #114
Conversation
GiaBaorr
requested review from
thongdanghoang,
huynhlephcvinh and
nganntqe170236
WalkthroughThis pull request introduces enterprise identifier support in user-related security and service components. It adds an Changes
Possibly related PRs
Suggested labels
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)
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: 3
🧹 Nitpick comments (2)
sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (1)
55-55: Consider renaming the method for clarity.While the implementation is correct, the method name
findByNamecould be more descriptive since it now filters by both name and enterprise. Consider renaming tofindByNameAndEnterprisefor better clarity.- Page<UUID> findByName(String name, UUID enterpriseId, Pageable pageable); + Page<UUID> findByNameAndEnterprise(String name, UUID enterpriseId, Pageable pageable);sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java (1)
19-20: LGTM! Good architectural change.The addition of enterprise ID to the security context is a good architectural decision that:
- Enables proper multi-tenancy support
- Aligns with the PR objective of adding enterprise ID into token
- Maintains immutability by using
List.copyOf()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(2 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java(2 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java(2 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build Java Modules (sep490-enterprise)
🔇 Additional comments (5)
sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (1)
51-51: LGTM! Enterprise filtering added correctly.The enterprise ID filter is properly integrated into the query using correct JPA entity navigation.
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java (1)
16-16: LGTM! Clean addition of enterprise ID field.The field is correctly marked as private and final, ensuring proper encapsulation and immutability. The Lombok annotations handle the constructor and getter generation appropriately.
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java (1)
23-25: LGTM! Well-implemented utility method.The method follows the established pattern in the class, using Optional for null safety and functional style for clean code.
sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java (2)
4-4: LGTM!The import is correctly added and follows package naming conventions.
102-113: Improve error handling for enterprise ID retrieval.While the enterprise-level data isolation is a good security measure, the current implementation could be improved:
- The
.orElseThrow()call should include a meaningful error message.- Consider handling the case where enterprise ID is not present more gracefully.
Consider applying this improvement:
- UUID enterpriseId = SecurityUtils.getCurrentUserEnterpriseId().orElseThrow(); + UUID enterpriseId = SecurityUtils.getCurrentUserEnterpriseId() + .orElseThrow(() -> new BusinessException( + "enterpriseId", + "user.search.enterprise.required", + Collections.emptyList() + ));Let's verify if the
BusinessExceptionis consistently used across the codebase for similar scenarios:✅ Verification successful
🏁 Script executed:
#!/bin/bash # Search for similar patterns of throwing BusinessException rg -A 2 "new BusinessException\\(" --type javaLength of output: 3303
Enhance enterprise ID error handling in search method
The updated suggestion to throw a custom BusinessException with a meaningful error message is consistent with similar patterns in the codebase (e.g., in DevResource.java and UserValidator.java). Using a lambda with detailed parameters is an improvement over the plain
.orElseThrow(), as it provides clarity on the error context.
- Update
SecurityUtils.getCurrentUserEnterpriseId().orElseThrow()to include a lambda that throws a BusinessException with a meaningful error message (e.g., "user.search.enterprise.required").
sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java
Outdated
Show resolved
Hide resolved
...springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java
Outdated
Show resolved
Hide resolved
GiaBaorr
force-pushed
the
feature/authorize-recheck
branch
from
1672338 to
72ac1a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (1)
47-55: Consider adding an index for enterprise-based queries.Since the repository now filters users by enterprise ID, consider adding a database index on the enterprise column to optimize query performance, especially for large datasets.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(2 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java(2 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java(2 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java(2 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (7)
- sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java
- sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java
- sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql
- sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java
🔇 Additional comments (2)
sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (2)
51-51: LGTM! Enterprise filtering condition is well-placed.The enterprise filtering condition is correctly added and properly integrated with the existing query logic.
55-55: LGTM! Method signature properly updated.The method signature has been correctly updated to include the enterprise ID parameter while maintaining proper parameter ordering and type safety.
GiaBaorr
force-pushed
the
feature/authorize-recheck
branch
from
72ac1a7 to
4b415f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java (1)
137-146: Consider a more specific method name.The current method name
performCreateUserActionis too generic. Consider renaming it to better reflect its specific responsibility of handling password generation and notification for new users.- private void performCreateUserAction(UserEntity user) { + private void handleNewUserPasswordAndNotification(UserEntity user) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(2 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java(2 hunks)sep490-idp/src/main/java/sep490/idp/mapper/decoratos/EnterpriseUserMapperDecorator.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java(2 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java(4 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java
- sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java
- sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java
🔇 Additional comments (5)
sep490-idp/src/main/java/sep490/idp/mapper/decoratos/EnterpriseUserMapperDecorator.java (1)
30-35: Verify the enterprise ID retrieval logic.The method relies on
SecurityUtils.getCurrentUserEnterpriseId()to get the enterprise ID. Let's verify this implementation.Run the following script to check the implementation:
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Check the implementation of getCurrentUserEnterpriseId. # Test: Search for getCurrentUserEnterpriseId implementation. ast-grep --pattern $'class SecurityUtils { $$$ getCurrentUserEnterpriseId() { $$$ } $$$ }'Length of output: 103
🏁 Script executed:
#!/bin/bash # Re-run search for SecurityUtils class and its getCurrentUserEnterpriseId implementation using ripgrep echo "Searching for class SecurityUtils definition..." rg "class SecurityUtils" -n . echo "Searching for getCurrentUserEnterpriseId implementation..." rg "getCurrentUserEnterpriseId" -n .Length of output: 946
🏁 Script executed:
#!/bin/bash # Print context around the getCurrentUserEnterpriseId implementation in SecurityUtils.java sed -n '15,45p' sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.javaLength of output: 1231
Enterprise ID retrieval logic verified.
The implementation of
SecurityUtils.getCurrentUserEnterpriseId()correctly retrieves the enterprise ID by mapping theUserContextDatato itsenterpriseIdproperty. The use oforElseThrow()in theEnterpriseUserMapperDecoratorensures that an exception is thrown when the ID is not present, which is the expected behavior.
- Confirmed that
SecurityUtils.getCurrentUserEnterpriseId()returns anOptional<UUID>by mappingUserContextData::getEnterpriseId.- The code in
EnterpriseUserMapperDecoratorproperly sets the enterprise ID using this logic.sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (2)
51-51: LGTM!The enterprise filtering condition is correctly placed and uses the proper field path.
55-55: Verify all callers of findByName are updated.The method signature change is correct. However, ensure all callers are updated to provide the enterprise ID parameter.
✅ Verification successful
🏁 Script executed:
#!/bin/bash # Description: Find all callers of findByName to verify they're updated with the new parameter. rg -A 5 'findByName\(' --type javaLength of output: 1346
All callers of findByName have been correctly updated to provide the enterprise ID parameter.
The usage inUserServiceImpl.javashows that the call now includes theenterpriseIdparameter as the second argument, aligning with the updated method signature inUserRepository.java.sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java (2)
4-4: LGTM!The SecurityUtils import is correctly added and properly placed.
131-135: LGTM!The refactoring improves code organization by extracting the create user logic into a separate method.
sep490-idp/src/main/java/sep490/idp/mapper/decoratos/EnterpriseUserMapperDecorator.java
Show resolved
Hide resolved
sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql
Outdated
Show resolved
Hide resolved
thongdanghoang
force-pushed
the
feature/authorize-recheck
branch
from
4b415f7 to
8f14dd7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (1)
50-52: LGTM! Consider adding an index for performance optimization.The enterprise filtering condition is correctly placed and properly uses parameter binding.
Consider adding an index on the enterprise column to optimize query performance, especially if the table has a large number of users.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java(2 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java(1 hunks)sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java(2 hunks)sep490-idp/src/main/java/sep490/idp/mapper/decoratos/EnterpriseUserMapperDecorator.java(1 hunks)sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java(1 hunks)sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java(1 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java(2 hunks)sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java(4 hunks)sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- sep490-idp/src/main/resources/db/migration/V0.0.1.7__RemoveConstraintUserEnterprise.sql
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/utils/SecurityUtils.java
- sep490-idp/src/main/java/sep490/idp/mapper/decoratos/EnterpriseUserMapperDecorator.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/JwtAuthenticationConverter.java
- sep490-idp/src/main/java/sep490/idp/service/impl/UserInfoService.java
- sep490-commons/springfw-impl/src/main/java/commons/springfw/impl/securities/UserContextData.java
- sep490-idp/src/main/java/sep490/idp/security/MvcUserContextData.java
- sep490-idp/src/main/java/sep490/idp/service/impl/UserServiceImpl.java
🔇 Additional comments (1)
sep490-idp/src/main/java/sep490/idp/repository/UserRepository.java (1)
55-55: LGTM! Method signature updated correctly.The method signature has been properly updated to include the enterprise ID parameter, maintaining proper parameter ordering and return type.
Summary by CodeRabbit