fix: Add panic for nil authorizer in installer (#115186)

This commit is contained in:
Charandas
2025-12-12 05:01:03 -08:00
committed by GitHub
parent 7805e18368
commit e525b529a8
8 changed files with 55 additions and 12 deletions
@@ -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)
}
}
}
@@ -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))
})
@@ -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,
@@ -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{}
}