Development: Enable bearer authentication
#9403
Conversation
WalkthroughThe pull request introduces significant changes to the JWT authentication mechanism within the WebSocket handshake process and the handling of JWT tokens. Key modifications include the extraction and validation of JWTs directly from the Changes
Possibly related PRs
Suggested labels
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmdsrc/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. 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
janthoXO
added
ready for review
ready to merge
too-long-open
|
In the past, there was a login mechanism that was very similar to that. Nevertheless, it has been replaced; I'm not sure about the reasons, but they should be checked :) |
|
There hasn't been any activity on this pull request recently. Therefore, this pull request has been automatically marked as stale and will be closed if no further activity occurs within seven days. Thank you for your contributions. |
we changed the authentication for the client as the token was stored in the local storage of the browser, which is not secure. As now we not only have the browser client but also other services which need to authenticate, we reintroduce the bearer authentication. The artemis client will still use the good to be secure but other services like the VSCode plugin will use a bearer token. |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: ASSERTIVE
📒 Files selected for processing (3)
src/test/java/de/tum/cit/aet/artemis/core/authentication/AuthenticationIntegrationTestHelper.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/authentication/InternalAuthenticationIntegrationTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilterTest.java(2 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
src/test/java/de/tum/cit/aet/artemis/core/authentication/AuthenticationIntegrationTestHelper.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/core/authentication/InternalAuthenticationIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilterTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
📓 Learnings (1)
src/test/java/de/tum/cit/aet/artemis/core/authentication/InternalAuthenticationIntegrationTest.java (3)
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/test/java/de/tum/in/www1/artemis/authentication/UserLocalVcIntegrationTest.java:29-33
Timestamp: 2024-10-08T15:35:52.595Z
Learning: When addressing missing test coverage, ensure that new tests specifically validate the functionality introduced in the PR, such as token-based authentication.
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/test/java/de/tum/in/www1/artemis/authentication/UserLocalVcIntegrationTest.java:29-33
Timestamp: 2024-10-08T15:35:42.972Z
Learning: When addressing missing test coverage, ensure that new tests specifically validate the functionality introduced in the PR, such as token-based authentication.
Learnt from: SimonEntholzer
PR: ls1intum/Artemis#8929
File: src/test/java/de/tum/in/www1/artemis/authentication/UserLocalVcIntegrationTest.java:29-33
Timestamp: 2024-07-16T19:27:15.402Z
Learning: When addressing missing test coverage, ensure that new tests specifically validate the functionality introduced in the PR, such as token-based authentication.
🔇 Additional comments (2)
src/test/java/de/tum/cit/aet/artemis/core/authentication/AuthenticationIntegrationTestHelper.java (1)
25-28: Implementation follows test conventions and maintains consistency.
The new method maintains the class's assertion style and complements the existing cookie validation method.
src/test/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilterTest.java (1)
50-50: LGTM! Descriptive test method name.
The rename from testJWTFilter to testJWTFilterCookie clearly indicates the test's focus on cookie-based JWT authentication, improving test readability and maintainability.
...est/java/de/tum/cit/aet/artemis/core/authentication/AuthenticationIntegrationTestHelper.java
Show resolved
Hide resolved
src/test/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilterTest.java
Show resolved
Hide resolved
| @@ -227,6 +227,7 @@ void testJWTAuthentication() throws Exception { | |||
|
|
|||
| MockHttpServletResponse response = request.postWithoutResponseBody("/api/public/authenticate", loginVM, HttpStatus.OK, httpHeaders); | |||
| AuthenticationIntegrationTestHelper.authenticationCookieAssertions(response.getCookie("jwt"), false); | |||
| AuthenticationIntegrationTestHelper.authenticationBearerTokenAssertions(response.getHeader("Authorization")); | |||
There was a problem hiding this comment.
🧹 Nitpick (assertive)
LGTM! Consider adding more test cases.
The addition of Bearer token validation in the authentication test is appropriate and aligns with the PR objectives. The test now properly validates both cookie-based and header-based JWT authentication.
Consider adding the following test cases to improve coverage:
- Test authentication with only Bearer token (no cookie)
- Test authentication with invalid Bearer token format
- Test authentication with expired Bearer token
Example structure for the first suggested test:
@Test
@WithAnonymousUser
void testJWTAuthenticationWithBearerTokenOnly() throws Exception {
LoginVM loginVM = new LoginVM();
loginVM.setUsername(USERNAME);
loginVM.setPassword(USER_PASSWORD);
loginVM.setRememberMe(false); // Disable cookie
HttpHeaders httpHeaders = new HttpHeaders();
httpHeaders.add("User-Agent", "Mozilla/5.0");
MockHttpServletResponse response = request.postWithoutResponseBody("/api/public/authenticate", loginVM, HttpStatus.OK, httpHeaders);
// Verify no cookie is present
assertThat(response.getCookie("jwt")).isNull();
// Verify Bearer token is present and valid
AuthenticationIntegrationTestHelper.authenticationBearerTokenAssertions(response.getHeader("Authorization"));
}
iyannsch
removed
the
too-long-open
janthoXO
changed the title
General: Enable bearer authenticationDevelopment: Enable bearer authentication
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-69: Consider logging authentication method conflictsWhen both authentication methods are present, it would be helpful to log this security-relevant event for monitoring and debugging purposes.
Apply this diff:
if (cookie != null && authHeader != null) { // Single Method Enforcement: Only one method of authentication is allowed + logger.warn("Request contains both JWT cookie and Authorization header - rejecting request"); httpServletResponse.setStatus(HttpServletResponse.SC_BAD_REQUEST); return null; }
99-105: Consider additional validation for bearer token formatWhile the current implementation checks for "Bearer " prefix, consider adding length validation to prevent potential security issues with malformed tokens.
Apply this diff:
private static @Nullable String getJwtFromBearer(@Nullable String jwtBearer) { - if (!StringUtils.hasText(jwtBearer) || !jwtBearer.startsWith("Bearer ")) { + if (!StringUtils.hasText(jwtBearer) || !jwtBearer.startsWith("Bearer ") || jwtBearer.length() < 8) { return null; } return jwtBearer.substring(7).trim(); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
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)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/de/tum/cit/aet/artemis/core/config/websocket/WebsocketConfiguration.java
🧰 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 (2)
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTFilter.java (2)
35-46: LGTM! Clean implementation of dual authentication handling
The changes properly integrate both cookie and bearer token authentication while maintaining thread safety with SecurityContextHolder. The early return on bad request status is a good practice for request validation.
57-78: Verify consistent JWT extraction across the codebase
Let's ensure that JWT extraction is consistently handled across the codebase to prevent security bypasses.
✅ Verification successful
Let me gather more specific information about JWT token handling to ensure there are no inconsistencies.
Based on the comprehensive analysis of the codebase, I can now provide the final response:
JWT extraction implementation is consistent and secure
The JWT extraction logic in JWTFilter is the primary and consistent implementation across the codebase. The only other JWT-related authentication occurs in PyrisJobService which uses a different context (internal service communication) and maintains proper token validation.
- The
JWTFilterproperly handles both cookie and header-based tokens with mutual exclusivity - Token validation is consistently performed through
TokenProvider.validateTokenForAuthority() - No security bypasses or inconsistent JWT extraction implementations were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other JWT extraction implementations that might bypass security checks
# Search for JWT-related token extractions
echo "Searching for JWT token extractions..."
rg -l "getHeader.*Authorization|getCookie.*jwt" --type java
# Search for SecurityContextHolder usage
echo "Searching for SecurityContextHolder usage..."
rg -l "SecurityContextHolder" --type java
# Search for TokenProvider usage
echo "Searching for TokenProvider usage..."
ast-grep --pattern 'class $_ {
$$$
$x: TokenProvider
$$$
}'
Length of output: 3337
Script:
#!/bin/bash
# Check for actual JWT token extraction implementations
echo "Checking JWT extraction implementations..."
rg -A 5 "getHeader\(\"Authorization\"\)|getCookie.*jwt" --type java
# Check for other security filter implementations
echo "Checking security filter implementations..."
rg -l "extends.*Filter|implements.*Filter" --type java
# Check for TokenProvider usage in authentication context
echo "Checking TokenProvider authentication usage..."
rg -B 2 -A 5 "TokenProvider.*authenticate|validateToken" --type java
Length of output: 18577
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
Mostly Code reviews are appreciated.
Otherwise you can test in postman that an authorization header is now also accepted.
it would also be good to just test the most common functionalities in the client to see if any bugs occur, which should not happen.
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
Summary by CodeRabbit
New Features
authorizemethod now returns the JWT access token as a string upon successful authentication.Bug Fixes
Documentation
@coderabbitai pause