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

Add auth spans and remove deduplication code for scopes (#89804)

Browse Source 
Adds more spans for timing in accesscontrol and remove permission deduplicating code after benchmarking

---------

Signed-off-by: Dave Henderson <dave.henderson@grafana.com>
Co-authored-by: Dave Henderson <dave.henderson@grafana.com>
Co-authored-by: Ieva <ieva.vasiljeva@grafana.com>
This commit is contained in:
Jeff Levin
2024-07-02 22:08:57 -08:00
committed by GitHub
co-authored by Dave Henderson Ieva
parent 5b6edc96d9
commit cfe8317d45
36 changed files with 279 additions and 97 deletions
Download Patch File Download Diff File Expand all files Collapse all files
pkg/services/authn/authnimpl/sync/oauth_token_sync.go
+7 -1
View File
@@ -9,19 +9,21 @@ import (
"golang.org/x/sync/singleflight"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/login/social"
"github.com/grafana/grafana/pkg/services/auth"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/oauthtoken"
)
func ProvideOAuthTokenSync(service oauthtoken.OAuthTokenService, sessionService auth.UserTokenService, socialService social.Service) *OAuthTokenSync {
func ProvideOAuthTokenSync(service oauthtoken.OAuthTokenService, sessionService auth.UserTokenService, socialService social.Service, tracer tracing.Tracer) *OAuthTokenSync {
return &OAuthTokenSync{
log.New("oauth_token.sync"),
service,
sessionService,
socialService,
new(singleflight.Group),
tracer,
}
}
@@ -31,9 +33,13 @@ type OAuthTokenSync struct {
sessionService auth.UserTokenService
socialService social.Service
singleflightGroup *singleflight.Group
tracer tracing.Tracer
}
func (s *OAuthTokenSync) SyncOauthTokenHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "oauth.sync.SyncOauthTokenHook")
defer span.End()
// only perform oauth token check if identity is a user
if !identity.ID.IsNamespace(authn.NamespaceUser) {
return nil
pkg/services/authn/authnimpl/sync/oauth_token_sync_test.go
+2
View File
@@ -10,6 +10,7 @@ import (
"golang.org/x/sync/singleflight"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/login/social"
"github.com/grafana/grafana/pkg/login/social/socialtest"
"github.com/grafana/grafana/pkg/services/auth"
@@ -128,6 +129,7 @@ func TestOAuthTokenSync_SyncOAuthTokenHook(t *testing.T) {
sessionService: sessionService,
socialService: socialService,
singleflightGroup: new(singleflight.Group),
tracer: tracing.InitializeTracerForTest(),
}
err := sync.SyncOauthTokenHook(context.Background(), tt.identity, nil)
pkg/services/authn/authnimpl/sync/org_sync.go
+14 -4
View File
@@ -7,6 +7,7 @@ import (
"sort"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/org"
@@ -14,8 +15,8 @@ import (
"github.com/grafana/grafana/pkg/setting"
)
func ProvideOrgSync(userService user.Service, orgService org.Service, accessControl accesscontrol.Service, cfg *setting.Cfg) *OrgSync {
return &OrgSync{userService, orgService, accessControl, cfg, log.New("org.sync")}
func ProvideOrgSync(userService user.Service, orgService org.Service, accessControl accesscontrol.Service, cfg *setting.Cfg, tracer tracing.Tracer) *OrgSync {
return &OrgSync{userService, orgService, accessControl, cfg, log.New("org.sync"), tracer}
}
type OrgSync struct {
@@ -23,11 +24,14 @@ type OrgSync struct {
orgService org.Service
accessControl accesscontrol.Service
cfg *setting.Cfg
log log.Logger
log log.Logger
tracer tracing.Tracer
}
func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "org.sync.SyncOrgRolesHook")
defer span.End()
if !id.ClientParams.SyncOrgRoles {
return nil
}
@@ -131,6 +135,9 @@ func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *a
}
func (s *OrgSync) SetDefaultOrgHook(ctx context.Context, currentIdentity *authn.Identity, r *authn.Request, err error) {
ctx, span := s.tracer.Start(ctx, "org.sync.SetDefaultOrgHook")
defer span.End()
if s.cfg.LoginDefaultOrgId < 1 || currentIdentity == nil || err != nil {
return
}
@@ -166,6 +173,9 @@ func (s *OrgSync) SetDefaultOrgHook(ctx context.Context, currentIdentity *authn.
}
func (s *OrgSync) validateUsingOrg(ctx context.Context, userID int64, orgID int64) (bool, error) {
ctx, span := s.tracer.Start(ctx, "org.sync.validateUsingOrg")
defer span.End()
query := org.GetUserOrgListQuery{UserID: userID}
result, err := s.orgService.GetUserOrgList(ctx, &query)
pkg/services/authn/authnimpl/sync/org_sync_test.go
+3
View File
@@ -10,6 +10,7 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/identity"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/accesscontrol/actest"
"github.com/grafana/grafana/pkg/services/authn"
@@ -116,6 +117,7 @@ func TestOrgSync_SyncOrgRolesHook(t *testing.T) {
orgService: tt.fields.orgService,
accessControl: tt.fields.accessControl,
log: tt.fields.log,
tracer: tracing.InitializeTracerForTest(),
}
if err := s.SyncOrgRolesHook(tt.args.ctx, tt.args.id, nil); (err != nil) != tt.wantErr {
t.Errorf("OrgSync.SyncOrgRolesHook() error = %v, wantErr %v", err, tt.wantErr)
@@ -214,6 +216,7 @@ func TestOrgSync_SetDefaultOrgHook(t *testing.T) {
accessControl: actest.FakeService{},
log: log.NewNopLogger(),
cfg: cfg,
tracer: tracing.InitializeTracerForTest(),
}
s.SetDefaultOrgHook(context.Background(), tt.identity, nil, tt.inputErr)
pkg/services/authn/authnimpl/sync/rbac_sync.go
+20 -7
View File
@@ -6,6 +6,7 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/errutil"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/accesscontrol"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
@@ -17,19 +18,24 @@ var (
errSyncPermissionsForbidden = errutil.Forbidden("permissions.sync.forbidden")
)
func ProvideRBACSync(acService accesscontrol.Service) *RBACSync {
func ProvideRBACSync(acService accesscontrol.Service, tracer tracing.Tracer) *RBACSync {
return &RBACSync{
ac: acService,
log: log.New("permissions.sync"),
ac: acService,
log: log.New("permissions.sync"),
tracer: tracer,
}
}
type RBACSync struct {
ac accesscontrol.Service
log log.Logger
ac accesscontrol.Service
log log.Logger
tracer tracing.Tracer
}
func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identity, _ *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "rbac.sync.SyncPermissionsHook")
defer span.End()
if !ident.ClientParams.SyncPermissions {
return nil
}
@@ -43,7 +49,8 @@ func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identit
if ident.Permissions == nil {
ident.Permissions = make(map[int64]map[string][]string, 1)
}
grouped := accesscontrol.GroupScopesByAction(permissions)
grouped := accesscontrol.GroupScopesByActionContext(ctx, permissions)
// Restrict access to the list of actions
actionsLookup := ident.ClientParams.FetchPermissionsParams.ActionsLookup
@@ -56,12 +63,15 @@ func (s *RBACSync) SyncPermissionsHook(ctx context.Context, ident *authn.Identit
}
grouped = filtered
}
ident.Permissions[ident.OrgID] = grouped
return nil
}
func (s *RBACSync) fetchPermissions(ctx context.Context, ident *authn.Identity) ([]accesscontrol.Permission, error) {
ctx, span := s.tracer.Start(ctx, "rbac.sync.fetchPermissions")
defer span.End()
permissions := make([]accesscontrol.Permission, 0, 8)
roles := ident.ClientParams.FetchPermissionsParams.Roles
if len(roles) > 0 {
@@ -94,6 +104,9 @@ var fixedCloudRoles = map[org.RoleType]string{
}
func (s *RBACSync) SyncCloudRoles(ctx context.Context, ident *authn.Identity, r *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "rbac.sync.SyncCloudRoles")
defer span.End()
// we only want to run this hook during login and if the module used is grafana com
if r.GetMeta(authn.MetaKeyAuthModule) != login.GrafanaComAuthModule {
return nil
pkg/services/authn/authnimpl/sync/rbac_sync_test.go
+7 -4
View File
@@ -5,6 +5,7 @@ import (
"testing"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/accesscontrol"
acmock "github.com/grafana/grafana/pkg/services/accesscontrol/mock"
"github.com/grafana/grafana/pkg/services/authn"
@@ -45,7 +46,7 @@ func TestRBACSync_SyncPermission(t *testing.T) {
require.NoError(t, err)
assert.Equal(t, 1, len(tt.identity.Permissions))
assert.Equal(t, accesscontrol.GroupScopesByAction(tt.expectedPermissions), tt.identity.Permissions[tt.identity.OrgID])
assert.Equal(t, accesscontrol.GroupScopesByActionContext(context.Background(), tt.expectedPermissions), tt.identity.Permissions[tt.identity.OrgID])
})
}
}
@@ -127,7 +128,8 @@ func TestRBACSync_SyncCloudRoles(t *testing.T) {
return nil
},
},
log: log.NewNopLogger(),
log: log.NewNopLogger(),
tracer: tracing.InitializeTracerForTest(),
}
req := &authn.Request{}
@@ -149,8 +151,9 @@ func setupTestEnv() *RBACSync {
},
}
s := &RBACSync{
ac: acMock,
log: log.NewNopLogger(),
ac: acMock,
log: log.NewNopLogger(),
tracer: tracing.InitializeTracerForTest(),
}
return s
}
pkg/services/authn/authnimpl/sync/user_sync.go
+30 -3
View File
@@ -7,6 +7,7 @@ import (
"github.com/grafana/grafana/pkg/apimachinery/errutil"
"github.com/grafana/grafana/pkg/infra/log"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/org"
@@ -47,15 +48,14 @@ var (
errSignupNotAllowed = errors.New("system administrator has disabled signup")
)
func ProvideUserSync(userService user.Service,
userProtectionService login.UserProtectionService,
authInfoService login.AuthInfoService, quotaService quota.Service) *UserSync {
func ProvideUserSync(userService user.Service, userProtectionService login.UserProtectionService, authInfoService login.AuthInfoService, quotaService quota.Service, tracer tracing.Tracer) *UserSync {
return &UserSync{
userService: userService,
authInfoService: authInfoService,
userProtectionService: userProtectionService,
quotaService: quotaService,
log: log.New("user.sync"),
tracer: tracer,
}
}
@@ -65,10 +65,14 @@ type UserSync struct {
userProtectionService login.UserProtectionService
quotaService quota.Service
log log.Logger
tracer tracing.Tracer
}
// SyncUserHook syncs a user with the database
func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "user.sync.SyncUserHook")
defer span.End()
if !id.ClientParams.SyncUser {
return nil
}
@@ -106,6 +110,9 @@ func (s *UserSync) SyncUserHook(ctx context.Context, id *authn.Identity, _ *auth
}
func (s *UserSync) FetchSyncedUserHook(ctx context.Context, identity *authn.Identity, r *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "user.sync.FetchSyncedUserHook")
defer span.End()
if !identity.ClientParams.FetchSyncedUser {
return nil
}
@@ -143,6 +150,9 @@ func (s *UserSync) FetchSyncedUserHook(ctx context.Context, identity *authn.Iden
}
func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identity, r *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "user.sync.SyncLastSeenHook")
defer span.End()
if r.GetMeta(authn.MetaKeyIsLogin) != "" {
// Do not sync last seen for login requests
return nil
@@ -177,6 +187,9 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit
}
func (s *UserSync) EnableUserHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
ctx, span := s.tracer.Start(ctx, "user.sync.EnableUserHook")
defer span.End()
if !identity.ClientParams.EnableUser {
return nil
}
@@ -196,6 +209,9 @@ func (s *UserSync) EnableUserHook(ctx context.Context, identity *authn.Identity,
}
func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, identity *authn.Identity, createConnection bool) error {
ctx, span := s.tracer.Start(ctx, "user.sync.upsertAuthConnection")
defer span.End()
if identity.AuthenticatedBy == "" {
return nil
}
@@ -222,6 +238,9 @@ func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, ident
}
func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id *authn.Identity, userAuth *login.UserAuth) error {
ctx, span := s.tracer.Start(ctx, "user.sync.updateUserAttributes")
defer span.End()
if errProtection := s.userProtectionService.AllowUserMapping(usr, id.AuthenticatedBy); errProtection != nil {
return errUserProtection.Errorf("user mapping not allowed: %w", errProtection)
}
@@ -273,6 +292,8 @@ func (s *UserSync) updateUserAttributes(ctx context.Context, usr *user.User, id
}
func (s *UserSync) createUser(ctx context.Context, id *authn.Identity) (*user.User, error) {
ctx, span := s.tracer.Start(ctx, "user.sync.createUser")
defer span.End()
// FIXME(jguer): this should be done in the user service
// quota check: we can have quotas on both global and org level
// therefore we need to query check quota for both user and org services
@@ -312,6 +333,9 @@ func (s *UserSync) createUser(ctx context.Context, id *authn.Identity) (*user.Us
}
func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user.User, *login.UserAuth, error) {
ctx, span := s.tracer.Start(ctx, "user.sync.getUser")
defer span.End()
// Check auth info fist
if identity.AuthID != "" && identity.AuthenticatedBy != "" {
query := &login.GetAuthInfoQuery{AuthId: identity.AuthID, AuthModule: identity.AuthenticatedBy}
@@ -361,6 +385,9 @@ func (s *UserSync) getUser(ctx context.Context, identity *authn.Identity) (*user
}
func (s *UserSync) lookupByOneOf(ctx context.Context, params login.UserLookupParams) (*user.User, error) {
ctx, span := s.tracer.Start(ctx, "user.sync.lookupByOneOf")
defer span.End()
var usr *user.User
var err error
pkg/services/authn/authnimpl/sync/user_sync_test.go
+6 -3
View File
@@ -7,6 +7,7 @@ import (
"github.com/stretchr/testify/assert"
"github.com/stretchr/testify/require"
"github.com/grafana/grafana/pkg/infra/tracing"
"github.com/grafana/grafana/pkg/services/authn"
"github.com/grafana/grafana/pkg/services/login"
"github.com/grafana/grafana/pkg/services/login/authinfoimpl"
@@ -426,7 +427,7 @@ func TestUserSync_SyncUserHook(t *testing.T) {
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
s := ProvideUserSync(tt.fields.userService, userProtection, tt.fields.authInfoService, tt.fields.quotaService)
s := ProvideUserSync(tt.fields.userService, userProtection, tt.fields.authInfoService, tt.fields.quotaService, tracing.InitializeTracerForTest())
err := s.SyncUserHook(tt.args.ctx, tt.args.id, nil)
if tt.wantErr {
require.Error(t, err)
@@ -462,7 +463,9 @@ func TestUserSync_FetchSyncedUserHook(t *testing.T) {
for _, tt := range tests {
t.Run(tt.desc, func(t *testing.T) {
s := UserSync{}
s := UserSync{
tracer: tracing.InitializeTracerForTest(),
}
err := s.FetchSyncedUserHook(context.Background(), tt.identity, tt.req)
require.ErrorIs(t, err, tt.expectedErr)
})
@@ -515,7 +518,7 @@ func TestUserSync_EnableDisabledUserHook(t *testing.T) {
return nil
}
s := UserSync{userService: userSvc}
s := UserSync{userService: userSvc, tracer: tracing.InitializeTracerForTest()}
err := s.EnableUserHook(context.Background(), tt.identity, nil)
require.NoError(t, err)
assert.Equal(t, tt.enableUser, called)