From 7e9b9c42d12ee99e866c2a7337de468dce2e8478 Mon Sep 17 00:00:00 2001 From: atbagan Date: Sat, 22 Aug 2026 10:57:00 -0500 Subject: [PATCH] fix(auth): stop treating any-team admins as platform admins When a request carries no X-Butler-Team header, the cluster, addon, GitOps and certificate checks short-circuited on UserSession.IsAdmin, which is true for an admin of any team. An admin of team alpha could therefore read, scale, delete, enable GitOps on and rotate certificates for every other team's clusters by omitting the header. This is the path the portal proxy uses when no team is selected. Platform roles are now checked explicitly: platform admins and viewers see every cluster, platform admins operate on any cluster, and everyone else is limited to teams where they hold the needed role. POST /teams/{name}/providers/test gets its own handler that requires an operate role on the named team; it previously reused the platform handler with no team check. --- internal/api/handlers/addons.go | 2 +- internal/api/handlers/certificates.go | 18 +--- internal/api/handlers/clusters.go | 18 ++-- .../api/handlers/clusters_operate_test.go | 101 ++++++++++++++++++ internal/api/handlers/gitops.go | 2 +- internal/api/handlers/providers.go | 18 ++++ internal/api/router.go | 2 +- 7 files changed, 136 insertions(+), 25 deletions(-) create mode 100644 internal/api/handlers/clusters_operate_test.go diff --git a/internal/api/handlers/addons.go b/internal/api/handlers/addons.go index 484ce69..a6e5853 100644 --- a/internal/api/handlers/addons.go +++ b/internal/api/handlers/addons.go @@ -1151,7 +1151,7 @@ func (h *AddonsHandler) checkOperatePermission(user *auth.UserSession, cluster * return nil } - if user.IsAdmin() { + if user.PlatformRole == auth.RoleAdmin { return nil } diff --git a/internal/api/handlers/certificates.go b/internal/api/handlers/certificates.go index 31984a8..7be46ae 100644 --- a/internal/api/handlers/certificates.go +++ b/internal/api/handlers/certificates.go @@ -300,13 +300,8 @@ func (h *CertificateHandler) GetCertificatesByCategory(w http.ResponseWriter, r // checkClusterAccess verifies the user has access to a cluster based on its teamRef. func (h *CertificateHandler) checkClusterAccess(user *auth.UserSession, cluster *unstructured.Unstructured) error { - // Platform admins can access all clusters - if user.IsPlatformAdmin { - return nil - } - - // Admins can access all clusters - if user.IsAdmin() { + // Platform roles can read certificates of all clusters + if user.IsPlatformViewerOrAbove() { return nil } @@ -329,12 +324,7 @@ func (h *CertificateHandler) checkClusterAccess(user *auth.UserSession, cluster // canRotateCertificates checks if the user has operator role for the cluster's team. func (h *CertificateHandler) canRotateCertificates(user *auth.UserSession, cluster *unstructured.Unstructured) bool { // Platform admins can rotate any certificates - if user.IsPlatformAdmin { - return true - } - - // Admins can rotate any certificates - if user.IsAdmin() { + if user.PlatformRole == auth.RoleAdmin { return true } @@ -350,7 +340,7 @@ func (h *CertificateHandler) canRotateCertificates(user *auth.UserSession, clust // canRotateCA checks if the user has admin role for CA rotation. func (h *CertificateHandler) canRotateCA(user *auth.UserSession, cluster *unstructured.Unstructured) bool { // Platform admins can rotate CA - if user.IsPlatformAdmin { + if user.PlatformRole == auth.RoleAdmin { return true } diff --git a/internal/api/handlers/clusters.go b/internal/api/handlers/clusters.go index 7a5a9c7..c7a9491 100644 --- a/internal/api/handlers/clusters.go +++ b/internal/api/handlers/clusters.go @@ -75,14 +75,15 @@ func (h *ClusterHandler) checkClusterAccess(user *auth.UserSession, cluster *uns return nil } - // No team context - use legacy behavior - // Admins can access all clusters - if user.IsAdmin() { + // No team context: platform roles see every cluster; everyone else + // sees the clusters of teams they belong to. Being an admin of some + // other team grants nothing here. + if user.IsPlatformViewerOrAbove() { return nil } // If cluster has no teamRef, it's a platform-level cluster - // Only admins should see these (already checked above) + // Only platform roles should see these (already checked above) if !found || clusterTeam == "" { return fmt.Errorf("forbidden: cluster is not associated with any team") } @@ -111,8 +112,9 @@ func (h *ClusterHandler) checkOperatePermission(user *auth.UserSession, teamRef return nil } - // No team context - use legacy behavior - if user.IsAdmin() { + // No team context: platform admins may operate on any cluster; + // everyone else needs an operate-capable role on the cluster's team. + if user.PlatformRole == auth.RoleAdmin { return nil } @@ -163,8 +165,8 @@ func (h *ClusterHandler) List(w http.ResponseWriter, r *http.Request) { if clusterTeam != user.SelectedTeam { continue } - } else if !user.IsAdmin() { - // No team context and not admin - filter by team membership + } else if !user.IsPlatformViewerOrAbove() { + // No team context and no platform role - filter by team membership if clusterTeam == "" { continue } diff --git a/internal/api/handlers/clusters_operate_test.go b/internal/api/handlers/clusters_operate_test.go new file mode 100644 index 0000000..6fb89af --- /dev/null +++ b/internal/api/handlers/clusters_operate_test.go @@ -0,0 +1,101 @@ +/* +Copyright 2026 The Butler Authors. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + + http://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +package handlers + +import ( + "testing" + + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + + "github.com/butlerdotdev/butler-server/internal/auth" +) + +func teamCluster(team string) *unstructured.Unstructured { + spec := map[string]interface{}{} + if team != "" { + spec["teamRef"] = map[string]interface{}{"name": team} + } + return &unstructured.Unstructured{Object: map[string]interface{}{"spec": spec}} +} + +// An admin of team alpha with no platform role. Before this change +// IsAdmin() made this session pass every no-header check. +func alphaAdmin() *auth.UserSession { + return &auth.UserSession{Teams: []auth.TeamMembership{{Name: "alpha", Role: auth.RoleAdmin}}} +} + +func TestNoHeaderChecks_TeamAdminIsNotPlatformAdmin(t *testing.T) { + ch := &ClusterHandler{} + ah := &AddonsHandler{} + gh := &GitOpsHandler{} + certs := &CertificateHandler{} + user := alphaAdmin() + + if err := ch.checkClusterAccess(user, teamCluster("alpha")); err != nil { + t.Fatalf("own-team visibility: %v", err) + } + if err := ch.checkClusterAccess(user, teamCluster("beta")); err == nil { + t.Fatal("team admin of alpha could see beta's cluster") + } + if err := ch.checkOperatePermission(user, "beta", "delete"); err == nil { + t.Fatal("team admin of alpha could operate on beta") + } + if err := ch.checkOperatePermission(user, "alpha", "delete"); err != nil { + t.Fatalf("own-team operate: %v", err) + } + if err := ah.checkOperatePermission(user, teamCluster("beta"), "install"); err == nil { + t.Fatal("team admin of alpha could install addons on beta") + } + if err := gh.checkOperatePermission(user, teamCluster("beta"), "enable"); err == nil { + t.Fatal("team admin of alpha could enable gitops on beta") + } + if err := certs.checkClusterAccess(user, teamCluster("beta")); err == nil { + t.Fatal("team admin of alpha could read beta's certificates") + } + if certs.canRotateCertificates(user, teamCluster("beta")) { + t.Fatal("team admin of alpha could rotate beta's certificates") + } + if !certs.canRotateCertificates(user, teamCluster("alpha")) { + t.Fatal("team admin of alpha should rotate alpha's certificates") + } +} + +func TestNoHeaderChecks_PlatformRoles(t *testing.T) { + ch := &ClusterHandler{} + certs := &CertificateHandler{} + admin := &auth.UserSession{PlatformRole: auth.RoleAdmin} + viewer := &auth.UserSession{PlatformRole: auth.RoleViewer} + + if err := ch.checkClusterAccess(viewer, teamCluster("beta")); err != nil { + t.Fatalf("platform viewer visibility: %v", err) + } + if err := ch.checkOperatePermission(viewer, "beta", "scale"); err == nil { + t.Fatal("platform viewer could operate") + } + if err := ch.checkOperatePermission(admin, "", "scale"); err != nil { + t.Fatalf("platform admin operate on team-less cluster: %v", err) + } + if err := certs.checkClusterAccess(viewer, teamCluster("beta")); err != nil { + t.Fatalf("platform viewer certificate read: %v", err) + } + if certs.canRotateCertificates(viewer, teamCluster("beta")) { + t.Fatal("platform viewer could rotate certificates") + } + if !certs.canRotateCA(admin, teamCluster("")) { + t.Fatal("platform admin should rotate CA of team-less cluster") + } +} diff --git a/internal/api/handlers/gitops.go b/internal/api/handlers/gitops.go index a4cc673..8e3e96b 100644 --- a/internal/api/handlers/gitops.go +++ b/internal/api/handlers/gitops.go @@ -2396,7 +2396,7 @@ func (h *GitOpsHandler) checkOperatePermission(user *auth.UserSession, cluster * return nil } - if user.IsAdmin() { + if user.PlatformRole == auth.RoleAdmin { return nil } diff --git a/internal/api/handlers/providers.go b/internal/api/handlers/providers.go index 4a2b7ef..35145a0 100644 --- a/internal/api/handlers/providers.go +++ b/internal/api/handlers/providers.go @@ -185,6 +185,24 @@ func (h *ProvidersHandler) Get(w http.ResponseWriter, r *http.Request) { writeJSON(w, http.StatusOK, provider.Object) } +// TestTeamConnection tests provider credentials on behalf of a team. +// Same body as TestConnection, but the caller must be able to manage +// the named team's providers; the platform-scoped route is gated by +// middleware instead. +func (h *ProvidersHandler) TestTeamConnection(w http.ResponseWriter, r *http.Request) { + teamName := chi.URLParam(r, "name") + user := auth.UserFromContext(r.Context()) + if user == nil { + writeError(w, http.StatusUnauthorized, "unauthorized") + return + } + if !user.CanOperateTeam(teamName) { + writeError(w, http.StatusForbidden, fmt.Sprintf("you don't have permission to manage providers for team '%s'", teamName)) + return + } + h.TestConnection(w, r) +} + // TestConnection tests provider credentials without creating anything. func (h *ProvidersHandler) TestConnection(w http.ResponseWriter, r *http.Request) { var req CreateProviderRequest diff --git a/internal/api/router.go b/internal/api/router.go index 9405a06..55ea033 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -504,7 +504,7 @@ func NewRouter(cfg RouterConfig) (http.Handler, error) { // Team provider management (team members can list, team admins can create/delete) r.Get("/teams/{name}/providers", providerHandler.ListTeamProviders) r.Post("/teams/{name}/providers", providerHandler.CreateTeamProvider) - r.Post("/teams/{name}/providers/test", providerHandler.TestConnection) + r.Post("/teams/{name}/providers/test", providerHandler.TestTeamConnection) r.Delete("/teams/{name}/providers/{namespace}/{providerName}", providerHandler.DeleteTeamProvider) // Team environment management (ADR-009). The Team admission