AuthN: Rebuild Authenticate so we only have to call it once in context handler (#61705)

* API: Add reqSignedIn to router groups

* AuthN: Add fall through in context handler

* AuthN: Add IsAnonymous field

* AuthN: add priority to context aware clients

* ContextHandler: Add comment

* AuthN: Add a simple priority queue

* AuthN: Add Name to client interface

* AuthN: register clients with function

* AuthN: update mock and fake to implement interface

* AuthN: rewrite test without reflection

* AuthN: add comment

* AuthN: fix queue insert

* AuthN: rewrite tests

* AuthN: make the queue generic so we can reuse it for hooks

* ContextHandler: Add fixme for auth headers

* AuthN: remove unused variable

* AuthN: use multierror

* AuthN: write proper tests for queue

* AuthN: Add queue item that can store the value and priority

Co-authored-by: Jo <joao.guerreiro@grafana.com>

pkg/services/authn/authnimpl/priority_queue.go

@@ -0,0 +1,34 @@
+package authnimpl
+
+func newQueue[T any]() *queue[T] {
+	return &queue[T]{items: []queueItem[T]{}}
+}
+
+type queue[T any] struct {
+	items []queueItem[T]
+}
+
+type queueItem[T any] struct {
+	v T
+	p uint
+}
+
+func (q *queue[T]) insert(v T, p uint) {
+	// no items in the queue so we just add it
+	if len(q.items) == 0 {
+		q.items = append(q.items, queueItem[T]{v, p})
+		return
+	}
+
+	// find the position in the queue the item should be placed based on priority
+	for i, item := range q.items {
+		if p < item.p {
+			q.items = append(q.items[:i+1], q.items[i:]...)
+			q.items[i] = queueItem[T]{v, p}
+			return
+		}
+	}
+
+	// item did not have higher priority then what is in the queue currently, so we need to add it to the end
+	q.items = append(q.items, queueItem[T]{v, p})
+}

pkg/services/authn/authnimpl/priority_queue_test.go

@@ -0,0 +1,48 @@
+package authnimpl
+
+import (
+	"testing"
+
+	"github.com/grafana/grafana/pkg/services/authn/authntest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/grafana/grafana/pkg/services/authn"
+)
+
+func TestQueue(t *testing.T) {
+	type testCase struct {
+		desc          string
+		clients       []authn.ContextAwareClient
+		expectedOrder []string
+	}
+
+	tests := []testCase{
+		{
+			desc: "expect correct order",
+			clients: []authn.ContextAwareClient{
+				&authntest.FakeClient{ExpectedName: "1", ExpectedPriority: 1},
+				&authntest.FakeClient{ExpectedName: "5", ExpectedPriority: 5},
+				&authntest.FakeClient{ExpectedName: "3", ExpectedPriority: 3},
+				&authntest.FakeClient{ExpectedName: "2", ExpectedPriority: 2},
+				&authntest.FakeClient{ExpectedName: "4", ExpectedPriority: 4},
+			},
+			expectedOrder: []string{"1", "2", "3", "4", "5"},
+		},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.desc, func(t *testing.T) {
+			q := newQueue[authn.ContextAwareClient]()
+			for _, c := range tt.clients {
+				q.insert(c, c.Priority())
+			}
+
+			require.Len(t, q.items, len(tt.expectedOrder))
+
+			for i := range q.items {
+				assert.Equal(t, q.items[i].v.Name(), tt.expectedOrder[i])
+			}
+		})
+	}
+}

pkg/services/authn/authnimpl/service.go

@@ -5,6 +5,7 @@ import (
 	"net/http"
 	"strconv"
 
+	"github.com/hashicorp/go-multierror"
 	"go.opentelemetry.io/otel/attribute"
 
 	"github.com/grafana/grafana/pkg/infra/log"
@@ -34,7 +35,8 @@ const (
 )
 
 var (
-	errDisabledIdentity = errutil.NewBase(errutil.StatusUnauthorized, "identity.disabled")
+	errCantAuthenticateReq = errutil.NewBase(errutil.StatusUnauthorized, "auth.unauthorized")
+	errDisabledIdentity    = errutil.NewBase(errutil.StatusUnauthorized, "identity.disabled")
 )
 
 // make sure service implements authn.Service interface
@@ -55,20 +57,23 @@ func ProvideService(
 		log:            log.New("authn.service"),
 		cfg:            cfg,
 		clients:        make(map[string]authn.Client),
+		clientQueue:    newQueue[authn.ContextAwareClient](),
 		tracer:         tracer,
 		sessionService: sessionService,
 		postAuthHooks:  []authn.PostAuthHookFn{},
 	}
 
-	s.clients[authn.ClientRender] = clients.ProvideRender(userService, renderService)
-	s.clients[authn.ClientAPIKey] = clients.ProvideAPIKey(apikeyService, userService)
+	s.RegisterClient(clients.ProvideRender(userService, renderService))
+	s.RegisterClient(clients.ProvideAPIKey(apikeyService, userService))
 
-	sessionClient := clients.ProvideSession(sessionService, userService, cfg.LoginCookieName, cfg.LoginMaxLifetime)
-	s.clients[authn.ClientSession] = sessionClient
-	s.RegisterPostAuthHook(sessionClient.RefreshTokenHook)
+	if cfg.LoginCookieName != "" {
+		sessionClient := clients.ProvideSession(sessionService, userService, cfg.LoginCookieName, cfg.LoginMaxLifetime)
+		s.RegisterClient(sessionClient)
+		s.RegisterPostAuthHook(sessionClient.RefreshTokenHook)
+	}
 
 	if s.cfg.AnonymousEnabled {
-		s.clients[authn.ClientAnonymous] = clients.ProvideAnonymous(cfg, orgService)
+		s.RegisterClient(clients.ProvideAnonymous(cfg, orgService))
 	}
 
 	var proxyClients []authn.ProxyClient
@@ -89,11 +94,11 @@ func ProvideService(
 	if len(passwordClients) > 0 {
 		passwordClient := clients.ProvidePassword(loginAttempts, passwordClients...)
 		if s.cfg.BasicAuthEnabled {
-			s.clients[authn.ClientBasic] = clients.ProvideBasic(passwordClient)
+			s.RegisterClient(clients.ProvideBasic(passwordClient))
 		}
 		// FIXME (kalleep): Remove the global variable and stick it into cfg
 		if !setting.DisableLoginForm {
-			s.clients[authn.ClientForm] = clients.ProvideForm(passwordClient)
+			s.RegisterClient(clients.ProvideForm(passwordClient))
 		}
 	}
 
@@ -102,12 +107,12 @@ func ProvideService(
 		if err != nil {
 			s.log.Error("failed to configure auth proxy", "err", err)
 		} else {
-			s.clients[authn.ClientProxy] = proxy
+			s.RegisterClient(proxy)
 		}
 	}
 
 	if s.cfg.JWTAuthEnabled {
-		s.clients[authn.ClientJWT] = clients.ProvideJWT(jwtService, cfg)
+		s.RegisterClient(clients.ProvideJWT(jwtService, cfg))
 	}
 
 	// FIXME (jguer): move to User package
@@ -126,9 +131,11 @@ func ProvideService(
 }
 
 type Service struct {
-	log     log.Logger
-	cfg     *setting.Cfg
-	clients map[string]authn.Client
+	log log.Logger
+	cfg *setting.Cfg
+
+	clients     map[string]authn.Client
+	clientQueue *queue[authn.ContextAwareClient]
 
 	tracer         tracing.Tracer
 	sessionService auth.UserTokenService
@@ -139,42 +146,54 @@ type Service struct {
 	postLoginHooks []authn.PostLoginHookFn
 }
 
-func (s *Service) Authenticate(ctx context.Context, client string, r *authn.Request) (*authn.Identity, bool, error) {
-	c, ok := s.clients[client]
-	if !ok {
-		return nil, false, nil
-	}
-
-	if !c.Test(ctx, r) {
-		return nil, false, nil
-	}
-
+func (s *Service) Authenticate(ctx context.Context, r *authn.Request) (*authn.Identity, error) {
 	ctx, span := s.tracer.Start(ctx, "authn.Authenticate")
 	defer span.End()
-	span.SetAttributes("authn.client", client, attribute.Key("authn.client").String(client))
 
+	var authErr error
+	for _, item := range s.clientQueue.items {
+		if item.v.Test(ctx, r) {
+			identity, err := s.authenticate(ctx, item.v, r)
+			if err != nil {
+				s.log.Warn("failed to authenticate", "client", item.v.Name(), "err", err)
+				authErr = multierror.Append(authErr, err)
+				// try next
+				continue
+			}
+
+			if identity != nil {
+				return identity, nil
+			}
+		}
+	}
+
+	if authErr != nil {
+		return nil, authErr
+	}
+
+	return nil, errCantAuthenticateReq.Errorf("cannot authenticate request")
+}
+
+func (s *Service) authenticate(ctx context.Context, c authn.Client, r *authn.Request) (*authn.Identity, error) {
 	r.OrgID = orgIDFromRequest(r)
 	identity, err := c.Authenticate(ctx, r)
 	if err != nil {
-		s.log.FromContext(ctx).Warn("auth client could not authenticate request", "client", client, "error", err)
-		span.AddEvents([]string{"message"}, []tracing.EventValue{{Str: "auth client could not authenticate request"}})
-		return nil, true, err
+		s.log.FromContext(ctx).Warn("auth client could not authenticate request", "client", c.Name(), "error", err)
+		return nil, err
 	}
 
-	// FIXME (kalleep): Handle disabled identities
-
 	for _, hook := range s.postAuthHooks {
 		if err := hook(ctx, identity, r); err != nil {
 			s.log.FromContext(ctx).Warn("post auth hook failed", "error", err, "id", identity)
-			return nil, false, err
+			return nil, err
 		}
 	}
 
 	if identity.IsDisabled {
-		return nil, true, errDisabledIdentity.Errorf("identity is disabled")
+		return nil, errDisabledIdentity.Errorf("identity is disabled")
 	}
 
-	return identity, true, nil
+	return identity, nil
 }
 
 func (s *Service) RegisterPostAuthHook(hook authn.PostAuthHookFn) {
@@ -182,18 +201,18 @@ func (s *Service) RegisterPostAuthHook(hook authn.PostAuthHookFn) {
 }
 
 func (s *Service) Login(ctx context.Context, client string, r *authn.Request) (identity *authn.Identity, err error) {
-	var ok bool
-	identity, ok, err = s.Authenticate(ctx, client, r)
-	if !ok {
-		return nil, authn.ErrClientNotConfigured.Errorf("client not configured: %s", client)
-	}
-
 	defer func() {
 		for _, hook := range s.postLoginHooks {
 			hook(ctx, identity, r, err)
 		}
 	}()
 
+	c, ok := s.clients[client]
+	if !ok {
+		return nil, authn.ErrClientNotConfigured.Errorf("client not configured: %s", client)
+	}
+
+	identity, err = s.authenticate(ctx, c, r)
 	if err != nil {
 		return nil, err
 	}
@@ -242,6 +261,13 @@ func (s *Service) RedirectURL(ctx context.Context, client string, r *authn.Reque
 	return redirectClient.RedirectURL(ctx, r)
 }
 
+func (s *Service) RegisterClient(c authn.Client) {
+	s.clients[c.Name()] = c
+	if cac, ok := c.(authn.ContextAwareClient); ok {
+		s.clientQueue.insert(cac, cac.Priority())
+	}
+}
+
 func orgIDFromRequest(r *authn.Request) int64 {
 	if r.HTTPRequest == nil {
 		return 0

pkg/services/authn/authnimpl/service_test.go

@@ -8,76 +8,106 @@ import (
 	"net/url"
 	"testing"
 
-	"github.com/grafana/grafana/pkg/services/auth"
-	"github.com/grafana/grafana/pkg/services/auth/authtest"
-	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/stretchr/testify/assert"
 
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/infra/tracing"
+	"github.com/grafana/grafana/pkg/services/auth"
+	"github.com/grafana/grafana/pkg/services/auth/authtest"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/authn/authntest"
+	"github.com/grafana/grafana/pkg/services/user"
 	"github.com/grafana/grafana/pkg/setting"
 )
 
 func TestService_Authenticate(t *testing.T) {
 	type TestCase struct {
-		desc           string
-		clientName     string
-		clientErr      error
-		clientIdentity *authn.Identity
-		expectedOK     bool
-		expectedErr    error
+		desc             string
+		clients          []authn.Client
+		expectedIdentity *authn.Identity
+		expectedErrors   []error
 	}
 
-	var clientErr = errors.New("some err")
+	var (
+		firstErr = errors.New("first")
+		lastErr  = errors.New("last")
+	)
 
 	tests := []TestCase{
 		{
-			desc:           "should succeed with authentication for configured client",
-			clientIdentity: &authn.Identity{},
-			clientName:     "fake",
-			expectedOK:     true,
+			desc: "should succeed with authentication for configured client",
+			clients: []authn.Client{
+				&authntest.FakeClient{ExpectedTest: true, ExpectedIdentity: &authn.Identity{ID: "user:1"}},
+			},
+			expectedIdentity: &authn.Identity{ID: "user:1"},
 		},
 		{
-			desc:       "should return false when client is not configured",
-			clientName: "gitlab",
-			expectedOK: false,
+			desc: "should succeed with authentication for second client when first test fail",
+			clients: []authn.Client{
+				&authntest.FakeClient{ExpectedName: "1", ExpectedPriority: 1, ExpectedTest: false},
+				&authntest.FakeClient{ExpectedName: "2", ExpectedPriority: 2, ExpectedTest: true, ExpectedIdentity: &authn.Identity{ID: "user:2"}},
+			},
+			expectedIdentity: &authn.Identity{ID: "user:2"},
 		},
 		{
-			desc:        "should return true and error when client could be used but failed to authenticate",
-			clientName:  "fake",
-			expectedOK:  true,
-			clientErr:   clientErr,
-			expectedErr: clientErr,
+			desc: "should succeed with authentication for third client when error happened in first",
+			clients: []authn.Client{
+				&authntest.FakeClient{ExpectedName: "1", ExpectedPriority: 2, ExpectedTest: false},
+				&authntest.FakeClient{ExpectedName: "2", ExpectedPriority: 1, ExpectedTest: true, ExpectedErr: errors.New("some error")},
+				&authntest.FakeClient{ExpectedName: "3", ExpectedPriority: 3, ExpectedTest: true, ExpectedIdentity: &authn.Identity{ID: "user:3"}},
+			},
+			expectedIdentity: &authn.Identity{ID: "user:3"},
 		},
 		{
-			desc:           "should return error if identity is disabled",
-			clientName:     "fake",
-			clientIdentity: &authn.Identity{IsDisabled: true},
-			expectedOK:     true,
-			expectedErr:    errDisabledIdentity,
+			desc: "should return error when no client could authenticate the request",
+			clients: []authn.Client{
+				&authntest.FakeClient{ExpectedName: "1", ExpectedPriority: 2, ExpectedTest: false},
+				&authntest.FakeClient{ExpectedName: "2", ExpectedPriority: 1, ExpectedTest: false},
+				&authntest.FakeClient{ExpectedName: "3", ExpectedPriority: 3, ExpectedTest: false},
+			},
+			expectedErrors: []error{errCantAuthenticateReq},
+		},
+		{
+			desc: "should return all errors in chain",
+			clients: []authn.Client{
+				&authntest.FakeClient{ExpectedName: "1", ExpectedPriority: 2, ExpectedTest: false},
+				&authntest.FakeClient{ExpectedName: "2", ExpectedPriority: 1, ExpectedTest: true, ExpectedErr: firstErr},
+				&authntest.FakeClient{ExpectedName: "3", ExpectedPriority: 3, ExpectedTest: true, ExpectedErr: lastErr},
+			},
+			expectedErrors: []error{firstErr, lastErr},
+		},
+		{
+			desc: "should return error on disabled identity",
+			clients: []authn.Client{
+				&authntest.FakeClient{ExpectedName: "1", ExpectedTest: true, ExpectedIdentity: &authn.Identity{IsDisabled: true}},
+			},
+			expectedErrors: []error{errDisabledIdentity},
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			svc := setupTests(t, func(svc *Service) {
-				svc.clients["fake"] = &authntest.FakeClient{
-					ExpectedIdentity: tt.clientIdentity,
-					ExpectedErr:      tt.clientErr,
-					ExpectedTest:     tt.expectedOK,
+				for _, c := range tt.clients {
+					svc.RegisterClient(c)
 				}
 			})
 
-			_, ok, err := svc.Authenticate(context.Background(), tt.clientName, &authn.Request{})
-			assert.Equal(t, tt.expectedOK, ok)
-			assert.ErrorIs(t, err, tt.expectedErr)
+			identity, err := svc.Authenticate(context.Background(), &authn.Request{})
+			if len(tt.expectedErrors) == 0 {
+				assert.NoError(t, err)
+				assert.EqualValues(t, tt.expectedIdentity, identity)
+			} else {
+				for _, e := range tt.expectedErrors {
+					assert.ErrorIs(t, err, e)
+				}
+				assert.Nil(t, identity)
+			}
 		})
 	}
 }
 
-func TestService_AuthenticateOrgID(t *testing.T) {
+func TestService_Authenticate_OrgID(t *testing.T) {
 	type TestCase struct {
 		desc          string
 		req           *authn.Request
@@ -123,18 +153,16 @@ func TestService_AuthenticateOrgID(t *testing.T) {
 		t.Run(tt.desc, func(t *testing.T) {
 			var calledWith int64
 			s := setupTests(t, func(svc *Service) {
-				svc.clients["fake"] = authntest.MockClient{
+				svc.RegisterClient(authntest.MockClient{
 					AuthenticateFunc: func(ctx context.Context, r *authn.Request) (*authn.Identity, error) {
 						calledWith = r.OrgID
 						return &authn.Identity{}, nil
 					},
-					TestFunc: func(ctx context.Context, r *authn.Request) bool {
-						return true
-					},
-				}
+					TestFunc: func(ctx context.Context, r *authn.Request) bool { return true },
+				})
 			})
 
-			_, _, _ = s.Authenticate(context.Background(), "fake", tt.req)
+			_, _ = s.Authenticate(context.Background(), tt.req)
 			assert.Equal(t, tt.expectedOrgID, calledWith)
 		})
 	}
@@ -157,7 +185,7 @@ func TestService_Login(t *testing.T) {
 
 	tests := []TestCase{
 		{
-			desc:             "should authenticate and create session for valid request",
+			desc:             "should login for valid request",
 			client:           "fake",
 			expectedClientOK: true,
 			expectedClientIdentity: &authn.Identity{
@@ -169,12 +197,12 @@ func TestService_Login(t *testing.T) {
 			},
 		},
 		{
-			desc:        "should not authenticate with invalid client",
+			desc:        "should not login with invalid client",
 			client:      "invalid",
 			expectedErr: authn.ErrClientNotConfigured,
 		},
 		{
-			desc:                   "should not authenticate non user identity",
+			desc:                   "should not login non user identity",
 			client:                 "fake",
 			expectedClientOK:       true,
 			expectedClientIdentity: &authn.Identity{ID: "apikey:1"},
@@ -185,11 +213,12 @@ func TestService_Login(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			s := setupTests(t, func(svc *Service) {
-				svc.clients["fake"] = &authntest.FakeClient{
+				svc.RegisterClient(&authntest.FakeClient{
+					ExpectedName:     "fake",
 					ExpectedErr:      tt.expectedClientErr,
 					ExpectedTest:     tt.expectedClientOK,
 					ExpectedIdentity: tt.expectedClientIdentity,
-				}
+				})
 				svc.sessionService = &authtest.FakeUserAuthTokenService{
 					CreateTokenProvider: func(ctx context.Context, user *user.User, clientIP net.IP, userAgent string) (*auth.UserToken, error) {
 						if tt.expectedSessionErr != nil {
@@ -240,10 +269,8 @@ func TestService_RedirectURL(t *testing.T) {
 	for _, tt := range tests {
 		t.Run(tt.desc, func(t *testing.T) {
 			service := setupTests(t, func(svc *Service) {
-				svc.clients["redirect"] = authntest.FakeRedirectClient{
-					ExpectedURL: tt.expectedURL,
-				}
-				svc.clients["non-redirect"] = &authntest.FakeClient{}
+				svc.RegisterClient(authntest.FakeRedirectClient{ExpectedName: "redirect", ExpectedURL: tt.expectedURL})
+				svc.RegisterClient(&authntest.FakeClient{ExpectedName: "non-redirect"})
 			})
 
 			u, err := service.RedirectURL(context.Background(), tt.client, nil)
@@ -265,10 +292,11 @@ func setupTests(t *testing.T, opts ...func(svc *Service)) *Service {
 	t.Helper()
 
 	s := &Service{
-		log:     log.NewNopLogger(),
-		cfg:     setting.NewCfg(),
-		clients: map[string]authn.Client{},
-		tracer:  tracing.InitializeTracerForTest(),
+		log:         log.NewNopLogger(),
+		cfg:         setting.NewCfg(),
+		clientQueue: newQueue[authn.ContextAwareClient](),
+		clients:     map[string]authn.Client{},
+		tracer:      tracing.InitializeTracerForTest(),
 	}
 
 	for _, o := range opts {