Development: Move public endpoints from AthenaResource to PublicAthenaResource
#10980
Conversation
github-actions
bot
added
server
WalkthroughThe repository export REST endpoints and their related secret validation logic have been removed from Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant PublicAthenaResource
participant AthenaRepositoryExportService
Client->>PublicAthenaResource: GET /api/athena/public/programming-exercises/{exerciseId}/submissions/{submissionId}/repository (with Authorization header)
PublicAthenaResource->>PublicAthenaResource: checkAthenaSecret(auth)
alt Secret valid
PublicAthenaResource->>AthenaRepositoryExportService: exportStudentSubmissionRepository(exerciseId, submissionId)
AthenaRepositoryExportService-->>PublicAthenaResource: Resource (zip file)
PublicAthenaResource-->>Client: ResponseEntity<Resource>
else Secret invalid
PublicAthenaResource-->>Client: AccessForbiddenException
end
Suggested labels
Suggested reviewers
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/athena/web/open/PublicAthenaResource.java (2)
31-48: Consider storing the secret hash computation result securely.The constructor correctly uses dependency injection and computes the secret hash once for performance. However, consider the security implications of storing the hash in memory.
For enhanced security, consider clearing the original secret from memory after hashing:
public PublicAthenaResource(AthenaRepositoryExportService athenaRepositoryExportService, @Value("${artemis.athena.secret}") String athenaSecret) { this.athenaRepositoryExportService = athenaRepositoryExportService; this.athenaSecretHash = hashSha256(athenaSecret); + // Clear the secret from memory for security + athenaSecret = null; }
55-60: Security implementation looks correct but consider timing attack protection.The secret validation correctly uses
MessageDigest.isEqual()which provides constant-time comparison to prevent timing attacks. The error handling is appropriate.Consider using a more descriptive log message for security monitoring:
-log.error("Athena secret does not match"); +log.warn("Unauthorized access attempt to Athena endpoint with invalid secret from request");src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java (1)
44-50: Class documentation should be updated to reflect the scope change.The class comment still mentions "Athena feedback suggestions" but doesn't reflect that repository access has been moved elsewhere.
Consider updating the class documentation:
/** - * REST controller for Athena feedback suggestions. + * REST controller for Athena feedback suggestions and module management. */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/athena/web/open/PublicAthenaResource.java(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/java/**/*.java`: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,de...
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/athena/web/AthenaResource.javasrc/main/java/de/tum/cit/aet/artemis/athena/web/open/PublicAthenaResource.java
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Codacy Static Code Analysis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build and Push Docker Image / Build Docker Image for ls1intum/artemis
- GitHub Check: Build .war artifact
- GitHub Check: Analyse
- GitHub Check: server-tests
- GitHub Check: server-style
- GitHub Check: client-tests
- GitHub Check: client-style
🔇 Additional comments (6)
src/main/java/de/tum/cit/aet/artemis/athena/web/open/PublicAthenaResource.java (5)
1-27: LGTM: Clean imports and package structure.The imports follow best practices by avoiding star imports and the package structure appropriately places this in the
web/opendirectory for public endpoints.
70-77: Endpoint implementation follows REST best practices.The method signature, annotations, and delegation pattern are well-implemented. The security bypass annotations with manual secret checking is appropriate for this use case.
86-93: Consistent implementation with proper logging.The template repository endpoint follows the same pattern as the submission endpoint with appropriate debug logging and parameter validation.
102-109: Solution repository endpoint is well-structured.Maintains consistency with other endpoints and properly delegates to the service layer.
118-125: Test repository endpoint completes the CRUD pattern correctly.The final endpoint maintains the established pattern and completes the repository type coverage needed for Athena integration.
src/main/java/de/tum/cit/aet/artemis/athena/web/AthenaResource.java (1)
77-91: Excellent refactoring that improves separation of concerns.The constructor simplification removes the repository export dependencies, making this class focused solely on feedback suggestions and module management. This follows the single responsibility principle well.
End-to-End (E2E) Test Results Summary
|
||||||||||||||||||||||||
|
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. |
Checklist
General
Server
Motivation and Context
Currently, the AthenaResource contains public endpoints, so Athena can retrieve the repositories as zip files.
However, to more easily highlight in the code that endpoints are public, we want to have them in a dedicated public resource.
Description
This PR moves the public endpoints from the AthenaResource to an additional PublicAthenaResource.
Steps for Testing
Prerequisites:
Testserver States
You can manage test servers using Helios. Check environment statuses in the environment list. To deploy to a test server, go to the CI/CD page, find your PR or branch, and trigger the deployment.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Unchanged
Summary by CodeRabbit
New Features
Refactor