From e79e98257f0c3668cbcc381fe89ba3013a2b8692 Mon Sep 17 00:00:00 2001 From: Charandas Batra Date: Thu, 18 Dec 2025 11:59:55 -0800 Subject: [PATCH] Revert "refactor: merge access checkers with original fallthrough behavior" This reverts commit 96451f948bf8d723845567d2a26e7ecffda483ba. --- .../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, 533 insertions(+), 422 deletions(-) create mode 100644 apps/provisioning/pkg/auth/session_access_checker.go create mode 100644 apps/provisioning/pkg/auth/session_access_checker_test.go create mode 100644 apps/provisioning/pkg/auth/token_access_checker.go create mode 100644 apps/provisioning/pkg/auth/token_access_checker_test.go delete mode 100644 apps/provisioning/pkg/auth/unified_access_checker.go delete 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 new file mode 100644 index 00000000000..00348ef8e59 --- /dev/null +++ b/apps/provisioning/pkg/auth/session_access_checker.go @@ -0,0 +1,88 @@ +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 new file mode 100644 index 00000000000..1e99a6e46db --- /dev/null +++ b/apps/provisioning/pkg/auth/session_access_checker_test.go @@ -0,0 +1,244 @@ +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 new file mode 100644 index 00000000000..d2c6f9e1c5b --- /dev/null +++ b/apps/provisioning/pkg/auth/token_access_checker.go @@ -0,0 +1,57 @@ +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 new file mode 100644 index 00000000000..bce0d3a77a0 --- /dev/null +++ b/apps/provisioning/pkg/auth/token_access_checker_test.go @@ -0,0 +1,137 @@ +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 deleted file mode 100644 index d62f8ecdb45..00000000000 --- a/apps/provisioning/pkg/auth/unified_access_checker.go +++ /dev/null @@ -1,137 +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" -) - -// 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 deleted file mode 100644 index 1388c500b42..00000000000 --- a/apps/provisioning/pkg/auth/unified_access_checker_test.go +++ /dev/null @@ -1,282 +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" -) - -// 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 01888b740cd..d211382255e 100644 --- a/pkg/registry/apis/provisioning/register.go +++ b/pkg/registry/apis/provisioning/register.go @@ -162,9 +162,13 @@ func NewAPIBuilder( parsers := resources.NewParserFactory(clients) resourceLister := resources.NewResourceListerForMigrations(unified) - // Create unified access checker that handles both token and session identity - // with the original fallthrough behavior - accessChecker := auth.NewAccessChecker(access, useExclusivelyAccessCheckerForAuthz) + // Create access checker based on mode + var accessChecker auth.AccessChecker + if useExclusivelyAccessCheckerForAuthz { + accessChecker = auth.NewTokenAccessChecker(access) + } else { + accessChecker = auth.NewSessionAccessChecker(access) + } b := &APIBuilder{ onlyApiServer: onlyApiServer,