fix(permissions): 🐛 Improved the security around the role "Manage Own Requests" (#4397)

* Secure ManageOwnRequests API paths

Fixes #4391

* Hide delete request option if user is not allowed

* Refactor CheckOwnRequests

* Fix deleteRequest test

* Improve performance and clean up code

* Fix manageOwnRequests check

* Refactor CheckCanManageRequest

src/Ombi.Core/Engine/BaseMediaEngine.cs

@@ -78,6 +78,32 @@ namespace Ombi.Core.Engine
             return _dbTv;
         }
 
+        protected async Task<RequestEngineResult> CheckCanManageRequest(BaseRequest request) {
+            var errorResult = new RequestEngineResult {
+                Result = false,
+                ErrorCode = ErrorCode.NoPermissions
+            };
+            var successResult = new RequestEngineResult { Result = true };
+            
+            // Admins can always manage requests
+            var isAdmin = await IsInRole(OmbiRoles.PowerUser) || await IsInRole(OmbiRoles.Admin);
+            if (isAdmin) {
+                return successResult;
+            }
+
+            // Users with 'ManageOwnRequests' can only manage their own requests
+            var canManageOwnRequests = await IsInRole(OmbiRoles.ManageOwnRequests);
+            if (!canManageOwnRequests) {
+                return errorResult;
+            }
+            var isRequestedBySameUser = ( await GetUser() ).Id == request.RequestedUser?.Id;
+            if (isRequestedBySameUser) {
+                return successResult;
+            }
+
+            return errorResult;
+        }
+
         public RequestCountModel RequestCount()
         {
             var movieQuery = MovieRepository.GetAll();

src/Ombi.Core/Engine/IMusicRequestEngine.cs

@@ -18,7 +18,7 @@ namespace Ombi.Core.Engine
         Task<int> GetTotal();
         Task<RequestEngineResult> MarkAvailable(int modelId);
         Task<RequestEngineResult> MarkUnavailable(int modelId);
-        Task RemoveAlbumRequest(int requestId);
+        Task<RequestEngineResult> RemoveAlbumRequest(int requestId);
         Task<RequestEngineResult> RequestAlbum(MusicAlbumRequestViewModel model);
         Task<IEnumerable<AlbumRequest>> SearchAlbumRequest(string search);
         Task<bool> UserHasRequest(string userId);

src/Ombi.Core/Engine/Interfaces/IMovieRequestEngine.cs

@@ -14,7 +14,7 @@ namespace Ombi.Core.Engine.Interfaces
         Task<IEnumerable<MovieRequests>> SearchMovieRequest(string search);
         Task<RequestEngineResult> RequestCollection(int collectionId, CancellationToken cancellationToken);
 
-        Task RemoveMovieRequest(int requestId);
+        Task<RequestEngineResult> RemoveMovieRequest(int requestId);
         Task RemoveAllMovieRequests();
         Task<MovieRequests> GetRequest(int requestId);
         Task<MovieRequests> UpdateMovieRequest(MovieRequests request);

src/Ombi.Core/Engine/Interfaces/ITvRequestEngine.cs

@@ -20,7 +20,7 @@ namespace Ombi.Core.Engine.Interfaces
         Task<TvRequests> UpdateTvRequest(TvRequests request);
         Task<IEnumerable<ChildRequests>> GetAllChldren(int tvId);
         Task<ChildRequests> UpdateChildRequest(ChildRequests request);
-        Task RemoveTvChild(int requestId);
+        Task<RequestEngineResult> RemoveTvChild(int requestId);
         Task<RequestEngineResult> ApproveChildRequest(int id);
         Task<IEnumerable<TvRequests>> GetRequestsLite();
         Task UpdateQualityProfile(int requestId, int profileId);

src/Ombi.Core/Engine/MovieRequestEngine.cs

@@ -654,11 +654,20 @@ namespace Ombi.Core.Engine
         /// </summary>
         /// <param name="requestId">The request identifier.</param>
         /// <returns></returns>
-        public async Task RemoveMovieRequest(int requestId)
+        public async Task<RequestEngineResult> RemoveMovieRequest(int requestId)
         {
             var request = await MovieRepository.GetAll().FirstOrDefaultAsync(x => x.Id == requestId);
+
+            var result = await CheckCanManageRequest(request);
+            if (result.IsError)
+                return result;
+                
             await MovieRepository.Delete(request);
             await _mediaCacheService.Purge();
+            return new RequestEngineResult
+            {
+                Result = true,
+            };
         }
 
         public async Task RemoveAllMovieRequests()

src/Ombi.Core/Engine/MusicRequestEngine.cs

@@ -404,10 +404,20 @@ namespace Ombi.Core.Engine
         /// </summary>
         /// <param name="requestId">The request identifier.</param>
         /// <returns></returns>
-        public async Task RemoveAlbumRequest(int requestId)
+        public async Task<RequestEngineResult> RemoveAlbumRequest(int requestId)
         {
             var request = await MusicRepository.GetAll().FirstOrDefaultAsync(x => x.Id == requestId);
+            
+            var result = await CheckCanManageRequest(request);
+            if (result.IsError)
+                return result;
+
             await MusicRepository.Delete(request);
+
+            return new RequestEngineResult
+            {
+                Result = true,
+            };
         }
 
         public async Task<bool> UserHasRequest(string userId)

src/Ombi.Core/Engine/RequestEngineResult.cs

@@ -7,7 +7,7 @@ namespace Ombi.Core.Engine
     {
         public bool Result { get; set; }
         public string Message { get; set; }
-        public bool IsError => !string.IsNullOrEmpty(ErrorMessage);
+        public bool IsError => ( !string.IsNullOrEmpty(ErrorMessage) || ErrorCode != null );
         public string ErrorMessage { get; set; }
         public ErrorCode? ErrorCode { get; set; }
         public int RequestId { get; set; }

src/Ombi.Core/Engine/TvRequestEngine.cs

@@ -749,10 +749,14 @@ namespace Ombi.Core.Engine
             return request;
         }
 
-        public async Task RemoveTvChild(int requestId)
+        public async Task<RequestEngineResult> RemoveTvChild(int requestId)
         {
             var request = await TvRepository.GetChild().FirstOrDefaultAsync(x => x.Id == requestId);
 
+            var result = await CheckCanManageRequest(request);
+            if (result.IsError)
+                return result;
+
             TvRepository.Db.ChildRequests.Remove(request);
             var all = TvRepository.Db.TvRequests.Include(x => x.ChildRequests);
             var parent = all.FirstOrDefault(x => x.Id == request.ParentRequestId);
@@ -766,6 +770,11 @@ namespace Ombi.Core.Engine
 
             await TvRepository.Db.SaveChangesAsync();
             await _mediaCacheService.Purge();
+
+            return new RequestEngineResult
+            {
+                Result = true,
+            };
         }
 
         public async Task RemoveTvRequest(int requestId)

src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.html

@@ -60,7 +60,7 @@
             <th mat-header-cell *matHeaderCellDef> </th>
             <td mat-cell *matCellDef="let element">
                 <button mat-raised-button color="accent" [routerLink]="'/details/artist/' + element.foreignArtistId">{{ 'Requests.Details' | translate}}</button>
-                <button mat-raised-button color="warn" (click)="openOptions(element)" *ngIf="isAdmin || manageOwnRequests"> {{ 'Requests.Options' | translate}}</button>
+                <button mat-raised-button color="warn" (click)="openOptions(element)" *ngIf="isAdmin || ( manageOwnRequests && element.requestedUser?.userName == userName )"> {{ 'Requests.Options' | translate}}</button>
             </td>
         </ng-container>
 

src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.ts

@@ -26,6 +26,7 @@ export class AlbumsGridComponent implements OnInit, AfterViewInit {
     public defaultOrder: string = "desc";
     public currentFilter: RequestFilterType = RequestFilterType.All;
     public manageOwnRequests: boolean;
+    public userName: string;
 
     public RequestFilter = RequestFilterType;
 
@@ -43,6 +44,7 @@ export class AlbumsGridComponent implements OnInit, AfterViewInit {
     constructor(private requestService: RequestServiceV2, private ref: ChangeDetectorRef,
                 private auth: AuthService, private storageService: StorageService) {
 
+        this.userName = auth.claims().name;
     }
 
     public ngOnInit() {

src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.html

@@ -76,7 +76,7 @@
             <th mat-header-cell *matHeaderCellDef> </th>
             <td mat-cell *matCellDef="let element">
                 <button id="detailsButton{{element.id}}" mat-raised-button color="accent" [routerLink]="'/details/movie/' + element.theMovieDbId">{{ 'Requests.Details' | translate}}</button>
-                <button id="optionsButton{{element.id}}" mat-raised-button color="warn" (click)="openOptions(element)" *ngIf="isAdmin || manageOwnRequests"> {{ 'Requests.Options' | translate}}</button>
+                <button id="optionsButton{{element.id}}" mat-raised-button color="warn" (click)="openOptions(element)" *ngIf="isAdmin || ( manageOwnRequests && element.requestedUser?.userName == userName ) "> {{ 'Requests.Options' | translate}}</button>
             </td>
         </ng-container>
 

src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.ts

@@ -31,6 +31,7 @@ export class MoviesGridComponent implements OnInit, AfterViewInit {
     public defaultOrder: string = "desc";
     public currentFilter: RequestFilterType = RequestFilterType.All;
     public selection = new SelectionModel<IMovieRequests>(true, []);
+    public userName: string;
 
     public RequestFilter = RequestFilterType;
 
@@ -46,10 +47,11 @@ export class MoviesGridComponent implements OnInit, AfterViewInit {
     @ViewChild(MatSort) sort: MatSort;
 
     constructor(private requestService: RequestServiceV2, private ref: ChangeDetectorRef,
-                private auth: AuthService, private storageService: StorageService,
-                private requestServiceV1: RequestService, private notification: NotificationService,
-                private translateService: TranslateService) {
+        private auth: AuthService, private storageService: StorageService,
+        private requestServiceV1: RequestService, private notification: NotificationService,
+        private translateService: TranslateService) {
 
+        this.userName = auth.claims().name;
     }
 
     public ngOnInit() {

src/Ombi/ClientApp/src/app/requests-list/components/options/request-options.component.ts

@@ -1,8 +1,10 @@
 import { Component, Inject } from '@angular/core';
 import { MAT_BOTTOM_SHEET_DATA, MatBottomSheetRef } from '@angular/material/bottom-sheet';
-import { RequestService } from '../../../services';
-import { RequestType } from '../../../interfaces';
+import { MessageService, RequestService } from '../../../services';
+import { IRequestEngineResult, RequestType } from '../../../interfaces';
 import { UpdateType } from '../../models/UpdateType';
+import { TranslateService } from '@ngx-translate/core';
+import { Observable } from 'rxjs';
 
 @Component({
   selector: 'request-options',
@@ -13,21 +15,31 @@ export class RequestOptionsComponent {
   public RequestType = RequestType;
 
   constructor(@Inject(MAT_BOTTOM_SHEET_DATA) public data: any,
-    private requestService: RequestService, private bottomSheetRef: MatBottomSheetRef<RequestOptionsComponent>) { }
+    private requestService: RequestService, 
+    private messageService: MessageService,
+    private bottomSheetRef: MatBottomSheetRef<RequestOptionsComponent>,
+    private translate: TranslateService) { }
 
   public async delete() {
+    var  request: Observable<IRequestEngineResult>;
     if (this.data.type === RequestType.movie) {
-      await this.requestService.removeMovieRequestAsync(this.data.id);
+      request = this.requestService.removeMovieRequestAsync(this.data.id);
     }
     if (this.data.type === RequestType.tvShow) {
-      await this.requestService.deleteChild(this.data.id).toPromise();
+      request = this.requestService.deleteChild(this.data.id);
     }
     if (this.data.type === RequestType.album) {
-      await this.requestService.removeAlbumRequest(this.data.id).toPromise();
+      request = this.requestService.removeAlbumRequest(this.data.id);
     }
-
-    this.bottomSheetRef.dismiss({type: UpdateType.Delete});
-    return;
+    request.subscribe(result => {
+      if (result.result) {
+          this.messageService.send(this.translate.instant("Requests.SuccessfullyDeleted"));
+          this.bottomSheetRef.dismiss({type: UpdateType.Delete});
+          return;
+      } else {
+          this.messageService.sendRequestEngineResultError(result);
+      }
+    });
   }
 
   public async approve() {

src/Ombi/ClientApp/src/app/services/request.service.ts

@@ -73,8 +73,8 @@ export class RequestService extends ServiceHelpers {
         this.http.delete(`${this.url}movie/${requestId}`, {headers: this.headers}).subscribe();
     }
 
-    public removeMovieRequestAsync(requestId: number) {
-        return this.http.delete(`${this.url}movie/${requestId}`, {headers: this.headers}).toPromise();
+    public removeMovieRequestAsync(requestId: number): Observable<IRequestEngineResult> {
+        return this.http.delete<IRequestEngineResult>(`${this.url}movie/${requestId}`, {headers: this.headers});
     }
 
     public updateMovieRequest(request: IMovieRequests): Observable<IMovieRequests> {
@@ -129,8 +129,8 @@ export class RequestService extends ServiceHelpers {
         return this.http.post<IRequestEngineResult>(`${this.url}tv/approve`, JSON.stringify(child), {headers: this.headers});
     }
 
-    public deleteChild(childId: number): Observable<boolean> {
-        return this.http.delete<boolean>(`${this.url}tv/child/${childId}`, {headers: this.headers});
+    public deleteChild(childId: number): Observable<IRequestEngineResult> {
+        return this.http.delete<IRequestEngineResult>(`${this.url}tv/child/${childId}`, {headers: this.headers});
     }
 
     public subscribeToMovie(requestId: number): Observable<boolean> {
@@ -185,7 +185,7 @@ export class RequestService extends ServiceHelpers {
         return this.http.get<IAlbumRequest[]>(`${this.url}music/search/${search}`, {headers: this.headers});
     }
 
-    public removeAlbumRequest(request: number): any {
-        return this.http.delete(`${this.url}music/${request}`, {headers: this.headers});
+    public removeAlbumRequest(request: number): Observable<IRequestEngineResult> {
+        return this.http.delete<IRequestEngineResult>(`${this.url}music/${request}`, {headers: this.headers});
     }
 }

src/Ombi/Controllers/V1/MusicRequestController.cs

@@ -113,9 +113,9 @@ namespace Ombi.Controllers.V1
         /// <returns></returns>
         [HttpDelete("{requestId:int}")]
         [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")]
-        public async Task DeleteRequest(int requestId)
+        public async Task<RequestEngineResult> DeleteRequest(int requestId)
         {
-            await _engine.RemoveAlbumRequest(requestId);
+            return await _engine.RemoveAlbumRequest(requestId);
         }
 
         /// <summary>

src/Ombi/Controllers/V1/RequestController.cs

@@ -128,9 +128,9 @@ namespace Ombi.Controllers.V1
         /// <returns></returns>
         [HttpDelete("movie/{requestId:int}")]
         [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")]
-        public async Task DeleteRequest(int requestId)
+        public async Task<RequestEngineResult> DeleteRequest(int requestId)
         {
-            await MovieRequestEngine.RemoveMovieRequest(requestId);
+            return await MovieRequestEngine.RemoveMovieRequest(requestId);
         }
 
         /// <summary>
@@ -324,7 +324,7 @@ namespace Ombi.Controllers.V1
         /// <param name="requestId">The request identifier.</param>
         /// <returns></returns>
         [HttpDelete("tv/{requestId:int}")]
-        [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")]
+        [Authorize(Roles = "Admin,PowerUser")]
         public async Task DeleteTvRequest(int requestId)
         {
             await TvRequestEngine.RemoveTvRequest(requestId);
@@ -437,10 +437,9 @@ namespace Ombi.Controllers.V1
         /// <returns></returns>
         [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")]
         [HttpDelete("tv/child/{requestId:int}")]
-        public async Task<bool> DeleteChildRequest(int requestId)
+        public async Task<RequestEngineResult> DeleteChildRequest(int requestId)
         {
-            await TvRequestEngine.RemoveTvChild(requestId);
-            return true;
+            return await TvRequestEngine.RemoveTvChild(requestId);
         }
 
 

src/Ombi/wwwroot/translations/en.json

@@ -198,6 +198,7 @@
             "Approved": "Successfully approved selected items"
         },
         "SuccessfullyApproved": "Successfully Approved",
+        "SuccessfullyDeleted": "Request successfully deleted",
         "NowAvailable": "Request is now available",
         "NowUnavailable": "Request is now unavailable",
         "SuccessfullyReprocessed": "Successfully Re-processed the request",

tests/cypress/tests/requests/requests.spec.ts

@@ -39,7 +39,7 @@ describe("Requests Tests", () => {
     row.optionsDelete.click();
 
     cy.wait('@deleteRequest').then((intercept) => {
-      expect(intercept.response.body).is.true;
+      expect(intercept.response.body.result).is.true;
     })
 
     row.title.should('not.exist');