-
Notifications
You must be signed in to change notification settings - Fork 0
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Spring security #10
Spring security #10
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! left minor comments
@@ -23,6 +24,7 @@ | |||
@Tag(name = "Book controller", description = "endpoints for managing library") | |||
@RequiredArgsConstructor | |||
@RestController | |||
//@PreAuthorize("hasAnyRole('ROLE_ADMIN', 'ROLE_USER')") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please don`t push commented code on remote
@@ -2,8 +2,10 @@ spring.application.name=BookStore | |||
spring.datasource.url=jdbc:mysql://localhost/book_store?serverTimezone=UTC | |||
spring.datasource.driverClassName=com.mysql.cj.jdbc.Driver | |||
spring.datasource.username=root | |||
spring.datasource.password=pass | |||
spring.datasource.password=!Vladius080197 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is unsafe to push passwords on git, you should left this field blank or use env variable
|
||
@Override | ||
public UserResponseDto register(UserRegistrationRequestDto requestDto) { | ||
if (userRepository.existsUserByEmail(requestDto.getEmail())) { | ||
Role role = roleRepo.findByRole(RoleName.ROLE_ADMIN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why any user that registers is admin?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good job! Let's improve your solution ;)
|
||
@Entity | ||
@Table(name = "users") | ||
@Getter | ||
@Setter | ||
public class User { | ||
public class User implements UserDetails { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is better to override all methods with UserDetails. In particular, the isEnabled method will have its own implementation in your solution ;)
import project.bookstore.model.Role; | ||
import project.bookstore.model.RoleName; | ||
|
||
public interface RoleRepo extends JpaRepository<Role, Integer> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public interface RoleRepo extends JpaRepository<Role, Integer> { | |
public interface RoleRepository extends JpaRepository<Role, Long> { |
Don't use abbreviations in the names of variables, classes, and methods.
Also, you used Long as an id variable type.
@Repository | ||
public interface UserRepo extends JpaRepository<User, Integer> { | ||
Boolean existsUserByEmail(String email); | ||
public interface UserRepo extends JpaRepository<User, Long> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
public interface UserRepo extends JpaRepository<User, Long> { | |
public interface UserRepository extends JpaRepository<User, Long> { |
type: tinyint | ||
defaultValueBoolean: false | ||
constraints: | ||
nullable: false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left one empty line at the end of the file ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
one comment
.authorizeHttpRequests( | ||
auth -> auth | ||
.requestMatchers( | ||
HttpMethod.POST, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpMethod.POST, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome 😎
No description provided.