From 334a32bca42f90198d9b720d2bdb710a583be47f Mon Sep 17 00:00:00 2001 From: sephrat <34862846+sephrat@users.noreply.github.com> Date: Thu, 11 Nov 2021 11:21:44 +0100 Subject: [PATCH] fix(permissions): :bug: 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 | 26 ++++++++++++++++ src/Ombi.Core/Engine/IMusicRequestEngine.cs | 2 +- .../Engine/Interfaces/IMovieRequestEngine.cs | 2 +- .../Engine/Interfaces/ITvRequestEngine.cs | 2 +- src/Ombi.Core/Engine/MovieRequestEngine.cs | 11 ++++++- src/Ombi.Core/Engine/MusicRequestEngine.cs | 12 +++++++- src/Ombi.Core/Engine/RequestEngineResult.cs | 2 +- src/Ombi.Core/Engine/TvRequestEngine.cs | 11 ++++++- .../albums-grid/albums-grid.component.html | 2 +- .../albums-grid/albums-grid.component.ts | 2 ++ .../movies-grid/movies-grid.component.html | 2 +- .../movies-grid/movies-grid.component.ts | 8 +++-- .../options/request-options.component.ts | 30 +++++++++++++------ .../src/app/services/request.service.ts | 12 ++++---- .../Controllers/V1/MusicRequestController.cs | 4 +-- src/Ombi/Controllers/V1/RequestController.cs | 11 ++++--- src/Ombi/wwwroot/translations/en.json | 1 + tests/cypress/tests/requests/requests.spec.ts | 2 +- 18 files changed, 106 insertions(+), 36 deletions(-) diff --git a/src/Ombi.Core/Engine/BaseMediaEngine.cs b/src/Ombi.Core/Engine/BaseMediaEngine.cs index 91f8a58b0..277144ab1 100644 --- a/src/Ombi.Core/Engine/BaseMediaEngine.cs +++ b/src/Ombi.Core/Engine/BaseMediaEngine.cs @@ -78,6 +78,32 @@ namespace Ombi.Core.Engine return _dbTv; } + protected async Task 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(); diff --git a/src/Ombi.Core/Engine/IMusicRequestEngine.cs b/src/Ombi.Core/Engine/IMusicRequestEngine.cs index 4dbb3b16a..fcdb06496 100644 --- a/src/Ombi.Core/Engine/IMusicRequestEngine.cs +++ b/src/Ombi.Core/Engine/IMusicRequestEngine.cs @@ -18,7 +18,7 @@ namespace Ombi.Core.Engine Task GetTotal(); Task MarkAvailable(int modelId); Task MarkUnavailable(int modelId); - Task RemoveAlbumRequest(int requestId); + Task RemoveAlbumRequest(int requestId); Task RequestAlbum(MusicAlbumRequestViewModel model); Task> SearchAlbumRequest(string search); Task UserHasRequest(string userId); diff --git a/src/Ombi.Core/Engine/Interfaces/IMovieRequestEngine.cs b/src/Ombi.Core/Engine/Interfaces/IMovieRequestEngine.cs index dfcd1b1da..7cc64bee2 100644 --- a/src/Ombi.Core/Engine/Interfaces/IMovieRequestEngine.cs +++ b/src/Ombi.Core/Engine/Interfaces/IMovieRequestEngine.cs @@ -14,7 +14,7 @@ namespace Ombi.Core.Engine.Interfaces Task> SearchMovieRequest(string search); Task RequestCollection(int collectionId, CancellationToken cancellationToken); - Task RemoveMovieRequest(int requestId); + Task RemoveMovieRequest(int requestId); Task RemoveAllMovieRequests(); Task GetRequest(int requestId); Task UpdateMovieRequest(MovieRequests request); diff --git a/src/Ombi.Core/Engine/Interfaces/ITvRequestEngine.cs b/src/Ombi.Core/Engine/Interfaces/ITvRequestEngine.cs index c93403b3e..6fdf52c56 100644 --- a/src/Ombi.Core/Engine/Interfaces/ITvRequestEngine.cs +++ b/src/Ombi.Core/Engine/Interfaces/ITvRequestEngine.cs @@ -20,7 +20,7 @@ namespace Ombi.Core.Engine.Interfaces Task UpdateTvRequest(TvRequests request); Task> GetAllChldren(int tvId); Task UpdateChildRequest(ChildRequests request); - Task RemoveTvChild(int requestId); + Task RemoveTvChild(int requestId); Task ApproveChildRequest(int id); Task> GetRequestsLite(); Task UpdateQualityProfile(int requestId, int profileId); diff --git a/src/Ombi.Core/Engine/MovieRequestEngine.cs b/src/Ombi.Core/Engine/MovieRequestEngine.cs index 63aa0d376..0d0234fc6 100644 --- a/src/Ombi.Core/Engine/MovieRequestEngine.cs +++ b/src/Ombi.Core/Engine/MovieRequestEngine.cs @@ -654,11 +654,20 @@ namespace Ombi.Core.Engine /// /// The request identifier. /// - public async Task RemoveMovieRequest(int requestId) + public async Task 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() diff --git a/src/Ombi.Core/Engine/MusicRequestEngine.cs b/src/Ombi.Core/Engine/MusicRequestEngine.cs index d0a9cc5d7..b9aa183d2 100644 --- a/src/Ombi.Core/Engine/MusicRequestEngine.cs +++ b/src/Ombi.Core/Engine/MusicRequestEngine.cs @@ -404,10 +404,20 @@ namespace Ombi.Core.Engine /// /// The request identifier. /// - public async Task RemoveAlbumRequest(int requestId) + public async Task 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 UserHasRequest(string userId) diff --git a/src/Ombi.Core/Engine/RequestEngineResult.cs b/src/Ombi.Core/Engine/RequestEngineResult.cs index 08c61b7ae..a2f4f93f4 100644 --- a/src/Ombi.Core/Engine/RequestEngineResult.cs +++ b/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; } diff --git a/src/Ombi.Core/Engine/TvRequestEngine.cs b/src/Ombi.Core/Engine/TvRequestEngine.cs index fd306f951..c05a23a48 100644 --- a/src/Ombi.Core/Engine/TvRequestEngine.cs +++ b/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 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) diff --git a/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.html b/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.html index 4fa687149..977b55a17 100644 --- a/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.html +++ b/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.html @@ -60,7 +60,7 @@ - + diff --git a/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.ts b/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.ts index e8f2899c7..d0529e018 100644 --- a/src/Ombi/ClientApp/src/app/requests-list/components/albums-grid/albums-grid.component.ts +++ b/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() { diff --git a/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.html b/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.html index 0e6e31521..61baa3eaa 100644 --- a/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.html +++ b/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.html @@ -76,7 +76,7 @@ - + diff --git a/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.ts b/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.ts index c033238b1..cad5a6c60 100644 --- a/src/Ombi/ClientApp/src/app/requests-list/components/movies-grid/movies-grid.component.ts +++ b/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(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() { diff --git a/src/Ombi/ClientApp/src/app/requests-list/components/options/request-options.component.ts b/src/Ombi/ClientApp/src/app/requests-list/components/options/request-options.component.ts index 625efe31c..aa98943ca 100644 --- a/src/Ombi/ClientApp/src/app/requests-list/components/options/request-options.component.ts +++ b/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) { } + private requestService: RequestService, + private messageService: MessageService, + private bottomSheetRef: MatBottomSheetRef, + private translate: TranslateService) { } public async delete() { + var request: Observable; 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() { diff --git a/src/Ombi/ClientApp/src/app/services/request.service.ts b/src/Ombi/ClientApp/src/app/services/request.service.ts index 7eca20955..d850f5d83 100644 --- a/src/Ombi/ClientApp/src/app/services/request.service.ts +++ b/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 { + return this.http.delete(`${this.url}movie/${requestId}`, {headers: this.headers}); } public updateMovieRequest(request: IMovieRequests): Observable { @@ -129,8 +129,8 @@ export class RequestService extends ServiceHelpers { return this.http.post(`${this.url}tv/approve`, JSON.stringify(child), {headers: this.headers}); } - public deleteChild(childId: number): Observable { - return this.http.delete(`${this.url}tv/child/${childId}`, {headers: this.headers}); + public deleteChild(childId: number): Observable { + return this.http.delete(`${this.url}tv/child/${childId}`, {headers: this.headers}); } public subscribeToMovie(requestId: number): Observable { @@ -185,7 +185,7 @@ export class RequestService extends ServiceHelpers { return this.http.get(`${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 { + return this.http.delete(`${this.url}music/${request}`, {headers: this.headers}); } } diff --git a/src/Ombi/Controllers/V1/MusicRequestController.cs b/src/Ombi/Controllers/V1/MusicRequestController.cs index df9d2f406..289c7cca1 100644 --- a/src/Ombi/Controllers/V1/MusicRequestController.cs +++ b/src/Ombi/Controllers/V1/MusicRequestController.cs @@ -113,9 +113,9 @@ namespace Ombi.Controllers.V1 /// [HttpDelete("{requestId:int}")] [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")] - public async Task DeleteRequest(int requestId) + public async Task DeleteRequest(int requestId) { - await _engine.RemoveAlbumRequest(requestId); + return await _engine.RemoveAlbumRequest(requestId); } /// diff --git a/src/Ombi/Controllers/V1/RequestController.cs b/src/Ombi/Controllers/V1/RequestController.cs index 961969b6f..cc686b506 100644 --- a/src/Ombi/Controllers/V1/RequestController.cs +++ b/src/Ombi/Controllers/V1/RequestController.cs @@ -128,9 +128,9 @@ namespace Ombi.Controllers.V1 /// [HttpDelete("movie/{requestId:int}")] [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")] - public async Task DeleteRequest(int requestId) + public async Task DeleteRequest(int requestId) { - await MovieRequestEngine.RemoveMovieRequest(requestId); + return await MovieRequestEngine.RemoveMovieRequest(requestId); } /// @@ -324,7 +324,7 @@ namespace Ombi.Controllers.V1 /// The request identifier. /// [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 /// [Authorize(Roles = "Admin,PowerUser,ManageOwnRequests")] [HttpDelete("tv/child/{requestId:int}")] - public async Task DeleteChildRequest(int requestId) + public async Task DeleteChildRequest(int requestId) { - await TvRequestEngine.RemoveTvChild(requestId); - return true; + return await TvRequestEngine.RemoveTvChild(requestId); } diff --git a/src/Ombi/wwwroot/translations/en.json b/src/Ombi/wwwroot/translations/en.json index ad9ee1ba2..8c3107d77 100644 --- a/src/Ombi/wwwroot/translations/en.json +++ b/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", diff --git a/tests/cypress/tests/requests/requests.spec.ts b/tests/cypress/tests/requests/requests.spec.ts index 23c5ccb09..fc1a7489c 100644 --- a/tests/cypress/tests/requests/requests.spec.ts +++ b/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');