feat: resolve RBAC inconsistencies for helm apps with unique identifiers

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
+// (<releaseName>-<namespace>-<clusterId>) 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

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,16 +522,58 @@ 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
 	/*//here cluster, env, namespace must not have double underscore in names, as we are using that for separator.
 	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
+// (<releaseName>-<namespace>-<clusterId>) 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) {

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
 }

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")
+	}
+}