From 63cd5a5625c22dcb5603b10d54228363bcaa70a6 Mon Sep 17 00:00:00 2001 From: Vardan Torosyan Date: Thu, 21 Dec 2023 20:42:05 +0100 Subject: [PATCH] Chore: Cleanup namespace and ID resolution (#79360) * Chore: Cleanup namespace ID resolution * Check for negative userID when relevant * Reuse existing function for parsing ID as int * Fix imports --- pkg/services/auth/idimpl/service.go | 2 +- pkg/services/authn/authnimpl/service.go | 26 +++++++------ .../authn/authnimpl/sync/oauth_token_sync.go | 2 +- pkg/services/authn/authnimpl/sync/org_sync.go | 11 +++++- .../authn/authnimpl/sync/user_sync.go | 37 +++++++++++++------ pkg/services/authn/clients/api_key.go | 8 +++- pkg/services/authn/clients/proxy.go | 8 +++- pkg/services/authn/identity.go | 17 --------- 8 files changed, 65 insertions(+), 46 deletions(-) diff --git a/pkg/services/auth/idimpl/service.go b/pkg/services/auth/idimpl/service.go index ca7ff43896f..566ad620838 100644 --- a/pkg/services/auth/idimpl/service.go +++ b/pkg/services/auth/idimpl/service.go @@ -102,7 +102,7 @@ func (s *Service) hook(ctx context.Context, identity *authn.Identity, _ *authn.R // FIXME(kalleep): we should probably lazy load this token, err := s.SignIdentity(ctx, identity) if err != nil { - namespace, id := identity.NamespacedID() + namespace, id := identity.GetNamespacedID() s.logger.Error("Failed to sign id token", "err", err, "namespace", namespace, "id", id) // for now don't return error so we don't break authentication from this hook return nil diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index adf0ad0e980..661b03aed42 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -263,7 +263,7 @@ func (s *Service) RegisterPostAuthHook(hook authn.PostAuthHookFn, priority uint) s.postAuthHooks.insert(hook, priority) } -func (s *Service) Login(ctx context.Context, client string, r *authn.Request) (identity *authn.Identity, err error) { +func (s *Service) Login(ctx context.Context, client string, r *authn.Request) (id *authn.Identity, err error) { ctx, span := s.tracer.Start(ctx, "authn.Login", trace.WithAttributes( attribute.String(attributeKeyClient, client), )) @@ -273,7 +273,7 @@ func (s *Service) Login(ctx context.Context, client string, r *authn.Request) (i defer func() { for _, hook := range s.postLoginHooks.items { - hook.v(ctx, identity, r, err) + hook.v(ctx, id, r, err) } }() @@ -284,36 +284,40 @@ func (s *Service) Login(ctx context.Context, client string, r *authn.Request) (i } r.SetMeta(authn.MetaKeyIsLogin, "true") - identity, err = s.authenticate(ctx, c, r) + id, err = s.authenticate(ctx, c, r) if err != nil { s.metrics.failedLogin.WithLabelValues(client).Inc() return nil, err } - namespace, id := identity.NamespacedID() - + namespace, namespaceID := id.GetNamespacedID() // Login is only supported for users - if namespace != authn.NamespaceUser || id <= 0 { + if namespace != authn.NamespaceUser { s.metrics.failedLogin.WithLabelValues(client).Inc() return nil, authn.ErrUnsupportedIdentity.Errorf("expected identity of type user but got: %s", namespace) } + intId, err := identity.IntIdentifier(namespace, namespaceID) + if err != nil { + return nil, err + } + addr := web.RemoteAddr(r.HTTPRequest) ip, err := network.GetIPFromAddress(addr) if err != nil { - s.log.FromContext(ctx).Debug("Failed to parse ip from address", "client", c.Name(), "id", identity.ID, "addr", addr, "error", err) + s.log.FromContext(ctx).Debug("Failed to parse ip from address", "client", c.Name(), "id", id.ID, "addr", addr, "error", err) } - sessionToken, err := s.sessionService.CreateToken(ctx, &user.User{ID: id}, ip, r.HTTPRequest.UserAgent()) + sessionToken, err := s.sessionService.CreateToken(ctx, &user.User{ID: intId}, ip, r.HTTPRequest.UserAgent()) if err != nil { s.metrics.failedLogin.WithLabelValues(client).Inc() - s.log.FromContext(ctx).Error("Failed to create session", "client", client, "id", identity.ID, "err", err) + s.log.FromContext(ctx).Error("Failed to create session", "client", client, "id", id.ID, "err", err) return nil, err } s.metrics.successfulLogin.WithLabelValues(client).Inc() - identity.SessionToken = sessionToken - return identity, nil + id.SessionToken = sessionToken + return id, nil } func (s *Service) RegisterPostLoginHook(hook authn.PostLoginHookFn, priority uint) { diff --git a/pkg/services/authn/authnimpl/sync/oauth_token_sync.go b/pkg/services/authn/authnimpl/sync/oauth_token_sync.go index 22bbe03ee40..c1a852b696b 100644 --- a/pkg/services/authn/authnimpl/sync/oauth_token_sync.go +++ b/pkg/services/authn/authnimpl/sync/oauth_token_sync.go @@ -40,7 +40,7 @@ type OAuthTokenSync struct { } func (s *OAuthTokenSync) SyncOauthTokenHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error { - namespace, _ := identity.NamespacedID() + namespace, _ := identity.GetNamespacedID() // only perform oauth token check if identity is a user if namespace != authn.NamespaceUser { return nil diff --git a/pkg/services/authn/authnimpl/sync/org_sync.go b/pkg/services/authn/authnimpl/sync/org_sync.go index a2203411385..43e1bbf0249 100644 --- a/pkg/services/authn/authnimpl/sync/org_sync.go +++ b/pkg/services/authn/authnimpl/sync/org_sync.go @@ -7,6 +7,7 @@ import ( "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/accesscontrol" + "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/org" "github.com/grafana/grafana/pkg/services/user" @@ -31,12 +32,18 @@ func (s *OrgSync) SyncOrgRolesHook(ctx context.Context, id *authn.Identity, _ *a ctxLogger := s.log.FromContext(ctx) - namespace, userID := id.NamespacedID() - if namespace != authn.NamespaceUser || userID <= 0 { + namespace, identifier := id.GetNamespacedID() + if namespace != authn.NamespaceUser { ctxLogger.Warn("Failed to sync org role, invalid namespace for identity", "id", id.ID, "namespace", namespace) return nil } + userID, err := identity.IntIdentifier(namespace, identifier) + if err != nil { + ctxLogger.Warn("Failed to sync org role, invalid ID for identity", "id", id.ID, "namespace", namespace, "err", err) + return nil + } + ctxLogger.Debug("Syncing organization roles", "id", id.ID, "extOrgRoles", id.OrgRoles) // don't sync org roles if none is specified if len(id.OrgRoles) == 0 { diff --git a/pkg/services/authn/authnimpl/sync/user_sync.go b/pkg/services/authn/authnimpl/sync/user_sync.go index 0b0a5b09b91..71a96c18124 100644 --- a/pkg/services/authn/authnimpl/sync/user_sync.go +++ b/pkg/services/authn/authnimpl/sync/user_sync.go @@ -6,6 +6,7 @@ import ( "fmt" "github.com/grafana/grafana/pkg/infra/log" + authidentity "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" @@ -109,13 +110,19 @@ func (s *UserSync) FetchSyncedUserHook(ctx context.Context, identity *authn.Iden if !identity.ClientParams.FetchSyncedUser { return nil } - namespace, id := identity.NamespacedID() + namespace, id := identity.GetNamespacedID() if namespace != authn.NamespaceUser { return nil } + userID, err := authidentity.IntIdentifier(namespace, id) + if err != nil { + s.log.FromContext(ctx).Warn("got invalid identity ID", "id", id, "err", err) + return nil + } + usr, err := s.userService.GetSignedInUserWithCacheCtx(ctx, &user.GetSignedInUserQuery{ - UserID: id, + UserID: userID, OrgID: r.OrgID, }) if err != nil { @@ -135,15 +142,15 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit return nil } - namespace, id := identity.NamespacedID() - - // do not sync invalid users - if id <= 0 { - return nil // skip sync + namespace, id := identity.GetNamespacedID() + if namespace != authn.NamespaceUser && namespace != authn.NamespaceServiceAccount { + return nil } - if namespace != authn.NamespaceUser && namespace != authn.NamespaceServiceAccount { - return nil // skip sync + userID, err := authidentity.IntIdentifier(namespace, id) + if err != nil { + s.log.FromContext(ctx).Warn("got invalid identity ID", "id", id, "err", err) + return nil } go func(userID int64) { @@ -158,7 +165,7 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit !errors.Is(err, user.ErrLastSeenUpToDate) { s.log.Error("Failed to update last_seen_at", "err", err, "userId", userID) } - }(id) + }(userID) return nil } @@ -168,12 +175,18 @@ func (s *UserSync) EnableUserHook(ctx context.Context, identity *authn.Identity, return nil } - namespace, id := identity.NamespacedID() + namespace, id := identity.GetNamespacedID() if namespace != authn.NamespaceUser { return nil } - return s.userService.Disable(ctx, &user.DisableUserCommand{UserID: id, IsDisabled: false}) + userID, err := authidentity.IntIdentifier(namespace, id) + if err != nil { + s.log.FromContext(ctx).Warn("got invalid identity ID", "id", id, "err", err) + return nil + } + + return s.userService.Disable(ctx, &user.DisableUserCommand{UserID: userID, IsDisabled: false}) } func (s *UserSync) upsertAuthConnection(ctx context.Context, userID int64, identity *authn.Identity, createConnection bool) error { diff --git a/pkg/services/authn/clients/api_key.go b/pkg/services/authn/clients/api_key.go index b4cf6bc42a5..34c3fc180fe 100644 --- a/pkg/services/authn/clients/api_key.go +++ b/pkg/services/authn/clients/api_key.go @@ -10,6 +10,7 @@ import ( "github.com/grafana/grafana/pkg/components/satokengen" "github.com/grafana/grafana/pkg/infra/log" "github.com/grafana/grafana/pkg/services/apikey" + authidentity "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/org" @@ -175,8 +176,13 @@ func (s *APIKey) Hook(ctx context.Context, identity *authn.Identity, r *authn.Re } func (s *APIKey) getAPIKeyID(ctx context.Context, identity *authn.Identity, r *authn.Request) (apiKeyID int64, exists bool) { - namespace, id := identity.NamespacedID() + namespace, identifier := identity.GetNamespacedID() + id, err := authidentity.IntIdentifier(namespace, identifier) + if err != nil { + s.log.Warn("Failed to parse ID from identifier", "err", err) + return -1, false + } if namespace == authn.NamespaceAPIKey { return id, true } diff --git a/pkg/services/authn/clients/proxy.go b/pkg/services/authn/clients/proxy.go index 06abe1d64b9..fd19789274d 100644 --- a/pkg/services/authn/clients/proxy.go +++ b/pkg/services/authn/clients/proxy.go @@ -12,6 +12,7 @@ import ( "time" "github.com/grafana/grafana/pkg/infra/log" + authidentity "github.com/grafana/grafana/pkg/services/auth/identity" "github.com/grafana/grafana/pkg/services/authn" "github.com/grafana/grafana/pkg/services/login" "github.com/grafana/grafana/pkg/services/user" @@ -135,11 +136,16 @@ func (c *Proxy) Hook(ctx context.Context, identity *authn.Identity, r *authn.Req return nil } - namespace, id := identity.NamespacedID() + namespace, identifier := identity.GetNamespacedID() if namespace != authn.NamespaceUser { return nil } + id, err := authidentity.IntIdentifier(namespace, identifier) + if err != nil { + c.log.Warn("Failed to cache proxy user", "error", err, "userId", identifier, "err", err) + return nil + } c.log.FromContext(ctx).Debug("Cache proxy user", "userId", id) bytes := []byte(strconv.FormatInt(id, 10)) if err := c.cache.Set(ctx, identity.ClientParams.CacheAuthProxyKey, bytes, time.Duration(c.cfg.AuthProxySyncTTL)*time.Minute); err != nil { diff --git a/pkg/services/authn/identity.go b/pkg/services/authn/identity.go index 74a827010cb..dc8f8cce04c 100644 --- a/pkg/services/authn/identity.go +++ b/pkg/services/authn/identity.go @@ -185,23 +185,6 @@ func (i *Identity) IsNil() bool { return i == nil } -// NamespacedID returns the namespace, e.g. "user" and the id for that namespace -// FIXME(kalleep): Replace with GetNamespacedID -func (i *Identity) NamespacedID() (string, int64) { - split := strings.Split(i.ID, ":") - if len(split) != 2 { - return "", -1 - } - - id, err := strconv.ParseInt(split[1], 10, 64) - if err != nil { - // FIXME (kalleep): Improve error handling - return "", -1 - } - - return split[0], id -} - // SignedInUser returns a SignedInUser from the identity. func (i *Identity) SignedInUser() *user.SignedInUser { namespace, id := i.GetNamespacedID()