Skip to content
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

Merged
merged 15 commits into from
May 15, 2024
Merged

Added JWT support #11

merged 15 commits into from
May 15, 2024

Conversation

nklimovych
Copy link
Owner

No description provided.

…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.
@nklimovych nklimovych changed the title Jwt impl Added JWT support May 13, 2024
nklimovych and others added 10 commits May 13, 2024 10:37
…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
Copy link

@d1sam d1sam left a 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.

@Email
String email,
@NotBlank
@Size(min = 8, message = "Password must be at least 8 characters long")
Copy link

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
@Email
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
@Email
@Email
@Size(min = 8, max = 20)

Comment on lines 43 to 47
String bearerToken = request.getHeader("Authorization");
if (bearerToken != null && bearerToken.startsWith("Bearer ")) {
return bearerToken.substring(7);
}
return null;
Copy link

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
@nklimovych nklimovych requested a review from d1sam May 14, 2024 12:40
Copy link

@andrii-hoienko andrii-hoienko left a 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

Comment on lines +60 to +65
@Bean
public AuthenticationManager authenticationManager(
AuthenticationConfiguration authenticationConfiguration
) throws Exception {
return authenticationConfiguration.getAuthenticationManager();
}

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?

Copy link
Owner Author

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);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
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;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
private static final int START_INDEX = 7;

unique: true

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
unique: true
unique: true

Copy link
Owner Author

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

@nklimovych nklimovych merged commit 7e352d8 into main May 15, 2024
2 checks passed
@nklimovych nklimovych deleted the jwt-impl branch May 15, 2024 13:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants