feat: Policy classes for entity-level authorization (#162)#257
Merged
Conversation
…ons (#162) - IPolicy<TResource>, AuthorizationResult, IAuthorizer/Authorizer in SimpleModule.Core.Authorization.Policies: instance-level rules (ownership, tenancy, state) on top of string permissions; deny wins across multiple policies, missing policy fails closed with MissingPolicyException - PolicyAuthorizationOptions.NotFoundActions maps denials to 404 (default: view) to prevent resource enumeration - Declarative .AuthorizeResource<T>(action) endpoint filter backed by IResourceResolver<T> - Source generator discovers IPolicy<T> implementors and emits scoped registrations in AddModules(); new SM0058 diagnostic requires policy resource types to be contracts DTOs - Notifications refactored as the reference implementation: NotificationPolicy (owner-or-admin), MarkReadEndpoint now does load -> authorize -> act via IAuthorizer - Docs: guide/policies.md, permissions cross-link, CONSTITUTION section and diagnostics table Closes #162
Addresses all 10 findings from the high-effort review of the IPolicy<T> feature: - Restore owner-scoped MarkReadAsync(id, userId) on INotificationsContracts; the unscoped FindAsync moves to module-internal INotificationStore so in-process callers can't bypass ownership (also restores 404 when the row vanishes between load and write) - SM0059 (Error): policy classes must be public — non-public policies were silently skipped at registration and failed only at runtime - SM0058 reworked: resource classified symbolically at discovery ([Dto] or .Contracts assembly), fixing false positives for [NoDtoGeneration]/IEvent contracts entities - PolicyFinder now scans contracts assemblies and nested types, matching the documented auto-discovery contract - AuthorizationResult.DenyAsNotFound(): per-decision 404 mapping; the Notifications module no longer mutates host-global PolicyAuthorizationOptions (action-string collision across modules) - AddSimpleModuleWorker registers IAuthorizer + options so background handlers can authorize resources like the web host - Generated registrations use TryAddEnumerable — manual registration can no longer cause a policy to execute twice per check - NotificationPolicy is owner-only again: admins are not exempt from instance rules (pre-refactor behavior), covered by an integration test - AuthorizeResource: missing route value now throws InvalidOperationException (loud misconfig) instead of 404; 6 new TestServer-based filter tests - SM0060 (Error): policies must be owned by the resource's module — deny-wins semantics made foreign policies a cross-module veto
Owner
Author
|
Pushed a hardening commit addressing all 10 findings from the high-effort code review: Security / correctness
Design / robustness Verification: full build clean under TreatWarningsAsErrors; Core 267 ✓ (+14), Generator 215 ✓ (incl. SM0059/SM0060 + multi-assembly/nested-discovery tests), Notifications 25 ✓, and all other module suites green. |
- Discovery completeness: PolicyFinder now scans the compiling (host) assembly and ALL contracts assemblies (mapped or not), visiting each assembly once — host-level policies were silently never registered - SM0059 uses effective accessibility (containing-type chain); internal resource types now fail SM0058 instead of emitting uncompilable code - SM0061 (Error): generic policy classes are rejected — they previously emitted unbound type parameters into AddModules() - Factory-registration gap: emitted comment and docs now state the real TryAddEnumerable semantics (type-based dedup; two-generic factory overload), and [ManualContractRegistration] opts a policy out of auto-registration entirely - PolicyAuthorizationOptions.NotFoundActions defaults to empty: the host-global action set silently converted explicit Deny(reason) into bare 404s; DenyAsNotFound is the per-decision mechanism - AuthorizeResource: template-aware absence handling — an optional or catch-all parameter omitted at runtime is 404, a parameter missing from the template is a loud InvalidOperationException - '.Contracts' suffix via AssemblyConventions with Ordinal comparison, matching ContractFinder (case-variant assemblies could silently skip the SM0060 ownership check) - SM0059/SM0061 deduped per class for multi-interface policies - AuthorizeResourceFilterTests: environment pinned to Production (5/6 tests failed under ASPNETCORE_ENVIRONMENT=Development via the developer exception page) and shared IClassFixture host - Notifications reference now exercises the declarative form: NotificationResolver + .AuthorizeResource<Notification>(MarkRead); MarkReadAsync is a single owner-scoped ExecuteUpdateAsync; CancellationToken propagated through FindAsync - CreateMultiAssemblyCompilation promoted to GeneratorTestHelper; Authorizer fail-closed guard moved before the policy loop
- Revert MarkReadAsync to tracked load-modify-save: the round-2 ExecuteUpdateAsync rewrite silently bypassed the SaveChanges interceptor pipeline (audit log entries, UpdatedAt, concurrency stamp rotation) — comment now pins the reason - [ManualContractRegistration] policies are exempt from SM0059/SM0061 (the module registers them itself, internal/generic is fine), matching the attribute's contract-side SM0028 semantics; attribute XML doc and a generator test cover the policy opt-out - IsContractsDto suffix match back to OrdinalIgnoreCase, agreeing with SymbolDiscovery's contracts classification (round-2 Ordinal tightening disagreed with the rest of the pipeline) - PolicyFinder walk refactored around a PolicyScanContext struct — removes the 7-param threading and the adjacent same-typed dictionary swap hazard - Authorizer back to the zero-alloc fail-closed flag (GetServices is already materialized; ToList was a pure copy per guarded request) - GeneratorTestHelper.CreateMultiAssemblyCompilation host side composed from CreateCompilation (one reference list); ContractAutoDiscoveryTests and DtoConventionTests private copies now delegate to it - AuthorizeResource filter test types moved to namespace scope (fixture no longer reaches into the test class) - Docs: SM0060 host-assembly exemption documented (CONSTITUTION + guide); FormRequest-before-AuthorizeResource ordering note; stale NotFoundActions default claims fixed in tasks/todo.md
…e rules, doc accuracy - MarkReadAsync treats DbUpdateConcurrencyException as idempotent success: concurrent duplicate mark-read (double-click, client retry) rotated the concurrency stamp and surfaced as a 500; the caller's desired outcome already holds - Manually-registered generic policies with CONCRETE resource types are no longer exempt from SM0058/SM0060 (new ResourceIsTypeParameter flag gates the noise suppression precisely; the attribute's documented 'resource rules still apply' guarantee now holds) - Docs: SM0060 exemption correctly described as 'assemblies that map to no module' rather than 'the host assembly' (a host with its own [Module] is not exempt); SM0059/SM0061 manual-registration waiver surfaced in the guide and rules table; stale ExecuteUpdateAsync / guard-first / AddScoped claims in tasks/todo.md corrected
…uter
The Inbox 'Mark read' / 'Mark all read' buttons called the plain JSON API
endpoints via Inertia's router.post(), which expects an Inertia-protocol
response and treats the endpoints' 204 No Content as 'Server error (204)' —
showing an error toast and never refreshing the list even though the backend
succeeded. Switch to fetch() + router.reload({ only: [...] }), matching the
convention used by the FileStorage/RateLimiting/Email pages. Genuine non-2xx
responses (e.g. a policy 404) are logged and skip the reload.
Surfaced by QA of the issue #162 mark-read flow; pre-existing (Inbox.tsx was
not touched by the policy work and the endpoint already returned 204).
Rolls out the issue #162 IPolicy<T> mechanism to the places the module survey found, and fixes adjacent coarse-permission gaps. FileStorage (refactor + hardening): - FileStoragePolicy : IPolicy<StoredFile> (uploader-or-admin; DenyAsNotFound) - GetById/Download/Delete now load -> authorize via IAuthorizer; removes the duplicated FileOwnershipCheck and the fragility that the by-id contract is unscoped. Non-owner now 404 (was 403) for anti-enumeration. Users (closes a real IDOR / broken object-level authorization): - GET/PUT/DELETE /api/users/{id} previously had only .RequireAuthorization() — any authenticated user could read/update/delete any account. Added the missing .RequirePermission(View/Update/Delete) gate plus UserPolicy (self-or-admin view/update, admin-only delete; view denials 404). Settings (coarse gaps found during the survey, not policy work): - GET /api/settings now requires Settings.View and no longer returns other users' User-scoped values (info-disclosure via ?scope=User). - Menu mutation endpoints + GetAvailablePages now require ManageMenus (were .RequireAuthorization() only); the public menu tree stays open to any authenticated user. Sessions: evaluated for a SessionPolicy and deliberately left as-is — SessionDto has no owner field, the 'block current session' rule is domain state already modeled by RevokeSessionResult, and the duplication is two ISessionContracts impls over different stores that a policy cannot unify. Tests: +4 FileStorage, +8 Users, +5 Settings policy/authorization tests; all 17 suites green. Browser-verified the full matrix (own 200 / others 404 / self 200 / cross-update 403 / non-admin delete 403).
…licies # Conflicts: # tasks/todo.md
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
1 participant
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.
Closes #162.
Adds Laravel-style policy classes — instance-level authorization layered on top of the existing string-permission system — then rolls the mechanism out across the modules that benefit and fixes the authorization gaps the rollout surfaced.
The mechanism (
SimpleModule.Core.Authorization.Policies)IPolicy<TResource>— per-resource verbs (view/update/delete+ custom) returningAuthorizationResult.Allow()/.Deny(reason)/.DenyAsNotFound(reason).IAuthorizer—CheckAsync(returns result) andAuthorizeAsync(throws on deny). Policies run in registration order, first deny wins; no registered policy fails closed withMissingPolicyException..AuthorizeResource<T>("action")endpoint filter (backed byIResourceResolver<T>) for handlers that don't otherwise need the entity, and imperativeIAuthorizer(load → authorize → act) for handlers that do.ForbiddenException(403);DenyAsNotFound()throwsNotFoundException(404) for anti-enumeration. Per-decision; a host-levelPolicyAuthorizationOptions.NotFoundActionsoverride also exists (empty by default).IPolicy<T>implementors (impl, contracts, and host assemblies, incl. nested classes) and registers them withTryAddEnumerable. Four diagnostics: SM0058 (resource must be a contracts DTO), SM0059 (policy must be public), SM0060 (policy owned by the resource's module), SM0061 (policy must not be generic) — all waived for[ManualContractRegistration]where applicable.Reference implementations (3 shipped policies)
NotificationPolicyAuthorizeResourceFileStoragePolicyIAuthorizerUserPolicyIAuthorizerSecurity fixes found during the module survey
GET/PUT/DELETE /api/users/{id}previously had only.RequireAuthorization(): any authenticated user could read, edit, or delete any account. Added the missingRequirePermission(View/Update/Delete)gate plusUserPolicy.FileStoragePolicy; non-owner is now 404 (was 403).GET /api/settingsnow requiresSettings.Viewand no longer leaks other users' User-scoped values via?scope=User; menu-mutation endpoints +GetAvailablePagesnow requireManageMenus(the public menu tree stays open to any authenticated user).Deliberately not changed
SessionPolicyand declined:SessionDtohas no owner field, the "can't revoke your current session" rule is domain state already modeled byRevokeSessionResult, and the duplication is twoISessionContractsimplementations over different backends that noIPolicy<SessionDto>can unify. The typed-result service method is the correct abstraction.Acceptance criteria (#162)
IPolicy<T>andIAuthorizerinSimpleModule.Coredocs/site/guide/policies.md) + CONSTITUTION sectionReview history & verification
/code-reviewto convergence over 4 hardening rounds (round 5 returned no findings); ran a full/qabrowser cycle (found + fixed the inbox 204 mark-read bug).7 commits: feat → 4 review-hardening rounds → QA fix → multi-module rollout.