-
Notifications
You must be signed in to change notification settings - Fork 297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Development
: Update LTI components to use Angular 18 practices
#9908
Development
: Update LTI components to use Angular 18 practices
#9908
Conversation
Development
: Update LTI components to use Angular 18 practices
WalkthroughThe changes in this pull request involve updating multiple components within the LTI module of the Angular application to be standalone components. This is achieved by adding the 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (14)
src/main/webapp/app/lti/lti13-select-course.component.ts (1)
Line range hint
24-39
: Implement proper subscription management and consistent async patterns.Several improvements can enhance this component:
- Implement
OnDestroy
to clean up subscriptions- Use consistent async patterns
- Add proper error typing
Consider applying these changes:
-export class LtiCoursesComponent implements OnInit { +export class LtiCoursesComponent implements OnInit, OnDestroy { public courses: OnlineCourseDtoModel[]; + private destroy$ = new Subject<void>(); constructor( private courseService: CourseManagementService, private sessionStorageService: SessionStorageService, private alertService: AlertService, ) {} - async ngOnInit() { + ngOnInit() { this.loadAndFilterCourses(); } + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + } loadAndFilterCourses() { const clientId = this.sessionStorageService.retrieve('clientRegistrationId'); - this.courseService.findAllOnlineCoursesWithRegistrationId(clientId).subscribe({ + this.courseService.findAllOnlineCoursesWithRegistrationId(clientId) + .pipe( + takeUntil(this.destroy$), + catchError((error: HttpErrorResponse) => { + this.alertService.error('error.unexpectedError', { + error: error.message, + }); + return EMPTY; + }) + ).subscribe({ next: (courseResponse: OnlineCourseDtoModel[]) => { this.courses = courseResponse; - }, - error: (error) => { - this.alertService.error('error.unexpectedError', { - error: error.message, - }); }, }); }This refactor:
- Prevents memory leaks by properly managing subscriptions
- Provides better error typing with
HttpErrorResponse
- Uses RxJS operators for error handling
- Removes unnecessary async/await since we're using observables
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (3)
4-4
: Use single quotes for string literalsAccording to the coding guidelines, string literals should use single quotes.
-import { TranslateDirective } from "../shared/language/translate.directive"; +import { TranslateDirective } from '../shared/language/translate.directive';
Line range hint
26-29
: Subscribe to route params should be properly managedThe subscription to route params should be properly managed to prevent memory leaks.
+ private destroy$ = new Subject<void>(); ngOnInit(): void { - this.route.params.subscribe((params) => { + this.route.params.pipe( + takeUntil(this.destroy$) + ).subscribe((params) => { this.courseId = Number(params['courseId']); });Also add:
ngOnDestroy(): void { this.destroy$.next(); this.destroy$.complete(); }
Line range hint
44-54
: Enhance error handling and user feedbackThe error handling could be improved by:
- Providing specific error details for debugging
- Adding proper loading state management for UI feedback
- Adding type safety for window messaging
.subscribe({ next: () => { this.registeredSuccessfully = true; }, - error: () => { + error: (error: HttpErrorResponse) => { this.registeredSuccessfully = false; + console.error('LTI registration failed:', error); }, }) .add(() => { this.isRegistering = false; - (window.opener || window.parent).postMessage({ subject: 'org.imsglobal.lti.close' }, '*'); + const targetWindow = (window.opener || window.parent) as Window; + targetWindow.postMessage({ subject: 'org.imsglobal.lti.close' }, '*'); });src/main/webapp/app/lti/lti13-select-content.component.ts (1)
1-6
: Use single quotes for string literalsAccording to the coding guidelines, string literals should use single quotes.
Apply this diff to fix the quotes:
-import { Component, ElementRef, NgZone, OnInit, SecurityContext, ViewChild, inject } from "@angular/core"; -import { ActivatedRoute } from "@angular/router"; -import { DomSanitizer } from "@angular/platform-browser"; -import { TranslateDirective } from "../shared/language/translate.directive"; -import { FormsModule } from "@angular/forms"; -import { ArtemisSharedPipesModule } from "../shared/pipes/shared-pipes.module"; +import { Component, ElementRef, NgZone, OnInit, SecurityContext, ViewChild, inject } from '@angular/core'; +import { ActivatedRoute } from '@angular/router'; +import { DomSanitizer } from '@angular/platform-browser'; +import { TranslateDirective } from '../shared/language/translate.directive'; +import { FormsModule } from '@angular/forms'; +import { ArtemisSharedPipesModule } from '../shared/pipes/shared-pipes.module';src/main/webapp/app/lti/lti13-exercise-launch.component.ts (5)
9-9
: Consider using absolute import pathReplace the relative import path with an absolute path for better maintainability.
-import { TranslateDirective } from '../shared/language/translate.directive'; +import { TranslateDirective } from 'app/shared/language/translate.directive';
Line range hint
41-67
: Add CSRF protection and input validationThe
sendRequest
method handles sensitive authentication data but lacks some security measures:
- No CSRF protection in the POST request
- No validation of state and idToken parameters before use
Consider applying these security enhancements:
sendRequest(): void { const state = this.route.snapshot.queryParamMap.get('state'); const idToken = this.route.snapshot.queryParamMap.get('id_token'); - if (!state || !idToken) { + if (!state?.match(/^[a-zA-Z0-9-_]+$/) || !idToken?.match(/^[a-zA-Z0-9-_]+\.([a-zA-Z0-9-_]+\.)?[a-zA-Z0-9-_]+$/)) { console.error('Required parameter for LTI launch missing'); this.isLaunching = false; return; } const requestBody = new HttpParams().set('state', state).set('id_token', idToken); this.http .post<LtiLaunchResponse>('api/public/lti13/auth-login', requestBody.toString(), { - headers: new HttpHeaders().set('Content-Type', 'application/x-www-form-urlencoded'), + headers: new HttpHeaders() + .set('Content-Type', 'application/x-www-form-urlencoded') + .set('X-XSRF-TOKEN', this.getCsrfToken()), })
Line range hint
127-146
: Enhance security of session storage handlingThe
storeLtiSessionData
method stores sensitive tokens in session storage without encryption:
- Consider using more secure storage methods
- Implement token expiration
- Add sanitization before storage
Consider implementing a secure storage service:
private secureStore(key: string, value: string): void { // Encrypt before storage const encryptedValue = this.encryptionService.encrypt(value); // Set expiration const data = { value: encryptedValue, expiry: Date.now() + (60 * 60 * 1000) // 1 hour }; this.sessionStorageService.store(key, JSON.stringify(data)); }
Line range hint
148-156
: Prevent potential memory leaksThe component subscribes to authentication state but doesn't unsubscribe, which could cause memory leaks.
Implement proper subscription management:
+private destroy$ = new Subject<void>(); replaceWindowLocationWrapper(url: string): void { this.ltiService.setShownViaLti(true); this.themeService.applyThemePreference(Theme.LIGHT); const path = new URL(url).pathname; this.router.navigate([path], { replaceUrl: true }); } +ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); +}
Line range hint
51-67
: Replace console.error with proper error loggingError logging to console exposes sensitive information in production.
Use a proper logging service:
-console.error('Required parameter for LTI launch missing'); +this.loggingService.logError('LTI launch failed: missing parameters', { + sensitive: false, + context: 'LTI Launch' +});src/main/webapp/app/lti/lti13-deep-linking.component.ts (4)
13-18
: LGTM! Consider organizing imports.The standalone component migration looks good. All necessary modules are imported and properly configured in the component decorator.
Consider organizing imports into these groups, separated by newlines:
- Angular core/common
- Application-specific imports
- Third-party libraries
import { Component, OnInit } from '@angular/core'; import { ActivatedRoute, Router } from '@angular/router'; import { HttpClient, HttpParams } from '@angular/common/http'; import { FormsModule } from '@angular/forms'; import { CourseManagementService } from 'app/course/manage/course-management.service'; import { Exercise } from 'app/entities/exercise.model'; import { SortService } from 'app/shared/service/sort.service'; import { AccountService } from 'app/core/auth/account.service'; import { Course } from 'app/entities/course.model'; import { AlertService } from 'app/core/util/alert.service'; import { ArtemisSharedModule } from 'app/shared/shared.module'; import { TranslateDirective } from '../shared/language/translate.directive'; import { ArtemisSharedComponentModule } from '../shared/components/shared-component.module'; import { ArtemisSharedCommonModule } from '../shared/shared-common.module'; import { faExclamationTriangle, faSort, faWrench } from '@fortawesome/free-solid-svg-icons'; import { FaIconComponent } from '@fortawesome/angular-fontawesome'; import { SessionStorageService } from 'ngx-webstorage';Also applies to: 23-24
Line range hint
26-51
: Prevent memory leaks by implementing proper subscription cleanup.The component has multiple subscriptions that need to be cleaned up to prevent memory leaks.
Implement the following changes:
-export class Lti13DeepLinkingComponent implements OnInit { +export class Lti13DeepLinkingComponent implements OnInit, OnDestroy { + private destroy$ = new Subject<void>(); + courseId: number; exercises: Exercise[]; selectedExercises?: Set<number> = new Set(); course: Course; // ... rest of the properties ... ngOnInit() { - this.route.params.subscribe((params) => { + this.route.params.pipe( + takeUntil(this.destroy$) + ).subscribe((params) => { // ... existing code ... }); } + ngOnDestroy() { + this.destroy$.next(); + this.destroy$.complete(); + }Also update the
redirectUserToLoginThenTargetLink
method:redirectUserToLoginThenTargetLink(currentLink: string): void { this.router.navigate(['/']).then(() => { - this.accountService.getAuthenticationState().subscribe((user) => { + this.accountService.getAuthenticationState().pipe( + takeUntil(this.destroy$) + ).subscribe((user) => { // ... existing code ... }); }); }
26-26
: Add class-level documentation.Missing JSDoc documentation for the component class.
Add comprehensive documentation:
+/** + * Component for handling LTI 1.3 deep linking functionality. + * Allows users to select exercises from a course and create deep links for LTI integration. + * Manages the selection, validation, and linking process while handling authentication + * and error states appropriately. + */ export class Lti13DeepLinkingComponent implements OnInit {
Line range hint
147-167
: Enhance error handling with specific error messages.The error handling in
sendDeepLinkRequest
could be more specific to help with debugging.Consider enhancing the error handling:
sendDeepLinkRequest() { if (this.selectedExercises?.size) { // ... existing code ... this.http.post<DeepLinkingResponse>(`api/lti13/deep-linking/${this.courseId}`, null, { observe: 'response', params: httpParams }).subscribe({ next: (response) => { if (response.status === 200) { if (response.body) { const targetLink = response.body.targetLinkUri; window.location.replace(targetLink); } } else { this.isLinking = false; - this.alertService.error('artemisApp.lti13.deepLinking.unknownError'); + this.alertService.error('artemisApp.lti13.deepLinking.responseError', { status: response.status }); } }, error: (error) => { this.isLinking = false; - onError(this.alertService, error); + const errorKey = error.status === 403 ? 'artemisApp.lti13.deepLinking.unauthorized' : + error.status === 404 ? 'artemisApp.lti13.deepLinking.notFound' : + 'artemisApp.lti13.deepLinking.unknownError'; + this.alertService.error(errorKey, { status: error.status }); }, }); } else { this.alertService.error('artemisApp.lti13.deepLinking.selectToLink'); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (7)
src/main/webapp/app/lti/lti-course-card.component.ts
(1 hunks)src/main/webapp/app/lti/lti.module.ts
(1 hunks)src/main/webapp/app/lti/lti13-deep-linking.component.ts
(1 hunks)src/main/webapp/app/lti/lti13-dynamic-registration.component.ts
(1 hunks)src/main/webapp/app/lti/lti13-exercise-launch.component.ts
(2 hunks)src/main/webapp/app/lti/lti13-select-content.component.ts
(1 hunks)src/main/webapp/app/lti/lti13-select-course.component.ts
(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- src/main/webapp/app/lti/lti.module.ts
🧰 Additional context used
📓 Path-based instructions (6)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
src/main/webapp/app/lti/lti13-deep-linking.component.ts (1)
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (1)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
src/main/webapp/app/lti/lti13-select-content.component.ts (1)
src/main/webapp/app/lti/lti13-select-course.component.ts (1)
🔇 Additional comments (9)
src/main/webapp/app/lti/lti-course-card.component.ts (1)
5-7
: LGTM! Imports follow Angular guidelines.
The new imports are correctly organized and follow the Angular style guide recommendations.
src/main/webapp/app/lti/lti13-select-course.component.ts (3)
6-7
: LGTM! Import statements are properly structured.
The new imports for standalone components follow Angular style guidelines and match the component's declared dependencies.
12-13
: LGTM! Component decorator properly configured for standalone mode.
The standalone configuration and imports are correctly implemented.
Line range hint 1-39
: Verify string literal consistency across the component.
Let's ensure all string literals follow the single quotes convention as per coding guidelines.
✅ Verification successful
String literals in the component follow the single quotes convention
The TypeScript component correctly uses single quotes for all string literals:
- Import paths: '@angular/core', 'ngx-webstorage', etc.
- Component selector: 'jhi-lti-courses-overview'
- Template URL: './lti13-select-course.component.html'
- Storage key: 'clientRegistrationId'
- Error message: 'error.unexpectedError'
The double quotes in the HTML template file are appropriate as they follow HTML conventions.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for double-quoted strings in the component and template
rg -l "\"[^\"]+\"" "src/main/webapp/app/lti/lti13-select-course.component.*"
Length of output: 171
Script:
#!/bin/bash
# First, let's find the exact file paths
fd "lti13-select-course.component" src/main/webapp
# Then check for double-quoted strings in those files
rg -l "\"[^\"]+\"" $(fd "lti13-select-course.component" src/main/webapp)
Length of output: 411
Script:
#!/bin/bash
# Let's see the actual content to verify string usage in both files
echo "=== TypeScript Component ==="
rg -A 1 "'" src/main/webapp/app/lti/lti13-select-course.component.ts
echo -e "\n=== Double quotes in TypeScript Component ==="
rg -A 1 "\"" src/main/webapp/app/lti/lti13-select-course.component.ts
echo -e "\n=== Template File ==="
cat src/main/webapp/app/lti/lti13-select-course.component.html
Length of output: 1946
src/main/webapp/app/lti/lti13-dynamic-registration.component.ts (2)
9-10
: LGTM! Good standalone component setup
The component is correctly configured as standalone with the necessary TranslateDirective import, which aligns with modern Angular practices and the localization requirements.
Line range hint 12-54
: Add unit tests for component logic
The PR objectives emphasize the importance of integration tests. Please ensure proper test coverage for:
- Successful registration flow
- Error handling scenarios
- Window message posting
- Route parameter handling
src/main/webapp/app/lti/lti13-select-content.component.ts (3)
18-19
: LGTM! Good job on making the component standalone
The component is correctly configured as standalone with the necessary imports. This change improves modularity and follows modern Angular practices.
22-24
: LGTM! Good use of modern DI pattern
The dependency injection using the inject()
function is well implemented. The approach:
- Reduces boilerplate compared to constructor injection
- Maintains proper typing
- Follows the no-prefix guideline for private members
Line range hint 56-58
: Verify complete XSS protection for deep linking
While the code correctly uses sanitize()
for the URL, let's verify that all potential XSS vectors are covered.
✅ Verification successful
Based on the results, I can generate the final response as we have sufficient information about the security measures in place:
Deep linking implementation has proper XSS protection
The code implements comprehensive XSS protection for deep linking through:
- Proper URL sanitization using both
DomSanitizer
andSecurityContext.URL
- No direct DOM manipulation through innerHTML/outerHTML that could bypass sanitization
- Strict parameter validation before form submission
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other deep linking related security measures
rg -l "deepLink" | xargs rg "sanitize|SecurityContext|DomSanitizer" -A 3
# Check for any direct DOM manipulation that might bypass sanitization
ast-grep --pattern 'innerHTML = $_'
ast-grep --pattern 'outerHTML = $_'
Length of output: 3569
…tion-lti-selection-components
…nal-migration-lti-selection-components' into chore/performance/angular-18-signal-migration-lti-selection-components
…tion-lti-selection-components
…tion-lti-selection-components
…tion-lti-selection-components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (3)
Line range hint
63-86
: Add unsubscribe handling for HTTP subscriptionThe HTTP subscription in
sendRequest()
should be cleaned up to prevent potential memory leaks.Consider using the async pipe in the template or implementing proper cleanup:
+ private destroy$ = new Subject<void>(); ngOnInit(): void { this.sendRequest(); } + ngOnDestroy(): void { + this.destroy$.next(); + this.destroy$.complete(); + } sendRequest(): void { // ... existing code ... this.http .post<LtiLaunchResponse>(...) + .pipe(takeUntil(this.destroy$)) .subscribe({ // ... existing code ... }); }
Line range hint
146-159
: Enhance URL validation in replaceWindowLocationWrapperThe URL handling could be vulnerable to open redirect attacks.
Add URL validation before navigation:
replaceWindowLocationWrapper(url: string): void { this.ltiService.setShownViaLti(true); this.themeService.applyThemePreference(Theme.LIGHT); let path; + // Whitelist of allowed paths + const allowedPaths = ['/lti/select-course']; if (url === '/lti/select-course') { path = url; } else { + try { const parsedUrl = new URL(url); + // Ensure URL is from the same origin or in allowedPaths + if (!allowedPaths.includes(parsedUrl.pathname) && parsedUrl.origin !== window.location.origin) { + console.error('Invalid URL detected'); + return; + } path = parsedUrl.pathname; + } catch (e) { + console.error('Invalid URL format'); + return; + } } this.router.navigate([path], { replaceUrl: true }); }
Line range hint
122-137
: Enhance error handling in storeLtiSessionDataThe error handling in
storeLtiSessionData
could be more robust.Consider adding specific error types and user feedback:
storeLtiSessionData(ltiIdToken: string, clientRegistrationId: string): void { if (!ltiIdToken) { - captureException(new Error('LTI ID token required to store session data.')); + const error = new Error('LTI ID token required to store session data.'); + captureException(error); + this.handleLtiLaunchError(); return; } if (!clientRegistrationId) { - captureException(new Error('LTI client registration ID required to store session data.')); + const error = new Error('LTI client registration ID required to store session data.'); + captureException(error); + this.handleLtiLaunchError(); return; } try { this.sessionStorageService.store('ltiIdToken', ltiIdToken); this.sessionStorageService.store('clientRegistrationId', clientRegistrationId); } catch (error) { - console.error('Failed to store session data:', error); + captureException(error); + this.handleLtiLaunchError(); } }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts
(2 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (1)
🔇 Additional comments (2)
src/main/webapp/app/lti/lti13-exercise-launch.component.ts (2)
Line range hint 1-17
: LGTM: Imports and type definitions are well-organized
The imports section includes all necessary modules and the type definition is properly structured.
18-32
: Verify template usage of TranslateDirective
While the component structure and dependency injection look good, let's verify that the TranslateDirective is being used in the template.
…tion-lti-selection-components
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works on Ts1+Moodle with test user 1
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reapprove
Checklist
General
Client
authorities
to all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
Migrating all LTI components to use Angular 18 practices
Description
Migrated LTI components in regard to standalone, input and signals
Adjusted tests to work with migrated components
Steps for Testing
Prerequisites:
(If Moodle throws an error in the iFrame, just refresh the page)
Testserver States
Note
These badges show the state of the test servers.
Green = Currently available, Red = Currently locked
Click on the badges to get to the test servers.
Review Progress
Performance Review
Code Review
Manual Tests
Test Coverage
Screenshots
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation
Chores