Merge pull request #115 from hackaburg/copilot/fix-rating-permission-issues

Fix rating service: permission enforcement, ownership checks, and value validation

Author: sezanzeb

backend/src/controllers/criteria-controller.ts

@@ -47,7 +47,6 @@ export class CriteriaController {
     @Param("id") criteriaId: number,
     @Body() { data: criteriaDTO }: { data: CriteriaDTO },
   ): Promise<CriteriaDTO> {
-    // TODO There is a TeamUpdateDTO. CriteriaUpdateDTO?
     const criteria = convertBetweenEntityAndDTO(criteriaDTO, Criteria);
     const updateCriteria = await this._ratings.updateCriteria(criteria);
     return convertBetweenEntityAndDTO(updateCriteria, CriteriaDTO);

backend/src/controllers/dto.ts

@@ -10,7 +10,9 @@ import {
   IsOptional,
   IsString,
   IsUrl,
+  Max,
   MaxLength,
+  Min,
   MinLength,
   ValidateNested,
 } from "class-validator";
@@ -602,6 +604,8 @@ export class RatingDTO {
   @ValidateNested()
   public critera!: CriteriaDTO;
   @Expose()
-  // 1 - 5
+  @IsInt()
+  @Min(1)
+  @Max(5)
   public rating!: number;
 }

backend/src/controllers/project-controller.ts

@@ -24,7 +24,6 @@ export class ProjectController {
     @Body() { data: projectDTO }: { data: ProjectDTO },
     @CurrentUser() user: User,
   ): Promise<ProjectDTO> {
-    // TODO ProjectUpdateDTO?
     const existing = await this._projects.getProjectByID(projectId);
 
     if (existing == null) {

backend/src/controllers/rating-controller.ts

@@ -36,7 +36,7 @@ export class RatingController {
     @CurrentUser() user: User,
   ): Promise<RatingDTO> {
     const rating = convertBetweenEntityAndDTO(ratingDTO, Rating);
-    const createdRating = await this._ratings.createRating(rating);
+    const createdRating = await this._ratings.createRating(rating, user);
     return convertBetweenEntityAndDTO(createdRating, RatingDTO);
   }
 

backend/src/entities/criteria.ts

@@ -10,5 +10,3 @@ export class Criteria {
   @Longtext()
   public description!: string;
 }
-
-// TODO Rating Project and Criteria DTO

backend/src/services/rating-service.ts

@@ -35,7 +35,7 @@ export interface IRatingService extends IService {
   /**
    * Delete single rating by id
    */
-  deleteRatingByID(id: number, currentUserId: User): Promise<void>;
+  deleteRatingByID(id: number, currentUser: User): Promise<void>;
   //
   //
   /**
@@ -102,20 +102,18 @@ export class RatingService implements IRatingService {
    * @param rating The rating to update
    */
   public async updateRating(rating: Rating, user: User): Promise<Rating> {
-    // TODO validate, throw Errors
-
     const originRating = await this._ratings.findOneBy({ id: rating.id });
 
-    // TODO only if user matches
-    await this.checkPermission(rating, user);
     if (!originRating) {
       throw new NotFoundError("Rating not found");
     }
-    const originRatingUser = originRating.user;
-    if (user.id != originRatingUser.id) {
-      throw new Error("")
+
+    if (user.id !== originRating.user.id) {
+      throw new ForbiddenError("You can only update your own ratings");
     }
 
+    await this.checkPermission(rating, user);
+
     return this._ratings.save(rating);
   }
 
@@ -124,8 +122,7 @@ export class RatingService implements IRatingService {
    * @param rating The rating to create
    */
   public async createRating(rating: Rating, user: User): Promise<Rating> {
-    // TODO validate
-    this.checkPermission(rating, user);
+    await this.checkPermission(rating, user);
     return this._ratings.save(rating);
   }
 
@@ -142,44 +139,43 @@ export class RatingService implements IRatingService {
    * Deletes a rating by its id.
    * @param id The id of the rating
    */
-  public async deleteRatingByID(id: number, currentUserId: User): Promise<void> {
+  public async deleteRatingByID(id: number, currentUser: User): Promise<void> {
     const rating = await this._ratings.findOneBy({ id });
 
     if (!rating) {
       throw new NotFoundError("Rating not found");
     }
 
-    await this.checkPermission(rating, currentUserId);
+    if (currentUser.id !== rating.user.id) {
+      throw new ForbiddenError("You can only delete your own ratings");
+    }
+
+    await this.checkPermission(rating, currentUser);
 
     await this._ratings.delete(id);
 
     return Promise.resolve();
   }
 
   private async checkPermission(rating: Rating, user: User): Promise<void> {
-    // TODO use this._database.blabla instead of _settings and such
-
     const settings = await this._settings.getSettings();
     if (!settings.allowRating) {
-      // TODO test
-      throw new ForbiddenError("Cannot create rating due to application settings")
+      throw new ForbiddenError("Rating is not allowed due to application settings")
     }
 
     const project = await this._projects.findOneBy({ id: rating.project.id });
     if (!project) {
       throw new NotFoundError("Project not found");
     }
     if (!project.allowRating) {
-      // TODO test
-      throw new ForbiddenError("Creating a rating for this project is not allowed")
+      throw new ForbiddenError("Rating this project is not allowed")
     }
 
     const team = await this._teams.findOneBy({ id: project.team.id })
     if (!team) {
       throw new NotFoundError("Team not found");
     }
     if (team.users.includes(user.id)) {
-      // TODO test
       throw new ForbiddenError("You can't rate your own project")
     }
   }