Feature/ssh/show ssh fingerprints 2 #9958
Conversation
github-actions
bot
added
tests
server
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 pmd (7.7.0)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshFingerprintsProviderService.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration. src/main/java/de/tum/cit/aet/artemis/programming/web/localvc/ssh/SshFingerprintsProviderResource.javaThe following rules are missing or misspelled in your ruleset file category/vm/bestpractices.xml: BooleanInstantiation, DontImportJavaLang, DuplicateImports, EmptyFinallyBlock, EmptyIfStmt, EmptyInitializer, EmptyStatementBlock, EmptyStatementNotInLoop, EmptySwitchStatements, EmptySynchronizedBlock, EmptyTryBlock, EmptyWhileStmt, ExcessiveClassLength, ExcessiveMethodLength, ImportFromSamePackage, MissingBreakInSwitch, SimplifyBooleanAssertion. Please check your ruleset configuration.
WalkthroughThe changes in this pull request introduce new functionalities for managing SSH fingerprints within the Artemis application. A new Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (12)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.java (1)
Line range hint
8-8: Prevent instantiation of utility classAdd a private constructor to prevent instantiation of this utility class.
public class HashUtils { + private HashUtils() { + throw new IllegalStateException("Utility class"); + } public static String getSha512Fingerprint(PublicKey key) {src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.ts (1)
14-14: Consider initializing sshFingerprints as empty objectInitialize
sshFingerprintsas an empty object to avoid potential undefined checks in the template.- protected sshFingerprints?: { [key: string]: string }; + protected sshFingerprints: { [key: string]: string } = {};src/main/java/de/tum/cit/aet/artemis/programming/web/localvc/ssh/SshFingerprintsProviderResource.java (3)
19-21: Improve JavaDoc documentationThe current JavaDoc comment is incomplete. It should describe what this controller manages.
/** - * REST controller for managing. + * REST controller for managing SSH fingerprint operations. */
27-31: Add final modifier to service fieldThe service field should be marked as final since it's initialized through constructor injection and never changed.
- SshFingerprintsProviderService sshFingerprintsProviderService; + private final SshFingerprintsProviderService sshFingerprintsProviderService;
33-43: Enhance endpoint documentation and consider cachingThe endpoint documentation should include response status codes and consider caching for performance optimization.
/** * GET /ssh-fingerprints * * @return the SSH fingerprints for the keys a user uses + * @response 200 Successfully retrieved SSH fingerprints + * @response 400 Failed to load SSH keys */ @GetMapping(value = "ssh-fingerprints", produces = MediaType.APPLICATION_JSON_VALUE) @EnforceAtLeastStudent @FeatureToggle(Feature.Exports) + @ResponseStatus(HttpStatus.OK) + @Cacheable(value = "ssh-fingerprints", condition = "#result != null") public ResponseEntity<Map<String, String>> getSshFingerprints() {src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshFingerprintsProviderService.java (1)
26-32: Consider adding monitoring metricsFor better operational visibility, consider adding metrics to track SSH key loading performance and errors.
+ private final MeterRegistry meterRegistry; private final SshServer sshServer; - public SshFingerprintsProviderService(SshServer sshServer) { + public SshFingerprintsProviderService(SshServer sshServer, MeterRegistry meterRegistry) { this.sshServer = sshServer; + this.meterRegistry = meterRegistry; + // Register metrics + meterRegistry.gauge("ssh.keys.total", fingerprints.size()); }src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.html (3)
37-37: Fix inconsistent stylingSome rows use the 'small-text' class while others don't. Maintain consistent styling across all fingerprint rows.
- <div class="row"> + <div class="row small-text">Also applies to: 48-48
3-67: Add loading and error statesThe template should handle loading and error states to improve user experience.
<div class="list-group d-block"> + @if (loading) { + <div class="list-group-item"> + <div class="d-flex justify-content-center"> + <div class="spinner-border" role="status"> + <span class="visually-hidden">Loading...</span> + </div> + </div> + </div> + } + @if (error) { + <div class="list-group-item text-danger"> + <span jhiTranslate="artemisApp.userSettings.sshSettingsPage.loadError"></span> + </div> + } <!-- Rest of the template -->
15-21: Improve accessibilityAdd ARIA labels to improve accessibility for screen readers.
- <div class="column left"> + <div class="column left" role="rowheader" aria-label="SSH key type"> - <div class="column right"> + <div class="column right" role="cell" aria-label="SSH fingerprint value">Also applies to: 27-32, 38-43, 49-54
src/test/java/de/tum/cit/aet/artemis/programming/icl/SshFingerprintsProviderIntegrationTest.java (1)
40-42: Consider future-proofing the RSA key sizeWhile 2048 bits is currently secure, consider using 4096 bits for better future-proofing. This aligns with NIST recommendations for longer-term security.
- keyPairGenerator.initialize(2048); + keyPairGenerator.initialize(4096);src/main/webapp/app/shared/user-settings/user-settings.route.ts (1)
64-70: Consider using a more specific page title for fingerprints viewThe current page title reuses the generic SSH settings title. Consider using a more specific title for better user navigation and accessibility.
- pageTitle: 'artemisApp.userSettings.categories.SSH_SETTINGS', + pageTitle: 'artemisApp.userSettings.categories.SSH_FINGERPRINTS',src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html (1)
Line range hint
30-36: Add OS-specific instructions for SSH key usageBased on previous feedback, we should include instructions for different operating systems to improve user experience.
<div class="d-flex flex-wrap"> <p> <span class="mt-4 font-medium" jhiTranslate="artemisApp.userSettings.sshSettingsPage.whatToUseSSHForInfo"> </span> <jhi-documentation-link [documentationType]="documentationType" [displayString]="'artemisApp.userSettings.sshSettingsPage.learnMore'"> </jhi-documentation-link> + <div class="mt-2"> + <h5 jhiTranslate="artemisApp.userSettings.sshSettingsPage.osInstructions.title"></h5> + <ul> + <li jhiTranslate="artemisApp.userSettings.sshSettingsPage.osInstructions.linux"></li> + <li jhiTranslate="artemisApp.userSettings.sshSettingsPage.osInstructions.macos"></li> + <li jhiTranslate="artemisApp.userSettings.sshSettingsPage.osInstructions.windows"></li> + </ul> + </div> </p> </div>
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (17)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshFingerprintsProviderService.java(1 hunks)src/main/java/de/tum/cit/aet/artemis/programming/web/localvc/ssh/SshFingerprintsProviderResource.java(1 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.html(1 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.scss(1 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.ts(1 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.service.ts(1 hunks)src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html(2 hunks)src/main/webapp/app/shared/user-settings/user-settings.module.ts(2 hunks)src/main/webapp/app/shared/user-settings/user-settings.route.ts(2 hunks)src/main/webapp/i18n/de/userSettings.json(1 hunks)src/main/webapp/i18n/en/userSettings.json(1 hunks)src/test/java/de/tum/cit/aet/artemis/programming/icl/SshFingerprintsProviderIntegrationTest.java(1 hunks)src/test/javascript/spec/component/account/ssh/ssh-user-settings-fingerprints.component.spec.ts(1 hunks)src/test/javascript/spec/component/account/ssh/ssh-user-settings-key-details.component.spec.ts(1 hunks)src/test/javascript/spec/component/account/ssh/ssh-user-settings.component.spec.ts(1 hunks)src/test/javascript/spec/service/ssh-user-settings-fingerprints.service.spec.ts(1 hunks)
✅ Files skipped from review due to trivial changes (3)
- src/test/javascript/spec/component/account/ssh/ssh-user-settings.component.spec.ts
- src/test/javascript/spec/component/account/ssh/ssh-user-settings-key-details.component.spec.ts
- src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.scss
🧰 Additional context used
📓 Path-based instructions (13)
src/main/webapp/app/shared/user-settings/user-settings.module.ts (1)
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html (1)
Pattern 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/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.html (1)
Pattern 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/shared/user-settings/user-settings.route.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/HashUtils.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/main/webapp/i18n/de/userSettings.json (1)
Pattern 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/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.component.ts (1)
src/main/java/de/tum/cit/aet/artemis/programming/web/localvc/ssh/SshFingerprintsProviderResource.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
src/test/javascript/spec/component/account/ssh/ssh-user-settings-fingerprints.component.spec.ts (1)
Pattern 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/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.service.ts (1)
src/test/java/de/tum/cit/aet/artemis/programming/icl/SshFingerprintsProviderIntegrationTest.java (1)
Pattern src/test/java/**/*.java: test_naming: descriptive; test_size: small_specific; fixed_data: true; junit5_features: true; assert_use: assertThat; assert_specificity: true; archunit_use: enforce_package_rules; db_query_count_tests: track_performance; util_service_factory_pattern: true; avoid_db_access: true; mock_strategy: static_mocks; context_restart_minimize: true
src/test/javascript/spec/service/ssh-user-settings-fingerprints.service.spec.ts (1)
Pattern 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/main/java/de/tum/cit/aet/artemis/programming/service/localvc/ssh/SshFingerprintsProviderService.java (1)
Pattern src/main/java/**/*.java: naming:CamelCase; principles:{single_responsibility,small_methods,no_duplication}; db:{perf_queries,datetime_not_timestamp}; rest:{stateless,singleton,delegate_logic,http_only,minimal_dtos}; dtos:{java_records,no_entities,min_data,single_resp}; di:constructor_injection; kiss:simple_code; file_handling:os_indep_paths; practices:{least_access,avoid_transactions,code_reuse,static_member_ref,prefer_primitives}; sql:{param_annotation,uppercase,avoid_subqueries};java:avoid_star_imports
📓 Learnings (1)
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html (1)
Learnt from: Hialus
PR: ls1intum/Artemis#9454
File: src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html:114-119
Timestamp: 2024-11-12T12:51:58.050Z
Learning: In `src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html`, when providing commands for copying SSH public keys, always include instructions for macOS and Windows users in addition to Linux.
🔇 Additional comments (5)
src/main/webapp/app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.service.ts (1)
1-3: LGTM: Proper imports and modern DI pattern
The service follows Angular's modern dependency injection pattern using the inject function and has clean imports.
src/main/webapp/app/shared/user-settings/user-settings.module.ts (1)
16-16: LGTM! Module changes follow Angular guidelines
The import and declaration of SshUserSettingsFingerprintsComponent are properly implemented following Angular's style guide.
Also applies to: 26-26
src/main/webapp/app/shared/user-settings/ssh-settings/ssh-user-settings.component.html (1)
106-110: LGTM! Clean implementation of fingerprints link
The fingerprints link is well-positioned and follows the existing styling patterns.
src/main/webapp/i18n/en/userSettings.json (1)
58-61: LGTM! Clear and well-structured translations.
The new SSH fingerprint-related translations are clear, informative, and maintain consistency with the existing translations. The explanatory text effectively communicates the purpose and importance of SSH fingerprints to users.
src/main/webapp/i18n/de/userSettings.json (1)
58-61: LGTM! Well-translated with correct informal tone.
The German translations are accurate, maintain consistency with the English version, and correctly use the informal "du" form as required by the coding guidelines. The technical explanations are clear and natural-sounding in German.
|
|
||
| @Injectable({ providedIn: 'root' }) | ||
| export class SshUserSettingsFingerprintsService { | ||
| error?: string; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling for HTTP requests
The service currently has an error property but doesn't utilize it. Consider implementing proper error handling for the HTTP request.
public async getSshFingerprints(): Promise<{ [key: string]: string }> {
- return await firstValueFrom(this.http.get<{ [key: string]: string }>('api/ssh-fingerprints'));
+ try {
+ return await firstValueFrom(this.http.get<{ [key: string]: string }>('api/ssh-fingerprints'));
+ } catch (err) {
+ this.error = err instanceof Error ? err.message : 'Failed to fetch SSH fingerprints';
+ throw err;
+ }
}Also applies to: 11-13
| public static String getSha256Fingerprint(PublicKey key) { | ||
| return KeyUtils.getFingerPrint(BuiltinDigests.sha256.create(), key); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation and documentation
The method should include parameter validation and proper JavaDoc documentation.
+ /**
+ * Generates a SHA-256 fingerprint for the given public key.
+ *
+ * @param key the public key to generate the fingerprint for
+ * @return the SHA-256 fingerprint as a string
+ * @throws IllegalArgumentException if the key is null
+ */
public static String getSha256Fingerprint(PublicKey key) {
+ if (key == null) {
+ throw new IllegalArgumentException("Public key cannot be null");
+ }
return KeyUtils.getFingerPrint(BuiltinDigests.sha256.create(), key);
}📝 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 static String getSha256Fingerprint(PublicKey key) { | |
| return KeyUtils.getFingerPrint(BuiltinDigests.sha256.create(), key); | |
| } | |
| /** | |
| * Generates a SHA-256 fingerprint for the given public key. | |
| * | |
| * @param key the public key to generate the fingerprint for | |
| * @return the SHA-256 fingerprint as a string | |
| * @throws IllegalArgumentException if the key is null | |
| */ | |
| public static String getSha256Fingerprint(PublicKey key) { | |
| if (key == null) { | |
| throw new IllegalArgumentException("Public key cannot be null"); | |
| } | |
| return KeyUtils.getFingerPrint(BuiltinDigests.sha256.create(), key); | |
| } |
| import { SshUserSettingsFingerprintsService } from 'app/shared/user-settings/ssh-settings/fingerprints/ssh-user-settings-fingerprints.service'; | ||
|
|
||
| @Component({ | ||
| selector: 'jhi-account-information', |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update component selector to match component name
The selector jhi-account-information doesn't match the component's purpose. Consider updating it to jhi-ssh-user-settings-fingerprints for consistency.
- selector: 'jhi-account-information',
+ selector: 'jhi-ssh-user-settings-fingerprints',📝 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.
| selector: 'jhi-account-information', | |
| selector: 'jhi-ssh-user-settings-fingerprints', |
| async ngOnInit() { | ||
| this.sshFingerprints = await this.sshUserSettingsService.getSshFingerprints(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add loading state and error handling
The component should handle loading states and potential errors from the service.
+ protected loading = false;
+ protected error?: string;
+
async ngOnInit() {
+ this.loading = true;
+ try {
this.sshFingerprints = await this.sshUserSettingsService.getSshFingerprints();
+ } catch (err) {
+ this.error = this.sshUserSettingsService.error;
+ } finally {
+ this.loading = 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.
| async ngOnInit() { | |
| this.sshFingerprints = await this.sshUserSettingsService.getSshFingerprints(); | |
| } | |
| protected loading = false; | |
| protected error?: string; | |
| async ngOnInit() { | |
| this.loading = true; | |
| try { | |
| this.sshFingerprints = await this.sshUserSettingsService.getSshFingerprints(); | |
| } catch (err) { | |
| this.error = this.sshUserSettingsService.error; | |
| } finally { | |
| this.loading = false; | |
| } | |
| } |
| it('should get SSH fingerprints', fakeAsync(() => { | ||
| sshFingerprintsService.getSshFingerprints(); | ||
| httpMock.expectOne({ method: 'GET', url: getUserUrl }); | ||
| })); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance test coverage with response handling and error scenarios
The current test only verifies that the HTTP call is made but doesn't test:
- Successful response handling
- Error scenarios
- Response type verification
Consider expanding the test suite with these scenarios:
it('should get SSH fingerprints', fakeAsync(() => {
+ const mockResponse = {
+ 'ssh-rsa': 'SHA256:abc123...',
+ 'ssh-ed25519': 'SHA256:xyz789...'
+ };
sshFingerprintsService.getSshFingerprints();
- httpMock.expectOne({ method: 'GET', url: getUserUrl });
+ const req = httpMock.expectOne({ method: 'GET', url: getUserUrl });
+ expect(req.request.responseType).toBe('json');
+ req.flush(mockResponse);
}));
+
+it('should handle error response', fakeAsync(() => {
+ sshFingerprintsService.getSshFingerprints().subscribe({
+ error: (error) => {
+ expect(error.status).toBe(404);
+ }
+ });
+ const req = httpMock.expectOne({ method: 'GET', url: getUserUrl });
+ req.flush('Not Found', { status: 404, statusText: 'Not Found' });
+}));Committable suggestion skipped: line range outside the PR's diff.
| public Map<String, String> getSshFingerPrints() { | ||
| Map<String, String> fingerprints = new HashMap<>(); | ||
| KeyPairProvider keyPairProvider = sshServer.getKeyPairProvider(); | ||
| if (keyPairProvider != null) { | ||
| try { | ||
| keyPairProvider.loadKeys(null).iterator() | ||
| .forEachRemaining(keyPair -> fingerprints.put(keyPair.getPublic().getAlgorithm(), HashUtils.getSha256Fingerprint(keyPair.getPublic()))); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add null checks and validate fingerprint format
The current implementation lacks proper null checks and fingerprint format validation.
public Map<String, String> getSshFingerPrints() {
Map<String, String> fingerprints = new HashMap<>();
KeyPairProvider keyPairProvider = sshServer.getKeyPairProvider();
- if (keyPairProvider != null) {
+ if (keyPairProvider == null) {
+ log.warn("No KeyPairProvider configured in SSH server");
+ return fingerprints;
+ }
+
+ try {
+ var keys = keyPairProvider.loadKeys(null);
+ if (keys == null) {
+ log.warn("No SSH keys loaded from provider");
+ return fingerprints;
+ }
+
+ keys.iterator().forEachRemaining(keyPair -> {
+ if (keyPair != null && keyPair.getPublic() != null) {
+ var fingerprint = HashUtils.getSha256Fingerprint(keyPair.getPublic());
+ if (fingerprint != null && fingerprint.startsWith("SHA256:")) {
+ fingerprints.put(keyPair.getPublic().getAlgorithm(), fingerprint);
+ } else {
+ log.warn("Invalid fingerprint format for key algorithm: {}",
+ keyPair.getPublic().getAlgorithm());
+ }
+ }
+ });📝 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 Map<String, String> getSshFingerPrints() { | |
| Map<String, String> fingerprints = new HashMap<>(); | |
| KeyPairProvider keyPairProvider = sshServer.getKeyPairProvider(); | |
| if (keyPairProvider != null) { | |
| try { | |
| keyPairProvider.loadKeys(null).iterator() | |
| .forEachRemaining(keyPair -> fingerprints.put(keyPair.getPublic().getAlgorithm(), HashUtils.getSha256Fingerprint(keyPair.getPublic()))); | |
| public Map<String, String> getSshFingerPrints() { | |
| Map<String, String> fingerprints = new HashMap<>(); | |
| KeyPairProvider keyPairProvider = sshServer.getKeyPairProvider(); | |
| if (keyPairProvider == null) { | |
| log.warn("No KeyPairProvider configured in SSH server"); | |
| return fingerprints; | |
| } | |
| try { | |
| var keys = keyPairProvider.loadKeys(null); | |
| if (keys == null) { | |
| log.warn("No SSH keys loaded from provider"); | |
| return fingerprints; | |
| } | |
| keys.iterator().forEachRemaining(keyPair -> { | |
| if (keyPair != null && keyPair.getPublic() != null) { | |
| var fingerprint = HashUtils.getSha256Fingerprint(keyPair.getPublic()); | |
| if (fingerprint != null && fingerprint.startsWith("SHA256:")) { | |
| fingerprints.put(keyPair.getPublic().getAlgorithm(), fingerprint); | |
| } else { | |
| log.warn("Invalid fingerprint format for key algorithm: {}", | |
| keyPair.getPublic().getAlgorithm()); | |
| } | |
| } | |
| }); |
| it('should display fingerprints', async () => { | ||
| await comp.ngOnInit(); | ||
| await fixture.whenStable(); | ||
|
|
||
| expect(fingerPintsServiceMock.getSshFingerprints).toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Expand test coverage
The current test only verifies that getSshFingerprints is called. Consider adding tests for:
- Error handling scenarios
- Empty/null fingerprints
- Correct rendering of fingerprint values
- Loading state handling
it('should handle error when fetching fingerprints', async () => {
fingerPintsServiceMock.getSshFingerprints.mockRejectedValue(new Error('Failed to fetch'));
await comp.ngOnInit();
await fixture.whenStable();
// Verify error handling
});
it('should handle empty fingerprints', async () => {
fingerPintsServiceMock.getSshFingerprints.mockResolvedValue({});
await comp.ngOnInit();
await fixture.whenStable();
// Verify UI handles empty state
});
it('should display all fingerprint types', async () => {
await comp.ngOnInit();
await fixture.whenStable();
fixture.detectChanges();
// Verify each fingerprint type is displayed correctly
});| const mockFingerprints: { [key: string]: string } = { | ||
| RSA: 'SHA512:abcde123', | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance mock data coverage
The mock fingerprints object only includes RSA type. Consider adding other supported types (EdDSA, ECDSA, EC) to ensure comprehensive testing.
const mockFingerprints: { [key: string]: string } = {
RSA: 'SHA512:abcde123',
+ EdDSA: 'SHA512:eddsatest',
+ ECDSA: 'SHA512:ecdsatest',
+ EC: 'SHA512:ectest',
};📝 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.
| const mockFingerprints: { [key: string]: string } = { | |
| RSA: 'SHA512:abcde123', | |
| }; | |
| const mockFingerprints: { [key: string]: string } = { | |
| RSA: 'SHA512:abcde123', | |
| EdDSA: 'SHA512:eddsatest', | |
| ECDSA: 'SHA512:ecdsatest', | |
| EC: 'SHA512:ectest', | |
| }; |
| @if (sshFingerprints && sshFingerprints['RSA']) { | ||
| <div class="row small-text"> | ||
| <div class="column left"> | ||
| {{ 'RSA' }} | ||
| </div> | ||
| <div class="column right"> | ||
| {{ sshFingerprints['RSA'] }} | ||
| </div> | ||
| </div> | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor repeated fingerprint display blocks
The template contains duplicate code blocks for different SSH key types. Consider extracting this into a reusable component or using @for with an array of key types.
- @if (sshFingerprints && sshFingerprints['RSA']) {
- <div class="row small-text">
- <div class="column left">
- {{ 'RSA' }}
- </div>
- <div class="column right">
- {{ sshFingerprints['RSA'] }}
- </div>
- </div>
- }
- // ... similar blocks for other types
+ @for (type of ['RSA', 'EdDSA', 'ECDSA', 'EC'] as type) {
+ @if (sshFingerprints && sshFingerprints[type]) {
+ <div class="row small-text">
+ <div class="column left">
+ {{ type === 'EdDSA' ? 'ED25519' : type }}
+ </div>
+ <div class="column right">
+ {{ sshFingerprints[type] }}
+ </div>
+ </div>
+ }
+ }Also applies to: 25-34, 36-45, 47-56
| @Nested | ||
| class SshFingerprintsProvider { | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void shouldReturnFingerprints() throws Exception { | ||
| var response = request.get("/api/ssh-fingerprints", HttpStatus.OK, Map.class); | ||
| assertThat(response.get("RSA")).isNotNull(); | ||
| assertThat(response.get("RSA")).isEqualTo(expectedFingerprint); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void shouldReturnNoFingerprintsWithoutKeyProviderSetup() throws Exception { | ||
| doReturn(null).when(sshServer).getKeyPairProvider(); | ||
|
|
||
| var response = request.get("/api/ssh-fingerprints", HttpStatus.OK, Map.class); | ||
| assertThat(response.isEmpty()).isTrue(); | ||
| } | ||
|
|
||
| @Test | ||
| @WithMockUser(username = TEST_PREFIX + "student1", roles = "USER") | ||
| void shouldReturnBadRequestWhenLoadKeysThrowsException() throws Exception { | ||
| doThrow(new IOException("Test exception")).when(keyPairProvider).loadKeys(null); | ||
|
|
||
| request.get("/api/ssh-fingerprints", HttpStatus.BAD_REQUEST, Map.class); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add test case for unauthorized access
The test suite covers successful retrieval, missing key provider, and exception scenarios. Consider adding a test case for unauthorized access to ensure proper security constraints.
Add this test method:
@Test
void shouldReturnUnauthorizedWithoutUser() throws Exception {
request.get("/api/ssh-fingerprints", HttpStatus.UNAUTHORIZED, Map.class);
}
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
Description
Steps for Testing
Prerequisites:
Exam Mode Testing
Prerequisites:
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Exam Mode Test
Performance Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Documentation
Tests