From e525b529a861614aa5f4870272f8a297bbce8b2c Mon Sep 17 00:00:00 2001 From: Charandas <542168+charandas@users.noreply.github.com> Date: Fri, 12 Dec 2025 05:01:03 -0800 Subject: [PATCH] fix: Add panic for nil authorizer in installer (#115186) --- pkg/registry/apis/service/register.go | 1 + pkg/registry/apps/correlations/register.go | 7 ++++ pkg/registry/apps/playlist/register.go | 8 +++++ pkg/registry/apps/quotas/register.go | 7 ++++ .../apiserver/appinstaller/installer.go | 2 ++ .../apiserver/appinstaller/installer_test.go | 33 ++++++++++++++----- .../apiserver/auth/authorizer/authorizer.go | 8 ++--- .../apiserver/auth/authorizer/role.go | 1 + 8 files changed, 55 insertions(+), 12 deletions(-) diff --git a/pkg/registry/apis/service/register.go b/pkg/registry/apis/service/register.go index 1002bff5bf6..db909b08777 100644 --- a/pkg/registry/apis/service/register.go +++ b/pkg/registry/apis/service/register.go @@ -38,6 +38,7 @@ func RegisterAPIService(features featuremgmt.FeatureToggles, apiregistration bui } func (b *ServiceAPIBuilder) GetAuthorizer() authorizer.Authorizer { + //nolint:staticcheck // not yet migrated to Resource Authorizer return roleauthorizer.NewRoleAuthorizer() } diff --git a/pkg/registry/apps/correlations/register.go b/pkg/registry/apps/correlations/register.go index 757af68a8ce..2a3b1f0bde5 100644 --- a/pkg/registry/apps/correlations/register.go +++ b/pkg/registry/apps/correlations/register.go @@ -5,6 +5,7 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/authorization/authorizer" restclient "k8s.io/client-go/rest" "github.com/grafana/grafana-app-sdk/app" @@ -16,6 +17,7 @@ import ( "github.com/grafana/grafana/pkg/apimachinery/utils" "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/services/apiserver/appinstaller" + roleauthorizer "github.com/grafana/grafana/pkg/services/apiserver/auth/authorizer" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" "github.com/grafana/grafana/pkg/services/correlations" "github.com/grafana/grafana/pkg/services/featuremgmt" @@ -60,6 +62,11 @@ func RegisterAppInstaller( return installer, nil } +func (a *AppInstaller) GetAuthorizer() authorizer.Authorizer { + //nolint:staticcheck // not yet migrated to Resource Authorizer + return roleauthorizer.NewRoleAuthorizer() +} + func (a *AppInstaller) GetLegacyStorage(requested schema.GroupVersionResource) rest.Storage { kind := correlationsV0.CorrelationKind() gvr := schema.GroupVersionResource{ diff --git a/pkg/registry/apps/playlist/register.go b/pkg/registry/apps/playlist/register.go index 52bbc0210f9..336270098e4 100644 --- a/pkg/registry/apps/playlist/register.go +++ b/pkg/registry/apps/playlist/register.go @@ -6,17 +6,20 @@ import ( metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apiserver/pkg/authorization/authorizer" restclient "k8s.io/client-go/rest" "github.com/grafana/grafana-app-sdk/app" appsdkapiserver "github.com/grafana/grafana-app-sdk/k8s/apiserver" "github.com/grafana/grafana-app-sdk/simple" + "github.com/grafana/grafana/apps/playlist/pkg/apis" playlistv0alpha1 "github.com/grafana/grafana/apps/playlist/pkg/apis/playlist/v0alpha1" playlistapp "github.com/grafana/grafana/apps/playlist/pkg/app" "github.com/grafana/grafana/pkg/apimachinery/utils" grafanarest "github.com/grafana/grafana/pkg/apiserver/rest" "github.com/grafana/grafana/pkg/services/apiserver/appinstaller" + roleauthorizer "github.com/grafana/grafana/pkg/services/apiserver/auth/authorizer" "github.com/grafana/grafana/pkg/services/apiserver/endpoints/request" "github.com/grafana/grafana/pkg/services/featuremgmt" playlistsvc "github.com/grafana/grafana/pkg/services/playlist" @@ -63,6 +66,11 @@ func RegisterAppInstaller( return installer, nil } +func (p *PlaylistAppInstaller) GetAuthorizer() authorizer.Authorizer { + //nolint:staticcheck // not yet migrated to Resource Authorizer + return roleauthorizer.NewRoleAuthorizer() +} + // GetLegacyStorage returns the legacy storage for the playlist app. func (p *PlaylistAppInstaller) GetLegacyStorage(requested schema.GroupVersionResource) grafanarest.Storage { gvr := playlistv0alpha1.PlaylistKind().GroupVersionResource() diff --git a/pkg/registry/apps/quotas/register.go b/pkg/registry/apps/quotas/register.go index b81d4fef5cf..c6b33bc4d00 100644 --- a/pkg/registry/apps/quotas/register.go +++ b/pkg/registry/apps/quotas/register.go @@ -3,12 +3,14 @@ package quotas import ( "github.com/grafana/grafana/apps/quotas/pkg/apis" "github.com/grafana/grafana/pkg/storage/unified/resource" + "k8s.io/apiserver/pkg/authorization/authorizer" restclient "k8s.io/client-go/rest" "github.com/grafana/grafana-app-sdk/app" appsdkapiserver "github.com/grafana/grafana-app-sdk/k8s/apiserver" "github.com/grafana/grafana-app-sdk/simple" quotasapp "github.com/grafana/grafana/apps/quotas/pkg/app" + roleauthorizer "github.com/grafana/grafana/pkg/services/apiserver/auth/authorizer" "github.com/grafana/grafana/pkg/services/featuremgmt" "github.com/grafana/grafana/pkg/setting" ) @@ -22,6 +24,11 @@ type QuotasAppInstaller struct { cfg *setting.Cfg } +func (a *QuotasAppInstaller) GetAuthorizer() authorizer.Authorizer { + //nolint:staticcheck // not yet migrated to Resource Authorizer + return roleauthorizer.NewRoleAuthorizer() +} + func RegisterAppInstaller( cfg *setting.Cfg, features featuremgmt.FeatureToggles, diff --git a/pkg/services/apiserver/appinstaller/installer.go b/pkg/services/apiserver/appinstaller/installer.go index c7b926bd5f3..d282b42f547 100644 --- a/pkg/services/apiserver/appinstaller/installer.go +++ b/pkg/services/apiserver/appinstaller/installer.go @@ -114,6 +114,8 @@ func RegisterAuthorizers( registrar.Register(gv, authorizer) logger.Debug("Registered authorizer", "group", gv.Group, "version", gv.Version, "app") } + } else { + panic("authorizer cannot be nil for api group: " + installer.GroupVersions()[0].Group) } } } diff --git a/pkg/services/apiserver/appinstaller/installer_test.go b/pkg/services/apiserver/appinstaller/installer_test.go index 89c4bfa0164..53c2d2dbedf 100644 --- a/pkg/services/apiserver/appinstaller/installer_test.go +++ b/pkg/services/apiserver/appinstaller/installer_test.go @@ -15,6 +15,7 @@ func TestRegisterAuthorizers(t *testing.T) { name string appInstallers []appsdkapiserver.AppInstaller expectedRegisters int + expectedPanic bool }{ { name: "empty installers list", @@ -30,7 +31,7 @@ func TestRegisterAuthorizers(t *testing.T) { }, }, }, - expectedRegisters: 0, + expectedPanic: true, }, { name: "single installer with authorizer provider", @@ -46,6 +47,20 @@ func TestRegisterAuthorizers(t *testing.T) { }, expectedRegisters: 1, }, + { + name: "single installer with invalid authorizer provider", + appInstallers: []appsdkapiserver.AppInstaller{ + &mockAppInstallerWithAuth{ + mockAppInstaller: &mockAppInstaller{ + groupVersions: []schema.GroupVersion{ + {Group: "test.example.com", Version: "v1"}, + }, + }, + mockAuthorizer: nil, + }, + }, + expectedPanic: true, + }, { name: "installer with multiple group versions", appInstallers: []appsdkapiserver.AppInstaller{ @@ -63,7 +78,7 @@ func TestRegisterAuthorizers(t *testing.T) { expectedRegisters: 3, }, { - name: "multiple installers with mixed authorizer support", + name: "multiple installers with authorizer support", appInstallers: []appsdkapiserver.AppInstaller{ &mockAppInstallerWithAuth{ mockAppInstaller: &mockAppInstaller{ @@ -73,11 +88,6 @@ func TestRegisterAuthorizers(t *testing.T) { }, mockAuthorizer: &mockAuthorizer{}, }, - &mockAppInstaller{ - groupVersions: []schema.GroupVersion{ - {Group: "other.example.com", Version: "v1"}, - }, - }, &mockAppInstallerWithAuth{ mockAppInstaller: &mockAppInstaller{ groupVersions: []schema.GroupVersion{ @@ -88,7 +98,7 @@ func TestRegisterAuthorizers(t *testing.T) { mockAuthorizer: &mockAuthorizer{}, }, }, - expectedRegisters: 3, // 1 from first installer + 2 from third installer + expectedRegisters: 3, // 1 from first installer + 2 from second installer }, } @@ -96,6 +106,13 @@ func TestRegisterAuthorizers(t *testing.T) { t.Run(tt.name, func(t *testing.T) { ctx := context.Background() registrar := &mockAuthorizerRegistrar{} + if tt.expectedPanic { + defer func() { + if r := recover(); r == nil { + t.Errorf("%s case did not panic as expected", t.Name()) + } + }() + } RegisterAuthorizers(ctx, tt.appInstallers, registrar) require.Equal(t, tt.expectedRegisters, len(registrar.registrations)) }) diff --git a/pkg/services/apiserver/auth/authorizer/authorizer.go b/pkg/services/apiserver/auth/authorizer/authorizer.go index f58c1d14bf8..dab8167deb0 100644 --- a/pkg/services/apiserver/auth/authorizer/authorizer.go +++ b/pkg/services/apiserver/auth/authorizer/authorizer.go @@ -38,12 +38,12 @@ func NewGrafanaBuiltInSTAuthorizer(cfg *setting.Cfg) *GrafanaAuthorizer { // Individual services may have explicit implementations apis := make(map[string]authorizer.Authorizer) + // The apiVersion flavors will run first and can return early when FGAC has appropriate rules authorizers = append(authorizers, &authorizerForAPI{apis}) - // org role is last -- and will return allow for verbs that match expectations - // The apiVersion flavors will run first and can return early when FGAC has appropriate rules - // NOTE: role authorizer is now used by some api groups as their specific authorizer - // but there are still some apis not directly registered in the embedded delegate that benefit from including it here + // org role authorizer is last -- and will return allow for verbs that match expectations + // it is only helpful here for remote APIs in some cloud use-cases. + //nolint:staticcheck // remove once build handler chains are untangled between local and remote APIs handling authorizers = append(authorizers, NewRoleAuthorizer()) return &GrafanaAuthorizer{ apis: apis, diff --git a/pkg/services/apiserver/auth/authorizer/role.go b/pkg/services/apiserver/auth/authorizer/role.go index 23164dbf556..e8e70dd01c8 100644 --- a/pkg/services/apiserver/auth/authorizer/role.go +++ b/pkg/services/apiserver/auth/authorizer/role.go @@ -19,6 +19,7 @@ var orgRoleNoneAsViewerAPIGroups = []string{ type roleAuthorizer struct{} +// Deprecated: NewRoleAuthorizer exists for apps that were launched with simplistic authorization requirements. Consider using NewResourceAuthorizer instead. func NewRoleAuthorizer() *roleAuthorizer { return &roleAuthorizer{} }