43 add security compatibility #57
Conversation
…yeredd/interview-evaluations into 43-add-security-compatibility
|
|
||
| @EnableJpaRepositories(basePackageClasses=PersonRepository.class) | ||
| @SpringBootApplication | ||
| public class InterviewEvaluationsApplication { |
There was a problem hiding this comment.
Add a private constructor to hide the implicit public one. 
|
@tjkemper please review this. |
|
This is a lot in one PR. Initial reactions
|
|
Claims have a secret: mySecret (see application.yml and JwtTokenUtil) |
… extra to be discarded when external token generation/validation is implemented
| private UserRepository userRepository; | ||
|
|
||
| @Override | ||
| public UserDetails loadUserByUsername(String username) throws UsernameNotFoundException { |
There was a problem hiding this comment.
Remove the declaration of thrown exception 'org.springframework.security.core.userdetails.UsernameNotFoundException' which is a runtime exception.
| private final boolean enabled; | ||
| private final Date lastPasswordResetDate; | ||
|
|
||
| public JwtUser( |
There was a problem hiding this comment.
Constructor has 9 parameters, which is greater than 7 authorized.
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| audience = (String) claims.get(CLAIM_KEY_AUDIENCE); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Either log or rethrow this exception.
| Date expires; | ||
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| expires = claims.getExpiration(); |
There was a problem hiding this comment.
NullPointerException might be thrown as 'claims' is nullable here
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| expires = claims.getExpiration(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Either log or rethrow this exception.
| Date created; | ||
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| created = new Date((Long) claims.get(CLAIM_KEY_CREATED)); |
There was a problem hiding this comment.
NullPointerException might be thrown as 'claims' is nullable here
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| created = new Date((Long) claims.get(CLAIM_KEY_CREATED)); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Either log or rethrow this exception.
| String refreshedToken; | ||
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| claims.put(CLAIM_KEY_CREATED, new Date()); |
There was a problem hiding this comment.
NullPointerException might be thrown as 'claims' is nullable here
| .setSigningKey(secret) | ||
| .parseClaimsJws(token) | ||
| .getBody(); | ||
| } catch (ExpiredJwtException | MalformedJwtException | SignatureException | UnsupportedJwtException | IllegalArgumentException e) { |
There was a problem hiding this comment.
Either log or rethrow this exception.
| final Claims claims = getClaimsFromToken(token); | ||
| claims.put(CLAIM_KEY_CREATED, new Date()); | ||
| refreshedToken = generateToken(claims); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Either log or rethrow this exception.
| String username; | ||
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| username = claims.getSubject(); |
There was a problem hiding this comment.
NullPointerException might be thrown as 'claims' is nullable here
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| username = claims.getSubject(); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
Either log or rethrow this exception.
| String audience; | ||
| try { | ||
| final Claims claims = getClaimsFromToken(token); | ||
| audience = (String) claims.get(CLAIM_KEY_AUDIENCE); |
There was a problem hiding this comment.
NullPointerException might be thrown as 'claims' is nullable here
tjkemper
left a comment
tjkemper
left a comment
There was a problem hiding this comment.
Overall good progress.
Focus on verifying the JWT has the correct signature.
The API does not need to worry about usernames/passwords or generating tokens.
| authReq.getPassword()) | ||
| ); | ||
|
|
||
| SecurityContextHolder.getContext().setAuthentication(authentication); |
There was a problem hiding this comment.
Let's avoid using SecurityContext. We want our API to be stateless.
| private String tokenHeader; | ||
|
|
||
| @Override | ||
| protected void doFilterInternal(HttpServletRequest request, HttpServletResponse response, FilterChain chain) throws ServletException, IOException { |
There was a problem hiding this comment.
You're on the right track here.
May want something like:
try {
Jwts.parser().setSigningKey(key).parseClaimsJws(compactJws);
//OK, we can trust this JWT
} catch (SignatureException e) {
//don't trust the JWT!
}
Notice we do not know anything about the user. We only care that the JWT was signed with our secret and it wasn't tampered with.
…cation to new project
|
Build fails for non-existent dependency (JwtAuthenticationFilter [8,37]) The dependency is a local second project. The dependency is in the POM and the directory is in the classpath. Any ideas as to why it can't find this dependency would be appreciated. POM: Classpath: |
…are to be removed and placed in a seperate Authorization Server service. All files in the 'static' folder likewise belong to the future Authorization Server service
|
|
||
| String token = req.getHeader(tokenHeader); | ||
| String username = jwtTokenUtil.getUsernameFromToken(token); | ||
| JwtUser user = (JwtUser) userDetailsService.loadUserByUsername(username); |
There was a problem hiding this comment.
Immediately return this expression instead of assigning it to the temporary variable "user".
|
|
||
| //This value can be found in the application.yml file | ||
| @RequestMapping(value = "${jwt.route.authentication.path}", method = RequestMethod.POST) | ||
| public ResponseEntity<?> createAuthenticationToken(@RequestBody JwtAuthenticationRequest authReq, Device dev) throws AuthenticationException{ |
There was a problem hiding this comment.
Remove usage of generic wildcard type.
Remove the declaration of thrown exception 'org.springframework.security.core.AuthenticationException' which is a runtime exception.
|
|
||
| //This value can be found in the application.yml file | ||
| @RequestMapping(value = "${jwt.route.authentication.refresh}", method = RequestMethod.GET) | ||
| public ResponseEntity<?> refreshAndGetAuthenticationToken(HttpServletRequest req){ |
There was a problem hiding this comment.
Remove usage of generic wildcard type.
…yeredd/interview-evaluations into 43-add-security-compatibility
|
SonarQube analysis reported 18 issues
Watch the comments in this conversation to review them. |
4 participants
Security compatibility using JWT.
Temporary token generator/validator is JwtTokenUtil.
Security classes (by package):
com.revature.config: WebSecurityConfig
com.revature.repositories: UserRepository
com.revature.model.security: Authority, AuthorityName, User
com.revature.security: JwtAuthenticationEntryPoint, JwtAuthenticationFilter, JwtAuthenticationRequest, JwtAuthenticationSuccessHandler, JwtTokenUtil (TEMP), JwtUser, JwtUserFactory
com.revature.security.controllers: AuthenticationRestController, UserRestController
com.revature.security.exceptions: JwtTokenMissingException
com.revature.security.service: JwtAuthenticationResponse, JwtUserDetailsServiceImpl