From 41bee274fd764a46348155acf968343fd989fd02 Mon Sep 17 00:00:00 2001 From: Dan Cech Date: Mon, 6 May 2024 14:17:34 -0400 Subject: [PATCH] Chore: Fix error handling in postDashboard, remove UserDisplayDTO, fix live redis client initialization (#87206) * clean up error handling in postDashboard and remove UserDisplayDTO * replace GetUserUID with GetUID and GetNamespacedUID, enforce namespace constant type * lint fix * lint fix * more lint fixes --- pkg/api/dashboard.go | 23 ++--- pkg/api/org_invite.go | 3 +- pkg/api/user.go | 2 +- pkg/login/social/connectors/gitlab_oauth.go | 2 +- pkg/login/social/connectors/google_oauth.go | 2 +- pkg/services/accesscontrol/accesscontrol.go | 25 ++--- .../accesscontrol/acimpl/service_test.go | 13 +-- .../accesscontrol/database/database_test.go | 2 +- .../accesscontrol/metadata_bench_test.go | 4 +- pkg/services/auth/identity/namespace.go | 87 ++++++++++------- pkg/services/auth/identity/requester.go | 13 ++- pkg/services/auth/idimpl/service.go | 2 +- pkg/services/authn/authnimpl/service.go | 2 +- pkg/services/authn/authnimpl/service_test.go | 2 +- pkg/services/authn/clients/api_key.go | 2 +- pkg/services/authn/identity.go | 13 ++- pkg/services/contexthandler/contexthandler.go | 2 +- .../dashboardimport/service/service.go | 7 +- .../dashboardsnapshots/database/database.go | 4 +- pkg/services/libraryelements/database.go | 9 +- pkg/services/live/features/dashboard.go | 44 +++++++-- pkg/services/live/live.go | 7 +- .../sqlstore/permissions/dashboard.go | 7 +- .../dashboard_filter_no_subquery.go | 7 +- pkg/services/user/identity.go | 94 +++++++++---------- pkg/services/user/model.go | 8 -- pkg/util/proxyutil/proxyutil.go | 3 +- 27 files changed, 206 insertions(+), 183 deletions(-) diff --git a/pkg/api/dashboard.go b/pkg/api/dashboard.go index bc24cc4d365..c273e798930 100644 --- a/pkg/api/dashboard.go +++ b/pkg/api/dashboard.go @@ -336,17 +336,13 @@ func (hs *HTTPServer) deleteDashboard(c *contextmodel.ReqContext) response.Respo return response.Error(http.StatusInternalServerError, "Failed to delete dashboard", err) } - userDTODisplay, err := user.NewUserDisplayDTOFromRequester(c.SignedInUser) - if err != nil { - return response.Error(http.StatusInternalServerError, "Error while parsing the user DTO model", err) - } - if hs.Live != nil { - err := hs.Live.GrafanaScope.Dashboards.DashboardDeleted(c.SignedInUser.GetOrgID(), userDTODisplay, dash.UID) + err := hs.Live.GrafanaScope.Dashboards.DashboardDeleted(c.SignedInUser.GetOrgID(), c.SignedInUser, dash.UID) if err != nil { hs.log.Error("Failed to broadcast delete info", "dashboard", dash.UID, "error", err) } } + return response.JSON(http.StatusOK, util.DynMap{ "title": dash.Title, "message": fmt.Sprintf("Dashboard %s deleted", dash.Title), @@ -440,7 +436,7 @@ func (hs *HTTPServer) postDashboard(c *contextmodel.ReqContext, cmd dashboards.S Overwrite: cmd.Overwrite, } - dashboard, err := hs.DashboardService.SaveDashboard(ctx, dashItem, allowUiUpdate) + dashboard, saveErr := hs.DashboardService.SaveDashboard(ctx, dashItem, allowUiUpdate) if hs.Live != nil { // Tell everyone listening that the dashboard changed @@ -448,18 +444,13 @@ func (hs *HTTPServer) postDashboard(c *contextmodel.ReqContext, cmd dashboards.S dashboard = dash // the original request } - userDTODisplay, err := user.NewUserDisplayDTOFromRequester(c.SignedInUser) - if err != nil { - return response.Error(http.StatusInternalServerError, "Error while parsing the user DTO model", err) - } - // This will broadcast all save requests only if a `gitops` observer exists. // gitops is useful when trying to save dashboards in an environment where the user can not save channel := hs.Live.GrafanaScope.Dashboards - liveerr := channel.DashboardSaved(c.SignedInUser.GetOrgID(), userDTODisplay, cmd.Message, dashboard, err) + liveerr := channel.DashboardSaved(c.SignedInUser.GetOrgID(), c.SignedInUser, cmd.Message, dashboard, saveErr) // When an error exists, but the value broadcast to a gitops listener return 202 - if liveerr == nil && err != nil && channel.HasGitOpsObserver(c.SignedInUser.GetOrgID()) { + if liveerr == nil && saveErr != nil && channel.HasGitOpsObserver(c.SignedInUser.GetOrgID()) { return response.JSON(http.StatusAccepted, util.DynMap{ "status": "pending", "message": "changes were broadcast to the gitops listener", @@ -471,8 +462,8 @@ func (hs *HTTPServer) postDashboard(c *contextmodel.ReqContext, cmd dashboards.S } } - if err != nil { - return apierrors.ToDashboardErrorResponse(ctx, hs.pluginStore, err) + if saveErr != nil { + return apierrors.ToDashboardErrorResponse(ctx, hs.pluginStore, saveErr) } // Clear permission cache for the user who's created the dashboard, so that new permissions are fetched for their next call diff --git a/pkg/api/org_invite.go b/pkg/api/org_invite.go index ee41146eacf..0c058ec822d 100644 --- a/pkg/api/org_invite.go +++ b/pkg/api/org_invite.go @@ -103,8 +103,7 @@ func (hs *HTTPServer) AddOrgInvite(c *contextmodel.ReqContext) response.Response namespace, identifier := c.SignedInUser.GetNamespacedID() var userID int64 - switch namespace { - case identity.NamespaceUser, identity.NamespaceServiceAccount: + if namespace == identity.NamespaceUser || namespace == identity.NamespaceServiceAccount { var err error userID, err = strconv.ParseInt(identifier, 10, 64) if err != nil { diff --git a/pkg/api/user.go b/pkg/api/user.go index d64ff7a15ca..d7df33c38e2 100644 --- a/pkg/api/user.go +++ b/pkg/api/user.go @@ -36,7 +36,7 @@ func (hs *HTTPServer) GetSignedInUser(c *contextmodel.ReqContext) response.Respo return response.JSON(http.StatusOK, user.UserProfileDTO{ IsGrafanaAdmin: c.SignedInUser.GetIsGrafanaAdmin(), OrgID: c.SignedInUser.GetOrgID(), - UID: strings.Join([]string{namespace, identifier}, ":"), + UID: c.SignedInUser.GetID().String(), Name: c.SignedInUser.NameOrFallback(), Email: c.SignedInUser.GetEmail(), Login: c.SignedInUser.GetLogin(), diff --git a/pkg/login/social/connectors/gitlab_oauth.go b/pkg/login/social/connectors/gitlab_oauth.go index b588d2c2ebf..da6307a5cdc 100644 --- a/pkg/login/social/connectors/gitlab_oauth.go +++ b/pkg/login/social/connectors/gitlab_oauth.go @@ -215,7 +215,7 @@ func (s *SocialGitlab) UserInfo(ctx context.Context, client *http.Client, token return userInfo, nil } -func (s *SocialGitlab) extractFromAPI(ctx context.Context, client *http.Client, token *oauth2.Token) (*userData, error) { +func (s *SocialGitlab) extractFromAPI(ctx context.Context, client *http.Client, _ *oauth2.Token) (*userData, error) { apiResp := &apiData{} response, err := s.httpGet(ctx, client, s.info.ApiUrl+"/user") if err != nil { diff --git a/pkg/login/social/connectors/google_oauth.go b/pkg/login/social/connectors/google_oauth.go index 0bda73cd62a..b70cce3263b 100644 --- a/pkg/login/social/connectors/google_oauth.go +++ b/pkg/login/social/connectors/google_oauth.go @@ -220,7 +220,7 @@ func (s *SocialGoogle) AuthCodeURL(state string, opts ...oauth2.AuthCodeOption) return s.SocialBase.Config.AuthCodeURL(state, opts...) } -func (s *SocialGoogle) extractFromToken(ctx context.Context, client *http.Client, token *oauth2.Token) (*googleUserData, error) { +func (s *SocialGoogle) extractFromToken(_ context.Context, _ *http.Client, token *oauth2.Token) (*googleUserData, error) { s.log.Debug("Extracting user info from OAuth token") idToken := token.Extra("id_token") diff --git a/pkg/services/accesscontrol/accesscontrol.go b/pkg/services/accesscontrol/accesscontrol.go index 6f7a0eb76cb..e84d5b8dd75 100644 --- a/pkg/services/accesscontrol/accesscontrol.go +++ b/pkg/services/accesscontrol/accesscontrol.go @@ -4,7 +4,6 @@ import ( "context" "errors" "fmt" - "strconv" "strings" "github.com/grafana/grafana/pkg/registry" @@ -100,22 +99,18 @@ func (s *SearchOptions) ComputeUserID() (int64, error) { if s.NamespacedID == "" { return 0, errors.New("namespacedID must be set") } - // Split namespaceID into namespace and ID - parts := strings.Split(s.NamespacedID, ":") - // Validate namespace ID format - if len(parts) != 2 { - return 0, fmt.Errorf("invalid namespaced ID: %s", s.NamespacedID) - } - // Validate namespace type is user or service account - if parts[0] != identity.NamespaceUser && parts[0] != identity.NamespaceServiceAccount { - return 0, fmt.Errorf("invalid namespace: %s", parts[0]) - } - // Validate namespace ID is a number - id, err := strconv.ParseInt(parts[1], 10, 64) + + id, err := identity.ParseNamespaceID(s.NamespacedID) if err != nil { - return 0, fmt.Errorf("invalid namespaced ID: %s", s.NamespacedID) + return 0, err } - return id, nil + + // Validate namespace type is user or service account + if id.Namespace() != identity.NamespaceUser && id.Namespace() != identity.NamespaceServiceAccount { + return 0, fmt.Errorf("invalid namespace: %s", id.Namespace()) + } + + return id.ParseInt() } type SyncUserRolesCommand struct { diff --git a/pkg/services/accesscontrol/acimpl/service_test.go b/pkg/services/accesscontrol/acimpl/service_test.go index 8ce30216268..b4ec36b4912 100644 --- a/pkg/services/accesscontrol/acimpl/service_test.go +++ b/pkg/services/accesscontrol/acimpl/service_test.go @@ -2,6 +2,7 @@ package acimpl import ( "context" + "fmt" "testing" "github.com/stretchr/testify/assert" @@ -537,7 +538,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{NamespacedID: identity.NamespaceServiceAccount + ":1"}, + searchOption: accesscontrol.SearchOptions{NamespacedID: fmt.Sprintf("%s:1", identity.NamespaceServiceAccount)}, ramRoles: map[string]*accesscontrol.RoleDTO{ string(roletype.RoleEditor): {Permissions: []accesscontrol.Permission{ {Action: accesscontrol.ActionTeamsRead, Scope: "teams:*"}, @@ -607,7 +608,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "ram only", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - NamespacedID: identity.NamespaceUser + ":2", + NamespacedID: fmt.Sprintf("%s:2", identity.NamespaceUser), }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(roletype.RoleEditor): {Permissions: []accesscontrol.Permission{ @@ -632,7 +633,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "stored only", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - NamespacedID: identity.NamespaceUser + ":2", + NamespacedID: fmt.Sprintf("%s:2", identity.NamespaceUser), }, storedPerms: map[int64][]accesscontrol.Permission{ 1: {{Action: accesscontrol.ActionTeamsRead, Scope: "teams:id:1"}}, @@ -652,7 +653,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "ram and stored", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - NamespacedID: identity.NamespaceUser + ":2", + NamespacedID: fmt.Sprintf("%s:2", identity.NamespaceUser), }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(roletype.RoleAdmin): {Permissions: []accesscontrol.Permission{ @@ -682,7 +683,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "check action prefix filter works correctly", searchOption: accesscontrol.SearchOptions{ ActionPrefix: "teams", - NamespacedID: identity.NamespaceUser + ":1", + NamespacedID: fmt.Sprintf("%s:1", identity.NamespaceUser), }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(roletype.RoleEditor): {Permissions: []accesscontrol.Permission{ @@ -704,7 +705,7 @@ func TestService_SearchUserPermissions(t *testing.T) { name: "check action filter works correctly", searchOption: accesscontrol.SearchOptions{ Action: accesscontrol.ActionTeamsRead, - NamespacedID: identity.NamespaceUser + ":1", + NamespacedID: fmt.Sprintf("%s:1", identity.NamespaceUser), }, ramRoles: map[string]*accesscontrol.RoleDTO{ string(roletype.RoleEditor): {Permissions: []accesscontrol.Permission{ diff --git a/pkg/services/accesscontrol/database/database_test.go b/pkg/services/accesscontrol/database/database_test.go index fda8acff224..6f9d4b3d8ca 100644 --- a/pkg/services/accesscontrol/database/database_test.go +++ b/pkg/services/accesscontrol/database/database_test.go @@ -549,7 +549,7 @@ func TestIntegrationAccessControlStore_SearchUsersPermissions(t *testing.T) { }, options: accesscontrol.SearchOptions{ ActionPrefix: "teams:", - NamespacedID: identity.NamespaceUser + ":1", + NamespacedID: fmt.Sprintf("%s:1", identity.NamespaceUser), }, wantPerm: map[int64][]accesscontrol.Permission{ 1: {{Action: "teams:read", Scope: "teams:id:1"}, {Action: "teams:read", Scope: "teams:id:10"}, diff --git a/pkg/services/accesscontrol/metadata_bench_test.go b/pkg/services/accesscontrol/metadata_bench_test.go index 6920acb6c0f..306e7bc8efa 100644 --- a/pkg/services/accesscontrol/metadata_bench_test.go +++ b/pkg/services/accesscontrol/metadata_bench_test.go @@ -8,7 +8,7 @@ import ( "github.com/stretchr/testify/assert" ) -func setupTestEnv(b *testing.B, resourceCount, permissionPerResource int) (map[string][]string, map[string]bool) { +func setupTestEnv(resourceCount, permissionPerResource int) (map[string][]string, map[string]bool) { res := map[string][]string{} ids := make(map[string]bool, resourceCount) @@ -25,7 +25,7 @@ func setupTestEnv(b *testing.B, resourceCount, permissionPerResource int) (map[s } func benchGetMetadata(b *testing.B, resourceCount, permissionPerResource int) { - permissions, ids := setupTestEnv(b, resourceCount, permissionPerResource) + permissions, ids := setupTestEnv(resourceCount, permissionPerResource) b.ResetTimer() var metadata map[string]Metadata diff --git a/pkg/services/auth/identity/namespace.go b/pkg/services/auth/identity/namespace.go index bcceffd8c8b..81ae0daa664 100644 --- a/pkg/services/auth/identity/namespace.go +++ b/pkg/services/auth/identity/namespace.go @@ -6,26 +6,43 @@ import ( "strings" ) +type Namespace string + const ( - NamespaceUser = "user" - NamespaceAPIKey = "api-key" - NamespaceServiceAccount = "service-account" - NamespaceAnonymous = "anonymous" - NamespaceRenderService = "render" - NamespaceAccessPolicy = "access-policy" + NamespaceUser Namespace = "user" + NamespaceAPIKey Namespace = "api-key" + NamespaceServiceAccount Namespace = "service-account" + NamespaceAnonymous Namespace = "anonymous" + NamespaceRenderService Namespace = "render" + NamespaceAccessPolicy Namespace = "access-policy" + NamespaceEmpty Namespace = "" ) -var AnonymousNamespaceID = MustNewNamespaceID(NamespaceAnonymous, 0) - -var namespaceLookup = map[string]struct{}{ - NamespaceUser: {}, - NamespaceAPIKey: {}, - NamespaceServiceAccount: {}, - NamespaceAnonymous: {}, - NamespaceRenderService: {}, - NamespaceAccessPolicy: {}, +func (n Namespace) String() string { + return string(n) } +func ParseNamespace(str string) (Namespace, error) { + switch str { + case string(NamespaceUser): + return NamespaceUser, nil + case string(NamespaceAPIKey): + return NamespaceAPIKey, nil + case string(NamespaceServiceAccount): + return NamespaceServiceAccount, nil + case string(NamespaceAnonymous): + return NamespaceAnonymous, nil + case string(NamespaceRenderService): + return NamespaceRenderService, nil + case string(NamespaceAccessPolicy): + return NamespaceAccessPolicy, nil + default: + return "", ErrInvalidNamespaceID.Errorf("got invalid namespace %s", str) + } +} + +var AnonymousNamespaceID = MustNewNamespaceID(NamespaceAnonymous, 0) + func ParseNamespaceID(str string) (NamespaceID, error) { var namespaceID NamespaceID @@ -34,13 +51,12 @@ func ParseNamespaceID(str string) (NamespaceID, error) { return namespaceID, ErrInvalidNamespaceID.Errorf("expected namespace id to have 2 parts") } - namespace, id := parts[0], parts[1] - - if _, ok := namespaceLookup[namespace]; !ok { - return namespaceID, ErrInvalidNamespaceID.Errorf("got invalid namespace %s", namespace) + namespace, err := ParseNamespace(parts[0]) + if err != nil { + return namespaceID, err } - namespaceID.id = id + namespaceID.id = parts[1] namespaceID.namespace = namespace return namespaceID, nil @@ -57,19 +73,16 @@ func MustParseNamespaceID(str string) NamespaceID { } // NewNamespaceID creates a new NamespaceID, will fail for invalid namespace. -func NewNamespaceID(namespace string, id int64) (NamespaceID, error) { - var namespaceID NamespaceID - if _, ok := namespaceLookup[namespace]; !ok { - return namespaceID, ErrInvalidNamespaceID.Errorf("got invalid namespace %s", namespace) - } - namespaceID.id = strconv.FormatInt(id, 10) - namespaceID.namespace = namespace - return namespaceID, nil +func NewNamespaceID(namespace Namespace, id int64) (NamespaceID, error) { + return NamespaceID{ + id: strconv.FormatInt(id, 10), + namespace: namespace, + }, nil } // MustNewNamespaceID creates a new NamespaceID, will panic for invalid namespace. // Suitable to use in tests or when we can guarantee that we pass a correct format. -func MustNewNamespaceID(namespace string, id int64) NamespaceID { +func MustNewNamespaceID(namespace Namespace, id int64) NamespaceID { namespaceID, err := NewNamespaceID(namespace, id) if err != nil { panic(err) @@ -79,17 +92,25 @@ func MustNewNamespaceID(namespace string, id int64) NamespaceID { // NewNamespaceIDUnchecked creates a new NamespaceID without checking if namespace is valid. // It us up to the caller to ensure that namespace is valid. -func NewNamespaceIDUnchecked(namespace string, id int64) NamespaceID { +func NewNamespaceIDUnchecked(namespace Namespace, id int64) NamespaceID { return NamespaceID{ id: strconv.FormatInt(id, 10), namespace: namespace, } } +// NewNamespaceIDString creates a new NamespaceID with a string id +func NewNamespaceIDString(namespace Namespace, id string) NamespaceID { + return NamespaceID{ + id: id, + namespace: namespace, + } +} + // FIXME: use this instead of encoded string through the codebase type NamespaceID struct { id string - namespace string + namespace Namespace } func (ni NamespaceID) ID() string { @@ -100,11 +121,11 @@ func (ni NamespaceID) ParseInt() (int64, error) { return strconv.ParseInt(ni.id, 10, 64) } -func (ni NamespaceID) Namespace() string { +func (ni NamespaceID) Namespace() Namespace { return ni.namespace } -func (ni NamespaceID) IsNamespace(expected ...string) bool { +func (ni NamespaceID) IsNamespace(expected ...Namespace) bool { return IsNamespace(ni.namespace, expected...) } diff --git a/pkg/services/auth/identity/requester.go b/pkg/services/auth/identity/requester.go index 3be473ec0a3..1e257792081 100644 --- a/pkg/services/auth/identity/requester.go +++ b/pkg/services/auth/identity/requester.go @@ -12,7 +12,12 @@ type Requester interface { GetID() NamespaceID // GetNamespacedID returns the namespace and ID of the active entity. // The namespace is one of the constants defined in pkg/services/auth/identity. - GetNamespacedID() (namespace string, identifier string) + GetNamespacedID() (namespace Namespace, identifier string) + // GetID returns namespaced id for the entity + GetUID() NamespaceID + // GetNamespacedID returns the namespace and ID of the active entity. + // The namespace is one of the constants defined in pkg/services/auth/identity. + GetNamespacedUID() (namespace Namespace, identifier string) // GetDisplayName returns the display name of the active entity. // The display name is the name if it is set, otherwise the login or email. GetDisplayName() string @@ -65,7 +70,7 @@ type Requester interface { } // IsNamespace returns true if namespace matches any expected namespace -func IsNamespace(namespace string, expected ...string) bool { +func IsNamespace(namespace Namespace, expected ...Namespace) bool { for _, e := range expected { if namespace == e { return true @@ -78,7 +83,7 @@ func IsNamespace(namespace string, expected ...string) bool { // IntIdentifier converts a string identifier to an int64. // Applicable for users, service accounts, api keys and renderer service. // Errors if the identifier is not initialized or if namespace is not recognized. -func IntIdentifier(namespace, identifier string) (int64, error) { +func IntIdentifier(namespace Namespace, identifier string) (int64, error) { if IsNamespace(namespace, NamespaceUser, NamespaceAPIKey, NamespaceServiceAccount, NamespaceRenderService) { id, err := strconv.ParseInt(identifier, 10, 64) if err != nil { @@ -98,7 +103,7 @@ func IntIdentifier(namespace, identifier string) (int64, error) { // UserIdentifier converts a string identifier to an int64. // Errors if the identifier is not initialized or if namespace is not recognized. // Returns 0 if the namespace is not user or service account -func UserIdentifier(namespace, identifier string) (int64, error) { +func UserIdentifier(namespace Namespace, identifier string) (int64, error) { userID, err := IntIdentifier(namespace, identifier) if err != nil { // FIXME: return this error once entity namespaces are handled by stores diff --git a/pkg/services/auth/idimpl/service.go b/pkg/services/auth/idimpl/service.go index 8d01cb74bdd..d0307bda5e5 100644 --- a/pkg/services/auth/idimpl/service.go +++ b/pkg/services/auth/idimpl/service.go @@ -82,7 +82,7 @@ func (s *Service) SignIdentity(ctx context.Context, id identity.Requester) (stri Claims: jwt.Claims{ Issuer: s.cfg.AppURL, Audience: getAudience(id.GetOrgID()), - Subject: getSubject(namespace, identifier), + Subject: getSubject(namespace.String(), identifier), Expiry: jwt.NewNumericDate(now.Add(tokenTTL)), IssuedAt: jwt.NewNumericDate(now), }, diff --git a/pkg/services/authn/authnimpl/service.go b/pkg/services/authn/authnimpl/service.go index 425236a5ef6..38bbcca0f2f 100644 --- a/pkg/services/authn/authnimpl/service.go +++ b/pkg/services/authn/authnimpl/service.go @@ -373,7 +373,7 @@ func (s *Service) resolveIdenity(ctx context.Context, orgID int64, namespaceID a }}, nil } - resolver, ok := s.idenityResolverClients[namespaceID.Namespace()] + resolver, ok := s.idenityResolverClients[namespaceID.Namespace().String()] if !ok { return nil, authn.ErrUnsupportedIdentity.Errorf("no resolver for : %s", namespaceID.Namespace()) } diff --git a/pkg/services/authn/authnimpl/service_test.go b/pkg/services/authn/authnimpl/service_test.go index 9795d136343..01f82c6d134 100644 --- a/pkg/services/authn/authnimpl/service_test.go +++ b/pkg/services/authn/authnimpl/service_test.go @@ -514,7 +514,7 @@ func TestService_ResolveIdentity(t *testing.T) { t.Run("should resolve for valid namespace if client is registered", func(t *testing.T) { svc := setupTests(t, func(svc *Service) { svc.RegisterClient(&authntest.MockClient{ - NamespaceFunc: func() string { return authn.NamespaceAPIKey }, + NamespaceFunc: func() string { return authn.NamespaceAPIKey.String() }, ResolveIdentityFunc: func(ctx context.Context, orgID int64, namespaceID authn.NamespaceID) (*authn.Identity, error) { return &authn.Identity{}, nil }, diff --git a/pkg/services/authn/clients/api_key.go b/pkg/services/authn/clients/api_key.go index d01e33e5766..b9d2e746f8a 100644 --- a/pkg/services/authn/clients/api_key.go +++ b/pkg/services/authn/clients/api_key.go @@ -135,7 +135,7 @@ func (s *APIKey) Priority() uint { } func (s *APIKey) Namespace() string { - return authn.NamespaceAPIKey + return authn.NamespaceAPIKey.String() } func (s *APIKey) ResolveIdentity(ctx context.Context, orgID int64, namespaceID authn.NamespaceID) (*authn.Identity, error) { diff --git a/pkg/services/authn/identity.go b/pkg/services/authn/identity.go index 51fc542fae9..70381f02742 100644 --- a/pkg/services/authn/identity.go +++ b/pkg/services/authn/identity.go @@ -71,16 +71,27 @@ type Identity struct { // IDToken is a signed token representing the identity that can be forwarded to plugins and external services. // Will only be set when featuremgmt.FlagIdForwarding is enabled. IDToken string + // UserUID is the unique identifier for the entity in the Grafana database. + UserUID string } func (i *Identity) GetID() NamespaceID { return i.ID } -func (i *Identity) GetNamespacedID() (namespace string, identifier string) { +func (i *Identity) GetNamespacedID() (namespace identity.Namespace, identifier string) { return i.ID.Namespace(), i.ID.ID() } +func (i *Identity) GetUID() NamespaceID { + ns, uid := i.GetNamespacedUID() + return identity.NewNamespaceIDString(ns, uid) +} + +func (i *Identity) GetNamespacedUID() (namespace identity.Namespace, identifier string) { + return i.ID.Namespace(), i.UserUID +} + func (i *Identity) GetAuthID() string { return i.AuthID } diff --git a/pkg/services/contexthandler/contexthandler.go b/pkg/services/contexthandler/contexthandler.go index c0087534720..b0b10d213d8 100644 --- a/pkg/services/contexthandler/contexthandler.go +++ b/pkg/services/contexthandler/contexthandler.go @@ -158,7 +158,7 @@ func (h *ContextHandler) addIDHeaderEndOfRequestFunc(ident identity.Requester) w return } - if _, ok := h.Cfg.IDResponseHeaderNamespaces[namespace]; !ok { + if _, ok := h.Cfg.IDResponseHeaderNamespaces[namespace.String()]; !ok { return } diff --git a/pkg/services/dashboardimport/service/service.go b/pkg/services/dashboardimport/service/service.go index 00cd032496c..ebd98259f79 100644 --- a/pkg/services/dashboardimport/service/service.go +++ b/pkg/services/dashboardimport/service/service.go @@ -109,12 +109,11 @@ func (s *ImportDashboardService) ImportDashboard(ctx context.Context, req *dashb req.FolderUid = folder.UID } - namespaceID, identifier := req.User.GetNamespacedID() + namespace, identifier := req.User.GetNamespacedID() userID := int64(0) - switch namespaceID { - case identity.NamespaceUser, identity.NamespaceServiceAccount: - userID, _ = identity.IntIdentifier(namespaceID, identifier) + if namespace == identity.NamespaceUser || namespace == identity.NamespaceServiceAccount { + userID, _ = identity.IntIdentifier(namespace, identifier) } saveCmd := dashboards.SaveDashboardCommand{ diff --git a/pkg/services/dashboardsnapshots/database/database.go b/pkg/services/dashboardsnapshots/database/database.go index 0acd68838bf..f3e32c23934 100644 --- a/pkg/services/dashboardsnapshots/database/database.go +++ b/pkg/services/dashboardsnapshots/database/database.go @@ -137,8 +137,8 @@ func (d *DashboardSnapshotStore) SearchDashboardSnapshots(ctx context.Context, q namespace, id := query.SignedInUser.GetNamespacedID() var userID int64 - switch namespace { - case identity.NamespaceServiceAccount, identity.NamespaceUser: + + if namespace == identity.NamespaceServiceAccount || namespace == identity.NamespaceUser { var err error userID, err = identity.IntIdentifier(namespace, id) if err != nil { diff --git a/pkg/services/libraryelements/database.go b/pkg/services/libraryelements/database.go index 4f5932ec6b5..0700ab9dc3b 100644 --- a/pkg/services/libraryelements/database.go +++ b/pkg/services/libraryelements/database.go @@ -140,8 +140,7 @@ func (l *LibraryElementService) createLibraryElement(c context.Context, signedIn userID := int64(0) namespaceID, identifier := signedInUser.GetNamespacedID() - switch namespaceID { - case identity.NamespaceUser, identity.NamespaceServiceAccount: + if namespaceID == identity.NamespaceUser || namespaceID == identity.NamespaceServiceAccount { userID, err = identity.IntIdentifier(namespaceID, identifier) if err != nil { l.log.Warn("Error while parsing userID", "namespaceID", namespaceID, "userID", identifier) @@ -589,8 +588,7 @@ func (l *LibraryElementService) patchLibraryElement(c context.Context, signedInU var userID int64 namespaceID, identifier := signedInUser.GetNamespacedID() - switch namespaceID { - case identity.NamespaceUser, identity.NamespaceServiceAccount: + if namespaceID == identity.NamespaceUser || namespaceID == identity.NamespaceServiceAccount { var errID error userID, errID = identity.IntIdentifier(namespaceID, identifier) if errID != nil { @@ -798,8 +796,7 @@ func (l *LibraryElementService) connectElementsToDashboardID(c context.Context, namespaceID, identifier := signedInUser.GetNamespacedID() userID := int64(0) - switch namespaceID { - case identity.NamespaceUser, identity.NamespaceServiceAccount: + if namespaceID == identity.NamespaceUser || namespaceID == identity.NamespaceServiceAccount { userID, err = identity.IntIdentifier(namespaceID, identifier) if err != nil { l.log.Warn("Failed to parse user ID from namespace identifier", "namespace", namespaceID, "identifier", identifier, "error", err) diff --git a/pkg/services/live/features/dashboard.go b/pkg/services/live/features/dashboard.go index 4cc6bd39ffe..e5d178a9895 100644 --- a/pkg/services/live/features/dashboard.go +++ b/pkg/services/live/features/dashboard.go @@ -14,7 +14,6 @@ import ( "github.com/grafana/grafana/pkg/services/guardian" "github.com/grafana/grafana/pkg/services/live/model" "github.com/grafana/grafana/pkg/services/org" - "github.com/grafana/grafana/pkg/services/user" ) type actionType string @@ -28,11 +27,39 @@ const ( GitopsChannel = "grafana/dashboard/gitops" ) +type userDisplayDTO struct { + ID int64 `json:"id,omitempty"` + UID string `json:"uid,omitempty"` + Name string `json:"name,omitempty"` + Login string `json:"login,omitempty"` + AvatarURL string `json:"avatarUrl"` +} + +// Static function to parse a requester into a userDisplayDTO +func newUserDisplayDTOFromRequester(requester identity.Requester) *userDisplayDTO { + userID := int64(0) + namespaceID, identifier := requester.GetNamespacedID() + if namespaceID == identity.NamespaceUser || namespaceID == identity.NamespaceServiceAccount { + userID, _ = identity.IntIdentifier(namespaceID, identifier) + } + namespaceID, uid := requester.GetNamespacedUID() + if namespaceID != identity.NamespaceUser && namespaceID != identity.NamespaceServiceAccount { + uid = "" + } + + return &userDisplayDTO{ + ID: userID, + UID: uid, + Login: requester.GetLogin(), + Name: requester.GetDisplayName(), + } +} + // DashboardEvent events related to dashboards type dashboardEvent struct { UID string `json:"uid"` Action actionType `json:"action"` // saved, editing, deleted - User *user.UserDisplayDTO `json:"user,omitempty"` + User *userDisplayDTO `json:"user,omitempty"` SessionID string `json:"sessionId,omitempty"` Message string `json:"message,omitempty"` Dashboard *dashboards.Dashboard `json:"dashboard,omitempty"` @@ -142,10 +169,7 @@ func (h *DashboardHandler) OnPublish(ctx context.Context, requester identity.Req } // Tell everyone who is editing - event.User, err = user.NewUserDisplayDTOFromRequester(requester) - if err != nil { - return model.PublishReply{}, backend.PublishStreamStatusNotFound, err - } + event.User = newUserDisplayDTOFromRequester(requester) msg, err := json.Marshal(event) if err != nil { @@ -177,7 +201,7 @@ func (h *DashboardHandler) publish(orgID int64, event dashboardEvent) error { } // DashboardSaved will broadcast to all connected dashboards -func (h *DashboardHandler) DashboardSaved(orgID int64, user *user.UserDisplayDTO, message string, dashboard *dashboards.Dashboard, err error) error { +func (h *DashboardHandler) DashboardSaved(orgID int64, requester identity.Requester, message string, dashboard *dashboards.Dashboard, err error) error { if err != nil && !h.HasGitOpsObserver(orgID) { return nil // only broadcast if it was OK } @@ -185,7 +209,7 @@ func (h *DashboardHandler) DashboardSaved(orgID int64, user *user.UserDisplayDTO msg := dashboardEvent{ UID: dashboard.UID, Action: ActionSaved, - User: user, + User: newUserDisplayDTOFromRequester(requester), Message: message, Dashboard: dashboard, } @@ -198,11 +222,11 @@ func (h *DashboardHandler) DashboardSaved(orgID int64, user *user.UserDisplayDTO } // DashboardDeleted will broadcast to all connected dashboards -func (h *DashboardHandler) DashboardDeleted(orgID int64, user *user.UserDisplayDTO, uid string) error { +func (h *DashboardHandler) DashboardDeleted(orgID int64, requester identity.Requester, uid string) error { return h.publish(orgID, dashboardEvent{ UID: uid, Action: ActionDeleted, - User: user, + User: newUserDisplayDTOFromRequester(requester), }) } diff --git a/pkg/services/live/live.go b/pkg/services/live/live.go index f580caf6a3b..975613835ad 100644 --- a/pkg/services/live/live.go +++ b/pkg/services/live/live.go @@ -54,7 +54,6 @@ import ( "github.com/grafana/grafana/pkg/services/pluginsintegration/pluginstore" "github.com/grafana/grafana/pkg/services/query" "github.com/grafana/grafana/pkg/services/secrets" - "github.com/grafana/grafana/pkg/services/user" "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/util" "github.com/grafana/grafana/pkg/util/errutil" @@ -138,7 +137,7 @@ func ProvideService(plugCtxProvider *plugincontext.Provider, cfg *setting.Cfg, r var managedStreamRunner *managedstream.Runner var redisClient *redis.Client if g.IsHA() && redisHealthy { - redisClient := redis.NewClient(&redis.Options{ + redisClient = redis.NewClient(&redis.Options{ Addr: g.Cfg.LiveHAEngineAddress, Password: g.Cfg.LiveHAEnginePassword, }) @@ -414,10 +413,10 @@ type DashboardActivityChannel interface { // gitops workflow that knows if the value was saved to the local database or not // in many cases all direct save requests will fail, but the request should be forwarded // to any gitops observers - DashboardSaved(orgID int64, user *user.UserDisplayDTO, message string, dashboard *dashboards.Dashboard, err error) error + DashboardSaved(orgID int64, requester identity.Requester, message string, dashboard *dashboards.Dashboard, err error) error // Called when a dashboard is deleted - DashboardDeleted(orgID int64, user *user.UserDisplayDTO, uid string) error + DashboardDeleted(orgID int64, requester identity.Requester, uid string) error // Experimental! Indicate is GitOps is active. This really means // someone is subscribed to the `grafana/dashboards/gitops` channel diff --git a/pkg/services/sqlstore/permissions/dashboard.go b/pkg/services/sqlstore/permissions/dashboard.go index 293a19b6bcf..7efaf7a17b8 100644 --- a/pkg/services/sqlstore/permissions/dashboard.go +++ b/pkg/services/sqlstore/permissions/dashboard.go @@ -136,10 +136,9 @@ func (f *accessControlDashboardPermissionFilter) buildClauses() { folderWildcards := accesscontrol.WildcardsFromPrefix(dashboards.ScopeFoldersPrefix) userID := int64(0) - namespaceID, identifier := f.user.GetNamespacedID() - switch namespaceID { - case identity.NamespaceUser, identity.NamespaceServiceAccount: - userID, _ = identity.IntIdentifier(namespaceID, identifier) + namespace, identifier := f.user.GetNamespacedID() + if namespace == identity.NamespaceUser || namespace == identity.NamespaceServiceAccount { + userID, _ = identity.IntIdentifier(namespace, identifier) } orgID := f.user.GetOrgID() diff --git a/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go b/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go index 120b432a57e..ac993bd4c9d 100644 --- a/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go +++ b/pkg/services/sqlstore/permissions/dashboard_filter_no_subquery.go @@ -34,10 +34,9 @@ func (f *accessControlDashboardPermissionFilterNoFolderSubquery) buildClauses() folderWildcards := accesscontrol.WildcardsFromPrefix(dashboards.ScopeFoldersPrefix) userID := int64(0) - namespaceID, identifier := f.user.GetNamespacedID() - switch namespaceID { - case identity.NamespaceUser, identity.NamespaceServiceAccount: - userID, _ = identity.IntIdentifier(namespaceID, identifier) + namespace, identifier := f.user.GetNamespacedID() + if namespace == identity.NamespaceUser || namespace == identity.NamespaceServiceAccount { + userID, _ = identity.IntIdentifier(namespace, identifier) } orgID := f.user.GetOrgID() diff --git a/pkg/services/user/identity.go b/pkg/services/user/identity.go index 5cd3ffe2622..45f8ed31b5a 100644 --- a/pkg/services/user/identity.go +++ b/pkg/services/user/identity.go @@ -2,6 +2,7 @@ package user import ( "fmt" + "strconv" "time" "github.com/grafana/grafana/pkg/models/roletype" @@ -56,36 +57,6 @@ func (u *SignedInUser) NameOrFallback() string { return u.Email } -// TODO: There's a need to remove this struct since it creates a circular dependency - -// DEPRECATED: This function uses `UserDisplayDTO` model which we want to remove -// In order to retrieve the user URL, we need the dto library. However, adding -// the dto library to the user service creates a circular dependency -func (u *SignedInUser) ToUserDisplayDTO() *UserDisplayDTO { - return &UserDisplayDTO{ - ID: u.UserID, - UID: u.UserUID, - Login: u.Login, - Name: u.Name, - // AvatarURL: dtos.GetGravatarUrl(u.GetEmail()), - } -} - -// Static function to parse a requester into a UserDisplayDTO -func NewUserDisplayDTOFromRequester(requester identity.Requester) (*UserDisplayDTO, error) { - userID := int64(0) - namespaceID, identifier := requester.GetNamespacedID() - if namespaceID == identity.NamespaceUser || namespaceID == identity.NamespaceServiceAccount { - userID, _ = identity.IntIdentifier(namespaceID, identifier) - } - - return &UserDisplayDTO{ - ID: userID, - Login: requester.GetLogin(), - Name: requester.GetDisplayName(), - }, nil -} - func (u *SignedInUser) HasRole(role roletype.RoleType) bool { if u.IsGrafanaAdmin { return true @@ -196,27 +167,52 @@ func (u *SignedInUser) GetOrgRole() roletype.RoleType { // GetID returns namespaced id for the entity func (u *SignedInUser) GetID() identity.NamespaceID { - switch { - case u.ApiKeyID != 0: - return namespacedID(identity.NamespaceAPIKey, u.ApiKeyID) - case u.IsServiceAccount: - return namespacedID(identity.NamespaceServiceAccount, u.UserID) - case u.UserID > 0: - return namespacedID(identity.NamespaceUser, u.UserID) - case u.IsAnonymous: - return namespacedID(identity.NamespaceAnonymous, 0) - case u.AuthenticatedBy == "render" && u.UserID == 0: - return namespacedID(identity.NamespaceRenderService, 0) - } - - return u.NamespacedID + ns, id := u.GetNamespacedID() + return identity.NewNamespaceIDString(ns, id) } // GetNamespacedID returns the namespace and ID of the active entity // The namespace is one of the constants defined in pkg/services/auth/identity -func (u *SignedInUser) GetNamespacedID() (string, string) { - id := u.GetID() - return id.Namespace(), id.ID() +func (u *SignedInUser) GetNamespacedID() (identity.Namespace, string) { + switch { + case u.ApiKeyID != 0: + return identity.NamespaceAPIKey, strconv.FormatInt(u.ApiKeyID, 10) + case u.IsServiceAccount: + return identity.NamespaceServiceAccount, strconv.FormatInt(u.UserID, 10) + case u.UserID > 0: + return identity.NamespaceUser, strconv.FormatInt(u.UserID, 10) + case u.IsAnonymous: + return identity.NamespaceAnonymous, "0" + case u.AuthenticatedBy == "render" && u.UserID == 0: + return identity.NamespaceRenderService, "0" + } + + return u.NamespacedID.Namespace(), u.NamespacedID.ID() +} + +// GetUID returns namespaced uid for the entity +func (u *SignedInUser) GetUID() identity.NamespaceID { + ns, uid := u.GetNamespacedUID() + return identity.NewNamespaceIDString(ns, uid) +} + +// GetNamespacedUID returns the namespace and UID of the active entity +// The namespace is one of the constants defined in pkg/services/auth/identity +func (u *SignedInUser) GetNamespacedUID() (identity.Namespace, string) { + switch { + case u.ApiKeyID != 0: + return identity.NamespaceAPIKey, fmt.Sprint(u.ApiKeyID) + case u.IsServiceAccount: + return identity.NamespaceServiceAccount, u.UserUID + case u.UserID > 0: + return identity.NamespaceUser, u.UserUID + case u.IsAnonymous: + return identity.NamespaceAnonymous, "" + case u.AuthenticatedBy == "render" && u.UserID == 0: + return identity.NamespaceRenderService, "" + } + + return identity.NamespaceEmpty, "" } func (u *SignedInUser) GetAuthID() string { @@ -260,7 +256,3 @@ func (u *SignedInUser) GetDisplayName() string { func (u *SignedInUser) GetIDToken() string { return u.IDToken } - -func namespacedID(namespace string, id int64) identity.NamespaceID { - return identity.NewNamespaceIDUnchecked(namespace, id) -} diff --git a/pkg/services/user/model.go b/pkg/services/user/model.go index 73276afc8b6..70f5a28f584 100644 --- a/pkg/services/user/model.go +++ b/pkg/services/user/model.go @@ -223,14 +223,6 @@ type ErrCaseInsensitiveLoginConflict struct { Users []User } -type UserDisplayDTO struct { - ID int64 `json:"id,omitempty"` - UID string `json:"uid,omitempty"` - Name string `json:"name,omitempty"` - Login string `json:"login,omitempty"` - AvatarURL string `json:"avatarUrl"` -} - func (e *ErrCaseInsensitiveLoginConflict) Unwrap() error { return ErrCaseInsensitive } diff --git a/pkg/util/proxyutil/proxyutil.go b/pkg/util/proxyutil/proxyutil.go index 0677fa5a396..3d4cbcdf245 100644 --- a/pkg/util/proxyutil/proxyutil.go +++ b/pkg/util/proxyutil/proxyutil.go @@ -115,8 +115,7 @@ func ApplyUserHeader(sendUserHeader bool, req *http.Request, user identity.Reque } namespace, _ := user.GetNamespacedID() - switch namespace { - case identity.NamespaceUser, identity.NamespaceServiceAccount: + if namespace == identity.NamespaceUser || namespace == identity.NamespaceServiceAccount { req.Header.Set(UserHeaderName, user.GetLogin()) } }