Programming exercises: Exchange of Artemis programming exercises via CodeAbility Sharing Platform
#10593
Conversation
github-actions
bot
added
server
WalkthroughThis change introduces a comprehensive integration between Artemis and the CodeAbility Sharing Platform for programming exercises. It implements a new "sharing" Spring profile, adds backend services and REST endpoints for import/export operations, and provides UI components for instructors to share and import exercises. The backend validates secure communication using a pre-shared key, supports regular configuration polling, and offers health checks. The frontend includes new routes, services, and components for managing sharing workflows, with internationalization and documentation updates. Extensive automated tests cover both import and export scenarios, ensuring correctness and reliability of the sharing feature. Changes
Sequence Diagram(s)sequenceDiagram
participant Instructor
participant ArtemisUI
participant ArtemisAPI
participant SharingPlatform
Instructor->>ArtemisUI: Click "Export to Sharing"
ArtemisUI->>ArtemisAPI: POST /sharing/export/{exerciseId}
ArtemisAPI->>SharingPlatform: Register export, send metadata
SharingPlatform-->>ArtemisAPI: Respond with redirect URL
ArtemisAPI-->>ArtemisUI: Return redirect URL
ArtemisUI->>Instructor: Redirect to Sharing Platform
Instructor->>SharingPlatform: Select exercise to import
SharingPlatform->>ArtemisUI: Redirect with basketToken, apiBaseURL, checksum
ArtemisUI->>ArtemisAPI: GET /sharing/import/basket?basketToken=...
ArtemisAPI->>SharingPlatform: Validate and fetch basket
SharingPlatform-->>ArtemisAPI: Return exercise data
ArtemisAPI-->>ArtemisUI: Return basket info
ArtemisUI->>Instructor: Show import dialog, pre-fill fields
Instructor->>ArtemisUI: Confirm import
ArtemisUI->>ArtemisAPI: POST /sharing/setup-import
ArtemisAPI->>Artemis: Import exercise, create resources
ArtemisAPI-->>ArtemisUI: Confirm import success
Assessment against linked issues
Possibly related PRs
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: 20
🧹 Nitpick comments (40)
src/main/webapp/i18n/en/sharing.json (1)
4-4: Fix typo in platform spellingThere's a typo in the title string - "Plattform" should be "Platform".
- "title": "Import from Sharing Plattform", + "title": "Import from Sharing Platform",src/main/java/de/tum/cit/aet/artemis/sharing/SharingSetupInfo.java (1)
14-34: Redundant getter methods in record class.Java records automatically generate accessor methods for each component, making these explicit getter methods unnecessary. While they don't harm functionality, they add redundancy to the code.
Consider removing the explicit getter methods since they're automatically provided by the Java record feature:
- /** - * the exercise - */ - public ProgrammingExercise getExercise() { - return exercise; - } - - /** - * the course - */ - public Course getCourse() { - return course; - } - - /** - * the sharing info from the sharing platform - */ - public SharingInfoDTO getSharingInfo() { - return sharingInfo; - }You can keep the JavaDoc comments above the record declaration if you want to document the components.
src/main/webapp/app/sharing/sharing.route.ts (1)
16-23: Consider consolidating route definitions.The additional definition with SHARING_ROUTES seems unnecessary since it's just a reference to sharingRoutes with no modifications.
- const SHARING_ROUTES = [...sharingRoutes]; - - export const featureOverviewState: Routes = [ - { - path: '', - children: SHARING_ROUTES, - }, - ]; + export const featureOverviewState: Routes = [ + { + path: '', + children: sharingRoutes, + }, + ];docs/admin/setup/sharing.rst (2)
17-19: Enhance documentation clarity with direct help link.The sharing platform is properly introduced with a link to the search page, but the reference to a "help menu" points to the same URL as the main platform.
Consider updating this to point to a specific help page or documentation section:
-For more details, visit the `help menu <https://search.sharing-codeability.uibk.ac.at/>`_ of the sharing platform. +For more details, visit the `help menu <https://search.sharing-codeability.uibk.ac.at/help>`_ of the sharing platform or contact the platform support.
65-67: Consider adding more context about EduID and GitLab registration.The authentication options are clearly explained, but more details about the actual registration flow would be helpful for instructors.
Consider adding a sentence about the exact steps for instructors to register or a note indicating where they can find more detailed registration instructions.
src/main/webapp/app/sharing/sharing.scss (1)
85-94: Eliminate duplicate hover styles.The same hover styles are defined twice in the file - once for media query with
hover: hovercondition and once as a regular selector.Remove the duplicate hover styles by consolidating them:
@media (hover: hover) { .container .box:hover .icon { top: 20px; left: calc(50% - 40px); width: 80px; height: 80px; border-radius: 50%; cursor: pointer; } } @media (hover: none) { .container .box .icon { position: relative; height: 80px; border-radius: 50%; margin: 0 auto; } } -/* Remove this duplicate section */ -.container .box:hover .icon { - top: 20px; - left: calc(50% - 40px); - width: 80px; - height: 80px; - border-radius: 50%; - cursor: pointer; -}Also applies to: 158-165
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (3)
16-21: Incorrect Javadoc for the general constructor.The Javadoc comment specifies that this constructor is for "connection failures" when it's actually a general-purpose constructor.
Update the Javadoc to better reflect the general nature of this constructor:
/** - * Creates a SharingException for connection failures. + * Creates a SharingException with a message. + * + * @param message The error message */ public SharingException(String message) { super(message); }
23-30: Incorrect Javadoc for the cause constructor.Similar to the first constructor, the Javadoc specifies that this constructor is for "connection failures" when it's actually a general-purpose constructor with cause.
Update the Javadoc to better reflect the general nature of this constructor:
/** - * Creates a SharingException for connection failures. + * Creates a SharingException with a message and cause. * + * @param message The error message * @param cause The underlying cause */ public SharingException(String message, Throwable cause) { super(message, cause); }
12-15: Consider using generated serialVersionUID.While using
1Las the serialVersionUID is common practice, it's better to use a generated value based on class structure to ensure proper serialization/deserialization compatibility.Consider generating a serialVersionUID using your IDE's tools or the
serialvercommand.@Serial -private static final long serialVersionUID = 1L; +private static final long serialVersionUID = 8372458748372991872L; // Generated valuesrc/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (5)
22-24: Add final modifier to class fieldsThe class fields
nameandinputStreamshould be marked asfinalsince they are initialized in the constructor and never changed afterward. This follows the principle of least access and promotes immutability.- private final String name; - - private final InputStream inputStream; + private final String name; + + private final InputStream inputStream;
8-10: Standardize on a single nullability annotation frameworkThe code mixes Jakarta's
@NotNulland Javax's@Nonnullannotations. Standardize on Jakarta annotations since they are the current standard for Jakarta EE.import jakarta.validation.constraints.NotNull; -import javax.annotation.Nonnull;
35-36: Replace @nonnull with Jakarta's @NotNullReplace the
@Nonnullannotations with Jakarta's@NotNullfor consistency and to follow Jakarta EE standards.@Override - @Nonnull + @NotNull public String getName() { @Override - @Nonnull + @NotNull public byte @NotNull [] getBytes() throws IOException { @Override - @Nonnull + @NotNull public @NotNull InputStream getInputStream() throws IOException { - public void transferTo(@NotNull File dest) throws IOException, IllegalStateException { + public void transferTo(@NotNull File dest) throws IOException, IllegalStateException {Also applies to: 71-72, 77-78, 83-83
76-80: Potential resource leak in getInputStream()The method returns the internal inputStream directly, which could lead to resource leaks if the caller doesn't close it properly. Consider returning a new BufferedInputStream wrapping the original to prevent the original stream from being closed prematurely.
@Override @NotNull public @NotNull InputStream getInputStream() throws IOException { - return this.inputStream; + return new BufferedInputStream(this.inputStream); }
16-19: Improve class documentationThe current JavaDoc is minimal. Expand it to include more details about the purpose, usage, and behavior of this class, particularly how it relates to the sharing functionality.
/** - * just a utility class to hold the zip file from sharing + * A utility class that implements Spring's MultipartFile interface for working with zip files from the sharing platform. + * This class wraps an InputStream and a filename, providing MultipartFile compatibility for use in + * file upload processing within the sharing module. + * + * This implementation is only active when the "sharing" Spring profile is enabled. */build.gradle (2)
157-158: Document the purpose of the new Sharing Platform dependencyAdd a comment explaining what the SharingPluginPlatformAPI dependency is used for and why version 1.1.1 was selected, to provide better context for future developers.
// Required by Sharing Platform - implementation group: 'org.codeability', name: 'SharingPluginPlatformAPI', version: '1.1.1' + // This API enables integration with the CodeAbility Sharing Platform for exchanging programming exercises + implementation group: 'org.codeability', name: 'SharingPluginPlatformAPI', version: '1.1.1'
157-158: Use consistent dependency declaration syntaxFor consistency with other dependencies in the file, consider using the shorthand notation with quotes instead of the group/name/version syntax.
- implementation group: 'org.codeability', name: 'SharingPluginPlatformAPI', version: '1.1.1' + implementation "org.codeability:SharingPluginPlatformAPI:1.1.1"src/main/webapp/app/sharing/sharing.model.ts (4)
3-4: Specify provider scope for injectable serviceWhen using
@Injectable(), specify the scope using theprovidedInproperty to follow Angular best practices. This helps with tree-shaking and dependency injection management.-@Injectable() +@Injectable({ + providedIn: 'root' +}) export class SharingInfo {
27-31: Improve error handling in the clear() methodThe
clear()method doesn't reset thechecksumproperty. Update the method to reset all properties to their initial values.public clear(): void { this.basketToken = ''; this.selectedExercise = 0; this.returnURL = ''; this.apiBaseURL = ''; + this.checksum = ''; }
68-72: Improve email validationThe current email validation regex is simplistic and may not handle all edge cases. Consider using a more robust regex or a validation library.
export function validateEmail(email: string): void { - const emailRegex = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + // More comprehensive email validation regex + const emailRegex = /^(([^<>()[\]\\.,;:\s@"]+(\.[^<>()[\]\\.,;:\s@"]+)*)|(".+"))@((\[[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}\.[0-9]{1,3}])|(([a-zA-Z\-0-9]+\.)+[a-zA-Z]{2,}))$/; if (!emailRegex.test(email)) { throw new Error('Invalid email address'); } }
47-61: Improve interface documentationAdd more detailed documentation to the
ShoppingBasketinterface, particularly explaining the types and purposes of its properties./** - * Represents a shopping basket containing exercises to be shared + * Represents a shopping basket containing exercises to be shared + * between Artemis instances through the CodeAbility Sharing Platform. + * Contains exercise information, user details, and token expiration data. */ export interface ShoppingBasket { + /** List of exercises in the shopping basket */ exerciseInfo: Array<SearchResultDTO>; + /** Information about the user performing the sharing operation */ userInfo: UserInfo; + /** Date when the basket token expires */ tokenValidUntil: Date; }src/main/webapp/app/sharing/sharing.component.html (4)
3-3: Remove inline styles and move to CSSThere are several inline styles in the template. These should be moved to a separate CSS file following Angular best practices for maintainability and separation of concerns.
- <table style="border-spacing: 50px 0px; border-collapse: separate"> + <table class="sharing-table"> - <td style="vertical-align: top"> + <td class="align-top"> - <div [ngStyle]="{ backgroundColor: course.color || ARTEMIS_DEFAULT_COLOR, width: '15px', height: '20px' }"> </div> + <div class="course-color-indicator" [style.backgroundColor]="course.color || ARTEMIS_DEFAULT_COLOR"> </div>Also applies to: 11-11, 50-50, 67-67, 82-82
25-34: Improve accessibility of form controlsThe radio buttons don't have associated labels that are properly connected with the
forattribute. Improve the accessibility by properly connecting the labels to the inputs.<td> <input id="{{ exercise.project.project_id }}" [(ngModel)]="sharingInfo.selectedExercise" (change)="onExerciseSelected(exercisePos)" [value]="exercisePos" name="selectedExercise" type="radio" + aria-labelledby="exercise-title-{{ exercisePos }}" required /> </td> -<td>{{ exercise.metadata.title }}</td> +<td id="exercise-title-{{ exercisePos }}">{{ exercise.metadata.title }}</td>
43-43: Improve date formatting for localizationThe date formatting in
getTokenExpiryDate().toLocaleString()might not be consistent across browsers. Consider using Angular's DatePipe for more consistent formatting.- <span jhiTranslate="artemisApp.sharing.expiresAt" [translateValues]="{ expirationDate: getTokenExpiryDate().toLocaleString() }">expires at: </span> + <span jhiTranslate="artemisApp.sharing.expiresAt" [translateValues]="{ expirationDate: getTokenExpiryDate() | date:'medium' }">expires at: </span>
98-98: Fix typo in translation keyThere's a typo in the "Plattform" word (should be "Platform") in the translation text. This should be corrected to maintain consistency.
- <span class="hidden-sm-down" jhiTranslate="artemisApp.programmingExercise.sharing.import">Import Programming Exercise from Sharing Plattform</span> + <span class="hidden-sm-down" jhiTranslate="artemisApp.programmingExercise.sharing.import">Import Programming Exercise from Sharing Platform</span>src/main/webapp/app/sharing/sharing.component.ts (2)
89-94: Use optional chaining for safer property access.The code can be simplified and made safer by using optional chaining when accessing nested properties.
- courseId(): number { - if (this.selectedCourse && this.selectedCourse.id) { - return this.selectedCourse.id; - } - return 0; - } + courseId(): number { + return this.selectedCourse?.id ?? 0; + }🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
116-123: Implement component cleanup in ngOnDestroy.The component should implement OnDestroy to clean up any subscriptions when the component is destroyed.
-export class SharingComponent implements OnInit { +export class SharingComponent implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + + // Keep existing code... + + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + }Then, modify your subscriptions to unsubscribe when the component is destroyed:
- this.route.params.subscribe((params) => { + this.route.params.pipe(takeUntil(this.destroy$)).subscribe((params) => {- this.route.queryParams.subscribe((qparams: Params) => { + this.route.queryParams.pipe(takeUntil(this.destroy$)).subscribe((qparams: Params) => {Don't forget to add the necessary imports:
import { Subject } from 'rxjs'; import { takeUntil } from 'rxjs/operators'; import { OnDestroy } from '@angular/core';src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (2)
67-70: Validate input parameters more thoroughly.While there is a check if the exercise is null, consider validating other critical fields from the
sharingSetupInfoobject for a more robust implementation.public ProgrammingExercise importProgrammingExerciseFromSharing(SharingSetupInfo sharingSetupInfo) throws SharingException, IOException, GitAPIException, URISyntaxException { if (sharingSetupInfo.getExercise() == null) { throw new SharingException("Exercise is null?"); } + if (sharingSetupInfo.getSharingInfo() == null) { + throw new SharingException("Sharing info is null"); + } + if (sharingSetupInfo.getCourse() == null) { + throw new SharingException("Course is null"); + }
74-77: Consider adding a log statement when setting the course.Adding a log statement would improve debugging capability and help track when courses need to be set manually.
if (sharingSetupInfo.getExercise().getCourseViaExerciseGroupOrCourseMember() == null) { + log.debug("Exercise course is null, setting from sharing setup info"); sharingSetupInfo.getExercise().setCourse(sharingSetupInfo.getCourse()); }src/main/webapp/app/programming/shared/actions/programming-exercise-instructor-exercise-sharing.component.ts (1)
42-71: Consider clarifying usage ofthis.sharingTab.
The code checksthis.sharingTabbut never sets it (commented-out code suggests a futurewindow.open). If you plan to open in a separate window, re-enable or remove this logic for clarity.src/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.java (1)
68-85: Use standard utility methods for 401/400 responses.
Lines 79 and 84 usebody(null). As a minor upkeep, consider returningResponseEntity.status(HttpStatus.BAD_REQUEST).build()or.status(HttpStatus.UNAUTHORIZED).build()for clarity.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistry.java (1)
40-75: Evaluate thread-safety ofHealthStatusWithHistory.
This class relies on anArrayList, which is not thread-safe. If multiple threads invoke these health checks, consider using a synchronized structure (e.g.,Collections.synchronizedList) or another thread-safe collection to avoid concurrency issues.src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts (2)
161-162: Consider adding explicit type annotation for maintainability.- isExportToSharingEnabled = false; + isExportToSharingEnabled: boolean = false;
290-302: Good error-handling approach but consider user feedback.
Currently, if an error occurs,isEnableddefaults to false silently. If user feedback is desired, you could log or display a warning for easier troubleshooting. Otherwise, this approach is acceptable.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
45-47: Class-level documentation is concise.
You might consider expanding the JavaDoc to elaborate on usage scenarios, but this is optional.src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (3)
134-157: Potential null handling improvement ingetBasketInfo
This method returns anOptional<ShoppingBasket>but logs errors and catches exceptions that ultimately lead to returning an empty Optional. Consider whether returning an empty Optional versus throwing a customized exception is most appropriate for cases like a parse failure or missing basket data. If returning empty is valid, adding more specific logging or user feedback might help debugging.
241-246: Clarify Docker localhost replacement
The logic replacing “localhost” with “host.docker.internal” is useful for local Docker scenarios but might cause surprising behavior in other environments. Consider making this step opt-in via configuration or environment checks beyond the single profile check (“docker”).
254-266: Fallback exception handling ingetProblemStatementFromBasket
Catching a genericException(line 263) makes it easy to overlook more specific handling. Additionally, the method rethrows aNotFoundExceptionwhile the cause may be I/O or parsing issues. Consider narrower exception handling or adding a custom exception message to convey the root cause more precisely.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (3)
55-80: Inheritance fromArrayListinHealthStatusWithHistory
ExtendingArrayListdirectly can be brittle. A more robust approach is composition, where you keep anArrayList<HealthStatus>as a field. This avoids exposing unwarrantedListfunctionality that might break the health history logic (like removing elements out of order).
227-233: Thread pool usage on application startup
triggerSharingReinitAfterApplicationStartcreates a new fixed thread pool using a single thread each time the app starts (line 231). Consider using a shared Executor or scheduling tasks via Spring’s@AsyncorTaskSchedulerto avoid potential resource leaks or unclosed thread pools if the service restarts frequently.
203-214: Potential confusion invalidateregex
The regexBearer\\s(.+)requires the exact prefix “Bearer ”. If receiving tokens without “Bearer” or having leading/trailing spaces, the match fails. Validate that the calling code consistently sets the correct format, or consider a more robust approach if you need flexibility.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (16)
docker/artemis-dev-local-vc-local-ci-mysql.ymlis excluded by!**/*.ymldocker/artemis.ymlis excluded by!**/*.ymldocs/admin/setup/sharing/sharingButtonArtemis.pngis excluded by!**/*.png,!**/*.pngdocs/admin/setup/sharing/sharingButtonSharing.pngis excluded by!**/*.png,!**/*.pngdocs/admin/setup/sharing/sharing_health1.pngis excluded by!**/*.png,!**/*.pngdocs/admin/setup/sharing/sharing_health2.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/artemis_import.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/artemis_import2.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_export.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_metadata.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_metadata2.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_namespace.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_search.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_success.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/config/application-dev.ymlis excluded by!**/*.ymlsrc/main/resources/config/application-sharing.ymlis excluded by!**/*.yml
📒 Files selected for processing (40)
build.gradle(2 hunks)docker/artemis/config/dev-local-vc-local-ci.env(1 hunks)docs/admin/extension-services.rst(1 hunks)docs/admin/setup/sharing.rst(1 hunks)docs/index.rst(1 hunks)docs/user/sharing.rst(1 hunks)gradle.properties(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistry.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/SharingSetupInfo.java(1 hunks)src/main/webapp/app/app.routes.ts(1 hunks)src/main/webapp/app/programming/manage/programming-exercise-detail.component.html(1 hunks)src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts(7 hunks)src/main/webapp/app/programming/manage/programming-exercise.component.ts(1 hunks)src/main/webapp/app/programming/manage/services/programming-exercise-sharing.service.ts(1 hunks)src/main/webapp/app/programming/manage/update/programming-exercise-creation-config.ts(1 hunks)src/main/webapp/app/programming/manage/update/programming-exercise-update.component.html(3 hunks)src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts(17 hunks)src/main/webapp/app/programming/shared/actions/programming-exercise-instructor-exercise-sharing.component.ts(1 hunks)src/main/webapp/app/sharing/search-result-dto.model.ts(1 hunks)src/main/webapp/app/sharing/sharing.component.html(1 hunks)src/main/webapp/app/sharing/sharing.component.ts(1 hunks)src/main/webapp/app/sharing/sharing.model.ts(1 hunks)src/main/webapp/app/sharing/sharing.route.ts(1 hunks)src/main/webapp/app/sharing/sharing.scss(1 hunks)src/main/webapp/i18n/de/programmingExercise.json(2 hunks)src/main/webapp/i18n/de/sharing.json(1 hunks)src/main/webapp/i18n/en/programmingExercise.json(1 hunks)src/main/webapp/i18n/en/sharing.json(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
`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/core/config/SecurityConfiguration.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.javasrc/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.javasrc/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.javasrc/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.javasrc/main/java/de/tum/cit/aet/artemis/core/config/Constants.javasrc/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistry.javasrc/main/java/de/tum/cit/aet/artemis/sharing/SharingSetupInfo.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.javasrc/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/programming/manage/update/programming-exercise-creation-config.tssrc/main/webapp/app/app.routes.tssrc/main/webapp/app/sharing/sharing.model.tssrc/main/webapp/app/sharing/sharing.route.tssrc/main/webapp/app/programming/manage/programming-exercise.component.tssrc/main/webapp/app/programming/manage/programming-exercise-detail.component.tssrc/main/webapp/app/sharing/sharing.component.tssrc/main/webapp/app/sharing/search-result-dto.model.tssrc/main/webapp/app/programming/manage/services/programming-exercise-sharing.service.tssrc/main/webapp/app/programming/manage/update/programming-exercise-update.component.tssrc/main/webapp/app/programming/shared/actions/programming-exercise-instructor-exercise-sharing.component.ts
`src/main/webapp/**/*.html`: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/**/*.html: @if and @for are new and valid Angular syntax replacing *ngIf and *ngFor. They should always be used over the old style.
src/main/webapp/app/programming/manage/programming-exercise-detail.component.htmlsrc/main/webapp/app/sharing/sharing.component.htmlsrc/main/webapp/app/programming/manage/update/programming-exercise-update.component.html
`src/main/webapp/i18n/de/**/*.json`: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/...
src/main/webapp/i18n/de/**/*.json: German language translations should be informal (dutzen) and should never be formal (sietzen). So the user should always be addressed with "du/dein" and never with "sie/ihr".
src/main/webapp/i18n/de/sharing.jsonsrc/main/webapp/i18n/de/programmingExercise.json
🧠 Learnings (4)
src/main/webapp/app/sharing/search-result-dto.model.ts (4)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:17-42
Timestamp: 2025-03-26T14:24:20.057Z
Learning: The types in `UserProvidedMetadataDTO` interface in `search-result-dto.model.ts` must match the sharing platform's contract exactly to maintain compatibility. Avoid modifying these types even if they don't follow internal TypeScript conventions.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:57-65
Timestamp: 2025-03-26T14:24:07.386Z
Learning: The ProjectDTO interface in src/main/webapp/app/sharing/search-result-dto.model.ts uses snake_case property names to maintain compatibility with the external sharing platform's API contract, which is an intentional deviation from Angular's camelCase convention.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:50-55
Timestamp: 2025-03-26T14:24:07.386Z
Learning: The `IExerciseType` enum in `src/main/webapp/app/sharing/search-result-dto.model.ts` must maintain its current naming (with 'I' prefix) and lowercase string values to ensure compatibility with the external sharing platform connector interface. This is an intentional exception to our TypeScript naming conventions.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:67-72
Timestamp: 2025-03-26T14:24:07.386Z
Learning: The properties in MetadataFileDTO (commit_id, indexing_date) and ProjectDTO (project_id, project_name, etc.) use snake_case as they map to an external sharing platform's API, making this an intentional exception to Angular's camelCase naming convention.
src/main/webapp/app/programming/manage/services/programming-exercise-sharing.service.ts (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/exercises/programming/manage/services/programming-exercise-sharing.service.ts:120-124
Timestamp: 2025-03-26T14:24:32.827Z
Learning: In the Artemis sharing platform integration, passing callBackUrl in exportProgrammingExerciseToSharing is safe as it's returned to the same authenticated user session through internal API endpoints.
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/exercises/programming/manage/services/programming-exercise-sharing.service.ts:120-124
Timestamp: 2025-03-26T14:24:32.827Z
Learning: In the Artemis sharing platform integration, passing callBackUrl in exportProgrammingExerciseToSharing is safe as it's returned to the same authenticated user session through internal API endpoints.
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java:11-31
Timestamp: 2025-03-26T14:24:12.367Z
Learning: Team decision is to retain POJOs (plain old Java beans) for DTO classes, instead of converting them to Java records.
🧬 Code Definitions (16)
src/main/webapp/app/sharing/sharing.model.ts (1)
src/main/webapp/app/sharing/search-result-dto.model.ts (1)
SearchResultDTO(1-9)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (2)
src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)
src/main/webapp/app/programming/manage/programming-exercise.component.ts (2)
src/main/webapp/app/sharing/sharing.component.ts (1)
Component(22-124)src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts (1)
Component(74-793)
src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)
src/main/webapp/app/sharing/sharing.component.ts (2)
src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts (1)
Component(74-793)src/main/webapp/app/sharing/sharing.model.ts (1)
ShoppingBasket(50-54)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (4)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-328)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(70-463)
src/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.java (2)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (1)
RestController(48-210)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (2)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (5)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-328)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
Service(29-256)
src/main/webapp/app/programming/manage/services/programming-exercise-sharing.service.ts (1)
src/main/webapp/app/sharing/sharing.model.ts (2)
Injectable(3-45)ShoppingBasket(50-54)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistry.java (2)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
Service(29-256)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(70-463)
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts (1)
src/main/webapp/app/programming/manage/update/programming-exercise-creation-config.ts (1)
ProgrammingExerciseCreationConfig(7-56)
src/main/java/de/tum/cit/aet/artemis/sharing/SharingSetupInfo.java (2)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-328)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (2)
src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)
src/main/webapp/app/programming/shared/actions/programming-exercise-instructor-exercise-sharing.component.ts (3)
src/main/webapp/app/sharing/sharing.component.ts (1)
Component(22-124)src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts (1)
Component(74-793)src/main/webapp/app/programming/manage/programming-exercise.component.ts (1)
Component(58-266)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (2)
src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)
🪛 Biome (1.9.4)
src/main/webapp/app/sharing/sharing.component.ts
[error] 90-90: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 ast-grep (0.31.1)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
[warning] 399-399: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 399-399: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (70)
docs/admin/extension-services.rst (1)
20-20: Documentation entry appears correctly added.The new entry for
setup/sharinghas been properly added to the table of contents, which aligns with the new sharing functionality being implemented.docs/index.rst (1)
42-42: Documentation entry correctly added to user guide section.The new entry for
user/sharinghas been properly added to the user guide section of the documentation. This addition aligns with the new sharing functionality being implemented.src/main/webapp/app/programming/manage/update/programming-exercise-creation-config.ts (1)
25-25: New property correctly added to support sharing platform integration.The
isImportFromSharingboolean property has been added to theProgrammingExerciseCreationConfigtype definition. This addition is necessary to support the new functionality for importing exercises from the sharing platform.src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java (1)
211-212:Details
❓ Verification inconclusive
Security configuration appears correct, but verify the token implementation.
The sharing API endpoints are configured to be publicly accessible (
permitAll()), with the comment indicating that security is handled via explicit security tokens. This approach is common for API endpoints that implement their own authentication mechanism.Please verify that the sharing endpoint implementation properly validates security tokens before allowing access to sensitive operations. The security model should ensure that only authorized sharing platform instances can access these endpoints.
🏁 Script executed:
#!/bin/bash # Check for proper token validation in sharing endpoints echo "Searching for token validation in sharing endpoints..." rg -A 10 "api/sharing" --include="*.java"Length of output: 602
The revised review comment is as follows:
Attention: Confirm Explicit Token Validation for Sharing Endpoints
The security configuration currently allows public access to the sharing endpoints via
.permitAll(). While the accompanying comment notes that explicit security tokens protect these endpoints, our automated search using the updated-g "*.java"command did not yield conclusive evidence that token validation logic is implemented for these endpoints.Please manually verify that the sharing endpoint implementation (e.g., via token validation filters or interceptors) indeed enforces security restrictions so that only authorized sharing platform instances can perform sensitive operations.
--- 🏁 Script executed: ```shell #!/bin/bash echo "Searching for token validation in sharing endpoints (using -g flag for java files)..." rg -A 10 "api/sharing" -g "*.java"Length of output: 3482
Based on the updated search output showing that the sharing service constructs URLs with a token parameter for callbacks (via the "sec" parameter) but without direct evidence of token validation logic in view of the "permitAll" configuration, please manually verify that the endpoint receiving these requests (likely the controller handling
/api/sharing/export/{token}) contains the necessary security checks. It is essential to confirm that the token provided in the "sec" parameter is rigorously validated before processing sensitive operations.Action Required:
- Verify that the endpoint receiving requests at
/api/sharing/export/**includes proper token validation (e.g., via filters, interceptors, or explicit controller logic).- Confirm that any token-based security in the sharing API is robust and restricts access solely to authorized instances.
--- 🏁 Script executed: ```shell #!/bin/bash echo "Searching for token validation logic in code handling the 'sec' parameter..." rg -A 10 'getParameter\("sec"\)' -g "*.java"Length of output: 337
Action Required: Confirm Token Validation on Sharing Endpoints
The security configuration in
SecurityConfiguration.javacurrently allows public access to sharing endpoints via.permitAll(), with the expectation that token-based validation is performed elsewhere. Our searches indicate that while the sharing service constructs URLs embedding a token via the "sec" parameter, we did not find explicit token validation logic (e.g., by looking forgetParameter("sec")).Please manually verify that the endpoint handling requests at
/api/sharing/export/**(or any controller/filter receiving the token) implements the appropriate authorization checks. Ensure that the token supplied through the "sec" parameter undergoes stringent validation before any sensitive operations are executed.
- Review the export endpoint controller or filters: Confirm that each request is rigorously validated for the correct token.
- Validate integration with token management: Ensure that only authorized sharing platform instances can access these endpoints.
src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
133-141: Implementation looks good and follows service pattern.The new
isSharing()method follows the same pattern as other profile check methods in this class and correctly delegates to theisProfileActive()method with the appropriate constant. The Java documentation is clear and descriptive.src/main/webapp/app/app.routes.ts (1)
204-213: Route configuration follows Angular standardsThe route definition follows proper Angular conventions with lazy loading, appropriate metadata, and clear path naming structure.
docker/artemis/config/dev-local-vc-local-ci.env (1)
5-5: Appropriate profile additionAdding the
sharingprofile to the active Spring profiles is consistent with the PR objectives of enabling the sharing functionality.src/main/webapp/i18n/en/sharing.json (1)
1-22: Well-structured internationalization stringsThe i18n strings are properly organized in namespaces that match the application structure, with appropriate entries for titles, messages, and error states.
gradle.properties (1)
55-56: Memory allocation increased to support new features.The JVM heap size has been increased from 2GB to 4GB, which should provide adequate memory for the new sharing functionality being implemented.
src/main/webapp/app/programming/manage/programming-exercise.component.ts (2)
56-56: New import added for sharing functionality.The SharingInfo model is being imported from the sharing module.
61-61: SharingInfo added to providers array.Correctly adds the SharingInfo service to the component's providers array, making it available for dependency injection within this component and its children.
src/main/webapp/app/programming/manage/programming-exercise-detail.component.html (1)
101-103: New sharing component conditionally rendered.The programming exercise sharing component is correctly displayed only when sharing is enabled and the user has instructor permissions. This follows Angular's best practices for conditional rendering using the new @if syntax.
src/main/java/de/tum/cit/aet/artemis/sharing/SharingSetupInfo.java (2)
1-8: Well-structured import statements.The package declaration and imports are organized clearly, with correct usage of the Spring Profile annotation to conditionally enable this component.
9-13: Appropriate use of Java record for data encapsulation.The
SharingSetupInforecord efficiently encapsulates the three components needed for sharing functionality. The@Profile("sharing")annotation ensures this class is only active when the sharing profile is enabled.src/main/webapp/i18n/en/programmingExercise.json (1)
758-764: LGTM! Clear and concise sharing-related translations.The new section added for sharing functionality provides clear and concise translations that align well with the rest of the file's structure and style.
src/main/webapp/i18n/de/sharing.json (1)
1-22: Clean and complete German translations for sharing functionality.The translations are comprehensive and match the content needed for the sharing functionality.
src/main/webapp/app/sharing/sharing.route.ts (1)
1-15: LGTM! Well-structured route configuration.The route definition follows Angular best practices with proper authority checks for the sharing component.
docs/user/sharing.rst (1)
31-34: Thorough and well-structured documentation.The documentation provides a clear explanation of the Sharing Platform functionality, which will be helpful for users.
docs/admin/setup/sharing.rst (4)
26-26: LGTM: Clear contact instructions for API key request.The documentation clearly explains how to obtain an API key for connecting to the sharing platform.
33-55: Configuration section is well-structured and complete.The documentation provides clear configuration options for both application-sharing.yml and .env file, making it accessible for different deployment scenarios.
71-85: Health monitoring section is thorough and helpful.The troubleshooting section with health indicator information is well-documented with screenshots and clear explanations of what to look for.
90-94: Good cross-referencing to user documentation.The conclusion properly links to the user documentation and explains what to expect when the setup is complete.
src/main/webapp/app/sharing/sharing.scss (1)
71-83: Well-structured responsive grid layout.The container class implements a responsive grid layout using modern CSS Grid features like
minmax()andauto-fit, creating a flexible and maintainable design.src/main/webapp/app/programming/manage/update/programming-exercise-update.component.html (2)
3-3: LGTM: Correctly uses new Angular syntax for conditionals.The code properly uses the new
@ifsyntax as required by the coding guidelines, replacing the older*ngIfdirective.Also applies to: 7-7
19-19: LGTM: Consistent integration of sharing import option.The
isImportproperty binding now includesisImportFromSharingconsistently across all component sections that depend on this flag.Also applies to: 64-64
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (2)
1-11: LGTM: Well-structured exception class with proper package and annotation.The class follows Java naming conventions and is properly annotated with
@Profile("sharing")to ensure it's only available when the sharing profile is active.
32-41: LGTM: Well-designed static factory method.The static factory method provides a convenient way to create connection error exceptions with standardized messages, which improves code readability and maintenance.
build.gradle (1)
160-161:Details
✅ Verification successful
Explain the reason for downgrading HTTP client libraries
The PR is downgrading
httpclient5from 5.4.2 to 5.4.1 andhttpcore5from 5.3.3 to 5.3.1. Please add a comment explaining why these downgrades are necessary, as they might introduce compatibility issues.- implementation "org.apache.httpcomponents.client5:httpclient5:5.4.1" - implementation "org.apache.httpcomponents.core5:httpcore5:5.3.1" + // Using specific versions for compatibility with the Sharing Platform API + implementation "org.apache.httpcomponents.client5:httpclient5:5.4.1" + implementation "org.apache.httpcomponents.core5:httpcore5:5.3.1"Please check for compatibility issues that might arise from downgrading these HTTP libraries, especially in relation to the rest of the application.
🏁 Script executed:
#!/bin/bash # Check for other components that might use these HTTP libraries echo "Searching for dependencies on httpclient5 and httpcore5 in the codebase..." echo "-----------------------------------------------------------------------" rg -l "httpclient5|httpcore5" --type java echo "-----------------------------------------------------------------------" echo "Checking if there are imports of org.apache.hc packages:" rg "import org.apache.hc" --type javaLength of output: 796
Clarification on HTTP Library Downgrade
The downgrade—fromhttpclient55.4.2 to 5.4.1 andhttpcore55.3.3 to 5.3.1—has been implemented to maintain compatibility with the Sharing Platform API. A search through the codebase shows that the only direct usage of these libraries is inLinkPreviewResource.java(viaorg.apache.hc.core5.net.InetAddressUtils), which confirms that this version choice is aligned with our integration requirements. Please ensure that an inline comment documenting this rationale is added inbuild.gradleas illustrated:- implementation "org.apache.httpcomponents.client5:httpclient5:5.4.1" - implementation "org.apache.httpcomponents.core5:httpcore5:5.3.1" + // Using specific versions for compatibility with the Sharing Platform API + implementation "org.apache.httpcomponents.client5:httpclient5:5.4.1" + implementation "org.apache.httpcomponents.core5:httpcore5:5.3.1"src/main/webapp/i18n/de/programmingExercise.json (2)
631-631: Improved label clarity.The change from "Abgaben" (Submissions) to "Abgabelimit" (Submission limit) provides a more precise description of what this label represents.
759-765: Added German translations for new sharing functionality.These new translations support the sharing feature implementation by providing German localization for the import and export functionalities, as well as error messages.
src/main/webapp/app/sharing/sharing.component.ts (1)
36-36: Use initialization when declaring class members.The
sharingInfofield is initialized immediately after declaration, which is a good practice that should be applied to other relevant fields as well.src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (3)
385-389: Added Spring profile constant for sharing functionality.The new constant
PROFILE_SHARINGwill be used to conditionally enable and configure the sharing functionality, which aligns with the PR objectives.
390-394: Added resource path constant for sharing config requests.This constant defines the API endpoint for accessing sharing configuration, making the codebase more maintainable by centralizing the path definition.
425-438: Added resource path constants for sharing import/export and enablement check.These constants define additional API endpoints for sharing functionality, including import, export, and a check to determine if sharing is enabled.
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (1)
20-25: Good use of class-level documentation and Spring annotations.The service class is properly documented and configured with appropriate Spring annotations for dependency injection and profile-based activation.
src/main/webapp/app/sharing/search-result-dto.model.ts (5)
1-9: Check correctness of 'ranking5' property name.
The property nameranking5might be a remnant or placeholder. Verify if this name is truly required by the external contract or if a more descriptive name (e.g.,ranking) is intended.
17-42: External contract alignment is correct.
These properties inUserProvidedMetadataDTOalign with the external sharing platform's naming. Keeping them intact preserves compatibility as per the retrieved learnings.
50-55: Enum naming and string values comply with external requirements.
Maintaining theIExerciseTypeprefix and using lowercase string values (with spaces) is consistent with the retrieved learnings.
57-65: Snake_case fields match external contract.
This interface uses snake_case property names for compatibility with the external sharing platform.
67-72: Properly follows the snake_case convention.
MetadataFileDTOlikewise adheres to the sharing platform's naming expectations.src/main/webapp/app/programming/shared/actions/programming-exercise-instructor-exercise-sharing.component.ts (3)
1-8: Imports are well-organized.
The imported modules and services appear correct and relevant.
9-24: Inline template usage appears fine.
Ensure that the translation key'artemisApp.programmingExercise.sharing.export'exists and is properly localized.
31-39: @input property naming is consistent.
exerciseIdfollows lower camelCase. There are no issues with naming per Angular style guidelines.src/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.java (1)
92-98: Status responses look consistent.
Returning200withtrueor503withfalseproperly conveys the feature's availability.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistry.java (1)
1-39: Service initialization is logical.
Registering a custom health check contributor in@PostConstructis standard practice. No issues here.src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts (7)
58-58: Imports from RxJS operators look fine.
No issues found regarding usage or best practices.
71-71: Import statement added.
Importing the instructor-sharing component aligns with the new share functionality. No issues.
72-72: Import statement added.
The new sharing service import is consistent with the new feature integration.
94-94: UI component reference.
RegisteringProgrammingExerciseInstructorExerciseSharingComponentin the imports array is correct.
175-175: Subscription property added.
Proper to keep a separate reference for unsubscribing. No issues.
181-182: Constructor injection for sharing service is properly done.
Follows dependency injection best practices.
311-311: Excellent memory leak prevention.
Unsubscribing inngOnDestroyis a best practice.src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (2)
94-98: New parameter accommodates sharing imports.
Method signature is clear, and JavaDoc references the parameter correctly.
162-168: Overloaded method maintains backward compatibility.
No issues found; this provides a convenient default.src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (1)
1-211: Overall integration appears correct, but verify security requirements.
- Methods like
setUpFromSharingImportandgetProblemStatementlack explicit role checks (@PreAuthorize).- If only instructors should import or retrieve details, consider restricting access to these endpoints for security.
- The approach for streaming and cleaning up the exported file is well-crafted and helps manage temporary file resources.
Are these endpoints intentionally open to all authenticated users, or should we limit them to certain roles?
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts (11)
10-10: No issues with new import
The import forProgrammingExerciseSharingServiceis clear and follows single-quote conventions.
45-45: New import referencingSharingInfo
Works well with the newly introduced sharing model, following Angular style guidelines.
60-60: Added import forButtonComponent
Straightforward import. No concerns noted.
80-80: RegisteredButtonComponentin imports
Good addition to the module’s imports array.
85-85: Service injection for sharing
Leverages Angular’sinject()function. No issues found.
150-151: New boolean flagisImportFromSharing
Naming is descriptive and aligns with property naming guidelines.
203-205: AddedsharingInfoproperty
Centralizes sharing-related data for the component. Implementation looks fine.
478-479: Import creation condition
Straightforward conditional block callingcreateProgrammingExerciseForImportFromSharing.
726-726: Extended condition for build plans
IncludingisImportFromSharingis consistent with the existing logic for Aeolus or file-based imports.
775-784: Handling save for sharing imports
Uses the service’ssetUpFromSharingImportcall with proper success/error handling. Good approach.
951-974: Conditionally loading problem statement from sharing
Avoids duplication by branching between file-based imports and sharing-based imports.src/main/webapp/app/programming/manage/services/programming-exercise-sharing.service.ts (1)
1-134: New service for programming exercise sharing
The overall structure is clean. Resource URLs are well organized, and date handling via Day.js is consistent. The HTTP operations and Observables appear correct. No major issues found.src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
1-130: DTO for sharing information
Implementation is straightforward with clear field definitions, getters/setters, and acheckChecksummethod. Follows best practices; no concerns.src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (2)
390-411: False positives in static analysis on line 399
The static analysis warning referencing TDES and ECB mode is a false positive because the algorithm isHmacSHA256, which is not a deprecated cipher. You can safely ignore that specific warning unless there are other references to TDES or DESede.🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 399-399: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA(desede-is-deprecated-java)
[warning] 399-399: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html(use-of-aes-ecb-java)
448-451:Details
❓ Verification inconclusive
Potential mismatch with actual Base64 characters
isValidTokenchecks the token against the regex^[a-zA-Z0-9_-]+$, but standard Base64 often includes+or/. If your tokens use standard Base64, they may be invalidated. Consider updating the regex or clarifying that you are always producing URL-safe Base64 without padding.Would you like to verify token usage in the codebase to confirm all tokens use URL-safe patterns?
🏁 Script executed:
#!/bin/bash # Searching for 'isValidToken' references and checking if tokens are indeed URL-safe B64 only rg -A 5 $'isValidToken'Length of output: 1396
Clarification Needed on Base64 Token Encoding and Decoding Consistency
The method
isValidToken(lines 448–451) checks tokens against the regex^[a-zA-Z0-9_-]+$, which only permits URL-safe Base64 characters. However, the subsequent decoding withBase64.getDecoder().decode(b64Token)expects standard Base64 (which includes+and/). Please verify that the tokens passed to this service are always produced in a URL-safe Base64 format. If they are, consider usingBase64.getUrlDecoder()for clarity and consistency; otherwise, update the regex to accept standard Base64 characters.
| // ===== SHARING ===== | ||
| { | ||
| path: 'sharing/import/:basketToken', | ||
| data: { | ||
| authorities: [Authority.USER], | ||
| pageTitle: 'sharing.import.title', | ||
| }, | ||
| // canActivate: [UserRouteAccessService], | ||
| loadComponent: () => import('./sharing/sharing.component').then((m) => m.SharingComponent), | ||
| }, |
There was a problem hiding this comment.
Security concern: Uncomment canActivate guard
While the route is configured with appropriate authorities, the canActivate guard is commented out, which means the route is accessible without proper authorization checks.
- // canActivate: [UserRouteAccessService],
+ canActivate: [UserRouteAccessService],📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // ===== SHARING ===== | |
| { | |
| path: 'sharing/import/:basketToken', | |
| data: { | |
| authorities: [Authority.USER], | |
| pageTitle: 'sharing.import.title', | |
| }, | |
| // canActivate: [UserRouteAccessService], | |
| loadComponent: () => import('./sharing/sharing.component').then((m) => m.SharingComponent), | |
| }, | |
| // ===== SHARING ===== | |
| { | |
| path: 'sharing/import/:basketToken', | |
| data: { | |
| authorities: [Authority.USER], | |
| pageTitle: 'sharing.import.title', | |
| }, | |
| canActivate: [UserRouteAccessService], | |
| loadComponent: () => import('./sharing/sharing.component').then((m) => m.SharingComponent), | |
| }, |
...ava/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
Show resolved
Hide resolved
github-project-automation
bot
moved this from Work In Progress
to Ready For Review
in Artemis Development
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java (2)
19-21: Consider making constants private.These constants are only used within this class and could be made private to reduce unnecessary exposure.
- public static final ZoneId UTC = ZoneId.of("UTC"); - - public static final DateTimeFormatter TIME_STAMP_FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yy HH:mm:ss"); + private static final ZoneId UTC = ZoneId.of("UTC"); + + private static final DateTimeFormatter TIME_STAMP_FORMATTER = DateTimeFormatter.ofPattern("dd.MM.yy HH:mm:ss");
30-52: Consider externalizing the timeout threshold.The health status determination uses a hardcoded value of 11 minutes. Consider making this configurable through a property to allow for adjustment without code changes.
+ private final int healthStatusTimeoutMinutes; + public SharingHealthIndicator(SharingConnectorService sharingConnectorService) { super(); this.sharingConnectorService = sharingConnectorService; + this.healthStatusTimeoutMinutes = 11; // Default value could be moved to a configuration property } @Override public Health health() { SharingConnectorService.HealthStatusWithHistory lastHealthStati = sharingConnectorService.getLastHealthStati(); Health.Builder health; if (lastHealthStati.getLastConnect() == null) { health = Health.down(); } - else if (lastHealthStati.getLastConnect().isBefore(Instant.now().minus(11, ChronoUnit.MINUTES))) { + else if (lastHealthStati.getLastConnect().isBefore(Instant.now().minus(healthStatusTimeoutMinutes, ChronoUnit.MINUTES))) { health = Health.unknown(); }src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java (2)
20-21: Review PMD suppression warning.The
@SuppressWarnings("PMD.ImmutableField")annotation is used, but the field doesn't need to be immutable since it's autowired by Spring. Consider removing this suppression if it's not necessary.- @SuppressWarnings("PMD.ImmutableField") private SharingHealthIndicator sharingHealthIndicator;
23-25: Remove empty initialization method.The
initTestCase()method is empty and does not perform any setup. Consider removing it if not needed.- @BeforeEach - void initTestCase() { - }src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java (2)
34-44: Use more descriptive test method names.The method name "trivialTests" doesn't clearly describe what is being tested. Consider renaming to better reflect the purpose, such as "testGettersAndSetters" or "testConfigurationProperties".
- void trivialTests() { + void testConfigurationProperties() {
49-50: Add a comment explaining the reason for the huge key test.It would be helpful to explain why testing with a large key is important (e.g., verifying that keys exceeding a certain length are rejected).
- String hugeKey = "huge" + "0123456789".repeat(50); + // Test with a key that exceeds maximum allowed length + String hugeKey = "huge" + "0123456789".repeat(50);src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (2)
18-18: Use the standard Jackson library instead of the shaded testcontainers version.
Relying onorg.testcontainers.shaded.com.fasterxml.jackson.databind.ObjectMappercould cause conflicts. Consider using the standardcom.fasterxml.jackson.databind.ObjectMapperfrom your project dependencies.
46-46: Rename method to improve consistency and avoid typographical discrepancies.
“Plattform” is likely a typographical variant. Consider renamingconnectRequestFromSharingPlattformtoconnectRequestFromSharingPlatformfor clarity.-public SharingPluginConfig connectRequestFromSharingPlattform() throws Exception { +public SharingPluginConfig connectRequestFromSharingPlatform() throws Exception {
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java(1 hunks)src/main/webapp/app/app.routes.ts(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/app.routes.ts
- src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java
🧰 Additional context used
📓 Path-based instructions (2)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/shared/base/AbstractSpringIntegrationIndependentTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java
`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/exercise/service/sharing/SharingHealthIndicator.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
🧬 Code Definitions (3)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java (1)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java (1)
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (1)
Component(23-78)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (5)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(70-463)
🔇 Additional comments (10)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationIndependentTest.java (2)
12-12: Addition of sharing profile import looks good.The import for
PROFILE_SHARINGconstant is correctly added to support the new sharing functionality.
47-47: Adding sharing profile to test configuration is appropriate.The addition of
PROFILE_SHARINGto the active profiles ensures that tests using this base class will have the sharing functionality available, which aligns with the PR objectives of integrating the CodeAbility Sharing Platform.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java (2)
15-17: Component and profile annotations look good.The class is correctly annotated with
@Componentand@Profile("sharing")to ensure it's only active when the sharing profile is enabled.
25-28: Constructor injection is correctly implemented.Using constructor injection for the dependency is the preferred approach as per the coding guidelines.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java (3)
32-39: Test validation is well structured.The test correctly verifies that the health indicator returns a UP status when the sharing platform is available.
41-52: History limit test is comprehensive.The test effectively verifies that the health status history does not exceed the defined limit, using appropriate assertions.
54-60: Health DOWN test is properly implemented.The test correctly verifies that the health indicator returns a DOWN status when the sharing platform is unavailable.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java (1)
46-59: API key validation test is thorough.The test covers various scenarios including null values, oversized keys, valid keys, and invalid keys. This ensures robust input validation.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (1)
69-76: Ensure test coverage for negative scenarios.
Whensuccessis false, this method callsreset(), but there appear to be no tests verifying that code path. Confirm coverage to prevent regressions in future changes.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
208-226: Check for concurrency race conditions invalidatewhenapiKeycan be updated.
This code may still face concurrency issues, as noted in a past comment. IfsetSharingApiKeyis called whilevalidateruns, unexpected outcomes could occur.
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
Outdated
Show resolved
Hide resolved
Michael-Breu-UIbk
force-pushed
the
feature/connecting-to-sharing-platform
branch
from
57c4be3 to
65bf329
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java (1)
63-67: Consider adding Javadoc and a short timeout variant for consistency.The method correctly follows the pattern of other REST template beans, but lacks Javadoc comments unlike other similar methods (e.g., aeolusRestTemplate). Additionally, most other profiles have a corresponding shortTimeout variant but there isn't one for the sharing profile.
Consider adding Javadoc and a short timeout variant:
@Bean @Profile(PROFILE_SHARING) + /** + * Creates a RestTemplate that can be used to communicate with the Sharing Platform + * + * @return a RestTemplate for sharing platform integration + */ public RestTemplate sharingRestTemplate() { return createRestTemplate(); } + @Bean + @Profile(PROFILE_SHARING) + /** + * Creates a RestTemplate with short timeouts that can be used to communicate with the Sharing Platform + * + * @return a RestTemplate with short timeouts for sharing platform integration + */ + public RestTemplate shortTimeoutSharingRestTemplate() { + return createShortTimeoutRestTemplate(); + }src/main/webapp/app/sharing/sharing.component.ts (2)
88-93: Use optional chaining for cleaner code.The
courseId()method could be simplified by leveraging optional chaining and nullish coalescing for clarity:- if (this.selectedCourse && this.selectedCourse.id) { - return this.selectedCourse.id; - } - return 0; + return this.selectedCourse?.id ?? 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 89-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
112-128: Fix console usage & indentation issues.Prettier flags indentation inconsistencies, and your linter warns about
consoleusage. Consider removing or replacing theconsole.logcalls with a dedicated logging or alert mechanism, and ensure the block is properly formatted according to your Prettier configuration:- this.router.navigate(importBaseRoute, { - queryParams: { - basketToken: this.sharingInfo.basketToken, - apiBaseUrl: this.sharingInfo.apiBaseURL, - returnUrl: this.sharingInfo.returnURL, - selectedExercise: this.sharingInfo.selectedExercise, - checksum: this.sharingInfo.checksum, - }, - }).then(nav => { - console.log(nav); // true if navigation is successful - }, err => { - console.log(err) // when there's an error - }); + this.router + .navigate(importBaseRoute, { + queryParams: { + basketToken: this.sharingInfo.basketToken, + apiBaseUrl: this.sharingInfo.apiBaseURL, + returnUrl: this.sharingInfo.returnURL, + selectedExercise: this.sharingInfo.selectedExercise, + checksum: this.sharingInfo.checksum, + }, + }) + .then( + (nav) => { + // handle successful navigation + }, + (err) => { + // handle navigation error + }, + );🧰 Tools
🪛 ESLint
[error] 115-115: Insert
⏎············(prettier/prettier)
[error] 116-116: Insert
····(prettier/prettier)
[error] 117-117: Insert
····(prettier/prettier)
[error] 118-118: Insert
····(prettier/prettier)
[error] 119-119: Replace
················with····················(prettier/prettier)
[error] 120-120: Insert
····(prettier/prettier)
[error] 121-121: Replace
················with····················(prettier/prettier)
[error] 123-123: Replace
}).then(navwith····})⏎············.then(⏎················(nav)(prettier/prettier)
[error] 124-124: Insert
········(prettier/prettier)
[error] 124-124: 'console' is not defined.
(no-undef)
[error] 125-125: Replace
},·errwith······},⏎················(err)(prettier/prettier)
[error] 126-126: Replace
············console.log(err)with····················console.log(err);(prettier/prettier)
[error] 126-126: 'console' is not defined.
(no-undef)
[error] 127-127: Replace
··········}with················},⏎············(prettier/prettier)
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java (1)
34-46: Clarify the test method name
The test nametrivialTestsis not descriptive. Consider renaming it to reflect the checks being performed, e.g.testInstallationNameAndApiKeyBehavior.-void trivialTests() { +void testInstallationNameAndApiKeyBehavior() {src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceTest.java (1)
146-159: Add debug logging for checksum mismatches
Consider adding a debug log after computing the checksum to facilitate troubleshooting when mismatches occur.+ // Insert optional debug log for better traceability + log.debug("Computed checksum: {}, for params: {}", checkSum, paramsToCheckSum);src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (1)
49-58: Connection verification
The code checks that the plugin name is "Artemis Sharing Connector." Consider refining the method name to correct the minor spelling of "Plattform."-public SharingPluginConfig connectRequestFromSharingPlattform() throws Exception { +public SharingPluginConfig connectRequestFromSharingPlatform() throws Exception {src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
232-235: Ensure cross-platform Docker localhost resolution
Replacinglocalhostwithhost.docker.internalonly works properly on certain Docker Desktop environments. This might fail on Linux systems wherehost.docker.internalis not automatically available. Consider parameterizing this at runtime or providing a fallback to accommodate different Docker installations.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/test/resources/de/tum/cit/aet/artemis/exercise/service/sharing/basket/sampleExercise.zipis excluded by!**/*.zip,!**/*.zip
📒 Files selected for processing (14)
src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java(1 hunks)src/main/webapp/app/programming/manage/programming-exercise-management.route.ts(1 hunks)src/main/webapp/app/sharing/sharing.component.html(1 hunks)src/main/webapp/app/sharing/sharing.component.ts(1 hunks)src/main/webapp/i18n/de/sharing.json(1 hunks)src/main/webapp/i18n/en/sharing.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java(1 hunks)src/test/resources/de/tum/cit/aet/artemis/exercise/service/sharing/basket/sampleBasket.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/test/resources/de/tum/cit/aet/artemis/exercise/service/sharing/basket/sampleBasket.json
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/webapp/i18n/de/sharing.json
- src/main/webapp/i18n/en/sharing.json
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java
- src/main/webapp/app/sharing/sharing.component.html
🧰 Additional context used
📓 Path-based instructions (3)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/programming/manage/programming-exercise-management.route.tssrc/main/webapp/app/sharing/sharing.component.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/exercise/service/sharing/ExerciseSharingResourceTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java
`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/core/config/RestTemplateConfiguration.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.javasrc/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
🧬 Code Definitions (5)
src/main/webapp/app/sharing/sharing.component.ts (3)
src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts (1)
Component(74-793)src/main/webapp/app/programming/manage/programming-exercise.component.ts (1)
Component(58-266)src/main/webapp/app/sharing/sharing.model.ts (1)
ShoppingBasket(50-54)
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java (1)
Profile(33-185)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java (1)
Component(15-53)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (4)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-328)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(69-454)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (5)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-328)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
Service(30-271)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(69-454)
🪛 Biome (1.9.4)
src/main/webapp/app/sharing/sharing.component.ts
[error] 89-89: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 ESLint
src/main/webapp/app/sharing/sharing.component.ts
[error] 115-115: Insert ⏎············
(prettier/prettier)
[error] 116-116: Insert ····
(prettier/prettier)
[error] 117-117: Insert ····
(prettier/prettier)
[error] 118-118: Insert ····
(prettier/prettier)
[error] 119-119: Replace ················ with ····················
(prettier/prettier)
[error] 120-120: Insert ····
(prettier/prettier)
[error] 121-121: Replace ················ with ····················
(prettier/prettier)
[error] 123-123: Replace }).then(nav with ····})⏎············.then(⏎················(nav)
(prettier/prettier)
[error] 124-124: Insert ········
(prettier/prettier)
[error] 124-124: 'console' is not defined.
(no-undef)
[error] 125-125: Replace },·err with ······},⏎················(err)
(prettier/prettier)
[error] 126-126: Replace ············console.log(err) with ····················console.log(err);
(prettier/prettier)
[error] 126-126: 'console' is not defined.
(no-undef)
[error] 127-127: Replace ··········} with ················},⏎············
(prettier/prettier)
🪛 ast-grep (0.31.1)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
[warning] 390-390: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 390-390: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (35)
src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java (1)
8-8: LGTM: Import follows the existing pattern.The static import for PROFILE_SHARING follows the existing pattern and avoids star imports as per the coding guidelines.
src/main/webapp/app/sharing/sharing.component.ts (1)
59-61: Duplicate Comment: Add error handling for API request.As previously noted, there's currently no error handling for the subscription. Consider handling errors to avoid unexpected runtime exceptions, for instance by adding an
error:callback block in thesubscribemethod.src/main/webapp/app/programming/manage/programming-exercise-management.route.ts (1)
61-72: Looks good.The route definition is consistent with the rest of the file and meets the specified authorities requirement.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java (5)
1-13: Well-structured test class imports and definitions
No issues found with these lines. The usage ofAbstractSpringIntegrationIndependentTestis consistent with JUnit 5 best practices.
18-22: Good use of dependency injection
The autowiring ofSharingPlatformMockProviderandSharingConnectorServiceclarifies test dependencies clearly.
24-32: BeforeEach & AfterEach
Connecting and resetting the mock provider ensures a pristine test environment for each test.
47-61: Comprehensive coverage for API key validation
This test method checks various edge cases (null, too large, valid, fake). It's robust.
15-17:Details
❓ Verification inconclusive
Check environment variable retrieval for tests
Storing the API key in the test environment is properly handled. Just ensure there is no leakage of real credentials in logs or commits.Run the following script to search for any direct references or accidental commits of a real API key:
🏁 Script executed:
#!/bin/bash # Searching for suspicious lines in the repository referencing the artemis.sharing.apikey property with a possible value rg "artemis\.sharing\.apikey\s*=\s*[A-Za-z0-9]+" -A 3 -B 3Length of output: 60
API Key Retrieval Appears Safe—but Please Manually Verify Sensitive Credential Handling
The
@Value("${artemis.sharing.apikey:#{null}}")injection in the test is correctly configured to avoid including a real API key. The initial search for hard-coded API key values didn't return any results. However, the output was inconclusive, so please manually confirm that no configuration or commit accidentally exposes real credentials.
- Verify that no files (especially configuration files) accidentally set a literal API key value.
- Confirm that logs do not output any sensitive API key information during tests.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceTest.java (9)
1-33: Imports and test class definition
No immediate issues. The usage ofMockMvcandMockRestServiceServeris appropriate for integration testing.
49-82: Test environment setup
The injection ofMockMvc,SharingConnectorService,restTemplate, andmockServer, along with@BeforeEachand@AfterEachmethods, follows typical test patterns.
83-90: Simple isUp test
Testing/api/sharing/config/isEnabledand verifying a "true" response is a straightforward way to confirm the sharing configuration is active.
91-106: High-level import test
testImportBasket()delegates toimportBasket()and keeps the test concise. Good structure for reuse.
103-124: Mock service calls for basket import
importBasket()verifies correct retrieval of sample basket data and checks the user’s email. Good use of mocks and assertions.
126-144: Basket not found scenario
Properly tests a 404 response when attempting to import a nonexistent basket.
161-170: Checksum utility
This method nicely centralizes the checksum calculation logic. The even-parameter check is a helpful validation.
176-182: Checksum mismatch test
Ensures the REST endpoint responds with 400 when provided an incorrect checksum.
184-218: Detailed exercise import test
importExerciseInfos()thoroughly validates the scenario by fetching a ZIP and parsing the returnedProgrammingExercise. Good coverage of normal operation.src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (7)
1-22: Shared test utility class
Provides a convenient setup to simulate and verify sharing platform interactions.
26-30: Constants
Defining clear constants for the test environment helps avoid magic strings and is easy to maintain.
32-36: Shared secret key injection
Using@Valuewith a safe fallback reduces the risk of missing environment variables. Just ensure no real keys are inadvertently committed.
38-42: MockMvc and connector service injection
Dependency injection is straightforward, aligning with typical Spring test approaches.
44-48: Default constructor
Although empty, it is valid for Spring initialization.
60-65: Reset logic
CallingsharingConnectorService.shutDown()ensures the platform is reset, preventing leftover state across tests.
72-79: Mock success or failure
Switching between a successful connection and a reset scenario is a flexible way to simulate different conditions in tests.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (5)
1-25: Class definition
Annotated with@Serviceand@Profile("sharing"), ensuring the class is only active under the sharing profile.
27-31: Logger usage
Using SLF4J for logging is standard and allows configurable log levels.
32-46: Dependency injection
The final fields clarify the service responsibilities: reuse existing file import logic, handle shared exercise service, and retrieve the user.
47-59: Constructor injection
Explicit constructor injection is aligned with best practices, making dependencies simpler to manage and test.
61-82: Reusing import logic
This method delegates the actual import toprogrammingExerciseImportFromFileService. The null checks, exception handling, and user retrieval are well-structured.src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (3)
209-223: Possible null return ingetCachedBasketItem
This concern was raised in a previous review. Returningnullhere can lead to aNullPointerExceptionin downstream calls (e.g.,getEntryFromBasket). Consider returning anOptional<SharingMultipartZipFile>or throwing an exception if no cached basket item is found.
331-373: Graceful handling of export errors
This concern was flagged previously. TheexportExerciseToSharingmethod logs errors but returnsnullon failure. Consider returning a dedicated error response or re-throwing aSharingException, allowing callers to differentiate between legitimatenulloutcomes and errors.
389-395: Static analysis false positive
The static analysis warnings about deprecated Triple DES or AES-ECB do not apply here because the algorithm used isHmacSHA256. No changes needed.🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 390-390: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA(desede-is-deprecated-java)
[warning] 390-390: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html(use-of-aes-ecb-java)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (3)
66-67: Invalid method call onArrayList
In prior feedback, removing the oldest element of anArrayListrequiresremove(0)rather thanremoveFirst(). Confirm if there's a custom extension method; if not, consider fixing to avoidNoSuchMethodError.
208-226: Potential concurrency issues invalidate
As noted previously, updatingsharingApiKeywhilevalidateis running could lead to race conditions. Consider synchronization, immutability, or at least improved logging for debugging.
232-237: Thread pool resource management
Previously noted: usingnewFixedThreadPool(1)without retaining its reference or shutting it down may lead to resource leaks. Consider a shared or gracefully managed executor, or use Spring’s async capabilities.
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
src/main/webapp/app/sharing/sharing.component.ts (1)
136-139: 🛠️ Refactor suggestionAdd error handling for the
getSharedExercisesrequest.This subscription does not handle any potential errors. If the call fails, the user won't receive feedback. Consider adding an error callback with
alertService.erroror similar to inform users of the failure.this.programmingExerciseSharingService.getSharedExercises(this.sharingInfo).subscribe({ next: (res: ShoppingBasket) => { this.shoppingBasket = res; }, + error: (error) => { + this.alertService.error('artemisApp.sharing.error.loadingBasket', { message: error.message }); + } });
🧹 Nitpick comments (18)
src/test/javascript/spec/component/sharing/sharing.component.spec.ts (4)
29-30: Remove commented-out code or convert it to a TODO note.Leaving commented-out lines can lead to confusion. If you don't plan on using this variable, please remove it; otherwise, add a TODO note explaining why it remains.
- // let accountServiceStub: jest.SpyInstance; - // ...
76-76: Use more descriptive test names."straight process" lacks clarity on what the scenario represents. Consider renaming it to something like "should load basket and courses, then handle navigation successfully" for more explicit meaning.
102-103: Expand or remove the "not really a good test" comment.You mention that validating
sortRows()here is "not really a good test." If so, consider removing or improving this test to ensure actual behavior verification. Minimally, please explain its utility or remove it if it's not necessary.
137-173: Consolidate repeated error handling checks.This block calls
sharingInfo.validate()multiple times in separate try/catch blocks. Consider a single parameterized test or a helper function to streamline repeated error expectations, improving maintainability and test readability.src/main/webapp/app/sharing/sharing.component.ts (2)
78-82: Use optional chaining for simpler code.To simplify the null/undefined check for
selectedCourse, use the optional chaining operator and nullish coalescing:- if (this.selectedCourse && this.selectedCourse.id) { - return this.selectedCourse.id; - } - return 0; + return this.selectedCourse?.id ?? 0;🧰 Tools
🪛 Biome (1.9.4)
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
129-139: Consider unsubscribing from route observables.The route subscriptions here may remain active after the component is destroyed unless handled by Angular's router-outlet. As a best practice, especially in complex components, implement
OnDestroyand unsubscribe to avoid potential memory leaks.src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java (1)
25-33: Fix typo in method nameThere's a typo in the method name:
connectRequestFromSharingPlattform()should beconnectRequestFromSharingPlatform()(one 't' instead of two).- void startUp() throws Exception { - sharingPlatformMockProvider.connectRequestFromSharingPlattform(); - } + void startUp() throws Exception { + sharingPlatformMockProvider.connectRequestFromSharingPlatform(); + }src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java (3)
97-97: Unused ObjectMapper declarationThe
objectMapperis declared but used only once in the class. Consider making it a local variable in the method where it's used to improve code clarity.
137-149: Same typo in connectRequestFromSharingPlattform()The same typo exists in this class's startup method:
connectRequestFromSharingPlattform()should beconnectRequestFromSharingPlatform().- void startUp() throws Exception { - sharingPlatformMockProvider.connectRequestFromSharingPlattform(); - } + void startUp() throws Exception { + sharingPlatformMockProvider.connectRequestFromSharingPlatform(); + }
166-175: Duplicate code in calculateCorrectChecksumThe method
calculateCorrectChecksumduplicates logic fromaddCorrectChecksum. Consider refactoring to avoid duplication by extracting the common parameter processing logic.-private String calculateCorrectChecksum(String... params) { - Map<String, String> paramsToCheckSum = new HashMap<>(); - assertThat(params.length % 2).isEqualTo(0).describedAs("paramList must contain even elements"); - for (int i = 0; i < params.length; i = i + 2) { - String paramName = params[i]; - String paramValue = params[i + 1]; - paramsToCheckSum.put(paramName, paramValue); - } - return SecretChecksumCalculator.calculateChecksum(paramsToCheckSum, sharingApiKey); -} +private Map<String, String> parseParamsToMap(String... params) { + Map<String, String> paramsMap = new HashMap<>(); + assertThat(params.length % 2).isEqualTo(0).describedAs("paramList must contain even elements"); + for (int i = 0; i < params.length; i = i + 2) { + String paramName = params[i]; + String paramValue = params[i + 1]; + paramsMap.put(paramName, paramValue); + } + return paramsMap; +} + +private String calculateCorrectChecksum(String... params) { + Map<String, String> paramsToCheckSum = parseParamsToMap(params); + return SecretChecksumCalculator.calculateChecksum(paramsToCheckSum, sharingApiKey); +} + +private MockHttpServletRequestBuilder addCorrectChecksum(MockHttpServletRequestBuilder request, String... params) { + Map<String, String> paramsToCheckSum = parseParamsToMap(params); + + // Add params to request + paramsToCheckSum.forEach((paramName, paramValue) -> request.queryParam(paramName, paramValue)); + + String checkSum = SecretChecksumCalculator.calculateChecksum(paramsToCheckSum, sharingApiKey); + request.queryParam("checksum", checkSum); + return request; +}src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java (2)
47-47: Commented out unused constantThere's a commented-out constant
SAMPLE_BASKET_TOKEN. Either remove it completely or uncomment it if it's needed for future tests.-// public static final String SAMPLE_BASKET_TOKEN = "sampleBasketToken.json";
58-65: Same typo in method nameThe same typo exists in this class:
connectRequestFromSharingPlattform()should beconnectRequestFromSharingPlatform().- void startUp() throws Exception { - sharingPlatformMockProvider.connectRequestFromSharingPlattform(); - } + void startUp() throws Exception { + sharingPlatformMockProvider.connectRequestFromSharingPlatform(); + }src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (2)
92-102: Consider logging checksum validation failure.
While returning a Bad Request response is correct when the checksum validation fails, adding a log statement or metric could help detect suspicious traffic and troubleshoot issues.if (SecretChecksumCalculator.checkChecksum(Map.of("returnURL", returnURL, "apiBaseURL", apiBaseURL), sharingConnectorService.getSharingApiKeyOrNull(), checksum)) { ... } else { + log.warn("Checksum validation failed for basketToken={}", basketToken); return ResponseEntity.badRequest().body(null); }
153-163: Optional: unify error handling strategy.
The method logs and returns an error message on exceptions. For consistency, consider using a global exception handler or a standardized error response structure across all sharing endpoints.src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (2)
188-213: Use of loading cache with file system cleanup.
Your approach to delete files upon cache eviction is good. However, consider concurrency scenarios (e.g., multiple threads attempting to load and evict the same file). Confirm that potential race conditions or partial file reads won't occur.Would you like assistance checking concurrency correctness with a script or additional test coverage?
261-277: Explain difference between problem statement vs. exercise details.
Since both retrieve data from the basket, clarify why the logic differs or unify them if duplication can be avoided. This can reduce maintenance effort and potential drift when updating one method but not the other.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (2)
209-226: Enhance debug logging on key mismatch.
When validation fails, you log a generic message. Adding more context (e.g., partial keys hashed, IP address) can help trace invalid attempts without disclosing sensitive information.
253-265: Consider robust reinit error handling.
WhenrestTemplate.getForObject(..., Boolean.class)fails, the method logs an info message but doesn’t retry or bubble up an error. If reinitialization is critical, consider a retry mechanism or explicit error handling strategy.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java(1 hunks)src/main/webapp/app/sharing/sharing.component.html(1 hunks)src/main/webapp/app/sharing/sharing.component.ts(1 hunks)src/main/webapp/app/sharing/sharing.model.ts(1 hunks)src/main/webapp/i18n/en/programmingExercise.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java(2 hunks)src/test/javascript/spec/component/sharing/sharing.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/webapp/app/sharing/sharing.model.ts
- src/main/webapp/app/sharing/sharing.component.html
- src/main/webapp/i18n/en/programmingExercise.json
🧰 Additional context used
📓 Path-based instructions (4)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logi...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/sharing/sharing.component.spec.ts
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/sharing/sharing.component.ts
`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/sharing/ExerciseSharingService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java
🧬 Code Definitions (7)
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java (1)
src/main/webapp/app/sharing/sharing.model.ts (1)
validate(37-44)
src/test/javascript/spec/component/sharing/sharing.component.spec.ts (4)
src/test/javascript/spec/helpers/mocks/service/mock-account.service.ts (1)
MockAccountService(7-44)src/test/javascript/spec/helpers/mocks/service/mock-translate.service.ts (1)
MockTranslateService(9-56)src/main/webapp/app/sharing/sharing.model.ts (1)
ShoppingBasket(50-54)src/main/webapp/app/entities/course.model.ts (1)
Course(59-179)
src/main/webapp/app/sharing/sharing.component.ts (3)
src/main/webapp/app/entities/course.model.ts (1)
Course(59-179)src/main/webapp/app/app.constants.ts (1)
ARTEMIS_DEFAULT_COLOR(19-19)src/main/webapp/app/sharing/sharing.model.ts (1)
ShoppingBasket(50-54)
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java (3)
src/main/webapp/app/entities/course.model.ts (1)
Course(59-179)src/test/java/de/tum/cit/aet/artemis/programming/AbstractProgrammingIntegrationLocalCILocalVCTest.java (1)
AbstractProgrammingIntegrationLocalCILocalVCTest(30-110)src/main/webapp/app/entities/programming/programming-exercise.model.ts (1)
ProgrammingExercise(50-101)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-328)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
Service(30-271)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(72-448)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (4)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.java (1)
ResponseUtil(18-37)
🪛 Biome (1.9.4)
src/main/webapp/app/sharing/sharing.component.ts
[error] 78-78: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 ast-grep (0.31.1)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
[warning] 386-386: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 386-386: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🔇 Additional comments (17)
src/test/java/de/tum/cit/aet/artemis/shared/base/AbstractSpringIntegrationLocalCILocalVCTest.java (2)
13-13: Added import for PROFILE_SHARING constantThe addition of this import is consistent with the pattern used for other profiles and supports the new sharing functionality being implemented.
76-77: Added sharing profile to active test profilesThe sharing profile has been correctly added to the active profiles used during testing. This ensures that the new sharing functionality is properly tested in integration tests that extend this class.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java (2)
13-16: Comprehensive JavaDoc commentThe class-level JavaDoc clearly explains the purpose of the test class and references related test classes for full functionality testing.
17-17: Test class naming follows JUnit 5 conventionsThe test class name follows good naming practices for JUnit 5, clearly indicating what it's testing.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java (4)
48-58: Class documentation and setupThe test class has good documentation and properly sets up the required components for testing the import features.
78-86: Consistent use of setUp and tearDown patternThe test follows the standard JUnit pattern of using
@BeforeEachand@AfterEachto set up and clean up test state.
151-164: Well implemented checksum utility methodThe
addCorrectChecksummethod is well implemented with proper validation of input parameters and clear error messages.
187-236: Comprehensive import test coverageThe test methods cover essential aspects of import functionality including validation of checksums, handling of missing resources, and successful imports.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java (4)
79-92: Good test for error handlingThis test verifies the correct error handling when repositories are missing, with proper assertions on the response status and content.
97-127: Comprehensive export testThe test for exporting with repositories is well structured and verifies multiple aspects including the redirect URL, parameters, and the actual binary content.
129-139: Well designed utility methodThe
convertToGetRequestBuildermethod is well designed, clearly documented with the@NotNullannotation, and follows good practices for URI parameter handling.
141-146: Test for invalid token security checkGood security test to verify that unauthorized access attempts with invalid tokens are properly rejected.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (1)
173-208: Review the lack of security annotation for file export.
This method currently relies on token validation alone. Ensure that no additional role-based restrictions are necessary or confirm that it is intentionally exposed for external use.Do you need to restrict this endpoint to specific roles, or is the token security adequate?
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (4)
164-174: Guard against null returns when constructing SharingMultipartZipFile.
Optional.of(zipFileItem)is constructed immediately after the cache retrieval. If the file is unexpectedly null or deleted, it could trigger anIOExceptionorNullPointerException. Double-check if the caching logic always guarantees a valid existing file before returning.
241-253: Potential security risk: HTML-escaping alone may be insufficient.
Escaping HTML tags helps mitigate XSS, but if the statement can contain Markdown or other syntax, deeper sanitization or a robust library approach may be required.
373-398: Ignore static analysis warnings about TDES or AES.
The code usesHmacSHA256, which is not DES-based nor ECB-based AES, so the tool’s deprecation warnings appear to be false positives.🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 386-386: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA(desede-is-deprecated-java)
[warning] 386-386: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html(use-of-aes-ecb-java)
411-431: Edge case: ensure the token can handle forward slashes.
If base64 tokens (without '=' padding) accidentally include '/', quickly verify they passisValidTokenor if replacement logic is needed. This can prevent unexpected 404 or token rejections.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
Outdated
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
Show resolved
Hide resolved
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
215-219: Null check needed to avoid potential NullPointerException
IfgetCachedBasketItemreturnsnull, callinggetInputStream()directly on the returned value ingetEntryFromBasket(line 291) can cause an NPE. This was previously flagged in older reviews and remains unresolved.Also applies to: 289-291
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (2)
208-226: Validate method thread-safety
The API key can be updated at runtime, yet there is no locking or final assignment forsharingApiKey. This might lead to inconsistent validation outcomes under concurrency. This issue was flagged before.
232-241: Reconsider creating and immediately shutting down a new ExecutorService
UsingnewSingleThreadExecutor()on each startup call may lead to overhead if the operation is retried or triggered repeatedly. A persistent or shared executor can avoid repeated instantiation and potential concurrency pitfalls. This was noted in past reviews.
🧹 Nitpick comments (18)
src/main/webapp/app/sharing/sharing.component.ts (4)
79-84: Enhance code with optional chaining.The
getTokenExpiryDatemethod can be simplified using optional chaining to make the code more concise and readable.getTokenExpiryDate(): Date { - if (this.shoppingBasket) { - return new Date(this.shoppingBasket.tokenValidUntil); - } - return new Date(); + return new Date(this.shoppingBasket?.tokenValidUntil ?? Date.now()); }
140-162: Improve error handling in navigation method.The
navigateToImportFromSharingmethod has error handling for navigation failures, but the returned values from the promise handlers aren't used properly. The first handler returns a boolean that isn't being utilized.navigateToImportFromSharing() { const importBaseRoute = ['/course-management', this.courseId(), 'programming-exercises']; importBaseRoute.push('import-from-sharing'); this.router .navigate(importBaseRoute, { queryParams: { basketToken: this.sharingInfo.basketToken, apiBaseUrl: this.sharingInfo.apiBaseURL, returnUrl: this.sharingInfo.returnURL, selectedExercise: this.sharingInfo.selectedExercise, checksum: this.sharingInfo.checksum, }, }) .then( - (nav) => { - return true; // true if navigation is successful - }, + () => { + // Navigation successful + }, (err) => { // just a technical branch, should never be called this.alertService.addErrorAlert(`Cannot navigate to imported exercise: ${err}`); // when there's an error }, ); }
1-13: Organize imports using categories.The imports follow Angular style guidelines, although they could be organized better by grouping them into categories: Angular core, third-party libraries, and application imports.
Consider organizing imports into categories:
- Angular core imports
- Third-party imports (e.g., @FortAwesome)
- Application imports
38-38: Use type for const variables.The
ARTEMIS_DEFAULT_COLORconstant is properly initialized from the app constants, but could benefit from explicit typing for better code readability and type safety.-readonly ARTEMIS_DEFAULT_COLOR = ARTEMIS_DEFAULT_COLOR; +readonly ARTEMIS_DEFAULT_COLOR: string = ARTEMIS_DEFAULT_COLOR;src/test/javascript/spec/component/sharing/sharing.component.spec.ts (2)
36-52: Use MockComponent instead of manually adding the component to declarations.The test module configuration includes
SharingComponentin the declarations, but since it's a standalone component, this is unnecessary and could lead to issues. Instead, useMockComponentfrom ng-mocks for testing.TestBed.configureTestingModule({ imports: [MockModule(ReactiveFormsModule)], - declarations: [SharingComponent, TranslatePipeMock, MockDirective(TranslateDirective)], + declarations: [TranslatePipeMock, MockDirective(TranslateDirective)], providers: [ provideHttpClient(), provideHttpClientTesting(), { provide: AccountService, useClass: MockAccountService }, { provide: AlertService, useClass: MockAlertService }, { provide: ActivatedRoute, useValue: route }, { provide: Router, useClass: MockRouter }, { provide: TranslateService, useClass: MockTranslateService }, { provide: SessionStorageService, useClass: MockSyncStorage }, MockProvider(ProfileService), MockProvider(StaticContentService), ], })
136-171: Add test for error handling in getSharedExercises.The tests cover basket token validation and course loading errors, but there's no specific test for error handling in the
getSharedExercisescall in the component.Consider adding a test case that simulates an error response from the
getSharedExercisesAPI call and verifies that the error handler in the component is called correctly.src/main/webapp/app/sharing/search-result-dto.model.ts (1)
16-21: Consider adding more documentation for PluginActionInfo.While the interface is well-defined, the properties could benefit from more detailed JSDoc comments explaining their purpose and usage.
/** * sharing platform plugin actions */ export interface PluginActionInfo { + /** Name of the plugin that provides the action */ plugin: string; + /** Type of action supported by the plugin */ action: string; + /** Command name that can be executed */ commandName: string; }src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (7)
20-22: Improve class-level documentationThe Javadoc comment is incomplete and doesn't fully describe the purpose and functionality of this class. Consider expanding it to explain that this class provides mock infrastructure for testing the integration with the CodeAbility Sharing Platform.
/** - * provides infrastructure to set up and shutdown + * Provides infrastructure to set up and shut down mock servers for the Sharing Platform. + * This class is used in tests to simulate interactions with the CodeAbility Sharing Platform, + * allowing for controlled testing of the sharing connector functionality. */
45-45: Configure ObjectMapper with appropriate settingsThe ObjectMapper is initialized without any specific configuration. For consistent test behavior, consider configuring it with standard settings that match the application's ObjectMapper configuration.
- private final ObjectMapper objectMapper = new ObjectMapper(); + private final ObjectMapper objectMapper = new ObjectMapper() + .configure(DeserializationFeature.FAIL_ON_UNKNOWN_PROPERTIES, false) + .registerModule(new JavaTimeModule());Additionally, import the required classes:
import com.fasterxml.jackson.databind.DeserializationFeature; import com.fasterxml.jackson.datatype.jsr310.JavaTimeModule;
47-48: Document or remove empty constructorThe empty constructor doesn't provide any functionality. Consider adding documentation explaining why it's needed or remove it if it's not necessary (as Spring can autowire dependencies without an explicit constructor).
+/** + * Default constructor required for Spring to instantiate this component. + */ public SharingPlatformMockProvider() { }
68-73: Complete the incomplete Javadoc for mockStatus methodThe Javadoc comment is incomplete and doesn't clearly explain what the method does.
/** - * registers or shuts down the required + * Registers with the sharing platform or shuts down the connection based on the success parameter. * * @param success Successful response or timeout. */
23-25: Consider implementing test lifecycle interfacesSince this class provides test infrastructure, consider implementing Spring's test lifecycle interfaces like
ApplicationContextInitializerorTestExecutionListenerto better integrate with Spring's test framework.This would allow for more systematic setup and teardown of the mock environment during test execution.
3-7: Use selective imports instead of static imports for better readabilityWhile static imports can make code more concise, they can also reduce readability by obscuring the source of methods. Consider using selective static imports only for frequently used assertion methods.
import static de.tum.cit.aet.artemis.core.config.Constants.PROFILE_SHARING; - import static org.assertj.core.api.Assertions.assertThat; + import org.assertj.core.api.Assertions; import static org.springframework.test.web.servlet.request.MockMvcRequestBuilders.get; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.content; import static org.springframework.test.web.servlet.result.MockMvcResultMatchers.status;Then update the assertion:
- assertThat(sharingPluginConfig.pluginName).isEqualTo("Artemis Sharing Connector"); + Assertions.assertThat(sharingPluginConfig.pluginName).isEqualTo("Artemis Sharing Connector");
33-37: Document the expected format and usage of the API keyThe Javadoc for the
sharingApiKeyfield doesn't explain what format is expected or how it should be used in authentication. Consider adding this information to help future developers./** - * the shared secret api key + * The shared secret API key used for authentication with the Sharing Platform. + * This key is expected to be a non-null string in the format required by the + * Sharing Platform API. It is injected from the application properties. */src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java (1)
43-44: Consider making the 11-minute threshold configurable.The current approach of hardcoding 11 minutes may be perceived as a "magic number." Allowing this threshold to be set via a configuration property would improve flexibility and maintainability.
- else if (lastHealthStati.getLastConnect().isBefore(Instant.now().minus(11, ChronoUnit.MINUTES))) { + long thresholdInMinutes = 11; // Could be injected from application properties + if (lastHealthStati.getLastConnect().isBefore(Instant.now().minus(thresholdInMinutes, ChronoUnit.MINUTES))) {src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java (1)
127-128: Use more generic or anonymized test data.The test currently uses an actual-looking email (
michael.breu@uibk.ac.at). Consider using an obviously fictitious or anonymized email address to avoid any unintended PII usage and keep test data generic.- assertThat(sb.userInfo.email).isEqualTo("michael.breu@uibk.ac.at"); + assertThat(sb.userInfo.email).isEqualTo("test.user@example.com");src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (1)
193-208: Consider using a more standard temporary file approach.Manually deleting the file in the
close()method of a customFileInputStreamworks, but there is a risk if multiple downloads happen simultaneously, or if an exception occurs before the stream is closed. A more robust approach might be:
- Use Java’s temporary file APIs (
Files.createTempFile) which are automatically cleaned up.- Employ a try-with-resources pattern to ensure the file is always deleted.
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
140-154: Consider returning more specific error responses
When the basket retrieval fails for reasons other than “basket not found,” returning an empty Optional might be conflated with a non-existent basket. Differentiating server errors from truly absent baskets can improve diagnosability for the client.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
docs/user/sharing.rst(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java(1 hunks)src/main/webapp/app/sharing/search-result-dto.model.ts(1 hunks)src/main/webapp/app/sharing/sharing.component.ts(1 hunks)src/main/webapp/app/sharing/sharing.scss(1 hunks)src/main/webapp/i18n/de/sharing.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java(1 hunks)src/test/javascript/spec/component/sharing/sharing.component.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/webapp/i18n/de/sharing.json
- src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java
- src/main/webapp/app/sharing/sharing.scss
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingServiceTest.java
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorHealthCheckRegistryTest.java
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorServiceTest.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java
🧰 Additional context used
📓 Path-based instructions (4)
`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/exercise/service/sharing/SharingHealthIndicator.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.javasrc/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logi...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/sharing/sharing.component.spec.ts
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/exercise/service/sharing/SharingPlatformMockProvider.javasrc/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/sharing/search-result-dto.model.tssrc/main/webapp/app/sharing/sharing.component.ts
🧠 Learnings (1)
src/main/webapp/app/sharing/search-result-dto.model.ts (4)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:17-42
Timestamp: 2025-04-07T13:18:35.405Z
Learning: The types in `UserProvidedMetadataDTO` interface in `search-result-dto.model.ts` must match the sharing platform's contract exactly to maintain compatibility. Avoid modifying these types even if they don't follow internal TypeScript conventions.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:57-65
Timestamp: 2025-04-07T13:18:26.733Z
Learning: The ProjectDTO interface in src/main/webapp/app/sharing/search-result-dto.model.ts uses snake_case property names to maintain compatibility with the external sharing platform's API contract, which is an intentional deviation from Angular's camelCase convention.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:50-55
Timestamp: 2025-04-07T13:18:26.733Z
Learning: The `IExerciseType` enum in `src/main/webapp/app/sharing/search-result-dto.model.ts` must maintain its current naming (with 'I' prefix) and lowercase string values to ensure compatibility with the external sharing platform connector interface. This is an intentional exception to our TypeScript naming conventions.
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/sharing/search-result-dto.model.ts:67-72
Timestamp: 2025-04-07T13:18:26.733Z
Learning: The properties in MetadataFileDTO (commit_id, indexing_date) and ProjectDTO (project_id, project_name, etc.) use snake_case as they map to an external sharing platform's API, making this an intentional exception to Angular's camelCase naming convention.
🧬 Code Graph Analysis (6)
src/test/javascript/spec/component/sharing/sharing.component.spec.ts (4)
src/test/javascript/spec/helpers/mocks/service/mock-account.service.ts (1)
MockAccountService(7-44)src/test/javascript/spec/helpers/mocks/service/mock-translate.service.ts (1)
MockTranslateService(9-56)src/main/webapp/app/sharing/sharing.model.ts (1)
ShoppingBasket(50-54)src/main/webapp/app/entities/course.model.ts (1)
Course(59-179)
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingPlatformMockProvider.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java (1)
Profile(33-185)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingHealthIndicator.java (1)
Component(15-53)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (4)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/core/util/ResponseUtil.java (1)
ResponseUtil(18-37)
src/main/webapp/app/sharing/sharing.component.ts (3)
src/main/webapp/app/entities/course.model.ts (1)
Course(59-179)src/main/webapp/app/app.constants.ts (1)
ARTEMIS_DEFAULT_COLOR(19-19)src/main/webapp/app/sharing/sharing.model.ts (1)
ShoppingBasket(50-54)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
Service(30-271)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (1)
Service(23-82)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (4)
src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java (1)
Profile(18-142)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(72-448)
🪛 Biome (1.9.4)
src/main/webapp/app/sharing/sharing.component.ts
[error] 82-85: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🪛 ast-grep (0.31.1)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
[warning] 386-386: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
[warning] 386-386: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (11)
src/main/webapp/app/sharing/sharing.component.ts (3)
22-31: LGTM - Component defined correctly with standalone: true.The component declaration follows Angular best practices using the standalone component pattern with proper imports. This approach improves modularity and tree-shaking capabilities.
167-190: Consider limiting route subscriptions withtake(1).The ngOnInit method already properly uses
take(1)to limit route parameter subscriptions, which prevents memory leaks and avoids calling handlers multiple times. This is a good practice that should be maintained.
109-111: Consider using nullish coalescing for courseId method.The
courseIdmethod uses optional chaining with nullish coalescing, which is a good practice but could be made more concise.src/test/javascript/spec/component/sharing/sharing.component.spec.ts (3)
75-110: Use expectation specificity in test assertions.The test assertions follow the coding guidelines for expectation specificity by using the correct matchers such as
toStrictEqual, which improves test precision.
115-115: Consider using specific date assertions.When testing dates, the custom matcher
toBeBetweenis used, which is good for handling time-sensitive assertions that might vary slightly during test execution.
129-133: Good use of Jest spy verification.The test correctly uses
toHaveBeenCalledOnce()to verify that the error handler was called exactly once, following the coding guidelines for spy call verification.src/main/webapp/app/sharing/search-result-dto.model.ts (5)
4-12: Well-structured DTO with appropriate interface definition.The
SearchResultDTOinterface is well-designed to represent search results from the sharing platform with all necessary properties.
26-51: Maintain exact contract compatibility with sharing platform.According to the retrieved learnings, the types in
UserProvidedMetadataDTOmust match the sharing platform's contract exactly to maintain compatibility, even if they don't follow internal TypeScript conventions.
65-70: Keep 'I' prefix and lowercase values for IExerciseType.The
IExerciseTypeenum correctly maintains its 'I' prefix and lowercase string values to ensure compatibility with the external sharing platform connector interface, which is an intentional exception to TypeScript naming conventions.
75-83: Snake_case properties match external API convention.The properties in
ProjectDTOuse snake_case as they map to an external sharing platform's API, making this an intentional exception to Angular's camelCase naming convention.
88-93: Snake_case consistent in MetadataFileDTO.The properties in
MetadataFileDTO(commit_id, indexing_date) correctly use snake_case to maintain compatibility with the external API contract.
| public SharingPluginConfig connectRequestFromSharingPlatform() throws Exception { | ||
| MvcResult result = restMockMvc | ||
| .perform(get("/api/sharing/config").queryParam("apiBaseUrl", SHARING_BASEURL_PLUGIN).queryParam("installationName", TEST_INSTALLATION_NAME) | ||
| .header("Authorization", sharingApiKey).contentType(MediaType.APPLICATION_JSON)) | ||
| .andExpect(content().contentType(MediaType.APPLICATION_JSON)).andExpect(status().isOk()).andReturn(); | ||
| String content = result.getResponse().getContentAsString(); | ||
| SharingPluginConfig sharingPluginConfig = objectMapper.readerFor(SharingPluginConfig.class).readValue(content); | ||
| assertThat(sharingPluginConfig.pluginName).isEqualTo("Artemis Sharing Connector"); | ||
| return sharingPluginConfig; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Javadoc and error handling for the connectRequestFromSharingPlatform method
This important method lacks documentation. Additionally, it contains an assertion that might be better placed in a test method rather than in a helper method.
+/**
+ * Simulates a connection request from the Sharing Platform to Artemis.
+ * This method sends a request to the Artemis sharing configuration endpoint with
+ * predefined test parameters and validates the response.
+ *
+ * @return The SharingPluginConfig received from the Artemis endpoint
+ * @throws Exception If the request fails or the response is invalid
+ */
public SharingPluginConfig connectRequestFromSharingPlatform() throws Exception {
+ if (sharingApiKey == null) {
+ throw new IllegalStateException("Sharing API key is not configured");
+ }
+
MvcResult result = restMockMvc
.perform(get("/api/sharing/config").queryParam("apiBaseUrl", SHARING_BASEURL_PLUGIN).queryParam("installationName", TEST_INSTALLATION_NAME)
.header("Authorization", sharingApiKey).contentType(MediaType.APPLICATION_JSON))
.andExpect(content().contentType(MediaType.APPLICATION_JSON)).andExpect(status().isOk()).andReturn();
String content = result.getResponse().getContentAsString();
SharingPluginConfig sharingPluginConfig = objectMapper.readerFor(SharingPluginConfig.class).readValue(content);
assertThat(sharingPluginConfig.pluginName).isEqualTo("Artemis Sharing Connector");
return sharingPluginConfig;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public SharingPluginConfig connectRequestFromSharingPlatform() throws Exception { | |
| MvcResult result = restMockMvc | |
| .perform(get("/api/sharing/config").queryParam("apiBaseUrl", SHARING_BASEURL_PLUGIN).queryParam("installationName", TEST_INSTALLATION_NAME) | |
| .header("Authorization", sharingApiKey).contentType(MediaType.APPLICATION_JSON)) | |
| .andExpect(content().contentType(MediaType.APPLICATION_JSON)).andExpect(status().isOk()).andReturn(); | |
| String content = result.getResponse().getContentAsString(); | |
| SharingPluginConfig sharingPluginConfig = objectMapper.readerFor(SharingPluginConfig.class).readValue(content); | |
| assertThat(sharingPluginConfig.pluginName).isEqualTo("Artemis Sharing Connector"); | |
| return sharingPluginConfig; | |
| } | |
| /** | |
| * Simulates a connection request from the Sharing Platform to Artemis. | |
| * This method sends a request to the Artemis sharing configuration endpoint with | |
| * predefined test parameters and validates the response. | |
| * | |
| * @return The SharingPluginConfig received from the Artemis endpoint | |
| * @throws Exception If the request fails or the response is invalid | |
| */ | |
| public SharingPluginConfig connectRequestFromSharingPlatform() throws Exception { | |
| if (sharingApiKey == null) { | |
| throw new IllegalStateException("Sharing API key is not configured"); | |
| } | |
| MvcResult result = restMockMvc | |
| .perform(get("/api/sharing/config") | |
| .queryParam("apiBaseUrl", SHARING_BASEURL_PLUGIN) | |
| .queryParam("installationName", TEST_INSTALLATION_NAME) | |
| .header("Authorization", sharingApiKey) | |
| .contentType(MediaType.APPLICATION_JSON)) | |
| .andExpect(content().contentType(MediaType.APPLICATION_JSON)) | |
| .andExpect(status().isOk()) | |
| .andReturn(); | |
| String content = result.getResponse().getContentAsString(); | |
| SharingPluginConfig sharingPluginConfig = objectMapper.readerFor(SharingPluginConfig.class).readValue(content); | |
| assertThat(sharingPluginConfig.pluginName).isEqualTo("Artemis Sharing Connector"); | |
| return sharingPluginConfig; | |
| } |
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts (1)
1290-1316: Sharing import initialization.The method properly extracts sharing information from query parameters and loads exercise details from the sharing platform.
There are some commented-out lines (1312-1315) that should be removed as they're no longer needed.
- /* - this.programmingExerciseSharingService.loadProblemStatementForExercises(this.sharingInfo).subscribe((statement: string) => { - this.programmingExercise.problemStatement = statement; - }); - */
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts(18 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java
🧰 Additional context used
📓 Path-based instructions (1)
`src/main/webapp/**/*.ts`: angular_style:https://angular.io/guide/styleguide;methods_in_html:false;lazy_loading:true;code_reuse:true;tests:meaningful;types:PascalCase;enums:PascalC...
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts
🧠 Learnings (1)
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#9909
File: src/main/webapp/app/exercises/programming/manage/services/programming-exercise-sharing.service.ts:120-124
Timestamp: 2025-04-07T13:18:44.502Z
Learning: In the Artemis sharing platform integration, passing callBackUrl in exportProgrammingExerciseToSharing is safe as it's returned to the same authenticated user session through internal API endpoints.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
🔇 Additional comments (16)
src/main/webapp/app/programming/manage/update/programming-exercise-update.component.ts (16)
10-10: Services properly injected.The ProgrammingExerciseSharingService is correctly imported for the integration with the CodeAbility Sharing Platform.
45-45: Model imported correctly.The SharingInfo model is properly imported from the sharing module.
60-60: Component import and registration looks good.The ButtonComponent is correctly imported and added to the imports array of the component.
Also applies to: 80-80
85-85: Service correctly injected.The ProgrammingExerciseSharingService is properly injected using the modern inject function.
154-154: Import flag added.The isImportFromSharing flag is used to track whether the exercise is being imported from the sharing platform.
207-209: Sharing info model initialized.The sharingInfo object is properly initialized to store information needed for communication with the sharing platform.
134-137: UI conditional logic for sharing imports.This conditional logic ensures that the short name field is displayed when importing from sharing, which is important for maintaining proper exercise identification.
353-353: Template loading condition updated.The conditions properly account for the new import from sharing scenario to determine when to load programming language templates.
Also applies to: 374-374
472-473: Import path detection and handling.The code correctly identifies when exercises are being imported from sharing and calls the appropriate method to handle the import flow.
Also applies to: 481-483
496-497: Course ID handling for sharing imports.The courseId retrieval is properly extended to handle the import from sharing case alongside the existing import from file case.
521-521: Problem statement handling for imports.This condition properly accounts for the sharing import case when determining if the problem statement is already loaded.
730-731: Build plan customization for sharing imports.The condition for customizing build plans with Aeolus is extended to include imports from sharing, which ensures proper CI/CD setup.
779-788: Exercise saving logic for sharing imports.The code correctly handles saving exercises imported from sharing by using the specialized sharing service methods.
955-978: Problem statement loading from sharing platform.This code provides a clean separation between standard template loading and fetching problem statements from the sharing platform.
The error handling is appropriate, providing user feedback and fallback to an empty statement if loading fails.
1321-1321: Configuration updated for sharing imports.The ProgrammingExerciseCreationConfig is correctly updated to include the isImportFromSharing flag.
1298-1304: Defensive programming when handling exercise details.Good practice to check if buildConfig exists and create a new one if needed before proceeding with the import process.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (13)
src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts (3)
171-171: Remove unnecessary 'async' keywords.The 'async' keywords are used in the test methods, but there are no 'await' calls within these functions. Since you're using HttpTestingController for HTTP request expectations instead of async/await pattern, these keywords are unnecessary.
- it('should be in sharing mode', async () => { + it('should be in sharing mode', () => {- it('should not be in sharing mode', async () => { + it('should not be in sharing mode', () => {Also applies to: 184-184
147-147: Remove duplicate mock for profileService.getProfileInfo.The mock for
profileService.getProfileInfois set up twice - once on line 143 and again on line 147, which is redundant.- jest.spyOn(profileService, 'getProfileInfo').mockReturnValue(of(profileInfo));
37-37: Remove or update the comment about merging files.The comment suggests that this file "could be merged into programming-exercise-detail.component.spec.ts if stabilized." This kind of implementation note should either be removed or updated to include specific criteria for when merging should occur, as it may become outdated or confusing to future developers.
src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingInfoDTOTest.java (2)
9-9: Use a more descriptive test class name.The class name
SharingInfoDTOTestis acceptable, but consider adding a suffix likeEqualsTestor elaborating further if you expect to have multiple test methods in the future.
11-46: Test method name should be more expressive.The method name
someEqualsTestslacks clarity on the intended scenario under test. Consider renaming it to something liketestEqualsMethodortestEqualityBehaviorfor greater clarity.src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (3)
68-71: Cover null course reference earlier if possible.While you check if
sharingSetupInfo.getExercise()is null, also consider validating the presence of a validcourseearlier if it is a required field. This helps ensure a more predictable state.
72-75: Validate existence of the zip file more explicitly.Currently, the code throws a
SharingExceptionifgetCachedBasketItem(...)returns empty. You might further log or clarify why the cached item is missing. This helps with future troubleshooting.
78-80: Set the course once at initialization if always needed.Setting the course if missing is correct, but centralizing this logic in a separate helper method or ensuring it is set by the caller might improve clarity and reduce coupling.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (2)
92-103: Return consistent error structures in the response body.Currently returning
badRequest().body(null)when checks fail. Consider returning a JSON object with an error message or reason for clarity and consistent error handling.
140-147: Consider returning detailed responses for errors.Similar to
getProblemStatement, consider returning a more descriptive response instead ofbadRequest().body(null).src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (3)
133-154: Graceful handling of basket retrieval.
This method returns anOptional.empty()whenever it encounters aRestClientException, which conflates unexpected REST errors with a legitimate “basket not found” scenario. Consider returning distinct results or creating a dedicated exception in unexpected error cases to help the caller distinguish between a valid “not found” response and other errors.
164-174: Consider aligning optional usage with exceptions.
Here, you are wrapping the cache retrieval in anOptional, yet still throw aSharingExceptionif an error occurs. Ensure this pattern is consistent: either rely on exceptions for errors and return a non-empty/emptyOptionalfor present/missing items, or unify your approach for clearer API usage.
348-392: Ensure the exported URL is secured.
The generated callback URL is appended with an HMAC, which is good. However, ensure that the final request to the sharing platform over the network is secured via HTTPS in production, especially since tokens and HMAC signatures are involved.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java(1 hunks)src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts(7 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java(1 hunks)src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingInfoDTOTest.java(1 hunks)src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceExportTest.java
- src/test/java/de/tum/cit/aet/artemis/exercise/service/sharing/ExerciseSharingResourceImportTest.java
- src/main/webapp/app/programming/manage/programming-exercise-detail.component.ts
🧰 Additional context used
📓 Path-based instructions (3)
`src/test/java/**/*.java`: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_...
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/exercise/service/sharing/SharingInfoDTOTest.java
`src/test/javascript/spec/**/*.ts`: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logi...
src/test/javascript/spec/**/*.ts: jest: true; mock: NgMocks; bad_practices: avoid_full_module_import; perf_improvements: mock_irrelevant_deps; service_testing: mock_http_for_logic; no_schema: avoid_NO_ERRORS_SCHEMA; expectation_specificity: true; solutions: {boolean: toBeTrue/False, reference: toBe, existence: toBeNull/NotNull, undefined: toBeUndefined, class_obj: toContainEntries/toEqual, spy_calls: {not_called: not.toHaveBeenCalled, once: toHaveBeenCalledOnce, with_value: toHaveBeenCalledWith|toHaveBeenCalledExactlyOnceWith}}
src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts
`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/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.javasrc/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.javasrc/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
🧬 Code Graph Analysis (4)
src/test/javascript/spec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts (4)
src/main/webapp/app/entities/programming/programming-exercise.model.ts (1)
ProgrammingExercise(50-101)src/main/webapp/app/shared/layouts/profiles/profile-info.model.ts (1)
ProfileInfo(6-63)src/test/javascript/spec/helpers/mocks/service/mock-profile.service.ts (1)
MockProfileService(4-9)src/test/javascript/spec/helpers/mocks/service/mock-programming-exercise.service.ts (1)
MockProgrammingExerciseService(6-26)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-332)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/sharing/SharingMultipartZipFile.java (1)
Profile(19-86)src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (1)
Service(72-472)
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (3)
src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java (1)
Constants(14-482)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java (1)
Profile(48-332)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java (1)
Profile(12-129)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/SharingConnectorService.java (1)
Service(30-275)
🪛 ast-grep (0.31.1)
src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java
[warning] 407-407: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 407-407: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
🔇 Additional comments (8)
src/main/java/de/tum/cit/aet/artemis/exercise/service/sharing/ProgrammingExerciseImportFromSharingService.java (1)
81-89: General import logic is sound.The try-catch rethrows the encountered exception, allowing error visibility. This is good practice for debugging.
src/main/java/de/tum/cit/aet/artemis/exercise/web/ExerciseSharingResource.java (3)
111-117: Endpoint name is aligned with functionality.The
setUpFromSharingImportmethod correctly uses a POST to initiate an import action. The[INSTRUCTOR,EDITOR,ADMIN]role check is also good.
125-132: Validate request body for completeness.You validate the checksum, but also consider verifying required fields like
exercisePosition. Null or invalid data could lead to unexpected behavior.Do you want to verify the presence of required fields using Bean Validation annotations or a custom check?
178-214: Local class deletion logic can cause concurrency conflicts.
NewFileInputStreamdeletes the file on close. If multiple reads are triggered, the file might get deleted prematurely. If this scenario is unlikely, this is fine; otherwise, ensure concurrency or access is managed.src/main/java/de/tum/cit/aet/artemis/sharing/ExerciseSharingService.java (4)
176-214: Verify concurrency handling for LoadingCache.
While Guava’sLoadingCacheis thread-safe, double-check that the rest of the code assumes safe concurrent usage (e.g., no shared mutable state). If concurrency is expected, confirm that any file I/O performed in the removal listener does not lead to race conditions when multiple files are being cached or removed simultaneously.
249-265: Good use of HTML escaping for user-provided content.
HtmlUtils.htmlEscape()helps mitigate injection risks in problem statements. This measure is important to ensure potentially malicious Markdown content is sanitized before rendering.
273-293: Validate unknown properties carefully.
By ignoring unknown properties in the JSON viaFAIL_ON_UNKNOWN_PROPERTIES = false, the service could overlook potential misconfiguration. Confirm that you genuinely need this loose parsing. If not, consider re-enabling strict checks to catch accidental property name mismatches.
400-418: Static analysis false positive regarding insecure cipher usage.
The tool’s warning about using TDES or AES-ECB appears to be a false positive. You are correctly using “HmacSHA256,” which is a recommended hashing algorithm for generating HMAC signatures. No change is needed here.🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 407-407: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA(desede-is-deprecated-java)
[warning] 407-407: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html(use-of-aes-ecb-java)
...ec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts
Show resolved
Hide resolved
...ec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts
Show resolved
Hide resolved
...ec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts
Show resolved
Hide resolved
...ec/component/programming-exercise/programming-exercise-detail.component.with-sharing.spec.ts
Show resolved
Hide resolved
| public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException { | ||
| InputStream repositoryStream; | ||
| try { | ||
| Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo); | ||
| if (cachedBasketItem.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
| repositoryStream = cachedBasketItem.get().getInputStream(); | ||
| } | ||
| catch (IOException | SharingException e) { | ||
| log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e); | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream);) { | ||
|
|
||
| ZipEntry entry; | ||
| while ((entry = zippedRepositoryStream.getNextEntry()) != null) { | ||
| Matcher matcher = matchingPattern.matcher(entry.getName()); | ||
| if (matcher.find()) { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| byte[] buffer = new byte[102400]; | ||
| int bytesRead; | ||
| while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) { | ||
| baos.write(buffer, 0, bytesRead); | ||
| } | ||
| String entryContent = baos.toString(StandardCharsets.UTF_8); | ||
| baos.close(); | ||
| zippedRepositoryStream.closeEntry(); | ||
| return Optional.of(entryContent); | ||
| } | ||
| zippedRepositoryStream.closeEntry(); | ||
| } | ||
| return null; // Not found | ||
| } | ||
| } |
There was a problem hiding this comment.
Return an empty Optional instead of null.
This method declares Optional<String> but returns null at line 337 if no matching entry is found. Returning null defeats the purpose of using an Optional.
Apply this fix:
- return null; // Not found
+ return Optional.empty(); // Not found📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException { | |
| InputStream repositoryStream; | |
| try { | |
| Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo); | |
| if (cachedBasketItem.isEmpty()) { | |
| return Optional.empty(); | |
| } | |
| repositoryStream = cachedBasketItem.get().getInputStream(); | |
| } | |
| catch (IOException | SharingException e) { | |
| log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e); | |
| return Optional.empty(); | |
| } | |
| try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream);) { | |
| ZipEntry entry; | |
| while ((entry = zippedRepositoryStream.getNextEntry()) != null) { | |
| Matcher matcher = matchingPattern.matcher(entry.getName()); | |
| if (matcher.find()) { | |
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| byte[] buffer = new byte[102400]; | |
| int bytesRead; | |
| while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) { | |
| baos.write(buffer, 0, bytesRead); | |
| } | |
| String entryContent = baos.toString(StandardCharsets.UTF_8); | |
| baos.close(); | |
| zippedRepositoryStream.closeEntry(); | |
| return Optional.of(entryContent); | |
| } | |
| zippedRepositoryStream.closeEntry(); | |
| } | |
| return null; // Not found | |
| } | |
| } | |
| public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException { | |
| InputStream repositoryStream; | |
| try { | |
| Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo); | |
| if (cachedBasketItem.isEmpty()) { | |
| return Optional.empty(); | |
| } | |
| repositoryStream = cachedBasketItem.get().getInputStream(); | |
| } | |
| catch (IOException | SharingException e) { | |
| log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e); | |
| return Optional.empty(); | |
| } | |
| try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream);) { | |
| ZipEntry entry; | |
| while ((entry = zippedRepositoryStream.getNextEntry()) != null) { | |
| Matcher matcher = matchingPattern.matcher(entry.getName()); | |
| if (matcher.find()) { | |
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| byte[] buffer = new byte[102400]; | |
| int bytesRead; | |
| while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) { | |
| baos.write(buffer, 0, bytesRead); | |
| } | |
| String entryContent = baos.toString(StandardCharsets.UTF_8); | |
| baos.close(); | |
| zippedRepositoryStream.closeEntry(); | |
| return Optional.of(entryContent); | |
| } | |
| zippedRepositoryStream.closeEntry(); | |
| } | |
| return Optional.empty(); // Not found | |
| } | |
| } |
github-actions
bot
removed
the
exercise
Michael-Breu-UIbk
force-pushed
the
feature/connecting-to-sharing-platform
branch
from
baa0029 to
67c7945
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (2)
77-87:⚠️ Potential issueFix thread safety issue with
lastConnectaccess.Methods
getLastConnect()andsetLastConnect()are not synchronized whileadd()is, creating a data race risk. Another thread could see a partially constructedInstantor outdated value.Make the field volatile or synchronize both methods:
- public Instant getLastConnect() { - return lastConnect; + public synchronized Instant getLastConnect() { + return lastConnect; } - public void setLastConnect() { - this.lastConnect = Instant.now(); + public synchronized void setLastConnect() { + this.lastConnect = Instant.now(); }
281-283: 🛠️ Refactor suggestionAvoid exposing internal mutable state to callers.
getLastHealthStati()returns the live list, allowing external code to modify it or iterate without synchronization, leading to potential data races.Return an unmodifiable copy:
-public HealthStatusWithHistory getLastHealthStati() { - return lastHealthStati; +public List<HealthStatus> getLastHealthStatiSnapshot() { + synchronized (lastHealthStati) { + return List.copyOf(lastHealthStati); + } }src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java (1)
455-458: 🛠️ Refactor suggestionExpand token validation logic.
The regex
"^[a-zA-Z0-9_-]+$"excludes certain valid Base64 characters (=,+, etc.), potentially causing valid tokens to be rejected.Broaden the pattern to include all valid Base64 characters:
private boolean isValidToken(String token) { // Implement validation logic, e.g., check for illegal characters or patterns - return token.matches("^[a-zA-Z0-9_-]+$"); + // Example alternative covering standard Base64 set (excluding whitespace or line breaks). + return token.matches("^[A-Za-z0-9+/=_-]+$"); }
🧹 Nitpick comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (1)
37-37: Fix typo in constant name.The constant name has a typo: "installationame" instead of "installation name".
- public static final String UNKNOWN_INSTALLATIONAME = "unknown installationame"; + public static final String UNKNOWN_INSTALLATION_NAME = "unknown installation name";src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java (1)
439-453: Return Optional instead of null for better API design.The method returns
nullto indicate that a file doesn't exist or the token is invalid, which requires null checks by callers and can lead toNullPointerExceptions.Use
Optional<File>for clearer semantics:-public File getExportedExerciseByToken(String b64Token) { +public Optional<File> getExportedExerciseByToken(String b64Token) { if (!isValidToken(b64Token)) { log.warn("Invalid token received: {}", b64Token); - return null; + return Optional.empty(); } String decodedToken = new String(Base64.getDecoder().decode(b64Token)); Path parent = Paths.get(repoDownloadClonePath, decodedToken + ".zip"); File exportedExercise = parent.toFile(); if (exportedExercise.exists()) { - return exportedExercise; + return Optional.of(exportedExercise); } else { - return null; + return Optional.empty(); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (14)
docs/admin/setup/sharing/sharingButtonArtemis.pngis excluded by!**/*.png,!**/*.pngdocs/admin/setup/sharing/sharingButtonSharing.pngis excluded by!**/*.png,!**/*.pngdocs/admin/setup/sharing/sharing_health1.pngis excluded by!**/*.png,!**/*.pngdocs/admin/setup/sharing/sharing_health2.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/artemis_import.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/artemis_import2.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_export.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_metadata.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_metadata2.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_namespace.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_search.pngis excluded by!**/*.png,!**/*.pngdocs/user/sharing/sharing_success.pngis excluded by!**/*.png,!**/*.pngsrc/main/resources/config/application-dev.ymlis excluded by!**/*.ymlsrc/main/resources/config/application-sharing.ymlis excluded by!**/*.yml
📒 Files selected for processing (22)
build.gradle(2 hunks)docker/artemis/config/dev-local-vc-local-ci.env(1 hunks)docs/admin/extension-services.rst(1 hunks)docs/admin/setup/sharing.rst(1 hunks)docs/index.rst(1 hunks)docs/user/sharing.rst(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java(3 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java(2 hunks)src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java(5 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ProgrammingExerciseImportFromSharingService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingException.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingHealthIndicator.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingMultipartZipFile.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingSetupInfo.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java(1 hunks)src/main/webapp/app/app.routes.ts(1 hunks)
✅ Files skipped from review due to trivial changes (2)
- build.gradle
- docker/artemis/config/dev-local-vc-local-ci.env
🚧 Files skipped from review as they are similar to previous changes (18)
- src/main/java/de/tum/cit/aet/artemis/core/service/ProfileService.java
- docs/admin/extension-services.rst
- src/main/java/de/tum/cit/aet/artemis/core/config/SecurityConfiguration.java
- docs/index.rst
- src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingSetupInfo.java
- src/main/java/de/tum/cit/aet/artemis/core/config/RestTemplateConfiguration.java
- docs/user/sharing.rst
- src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingException.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ProgrammingExerciseImportFromSharingService.java
- docs/admin/setup/sharing.rst
- src/main/java/de/tum/cit/aet/artemis/core/config/Constants.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingMultipartZipFile.java
- src/main/webapp/app/app.routes.ts
- src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingHealthIndicator.java
- src/main/java/de/tum/cit/aet/artemis/programming/web/ExerciseSharingResource.java
- src/main/java/de/tum/cit/aet/artemis/core/web/SharingSupportResource.java
- src/main/java/de/tum/cit/aet/artemis/core/dto/SharingInfoDTO.java
- src/main/java/de/tum/cit/aet/artemis/programming/service/ProgrammingExerciseImportFromFileService.java
🧰 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/programming/service/sharing/SharingConnectorService.javasrc/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java
🧠 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10593
File: src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java:56-84
Timestamp: 2025-04-17T11:21:12.067Z
Learning: In the Artemis codebase, extending ArrayList to implement a bounded list (as done in HealthStatusWithHistory) is considered an acceptable design pattern by the team.
🧬 Code Graph Analysis (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (4)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingMultipartZipFile.java (1)
Profile(18-91)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ProgrammingExerciseImportFromSharingService.java (1)
Service(21-87)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java (1)
Service(70-470)
🪛 ast-grep (0.31.1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java
[warning] 405-405: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 405-405: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Mend Security Check
🔇 Additional comments (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (1)
218-236: 🛠️ Refactor suggestionImprove null check in API key validation.
The method doesn't handle the case where
sharingApiKeyis null, which could lead to aNullPointerExceptionwhen comparing with the provided key.Add a null check for
sharingApiKey:public boolean validate(String apiKey) { if (apiKey == null || apiKey.length() > 200) { // this is just in case, somebody tries an attack lastHealthStati.add(new HealthStatus("Failed api Key validation")); return false; } Pattern p = Pattern.compile("Bearer\\s(.+)"); Matcher m = p.matcher(apiKey); if (m.matches()) { apiKey = m.group(1); } - boolean success = Objects.equals(sharingApiKey, apiKey); + boolean success = sharingApiKey != null && Objects.equals(sharingApiKey, apiKey); if (!success) { lastHealthStati.add(new HealthStatus("Failed api Key validation (unequal)")); } return success; }Likely an incorrect or invalid review comment.
| public SharingPluginConfig getPluginConfig(URL apiBaseUrl, Optional<String> installationName) { | ||
| this.sharingApiBaseUrl = apiBaseUrl; | ||
| try { | ||
| this.installationName = installationName.orElse(InetAddress.getLocalHost().getCanonicalHostName()); | ||
| } | ||
| catch (UnknownHostException e) { | ||
| this.installationName = UNKNOWN_INSTALLATIONAME; | ||
| } | ||
| SharingPluginConfig.Action action = new SharingPluginConfig.Action("Import", "/sharing/import", actionName, | ||
| "metadata.format.stream().anyMatch(entry->entry=='artemis' || entry=='Artemis').get()"); | ||
| lastHealthStati.add(new HealthStatus("Delivered Sharing Config Status to " + apiBaseUrl)); | ||
| lastHealthStati.setLastConnect(); | ||
| return new SharingPluginConfig("Artemis Sharing Connector", new SharingPluginConfig.Action[] { action }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Method modifies state while providing configuration.
This method both returns a configuration and modifies the service's state by setting sharingApiBaseUrl and installationName, which could lead to unexpected behavior in a concurrent environment.
Consider separating the side effects from the configuration retrieval:
public SharingPluginConfig getPluginConfig(URL apiBaseUrl, Optional<String> installationName) {
- this.sharingApiBaseUrl = apiBaseUrl;
- try {
- this.installationName = installationName.orElse(InetAddress.getLocalHost().getCanonicalHostName());
- }
- catch (UnknownHostException e) {
- this.installationName = UNKNOWN_INSTALLATIONAME;
- }
+ // Get installation name without modifying state
+ String resolvedInstallationName;
+ try {
+ resolvedInstallationName = installationName.orElse(InetAddress.getLocalHost().getCanonicalHostName());
+ }
+ catch (UnknownHostException e) {
+ resolvedInstallationName = UNKNOWN_INSTALLATION_NAME;
+ }
+
+ // Update state in a separate method
+ updateConnectionInfo(apiBaseUrl, resolvedInstallationName);
+
SharingPluginConfig.Action action = new SharingPluginConfig.Action("Import", "/sharing/import", actionName,
"metadata.format.stream().anyMatch(entry->entry=='artemis' || entry=='Artemis').get()");
lastHealthStati.add(new HealthStatus("Delivered Sharing Config Status to " + apiBaseUrl));
lastHealthStati.setLastConnect();
- return new SharingPluginConfig("Artemis Sharing Connector", new SharingPluginConfig.Action[] { action });
+ return new SharingPluginConfig("Artemis Sharing Connector", new SharingPluginConfig.Action[] { action });
}
+/**
+ * Updates the connection information.
+ *
+ * @param apiBaseUrl the base URL for the sharing API
+ * @param installationName the installation name
+ */
+private synchronized void updateConnectionInfo(URL apiBaseUrl, String installationName) {
+ this.sharingApiBaseUrl = apiBaseUrl;
+ this.installationName = installationName;
+}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public SharingPluginConfig getPluginConfig(URL apiBaseUrl, Optional<String> installationName) { | |
| this.sharingApiBaseUrl = apiBaseUrl; | |
| try { | |
| this.installationName = installationName.orElse(InetAddress.getLocalHost().getCanonicalHostName()); | |
| } | |
| catch (UnknownHostException e) { | |
| this.installationName = UNKNOWN_INSTALLATIONAME; | |
| } | |
| SharingPluginConfig.Action action = new SharingPluginConfig.Action("Import", "/sharing/import", actionName, | |
| "metadata.format.stream().anyMatch(entry->entry=='artemis' || entry=='Artemis').get()"); | |
| lastHealthStati.add(new HealthStatus("Delivered Sharing Config Status to " + apiBaseUrl)); | |
| lastHealthStati.setLastConnect(); | |
| return new SharingPluginConfig("Artemis Sharing Connector", new SharingPluginConfig.Action[] { action }); | |
| public SharingPluginConfig getPluginConfig(URL apiBaseUrl, Optional<String> installationName) { | |
| // Get installation name without modifying state | |
| String resolvedInstallationName; | |
| try { | |
| resolvedInstallationName = installationName.orElse(InetAddress.getLocalHost().getCanonicalHostName()); | |
| } catch (UnknownHostException e) { | |
| resolvedInstallationName = UNKNOWN_INSTALLATION_NAME; | |
| } | |
| // Update state in a separate method | |
| updateConnectionInfo(apiBaseUrl, resolvedInstallationName); | |
| SharingPluginConfig.Action action = new SharingPluginConfig.Action("Import", "/sharing/import", actionName, | |
| "metadata.format.stream().anyMatch(entry->entry=='artemis' || entry=='Artemis').get()"); | |
| lastHealthStati.add(new HealthStatus("Delivered Sharing Config Status to " + apiBaseUrl)); | |
| lastHealthStati.setLastConnect(); | |
| return new SharingPluginConfig("Artemis Sharing Connector", new SharingPluginConfig.Action[] { action }); | |
| } | |
| /** | |
| * Updates the connection information. | |
| * | |
| * @param apiBaseUrl the base URL for the sharing API | |
| * @param installationName the installation name | |
| */ | |
| private synchronized void updateConnectionInfo(URL apiBaseUrl, String installationName) { | |
| this.sharingApiBaseUrl = apiBaseUrl; | |
| this.installationName = installationName; | |
| } |
| private String createHMAC(String base64token) { | ||
| // selects HMAC-method (here HmacSHA256) | ||
| String algorithm = "HmacSHA256"; | ||
| String psk = sharingConnectorService.getSharingApiKeyOrNull(); | ||
|
|
||
| SecretKeySpec secretKeySpec = new SecretKeySpec(psk.getBytes(StandardCharsets.UTF_8), algorithm); | ||
|
|
||
| try { | ||
| Mac mac = Mac.getInstance(algorithm); | ||
| mac.init(secretKeySpec); | ||
| byte[] hmacBytes = mac.doFinal(base64token.getBytes(StandardCharsets.UTF_8)); | ||
|
|
||
| return Base64.getEncoder().encodeToString(hmacBytes); | ||
| } | ||
| catch (NoSuchAlgorithmException | InvalidKeyException e) { | ||
| return Base64.getEncoder().encodeToString(new byte[] {}); | ||
| } | ||
|
|
||
| } |
There was a problem hiding this comment.
Add null check for API key in HMAC creation.
The method doesn't handle the case where sharingApiKey is null, which could lead to a NullPointerException.
Add a null check:
private String createHMAC(String base64token) {
// selects HMAC-method (here HmacSHA256)
String algorithm = "HmacSHA256";
String psk = sharingConnectorService.getSharingApiKeyOrNull();
+
+ if (psk == null) {
+ log.warn("Cannot create HMAC: API key is null");
+ return Base64.getEncoder().encodeToString(new byte[] {});
+ }
SecretKeySpec secretKeySpec = new SecretKeySpec(psk.getBytes(StandardCharsets.UTF_8), algorithm);
try {
Mac mac = Mac.getInstance(algorithm);
mac.init(secretKeySpec);
byte[] hmacBytes = mac.doFinal(base64token.getBytes(StandardCharsets.UTF_8));
return Base64.getEncoder().encodeToString(hmacBytes);
}
catch (NoSuchAlgorithmException | InvalidKeyException e) {
+ log.warn("Error creating HMAC", e);
return Base64.getEncoder().encodeToString(new byte[] {});
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private String createHMAC(String base64token) { | |
| // selects HMAC-method (here HmacSHA256) | |
| String algorithm = "HmacSHA256"; | |
| String psk = sharingConnectorService.getSharingApiKeyOrNull(); | |
| SecretKeySpec secretKeySpec = new SecretKeySpec(psk.getBytes(StandardCharsets.UTF_8), algorithm); | |
| try { | |
| Mac mac = Mac.getInstance(algorithm); | |
| mac.init(secretKeySpec); | |
| byte[] hmacBytes = mac.doFinal(base64token.getBytes(StandardCharsets.UTF_8)); | |
| return Base64.getEncoder().encodeToString(hmacBytes); | |
| } | |
| catch (NoSuchAlgorithmException | InvalidKeyException e) { | |
| return Base64.getEncoder().encodeToString(new byte[] {}); | |
| } | |
| } | |
| private String createHMAC(String base64token) { | |
| // selects HMAC-method (here HmacSHA256) | |
| String algorithm = "HmacSHA256"; | |
| String psk = sharingConnectorService.getSharingApiKeyOrNull(); | |
| if (psk == null) { | |
| log.warn("Cannot create HMAC: API key is null"); | |
| return Base64.getEncoder().encodeToString(new byte[] {}); | |
| } | |
| SecretKeySpec secretKeySpec = new SecretKeySpec(psk.getBytes(StandardCharsets.UTF_8), algorithm); | |
| try { | |
| Mac mac = Mac.getInstance(algorithm); | |
| mac.init(secretKeySpec); | |
| byte[] hmacBytes = mac.doFinal(base64token.getBytes(StandardCharsets.UTF_8)); | |
| return Base64.getEncoder().encodeToString(hmacBytes); | |
| } | |
| catch (NoSuchAlgorithmException | InvalidKeyException e) { | |
| log.warn("Error creating HMAC", e); | |
| return Base64.getEncoder().encodeToString(new byte[] {}); | |
| } | |
| } |
🧰 Tools
🪛 ast-grep (0.31.1)
[warning] 405-405: Triple DES (3DES or DESede) is considered deprecated. AES is the recommended cipher. Upgrade to use AES.
Context: Mac.getInstance(algorithm)
Note: [CWE-326]: Inadequate Encryption Strength [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://find-sec-bugs.github.io/bugs.htm#TDES_USAGE
- https://csrc.nist.gov/News/2017/Update-to-Current-Use-and-Deprecation-of-TDEA
(desede-is-deprecated-java)
[warning] 405-405: Use of AES with ECB mode detected. ECB doesn't provide message confidentiality and is not semantically secure so should not be used. Instead, use a strong, secure cipher: Cipher.getInstance("AES/CBC/PKCS7PADDING"). See https://owasp.org/www-community/Using_the_Java_Cryptographic_Extensions for more information.
Context: Mac.getInstance(algorithm)
Note: [CWE-327]: Use of a Broken or Risky Cryptographic Algorithm [OWASP A03:2017]: Sensitive Data Exposure [OWASP A02:2021]: Cryptographic Failures [REFERENCES]
- https://owasp.org/Top10/A02_2021-Cryptographic_Failures
- https://googleprojectzero.blogspot.com/2022/10/rc4-is-still-considered-harmful.html
(use-of-aes-ecb-java)
| public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException { | ||
| InputStream repositoryStream; | ||
| try { | ||
| Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo); | ||
| if (cachedBasketItem.isEmpty()) { | ||
| return Optional.empty(); | ||
| } | ||
| repositoryStream = cachedBasketItem.get().getInputStream(); | ||
| } | ||
| catch (IOException | SharingException e) { | ||
| log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e); | ||
| return Optional.empty(); | ||
| } | ||
|
|
||
| try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream)) { | ||
|
|
||
| ZipEntry entry; | ||
| while ((entry = zippedRepositoryStream.getNextEntry()) != null) { | ||
| Matcher matcher = matchingPattern.matcher(entry.getName()); | ||
| if (matcher.find()) { | ||
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | ||
| byte[] buffer = new byte[102400]; | ||
| int bytesRead; | ||
| while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) { | ||
| baos.write(buffer, 0, bytesRead); | ||
| } | ||
| String entryContent = baos.toString(StandardCharsets.UTF_8); | ||
| baos.close(); | ||
| zippedRepositoryStream.closeEntry(); | ||
| return Optional.of(entryContent); | ||
| } | ||
| zippedRepositoryStream.closeEntry(); | ||
| } | ||
| return Optional.empty(); // Not found | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize ZipInputStream resource handling.
The method calls closeEntry() redundantly for each entry, and it doesn't close the ByteArrayOutputStream in all code paths.
Improve the resource handling:
public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException {
InputStream repositoryStream;
try {
Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo);
if (cachedBasketItem.isEmpty()) {
return Optional.empty();
}
repositoryStream = cachedBasketItem.get().getInputStream();
}
catch (IOException | SharingException e) {
log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e);
return Optional.empty();
}
try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream)) {
ZipEntry entry;
while ((entry = zippedRepositoryStream.getNextEntry()) != null) {
Matcher matcher = matchingPattern.matcher(entry.getName());
if (matcher.find()) {
- ByteArrayOutputStream baos = new ByteArrayOutputStream();
+ try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) {
byte[] buffer = new byte[102400];
int bytesRead;
while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) {
baos.write(buffer, 0, bytesRead);
}
String entryContent = baos.toString(StandardCharsets.UTF_8);
- baos.close();
zippedRepositoryStream.closeEntry();
return Optional.of(entryContent);
+ }
}
zippedRepositoryStream.closeEntry();
}
return Optional.empty(); // Not found
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException { | |
| InputStream repositoryStream; | |
| try { | |
| Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo); | |
| if (cachedBasketItem.isEmpty()) { | |
| return Optional.empty(); | |
| } | |
| repositoryStream = cachedBasketItem.get().getInputStream(); | |
| } | |
| catch (IOException | SharingException e) { | |
| log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e); | |
| return Optional.empty(); | |
| } | |
| try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream)) { | |
| ZipEntry entry; | |
| while ((entry = zippedRepositoryStream.getNextEntry()) != null) { | |
| Matcher matcher = matchingPattern.matcher(entry.getName()); | |
| if (matcher.find()) { | |
| ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| byte[] buffer = new byte[102400]; | |
| int bytesRead; | |
| while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) { | |
| baos.write(buffer, 0, bytesRead); | |
| } | |
| String entryContent = baos.toString(StandardCharsets.UTF_8); | |
| baos.close(); | |
| zippedRepositoryStream.closeEntry(); | |
| return Optional.of(entryContent); | |
| } | |
| zippedRepositoryStream.closeEntry(); | |
| } | |
| return Optional.empty(); // Not found | |
| } | |
| public Optional<String> getEntryFromBasket(Pattern matchingPattern, SharingInfoDTO sharingInfo) throws IOException { | |
| InputStream repositoryStream; | |
| try { | |
| Optional<SharingMultipartZipFile> cachedBasketItem = this.getCachedBasketItem(sharingInfo); | |
| if (cachedBasketItem.isEmpty()) { | |
| return Optional.empty(); | |
| } | |
| repositoryStream = cachedBasketItem.get().getInputStream(); | |
| } catch (IOException | SharingException e) { | |
| log.error("Cannot read input Template for {}", sharingInfo.getBasketToken(), e); | |
| return Optional.empty(); | |
| } | |
| try (ZipInputStream zippedRepositoryStream = new ZipInputStream(repositoryStream)) { | |
| ZipEntry entry; | |
| while ((entry = zippedRepositoryStream.getNextEntry()) != null) { | |
| Matcher matcher = matchingPattern.matcher(entry.getName()); | |
| if (matcher.find()) { | |
| - ByteArrayOutputStream baos = new ByteArrayOutputStream(); | |
| + try (ByteArrayOutputStream baos = new ByteArrayOutputStream()) { | |
| byte[] buffer = new byte[102400]; | |
| int bytesRead; | |
| while ((bytesRead = zippedRepositoryStream.read(buffer)) != -1) { | |
| baos.write(buffer, 0, bytesRead); | |
| } | |
| String entryContent = baos.toString(StandardCharsets.UTF_8); | |
| - baos.close(); | |
| zippedRepositoryStream.closeEntry(); | |
| return Optional.of(entryContent); | |
| + } | |
| } | |
| zippedRepositoryStream.closeEntry(); | |
| } | |
| return Optional.empty(); // Not found | |
| } | |
| } |
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (2)
77-83: Unsynchronised access tolastConnectremainsPrevious reviews already pointed out that
getLastConnect()/setLastConnect()are not thread-safe whileadd()is. The race condition is still present in the current revision.
Consider either marking the fieldvolatileor synchronising both getters and setters.
281-283: Live list is leaked → callers can mutate internal state
getLastHealthStati()exposes the mutableArrayList, so external code can add/remove entries without the class knowing, breaking the ring-buffer guarantee and thread-safety assumptions.
Return an unmodifiable copy or wrap the list defensively.(Identical concern raised in an earlier review.)
🧹 Nitpick comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (3)
37-38: Typo in constant name can be confusing and propagate to other code
UNKNOWN_INSTALLATIONAMEmisses an “N” in “NAME”. Even though it’s only internal, typos in public constants tend to spread across the codebase and tests.-public static final String UNKNOWN_INSTALLATIONAME = "unknown installationame"; +public static final String UNKNOWN_INSTALLATION_NAME = "unknown installation name";(Remember to adjust the two usages at lines 200-204 accordingly.)
Also applies to: 200-204
225-229: Pre-compile regex to avoid repeated parsing
Pattern.compile("Bearer\\s(.+)")runs on every validation call. Compiling it once as aprivate static final Pattern BEARER_PATTERNis cheaper and threadsafe.-Pattern p = Pattern.compile("Bearer\\s(.+)"); -Matcher m = p.matcher(apiKey); +Matcher m = BEARER_PATTERN.matcher(apiKey);
242-251: Manual thread management can be replaced with Spring async facilitiesCreating an ad-hoc
ExecutorServicefor a one-shot task adds boilerplate and risks silent failures. Using@Async(with thetaskExecutoralready provided by Spring Boot) or scheduling the call viaApplicationRunnerkeeps the code simpler and leverages Spring’s lifecycle handling.No functional bug, but worth considering for maintainability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.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/programming/service/sharing/SharingConnectorService.java
🧠 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10593
File: src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java:56-84
Timestamp: 2025-04-17T11:21:12.067Z
Learning: In the Artemis codebase, extending ArrayList to implement a bounded list (as done in HealthStatusWithHistory) is considered an acceptable design pattern by the team.
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (3)
61-89: Thread safety concerns withgetLastConnect()andsetLastConnect().While
add()is synchronized, the methods for accessing and modifyinglastConnectare not, creating potential race conditions.Based on previous discussions, extending ArrayList for a bounded list is an acceptable pattern in this codebase, but thread-safety should still be addressed for all state-modifying operations.
197-210: Method combines state mutation with configuration retrieval.This method both returns configuration and modifies service state by setting
sharingApiBaseUrlandinstallationName, which could lead to unexpected behavior in concurrent environments.Consider separating the configuration retrieval from the state update as suggested in previous comments.
281-283: Avoid exposing internal mutable state to callers.The
getLastHealthStati()method returns the live list, allowing external code to mutate it or iterate without synchronization.Consider returning an unmodifiable copy of the list to prevent accidental mutation and potential thread-safety issues, as suggested in previous comments.
🧹 Nitpick comments (6)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (6)
37-37: Fix typo in constant name.The constant name has a typo: "UNKNOWN_INSTALLATIONAME" should be "UNKNOWN_INSTALLATION_NAME" (missing "N").
-public static final String UNKNOWN_INSTALLATIONAME = "unknown installationame"; +public static final String UNKNOWN_INSTALLATION_NAME = "unknown installation name";
111-111: Consider setting a default empty string forsharingApiKey.For security fields like API keys, providing a non-null default value can help prevent NullPointerExceptions and make validation more robust.
-@Value("${artemis.sharing.apikey:#{null}}") +@Value("${artemis.sharing.apikey:}")
203-203: Update error fallback to use corrected constant name.Once the constant name is fixed as suggested earlier, update this reference too.
- this.installationName = UNKNOWN_INSTALLATIONAME; + this.installationName = UNKNOWN_INSTALLATION_NAME;
218-236: Follow consistent pattern for null/empty checks.The method does explicit null checking but could benefit from using a utility method like
StringUtils.isEmpty()or Java's built-inObjects.isNull(). Also, it would be more appropriate to return early when validation fails.public boolean validate(String apiKey) { - if (apiKey == null || apiKey.length() > 200) { + if (apiKey == null) { + lastHealthStati.add(new HealthStatus("Failed api Key validation: null key")); + return false; + } + + if (apiKey.length() > 200) { // this is just in case, somebody tries an attack - lastHealthStati.add(new HealthStatus("Failed api Key validation")); - + lastHealthStati.add(new HealthStatus("Failed api Key validation: key exceeds maximum length")); return false; } Pattern p = Pattern.compile("Bearer\\s(.+)"); Matcher m = p.matcher(apiKey); if (m.matches()) { apiKey = m.group(1); } boolean success = Objects.equals(sharingApiKey, apiKey); if (!success) { lastHealthStati.add(new HealthStatus("Failed api Key validation (unequal)")); } return success; }
242-251: Use a more structured approach for handling async tasks.Consider using Spring's
TaskExecutorinstead of directly usingExecutorsfor better integration with Spring's lifecycle and application context.-@EventListener(ApplicationReadyEvent.class) -public void triggerSharingReinitAfterApplicationStart() { - ExecutorService executor = Executors.newSingleThreadExecutor(); - - executor.submit(() -> { - triggerReinit(); - }); - - executor.shutdown(); -} +@Autowired +private TaskExecutor taskExecutor; + +@EventListener(ApplicationReadyEvent.class) +public void triggerSharingReinitAfterApplicationStart() { + taskExecutor.execute(this::triggerReinit); +}
274-277: Improve exception handling with more detailed logging.When an exception occurs, log both the exception message and exception type. Also, update the health status to reflect the failure.
} catch (Exception e) { - log.info("Failed to request reinitialization from Sharing Platform: {}", e.getMessage()); + log.warn("Failed to request reinitialization from Sharing Platform: {} - {}", e.getClass().getSimpleName(), e.getMessage()); + lastHealthStati.add(new HealthStatus("Failed to reinitialize Sharing Platform: " + e.getMessage())); }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
src/main/resources/config/application-sharing.ymlis excluded by!**/*.yml
📒 Files selected for processing (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.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/programming/service/sharing/SharingConnectorService.java
🧠 Learnings (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (1)
Learnt from: Wallenstein61
PR: ls1intum/Artemis#10593
File: src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java:56-84
Timestamp: 2025-04-17T11:21:12.067Z
Learning: In the Artemis codebase, extending ArrayList to implement a bounded list (as done in HealthStatusWithHistory) is considered an acceptable design pattern by the team.
🧬 Code Graph Analysis (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java (3)
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingMultipartZipFile.java (1)
Profile(18-91)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingException.java (1)
Profile(10-42)src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/ExerciseSharingService.java (1)
Service(70-470)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Codacy Static Code Analysis
| String reInitUrlWithApiKey = UriComponentsBuilder.fromUriString(sharingUrl).pathSegment("api", "pluginIF", "v0.1", "reInitialize").queryParam("apiKey", sharingApiKey) | ||
| .encode().toUriString(); | ||
| try { | ||
| restTemplate.getForObject(reInitUrlWithApiKey, Boolean.class); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for HTTP request.
The restTemplate.getForObject() call should handle specific HTTP-related exceptions and consider using a timeout to avoid blocking in case of network issues.
- String reInitUrlWithApiKey = UriComponentsBuilder.fromUriString(sharingUrl).pathSegment("api", "pluginIF", "v0.1", "reInitialize").queryParam("apiKey", sharingApiKey)
- .encode().toUriString();
- try {
- restTemplate.getForObject(reInitUrlWithApiKey, Boolean.class);
+ String reInitUrlWithApiKey = UriComponentsBuilder.fromUriString(sharingUrl)
+ .pathSegment("api", "pluginIF", "v0.1", "reInitialize")
+ .queryParam("apiKey", sharingApiKey)
+ .encode().toUriString();
+ try {
+ Boolean result = restTemplate.getForObject(reInitUrlWithApiKey, Boolean.class);
+ if (Boolean.TRUE.equals(result)) {
+ lastHealthStati.add(new HealthStatus("Successfully reinitialized Sharing Platform"));
+ } else {
+ lastHealthStati.add(new HealthStatus("Sharing Platform reinitialization returned false"));
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| String reInitUrlWithApiKey = UriComponentsBuilder.fromUriString(sharingUrl).pathSegment("api", "pluginIF", "v0.1", "reInitialize").queryParam("apiKey", sharingApiKey) | |
| .encode().toUriString(); | |
| try { | |
| restTemplate.getForObject(reInitUrlWithApiKey, Boolean.class); | |
| String reInitUrlWithApiKey = UriComponentsBuilder.fromUriString(sharingUrl) | |
| .pathSegment("api", "pluginIF", "v0.1", "reInitialize") | |
| .queryParam("apiKey", sharingApiKey) | |
| .encode().toUriString(); | |
| try { | |
| Boolean result = restTemplate.getForObject(reInitUrlWithApiKey, Boolean.class); | |
| if (Boolean.TRUE.equals(result)) { | |
| lastHealthStati.add(new HealthStatus("Successfully reinitialized Sharing Platform")); | |
| } else { | |
| lastHealthStati.add(new HealthStatus("Sharing Platform reinitialization returned false")); | |
| } |
🤖 Prompt for AI Agents (early access)
In
src/main/java/de/tum/cit/aet/artemis/programming/service/sharing/SharingConnectorService.java
around lines 270 to 273, the restTemplate.getForObject() call lacks specific
HTTP exception handling and does not use a timeout. Modify the code to catch
specific exceptions such as HttpClientErrorException and ResourceAccessException
to handle HTTP errors and network issues gracefully. Additionally, configure the
RestTemplate with appropriate connection and read timeouts to prevent blocking
during network delays.
|
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. |
This pull resolves #9909
Checklist
General
Server
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Changes affecting Programming Exercises
Motivation and Context
The CodeAbility Project currently operates and maintains a platform for sharing (among other content) programming exercises between interested parties. It is open to the programming teaching community.
CodeAbility also operates an Artemis instance that serves its 6 Austrian partners as a platform for programming teaching.
Artemis is currently a major source of programming exercises, as well as a potential target for re-using programming exercises.
However the exchange between separated instances of Artemis is only supported by a manual exchange of zipped exercises files.
We have implemented a REST-based connector interface in the sharing platform to import and export exercises between interested applications.
This connector interface allows among other features the import and export of programming exercises in an appropriate format.
We have already implemented an prototype integration of Artemis with the sharing platform based on the connector interface in order to allow for the smooth sharing of exercises.
Description
An additional spring profile "sharing" is implemented, that enables the functionality to export/import programming exercises via the sharing plattform.
The sharing requires an registration of the artemis instance at artemis-support-informatik@uibk.ac.at. During the registration a pre shared key is exchanged, that allows the communication between artemis and the sharing plattform.
When the sharing profile is enabled, (and configuration is initialized properly), the the sharing platform connects regularly with the artemis instance to query the connector configuration.
Additional the artemis instance can trigger an configuration request for the sharing plattform at application startup.
Details can be found at https://sharing-codeability.uibk.ac.at/development/sharing/codeability-sharing-platform/-/wikis/Artemis-Connection/Connector-Interface .
A preliminary setup guide can be found at https://development.pages.sharing-codeability.uibk.ac.at/artemis/artemis/admin/setup/sharing.html .
Steps for Testing
Prerequisites:
Follow the steps at https://development.pages.sharing-codeability.uibk.ac.at/artemis/artemis/user/sharing.html for export to Artemis and the import back to artemis.
Exam Mode Testing
This feature is not relevant for exam mode testing.
Testserver States
TODO: In progress.
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
Remark: Import and Export of programming exercises does not influence any performance with student features.
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
TODO: work in progress
Screenshots
Summary by CodeRabbit
New Features
Documentation
Localization
Bug Fixes
Tests
Chores