Development: Enable bearer authentication
#9403
Conversation
WalkthroughThe pull request introduces significant changes to the JWT validation process across multiple classes. The Changes
Possibly related PRs
Suggested labels
Suggested reviewers
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
Documentation and Community
|
github-actions
bot
added
server
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range comments (3)
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java (3)
Line range hint
72-89: LGTM! Consider standardizing error responses.The changes to the
authorizemethod align well with the PR objectives. Returning the JWT token in the response body enables clients that can't use cookies to authenticate successfully. This change maintains RESTful principles and doesn't introduce apparent security risks.A minor suggestion for improvement:
Consider standardizing the error response for consistency with the
authorizeSAML2method. You could include an error message in the response body or a custom header for unauthorized requests:- return ResponseEntity.status(HttpStatus.UNAUTHORIZED).build(); + return ResponseEntity.status(HttpStatus.UNAUTHORIZED) + .header("X-artemisApp-error", "Invalid credentials") + .build();This change would provide more informative error responses across different authentication methods.
Line range hint
1-156: Consider updating documentation and client-side code.The changes to the
authorizemethod successfully enable bearer authentication without affecting the overall structure or responsibility of thePublicUserJwtResourceclass. This modification aligns well with the PR objectives and maintains backwards compatibility.To ensure a smooth transition and maintain system coherence:
- Update any relevant API documentation to reflect the new response format of the
authorizeendpoint.- Review and update client-side code that interacts with this endpoint to handle the new response format correctly.
- Consider adding a note in the PR description about potential client-side changes required.
These steps will help maintain consistency across the system and prevent potential integration issues.
Line range hint
1-156: Refactor suggestion: Improve code reuse for error handling.The code generally adheres well to the provided coding guidelines. To further improve code quality and maintainability:
Consider extracting the error handling logic into a separate method to promote code reuse. This change would align with the principles of avoiding duplication and improving maintainability. Here's a suggested refactor:
private ResponseEntity<String> handleAuthenticationError(String errorMessage) { log.warn(errorMessage); return ResponseEntity.status(HttpStatus.UNAUTHORIZED) .header("X-artemisApp-error", errorMessage) .build(); }Then, in the
authorizemethod:catch (BadCredentialsException ex) { return handleAuthenticationError("Wrong credentials during login for user " + loginVM.getUsername()); }This refactoring would make it easier to maintain consistent error handling across different authentication methods in the future.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
- src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (2 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (4)
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java (1)
Line range hint
1-156: Overall assessment: Changes are well-implemented and align with project goals.The modifications to the
PublicUserJwtResourceclass successfully implement bearer authentication as intended. The changes are focused, maintain backwards compatibility, and adhere to the project's coding guidelines.Key points:
- The
authorizemethod now returns the JWT token in the response body, enabling clients that can't use cookies.- The changes don't introduce apparent security vulnerabilities.
- The code maintains good quality and follows most coding guidelines.
Suggestions for improvement:
- Standardize error responses across authentication methods.
- Extract error handling logic to a separate method for better code reuse.
- Update relevant documentation and client-side code to reflect the changes.
These improvements will enhance code maintainability, consistency, and overall system integration. Once these minor adjustments are addressed, the changes appear ready for merging.
src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java (2)
22-22: LGTM: Import changes align with the new JWT extraction method.The removal of the
Cookieimport and addition ofHttpServletRequestimport are consistent with the changes in thebeforeHandshakemethod, supporting the new JWT extraction approach.
Line range hint
1-389: Overall changes enhance WebSocket authentication while maintaining existing functionality.The modifications to this file successfully implement bearer token authentication for WebSocket connections, as outlined in the PR objectives. The changes are minimal and focused, affecting only the necessary parts of the code:
- Updated imports to support the new JWT extraction method.
- Modified the
beforeHandshakemethod to useJWTFilter.extractValidJwt, enabling both cookie-based and bearer token authentication.These changes improve the flexibility of the authentication mechanism, particularly benefiting clients that cannot use cookies, such as the mentioned VSCode Plugin. The implementation adheres to the coding guidelines, especially the "stateless" principle for REST APIs.
The rest of the file remains unchanged, preserving the existing WebSocket configuration and functionality. This targeted approach minimizes the risk of unintended side effects while achieving the desired enhancement.
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (1)
55-65: Ensure consistent token validation orderCurrently, the method first checks the cookie for a valid JWT and then the
Authorizationheader. Ensure that this order aligns with the intended authentication precedence. If theAuthorizationheader should take precedence, consider checking it first.To confirm the desired precedence, please verify if clients are expected to prioritize the
Authorizationheader over cookies.
src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
janthoXO
added
ready for review
and removed
security
There was a problem hiding this comment.
Nice changes! I really like the way you adapt to existing code and implement such an important feature in such a lightweight way 🔥
I left a couple of aspects, please answer in the thread. You also have some server-tests failing which might need your attention..
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (1)
- src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (5)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (5)
12-12: Correct Use of@NullableAnnotationThe
@Nullableannotation is now correctly imported fromorg.springframework.lang.Nullable, enhancing compatibility within the Spring Framework and avoiding potential conflicts withjavax.annotation.Nullable.
35-43: Proper Extraction and Authentication of JWT TokensThe
doFiltermethod has been appropriately updated to extract the JWT token usingextractValidJwt. The authentication is effectively set in theSecurityContextHolderwhen a valid token is found, ensuring that both cookie-based and header-based tokens are supported.
54-64: Efficient Consolidation of Token Extraction LogicThe
extractValidJwtmethod efficiently consolidates the logic for retrieving a valid JWT token from both the cookie and the Authorization header. This improves code readability and adheres to the DRY (Don't Repeat Yourself) principle.
72-77: Robust Handling ingetJwtFromCookieMethodThe
getJwtFromCookiemethod correctly checks for a null cookie and returns the JWT value when available. This ensures robustness in scenarios where the cookie might not be present.
85-91: Accurate Extraction and Trimming ingetJwtFromBearerMethodThe
getJwtFromBearermethod accurately extracts the JWT token from the Authorization header and trims any leading or trailing whitespace. This prevents token validation issues due to formatting inconsistencies.
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
janthoXO
changed the title
General Enable bearer authentication
janthoXO
changed the title
General Enable bearer authenticationGeneral: Enable bearer authentication
janthoXO
requested review from
SimonEntholzer,
pzdr7,
magkue,
az108,
magaupp and
bassner
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java
Outdated
Show resolved
Hide resolved
janthoXO
dismissed stale reviews from coderabbitai[bot] and iyannsch
via
b59f8ad
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (2)
- src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java (1 hunks)
- src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_importssrc/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (1)
Pattern
src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (2)
65-85: Add security audit loggingFor security-critical operations, it's important to log authentication attempts and failures. This helps with security auditing and troubleshooting.
Apply this diff to add logging:
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; public class JWTFilter extends GenericFilterBean { + private static final Logger logger = LoggerFactory.getLogger(JWTFilter.class); public static @Nullable String extractValidJwt(HttpServletRequest httpServletRequest, TokenProvider tokenProvider) { var cookie = WebUtils.getCookie(httpServletRequest, JWT_COOKIE_NAME); var authHeader = httpServletRequest.getHeader(AUTHORIZATION_HEADER); if (cookie == null && authHeader == null) { + logger.debug("No authentication token found in request"); return null; } if (cookie != null && authHeader != null) { + logger.warn("Multiple authentication methods detected in request from IP: {}", httpServletRequest.getRemoteAddr()); throw new IllegalArgumentException("Multiple authentication methods detected: Both JWT cookie and Bearer token are present"); } String jwtToken = cookie != null ? getJwtFromCookie(cookie) : getJwtFromBearer(authHeader); if (!isJwtValid(tokenProvider, jwtToken)) { + logger.warn("Invalid authentication token detected from IP: {}", httpServletRequest.getRemoteAddr()); return null; } + logger.debug("Valid authentication token processed"); return jwtToken; }
Security concern: JWT tokens are being logged
There are two instances where JWT tokens are being logged, which could potentially expose sensitive authentication information:
- In
TokenProvider.java:log.info("Invalid JWT token: {}", authToken);
- In
PublicLtiResource.java:log.info("LTI request: JWT token is invalid: {}", token, e);These logging statements should be modified to:
- Remove the actual token value from the log message
- Use appropriate log levels (e.g., debug for invalid tokens)
- Only log non-sensitive metadata if needed for debugging
🔗 Analysis chain
Line range hint
1-124: Verify token logging practicesLet's ensure that JWT tokens are not being logged anywhere in the codebase to prevent security risks.
Let me analyze the search results carefully. I notice two instances of token logging in the codebase:
- In
TokenProvider.java:log.info("Invalid JWT token: {}", authToken);
- In
PublicLtiResource.java:log.info("LTI request: JWT token is invalid: {}", token, e);Let me verify if these are the only instances and check their context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for potential JWT token logging # Expected results: No instances of logging JWT tokens should be found echo "Checking for potential JWT token logging..." # Search for logging statements that might contain JWT tokens rg -i "log\.(debug|info|warn|error).*jwt.*token|bearer" --type java # Search for debug print statements that might expose tokens rg -i "system\.out\.print.*jwt.*token|bearer" --type java echo "Note: Review any matches carefully to ensure tokens are not being logged"Length of output: 10227
Script:
#!/bin/bash # Get more context around the logging statements echo "Context for TokenProvider.java logging:" rg -B 5 -A 5 'log\.info.*Invalid JWT token' --type java echo -e "\nContext for PublicLtiResource.java logging:" rg -B 5 -A 5 'LTI request: JWT token is invalid' --type javaLength of output: 2808
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (1)
50-53:
Clear SecurityContext when JWT is invalid
When no valid JWT is found, the SecurityContext should be explicitly cleared to prevent any potential security issues from previous authentications.
Apply this diff to improve security:
if (jwtToken != null) {
Authentication authentication = this.tokenProvider.getAuthentication(jwtToken);
SecurityContextHolder.getContext().setAuthentication(authentication);
+} else {
+ SecurityContextHolder.clearContext();
}Likely invalid or redundant comment.
|
|
||
| if (cookie != null && authHeader != null) { | ||
| // Single Method Enforcement: Only one method of authentication is allowed | ||
| throw new IllegalArgumentException("Multiple authentication methods detected: Both JWT cookie and Bearer token are present"); |
There was a problem hiding this comment.
If I remember correctly, we wanted to return no reason to the client if that happens (as it can only come from unintended use)?
There was a problem hiding this comment.
This error message is only handled internally. The client does only see a bad request.
A few lines above handles this error. And only returns a badrequest without body
catch (IllegalArgumentException e) { httpServletResponse.sendError(HttpServletResponse.SC_BAD_REQUEST); return; }
| void testJWTFilterBearer() throws Exception { | ||
| UsernamePasswordAuthenticationToken authentication = new UsernamePasswordAuthenticationToken("test-user", "test-password", | ||
| Collections.singletonList(new SimpleGrantedAuthority(Role.STUDENT.getAuthority()))); | ||
|
|
||
| String jwt = tokenProvider.createToken(authentication, false); | ||
| MockHttpServletRequest request = new MockHttpServletRequest(); | ||
| request.setCookies(new Cookie(JWTFilter.JWT_COOKIE_NAME, jwt)); | ||
| request.addHeader("Authorization", "Bearer " + jwt); | ||
| request.setRequestURI("/api/test"); | ||
|
|
||
| MockHttpServletResponse response = new MockHttpServletResponse(); | ||
| MockFilterChain filterChain = new MockFilterChain(); | ||
| jwtFilter.doFilter(request, response, filterChain); | ||
| assertThat(response.getStatus()).isEqualTo(HttpStatus.BAD_REQUEST.value()); | ||
| } | ||
|
|
||
| @Test | ||
| void testJWTFilterCookieAndBearer() throws Exception { |
There was a problem hiding this comment.
I think you swapped the names of these tests. The first one has something to do with a cookie, not the second, right?
Checklist
General
Server
Motivation and Context
For some clients it is best practise or they are only able to use authentication with a
AuthorizationHeader, so called bearer. One client which is not able to use cookies is the VSCode Plugin. Therefore Artemis should be adjusted to use Bearer Tokens as wellDescription
The Authentication now also accepts JWT in the Authentication Header. It checks first for a valid JWT in the cookie and afterwards in the Authentication Header.
The authenticate endpoint is adjusted accordingly to also return the token in the body
Steps for Testing
Postman testing or code review:
Postman Testing:
ArtemisTokens.postman_collection.json
Send a request to POST
{{base_url}}/api/public/authenticateVerify that you get a token in the body response
Send a request to GET
{{base_url}}/api/courses/for-dashboardVerify that you get a 200 response
Send a request to GET
{{base_url}}/api/courses/for-dashboardwith the headerAuthorization:Bearer {{access_token}}from the body of the authenticate requestverify that you get a 400 because you used both Authorization Header and Cookie
Delete the cookie in Postman
Send a request to GET
{{base_url}}/api/courses/for-dashboardwith the headerAuthorization:Bearer {{access_token}}from the body of the authenticate requestverify you now get a 200 because only the Authorization Header is used
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
Chores
@coderabbitai pause