public/grafana
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Fix: Move the hidden users exclusion to the DB layer (#115254)

Browse Source 
* Move the hidden users exclusion to the store layer

* Address Copilot's feedback

* Improve test case name
This commit is contained in:
Misi
2025-12-16 09:37:59 +01:00
committed by GitHub
parent 2d6c1c4e9e
commit 6350b26326
6 changed files with 162 additions and 10 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/api/org_users.go
+1 -3
View File
@@ -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
pkg/api/org_users_test.go
+18 -1
View File
@@ -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()
pkg/services/org/model.go
+2
View File
@@ -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
}
pkg/services/org/orgimpl/org.go
+1
View File
@@ -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,
pkg/services/org/orgimpl/store.go
+31
View File
@@ -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
}
pkg/services/org/orgimpl/store_test.go
+109 -6
View File
@@ -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) {