Auth: Fix Last Seen being updated on every request (#72036)

* make sure LastSeen hook has information to decide if update is necessary

* make user service check if it should update the user's last seen

* do not run last seen hook if is a login request

* make service return error when last seen is up to date

* fix err

* Update pkg/services/contexthandler/contexthandler.go

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

* fix golint

---------

Co-authored-by: Gabriel MABILLE <gamab@users.noreply.github.com>

pkg/services/authn/authn.go

@@ -37,6 +37,7 @@ const (
 const (
 	MetaKeyUsername   = "username"
 	MetaKeyAuthModule = "authModule"
+	MetaKeyIsLogin    = "isLogin"
 )
 
 // ClientParams are hints to the auth service about how to handle the identity management

pkg/services/authn/authnimpl/service.go

@@ -155,7 +155,7 @@ func ProvideService(
 	s.RegisterPostAuthHook(userSyncService.SyncUserHook, 10)
 	s.RegisterPostAuthHook(userSyncService.EnableDisabledUserHook, 20)
 	s.RegisterPostAuthHook(orgUserSyncService.SyncOrgRolesHook, 30)
-	s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 40)
+	s.RegisterPostAuthHook(userSyncService.SyncLastSeenHook, 120)
 
 	if features.IsEnabled(featuremgmt.FlagAccessTokenExpirationCheck) {
 		s.RegisterPostAuthHook(sync.ProvideOAuthTokenSync(oauthTokenService, sessionService, socialService).SyncOauthTokenHook, 60)
@@ -270,6 +270,7 @@ func (s *Service) Login(ctx context.Context, client string, r *authn.Request) (i
 		return nil, authn.ErrClientNotConfigured.Errorf("client not configured: %s", client)
 	}
 
+	r.SetMeta(authn.MetaKeyIsLogin, "true")
 	identity, err = s.authenticate(ctx, c, r)
 	if err != nil {
 		s.metrics.failedLogin.WithLabelValues(client).Inc()

pkg/services/authn/authnimpl/sync/user_sync.go

@@ -3,7 +3,6 @@ package sync
 import (
 	"context"
 	"errors"
-	"time"
 
 	"github.com/grafana/grafana/pkg/infra/log"
 	"github.com/grafana/grafana/pkg/services/authn"
@@ -129,7 +128,12 @@ func (s *UserSync) FetchSyncedUserHook(ctx context.Context, identity *authn.Iden
 	return nil
 }
 
-func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identity, _ *authn.Request) error {
+func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identity, r *authn.Request) error {
+	if r.GetMeta(authn.MetaKeyIsLogin) != "" {
+		// Do not sync last seen for login requests
+		return nil
+	}
+
 	namespace, id := identity.NamespacedID()
 
 	if namespace != authn.NamespaceUser && namespace != authn.NamespaceServiceAccount {
@@ -137,10 +141,6 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit
 		return nil
 	}
 
-	if !shouldUpdateLastSeen(identity.LastSeenAt) {
-		return nil
-	}
-
 	go func(userID int64) {
 		defer func() {
 			if err := recover(); err != nil {
@@ -148,7 +148,9 @@ func (s *UserSync) SyncLastSeenHook(ctx context.Context, identity *authn.Identit
 			}
 		}()
 
-		if err := s.userService.UpdateLastSeenAt(context.Background(), &user.UpdateUserLastSeenAtCommand{UserID: userID}); err != nil {
+		if err := s.userService.UpdateLastSeenAt(context.Background(),
+			&user.UpdateUserLastSeenAtCommand{UserID: userID, OrgID: r.OrgID}); err != nil &&
+			!errors.Is(err, user.ErrLastSeenUpToDate) {
 			s.log.Error("Failed to update last_seen_at", "err", err, "userId", userID)
 		}
 	}(id)
@@ -393,7 +395,3 @@ func syncSignedInUserToIdentity(usr *user.SignedInUser, identity *authn.Identity
 	identity.IsDisabled = usr.IsDisabled
 	identity.IsGrafanaAdmin = &usr.IsGrafanaAdmin
 }
-
-func shouldUpdateLastSeen(t time.Time) bool {
-	return time.Since(t) > time.Minute*5
-}

pkg/services/contexthandler/contexthandler.go

@@ -242,7 +242,8 @@ func (h *ContextHandler) Middleware(next http.Handler) http.Handler {
 			// update last seen every 5min
 			if reqContext.ShouldUpdateLastSeenAt() {
 				reqContext.Logger.Debug("Updating last user_seen_at", "user_id", reqContext.UserID)
-				if err := h.userService.UpdateLastSeenAt(mContext.Req.Context(), &user.UpdateUserLastSeenAtCommand{UserID: reqContext.UserID}); err != nil {
+				err := h.userService.UpdateLastSeenAt(mContext.Req.Context(), &user.UpdateUserLastSeenAtCommand{UserID: reqContext.UserID, OrgID: reqContext.OrgID})
+				if err != nil && !errors.Is(err, user.ErrLastSeenUpToDate) {
 					reqContext.Logger.Error("Failed to update last_seen_at", "error", err)
 				}
 			}

pkg/services/user/model.go

@@ -27,6 +27,7 @@ var (
 	ErrLastGrafanaAdmin  = errors.New("cannot remove last grafana admin")
 	ErrProtectedUser     = errors.New("cannot adopt protected user")
 	ErrNoUniqueID        = errors.New("identifying id not found")
+	ErrLastSeenUpToDate  = errors.New("last seen is already up to date")
 )
 
 type User struct {
@@ -95,6 +96,7 @@ type ChangeUserPasswordCommand struct {
 
 type UpdateUserLastSeenAtCommand struct {
 	UserID int64
+	OrgID  int64
 }
 
 type SetUsingOrgCommand struct {

pkg/services/user/userimpl/user.go

@@ -219,9 +219,26 @@ func (s *Service) ChangePassword(ctx context.Context, cmd *user.ChangeUserPasswo
 }
 
 func (s *Service) UpdateLastSeenAt(ctx context.Context, cmd *user.UpdateUserLastSeenAtCommand) error {
+	u, err := s.GetSignedInUserWithCacheCtx(ctx, &user.GetSignedInUserQuery{
+		UserID: cmd.UserID,
+		OrgID:  cmd.OrgID,
+	})
+
+	if err != nil {
+		return err
+	}
+
+	if !shouldUpdateLastSeen(u.LastSeenAt) {
+		return user.ErrLastSeenUpToDate
+	}
+
 	return s.store.UpdateLastSeenAt(ctx, cmd)
 }
 
+func shouldUpdateLastSeen(t time.Time) bool {
+	return time.Since(t) > time.Minute*5
+}
+
 func (s *Service) SetUsingOrg(ctx context.Context, cmd *user.SetUsingOrgCommand) error {
 	getOrgsForUserCmd := &org.GetUserOrgListQuery{UserID: cmd.UserID}
 	orgsForUser, err := s.orgService.GetUserOrgList(ctx, getOrgsForUserCmd)

pkg/services/user/userimpl/user_test.go

@@ -4,6 +4,7 @@ import (
 	"context"
 	"errors"
 	"testing"
+	"time"
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
@@ -288,3 +289,29 @@ func (f *FakeUserStore) Search(ctx context.Context, query *user.SearchUsersQuery
 func (f *FakeUserStore) Count(ctx context.Context) (int64, error) {
 	return 0, nil
 }
+
+func TestUpdateLastSeenAt(t *testing.T) {
+	userStore := newUserStoreFake()
+	orgService := orgtest.NewOrgServiceFake()
+	userService := Service{
+		store:        userStore,
+		orgService:   orgService,
+		cacheService: localcache.ProvideService(),
+		teamService:  &teamtest.FakeService{},
+	}
+	userService.cfg = setting.NewCfg()
+
+	t.Run("update last seen at", func(t *testing.T) {
+		userStore.ExpectedSignedInUser = &user.SignedInUser{UserID: 1, OrgID: 1, Email: "email", Login: "login", Name: "name", LastSeenAt: time.Now().Add(-10 * time.Minute)}
+		err := userService.UpdateLastSeenAt(context.Background(), &user.UpdateUserLastSeenAtCommand{UserID: 1, OrgID: 1})
+		require.NoError(t, err)
+	})
+
+	userService.cacheService.Flush()
+
+	t.Run("do not update last seen at", func(t *testing.T) {
+		userStore.ExpectedSignedInUser = &user.SignedInUser{UserID: 1, OrgID: 1, Email: "email", Login: "login", Name: "name", LastSeenAt: time.Now().Add(-1 * time.Minute)}
+		err := userService.UpdateLastSeenAt(context.Background(), &user.UpdateUserLastSeenAtCommand{UserID: 1, OrgID: 1})
+		require.ErrorIs(t, err, user.ErrLastSeenUpToDate, err)
+	})
+}