diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index 9241354912c..d8bc38f838f 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -3,10 +3,8 @@ package accesscontrol import ( "context" "fmt" - "strconv" "strings" - "github.com/grafana/authlib/claims" "go.opentelemetry.io/otel" "go.opentelemetry.io/otel/attribute" "go.opentelemetry.io/otel/trace" @@ -90,7 +88,7 @@ type SearchOptions struct { Action string ActionSets []string Scope string - TypedID string // ID of the identity (ex: user:3, service-account:4) + UserID int64 wildcards Wildcards // private field computed based on the Scope RolePrefixes []string } @@ -110,19 +108,6 @@ func (s *SearchOptions) Wildcards() []string { return s.wildcards } -func (s *SearchOptions) ComputeUserID() (int64, error) { - typ, id, err := claims.ParseTypeID(s.TypedID) - if err != nil { - return 0, err - } - - if !claims.IsIdentityType(typ, claims.TypeUser, claims.TypeServiceAccount) { - return 0, fmt.Errorf("invalid type: %s", typ) - } - - return strconv.ParseInt(id, 10, 64) -} - type SyncUserRolesCommand struct { UserID int64 // name of roles the user should have diff --git a/pkg/services/accesscontrol/acimpl/service.go b/pkg/services/accesscontrol/acimpl/service.go index d77ff7092df..d4fe1735f05 100644 --- a/pkg/services/accesscontrol/acimpl/service.go +++ b/pkg/services/accesscontrol/acimpl/service.go @@ -53,7 +53,7 @@ var OSSRolesPrefixes = []string{accesscontrol.ManagedRolePrefix, accesscontrol.E func ProvideService( cfg *setting.Cfg, db db.DB, routeRegister routing.RouteRegister, cache *localcache.CacheService, - accessControl accesscontrol.AccessControl, actionResolver accesscontrol.ActionResolver, + accessControl accesscontrol.AccessControl, userService user.Service, actionResolver accesscontrol.ActionResolver, features featuremgmt.FeatureToggles, tracer tracing.Tracer, zclient zanzana.Client, permRegistry permreg.PermissionRegistry, lock *serverlock.ServerLockService, ) (*Service, error) { @@ -70,7 +70,7 @@ func ProvideService( lock, ) - api.NewAccessControlAPI(routeRegister, accessControl, service, features).RegisterAPIEndpoints() + api.NewAccessControlAPI(routeRegister, accessControl, service, userService, features).RegisterAPIEndpoints() if err := accesscontrol.DeclareFixedRoles(service, cfg); err != nil { return nil, err } @@ -526,20 +526,14 @@ func (s *Service) SearchUsersPermissions(ctx context.Context, usr identity.Reque // Limit roles to available in OSS options.RolePrefixes = OSSRolesPrefixes - if options.TypedID != "" { - userID, err := options.ComputeUserID() - if err != nil { - s.log.Error("Failed to resolve user ID", "error", err) - return nil, err - } - + if options.UserID > 0 { // Reroute to the user specific implementation of search permissions // because it leverages the user permission cache. userPerms, err := s.SearchUserPermissions(ctx, usr.GetOrgID(), options) if err != nil { return nil, err } - return map[int64][]accesscontrol.Permission{userID: userPerms}, nil + return map[int64][]accesscontrol.Permission{options.UserID: userPerms}, nil } timer := prometheus.NewTimer(metrics.MAccessSearchPermissionsSummary) @@ -641,7 +635,7 @@ func (s *Service) SearchUserPermissions(ctx context.Context, orgID int64, search timer := prometheus.NewTimer(metrics.MAccessPermissionsSummary) defer timer.ObserveDuration() - if searchOptions.TypedID == "" { + if searchOptions.UserID <= 0 { return nil, fmt.Errorf("expected namespaced ID to be specified") } @@ -655,20 +649,15 @@ func (s *Service) searchUserPermissions(ctx context.Context, orgID int64, search ctx, span := tracer.Start(ctx, "accesscontrol.acimpl.searchUserPermissions") defer span.End() - userID, err := searchOptions.ComputeUserID() - if err != nil { - return nil, err - } - // Get permissions for user's basic roles from RAM - roleList, err := s.store.GetUsersBasicRoles(ctx, []int64{userID}, orgID) + roleList, err := s.store.GetUsersBasicRoles(ctx, []int64{searchOptions.UserID}, orgID) if err != nil { return nil, fmt.Errorf("could not fetch basic roles for the user: %w", err) } var roles []string var ok bool - if roles, ok = roleList[userID]; !ok { - return nil, fmt.Errorf("found no basic roles for user %d in organisation %d", userID, orgID) + if roles, ok = roleList[searchOptions.UserID]; !ok { + return nil, fmt.Errorf("found no basic roles for user %d in organisation %d", searchOptions.UserID, orgID) } permissions := make([]accesscontrol.Permission, 0) for _, builtin := range roles { @@ -692,13 +681,13 @@ func (s *Service) searchUserPermissions(ctx context.Context, orgID int64, search if err != nil { return nil, err } - permissions = append(permissions, dbPermissions[userID]...) + permissions = append(permissions, dbPermissions[searchOptions.UserID]...) if s.features.IsEnabled(ctx, featuremgmt.FlagAccessActionSets) && len(searchOptions.ActionSets) != 0 { permissions = s.actionResolver.ExpandActionSetsWithFilter(permissions, GetActionFilter(searchOptions)) } - key, err := accesscontrol.GetSearchPermissionCacheKey(s.log, &user.SignedInUser{UserID: userID, OrgID: orgID}, searchOptions) + key, err := accesscontrol.GetSearchPermissionCacheKey(s.log, &user.SignedInUser{UserID: searchOptions.UserID, OrgID: orgID}, searchOptions) if err != nil { s.log.Warn("failed to create search permission cache key", "err", err) } else { @@ -712,14 +701,9 @@ func (s *Service) searchUserPermissionsFromCache(ctx context.Context, orgID int6 _, span := tracer.Start(ctx, "accesscontrol.acimpl.searchUserPermissionsFromCache") defer span.End() - userID, err := searchOptions.ComputeUserID() - if err != nil { - return nil, false - } - // Create a temp signed in user object to retrieve cache key tempUser := &user.SignedInUser{ - UserID: userID, + UserID: searchOptions.UserID, OrgID: orgID, } diff --git a/pkg/services/accesscontrol/acimpl/service_bench_test.go b/pkg/services/accesscontrol/acimpl/service_bench_test.go index e15bd4e76ec..8749f418837 100644 --- a/pkg/services/accesscontrol/acimpl/service_bench_test.go +++ b/pkg/services/accesscontrol/acimpl/service_bench_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/require" - "github.com/grafana/authlib/claims" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/log" @@ -262,7 +261,7 @@ func benchSearchUserWithAction(b *testing.B, usersCount, resourceCount int) { for n := 0; n < b.N; n++ { usersPermissions, err := acService.SearchUsersPermissions(context.Background(), siu, - accesscontrol.SearchOptions{Action: "resources:action2", TypedID: claims.NewTypeID(claims.TypeUser, "14")}) + accesscontrol.SearchOptions{Action: "resources:action2", UserID: 14}) require.NoError(b, err) require.Len(b, usersPermissions, 1) for _, permissions := range usersPermissions { diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index 88547917009..2dc2303443c 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -8,7 +8,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/grafana/authlib/claims" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/localcache" @@ -547,7 +546,7 @@ func TestService_SearchUsersPermissions(t *testing.T) { // only the user's basic roles and the user's stored permissions name: "check namespacedId filter works correctly", siuPermissions: listAllPerms, - searchOption: accesscontrol.SearchOptions{TypedID: claims.NewTypeID(claims.TypeServiceAccount, "1")}, + searchOption: accesscontrol.SearchOptions{UserID: 1}, ramRoles: map[string]*accesscontrol.RoleDTO{ string(identity.RoleEditor): {Permissions: []accesscontrol.Permission{ {Action: accesscontrol.ActionTeamsRead, Scope: "teams:*"}, @@ -619,7 +618,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "ram only", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - TypedID: claims.NewTypeID(claims.TypeUser, "2"), + UserID: 2, }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(identity.RoleEditor): {Permissions: []accesscontrol.Permission{ @@ -644,7 +643,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "stored only", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - TypedID: claims.NewTypeID(claims.TypeUser, "2"), + UserID: 2, }, storedPerms: map[int64][]accesscontrol.Permission{ 1: {{Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:1"}}, @@ -664,7 +663,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "ram and stored", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - TypedID: claims.NewTypeID(claims.TypeUser, "2"), + UserID: 2, }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(identity.RoleAdmin): {Permissions: []accesscontrol.Permission{ @@ -694,7 +693,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "check action prefix filter works correctly", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - TypedID: claims.NewTypeID(claims.TypeUser, "1"), + UserID: 1, }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(identity.RoleEditor): {Permissions: []accesscontrol.Permission{ @@ -715,8 +714,8 @@ func TestService_SearchUserPermissions(t *testing.T) { { name: "check action filter works correctly", searchOption: accesscontrol.SearchOptions{ - Action: accesscontrol.ActionTeamsRead, - TypedID: claims.NewTypeID(claims.TypeUser, "1"), + Action: accesscontrol.ActionTeamsRead, + UserID: 1, }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(identity.RoleEditor): {Permissions: []accesscontrol.Permission{ @@ -737,8 +736,8 @@ func TestService_SearchUserPermissions(t *testing.T) { { name: "check action sets are correctly included if an action is specified", searchOption: accesscontrol.SearchOptions{ - Action: "dashboards:read", - TypedID: claims.NewTypeID(claims.TypeUser, "1"), + Action: "dashboards:read", + UserID: 1, }, withActionSets: true, actionSets: map[string][]string{ @@ -771,7 +770,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "check action sets are correctly included if an action prefix is specified", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "dashboards", - TypedID: claims.NewTypeID(claims.TypeUser, "1"), + UserID: 1, }, withActionSets: true, actionSets: map[string][]string{ diff --git a/pkg/services/accesscontrol/api/api.go b/pkg/services/accesscontrol/api/api.go index 5148eb70206..22c338e1f8b 100644 --- a/pkg/services/accesscontrol/api/api.go +++ b/pkg/services/accesscontrol/api/api.go @@ -1,7 +1,14 @@ package api import ( + "context" + "errors" + "fmt" "net/http" + "strconv" + + "github.com/grafana/authlib/claims" + "go.opentelemetry.io/otel" "github.com/grafana/grafana/pkg/api/response" "github.com/grafana/grafana/pkg/api/routing" @@ -10,16 +17,17 @@ import ( ac "github.com/grafana/grafana/pkg/services/accesscontrol" contextmodel "github.com/grafana/grafana/pkg/services/contexthandler/model" "github.com/grafana/grafana/pkg/services/featuremgmt" - "go.opentelemetry.io/otel" + "github.com/grafana/grafana/pkg/services/user" ) var tracer = otel.Tracer("github.com/grafana/grafana/pkg/services/accesscontrol/api") func NewAccessControlAPI(router routing.RouteRegister, accesscontrol ac.AccessControl, service ac.Service, - features featuremgmt.FeatureToggles) *AccessControlAPI { + userSvc user.Service, features featuremgmt.FeatureToggles) *AccessControlAPI { return &AccessControlAPI{ RouteRegister: router, Service: service, + userSvc: userSvc, AccessControl: accesscontrol, features: features, } @@ -29,6 +37,7 @@ type AccessControlAPI struct { Service ac.Service AccessControl ac.AccessControl RouteRegister routing.RouteRegister + userSvc user.Service features featuremgmt.FeatureToggles } @@ -81,7 +90,20 @@ func (api *AccessControlAPI) searchUsersPermissions(c *contextmodel.ReqContext) ActionPrefix: c.Query("actionPrefix"), Action: c.Query("action"), Scope: c.Query("scope"), - TypedID: c.Query("namespacedId"), + } + + // namespacedId is the typed identifier of an identity + // it is specified using user/service account IDs or UIDs (ex: user:3, service-account:4, user:adisufjf93e9sd) + if typedID := c.Query("namespacedId"); typedID != "" { + userID, err := api.ComputeUserID(ctx, c.Query("namespacedId")) + if err != nil { + if errors.Is(err, user.ErrUserNotFound) { + return response.JSON(http.StatusBadRequest, err.Error()) + } + return response.JSON(http.StatusInternalServerError, err.Error()) + } + + searchOptions.UserID = userID } // Validate inputs @@ -89,7 +111,7 @@ func (api *AccessControlAPI) searchUsersPermissions(c *contextmodel.ReqContext) return response.JSON(http.StatusBadRequest, "'action' and 'actionPrefix' are mutually exclusive") } - if searchOptions.TypedID == "" && searchOptions.ActionPrefix == "" && searchOptions.Action == "" { + if searchOptions.UserID <= 0 && searchOptions.ActionPrefix == "" && searchOptions.Action == "" { return response.JSON(http.StatusBadRequest, "at least one search option must be provided") } @@ -106,3 +128,30 @@ func (api *AccessControlAPI) searchUsersPermissions(c *contextmodel.ReqContext) return response.JSON(http.StatusOK, permsByAction) } + +func (api *AccessControlAPI) ComputeUserID(ctx context.Context, typedID string) (int64, error) { + if typedID == "" { + return -1, nil + } + + typ, idStr, err := claims.ParseTypeID(typedID) + if err != nil { + return 0, err + } + + if !claims.IsIdentityType(typ, claims.TypeUser, claims.TypeServiceAccount) { + return 0, fmt.Errorf("invalid type: %s", typ) + } + + id, err := strconv.ParseInt(idStr, 10, 64) + if err == nil { + return id, nil + } + + user, err := api.userSvc.GetByUID(ctx, &user.GetUserByUIDQuery{UID: idStr}) + if err != nil { + return 0, err + } + + return user.ID, nil +} diff --git a/pkg/services/accesscontrol/api/api_test.go b/pkg/services/accesscontrol/api/api_test.go index ed5c8bae082..4068b3204d7 100644 --- a/pkg/services/accesscontrol/api/api_test.go +++ b/pkg/services/accesscontrol/api/api_test.go @@ -5,6 +5,7 @@ import ( "net/http" "testing" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/grafana/grafana/pkg/api/routing" @@ -13,6 +14,7 @@ import ( "github.com/grafana/grafana/pkg/services/datasources" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/services/user" + "github.com/grafana/grafana/pkg/services/user/usertest" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/web/webtest" ) @@ -40,7 +42,7 @@ func TestAPI_getUserActions(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { acSvc := actest.FakeService{ExpectedPermissions: tt.permissions} - api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, featuremgmt.WithFeatures()) + api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, &usertest.FakeUserService{}, featuremgmt.WithFeatures()) api.RegisterAPIEndpoints() server := webtest.NewServer(t, api.RouteRegister) @@ -93,7 +95,7 @@ func TestAPI_getUserPermissions(t *testing.T) { for _, tt := range tests { t.Run(tt.desc, func(t *testing.T) { acSvc := actest.FakeService{ExpectedPermissions: tt.permissions} - api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, featuremgmt.WithFeatures()) + api := NewAccessControlAPI(routing.NewRouteRegister(), actest.FakeAccessControl{}, acSvc, &usertest.FakeUserService{}, featuremgmt.WithFeatures()) api.RegisterAPIEndpoints() server := webtest.NewServer(t, api.RouteRegister) @@ -149,6 +151,19 @@ func TestAccessControlAPI_searchUsersPermissions(t *testing.T) { expectedCode: http.StatusOK, expectedOutput: map[int64]map[string][]string{2: {"users:read": {"users:*"}}}, }, + { + desc: "Should resolve UID based identifier to the corresponding ID", + filters: "?namespacedId=user:user_2_uid", + permissions: map[int64][]ac.Permission{2: {{Action: "users:read", Scope: "users:*"}}}, + expectedCode: http.StatusOK, + expectedOutput: map[int64]map[string][]string{2: {"users:read": {"users:*"}}}, + }, + { + desc: "Should fail if cannot resolve UID based identifier", + filters: "?namespacedId=user:non_existent_uid", + permissions: map[int64][]ac.Permission{2: {{Action: "users:read", Scope: "users:*"}}}, + expectedCode: http.StatusBadRequest, + }, { desc: "Should reduce permissions", filters: "?namespacedId=service-account:2", @@ -174,7 +189,10 @@ func TestAccessControlAPI_searchUsersPermissions(t *testing.T) { t.Run(tt.desc, func(t *testing.T) { acSvc := actest.FakeService{ExpectedUsersPermissions: tt.permissions} accessControl := actest.FakeAccessControl{ExpectedEvaluate: true} // Always allow access to the endpoint - api := NewAccessControlAPI(routing.NewRouteRegister(), accessControl, acSvc, featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)) + mockUserSvc := usertest.NewMockService(t) + mockUserSvc.On("GetByUID", mock.Anything, &user.GetUserByUIDQuery{UID: "user_2_uid"}).Return(&user.User{ID: 2}, nil).Maybe() + mockUserSvc.On("GetByUID", mock.Anything, &user.GetUserByUIDQuery{UID: "non_existent_uid"}).Return(nil, user.ErrUserNotFound).Maybe() + api := NewAccessControlAPI(routing.NewRouteRegister(), accessControl, acSvc, mockUserSvc, featuremgmt.WithFeatures(featuremgmt.FlagAccessControlOnCall)) api.RegisterAPIEndpoints() server := webtest.NewServer(t, api.RouteRegister) diff --git a/pkg/services/accesscontrol/database/database.go b/pkg/services/accesscontrol/database/database.go index 34ad42c332e..cb89e51145c 100644 --- a/pkg/services/accesscontrol/database/database.go +++ b/pkg/services/accesscontrol/database/database.go @@ -175,15 +175,6 @@ func (s *AccessControlStore) SearchUsersPermissions(ctx context.Context, orgID i } dbPerms := make([]UserRBACPermission, 0) - userID := int64(-1) - if options.TypedID != "" { - var err error - userID, err = options.ComputeUserID() - if err != nil { - return nil, err - } - } - if err := s.sql.WithDbSession(ctx, func(sess *db.Session) error { roleNameFilterJoin := "" if len(options.RolePrefixes) > 0 { @@ -193,28 +184,25 @@ func (s *AccessControlStore) SearchUsersPermissions(ctx context.Context, orgID i params := []any{} direct := userAssignsSQL - if userID >= 0 { - direct += " WHERE ur.user_id = ?" - params = append(params, userID) - } - team := teamAssignsSQL - if userID >= 0 { - team += " WHERE tm.user_id = ?" - params = append(params, userID) - } - basic := basicRoleAssignsSQL - if userID >= 0 { + + if options.UserID > 0 { + direct += " WHERE ur.user_id = ?" + params = append(params, options.UserID) + + team += " WHERE tm.user_id = ?" + params = append(params, options.UserID) + basic += " WHERE ou.user_id = ?" - params = append(params, userID) + params = append(params, options.UserID) } grafanaAdmin := fmt.Sprintf(grafanaAdminAssignsSQL, s.sql.Quote("user")) params = append(params, accesscontrol.RoleGrafanaAdmin) - if userID >= 0 { + if options.UserID > 0 { grafanaAdmin += " AND sa.user_id = ?" - params = append(params, userID) + params = append(params, options.UserID) } // Find permissions diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go index b1f32862085..4cb2491671f 100644 --- a/pkg/services/accesscontrol/database/database_test.go +++ b/pkg/services/accesscontrol/database/database_test.go @@ -9,7 +9,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/grafana/authlib/claims" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/infra/localcache" "github.com/grafana/grafana/pkg/infra/tracing" @@ -625,7 +624,7 @@ func TestIntegrationAccessControlStore_SearchUsersPermissions(t *testing.T) { }, options: accesscontrol.SearchOptions{ ActionPrefix: "teams:", - TypedID: claims.NewTypeID(claims.TypeUser, "1"), + UserID: 1, }, wantPerm: map[int64][]accesscontrol.Permission{ 1: {{Action: "teams:read", Scope: "teams:id:1"}, {Action: "teams:read", Scope: "teams:id:10"},