Skip to content

fix: add events read permission to project RBAC roles#822

Merged
Gkrumbach07 merged 1 commit intomainfrom
worktree-bad-perm
Mar 5, 2026
Merged

fix: add events read permission to project RBAC roles#822
Gkrumbach07 merged 1 commit intomainfrom
worktree-bad-perm

Conversation

@Gkrumbach07
Copy link
Copy Markdown
Contributor

Summary

  • Adds events read access (get, list, watch) to ambient-project-view, ambient-project-edit, and ambient-project-admin ClusterRoles
  • Fixes 403 error when the frontend calls GetSessionPodEvents — the handler uses the user's own K8s token but the RBAC roles were missing permission to list events in the core API group

Test plan

  • Deploy updated ClusterRoles to a cluster
  • Verify a user with any project role can view pod events for a session in the frontend without a 403

🤖 Generated with Claude Code

The GetSessionPodEvents handler uses the user's own K8s token to list
pod events, but the project ClusterRoles were missing permission to
list events in the core API group, causing a 403 for all users.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
@Gkrumbach07 Gkrumbach07 merged commit e5794d4 into main Mar 5, 2026
@Gkrumbach07 Gkrumbach07 deleted the worktree-bad-perm branch March 5, 2026 17:15
Copy link
Copy Markdown
Contributor

@ktdreyer ktdreyer left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Mar 5, 2026

Claude Code Review

Summary

This PR adds events read permissions (get, list, watch) to all three project RBAC roles (view, edit, admin) across both the individual ClusterRole YAML files and the combined cluster-roles.yaml. It is a minimal, focused fix resolving a real 403 error when the frontend calls GetSessionPodEvents via the user token. The change is correct and safe.

Issues by Severity

Blocker Issues

None

Critical Issues

None

Major Issues

None

Minor Issues

1. Dual-source RBAC definitions risk drift over time

Files: components/manifests/base/rbac/ambient-project-{admin,edit,view}-clusterrole.yaml AND components/manifests/base/rbac/cluster-roles.yaml

The events rules were correctly added to both locations in this PR. However, these files appear to define the same ClusterRoles in two places. Future PRs that update one set but not the other will cause silent role mismatches at deploy time. This is a pre-existing concern, but this PR increases its surface area.

Suggestion: Note in cluster-roles.yaml or a README that these files must stay in sync, or consolidate to a single Kustomize source of truth.

2. Test plan not completed before merge

The PR description has unchecked test plan items at merge time. For RBAC changes, confirming deployment and 403 resolution are the minimum validation steps.

Suggestion: For future RBAC PRs, require test plan confirmation before merge.

Positive Highlights

  • Correct security posture: permissions are strictly read-only (get, list, watch) with no write verbs, appropriate for monitoring use cases.
  • Consistent application: all three roles (view/edit/admin) get the same events permission, which is correct even for the least-privileged viewer role.
  • Aligns with auth architecture: GetSessionPodEvents already uses GetK8sClientsForRequest (user-scoped token), so K8s RBAC naturally enforces namespace isolation. The handler was correct; only the role was missing the permission.
  • Clear YAML comments make intent obvious to future reviewers.
  • Minimal diff: 27 additions, 0 deletions with no collateral changes.

Recommendations

  1. Investigate whether cluster-roles.yaml and the individual *-clusterrole.yaml files are both deployed to clusters. If so, consolidate via Kustomize to eliminate dual-maintenance drift risk.
  2. For future RBAC PRs, require test plan confirmation before merge.

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.

2 participants