Adding Salesforce User Disconnect API #76
Conversation
WalkthroughThe changes primarily focus on adding a new feature to disconnect a user from Salesforce CRM. This includes the creation of new classes, methods, and tests to handle the disconnection process. The code also introduces caching in repositories, error handling, and logging in DynamoDB operations. Additionally, there are minor refactoring and formatting changes. Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 20
Files ignored due to filter (6)
- src/test/resources/data/functional/controllers/authController/Disconnect.scenarios.json
- src/test/resources/data/functional/controllers/authController/SalesforceConnect.scenarios.json
- src/test/resources/fixtures/common/repositories/salesforceOauthToken/ActiveSalesforceOauthToken.json
- src/test/resources/fixtures/common/repositories/salesforceUser/DeletedSalesforceUser.json
- src/test/resources/fixtures/functional/controllers/authController/PostDisconnectFixture.json
- src/test/resources/fixtures/functional/controllers/authController/PostSalesforceConnectFixture.json
Files selected for processing (19)
- src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java (3 hunks)
- src/main/java/com/salessparrow/api/controllers/AuthController.java (3 hunks)
- src/main/java/com/salessparrow/api/lib/AwsKms.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/UserLoginCookieAuth.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectSalesforceUser.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUser.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUserFactory.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/globalConstants/SalesforceConstants.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/httpLib/HttpClient.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java (2 hunks)
- src/main/java/com/salessparrow/api/repositories/SalesforceOauthTokenRepository.java (3 hunks)
- src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java (4 hunks)
- src/main/java/com/salessparrow/api/services/auth/DisconnectUserService.java (1 hunks)
- src/main/java/com/salessparrow/api/services/salesforce/AuthService.java (8 hunks)
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostDisconnectTest.java (1 hunks)
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostLogoutTest.java (5 hunks)
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostSalesforceConnectTest.java (11 hunks)
- src/test/java/com/salessparrow/api/helper/Common.java (1 hunks)
- src/test/java/com/salessparrow/api/unit/lib/salesforce/wrappers/SalesforceTokensTest.java (1 hunks)
Files skipped from review due to trivial changes (7)
- src/main/java/com/salessparrow/api/lib/AwsKms.java
- src/main/java/com/salessparrow/api/lib/UserLoginCookieAuth.java
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUser.java
- src/main/java/com/salessparrow/api/lib/globalConstants/SalesforceConstants.java
- src/main/java/com/salessparrow/api/lib/httpLib/HttpClient.java
- src/main/java/com/salessparrow/api/repositories/SalesforceOauthTokenRepository.java
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostLogoutTest.java
Additional comments (Suppressed): 14
src/main/java/com/salessparrow/api/controllers/AuthController.java (2)
22-23: The
DisconnectUserServiceis imported but the old code does not show any previous import of this service. Ensure that this service has been properly implemented and tested before integrating it into theAuthController.42-43: The
DisconnectUserServiceis being autowired here. Make sure that this service is correctly annotated with@Serviceor@Componentin its implementation to ensure Spring's Dependency Injection works as expected.src/test/java/com/salessparrow/api/helper/Common.java (1)
- 58-74: The
loadScenariosDatamethod has been updated to accept a key parameter. This change allows the method to return specific scenario data based on the provided key, which improves the flexibility of test data loading. However, ensure that all calls to this function throughout the codebase have been updated to match the new signature.src/test/java/com/salessparrow/api/functional/controllers/authController/PostSalesforceConnectTest.java (9)
14-18: The import of
java.util.HashMapandjava.util.Maphas been removed. Ensure that these classes are not used elsewhere in the code, otherwise it will result in a compilation error.36-42: The class
SalesforceGetTokenshas been replaced withSalesforceTokens. Make sure that the methods and functionalities provided bySalesforceGetTokensare available inSalesforceTokensto avoid any runtime errors.49-50: The
ResourceLoaderbean is no longer autowired. If this bean was used in the previous version of the code, ensure that its removal does not affect the functionality of the test.62-68: The mock for
SalesforceGetTokenshas been replaced with a mock forSalesforceTokens. Ensure that all the methods invoked on this mock are available inSalesforceTokens.89-96: The method
loadTestData()has been replaced withcommon.loadScenariosData(). Verify that this new method provides the same functionality as the old one.102-111: The mocks for
mockGetTokens.getTokens()andmockGetIdentity.getUserIdentity()have been set up. Ensure that these mocks return the expected data for the tests.137-144: The method
loadTestData()has been replaced withcommon.loadScenariosData(). Verify that this new method provides the same functionality as the old one.147-154: The mock for
mockGetTokens.getTokens()has been set up. Ensure that this mock returns the expected data for the tests.175-217: A new test method
testPostSalesforceConnectDisconnectedUserSignup()has been added. This method seems to be testing the signup functionality for a disconnected Salesforce user. The test setup and assertions look correct.src/main/java/com/salessparrow/api/services/salesforce/AuthService.java (2)
28-28: The class
SalesforceTokenshas been renamed fromSalesforceGetTokens. Ensure that all references to this class throughout the codebase have been updated to match the new name.116-116: The method
getTokensof theSalesforceTokensclass is being called. Make sure that this method still exists in the renamed class and its functionality remains the same.
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUserFactory.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectSalesforceUser.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/services/salesforce/AuthService.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/services/salesforce/AuthService.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 4
Files selected for processing (4)
- src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java (3 hunks)
- src/main/java/com/salessparrow/api/repositories/SalesforceOauthTokenRepository.java (2 hunks)
- src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java (3 hunks)
- src/main/java/com/salessparrow/api/services/salesforce/AuthService.java (6 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/salessparrow/api/services/salesforce/AuthService.java
Additional comments (Suppressed): 4
src/main/java/com/salessparrow/api/repositories/SalesforceOauthTokenRepository.java (3)
22-24: The constructor is unchanged and correctly injects the
DynamoDBMapperdependency.83-89: Exception handling remains consistent with the previous implementation, wrapping any caught exceptions in a
CustomException.92-110: A new method
deleteSalesforceOauthTokenBySalesforceOauthTokenhas been added to delete a Salesforce OAuth token from the DynamoDB table. The method uses thedynamoDBMapper.delete()function to perform the deletion operation. It also includes exception handling similar to other methods in the class. However, it's important to ensure that this method is used carefully as it directly deletes the record from the database based on the providedSalesforceOauthTokenobject.Also, the method is annotated with
@CacheEvict, which will remove the corresponding cache entry after the deletion operation. This is a good practice to keep the cache and database in sync.Please verify that all calls to this method are made with valid
SalesforceOauthTokenobjects and that the cache eviction works as expected.src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java (1)
- 93-93: The
@CacheEvictannotation is used correctly here to ensure that the cache is evicted for the specific user when their data is removed. This ensures that any subsequent calls to retrieve the user data will not return stale data from the cache.
src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 6
Files selected for processing (3)
- src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java (3 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectSalesforceUser.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java (2 hunks)
Additional comments (Suppressed): 1
src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java (1)
- 57-70: This newly added method
save(T object, DynamoDBMapperConfig config)provides the ability to save an object with a specific configuration. Ensure that this method is used appropriately throughout the codebase where specific configurations are required for saving objects.
src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 1
Files selected for processing (1)
- src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java (2 hunks)
Additional comments (Suppressed): 2
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java (2)
22-32: The class name has been changed from
SalesforceGetTokenstoSalesforceTokens, and a new method for revoking tokens has been added. Ensure that all references to this class and its methods have been updated throughout the codebase.56-64: This exception handling block is throwing a
CustomExceptionwith an error object that includes a list of identifiers. Make sure these identifiers are meaningful and documented somewhere so that when this exception is thrown, it can be traced back to the exact issue.
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Review Status
Actionable comments generated: 0
Files selected for processing (1)
- src/main/java/com/salessparrow/api/config/SecurityConfig.java (1 hunks)
Files skipped from review due to trivial changes (1)
- src/main/java/com/salessparrow/api/config/SecurityConfig.java
There was a problem hiding this comment.
Review Status
Actionable comments generated: 14
Files selected for processing (18)
- src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java (2 hunks)
- src/main/java/com/salessparrow/api/config/SecurityConfig.java (1 hunks)
- src/main/java/com/salessparrow/api/controllers/AuthController.java (3 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectSalesforceUser.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUser.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUserFactory.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/globalConstants/SalesforceConstants.java (1 hunks)
- src/main/java/com/salessparrow/api/lib/httpLib/HttpClient.java (2 hunks)
- src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java (2 hunks)
- src/main/java/com/salessparrow/api/repositories/SalesforceOauthTokenRepository.java (1 hunks)
- src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java (2 hunks)
- src/main/java/com/salessparrow/api/services/auth/DisconnectUserService.java (1 hunks)
- src/main/java/com/salessparrow/api/services/salesforce/AuthService.java (5 hunks)
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostDisconnectTest.java (1 hunks)
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostLogoutTest.java (5 hunks)
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostSalesforceConnectTest.java (8 hunks)
- src/test/java/com/salessparrow/api/helper/Common.java (2 hunks)
- src/test/java/com/salessparrow/api/unit/lib/salesforce/wrappers/SalesforceTokensTest.java (1 hunks)
Files skipped from review due to trivial changes (6)
- src/main/java/com/salessparrow/api/config/SecurityConfig.java
- src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectUser.java
- src/main/java/com/salessparrow/api/lib/globalConstants/SalesforceConstants.java
- src/main/java/com/salessparrow/api/lib/httpLib/HttpClient.java
- src/main/java/com/salessparrow/api/repositories/SalesforceOauthTokenRepository.java
- src/test/java/com/salessparrow/api/functional/controllers/authController/PostLogoutTest.java
Additional comments (Suppressed): 21
src/main/java/com/salessparrow/api/lib/salesforce/wrappers/SalesforceTokens.java (1)
- 22-32: The class
SalesforceTokenshas been renamed fromSalesforceGetTokens, and the method signature remains the same. Please verify that all references to this class and its methods have been updated accordingly throughout the codebase.src/main/java/com/salessparrow/api/config/DynamoDBConfiguration.java (1)
- 96-96: No issues found in this hunk. The configuration class ends correctly.
src/main/java/com/salessparrow/api/repositories/SalesforceUserRepository.java (1)
- 79-79: Ensure that the cache eviction is working as expected after the removal of Salesforce user data. The key used for cache eviction should match the one used in caching the Salesforce user data.
src/main/java/com/salessparrow/api/lib/crmActions/disconnectUser/DisconnectSalesforceUser.java (1)
- 58-59: Ensure that the
revokeTokensmethod handles any exceptions that may occur during the revocation process. Unhandled exceptions could cause the application to crash or behave unexpectedly.src/main/java/com/salessparrow/api/controllers/AuthController.java (2)
19-25: The import for
DisconnectUserServicehas been added correctly.39-47: The
DisconnectUserServicehas been autowired correctly. Ensure that the service is properly defined as a Spring Bean in the application context.src/test/java/com/salessparrow/api/helper/Common.java (2)
58-73: The new method
loadScenariosData(String location, String key)has been added to load test scenario data from a specific location and key. This method uses Jackson'sObjectMapperto read the JSON data from the resource file and returns a list ofScenarioobjects associated with the specified key.105-108: No significant changes have been made in these lines.
src/main/java/com/salessparrow/api/services/salesforce/AuthService.java (5)
28-28: The class
SalesforceGetTokenshas been renamed toSalesforceTokens. Ensure that all references to this class throughout the codebase have been updated to match the new name.79-79: The
salesforceGetTokensobject has been replaced withsalesforceTokens. Make sure that thegetTokensmethod in theSalesforceTokensclass behaves as expected and doesn't introduce any breaking changes.119-125: The
salesforceGetTokensobject has been replaced withsalesforceTokensin thefetchOauthTokensFromSalesforcemethod. Verify that thegetTokensmethod in theSalesforceTokensclass behaves as expected and doesn't introduce any breaking changes.209-219: The condition for checking if a Salesforce user is active has been added. This change seems to be improving the logic by considering the user status before proceeding.
245-267: The logic for creating a Salesforce user has been updated to handle cases where the user status is
DELETED. This change improves the handling of user data by considering different user statuses.src/test/java/com/salessparrow/api/functional/controllers/authController/PostSalesforceConnectTest.java (8)
14-18: The import of
java.util.HashMapandjava.util.Maphas been removed. Ensure that these imports are not used elsewhere in the code.21-32: The import of
org.springframework.core.io.Resourceandorg.springframework.core.io.ResourceLoaderhas been removed, and there is no new import added to replace them. Make sure that these classes are not used elsewhere in the code.36-42: The class
SalesforceGetTokenshas been replaced withSalesforceTokens. Verify that all instances ofSalesforceGetTokenshave been replaced withSalesforceTokensthroughout the codebase.46-51: The
ResourceLoaderautowired dependency has been removed. Ensure that this dependency is not required elsewhere in the code.62-68: The mock bean
mockGetTokensnow mocksSalesforceTokensinstead ofSalesforceGetTokens. Confirm that all methods previously called onmockGetTokensexist inSalesforceTokens.89-97: The method
loadTestData(currentFunctionName)has been replaced withcommon.loadScenariosData(...). Ensure that the new method provides the same functionality as the old one.139-147: Similar to the previous comment, the method
loadTestData(currentFunctionName)has been replaced withcommon.loadScenariosData(...). Ensure that the new method provides the same functionality as the old one.179-229: A new test case
testPostSalesforceConnectDisconnectedUserSignup()has been added. This test case seems to be testing the signup functionality for a disconnected user. The test case looks well-written and covers different scenarios based on the test data. It also properly mocks the responses forgetTokensandgetIdentity, performs the POST request, and asserts the expected output.
| /** | ||
| * Revokes tokens from Salesforce using the access/refresh token. | ||
| * @param instanceUrl Instance URL | ||
| * @param token Refresh token | ||
| * @return HttpResponse | ||
| */ | ||
| public HttpResponse revokeTokens(String instanceUrl, String token) { | ||
| String salesforceRevokeTokensEndpoint = instanceUrl + salesforceConstants.revokeTokensUrl(); | ||
|
|
||
| String requestBody = "token=" + token; | ||
|
|
||
| Map<String, String> headers = new HashMap<>(); | ||
| headers.put("content-type", "application/x-www-form-urlencoded"); | ||
|
|
||
| HttpResponse response = null; | ||
| try { | ||
| response = HttpClient.makePostRequest(salesforceRevokeTokensEndpoint, headers, requestBody, 10000); | ||
| } | ||
| catch (Exception e) { | ||
| throw new CustomException(new ErrorObject("l_s_w_srt_rt_1", "something_went_wrong", e.getMessage())); | ||
| } | ||
| return response; | ||
| } |
There was a problem hiding this comment.
This new method revokeTokens is introduced to revoke tokens from Salesforce. It seems to be correctly implemented, but it's important to handle exceptions properly. In case of an exception, a custom exception is thrown with a generic error message "something_went_wrong". It would be more helpful to provide a more specific error message related to the failure of token revocation.
- throw new CustomException(new ErrorObject("l_s_w_srt_rt_1", "something_went_wrong", e.getMessage()));
+ throw new CustomException(new ErrorObject("l_s_w_srt_rt_1", "Failed to revoke Salesforce tokens", e.getMessage()));| long duration = System.currentTimeMillis() - startTimestamp; | ||
| logger.debug("({} ms)DBQuery:Save: table-{}", duration, object.getClass().getSimpleName()); | ||
| } | ||
|
|
||
| @Override | ||
| public <T> void save(T object, DynamoDBMapperConfig config) { | ||
| long startTimestamp = System.currentTimeMillis(); | ||
| try { | ||
| defaultMapper.save(object, config); | ||
| } | ||
| catch (Exception e) { | ||
| logger.debug("DBQuery:Save: table-{}", object.getClass().getSimpleName()); | ||
| logger.error("DBQuery:Save: exception-{}", e); | ||
| throw new RuntimeException("Error during save database operation", e); | ||
| } | ||
|
|
||
| long duration = System.currentTimeMillis() - startTimestamp; | ||
| logger.debug("({} ms)DBQuery:Save: table-{}", duration, object.getClass().getSimpleName()); | ||
| } |
There was a problem hiding this comment.
The save method has been overridden to add logging and error handling. This is a good practice as it provides visibility into the database operations and helps in debugging issues. However, the exception handling could be improved by throwing a more specific exception instead of a generic RuntimeException. Consider creating a custom exception class for database operation failures.
- throw new RuntimeException("Error during save database operation", e);
+ throw new DatabaseOperationException("Error during save database operation", e);| User currentUser = (User) request.getAttribute("current_user"); | ||
| disconnectUserFactory.disconnect(currentUser); |
There was a problem hiding this comment.
The code assumes that the current_user attribute is always present in the request and can be cast to a User. If this is not the case, a NullPointerException or ClassCastException could occur. It would be safer to check if the attribute exists and is of the correct type before casting.
- User currentUser = (User) request.getAttribute("current_user");
- disconnectUserFactory.disconnect(currentUser);
+ Object userAttribute = request.getAttribute("current_user");
+ if (userAttribute instanceof User) {
+ User currentUser = (User) userAttribute;
+ disconnectUserFactory.disconnect(currentUser);
+ } else {
+ // Handle error: attribute missing or not a User
+ }| package com.salessparrow.api.lib.crmActions.disconnectUser; | ||
|
|
||
| import org.springframework.beans.factory.annotation.Autowired; | ||
| import org.springframework.stereotype.Component; | ||
|
|
||
| import com.salessparrow.api.domain.User; | ||
| import com.salessparrow.api.exception.CustomException; | ||
| import com.salessparrow.api.lib.errorLib.ErrorObject; | ||
| import com.salessparrow.api.lib.globalConstants.UserConstants; | ||
|
|
||
| /** | ||
| * Factory for disconnecting a user from the CRM based on the user kind. | ||
| */ | ||
| @Component | ||
| public class DisconnectUserFactory { | ||
|
|
||
| @Autowired | ||
| private DisconnectSalesforceUser disconnectSalesforceUser; | ||
|
|
||
| /** | ||
| * Disconnect a user from the CRM based on the user kind. | ||
| * @param user | ||
| * @return void | ||
| */ | ||
| public void disconnect(User user) { | ||
|
|
||
| switch (user.getUserKind()) { | ||
| case UserConstants.SALESFORCE_USER_KIND: | ||
| disconnectSalesforceUser.disconnect(user); | ||
| break; | ||
| default: | ||
| throw new CustomException( | ||
| new ErrorObject("l_ca_du_duf_d_1", "something_went_wrong", "Invalid user kind.")); | ||
| } | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
The DisconnectUserFactory class currently only supports disconnecting Salesforce users. If more CRM platforms are to be supported in the future, consider using a map of DisconnectUser implementations instead of individual fields for each type. This would make it easier to add support for new user kinds without modifying this class.
- @Autowired
- private DisconnectSalesforceUser disconnectSalesforceUser;
+ private final Map<String, DisconnectUser> disconnectUserMap;
+ @Autowired
+ public DisconnectUserFactory(List<DisconnectUser> disconnectUsers) {
+ this.disconnectUserMap = disconnectUsers.stream()
+ .collect(Collectors.toMap(DisconnectUser::getUserKind, Function.identity()));
+ }
public void disconnect(User user) {
- switch (user.getUserKind()) {
- case UserConstants.SALESFORCE_USER_KIND:
- disconnectSalesforceUser.disconnect(user);
- break;
- default:
- throw new CustomException(
- new ErrorObject("l_ca_du_duf_d_1", "something_went_wrong", "Invalid user kind."));
- }
+ DisconnectUser disconnectUser = disconnectUserMap.get(user.getUserKind());
+ if (disconnectUser == null) {
+ throw new CustomException(
+ new ErrorObject("l_ca_du_duf_d_1", "something_went_wrong", "Invalid user kind."));
+ }
+ disconnectUser.disconnect(user);
}This change also makes the code more aligned with the Open-Closed Principle (OCP) of SOLID principles, as it allows adding new functionality (support for new user kinds) by adding new code (a new DisconnectUser implementation), not by changing existing code.
| } | ||
| } | ||
|
|
||
| } | ||
| @CacheEvict(value = CacheConstants.SS_SALESFORCE_USER_CACHE, key = "#externalUserId") | ||
| public void removeSalesforceUserData(String externalUserId) { | ||
| SalesforceUser salesforceUser = getSalesforceUserByExternalUserId(externalUserId); | ||
| salesforceUser.setIdentityUrl(null); | ||
| salesforceUser.setExternalOrganizationId(null); | ||
| salesforceUser.setName(null); | ||
| salesforceUser.setEmail(null); | ||
| salesforceUser.setUserKind(null); | ||
| salesforceUser.setCookieToken(null); | ||
| salesforceUser.setEncryptionSalt(null); | ||
| salesforceUser.setStatus(SalesforceUser.Status.DELETED); | ||
|
|
||
| try { | ||
| dynamoDBMapper.save(salesforceUser, | ||
| new DynamoDBMapperConfig.Builder().withSaveBehavior(DynamoDBMapperConfig.SaveBehavior.UPDATE) | ||
| .build()); | ||
| } | ||
| catch (Exception e) { | ||
| throw new CustomException(new ErrorObject("r_sur_r_1", "something_went_wrong", e.getMessage())); | ||
| } | ||
| } |
There was a problem hiding this comment.
The new method removeSalesforceUserData is introduced to remove user-specific data from the SalesforceUser object and update its status to DELETED. This method is correctly using DynamoDBMapper's save method with SaveBehavior.UPDATE, which ensures that only non-null attributes are updated in the database. However, it would be better to handle specific exceptions rather than a generic Exception. For instance, AmazonDynamoDBException could be caught for AWS service related errors.
- catch (Exception e) {
+ catch (AmazonDynamoDBException e) {
throw new CustomException(new ErrorObject("r_sur_r_1", "something_went_wrong", e.getMessage()));
}| @PostMapping("/disconnect") | ||
| public ResponseEntity<String> disconnect(HttpServletRequest request) { | ||
| logger.info("User disconnect request received"); | ||
|
|
||
| disconnectUserService.disconnect(request); | ||
|
|
||
| logger.info("Clearing user cookie"); | ||
| HttpHeaders headers = new HttpHeaders(); | ||
| headers = cookieHelper.clearUserCookie(headers); | ||
|
|
||
| logger.info("User disconnected successfully"); | ||
| return ResponseEntity.noContent().headers(headers).build(); | ||
| } |
There was a problem hiding this comment.
The disconnect endpoint has been implemented correctly. It logs the request, calls the disconnectUserService to handle the disconnection, clears the user cookie, and returns a no content response. However, it lacks error handling. If an exception occurs during the disconnection process, it should be caught and handled appropriately. Here's a suggestion on how to improve this:
@PostMapping("/disconnect")
public ResponseEntity<String> disconnect(HttpServletRequest request) {
logger.info("User disconnect request received");
+ try {
disconnectUserService.disconnect(request);
logger.info("Clearing user cookie");
HttpHeaders headers = new HttpHeaders();
headers = cookieHelper.clearUserCookie(headers);
logger.info("User disconnected successfully");
return ResponseEntity.noContent().headers(headers).build();
+ } catch (Exception e) {
+ logger.error("Error occurred while disconnecting user", e);
+ return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body("An error occurred while disconnecting user");
+ }
}| public void testPostDisconnectSuccess() throws Exception { | ||
| String currentFunctionName = new Object() { | ||
| }.getClass().getEnclosingMethod().getName(); | ||
| FixtureData fixtureData = common.loadFixture( | ||
| "classpath:fixtures/functional/controllers/authController/PostDisconnectFixture.json", | ||
| currentFunctionName); | ||
| loadFixture.perform(fixtureData); | ||
|
|
||
| List<Scenario> testDataItems = common.loadScenariosData( | ||
| "classpath:data/functional/controllers/authController/Disconnect.scenarios.json", currentFunctionName); | ||
|
|
||
| for (Scenario testDataItem : testDataItems) { | ||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| String cookieValue = Constants.SALESFORCE_ACTIVE_USER_COOKIE_VALUE; | ||
|
|
||
| HttpResponse getTokensMockRes = new HttpResponse(); | ||
| getTokensMockRes | ||
| .setResponseBody(objectMapper.writeValueAsString(testDataItem.getMocks().get("revokeTokens"))); | ||
|
|
||
| when(mockGetTokens.getTokens(anyString(), anyString())).thenReturn(getTokensMockRes); | ||
|
|
||
| ResultActions resultActions = mockMvc.perform(MockMvcRequestBuilders.post("/api/v1/auth/disconnect") | ||
| .cookie(new Cookie(CookieConstants.USER_LOGIN_COOKIE_NAME, cookieValue)) | ||
| .contentType(MediaType.APPLICATION_JSON)); | ||
|
|
||
| String actualOutput = resultActions.andReturn().getResponse().getContentAsString(); | ||
|
|
||
| if (testDataItem.getOutput().get("success").equals(true)) { | ||
| assertEquals(204, resultActions.andReturn().getResponse().getStatus()); | ||
| } | ||
| else { | ||
| common.compareErrors(testDataItem, actualOutput); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The test testPostDisconnectSuccess mocks the getTokens method of SalesforceTokens class. However, according to the PR summary, a new method revokeTokens has been added to this class which should be used for revoking tokens. Please ensure that the correct method is being mocked and tested.
- when(mockGetTokens.getTokens(anyString(), anyString())).thenReturn(getTokensMockRes);
+ when(mockGetTokens.revokeTokens(anyString(), anyString())).thenReturn(getTokensMockRes);| public void testPostDisconnectNoTokens() throws Exception { | ||
| String currentFunctionName = new Object() { | ||
| }.getClass().getEnclosingMethod().getName(); | ||
| FixtureData fixtureData = common.loadFixture( | ||
| "classpath:fixtures/functional/controllers/authController/PostDisconnectFixture.json", | ||
| currentFunctionName); | ||
| loadFixture.perform(fixtureData); | ||
|
|
||
| List<Scenario> testDataItems = common.loadScenariosData( | ||
| "classpath:data/functional/controllers/authController/Disconnect.scenarios.json", currentFunctionName); | ||
|
|
||
| for (Scenario testDataItem : testDataItems) { | ||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| String cookieValue = Constants.SALESFORCE_ACTIVE_USER_COOKIE_VALUE; | ||
|
|
||
| HttpResponse getTokensMockRes = new HttpResponse(); | ||
| getTokensMockRes | ||
| .setResponseBody(objectMapper.writeValueAsString(testDataItem.getMocks().get("revokeTokens"))); | ||
|
|
||
| when(mockGetTokens.getTokens(anyString(), anyString())).thenReturn(getTokensMockRes); | ||
|
|
||
| ResultActions resultActions = mockMvc.perform(MockMvcRequestBuilders.post("/api/v1/auth/disconnect") | ||
| .cookie(new Cookie(CookieConstants.USER_LOGIN_COOKIE_NAME, cookieValue)) | ||
| .contentType(MediaType.APPLICATION_JSON)); | ||
|
|
||
| String actualOutput = resultActions.andReturn().getResponse().getContentAsString(); | ||
|
|
||
| if (resultActions.andReturn().getResponse().getStatus() != 204) { | ||
| common.compareErrors(testDataItem, actualOutput); | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Similar issue in the testPostDisconnectNoTokens test. The getTokens method is being mocked instead of the revokeTokens method. Please update the mock to use the correct method.
- when(mockGetTokens.getTokens(anyString(), anyString())).thenReturn(getTokensMockRes);
+ when(mockGetTokens.revokeTokens(anyString(), anyString())).thenReturn(getTokensMockRes);| @Test | ||
| public void testRevokeTokensSuccess() throws Exception { | ||
| MockedStatic<HttpClient> httpClientMockedStatic = Mockito.mockStatic(HttpClient.class); | ||
|
|
||
| String instanceUrl = "https://example.com"; | ||
| String refreshToken = "your-refresh-token"; | ||
|
|
||
| HttpResponse expectedResponse = new HttpResponse(); | ||
| expectedResponse.setResponseBody(""); | ||
|
|
||
| httpClientMockedStatic.when(() -> HttpClient.makePostRequest(anyString(), anyMap(), any(), anyInt())) | ||
| .thenReturn(expectedResponse); | ||
|
|
||
| HttpResponse response = salesforceTokens.revokeTokens(instanceUrl, refreshToken); | ||
|
|
||
| assertEquals(expectedResponse, response); | ||
|
|
||
| httpClientMockedStatic.close(); | ||
| } |
There was a problem hiding this comment.
The use of MockedStatic is not recommended as it can lead to brittle tests. Instead, consider mocking the HttpClient instance that's injected into SalesforceTokens. This way, you can control its behavior in a more predictable and maintainable manner.
- MockedStatic<HttpClient> httpClientMockedStatic = Mockito.mockStatic(HttpClient.class);
+ Mockito.when(httpClientMock.doPost(anyString(), anyMap(), any(), anyInt())).thenReturn(expectedResponse);| @Test | ||
| public void testRevokeTokensFailure() throws Exception { | ||
| MockedStatic<HttpClient> httpClientMockedStatic = Mockito.mockStatic(HttpClient.class); | ||
|
|
||
| String instanceUrl = "https://example.com"; | ||
| String refreshToken = "invalid-refresh-token"; | ||
|
|
||
| httpClientMockedStatic.when(() -> HttpClient.makePostRequest(anyString(), anyMap(), any(), anyInt())) | ||
| .thenThrow(new RuntimeException("Invalid refresh token")); | ||
|
|
||
| assertThrows(CustomException.class, () -> { | ||
| salesforceTokens.revokeTokens(instanceUrl, refreshToken); | ||
| }); | ||
| httpClientMockedStatic.close(); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, avoid using MockedStatic. Instead, mock the behavior of the HttpClient instance.
- MockedStatic<HttpClient> httpClientMockedStatic = Mockito.mockStatic(HttpClient.class);
+ Mockito.when(httpClientMock.doPost(anyString(), anyMap(), any(), anyInt())).thenThrow(new RuntimeException("Invalid refresh token"));
This pull request introduces a new Salesforce User Disconnect API that handles the revocation of tokens and deletion of Salesforce-specific data from the user table. This enhancement improves user data management and ensures that user data is securely cleared when disconnecting from Salesforce.
Summary by CodeRabbit
SalesforceGetTokenstoSalesforceTokensand added a new methodrevokeTokensfor token revocation in Salesforce.DynamoDBConfigurationand various repositories.HttpClientby adding null checks for content type.SecurityConfig, as it's handled by the reverse proxy server.