Skip to content

Clear external auth on account claim updates - #8275

Merged
neilalexander merged 1 commit into
nats-io:mainfrom
0xasritha:fix/issue-8272
Jun 15, 2026
Merged

Clear external auth on account claim updates#8275
neilalexander merged 1 commit into
nats-io:mainfrom
0xasritha:fix/issue-8272

Conversation

@0xasritha

Copy link
Copy Markdown
Contributor

Closes #8272

Summary

  • Clear cached external authorization when refreshed account claims no longer enable external auth.
  • Add regression coverage for auth users, allowed accounts, and xkey state being removed by an account claim update.

Testing

  • go test ./server -run TestAuthCallout -count=1
  • golangci-lint run --timeout=5m --config=.golangci.yml ./server
  • codex review --base upstream/main

Signed-off-by: asritha <101123140+0xasritha@users.noreply.github.com>
@MauriceVanVeen

Copy link
Copy Markdown
Member

@claude, review.

@synadia-claude-reviewer

synadia-claude-reviewer Bot commented Jun 8, 2026

Copy link
Copy Markdown

Claude finished @MauriceVanVeen's task in 2m 14s —— View job


Code Review

  • Gather context and read changed files
  • Review server/accounts.go changes
  • Review server/auth_callout_test.go changes
  • Post review findings

Summary

This is a correct and minimal bug fix. When updateAccountClaimsWithRefresh is called with new claims that no longer have external authorization enabled, the extAuth field was not being cleared — meaning stale auth callout configuration could persist across claim refreshes. The fix adds the missing else branch.


server/accounts.go — Core Fix

accounts.go:3416-3423

if ac.HasExternalAuthorization() {
    a.extAuth = &jwt.ExternalAuthorization{}
    a.extAuth.AuthUsers.Add(ac.Authorization.AuthUsers...)
    a.extAuth.AllowedAccounts.Add(ac.Authorization.AllowedAccounts...)
    a.extAuth.XKey = ac.Authorization.XKey
} else {
    a.extAuth = nil  // ← new
}

Correctness: The change is properly placed inside the a.mu.Lock() block (line 3391), so concurrent access is handled correctly. The fix correctly mirrors the "set on enable / clear on disable" pattern used throughout the rest of updateAccountClaimsWithRefresh (e.g., a.usersRevoked, a.jsLimits).

Potential gap — no eviction of existing connections: The function walks connected clients at lines 3849–3911 and disconnects clients affected by revocations and connection limit changes, but there is no eviction of clients that were connected via the now-removed auth callout. Clearing extAuth prevents new callout-delegated connections, but existing ones remain active. Whether this is a concern depends on the threat model (whether a claim refresh that removes external auth should actively terminate existing auth-callout sessions), but it's worth noting as a potential follow-up.


server/auth_callout_test.go — Regression Test

TestAuthCalloutUpdateAccountClaimsClearsExternalAuthorization (line 398)

The test is well-structured:

  1. Creates a minimal server with defaultServerOptions (appropriate for unit-testing claim state without full server startup)
  2. Registers an account and applies claims with AuthUsers, AllowedAccounts, and XKey set
  3. Asserts all three fields are active
  4. Applies new claims without external auth and asserts all three fields are cleared

Coverage is thorough for the state management being fixed. Minor observations:

  • Pre-existing typo in isAllowedAcount (line 3357 of accounts.go, missing 'c'): the test correctly calls the existing function name — this is not introduced by this PR, but it could be fixed opportunistically.
  • The test does not cover partial updates (e.g., updating from one set of auth users to a different set), but that path is already guarded by the unconditional a.extAuth = &jwt.ExternalAuthorization{} re-allocation on line 3417, so it's not a risk.
  • There is no test asserting that new connection attempts are rejected after the claim update, but given the fix is purely state management and the auth path (auth.go:651) reads hasExternalAuth() directly, this is implicitly covered.

Verdict

Looks good. The fix is correct, minimal, and well-tested. The main open question is whether active connections via the removed auth callout should also be evicted, which may be tracked as a follow-up if the issue description doesn't already address it.

@MauriceVanVeen

Copy link
Copy Markdown
Member

Generally LGTM, but I'd agree that active connections using auth callout should be evicted if it gets disabled.

@neilalexander
neilalexander merged commit c16afd1 into nats-io:main Jun 15, 2026
45 of 46 checks passed
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.

JWT account updates leave stale external authorization enabled

3 participants