Skip to content

fix(auth): deny by default with an authorization FallbackPolicy - #327

Open
rlorenzo wants to merge 2 commits into
refactor/raps-members-apicontrollerfrom
fix/authorization-fallback-policy
Open

fix(auth): deny by default with an authorization FallbackPolicy#327
rlorenzo wants to merge 2 commits into
refactor/raps-members-apicontrollerfrom
fix/authorization-fallback-policy

Conversation

@rlorenzo

@rlorenzo rlorenzo commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Stacked on #326.

Why

Attribute-routed controllers aren't covered by the RequireAuthorization() on the conventional routes, so a controller that forgot [Authorize] was reachable unauthenticated. That is how MembersController ended up exposed in #324. This makes the failure mode "denied" instead of "public".

What changed

options.FallbackPolicy = options.DefaultPolicy in Program.cs, plus [AllowAnonymous] on what answers anonymously by design: CMSController (public files, checks per-file permissions itself), LayoutController (permission-filtered nav), and LoggedInUserController (must answer before the caller knows if anyone is signed in). HomeController's sign-in and error actions already had it.

The health endpoints are the non-obvious part. /health, /health/detail, and the /healthchecks UI are endpoint mappings, not controllers, so they carry no authorization metadata and the fallback would have caught all three. Their own comments explain why that hurts: Jenkins has no CAS credentials, and /health/detail is deliberately reachable when auth is degraded. All three now have .AllowAnonymous(). Hangfire already had .RequireAuthorization().

Before merging

The endpoint audit here was static, from routing and attributes. Worth exercising sign-in, the public nav, and a Jenkins /health probe on TEST first.

@codecov-commenter

codecov-commenter commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 0% with 3 lines in your changes missing coverage. Please review.
✅ Project coverage is 42.54%. Comparing base (e96a4a8) to head (b2ac674).

Files with missing lines Patch % Lines
web/Classes/HealthChecks/HealthCheckExtensions.cs 0.00% 3 Missing ⚠️
Additional details and impacted files
@@                         Coverage Diff                          @@
##           refactor/raps-members-apicontroller     #327   +/-   ##
====================================================================
  Coverage                                42.54%   42.54%           
====================================================================
  Files                                      994      994           
  Lines                                    49933    49933           
  Branches                                  5899     5899           
====================================================================
  Hits                                     21244    21244           
  Misses                                   27744    27744           
  Partials                                   945      945           
Flag Coverage Δ
backend 40.54% <0.00%> (ø)
frontend 58.96% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
web/Areas/CMS/Controllers/CMSController.cs 0.00% <ø> (ø)
web/Controllers/LayoutController.cs 0.00% <ø> (ø)
web/Controllers/LoggedInUserController.cs 0.00% <ø> (ø)
web/Classes/HealthChecks/HealthCheckExtensions.cs 0.00% <0.00%> (ø)

@rlorenzo
rlorenzo marked this pull request as ready for review August 28, 2026 18:34
@rlorenzo
rlorenzo requested a lite review from Copilot August 28, 2026 18:36

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR hardens VIPER’s authorization posture by introducing an ASP.NET Core FallbackPolicy so endpoints without explicit authorization metadata are denied by default, preventing accidental public exposure of attribute-routed controllers.

Changes:

  • Set options.FallbackPolicy = options.DefaultPolicy to deny endpoints lacking auth metadata.
  • Explicitly opt specific controllers and health-check endpoint mappings out of the fallback via [AllowAnonymous] / .AllowAnonymous().
  • Add a unit test that pins the three intentionally-anonymous controllers to require [AllowAnonymous].

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
web/Program.cs Adds the authorization fallback policy to deny-by-default for endpoints without auth metadata.
web/Controllers/LoggedInUserController.cs Marks the logged-in-user API as explicitly anonymous.
web/Controllers/LayoutController.cs Marks the layout/nav API as explicitly anonymous.
web/Classes/HealthChecks/HealthCheckExtensions.cs Applies .AllowAnonymous() to health endpoints and the healthchecks UI mappings to preserve liveness/diagnostics.
web/Areas/CMS/Controllers/CMSController.cs Marks CMS file serving controller as explicitly anonymous (permission enforced internally).
test/Classes/AuthorizationFallbackTests.cs Adds a reflection-based test ensuring key controllers retain [AllowAnonymous].
Suppressed comments (1)

test/Classes/AuthorizationFallbackTests.cs:23

  • The test name and failure message read like they’re asserting the fallback policy behavior, but the assertion is specifically about presence of [AllowAnonymous] on the controller type. Renaming and tweaking the message makes failures more actionable.
        public void AnonymousControllers_OptOutOfTheFallbackPolicy(Type controller)
        {
            Assert.True(controller.GetCustomAttributes(typeof(AllowAnonymousAttribute), inherit: true).Length > 0,
                $"{controller.Name} answers anonymously by design and must carry [AllowAnonymous], "
                + "otherwise the FallbackPolicy denies it");

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread web/Program.cs Outdated
Comment thread test/Classes/AuthorizationFallbackTests.cs
@rlorenzo
rlorenzo force-pushed the fix/authorization-fallback-policy branch from 5f76d9e to 49e8e8c Compare August 28, 2026 18:46
@codecov-commenter

Copy link
Copy Markdown

Bundle Report

Bundle size has no change ✅

@rlorenzo
rlorenzo force-pushed the fix/authorization-fallback-policy branch from 49e8e8c to 885a541 Compare August 28, 2026 19:22
@rlorenzo
rlorenzo requested a lite review from Copilot August 28, 2026 19:22

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Copilot reviewed 6 out of 7 changed files in this pull request and generated 1 comment.

Comment thread test/Classes/AuthorizationFallbackTests.cs
@rlorenzo

Copy link
Copy Markdown
Contributor Author

@coderabbitai full review

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor
✅ Action performed

Full review finished.

@coderabbitai

coderabbitai Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 3fab7b77-1df2-4370-8fd1-54b4d5905040

📥 Commits

Reviewing files that changed from the base of the PR and between 2674a27 and 885a541.

📒 Files selected for processing (7)
  • .review-pr-ignored-327
  • test/Classes/AuthorizationFallbackTests.cs
  • web/Areas/CMS/Controllers/CMSController.cs
  • web/Classes/HealthChecks/HealthCheckExtensions.cs
  • web/Controllers/LayoutController.cs
  • web/Controllers/LoggedInUserController.cs
  • web/Program.cs

Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review.


📝 Walkthrough

Walkthrough

The application now applies the default authorization policy to unmatched endpoints. CMS, layout, logged-in-user, and health-check endpoints explicitly allow anonymous access. Tests verify the required controller annotations.

Changes

Authorization fallback enforcement

Layer / File(s) Summary
Fallback policy configuration
web/Program.cs
The authorization setup assigns DefaultPolicy to FallbackPolicy, requiring unmatched endpoints to satisfy the default policy.
Anonymous endpoint opt-outs
web/Areas/CMS/Controllers/CMSController.cs, web/Controllers/LayoutController.cs, web/Controllers/LoggedInUserController.cs, web/Classes/HealthChecks/HealthCheckExtensions.cs, test/Classes/AuthorizationFallbackTests.cs
The selected controllers use [AllowAnonymous]. The three health-check mappings use .AllowAnonymous(). Tests verify the controller annotations.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Merge Risk: ⚪ Minimal · up to 885a5

This PR changes authorization to deny by default while preserving explicitly public endpoints and adding focused coverage. No actionable merge-blocking risk remains at the current head beyond normal checks and review.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 33.33% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 3 functions across 6 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description check ✅ Passed The description clearly explains the authorization fallback change, the anonymous exceptions, and the required validation before merging.
Title check ✅ Passed The title clearly and concisely identifies the main change: denying access by default with an authorization FallbackPolicy.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
  • Fix all pre-merge checks with AI
✨ Finishing Touches 💡 2
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🛠️ Fix failing CI checks 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/authorization-fallback-policy

Comment @coderabbitai help to get the list of available commands.

Attribute-routed controllers are not covered by the RequireAuthorization()
on the conventional routes, so a controller that forgot [Authorize] was
reachable unauthenticated. That is what let MembersController drift.

- Exempt the endpoints that answer anonymously by design: the CAS sign-in
  and error actions already carried [AllowAnonymous], CMS serves public
  files and checks per-file permissions itself, Layout returns a
  permission-filtered nav, and LoggedInUser reports who is signed in
- Exempt the health endpoints, which are deliberately reachable when the
  auth subsystems are degraded and have no CAS credentials from Jenkins
@rlorenzo
rlorenzo force-pushed the fix/authorization-fallback-policy branch from 885a541 to 731c515 Compare August 29, 2026 00:54
The review-pr workflow writes .review-pr-ignored-<pr> in the repo root to
track dismissed threads. It was untracked but not ignored, so a git add -A
swept one into a commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants