From 6350b26326e6b6bbc7b9d8b538f6a06572239363 Mon Sep 17 00:00:00 2001 From: Misi Date: Tue, 16 Dec 2025 09:37:59 +0100 Subject: [PATCH] Fix: Move the hidden users exclusion to the DB layer (#115254) * Move the hidden users exclusion to the store layer * Address Copilot's feedback * Improve test case name --- pkg/api/org_users.go | 4 +- pkg/api/org_users_test.go | 19 +++- pkg/services/org/model.go | 2 + pkg/services/org/orgimpl/org.go | 1 + pkg/services/org/orgimpl/store.go | 31 +++++++ pkg/services/org/orgimpl/store_test.go | 115 +++++++++++++++++++++++-- 6 files changed, 162 insertions(+), 10 deletions(-) diff --git a/pkg/api/org_users.go b/pkg/api/org_users.go index 8a10cc24944..37b459f0e69 100644 --- a/pkg/api/org_users.go +++ b/pkg/api/org_users.go @@ -294,6 +294,7 @@ func (hs *HTTPServer) SearchOrgUsersWithPaging(c *contextmodel.ReqContext) respo } func (hs *HTTPServer) searchOrgUsersHelper(c *contextmodel.ReqContext, query *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { + query.ExcludeHiddenUsers = true result, err := hs.orgService.SearchOrgUsers(c.Req.Context(), query) if err != nil { return nil, err @@ -303,9 +304,6 @@ func (hs *HTTPServer) searchOrgUsersHelper(c *contextmodel.ReqContext, query *or userIDs := map[string]bool{} authLabelsUserIDs := make([]int64, 0, len(result.OrgUsers)) for _, user := range result.OrgUsers { - if dtos.IsHiddenUser(user.Login, c.SignedInUser, hs.Cfg) { - continue - } user.AvatarURL = dtos.GetGravatarUrl(hs.Cfg, user.Email) userIDs[fmt.Sprint(user.UserID)] = true diff --git a/pkg/api/org_users_test.go b/pkg/api/org_users_test.go index a43b5c7edcf..c8313ecefce 100644 --- a/pkg/api/org_users_test.go +++ b/pkg/api/org_users_test.go @@ -171,11 +171,16 @@ func TestIntegrationOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { orgService.ExpectedSearchOrgUsersResult = &org.SearchOrgUsersQueryResult{ OrgUsers: []*org.OrgUserDTO{ {Login: testUserLogin, Email: "testUser@grafana.com"}, - {Login: "user1", Email: "user1@grafana.com"}, {Login: "user2", Email: "user2@grafana.com"}, }, } + orgService.SearchOrgUsersFn = func(ctx context.Context, query *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { + require.True(t, query.ExcludeHiddenUsers) + return orgService.ExpectedSearchOrgUsersResult, nil + } + defer func() { orgService.SearchOrgUsersFn = nil }() + sc.handlerFunc = hs.GetOrgUsersForCurrentOrg sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() @@ -191,6 +196,18 @@ func TestIntegrationOrgUsersAPIEndpoint_userLoggedIn(t *testing.T) { loggedInUserScenarioWithRole(t, "When calling GET as an admin on", "GET", "api/org/users/lookup", "api/org/users/lookup", org.RoleAdmin, func(sc *scenarioContext) { + orgService.ExpectedSearchOrgUsersResult = &org.SearchOrgUsersQueryResult{ + OrgUsers: []*org.OrgUserDTO{ + {Login: testUserLogin, Email: "testUser@grafana.com"}, + {Login: "user2", Email: "user2@grafana.com"}, + }, + } + orgService.SearchOrgUsersFn = func(ctx context.Context, query *org.SearchOrgUsersQuery) (*org.SearchOrgUsersQueryResult, error) { + require.True(t, query.ExcludeHiddenUsers) + return orgService.ExpectedSearchOrgUsersResult, nil + } + defer func() { orgService.SearchOrgUsersFn = nil }() + sc.handlerFunc = hs.GetOrgUsersForCurrentOrgLookup sc.fakeReqWithParams("GET", sc.url, map[string]string{}).exec() diff --git a/pkg/services/org/model.go b/pkg/services/org/model.go index 7873e899eb3..ac0268e051c 100644 --- a/pkg/services/org/model.go +++ b/pkg/services/org/model.go @@ -188,6 +188,8 @@ type SearchOrgUsersQuery struct { SortOpts []model.SortOption // Flag used to allow oss edition to query users without access control DontEnforceAccessControl bool + // Flag used to exclude hidden users from the result + ExcludeHiddenUsers bool User identity.Requester } diff --git a/pkg/services/org/orgimpl/org.go b/pkg/services/org/orgimpl/org.go index 423a4bc8b8d..6df28368f4c 100644 --- a/pkg/services/org/orgimpl/org.go +++ b/pkg/services/org/orgimpl/org.go @@ -27,6 +27,7 @@ func ProvideService(db db.DB, cfg *setting.Cfg, quotaService quota.Service) (org db: db, dialect: db.GetDialect(), log: log, + cfg: cfg, }, cfg: cfg, log: log, diff --git a/pkg/services/org/orgimpl/store.go b/pkg/services/org/orgimpl/store.go index 50bbd68ec82..7e03db60e34 100644 --- a/pkg/services/org/orgimpl/store.go +++ b/pkg/services/org/orgimpl/store.go @@ -8,6 +8,7 @@ import ( "strings" "time" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" @@ -16,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/services/sqlstore" "github.com/grafana/grafana/pkg/services/sqlstore/migrator" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" ) @@ -53,6 +55,7 @@ type sqlStore struct { //TODO: moved to service log log.Logger deletes []string + cfg *setting.Cfg } func (ss *sqlStore) Get(ctx context.Context, orgID int64) (*org.Org, error) { @@ -560,6 +563,14 @@ func (ss *sqlStore) SearchOrgUsers(ctx context.Context, query *org.SearchOrgUser whereParams = append(whereParams, acFilter.Args...) } + if query.ExcludeHiddenUsers { + cond, params := buildHiddenUsersFilter(query.User, ss.cfg.HiddenUsers) + if cond != "" { + whereConditions = append(whereConditions, cond) + whereParams = append(whereParams, params...) + } + } + if query.Query != "" { sql1, param1 := ss.dialect.LikeOperator("email", true, query.Query, true) sql2, param2 := ss.dialect.LikeOperator("name", true, query.Query, true) @@ -825,3 +836,23 @@ func removeUserOrg(sess *db.Session, userID int64) error { func (ss *sqlStore) RegisterDelete(query string) { ss.deletes = append(ss.deletes, query) } + +func buildHiddenUsersFilter(requester identity.Requester, hiddenUsersMap map[string]struct{}) (string, []any) { + if requester != nil && requester.GetIsGrafanaAdmin() { + return "", nil + } + + hiddenUsers := make([]any, 0) + for user := range hiddenUsersMap { + if requester != nil && user == requester.GetLogin() { + continue + } + hiddenUsers = append(hiddenUsers, user) + } + + if len(hiddenUsers) > 0 { + return "u.login NOT IN (?" + strings.Repeat(",?", len(hiddenUsers)-1) + ")", hiddenUsers + } + + return "", nil +} diff --git a/pkg/services/org/orgimpl/store_test.go b/pkg/services/org/orgimpl/store_test.go index 5cd7c356a5c..54f8e9fda39 100644 --- a/pkg/services/org/orgimpl/store_test.go +++ b/pkg/services/org/orgimpl/store_test.go @@ -820,8 +820,9 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { db: store, dialect: store.GetDialect(), log: log.NewNopLogger(), + cfg: cfg, } - // orgUserStore.cfg.Skip + orgSvc, userSvc := createOrgAndUserSvc(t, store, cfg) o, err := orgSvc.CreateWithMember(context.Background(), &org.CreateOrgCommand{Name: "test org"}) @@ -829,6 +830,14 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { seedOrgUsers(t, &orgUserStore, 10, userSvc, o.ID) + user1, err := userSvc.GetByLogin(context.Background(), &user.GetUserByLoginQuery{LoginOrEmail: "user-1"}) + require.NoError(t, err) + + cfg.HiddenUsers = map[string]struct{}{ + "user-1": {}, + "user-2": {}, + } + tests := []struct { desc string query *org.SearchOrgUsersQuery @@ -840,7 +849,7 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { OrgID: o.ID, User: &user.SignedInUser{ OrgID: o.ID, - Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, }, }, expectedNumUsers: 10, @@ -851,7 +860,7 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { OrgID: o.ID, User: &user.SignedInUser{ OrgID: o.ID, - Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: {""}}}, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {""}}}, }, }, expectedNumUsers: 0, @@ -862,8 +871,8 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { OrgID: o.ID, User: &user.SignedInUser{ OrgID: o.ID, - Permissions: map[int64]map[string][]string{1: {accesscontrol.ActionOrgUsersRead: { - "users:id:1", + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: { + "users:id:2", "users:id:5", "users:id:9", }}}, @@ -871,6 +880,55 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { }, expectedNumUsers: 3, }, + { + desc: "should exclude hidden users when ExcludeHiddenUsers is true and user is nil", + query: &org.SearchOrgUsersQuery{ + OrgID: o.ID, + ExcludeHiddenUsers: true, + User: nil, + DontEnforceAccessControl: true, + }, + expectedNumUsers: 8, + }, + { + desc: "should not exclude hidden users when ExcludeHiddenUsers is true and user is Grafana Admin", + query: &org.SearchOrgUsersQuery{ + OrgID: o.ID, + ExcludeHiddenUsers: true, + User: &user.SignedInUser{ + OrgID: o.ID, + IsGrafanaAdmin: true, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + }, + expectedNumUsers: 10, + }, + { + desc: "should return all users if ExcludeHiddenUsers is false", + query: &org.SearchOrgUsersQuery{ + OrgID: o.ID, + ExcludeHiddenUsers: false, + User: &user.SignedInUser{ + OrgID: o.ID, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + }, + expectedNumUsers: 10, + }, + { + desc: "should include the hidden user when the request is made by the hidden user and ExcludeHiddenUsers is true", + query: &org.SearchOrgUsersQuery{ + OrgID: o.ID, + ExcludeHiddenUsers: true, + User: &user.SignedInUser{ + UserID: user1.ID, + Login: user1.Login, + OrgID: o.ID, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + }, + expectedNumUsers: 9, + }, } for _, tt := range tests { @@ -879,13 +937,58 @@ func TestIntegration_SQLStore_SearchOrgUsers(t *testing.T) { require.NoError(t, err) assert.Len(t, result.OrgUsers, tt.expectedNumUsers) - if !hasWildcardScope(tt.query.User, accesscontrol.ActionOrgUsersRead) { + // No pagination is applied, so TotalCount should equal to number of returned users + assert.Equal(t, int64(tt.expectedNumUsers), result.TotalCount) + + if tt.query.User != nil && !hasWildcardScope(tt.query.User, accesscontrol.ActionOrgUsersRead) && !tt.query.User.GetIsGrafanaAdmin() { for _, u := range result.OrgUsers { assert.Contains(t, tt.query.User.GetPermissions()[accesscontrol.ActionOrgUsersRead], fmt.Sprintf("users:id:%d", u.UserID)) } } }) } + + t.Run("should paginate correctly when ExcludeHiddenUsers is true", func(t *testing.T) { + query := &org.SearchOrgUsersQuery{ + OrgID: o.ID, + ExcludeHiddenUsers: true, + User: &user.SignedInUser{ + OrgID: o.ID, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + Limit: 5, + Page: 1, + } + result, err := orgUserStore.SearchOrgUsers(context.Background(), query) + require.NoError(t, err) + assert.Len(t, result.OrgUsers, 5) + assert.Equal(t, int64(8), result.TotalCount) + + query.Page = 2 + result, err = orgUserStore.SearchOrgUsers(context.Background(), query) + require.NoError(t, err) + assert.Len(t, result.OrgUsers, 3) + assert.Equal(t, int64(8), result.TotalCount) + }) + + t.Run("should return all users if HiddenUsers is empty", func(t *testing.T) { + oldHiddenUsers := cfg.HiddenUsers + cfg.HiddenUsers = make(map[string]struct{}) + defer func() { cfg.HiddenUsers = oldHiddenUsers }() + + query := &org.SearchOrgUsersQuery{ + OrgID: o.ID, + ExcludeHiddenUsers: true, + User: &user.SignedInUser{ + OrgID: o.ID, + Permissions: map[int64]map[string][]string{o.ID: {accesscontrol.ActionOrgUsersRead: {accesscontrol.ScopeUsersAll}}}, + }, + } + result, err := orgUserStore.SearchOrgUsers(context.Background(), query) + require.NoError(t, err) + assert.Len(t, result.OrgUsers, 10) + assert.Equal(t, int64(10), result.TotalCount) + }) } func TestIntegration_SQLStore_RemoveOrgUser(t *testing.T) {