Close the review gaps from #1899: Cloud role gates, Helm write checks, no-access banner

[nadaverell]

Follow-up to #1899: closes the gaps from its review. RAD-578.

Must-fix

• Registered OCI chart sources are owner-only in Radar Cloud. Upgrade discovery picks the highest version across all registered prefixes, so any Cloud user, even one with no cluster RBAC, could add a prefix that feeds someone else's upgrade. Add and remove now take the owner gate, like the rest of Radar's own config, and the Track chart source dialog hides the controls from non-owners.
• Under Cloud, a request with no role group is refused by owner-gated endpoints. "No role" skips the gate so OSS keeps its own config, but under Cloud it meant the Hub's radar:system identity passed owner-only settings. OSS behaviour is unchanged.
• Per-user capability caches are keyed on username and groups. They now decide Helm writes. Keyed by username alone, a user who lost an IdP group kept the old answer for up to a minute.

Smaller fixes

• Release reads map only a Kubernetes denial to 403. A chart repository's own 401/403 is no longer reported as the caller's RBAC.
• When Kubernetes refuses the Helm release listing for issues, it is logged once per identity, so missing Helm alerts leave a trace.
• The no-access banner asks /api/auth/me?check=namespaces, which runs namespace discovery, so it can't vanish when the permission cache expires. It keeps its last definite answer through a check that couldn't decide, and polls until access is confirmed.
• Helm actions stay disabled until the per-namespace permission check succeeds, including when it fails.

Tests

• New: a user losing a group (end to end through requireHelmWrite); the Cloud no-role deny; the owner gate on chart sources; repository 401/403 vs RBAC denial; Impersonate-Group on streaming rollback; the banner's discovery path failing closed.
• go test (both modules), tsc, and web vitest pass.

Verified on EKS (radar-test-nonprod), proxy auth with simulated users

• Banner still shown 150 s in, past the permission cache expiry; it cleared by itself 64 s after a binding was added.
• A Hub viewer whose IdP group grants edit in one namespace: Helm rollback and uninstall work there and are refused elsewhere. Dropping the group takes effect on the next request.
• A user who can list pods but not Secrets: the Helm-denied log line appears once across repeated calls.
• Not live-testable locally: the Cloud no-role deny and the owner gate under real Cloud mode, which only accepts requests over the Hub tunnel. Unit tests cover both.

Docs: skyhook-dev/radar-docs#129 (owner-only chart sources).

---

Note

Medium Risk
Changes authorization for Cloud config (OCI sources, no-tier denial) and Helm write caching keyed on groups; incorrect behavior would block legitimate admins or briefly allow stale permissions, but scope is bounded and covered by new tests.

Overview
Follow-up to #1899 that tightens Radar Cloud config auth, Helm/RBAC correctness, and the no-namespace-access UX.

Cloud config: Registered OCI chart sources are now owner-only in Cloud (requireHelmSourceConfigWrite), with matching UI in the track-chart-source dialog. In Cloud mode, callers without a radar:owner|member|viewer tier (e.g. Hub radar:system) are denied on tier-gated Radar config instead of bypassing the gate like OSS.

RBAC caching & Helm: Per-user capability caches (including namespace Helm-write SARs) are keyed by username + groups via IdentityCacheKey, so revoking an IdP group stops reusing stale “can write” for up to the TTL. Release read errors map to 403 only for Kubernetes RBAC (isReleaseReadForbidden), not chart-repo 401/403. Helm issues omit logging is once per identity when Secret list is forbidden.

No-access banner: /api/auth/me?check=namespaces runs namespace discovery; the banner polls that path, keeps the last boolean noNamespaceAccess, and Helm actions stay disabled while per-namespace write capability is unknown (helmWriteUnknown).

Docs note owner-only chart sources and no-role Cloud denial on config.

^{Reviewed by Cursor Bugbot for commit 7c39b11. Bugbot is set up for automated code reviews on this repo. Configure here.}

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Close Cloud authorization, Helm permission, and access-banner gaps

🐞 Bug fix 🧪 Tests 📝 Documentation 🕐 40+ Minutes

AI Description

• Restricts Cloud chart-source changes to owners and denies roleless Cloud configuration requests.
• Keys capability decisions by user groups and disables Helm actions until namespace permissions are
 confirmed.
• Preserves no-access warnings through inconclusive checks and distinguishes repository errors from
 Kubernetes denials.

Diagram

graph TD
  U["Authenticated user"] --> R{"Owner gate"} -- "Owner" --> O["OCI sources"]
  U --> K["Kubernetes SAR"] --> I["Identity cache"] --> H["Helm writes"]
  K --> N["Namespace discovery"] --> B["Access banner"]

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Run an uncached SAR for every Helm write

• ➕ Kubernetes RBAC changes would affect write checks immediately, without a capability-cache TTL.
• ➖ Adds an API-server request and latency to every write; does not replace the UI's namespace capability check.

Recommendation: Keep the identity-keyed cache and existing namespace write gate: they prevent a changed group set from inheriting its previous verdict without adding a SAR to every write. The remaining cache TTL for Kubernetes RBAC changes is a separate trade-off.

Files changed (15) +249 / -53

Bug fix (9) +124 / -42

handlers.goLimit release-read 403 responses to Kubernetes denials +14/-4

Limit release-read 403 responses to Kubernetes denials

• Distinguishes Kubernetes RBAC errors from chart-repository 401/403 responses. Clarifies that OCI source changes are shared configuration subject to the configured write gate.

internal/helm/handlers.go

capabilities.goKey user capability caches by groups +10/-8

Key user capability caches by groups

• Uses a canonical username-and-groups identity for both global and namespace capability caches, preventing changed group memberships from reusing earlier SAR verdicts.

internal/k8s/capabilities.go

issues_handler.goLog forbidden Helm issue listings once +8/-0

Log forbidden Helm issue listings once

• Emits a one-time denial log when Kubernetes prevents release-Secret listing, making omitted Helm issues observable without logging every poll.

internal/server/issues_handler.go

server.goEnforce owner access and support explicit namespace checks +35/-15

Enforce owner access and support explicit namespace checks

• Wires OCI source mutations through the Cloud owner gate and denies roleless requests in Cloud mode. Adds opt-in namespace discovery to auth/me and returns an explicit access result when cached permissions are known.

internal/server/server.go

permissions.goExpose the canonical identity cache key +6/-0

Expose the canonical identity cache key

• Makes the existing username-and-groups key available to capability caches outside the auth package.

pkg/auth/permissions.go

client.tsPoll namespace access and fail closed on unknown Helm permissions +27/-5

Poll namespace access and fail closed on unknown Helm permissions

• Adds a discovery-backed namespace-access query that polls until access is explicitly confirmed. Disables authenticated Helm actions while the target namespace's write permission is unknown.

web/src/api/client.ts

NoClusterAccessBanner.tsxPreserve the last definite access-banner answer +10/-7

Preserve the last definite access-banner answer

• Uses the namespace-discovery query and retains the last known access result when a later check cannot determine permissions.

web/src/components/NoClusterAccessBanner.tsx

TrackChartSourceDialog.tsxHide chart-source edits from non-owners +10/-2

Hide chart-source edits from non-owners

• Disables add and remove controls for non-owners in Cloud and explains the owner-only restriction.

web/src/components/helm/TrackChartSourceDialog.tsx

CapabilitiesContext.tsxExpose unknown namespace Helm-write status +4/-1

Expose unknown namespace Helm-write status

• Marks Helm write permission as unknown when a namespace check has no result because it is pending or failed, rather than relying on the global capability.

web/src/contexts/CapabilitiesContext.tsx

Tests (5) +124 / -10

helm_write_gate_test.goTest lost-group writes and repository errors +17/-0

Test lost-group writes and repository errors

• Verifies that removing an IdP group denies a subsequent namespace Helm write. Covers Kubernetes denial text versus chart-repository authorization errors.

internal/helm/helm_write_gate_test.go

rollback_impersonation_test.goAssert group impersonation during streaming rollback +8/-0

Assert group impersonation during streaming rollback

• Checks that rollback requests carry the caller's IdP group alongside Impersonate-User.

internal/helm/rollback_impersonation_test.go

capabilities_cache_test.goTest namespace capability cache identity keys +14/-1

Test namespace capability cache identity keys

• Adapts the invalidation test to the new key and verifies that group removal changes the key while group reordering does not.

internal/k8s/capabilities_cache_test.go

cloud_role_gate_test.goCover roleless Cloud and chart-source owner gates +58/-0

Cover roleless Cloud and chart-source owner gates

• Adds tests for the OSS no-role bypass, Cloud roleless denial, and owner-only chart-source configuration.

internal/server/cloud_role_gate_test.go

server_auth_test.goTest definite and inconclusive namespace answers +27/-9

Test definite and inconclusive namespace answers

• Expects explicit false results for known access and an absent result when the banner's discovery cannot run.

internal/server/server_auth_test.go

Documentation (1) +1 / -1

authentication.mdDocument Cloud configuration gates +1/-1

Document Cloud configuration gates

• Clarifies that registered OCI chart sources require the Cloud owner role and that roleless Cloud identities cannot change Radar configuration.

docs/authentication.md

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (1) 📘 Rule violations (1) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. Chart repository failures lack error logs ✓ Resolved

Description

writeReleaseReadError now routes a chart repository's 401 or 403 to a 500 response without logging
the triggering error. When a release read encounters that repository failure, the affected handlers
call this helper directly, so the response is sent without the required preceding module/action log.

Code

internal/helm/handlers.go[34]

+	if isReleaseReadForbidden(err) {

Evidence

The new condition excludes repository 401/403 errors from the forbidden branch. The helper sends
other errors as 500, and a release-read handler calls it without first logging the error.

Rule 3036628: Log 500 errors with standardized module/action format before writing the response
internal/helm/handlers.go[33-46]
internal/helm/handlers.go[240-249]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Repository authorization failures newly reach the 500 branch without a preceding standardized error log.
## Fix Focus Areas
- internal/helm/handlers.go[33-39]
- internal/helm/handlers.go[240-249]
## Recommended Fix
Before each affected handler sends a 500 through the shared helper, log the triggering error using the required `[module] Failed to <action> %s/%s: %v` format with its namespace and release name.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Disconnected namespace checks return success 📘 Rule violation ☼ Reliability

Description

handleAuthMe now invokes cluster-backed namespace discovery for check=namespaces but never calls
s.requireConnected(w) before that operation. When the cluster is disconnected, the handler skips
discovery and still writes a successful auth response with no namespace-access answer instead of the
connectivity error required for cluster-touching handlers.

Code

internal/server/server.go[R5226-5228]

+			if r.URL.Query().Get("check") == "namespaces" {
+				s.getUserNamespaces(r, nil)
+			}

Evidence

The new query branch calls getUserNamespaces, which reaches the Kubernetes client, resource cache
and namespace discovery. handleAuthMe has no requireConnected call and writes its normal JSON
response when the connection check is false.

Rule 3036634: Cluster-touching HTTP handlers must enforce connectivity with requireConnected
internal/server/server.go[5193-5235]
internal/server/server.go[5318-5374]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The namespace-discovery request uses cluster resources without the required connectivity guard, while plain auth requests must remain available during startup.
## Fix Focus Areas
- internal/server/server.go[5193-5235]
## Recommended Fix
Separate the cluster-touching namespace check from the plain auth response path. Call `s.requireConnected(w)` before discovery in the cluster-touching handler and preserve the existing startup behavior for plain `/auth/me` requests.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. Helm actions enable after an inconclusive check 🐞 Bug ≡ Correctness

Description

helmWriteUnknown treats any returned namespace capability object as a definite answer, but the
server can return global helmWrite after a namespace permission check errors. When the global
value is true, Helm controls become enabled despite the namespace check being inconclusive, and the
subsequent write request is refused.

Code

web/src/contexts/CapabilitiesContext.tsx[170]

+    helmWriteUnknown: Boolean(namespace && !nsCaps && (isPending || error)),

Evidence

The server logs namespace-check errors but still writes a successful capabilities response. Its
merge retains a global grant on an individual check error, while the error flags are not serialized;
the new frontend check therefore sees nsCaps and allows the action. The Helm write handler
separately rejects an unverifiable check.

internal/server/server.go[1380-1392]
internal/server/server.go[1443-1454]
internal/k8s/capabilities.go[167-176]
web/src/contexts/CapabilitiesContext.tsx[164-171]
web/src/api/client.ts[2335-2345]
internal/helm/handlers.go[1242-1259]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The namespace capabilities endpoint can report a global Helm grant when its namespace check errors, so the new client-side unknown check treats an inconclusive result as permission to act.
## Fix Focus Areas
- internal/server/server.go[1380-1392]
- internal/server/server.go[1443-1454]
- web/src/contexts/CapabilitiesContext.tsx[164-171]
## Recommended Fix
Expose a definite namespace Helm-check status in the response, or fail the namespace request when that check cannot be completed. Make `helmWriteUnknown` use that status rather than the presence of a response object.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

View medium (1)

4. A new user can see the prior user's warning ✓ Resolved

Description

NoClusterAccessBanner retains its last definite response without checking whether that response
belongs to the current authenticated identity. If proxy authentication changes the caller while the
banner remains mounted and the next namespace discovery cannot answer, the banner continues
displaying the prior username, groups, and no-access verdict.

Code

web/src/components/NoClusterAccessBanner.tsx[R17-20]

+  const [me, setMe] = useState<typeof data>()
useEffect(() => {
-    void refetch()
-  }, [refetch])
+    if (data && typeof data.noNamespaceAccess === 'boolean') setMe(data)
+  }, [data])

Evidence

The new local state updates only for a boolean verdict, whereas the polling endpoint returns the
current username and groups even when discovery cannot populate a verdict. A failed discovery leaves
noNamespaceAccess absent, so the effect does not replace a retained answer from a different
caller.

web/src/components/NoClusterAccessBanner.tsx[11-30]
web/src/api/client.ts[2278-2286]
internal/server/server.go[5203-5210]
internal/server/server.go[5226-5230]
internal/server/server.go[5326-5350]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The banner preserves a previous caller's no-access answer when the current caller's discovery returns no definite answer.
## Fix Focus Areas
- web/src/components/NoClusterAccessBanner.tsx[11-20]
- web/src/api/client.ts[2278-2286]
## Recommended Fix
Associate the retained verdict with its username and groups, and clear it when the current response identifies a different caller. Preserve an undecided response only for the same identity.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

5. Helm-denied log fires once per username, forever ✓ Resolved

Description

nativeHelmIssuesForRequest records denials in helmIssuesDeniedLogged under the username alone,
and the package-level map never expires or clears entries. After one denial is logged, another
denial for that user under different groups, after a cluster context change, or after access is
restored and later lost produces no diagnostic when Helm issue listing fails.

Code

internal/server/issues_handler.go[R221-225]

+		} else if _, seen := helmIssuesDeniedLogged.LoadOrStore(username, struct{}{}); !seen {
+			// Logged once per identity: the alerts worker polls this, and a
+			// cluster without a Secret-read binding would otherwise drop Helm
+			// alerts with no trace anywhere.
+			log.Printf("[issues] Helm release issues omitted for %q: Kubernetes denied listing release Secrets", username)

Evidence

The listing call receives both username and groups, but helmIssuesDeniedLogged.LoadOrStore stores
only the username in a package-level map with no expiry or deletion. This differs from the
group-aware identity key used elsewhere in the PR, and the denial map is not included in cluster
context-switch cache invalidation.

internal/server/issues_handler.go[194-194]
internal/server/issues_handler.go[218-228]
pkg/auth/permissions.go[105-109]
internal/server/issues_handler.go[194-225]
pkg/auth/permissions.go[87-108]
internal/server/namespace_scope.go[166-180]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Helm issue-listing denials are deduplicated by username indefinitely, suppressing diagnostics for changed groups, a different cluster context, or a denial that returns after access succeeds.
## Fix Focus Areas
- internal/server/issues_handler.go[194-225]
- pkg/auth/permissions.go[105-108]
## Recommended Fix
Key denial deduplication with `pkgauth.IdentityCacheKey(username, groups)`. Allow a later denial to be logged by storing a timestamp and logging again after a TTL (for example, one hour), or by deleting the entry when listing succeeds. Reset or scope the deduplication state on cluster changes so denials in another cluster remain visible.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗