AuthToken: Remove client token rotation feature toggle (#82886)

* Remove usage of client token rotation flag

* Remove client token rotation feature toggle

pkg/services/authn/authnimpl/service.go

@@ -94,7 +94,7 @@ func ProvideService(
 	s.RegisterClient(clients.ProvideAPIKey(apikeyService, userService))
 
 	if cfg.LoginCookieName != "" {
-		s.RegisterClient(clients.ProvideSession(cfg, sessionService, features))
+		s.RegisterClient(clients.ProvideSession(cfg, sessionService))
 	}
 
 	var proxyClients []authn.ProxyClient

pkg/services/authn/clients/session.go

@@ -2,27 +2,20 @@ package clients
 
 import (
 	"context"
-	"errors"
 	"net/url"
 	"time"
 
 	"github.com/grafana/grafana/pkg/infra/log"
-	"github.com/grafana/grafana/pkg/infra/network"
 	"github.com/grafana/grafana/pkg/services/auth"
 	"github.com/grafana/grafana/pkg/services/authn"
-	"github.com/grafana/grafana/pkg/services/featuremgmt"
 	"github.com/grafana/grafana/pkg/setting"
-	"github.com/grafana/grafana/pkg/web"
 )
 
-var _ authn.HookClient = new(Session)
 var _ authn.ContextAwareClient = new(Session)
 
-func ProvideSession(cfg *setting.Cfg, sessionService auth.UserTokenService,
-	features featuremgmt.FeatureToggles) *Session {
+func ProvideSession(cfg *setting.Cfg, sessionService auth.UserTokenService) *Session {
 	return &Session{
 		cfg:            cfg,
-		features:       features,
 		sessionService: sessionService,
 		log:            log.New(authn.ClientSession),
 	}
@@ -30,7 +23,6 @@ func ProvideSession(cfg *setting.Cfg, sessionService auth.UserTokenService,
 
 type Session struct {
 	cfg            *setting.Cfg
-	features       featuremgmt.FeatureToggles
 	sessionService auth.UserTokenService
 	log            log.Logger
 }
@@ -55,10 +47,8 @@ func (s *Session) Authenticate(ctx context.Context, r *authn.Request) (*authn.Id
 		return nil, err
 	}
 
-	if s.features.IsEnabled(ctx, featuremgmt.FlagClientTokenRotation) {
-		if token.NeedsRotation(time.Duration(s.cfg.TokenRotationIntervalMinutes) * time.Minute) {
-			return nil, authn.ErrTokenNeedsRotation.Errorf("token needs to be rotated")
-		}
+	if token.NeedsRotation(time.Duration(s.cfg.TokenRotationIntervalMinutes) * time.Minute) {
+		return nil, authn.ErrTokenNeedsRotation.Errorf("token needs to be rotated")
 	}
 
 	return &authn.Identity{
@@ -86,40 +76,3 @@ func (s *Session) Test(ctx context.Context, r *authn.Request) bool {
 func (s *Session) Priority() uint {
 	return 60
 }
-
-func (s *Session) Hook(ctx context.Context, identity *authn.Identity, r *authn.Request) error {
-	if identity.SessionToken == nil || s.features.IsEnabled(ctx, featuremgmt.FlagClientTokenRotation) {
-		return nil
-	}
-
-	r.Resp.Before(func(w web.ResponseWriter) {
-		if w.Written() || errors.Is(ctx.Err(), context.Canceled) {
-			return
-		}
-
-		// FIXME (jguer): get real values
-		addr := web.RemoteAddr(r.HTTPRequest)
-		userAgent := r.HTTPRequest.UserAgent()
-
-		// addr := reqContext.RemoteAddr()
-		ip, err := network.GetIPFromAddress(addr)
-		if err != nil {
-			s.log.Debug("Failed to get client IP address", "addr", addr, "err", err)
-			ip = nil
-		}
-		rotated, newToken, err := s.sessionService.TryRotateToken(ctx, identity.SessionToken, ip, userAgent)
-		if err != nil {
-			s.log.Error("Failed to rotate token", "error", err)
-			return
-		}
-
-		if rotated {
-			identity.SessionToken = newToken
-			s.log.Debug("Rotated session token", "user", identity.ID)
-
-			authn.WriteSessionCookie(w, s.cfg, identity.SessionToken)
-		}
-	})
-
-	return nil
-}

pkg/services/authn/clients/session_test.go

@@ -2,7 +2,6 @@ package clients
 
 import (
 	"context"
-	"net"
 	"net/http"
 	"testing"
 	"time"
@@ -14,9 +13,7 @@ import (
 	"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/featuremgmt"
 	"github.com/grafana/grafana/pkg/setting"
-	"github.com/grafana/grafana/pkg/web"
 )
 
 func TestSession_Test(t *testing.T) {
@@ -29,7 +26,7 @@ func TestSession_Test(t *testing.T) {
 	cfg := setting.NewCfg()
 	cfg.LoginCookieName = ""
 	cfg.LoginMaxLifetime = 20 * time.Second
-	s := ProvideSession(cfg, &authtest.FakeUserAuthTokenService{}, featuremgmt.WithFeatures())
+	s := ProvideSession(cfg, &authtest.FakeUserAuthTokenService{})
 
 	disabled := s.Test(context.Background(), &authn.Request{HTTPRequest: validHTTPReq})
 	assert.False(t, disabled)
@@ -64,7 +61,6 @@ func TestSession_Authenticate(t *testing.T) {
 
 	type fields struct {
 		sessionService auth.UserTokenService
-		features       featuremgmt.FeatureToggles
 	}
 	type args struct {
 		r *authn.Request
@@ -80,7 +76,6 @@ func TestSession_Authenticate(t *testing.T) {
 			name: "cookie not found",
 			fields: fields{
 				sessionService: &authtest.FakeUserAuthTokenService{},
-				features:       featuremgmt.WithFeatures(),
 			},
 			args:    args{r: &authn.Request{HTTPRequest: &http.Request{}}},
 			wantID:  nil,
@@ -92,7 +87,6 @@ func TestSession_Authenticate(t *testing.T) {
 				sessionService: &authtest.FakeUserAuthTokenService{LookupTokenProvider: func(ctx context.Context, unhashedToken string) (*auth.UserToken, error) {
 					return validToken, nil
 				}},
-				features: featuremgmt.WithFeatures(),
 			},
 			args: args{r: &authn.Request{HTTPRequest: validHTTPReq}},
 			wantID: &authn.Identity{
@@ -106,7 +100,7 @@ func TestSession_Authenticate(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "should return error for token that needs rotation if ClientTokenRotation is enabled",
+			name: "should return error for token that needs rotation",
 			fields: fields{
 				sessionService: &authtest.FakeUserAuthTokenService{LookupTokenProvider: func(ctx context.Context, unhashedToken string) (*auth.UserToken, error) {
 					return &auth.UserToken{
@@ -114,18 +108,16 @@ func TestSession_Authenticate(t *testing.T) {
 						RotatedAt:     time.Now().Add(-11 * time.Minute).Unix(),
 					}, nil
 				}},
-				features: featuremgmt.WithFeatures(featuremgmt.FlagClientTokenRotation),
 			},
 			args:    args{r: &authn.Request{HTTPRequest: validHTTPReq}},
 			wantErr: true,
 		},
 		{
-			name: "should return identity for token that don't need rotation if ClientTokenRotation is enabled",
+			name: "should return identity for token that don't need rotation",
 			fields: fields{
 				sessionService: &authtest.FakeUserAuthTokenService{LookupTokenProvider: func(ctx context.Context, unhashedToken string) (*auth.UserToken, error) {
 					return validToken, nil
 				}},
-				features: featuremgmt.WithFeatures(featuremgmt.FlagClientTokenRotation),
 			},
 			args: args{r: &authn.Request{HTTPRequest: validHTTPReq}},
 			wantID: &authn.Identity{
@@ -145,7 +137,7 @@ func TestSession_Authenticate(t *testing.T) {
 			cfg.LoginCookieName = cookieName
 			cfg.TokenRotationIntervalMinutes = 10
 			cfg.LoginMaxLifetime = 20 * time.Second
-			s := ProvideSession(cfg, tt.fields.sessionService, tt.fields.features)
+			s := ProvideSession(cfg, tt.fields.sessionService)
 
 			got, err := s.Authenticate(context.Background(), tt.args.r)
 			require.True(t, (err != nil) == tt.wantErr, err)
@@ -157,73 +149,3 @@ func TestSession_Authenticate(t *testing.T) {
 		})
 	}
 }
-
-type fakeResponseWriter struct {
-	Status      int
-	HeaderStore http.Header
-}
-
-func (f *fakeResponseWriter) Header() http.Header {
-	return f.HeaderStore
-}
-
-func (f *fakeResponseWriter) Write([]byte) (int, error) {
-	return 0, nil
-}
-
-func (f *fakeResponseWriter) WriteHeader(statusCode int) {
-	f.Status = statusCode
-}
-
-func TestSession_Hook(t *testing.T) {
-	t.Run("should rotate token", func(t *testing.T) {
-		cfg := setting.NewCfg()
-		cfg.LoginCookieName = "grafana-session"
-		cfg.LoginMaxLifetime = 20 * time.Second
-		s := ProvideSession(cfg, &authtest.FakeUserAuthTokenService{
-			TryRotateTokenProvider: func(ctx context.Context, token *auth.UserToken, clientIP net.IP, userAgent string) (bool, *auth.UserToken, error) {
-				token.UnhashedToken = "new-token"
-				return true, token, nil
-			},
-		}, featuremgmt.WithFeatures())
-
-		sampleID := &authn.Identity{
-			SessionToken: &auth.UserToken{
-				Id:     1,
-				UserId: 1,
-			},
-		}
-
-		mockResponseWriter := &fakeResponseWriter{
-			Status:      0,
-			HeaderStore: map[string][]string{},
-		}
-
-		resp := &authn.Request{
-			HTTPRequest: &http.Request{
-				Header: map[string][]string{},
-			},
-			Resp: web.NewResponseWriter(http.MethodConnect, mockResponseWriter),
-		}
-
-		err := s.Hook(context.Background(), sampleID, resp)
-		require.NoError(t, err)
-
-		resp.Resp.WriteHeader(201)
-		require.Equal(t, 201, mockResponseWriter.Status)
-
-		assert.Equal(t, "new-token", sampleID.SessionToken.UnhashedToken)
-		require.Len(t, mockResponseWriter.HeaderStore, 1)
-		assert.Equal(t, "grafana-session=new-token; Path=/; Max-Age=20; HttpOnly",
-			mockResponseWriter.HeaderStore.Get("set-cookie"), mockResponseWriter.HeaderStore)
-	})
-
-	t.Run("should not rotate token with feature flag", func(t *testing.T) {
-		s := ProvideSession(setting.NewCfg(), nil, featuremgmt.WithFeatures(featuremgmt.FlagClientTokenRotation))
-
-		req := &authn.Request{}
-		identity := &authn.Identity{}
-		err := s.Hook(context.Background(), identity, req)
-		require.NoError(t, err)
-	})
-}

pkg/services/contexthandler/contexthandler.go

@@ -3,7 +3,6 @@ package contexthandler
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"net/http"
 
@@ -13,7 +12,6 @@ import (
 	"github.com/grafana/grafana/pkg/api/response"
 	"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/identity"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/contexthandler/ctxkey"
@@ -115,11 +113,6 @@ func (h *ContextHandler) Middleware(next http.Handler) http.Handler {
 
 		identity, err := h.authnService.Authenticate(reqContext.Req.Context(), &authn.Request{HTTPRequest: reqContext.Req, Resp: reqContext.Resp})
 		if err != nil {
-			if errors.Is(err, auth.ErrInvalidSessionToken) || errors.Is(err, authn.ErrExpiredAccessToken) {
-				// Burn the cookie in case of invalid, expired or missing token
-				reqContext.Resp.Before(h.deleteInvalidCookieEndOfRequestFunc(reqContext))
-			}
-
 			// Hack: set all errors on LookupTokenErr, so we can check it in auth middlewares
 			reqContext.LookupTokenErr = err
 		} else {
@@ -170,22 +163,6 @@ func (h *ContextHandler) addIDHeaderEndOfRequestFunc(ident identity.Requester) w
 	}
 }
 
-func (h *ContextHandler) deleteInvalidCookieEndOfRequestFunc(reqContext *contextmodel.ReqContext) web.BeforeFunc {
-	return func(w web.ResponseWriter) {
-		if h.features.IsEnabled(reqContext.Req.Context(), featuremgmt.FlagClientTokenRotation) {
-			return
-		}
-
-		if w.Written() {
-			reqContext.Logger.Debug("Response written, skipping invalid cookie delete")
-			return
-		}
-
-		reqContext.Logger.Debug("Expiring invalid cookie")
-		authn.DeleteSessionCookie(reqContext.Resp, h.Cfg)
-	}
-}
-
 type authHTTPHeaderListContextKey struct{}
 
 var authHTTPHeaderListKey = authHTTPHeaderListContextKey{}

pkg/services/contexthandler/contexthandler_test.go

@@ -10,7 +10,6 @@ import (
 
 	"github.com/grafana/grafana/pkg/api/routing"
 	"github.com/grafana/grafana/pkg/infra/tracing"
-	"github.com/grafana/grafana/pkg/services/auth"
 	"github.com/grafana/grafana/pkg/services/authn"
 	"github.com/grafana/grafana/pkg/services/authn/authntest"
 	"github.com/grafana/grafana/pkg/services/contexthandler"
@@ -111,46 +110,6 @@ func TestContextHandler(t *testing.T) {
 		require.NoError(t, res.Body.Close())
 	})
 
-	t.Run("should delete session cookie on invalid session", func(t *testing.T) {
-		handler := contexthandler.ProvideService(
-			setting.NewCfg(),
-			tracing.InitializeTracerForTest(),
-			featuremgmt.WithFeatures(),
-			&authntest.FakeService{ExpectedErr: auth.ErrInvalidSessionToken},
-		)
-
-		server := webtest.NewServer(t, routing.NewRouteRegister())
-		server.Mux.Use(handler.Middleware)
-		server.Mux.Get("/api/handler", func(c *contextmodel.ReqContext) {})
-
-		res, err := server.Send(server.NewGetRequest("/api/handler"))
-		require.NoError(t, err)
-		cookies := res.Cookies()
-		require.Len(t, cookies, 1)
-		require.Equal(t, cookies[0].String(), "grafana_session_expiry=; Path=/; Max-Age=0")
-		require.NoError(t, res.Body.Close())
-	})
-
-	t.Run("should delete session cookie when oauth token refresh failed", func(t *testing.T) {
-		handler := contexthandler.ProvideService(
-			setting.NewCfg(),
-			tracing.InitializeTracerForTest(),
-			featuremgmt.WithFeatures(),
-			&authntest.FakeService{ExpectedErr: authn.ErrExpiredAccessToken.Errorf("")},
-		)
-
-		server := webtest.NewServer(t, routing.NewRouteRegister())
-		server.Mux.Use(handler.Middleware)
-		server.Mux.Get("/api/handler", func(c *contextmodel.ReqContext) {})
-
-		res, err := server.Send(server.NewGetRequest("/api/handler"))
-		require.NoError(t, err)
-		cookies := res.Cookies()
-		require.Len(t, cookies, 1)
-		require.Equal(t, cookies[0].String(), "grafana_session_expiry=; Path=/; Max-Age=0")
-		require.NoError(t, res.Body.Close())
-	})
-
 	t.Run("should store auth header in context", func(t *testing.T) {
 		cfg := setting.NewCfg()
 		cfg.JWTAuth.Enabled = true

pkg/services/featuremgmt/registry.go

@@ -382,14 +382,6 @@ var (
 			FrontendOnly: false,
 			Owner:        grafanaObservabilityMetricsSquad,
 		},
-		{
-			Name:           "clientTokenRotation",
-			Description:    "Replaces the current in-request token rotation so that the client initiates the rotation",
-			Stage:          FeatureStageGeneralAvailability,
-			Expression:     "true",
-			Owner:          identityAccessTeam,
-			AllowSelfServe: false,
-		},
 		{
 			Name:           "prometheusDataplane",
 			Description:    "Changes responses to from Prometheus to be compliant with the dataplane specification. In particular, when this feature toggle is active, the numeric `Field.Name` is set from 'Value' to the value of the `__name__` label.",

pkg/services/featuremgmt/toggles_gen.csv

@@ -49,7 +49,6 @@ prometheusMetricEncyclopedia,GA,@grafana/observability-metrics,false,false,true
 influxdbBackendMigration,GA,@grafana/observability-metrics,false,false,true
 influxqlStreamingParser,experimental,@grafana/observability-metrics,false,false,false
 influxdbRunQueriesInParallel,privatePreview,@grafana/observability-metrics,false,false,false
-clientTokenRotation,GA,@grafana/identity-access-team,false,false,false
 prometheusDataplane,GA,@grafana/observability-metrics,false,false,false
 lokiMetricDataplane,GA,@grafana/observability-logs,false,false,false
 lokiLogsDataplane,experimental,@grafana/observability-logs,false,false,false

pkg/services/featuremgmt/toggles_gen.go

@@ -207,10 +207,6 @@ const (
 	// Enables running InfluxDB Influxql queries in parallel
 	FlagInfluxdbRunQueriesInParallel = "influxdbRunQueriesInParallel"
 
-	// FlagClientTokenRotation
-	// Replaces the current in-request token rotation so that the client initiates the rotation
-	FlagClientTokenRotation = "clientTokenRotation"
-
 	// FlagPrometheusDataplane
 	// Changes responses to from Prometheus to be compliant with the dataplane specification. In particular, when this feature toggle is active, the numeric `Field.Name` is set from 'Value' to the value of the `__name__` label.
 	FlagPrometheusDataplane = "prometheusDataplane"

pkg/services/featuremgmt/toggles_gen.json

@@ -1287,7 +1287,8 @@
       "metadata": {
         "name": "clientTokenRotation",
         "resourceVersion": "1707928895402",
-        "creationTimestamp": "2024-02-14T16:41:35Z"
+        "creationTimestamp": "2024-02-14T16:41:35Z",
+        "deletionTimestamp": "2024-02-16T10:26:09Z"
       },
       "spec": {
         "description": "Replaces the current in-request token rotation so that the client initiates the rotation",