User reg with GitHub profile data#4#98
Conversation
…ser have hidden email, we don't get a null-value in return but rather a default value.
ghost
linked an issue
WalkthroughThe pull request introduces enhancements to the user management system, focusing on OAuth2 authentication and user creation. The changes include modifications to the Changes
Sequence DiagramsequenceDiagram
participant OAuth2 as OAuth2 Provider
participant LoginController as Login Controller
participant UserService as User Service
participant UserRepository as User Repository
participant Database as Database
OAuth2->>LoginController: Authenticate User
LoginController->>UserService: Check User Existence
UserService->>UserRepository: existsById(userId)
UserRepository-->>UserService: User Exists/Not Exists
alt User Does Not Exist
LoginController->>UserService: Create New User
UserService->>UserRepository: save(user)
UserRepository->>Database: Insert User
else User Exists
LoginController->>LoginController: Log User Exists
end
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🔭 Outside diff range comments (2)
src/main/java/org/fungover/system2024/user/entity/User.java (2)
Line range hint
23-24: Follow Java naming conventions.Use camelCase instead of snake_case for field names in Java classes.
-private String first_name; +private String firstName; -private String last_name; +private String lastName;Also applies to: 28-29
Line range hint
48-63: Update equals() and add hashCode() method.The equals() method should include the id field for proper entity comparison, and a corresponding hashCode() method should be implemented.
@Override public boolean equals(Object o) { if (this == o) return true; if (o == null || getClass() != o.getClass()) return false; User user = (User) o; + if (id != null ? !id.equals(user.id) : user.id != null) return false; - if (!first_name.equals(user.first_name)) return false; - if (!last_name.equals(user.last_name)) return false; + if (!firstName.equals(user.firstName)) return false; + if (!lastName.equals(user.lastName)) return false; if (!email.equals(user.email)) return false; return password.equals(user.password); } +@Override +public int hashCode() { + int result = id != null ? id.hashCode() : 0; + result = 31 * result + firstName.hashCode(); + result = 31 * result + lastName.hashCode(); + result = 31 * result + email.hashCode(); + result = 31 * result + password.hashCode(); + return result; +}
🧹 Nitpick comments (4)
src/main/java/org/fungover/system2024/controller/LoginController.java (2)
18-19: Use Constructor Injection Instead of Field InjectionUsing constructor injection is recommended over field injection (
@Autowiredon fields) as it promotes immutability, better testability, and follows best practices in Spring.You can modify the code as follows:
@Controller public class LoginController { private final UserService userService; private final Logger logger = LoggerFactory.getLogger(LoginController.class); public LoginController(UserService userService) { this.userService = userService; } @GetMapping("/") public String index(@AuthenticationPrincipal OAuth2User principal, Model model) { // existing code... } }
63-63: Add Error Handling for User CreationCurrently, there is no error handling around
userService.save(user);. If the save operation fails (e.g., due to database constraints), the application may crash.Wrap the save operation in a try-catch block to handle potential exceptions:
try { userService.save(user); logger.info("New user created with email '{}'", user.getEmail()); } catch (Exception e) { logger.error("Failed to create new user: {}", e.getMessage()); // handle the error appropriately }src/main/java/org/fungover/system2024/user/entity/User.java (1)
Line range hint
31-34: Consider case-insensitive email handling.Since email addresses are case-insensitive, consider adding a PrePersist/PreUpdate hook to normalize email addresses to lowercase to prevent duplicate entries that differ only in case.
@Size (max = 100) @NotBlank @Email @Column(name = "email", nullable = false, unique = true) private String email; +@PrePersist +@PreUpdate +private void prepareEmail() { + if (email != null) { + email = email.toLowerCase(); + } +}src/main/resources/schema.sql (1)
7-8: Reconsider default values for required fields.Using default values for email and password fields might mask validation issues and create security concerns. Consider:
- Removing default values and enforcing proper validation
- Using a more secure default password if defaults are necessary
- Adding a boolean flag to identify auto-generated credentials
- email VARCHAR(255) NOT NULL DEFAULT 'unkwown@mailaddress.something', - password VARCHAR(255) NOT NULL DEFAULT 'unknown_password', + email VARCHAR(255) NOT NULL, + password VARCHAR(255) NOT NULL, + is_temporary_credentials BOOLEAN NOT NULL DEFAULT FALSE,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/main/java/org/fungover/system2024/controller/LoginController.java(3 hunks)src/main/java/org/fungover/system2024/user/UserService.java(2 hunks)src/main/java/org/fungover/system2024/user/entity/User.java(1 hunks)src/main/java/org/fungover/system2024/user/repository/UserRepository.java(1 hunks)src/main/resources/application.properties(1 hunks)src/main/resources/schema.sql(1 hunks)
🔇 Additional comments (2)
src/main/java/org/fungover/system2024/user/entity/User.java (1)
17-19: LGTM! Proper ID configuration.The ID field is correctly configured with auto-generation strategy.
src/main/resources/application.properties (1)
21-22: LGTM! Appropriate OAuth2 configuration.The GitHub OAuth2 scopes are correctly configured with the minimum required permissions for user profile data access.
| if (!userService.existsById(userId)) { | ||
| User user = new User(); | ||
|
|
||
| if(principal.getAttribute("email") !=null) { | ||
| user.setEmail(principal.getAttribute("email")); | ||
| } else { | ||
| user.setEmail("unknown@example.something"); | ||
| } | ||
|
|
||
| if(principal.getAttribute("name") != null) { | ||
| user.setFirst_name(principal.getAttribute("name")); | ||
| } else { | ||
| user.setFirst_name("John"); | ||
| } | ||
|
|
||
| if (principal.getAttribute("name") != null) { | ||
| user.setLast_name(principal.getAttribute("name")); | ||
| } else { | ||
| user.setLast_name("Doe"); | ||
| } | ||
|
|
||
| if(principal.getAttribute("password") != null) { | ||
| user.setPassword(principal.getAttribute("password")); | ||
| } else { | ||
| user.setPassword("no password from this kid"); | ||
| } | ||
|
|
||
| userService.save(user); | ||
|
|
||
| logger.info("New user created with ID '{}'", userId); | ||
| } else { |
There was a problem hiding this comment.
Check User Existence by Email Instead of ID
Relying on the OAuth2 provider's user ID may not be consistent across different providers or may not be unique in your system. Checking user existence by email ensures that duplicate accounts aren't created and respects uniqueness constraints on the email field.
Update the code to check for user existence by email:
String email = principal.getAttribute("email");
if (email != null && !userService.existsByEmail(email)) {
// proceed to create user
} else {
// handle existing user or missing email
}Ensure that UserService and UserRepository have the existsByEmail method implemented.
| if(principal.getAttribute("email") !=null) { | ||
| user.setEmail(principal.getAttribute("email")); | ||
| } else { | ||
| user.setEmail("unknown@example.something"); | ||
| } |
There was a problem hiding this comment.
Handle Missing Email Appropriately
Setting a default email like "unknown@example.something" when the email is not available can cause issues with uniqueness constraints and doesn't provide meaningful information.
Consider the following options:
- Prompt the user to provide an email address.
- Abort the registration process if the email is essential.
- Modify the
Userentity to allownullemails if appropriate.
Ensure that the application handles cases where the email is missing without causing exceptions or invalid data entries.
| if(principal.getAttribute("name") != null) { | ||
| user.setFirst_name(principal.getAttribute("name")); | ||
| } else { | ||
| user.setFirst_name("John"); | ||
| } | ||
|
|
||
| if (principal.getAttribute("name") != null) { | ||
| user.setLast_name(principal.getAttribute("name")); | ||
| } else { | ||
| user.setLast_name("Doe"); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Properly Separate First Name and Last Name
Currently, both first_name and last_name are set to the same value from principal.getAttribute("name"), which likely contains the full name.
Refactor the code to split the full name:
String fullName = principal.getAttribute("name");
if (fullName != null) {
String[] nameParts = fullName.split(" ", 2);
user.setFirst_name(nameParts[0]);
if (nameParts.length > 1) {
user.setLast_name(nameParts[1]);
} else {
user.setLast_name("");
}
} else {
user.setFirst_name("John");
user.setLast_name("Doe");
}Alternatively, if the OAuth2 provider offers separate attributes for first name and last name (e.g., "given_name" and "family_name"), use those instead.
| if(principal.getAttribute("password") != null) { | ||
| user.setPassword(principal.getAttribute("password")); | ||
| } else { | ||
| user.setPassword("no password from this kid"); | ||
| } |
There was a problem hiding this comment.
Avoid Setting Password Directly and Handle Authentication Appropriately
OAuth2 providers typically do not supply a user's password. Setting a placeholder password like "no password from this kid" is insecure and may lead to security vulnerabilities.
Consider the following actions:
- Remove the password handling in this context.
- Modify the
Userentity to accommodate users authenticated via OAuth2 without requiring a password. - If a password is necessary for your application, consider generating a secure random password and prompting users to change it.
Example modification:
- if(principal.getAttribute("password") != null) {
- user.setPassword(principal.getAttribute("password"));
- } else {
- user.setPassword("no password from this kid");
- }Instead, you might remove this block or adjust your User entity accordingly.
| if (!userService.existsById(userId)) { | ||
| User user = new User(); |
There was a problem hiding this comment.
Verify User Identification Method
Using principal.getAttribute("id") as an Integer may not be reliable since the id attribute from OAuth2 providers can be a String or another data type. This might lead to ClassCastException at runtime.
Consider retrieving the user ID as a String:
- Integer userId = principal.getAttribute("id");
+ String userId = principal.getAttribute("id");Ensure that the User entity and related methods are updated to accommodate a String type for the user ID.
Committable suggestion skipped: line range outside the PR's diff.
| import org.springframework.data.repository.ListCrudRepository; | ||
|
|
||
| public interface UserRepository extends ListCrudRepository<User, Integer> { | ||
| public interface UserRepository extends JpaRepository<User, Integer> { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Method to Check User Existence by Email
To support checking for existing users by email in the LoginController, add a method to the UserRepository.
Add the following method:
public interface UserRepository extends JpaRepository<User, String> {
boolean existsByEmail(String email);
}Note: Ensure that the User entity has the email field annotated with @Column(unique = true) if you want to enforce uniqueness at the database level.
| public boolean existsById(Integer userId) { | ||
| return userRepository.existsById(userId); | ||
| } | ||
| public void save(User user) { | ||
| userRepository.save(user); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update Methods to Support Checking and Saving by Email
In line with checking user existence by email, add the existsByEmail method in UserService and adjust other methods if necessary.
Add the following method:
public boolean existsByEmail(String email) {
return userRepository.existsByEmail(email);
}Update the save method if you need to handle any additional logic related to email:
public void save(User user) {
userRepository.save(user);
log.info("User with email '{}' saved successfully.", user.getEmail());
}| import lombok.extern.slf4j.Slf4j; | ||
| import org.fungover.system2024.exception.ResourceNotFoundException; | ||
| import org.fungover.system2024.user.dto.UserDto; | ||
| import org.fungover.system2024.user.entity.User; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Ensure Consistent User ID Data Types Across the Application
Since the User entity uses Integer for the user ID, but OAuth2 might provide an ID as a String, consider updating the User entity and related repositories to use String for the user ID.
Update the User entity:
- private Integer id;
+ private String id;Update the UserRepository and UserService accordingly to reflect this change.
Committable suggestion skipped: line range outside the PR's diff.
| last_name VARCHAR(255) NOT NULL, | ||
| email VARCHAR(255) NOT NULL, | ||
| password VARCHAR(255) NOT NULL, | ||
| email VARCHAR(255) NOT NULL DEFAULT 'unkwown@mailaddress.something', |
There was a problem hiding this comment.
Fix typo in default email value.
The word "unknown" is misspelled as "unkwown" in the default email value.
- email VARCHAR(255) NOT NULL DEFAULT 'unkwown@mailaddress.something',
+ email VARCHAR(255) NOT NULL DEFAULT 'unknown@mailaddress.something',📝 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.
| email VARCHAR(255) NOT NULL DEFAULT 'unkwown@mailaddress.something', | |
| email VARCHAR(255) NOT NULL DEFAULT 'unknown@mailaddress.something', |
AnnaLenaO
left a comment
AnnaLenaO
left a comment
There was a problem hiding this comment.
The code is easy to read and follow. Very nice!
Coderabbitai suggests we can verify by user email in database, instead of user ID. That could also solve the problem with the project crashing if such a user already exists in database.
| email VARCHAR(255) NOT NULL, | ||
| password VARCHAR(255) NOT NULL, | ||
| email VARCHAR(255) NOT NULL DEFAULT 'unkwown@mailaddress.something', | ||
| password VARCHAR(255) NOT NULL DEFAULT 'unknown_password', |
There was a problem hiding this comment.
Suggest to remove DEFAULT 'unknown_password' due to security risk.
It might be possible to sync how the password it set, with som other implementation for the application.
| import org.springframework.data.repository.ListCrudRepository; | ||
|
|
||
| public interface UserRepository extends ListCrudRepository<User, Integer> { | ||
| public interface UserRepository extends JpaRepository<User, Integer> { |
There was a problem hiding this comment.
For this application it might be enough with extends ListCrudRepository or extends ListPagingAndSortingRepository, instead of the full etends JpaRepository.
1 participant
Created logic to get Github-user data and save that information in variables. Then creating a new user in database based on the values we have from our GitHub-user data. As of now, we can create a new user but if we try to create an another one with same email address, the projekt will crash. You need to remove the created user from database (user table) to try it again.
Summary by CodeRabbit
Release Notes
New Features
Improvements
Configuration Updates
These changes improve the application's user management and authentication processes, providing a more robust and flexible user experience.