From c8c2f4932d142ddf13d109c241436c5b1bd92b96 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ren=C3=A9=20Schultz=20Madsen?= Date: Fri, 18 Sep 2026 15:05:42 +0200 Subject: [PATCH] fix(security): scope the working-hours export and grid to the caller MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both the single-worker Excel export and the working-hours grid took a site id straight from the request and never checked whether the caller was entitled to it. Any signed-in user could read a colleague's hours by guessing a site id: the grid returned 200 with their rows, and the export handed back their workbook. A test pins both. Both now resolve the caller through SiteScopeResolver, the helper the all-workers export already uses, and refuse anything outside that scope: an admin reaches every site, a manager their own plus the sites in their managed tags, a worker only their own. Refusals are deliberately indistinguishable. An unknown site id is in nobody's allow-set, an admin's included, so it answers exactly as an out-of-scope one does — same message, same status, and the narrowing happens in memory so the work does not vary with the id either. Without that, the two answers differed and could be used to enumerate which site ids exist. Index is now a gate over a private IndexUnscoped. The all-workers export calls the unscoped body directly because it has already narrowed its site ids, and gating the shared body would re-resolve the caller's scope once per site in the workbook. Two admin-visible changes, both on requests that produce no data today: a bogus site id now answers "Worker not found." instead of the crash-path message, as does a site that has plan registrations but no live assignment. Co-Authored-By: Claude Opus 5 --- .../DagsoversigtWorksheetExportTests.cs | 4 +- .../ExportTagFilterAndSiteTagsTests.cs | 319 +++++++++++++++++- .../MobileFlexRecomputeAndCascadeTests.cs | 9 + .../ReconcileServiceTests.cs | 6 +- .../TimePlanning.Pn.Test/TestBaseSetup.cs | 23 +- .../WorkingHoursDisplayParityTests.cs | 6 +- .../WorkingHoursExcelExportE2ETests.cs | 6 +- .../WorkingHoursExcelExportTagsColumnTests.cs | 4 +- .../WorkingHoursExcelHolidayColumnTests.cs | 4 +- .../WorkingHoursExcelShiftColumnOrderTests.cs | 4 +- .../TimePlanningWorkingHoursService.cs | 120 ++++++- 11 files changed, 474 insertions(+), 31 deletions(-) diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs index 8a0fd3a5..52801f00 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/DagsoversigtWorksheetExportTests.cs @@ -11,7 +11,6 @@ using Microting.eForm.Infrastructure.Constants; using Microting.eFormApi.BasePn.Abstractions; using Microting.EformAngularFrontendBase.Infrastructure.Data; -using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; using Microting.eFormApi.BasePn.Infrastructure.Helpers.PluginDbOptions; using Microting.TimePlanningBase.Infrastructure.Data.Entities; using NSubstitute; @@ -80,8 +79,7 @@ public async Task SetUpTest() // The all-workers export scopes its site list to the signed-in caller, // so these fixtures need a real one. Admin: scoping is a no-op. - var adminUserId = await GetBaseDbContextWithAdminAsync(); - userService.GetCurrentUserAsync().Returns(new EformUser { Id = adminUserId }); + await SeedAdminCallerAsync(userService); _service = new TimePlanningWorkingHoursService( Substitute.For>(), diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ExportTagFilterAndSiteTagsTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ExportTagFilterAndSiteTagsTests.cs index 16e5fd0d..21ab09d2 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ExportTagFilterAndSiteTagsTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ExportTagFilterAndSiteTagsTests.cs @@ -1,6 +1,7 @@ using System; using System.Collections.Generic; using System.Globalization; +using System.IO; using System.Linq; using System.Threading.Tasks; using DocumentFormat.OpenXml.Packaging; @@ -19,6 +20,7 @@ using Microting.eFormApi.BasePn.Abstractions; using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; using Microting.eFormApi.BasePn.Infrastructure.Helpers.PluginDbOptions; +using Microting.eFormApi.BasePn.Infrastructure.Models.API; using Microting.EformAngularFrontendBase.Infrastructure.Data; using NSubstitute; using NUnit.Framework; @@ -102,8 +104,7 @@ public async Task SetUpTest() // test needs one. The default is an admin, for whom scoping is a no-op; // the scoping tests below seed a second, narrower caller into the same // context and re-point the substitute at it. - var adminUserId = await GetBaseDbContextWithAdminAsync(); - _userService.GetCurrentUserAsync().Returns(new EformUser { Id = adminUserId }); + await SeedAdminCallerAsync(_userService); _workingHoursService = new TimePlanningWorkingHoursService( Substitute.For>(), @@ -511,10 +512,324 @@ public async Task GetSiteTags_ManagerUser_ReturnsOnlySitesInTheirManagedTags() "A manager sees the sites in their managed tags plus their own — and nothing else"); } + // ------------------------------------------------------------------ + // 4. Single-worker export: the requested SiteId must be in the caller's + // scope. It arrives straight from the query string, so an unscoped + // overload let any signed-in user export any worker by guessing an id. + // ------------------------------------------------------------------ + + /// + /// The counterpart that decides whether the scoping is safe to ship: an + /// admin still exports any site they ask for, exactly as before. + /// + [Test] + public async Task SingleWorkerExport_AdminUser_CanExportAnySite() + { + var date = new DateTime(2026, 7, 26); + await SeedSiteAndPlanRegistration(siteUid: 9701, employeeNo: "1", date: date); + // A site belonging to somebody else entirely, and a manager elsewhere in + // the system: neither may narrow an ADMIN's reach. + await SeedSiteAndPlanRegistration( + siteUid: 9702, employeeNo: "2", date: date, email: "other9702@example.com", isManager: true); + + // The caller stays the admin seeded in SetUp. + await AssertExportSucceeded( + await SingleWorkerExport(9701, date), + "An admin must still export any site"); + await AssertExportSucceeded( + await SingleWorkerExport(9702, date), + "Scoping must be a no-op for an admin — including for another manager's site"); + } + + /// + /// A manager's reach on this endpoint is the same one the planning board + /// gives them: the sites carrying the tags they manage, plus their own. + /// + [Test] + public async Task SingleWorkerExport_ManagerUser_CanExportTheirOwnSiteAndOneInTheirManagedTags() + { + var date = new DateTime(2026, 7, 27); + await SeedManagerWithManagedTag( + email: "manager9711@example.com", + managerSiteUid: 9711, managedSiteUid: 9712, outsideSiteUid: 9713, date: date); + + await AssertExportSucceeded( + await SingleWorkerExport(9711, date), + "A manager must be able to export their own site"); + await AssertExportSucceeded( + await SingleWorkerExport(9712, date), + "A manager must be able to export a worker inside a tag they manage"); + } + + [Test] + public async Task SingleWorkerExport_ManagerUser_RefusesASiteOutsideTheirManagedTags() + { + var date = new DateTime(2026, 7, 28); + + // Seeded BEFORE the caller is switched, so the export below is first + // proved to work for the admin. Without that, a refusal could just as + // well mean the site was never exportable in this fixture. + await SeedSiteAndPlanRegistration(siteUid: 9723, employeeNo: "3", date: date); + await AssertExportSucceeded( + await SingleWorkerExport(9723, date), + "Precondition: the site must be exportable at all"); + + await SeedManagerWithManagedTag( + email: "manager9721@example.com", + managerSiteUid: 9721, managedSiteUid: 9722, outsideSiteUid: 9723, date: date, + seedOutsideSite: false); + + AssertExportRefused( + await SingleWorkerExport(9723, date), + "A site outside the manager's tags is not on their page and must not be exportable either"); + } + + /// + /// A plain worker sees exactly one row on the planning board. Unscoped, they + /// could download any colleague's hours by editing the site id in the URL. + /// + [Test] + public async Task SingleWorkerExport_PlainWorker_ExportsOwnSiteButIsRefusedAnother() + { + const string email = "worker9731@example.com"; + var date = new DateTime(2026, 7, 29); + await SeedSiteAndPlanRegistration( + siteUid: 9731, employeeNo: "1", date: date, email: email); + await SeedSiteAndPlanRegistration(siteUid: 9732, employeeNo: "2", date: date); + + // Same reasoning as above: prove the colleague's site is exportable + // before asserting that this caller cannot export it. + await AssertExportSucceeded( + await SingleWorkerExport(9732, date), + "Precondition: the colleague's site must be exportable at all"); + + await SeedNonAdminCallerAsync(email); + + await AssertExportSucceeded( + await SingleWorkerExport(9731, date), + "A worker must still be able to export their own hours"); + AssertExportRefused( + await SingleWorkerExport(9732, date), + "A plain worker must not be able to export a colleague by guessing their site id"); + } + + /// + /// The refusal must be blind. Out-of-scope and "no such site" answer with + /// the same body, or the difference between them IS an oracle: a caller + /// walks the id space and learns which workers exist. + /// + [Test] + public async Task SingleWorkerExport_UnknownSiteIdAndOutOfScopeSiteId_AnswerIdentically() + { + const string email = "worker9741@example.com"; + var date = new DateTime(2026, 7, 30); + await SeedSiteAndPlanRegistration( + siteUid: 9741, employeeNo: "1", date: date, email: email); + await SeedSiteAndPlanRegistration(siteUid: 9742, employeeNo: "2", date: date); + + await SeedNonAdminCallerAsync(email); + + var outOfScope = await SingleWorkerExport(9742, date); + // 999_941 is deliberately in no table at all. + var unknown = await SingleWorkerExport(999_941, date); + + AssertExportRefused(outOfScope, "A colleague's site must be refused"); + AssertExportRefused(unknown, "An id that names no site must be refused"); + Assert.That(unknown.Message, Is.EqualTo(outOfScope.Message), + "A real-but-forbidden id and a non-existent id must be indistinguishable, " + + "or the caller can enumerate which site ids exist"); + } + + // ------------------------------------------------------------------ + // 5. The same scope on the JSON grid behind POST working-hours/index. + // The export reads its rows from here, so leaving this open would + // hand back as JSON exactly what the export now refuses. + // ------------------------------------------------------------------ + + [Test] + public async Task WorkingHoursIndex_AdminUser_CanReadAnySite() + { + var date = new DateTime(2026, 8, 3); + await SeedSiteAndPlanRegistration(siteUid: 9751, employeeNo: "1", date: date); + await SeedSiteAndPlanRegistration( + siteUid: 9752, employeeNo: "2", date: date, email: "other9752@example.com", isManager: true); + + // The caller stays the admin seeded in SetUp. + AssertIndexSucceeded(await WorkingHoursIndex(9751, date), "An admin must still read any site"); + AssertIndexSucceeded(await WorkingHoursIndex(9752, date), + "Scoping must be a no-op for an admin — including for another manager's site"); + } + + [Test] + public async Task WorkingHoursIndex_ManagerUser_ReadsTheirOwnSiteAndOneInTheirManagedTags() + { + var date = new DateTime(2026, 8, 4); + await SeedManagerWithManagedTag( + email: "manager9761@example.com", + managerSiteUid: 9761, managedSiteUid: 9762, outsideSiteUid: 9763, date: date); + + AssertIndexSucceeded(await WorkingHoursIndex(9761, date), "A manager must be able to read their own site"); + AssertIndexSucceeded(await WorkingHoursIndex(9762, date), + "A manager must be able to read a worker inside a tag they manage"); + } + + [Test] + public async Task WorkingHoursIndex_ManagerUser_RefusesASiteOutsideTheirManagedTags() + { + var date = new DateTime(2026, 8, 5); + + // Seeded and read as the admin first, so the refusal below cannot be + // explained by the site simply not being readable in this fixture. + await SeedSiteAndPlanRegistration(siteUid: 9773, employeeNo: "3", date: date); + AssertIndexSucceeded(await WorkingHoursIndex(9773, date), + "Precondition: the site must be readable at all"); + + await SeedManagerWithManagedTag( + email: "manager9771@example.com", + managerSiteUid: 9771, managedSiteUid: 9772, outsideSiteUid: 9773, date: date, + seedOutsideSite: false); + + AssertIndexRefused(await WorkingHoursIndex(9773, date), + "A site outside the manager's tags is not on their page and must not be readable either"); + } + + [Test] + public async Task WorkingHoursIndex_PlainWorker_ReadsOwnSiteButIsRefusedAnother() + { + const string email = "worker9781@example.com"; + var date = new DateTime(2026, 8, 6); + await SeedSiteAndPlanRegistration( + siteUid: 9781, employeeNo: "1", date: date, email: email); + await SeedSiteAndPlanRegistration(siteUid: 9782, employeeNo: "2", date: date); + + AssertIndexSucceeded(await WorkingHoursIndex(9782, date), + "Precondition: the colleague's site must be readable at all"); + + await SeedNonAdminCallerAsync(email); + + AssertIndexSucceeded(await WorkingHoursIndex(9781, date), + "A worker must still be able to read their own hours"); + AssertIndexRefused(await WorkingHoursIndex(9782, date), + "A plain worker must not be able to read a colleague by guessing their site id"); + } + + /// The same blindness the export owes, on the JSON route. + [Test] + public async Task WorkingHoursIndex_UnknownSiteIdAndOutOfScopeSiteId_AnswerIdentically() + { + const string email = "worker9791@example.com"; + var date = new DateTime(2026, 8, 7); + await SeedSiteAndPlanRegistration( + siteUid: 9791, employeeNo: "1", date: date, email: email); + await SeedSiteAndPlanRegistration(siteUid: 9792, employeeNo: "2", date: date); + + await SeedNonAdminCallerAsync(email); + + var outOfScope = await WorkingHoursIndex(9792, date); + var unknown = await WorkingHoursIndex(999_991, date); + + AssertIndexRefused(outOfScope, "A colleague's site must be refused"); + AssertIndexRefused(unknown, "An id that names no site must be refused"); + Assert.That(unknown.Message, Is.EqualTo(outOfScope.Message), + "A real-but-forbidden id and a non-existent id must be indistinguishable, " + + "or the caller can enumerate which site ids exist"); + } + // ------------------------------------------------------------------ // Helpers // ------------------------------------------------------------------ + /// Runs the working-hours grid for one day — the JSON route the + /// export reads its rows from. + private async Task>> WorkingHoursIndex( + int siteUid, DateTime date) => + await _workingHoursService.Index( + new TimePlanningWorkingHoursRequestModel + { + SiteId = siteUid, + DateFrom = date, + DateTo = date, + }); + + private static void AssertIndexSucceeded( + OperationDataResult> result, string because) + { + Assert.That(result.Success, Is.True, $"{because} (message: {result.Message})"); + Assert.That(result.Model, Is.Not.Null, because); + } + + /// Pins the refusal itself: without the message assertion this + /// would also pass on the catch-all ErrorWhileObtainingPlannings a crash + /// returns, which is the very body the oracle tests forbid. + private static void AssertIndexRefused( + OperationDataResult> result, string because) + { + Assert.That(result.Success, Is.False, because); + Assert.That(result.Message, Is.EqualTo("SiteNotFound"), + "The refusal must be the plugin's own not-found message, not a generic read failure"); + Assert.That(result.Model, Is.Null, "A refused read must hand back no rows at all"); + } + + /// Runs the single-worker export for one day and hands back the raw + /// result, so a test can assert on a refusal as well as on a workbook. + private async Task> SingleWorkerExport(int siteUid, DateTime date) => + await _workingHoursService.GenerateExcelDashboard( + new TimePlanningWorkingHoursRequestModel + { + SiteId = siteUid, + DateFrom = date, + DateTo = date, + }); + + private static async Task AssertExportSucceeded(OperationDataResult result, string because) + { + Assert.That(result.Success, Is.True, $"{because} (message: {result.Message})"); + Assert.That(result.Model, Is.Not.Null, because); + await result.Model!.DisposeAsync(); + } + + /// Pins the refusal itself, not merely "not a success": asserting on + /// the message separates a deliberate scope refusal from the catch-all + /// ErrorWhileCreatingExcelFile a crash would return. (The fixture's + /// localization substitute echoes the key it is handed.) + private static void AssertExportRefused(OperationDataResult result, string because) + { + Assert.That(result.Success, Is.False, because); + Assert.That(result.Message, Is.EqualTo("SiteNotFound"), + "The refusal must be the plugin's own not-found message, not a generic export failure"); + Assert.That(result.Model, Is.Null, "A refused export must hand back no workbook at all"); + } + + /// Seeds a manager who manages the tag "EL", a site carrying it and + /// a site carrying "Brand" instead, then points the caller at the manager. + /// Pass false when the caller has already + /// seeded that site itself. + private async Task SeedManagerWithManagedTag( + string email, int managerSiteUid, int managedSiteUid, int outsideSiteUid, DateTime date, + bool seedOutsideSite = true) + { + var managerAssignedSite = await SeedSiteAndPlanRegistration( + siteUid: managerSiteUid, employeeNo: "1", date: date, email: email, isManager: true); + await SeedSiteAndPlanRegistration(siteUid: managedSiteUid, employeeNo: "2", date: date); + if (seedOutsideSite) + { + await SeedSiteAndPlanRegistration(siteUid: outsideSiteUid, employeeNo: "3", date: date); + } + + var elTagId = await TagSiteByUid(managedSiteUid, "EL"); + await TagSiteByUid(outsideSiteUid, "Brand"); + await new AssignedSiteManagingTagEntity + { + AssignedSiteId = managerAssignedSite.Id, + TagId = elTagId, + WorkflowState = Constants.WorkflowStates.Created, + CreatedByUserId = 1, + UpdatedByUserId = 1, + }.Create(TimePlanningPnDbContext!); + + await SeedNonAdminCallerAsync(email); + } + /// Seeds a second, non-admin eform user into the fixture's /// BaseDbContext and points the IUserService substitute at it, so the next /// service call resolves that caller instead of the default admin. Whether diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/MobileFlexRecomputeAndCascadeTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/MobileFlexRecomputeAndCascadeTests.cs index 09d4a245..321d67d9 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/MobileFlexRecomputeAndCascadeTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/MobileFlexRecomputeAndCascadeTests.cs @@ -59,6 +59,15 @@ public async Task SetUpTest() var sdkDb = core.DbContextHelper.GetDbContext(); // --- SDK graph: site + worker + siteworker, keyed by the user's email --- + // + // The email keying is load-bearing beyond mere lookup: this fixture's + // EformUser carries no admin role and no security groups, so the + // working-hours grid resolves it through the PLAIN-WORKER branch of the + // caller scope — own site only, named by the SDK Worker that matches + // this email. The Index call below asks for SiteUid, the very site this + // worker is linked to, which is the only reason it is not refused. + // Break the email match, or link the worker to a different site, and + // every scoped call in this fixture starts failing with "SiteNotFound". var language = await sdkDb.Languages.FirstOrDefaultAsync(l => l.LanguageCode == "da"); if (language == null) { diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs index 30d3cedd..68b370e6 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/ReconcileServiceTests.cs @@ -826,6 +826,10 @@ async Task Stored(PlanRegistration row) => [Test] public async Task WorkingHoursIndex_MarksReconciledLockedDaysAsIsLocked() { + // Index scopes the requested site to the signed-in caller; this seeds + // the admin user and points _userService at it ("me"). + await using var baseDbContext = GetBaseDbContext(); + await BuildAdminIndexServiceAsync(baseDbContext); await SeedAssignedSiteAsync(930); // Keep the MaxDaysEditable window out of the way, so only the // reconciled lock can set IsLocked on these past days. @@ -835,7 +839,7 @@ public async Task WorkingHoursIndex_MarksReconciledLockedDaysAsIsLocked() await SeedReconciledBoundaryAsync(930, DateTime.Now.Date.AddDays(-3)); await SeedPlain(930, DateTime.Now.Date.AddDays(-1)); - var result = await BuildWorkingHoursService().Index(new TimePlanningWorkingHoursRequestModel + var result = await BuildWorkingHoursService(baseDbContext).Index(new TimePlanningWorkingHoursRequestModel { SiteId = 930, DateFrom = DateTime.Now.Date.AddDays(-10), diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/TestBaseSetup.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/TestBaseSetup.cs index 6e0a0b3f..3480a845 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/TestBaseSetup.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/TestBaseSetup.cs @@ -5,9 +5,11 @@ using eFormCore; using Microsoft.EntityFrameworkCore; using Microting.eForm.Infrastructure; +using Microting.eFormApi.BasePn.Abstractions; using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; using Microting.EformAngularFrontendBase.Infrastructure.Data; using Microting.TimePlanningBase.Infrastructure.Data; +using NSubstitute; using NUnit.Framework; using Testcontainers.MariaDb; using TimePlanning.Pn.Infrastructure.Data.Seed; @@ -114,10 +116,9 @@ protected BaseDbContext GetBaseDbContext() /// /// Seeds a holding one admin user and /// returns that user's id. Services that scope their result to the signed-in - /// caller — the planning board, the site-tags lookup, the all-workers - /// export — need a real caller to resolve; this is the admin caller, for - /// whom scoping is a no-op. Point the IUserService substitute's - /// GetCurrentUserAsync at the returned id. + /// caller — the planning board, the site-tags lookup, the working-hours grid, + /// both exports — need a real caller to resolve; this is the admin caller, + /// for whom scoping is a no-op. /// protected async Task GetBaseDbContextWithAdminAsync(string email = "admin@example.com") { @@ -144,6 +145,20 @@ protected async Task GetBaseDbContextWithAdminAsync(string email = "admin@e return user.Id; } + /// + /// Seeds the admin caller AND points at it — + /// the two halves belong together, because a service handed + /// without a matching + /// GetCurrentUserAsync resolves no caller at all and every scoped + /// call fails. + /// + protected async Task SeedAdminCallerAsync( + IUserService userService, string email = "admin@example.com") + { + var adminUserId = await GetBaseDbContextWithAdminAsync(email); + userService.GetCurrentUserAsync().Returns(new EformUser { Id = adminUserId }); + } + /// /// The connection string of the plugin database /// migrates, for tests that must build a context the way production does diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursDisplayParityTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursDisplayParityTests.cs index 642b8358..8fab00b7 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursDisplayParityTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursDisplayParityTests.cs @@ -72,12 +72,16 @@ public async Task SetUpTest() SnapshotEnabled = "0" }); + // Index scopes the requested site to the signed-in caller, so this + // fixture needs a real one. Admin: scoping is a no-op. + await SeedAdminCallerAsync(userService); + _service = new TimePlanningWorkingHoursService( Substitute.For>(), TimePlanningPnDbContext!, userService, localizationService, - baseDbContext: null!, + baseDbContext: SeededBaseDbContext!, options, coreService); } diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportE2ETests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportE2ETests.cs index 3418f40b..b556e478 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportE2ETests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportE2ETests.cs @@ -74,12 +74,16 @@ public async Task SetUpTest() SnapshotEnabled = "0" }); + // The export scopes the requested site to the signed-in caller, so this + // fixture needs a real one. Admin: scoping is a no-op. + await SeedAdminCallerAsync(userService); + _service = new TimePlanningWorkingHoursService( Substitute.For>(), TimePlanningPnDbContext!, userService, localizationService, - baseDbContext: null!, + baseDbContext: SeededBaseDbContext!, options, coreService); } diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportTagsColumnTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportTagsColumnTests.cs index 8753e51e..1d204aff 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportTagsColumnTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelExportTagsColumnTests.cs @@ -10,7 +10,6 @@ using Microting.eForm.Infrastructure.Constants; using Microting.eFormApi.BasePn.Abstractions; using Microting.EformAngularFrontendBase.Infrastructure.Data; -using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; using Microting.eFormApi.BasePn.Infrastructure.Helpers.PluginDbOptions; using NSubstitute; using NUnit.Framework; @@ -81,8 +80,7 @@ public async Task SetUpTest() // The all-workers export scopes its site list to the signed-in caller, // so these fixtures need a real one. Admin: scoping is a no-op. - var adminUserId = await GetBaseDbContextWithAdminAsync(); - userService.GetCurrentUserAsync().Returns(new EformUser { Id = adminUserId }); + await SeedAdminCallerAsync(userService); _service = new TimePlanningWorkingHoursService( Substitute.For>(), diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelHolidayColumnTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelHolidayColumnTests.cs index 3f511a9e..ebd697d1 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelHolidayColumnTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelHolidayColumnTests.cs @@ -10,7 +10,6 @@ using Microting.eForm.Infrastructure.Constants; using Microting.eFormApi.BasePn.Abstractions; using Microting.EformAngularFrontendBase.Infrastructure.Data; -using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; using Microting.eFormApi.BasePn.Infrastructure.Helpers.PluginDbOptions; using NSubstitute; using NUnit.Framework; @@ -108,8 +107,7 @@ public async Task SetUpTest() // The all-workers export scopes its site list to the signed-in caller, // so these fixtures need a real one. Admin: scoping is a no-op. - var adminUserId = await GetBaseDbContextWithAdminAsync(); - userService.GetCurrentUserAsync().Returns(new EformUser { Id = adminUserId }); + await SeedAdminCallerAsync(userService); _service = new TimePlanningWorkingHoursService( Substitute.For>(), diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelShiftColumnOrderTests.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelShiftColumnOrderTests.cs index 8b886e57..d7e1320f 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelShiftColumnOrderTests.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn.Test/WorkingHoursExcelShiftColumnOrderTests.cs @@ -10,7 +10,6 @@ using Microting.eForm.Infrastructure.Constants; using Microting.eFormApi.BasePn.Abstractions; using Microting.EformAngularFrontendBase.Infrastructure.Data; -using Microting.eFormApi.BasePn.Infrastructure.Database.Entities; using Microting.eFormApi.BasePn.Infrastructure.Helpers.PluginDbOptions; using NSubstitute; using NUnit.Framework; @@ -104,8 +103,7 @@ public async Task SetUpTest() // The all-workers export scopes its site list to the signed-in caller, // so these fixtures need a real one. Admin: scoping is a no-op. - var adminUserId = await GetBaseDbContextWithAdminAsync(); - userService.GetCurrentUserAsync().Returns(new EformUser { Id = adminUserId }); + await SeedAdminCallerAsync(userService); _service = new TimePlanningWorkingHoursService( Substitute.For>(), diff --git a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs index b5eb4268..5d3999a0 100644 --- a/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs +++ b/eFormAPI/Plugins/TimePlanning.Pn/TimePlanning.Pn/Services/TimePlanningWorkingHoursService/TimePlanningWorkingHoursService.cs @@ -30,6 +30,7 @@ OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE using DocumentFormat.OpenXml.Packaging; using DocumentFormat.OpenXml.Spreadsheet; using DocumentFormat.OpenXml.Validation; +using Microting.eForm.Infrastructure; using Microting.eForm.Infrastructure.Data.Entities; using Microting.EformAngularFrontendBase.Infrastructure.Data; using Microting.TimePlanningBase.Infrastructure.Data.Entities; @@ -69,8 +70,88 @@ public class TimePlanningWorkingHoursService( IEFormCoreService coreHelper) : ITimePlanningWorkingHoursService { + /// + /// The set of sites the signed-in caller may see — the same scope the + /// planning board and the export dialog's worker count apply. Check + /// before reading anything else. + /// + private async Task ResolveScopeAsync(MicrotingDbContext sdkContext) + { + var assignedSites = await dbContext.AssignedSites + .AsNoTracking() + .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) + .ToListAsync(); + return await SiteScopeResolver + .ResolveForCurrentUserAsync(assignedSites, dbContext, sdkContext, baseDbContext, userService) + .ConfigureAwait(false); + } + + /// + /// Null when the signed-in caller may see , + /// otherwise the localization key to refuse with. + /// + /// + /// Every site-specific reason answers with the SAME key on purpose. An + /// unknown site id, a site whose AssignedSite is removed, and a real site + /// belonging to somebody else are indistinguishable to the caller — + /// otherwise the two different 400 bodies let anyone enumerate which site + /// ids exist. The caller-specific keys the resolver itself returns + /// (UserNotFound and friends) do not reopen that: they depend only on who + /// is asking, so they are the same for every id that caller tries. + /// + /// An unknown id is refused for an admin too, and that is what makes the + /// two cases collapse: no AssignedSite row means the id is in nobody's + /// scope, admin's included. + /// + private async Task ResolveSiteAccessErrorAsync(int siteId, MicrotingDbContext sdkContext) + { + var scope = await ResolveScopeAsync(sdkContext); + if (scope.ErrorKey != null) + { + return scope.ErrorKey; + } + + return scope.Narrow([siteId]).Count == 0 ? "SiteNotFound" : null; + } + + /// + /// The working-hours grid for one site. SiteId comes from the request body, + /// so the caller's scope is checked here before any data is read; the export + /// paths call instead, having resolved the very + /// same scope once for the whole workbook. + /// public async Task>> Index( TimePlanningWorkingHoursRequestModel model) + { + try + { + var core = await coreHelper.GetCore(); + await using var sdkDbContext = core.DbContextHelper.GetDbContext(); + var accessError = await ResolveSiteAccessErrorAsync(model.SiteId, sdkDbContext); + if (accessError != null) + { + return new OperationDataResult>( + false, localizationService.GetString(accessError)); + } + } + catch (Exception ex) + { + SentrySdk.CaptureException(ex); + logger.LogError(ex.Message); + return new OperationDataResult>( + false, localizationService.GetString("ErrorWhileObtainingPlannings")); + } + + return await IndexUnscoped(model); + } + + /// + /// without the caller-scope check. Private, and named + /// for what it omits: every caller must have established that the signed-in + /// user may see model.SiteId before calling it. + /// + private async Task>> IndexUnscoped( + TimePlanningWorkingHoursRequestModel model) { try { @@ -2735,6 +2816,26 @@ public async Task> GenerateExcelDashboard(TimePlanni { var core = await coreHelper.GetCore(); var sdkContext = core.DbContextHelper.GetDbContext(); + + // Scope to the caller, from the same code the planning board, the + // export dialog's worker count and the all-workers export use. + // SiteId arrives straight from the query string, so without this any + // signed-in user could export any worker's hours by guessing an id. + // + // Before the lookups below, not after: those throw into the catch + // for an id that names no site, and that second, distinguishable + // 400 body would tell the guesser which ids are real. Refusing here + // gives the unknown id and the out-of-scope id one answer. + var accessError = await ResolveSiteAccessErrorAsync(model.SiteId, sdkContext); + if (accessError != null) + { + // Refuse outright rather than narrow: an empty or substituted + // workbook would read as a successful export of the worker that + // was asked for. + return new OperationDataResult(false, + localizationService.GetString(accessError)); + } + var site = await sdkContext.Sites.FirstAsync(x => x.MicrotingUid == model.SiteId); var siteWorker = await sdkContext.SiteWorkers.FirstAsync(x => x.SiteId == site.Id); var worker = await sdkContext.Workers.FirstAsync(x => x.Id == siteWorker!.WorkerId); @@ -2778,8 +2879,10 @@ public async Task> GenerateExcelDashboard(TimePlanni var timeStamp = $"{DateTime.UtcNow:yyyyMMdd_HHmmss}_{Guid.NewGuid():N}"; var filePath = Path.Combine(Path.GetTempPath(), "results", $"{timeStamp}_.xlsx"); - // Fetch data early so we can pre-compute pay lines for header discovery - var content = await Index(model); + // Fetch data early so we can pre-compute pay lines for header + // discovery. Unscoped: the gate at the top of this method has + // already cleared this SiteId for this caller. + var content = await IndexUnscoped(model); if (!content.Success) return new OperationDataResult(false, content.Message); // remove the first entry from the content.Model @@ -3373,13 +3476,7 @@ public async Task> GenerateExcelDashboard( // export dialog's worker count use. Without this a manager saw "3 // workers" in the dialog and downloaded the whole organisation — // and any non-admin could export every worker in the system. - var assignedSites = await dbContext.AssignedSites - .AsNoTracking() - .Where(x => x.WorkflowState != Constants.WorkflowStates.Removed) - .ToListAsync(); - var scope = await SiteScopeResolver - .ResolveForCurrentUserAsync(assignedSites, dbContext, sdkContext, baseDbContext, userService) - .ConfigureAwait(false); + var scope = await ResolveScopeAsync(sdkContext); if (scope.ErrorKey != null) { return new OperationDataResult(false, @@ -3469,7 +3566,10 @@ public async Task> GenerateExcelDashboard( } } - var dataResult = await Index(new TimePlanningWorkingHoursRequestModel + // Unscoped: siteIds was narrowed to the caller's scope above, and + // re-resolving that same scope once per site would be N round + // trips for an answer that cannot change between them. + var dataResult = await IndexUnscoped(new TimePlanningWorkingHoursRequestModel { DateFrom = model.DateFrom, DateTo = model.DateTo,