Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: simplify checker for access to namespaces #4666

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Feb 1, 2025

Closes #3702

What changed?

This PR greatly simplifies the check for access to namespace. If/when this is merged:

  • A user with permission to list namespaces will see all namespaces.
  • A user with permission to get configmaps in the namespace will include the namespace in the list the user is allowed to see. We could probably use any namespace resource for this check, but configmaps was selected mainly because all kube namespaces contains the kube-root-ca.crt configmap.

All the access to kube APIs impersonates the logged-in user in weave-gitops, so this should be a safe change. We might need to add additional RBAC checks in UI to avoid enabling features the logged-in user doesn't have access to, which will result in an error if used.

For now, I have left the old checker as "dead" code, including tests. Please let me know if I should delete it (we probably should).

Why was this change made?

The current logic appears complex, hard to maintain, and seems somehow disconnected from the fact that weave-gitops impersonate users. The current check also requires that a user has read access to secrets - which cannot be required for all use of weave-gitops, ref. #3702. If some features require RBAC to secrets, we should instead guard those features with an additional SelfSubjectAccessReview/SelfSubjectRulesReview check.

How was this change implemented?

How did you validate the change?

I have made tests unit testing the new checker. It's challenging to make my local workstation environment run weve-gitops, so I would appreciate some help in verifying that this change doesn't totally break the UI with errors.

Release notes

Documentation Changes

casibbald
casibbald previously approved these changes Feb 1, 2025
@mproffitt
Copy link
Contributor

For now, I have left the old checker as "dead" code

In general I would prefer not to leave dead code lying around and as long as this code has no further use, I would opt for removing it. We can always recover it from history if we decide some part of it is required in the future.

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.

Minimal rbac to view and reconcile resources in UI
3 participants