Spike: Auth S2 (only API permissions, no resource scopes)#5499
Closed
ramonsmits wants to merge 32 commits into
Closed
Spike: Auth S2 (only API permissions, no resource scopes)#5499ramonsmits wants to merge 32 commits into
ramonsmits wants to merge 32 commits into
Conversation
- Remove unreachable wildcard-with-scope branch in IsInScope (dead code: * grants always have null scope, already handled by the Scope==null check above it) - Fully qualify IPermissionEvaluator cref in RealmAccessClaimsTransformation XML doc (type not imported in that project) - Add PermissionEvaluatorTests: cross-grant isolation — deny in role-a must not suppress allow in role-b for the same permission - Add RbacPolicyLoaderTests: LoadFromFile with a non-existent path throws RbacPolicyException whose message contains the path
Adds IAuthorizationAuditLog / AuthorizationAuditLog (task 0.10) which emits structured log entries on the ServiceControl.Audit category via a [LoggerMessage] source-generated method. Adds RecordingLoggerProvider (task 0.11) to ServiceControl.AcceptanceTesting so both unit and acceptance tests can assert on captured log entries; adds that project reference to Infrastructure.Tests to enable the audit-log unit tests.
…t (tasks 0.12–0.14) Task 0.12: GET /api/me/permissions controller (MePermissionsController) marked [AuthenticatedOnly]; acceptance test in Security/Authorization/. Task 0.13: AddServiceControlAuthorization extension that early-returns when OIDC is disabled; registers Func<RbacPolicy>, IPermissionEvaluator, IAuthorizationAuditLog and RealmAccessClaimsTransformation; wired into RunCommand and the acceptance-test runner. Acceptance test for the OIDC-disabled non-breaking guarantee. Task 0.14: RequirePermissionAttribute and AuthenticatedOnlyAttribute markers; Every_endpoint_declares_an_authorization_decision completeness test with Phase 0 baseline of all uncovered endpoints (baseline shrinks to empty as Phase 1+ wires permissions). Supporting: add IServiceProvider Services to IAcceptanceTestInfrastructureProvider so the completeness test can resolve EndpointDataSource; propagated to all implementing classes (primary/audit/monitoring/multi-instance test runners); add LoadedAt to RbacPolicy (loader sets it); expose Allow/Deny on ResourceScope for the descriptor projection; extend MockOidcServer with GenerateTokenWithRealmRoles for role-carrying acceptance tests.
Add all pre-existing API endpoints that lack authorization attributes to Phase0Baseline so the authorization coverage test passes. Accounts for actual route patterns emitted by ASP.NET Core (with constraints, PATCH/POST multi-method routes, etc.) rather than the legacy patterns assumed when the baseline was first drafted.
… cleanup Fix 1 (CRITICAL): MePermissionsController now resolves IPermissionEvaluator via IServiceProvider.GetService<>() instead of mandatory constructor injection. When OIDC is disabled the services are not registered and the endpoint returns 404 Not Found (spec §4 non-breaking guarantee) rather than a 500 from a failed DI activation. Acceptance test renamed to Me_permissions_endpoint_returns_404_when_auth_disabled and now asserts exactly HttpStatusCode.NotFound. Fix 2 (IMPORTANT): PermissionEvaluator.Resolve deduplicates identical grants (same permission + same scope) using a canonical string key — a user in two roles both granting messages:view now yields exactly one entry. Different scopes for the same permission are preserved (OR semantics). Added XML doc comments on Resolve and EffectivePermissions stating the OR semantics contract. Added two unit tests: deduplication of identical grants and preservation of distinct-scope grants. Fix 3 (MINOR): Renamed RecordingLoggerProvider.GetEntries(string) to EntriesFor(string) (Entries(string) collides with the Entries property in C#) and updated all call sites in AuthorizationAuditLogTests. Added a comment in AuthorizationHostApplicationBuilderExtensions noting that the authenticated-user FallbackPolicy (spec §5.5) is already registered by AddServiceControlAuthentication — confirmed present at HostApplicationBuilderExtensions.cs:98.
…heck test Task A: Expand Permissions.cs with all Phase 1+ catalogue constants grouped by area (endpoints, heartbeats, custom checks, sagas, event log, licensing, notifications, redirects, queues, throughput, connections, monitoring, audit). The existing messages:* and recoverabilitygroups:* constants are preserved as-is. Task B: Update rbac.yaml defaults — sc-viewer gets every :view permission across all areas; sc-operator gets every :view plus the write permissions for messages, recoverability, endpoints, custom checks, notifications, redirects, queues, throughput, and connections (licensing:manage is admin-only); sc-admin unchanged. Task C: Add KnownUnenforcedPermissions.cs (all constants unenforced on base) and Catalogue_completeness_tests.cs — two unit-style NUnit tests that assert (1) every catalogue constant is either enforced or declared as known-unenforced, and (2) no stale entries linger in KnownUnenforcedPermissions after enforcement is wired.
…rizationExtensions Implements the S2 policy-attribute enforcement variant: - PermissionRequirement / PermissionVerbHandler / PermissionPolicyProvider (mirrored from S3) - IResourceScopeChecker + ResourceScopeChecker: S2-specific inline helper injected into controllers; EnforceAsync writes a structured 403 and returns non-null on deny, null on allow - AllowAllResourceScopeChecker registered when OIDC is disabled (preserves pre-RBAC behaviour) - S2AuthorizationExtensions.AddServiceControlS2Authorization wired in RunCommand and runner - RetryMessagesController: [RequirePermission]+[Authorize] verb gate + IResourceScopeChecker inline scope check on RetryMessageBy (vertical slice); other retry actions get verb gate only - KnownUnenforcedPermissions: remove MessagesRetry (now enforced) - Phase0Baseline: remove 5 retry routes (now carry [RequirePermission])
…udit/OIDC-disabled) Mirrors When_retrying_a_failed_message.cs from tf3651-authz-s3. Covers: (a) sc-operator→202, (b) sc-viewer→403, (c) scoped in-scope→202/out-of-scope→403, (d) allow+deny decisions in ServiceControl.Audit log (deny includes queue address), (e) OIDC disabled→202 unauthenticated.
…viceProvider is alive When a custom IAuthorizationPolicyProvider is registered, ASP.NET Core's RouteEndpointDataSource resolves endpoint metadata lazily via IServiceProvider. Accessing Endpoints after Run() completes causes ObjectDisposedException. Same fix applied on tf3651-authz-s3.
…and FilterByQueueScope to RavenDB (R1 plumbing)
…verability controllers
…nd Phase0Baseline; fix nullable warnings
- Register `AllowAllPermissionEvaluator` as no-op `IPermissionEvaluator` when OIDC is disabled — controllers injecting `IPermissionEvaluator` (e.g. GetAllErrorsController) were failing with a DI resolution 500 in auth-disabled tests. - Remove duplicate `using ServiceControl.Infrastructure.WebApi` in 5 controllers that already had the relative `using Infrastructure.WebApi` (CS0105 warning). - Update `FilterBySentTimeRange` signature to accept `DateTimeRange?` to match the interface change (eliminates CS8767 nullable mismatch warnings). - Fix `When_authentication_is_enabled.Should_accept_requests_with_valid_bearer_token`: use `sc-operator` role so the request passes the `messages:view` verb gate. - Fix `ErrorsGet_scoped_role_returns_filtered_results`: add required `MessageMetadata` (IsSystemMessage, MessageType, TimeSent, ReceivingEndpoint, etc.) to the test's `BuildFailedMessage` helper so the FailedMessageViewTransformer can deserialise results. - Fix `ErrorsSummary_operator_receives_200` → `ErrorsSummary_operator_passes_auth_gate`: assert `Is.Not.EqualTo(Forbidden)` since the facet index may not be populated in the embedded test environment; the 403 gate is what we're verifying.
Delete RequirePermissionAttribute — enforcement is carried by [Authorize(Policy = X)] (resolved by PermissionPolicyProvider), which already embeds the same permission string. The duplicate marker was decorative and added noise without value. Update the endpoint-completeness test and catalogue cross-check to detect permission coverage via [Authorize(Policy = X)] where X ∈ Permissions.All, anchoring the ServiceControl assembly scan on ConnectionController instead of the now-deleted attribute type.
Remove [RequirePermission(...)] from all 15 controller actions (S2). [Authorize(Policy = X)] (resolved by PermissionPolicyProvider) is the sole enforcement mechanism and already carries the same permission string — the extra attribute was decorative duplication.
…NOT-starts-with
(a) Deny prefix patterns (e.g. 'Finance.*') were using WhereNotEquals on the
bare prefix 'Finance', denying only the exact string and silently allowing
'Finance.payroll', 'Finance.ap', etc. Fix: use AndAlso().Not.WhereStartsWith
with the prefix including the trailing dot, mirroring the allow-prefix logic.
RavenQueryExtensions on s3 was already correct; ResourceScope.Matches was not.
(b) Allow and deny patterns were not lowercased before comparison. Queue addresses
are stored in lowercase in the index, so 'Sales.*' would not match 'sales.orders'.
Fix: call .ToLowerInvariant() on every pattern in ResourceScope.Matches before
comparison, consistent with what FilterByQueueScope already does at query time.
Adds unit tests covering both bugs.
…s (S2)
The DELETE /api/recoverability/unacknowledgedgroups/{groupId} endpoint had no
[Authorize] attribute on S2, allowing unauthenticated callers to acknowledge
retry/archive operations. S3 and S4 already require recoverabilitygroups:view.
Aligns S2 to the same policy.
…rs.WriteScopeDenied403 (S2) FailureGroupsRetryController, ArchiveController, and UnarchiveController on S2 were inlining their own 403 JSON body with a bespoke reason string. S4 already uses AuthorizationHelpers.WriteScopeDenied403 for shape and prose consistency. This change aligns S2 to the same helper.
S2 was allowing scoped users to add or delete group comments without a scope check. Adds EnforceGroupScopeAsync helper (mirroring S4) to FailureGroupsController and applies the guard to both EditComment and DeleteComment actions.
…r case-insensitive matching ResourceScope.Matches was lowercasing the pattern but not the resource, causing mixed-case queue addresses (e.g. 'Sales.OrderHandler@localhost') to fail matching against lowercased policy patterns (e.g. 'sales.*'). Since queue addresses in memory may be mixed-case while the RavenDB index stores them lowercase, both sides must be normalised before comparison.
#nullable enable was added to ErrorMessagesDataStore.cs and RavenQueryExtensions.cs as part of the RBAC scope filtering work. Several pre-existing members had nullable mismatches that became hard errors: - Sort<TSource>: CriticalTime/DeliveryTime/ProcessingTime are nullable TimeSpan?; use null-forgiving operator for Expression<Func<T,object>> - ErrorLastBy: returns null! for missing messages (caller checks for null) - Map: GetLastModifiedFor returns nullable DateTime?; value is always set for persisted documents — suppress with null-forgiving operator - DocumentPatchResult.Document: deserialization target, may be null - FetchFromFailedMessage: byte[] body initialised to null — mark nullable
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.
No description provided.