From 933e968d3491ba54a3d7a208eaa328dd26408d5e Mon Sep 17 00:00:00 2001 From: Trisha Anand Date: Tue, 8 Apr 2025 15:59:38 +0530 Subject: [PATCH] chore: Removing user repo fetches only by email (#40165) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## 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 sanity ### :mag: Cypress test results > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: > Commit: ea2dba000d77e0b8cd7a46ba2259a076bfb4086e > Cypress dashboard. > Tags: `@tag.Sanity` > Spec: >
Tue, 08 Apr 2025 10:21:54 UTC ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [ ] No ## Summary by CodeRabbit - **Refactor** - Streamlined user account and session management workflows by integrating organization-specific checks into processes like password resets, invitations, and general user retrieval. - Phased out older lookup methods in favor of a more robust, organization-aware approach that enhances both accuracy and security. --- .../repositories/ce/UserRepositoryCE.java | 1 + .../services/ce/SessionUserServiceCEImpl.java | 4 +- .../server/services/ce/UserServiceCEImpl.java | 120 +++++++++--------- .../UserAndAccessManagementServiceImpl.java | 4 +- .../UserAndAccessManagementServiceCEImpl.java | 12 +- ...cessManagementServiceCECompatibleImpl.java | 4 + 6 files changed, 80 insertions(+), 65 deletions(-) diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java index 54fa6ec92c..94177d38cc 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/repositories/ce/UserRepositoryCE.java @@ -11,6 +11,7 @@ import java.util.Set; public interface UserRepositoryCE extends BaseRepository, CustomUserRepository { + @Deprecated Mono findByEmail(String email); Mono findFirstByEmailIgnoreCaseAndOrganizationIdOrderByCreatedAtDesc(String email, String organizationId); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SessionUserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SessionUserServiceCEImpl.java index e004fc9497..5a5d6babf4 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SessionUserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/SessionUserServiceCEImpl.java @@ -44,7 +44,9 @@ public class SessionUserServiceCEImpl implements SessionUserServiceCE { @Override public Mono refreshCurrentUser(ServerWebExchange exchange) { return Mono.zip( - getCurrentUser().map(User::getEmail).flatMap(userRepository::findByEmail), + getCurrentUser() + .flatMap(sessionUser -> userRepository.findByEmailAndOrganizationId( + sessionUser.getEmail(), sessionUser.getOrganizationId())), ReactiveSecurityContextHolder.getContext(), exchange.getSession()) .flatMap(tuple -> { diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java index 6c96296822..e0baf16049 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/UserServiceCEImpl.java @@ -324,6 +324,8 @@ public class UserServiceCEImpl extends BaseService .switchIfEmpty(Mono.error(new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, USER, ORGANIZATION))) .cache(); + Mono orgIdMono = organizationMono.map(Organization::getId); + return organizationMono .flatMap(organization -> passwordResetTokenRepository.findByEmailAndOrganizationId( emailTokenDTO.getEmail(), organization.getId())) @@ -337,69 +339,69 @@ public class UserServiceCEImpl extends BaseService return emailTokenDTO.getEmail(); } }) - .flatMap(emailAddress -> repository - .findByEmail(emailAddress) - .switchIfEmpty(Mono.error( - new AppsmithException(AppsmithError.NO_RESOURCE_FOUND, FieldName.USER, emailAddress))) - .zipWith(organizationMono) - .flatMap(tuple -> { - User userFromDb = tuple.getT1(); - OrganizationConfiguration organizationConfiguration = - tuple.getT2().getOrganizationConfiguration(); - boolean isStrongPasswordPolicyEnabled = organizationConfiguration != null - && Boolean.TRUE.equals( - organizationConfiguration.getIsStrongPasswordPolicyEnabled()); + .zipWith(orgIdMono) + .flatMap(tuple -> { + String emailAddress = tuple.getT1(); + String orgId = tuple.getT2(); + return repository + .findByEmailAndOrganizationId(emailAddress, orgId) + .switchIfEmpty(Mono.error(new AppsmithException( + AppsmithError.NO_RESOURCE_FOUND, FieldName.USER, emailAddress))); + }) + .zipWith(organizationMono) + .flatMap(tuple -> { + User userFromDb = tuple.getT1(); + OrganizationConfiguration organizationConfiguration = + tuple.getT2().getOrganizationConfiguration(); + boolean isStrongPasswordPolicyEnabled = organizationConfiguration != null + && Boolean.TRUE.equals(organizationConfiguration.getIsStrongPasswordPolicyEnabled()); - if (!validateUserPassword(user.getPassword(), isStrongPasswordPolicyEnabled)) { - return isStrongPasswordPolicyEnabled - ? Mono.error(new AppsmithException( - AppsmithError.INSUFFICIENT_PASSWORD_STRENGTH, - LOGIN_PASSWORD_MIN_LENGTH, - LOGIN_PASSWORD_MAX_LENGTH)) - : Mono.error(new AppsmithException( - AppsmithError.INVALID_PASSWORD_LENGTH, - LOGIN_PASSWORD_MIN_LENGTH, - LOGIN_PASSWORD_MAX_LENGTH)); - } + if (!validateUserPassword(user.getPassword(), isStrongPasswordPolicyEnabled)) { + return isStrongPasswordPolicyEnabled + ? Mono.error(new AppsmithException( + AppsmithError.INSUFFICIENT_PASSWORD_STRENGTH, + LOGIN_PASSWORD_MIN_LENGTH, + LOGIN_PASSWORD_MAX_LENGTH)) + : Mono.error(new AppsmithException( + AppsmithError.INVALID_PASSWORD_LENGTH, + LOGIN_PASSWORD_MIN_LENGTH, + LOGIN_PASSWORD_MAX_LENGTH)); + } - // User has verified via the forgot password token verfication route. Allow the user to set - // new password. - userFromDb.setPasswordResetInitiated(false); - userFromDb.setPassword(passwordEncoder.encode(user.getPassword())); + // User has verified via the forgot password token verfication route. Allow the user to set + // new password. + userFromDb.setPasswordResetInitiated(false); + userFromDb.setPassword(passwordEncoder.encode(user.getPassword())); - // If the user has been invited but has not signed up yet, and is following the route of - // reset - // password flow to set up their password, enable the user's account as well - userFromDb.setIsEnabled(true); + // If the user has been invited but has not signed up yet, and is following the route of + // reset + // password flow to set up their password, enable the user's account as well + userFromDb.setIsEnabled(true); - return organizationService - .getCurrentUserOrganizationId() - .flatMap( - organizationId -> passwordResetTokenRepository.findByEmailAndOrganizationId( - userFromDb.getEmail(), organizationId)) - .switchIfEmpty(Mono.error(new AppsmithException( - AppsmithError.NO_RESOURCE_FOUND, - FieldName.TOKEN, - emailTokenDTO.getToken()))) - .flatMap(passwordResetTokenRepository::delete) - .then(repository.save(userFromDb)) - .doOnSuccess(result -> { - // In a separate thread, we delete all other sessions of this user. - sessionUserService - .logoutAllSessions(userFromDb.getEmail()) - .subscribeOn(Schedulers.boundedElastic()) - .subscribe(); + return organizationService + .getCurrentUserOrganizationId() + .flatMap(organizationId -> passwordResetTokenRepository.findByEmailAndOrganizationId( + userFromDb.getEmail(), organizationId)) + .switchIfEmpty(Mono.error(new AppsmithException( + AppsmithError.NO_RESOURCE_FOUND, FieldName.TOKEN, emailTokenDTO.getToken()))) + .flatMap(passwordResetTokenRepository::delete) + .then(repository.save(userFromDb)) + .doOnSuccess(result -> { + // In a separate thread, we delete all other sessions of this user. + sessionUserService + .logoutAllSessions(userFromDb.getEmail()) + .subscribeOn(Schedulers.boundedElastic()) + .subscribe(); - // we reset the counter for user's login attempts once password is reset - rateLimitService - .resetCounter( - RateLimitConstants.BUCKET_KEY_FOR_LOGIN_API, - userFromDb.getEmail()) - .subscribeOn(Schedulers.boundedElastic()) - .subscribe(); - }) - .thenReturn(true); - })); + // we reset the counter for user's login attempts once password is reset + rateLimitService + .resetCounter( + RateLimitConstants.BUCKET_KEY_FOR_LOGIN_API, userFromDb.getEmail()) + .subscribeOn(Schedulers.boundedElastic()) + .subscribe(); + }) + .thenReturn(true); + }); } @Override @@ -696,7 +698,7 @@ public class UserServiceCEImpl extends BaseService .flatMap(user -> updateWithoutPermission(user.getId(), updates) .then( exchange == null - ? repository.findByEmail(user.getEmail()) + ? findByEmail(user.getEmail()) : sessionUserService.refreshCurrentUser(exchange))) .cache(); monos.add(updatedUserMono.then()); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/UserAndAccessManagementServiceImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/UserAndAccessManagementServiceImpl.java index 7f02370b77..0882fbf3ab 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/UserAndAccessManagementServiceImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/UserAndAccessManagementServiceImpl.java @@ -1,6 +1,7 @@ package com.appsmith.server.solutions; import com.appsmith.server.configurations.CommonConfig; +import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.services.AnalyticsService; import com.appsmith.server.services.CaptchaService; @@ -28,8 +29,8 @@ public class UserAndAccessManagementServiceImpl extends UserAndAccessManagementS PermissionGroupPermission permissionGroupPermission, EmailService emailService, CommonConfig commonConfig, + UserOrganizationHelper userOrganizationHelper, CaptchaService captchaService) { - super( sessionUserService, permissionGroupService, @@ -40,6 +41,7 @@ public class UserAndAccessManagementServiceImpl extends UserAndAccessManagementS permissionGroupPermission, emailService, commonConfig, + userOrganizationHelper, captchaService); } } diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java index e17a05c101..bb76f9bd68 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserAndAccessManagementServiceCEImpl.java @@ -9,6 +9,7 @@ import com.appsmith.server.domains.Workspace; import com.appsmith.server.dtos.InviteUsersDTO; import com.appsmith.server.exceptions.AppsmithError; import com.appsmith.server.exceptions.AppsmithException; +import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.helpers.ValidationUtils; import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.services.AnalyticsService; @@ -47,7 +48,7 @@ public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManage private final UserService userService; private final PermissionGroupPermission permissionGroupPermission; private final EmailService emailService; - private final CommonConfig commonConfig; + private final UserOrganizationHelper userOrganizationHelper; private final CaptchaService captchaService; @@ -61,6 +62,7 @@ public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManage PermissionGroupPermission permissionGroupPermission, EmailService emailService, CommonConfig commonConfig, + UserOrganizationHelper userOrganizationHelper, CaptchaService captchaService) { this.sessionUserService = sessionUserService; @@ -71,7 +73,7 @@ public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManage this.userService = userService; this.emailService = emailService; this.permissionGroupPermission = permissionGroupPermission; - this.commonConfig = commonConfig; + this.userOrganizationHelper = userOrganizationHelper; this.captchaService = captchaService; } @@ -160,8 +162,10 @@ public class UserAndAccessManagementServiceCEImpl implements UserAndAccessManage eventData.put(FieldName.WORKSPACE, workspace); List defaultPermissionGroups = tuple.getT3(); - Mono getUserFromDbAndCheckIfUserExists = userRepository - .findByEmail(username) + Mono getUserFromDbAndCheckIfUserExists = userOrganizationHelper + .getCurrentUserOrganizationId() + .flatMap(organizationId -> + userRepository.findByEmailAndOrganizationId(username, organizationId)) .flatMap(user -> throwErrorIfUserAlreadyExistsInWorkspace(user, defaultPermissionGroups) .thenReturn(user)); diff --git a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce_compatible/UserAndAccessManagementServiceCECompatibleImpl.java b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce_compatible/UserAndAccessManagementServiceCECompatibleImpl.java index ed2f1058c6..eadb3cf4f2 100644 --- a/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce_compatible/UserAndAccessManagementServiceCECompatibleImpl.java +++ b/app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce_compatible/UserAndAccessManagementServiceCECompatibleImpl.java @@ -1,6 +1,7 @@ package com.appsmith.server.solutions.ce_compatible; import com.appsmith.server.configurations.CommonConfig; +import com.appsmith.server.helpers.UserOrganizationHelper; import com.appsmith.server.repositories.UserRepository; import com.appsmith.server.services.AnalyticsService; import com.appsmith.server.services.CaptchaService; @@ -16,6 +17,7 @@ import org.springframework.stereotype.Component; @Component public class UserAndAccessManagementServiceCECompatibleImpl extends UserAndAccessManagementServiceCEImpl implements UserAndAccessManagementServiceCECompatible { + public UserAndAccessManagementServiceCECompatibleImpl( SessionUserService sessionUserService, PermissionGroupService permissionGroupService, @@ -26,6 +28,7 @@ public class UserAndAccessManagementServiceCECompatibleImpl extends UserAndAcces PermissionGroupPermission permissionGroupPermission, EmailService emailService, CommonConfig commonConfig, + UserOrganizationHelper userOrganizationHelper, CaptchaService captchaService) { super( sessionUserService, @@ -37,6 +40,7 @@ public class UserAndAccessManagementServiceCECompatibleImpl extends UserAndAcces permissionGroupPermission, emailService, commonConfig, + userOrganizationHelper, captchaService); } }