Create/8080/create facebook login #454
Conversation
WalkthroughThis pull request introduces a new CORS filter configuration and updates the security settings to allow unauthenticated access for Facebook login and authentication check endpoints. A new POST endpoint for Facebook login is added to the controller. The Facebook security service interface and its implementation now include methods to authenticate using a Facebook token and language, handle user creation, and manage error scenarios. Additionally, new tests have been introduced to cover the enhanced authentication flow, and the configuration file now contains a property for retrieving Facebook user info. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant C as FacebookSecurityController
participant S as FacebookSecurityServiceImpl
participant FB as Facebook API
U->>C: POST /facebookSecurity/login {token, language}
C->>S: authenticate(fbToken, language)
S->>FB: getUserInfoFromFacebook(fbToken)
FB-->>S: User details or error response
alt Valid Response
S->>S: processAuthentication & handleNewUser if needed
S-->>C: SuccessSignInDto
C-->>U: 200 OK with authentication details
else Invalid Response
S-->>C: Error (e.g., missing parameters or invalid data)
C-->>U: Error response
end
Suggested reviewers
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
core/src/main/java/greencity/config/CorsFilterConfig.java (1)
1-29: Consider environment-specific configurationsSince this is for Facebook login integration, you'll need different CORS configurations for various environments:
- Local development (localhost)
- Staging/Testing
- Production
Consider implementing a configuration class that loads different CORS settings based on the active Spring profile:
@Configuration public class CorsProperties { @Value("${spring.profiles.active:default}") private String activeProfile; @Bean @ConfigurationProperties(prefix = "cors") public CorsSettings corsSettings() { // Load different settings based on activeProfile return new CorsSettings(); } }service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (2)
74-75: Consider removing embedded query parameters
DefininguserInfoUrlwith query parameters might lead to redundant queries when appending them again at runtime. A more flexible design is to store only the base URL, then dynamically append query parameters in the code.
208-218: Handle partial failures more gracefully
WhilerestClient.createUbsProfileis called after the user is saved, consider how the system should behave if this external call fails (e.g., partial data in your system, but no external profile). A more robust error-handling approach or a compensating transaction may be beneficial.core/src/main/java/greencity/security/controller/FacebookSecurityController.java (1)
61-64: Validate request body keys
The method assumes"token"and"lang"keys exist. Consider adding checks or default values to handle missing or empty entries and avoid potential null pointer issues.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/main/java/greencity/config/CorsFilterConfig.java(1 hunks)core/src/main/java/greencity/config/SecurityConfig.java(1 hunks)core/src/main/java/greencity/security/controller/FacebookSecurityController.java(2 hunks)service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service-api/src/main/java/greencity/security/service/FacebookSecurityService.java(1 hunks)service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java(3 hunks)
🧰 Additional context used
🪛 GitHub Actions: CI/CD GreenCityUser
service-api/src/main/java/greencity/constant/ErrorMessage.java
[error] 67-67: Blank line at end of block should be removed
🔇 Additional comments (11)
core/src/main/java/greencity/config/CorsFilterConfig.java (2)
13-15: Well-structured filter configuration!Good use of
@Componentand@Order(Ordered.HIGHEST_PRECEDENCE)annotations. ExtendingOncePerRequestFilteris the recommended approach for implementing custom filters in Spring.
23-26: LGTM! Proper handling of preflight requests.The OPTIONS preflight request handling is implemented correctly, following best practices by returning 200 OK status.
service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (5)
3-5: Imports are consistent and relevant
The addition of these imports (e.g., Jackson, HttpClient, custom entities) aligns with the introduced functionality for Facebook-based authentication and user profile processing.Also applies to: 7-7, 10-10, 12-12, 14-14, 16-18, 21-23, 26-26, 28-31, 33-33, 36-39
147-173: Verify mapping of language string to numeric ID
The logic usesmodelMapper.map(language, Long.class)to populateLanguage.id, which could cause errors iflanguageis not numeric. Ensure the source format forlanguageis indeed numeric, or adapt the code to map from a language code instead.
175-192: Appropriate null check for token and language
The method correctly throws an exception if either is missing. However, confirm that the passedlanguagestring is validated (e.g., to ensure it maps to a valid entry) before calling downstream methods.
194-206: Straightforward authentication flow
The logic to handle new vs. existing users is clear, with proper checks for deactivation. The code is well-structured and ensures user status is validated.
220-229: Transactional user creation looks correct
UsingTransactionTemplateensures consistency when saving the new user. This approach is appropriate, and there are no obvious concerns with concurrency here.service-api/src/main/java/greencity/security/service/FacebookSecurityService.java (1)
28-28: New authenticate method
Exposes a clear approach for token-based login with an additional language parameter. This interface extension is consistent with the implementation changes in the service class.core/src/main/java/greencity/security/controller/FacebookSecurityController.java (1)
14-15: New imports align with incoming request data
ImportingPostMapping,RequestBody, andMapis necessary for the new login method. No issues noted.Also applies to: 19-19
service-api/src/main/java/greencity/constant/ErrorMessage.java (1)
61-61: LGTM! Clear and consistent error message.The new error message constant follows the established pattern and clearly indicates the missing parameters.
core/src/main/java/greencity/config/SecurityConfig.java (1)
133-135: LGTM! Security configuration for Facebook login endpoints.The new endpoints are correctly configured to allow unauthenticated access for Facebook login functionality.
Please ensure that:
- The
/facebookSecurity/loginendpoints are properly protected against CSRF attacks.- The
/check-authendpoint doesn't expose sensitive information.Let's verify the implementation of these endpoints:
| response.setHeader("Access-Control-Allow-Origin", "https://eb10-91-245-77-57.ngrok-free.app"); | ||
| response.setHeader("Access-Control-Allow-Methods", "GET, POST, OPTIONS"); | ||
| response.setHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, ngrok-skip-browser-warning"); | ||
| response.setHeader("Access-Control-Allow-Credentials", "true"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Externalize CORS configuration values
The hard-coded ngrok URL and CORS settings should be moved to configuration properties:
- ngrok URLs are temporary and will change
- CORS settings may differ between environments (dev/staging/prod)
Consider this approach:
+@Value("${cors.allowed-origin}")
+private String allowedOrigin;
+@Value("${cors.allowed-methods}")
+private String allowedMethods;
+@Value("${cors.allowed-headers}")
+private String allowedHeaders;
@Override
protected void doFilterInternal(...) {
- response.setHeader("Access-Control-Allow-Origin", "https://eb10-91-245-77-57.ngrok-free.app");
- response.setHeader("Access-Control-Allow-Methods", "GET, POST, OPTIONS");
- response.setHeader("Access-Control-Allow-Headers", "Content-Type, Authorization, ngrok-skip-browser-warning");
+ response.setHeader("Access-Control-Allow-Origin", allowedOrigin);
+ response.setHeader("Access-Control-Allow-Methods", allowedMethods);
+ response.setHeader("Access-Control-Allow-Headers", allowedHeaders);Add to application.properties/yaml:
cors.allowed-origin=https://your-frontend-domain.com
cors.allowed-methods=GET,POST,OPTIONS
cors.allowed-headers=Content-Type,Authorization| private UserInfo getUserInfoFromFacebook(String accessToken) throws IOException { | ||
| String requestUrl = userInfoUrl + "?fields=id,name,email,picture&access_token=" + accessToken; | ||
| HttpGet request = new HttpGet(requestUrl); | ||
| try (CloseableHttpResponse response = (CloseableHttpResponse) httpClient.execute(request)) { | ||
| int statusCode = response.getStatusLine().getStatusCode(); | ||
| if (statusCode != 200) { | ||
| throw new IOException("Facebook API returned status: " + statusCode); | ||
| } | ||
| String jsonResponse = EntityUtils.toString(response.getEntity()); | ||
| JsonNode jsonNode = objectMapper.readTree(jsonResponse); | ||
| String name = jsonNode.has("name") ? jsonNode.get("name").asText() : "Unknown"; | ||
| String email = jsonNode.has("email") ? jsonNode.get("email").asText() : null; | ||
| String pictureUrl = Optional.ofNullable(jsonNode.get("picture")) | ||
| .map(p -> p.get("data")) | ||
| .map(d -> d.get("url")) | ||
| .map(JsonNode::asText) | ||
| .orElse(null); | ||
| UserInfo userInfo = new UserInfo(); | ||
| userInfo.setName(name); | ||
| userInfo.setEmail(email); | ||
| userInfo.setPicture(pictureUrl); | ||
| return userInfo; | ||
| } | ||
| } |
There was a problem hiding this comment.
Duplicate query parameters in Facebook request
Because userInfoUrl already includes ?fields=id,name,email,picture&access_token=, appending ?fields=... again may lead to malformed URLs and potential request failures.
Here’s a recommended fix to remove the repeated query parameters in line 237:
-String requestUrl = userInfoUrl + "?fields=id,name,email,picture&access_token=" + accessToken;
+String requestUrl = userInfoUrl + accessToken;📝 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 UserInfo getUserInfoFromFacebook(String accessToken) throws IOException { | |
| String requestUrl = userInfoUrl + "?fields=id,name,email,picture&access_token=" + accessToken; | |
| HttpGet request = new HttpGet(requestUrl); | |
| try (CloseableHttpResponse response = (CloseableHttpResponse) httpClient.execute(request)) { | |
| int statusCode = response.getStatusLine().getStatusCode(); | |
| if (statusCode != 200) { | |
| throw new IOException("Facebook API returned status: " + statusCode); | |
| } | |
| String jsonResponse = EntityUtils.toString(response.getEntity()); | |
| JsonNode jsonNode = objectMapper.readTree(jsonResponse); | |
| String name = jsonNode.has("name") ? jsonNode.get("name").asText() : "Unknown"; | |
| String email = jsonNode.has("email") ? jsonNode.get("email").asText() : null; | |
| String pictureUrl = Optional.ofNullable(jsonNode.get("picture")) | |
| .map(p -> p.get("data")) | |
| .map(d -> d.get("url")) | |
| .map(JsonNode::asText) | |
| .orElse(null); | |
| UserInfo userInfo = new UserInfo(); | |
| userInfo.setName(name); | |
| userInfo.setEmail(email); | |
| userInfo.setPicture(pictureUrl); | |
| return userInfo; | |
| } | |
| } | |
| private UserInfo getUserInfoFromFacebook(String accessToken) throws IOException { | |
| - String requestUrl = userInfoUrl + "?fields=id,name,email,picture&access_token=" + accessToken; | |
| + String requestUrl = userInfoUrl + accessToken; | |
| HttpGet request = new HttpGet(requestUrl); | |
| try (CloseableHttpResponse response = (CloseableHttpResponse) httpClient.execute(request)) { | |
| int statusCode = response.getStatusLine().getStatusCode(); | |
| if (statusCode != 200) { | |
| throw new IOException("Facebook API returned status: " + statusCode); | |
| } | |
| String jsonResponse = EntityUtils.toString(response.getEntity()); | |
| JsonNode jsonNode = objectMapper.readTree(jsonResponse); | |
| String name = jsonNode.has("name") ? jsonNode.get("name").asText() : "Unknown"; | |
| String email = jsonNode.has("email") ? jsonNode.get("email").asText() : null; | |
| String pictureUrl = Optional.ofNullable(jsonNode.get("picture")) | |
| .map(p -> p.get("data")) | |
| .map(d -> d.get("url")) | |
| .map(JsonNode::asText) | |
| .orElse(null); | |
| UserInfo userInfo = new UserInfo(); | |
| userInfo.setName(name); | |
| userInfo.setEmail(email); | |
| userInfo.setPicture(pictureUrl); | |
| return userInfo; | |
| } | |
| } |
| public static final String WARNING_GIT_DIRECTORY_NOT_FOUND = | ||
| "WARNING: .git directory not found. Git commit info will be unavailable."; | ||
| public static final String GIT_REPOSITORY_NOT_INITIALIZED = | ||
| "Git repository not initialized. Commit info is unavailable."; | ||
| public static final String FAILED_TO_FETCH_COMMIT_INFO = "Failed to fetch commit info due to I/O error: "; | ||
|
|
There was a problem hiding this comment.
Remove blank line at end of block.
The pipeline failure indicates that the blank line at the end of the block should be removed.
Apply this diff to fix the style issue:
-📝 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.
🧰 Tools
🪛 GitHub Actions: CI/CD GreenCityUser
[error] 67-67: Blank line at end of block should be removed
…m/ita-social-projects/GreenCityUser into create/8080/create-facebook-login
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (1)
236-259:⚠️ Potential issueFix duplicate query parameters in Facebook request URL.
The query parameters are already included in the
userInfoUrlvalue.Fix the URL construction:
- String requestUrl = userInfoUrl + "?fields=id,name,email,picture&access_token=" + accessToken; + String requestUrl = userInfoUrl + "?access_token=" + accessToken + "&fields=id,name,email,picture";
🧹 Nitpick comments (5)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (3)
56-60: Consider enhancing the setUp method with mock behavior initialization.The setup method could be enhanced by adding common mock behavior that's used across multiple tests.
Add common mock behavior in setUp:
@BeforeEach void setUp() { ReflectionTestUtils.setField(facebookSecurityService, "address", "http://localhost:8080"); ReflectionTestUtils.setField(facebookSecurityService, "jwtTool", jwtTool); + when(modelMapper.map(any(String.class), eq(Long.class))).thenReturn(1L); }
62-81: Enhance test coverage with additional assertions.The test verifies essential user properties but could be more comprehensive.
Add assertions for:
- UUID not being null
- Registration and last activity dates being set
- Notification preferences being initialized
@Test void createNewUser_ShouldReturnUserWithDefaultValues() { // ... existing code ... assertEquals(1L, user.getLanguage().getId()); + assertNotNull(user.getUuid(), "UUID should not be null"); + assertNotNull(user.getDateOfRegistration(), "Registration date should be set"); + assertNotNull(user.getLastActivityTime(), "Last activity time should be set"); + assertFalse(user.getNotificationPreferences().isEmpty(), "Notification preferences should be initialized"); }
83-88: Add test cases for all null parameter combinations.The test should verify behavior for both null token and null language scenarios.
Add test cases:
@Test void authenticate_ShouldThrowException_WhenTokenOrLanguageIsNull() { - IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, - () -> facebookSecurityService.authenticate(null, "en")); - assertEquals(ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING, exception.getMessage()); + // Test null token + IllegalArgumentException tokenException = assertThrows(IllegalArgumentException.class, + () -> facebookSecurityService.authenticate(null, "en")); + assertEquals(ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING, tokenException.getMessage()); + + // Test null language + IllegalArgumentException langException = assertThrows(IllegalArgumentException.class, + () -> facebookSecurityService.authenticate("token", null)); + assertEquals(ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING, langException.getMessage()); }service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (2)
175-192: Enhance error handling with more specific error messages.The error handling could be more informative by providing specific error messages for different failure scenarios.
Consider enhancing error messages:
} catch (IOException e) { - throw new IllegalArgumentException(ErrorMessage.BAD_FACEBOOK_TOKEN + e.getMessage()); + String errorDetail = e.getMessage(); + if (errorDetail.contains("status: 400")) { + throw new IllegalArgumentException(ErrorMessage.BAD_FACEBOOK_TOKEN); + } else if (errorDetail.contains("status: 401")) { + throw new IllegalArgumentException("Facebook token has expired"); + } else { + throw new IllegalArgumentException("Failed to authenticate with Facebook: " + errorDetail); + } }
220-228: Optimize UUID generation timing.The UUID is currently generated after the user entity is created but before saving. Consider generating it during user creation.
Move UUID generation to user creation:
User saveNewUser(User newUser) { TransactionTemplate transactionTemplate = new TransactionTemplate(transactionManager); return transactionTemplate.execute(status -> { - newUser.setUuid(UUID.randomUUID().toString()); Long id = userRepo.save(newUser).getId(); newUser.setId(id); return newUser; }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
service-api/src/main/java/greencity/constant/ErrorMessage.java(1 hunks)service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java(3 hunks)service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service-api/src/main/java/greencity/constant/ErrorMessage.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
| User createNewUser(String email, String userName, String profilePicture, String language) { | ||
| User user = User.builder() | ||
| .email(email) | ||
| .name(userName) | ||
| .role(Role.ROLE_USER) | ||
| .dateOfRegistration(LocalDateTime.now()) | ||
| .lastActivityTime(LocalDateTime.now()) | ||
| .userStatus(UserStatus.ACTIVATED) | ||
| .emailNotification(EmailNotification.DISABLED) | ||
| .refreshTokenKey(jwtTool.generateTokenKey()) | ||
| .profilePicturePath(profilePicture) | ||
| .showLocation(ProfilePrivacyPolicy.PUBLIC) | ||
| .showEcoPlace(ProfilePrivacyPolicy.PUBLIC) | ||
| .showToDoList(ProfilePrivacyPolicy.PUBLIC) | ||
| .rating(DEFAULT_RATING) | ||
| .language(Language.builder().id(modelMapper.map(language, Long.class)).build()) | ||
| .build(); | ||
| Set<UserNotificationPreference> userNotificationPreferences = Arrays.stream(EmailPreference.values()) | ||
| .map(emailPreference -> UserNotificationPreference.builder() | ||
| .user(user) | ||
| .emailPreference(emailPreference) | ||
| .periodicity(EmailPreferencePeriodicity.TWICE_A_DAY) | ||
| .build()) | ||
| .collect(Collectors.toSet()); | ||
| user.setNotificationPreferences(userNotificationPreferences); | ||
| return user; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider more restrictive default privacy settings.
Setting all privacy settings to PUBLIC by default may not be the best practice for user privacy.
Consider more privacy-conscious defaults:
- .showLocation(ProfilePrivacyPolicy.PUBLIC)
- .showEcoPlace(ProfilePrivacyPolicy.PUBLIC)
- .showToDoList(ProfilePrivacyPolicy.PUBLIC)
+ .showLocation(ProfilePrivacyPolicy.PRIVATE)
+ .showEcoPlace(ProfilePrivacyPolicy.FRIENDS_ONLY)
+ .showToDoList(ProfilePrivacyPolicy.FRIENDS_ONLY)📝 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 createNewUser(String email, String userName, String profilePicture, String language) { | |
| User user = User.builder() | |
| .email(email) | |
| .name(userName) | |
| .role(Role.ROLE_USER) | |
| .dateOfRegistration(LocalDateTime.now()) | |
| .lastActivityTime(LocalDateTime.now()) | |
| .userStatus(UserStatus.ACTIVATED) | |
| .emailNotification(EmailNotification.DISABLED) | |
| .refreshTokenKey(jwtTool.generateTokenKey()) | |
| .profilePicturePath(profilePicture) | |
| .showLocation(ProfilePrivacyPolicy.PUBLIC) | |
| .showEcoPlace(ProfilePrivacyPolicy.PUBLIC) | |
| .showToDoList(ProfilePrivacyPolicy.PUBLIC) | |
| .rating(DEFAULT_RATING) | |
| .language(Language.builder().id(modelMapper.map(language, Long.class)).build()) | |
| .build(); | |
| Set<UserNotificationPreference> userNotificationPreferences = Arrays.stream(EmailPreference.values()) | |
| .map(emailPreference -> UserNotificationPreference.builder() | |
| .user(user) | |
| .emailPreference(emailPreference) | |
| .periodicity(EmailPreferencePeriodicity.TWICE_A_DAY) | |
| .build()) | |
| .collect(Collectors.toSet()); | |
| user.setNotificationPreferences(userNotificationPreferences); | |
| return user; | |
| } | |
| User createNewUser(String email, String userName, String profilePicture, String language) { | |
| User user = User.builder() | |
| .email(email) | |
| .name(userName) | |
| .role(Role.ROLE_USER) | |
| .dateOfRegistration(LocalDateTime.now()) | |
| .lastActivityTime(LocalDateTime.now()) | |
| .userStatus(UserStatus.ACTIVATED) | |
| .emailNotification(EmailNotification.DISABLED) | |
| .refreshTokenKey(jwtTool.generateTokenKey()) | |
| .profilePicturePath(profilePicture) | |
| .showLocation(ProfilePrivacyPolicy.PRIVATE) | |
| .showEcoPlace(ProfilePrivacyPolicy.FRIENDS_ONLY) | |
| .showToDoList(ProfilePrivacyPolicy.FRIENDS_ONLY) | |
| .rating(DEFAULT_RATING) | |
| .language(Language.builder().id(modelMapper.map(language, Long.class)).build()) | |
| .build(); | |
| Set<UserNotificationPreference> userNotificationPreferences = Arrays.stream(EmailPreference.values()) | |
| .map(emailPreference -> UserNotificationPreference.builder() | |
| .user(user) | |
| .emailPreference(emailPreference) | |
| .periodicity(EmailPreferencePeriodicity.TWICE_A_DAY) | |
| .build()) | |
| .collect(Collectors.toSet()); | |
| user.setNotificationPreferences(userNotificationPreferences); | |
| return user; | |
| } |
| @Value("https://graph.facebook.com/v22.0/me?fields=id,name,email,picture&access_token=") | ||
| private String userInfoUrl; |
There was a problem hiding this comment.
Review Facebook Graph API version and URL configuration.
The hardcoded Graph API version (v22.0) seems incorrect, and query parameters are duplicated.
- Update to the correct API version (current is v18.0)
- Remove duplicate query parameters:
-@Value("https://graph.facebook.com/v22.0/me?fields=id,name,email,picture&access_token=")
+@Value("https://graph.facebook.com/v18.0/me")
private String userInfoUrl;📝 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.
| @Value("https://graph.facebook.com/v22.0/me?fields=id,name,email,picture&access_token=") | |
| private String userInfoUrl; | |
| @Value("https://graph.facebook.com/v18.0/me") | |
| private String userInfoUrl; |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
service/src/test/java/greencity/security/service/FacebookSecurityServiceTest.java (2)
18-23: Consider extracting test configuration values.While using
ReflectionTestUtilsis appropriate for testing, the hardcoded configuration values should be moved to a test configuration file or constants class.+ private static final String TEST_ADDRESS = "http://localhost:8080"; + private static final String TEST_APP_ID = "12345"; + private static final String TEST_APP_SECRET = "6789"; @BeforeEach public void init() { - ReflectionTestUtils.setField(facebookSecurityService, "address", "http://localhost:8080"); - ReflectionTestUtils.setField(facebookSecurityService, "facebookAppId", "12345"); - ReflectionTestUtils.setField(facebookSecurityService, "facebookAppSecret", "6789"); + ReflectionTestUtils.setField(facebookSecurityService, "address", TEST_ADDRESS); + ReflectionTestUtils.setField(facebookSecurityService, "facebookAppId", TEST_APP_ID); + ReflectionTestUtils.setField(facebookSecurityService, "facebookAppSecret", TEST_APP_SECRET); }
25-33: Enhance URL generation test with additional assertions.The test verifies basic URL generation but could be more robust.
Consider adding assertions to verify:
- URL contains all required OAuth parameters
- Parameters are properly URL encoded
- Different app IDs and redirect URLs
Example enhancement:
@Test void generateFacebookAuthorizeURLTest() { String url = facebookSecurityService.generateFacebookAuthorizeURL(); assertAll( () -> assertTrue(url.contains("client_id=" + TEST_APP_ID)), () -> assertTrue(url.contains("redirect_uri=")), () -> assertTrue(url.contains("scope=email")), () -> assertEquals(expected, url) ); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (2)
service/src/test/java/greencity/security/service/FacebookSecurityServiceTest.java (2)
12-17: 🛠️ Refactor suggestionTest class structure looks good, but consider expanding test coverage.
The class is well-structured with proper Mockito setup. However, given that this implements Facebook login functionality, we should have additional test cases for authentication scenarios.
Consider adding test cases for:
- Facebook token validation
- User authentication flow
- Error handling scenarios
1-34: 🛠️ Refactor suggestionAdd missing test cases for comprehensive Facebook login coverage.
The current test suite only covers URL generation. Based on the PR objectives of implementing Facebook login, we need additional test cases.
Add test methods for:
@Test void authenticateWithValidTokenTest() { // Test successful authentication flow } @Test void authenticateWithInvalidTokenTest() { // Test error handling for invalid tokens } @Test void authenticateWithMissingLanguageTest() { // Test validation of required parameters } @Test void handleFacebookApiErrorTest() { // Test handling of Facebook API errors }
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (5)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (5)
40-41: Add class-level documentation.Consider adding Javadoc to describe the test class's purpose and scope. This helps other developers understand what aspects of Facebook authentication are being tested.
@ExtendWith(MockitoExtension.class) +/** + * Test class for FacebookSecurityServiceImpl that verifies Facebook authentication, + * user creation, and token generation functionality. + */ class FacebookSecurityServiceImplTest {
81-100: Enhance createNewUser test coverage.Consider adding edge cases to test input validation:
@Test void createNewUser_ShouldThrowException_WhenEmailIsInvalid() { String invalidEmail = "invalid-email"; assertThrows(IllegalArgumentException.class, () -> facebookSecurityService.createNewUser(invalidEmail, "name", "pic", "1")); }
102-119: Add missing null language test case.The authenticate method should be tested with a null language parameter.
@Test void authenticate_ShouldThrowException_WhenLanguageIsNull() { IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, () -> facebookSecurityService.authenticate("valid_token", null)); assertEquals(ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING, exception.getMessage()); }
121-145: Add test for non-existent user scenario.The processAuthentication method should be tested when the user doesn't exist in the system.
@Test void processAuthentication_ShouldCreateNewUser_WhenUserDoesNotExist() { String email = "[email protected]"; when(userService.findByEmail(email)).thenReturn(null); when(jwtTool.createAccessToken(anyString(), any())).thenReturn("accessToken"); when(jwtTool.createRefreshToken(any())).thenReturn("refreshToken"); SuccessSignInDto result = facebookSecurityService.processAuthentication( email, "New User", "profile.jpg", "1"); assertNotNull(result); verify(userRepo).save(any(User.class)); }
147-174: Add error handling test cases.Consider adding tests for error scenarios in saveNewUser and getSuccessSignInDto methods:
@Test void saveNewUser_ShouldThrowException_WhenSaveFails() { User newUser = new User(); when(userRepo.save(any(User.class))).thenThrow(new RuntimeException("Database error")); assertThrows(RuntimeException.class, () -> facebookSecurityService.saveNewUser(newUser)); } @Test void getSuccessSignInDto_ShouldThrowException_WhenTokenGenerationFails() { UserVO userVO = new UserVO(); when(jwtTool.createAccessToken(anyString(), any())) .thenThrow(new RuntimeException("Token generation failed")); assertThrows(RuntimeException.class, () -> facebookSecurityService.getSuccessSignInDto(userVO)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (1)
40-175: Well-structured test class with comprehensive coverage!The test class demonstrates good practices:
- Clear test method names following BDD style
- Proper use of mocking and assertions
- Comprehensive coverage of happy paths
Consider the suggested improvements to make the test suite more robust:
- Add class-level documentation
- Include edge cases for input validation
- Add tests for non-existent user scenarios
- Enhance error handling coverage
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (2)
175-206: Enhance logging for better traceability.Consider adding debug logs for important authentication steps to aid in troubleshooting:
public SuccessSignInDto authenticate(String fbToken, String language) { + log.debug("Starting Facebook authentication for language: {}", language); if (fbToken == null || language == null) { + log.warn("Authentication failed: Facebook token or language is missing"); throw new IllegalArgumentException(ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING); } try { UserInfo userInfo = getUserInfoFromFacebook(fbToken); + log.debug("Retrieved user info from Facebook for email: {}", userInfo.getEmail());
220-228: Consider checking UUID uniqueness.While UUID collisions are extremely rare, in a high-security context, it's good practice to verify uniqueness before saving.
User saveNewUser(User newUser) { TransactionTemplate transactionTemplate = new TransactionTemplate(transactionManager); return transactionTemplate.execute(status -> { - newUser.setUuid(UUID.randomUUID().toString()); + String uuid; + do { + uuid = UUID.randomUUID().toString(); + } while (userRepo.existsByUuid(uuid)); + newUser.setUuid(uuid); Long id = userRepo.save(newUser).getId(); newUser.setId(id); return newUser; }); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
core/src/main/resources/application.properties(1 hunks)service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (3)
core/src/main/resources/application.properties (1)
32-33: Facebook API Configuration VerificationThe new property
facebook.resource.userInfoUriis a well-placed addition and critical for enabling Facebook login. Please verify that:
- The trailing equals sign (
=) is intentional, thereby allowing the dynamic appending of the access token at runtime.- The specified API version (
v22.0) and the requested fields (id,name,email,picture) meet current integration requirements. Future changes in the Facebook API might require a quick update to this URI.service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (2)
147-173: Consider more restrictive default privacy settings.Setting all privacy settings to PUBLIC by default may not be the best practice for user privacy.
236-259: Duplicate query parameters in Facebook request URL.The URL construction contains duplicate query parameters.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
core/src/test/java/greencity/security/controller/FacebookSecurityControllerTest.java (1)
59-66: Consider enhancing test coverage with additional scenarios.While the happy path test is good, consider adding tests for:
- Invalid/missing token
- Invalid/missing language
- Error responses from the service layer
Example test for error scenario:
@Test void authenticateTest() throws Exception { mockMvc.perform(post("/facebookSecurity/login") .contentType("application/json") .content("{\"token\":\"almostSecretToken\", \"lang\":\"en\"}")) .andExpect(status().isOk()); verify(facebookSecurityService).authenticate("almostSecretToken", "en"); } +@Test +void authenticate_ShouldReturnBadRequest_WhenTokenIsMissing() throws Exception { + mockMvc.perform(post("/facebookSecurity/login") + .contentType("application/json") + .content("{\"lang\":\"en\"}")) + .andExpect(status().isBadRequest()); + verify(facebookSecurityService, never()).authenticate(any(), any()); +}service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (2)
91-96: Consider enhancing setup with common test data.The setup method could be improved by initializing common test data used across multiple tests.
Example enhancement:
@BeforeEach void setUp() { ReflectionTestUtils.setField(facebookSecurityService, "address", "http://localhost:8080"); ReflectionTestUtils.setField(facebookSecurityService, "jwtTool", jwtTool); ReflectionTestUtils.setField(facebookSecurityService, "restClient", restClient); + // Common test data + ReflectionTestUtils.setField(facebookSecurityService, "facebookAppId", "12345"); + ReflectionTestUtils.setField(facebookSecurityService, "facebookAppSecret", "6789"); }
207-218: Consider adding timeout for external API calls.When testing external API calls, it's good practice to add timeouts to prevent tests from hanging.
Example:
@Test +@Timeout(value = 5, unit = TimeUnit.SECONDS) void getUserInfoFromFacebook_ShouldThrowIOException_WhenResponseStatusIsNot200() throws IOException { // ... existing test code ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
core/src/test/java/greencity/security/controller/FacebookSecurityControllerTest.java(2 hunks)service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java(3 hunks)service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (5)
core/src/test/java/greencity/security/controller/FacebookSecurityControllerTest.java (1)
3-24: LGTM! Well-organized imports.The imports are properly organized and include all necessary testing utilities for the new functionality.
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (4)
3-90: LGTM! Well-structured mock setup.Good practice using mocks for external dependencies (HttpClient, ObjectMapper, etc.) and proper organization of imports.
98-117: LGTM! Thorough test for user creation.The test properly verifies all default values for a new user.
119-136: LGTM! Good error handling tests.Tests properly verify error scenarios for null token/language and invalid Facebook responses.
138-162: LGTM! Comprehensive authentication flow tests.Tests cover both successful authentication and deactivated user scenarios.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (2)
49-81: Well-structured mock declarations with room for optimization.The comprehensive setup of mock objects follows good testing practices. However, consider documenting the purpose of each mock object with comments, especially for less obvious dependencies like
PlatformTransactionManagerandRestClient.Add comments to group related mocks and explain their purpose:
@Mock + // User-related dependencies private ModelMapper modelMapper; private UserRepo userRepo; private User user; + // Authentication-related dependencies private JwtTool jwtTool; private UserService userService; + // HTTP client dependencies for Facebook API calls private CloseableHttpClient httpClient; private CloseableHttpResponse httpResponse; private StatusLine statusLine;
211-241: Enhance test coverage with edge cases.While the
handleNewUsertest covers the happy path well, consider adding test cases for edge scenarios:
- User with very long name
- Special characters in email/name
- Missing profile picture
- Invalid language code
Add test methods for edge cases:
@Test void handleNewUser_ShouldHandleEdgeCases() { // Test very long name (e.g., 256 characters) String longName = "a".repeat(256); // Test special characters String nameWithSpecialChars = "User@#$%^&*()"; // Test missing profile picture String nullProfilePicture = null; // Test invalid language String invalidLanguage = "999"; // Add assertions for each case // ... }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (1)
1-254: Overall well-structured test suite with good coverage.The test class demonstrates good testing practices with comprehensive coverage of the Facebook authentication flow. The test methods are well-organized, follow clear naming conventions, and include both positive and negative test cases.
| @Test | ||
| void getUserInfoFromFacebook_ShouldThrowIOException_WhenResponseStatusIsNot200() throws IOException { | ||
| String accessToken = "invalid_token"; | ||
|
|
||
| when(httpClient.execute(any(HttpGet.class))).thenReturn(httpResponse); | ||
| when(httpResponse.getStatusLine()).thenReturn(statusLine); | ||
| when(statusLine.getStatusCode()).thenReturn(400); | ||
|
|
||
| IOException exception = | ||
| assertThrows(IOException.class, () -> facebookSecurityService.getUserInfoFromFacebook(accessToken)); | ||
| assertTrue(exception.getMessage().contains("Facebook API returned status")); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consolidate duplicate test cases for Facebook API error handling.
The test cases getUserInfoFromFacebook_ShouldThrowIOException_WhenResponseStatusIsNot200 and getUserInfoFromFacebook_ShouldThrowException_WhenFacebookReturnsError are testing the same scenario with identical setup and assertions.
Remove one of the duplicate test methods and enhance the remaining one with additional test cases for different HTTP status codes:
- @Test
- void getUserInfoFromFacebook_ShouldThrowException_WhenFacebookReturnsError() throws IOException {
- String accessToken = "invalid_token";
- when(httpClient.execute(any(HttpGet.class))).thenReturn(httpResponse);
- when(httpResponse.getStatusLine()).thenReturn(statusLine);
- when(statusLine.getStatusCode()).thenReturn(400);
-
- IOException exception =
- assertThrows(IOException.class, () -> facebookSecurityService.getUserInfoFromFacebook(accessToken));
- assertTrue(exception.getMessage().contains("Facebook API returned status"));
- }
@Test
void getUserInfoFromFacebook_ShouldThrowIOException_WhenResponseStatusIsNot200() throws IOException {
String accessToken = "invalid_token";
+ // Test multiple error status codes
+ int[] errorCodes = {400, 401, 403, 500};
+ for (int statusCode : errorCodes) {
when(httpClient.execute(any(HttpGet.class))).thenReturn(httpResponse);
when(httpResponse.getStatusLine()).thenReturn(statusLine);
- when(statusLine.getStatusCode()).thenReturn(400);
+ when(statusLine.getStatusCode()).thenReturn(statusCode);
IOException exception =
assertThrows(IOException.class, () -> facebookSecurityService.getUserInfoFromFacebook(accessToken));
assertTrue(exception.getMessage().contains("Facebook API returned status"));
+ assertTrue(exception.getMessage().contains(String.valueOf(statusCode)));
+ }
}Also applies to: 243-253
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (2)
212-215: Enhance error handling for UBS profile creation.The current implementation wraps all RestClientException cases with a generic message. Consider:
- Logging the specific error details
- Providing more context in the error message
try { restClient.createUbsProfile(modelMapper.map(savedUser, UbsProfileCreationDto.class)); } catch (RestClientException e) { - throw new RestClientException(ErrorMessage.TRANSACTION_FAILED, e); + log.error("Failed to create UBS profile for user {}: {}", savedUser.getEmail(), e.getMessage()); + throw new RestClientException( + String.format("%s: Failed to create UBS profile - %s", + ErrorMessage.TRANSACTION_FAILED, + e.getMessage()), + e); }
175-192: Consider adding rate limiting for authentication attempts.To prevent potential brute force attacks, consider implementing rate limiting for the authenticate method. This could be done using Spring's @ratelimiter or a similar mechanism.
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (5)
84-89: Consider enhancing the setUp method with common mock configurations.The current setUp method only sets field values. Consider moving common mock configurations here to reduce duplication across test methods.
@BeforeEach void setUp() { ReflectionTestUtils.setField(facebookSecurityService, "address", "http://localhost:8060"); ReflectionTestUtils.setField(facebookSecurityService, "jwtTool", jwtTool); ReflectionTestUtils.setField(facebookSecurityService, "restClient", restClient); + // Common mock configurations + when(jwtTool.createAccessToken(anyString(), any())).thenReturn("accessToken"); + when(jwtTool.createRefreshToken(any())).thenReturn("refreshToken"); + when(modelMapper.map(anyString(), eq(Long.class))).thenReturn(1L); }
112-129: Consider using parameterized tests for authentication error scenarios.The authentication error tests could be combined into a parameterized test to improve maintainability and coverage.
+@ParameterizedTest +@MethodSource("provideAuthenticationErrorCases") +void authenticate_ShouldThrowException_ForInvalidCases(String token, String language, String expectedMessage) { + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> facebookSecurityService.authenticate(token, language)); + assertEquals(expectedMessage, exception.getMessage()); +} + +private static Stream<Arguments> provideAuthenticationErrorCases() { + return Stream.of( + Arguments.of(null, "en", ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING), + Arguments.of("token", null, ErrorMessage.FB_TOKEN_OR_LANGUAGE_MISSING), + Arguments.of("invalid_token", "en", ErrorMessage.BAD_FACEBOOK_TOKEN) + ); +}
157-184: Add test cases for edge cases in user management.Consider adding tests for:
- User with missing required fields
- User with invalid email format
- Duplicate email scenarios
+@Test +void saveNewUser_ShouldThrowException_WhenEmailIsInvalid() { + User newUser = new User(); + newUser.setEmail("invalid-email"); + + assertThrows(IllegalArgumentException.class, () -> facebookSecurityService.saveNewUser(newUser)); +}
186-198: Enhance Facebook URL validation.Consider using a URL parser to validate the generated URL structure and query parameters instead of string comparison.
@Test void generateFacebookAuthorizeURLTest() { ReflectionTestUtils.setField(facebookSecurityService, "address", "http://localhost:8080"); ReflectionTestUtils.setField(facebookSecurityService, "facebookAppId", "12345"); ReflectionTestUtils.setField(facebookSecurityService, "facebookAppSecret", "6789"); String actual = facebookSecurityService.generateFacebookAuthorizeURL(); - assertEquals(expected, actual); + URI uri = new URI(actual); + Map<String, String> params = parseQueryParams(uri.getQuery()); + + assertEquals("https", uri.getScheme()); + assertEquals("www.facebook.com", uri.getHost()); + assertEquals("/v2.5/dialog/oauth", uri.getPath()); + assertEquals("12345", params.get("client_id")); + assertEquals("code", params.get("response_type")); + assertTrue(params.get("redirect_uri").contains("facebookSecurity/facebook")); }
257-269: Add assertions for default user properties.The test should verify that the created user has the expected default properties (role, status, etc.).
@Test void createNewUser_ShouldReturnUserWithGivenEmailAndUserName() { String email = "[email protected]"; String userName = "testUser"; when(jwtTool.generateTokenKey()).thenReturn("fakeTokenKey"); User user = facebookSecurityService.createNewUser(email, userName); assertNotNull(user); assertEquals(email, user.getEmail()); assertEquals(userName, user.getName()); assertEquals("fakeTokenKey", user.getRefreshTokenKey()); + assertEquals(Role.ROLE_USER, user.getRole()); + assertEquals(UserStatus.ACTIVATED, user.getUserStatus()); + assertEquals(EmailNotification.DISABLED, user.getEmailNotification()); + assertNotNull(user.getUuid()); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java(4 hunks)service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (4)
service/src/main/java/greencity/security/service/FacebookSecurityServiceImpl.java (3)
158-160: Consider more restrictive default privacy settings.Setting all privacy settings to PUBLIC by default may not align with privacy best practices.
237-237: Fix duplicate query parameters in Facebook request URL.The URL construction includes duplicate query parameters as the
userInfoUrlalready contains them.
248-252: Well-implemented JSON response handling!Good use of Optional for safe navigation of the JSON response tree, preventing potential NullPointerExceptions.
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (1)
245-255: Duplicate test method detected.This test method is a duplicate of
getUserInfoFromFacebook_ShouldThrowIOException_WhenResponseStatusIsNot200.Please refer to the previous review comment about consolidating duplicate test cases for Facebook API error handling.
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (3)
79-84: Consider enhancing the setUp method with better documentation and configuration.The setup method could benefit from:
- A descriptive comment explaining the configuration
- Using a constant for the hardcoded address
@BeforeEach void setUp() { + // Configure base URL for Facebook authentication and inject required dependencies + private static final String BASE_URL = "http://localhost:8060"; - ReflectionTestUtils.setField(facebookSecurityService, "address", "http://localhost:8060"); + ReflectionTestUtils.setField(facebookSecurityService, "address", BASE_URL); ReflectionTestUtils.setField(facebookSecurityService, "jwtTool", jwtTool); ReflectionTestUtils.setField(facebookSecurityService, "restClient", restClient); }
86-105: Enhance test assertions for user creation.The test could be more robust by adding assertions for:
- UUID generation
- Non-null checks for all user fields
- Default values for unspecified fields
@Test void createNewUser_ShouldReturnUserWithDefaultValues() { String email = "[email protected]"; String userName = "Test User"; String profilePicture = "profile.jpg"; String language = "1"; when(modelMapper.map(language, Long.class)).thenReturn(1L); User user = facebookSecurityService.createNewUser(email, userName, profilePicture, language); assertNotNull(user); + assertNotNull(user.getUuid(), "UUID should be generated"); assertEquals(email, user.getEmail()); assertEquals(userName, user.getName()); assertEquals(Role.ROLE_USER, user.getRole()); assertEquals(UserStatus.ACTIVATED, user.getUserStatus()); assertEquals(EmailNotification.DISABLED, user.getEmailNotification()); assertEquals(profilePicture, user.getProfilePicturePath()); assertEquals(ProfilePrivacyPolicy.PUBLIC, user.getShowLocation()); assertEquals(1L, user.getLanguage().getId()); + assertNotNull(user.getVerifyEmail(), "Verify email should be initialized"); + assertNotNull(user.getDateOfRegistration(), "Registration date should be set"); }
114-124: Consider testing additional error scenarios.The test for invalid Facebook data could be expanded to cover different types of API responses and error conditions.
@Test void authenticate_ShouldThrowException_WhenFacebookReturnsInvalidData() throws IOException { String fbToken = "invalid_token"; - HttpGet mockRequest = new HttpGet("http://fake-url.com"); when(httpClient.execute(any(HttpGet.class))).thenReturn(httpResponse); when(httpResponse.getStatusLine()).thenReturn(statusLine); - when(statusLine.getStatusCode()).thenReturn(400); + // Test different error scenarios + int[] errorCodes = {400, 401, 403, 500}; + for (int statusCode : errorCodes) { + when(statusLine.getStatusCode()).thenReturn(statusCode); + IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, + () -> facebookSecurityService.authenticate(fbToken, "en")); + assertTrue(exception.getMessage().contains(ErrorMessage.BAD_FACEBOOK_TOKEN)); + assertTrue(exception.getMessage().contains(String.valueOf(statusCode))); + } - IllegalArgumentException exception = assertThrows(IllegalArgumentException.class, - () -> facebookSecurityService.authenticate(fbToken, "en")); - assertTrue(exception.getMessage().contains(ErrorMessage.BAD_FACEBOOK_TOKEN)); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: build
🔇 Additional comments (1)
service/src/test/java/greencity/security/service/FacebookSecurityServiceImplTest.java (1)
195-206: Consolidate duplicate test methods for Facebook API error handling.The test methods
getUserInfoFromFacebook_ShouldThrowIOException_WhenResponseStatusIsNot200andgetUserInfoFromFacebook_ShouldThrowException_WhenFacebookReturnsErrorare testing the same scenario.Also applies to: 240-250
create methods that allow user to login using FaceBook account, add controller for FaceBook login
Summary by CodeRabbit
New Features
Bug Fixes
Tests