Programming exercises: Add tool token support
#9408
Conversation
github-actions
bot
added
server
|
As discussed before, I do not think that it is a good idea to offer a |
its still a draft, so I would like to first have a discussion about it, before declining this PR. |
We discussed this already several times. I'm not sure if additional discussions help, when my arguments against it are ignored :-( |
we agreed on a meeting in the afternoon, so please let us have this meeting first |
|
After the meeting the proposed solution looks as follows:
@krusche will provide further feedback on that |
janthoXO
changed the title
Programming Exercise Add re-key endpointProgramming Exercise: Add re-key endpoint
janthoXO
changed the title
Programming Exercise: Add re-key endpointProgramming exercises: Add re-key endpoint
janthoXO
changed the title
Programming exercises: Add re-key endpointProgramming exercises: Add theia token for redirect
|
TODO, make endpoint only available if theia profile is active |
src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
github-actions
bot
added
client
github-actions
bot
removed
client
Mtze
added
maintainer-approved
github-actions
bot
added
the
assessment
bassner
dismissed stale reviews from Mtze, az108, muradium, SimonEntholzer, coderabbitai[bot], and iyannsch
The base branch was changed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/ToolsInterceptor.java (1)
63-64: Optimize tools claim matching logicTo improve performance and readability, consider converting
toolsClaimListto aSetfor efficient lookup.Apply this change:
var toolsClaimList = toolsClaim.split(","); + var toolsClaimSet = new HashSet<>(Arrays.asList(toolsClaimList)); - if (Arrays.stream(allowedTools).noneMatch(tool -> Arrays.asList(toolsClaimList).contains(tool.toString()))) { + if (Arrays.stream(allowedTools).noneMatch(tool -> toolsClaimSet.contains(tool.toString()))) {src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java (1)
194-194: Consider adding rate limiting for token creation.This endpoint creates new VCS access tokens and should be protected against potential abuse through rate limiting.
Consider implementing rate limiting using Spring's @ratelimiter or a similar mechanism to prevent token generation abuse.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultResource.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/AllowedTools.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/ToolTokenType.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/ToolsInterceptor.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/security/jwt/TokenProvider.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/TokenResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/de/tum/cit/aet/artemis/core/security/jwt/TokenProvider.java
- src/main/java/de/tum/cit/aet/artemis/core/web/open/PublicUserJwtResource.java
🧰 Additional context used
📓 Path-based instructions (10)
src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/ToolTokenType.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/allowedTools/AllowedTools.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/web/TokenResource.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/allowedTools/ToolsInterceptor.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/assessment/web/ResultResource.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/web/AccountResource.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/JWTCookieService.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/config/WebConfigurer.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/web/CourseResource.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/exercise/web/ParticipationResource.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 (10)
src/main/java/de/tum/cit/aet/artemis/core/web/TokenResource.java (1)
51-70: Method implementation is correct and follows coding standards
The convertCookieToToolToken method is well-implemented, and the logic is clear.
src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/ToolTokenType.java (1)
1-5: Enum declaration is correct
The ToolTokenType enum is properly defined.
src/main/java/de/tum/cit/aet/artemis/core/security/allowedTools/AllowedTools.java (1)
1-13: Annotation definition is correct
The AllowedTools annotation is properly defined with correct retention and target policies.
src/main/java/de/tum/cit/aet/artemis/core/security/jwt/JWTCookieService.java (3)
41-42: LGTM! Good use of method delegation.
The change maintains backward compatibility while reusing code.
44-53: LGTM! Well-documented method with clear parameters.
The method signature and implementation properly handle the tool claim requirement.
55-64:
Add duration validation to prevent security risks.
The method should validate the duration parameter to prevent creation of tokens with excessive lifetimes. According to PR objectives, tokens should have a maximum validity of one day.
Apply this validation:
public ResponseCookie buildLoginCookie(long duration, ToolTokenType tool) {
+ if (duration > Duration.ofDays(1).toMillis()) {
+ throw new IllegalArgumentException("Token duration cannot exceed 24 hours");
+ }
String jwt = tokenProvider.createToken(SecurityContextHolder.getContext().getAuthentication(), duration, tool);
return buildJWTCookie(jwt, Duration.of(duration, ChronoUnit.MILLIS));
}Likely invalid or redundant comment.
src/main/java/de/tum/cit/aet/artemis/core/config/WebConfigurer.java (1)
43-56: LGTM! Good use of constructor injection and interface implementation.
The changes follow Spring best practices for dependency injection and configuration.
src/main/java/de/tum/cit/aet/artemis/core/web/AccountResource.java (1)
177-177: Verify interaction between security annotations.
The method combines @EnforceAtLeastStudent with @AllowedTools. Verify that these annotations work together as intended and don't create security gaps.
✅ Verification successful
Security annotations work together as intended
Based on the search results, the combination of @EnforceAtLeastStudent and @AllowedTools is a well-established pattern across the codebase. The ToolsInterceptor implementation shows proper handling of the @AllowedTools annotation, where it:
- Checks for the annotation's presence
- Validates the tool token claims against allowed values
- Enforces access control independently from other security annotations
This means the annotations work in a complementary way:
@EnforceAtLeastStudentenforces user role requirements@AllowedToolsadds an additional layer of tool-specific access control
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for other combinations of these annotations and their handling
rg -B 2 -A 2 "@EnforceAtLeastStudent.*@AllowedTools|@AllowedTools.*@EnforceAtLeastStudent"
# Check ToolsInterceptor implementation for annotation handling
ast-grep --pattern 'class ToolsInterceptor {
$$$
preHandle($$$) {
$$$
}
$$$
}'
Length of output: 309
Script:
#!/bin/bash
# Search for AllowedTools annotation usage and implementation
rg -B 2 -A 2 "@AllowedTools"
# Search for ToolsInterceptor or similar security interceptor classes
fd -e java -exec rg -l "class.*Interceptor.*implements.*HandlerInterceptor"
# Search for security configuration related to tools
fd -e java -exec rg -l "SecurityConfiguration|WebSecurityConfig"
# Look for annotation handler implementations
fd -e java -exec rg -l "ToolTokenType|@AllowedTools"
Length of output: 9433
src/main/java/de/tum/cit/aet/artemis/assessment/web/ResultResource.java (1)
46-47: Security enhancement: Tool-based access control added for result details endpoint
The addition of @AllowedTools(ToolTokenType.SCORPIO) properly restricts the result details endpoint to only be accessible with SCORPIO tool tokens, complementing the existing role-based access control.
Let's verify that this is the only endpoint that needs SCORPIO tool access for result details:
Also applies to: 171-171
✅ Verification successful
Let me analyze the results and generate additional verification scripts to get a clearer picture.
The first search will help us understand all endpoints in ResultResource.java. The second search will identify other classes that might handle result details. The third search will show if SCORPIO tool access is consistently applied across similar endpoints.
Based on the results, I can now generate the final response:
Verified: SCORPIO tool access control is correctly applied to result details endpoint
The @AllowedTools(ToolTokenType.SCORPIO) annotation is appropriately applied to the result details endpoint in ResultResource.java. The search results show that:
- This is the only endpoint handling result details that requires SCORPIO tool access
- SCORPIO tool access is consistently used across related endpoints in the system for similar sensitive operations (e.g., participation management, authentication)
- The
PublicResultResourceexists but doesn't expose result details functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other result details endpoints that might need the same restriction
ast-grep --pattern 'getResultDetails($$$) {
$$$
}'
# Search for similar endpoints that might need the same protection
rg -A 2 "result.*details"
Length of output: 12664
Script:
#!/bin/bash
# Search for other endpoints in ResultResource.java that might need SCORPIO tool access
ast-grep --pattern 'class ResultResource {
$$$
}'
# Search for other classes that handle result details
rg -l "ResultResource|result.*details.*endpoint"
# Search for other usages of AllowedTools annotation with SCORPIO
rg -A 2 "@AllowedTools.*SCORPIO"
Length of output: 4943
src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java (1)
96-97: Security enhancement: Tool-based access control added for course dashboard endpoints
The addition of @AllowedTools(ToolTokenType.SCORPIO) properly restricts both course dashboard endpoints to only be accessible with SCORPIO tool tokens:
getCourseForDashboard: For retrieving single course dashboard datagetCoursesForDashboard: For retrieving multiple courses dashboard data
This complements the existing role-based access control.
Let's verify that these are the only course dashboard endpoints that need SCORPIO tool access:
Also applies to: 589-589, 654-654
✅ Verification successful
Let me analyze the results and generate additional verification scripts to check for other endpoints that might need SCORPIO tool access.
Let me search for any other course dashboard endpoints in the codebase to ensure we haven't missed any.
Based on the search results, I can now provide a final response:
Security enhancement: Tool-based access control correctly applied
The SCORPIO tool access restriction has been correctly applied to the only two course dashboard endpoints that need it:
getCourseForDashboard: For retrieving single course dashboard datagetCoursesForDashboard: For retrieving multiple courses dashboard data
The other dashboard-related endpoints in CourseResource.java don't need SCORPIO tool access as they serve different purposes and already have appropriate access controls:
/for-lti-dashboard: Protected by@EnforceAtLeastInstructorand LTI profile/for-assessment-dashboard: Protected by@EnforceAtLeastTutorInCourse/stats-for-assessment-dashboard: Protected by@EnforceAtLeastTutor
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other course dashboard endpoints that might need the same restriction
ast-grep --pattern '(getCourseForDashboard|getCoursesForDashboard)($$$) {
$$$
}'
# Search for similar endpoints that might need the same protection
rg -A 2 "course.*dashboard"
Length of output: 65739
Script:
#!/bin/bash
# Search for all course dashboard related endpoints in CourseResource.java
ast-grep --pattern 'class CourseResource {
$$$
@$_($$$)
@$_($$$)
$_($$$) {
$$$
}
$$$
}'
# Search for other endpoints that might need SCORPIO tool access
rg -A 2 "@AllowedTools" src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java
Length of output: 679
Script:
#!/bin/bash
# Search for other course dashboard endpoints in CourseResource.java
rg -A 5 "@GetMapping.*dashboard" src/main/java/de/tum/cit/aet/artemis/core/web/CourseResource.java
# Search for any other endpoints that return CourseForDashboardDTO or CoursesForDashboardDTO
ast-grep --pattern '($_) ResponseEntity<$_ForDashboardDTO> $_($$$) {
$$$
}'
Length of output: 2138
| @Override | ||
| public void addInterceptors(InterceptorRegistry registry) { | ||
| registry.addInterceptor(toolsInterceptor); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider restricting interceptor to specific paths.
The interceptor is currently registered globally. Consider restricting it to relevant API paths to avoid unnecessary overhead and potential security implications.
@Override
public void addInterceptors(InterceptorRegistry registry) {
- registry.addInterceptor(toolsInterceptor);
+ registry.addInterceptor(toolsInterceptor)
+ .addPathPatterns("/api/**")
+ .excludePathPatterns("/api/public/**");
}Committable suggestion skipped: line range outside the PR's diff.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ParticipationResource.java
Show resolved
Hide resolved
Tip
This PR stacks on top of
feature/bearer-supportChecklist
General
Server
Changes affecting Programming Exercises
Description
Adds an endpoint to generate a new token for the user of tools. This token has restricted access to only a subset of endpoints.
This is primarily needed for the online IDE and the scorpio VSCode extension
Steps for Testing
Postman testing or code review:
Postman Testing:
ArtemisTokens.postman_collection.json
Send a request to POST
{{base_url}}/api/public/authenticate?tool=SCORPIOSend a request to GET
{{base_url}}/api/courses/for-dashboardVerify that you get a response 200
Send a request to an GET
{{base_url}}/api/ide-settingsverify that you get an 403 Forbidden
Send a Send a request to POST
{{base_url}}/api/public/authenticateSend a request to GET
{{base_url}}/api/courses/for-dashboardSend a request to an GET
{{base_url}}/api/ide-settingsVerify a 200 response
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
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
New Features
AllowedToolsannotation for enhanced access control.ToolTokenTypeenumeration for tool-specific token management.TokenResourcefor managing tool tokens via REST API.ToolsInterceptorfor access control based on JWT claims.Improvements
Bug Fixes
Chores