SCRUM-386 fix(sites): restrict site shift templates to assigned emplo… - #362
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Automated PR Validation FeedbackAll Checks Passed!
Ready for maintainer review! |
…yees
## Summary
Closes the IDOR on `GET /api/v1/sites/{siteId}/shift-templates` where any role-authorized user could read any site's templates. `Manager`/`HR` are now scoped to their `EmployeeSite` assignments, while `Admin`/`SuperAdmin`/`HRAdmin` bypass the site check. Unassigned or claim-less callers fail closed with `403`.
## What Changed
### Application Layer
- `GetSiteShiftTemplatesQuery` now carries `ActorEmployeeId` and `BypassSiteAccess` (both optional with safe defaults).
- `GetSiteShiftTemplatesQueryHandler` checks site existence first (`404`), then `EmployeeSite` membership for non-bypass callers (`403 "You do not have access to this site."`), via a new `HasSiteAccessAsync` helper. Missing actor fails closed.
- `Result`/`Result<T>` gain `ResultErrorType.Forbidden` + `IsForbidden` + `Forbidden()`.
### API / Controllers
- `GetSitesController.GetSiteShiftTemplates` passes `GetActorEmployeeId()` + `HasSiteBypass()` (`Admin`/`SuperAdmin`/`HRAdmin`) into the query and maps `IsForbidden` to `403 { message }`.
### Authorization & Organization Isolation
- Site-level isolation enforced through `EmployeeSites` (`EmployeeId`, `SiteId` composite key). No tenant concept exists in the codebase, so no tenant check was added.
## Security
- Fixes Broken Access Control on the site-scoped read path; template scoping `Where(ShiftTemplateSites.Any(SiteId == ...))` is unchanged.
- Fail-closed: null `ActorEmployeeId` with no bypass returns `403`, not `200` or `400`.
- Known limitation: `404`-before-`403` ordering means `404` vs `403` still reveals whether a site id exists. Accepted to keep `404` semantics for genuinely missing sites.
## API Behavior Changes
- Endpoints affected: `GET /api/v1/sites/{siteId}/shift-templates` only.
- 401 Unauthorized: unchanged (JWT bearer, role gate `HRAdmin,Admin,Manager,HR,SuperAdmin`).
- 403 Forbidden: new — `Manager`/`HR` without an `EmployeeSite` link to the requested site, or with no resolvable actor claim.
- 404 Not Found: unchanged — unknown `siteId`.
- Breaking changes: intentional — unassigned `Manager`/`HR` calls that previously returned `200` now return `403`. Bypass roles and assigned users are unaffected.
## Testing
- Unit tests: existing `GetSiteShiftTemplatesTests` updated for the new handler constructor (added `EmployeeSite` repository) and privileged bypass on the pre-existing filtering/search cases.
- Authorization tests: 4 new cases — assigned user succeeds, cross-site user gets `IsForbidden`, null actor gets `IsForbidden`, bypass without assignment succeeds.
- Executed: `dotnet build HR_system.slnx` — 0 warnings, 0 errors; filtered `GetSiteShiftTemplatesTests` — 17/17 passed; full `Buy2.Domain.Tests` — 714/714 passed.
## Backward Compatibility
- Query constructor is backward compatible for 2-arg callers via optional parameters. Direct `GetSiteShiftTemplatesQueryHandler` constructions need the third `EmployeeSite` repository (DI resolves it automatically in production).
- Response DTO shape is unchanged.
## Important Implementation Details
- `HasSiteAccessAsync` is a single `AnyAsync` on `EmployeeSites` with `AsNoTracking`; handler complexity stays within limits.
- No pagination or filtering semantics were changed on this endpoint.
## Checklist
- [x] Build passes
- [x] Tests pass
- [x] Relevant authorization scenarios are covered
- [x] Organization / tenant isolation is preserved
- [x] API behavior is documented where changed
- [x] No unintended breaking changes
- [x] Security implications have been reviewed
Mahmoud-Alim
force-pushed
the
fix/idor-site-shift-templates-access-control
branch
from
September 15, 2026 03:54
59aed12 to
e90d779
Compare
Automated PR Validation FeedbackAll Checks Passed!
Ready for maintainer review! |
Moha-sami
approved these changes
Sep 15, 2026
Moha-sami
left a comment
Moha-sami
left a comment
Owner
There was a problem hiding this comment.
Approved. Site shift template IDOR fix and authorization tests look great.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes the IDOR on
GET /api/v1/sites/{siteId}/shift-templateswhere any role-authorized user could read any site's templates.Manager/HRare now scoped to theirEmployeeSiteassignments, whileAdmin/SuperAdmin/HRAdminbypass the site check. Unassigned or claim-less callers fail closed with403.What Changed
Application Layer
GetSiteShiftTemplatesQuerynow carriesActorEmployeeIdandBypassSiteAccess(both optional with safe defaults).GetSiteShiftTemplatesQueryHandlerchecks site existence first (404), thenEmployeeSitemembership for non-bypass callers (403 "You do not have access to this site."), via a newHasSiteAccessAsynchelper. Missing actor fails closed.Result/Result<T>gainResultErrorType.Forbidden+IsForbidden+Forbidden().API / Controllers
GetSitesController.GetSiteShiftTemplatespassesGetActorEmployeeId()+HasSiteBypass()(Admin/SuperAdmin/HRAdmin) into the query and mapsIsForbiddento403 { message }.Authorization & Organization Isolation
EmployeeSites(EmployeeId,SiteIdcomposite key). No tenant concept exists in the codebase, so no tenant check was added.Security
Where(ShiftTemplateSites.Any(SiteId == ...))is unchanged.ActorEmployeeIdwith no bypass returns403, not200or400.404-before-403ordering means404vs403still reveals whether a site id exists. Accepted to keep404semantics for genuinely missing sites.API Behavior Changes
GET /api/v1/sites/{siteId}/shift-templatesonly.HRAdmin,Admin,Manager,HR,SuperAdmin).Manager/HRwithout anEmployeeSitelink to the requested site, or with no resolvable actor claim.siteId.Manager/HRcalls that previously returned200now return403. Bypass roles and assigned users are unaffected.Testing
GetSiteShiftTemplatesTestsupdated for the new handler constructor (addedEmployeeSiterepository) and privileged bypass on the pre-existing filtering/search cases.IsForbidden, null actor getsIsForbidden, bypass without assignment succeeds.dotnet build HR_system.slnx— 0 warnings, 0 errors; filteredGetSiteShiftTemplatesTests— 17/17 passed; fullBuy2.Domain.Tests— 714/714 passed.Backward Compatibility
GetSiteShiftTemplatesQueryHandlerconstructions need the thirdEmployeeSiterepository (DI resolves it automatically in production).Important Implementation Details
HasSiteAccessAsyncis a singleAnyAsynconEmployeeSiteswithAsNoTracking; handler complexity stays within limits.Checklist