From 1679300d2e101c321c328f393bf8d23dc4376f5b Mon Sep 17 00:00:00 2001 From: Shivam-nagar23 Date: Fri, 11 Sep 2026 08:31:15 +0530 Subject: [PATCH] feat: resolve RBAC inconsistencies for helm apps with unique identifiers When an external helm app is linked to the chart store, app_name holds the unique identifier (--) while display_name holds the release name. RBAC policies are always created against the release name, since that is what the listing APIs expose to the user. EnforcerUtil did not account for this: - GetHelmObjectByAppNameAndEnvId looked the app up by app_name only, so a release-name lookup returned ErrNoRows and the object collapsed to "//", making the app invisible to any non super-admin user. - Both it and GetHelmObject built the object from application.AppName, i.e. the unique identifier, which never matched the granted policy. - Neither substituted the "unassigned" project when team_id is 0, so an app linked without a project produced an empty project segment. EnforcerUtilHelm.GetAppRBACNameByInstalledAppId and GetAppRBACNameByInstalledAppIdAndTeamId had the same raw AppName problem. Resolve the app the same way getAppObject already does: reconstruct the unique identifier from the environment and try that first, falling back to the plain name for regular chart store apps, legacy rows and callers that already pass the identifier. Emit the display name via the new App.GetAppNameForRbac(), and fall back to the unassigned project when no project is assigned. Fixing the primitives covers all downstream call sites without touching the handlers. Co-Authored-By: Claude Opus 5 (1M context) --- internal/sql/repository/app/AppRepository.go | 12 ++ util/rbac/EnforcerUtil.go | 72 +++++-- util/rbac/EnforcerUtilHelm.go | 8 +- util/rbac/EnforcerUtilHelmObject_test.go | 212 +++++++++++++++++++ 4 files changed, 285 insertions(+), 19 deletions(-) create mode 100644 util/rbac/EnforcerUtilHelmObject_test.go diff --git a/internal/sql/repository/app/AppRepository.go b/internal/sql/repository/app/AppRepository.go index d237ec8f7a..538490fbc9 100644 --- a/internal/sql/repository/app/AppRepository.go +++ b/internal/sql/repository/app/AppRepository.go @@ -53,6 +53,18 @@ func (app *App) IsAppJobOrExternalType() bool { return len(app.DisplayName) > 0 } +// GetAppNameForRbac returns the app name that rbac objects are built with. +// For an external helm app linked to chart store, app_name holds the unique identifier +// (--) while display_name holds the release name. Rbac policies are +// always created against the release name, as that is what is shown to the user, so display_name must +// win whenever it is set. +func (app *App) GetAppNameForRbac() string { + if len(app.DisplayName) > 0 { + return app.DisplayName + } + return app.AppName +} + type AppRepository interface { SaveWithTxn(pipelineGroup *App, tx *pg.Tx) error Update(app *App) error diff --git a/util/rbac/EnforcerUtil.go b/util/rbac/EnforcerUtil.go index 51dcd1aaaf..b619a9d2e0 100644 --- a/util/rbac/EnforcerUtil.go +++ b/util/rbac/EnforcerUtil.go @@ -18,6 +18,7 @@ package rbac import ( "fmt" + bean3 "github.com/devtron-labs/devtron/api/helm-app/service/bean" "github.com/devtron-labs/devtron/pkg/app/dbMigration" repository2 "github.com/devtron-labs/devtron/pkg/cluster/environment/repository" bean2 "github.com/devtron-labs/devtron/pkg/k8s/application/bean" @@ -464,6 +465,8 @@ func (impl EnforcerUtilImpl) GetHelmObject(appId int, envId int) (string, string impl.logger.Errorw("error on fetching data for rbac object", "err", err) return fmt.Sprintf("%s/%s/%s", "", "", ""), "" } + appNameForRbac := application.GetAppNameForRbac() + teamName := getTeamNameForHelmRbac(application) clusterName := env.Cluster.ClusterName namespace := env.Namespace @@ -486,31 +489,26 @@ func (impl EnforcerUtilImpl) GetHelmObject(appId int, envId int) (string, string }*/ if environmentIdentifier2 == "" { - return fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName), "" + return fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac), "" } - return fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName), - fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier2, application.AppName) + return fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac), + fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier2, appNameForRbac) } func (impl EnforcerUtilImpl) GetHelmObjectByAppNameAndEnvId(appName string, envId int) (string, string) { - application, err := impl.appRepo.FindAppAndProjectByAppName(appName) - if err == pg.ErrMultiRows { - application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(appName) - if err != nil { - impl.logger.Errorw("error on fetching data for rbac object", "appName", appName, "err", err) - return fmt.Sprintf("%s/%s/%s", "", "", ""), "" - } - } + env, err := impl.environmentRepository.FindById(envId) if err != nil { impl.logger.Errorw("error on fetching data for rbac object", "err", err) return fmt.Sprintf("%s/%s/%s", "", "", ""), "" } - env, err := impl.environmentRepository.FindById(envId) + application, err := impl.getAppForHelmRbac(appName, env.ClusterId, env.Namespace) if err != nil { - impl.logger.Errorw("error on fetching data for rbac object", "err", err) + impl.logger.Errorw("error on fetching data for rbac object", "appName", appName, "err", err) return fmt.Sprintf("%s/%s/%s", "", "", ""), "" } + appNameForRbac := application.GetAppNameForRbac() + teamName := getTeamNameForHelmRbac(application) clusterName := env.Cluster.ClusterName namespace := env.Namespace environmentIdentifier := env.EnvironmentIdentifier @@ -524,7 +522,7 @@ func (impl EnforcerUtilImpl) GetHelmObjectByAppNameAndEnvId(appName string, envI } } if environmentIdentifier2 == "" { - return strings.ToLower(fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName)), "" + return strings.ToLower(fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac)), "" } //TODO - FIX required for futuristic permission for cluster__* all environment for migrated environment identifier only @@ -532,8 +530,50 @@ func (impl EnforcerUtilImpl) GetHelmObjectByAppNameAndEnvId(appName string, envI if !strings.HasPrefix(env.EnvironmentIdentifier, fmt.Sprintf("%s__", env.Cluster.ClusterName)) { environmentIdentifier = fmt.Sprintf("%s__%s", env.Cluster.ClusterName, env.EnvironmentIdentifier) }*/ - return strings.ToLower(fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier, application.AppName)), - strings.ToLower(fmt.Sprintf("%s/%s/%s", application.Team.Name, environmentIdentifier2, application.AppName)) + return strings.ToLower(fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier, appNameForRbac)), + strings.ToLower(fmt.Sprintf("%s/%s/%s", teamName, environmentIdentifier2, appNameForRbac)) +} + +// getTeamNameForHelmRbac returns the project name to be used while building a helm rbac object. +// An external helm app can be linked to chart store without a project being assigned to it, in which case +// team_id stays 0 and the joined Team row is empty. Permissions for such apps are granted under the +// "unassigned" project, so that literal has to be used instead of an empty project name. +func getTeamNameForHelmRbac(application *app.App) string { + if application.TeamId == 0 { + return repository3.UNASSIGNED_PROJECT + } + return application.Team.Name +} + +// getAppForHelmRbac resolves the app row for a helm rbac object, given a name that may either be the +// app_name stored in db or the display_name shown to the user. +// For an external helm app linked to chart store, app_name is the unique identifier +// (--) while callers here generally only have the release name, so the +// identifier is reconstructed from the environment and tried first. The plain-name lookup is kept as a +// fallback for regular chart store apps, for legacy rows created before the unique identifier existed, and +// for callers that already pass the identifier. +func (impl EnforcerUtilImpl) getAppForHelmRbac(appName string, clusterId int, namespace string) (*app.App, error) { + appIdentifier := &bean3.AppIdentifier{ + ClusterId: clusterId, + Namespace: namespace, + ReleaseName: appName, + } + uniqueAppNameIdentifier := appIdentifier.GetUniqueAppNameIdentifier() + application, err := impl.appRepo.FindAppAndProjectByAppName(uniqueAppNameIdentifier) + if err == pg.ErrMultiRows { + application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(uniqueAppNameIdentifier) + } + if application == nil || err == pg.ErrNoRows { + impl.logger.Debugw("app not found by unique identifier, falling back to app name", "appIdentifier", uniqueAppNameIdentifier, "appName", appName) + application, err = impl.appRepo.FindAppAndProjectByAppName(appName) + if err == pg.ErrMultiRows { + application, err = impl.dbMigration.FixMultipleAppsForInstalledApp(appName) + } + } + if err != nil { + return nil, err + } + return application, nil } func (impl EnforcerUtilImpl) GetHelmObjectByProjectIdAndEnvId(teamId int, envId int) (string, string) { diff --git a/util/rbac/EnforcerUtilHelm.go b/util/rbac/EnforcerUtilHelm.go index 268092a6e9..1631db5223 100644 --- a/util/rbac/EnforcerUtilHelm.go +++ b/util/rbac/EnforcerUtilHelm.go @@ -177,7 +177,9 @@ func (impl EnforcerUtilHelmImpl) GetAppRBACNameByInstalledAppId(installedAppVers impl.logger.Errorw("error in fetching installed app version data", "err", err) return fmt.Sprintf("%s/%s/%s", "", "", ""), fmt.Sprintf("%s/%s/%s", "", "", "") } - rbacOne := fmt.Sprintf("%s/%s/%s", InstalledApp.App.Team.Name, InstalledApp.Environment.EnvironmentIdentifier, InstalledApp.App.AppName) + appNameForRbac := InstalledApp.App.GetAppNameForRbac() + teamName := getTeamNameForHelmRbac(&InstalledApp.App) + rbacOne := fmt.Sprintf("%s/%s/%s", teamName, InstalledApp.Environment.EnvironmentIdentifier, appNameForRbac) if InstalledApp.Environment.IsVirtualEnvironment { return rbacOne, "" @@ -186,7 +188,7 @@ func (impl EnforcerUtilHelmImpl) GetAppRBACNameByInstalledAppId(installedAppVers var rbacTwo string if !InstalledApp.Environment.IsVirtualEnvironment { if InstalledApp.Environment.EnvironmentIdentifier != InstalledApp.Environment.Cluster.ClusterName+"__"+InstalledApp.Environment.Namespace { - rbacTwo = fmt.Sprintf("%s/%s/%s", InstalledApp.App.Team.Name, InstalledApp.Environment.Cluster.ClusterName+"__"+InstalledApp.Environment.Namespace, InstalledApp.App.AppName) + rbacTwo = fmt.Sprintf("%s/%s/%s", teamName, InstalledApp.Environment.Cluster.ClusterName+"__"+InstalledApp.Environment.Namespace, appNameForRbac) return rbacOne, rbacTwo } } @@ -205,6 +207,6 @@ func (impl EnforcerUtilHelmImpl) GetAppRBACNameByInstalledAppIdAndTeamId(install impl.logger.Errorw("error in fetching project by teamID", "err", err) return fmt.Sprintf("%s/%s/%s", "", "", "") } - rbac := fmt.Sprintf("%s/%s/%s", project.Name, installedApp.Environment.EnvironmentIdentifier, installedApp.App.AppName) + rbac := fmt.Sprintf("%s/%s/%s", project.Name, installedApp.Environment.EnvironmentIdentifier, installedApp.App.GetAppNameForRbac()) return rbac } diff --git a/util/rbac/EnforcerUtilHelmObject_test.go b/util/rbac/EnforcerUtilHelmObject_test.go new file mode 100644 index 0000000000..b0416f5042 --- /dev/null +++ b/util/rbac/EnforcerUtilHelmObject_test.go @@ -0,0 +1,212 @@ +/* + * Copyright (c) 2020-2024. Devtron Inc. + */ + +package rbac + +import ( + "testing" + + "github.com/devtron-labs/devtron/internal/sql/repository/app" + environmentRepository "github.com/devtron-labs/devtron/pkg/cluster/environment/repository" + clusterRepository "github.com/devtron-labs/devtron/pkg/cluster/repository" + teamRepo "github.com/devtron-labs/devtron/pkg/team/repository" + "github.com/go-pg/pg" + "go.uber.org/zap" +) + +// stubAppRepo overrides only the lookups used while building helm rbac objects; every other method of the +// interface is left nil and will panic if a code path unexpectedly reaches it. +type stubAppRepo struct { + app.AppRepository + byName map[string]*app.App + byId map[int]*app.App +} + +func (s *stubAppRepo) FindAppAndProjectByAppName(appName string) (*app.App, error) { + if a, ok := s.byName[appName]; ok { + return a, nil + } + // mirrors the real repository, which returns an empty (non nil) app alongside ErrNoRows + return &app.App{}, pg.ErrNoRows +} + +func (s *stubAppRepo) FindAppAndProjectByAppId(appId int) (*app.App, error) { + if a, ok := s.byId[appId]; ok { + return a, nil + } + return &app.App{}, pg.ErrNoRows +} + +type stubEnvRepo struct { + environmentRepository.EnvironmentRepository + env *environmentRepository.Environment +} + +func (s *stubEnvRepo) FindById(id int) (*environmentRepository.Environment, error) { + if s.env == nil || s.env.Id != id { + return nil, pg.ErrNoRows + } + return s.env, nil +} + +const ( + testClusterName = "default_cluster" + testNamespace = "devtron-demo" + testEnvId = 1 + testClusterId = 1 +) + +// linkedExternalApp is how an external helm app looks in the app table once it is linked to chart store: +// app_name carries the unique identifier while display_name carries the release name. +func linkedExternalApp(id int, releaseName string, teamId int, teamName string) *app.App { + return &app.App{ + Id: id, + AppName: releaseName + "-" + testNamespace + "-1", + DisplayName: releaseName, + TeamId: teamId, + Team: teamRepo.Team{Name: teamName}, + } +} + +func newTestEnforcerUtil(envIdentifier string) *EnforcerUtilImpl { + nginx := linkedExternalApp(1, "nginx", 2, "myproject") + // linked with no project assigned -> team_id stays 0 and the joined team row is empty + redis := linkedExternalApp(2, "redis", 0, "") + // a regular chart store app, which has no display name at all + grafana := &app.App{Id: 3, AppName: "grafana", TeamId: 2, Team: teamRepo.Team{Name: "myproject"}} + + appRepo := &stubAppRepo{ + byName: map[string]*app.App{ + nginx.AppName: nginx, + redis.AppName: redis, + grafana.AppName: grafana, + }, + byId: map[int]*app.App{1: nginx, 2: redis, 3: grafana}, + } + envRepo := &stubEnvRepo{env: &environmentRepository.Environment{ + Id: testEnvId, + Name: testNamespace, + ClusterId: testClusterId, + Namespace: testNamespace, + EnvironmentIdentifier: envIdentifier, + Cluster: &clusterRepository.Cluster{Id: testClusterId, ClusterName: testClusterName}, + }} + return &EnforcerUtilImpl{logger: zap.NewNop().Sugar(), appRepo: appRepo, environmentRepository: envRepo} +} + +func TestGetHelmObjectByAppNameAndEnvId(t *testing.T) { + envIdentifier := testClusterName + "__" + testNamespace + tests := []struct { + name string + appName string + wantObject string + wantObject2 string + }{ + { + name: "external app linked to chart store is resolved by its display name", + // this is the regression: the listing APIs hand over the release name, but app_name in db holds + // the unique identifier, so the lookup used to miss and the object came back as "//" + appName: "nginx", + wantObject: "myproject/" + envIdentifier + "/nginx", + }, + { + name: "external app linked without a project falls under the unassigned project", + appName: "redis", + wantObject: teamRepo.UNASSIGNED_PROJECT + "/" + envIdentifier + "/redis", + }, + { + name: "regular chart store app keeps working via the plain app name", + appName: "grafana", + wantObject: "myproject/" + envIdentifier + "/grafana", + }, + { + name: "caller passing the unique identifier still gets the display name based object", + // some handlers hand over installedApp.App.AppName directly + appName: "nginx-" + testNamespace + "-1", + wantObject: "myproject/" + envIdentifier + "/nginx", + }, + { + name: "unknown app yields an empty object", + appName: "does-not-exist", + wantObject: "//", + }, + } + impl := newTestEnforcerUtil(envIdentifier) + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + object, object2 := impl.GetHelmObjectByAppNameAndEnvId(tt.appName, testEnvId) + if object != tt.wantObject { + t.Errorf("object = %q, want %q", object, tt.wantObject) + } + if object2 != tt.wantObject2 { + t.Errorf("object2 = %q, want %q", object2, tt.wantObject2) + } + }) + } +} + +// When the environment identifier is not clusterName__namespace, both the migrated and the futuristic +// permission objects are returned, and each has to carry the display name. +func TestGetHelmObjectByAppNameAndEnvId_FuturisticPermissionObject(t *testing.T) { + impl := newTestEnforcerUtil("devtron-demo") + object, object2 := impl.GetHelmObjectByAppNameAndEnvId("nginx", testEnvId) + if want := "myproject/devtron-demo/nginx"; object != want { + t.Errorf("object = %q, want %q", object, want) + } + if want := "myproject/" + testClusterName + "__" + testNamespace + "/nginx"; object2 != want { + t.Errorf("object2 = %q, want %q", object2, want) + } +} + +// GetHelmObjectByAppNameAndEnvId lowercases its objects, so a mixed case release name still has to line up +// with the policy that was stored for it. +func TestGetHelmObjectByAppNameAndEnvId_LowerCased(t *testing.T) { + envIdentifier := testClusterName + "__" + testNamespace + mixed := &app.App{ + Id: 4, + AppName: "MyRelease-" + testNamespace + "-1", + DisplayName: "MyRelease", + TeamId: 2, + Team: teamRepo.Team{Name: "MyProject"}, + } + impl := newTestEnforcerUtil(envIdentifier) + impl.appRepo.(*stubAppRepo).byName[mixed.AppName] = mixed + + object, _ := impl.GetHelmObjectByAppNameAndEnvId("MyRelease", testEnvId) + if want := "myproject/" + envIdentifier + "/myrelease"; object != want { + t.Errorf("object = %q, want %q", object, want) + } +} + +func TestGetHelmObject(t *testing.T) { + envIdentifier := testClusterName + "__" + testNamespace + impl := newTestEnforcerUtil(envIdentifier) + + // app id lookup always succeeded, but the object used to be built from app_name (the unique identifier) + object, _ := impl.GetHelmObject(1, testEnvId) + if want := "myproject/" + envIdentifier + "/nginx"; object != want { + t.Errorf("object = %q, want %q", object, want) + } + + object, _ = impl.GetHelmObject(2, testEnvId) + if want := teamRepo.UNASSIGNED_PROJECT + "/" + envIdentifier + "/redis"; object != want { + t.Errorf("object = %q, want %q", object, want) + } + + object, _ = impl.GetHelmObject(3, testEnvId) + if want := "myproject/" + envIdentifier + "/grafana"; object != want { + t.Errorf("object = %q, want %q", object, want) + } +} + +func TestGetAppNameForRbac(t *testing.T) { + external := &app.App{AppName: "nginx-devtron-demo-1", DisplayName: "nginx"} + if got := external.GetAppNameForRbac(); got != "nginx" { + t.Errorf("GetAppNameForRbac() = %q, want %q", got, "nginx") + } + regular := &app.App{AppName: "grafana"} + if got := regular.GetAppNameForRbac(); got != "grafana" { + t.Errorf("GetAppNameForRbac() = %q, want %q", got, "grafana") + } +}