Fix AllowOnlyAuthenticated access requirement causing 401 after successful login

[Copilot]

When configuring AccessRequirement = AllowOnlyAuthenticated with built-in username/password auth, users received HTTP 401 after a successful login due to two bugs in the authentication/authorization logic.

Changes

SilkierQuartzDefaultAuthorizationHandler

• Refactored chained negative-condition pattern into explicit, sequential checks per requirement type
• Consolidated the "not authenticated" guard into a single null-safe early-exit (context.User?.Identity?.IsAuthenticated != true) before requirement-specific logic, preventing NullReferenceException with anonymous ClaimsPrincipal instances
• AllowOnlyAuthenticated: now explicitly succeeds when user is authenticated, fails otherwise
• AllowOnlyUsersWithClaim: now explicitly succeeds/fails based on claim presence
• Unknown AccessRequirement values now default to context.Fail() (deny-by-default) instead of silently succeeding

AuthenticateController — GET Login

• Extracted IsUserAuthenticated() helper using an explicit switch that applies the correct check per AccessRequirement:
  • AllowAnonymous → always true
  • AllowOnlyAuthenticated → null-safe IsAuthenticated check only (no claim required)
  • AllowOnlyUsersWithClaim → null-safe IsAuthenticated && HasClaim
  • default → false (deny-by-default, consistent with the authorization handler)
• Previously, the GET handler always checked HasClaim regardless of the configured requirement, so users authenticated via an external identity provider (without the SilkierQuartz-specific claim) were incorrectly re-shown the login form even when AllowOnlyAuthenticated was set

Unit tests — SilkierQuartzDefaultAuthorizationHandlerTests

• Added 13 focused unit tests covering all security-sensitive branches of SilkierQuartzDefaultAuthorizationHandler:
  • AllowAnonymous: anonymous and authenticated principals both succeed
  • AllowOnlyAuthenticated: authenticated succeeds; unauthenticated, null-identity, and anonymous principals fail without exception; authenticated user without claim still succeeds
  • AllowOnlyUsersWithClaim: correct claim succeeds; missing/wrong claim, unauthenticated, and null-identity anonymous principals all fail without exception
  • Unknown AccessRequirement: deny-by-default (authenticated user is denied)
  • SkipDefaultRequirementHandler = true: context remains pending (neither succeed nor fail)

[Copilot]

Pull request overview

Fixes an authorization/authentication mismatch that caused users to get HTTP 401 after successfully logging in when AccessRequirement = AllowOnlyAuthenticated, by aligning the controller’s “already logged in” detection with the configured requirement and tightening the default authorization handler logic.

Changes:

• Updates SilkierQuartzDefaultAuthorizationHandler to use explicit, sequential checks per AccessRequirement, and to deny-by-default for unknown values.
• Updates AuthenticateController GET /Login to use a helper (IsUserAuthenticated()) that applies the correct rule per AccessRequirement (claim check only when required).

Reviewed changes

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

File	Description
src/SilkierQuartz/Controllers/AuthenticateController.cs	Adds IsUserAuthenticated() and uses it to decide whether to show the login form vs redirecting.
src/SilkierQuartz/Authorization/SilkierQuartzDefaultAuthorizationHandler.cs	Refactors requirement evaluation and changes unknown requirement handling to fail (deny-by-default).

[Copilot]

IsUserAuthenticated() treats every AccessRequirement other than AllowOnlyAuthenticated as “must have claim”. That makes the controller’s notion of authentication inconsistent with the authorization handler (which now deny-by-default for unknown values) and also doesn’t explicitly handle AllowOnlyUsersWithClaim/AllowAnonymous. Consider switching to an explicit switch/if chain: return true for AllowAnonymous, return IsAuthenticated for AllowOnlyAuthenticated, return IsAuthenticated && HasClaim for AllowOnlyUsersWithClaim, and return false for any unknown value to match the handler’s behavior.

Suggested change

	if (HttpContext.User == null || !HttpContext.User.Identity.IsAuthenticated)
	return false;
	if (authenticationOptions.AccessRequirement == SilkierQuartzAuthenticationOptions.SimpleAccessRequirement.AllowOnlyAuthenticated)
	return true;
	return HttpContext.User.HasClaim(authenticationOptions.SilkierQuartzClaim, authenticationOptions.SilkierQuartzClaimValue);
	switch (authenticationOptions.AccessRequirement)
	{
	case SilkierQuartzAuthenticationOptions.SimpleAccessRequirement.AllowAnonymous:
	return true;
	case SilkierQuartzAuthenticationOptions.SimpleAccessRequirement.AllowOnlyAuthenticated:
	return HttpContext.User != null
	&& HttpContext.User.Identity != null
	&& HttpContext.User.Identity.IsAuthenticated;
	case SilkierQuartzAuthenticationOptions.SimpleAccessRequirement.AllowOnlyUsersWithClaim:
	return HttpContext.User != null
	&& HttpContext.User.Identity != null
	&& HttpContext.User.Identity.IsAuthenticated
	&& HttpContext.User.HasClaim(authenticationOptions.SilkierQuartzClaim, authenticationOptions.SilkierQuartzClaimValue);
	default:
	return false;
	}

[Copilot]

context.User.Identity may be null when the request is unauthenticated (ClaimsPrincipal with no identities). Accessing context.User.Identity.IsAuthenticated can throw and turn an auth failure into a 500. Use a null-safe check (e.g., context.User?.Identity?.IsAuthenticated == true) before deciding to fail.

[maikebing]

@copilot apply changes based on this feedback

[Copilot]

Fixed in commit 1ebb56b. Changed context.User.Identity.IsAuthenticated to context.User?.Identity?.IsAuthenticated != true to safely handle anonymous principals with null identities. Also applied the same null-safe pattern and an explicit switch in IsUserAuthenticated() so both places are consistent.

[Copilot]

The updated requirement evaluation is security-sensitive (AllowOnlyAuthenticated vs AllowOnlyUsersWithClaim, plus deny-by-default for unknown values) but there are no unit tests covering these branches. Adding focused unit tests for SilkierQuartzDefaultAuthorizationHandler (authenticated/unauthenticated, with/without claim, and unknown requirement) would prevent regressions like the 401-after-login issue.

[maikebing]

@copilot apply changes based on this feedback

[Copilot]

Added 13 unit tests in commit fdf7db7 (test/SilkierQuartzDefaultAuthorizationHandlerTests.cs) covering all branches:

• AllowAnonymous — anonymous and authenticated principals both succeed
• AllowOnlyAuthenticated — authenticated succeeds; unauthenticated, null-identity anonymous principal, and anonymous all fail without throwing; authenticated user without the claim still succeeds (no claim required for this mode)
• AllowOnlyUsersWithClaim — correct claim succeeds; missing claim, wrong claim value, unauthenticated, and anonymous (null Identity) all fail without throwing
• Unknown AccessRequirement — deny-by-default, authenticated user is denied
• SkipDefaultRequirementHandler = true — context remains pending (neither succeeded nor failed)