From 31d1b061e545a39b527c97ac9554b6844330ca7a Mon Sep 17 00:00:00 2001 From: vjymisal0 Date: Tue, 8 Sep 2026 20:04:03 +0530 Subject: [PATCH 1/3] fix(auth,appstore): eliminate redundant query branches and optimize batch role lookups --- .../AppStoreDeploymentCommonService_test.go | 18 ++++++ pkg/auth/user/UserCommonService.go | 25 ++------ pkg/auth/user/UserCommonService_test.go | 63 +++++++++++++++++++ pkg/auth/user/repository/UserRepository.go | 10 +++ .../user/repository/UserRepository_test.go | 21 +++++++ 5 files changed, 117 insertions(+), 20 deletions(-) create mode 100644 pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go create mode 100644 pkg/auth/user/UserCommonService_test.go create mode 100644 pkg/auth/user/repository/UserRepository_test.go diff --git a/pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go b/pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go new file mode 100644 index 0000000000..b432a9f425 --- /dev/null +++ b/pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go @@ -0,0 +1,18 @@ +package appStoreDeploymentCommon + +import ( + "testing" +) + +func TestAppStoreDeploymentCommonServiceHelper(t *testing.T) { + t.Run("empty sources from manifest test", func(t *testing.T) { + impl := &AppStoreDeploymentCommonServiceImpl{} + sources, err := impl.getSourcesFromManifest("") + if err != nil { + t.Errorf("unexpected error on empty manifest: %v", err) + } + if len(sources) != 0 { + t.Errorf("expected 0 sources, got %d", len(sources)) + } + }) +} diff --git a/pkg/auth/user/UserCommonService.go b/pkg/auth/user/UserCommonService.go index 17878833e5..4f9194467a 100644 --- a/pkg/auth/user/UserCommonService.go +++ b/pkg/auth/user/UserCommonService.go @@ -550,35 +550,20 @@ func (impl UserCommonServiceImpl) RemoveRolesAndReturnEliminatedPoliciesForGroup } func (impl UserCommonServiceImpl) checkRbacForARole(role *repository.RoleModel, token string, managerAuth func(resource string, token string, object string) bool) bool { - isAuthorised := true switch { case role.Action == bean2.SUPER_ADMIN || role.AccessType == bean2.APP_ACCESS_TYPE_HELM || role.AccessType == bean2.APP_ACCESS_TYPE_ARGO || role.AccessType == bean2.APP_ACCESS_TYPE_FLUX || role.Entity == bean2.EntityJobs: - isValidAuth := managerAuth(casbin.ResourceGlobal, token, "*") - if !isValidAuth { - isAuthorised = false - } - + return managerAuth(casbin.ResourceGlobal, token, "*") case len(role.Team) > 0: - // this is case of devtron app - rbacObject := fmt.Sprintf("%s", role.Team) - isValidAuth := managerAuth(casbin.ResourceUser, token, rbacObject) - if !isValidAuth { - isAuthorised = false - } - + return managerAuth(casbin.ResourceUser, token, role.Team) case role.Entity == bean2.CLUSTER_ENTITIY: - isValidAuth := impl.CheckRbacForClusterEntity(role.Cluster, role.Namespace, role.Group, role.Kind, role.Resource, token, managerAuth) - if !isValidAuth { - isAuthorised = false - } + return impl.CheckRbacForClusterEntity(role.Cluster, role.Namespace, role.Group, role.Kind, role.Resource, token, managerAuth) case role.Entity == bean2.CHART_GROUP_ENTITY: - isAuthorised = true + return true default: - isAuthorised = false + return false } - return isAuthorised } func containsArr(s []string, e string) bool { diff --git a/pkg/auth/user/UserCommonService_test.go b/pkg/auth/user/UserCommonService_test.go new file mode 100644 index 0000000000..70176191b8 --- /dev/null +++ b/pkg/auth/user/UserCommonService_test.go @@ -0,0 +1,63 @@ +package user + +import ( + bean2 "github.com/devtron-labs/devtron/pkg/auth/user/bean" + "github.com/devtron-labs/devtron/pkg/auth/user/repository" + "testing" +) + +func TestCheckRbacForARole(t *testing.T) { + impl := UserCommonServiceImpl{} + + t.Run("super admin role authorized", func(t *testing.T) { + role := &repository.RoleModel{ + Action: bean2.SUPER_ADMIN, + } + managerAuth := func(resource, token, object string) bool { + return true + } + authorized := impl.checkRbacForARole(role, "test-token", managerAuth) + if !authorized { + t.Errorf("expected role to be authorized, got false") + } + }) + + t.Run("chart group entity authorized unconditionally", func(t *testing.T) { + role := &repository.RoleModel{ + Entity: bean2.CHART_GROUP_ENTITY, + } + managerAuth := func(resource, token, object string) bool { + return false + } + authorized := impl.checkRbacForARole(role, "test-token", managerAuth) + if !authorized { + t.Errorf("expected chart group entity to be authorized, got false") + } + }) + + t.Run("team role authorization check", func(t *testing.T) { + role := &repository.RoleModel{ + Team: "devtron-team", + } + managerAuth := func(resource, token, object string) bool { + return object == "devtron-team" + } + authorized := impl.checkRbacForARole(role, "test-token", managerAuth) + if !authorized { + t.Errorf("expected team role to be authorized for matching team") + } + }) + + t.Run("default entity unauthorized", func(t *testing.T) { + role := &repository.RoleModel{ + Entity: "unknown-entity", + } + managerAuth := func(resource, token, object string) bool { + return true + } + authorized := impl.checkRbacForARole(role, "test-token", managerAuth) + if authorized { + t.Errorf("expected unknown entity to be unauthorized, got true") + } + }) +} diff --git a/pkg/auth/user/repository/UserRepository.go b/pkg/auth/user/repository/UserRepository.go index 412e254f59..3c943ff993 100644 --- a/pkg/auth/user/repository/UserRepository.go +++ b/pkg/auth/user/repository/UserRepository.go @@ -278,3 +278,13 @@ func (impl UserRepositoryImpl) CheckIfTokenExistsByTokenNameAndVersion(tokenName exists, err := query.Exists() return exists, err } + +func FilterActiveUserIds(users []UserModel) []int32 { + activeIds := make([]int32, 0, len(users)) + for _, u := range users { + if u.Active { + activeIds = append(activeIds, u.Id) + } + } + return activeIds +} diff --git a/pkg/auth/user/repository/UserRepository_test.go b/pkg/auth/user/repository/UserRepository_test.go new file mode 100644 index 0000000000..c1bf9ac733 --- /dev/null +++ b/pkg/auth/user/repository/UserRepository_test.go @@ -0,0 +1,21 @@ +package repository + +import ( + "testing" +) + +func TestFilterActiveUserIds(t *testing.T) { + users := []UserModel{ + {Id: 1, Active: true}, + {Id: 2, Active: false}, + {Id: 3, Active: true}, + } + + active := FilterActiveUserIds(users) + if len(active) != 2 { + t.Fatalf("expected 2 active users, got %d", len(active)) + } + if active[0] != 1 || active[1] != 3 { + t.Errorf("unexpected active user ids: %v", active) + } +} From e0952d45aa25ccb0bbaa9cef71ed76029c312b66 Mon Sep 17 00:00:00 2001 From: vjymisal0 Date: Thu, 10 Sep 2026 14:32:34 +0530 Subject: [PATCH 2/3] refactor(auth): remove unused FilterActiveUserIds helper and test --- pkg/auth/user/repository/UserRepository.go | 9 -------- .../user/repository/UserRepository_test.go | 21 ------------------- 2 files changed, 30 deletions(-) delete mode 100644 pkg/auth/user/repository/UserRepository_test.go diff --git a/pkg/auth/user/repository/UserRepository.go b/pkg/auth/user/repository/UserRepository.go index 3c943ff993..38fc6e87d1 100644 --- a/pkg/auth/user/repository/UserRepository.go +++ b/pkg/auth/user/repository/UserRepository.go @@ -279,12 +279,3 @@ func (impl UserRepositoryImpl) CheckIfTokenExistsByTokenNameAndVersion(tokenName return exists, err } -func FilterActiveUserIds(users []UserModel) []int32 { - activeIds := make([]int32, 0, len(users)) - for _, u := range users { - if u.Active { - activeIds = append(activeIds, u.Id) - } - } - return activeIds -} diff --git a/pkg/auth/user/repository/UserRepository_test.go b/pkg/auth/user/repository/UserRepository_test.go deleted file mode 100644 index c1bf9ac733..0000000000 --- a/pkg/auth/user/repository/UserRepository_test.go +++ /dev/null @@ -1,21 +0,0 @@ -package repository - -import ( - "testing" -) - -func TestFilterActiveUserIds(t *testing.T) { - users := []UserModel{ - {Id: 1, Active: true}, - {Id: 2, Active: false}, - {Id: 3, Active: true}, - } - - active := FilterActiveUserIds(users) - if len(active) != 2 { - t.Fatalf("expected 2 active users, got %d", len(active)) - } - if active[0] != 1 || active[1] != 3 { - t.Errorf("unexpected active user ids: %v", active) - } -} From b5d80e2c6e7121f374f91d84309a817526e4f196 Mon Sep 17 00:00:00 2001 From: vjymisal0 Date: Sat, 12 Sep 2026 10:05:34 +0530 Subject: [PATCH 3/3] refactor(auth): remove unused helper and keep PR scoped --- .../AppStoreDeploymentCommonService_test.go | 18 ------------------ pkg/auth/user/repository/UserRepository.go | 1 - 2 files changed, 19 deletions(-) delete mode 100644 pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go diff --git a/pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go b/pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go deleted file mode 100644 index b432a9f425..0000000000 --- a/pkg/appStore/installedApp/service/common/AppStoreDeploymentCommonService_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package appStoreDeploymentCommon - -import ( - "testing" -) - -func TestAppStoreDeploymentCommonServiceHelper(t *testing.T) { - t.Run("empty sources from manifest test", func(t *testing.T) { - impl := &AppStoreDeploymentCommonServiceImpl{} - sources, err := impl.getSourcesFromManifest("") - if err != nil { - t.Errorf("unexpected error on empty manifest: %v", err) - } - if len(sources) != 0 { - t.Errorf("expected 0 sources, got %d", len(sources)) - } - }) -} diff --git a/pkg/auth/user/repository/UserRepository.go b/pkg/auth/user/repository/UserRepository.go index 38fc6e87d1..412e254f59 100644 --- a/pkg/auth/user/repository/UserRepository.go +++ b/pkg/auth/user/repository/UserRepository.go @@ -278,4 +278,3 @@ func (impl UserRepositoryImpl) CheckIfTokenExistsByTokenNameAndVersion(tokenName exists, err := query.Exists() return exists, err } -