From 82bcfba64b1adeb70791969a1faa595389cb1d33 Mon Sep 17 00:00:00 2001 From: Ryan McKinley Date: Fri, 26 Sep 2025 22:44:02 +0300 Subject: [PATCH] Preferences: refactor experimental apiserver and improve tests (#111596) --- apps/preferences/kinds/preferences.cue | 8 +- .../v1alpha1/preferences_client_gen.go | 4 +- .../preferences/v1alpha1/stars_client_gen.go | 4 +- .../pkg/apis/preferences_manifest.go | 23 ++ pkg/apimachinery/identity/context.go | 1 + pkg/registry/apis/preferences/authorizer.go | 137 ++++--- .../apis/preferences/authorizer_test.go | 351 ++++++++++++++++++ .../apis/preferences/legacy/preferences.go | 28 +- pkg/registry/apis/preferences/legacy/sql.go | 22 +- pkg/registry/apis/preferences/legacy/stars.go | 19 +- .../{current.go => merged_preferences.go} | 27 +- ...ent_test.go => merged_preferences_test.go} | 0 pkg/registry/apis/preferences/register.go | 70 ++-- .../preferences/{stars.go => update_stars.go} | 4 +- .../{stars_test.go => update_stars_test.go} | 0 pkg/registry/apis/preferences/utils/names.go | 12 +- .../apis/preferences/utils/names_test.go | 4 +- pkg/registry/apis/preferences/utils/teams.go | 13 + .../apis/preferences/utils/teams_mock.go | 156 ++++++++ .../preferences.grafana.app-v1alpha1.json | 62 ++-- .../apis/preferences/preferences_test.go | 14 +- pkg/tests/apis/preferences/stars_test.go | 259 +++++++------ .../preferences/v1alpha1/endpoints.gen.ts | 18 +- 23 files changed, 946 insertions(+), 290 deletions(-) create mode 100644 pkg/registry/apis/preferences/authorizer_test.go rename pkg/registry/apis/preferences/{current.go => merged_preferences.go} (83%) rename pkg/registry/apis/preferences/{current_test.go => merged_preferences_test.go} (100%) rename pkg/registry/apis/preferences/{stars.go => update_stars.go} (97%) rename pkg/registry/apis/preferences/{stars_test.go => update_stars_test.go} (100%) create mode 100644 pkg/registry/apis/preferences/utils/teams.go create mode 100644 pkg/registry/apis/preferences/utils/teams_mock.go diff --git a/apps/preferences/kinds/preferences.cue b/apps/preferences/kinds/preferences.cue index 6c39f5ef7ec..7af54e89d7b 100644 --- a/apps/preferences/kinds/preferences.cue +++ b/apps/preferences/kinds/preferences.cue @@ -40,21 +40,21 @@ preferencesV1alpha1: { // Navigation preferences navbar?: #NavbarPreference - } @cuetsy(kind="interface") + } #QueryHistoryPreference: { // one of: '' | 'query' | 'starred'; homeTab?: string - } @cuetsy(kind="interface") + } #CookiePreferences: { analytics?: {} performance?: {} functional?: {} - } @cuetsy(kind="interface") + } #NavbarPreference: { bookmarkUrls: [...string] - } @cuetsy(kind="interface") + } } } diff --git a/apps/preferences/pkg/apis/preferences/v1alpha1/preferences_client_gen.go b/apps/preferences/pkg/apis/preferences/v1alpha1/preferences_client_gen.go index 877d49ceede..380bafb16bf 100644 --- a/apps/preferences/pkg/apis/preferences/v1alpha1/preferences_client_gen.go +++ b/apps/preferences/pkg/apis/preferences/v1alpha1/preferences_client_gen.go @@ -76,7 +76,7 @@ func (c *PreferencesClient) Patch(ctx context.Context, identifier resource.Ident return c.client.Patch(ctx, identifier, req, opts) } -func (c *PreferencesClient) UpdateStatus(ctx context.Context, newStatus PreferencesStatus, opts resource.UpdateOptions) (*Preferences, error) { +func (c *PreferencesClient) UpdateStatus(ctx context.Context, identifier resource.Identifier, newStatus PreferencesStatus, opts resource.UpdateOptions) (*Preferences, error) { return c.client.Update(ctx, &Preferences{ TypeMeta: metav1.TypeMeta{ Kind: PreferencesKind().Kind(), @@ -84,6 +84,8 @@ func (c *PreferencesClient) UpdateStatus(ctx context.Context, newStatus Preferen }, ObjectMeta: metav1.ObjectMeta{ ResourceVersion: opts.ResourceVersion, + Namespace: identifier.Namespace, + Name: identifier.Name, }, Status: newStatus, }, resource.UpdateOptions{ diff --git a/apps/preferences/pkg/apis/preferences/v1alpha1/stars_client_gen.go b/apps/preferences/pkg/apis/preferences/v1alpha1/stars_client_gen.go index c4badb4387c..3a607012db3 100644 --- a/apps/preferences/pkg/apis/preferences/v1alpha1/stars_client_gen.go +++ b/apps/preferences/pkg/apis/preferences/v1alpha1/stars_client_gen.go @@ -76,7 +76,7 @@ func (c *StarsClient) Patch(ctx context.Context, identifier resource.Identifier, return c.client.Patch(ctx, identifier, req, opts) } -func (c *StarsClient) UpdateStatus(ctx context.Context, newStatus StarsStatus, opts resource.UpdateOptions) (*Stars, error) { +func (c *StarsClient) UpdateStatus(ctx context.Context, identifier resource.Identifier, newStatus StarsStatus, opts resource.UpdateOptions) (*Stars, error) { return c.client.Update(ctx, &Stars{ TypeMeta: metav1.TypeMeta{ Kind: StarsKind().Kind(), @@ -84,6 +84,8 @@ func (c *StarsClient) UpdateStatus(ctx context.Context, newStatus StarsStatus, o }, ObjectMeta: metav1.ObjectMeta{ ResourceVersion: opts.ResourceVersion, + Namespace: identifier.Namespace, + Name: identifier.Name, }, Status: newStatus, }, resource.UpdateOptions{ diff --git a/apps/preferences/pkg/apis/preferences_manifest.go b/apps/preferences/pkg/apis/preferences_manifest.go index f8b8d05bc09..b0a3468058a 100644 --- a/apps/preferences/pkg/apis/preferences_manifest.go +++ b/apps/preferences/pkg/apis/preferences_manifest.go @@ -13,6 +13,7 @@ import ( "github.com/grafana/grafana-app-sdk/app" "github.com/grafana/grafana-app-sdk/resource" "k8s.io/apimachinery/pkg/runtime" + "k8s.io/kube-openapi/pkg/spec3" v1alpha1 "github.com/grafana/grafana/apps/preferences/pkg/apis/preferences/v1alpha1" ) @@ -66,6 +67,10 @@ var appManifestData = app.ManifestData{ Schema: &versionSchemaStarsv1alpha1, }, }, + Routes: app.ManifestVersionRoutes{ + Namespaced: map[string]spec3.PathProps{}, + Cluster: map[string]spec3.PathProps{}, + }, }, }, } @@ -95,6 +100,7 @@ var customRouteToGoResponseType = map[string]any{} // ManifestCustomRouteResponsesAssociator returns the associated response go type for a given kind, version, custom route path, and method, if one exists. // kind may be empty for custom routes which are not kind subroutes. Leading slashes are removed from subroute paths. // If there is no association for the provided kind, version, custom route path, and method, exists will return false. +// Resource routes (those without a kind) should prefix their route with "/" if the route is namespaced (otherwise the route is assumed to be cluster-scope) func ManifestCustomRouteResponsesAssociator(kind, version, path, verb string) (goType any, exists bool) { if len(path) > 0 && path[0] == '/' { path = path[1:] @@ -113,8 +119,22 @@ func ManifestCustomRouteQueryAssociator(kind, version, path, verb string) (goTyp return goType, exists } +var customRouteToGoRequestBodyType = map[string]any{} + +func ManifestCustomRouteRequestBodyAssociator(kind, version, path, verb string) (goType any, exists bool) { + if len(path) > 0 && path[0] == '/' { + path = path[1:] + } + goType, exists = customRouteToGoRequestBodyType[fmt.Sprintf("%s|%s|%s|%s", version, kind, path, strings.ToUpper(verb))] + return goType, exists +} + type GoTypeAssociator struct{} +func NewGoTypeAssociator() *GoTypeAssociator { + return &GoTypeAssociator{} +} + func (g *GoTypeAssociator) KindToGoType(kind, version string) (goType resource.Kind, exists bool) { return ManifestGoTypeAssociator(kind, version) } @@ -124,3 +144,6 @@ func (g *GoTypeAssociator) CustomRouteReturnGoType(kind, version, path, verb str func (g *GoTypeAssociator) CustomRouteQueryGoType(kind, version, path, verb string) (goType runtime.Object, exists bool) { return ManifestCustomRouteQueryAssociator(kind, version, path, verb) } +func (g *GoTypeAssociator) CustomRouteRequestBodyGoType(kind, version, path, verb string) (goType any, exists bool) { + return ManifestCustomRouteRequestBodyAssociator(kind, version, path, verb) +} diff --git a/pkg/apimachinery/identity/context.go b/pkg/apimachinery/identity/context.go index f98738db58d..664f519dd8f 100644 --- a/pkg/apimachinery/identity/context.go +++ b/pkg/apimachinery/identity/context.go @@ -139,6 +139,7 @@ var serviceIdentityTokenPermissions = []string{ "secret.grafana.app:*", "query.grafana.app:*", "iam.grafana.app:*", + "preferences.grafana.app:*", // Secrets Manager uses a custom verb for secret decryption, and its authorizer does not allow wildcard permissions. "secret.grafana.app/securevalues:decrypt", diff --git a/pkg/registry/apis/preferences/authorizer.go b/pkg/registry/apis/preferences/authorizer.go index a09628f2b76..8eae3c4248f 100644 --- a/pkg/registry/apis/preferences/authorizer.go +++ b/pkg/registry/apis/preferences/authorizer.go @@ -6,55 +6,98 @@ import ( "k8s.io/apiserver/pkg/authorization/authorizer" + "github.com/grafana/authlib/authz" + "github.com/grafana/grafana-app-sdk/logging" "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/registry/apis/preferences/utils" ) -func (b *APIBuilder) GetAuthorizer() authorizer.Authorizer { - return authorizer.AuthorizerFunc( - func(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) { - user, err := identity.GetRequester(ctx) - if err != nil { - return authorizer.DecisionDeny, "valid user is required", err - } - - if !attr.IsResourceRequest() || user.GetIsGrafanaAdmin() || attr.GetName() == "" { - return authorizer.DecisionAllow, "", nil - } - - name, found := utils.ParseOwnerFromName(attr.GetName()) - if !found { - return authorizer.DecisionDeny, "invalid name", nil - } - - if attr.GetResource() == "stars" && name.Owner != utils.UserResourceOwner { - return authorizer.DecisionDeny, "stars only support users", nil - } - - switch name.Owner { - case utils.NamespaceResourceOwner: - return authorizer.DecisionAllow, "", nil - - case utils.UserResourceOwner: - if user.GetUID() == name.Name { - return authorizer.DecisionAllow, "", nil - } - return authorizer.DecisionDeny, "you may only fetch your own preferences", nil - - case utils.TeamResourceOwner: - admin := !attr.IsReadOnly() // we need admin to for non read only commands - teams, err := b.sql.GetTeams(ctx, user.GetOrgID(), user.GetUID(), admin) - if err != nil { - return authorizer.DecisionDeny, "error fetching teams", err - } - if slices.Contains(teams, name.Name) { - return authorizer.DecisionAllow, "", nil - } - return authorizer.DecisionDeny, "not a team member", nil - - default: - } - - return authorizer.DecisionDeny, "invalid name", nil - }) +type authorizeFromName struct { + teams utils.TeamService + oknames []string + resource map[string][]utils.ResourceOwner // may include unknown +} + +func (a *authorizeFromName) Authorize(ctx context.Context, attr authorizer.Attributes) (authorizer.Decision, string, error) { + user, err := identity.GetRequester(ctx) + if err != nil || user == nil { + return authorizer.DecisionDeny, "valid user is required", err + } + + if !attr.IsResourceRequest() { + return authorizer.DecisionNoOpinion, "", nil + } + + owners, ok := a.resource[attr.GetResource()] + if !ok { + return authorizer.DecisionDeny, "missing resource name", nil + } + + // Check if the request includes explicit permissions + res := authz.CheckServicePermissions(user, attr.GetAPIGroup(), attr.GetResource(), attr.GetVerb()) + if !res.Allowed { + log := logging.FromContext(ctx) + log.Info("calling service lacks required permissions", + "isServiceCall", res.ServiceCall, + "apiGroup", attr.GetAPIGroup(), + "resource", attr.GetResource(), + "verb", attr.GetVerb(), + "permissions", len(res.Permissions), + ) + return authorizer.DecisionDeny, "calling service lacks required permissions", nil + } + + if attr.GetName() == "" { + if attr.IsReadOnly() { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "mutating request without a name", nil + } + + // the pseudo sub-resource + if a.oknames != nil && slices.Contains(a.oknames, attr.GetName()) { + return authorizer.DecisionAllow, "", nil + } + + info, _ := utils.ParseOwnerFromName(attr.GetName()) + if !slices.Contains(owners, info.Owner) { + return authorizer.DecisionDeny, "unsupported owner type", nil + } + + switch info.Owner { + case utils.NamespaceResourceOwner: + if attr.IsReadOnly() { + // Everyone can see the namespace + return authorizer.DecisionAllow, "", nil + } + if user.GetOrgRole() == identity.RoleAdmin { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "must be an org admin to edit", nil + + case utils.UserResourceOwner: + if user.GetIdentifier() == info.Identifier { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "your are not the owner of the resource", nil + + case utils.TeamResourceOwner: + if a.teams == nil { + return authorizer.DecisionDeny, "team checker not configured", err + } + ok, err := a.teams.InTeam(ctx, user, info.Identifier, !attr.IsReadOnly()) + if err != nil { + return authorizer.DecisionDeny, "error fetching teams", err + } + if ok { + return authorizer.DecisionAllow, "", nil + } + return authorizer.DecisionDeny, "you are not a member of the referenced team", nil + + case utils.UnknownResourceOwner: + return authorizer.DecisionAllow, "", nil + } + + // the owner was not explicitly allowed + return authorizer.DecisionDeny, "", nil } diff --git a/pkg/registry/apis/preferences/authorizer_test.go b/pkg/registry/apis/preferences/authorizer_test.go new file mode 100644 index 00000000000..2b060bf6712 --- /dev/null +++ b/pkg/registry/apis/preferences/authorizer_test.go @@ -0,0 +1,351 @@ +package preferences + +import ( + "context" + "fmt" + "testing" + + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + "k8s.io/apiserver/pkg/authorization/authorizer" + + "github.com/grafana/authlib/authn" + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/grafana/grafana/pkg/registry/apis/preferences/utils" +) + +type expect struct { + decision authorizer.Decision + reason string + err string +} + +type testCase struct { + name string + user identity.Requester + attrs authorizer.Attributes + expect expect + breakpoint bool +} + +func TestAuthorizer_Authorize(t *testing.T) { + userABC := &identity.StaticRequester{ + UserUID: "abc", + OrgRole: identity.RoleViewer, + AccessTokenClaims: &authn.Claims[authn.AccessTokenClaims]{ + Rest: authn.AccessTokenClaims{ + DelegatedPermissions: []string{"group/stars:*", "group/preferences:*", "group/ns:*"}, + }, + }, + } + + tests := []struct { + name string + teams func(t *testing.T) utils.TeamService + resource map[string][]utils.ResourceOwner + check []testCase + }{ + { + name: "stars", + resource: map[string][]utils.ResourceOwner{ + "stars": {utils.UserResourceOwner}, + }, + check: []testCase{{ + name: "matches user", + user: userABC, + attrs: authorizer.AttributesRecord{ + Verb: "get", + APIGroup: "group", + Resource: "stars", + Name: "user-abc", // note this matches in input user name + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionAllow, + }, + }, { + name: "different user", + user: userABC, + attrs: authorizer.AttributesRecord{ + Verb: "get", + APIGroup: "group", + Resource: "stars", + Name: "user-xyz", // not abc + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "your are not the owner of the resource", + }, + }}, + }, { + name: "fast path", + resource: map[string][]utils.ResourceOwner{ + "stars": {utils.UserResourceOwner}, + "preferences": {utils.TeamResourceOwner}, + }, + check: []testCase{{ + name: "missing user", + attrs: authorizer.AttributesRecord{}, + expect: expect{ + decision: authorizer.DecisionDeny, + err: "a Requester was not found in the context", + }, + }, { + name: "not a resource", + user: &identity.StaticRequester{}, + attrs: authorizer.AttributesRecord{ + ResourceRequest: false, + }, + expect: expect{ + decision: authorizer.DecisionNoOpinion, + }, + }, { + name: "unknown resource", + user: &identity.StaticRequester{}, + attrs: authorizer.AttributesRecord{ + Resource: "xxxx", + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "missing resource name", + }, + }, { + name: "missing service permissions", + user: &identity.StaticRequester{ + UserUID: "abc", + AccessTokenClaims: &authn.Claims[authn.AccessTokenClaims]{ + Rest: authn.AccessTokenClaims{ + DelegatedPermissions: []string{""}, + }, + }, + }, + attrs: authorizer.AttributesRecord{ + Resource: "stars", + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "calling service lacks required permissions", + }, + }, { + name: "wrong owner type", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "stars", + ResourceRequest: true, + Verb: "create", // missing name + Name: "team-xxx", // not supported + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "unsupported owner type", + }, + }, { + name: "unknown resource", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "stars", + ResourceRequest: true, + Verb: "create", // missing name + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "mutating request without a name", + }, + }, { + name: "list request", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "stars", + ResourceRequest: true, + Verb: "list", // no name + }, + expect: expect{ + decision: authorizer.DecisionAllow, + }, + }, { + name: "teams request (but not configured)", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "preferences", + ResourceRequest: true, + Verb: "get", + Name: "team-XYZ", + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "team checker not configured", + }, + }}, + }, { + name: "unknown owner", + resource: map[string][]utils.ResourceOwner{ + "stars": {utils.UnknownResourceOwner}, + }, + check: []testCase{{ + name: "get", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "stars", + Name: "something-not-an-owner", + ResourceRequest: true, + Verb: "get", + }, + expect: expect{ + decision: authorizer.DecisionAllow, + }, + }}, + }, { + name: "namespace", + resource: map[string][]utils.ResourceOwner{ + "ns": {utils.NamespaceResourceOwner}, + }, + check: []testCase{{ + name: "readonly", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "ns", + ResourceRequest: true, + Verb: "get", + Name: "namespace", + }, + expect: expect{ + decision: authorizer.DecisionAllow, + }, + }, { + name: "mutating", + user: userABC, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "ns", + ResourceRequest: true, + Verb: "create", + Name: "namespace", + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "must be an org admin to edit", + }, + }, { + name: "org admin", + user: &identity.StaticRequester{ + UserUID: "abc", + OrgRole: identity.RoleAdmin, + AccessTokenClaims: &authn.Claims[authn.AccessTokenClaims]{ + Rest: authn.AccessTokenClaims{ + DelegatedPermissions: []string{"group/ns:create"}, + }, + }, + }, + attrs: authorizer.AttributesRecord{ + APIGroup: "group", + Resource: "ns", + ResourceRequest: true, + Verb: "create", + Name: "namespace", + }, + expect: expect{ + decision: authorizer.DecisionAllow, + }, + }}, + }, { + name: "preferences teams", + teams: func(t *testing.T) utils.TeamService { + teams := utils.NewMockTeamService(t) + teams.On("InTeam", mock.Anything, userABC, "xyz", false).Return(true, nil) + teams.On("InTeam", mock.Anything, userABC, "456", false).Return(false, nil) + teams.On("InTeam", mock.Anything, userABC, "XXX", false).Return(true, fmt.Errorf("error from team")) + return teams + }, + resource: map[string][]utils.ResourceOwner{ + "preferences": { + utils.TeamResourceOwner, + }, + }, + check: []testCase{{ + name: "user in team", + user: userABC, + attrs: authorizer.AttributesRecord{ + Verb: "get", + APIGroup: "group", + Resource: "preferences", + Name: "team-xyz", + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionAllow, + }, + }, { + name: "user not in team", + user: userABC, + attrs: authorizer.AttributesRecord{ + Verb: "get", + APIGroup: "group", + Resource: "preferences", + Name: "team-456", + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "you are not a member of the referenced team", + }, + }, { + name: "team error", + user: userABC, + attrs: authorizer.AttributesRecord{ + Verb: "get", + APIGroup: "group", + Resource: "preferences", + Name: "team-XXX", + ResourceRequest: true, + }, + expect: expect{ + decision: authorizer.DecisionDeny, + reason: "error fetching teams", + err: "error from team", + }, + }}, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + authz := &authorizeFromName{ + resource: tt.resource, + } + if tt.teams != nil { + authz.teams = tt.teams(t) + } + for _, check := range tt.check { + t.Run(check.name, func(t *testing.T) { + ctx := context.Background() + if check.user != nil { + ctx = identity.WithRequester(ctx, check.user) + } + e := check.expect + if check.breakpoint { + require.True(t, true) // Can set breakpoint in IDE here + } + d, r, err := authz.Authorize(ctx, check.attrs) + if e.err != "" { + require.ErrorContains(t, err, e.err) + return + } + require.NoError(t, err) + require.Equal(t, e.decision, d) + if e.reason != "" { + require.Equal(t, e.reason, r) + } + }) + } + }) + } +} diff --git a/pkg/registry/apis/preferences/legacy/preferences.go b/pkg/registry/apis/preferences/legacy/preferences.go index 39ff2fa4707..e9fd6b32aed 100644 --- a/pkg/registry/apis/preferences/legacy/preferences.go +++ b/pkg/registry/apis/preferences/legacy/preferences.go @@ -3,7 +3,6 @@ package legacy import ( "context" "fmt" - "slices" "strconv" "strings" "time" @@ -85,34 +84,19 @@ func (s *preferenceStorage) Get(ctx context.Context, name string, options *metav if err != nil { return nil, err } - user, err := identity.GetRequester(ctx) - if err != nil { - return nil, err - } owner, ok := utils.ParseOwnerFromName(name) if !ok { return nil, preferences.PreferencesResourceInfo.NewNotFound(name) } + // NOTE: the authorizer already checked if this request is allowed found, _, err := s.sql.listPreferences(ctx, ns.Value, ns.OrgID, func(req *preferencesQuery) (bool, error) { switch owner.Owner { case utils.UserResourceOwner: - if !user.GetIsGrafanaAdmin() && name != user.GetUID() { - return false, fmt.Errorf("you may only fetch your own preferences") - } - req.UserUID = owner.Name + req.UserUID = owner.Identifier return false, nil case utils.TeamResourceOwner: - if !user.GetIsGrafanaAdmin() { - teams, err := s.sql.GetTeams(ctx, ns.OrgID, user.GetRawIdentifier(), false) - if err != nil { - return false, err - } - if !slices.Contains(teams, owner.Name) { - return false, fmt.Errorf("you may only fetch teams you belong to") - } - } - req.TeamUID = owner.Name + req.TeamUID = owner.Identifier return false, nil case utils.NamespaceResourceOwner: return false, nil @@ -136,13 +120,13 @@ func asPreferencesResource(ns string, p *preferenceModel) preferences.Preference owner := utils.OwnerReference{} if p.TeamUID.Valid { owner.Owner = utils.TeamResourceOwner - owner.Name = p.TeamUID.String + owner.Identifier = p.TeamUID.String } else if p.UserUID.Valid { owner.Owner = utils.UserResourceOwner - owner.Name = p.UserUID.String + owner.Identifier = p.UserUID.String } else { owner.Owner = utils.NamespaceResourceOwner - owner.Name = "" + owner.Identifier = "" } obj := preferences.Preferences{ ObjectMeta: metav1.ObjectMeta{ diff --git a/pkg/registry/apis/preferences/legacy/sql.go b/pkg/registry/apis/preferences/legacy/sql.go index 15419db329e..bf3e8900f81 100644 --- a/pkg/registry/apis/preferences/legacy/sql.go +++ b/pkg/registry/apis/preferences/legacy/sql.go @@ -230,7 +230,10 @@ func (s *LegacySQL) ListPreferences(ctx context.Context, ns string, user identit func(req *preferencesQuery) (bool, error) { if user != nil { req.UserUID = user.GetRawIdentifier() - teams, err = s.GetTeams(ctx, info.OrgID, req.UserUID, false) + teams, err = s.GetTeams(ctx, &identity.StaticRequester{ + OrgID: info.OrgID, + UserUID: req.UserUID, + }, false) req.UserTeams = teams } return needsRV, err @@ -260,13 +263,26 @@ func (s *LegacySQL) ListPreferences(ctx context.Context, ns string, user identit return list, nil } -func (s *LegacySQL) GetTeams(ctx context.Context, orgId int64, user string, admin bool) ([]string, error) { +func (s *LegacySQL) InTeam(ctx context.Context, id authlib.AuthInfo, team string, admin bool) (bool, error) { + // Could be faster, but find for now + teams, err := s.GetTeams(ctx, id, admin) + if err != nil { + return false, err + } + return slices.Contains(teams, team), nil +} + +func (s *LegacySQL) GetTeams(ctx context.Context, id authlib.AuthInfo, admin bool) ([]string, error) { sql, err := s.db(ctx) if err != nil { return nil, err } - req := newTeamsQueryReq(sql, orgId, user, admin) + xid, ok := id.(identity.Requester) + if !ok { + return nil, fmt.Errorf("expected identity.Requester") + } + req := newTeamsQueryReq(sql, xid.GetOrgID(), id.GetUID(), admin) q, err := sqltemplate.Execute(sqlTeams, req) if err != nil { diff --git a/pkg/registry/apis/preferences/legacy/stars.go b/pkg/registry/apis/preferences/legacy/stars.go index 26b6a5d574a..87f5300799d 100644 --- a/pkg/registry/apis/preferences/legacy/stars.go +++ b/pkg/registry/apis/preferences/legacy/stars.go @@ -18,6 +18,7 @@ import ( authlib "github.com/grafana/authlib/types" dashboardsV1 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1beta1" preferences "github.com/grafana/grafana/apps/preferences/pkg/apis/preferences/v1alpha1" + "github.com/grafana/grafana/pkg/apimachinery/identity" "github.com/grafana/grafana/pkg/registry/apis/preferences/utils" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" "github.com/grafana/grafana/pkg/services/star" @@ -91,8 +92,18 @@ func (s *DashboardStarsStorage) List(ctx context.Context, options *internalversi return nil, fmt.Errorf("cross cluster listing is not supported") } + userInfo, err := identity.GetRequester(ctx) + if err != nil { + return nil, err + } + + user := userInfo.GetUID() + if userInfo.GetIsGrafanaAdmin() || userInfo.GetIdentityType() == authlib.TypeAccessPolicy { + user = "" // can see everything + } + list := &preferences.StarsList{} - found, rv, err := s.sql.GetStars(ctx, ns.OrgID, "") + found, rv, err := s.sql.GetStars(ctx, ns.OrgID, user) if err != nil { return nil, err } @@ -126,7 +137,7 @@ func (s *DashboardStarsStorage) Get(ctx context.Context, name string, options *m return nil, err } - found, _, err := s.sql.GetStars(ctx, ns.OrgID, owner.Name) + found, _, err := s.sql.GetStars(ctx, ns.OrgID, owner.Identifier) if err != nil { return nil, err } @@ -157,7 +168,7 @@ func (s *DashboardStarsStorage) write(ctx context.Context, obj *preferences.Star } user, err := s.users.GetByUID(ctx, &user.GetUserByUIDQuery{ - UID: owner.Name, + UID: owner.Identifier, }) if err != nil { return nil, err @@ -176,7 +187,7 @@ func (s *DashboardStarsStorage) write(ctx context.Context, obj *preferences.Star }}, err } - current, _, err := s.sql.GetStars(ctx, ns.OrgID, owner.Name) + current, _, err := s.sql.GetStars(ctx, ns.OrgID, owner.Identifier) if err != nil { return nil, err } diff --git a/pkg/registry/apis/preferences/current.go b/pkg/registry/apis/preferences/merged_preferences.go similarity index 83% rename from pkg/registry/apis/preferences/current.go rename to pkg/registry/apis/preferences/merged_preferences.go index 3a8ab55d646..2b5c0e6351e 100644 --- a/pkg/registry/apis/preferences/current.go +++ b/pkg/registry/apis/preferences/merged_preferences.go @@ -18,13 +18,13 @@ import ( "github.com/grafana/grafana/pkg/util/errhttp" ) -type calculator struct { +type merger struct { defaults preferences.PreferencesSpec sql *legacy.LegacySQL } -func newCalculator(cfg *setting.Cfg, sql *legacy.LegacySQL) *calculator { - return &calculator{ +func newMerger(cfg *setting.Cfg, sql *legacy.LegacySQL) *merger { + return &merger{ sql: sql, defaults: preferences.PreferencesSpec{ Theme: &cfg.DefaultTheme, @@ -35,17 +35,17 @@ func newCalculator(cfg *setting.Cfg, sql *legacy.LegacySQL) *calculator { } } -func (s *calculator) GetAPIRoutes(defs map[string]common.OpenAPIDefinition) *builder.APIRoutes { +func (s *merger) GetAPIRoutes(defs map[string]common.OpenAPIDefinition) *builder.APIRoutes { schema := defs["github.com/grafana/grafana/apps/preferences/pkg/apis/preferences/v1alpha1.Preference"].Schema return &builder.APIRoutes{ Namespace: []builder.APIRouteHandler{ { - Path: "current", // calculate? + Path: "preferences/merged", Spec: &spec3.PathProps{ Get: &spec3.Operation{ OperationProps: spec3.OperationProps{ - OperationId: "currentPreferences", + OperationId: "mergedPreferences", Tags: []string{"Preferences"}, Description: "Get preferences for requester. This combines the user preferences with the team and global defaults", Parameters: []*spec3.Parameter{ @@ -87,7 +87,7 @@ func (s *calculator) GetAPIRoutes(defs map[string]common.OpenAPIDefinition) *bui } } -func (s *calculator) Current(w http.ResponseWriter, r *http.Request) { +func (s *merger) Current(w http.ResponseWriter, r *http.Request) { ctx := r.Context() user, err := identity.GetRequester(ctx) if err != nil { @@ -115,15 +115,18 @@ func (s *calculator) Current(w http.ResponseWriter, r *http.Request) { // items should be in ascending order of importance func merge(defaults preferences.PreferencesSpec, items []preferences.Preferences) (*preferences.Preferences, error) { p := &preferences.Preferences{ - TypeMeta: preferences.PreferencesResourceInfo.TypeMeta(), - ObjectMeta: v1.ObjectMeta{ - CreationTimestamp: v1.Now(), - }, - Spec: defaults, + TypeMeta: preferences.PreferencesResourceInfo.TypeMeta(), + ObjectMeta: v1.ObjectMeta{}, + Spec: defaults, } // Iterate in reverse order (least relevant to most relevant) for _, v := range items { + // Set the time from the most recent change + if p.CreationTimestamp.IsZero() || v.CreationTimestamp.After(p.CreationTimestamp.Time) { + p.CreationTimestamp = v.CreationTimestamp + } + if err := mergo.Merge(&p.Spec, &v.Spec, mergo.WithOverride); err != nil { return nil, err } diff --git a/pkg/registry/apis/preferences/current_test.go b/pkg/registry/apis/preferences/merged_preferences_test.go similarity index 100% rename from pkg/registry/apis/preferences/current_test.go rename to pkg/registry/apis/preferences/merged_preferences_test.go diff --git a/pkg/registry/apis/preferences/register.go b/pkg/registry/apis/preferences/register.go index da6d2740ae5..2c3d3623c62 100644 --- a/pkg/registry/apis/preferences/register.go +++ b/pkg/registry/apis/preferences/register.go @@ -6,6 +6,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/authorization/authorizer" "k8s.io/apiserver/pkg/registry/rest" genericapiserver "k8s.io/apiserver/pkg/server" "k8s.io/kube-openapi/pkg/common" @@ -17,6 +18,7 @@ import ( grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/infra/db" "github.com/grafana/grafana/pkg/registry/apis/preferences/legacy" + "github.com/grafana/grafana/pkg/registry/apis/preferences/utils" "github.com/grafana/grafana/pkg/services/apiserver/builder" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -30,13 +32,11 @@ import ( var _ builder.APIGroupBuilder = (*APIBuilder)(nil) type APIBuilder struct { - namespacer request.NamespaceMapper - sql *legacy.LegacySQL + authorizer authorizer.Authorizer + legacyStars *legacy.DashboardStarsStorage + legacyPrefs rest.Storage - stars star.Service - prefs pref.Service - users user.Service - calculator *calculator // joins all preferences + merger *merger // joins all preferences } func RegisterAPIService( @@ -55,13 +55,29 @@ func RegisterAPIService( sql := legacy.NewLegacySQL(legacysql.NewDatabaseProvider(db)) builder := &APIBuilder{ - prefs: prefs, // for writing - stars: stars, // for writing - users: users, // for writing - namespacer: request.GetNamespaceMapper(cfg), - sql: sql, - calculator: newCalculator(cfg, sql), + merger: newMerger(cfg, sql), + authorizer: &authorizeFromName{ + oknames: []string{"merged"}, + teams: sql, // should be from the IAM service + resource: map[string][]utils.ResourceOwner{ + "stars": {utils.UserResourceOwner}, + "preferences": { + utils.NamespaceResourceOwner, + utils.TeamResourceOwner, + utils.UserResourceOwner, + }, + }, + }, } + + namespacer := request.GetNamespaceMapper(cfg) + if prefs != nil { + builder.legacyPrefs = legacy.NewPreferencesStorage(namespacer, sql) + } + if stars != nil { + builder.legacyStars = legacy.NewDashboardStarsStorage(stars, users, namespacer, sql) + } + apiregistration.RegisterAPI(builder) return builder } @@ -92,49 +108,49 @@ func (b *APIBuilder) UpdateAPIGroupInfo(apiGroupInfo *genericapiserver.APIGroupI // Configure Stars Dual writer resource := preferences.StarsResourceInfo var stars grafanarest.Storage - unified, err := grafanaregistry.NewRegistryStore(opts.Scheme, resource, opts.OptsGetter) + stars, err := grafanaregistry.NewRegistryStore(opts.Scheme, resource, opts.OptsGetter) if err != nil { return err } - stars = unified - if b.stars != nil && opts.DualWriteBuilder != nil { - legacy := legacy.NewDashboardStarsStorage(b.stars, b.users, b.namespacer, b.sql) - stars, err = opts.DualWriteBuilder(resource.GroupResource(), legacy, unified) + if b.legacyStars != nil && opts.DualWriteBuilder != nil { + stars, err = opts.DualWriteBuilder(resource.GroupResource(), b.legacyStars, stars) if err != nil { return err } } storage[resource.StoragePath()] = stars - storage[resource.StoragePath("write")] = &starsREST{ - store: stars, - } + storage[resource.StoragePath("update")] = &starsREST{store: stars} // Configure Preferences prefs := preferences.PreferencesResourceInfo - storage[prefs.StoragePath()] = legacy.NewPreferencesStorage(b.namespacer, b.sql) + storage[prefs.StoragePath()] = b.legacyPrefs apiGroupInfo.VersionedResourcesStorageMap[preferences.APIVersion] = storage return nil } +func (b *APIBuilder) GetAuthorizer() authorizer.Authorizer { + return b.authorizer +} + func (b *APIBuilder) GetOpenAPIDefinitions() common.GetOpenAPIDefinitions { return preferences.GetOpenAPIDefinitions } func (b *APIBuilder) GetAPIRoutes(gv schema.GroupVersion) *builder.APIRoutes { defs := b.GetOpenAPIDefinitions()(func(path string) spec.Ref { return spec.Ref{} }) - return b.calculator.GetAPIRoutes(defs) + return b.merger.GetAPIRoutes(defs) } func (b *APIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3.OpenAPI, error) { oas.Info.Description = "Grafana preferences" root := "/apis/" + b.GetGroupVersion().String() + "/" - writeKey := root + "namespaces/{namespace}/stars/{name}/write" - delete(oas.Paths.Paths, writeKey) + updateKey := root + "namespaces/{namespace}/stars/{name}/update" + delete(oas.Paths.Paths, updateKey) // Add the group/kind/id properties to the path - stars, ok := oas.Paths.Paths[writeKey+"/{path}"] + stars, ok := oas.Paths.Paths[updateKey+"/{path}"] if !ok || stars == nil { return nil, fmt.Errorf("unable to find write path") } @@ -175,8 +191,8 @@ func (b *APIBuilder) PostProcessOpenAPI(oas *spec3.OpenAPI) (*spec3.OpenAPI, err stars.Delete.Description = "Remove a starred item" stars.Delete.OperationId = "removeStar" - delete(oas.Paths.Paths, writeKey+"/{path}") - oas.Paths.Paths[writeKey+"/{group}/{kind}/{id}"] = stars + delete(oas.Paths.Paths, updateKey+"/{path}") + oas.Paths.Paths[updateKey+"/{group}/{kind}/{id}"] = stars return oas, nil } diff --git a/pkg/registry/apis/preferences/stars.go b/pkg/registry/apis/preferences/update_stars.go similarity index 97% rename from pkg/registry/apis/preferences/stars.go rename to pkg/registry/apis/preferences/update_stars.go index e082e8883f7..bf3a4c0241d 100644 --- a/pkg/registry/apis/preferences/stars.go +++ b/pkg/registry/apis/preferences/update_stars.go @@ -64,12 +64,12 @@ func (r *starsREST) Connect(ctx context.Context, name string, _ runtime.Object, if !found || parsed.Owner != utils.UserResourceOwner { return nil, fmt.Errorf("only works with user stars") } - if user.GetIdentifier() != parsed.Name { + if user.GetIdentifier() != parsed.Identifier { return nil, fmt.Errorf("must request as the given user") } return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - item, err := itemFromPath(req.URL.Path, fmt.Sprintf("/%s/write", name)) + item, err := itemFromPath(req.URL.Path, fmt.Sprintf("/%s/update", name)) if err != nil { responder.Error(err) return diff --git a/pkg/registry/apis/preferences/stars_test.go b/pkg/registry/apis/preferences/update_stars_test.go similarity index 100% rename from pkg/registry/apis/preferences/stars_test.go rename to pkg/registry/apis/preferences/update_stars_test.go diff --git a/pkg/registry/apis/preferences/utils/names.go b/pkg/registry/apis/preferences/utils/names.go index 67da127d786..0433869ea10 100644 --- a/pkg/registry/apis/preferences/utils/names.go +++ b/pkg/registry/apis/preferences/utils/names.go @@ -17,15 +17,15 @@ const ( ) type OwnerReference struct { - Owner ResourceOwner // the resource owner - Name string // the team|user name + Owner ResourceOwner // the resource owner + Identifier string // the team|user name } func (o OwnerReference) AsName() string { - if o.Name == "" || o.Owner == NamespaceResourceOwner { + if o.Identifier == "" || o.Owner == NamespaceResourceOwner { return string(o.Owner) } - return string(o.Owner) + "-" + o.Name + return string(o.Owner) + "-" + o.Identifier } func ParseOwnerFromName(name string) (OwnerReference, bool) { @@ -33,9 +33,9 @@ func ParseOwnerFromName(name string) (OwnerReference, bool) { if found && len(after) > 0 { switch before { case "user": - return OwnerReference{Owner: UserResourceOwner, Name: after}, true + return OwnerReference{Owner: UserResourceOwner, Identifier: after}, true case "team": - return OwnerReference{Owner: TeamResourceOwner, Name: after}, true + return OwnerReference{Owner: TeamResourceOwner, Identifier: after}, true } } else if name == "namespace" { return OwnerReference{Owner: NamespaceResourceOwner}, true diff --git a/pkg/registry/apis/preferences/utils/names_test.go b/pkg/registry/apis/preferences/utils/names_test.go index e1e5fc77abd..b02ef1c86e6 100644 --- a/pkg/registry/apis/preferences/utils/names_test.go +++ b/pkg/registry/apis/preferences/utils/names_test.go @@ -24,7 +24,7 @@ func TestLegacyAuthorizer(t *testing.T) { { name: "with user", input: "user-a", - output: utils.OwnerReference{Owner: utils.UserResourceOwner, Name: "a"}, + output: utils.OwnerReference{Owner: utils.UserResourceOwner, Identifier: "a"}, found: true, }, { @@ -36,7 +36,7 @@ func TestLegacyAuthorizer(t *testing.T) { { name: "with team", input: "team-b", - output: utils.OwnerReference{Owner: utils.TeamResourceOwner, Name: "b"}, + output: utils.OwnerReference{Owner: utils.TeamResourceOwner, Identifier: "b"}, found: true, }, { diff --git a/pkg/registry/apis/preferences/utils/teams.go b/pkg/registry/apis/preferences/utils/teams.go new file mode 100644 index 00000000000..5222e49ac44 --- /dev/null +++ b/pkg/registry/apis/preferences/utils/teams.go @@ -0,0 +1,13 @@ +package utils + +import ( + "context" + + authlib "github.com/grafana/authlib/types" +) + +//go:generate mockery --name TeamService --structname MockTeamService --inpackage --filename teams_mock.go --with-expecter +type TeamService interface { + InTeam(ctx context.Context, id authlib.AuthInfo, team string, admin bool) (bool, error) + GetTeams(ctx context.Context, id authlib.AuthInfo, admin bool) ([]string, error) +} diff --git a/pkg/registry/apis/preferences/utils/teams_mock.go b/pkg/registry/apis/preferences/utils/teams_mock.go new file mode 100644 index 00000000000..73a39c27d7c --- /dev/null +++ b/pkg/registry/apis/preferences/utils/teams_mock.go @@ -0,0 +1,156 @@ +// Code generated by mockery v2.53.4. DO NOT EDIT. + +package utils + +import ( + context "context" + + types "github.com/grafana/authlib/types" + mock "github.com/stretchr/testify/mock" +) + +// MockTeamService is an autogenerated mock type for the TeamService type +type MockTeamService struct { + mock.Mock +} + +type MockTeamService_Expecter struct { + mock *mock.Mock +} + +func (_m *MockTeamService) EXPECT() *MockTeamService_Expecter { + return &MockTeamService_Expecter{mock: &_m.Mock} +} + +// GetTeams provides a mock function with given fields: ctx, id, admin +func (_m *MockTeamService) GetTeams(ctx context.Context, id types.AuthInfo, admin bool) ([]string, error) { + ret := _m.Called(ctx, id, admin) + + if len(ret) == 0 { + panic("no return value specified for GetTeams") + } + + var r0 []string + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, types.AuthInfo, bool) ([]string, error)); ok { + return rf(ctx, id, admin) + } + if rf, ok := ret.Get(0).(func(context.Context, types.AuthInfo, bool) []string); ok { + r0 = rf(ctx, id, admin) + } else { + if ret.Get(0) != nil { + r0 = ret.Get(0).([]string) + } + } + + if rf, ok := ret.Get(1).(func(context.Context, types.AuthInfo, bool) error); ok { + r1 = rf(ctx, id, admin) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockTeamService_GetTeams_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'GetTeams' +type MockTeamService_GetTeams_Call struct { + *mock.Call +} + +// GetTeams is a helper method to define mock.On call +// - ctx context.Context +// - id types.AuthInfo +// - admin bool +func (_e *MockTeamService_Expecter) GetTeams(ctx interface{}, id interface{}, admin interface{}) *MockTeamService_GetTeams_Call { + return &MockTeamService_GetTeams_Call{Call: _e.mock.On("GetTeams", ctx, id, admin)} +} + +func (_c *MockTeamService_GetTeams_Call) Run(run func(ctx context.Context, id types.AuthInfo, admin bool)) *MockTeamService_GetTeams_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(types.AuthInfo), args[2].(bool)) + }) + return _c +} + +func (_c *MockTeamService_GetTeams_Call) Return(_a0 []string, _a1 error) *MockTeamService_GetTeams_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockTeamService_GetTeams_Call) RunAndReturn(run func(context.Context, types.AuthInfo, bool) ([]string, error)) *MockTeamService_GetTeams_Call { + _c.Call.Return(run) + return _c +} + +// InTeam provides a mock function with given fields: ctx, id, team, admin +func (_m *MockTeamService) InTeam(ctx context.Context, id types.AuthInfo, team string, admin bool) (bool, error) { + ret := _m.Called(ctx, id, team, admin) + + if len(ret) == 0 { + panic("no return value specified for InTeam") + } + + var r0 bool + var r1 error + if rf, ok := ret.Get(0).(func(context.Context, types.AuthInfo, string, bool) (bool, error)); ok { + return rf(ctx, id, team, admin) + } + if rf, ok := ret.Get(0).(func(context.Context, types.AuthInfo, string, bool) bool); ok { + r0 = rf(ctx, id, team, admin) + } else { + r0 = ret.Get(0).(bool) + } + + if rf, ok := ret.Get(1).(func(context.Context, types.AuthInfo, string, bool) error); ok { + r1 = rf(ctx, id, team, admin) + } else { + r1 = ret.Error(1) + } + + return r0, r1 +} + +// MockTeamService_InTeam_Call is a *mock.Call that shadows Run/Return methods with type explicit version for method 'InTeam' +type MockTeamService_InTeam_Call struct { + *mock.Call +} + +// InTeam is a helper method to define mock.On call +// - ctx context.Context +// - id types.AuthInfo +// - team string +// - admin bool +func (_e *MockTeamService_Expecter) InTeam(ctx interface{}, id interface{}, team interface{}, admin interface{}) *MockTeamService_InTeam_Call { + return &MockTeamService_InTeam_Call{Call: _e.mock.On("InTeam", ctx, id, team, admin)} +} + +func (_c *MockTeamService_InTeam_Call) Run(run func(ctx context.Context, id types.AuthInfo, team string, admin bool)) *MockTeamService_InTeam_Call { + _c.Call.Run(func(args mock.Arguments) { + run(args[0].(context.Context), args[1].(types.AuthInfo), args[2].(string), args[3].(bool)) + }) + return _c +} + +func (_c *MockTeamService_InTeam_Call) Return(_a0 bool, _a1 error) *MockTeamService_InTeam_Call { + _c.Call.Return(_a0, _a1) + return _c +} + +func (_c *MockTeamService_InTeam_Call) RunAndReturn(run func(context.Context, types.AuthInfo, string, bool) (bool, error)) *MockTeamService_InTeam_Call { + _c.Call.Return(run) + return _c +} + +// NewMockTeamService creates a new instance of MockTeamService. It also registers a testing interface on the mock and a cleanup function to assert the mocks expectations. +// The first argument is typically a *testing.T value. +func NewMockTeamService(t interface { + mock.TestingT + Cleanup(func()) +}) *MockTeamService { + mock := &MockTeamService{} + mock.Mock.Test(t) + + t.Cleanup(func() { mock.AssertExpectations(t) }) + + return mock +} diff --git a/pkg/tests/apis/openapi_snapshots/preferences.grafana.app-v1alpha1.json b/pkg/tests/apis/openapi_snapshots/preferences.grafana.app-v1alpha1.json index e009c928585..c5d87c4088d 100644 --- a/pkg/tests/apis/openapi_snapshots/preferences.grafana.app-v1alpha1.json +++ b/pkg/tests/apis/openapi_snapshots/preferences.grafana.app-v1alpha1.json @@ -36,36 +36,6 @@ } } }, - "/apis/preferences.grafana.app/v1alpha1/namespaces/{namespace}/current": { - "get": { - "tags": [ - "Preferences" - ], - "description": "Get preferences for requester. This combines the user preferences with the team and global defaults", - "operationId": "currentPreferences", - "parameters": [ - { - "name": "namespace", - "in": "path", - "description": "workspace", - "required": true, - "schema": { - "type": "string" - }, - "example": "default" - } - ], - "responses": { - "200": { - "content": { - "application/json": { - "schema": {} - } - } - } - } - } - }, "/apis/preferences.grafana.app/v1alpha1/namespaces/{namespace}/preferences": { "get": { "tags": [ @@ -224,6 +194,36 @@ } ] }, + "/apis/preferences.grafana.app/v1alpha1/namespaces/{namespace}/preferences/merged": { + "get": { + "tags": [ + "Preferences" + ], + "description": "Get preferences for requester. This combines the user preferences with the team and global defaults", + "operationId": "mergedPreferences", + "parameters": [ + { + "name": "namespace", + "in": "path", + "description": "workspace", + "required": true, + "schema": { + "type": "string" + }, + "example": "default" + } + ], + "responses": { + "200": { + "content": { + "application/json": { + "schema": {} + } + } + } + } + } + }, "/apis/preferences.grafana.app/v1alpha1/namespaces/{namespace}/preferences/{name}": { "get": { "tags": [ @@ -1122,7 +1122,7 @@ } ] }, - "/apis/preferences.grafana.app/v1alpha1/namespaces/{namespace}/stars/{name}/write/{group}/{kind}/{id}": { + "/apis/preferences.grafana.app/v1alpha1/namespaces/{namespace}/stars/{name}/update/{group}/{kind}/{id}": { "put": { "tags": [ "Stars" diff --git a/pkg/tests/apis/preferences/preferences_test.go b/pkg/tests/apis/preferences/preferences_test.go index 04152b4026a..01caaae8318 100644 --- a/pkg/tests/apis/preferences/preferences_test.go +++ b/pkg/tests/apis/preferences/preferences_test.go @@ -127,15 +127,15 @@ func TestIntegrationPreferences(t *testing.T) { `"regionalFormat":"" }`, string(jj)) - current := apis.DoRequest(helper, apis.RequestParams{ + merged := apis.DoRequest(helper, apis.RequestParams{ User: clientAdmin.Args.User, Method: http.MethodGet, - Path: "/apis/preferences.grafana.app/v1alpha1/namespaces/default/current", + Path: "/apis/preferences.grafana.app/v1alpha1/namespaces/default/preferences/merged", }, &preferences.Preferences{}) - require.Equal(t, http.StatusOK, current.Response.StatusCode, "get current preferences") - require.Equal(t, "saturday", *current.Result.Spec.WeekStart) // from user - require.Equal(t, "africa", *current.Result.Spec.Timezone) // from team - require.Equal(t, "dark", *current.Result.Spec.Theme) // from org - require.Equal(t, "en-US", *current.Result.Spec.Language) // settings.ini + require.Equal(t, http.StatusOK, merged.Response.StatusCode, "get merged preferences") + require.Equal(t, "saturday", *merged.Result.Spec.WeekStart) // from user + require.Equal(t, "africa", *merged.Result.Spec.Timezone) // from team + require.Equal(t, "dark", *merged.Result.Spec.Theme) // from org + require.Equal(t, "en-US", *merged.Result.Spec.Language) // settings.ini }) } diff --git a/pkg/tests/apis/preferences/stars_test.go b/pkg/tests/apis/preferences/stars_test.go index 728d5adb8ce..93106383cfa 100644 --- a/pkg/tests/apis/preferences/stars_test.go +++ b/pkg/tests/apis/preferences/stars_test.go @@ -13,7 +13,9 @@ import ( dashboardV1 "github.com/grafana/grafana/apps/dashboard/pkg/apis/dashboard/v1beta1" preferences "github.com/grafana/grafana/apps/preferences/pkg/apis/preferences/v1alpha1" + grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/services/featuremgmt" + "github.com/grafana/grafana/pkg/setting" "github.com/grafana/grafana/pkg/tests/apis" "github.com/grafana/grafana/pkg/tests/testinfra" "github.com/grafana/grafana/pkg/util/testutil" @@ -22,127 +24,160 @@ import ( func TestIntegrationStars(t *testing.T) { testutil.SkipIntegrationTestInShortMode(t) - helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ - AppModeProduction: false, // required for experimental APIs - DisableAnonymous: true, - EnableFeatureToggles: []string{ - featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, - }, - }) - - t.Run("legacy dashboard stars", func(t *testing.T) { - ctx := context.Background() - starsClient := helper.GetResourceClient(apis.ResourceClientArgs{ - User: helper.Org1.Admin, - GVR: preferences.StarsResourceInfo.GroupVersionResource(), - }) - dashboardClient := helper.GetResourceClient(apis.ResourceClientArgs{ - User: helper.Org1.Admin, - GVR: dashboardV1.DashboardResourceInfo.GroupVersionResource(), + for _, mode := range []grafanarest.DualWriterMode{ + grafanarest.Mode0, + grafanarest.Mode2, // anything past 2 will fail + } { + helper := apis.NewK8sTestHelper(t, testinfra.GrafanaOpts{ + AppModeProduction: false, // required for experimental APIs + DisableAnonymous: true, + EnableFeatureToggles: []string{ + featuremgmt.FlagGrafanaAPIServerWithExperimentalAPIs, + }, + UnifiedStorageConfig: map[string]setting.UnifiedStorageConfig{ + "dashboards.dashboard.grafana.app": { + DualWriterMode: mode, + }, + "folders.folder.grafana.app": { + DualWriterMode: mode, + }, + "stars.preferences.grafana.app": { + DualWriterMode: mode, + }, + "preferences.preferences.grafana.app": { + DualWriterMode: mode, + }, + }, }) - // Create 5 dashboards - for i := range 5 { - _, err := dashboardClient.Resource.Create(context.Background(), &unstructured.Unstructured{ - Object: map[string]any{ - "apiVersion": dashboardV1.DashboardResourceInfo.GroupVersion().String(), - "kind": "Dashboard", + t.Run(fmt.Sprintf("test stars (mode:%d)", mode), func(t *testing.T) { + ctx := context.Background() + starsClient := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + GVR: preferences.StarsResourceInfo.GroupVersionResource(), + }) + starsClientViewer := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Viewer, + GVR: preferences.StarsResourceInfo.GroupVersionResource(), + }) + dashboardClient := helper.GetResourceClient(apis.ResourceClientArgs{ + User: helper.Org1.Admin, + GVR: dashboardV1.DashboardResourceInfo.GroupVersionResource(), + }) + + // Create 5 dashboards + for i := range 5 { + _, err := dashboardClient.Resource.Create(context.Background(), &unstructured.Unstructured{ + Object: map[string]any{ + "apiVersion": dashboardV1.DashboardResourceInfo.GroupVersion().String(), + "kind": "Dashboard", + "metadata": map[string]any{ + "name": fmt.Sprintf("test-%d", i), + }, + "spec": map[string]any{ + "title": fmt.Sprintf("test %d", i), + "schemaVersion": 42, // not really! + "panels": []any{}, + }, + }, + }, metav1.CreateOptions{}) + require.NoError(t, err) + } + found, err := dashboardClient.Resource.List(context.Background(), metav1.ListOptions{}) + require.NoError(t, err) + require.Len(t, found.Items, 5, "should be 5 dashboards") + + // List is empty when we start + rsp, err := starsClient.Resource.List(ctx, metav1.ListOptions{}) + require.NoError(t, err) + require.Empty(t, rsp.Items, "no stars saved yet") + + raw := make(map[string]any) + legacyResponse := apis.DoRequest(helper, apis.RequestParams{ + User: starsClient.Args.User, + Method: http.MethodPost, + Path: "/api/user/stars/dashboard/uid/test-2", + }, &raw) + require.Equal(t, http.StatusOK, legacyResponse.Response.StatusCode, "add dashboard star") + legacyResponse = apis.DoRequest(helper, apis.RequestParams{ + User: starsClient.Args.User, + Method: http.MethodPost, + Path: "/api/user/stars/dashboard/uid/test-3", + }, &raw) + require.Equal(t, http.StatusOK, legacyResponse.Response.StatusCode, "add dashboard star") + + // List values and compare results + rsp, err = starsClient.Resource.List(ctx, metav1.ListOptions{}) + require.NoError(t, err) + stars := typed(t, rsp, &preferences.StarsList{}) + + require.Len(t, stars.Items, 1, "user stars should exist") + require.Equal(t, "user-"+starsClient.Args.User.Identity.GetIdentifier(), + stars.Items[0].GetName(), "star resource for user") + resources := stars.Items[0].Spec.Resource + require.Len(t, resources, 1) + require.Equal(t, "dashboard.grafana.app", resources[0].Group) + require.Equal(t, "Dashboard", resources[0].Kind) + require.ElementsMatch(t, []string{"test-2", "test-3"}, resources[0].Names) + + // Remove one star + legacyResponse = apis.DoRequest(helper, apis.RequestParams{ + User: starsClient.Args.User, + Method: http.MethodDelete, + Path: "/api/user/stars/dashboard/uid/test-3", + }, &raw) + require.Equal(t, http.StatusOK, legacyResponse.Response.StatusCode, "removed dashboard star") + + rspObj, err := starsClient.Resource.Get(ctx, "user-"+starsClient.Args.User.Identity.GetIdentifier(), metav1.GetOptions{}) + require.NoError(t, err) + + after := typed(t, rspObj, &preferences.Stars{}) + resources = after.Spec.Resource + require.Len(t, resources, 1) + require.Equal(t, "dashboard.grafana.app", resources[0].Group) + require.Equal(t, "Dashboard", resources[0].Kind) + require.Equal(t, []string{"test-2"}, resources[0].Names) + + // Change stars via k8s update + rspObj, err = starsClient.Resource.Update(ctx, &unstructured.Unstructured{ + Object: map[string]interface{}{ "metadata": map[string]any{ - "name": fmt.Sprintf("test-%d", i), + "name": "user-" + starsClient.Args.User.Identity.GetIdentifier(), + "namespace": "default", }, "spec": map[string]any{ - "title": fmt.Sprintf("test %d", i), - "schemaVersion": 42, // not really! - "panels": []any{}, - }, - }, - }, metav1.CreateOptions{}) - require.NoError(t, err) - } - found, err := dashboardClient.Resource.List(context.Background(), metav1.ListOptions{}) - require.NoError(t, err) - require.Len(t, found.Items, 5, "should be 5 dashboards") - - // List is empty when we start - rsp, err := starsClient.Resource.List(ctx, metav1.ListOptions{}) - require.NoError(t, err) - require.Empty(t, rsp.Items, "no stars saved yet") - - raw := make(map[string]any) - legacyResponse := apis.DoRequest(helper, apis.RequestParams{ - User: starsClient.Args.User, - Method: http.MethodPost, - Path: "/api/user/stars/dashboard/uid/test-2", - }, &raw) - require.Equal(t, http.StatusOK, legacyResponse.Response.StatusCode, "add dashboard star") - legacyResponse = apis.DoRequest(helper, apis.RequestParams{ - User: starsClient.Args.User, - Method: http.MethodPost, - Path: "/api/user/stars/dashboard/uid/test-3", - }, &raw) - require.Equal(t, http.StatusOK, legacyResponse.Response.StatusCode, "add dashboard star") - - // List values and compare results - rsp, err = starsClient.Resource.List(ctx, metav1.ListOptions{}) - require.NoError(t, err) - stars := typed(t, rsp, &preferences.StarsList{}) - - require.Len(t, stars.Items, 1, "user stars should exist") - require.Equal(t, "user-"+starsClient.Args.User.Identity.GetIdentifier(), - stars.Items[0].GetName(), "star resource for user") - resources := stars.Items[0].Spec.Resource - require.Len(t, resources, 1) - require.Equal(t, "dashboard.grafana.app", resources[0].Group) - require.Equal(t, "Dashboard", resources[0].Kind) - require.ElementsMatch(t, []string{"test-2", "test-3"}, resources[0].Names) - - // Remove one star - legacyResponse = apis.DoRequest(helper, apis.RequestParams{ - User: starsClient.Args.User, - Method: http.MethodDelete, - Path: "/api/user/stars/dashboard/uid/test-3", - }, &raw) - require.Equal(t, http.StatusOK, legacyResponse.Response.StatusCode, "removed dashboard star") - - rspObj, err := starsClient.Resource.Get(ctx, "user-"+starsClient.Args.User.Identity.GetIdentifier(), metav1.GetOptions{}) - require.NoError(t, err) - - after := typed(t, rspObj, &preferences.Stars{}) - resources = after.Spec.Resource - require.Len(t, resources, 1) - require.Equal(t, "dashboard.grafana.app", resources[0].Group) - require.Equal(t, "Dashboard", resources[0].Kind) - require.Equal(t, []string{"test-2"}, resources[0].Names) - - // Change stars via k8s update - rspObj, err = starsClient.Resource.Update(ctx, &unstructured.Unstructured{ - Object: map[string]interface{}{ - "metadata": map[string]any{ - "name": "user-" + starsClient.Args.User.Identity.GetIdentifier(), - "namespace": "default", - }, - "spec": map[string]any{ - "resource": []map[string]any{ - { - "group": "dashboard.grafana.app", - "kind": "Dashboard", - "names": []string{"test-2", "aaa", "bbb"}, + "resource": []map[string]any{ + { + "group": "dashboard.grafana.app", + "kind": "Dashboard", + "names": []string{"test-2", "aaa", "bbb"}, + }, }, }, }, - }, - }, metav1.UpdateOptions{}) - require.NoError(t, err) + }, metav1.UpdateOptions{}) + require.NoError(t, err) - after = typed(t, rspObj, &preferences.Stars{}) - resources = after.Spec.Resource - require.Len(t, resources, 1) - require.Equal(t, "dashboard.grafana.app", resources[0].Group) - require.Equal(t, "Dashboard", resources[0].Kind) - require.ElementsMatch(t, - []string{"test-2", "aaa", "bbb"}, // NOTE 2 stays, 3 removed, added aaa+bbb - resources[0].Names) - }) + after = typed(t, rspObj, &preferences.Stars{}) + resources = after.Spec.Resource + require.Len(t, resources, 1) + require.Equal(t, "dashboard.grafana.app", resources[0].Group) + require.Equal(t, "Dashboard", resources[0].Kind) + require.ElementsMatch(t, + []string{"test-2", "aaa", "bbb"}, // NOTE 2 stays, 3 removed, added aaa+bbb + resources[0].Names) + + // Viewer does not have any stars + rsp, err = starsClientViewer.Resource.List(ctx, metav1.ListOptions{}) + require.NoError(t, err) + require.Empty(t, rsp.Items, "expect empty list") + + // Not allowed to see another user's stars + rspObj, err = starsClientViewer.Resource.Get(ctx, "user-"+starsClient.Args.User.Identity.GetIdentifier(), metav1.GetOptions{}) + require.Error(t, err) + require.Nil(t, rspObj) + }) + } } func typed[T any](t *testing.T, obj any, out T) T { diff --git a/public/app/api/clients/preferences/v1alpha1/endpoints.gen.ts b/public/app/api/clients/preferences/v1alpha1/endpoints.gen.ts index 09d17608083..ac0a34e9af5 100644 --- a/public/app/api/clients/preferences/v1alpha1/endpoints.gen.ts +++ b/public/app/api/clients/preferences/v1alpha1/endpoints.gen.ts @@ -10,10 +10,6 @@ const injectedRtkApi = api query: () => ({ url: `/apis/preferences.grafana.app/v1alpha1/` }), providesTags: ['API Discovery'], }), - currentPreferences: build.query({ - query: () => ({ url: `/current` }), - providesTags: ['Preferences'], - }), listPreferences: build.query({ query: (queryArg) => ({ url: `/preferences`, @@ -33,6 +29,10 @@ const injectedRtkApi = api }), providesTags: ['Preferences'], }), + mergedPreferences: build.query({ + query: () => ({ url: `/preferences/merged` }), + providesTags: ['Preferences'], + }), getPreferences: build.query({ query: (queryArg) => ({ url: `/preferences/${queryArg.name}`, @@ -153,14 +153,14 @@ const injectedRtkApi = api }), addStar: build.mutation({ query: (queryArg) => ({ - url: `/stars/${queryArg.name}/write/${queryArg.group}/${queryArg.kind}/${queryArg.id}`, + url: `/stars/${queryArg.name}/update/${queryArg.group}/${queryArg.kind}/${queryArg.id}`, method: 'PUT', }), invalidatesTags: ['Stars'], }), removeStar: build.mutation({ query: (queryArg) => ({ - url: `/stars/${queryArg.name}/write/${queryArg.group}/${queryArg.kind}/${queryArg.id}`, + url: `/stars/${queryArg.name}/update/${queryArg.group}/${queryArg.kind}/${queryArg.id}`, method: 'DELETE', }), invalidatesTags: ['Stars'], @@ -171,8 +171,6 @@ const injectedRtkApi = api export { injectedRtkApi as generatedAPI }; export type GetApiResourcesApiResponse = /** status 200 OK */ ApiResourceList; export type GetApiResourcesApiArg = void; -export type CurrentPreferencesApiResponse = /** status 200 undefined */ any; -export type CurrentPreferencesApiArg = void; export type ListPreferencesApiResponse = /** status 200 OK */ PreferencesList; export type ListPreferencesApiArg = { /** allowWatchBookmarks requests watch events with type "BOOKMARK". Servers that do not implement bookmarks may ignore this flag and bookmarks are sent at the server's discretion. Clients should not assume bookmarks are returned at any specific interval, nor may they assume the server will send any BOOKMARK event during a session. If this is not a watch, this field is ignored. */ @@ -218,6 +216,8 @@ export type ListPreferencesApiArg = { /** Watch for changes to the described resources and return them as a stream of add, update, and remove notifications. Specify resourceVersion. */ watch?: boolean; }; +export type MergedPreferencesApiResponse = /** status 200 undefined */ any; +export type MergedPreferencesApiArg = void; export type GetPreferencesApiResponse = /** status 200 OK */ Preferences; export type GetPreferencesApiArg = { /** name of the Preferences */ @@ -704,8 +704,8 @@ export type Status = { export type Patch = object; export const { useGetApiResourcesQuery, - useCurrentPreferencesQuery, useListPreferencesQuery, + useMergedPreferencesQuery, useGetPreferencesQuery, useListStarsQuery, useCreateStarsMutation,