diff --git a/.github/rulesets/main.json b/.github/rulesets/main.json index d90346e..497a799 100644 --- a/.github/rulesets/main.json +++ b/.github/rulesets/main.json @@ -1,9 +1,15 @@ { - "_comment": "Declarative GitHub Repository Ruleset for the default branch (main). Apply manually with `gh api -X PUT /repos/{owner}/{repo}/rulesets/{id}` (snippet in docs/operations.md). Drift detection runs in CI. Replaces the legacy classic-branch-protection JSON; rulesets are the modern API and have a much simpler drift-comparison surface.", + "_comment": "Declarative GitHub Repository Ruleset for the default branch (main). Apply manually with `gh api -X PUT /repos/{owner}/{repo}/rulesets/{id}` (snippet in docs/operations.md). Drift detection runs in CI. The repository administrator (actor_id 5 = the Repository Admin role) is allowed to bypass via `--admin --squash` for solo-maintainer self-merge — every external contributor still needs CODEOWNER approval, the squash-only merge_methods enforce linear history, and the strict required-status-checks list is the supply-chain gate (build-test, bicep-lint, dependency-review, CodeQL, actionlint, gitleaks, scorecard, pinact).", "name": "main-branch-protection", "target": "branch", "enforcement": "active", - "bypass_actors": [], + "bypass_actors": [ + { + "actor_id": 5, + "actor_type": "RepositoryRole", + "bypass_mode": "always" + } + ], "conditions": { "ref_name": { "include": ["~DEFAULT_BRANCH"], @@ -17,9 +23,9 @@ { "type": "pull_request", "parameters": { - "required_approving_review_count": 0, + "required_approving_review_count": 1, "dismiss_stale_reviews_on_push": true, - "require_code_owner_review": false, + "require_code_owner_review": true, "require_last_push_approval": false, "required_review_thread_resolution": true, "required_reviewers": [], @@ -38,7 +44,8 @@ { "context": "Analyze (csharp)" }, { "context": "Analyze (actions)" }, { "context": "actionlint" }, - { "context": "gitleaks" } + { "context": "gitleaks" }, + { "context": "pinact" } ] } } diff --git a/.github/workflows/repo-rulesets.yml b/.github/workflows/repo-rulesets.yml index 6da86e1..afd1013 100644 --- a/.github/workflows/repo-rulesets.yml +++ b/.github/workflows/repo-rulesets.yml @@ -52,11 +52,16 @@ jobs: # Strip API-only fields that the server adds on read so they don't # show up as drift. Everything else is part of the user-authored shape. - STRIP='del(.id, .source_type, .source, .node_id, .created_at, .updated_at, .current_user_can_bypass, ._links, .links)' + # `bypass_actors` is also stripped because the default GITHUB_TOKEN does + # not have permission to read it (it requires repo-admin scope), so the + # API returns it as missing/null in CI even when it's set on the live + # ruleset. The committed file still records the intended bypass actors + # for documentation; verify them out-of-band with an admin PAT. + STRIP='del(.id, .source_type, .source, .node_id, .created_at, .updated_at, .current_user_can_bypass, ._links, .links, .bypass_actors)' # Optional fields that the API may or may not return when empty. # Normalize both sides to "field absent if empty" so they compare # equal regardless of which shape the API happens to use. - NORMALIZE='if .bypass_actors == [] then del(.bypass_actors) else . end' + NORMALIZE='.' drift=0 for f in "${configs[@]}"; do diff --git a/.github/workflows/scorecard.yml b/.github/workflows/scorecard.yml index 8079f79..b3a2ce0 100644 --- a/.github/workflows/scorecard.yml +++ b/.github/workflows/scorecard.yml @@ -28,7 +28,7 @@ jobs: id-token: write steps: - name: Checkout - uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5.0.1 + uses: actions/checkout@de0fac2e4500dabe0009e67214ff5f5447ce83dd # v6.0.2 with: persist-credentials: false @@ -43,13 +43,13 @@ jobs: # Keep raw results around for 5 days so a failed run can be inspected # without re-running the analysis (which costs API calls). - name: Upload artifact - uses: actions/upload-artifact@ea165f8d65b6e75b540449e92b4886f43607fa02 # v4.6.2 + uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7.0.1 with: name: SARIF file path: results.sarif retention-days: 5 - name: Upload to code-scanning - uses: github/codeql-action/upload-sarif@48ab28a6f5dbc2a99bf1e0131198dd8f1df78169 # v3.28.0 + uses: github/codeql-action/upload-sarif@95e58e9a2cdfd71adc6e0353d5c52f41a045d225 # v4.35.2 with: sarif_file: results.sarif diff --git a/src/Ftgo.Restaurants.Api/Controllers/RestaurantsController.cs b/src/Ftgo.Restaurants.Api/Controllers/RestaurantsController.cs index 40fd03a..b0068dc 100644 --- a/src/Ftgo.Restaurants.Api/Controllers/RestaurantsController.cs +++ b/src/Ftgo.Restaurants.Api/Controllers/RestaurantsController.cs @@ -1,4 +1,3 @@ -using Ftgo.Auth; using Microsoft.AspNetCore.Authorization; using Microsoft.AspNetCore.Mvc; @@ -6,11 +5,11 @@ namespace Ftgo.Restaurants.Api.Controllers; [ApiController] [Route("api/[controller]")] -[Authorize(Roles = "Restaurants.Read.All")] -[RequireClientApp] public sealed class RestaurantsController : ControllerBase { + /// App-only multi-tenant endpoint. Bound to the named policy. [HttpGet("system")] + [Authorize(Policy = RestaurantsAuthorizationPolicies.App)] public IActionResult System() => Ok(new { service = "Ftgo.Restaurants.Api", diff --git a/src/Ftgo.Restaurants.Api/Program.cs b/src/Ftgo.Restaurants.Api/Program.cs index a11124d..410a1e4 100644 --- a/src/Ftgo.Restaurants.Api/Program.cs +++ b/src/Ftgo.Restaurants.Api/Program.cs @@ -1,4 +1,5 @@ using Ftgo.Auth; +using Ftgo.Restaurants.Api; using Scalar.AspNetCore; var builder = WebApplication.CreateBuilder(args); @@ -9,6 +10,16 @@ builder.Services.AddControllers(); builder.Services.AddHealthChecks(); +// Doctrine: app-only multi-tenant resource API. One named policy combining +// (a) Restaurants.Read.All app-role, (b) azp allow-list, (c) rejection of any token +// that also carries `scp`. Never use [Authorize(Roles=...)] when MapInboundClaims=false — +// the framework looks for ClaimTypes.Role, not the short `roles` name Entra emits. +// See eng-guide ch02 §10 / decision-trees.md Tree 4. +builder.Services.AddAuthorization(o => +{ + o.AddAppPolicy(RestaurantsAuthorizationPolicies.App, "Restaurants.Read.All"); +}); + builder.Services.AddEntraAuthRateLimiter(); builder.Services.AddEntraAuthForwardedHeaders(); diff --git a/src/Ftgo.Restaurants.Api/RestaurantsAuthorizationPolicies.cs b/src/Ftgo.Restaurants.Api/RestaurantsAuthorizationPolicies.cs new file mode 100644 index 0000000..1e5418e --- /dev/null +++ b/src/Ftgo.Restaurants.Api/RestaurantsAuthorizationPolicies.cs @@ -0,0 +1,23 @@ +namespace Ftgo.Restaurants.Api; + +/// +/// Named authorization policy identifiers for Restaurants.Api. +/// +/// +/// Doctrine: app-only multi-tenant resource APIs expose one named policy that combines +/// the app-role check with the azp allow-list and rejects any token that also +/// carries scp. Never use [Authorize(Roles = "...")] on a JWT scheme that +/// has MapInboundClaims = false — the framework looks for claims of type +/// ClaimTypes.Role, but Entra emits the role claim under the short name roles, +/// so the role check would silently fail and either lock everyone out or (worse) lock no +/// one out depending on what other Authorize attributes are also applied. See +/// dotnet-engineering-guide ch02 §10 and +/// decision-trees.md +/// Tree 4. +/// +public static class RestaurantsAuthorizationPolicies +{ + /// App-only (S2S, multi-tenant) callers — requires roles=Restaurants.Read.All, + /// an allow-listed azp, and rejects tokens with scp. + public const string App = "RestaurantsApp"; +} diff --git a/tests/Ftgo.Auth.Tests/RestaurantsAppPolicyEndToEndTests.cs b/tests/Ftgo.Auth.Tests/RestaurantsAppPolicyEndToEndTests.cs new file mode 100644 index 0000000..b7a8552 --- /dev/null +++ b/tests/Ftgo.Auth.Tests/RestaurantsAppPolicyEndToEndTests.cs @@ -0,0 +1,161 @@ +using System.Net; +using System.Net.Http.Headers; +using Ftgo.Auth; +using Microsoft.AspNetCore.Authorization; +using Microsoft.AspNetCore.Builder; +using Microsoft.AspNetCore.Hosting; +using Microsoft.AspNetCore.Http; +using Microsoft.AspNetCore.TestHost; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; + +namespace Ftgo.Auth.Tests; + +/// +/// End-to-end coverage of the multi-tenant app-only policy used by Ftgo.Restaurants.Api. +/// Mirrors but for the single-policy (app-only) +/// resource API shape — proves the policy enforces role + azp allow-list + scp-rejection. +/// +/// +/// This test exists specifically to catch the regression where an author writes +/// [Authorize(Roles = "Restaurants.Read.All")] [RequireClientApp] instead of using the +/// named policy. With MapInboundClaims = false (doctrine), the framework looks for +/// claims of type , but the JWT carries +/// the role under the short name roles, so the role check is silently skipped — the +/// endpoint then accepts any allow-listed app regardless of role. The two `_PolicyMustEnforceRole` +/// tests exercise that exact failure mode. +/// +public sealed class RestaurantsAppPolicyEndToEndTests : IAsyncLifetime +{ +#pragma warning disable IDE1006 + private const string AllowedAppId = "22222222-2222-2222-2222-222222222222"; + private const string AppPolicy = "RestaurantsApp"; + private const string Role = "Restaurants.Read.All"; +#pragma warning restore IDE1006 + + private IHost _host = null!; + private HttpClient _client = null!; + + public async ValueTask InitializeAsync() + { + _host = await new HostBuilder() + .ConfigureWebHost(web => web + .UseTestServer() + .ConfigureServices(services => + { + services.AddRouting(); + services.AddSingleton>(_ => + new StaticOptionsMonitor(new EntraAuthOptions + { + AllowedClientApps = [AllowedAppId], + })); + + services.AddAuthentication(TestAuthHandler.SchemeName) + .AddScheme(TestAuthHandler.SchemeName, _ => { }); + + services.AddAuthorization(o => + { + o.AddAppPolicy(AppPolicy, Role); + + // Pin the test scheme onto the doctrine policy so MapGet([Authorize(Policy=...)]) + // resolves against the Test handler (mirrors the JWT scheme in production). + var existing = o.GetPolicy(AppPolicy)!; + o.AddPolicy(AppPolicy, new AuthorizationPolicyBuilder(TestAuthHandler.SchemeName) + .Combine(existing) + .Build()); + }); + services.AddSingleton(); + services.AddSingleton(); + }) + .Configure(app => + { + app.UseRouting(); + app.UseAuthentication(); + app.UseAuthorization(); + app.UseEndpoints(e => + { + e.MapGet("/system", () => Results.Ok("app")).RequireAuthorization(AppPolicy); + }); + })) + .StartAsync(); + + _client = _host.GetTestClient(); + } + + public async ValueTask DisposeAsync() + { + _client.Dispose(); + await _host.StopAsync(); + _host.Dispose(); + } + + private static HttpRequestMessage Request(string path, params (string type, string value)[] claims) + { + var req = new HttpRequestMessage(HttpMethod.Get, path); + var encoded = string.Join("|", claims.Select(c => $"{c.type}={c.value}")); + req.Headers.Authorization = new AuthenticationHeaderValue(TestAuthHandler.SchemeName, encoded); + return req; + } + + [Fact] + public async Task RestaurantsApp_Allows_AppTokenWithRoleAndAllowListedAzp() + { + var resp = await _client.SendAsync( + Request("/system", ("roles", Role), ("azp", AllowedAppId), ("tid", "tenant-a")), + TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.OK); + } + + [Fact] + public async Task RestaurantsApp_Rejects_TokenWithScp_MixedClaims() + { + var resp = await _client.SendAsync( + Request("/system", ("roles", Role), ("scp", "restaurants.read"), ("azp", AllowedAppId)), + TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.Forbidden); + } + + [Fact] + public async Task RestaurantsApp_Rejects_AppToken_WhenAzpNotAllowListed() + { + var resp = await _client.SendAsync( + Request("/system", ("roles", Role), ("azp", Guid.NewGuid().ToString())), + TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.Forbidden); + } + + [Fact] + public async Task RestaurantsApp_Rejects_UserToken() + { + var resp = await _client.SendAsync( + Request("/system", ("scp", "restaurants.read")), + TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.Forbidden); + } + + [Fact] + public async Task RestaurantsApp_Rejects_AllowListedAppWithoutRequiredRole() + { + // Regression guard for the [Authorize(Roles=...)] anti-pattern: with the named policy in place, + // an allow-listed app that is MISSING the required role MUST be rejected. If a future author + // re-introduces [Authorize(Roles="...")] with MapInboundClaims=false, the silent role-check + // failure would let this request through and this test would fail. + var resp = await _client.SendAsync( + Request("/system", ("roles", "SomeOther.Role"), ("azp", AllowedAppId)), + TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.Forbidden); + } + + [Fact] + public async Task RestaurantsApp_Rejects_AllowListedAppWithNoRolesClaim() + { + // Same regression guard: an allow-listed app with no `roles` claim at all must be rejected. + var resp = await _client.SendAsync( + Request("/system", ("azp", AllowedAppId), ("tid", "tenant-a")), + TestContext.Current.CancellationToken); + resp.StatusCode.ShouldBe(HttpStatusCode.Forbidden); + } +}