-
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
Added JWT support #11
Conversation
…in beans. Implemented UserDetails interface in the User class. Added the implementation for UserDetailsService interface. Added Role entity. Added RoleRepository. Added roles field to the User entity. Added Liquibase changesets to insert roles into DB and assign roles for users.
…in beans. Implemented UserDetails interface in the User class. Added the implementation for UserDetailsService interface. Added Role entity. Added RoleRepository. Added roles field to the User entity. Added Liquibase changesets to insert roles into DB and assign roles for users.
# Conflicts: # src/main/java/mate/academy/bookstore/model/Role.java # src/main/java/mate/academy/bookstore/repository/role/RoleRepository.java # src/main/java/mate/academy/bookstore/service/impl/UserServiceImpl.java # src/main/resources/db/changelog/changes/03-create-roles-table.yaml # src/main/resources/db/changelog/changes/05-insert-roles-into-database.yaml # src/main/resources/db/changelog/changes/07-assign-admin-user-to-role-admin.yaml
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, but there are some minor changes, that should be done according my comments.
String email, | ||
@NotBlank | ||
@Size(min = 8, message = "Password must be at least 8 characters 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.
what about max parameter?
|
||
public record UserLoginRequestDto( | ||
@NotBlank | ||
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.
@Size(min = 8, max = 20) |
src/main/java/mate/academy/bookstore/exception/GlobalExceptionHandler.java
Outdated
Show resolved
Hide resolved
String bearerToken = request.getHeader("Authorization"); | ||
if (bearerToken != null && bearerToken.startsWith("Bearer ")) { | ||
return bearerToken.substring(7); | ||
} | ||
return null; |
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 avoid magic numbers and String literals
…nd auth header strings to constant. Removed redundant AuthenticationException handling in GlobalExceptionHandler. Added max size limits for email and password in UserLoginRequestDto
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.
Nice, left minor comments
@Bean | ||
public AuthenticationManager authenticationManager( | ||
AuthenticationConfiguration authenticationConfiguration | ||
) throws Exception { | ||
return authenticationConfiguration.getAuthenticationManager(); | ||
} |
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.
Is it necessary to add this?
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.
I added it because Spring can't awtowire AuthenticationManager and app failed to start:
Could not autowire. No beans of 'AuthenticationManager' type found.
private String getToken(HttpServletRequest request) { | ||
String bearerToken = request.getHeader(AUTHORIZATION); | ||
if (bearerToken != null && bearerToken.startsWith(BEARER_PREFIX)) { | ||
return bearerToken.substring(START_INDEX); |
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.
return bearerToken.substring(START_INDEX); | |
return bearerToken.substring(BEARER_PREFIX.length); |
@RequiredArgsConstructor | ||
@Component | ||
public class JwtAuthenticationFilter extends OncePerRequestFilter { | ||
private static final int START_INDEX = 7; |
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.
private static final int START_INDEX = 7; |
unique: true |
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.
unique: true | |
unique: true | |
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.
I believe GitHub doesn't display empty lines at the end because it already exists
No description provided.