From eb7e2781c10a1186b25a145edf04ae1ea3ea2517 Mon Sep 17 00:00:00 2001 From: Joseph Milazzo Date: Thu, 20 Jan 2022 07:46:59 -0800 Subject: [PATCH] Validate Download Claim (#971) * Partially complete, got some code to validate your Role. Needs to be applied to all methods and made a filter. * Cleaned up the code on the backend to validate each call. The reason the RequireDownloadRole doesn't work is that the user still has the claim in their token so the simple validation isn't working. We need explicit checks. * Don't allow users to download files if they have lost the claim but not refreshed token. * Don't allow users to download files if they have lost the claim but not refreshed token. --- API/Controllers/DownloadController.cs | 26 ++++++++++++++----- API/Data/Metadata/ComicInfo.cs | 11 -------- API/Errors/ApiException.cs | 2 +- API/Extensions/IdentityServiceExtensions.cs | 1 + .../app/_interceptors/error.interceptor.ts | 12 ++++++++- .../app/shared/_services/download.service.ts | 4 +-- 6 files changed, 35 insertions(+), 21 deletions(-) diff --git a/API/Controllers/DownloadController.cs b/API/Controllers/DownloadController.cs index c253fb9ee..09388d3af 100644 --- a/API/Controllers/DownloadController.cs +++ b/API/Controllers/DownloadController.cs @@ -4,6 +4,7 @@ using System.IO; using System.Linq; using System.Threading.Tasks; using API.Comparators; +using API.Constants; using API.Data; using API.DTOs.Downloads; using API.Entities; @@ -13,33 +14,32 @@ using API.Services; using API.SignalR; using Kavita.Common; using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Identity; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.SignalR; namespace API.Controllers { - [Authorize(Policy = "RequireDownloadRole")] + [Authorize(Policy="RequireDownloadRole")] public class DownloadController : BaseApiController { private readonly IUnitOfWork _unitOfWork; private readonly IArchiveService _archiveService; private readonly IDirectoryService _directoryService; - private readonly ICacheService _cacheService; private readonly IDownloadService _downloadService; private readonly IHubContext _messageHub; - private readonly NumericComparer _numericComparer; + private readonly UserManager _userManager; private const string DefaultContentType = "application/octet-stream"; public DownloadController(IUnitOfWork unitOfWork, IArchiveService archiveService, IDirectoryService directoryService, - ICacheService cacheService, IDownloadService downloadService, IHubContext messageHub) + IDownloadService downloadService, IHubContext messageHub, UserManager userManager) { _unitOfWork = unitOfWork; _archiveService = archiveService; _directoryService = directoryService; - _cacheService = cacheService; _downloadService = downloadService; _messageHub = messageHub; - _numericComparer = new NumericComparer(); + _userManager = userManager; } [HttpGet("volume-size")] @@ -63,9 +63,12 @@ namespace API.Controllers return Ok(_directoryService.GetTotalSize(files.Select(c => c.FilePath))); } + [Authorize(Policy="RequireDownloadRole")] [HttpGet("volume")] public async Task DownloadVolume(int volumeId) { + if (!await HasDownloadPermission()) return BadRequest("You do not have permission"); + var files = await _unitOfWork.VolumeRepository.GetFilesForVolume(volumeId); var volume = await _unitOfWork.VolumeRepository.GetVolumeByIdAsync(volumeId); var series = await _unitOfWork.SeriesRepository.GetSeriesByIdAsync(volume.SeriesId); @@ -79,6 +82,13 @@ namespace API.Controllers } } + private async Task HasDownloadPermission() + { + var user = await _unitOfWork.UserRepository.GetUserByUsernameAsync(User.GetUsername()); + var roles = await _userManager.GetRolesAsync(user); + return roles.Contains(PolicyConstants.DownloadRole) || roles.Contains(PolicyConstants.AdminRole); + } + private async Task GetFirstFileDownload(IEnumerable files) { var (bytes, contentType, fileDownloadName) = await _downloadService.GetFirstFileDownload(files); @@ -88,6 +98,7 @@ namespace API.Controllers [HttpGet("chapter")] public async Task DownloadChapter(int chapterId) { + if (!await HasDownloadPermission()) return BadRequest("You do not have permission"); var files = await _unitOfWork.ChapterRepository.GetFilesForChapterAsync(chapterId); var chapter = await _unitOfWork.ChapterRepository.GetChapterAsync(chapterId); var volume = await _unitOfWork.VolumeRepository.GetVolumeByIdAsync(chapter.VolumeId); @@ -120,6 +131,7 @@ namespace API.Controllers [HttpGet("series")] public async Task DownloadSeries(int seriesId) { + if (!await HasDownloadPermission()) return BadRequest("You do not have permission"); var files = await _unitOfWork.SeriesRepository.GetFilesForSeries(seriesId); var series = await _unitOfWork.SeriesRepository.GetSeriesByIdAsync(seriesId); try @@ -135,6 +147,8 @@ namespace API.Controllers [HttpPost("bookmarks")] public async Task DownloadBookmarkPages(DownloadBookmarkDto downloadBookmarkDto) { + if (!await HasDownloadPermission()) return BadRequest("You do not have permission"); + // We know that all bookmarks will be for one single seriesId var user = await _unitOfWork.UserRepository.GetUserByUsernameAsync(User.GetUsername()); var series = await _unitOfWork.SeriesRepository.GetSeriesByIdAsync(downloadBookmarkDto.Bookmarks.First().SeriesId); diff --git a/API/Data/Metadata/ComicInfo.cs b/API/Data/Metadata/ComicInfo.cs index cc7154f93..0f213d848 100644 --- a/API/Data/Metadata/ComicInfo.cs +++ b/API/Data/Metadata/ComicInfo.cs @@ -103,17 +103,6 @@ namespace API.Data.Metadata info.Characters = Parser.Parser.CleanAuthor(info.Characters); info.Translator = Parser.Parser.CleanAuthor(info.Translator); info.CoverArtist = Parser.Parser.CleanAuthor(info.CoverArtist); - - - // if (!string.IsNullOrEmpty(info.Web)) - // { - // // ComicVine stores the Issue number in Number field and does not use Volume. - // if (!info.Web.Contains("https://comicvine.gamespot.com/")) return; - // if (info.Volume.Equals("1")) - // { - // info.Volume = Parser.Parser.DefaultVolume; - // } - // } } diff --git a/API/Errors/ApiException.cs b/API/Errors/ApiException.cs index ce1792f72..1d570e8ff 100644 --- a/API/Errors/ApiException.cs +++ b/API/Errors/ApiException.cs @@ -13,4 +13,4 @@ Details = details; } } -} \ No newline at end of file +} diff --git a/API/Extensions/IdentityServiceExtensions.cs b/API/Extensions/IdentityServiceExtensions.cs index 1d0638e67..cf2ca4635 100644 --- a/API/Extensions/IdentityServiceExtensions.cs +++ b/API/Extensions/IdentityServiceExtensions.cs @@ -4,6 +4,7 @@ using API.Constants; using API.Data; using API.Entities; using Microsoft.AspNetCore.Authentication.JwtBearer; +using Microsoft.AspNetCore.Authorization.Infrastructure; using Microsoft.AspNetCore.Identity; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; diff --git a/UI/Web/src/app/_interceptors/error.interceptor.ts b/UI/Web/src/app/_interceptors/error.interceptor.ts index a924b6b2e..6e568020b 100644 --- a/UI/Web/src/app/_interceptors/error.interceptor.ts +++ b/UI/Web/src/app/_interceptors/error.interceptor.ts @@ -83,6 +83,10 @@ export class ErrorInterceptor implements HttpInterceptor { } else { console.error('error:', error); if (error.statusText === 'Bad Request') { + if (error.error instanceof Blob) { + this.toastr.error('There was an issue downloading this file or you do not have permissions', error.status); + return; + } this.toastr.error(error.error, error.status); } else { this.toastr.error(error.statusText === 'OK' ? error.error : error.statusText, error.status); @@ -101,7 +105,13 @@ export class ErrorInterceptor implements HttpInterceptor { console.log('500 error: ', error); } this.toastr.error(err.message); - } else { + } else if (error.hasOwnProperty('message') && error.message.trim() !== '') { + if (error.message != 'User is not authenticated') { + console.log('500 error: ', error); + } + this.toastr.error(error.message); + } + else { this.toastr.error('There was an unknown critical error.'); console.error('500 error:', error); } diff --git a/UI/Web/src/app/shared/_services/download.service.ts b/UI/Web/src/app/shared/_services/download.service.ts index 882d789c8..b2ed45285 100644 --- a/UI/Web/src/app/shared/_services/download.service.ts +++ b/UI/Web/src/app/shared/_services/download.service.ts @@ -1,4 +1,4 @@ -import { HttpClient } from '@angular/common/http'; +import { HttpClient, HttpErrorResponse } from '@angular/common/http'; import { Inject, Injectable } from '@angular/core'; import { Series } from 'src/app/_models/series'; import { environment } from 'src/environments/environment'; @@ -10,7 +10,7 @@ import { asyncScheduler, Observable } from 'rxjs'; import { SAVER, Saver } from '../_providers/saver.provider'; import { download, Download } from '../_models/download'; import { PageBookmark } from 'src/app/_models/page-bookmark'; -import { throttleTime } from 'rxjs/operators'; +import { catchError, throttleTime } from 'rxjs/operators'; const DEBOUNCE_TIME = 100;