From 96451f948bf8d723845567d2a26e7ecffda483ba Mon Sep 17 00:00:00 2001 From: Roberto Jimenez Sanchez Date: Thu, 18 Dec 2025 18:34:44 +0100 Subject: [PATCH] refactor: merge access checkers with original fallthrough behavior Merge tokenAccessChecker and sessionAccessChecker into a unified access checker that implements the original fallthrough behavior: 1. First try to get identity from access token (authlib.AuthInfoFrom) 2. If token exists AND (is TypeAccessPolicy OR useExclusivelyAccessCheckerForAuthz), use the access checker with token identity 3. If no token or conditions not met, fall back to session identity (identity.GetRequester) with optional role-based fallback This fixes the issue where settings/stats/connections endpoints were failing in MT mode because the tokenAccessChecker was returning an error when there was no auth info in context, instead of falling through to session-based authorization. The unified checker now properly handles: - MT mode: tries token first, falls back to session if no token - ST mode: only uses token for AccessPolicy identities, otherwise session - Role fallback: applies when configured and access checker denies --- .../pkg/auth/session_access_checker.go | 88 ------ .../pkg/auth/session_access_checker_test.go | 244 --------------- .../pkg/auth/token_access_checker.go | 57 ---- .../pkg/auth/token_access_checker_test.go | 137 --------- .../pkg/auth/unified_access_checker.go | 137 +++++++++ .../pkg/auth/unified_access_checker_test.go | 282 ++++++++++++++++++ pkg/registry/apis/provisioning/register.go | 10 +- 7 files changed, 422 insertions(+), 533 deletions(-) delete mode 100644 apps/provisioning/pkg/auth/session_access_checker.go delete mode 100644 apps/provisioning/pkg/auth/session_access_checker_test.go delete mode 100644 apps/provisioning/pkg/auth/token_access_checker.go delete mode 100644 apps/provisioning/pkg/auth/token_access_checker_test.go create mode 100644 apps/provisioning/pkg/auth/unified_access_checker.go create mode 100644 apps/provisioning/pkg/auth/unified_access_checker_test.go diff --git a/apps/provisioning/pkg/auth/session_access_checker.go b/apps/provisioning/pkg/auth/session_access_checker.go deleted file mode 100644 index 00348ef8e59..00000000000 --- a/apps/provisioning/pkg/auth/session_access_checker.go +++ /dev/null @@ -1,88 +0,0 @@ -package auth - -import ( - "context" - "fmt" - "strings" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" - - authlib "github.com/grafana/authlib/types" - "github.com/grafana/grafana/pkg/apimachinery/identity" -) - -// sessionAccessChecker implements AccessChecker using Grafana session identity. -type sessionAccessChecker struct { - inner authlib.AccessChecker - fallbackRole identity.RoleType -} - -// NewSessionAccessChecker creates an AccessChecker that gets identity from Grafana -// sessions via GetRequester(ctx). Supports optional role-based fallback via -// WithFallbackRole for backwards compatibility. -func NewSessionAccessChecker(inner authlib.AccessChecker) AccessChecker { - return &sessionAccessChecker{ - inner: inner, - fallbackRole: "", - } -} - -// WithFallbackRole returns a new AccessChecker with the specified fallback role. -func (c *sessionAccessChecker) WithFallbackRole(role identity.RoleType) AccessChecker { - return &sessionAccessChecker{ - inner: c.inner, - fallbackRole: role, - } -} - -// Check performs an access check with optional role-based fallback. -// Returns nil if access is allowed, or an appropriate API error if denied. -func (c *sessionAccessChecker) Check(ctx context.Context, req authlib.CheckRequest, folder string) error { - // Get identity from Grafana session - requester, err := identity.GetRequester(ctx) - if err != nil { - return apierrors.NewUnauthorized(fmt.Sprintf("failed to get requester: %v", err)) - } - - // Fill in namespace from identity if not provided - if req.Namespace == "" { - req.Namespace = requester.GetNamespace() - } - - // Perform the access check - rsp, err := c.inner.Check(ctx, requester, req, folder) - - // Build the GroupResource for error messages - gr := schema.GroupResource{Group: req.Group, Resource: req.Resource} - - // No fallback configured, return result directly - if c.fallbackRole == "" { - if err != nil { - return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("access check failed: %w", err)) - } - if !rsp.Allowed { - return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("permission denied")) - } - return nil - } - - // Fallback is configured - apply fallback logic - if err != nil { - if requester.GetOrgRole().Includes(c.fallbackRole) { - return nil // Fallback succeeded - } - return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("access check failed: %w", err)) - } - - if rsp.Allowed { - return nil - } - - // Fall back to role for backwards compatibility - if requester.GetOrgRole().Includes(c.fallbackRole) { - return nil // Fallback succeeded - } - - return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("%s role is required", strings.ToLower(string(c.fallbackRole)))) -} diff --git a/apps/provisioning/pkg/auth/session_access_checker_test.go b/apps/provisioning/pkg/auth/session_access_checker_test.go deleted file mode 100644 index 1e99a6e46db..00000000000 --- a/apps/provisioning/pkg/auth/session_access_checker_test.go +++ /dev/null @@ -1,244 +0,0 @@ -package auth - -import ( - "context" - "errors" - "testing" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - - authlib "github.com/grafana/authlib/types" - "github.com/grafana/grafana/pkg/apimachinery/identity" - "github.com/grafana/grafana/pkg/services/user" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -// mockRequester implements identity.Requester for testing. -type mockRequester struct { - identity.Requester - orgRole identity.RoleType - identityType authlib.IdentityType - namespace string -} - -func (m *mockRequester) GetOrgRole() identity.RoleType { - return m.orgRole -} - -func (m *mockRequester) GetIdentityType() authlib.IdentityType { - return m.identityType -} - -func (m *mockRequester) GetNamespace() string { - return m.namespace -} - -func TestSessionAccessChecker_Check(t *testing.T) { - ctx := context.Background() - req := authlib.CheckRequest{ - Verb: "get", - Group: "provisioning.grafana.app", - Resource: "repositories", - Name: "test-repo", - Namespace: "default", - } - - tests := []struct { - name string - fallbackRole identity.RoleType - innerResponse authlib.CheckResponse - innerErr error - requester *mockRequester - expectAllow bool - }{ - { - name: "allowed by checker", - fallbackRole: identity.RoleAdmin, - innerResponse: authlib.CheckResponse{Allowed: true}, - requester: &mockRequester{orgRole: identity.RoleViewer, identityType: authlib.TypeUser}, - expectAllow: true, - }, - { - name: "denied by checker, fallback to admin role succeeds", - fallbackRole: identity.RoleAdmin, - innerResponse: authlib.CheckResponse{Allowed: false}, - requester: &mockRequester{orgRole: identity.RoleAdmin, identityType: authlib.TypeUser}, - expectAllow: true, - }, - { - name: "denied by checker, fallback to admin role fails for viewer", - fallbackRole: identity.RoleAdmin, - innerResponse: authlib.CheckResponse{Allowed: false}, - requester: &mockRequester{orgRole: identity.RoleViewer, identityType: authlib.TypeUser}, - expectAllow: false, - }, - { - name: "error from checker, fallback to admin role succeeds", - fallbackRole: identity.RoleAdmin, - innerErr: errors.New("access check failed"), - requester: &mockRequester{orgRole: identity.RoleAdmin, identityType: authlib.TypeUser}, - expectAllow: true, - }, - { - name: "error from checker, fallback fails for viewer", - fallbackRole: identity.RoleAdmin, - innerErr: errors.New("access check failed"), - requester: &mockRequester{orgRole: identity.RoleViewer, identityType: authlib.TypeUser}, - expectAllow: false, - }, - { - name: "denied, editor fallback succeeds for editor", - fallbackRole: identity.RoleEditor, - innerResponse: authlib.CheckResponse{Allowed: false}, - requester: &mockRequester{orgRole: identity.RoleEditor, identityType: authlib.TypeUser}, - expectAllow: true, - }, - { - name: "denied, editor fallback fails for viewer", - fallbackRole: identity.RoleEditor, - innerResponse: authlib.CheckResponse{Allowed: false}, - requester: &mockRequester{orgRole: identity.RoleViewer, identityType: authlib.TypeUser}, - expectAllow: false, - }, - { - name: "no fallback configured, denied stays denied", - fallbackRole: "", // no fallback - innerResponse: authlib.CheckResponse{Allowed: false}, - requester: &mockRequester{orgRole: identity.RoleAdmin, identityType: authlib.TypeUser}, - expectAllow: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: tt.innerResponse, - err: tt.innerErr, - } - - checker := NewSessionAccessChecker(mock) - if tt.fallbackRole != "" { - checker = checker.WithFallbackRole(tt.fallbackRole) - } - - // Add requester to context - testCtx := identity.WithRequester(ctx, tt.requester) - - err := checker.Check(testCtx, req, "") - - if tt.expectAllow { - require.NoError(t, err) - } else { - require.Error(t, err) - assert.True(t, apierrors.IsForbidden(err), "expected Forbidden error, got: %v", err) - } - }) - } -} - -func TestSessionAccessChecker_NoRequester(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: true}, - } - - checker := NewSessionAccessChecker(mock) - err := checker.Check(context.Background(), authlib.CheckRequest{}, "") - - require.Error(t, err) - assert.True(t, apierrors.IsUnauthorized(err), "expected Unauthorized error") -} - -func TestSessionAccessChecker_WithFallbackRole_ImmutableOriginal(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: false}, - } - - original := NewSessionAccessChecker(mock) - withAdmin := original.WithFallbackRole(identity.RoleAdmin) - withEditor := original.WithFallbackRole(identity.RoleEditor) - - ctx := identity.WithRequester(context.Background(), &mockRequester{ - orgRole: identity.RoleEditor, - identityType: authlib.TypeUser, - }) - - req := authlib.CheckRequest{} - - // Original should deny (no fallback) - err := original.Check(ctx, req, "") - require.Error(t, err, "original should deny without fallback") - - // WithAdmin should deny for editor - err = withAdmin.Check(ctx, req, "") - require.Error(t, err, "admin fallback should deny for editor") - - // WithEditor should allow for editor - err = withEditor.Check(ctx, req, "") - require.NoError(t, err, "editor fallback should allow for editor") -} - -func TestSessionAccessChecker_WithFallbackRole_ChainedCalls(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: false}, - } - - // Ensure chained WithFallbackRole calls work correctly - checker := NewSessionAccessChecker(mock). - WithFallbackRole(identity.RoleAdmin). - WithFallbackRole(identity.RoleEditor) // This should override admin - - ctx := identity.WithRequester(context.Background(), &mockRequester{ - orgRole: identity.RoleEditor, - identityType: authlib.TypeUser, - }) - - err := checker.Check(ctx, authlib.CheckRequest{}, "") - require.NoError(t, err, "last fallback (editor) should be used") -} - -func TestSessionAccessChecker_RealSignedInUser(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: false}, - } - - checker := NewSessionAccessChecker(mock).WithFallbackRole(identity.RoleAdmin) - - // Use a real SignedInUser - signedInUser := &user.SignedInUser{ - UserID: 1, - OrgID: 1, - OrgRole: identity.RoleAdmin, - } - - ctx := identity.WithRequester(context.Background(), signedInUser) - - err := checker.Check(ctx, authlib.CheckRequest{}, "") - require.NoError(t, err, "admin user should be allowed via fallback") -} - -func TestSessionAccessChecker_FillsNamespace(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: true}, - } - - checker := NewSessionAccessChecker(mock) - - ctx := identity.WithRequester(context.Background(), &mockRequester{ - orgRole: identity.RoleAdmin, - identityType: authlib.TypeUser, - namespace: "org-123", - }) - - // Request without namespace - req := authlib.CheckRequest{ - Verb: "get", - Group: "provisioning.grafana.app", - Resource: "repositories", - Name: "test-repo", - // Namespace intentionally empty - } - - err := checker.Check(ctx, req, "") - require.NoError(t, err) -} diff --git a/apps/provisioning/pkg/auth/token_access_checker.go b/apps/provisioning/pkg/auth/token_access_checker.go deleted file mode 100644 index d2c6f9e1c5b..00000000000 --- a/apps/provisioning/pkg/auth/token_access_checker.go +++ /dev/null @@ -1,57 +0,0 @@ -package auth - -import ( - "context" - "fmt" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/runtime/schema" - - authlib "github.com/grafana/authlib/types" - "github.com/grafana/grafana/pkg/apimachinery/identity" -) - -// tokenAccessChecker implements AccessChecker using access tokens from context. -type tokenAccessChecker struct { - inner authlib.AccessChecker -} - -// NewTokenAccessChecker creates an AccessChecker that gets identity from access tokens -// via AuthInfoFrom(ctx). Role-based fallback is not supported. -func NewTokenAccessChecker(inner authlib.AccessChecker) AccessChecker { - return &tokenAccessChecker{inner: inner} -} - -// WithFallbackRole returns the same checker since fallback is not supported. -func (c *tokenAccessChecker) WithFallbackRole(_ identity.RoleType) AccessChecker { - return c -} - -// Check performs an access check using AuthInfo from context. -// Returns nil if access is allowed, or an appropriate API error if denied. -func (c *tokenAccessChecker) Check(ctx context.Context, req authlib.CheckRequest, folder string) error { - // Get identity from access token in context - id, ok := authlib.AuthInfoFrom(ctx) - if !ok { - return apierrors.NewUnauthorized("no auth info in context") - } - - // Fill in namespace from identity if not provided - if req.Namespace == "" { - req.Namespace = id.GetNamespace() - } - - // Perform the access check - rsp, err := c.inner.Check(ctx, id, req, folder) - - // Build the GroupResource for error messages - gr := schema.GroupResource{Group: req.Group, Resource: req.Resource} - - if err != nil { - return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("access check failed: %w", err)) - } - if !rsp.Allowed { - return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("permission denied")) - } - return nil -} diff --git a/apps/provisioning/pkg/auth/token_access_checker_test.go b/apps/provisioning/pkg/auth/token_access_checker_test.go deleted file mode 100644 index bce0d3a77a0..00000000000 --- a/apps/provisioning/pkg/auth/token_access_checker_test.go +++ /dev/null @@ -1,137 +0,0 @@ -package auth - -import ( - "context" - "errors" - "testing" - - apierrors "k8s.io/apimachinery/pkg/api/errors" - - authlib "github.com/grafana/authlib/types" - "github.com/grafana/grafana/pkg/apimachinery/identity" - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" -) - -func TestTokenAccessChecker_Check(t *testing.T) { - req := authlib.CheckRequest{ - Verb: "get", - Group: "provisioning.grafana.app", - Resource: "repositories", - Name: "test-repo", - Namespace: "default", - } - - tests := []struct { - name string - innerResponse authlib.CheckResponse - innerErr error - authInfo *identity.StaticRequester - expectAllow bool - }{ - { - name: "allowed by checker", - innerResponse: authlib.CheckResponse{Allowed: true}, - authInfo: &identity.StaticRequester{Type: authlib.TypeUser}, - expectAllow: true, - }, - { - name: "denied by checker", - innerResponse: authlib.CheckResponse{Allowed: false}, - authInfo: &identity.StaticRequester{Type: authlib.TypeUser}, - expectAllow: false, - }, - { - name: "error from checker", - innerErr: errors.New("access check failed"), - authInfo: &identity.StaticRequester{Type: authlib.TypeUser}, - expectAllow: false, - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: tt.innerResponse, - err: tt.innerErr, - } - - checker := NewTokenAccessChecker(mock) - - // Add auth info to context - testCtx := authlib.WithAuthInfo(context.Background(), tt.authInfo) - - err := checker.Check(testCtx, req, "") - - if tt.expectAllow { - require.NoError(t, err) - } else { - require.Error(t, err) - assert.True(t, apierrors.IsForbidden(err), "expected Forbidden error, got: %v", err) - } - }) - } -} - -func TestTokenAccessChecker_NoAuthInfo(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: true}, - } - - checker := NewTokenAccessChecker(mock) - err := checker.Check(context.Background(), authlib.CheckRequest{}, "") - - require.Error(t, err) - assert.True(t, apierrors.IsUnauthorized(err), "expected Unauthorized error") -} - -func TestTokenAccessChecker_WithFallbackRole_IsNoOp(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: false}, - } - - checker := NewTokenAccessChecker(mock) - checkerWithFallback := checker.WithFallbackRole(identity.RoleAdmin) - - // They should be the same instance - assert.Same(t, checker, checkerWithFallback, "WithFallbackRole should return same instance") -} - -func TestTokenAccessChecker_FillsNamespace(t *testing.T) { - mock := &mockInnerAccessChecker{ - response: authlib.CheckResponse{Allowed: true}, - } - - checker := NewTokenAccessChecker(mock) - - ctx := authlib.WithAuthInfo(context.Background(), &identity.StaticRequester{ - Type: authlib.TypeUser, - Namespace: "org-123", - }) - - // Request without namespace - req := authlib.CheckRequest{ - Verb: "get", - Group: "provisioning.grafana.app", - Resource: "repositories", - Name: "test-repo", - // Namespace intentionally empty - } - - err := checker.Check(ctx, req, "") - require.NoError(t, err) -} - -// mockInnerAccessChecker implements authlib.AccessChecker for testing. -type mockInnerAccessChecker struct { - response authlib.CheckResponse - err error -} - -func (m *mockInnerAccessChecker) Check(_ context.Context, _ authlib.AuthInfo, _ authlib.CheckRequest, _ string) (authlib.CheckResponse, error) { - return m.response, m.err -} - -func (m *mockInnerAccessChecker) Compile(_ context.Context, _ authlib.AuthInfo, _ authlib.ListRequest) (authlib.ItemChecker, authlib.Zookie, error) { - return nil, nil, nil -} diff --git a/apps/provisioning/pkg/auth/unified_access_checker.go b/apps/provisioning/pkg/auth/unified_access_checker.go new file mode 100644 index 00000000000..d62f8ecdb45 --- /dev/null +++ b/apps/provisioning/pkg/auth/unified_access_checker.go @@ -0,0 +1,137 @@ +package auth + +import ( + "context" + "fmt" + "strings" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/runtime/schema" + + authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/apimachinery/identity" +) + +// unifiedAccessChecker implements AccessChecker with the original fallthrough behavior: +// 1. First try to get identity from access token (authlib.AuthInfoFrom) +// 2. If token exists and conditions are met, use the access checker with token identity +// 3. If no token, fall back to session identity (identity.GetRequester) +// 4. Apply role-based fallback if configured +type unifiedAccessChecker struct { + inner authlib.AccessChecker + fallbackRole identity.RoleType + useExclusivelyAccessCheckerForAuthz bool +} + +// NewAccessChecker creates an AccessChecker that implements the original fallthrough behavior. +// +// When useExclusivelyAccessCheckerForAuthz is true (MT mode), it will: +// - Try to get identity from access token first +// - If token exists, use the access checker +// - If no token, fall back to session identity +// +// When useExclusivelyAccessCheckerForAuthz is false (ST mode), it will: +// - Try to get identity from access token first +// - If token exists AND is TypeAccessPolicy, use the access checker +// - Otherwise, fall back to session identity with role-based fallback +func NewAccessChecker(inner authlib.AccessChecker, useExclusivelyAccessCheckerForAuthz bool) AccessChecker { + return &unifiedAccessChecker{ + inner: inner, + fallbackRole: "", + useExclusivelyAccessCheckerForAuthz: useExclusivelyAccessCheckerForAuthz, + } +} + +// WithFallbackRole returns a new AccessChecker with the specified fallback role. +// The fallback role is applied when the access checker denies access but the user +// has the required org role. This is primarily used in ST mode for backwards compatibility. +func (c *unifiedAccessChecker) WithFallbackRole(role identity.RoleType) AccessChecker { + return &unifiedAccessChecker{ + inner: c.inner, + fallbackRole: role, + useExclusivelyAccessCheckerForAuthz: c.useExclusivelyAccessCheckerForAuthz, + } +} + +// Check performs an access check with the original fallthrough behavior. +// Returns nil if access is allowed, or an appropriate API error if denied. +func (c *unifiedAccessChecker) Check(ctx context.Context, req authlib.CheckRequest, folder string) error { + gr := schema.GroupResource{Group: req.Group, Resource: req.Resource} + + // First try: get identity from access token + if info, ok := authlib.AuthInfoFrom(ctx); ok { + // When running as standalone API server, the identity type may not always match TypeAccessPolicy + // so we allow it to use the access checker if there is any auth info available + if authlib.IsIdentityType(info.GetIdentityType(), authlib.TypeAccessPolicy) || c.useExclusivelyAccessCheckerForAuthz { + return c.checkWithAuthInfo(ctx, info, req, folder, gr) + } + } + + // Fallback: get identity from Grafana session + requester, err := identity.GetRequester(ctx) + if err != nil { + return apierrors.NewUnauthorized(fmt.Sprintf("no identity in context: %v", err)) + } + + return c.checkWithRequester(ctx, requester, req, folder, gr) +} + +// checkWithAuthInfo performs access check using AuthInfo from access token. +func (c *unifiedAccessChecker) checkWithAuthInfo(ctx context.Context, info authlib.AuthInfo, req authlib.CheckRequest, folder string, gr schema.GroupResource) error { + // Fill in namespace from identity if not provided + if req.Namespace == "" { + req.Namespace = info.GetNamespace() + } + + // Perform the access check + rsp, err := c.inner.Check(ctx, info, req, folder) + + if err != nil { + return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("access check failed: %w", err)) + } + if !rsp.Allowed { + return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("permission denied")) + } + return nil +} + +// checkWithRequester performs access check using Requester from session with optional role fallback. +func (c *unifiedAccessChecker) checkWithRequester(ctx context.Context, requester identity.Requester, req authlib.CheckRequest, folder string, gr schema.GroupResource) error { + // Fill in namespace from identity if not provided + if req.Namespace == "" { + req.Namespace = requester.GetNamespace() + } + + // Perform the access check + rsp, err := c.inner.Check(ctx, requester, req, folder) + + // No fallback configured, return result directly + if c.fallbackRole == "" { + if err != nil { + return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("access check failed: %w", err)) + } + if !rsp.Allowed { + return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("permission denied")) + } + return nil + } + + // Fallback is configured - apply fallback logic + if err != nil { + if requester.GetOrgRole().Includes(c.fallbackRole) { + return nil // Fallback succeeded + } + return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("access check failed: %w", err)) + } + + if rsp.Allowed { + return nil + } + + // Fall back to role for backwards compatibility + if requester.GetOrgRole().Includes(c.fallbackRole) { + return nil // Fallback succeeded + } + + return apierrors.NewForbidden(gr, req.Name, fmt.Errorf("%s role is required", strings.ToLower(string(c.fallbackRole)))) +} diff --git a/apps/provisioning/pkg/auth/unified_access_checker_test.go b/apps/provisioning/pkg/auth/unified_access_checker_test.go new file mode 100644 index 00000000000..1388c500b42 --- /dev/null +++ b/apps/provisioning/pkg/auth/unified_access_checker_test.go @@ -0,0 +1,282 @@ +package auth + +import ( + "context" + "errors" + "testing" + + apierrors "k8s.io/apimachinery/pkg/api/errors" + + authlib "github.com/grafana/authlib/types" + "github.com/grafana/grafana/pkg/apimachinery/identity" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockInnerAccessChecker is a mock implementation of authlib.AccessChecker for testing. +type mockInnerAccessChecker struct { + response authlib.CheckResponse + err error +} + +func (m *mockInnerAccessChecker) Check(_ context.Context, _ authlib.AuthInfo, _ authlib.CheckRequest, _ string) (authlib.CheckResponse, error) { + return m.response, m.err +} + +func (m *mockInnerAccessChecker) Compile(_ context.Context, _ authlib.AuthInfo, _ authlib.ListRequest) (authlib.ItemChecker, authlib.Zookie, error) { + return nil, authlib.NoopZookie{}, nil +} + +func TestUnifiedAccessChecker_WithTokenIdentity(t *testing.T) { + req := authlib.CheckRequest{ + Verb: "get", + Group: "provisioning.grafana.app", + Resource: "repositories", + Name: "test-repo", + Namespace: "default", + } + + tests := []struct { + name string + innerResponse authlib.CheckResponse + innerErr error + useExclusivelyAccessCheckerForAuthz bool + authInfo *identity.StaticRequester + expectAllow bool + }{ + { + name: "allowed by checker with token in MT mode", + innerResponse: authlib.CheckResponse{Allowed: true}, + useExclusivelyAccessCheckerForAuthz: true, + authInfo: &identity.StaticRequester{Type: authlib.TypeUser}, + expectAllow: true, + }, + { + name: "denied by checker with token in MT mode", + innerResponse: authlib.CheckResponse{Allowed: false}, + useExclusivelyAccessCheckerForAuthz: true, + authInfo: &identity.StaticRequester{Type: authlib.TypeUser}, + expectAllow: false, + }, + { + name: "allowed by checker with AccessPolicy in ST mode", + innerResponse: authlib.CheckResponse{Allowed: true}, + useExclusivelyAccessCheckerForAuthz: false, + authInfo: &identity.StaticRequester{Type: authlib.TypeAccessPolicy}, + expectAllow: true, + }, + { + name: "error from checker", + innerErr: errors.New("access check failed"), + useExclusivelyAccessCheckerForAuthz: true, + authInfo: &identity.StaticRequester{Type: authlib.TypeUser}, + expectAllow: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockInnerAccessChecker{ + response: tt.innerResponse, + err: tt.innerErr, + } + + checker := NewAccessChecker(mock, tt.useExclusivelyAccessCheckerForAuthz) + + // Add auth info to context + testCtx := authlib.WithAuthInfo(context.Background(), tt.authInfo) + + err := checker.Check(testCtx, req, "") + + if tt.expectAllow { + require.NoError(t, err) + } else { + require.Error(t, err) + assert.True(t, apierrors.IsForbidden(err), "expected Forbidden error, got: %v", err) + } + }) + } +} + +func TestUnifiedAccessChecker_WithSessionIdentity(t *testing.T) { + req := authlib.CheckRequest{ + Verb: "get", + Group: "provisioning.grafana.app", + Resource: "repositories", + Name: "test-repo", + Namespace: "default", + } + + tests := []struct { + name string + innerResponse authlib.CheckResponse + innerErr error + fallbackRole identity.RoleType + requesterRole identity.RoleType + expectAllow bool + }{ + { + name: "allowed by checker without fallback", + innerResponse: authlib.CheckResponse{Allowed: true}, + fallbackRole: "", + requesterRole: identity.RoleViewer, + expectAllow: true, + }, + { + name: "denied by checker without fallback", + innerResponse: authlib.CheckResponse{Allowed: false}, + fallbackRole: "", + requesterRole: identity.RoleViewer, + expectAllow: false, + }, + { + name: "denied by checker but allowed by admin fallback", + innerResponse: authlib.CheckResponse{Allowed: false}, + fallbackRole: identity.RoleAdmin, + requesterRole: identity.RoleAdmin, + expectAllow: true, + }, + { + name: "denied by checker and fallback role not met", + innerResponse: authlib.CheckResponse{Allowed: false}, + fallbackRole: identity.RoleAdmin, + requesterRole: identity.RoleViewer, + expectAllow: false, + }, + { + name: "error from checker but allowed by fallback", + innerErr: errors.New("access check failed"), + fallbackRole: identity.RoleAdmin, + requesterRole: identity.RoleAdmin, + expectAllow: true, + }, + { + name: "error from checker and fallback role not met", + innerErr: errors.New("access check failed"), + fallbackRole: identity.RoleAdmin, + requesterRole: identity.RoleViewer, + expectAllow: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + mock := &mockInnerAccessChecker{ + response: tt.innerResponse, + err: tt.innerErr, + } + + // ST mode (useExclusivelyAccessCheckerForAuthz=false) with non-AccessPolicy identity + // will fall through to session-based check + checker := NewAccessChecker(mock, false) + if tt.fallbackRole != "" { + checker = checker.WithFallbackRole(tt.fallbackRole) + } + + // Create context with session identity (no token auth info) + requester := &identity.StaticRequester{ + Type: authlib.TypeUser, + OrgRole: tt.requesterRole, + } + testCtx := identity.WithRequester(context.Background(), requester) + + err := checker.Check(testCtx, req, "") + + if tt.expectAllow { + require.NoError(t, err) + } else { + require.Error(t, err) + assert.True(t, apierrors.IsForbidden(err), "expected Forbidden error, got: %v", err) + } + }) + } +} + +func TestUnifiedAccessChecker_FallsBackToSessionWhenNoToken(t *testing.T) { + req := authlib.CheckRequest{ + Verb: "get", + Group: "provisioning.grafana.app", + Resource: "repositories", + Name: "test-repo", + Namespace: "default", + } + + mock := &mockInnerAccessChecker{ + response: authlib.CheckResponse{Allowed: true}, + } + + // MT mode but no token in context - should fall back to session + checker := NewAccessChecker(mock, true) + + // Create context with only session identity (no token auth info) + requester := &identity.StaticRequester{ + Type: authlib.TypeUser, + OrgRole: identity.RoleAdmin, + } + testCtx := identity.WithRequester(context.Background(), requester) + + err := checker.Check(testCtx, req, "") + require.NoError(t, err, "should succeed using session identity when no token is present") +} + +func TestUnifiedAccessChecker_NoIdentityReturnsUnauthorized(t *testing.T) { + req := authlib.CheckRequest{ + Verb: "get", + Group: "provisioning.grafana.app", + Resource: "repositories", + Name: "test-repo", + Namespace: "default", + } + + mock := &mockInnerAccessChecker{ + response: authlib.CheckResponse{Allowed: true}, + } + + checker := NewAccessChecker(mock, true) + + // Empty context with no identity + err := checker.Check(context.Background(), req, "") + require.Error(t, err) + assert.True(t, apierrors.IsUnauthorized(err), "expected Unauthorized error, got: %v", err) +} + +func TestUnifiedAccessChecker_WithFallbackRole(t *testing.T) { + mock := &mockInnerAccessChecker{} + checker := NewAccessChecker(mock, false) + + // WithFallbackRole should return a new checker with the role configured + checkerWithAdmin := checker.WithFallbackRole(identity.RoleAdmin) + checkerWithEditor := checker.WithFallbackRole(identity.RoleEditor) + + // They should be different instances + assert.NotEqual(t, checker, checkerWithAdmin) + assert.NotEqual(t, checkerWithAdmin, checkerWithEditor) +} + +func TestUnifiedAccessChecker_FillsNamespaceFromIdentity(t *testing.T) { + mock := &mockInnerAccessChecker{ + response: authlib.CheckResponse{Allowed: true}, + } + + checker := NewAccessChecker(mock, true) + + // Request without namespace + req := authlib.CheckRequest{ + Verb: "get", + Group: "provisioning.grafana.app", + Resource: "repositories", + Name: "test-repo", + // Namespace not set + } + + // Create context with identity that has a namespace + requester := &identity.StaticRequester{ + Type: authlib.TypeUser, + OrgRole: identity.RoleAdmin, + Namespace: "stacks-123", + } + testCtx := authlib.WithAuthInfo(context.Background(), requester) + + err := checker.Check(testCtx, req, "") + require.NoError(t, err) +} diff --git a/pkg/registry/apis/provisioning/register.go b/pkg/registry/apis/provisioning/register.go index d211382255e..01888b740cd 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -162,13 +162,9 @@ func NewAPIBuilder( parsers := resources.NewParserFactory(clients) resourceLister := resources.NewResourceListerForMigrations(unified) - // Create access checker based on mode - var accessChecker auth.AccessChecker - if useExclusivelyAccessCheckerForAuthz { - accessChecker = auth.NewTokenAccessChecker(access) - } else { - accessChecker = auth.NewSessionAccessChecker(access) - } + // Create unified access checker that handles both token and session identity + // with the original fallthrough behavior + accessChecker := auth.NewAccessChecker(access, useExclusivelyAccessCheckerForAuthz) b := &APIBuilder{ onlyApiServer: onlyApiServer,