Development: Restrict course detail page access
#9834
Conversation
github-actions
bot
added
the
client
cremertim
changed the title
Development Restrict Course Overview Access in the FrontendDevelopment: Restrict Course Overview Access in the Frontend
cremertim
changed the title
Development: Restrict Course Overview Access in the FrontendDevelopment: Restrict Course Overview Access
WalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 2
🧹 Outside diff range and nitpick comments (10)
src/main/webapp/app/overview/courses-routing.module.ts (4)
200-200: Consider adding UserRouteAccessService for consistency.Other protected routes use both
UserRouteAccessServiceandCourseOverviewGuardService. For consistency, consider addingUserRouteAccessServicehere as well.-canActivate: [CourseOverviewGuardService], +canActivate: [UserRouteAccessService, CourseOverviewGuardService],
Line range hint
221-235: Add CourseOverviewGuardService to the child route.The tutorial group details route should also be protected by
CourseOverviewGuardServiceto prevent direct access.{ path: ':tutorialGroupId', component: CourseTutorialGroupDetailComponent, data: { authorities: [Authority.USER], pageTitle: 'overview.tutorialGroups', hasSidebar: true, showRefreshButton: true, }, - canActivate: [UserRouteAccessService], + canActivate: [UserRouteAccessService, CourseOverviewGuardService], loadChildren: () => import('../overview/tutorial-group-details/course-tutorial-group-details.module').then((m) => m.CourseTutorialGroupDetailsModule), },
280-280: Consider adding UserRouteAccessService for consistency.For consistency with other routes and to ensure proper authentication, consider adding
UserRouteAccessService.-canActivate: [CourseOverviewGuardService], +canActivate: [UserRouteAccessService, CourseOverviewGuardService],
Line range hint
170-280: Review the overall route protection strategy.The current implementation shows inconsistencies in how routes are protected:
- Some routes use both guards while others only use
CourseOverviewGuardService- Child routes generally lack the
CourseOverviewGuardServiceprotection- There's no clear pattern for when both guards should be used vs. just one
Consider:
- Documenting the guard usage strategy
- Implementing a consistent approach across all routes
- Creating a combined guard that incorporates both user access and course-specific checks
src/main/webapp/app/overview/course-overview-guard.service.ts (6)
20-22: Correct typos in method documentation.There are a couple of typos in the method documentation that should be corrected for clarity:
- "route route" should be "route".
- "CourseD" should be "course".
Apply this diff to fix the typos:
/** - * Check if the client can activate a course overview route route. - * @return true if CourseD is enabled for this instance, false otherwise + * Check if the client can activate a course overview route. + * @return true if the course is enabled for this instance, false otherwise */
14-17: Prefer constructor injection overinject()for dependency injection.While the
inject()function is valid, using constructor injection is recommended for services according to the Angular Style Guide. Constructor injection improves readability and testability.Refactor the service to use constructor injection:
export class CourseOverviewGuardService implements CanActivate { - private courseStorageService = inject(CourseStorageService); - private courseManagementService = inject(CourseManagementService); - private router = inject(Router); - private serverDateService = inject(ArtemisServerDateService); + constructor( + private courseStorageService: CourseStorageService, + private courseManagementService: CourseManagementService, + private router: Router, + private serverDateService: ArtemisServerDateService, + ) {}Ensure to update any references to
this.courseStorageService,this.courseManagementService,this.router, andthis.serverDateServiceaccordingly.
49-77: RefactorhandleReturnto be a class method.Declaring
handleReturnas a class method instead of an arrow function enhances code consistency and aligns with the Angular Style Guide.Apply this diff to refactor the method:
-export class CourseOverviewGuardService implements CanActivate { - // ... - handleReturn = (course?: Course, type?: string): Observable<boolean> => { + handleReturn(course?: Course, type?: string): Observable<boolean> { let hasAccess: boolean; // Rest of the method remains the sameRemember to remove the equals sign and the arrow function syntax.
79-88: OptimizehasVisibleExamsmethod withsomefunction.You can simplify the exam visibility check using the
somearray method for better readability.Refactor the method as follows:
hasVisibleExams(course?: Course): boolean { - if (course?.exams) { - for (const exam of course.exams) { - if (exam.visibleDate && dayjs(exam.visibleDate).isBefore(this.serverDateService.now())) { - return true; - } - } - } - return false; + return course?.exams?.some( + (exam) => exam.visibleDate && dayjs(exam.visibleDate).isBefore(this.serverDateService.now()), + ) ?? false; }
23-47: Handle potential errors when fetching course data.Currently, there's no error handling in the observable pipeline when fetching the course. Unhandled errors could lead to unexpected behavior.
Consider adding error handling to the observable chain:
return this.courseManagementService.find(courseIdNumber).pipe( switchMap((res) => { if (res.body) { // Store course in cache this.courseStorageService.updateCourse(res.body); } // Flatten the result to return Observable<boolean> directly return this.handleReturn(this.courseStorageService.getCourse(courseIdNumber), path); + }), + catchError(() => { + // Handle the error, possibly redirect or return false + return of(false); }), );Ensure to import
catchErrorfromrxjs/operators.
Line range hint
93-93: Add unit tests forCourseOverviewGuardService.Including meaningful unit tests will help ensure the guard functions correctly under various scenarios.
Would you like assistance in creating unit tests for this service?
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/overview/course-dashboard/course-dashboard-guard.service.ts(0 hunks)src/main/webapp/app/overview/course-overview-guard.service.ts(1 hunks)src/main/webapp/app/overview/courses-routing.module.ts(7 hunks)
💤 Files with no reviewable changes (1)
- src/main/webapp/app/overview/course-dashboard/course-dashboard-guard.service.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/main/webapp/app/overview/course-overview-guard.service.ts (1)
src/main/webapp/app/overview/courses-routing.module.ts (1)
🔇 Additional comments (4)
src/main/webapp/app/overview/courses-routing.module.ts (4)
15-15: LGTM: Import changes align with the new guard implementation.
The import of CourseOverviewGuardService is correctly added to support the new access control mechanism.
189-189: LGTM: Comprehensive route protection with both guards.
The combination of UserRouteAccessService and CourseOverviewGuardService provides both authentication and course-specific access control.
Line range hint 170-182: Consider adding the guard to child routes.
While the parent route is protected by CourseOverviewGuardService, the child routes (especially the competency details route) should also be protected to prevent direct access.
Consider adding the guard to the child routes:
{
path: ':competencyId',
+ canActivate: [CourseOverviewGuardService],
loadChildren: () => import('../overview/course-competencies/course-competencies-details.module').then((m) => m.ArtemisCourseCompetenciesDetailsModule),
},Line range hint 246-261: Verify exam participation access requirements.
- The exam participation route should be protected by
CourseOverviewGuardServiceto prevent direct access. - Please verify if there are any special access requirements for exam participation that need to be considered.
{
path: ':examId',
component: ExamParticipationComponent,
data: {
authorities: [Authority.USER],
pageTitle: 'overview.exams',
hasSidebar: true,
showRefreshButton: true,
},
- canActivate: [UserRouteAccessService],
+ canActivate: [UserRouteAccessService, CourseOverviewGuardService],
canDeactivate: [PendingChangesGuard],
loadChildren: () => import('../exam/participate/exam-participation.module').then((m) => m.ArtemisExamParticipationModule),
},
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/main/webapp/app/overview/course-overview-guard.service.ts (3)
51-72: Simplify route access logic using an object literal pattern.The switch statement could be simplified for better maintainability and readability.
- switch (type) { - case 'exams': - hasAccess = this.hasVisibleExams(course); - break; - case 'competencies': - hasAccess = !!(course?.numberOfCompetencies || course?.numberOfPrerequisites); - break; - // ... other cases - default: - hasAccess = false; - } + const accessChecks = { + 'exams': () => this.hasVisibleExams(course), + 'competencies': () => !!(course?.numberOfCompetencies || course?.numberOfPrerequisites), + 'tutorial-groups': () => !!course?.numberOfTutorialGroups, + 'dashboard': () => course?.studentCourseAnalyticsDashboardEnabled ?? false, + 'faq': () => course?.faqEnabled ?? false, + 'learning-path': () => course?.learningPathsEnabled ?? false + }; + hasAccess = type ? (accessChecks[type]?.() ?? false) : false;
79-88: Optimize exam visibility check using Array.some().The current implementation can be simplified and made more efficient by using Array methods.
hasVisibleExams(course?: Course): boolean { - if (course?.exams) { - for (const exam of course.exams) { - if (exam.visibleDate && dayjs(exam.visibleDate).isBefore(this.serverDateService.now())) { - return true; - } - } - } - return false; + return course?.exams?.some( + exam => exam.visibleDate && dayjs(exam.visibleDate).isBefore(this.serverDateService.now()) + ) ?? false; }Also, consider using UTC dates for comparison to avoid timezone-related issues:
- dayjs(exam.visibleDate).isBefore(this.serverDateService.now()) + dayjs.utc(exam.visibleDate).isBefore(this.serverDateService.now())
1-89: Consider implementing a caching strategy for course data.To improve performance and reduce server load:
- Implement a TTL (Time-To-Live) for cached course data
- Consider prefetching course data for likely navigation paths
- Add error boundaries for failed course fetches
This would enhance the user experience by reducing loading times and providing better error handling.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
src/main/webapp/app/overview/course-overview-guard.service.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-overview-guard.service.ts (1)
🔇 Additional comments (1)
src/main/webapp/app/overview/course-overview-guard.service.ts (1)
1-18: LGTM! Well-structured service setup.
The service follows Angular best practices with proper dependency injection using the modern inject() function and appropriate class naming conventions.
SimonKaran13
temporarily deployed
to
artemis-test2.artemis.cit.tum.de
GitHub Actions
Inactive
SimonKaran13
temporarily deployed
to
artemis-test6.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
I tested the following on TS6
- I tested the Exams tab is accessible when a visible exam exists and inaccessible otherwise.
- Similarly, the Competencies tab is accessible only when competencies or prerequisites are present.
- The Tutorial Groups tab requires at least one tutorial group to be visible.
- For the FAQs tab, access depends on whether it is enabled in the course settings.
- Learning Paths tab is accessible only when enabled, even if there is not content (expected behavior).
- Finally, I verified that users are redirected to the Exercises tab if none of these features are accessible.
Sometimes right after activating/deactivating a feature it could still be the case that you are not shown/shown the feature or are wrongly redirected, but this is probably due to caching and after reloading the page a couple of times it works as expected.
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
HawKhiem
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
src/main/webapp/app/overview/course-overview-guard.ts (2)
35-35: Fix typo and improve comment clarityThe comment has a typo ("misses" should be "missing") and could be more descriptive.
-//we need to load the course from the server to check if the user has access to the requested route. The course in the cache might not be sufficient (e.g. misses exams or lectures) +// Load fresh course data from the server to ensure accurate access control. +// Cached course data might be incomplete (e.g., missing exams or lectures)
89-98: Optimize exam visibility checkConsider using Array methods for better readability and potentially better performance.
hasVisibleExams(course?: Course): boolean { - if (course?.exams) { - for (const exam of course.exams) { - if (exam.visibleDate && dayjs(exam.visibleDate).isBefore(this.serverDateService.now())) { - return true; - } - } - } - return false; + return course?.exams?.some( + exam => exam.visibleDate && dayjs(exam.visibleDate).isBefore(this.serverDateService.now()) + ) ?? false; }
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (3)
src/main/webapp/app/overview/course-overview-guard.ts(1 hunks)src/main/webapp/app/overview/courses-routing.module.ts(10 hunks)src/test/javascript/spec/component/overview/course-overview-guard.spec.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/webapp/app/overview/courses-routing.module.ts
- src/test/javascript/spec/component/overview/course-overview-guard.spec.ts
🧰 Additional context used
📓 Path-based instructions (1)
src/main/webapp/app/overview/course-overview-guard.ts (1)
🔇 Additional comments (5)
src/main/webapp/app/overview/course-overview-guard.ts (5)
1-19: LGTM! Clean implementation of dependency injection
The class follows Angular's best practices using the modern inject() function for dependency injection.
29-29:
Add error handling and type safety
The current implementation has two potential issues:
- No error handling for HTTP request failures
- No validation for parsed courseId
const courseIdNumber = parseInt(courseIdString, 10);
+if (isNaN(courseIdNumber)) {
+ return of(false);
+}
return this.courseManagementService.findOneForDashboard(courseIdNumber).pipe(
switchMap((res) => {
if (res.body) {
// Store course in cache
this.courseStorageService.updateCourse(res.body);
}
// Flatten the result to return Observable<boolean> directly
return this.handleReturn(this.courseStorageService.getCourse(courseIdNumber), path);
}),
+ catchError((error) => {
+ console.error('Failed to load course:', error);
+ return of(false);
+ }),
);Also applies to: 36-45
56-56:
Ensure lectures array has content before granting access
The current check doesn't verify if there are actually any lectures available.
84-84:
Prevent navigation with undefined course ID
Navigation should be safeguarded against undefined course ID.
82-86:
Prevent potential memory leaks from router navigation
The router navigation subscription should be properly handled to prevent memory leaks.
if (!hasAccess) {
- this.router.navigate([`/courses/${course?.id}/exercises`]);
+ if (course?.id) {
+ this.router.navigate([`/courses/${course.id}/exercises`])
+ .catch(error => console.error('Navigation failed:', error));
+ }
}Likely invalid or redundant comment.
PaRangger
temporarily deployed
to
artemis-test3.artemis.cit.tum.de
GitHub Actions
Inactive
|
github-actions
bot
added
the
deployment-error
flbrgit
removed
the
deployment-error
krusche
changed the title
Development: Restrict Course Overview AccessDevelopment: Restrict course detail page access
krusche
deleted the
chore/communication/restrict-course-overview-access
branch
Checklist
General
Client
authoritiesto all new routes and checked the course groups for displaying navigation elements (links, buttons).Motivation and Context
If the users want to, they can always access any of the subpages in the Course Overview, regardless of the course configuration. This is possible by inserting the corresponding url e.g /courses/{courseId}/faq
Description
I implemented an CourseOverviewGuardService to restrict access of the sites
Steps for 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
Test Coverage
Summary by CodeRabbit
Summary by CodeRabbit
New Features
CourseOverviewGuardto manage access to course overview routes.Bug Fixes
CourseDashboardGuardfor improved access control across various routes.Chores