chore: Remove fetching users without organizationId#39656
chore: Remove fetching users without organizationId#39656trishaanand merged 6 commits intoreleasefrom
Conversation
WalkthroughThis pull request integrates organization context into authentication and user management. Constructor signatures across various authentication handlers and services are updated to include a new Changes
Suggested labels
Suggested reviewers
Poem
🪧 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
Documentation and Community
|
github-actions
bot
added
the
skip-changelog
![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
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/UserRepositoryTest.java (1)
49-54: Revised tests now adequately include org ID in the user setup and queries.A helper method (or builder) to streamline repetitive “organizationId” assignment across multiple tests might improve code maintainability.
Also applies to: 67-72
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOAuth2UserServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOidcUserServiceImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationSuccessHandlerCE.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomFormLoginServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/domains/PasswordResetToken.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserOrganizationHelper.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserOrganizationHelperImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserOrganizationHelperCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserOrganizationHelperCEImpl.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java(0 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/PasswordResetTokenRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java(1 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java(3 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/UserRepositoryTest.java(6 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java(6 hunks)
💤 Files with no reviewable changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/CustomUserRepositoryCE.java
🧰 Additional context used
🧠 Learnings (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java (1)
Learnt from: trishaanand
PR: appsmithorg/appsmith#38359
File: app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java:78-82
Timestamp: 2024-12-25T06:50:40.623Z
Learning: In `CustomOAuth2UserServiceCEImpl`, the second repository call for finding users by case-insensitive email is needed because older users might have uppercase or mixed-case emails, while new users have emails stored in lowercase.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOAuth2UserServiceImpl.java (1)
Learnt from: trishaanand
PR: appsmithorg/appsmith#38359
File: app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java:78-82
Timestamp: 2024-12-25T06:50:40.623Z
Learning: In `CustomOAuth2UserServiceCEImpl`, the second repository call for finding users by case-insensitive email is needed because older users might have uppercase or mixed-case emails, while new users have emails stored in lowercase.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOidcUserServiceImpl.java (1)
Learnt from: trishaanand
PR: appsmithorg/appsmith#38359
File: app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java:78-82
Timestamp: 2024-12-25T06:50:40.623Z
Learning: In `CustomOAuth2UserServiceCEImpl`, the second repository call for finding users by case-insensitive email is needed because older users might have uppercase or mixed-case emails, while new users have emails stored in lowercase.
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (64)
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserOrganizationHelper.java (1)
1-5: LGTM: Clean interface inheritance patternThe interface properly extends the CE (Community Edition) version, following the established inheritance pattern for the codebase.
app/server/appsmith-server/src/main/java/com/appsmith/server/domains/PasswordResetToken.java (1)
20-20: Add organization context to password reset tokensThe addition of the
organizationIdfield aligns with the PR objective to remove user fetching without organization context. This change will ensure password reset operations are organization-scoped.app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/UserOrganizationHelperImpl.java (1)
1-14: LGTM: Standard Spring component implementationThe implementation follows good Spring practices with proper dependency injection and extension of the base CE implementation.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserOrganizationHelperCE.java (1)
1-11: Well-designed reactive interface with clear documentationThe interface clearly defines its purpose with proper Javadoc. The reactive return type (Mono) aligns with the application's reactive architecture pattern.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImpl.java (2)
4-4: Import added for new dependencyAdding the import for UserOrganizationHelper to support the constructor parameter change.
15-16: Updated constructor to include organization contextConstructor now accepts UserOrganizationHelper and passes it to the superclass, aligning with the PR objective to ensure users are fetched with organization context.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/PasswordResetTokenRepositoryCE.java (1)
9-9:Details
✅ Verification successful
Method signature updated to include organization context
Method now requires organizationId as an additional parameter, ensuring that password reset tokens are organization-specific. This change is consistent with the approach of making all user operations organization-aware.
Run the following script to verify all callers of this method have been updated:
🏁 Script executed:
#!/bin/bash # Find all references to the old findByEmail method in the codebase rg "findByEmail\s*\(\s*\w+\s*\)" --type javaLength of output: 1827
PasswordResetTokenRepositoryCE update verified – no outdated call-sites found
I ran the script and confirmed that no calls to the old method (i.e. findByEmail without organization context) are made on the PasswordResetTokenRepository. The remaining references to findByEmail appear in tests and in the userRepository context, which are unrelated to this update.
app/server/appsmith-server/src/main/java/com/appsmith/server/helpers/ce/UserOrganizationHelperCEImpl.java (4)
1-12: New helper class for organization contextNew implementation that handles organization retrieval logic, supporting the PR objective to make user operations organization-aware.
13-24: Constructor and dependency injectionThe class properly injects required dependencies for organization repository and caching.
25-28: Organization ID retrieval methodImplementation of the interface method delegates to the private helper method.
30-43: Cache-optimized organization ID retrievalGood implementation using caching to optimize organization ID lookups. The method first checks the cache before querying the database, which will improve performance for repeated calls.
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java (2)
16-16:Details
❓ Verification inconclusive
Updated method signature to require organization context
Method now requires organizationId for user lookups, enforcing the requirement that users must be fetched with organization context.
Run the following script to verify all callers of this method have been updated:
🏁 Script executed:
#!/bin/bash # Find all references to the old method and verify they've been updated rg "findFirstByEmailIgnoreCaseOrderByCreatedAtDesc\s*\(\s*\w+\s*\)" --type javaLength of output: 81
Action Required: Confirm Caller Updates for Organization-based Lookups
The method signature in
app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.javanow requires an organization ID:Mono<User> findFirstByEmailIgnoreCaseAndOrganizationIdOrderByCreatedAtDesc(String email, String organizationId);Preliminary script searches for outdated references (
findFirstByEmailIgnoreCaseOrderByCreatedAtDesc()) yielded no results. However, due to the low-confidence output from the regex search, please verify manually that all callers now include the organization ID parameter.
18-18:Details
❓ Verification inconclusive
Updated bulk user lookup to include organization context
Method for fetching multiple users now requires organizationId, consistent with the pattern of making all user lookups organization-aware.
Run the following script to verify all callers of this method have been updated:
🏁 Script executed:
#!/bin/bash # Find all references to the old method and verify they've been updated rg "findAllByEmailIn\s*\(\s*\w+\s*\)" --type javaLength of output: 51
Bulk user lookup update verified – please manually confirm all references
The updated method for fetching multiple users now correctly requires an organizationId, making all user lookups organization-aware. A search for occurrences of the old method pattern (
findAllByEmailIn(...)) returned no results, which suggests that all callers have been updated. However, because the output was empty and might be affected by the search quality, please perform a manual verification of affected call sites to ensure no legacy usage remains.app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/AuthenticationSuccessHandlerCE.java (2)
78-81: User retrieval logic has been enhanced with organization context.The implementation now properly scopes user lookup by organization ID in the
isVerificationRequiredmethod, which is a more secure approach.
373-375: User retrieval logic has been correctly updated with organization context.The changes in
createDefaultApplicationmethod maintain consistency with the organization-aware user retrieval pattern implemented throughout the codebase.app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOidcUserServiceCEImpl.java (4)
8-8: Import added for UserOrganizationHelper.This import supports the organization-aware user retrieval implementation.
32-32: New dependency field added for UserOrganizationHelper.The private field is correctly declared to store the helper instance.
35-40: Constructor updated to include UserOrganizationHelper.The constructor signature has been properly updated to include the new dependency and initialize the field.
88-95: User lookup now scoped to organization context.The
findByUsernamemethod has been updated to retrieve users within their organization context, which enhances security by preventing cross-organization user access.app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java (4)
515-516: Test updated to match new repository method signature.The mock setup correctly reflects the repository method change to include organization ID.
545-546: Test correctly updated for organization-aware token retrieval.The mock is properly configured to use the new method signature with organization ID parameter.
559-560: Token verification test updated for organization context.The mock correctly uses the new method signature that includes organization ID.
591-592: Reset password tests updated to include organization context.Both mock configurations correctly use the new repository method that includes organization ID.
Also applies to: 603-604
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomFormLoginServiceCEImpl.java (4)
5-5: Import added for UserOrganizationHelper.This import supports the organization-aware user retrieval implementation.
20-20: New dependency field added for UserOrganizationHelper.The private field is correctly declared to store the helper instance.
23-26: Constructor updated to include UserOrganizationHelper.The constructor signature has been properly updated to include the new dependency and initialize the field.
37-44: User lookup now scoped to organization context.The implementation now properly retrieves users within their organization context, first trying an exact match and then falling back to a case-insensitive search if needed. This enhances security by preventing cross-organization user access.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOAuth2UserServiceImpl.java (2)
3-4: Import added correctly for UserOrganizationHelper.The import for UserOrganizationHelper has been added correctly to support the constructor parameter change.
23-25: Constructor properly updated with new parameter.The constructor signature has been updated to include UserOrganizationHelper as required by the new implementation.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/CustomOidcUserServiceImpl.java (2)
3-4: Import added correctly for UserOrganizationHelper.The import for UserOrganizationHelper has been added correctly to support the constructor parameter change.
24-26: Constructor properly updated with new parameter.The constructor signature has been updated to include UserOrganizationHelper and correctly passes it to the superclass.
app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java (6)
3-5: Import added correctly for UserOrganizationHelper.The import for UserOrganizationHelper has been added correctly to support the mock bean.
18-18: Import added for Mockito any matcher.The import for Mockito any() matcher has been added correctly to support the updated repository method calls that now include organization ID.
27-28: Mock bean added for UserOrganizationHelper.The UserOrganizationHelper has been correctly added as a mock bean for testing.
32-32: Test setup updated to reflect constructor changes.The test setup has been correctly updated to pass the mocked UserOrganizationHelper to the CustomFormLoginServiceImpl constructor.
39-40: Repository method call updated to include organization ID.The repository method call has been updated to use the new method signature that includes organization ID, with the any() matcher for flexibility in testing.
55-56: Repository method call updated to include organization ID.The repository method call in the second test has been updated to use the new method signature that includes organization ID.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/OrganizationServiceImpl.java (3)
5-5: Import added correctly for UserOrganizationHelper.The import for UserOrganizationHelper has been added correctly to support the constructor parameter change.
27-29: Constructor parameter list updated correctly.The constructor parameter list has been extended to include UserOrganizationHelper as the last parameter.
38-39: Super constructor call updated correctly.The call to the superclass constructor has been updated to include the userOrganizationHelper parameter.
app/server/appsmith-server/src/main/java/com/appsmith/server/authentication/handlers/ce/CustomOAuth2UserServiceCEImpl.java (5)
8-8: Import addition looks good.
30-30: New helper field declaration is fine.
33-34: Constructor parameter inclusion is logical.
37-37: Field assignment in constructor is valid.
84-89: Organization-based user lookup is consistent with the PR goal.app/server/appsmith-server/src/test/java/com/appsmith/server/repositories/UserRepositoryTest.java (4)
31-31: Use of random UUID for ORG_ID is good for isolating test data.
85-86: Including organizationId for multiple user variants ensures thorough coverage.Also applies to: 91-92, 95-96
109-110: Additional checks with different emails confirm org-based queries.Also applies to: 113-114, 117-118, 121-126
143-144: Org ID assignment and updated findAll with org constraints are consistent with new logic.Also applies to: 148-148
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/OrganizationServiceCEImpl.java (5)
17-17: Import for UserOrganizationHelper is aligned with usage in this service.
55-56: Storing UserOrganizationHelper as a final field is appropriate.
67-67: Constructor injection of the helper is consistent with Spring best practices.
75-75: Assigning the helper field in the constructor is straightforward.
80-80: Delegating organization ID retrieval to userOrganizationHelper is consistent and clean.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (10)
157-159: Scoped findByEmail now uses organization context.
163-164: New findByEmailAndOrganizationId method adds clarity.
192-214: Org-based forgot-password token flow is a good security enhancement.
293-295: Password reset token verification is correctly tied to organization.
317-320: Fetching current organization before password reset ensures correct scoping.
323-324: Chaining org data to password reset lookup is consistent.
370-397: Removal of no-org user lookups is in line with the PR’s intent to require organization context.
461-465: User creation check now properly queries by organization.
746-750: Retrieving multiple users by email & org ID ensures correct filtering.
772-779: Email-verification flow now scoped to user’s organization.
|
|
||
| private UserRepository repository; | ||
| private UserService userService; | ||
| private UserOrganizationHelper userOrganizationHelper; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Private field added but not initialized in constructor.
The private field userOrganizationHelper has been added but isn't being initialized in the constructor. Since it's passed to the superclass, it should also be assigned to the field.
public CustomOidcUserServiceImpl(
UserRepository repository, UserService userService, UserOrganizationHelper userOrganizationHelper) {
super(repository, userService, userOrganizationHelper);
this.repository = repository;
this.userService = userService;
+ this.userOrganizationHelper = userOrganizationHelper;
}📝 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.
| private UserOrganizationHelper userOrganizationHelper; | |
| public CustomOidcUserServiceImpl( | |
| UserRepository repository, UserService userService, UserOrganizationHelper userOrganizationHelper) { | |
| super(repository, userService, userOrganizationHelper); | |
| this.repository = repository; | |
| this.userService = userService; | |
| this.userOrganizationHelper = userOrganizationHelper; | |
| } |
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (6)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java(4 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java(6 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java(2 hunks)app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java(9 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (17)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java (6)
69-69: Added import for Mockito's any() matcherThe addition of this import is necessary to support the updated method signatures that now require an additional organizationId parameter.
515-516: Method signature updated to include organizationIdThe change from
findByEmail(testEmail)tofindByEmailAndOrganizationId(testEmail, any())correctly updates the test to match the new repository method signature that requires an organizationId.
545-546: Updated method to include organizationId parameterThe passwordResetTokenRepository mock setup properly uses the new method signature that requires an organizationId parameter.
559-560: Added organizationId parameter to test methodThis change correctly updates the test helper method to use the new parameter requirements.
591-592: Updated repository mock to include organizationId parameterThe mock correctly uses the new method signature that requires the organizationId parameter.
603-604: Updated method signature to include organizationIdThis change adapts the test to the updated repository method that now requires an organizationId parameter.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (6)
191-219: Properly integrated organization context in forgotPasswordTokenGenerateThe implementation now correctly retrieves the current user's organization ID and uses it to find users, ensuring password reset functionality operates within organizational boundaries.
292-295: Organization-specific token verification enhances securityThe token verification logic now includes organization ID checks, preventing cross-organization token validation attempts.
317-324: Organization context properly integrated in resetPasswordAfterForgotPasswordOrganization context is now correctly retrieved before validating the password reset token, maintaining consistent organization-level security boundaries.
461-466: Organization context added to user creation processThe implementation now correctly checks for existing users within the same organization, preventing duplicate user records across organization boundaries.
747-749: Organization filtering added to getAllByEmails methodThe method now properly scopes user lookups to the current organization context.
772-778: Email verification now respects organization boundariesThe implementation properly includes organization context when finding users for email verification, ensuring operations remain within organizational boundaries.
app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java (5)
4-4: New import for UserOrganizationHelper added.This import supports the new architecture requiring organization context for user authentication.
18-18: Added import for Mockito's any() matcher.This import is needed for the updated repository method calls that now include organization ID parameter.
27-29: Added mock for UserOrganizationHelper.The
@MockBeanannotation correctly sets up the mock for integration with the service.
39-40: Updated repository method to include organization ID parameter.This change aligns with the PR objective to require organization context when fetching users.
50-71:Details
❌ Incorrect review comment
The second test method has undefined variables and structural issues.
The variables
userandsampleEmail2are referenced but not defined in the visible code. Additionally, there are structural issues with duplicate mock setups and an incomplete StepVerifier statement on line 58.
🏁 Script executed:
#!/bin/bash # Check if there are definitions for user and sampleEmail2 variables in the test class grep -A 10 "private User user" app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.java grep -A 3 "sampleEmail2" app/server/appsmith-server/src/test/java/com/appsmith/server/authentication/handlers/CustomFormLoginServiceImplUnitTest.javaLength of output: 1413
Attention: Clarification on Test Method Concerns
After verification, it turns out:
- Variables Definition: Both
sampleEmail2anduserare defined inline within the test method.- Test Structure: The mock setups are not duplicated, and the
StepVerifierchain correctly concludes with.verifyComplete().Please disregard the initial remarks about undefined variables and incomplete or duplicated setups.
Likely an incorrect or invalid review comment.
| public void setUp() { | ||
| reactiveUserDetailsService = new CustomFormLoginServiceImpl(repository); | ||
| reactiveUserDetailsService = new CustomFormLoginServiceImpl(repository, userOrganizationHelper); | ||
| } |
There was a problem hiding this comment.
Updated service constructor to include UserOrganizationHelper.
The repository field appears to be null at this point as there's no initialization visible in the provided code.
- reactiveUserDetailsService = new CustomFormLoginServiceImpl(repository, userOrganizationHelper);
+ repository = Mockito.mock(UserRepository.class);
+ reactiveUserDetailsService = new CustomFormLoginServiceImpl(repository, userOrganizationHelper);Committable suggestion skipped: line range outside the PR's diff.
| user.setEmail(sampleEmail2.toLowerCase()); | ||
|
|
There was a problem hiding this comment.
Duplicate mock configuration in second test.
There's a duplicate mock setup for the same method at lines 61-63. Also, line 58 appears to be an incomplete statement.
- Mockito.when(repository.findFirstByEmailIgnoreCaseAndOrganizationIdOrderByCreatedAtDesc(sampleEmail2, any()))
- .thenReturn(Mono.just(user));
-
- StepVerifier.create(reactiveUserDetailsService.findByUsername(sampleEmail2))📝 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.
| user.setEmail(sampleEmail2.toLowerCase()); | |
| user.setEmail(sampleEmail2.toLowerCase()); |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java (1)
153-155: Retrieve organizationId using UserOrganizationHelper.The implementation correctly retrieves the organization ID. Note that the use of
.block()is acceptable in test code, but would be an anti-pattern in production reactive code.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceWithDisabledSignupTest.java (3)
10-10: New import for UserOrganizationHelper.Clean addition of the necessary import statement for the new helper class.
51-53: Added UserOrganizationHelper as autowired dependency.Good implementation of dependency injection for the new helper class. This follows Spring best practices for test classes.
159-159: Set organizationId on new user.This change properly implements the requirement to associate users with an organization ID, which aligns with the PR objective of removing support for users without an organization.
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (4)
192-220: Minor grammar correction and fallback logic check.
When describing the user with the provided email address, consider changing "an user" to "a user" to ensure correct grammar.
Also, you are performing a fallback search with case-insensitive logic. You may want to unify this logic across the codebase for consistency.
370-374: Duplicate token retrieval logic.
This code closely resembles the logic at lines 192-220 for retrieving password reset tokens. Consider extracting this into a helper method to avoid repetition.
461-466: Unify repeated user retrieval logic.
The fallback search for ignoring case repeats patterns used elsewhere. Consider adding a utility method to consistently handle user retrieval by email and organization ID.
772-778: Repetitive fallback logic in user retrieval.
This approach replicates similar sequences from lines 192-220 and lines 461-466. Factor out a shared helper method to reduce duplication.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(7 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(2 hunks)app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: perform-test / client-build / client-build
- GitHub Check: perform-test / rts-build / build
- GitHub Check: perform-test / server-build / server-unit-tests
- GitHub Check: server-unit-tests / server-unit-tests
- GitHub Check: server-spotless / spotless-check
🔇 Additional comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java (3)
319-320: Caching the organization context may store errors.
Because.cache()is used, if the first subscription fails to retrieve the organization, subsequent subscriptions won't retry. Ensure this behavior is desired.
322-324: Looks good with the new organization-based lookup.
This approach is consistent with the rest of the changes ensuring the token retrieval is organization-specific.
747-749: Consistent organization-based user lookup.
Glad to see theorganizationIdbeing used to ensure user retrieval is bounded by an organization. Keep an eye out for scenarios needing case-insensitivity to remain uniform.
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java
Show resolved
Hide resolved
Failed server tests
|
| } | ||
|
|
||
| @Override | ||
| public Mono<User> findByEmailAndOrganizationId(String email, String organizationId) { |
There was a problem hiding this comment.
Don't we want to introduce the permission check here considering we are swithing the unique constraints to email + orgId.
There was a problem hiding this comment.
Oh, this wasn't being used at all in the code. So, jsut removed it. The day we need it, we can add it back with organizationId also as a parameter.
| this.defaultOrganizationId = organizationId; | ||
| return organizationId; | ||
| }); | ||
| return userOrganizationHelper.getCurrentUserOrganizationId(); |
There was a problem hiding this comment.
Should we also do this refactor in CacheableRepositoryHelper?
|
All the cypress tests and server unit tests passed before adding password reset token migration. Removing the label to trigger the cypress tests now since it wouldnt be required post the migration. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration068_AddOrganizationIdToPasswordResetToken.java (3)
26-27: Consider documenting empty rollback.The empty rollback implementation means this migration cannot be undone. While this is generally acceptable for additive changes, it's worth documenting this limitation with a comment for future maintainers.
@RollbackExecution -public void rollbackExecution() {} +public void rollbackExecution() { + // No rollback implemented as this migration only adds data +}
45-49: Consider adding batching for large collections.If there are a large number of password reset tokens, this operation could take significant time. For production systems with many users, consider implementing a batched approach to avoid long-running operations.
29-55: Consider adding transaction support for data consistency.For MongoDB 4.0+, consider wrapping the operation in a transaction to ensure atomicity, especially if this migration will be coordinated with other changes.
@Execution public void execute() { try { + // Start a session and transaction for atomicity + mongoTemplate.getMongoOperations().execute(client -> { + try (ClientSession session = client.startSession()) { + session.startTransaction(); + try { // Get the organization ID from the organization collection Document organization = mongoTemplate.findOne(new Query(), Document.class, "organization"); if (organization == null) { log.info("No organization found to migrate password reset tokens"); return; } String organizationId = organization.getObjectId("_id").toString(); // Update all password reset tokens that don't have an organizationId Query query = new Query(where("organizationId").exists(false)); List<Document> pipeline = Arrays.asList(new Document("$set", new Document("organizationId", organizationId))); long updatedCount = mongoTemplate .getCollection("passwordResetToken") .updateMany(query.getQueryObject(), pipeline) .getModifiedCount(); log.info("Successfully set organizationId for {} password reset token documents", updatedCount); + session.commitTransaction(); + } catch (Exception e) { + session.abortTransaction(); + throw e; + } + return null; + } + }); } catch (Exception e) { log.error("Error while setting organizationId for password reset tokens: ", e); throw e; } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro (Legacy)
📒 Files selected for processing (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration068_AddOrganizationIdToPasswordResetToken.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: server-spotless / spotless-check
- GitHub Check: server-unit-tests / server-unit-tests
🔇 Additional comments (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/migrations/db/ce/Migration068_AddOrganizationIdToPasswordResetToken.java (4)
16-18: Well-defined migration class with clear purpose.The class follows proper Mongock conventions with appropriate annotations and naming. The migration is appropriately ordered at "068".
41-44: LGTM: Query and update pipeline are correctly structured.The query properly identifies documents without an organizationId field, and the update pipeline correctly sets the field value.
50-54: Good error handling and logging practices.The code properly logs both success information and detailed error cases, making debugging easier.
32-38:Details
❓ Verification inconclusive
Verify behavior with multiple organizations.
The code retrieves a single organization without any filtering criteria. If there are multiple organizations in the database, it will use whichever one is returned first. Consider if this is the intended behavior or if a specific organization should be targeted.
🏁 Script executed:
#!/bin/bash # Check if there are multiple organizations in the database # This would help determine if the current approach is appropriate # Count organizations in the collection echo "Checking for multiple organizations..." find_orgs="db.getCollection('organization').count()" echo "MongoDB query that would be executed: $find_orgs" # Check if there's a specific primary/default organization flag that should be used echo "Checking for organization schema to see if there's a primary flag..." find_org_schema="db.getCollection('organization').findOne()" echo "MongoDB query that would be executed: $find_org_schema"Length of output: 716
Action Required: Clarify Multi-Organization Behavior
The current migration code retrieves an organization with an unfiltered
findOnecall. This means that if multiple organizations exist in the database, the migration will arbitrarily use the first one returned. Please verify whether this behavior is intentional. If multiple organizations are expected, consider explicitly selecting the primary or default organization—or add documentation to clarify why the first result is acceptable.
## Description > [!TIP] > _Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team)._ > > _Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR._ Fixes #`Issue Number` _or_ Fixes `Issue URL` > [!WARNING] > _If no issue exists, please create an issue first, and check with the maintainers if the issue is valid._ ## Automation /test all ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!WARNING] > Tests have not run on the HEAD f0cb554 yet > <hr>Wed, 12 Mar 2025 13:55:54 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit - **New Features** - Enhanced user authentication: Login, social sign-in, password reset, and email verification processes now use organization-specific context to improve security and accuracy. - Added organization ID to password reset tokens for better context integrity. - **Refactor** - Internal workflows have been streamlined to ensure that all user-related operations reliably respect the organization context, reducing potential cross-organization issues. - Introduction of `UserOrganizationHelper` for managing organization-specific logic across user services. - **Bug Fixes** - Updated user retrieval methods to ensure they are scoped to the correct organization, enhancing accuracy in user-related operations. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
2 participants
Description
Tip
Add a TL;DR when the description is longer than 500 words or extremely technical (helps the content, marketing, and DevRel team).
Please also include relevant motivation and context. List any dependencies that are required for this change. Add links to Notion, Figma or any other documents that might be relevant to the PR.
Fixes #
Issue Numberor
Fixes
Issue URLWarning
If no issue exists, please create an issue first, and check with the maintainers if the issue is valid.
Automation
/test all
🔍 Cypress test results
Warning
Tests have not run on the HEAD f0cb554 yet
Wed, 12 Mar 2025 13:55:54 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Refactor
UserOrganizationHelperfor managing organization-specific logic across user services.Bug Fixes