From 65a14da5a918a34f5a063dfc57e5a07c2944ef77 Mon Sep 17 00:00:00 2001 From: Shaun Walker Date: Tue, 15 Jun 2021 19:11:00 -0400 Subject: [PATCH] improve validation and exception handling in API Controllers --- .../Modules/Admin/Dashboard/Index.razor | 4 +- .../Services/Interfaces/IJobLogService.cs | 8 +--- .../Services/Interfaces/ITenantService.cs | 21 --------- Oqtane.Client/Services/JobLogService.cs | 14 ------ Oqtane.Client/Services/TenantService.cs | 15 ------ Oqtane.Server/Controllers/AliasController.cs | 15 ++++-- Oqtane.Server/Controllers/JobController.cs | 46 ++++++++++++++++--- Oqtane.Server/Controllers/JobLogController.cs | 41 +---------------- Oqtane.Server/Controllers/TenantController.cs | 35 -------------- Oqtane.Server/Controllers/ThemeController.cs | 12 +++++ Oqtane.Server/Repository/AliasRepository.cs | 19 ++++++-- .../Repository/Interfaces/IAliasRepository.cs | 3 +- .../Repository/Interfaces/IJobRepository.cs | 3 +- Oqtane.Server/Repository/JobRepository.cs | 15 +++++- 14 files changed, 101 insertions(+), 150 deletions(-) diff --git a/Oqtane.Client/Modules/Admin/Dashboard/Index.razor b/Oqtane.Client/Modules/Admin/Dashboard/Index.razor index 0c7cd842..6c665ba2 100644 --- a/Oqtane.Client/Modules/Admin/Dashboard/Index.razor +++ b/Oqtane.Client/Modules/Admin/Dashboard/Index.razor @@ -2,7 +2,7 @@ @inherits ModuleBase @inject IPageService PageService @inject IUserService UserService -@inject IStringLocalizer Localizer +@inject IStringLocalizer SharedLocalizer
@foreach (var p in _pages) @@ -12,7 +12,7 @@ string url = NavigateUrl(p.Path);
-

@Localizer[p.Name] +

@SharedLocalizer[p.Name]
} diff --git a/Oqtane.Client/Services/Interfaces/IJobLogService.cs b/Oqtane.Client/Services/Interfaces/IJobLogService.cs index 015d83f0..e198bf9f 100644 --- a/Oqtane.Client/Services/Interfaces/IJobLogService.cs +++ b/Oqtane.Client/Services/Interfaces/IJobLogService.cs @@ -1,4 +1,4 @@ -using Oqtane.Models; +using Oqtane.Models; using System.Collections.Generic; using System.Threading.Tasks; @@ -9,11 +9,5 @@ namespace Oqtane.Services Task> GetJobLogsAsync(); Task GetJobLogAsync(int jobLogId); - - Task AddJobLogAsync(JobLog jobLog); - - Task UpdateJobLogAsync(JobLog jobLog); - - Task DeleteJobLogAsync(int jobLogId); } } diff --git a/Oqtane.Client/Services/Interfaces/ITenantService.cs b/Oqtane.Client/Services/Interfaces/ITenantService.cs index 12738f58..cbb4e526 100644 --- a/Oqtane.Client/Services/Interfaces/ITenantService.cs +++ b/Oqtane.Client/Services/Interfaces/ITenantService.cs @@ -21,26 +21,5 @@ namespace Oqtane.Services /// ID-reference of the /// Task GetTenantAsync(int tenantId); - - /// - /// Add / save another to the database - /// - /// A object containing the configuration - /// - Task AddTenantAsync(Tenant tenant); - - /// - /// Update the information in the database. - /// - /// - /// - Task UpdateTenantAsync(Tenant tenant); - - /// - /// Delete / remove a - /// - /// - /// - Task DeleteTenantAsync(int tenantId); } } diff --git a/Oqtane.Client/Services/JobLogService.cs b/Oqtane.Client/Services/JobLogService.cs index 6d9a3e56..6c2f0a73 100644 --- a/Oqtane.Client/Services/JobLogService.cs +++ b/Oqtane.Client/Services/JobLogService.cs @@ -30,19 +30,5 @@ namespace Oqtane.Services { return await GetJsonAsync($"{Apiurl}/{jobLogId}"); } - - public async Task AddJobLogAsync(JobLog joblog) - { - return await PostJsonAsync(Apiurl, joblog); - } - - public async Task UpdateJobLogAsync(JobLog joblog) - { - return await PutJsonAsync($"{Apiurl}/{joblog.JobLogId}", joblog); - } - public async Task DeleteJobLogAsync(int jobLogId) - { - await DeleteAsync($"{Apiurl}/{jobLogId}"); - } } } diff --git a/Oqtane.Client/Services/TenantService.cs b/Oqtane.Client/Services/TenantService.cs index 52b3d345..6c02b9a9 100644 --- a/Oqtane.Client/Services/TenantService.cs +++ b/Oqtane.Client/Services/TenantService.cs @@ -30,20 +30,5 @@ namespace Oqtane.Services { return await GetJsonAsync($"{Apiurl}/{tenantId}"); } - - public async Task AddTenantAsync(Tenant tenant) - { - return await PostJsonAsync(Apiurl, tenant); - } - - public async Task UpdateTenantAsync(Tenant tenant) - { - return await PutJsonAsync($"{Apiurl}/{tenant.TenantId}", tenant); - } - - public async Task DeleteTenantAsync(int tenantId) - { - await DeleteAsync($"{Apiurl}/{tenantId}"); - } } } diff --git a/Oqtane.Server/Controllers/AliasController.cs b/Oqtane.Server/Controllers/AliasController.cs index a61f2ed4..644fa278 100644 --- a/Oqtane.Server/Controllers/AliasController.cs +++ b/Oqtane.Server/Controllers/AliasController.cs @@ -72,7 +72,7 @@ namespace Oqtane.Controllers [Authorize(Roles = RoleNames.Host)] public Alias Put(int id, [FromBody] Alias alias) { - if (ModelState.IsValid) + if (ModelState.IsValid && _aliases.GetAlias(alias.AliasId, false) != null) { alias = _aliases.UpdateAlias(alias); _logger.Log(LogLevel.Information, this, LogFunction.Update, "Alias Updated {Alias}", alias); @@ -91,8 +91,17 @@ namespace Oqtane.Controllers [Authorize(Roles = RoleNames.Host)] public void Delete(int id) { - _aliases.DeleteAlias(id); - _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Alias Deleted {AliasId}", id); + var alias = _aliases.GetAlias(id); + if (alias != null) + { + _aliases.DeleteAlias(id); + _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Alias Deleted {AliasId}", id); + } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Alias Delete Attempt {AliasId}", id); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + } } } } diff --git a/Oqtane.Server/Controllers/JobController.cs b/Oqtane.Server/Controllers/JobController.cs index db4f4bfb..356fd75f 100644 --- a/Oqtane.Server/Controllers/JobController.cs +++ b/Oqtane.Server/Controllers/JobController.cs @@ -9,6 +9,7 @@ using Microsoft.Extensions.DependencyInjection; using Oqtane.Enums; using Oqtane.Infrastructure; using Oqtane.Repository; +using System.Net; namespace Oqtane.Controllers { @@ -52,6 +53,12 @@ namespace Oqtane.Controllers job = _jobs.AddJob(job); _logger.Log(LogLevel.Information, this, LogFunction.Create, "Job Added {Job}", job); } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Job Post Attempt {Alias}", job); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + job = null; + } return job; } @@ -60,11 +67,17 @@ namespace Oqtane.Controllers [Authorize(Roles = RoleNames.Host)] public Job Put(int id, [FromBody] Job job) { - if (ModelState.IsValid) + if (ModelState.IsValid && _jobs.GetJob(job.JobId, false) != null) { job = _jobs.UpdateJob(job); _logger.Log(LogLevel.Information, this, LogFunction.Update, "Job Updated {Job}", job); } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Job Put Attempt {Alias}", job); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + job = null; + } return job; } @@ -73,8 +86,17 @@ namespace Oqtane.Controllers [Authorize(Roles = RoleNames.Host)] public void Delete(int id) { - _jobs.DeleteJob(id); - _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Job Deleted {JobId}", id); + var job = _jobs.GetJob(id); + if (job != null) + { + _jobs.DeleteJob(id); + _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Job Deleted {JobId}", id); + } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Job Delete Attempt {JobId}", id); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + } } // GET api//start @@ -83,12 +105,17 @@ namespace Oqtane.Controllers public void Start(int id) { Job job = _jobs.GetJob(id); - Type jobtype = Type.GetType(job.JobType); - if (jobtype != null) + if (job != null) { + Type jobtype = Type.GetType(job.JobType); var jobobject = ActivatorUtilities.CreateInstance(_serviceProvider, jobtype); ((IHostedService)jobobject).StartAsync(new System.Threading.CancellationToken()); } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Job Start Attempt {JobId}", id); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + } } // GET api//stop @@ -97,12 +124,17 @@ namespace Oqtane.Controllers public void Stop(int id) { Job job = _jobs.GetJob(id); - Type jobtype = Type.GetType(job.JobType); - if (jobtype != null) + if (job != null) { + Type jobtype = Type.GetType(job.JobType); var jobobject = ActivatorUtilities.CreateInstance(_serviceProvider, jobtype); ((IHostedService)jobobject).StopAsync(new System.Threading.CancellationToken()); } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Job Stop Attempt {JobId}", id); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + } } } } diff --git a/Oqtane.Server/Controllers/JobLogController.cs b/Oqtane.Server/Controllers/JobLogController.cs index 39fd8ac9..5f711e4f 100644 --- a/Oqtane.Server/Controllers/JobLogController.cs +++ b/Oqtane.Server/Controllers/JobLogController.cs @@ -1,10 +1,8 @@ using System.Collections.Generic; using Microsoft.AspNetCore.Mvc; using Microsoft.AspNetCore.Authorization; -using Oqtane.Enums; using Oqtane.Models; using Oqtane.Shared; -using Oqtane.Infrastructure; using Oqtane.Repository; namespace Oqtane.Controllers @@ -13,12 +11,10 @@ namespace Oqtane.Controllers public class JobLogController : Controller { private readonly IJobLogRepository _jobLogs; - private readonly ILogManager _logger; - public JobLogController(IJobLogRepository jobLogs, ILogManager logger) + public JobLogController(IJobLogRepository jobLogs) { _jobLogs = jobLogs; - _logger = logger; } // GET: api/ @@ -36,40 +32,5 @@ namespace Oqtane.Controllers { return _jobLogs.GetJobLog(id); } - - // POST api/ - [HttpPost] - [Authorize(Roles = RoleNames.Host)] - public JobLog Post([FromBody] JobLog jobLog) - { - if (ModelState.IsValid) - { - jobLog = _jobLogs.AddJobLog(jobLog); - _logger.Log(LogLevel.Information, this, LogFunction.Create, "Job Log Added {JobLog}", jobLog); - } - return jobLog; - } - - // PUT api//5 - [HttpPut("{id}")] - [Authorize(Roles = RoleNames.Host)] - public JobLog Put(int id, [FromBody] JobLog jobLog) - { - if (ModelState.IsValid) - { - jobLog = _jobLogs.UpdateJobLog(jobLog); - _logger.Log(LogLevel.Information, this, LogFunction.Update, "Job Log Updated {JobLog}", jobLog); - } - return jobLog; - } - - // DELETE api//5 - [HttpDelete("{id}")] - [Authorize(Roles = RoleNames.Host)] - public void Delete(int id) - { - _jobLogs.DeleteJobLog(id); - _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Job Log Deleted {JobLogId}", id); - } } } diff --git a/Oqtane.Server/Controllers/TenantController.cs b/Oqtane.Server/Controllers/TenantController.cs index 6f6fb4e8..1b01d0f1 100644 --- a/Oqtane.Server/Controllers/TenantController.cs +++ b/Oqtane.Server/Controllers/TenantController.cs @@ -36,40 +36,5 @@ namespace Oqtane.Controllers { return _tenants.GetTenant(id); } - - // POST api/ - [HttpPost] - [Authorize(Roles = RoleNames.Host)] - public Tenant Post([FromBody] Tenant tenant) - { - if (ModelState.IsValid) - { - tenant = _tenants.AddTenant(tenant); - _logger.Log(LogLevel.Information, this, LogFunction.Create, "Tenant Added {TenantId}", tenant.TenantId); - } - return tenant; - } - - // PUT api//5 - [HttpPut("{id}")] - [Authorize(Roles = RoleNames.Host)] - public Tenant Put(int id, [FromBody] Tenant tenant) - { - if (ModelState.IsValid) - { - tenant = _tenants.UpdateTenant(tenant); - _logger.Log(LogLevel.Information, this, LogFunction.Update, "Tenant Updated {TenantId}", tenant.TenantId); - } - return tenant; - } - - // DELETE api//5 - [HttpDelete("{id}")] - [Authorize(Roles = RoleNames.Host)] - public void Delete(int id) - { - _tenants.DeleteTenant(id); - _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Tenant Deleted {TenantId}", id); - } } } diff --git a/Oqtane.Server/Controllers/ThemeController.cs b/Oqtane.Server/Controllers/ThemeController.cs index 7c05b0ba..dd8bae6f 100644 --- a/Oqtane.Server/Controllers/ThemeController.cs +++ b/Oqtane.Server/Controllers/ThemeController.cs @@ -11,6 +11,7 @@ using Oqtane.Enums; using Oqtane.Infrastructure; using Oqtane.Repository; using System.Text.Json; +using System.Net; // ReSharper disable StringIndexOfIsCultureSpecific.1 @@ -84,6 +85,11 @@ namespace Oqtane.Controllers _themes.DeleteTheme(theme.ThemeName); _logger.Log(LogLevel.Information, this, LogFunction.Delete, "Theme Removed For {ThemeName}", theme.ThemeName); } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Theme Delete Attempt {Themename}", themename); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + } } // GET: api//templates @@ -141,6 +147,12 @@ namespace Oqtane.Controllers ProcessTemplatesRecursively(new DirectoryInfo(templatePath), rootPath, rootFolder.Name, templatePath, theme); _logger.Log(LogLevel.Information, this, LogFunction.Create, "Theme Created {Theme}", theme); } + else + { + _logger.Log(LogLevel.Error, this, LogFunction.Security, "Unauthorized Theme Post Attempt {Theme}", theme); + HttpContext.Response.StatusCode = (int)HttpStatusCode.Forbidden; + theme = null; + } return theme; } diff --git a/Oqtane.Server/Repository/AliasRepository.cs b/Oqtane.Server/Repository/AliasRepository.cs index 619724dc..9e4e82d9 100644 --- a/Oqtane.Server/Repository/AliasRepository.cs +++ b/Oqtane.Server/Repository/AliasRepository.cs @@ -45,15 +45,28 @@ namespace Oqtane.Repository public Alias GetAlias(int aliasId) { - return _db.Alias.Find(aliasId); + return GetAlias(aliasId, true); } - public Alias GetAlias(string name) + public Alias GetAlias(int aliasId, bool tracking) + { + if (tracking) + { + return _db.Alias.Find(aliasId); + } + else + { + return _db.Alias.AsNoTracking().FirstOrDefault(item => item.AliasId == aliasId); + } + } + + // lookup alias based on url - note that alias values are hierarchical + public Alias GetAlias(string url) { Alias alias = null; List aliases = GetAliases().ToList(); - var segments = name.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); + var segments = url.Split(new[] { '/' }, StringSplitOptions.RemoveEmptyEntries); // iterate segments to find keywords int start = segments.Length; diff --git a/Oqtane.Server/Repository/Interfaces/IAliasRepository.cs b/Oqtane.Server/Repository/Interfaces/IAliasRepository.cs index 5422ab07..81a42d68 100644 --- a/Oqtane.Server/Repository/Interfaces/IAliasRepository.cs +++ b/Oqtane.Server/Repository/Interfaces/IAliasRepository.cs @@ -9,7 +9,8 @@ namespace Oqtane.Repository Alias AddAlias(Alias alias); Alias UpdateAlias(Alias alias); Alias GetAlias(int aliasId); - Alias GetAlias(string name); + Alias GetAlias(int aliasId, bool tracking); + Alias GetAlias(string url); void DeleteAlias(int aliasId); } } diff --git a/Oqtane.Server/Repository/Interfaces/IJobRepository.cs b/Oqtane.Server/Repository/Interfaces/IJobRepository.cs index 12be85c1..e45b0231 100644 --- a/Oqtane.Server/Repository/Interfaces/IJobRepository.cs +++ b/Oqtane.Server/Repository/Interfaces/IJobRepository.cs @@ -1,4 +1,4 @@ -using System.Collections.Generic; +using System.Collections.Generic; using Oqtane.Models; namespace Oqtane.Repository @@ -9,6 +9,7 @@ namespace Oqtane.Repository Job AddJob(Job job); Job UpdateJob(Job job); Job GetJob(int jobId); + Job GetJob(int jobId, bool tracking); void DeleteJob(int jobId); } } diff --git a/Oqtane.Server/Repository/JobRepository.cs b/Oqtane.Server/Repository/JobRepository.cs index cbe51c19..37b38521 100644 --- a/Oqtane.Server/Repository/JobRepository.cs +++ b/Oqtane.Server/Repository/JobRepository.cs @@ -1,4 +1,4 @@ -using System; +using System; using System.Collections.Generic; using System.Linq; using Microsoft.EntityFrameworkCore; @@ -48,6 +48,19 @@ namespace Oqtane.Repository return _db.Job.Find(jobId); } + public Job GetJob(int jobId, bool tracking) + { + if (tracking) + { + return _db.Job.Find(jobId); + } + else + { + return _db.Job.AsNoTracking().FirstOrDefault(item => item.JobId == jobId); + } + + } + public void DeleteJob(int jobId) { Job job = _db.Job.Find(jobId);