From df5a772b3486f7b68af5ee3286c8188a1550b624 Mon Sep 17 00:00:00 2001 From: Adrian Kunz Date: Mon, 6 Nov 2023 21:10:49 +0100 Subject: [PATCH 1/4] fix(assignments-service): Prevent unauthorized access to solution points via courses --- .../src/course/course.controller.ts | 3 ++- .../assignments/src/course/course.service.ts | 18 +++++++++++++++--- 2 files changed, 17 insertions(+), 4 deletions(-) diff --git a/services/apps/assignments/src/course/course.controller.ts b/services/apps/assignments/src/course/course.controller.ts index 779ce5448..9dc30813d 100644 --- a/services/apps/assignments/src/course/course.controller.ts +++ b/services/apps/assignments/src/course/course.controller.ts @@ -61,8 +61,9 @@ export class CourseController { @ApiOkResponse({type: [CourseStudent]}) async getStudents( @Param('id') id: string, + @AuthUser() user: UserToken, ): Promise { - return this.courseService.getStudents(id); + return this.courseService.getStudents(id, user.sub); } @Patch(':id') diff --git a/services/apps/assignments/src/course/course.service.ts b/services/apps/assignments/src/course/course.service.ts index d0f7cced2..d5a8ae289 100644 --- a/services/apps/assignments/src/course/course.service.ts +++ b/services/apps/assignments/src/course/course.service.ts @@ -1,12 +1,13 @@ import {EventService} from '@mean-stream/nestx'; import {Injectable} from '@nestjs/common'; import {InjectModel} from '@nestjs/mongoose'; -import {FilterQuery, Model} from 'mongoose'; +import {FilterQuery, Model, Types} from 'mongoose'; import {AuthorInfo} from '../solution/solution.schema'; import {SolutionService} from '../solution/solution.service'; import {idFilter} from '../utils'; import {CourseStudent, CreateCourseDto, UpdateCourseDto} from './course.dto'; import {Course, CourseDocument} from './course.schema'; +import {MemberService} from "@app/member"; @Injectable() export class CourseService { @@ -14,6 +15,7 @@ export class CourseService { @InjectModel(Course.name) private model: Model, private solutionService: SolutionService, private eventService: EventService, + private memberService: MemberService, ) { } @@ -34,14 +36,24 @@ export class CourseService { return this.model.findOne(idFilter(id)).exec(); } - async getStudents(id: string): Promise { + async getStudents(id: string, user: string): Promise { const course = await this.findOne(id); if (!course) { return []; } + + const userMembers = await this.memberService.findAll({ + parent: {$in: course.assignments.map(a => new Types.ObjectId(a))}, + user, + }); + const courseAssignmentsWhereUserIsMember = userMembers.map(m => m.parent.toString()); + if (!courseAssignmentsWhereUserIsMember.length) { + return []; + } + const students = new Map(); const solutions = await this.solutionService.model.aggregate([ - {$match: {assignment: {$in: course.assignments}}}, + {$match: {assignment: {$in: courseAssignmentsWhereUserIsMember}}}, {$addFields: {id: {$toString: '$_id'}}}, { $lookup: { From d95c2ac347ca396854180a602956cfa8c3987d0a Mon Sep 17 00:00:00 2001 From: Adrian Kunz Date: Mon, 6 Nov 2023 21:19:27 +0100 Subject: [PATCH 2/4] refactor(assignments-service): Remove idFilter helper --- .../apps/assignments/src/assignment/assignment.service.ts | 8 ++++---- services/apps/assignments/src/comment/comment.service.ts | 7 +++---- services/apps/assignments/src/course/course.service.ts | 7 +++---- .../apps/assignments/src/solution/solution.service.ts | 8 ++++---- services/apps/assignments/src/utils.ts | 4 ---- 5 files changed, 14 insertions(+), 20 deletions(-) diff --git a/services/apps/assignments/src/assignment/assignment.service.ts b/services/apps/assignments/src/assignment/assignment.service.ts index a46d0a542..f74e134e0 100644 --- a/services/apps/assignments/src/assignment/assignment.service.ts +++ b/services/apps/assignments/src/assignment/assignment.service.ts @@ -3,7 +3,7 @@ import {UserToken} from '@app/keycloak-auth'; import {Injectable} from '@nestjs/common'; import {InjectModel} from '@nestjs/mongoose'; import {FilterQuery, Model, UpdateQuery} from 'mongoose'; -import {generateToken, idFilter} from '../utils'; +import {generateToken} from '../utils'; import {CreateAssignmentDto, ReadAssignmentDto, ReadTaskDto, UpdateAssignmentDto} from './assignment.dto'; import {Assignment, AssignmentDocument, Task} from './assignment.schema'; import {MemberService} from "@app/member"; @@ -46,7 +46,7 @@ export class AssignmentService { } async findOne(id: string): Promise { - return this.model.findOne(idFilter(id)).exec(); + return this.model.findById(id).exec(); } mask(assignment: Assignment): ReadAssignmentDto { @@ -77,13 +77,13 @@ export class AssignmentService { update[`classroom.${key}`] = value; } } - const updated = await this.model.findOneAndUpdate(idFilter(id), update, {new: true}).exec(); + const updated = await this.model.findByIdAndUpdate(id, update, {new: true}).exec(); updated && this.emit('updated', updated); return updated; } async remove(id: string): Promise { - const deleted = await this.model.findOneAndDelete(idFilter(id)).exec(); + const deleted = await this.model.findByIdAndDelete(id).exec(); deleted && this.emit('deleted', deleted); return deleted; } diff --git a/services/apps/assignments/src/comment/comment.service.ts b/services/apps/assignments/src/comment/comment.service.ts index c435e83cf..d002334b0 100644 --- a/services/apps/assignments/src/comment/comment.service.ts +++ b/services/apps/assignments/src/comment/comment.service.ts @@ -3,7 +3,6 @@ import {UserToken} from '@app/keycloak-auth'; import {Injectable} from '@nestjs/common'; import {InjectModel} from '@nestjs/mongoose'; import {FilterQuery, Model} from 'mongoose'; -import {idFilter} from '../utils'; import {CreateCommentDto, UpdateCommentDto} from './comment.dto'; import {Comment, CommentDocument} from './comment.schema'; @@ -34,17 +33,17 @@ export class CommentService { } async findOne(id: string): Promise { - return this.model.findOne(idFilter(id)).exec(); + return this.model.findById(id).exec(); } async update(id: string, dto: UpdateCommentDto): Promise { - const updated = await this.model.findOneAndUpdate(idFilter(id), dto, {new: true}).exec(); + const updated = await this.model.findByIdAndUpdate(id, dto, {new: true}).exec(); updated && this.emit('updated', updated); return updated; } async remove(id: string): Promise { - const deleted = await this.model.findOneAndDelete(idFilter(id)).exec(); + const deleted = await this.model.findByIdAndDelete(id).exec(); deleted && this.emit('deleted', deleted); return deleted; } diff --git a/services/apps/assignments/src/course/course.service.ts b/services/apps/assignments/src/course/course.service.ts index d5a8ae289..24b295ca4 100644 --- a/services/apps/assignments/src/course/course.service.ts +++ b/services/apps/assignments/src/course/course.service.ts @@ -4,7 +4,6 @@ import {InjectModel} from '@nestjs/mongoose'; import {FilterQuery, Model, Types} from 'mongoose'; import {AuthorInfo} from '../solution/solution.schema'; import {SolutionService} from '../solution/solution.service'; -import {idFilter} from '../utils'; import {CourseStudent, CreateCourseDto, UpdateCourseDto} from './course.dto'; import {Course, CourseDocument} from './course.schema'; import {MemberService} from "@app/member"; @@ -33,7 +32,7 @@ export class CourseService { } async findOne(id: string): Promise { - return this.model.findOne(idFilter(id)).exec(); + return this.model.findById(id).exec(); } async getStudents(id: string, user: string): Promise { @@ -120,13 +119,13 @@ export class CourseService { } async update(id: string, dto: UpdateCourseDto): Promise { - const updated = await this.model.findOneAndUpdate(idFilter(id), dto, {new: true}).exec(); + const updated = await this.model.findByIdAndUpdate(id, dto, {new: true}).exec(); updated && this.emit('updated', updated); return updated; } async remove(id: string): Promise { - const deleted = await this.model.findOneAndDelete(idFilter(id)).exec(); + const deleted = await this.model.findByIdAndDelete(id).exec(); deleted && this.emit('deleted', deleted); return deleted; } diff --git a/services/apps/assignments/src/solution/solution.service.ts b/services/apps/assignments/src/solution/solution.service.ts index 28edb05ce..3e355e80e 100644 --- a/services/apps/assignments/src/solution/solution.service.ts +++ b/services/apps/assignments/src/solution/solution.service.ts @@ -3,7 +3,7 @@ import {UserToken} from '@app/keycloak-auth'; import {Injectable} from '@nestjs/common'; import {InjectModel} from '@nestjs/mongoose'; import {FilterQuery, Model, UpdateQuery} from 'mongoose'; -import {generateToken, idFilter} from '../utils'; +import {generateToken} from '../utils'; import {BatchUpdateSolutionDto, CreateSolutionDto, ReadSolutionDto, UpdateSolutionDto} from './solution.dto'; import {Solution, SolutionDocument} from './solution.schema'; @@ -37,11 +37,11 @@ export class SolutionService { } async findOne(id: string): Promise { - return this.model.findOne(idFilter(id)).exec(); + return this.model.findById(id).exec(); } async update(id: string, dto: UpdateSolutionDto): Promise { - const updated = await this.model.findOneAndUpdate(idFilter(id), dto, {new: true}).exec(); + const updated = await this.model.findByIdAndUpdate(id, dto, {new: true}).exec(); updated && this.emit('updated', updated); return updated; } @@ -83,7 +83,7 @@ export class SolutionService { } async remove(id: string): Promise { - const deleted = await this.model.findOneAndDelete(idFilter(id)).exec(); + const deleted = await this.model.findByIdAndDelete(id).exec(); deleted && this.emit('deleted', deleted); return deleted; } diff --git a/services/apps/assignments/src/utils.ts b/services/apps/assignments/src/utils.ts index 7757ce7ed..46578d026 100644 --- a/services/apps/assignments/src/utils.ts +++ b/services/apps/assignments/src/utils.ts @@ -11,10 +11,6 @@ export function generateToken(): string { return new Array(4).fill(0).map((_, index) => hex.substr(index * 4, 4)).join('-'); } -export function idFilter(id: string): FilterQuery { - return isUUID(id) ? {id} : {_id: id}; -} - export function eventStream(source: Observable>, name: string): Observable { return merge( source.pipe( From 1e341b29d513c07a8ae6f93bee82b096889fe61b Mon Sep 17 00:00:00 2001 From: Adrian Kunz Date: Mon, 6 Nov 2023 21:22:51 +0100 Subject: [PATCH 3/4] refactor(assignments-service): Use MongooseRepository for courses --- .../src/course-member/course-auth.guard.ts | 3 +- .../src/course/course.controller.ts | 20 ++++---- .../assignments/src/course/course.service.ts | 47 ++++--------------- 3 files changed, 23 insertions(+), 47 deletions(-) diff --git a/services/apps/assignments/src/course-member/course-auth.guard.ts b/services/apps/assignments/src/course-member/course-auth.guard.ts index e118f8101..f80d78932 100644 --- a/services/apps/assignments/src/course-member/course-auth.guard.ts +++ b/services/apps/assignments/src/course-member/course-auth.guard.ts @@ -5,6 +5,7 @@ import {Observable} from 'rxjs'; import {notFound} from '@mean-stream/nestx'; import {CourseService} from "../course/course.service"; import {MemberService} from "@app/member"; +import {Types} from "mongoose"; @Injectable() export class CourseAuthGuard implements CanActivate { @@ -22,7 +23,7 @@ export class CourseAuthGuard implements CanActivate { } async checkAuth(id: string, user: UserToken): Promise { - const course = await this.courseService.findOne(id) ?? notFound(id); + const course = await this.courseService.find(new Types.ObjectId(id)) ?? notFound(id); return course.createdBy === user.sub || !!await this.memberService.findOne({ parent: course._id, user: user.sub, diff --git a/services/apps/assignments/src/course/course.controller.ts b/services/apps/assignments/src/course/course.controller.ts index 9dc30813d..95977e318 100644 --- a/services/apps/assignments/src/course/course.controller.ts +++ b/services/apps/assignments/src/course/course.controller.ts @@ -1,12 +1,12 @@ import {Auth, AuthUser, UserToken} from '@app/keycloak-auth'; -import {NotFound} from '@mean-stream/nestx'; +import {NotFound, ObjectIdPipe} from '@mean-stream/nestx'; import {Body, Controller, Delete, Get, Param, ParseArrayPipe, Patch, Post, Query} from '@nestjs/common'; import {ApiCreatedResponse, ApiOkResponse, ApiOperation, ApiTags} from '@nestjs/swagger'; import {CourseStudent, CreateCourseDto, UpdateCourseDto} from './course.dto'; import {Course} from './course.schema'; import {CourseService} from './course.service'; import {CourseAuth} from "../course-member/course-auth.decorator"; -import {FilterQuery} from "mongoose"; +import {FilterQuery, Types} from "mongoose"; import {MemberService} from "@app/member"; const forbiddenResponse = 'Not owner.'; @@ -27,7 +27,7 @@ export class CourseController { @Body() dto: CreateCourseDto, @AuthUser() user: UserToken, ): Promise { - return this.courseService.create(dto, user.sub); + return this.courseService.create({...dto, createdBy: user.sub}); } @Get() @@ -50,8 +50,10 @@ export class CourseController { @Get(':id') @NotFound() @ApiOkResponse({type: Course}) - async findOne(@Param('id') id: string): Promise { - return this.courseService.findOne(id); + async findOne( + @Param('id', ObjectIdPipe) id: Types.ObjectId, + ): Promise { + return this.courseService.find(id); } @Get(':id/students') @@ -60,7 +62,7 @@ export class CourseController { @NotFound() @ApiOkResponse({type: [CourseStudent]}) async getStudents( - @Param('id') id: string, + @Param('id', ObjectIdPipe) id: Types.ObjectId, @AuthUser() user: UserToken, ): Promise { return this.courseService.getStudents(id, user.sub); @@ -71,7 +73,7 @@ export class CourseController { @NotFound() @ApiOkResponse({type: Course}) async update( - @Param('id') id: string, + @Param('id', ObjectIdPipe) id: Types.ObjectId, @Body() dto: UpdateCourseDto, ): Promise { return this.courseService.update(id, dto); @@ -82,8 +84,8 @@ export class CourseController { @NotFound() @ApiOkResponse({type: Course}) async remove( - @Param('id') id: string, + @Param('id', ObjectIdPipe) id: Types.ObjectId, ): Promise { - return this.courseService.remove(id); + return this.courseService.delete(id); } } diff --git a/services/apps/assignments/src/course/course.service.ts b/services/apps/assignments/src/course/course.service.ts index 24b295ca4..d1c2c7daf 100644 --- a/services/apps/assignments/src/course/course.service.ts +++ b/services/apps/assignments/src/course/course.service.ts @@ -1,42 +1,27 @@ -import {EventService} from '@mean-stream/nestx'; +import {EventRepository, EventService, MongooseRepository} from '@mean-stream/nestx'; import {Injectable} from '@nestjs/common'; import {InjectModel} from '@nestjs/mongoose'; -import {FilterQuery, Model, Types} from 'mongoose'; +import {Model, Types} from 'mongoose'; import {AuthorInfo} from '../solution/solution.schema'; import {SolutionService} from '../solution/solution.service'; -import {CourseStudent, CreateCourseDto, UpdateCourseDto} from './course.dto'; +import {CourseStudent} from './course.dto'; import {Course, CourseDocument} from './course.schema'; import {MemberService} from "@app/member"; @Injectable() -export class CourseService { +@EventRepository() +export class CourseService extends MongooseRepository { constructor( - @InjectModel(Course.name) private model: Model, + @InjectModel(Course.name) model: Model, private solutionService: SolutionService, private eventService: EventService, private memberService: MemberService, ) { + super(model); } - async create(dto: CreateCourseDto, userId?: string): Promise { - const created = await this.model.create({ - ...dto, - createdBy: userId, - }); - this.emit('created', created); - return created; - } - - async findAll(filter: FilterQuery = {}): Promise { - return this.model.find(filter).exec(); - } - - async findOne(id: string): Promise { - return this.model.findById(id).exec(); - } - - async getStudents(id: string, user: string): Promise { - const course = await this.findOne(id); + async getStudents(id: Types.ObjectId, user: string): Promise { + const course = await this.find(id); if (!course) { return []; } @@ -118,19 +103,7 @@ export class CourseService { return Array.from(new Set(students.values())); } - async update(id: string, dto: UpdateCourseDto): Promise { - const updated = await this.model.findByIdAndUpdate(id, dto, {new: true}).exec(); - updated && this.emit('updated', updated); - return updated; - } - - async remove(id: string): Promise { - const deleted = await this.model.findByIdAndDelete(id).exec(); - deleted && this.emit('deleted', deleted); - return deleted; - } - - private emit(event: string, course: CourseDocument) { + emit(event: string, course: CourseDocument) { this.eventService.emit(`courses.${course.id}.${event}`, course); } } From 1ea1388c1aa478408d4ee209654f3476afabba46 Mon Sep 17 00:00:00 2001 From: Adrian Kunz Date: Tue, 7 Nov 2023 10:30:30 +0100 Subject: [PATCH 4/4] fix(assignments-service): Numeric ordering for assignment sorting --- .../apps/assignments/src/assignment/assignment.service.ts | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/services/apps/assignments/src/assignment/assignment.service.ts b/services/apps/assignments/src/assignment/assignment.service.ts index f74e134e0..e8f5ddb2d 100644 --- a/services/apps/assignments/src/assignment/assignment.service.ts +++ b/services/apps/assignments/src/assignment/assignment.service.ts @@ -42,7 +42,10 @@ export class AssignmentService { } async findAll(where: FilterQuery = {}): Promise { - return this.model.find(where).sort({title: 1}).exec(); + return this.model.find(where).sort({title: 1}).collation({ + locale: 'en', + numericOrdering: true, + }).exec(); } async findOne(id: string): Promise {