fix/telemetry(auth): return authenticated context from session.SetActorFromUser (#62701)

This change fixes telemetry for various sign-in, sign-up telemetry events - `session.SetActorFromUser` accepts a context and returns a new one, but never does anything to the returned context. It makes sense for this to authenticate the context because it is only ever used on a success case, and in turn this fixes telemetry by providing the telemetry SDK an actor to extract a user ID for.

For the events affected by `session.SetActorFromUser` I've incremented the event version to `2`.

## Test plan

1. Test on sign-in-success that a event with the user is recorded
2. Test on `session.SetActorFromUser` to verify context is authenticated

cmd/frontend/external/session/session.go

@@ -5,8 +5,16 @@ package session
 import "github.com/sourcegraph/sourcegraph/internal/session"
 
 var (
-	ResetMockSessionStore   = session.ResetMockSessionStore
-	SetActor                = session.SetActor
+	ResetMockSessionStore = session.ResetMockSessionStore
+	// SetActor sets the actor in the session, or removes it if actor == nil. If no session exists, a
+	// new session is created.
+	//
+	// 🚨 SECURITY: Should only be called after user is successfully authenticated.
+	SetActor = session.SetActor
+	// SetActorFromUser creates an actor from a user, sets it in the session, and
+	// returns a context with the user attached.
+	//
+	// 🚨 SECURITY: Should only be called after user is successfully authenticated.
 	SetActorFromUser        = session.SetActorFromUser
 	SetData                 = session.SetData
 	GetData                 = session.GetData

cmd/frontend/internal/app/sign_out.go

@@ -52,13 +52,13 @@ func serveSignOutHandler(logger log.Logger, db database.DB) http.HandlerFunc {
 		var err error
 		if err = session.InvalidateSessionCurrentUser(w, r, db); err != nil {
 			recordSecurityEvent(r, db, database.SecurityEventNameSignOutFailed, err)
-			recorder.Record(ctx, telemetry.FeatureSignOut, telemetry.ActionFailed, nil)
+			recorder.Record(ctx, "signOut", telemetry.ActionFailed, nil)
 			logger.Error("serveSignOutHandler", log.Error(err))
 		}
 
 		if err = session.SetActor(w, r, nil, 0, time.Time{}); err != nil {
 			recordSecurityEvent(r, db, database.SecurityEventNameSignOutFailed, err)
-			recorder.Record(ctx, telemetry.FeatureSignOut, telemetry.ActionFailed, nil)
+			recorder.Record(ctx, "signOut", telemetry.ActionFailed, nil)
 			logger.Error("serveSignOutHandler", log.Error(err))
 		}
 
@@ -71,7 +71,7 @@ func serveSignOutHandler(logger log.Logger, db database.DB) http.HandlerFunc {
 		if err == nil {
 			recordSecurityEvent(r, db, database.SecurityEventNameSignOutSucceeded, nil)
 
-			recorder.Record(ctx, telemetry.FeatureSignOut, telemetry.ActionSucceeded, nil)
+			recorder.Record(ctx, "signOut", telemetry.ActionSucceeded, nil)
 		}
 
 		http.Redirect(w, r, "/search", http.StatusSeeOther)

internal/auth/userpasswd/handlers.go

@@ -109,6 +109,8 @@ func checkEmailAbuse(ctx context.Context, db database.DB, addr string) (abused b
 func handleSignUp(logger log.Logger, db database.DB, eventRecorder *telemetry.EventRecorder,
 	w http.ResponseWriter, r *http.Request, failIfNewUserIsNotInitialSiteAdmin bool,
 ) {
+	ctx := r.Context()
+
 	if r.Method != "POST" {
 		http.Error(w, fmt.Sprintf("unsupported method %s", r.Method), http.StatusBadRequest)
 		return
@@ -118,13 +120,15 @@ func handleSignUp(logger log.Logger, db database.DB, eventRecorder *telemetry.Ev
 		http.Error(w, "could not decode request body", http.StatusBadRequest)
 		return
 	}
-	err, statusCode, usr := unsafeSignUp(r.Context(), logger, db, creds, failIfNewUserIsNotInitialSiteAdmin)
+	err, statusCode, usr := unsafeSignUp(ctx, logger, db, creds, failIfNewUserIsNotInitialSiteAdmin)
 	if err != nil {
 		http.Error(w, err.Error(), statusCode)
 		return
 	}
 
-	if _, err := session.SetActorFromUser(r.Context(), w, r, usr, 0); err != nil {
+	// Write the session cookie and get an authenticated context
+	ctx, err = session.SetActorFromUser(ctx, w, r, usr, 0)
+	if err != nil {
 		httpLogError(logger.Error, w, fmt.Sprintf("Could not create new user session: %s", err.Error()), http.StatusInternalServerError, log.Error(err))
 	}
 
@@ -155,16 +159,17 @@ func handleSignUp(logger log.Logger, db database.DB, eventRecorder *telemetry.Ev
 		})
 	}
 
-	// New event - we record legacy event manually for now, hence teestore.WithoutV1
-	// TODO: Remove in 5.3
+	// New event - we record legacy event manually for now below, hence
+	// teestore.WithoutV1
 	events := telemetry.NewBestEffortEventRecorder(logger, eventRecorder)
-	events.Record(teestore.WithoutV1(r.Context()), telemetry.FeatureSignUp, telemetry.ActionSucceeded, &telemetry.EventParameters{
+	events.Record(teestore.WithoutV1(ctx), "signUp", telemetry.ActionSucceeded, &telemetry.EventParameters{
+		Version: 2,
 		Metadata: telemetry.EventMetadata{
 			"failIfNewUserIsNotInitialSiteAdmin": telemetry.Bool(failIfNewUserIsNotInitialSiteAdmin),
 		},
 	})
 	//lint:ignore SA1019 existing usage of deprecated functionality. TODO: Use only the new V2 event instead.
-	if err = usagestats.LogBackendEvent(db, usr.ID, deviceid.FromContext(r.Context()), "SignUpSucceeded", nil, nil, featureflag.GetEvaluatedFlagSet(r.Context()), nil); err != nil {
+	if err = usagestats.LogBackendEvent(db, usr.ID, deviceid.FromContext(ctx), "SignUpSucceeded", nil, nil, featureflag.GetEvaluatedFlagSet(r.Context()), nil); err != nil {
 		logger.Warn("Failed to log event SignUpSucceeded", log.Error(err))
 	}
 }
@@ -327,7 +332,9 @@ func HandleSignIn(logger log.Logger, db database.DB, store LockoutStore, recorde
 		telemetrySignInResult := telemetry.ActionFailed
 		defer func() {
 			recordSignInSecurityEvent(r, db, &user, &signInResult)
-			events.Record(ctx, telemetry.FeatureSignIn, telemetrySignInResult, nil)
+			events.Record(ctx, "signIn", telemetrySignInResult, &telemetry.EventParameters{
+				Version: 2,
+			})
 			checkAccountLockout(store, &user, &signInResult)
 		}()
 
@@ -383,7 +390,7 @@ func HandleSignIn(logger log.Logger, db database.DB, store LockoutStore, recorde
 			return
 		}
 
-		// Write the session cookie
+		// Write the session cookie and get an authenticated context
 		ctx, err = session.SetActorFromUser(ctx, w, r, &user, 0)
 		if err != nil {
 			httpLogError(logger.Error, w, fmt.Sprintf("Could not create new user session: %s", err.Error()), http.StatusInternalServerError, log.Error(err))

internal/auth/userpasswd/handlers_test.go

@@ -456,7 +456,8 @@ func TestHandleSignUp(t *testing.T) {
 			logger = logtest.Scoped(t)
 		}
 
-		h := HandleSignUp(logger, db, telemetry.NewEventRecorder(telemetrytest.NewMockEventsStore()))
+		telemetryRecorder, telemetryStore := telemetrytest.NewRecorder()
+		h := HandleSignUp(logger, db, telemetryRecorder)
 
 		body := strings.NewReader(`{
 			"email": "test@test.com",
@@ -475,6 +476,13 @@ func TestHandleSignUp(t *testing.T) {
 
 		mockrequire.CalledOnce(t, authz.GrantPendingPermissionsFunc)
 		mockrequire.CalledOnce(t, users.CreateFunc)
+
+		// Signup success event should be recorded, with user
+		events := telemetryStore.CollectStoredEvents()
+		require.Len(t, events, 1)
+		assert.Equal(t, events[0].Feature, "signUp")
+		assert.Equal(t, events[0].Action, string(telemetry.ActionSucceeded))
+		assert.NotEmpty(t, events[0].GetUser().GetUserId())
 	})
 }
 

internal/session/BUILD.bazel

@@ -44,5 +44,7 @@ go_test(
         "//internal/types",
         "//lib/errors",
         "@com_github_sourcegraph_log//logtest",
+        "@com_github_stretchr_testify//assert",
+        "@com_github_stretchr_testify//require",
     ],
 )

internal/session/session.go

@@ -219,6 +219,10 @@ func GetData(r *http.Request, key string, value any) error {
 	return nil
 }
 
+// SetActorFromUser creates an actor from a user, sets it in the session, and
+// returns a context with the user attached.
+//
+// 🚨 SECURITY: Should only be called after user is successfully authenticated.
 func SetActorFromUser(ctx context.Context, w http.ResponseWriter, r *http.Request, user *types.User, expiryPeriod time.Duration) (context.Context, error) {
 	info, err := licensing.GetConfiguredProductLicenseInfo()
 	if err != nil {
@@ -229,12 +233,16 @@ func SetActorFromUser(ctx context.Context, w http.ResponseWriter, r *http.Reques
 		return ctx, errors.New("Sourcegraph license is expired. Only admins are allowed to sign in.")
 	}
 
-	// Write the session cookie
-	actor := sgactor.Actor{
+	// Authentication passed at this point, this is our actor
+	act := sgactor.Actor{
 		UID: user.ID,
 	}
 
-	return ctx, SetActor(w, r, &actor, expiryPeriod, user.CreatedAt)
+	// Add actor to the context, because we return it
+	ctx = actor.WithActor(ctx, &act)
+
+	// Write the session cookie with SetActor
+	return ctx, SetActor(w, r, &act, expiryPeriod, user.CreatedAt)
 }
 
 // SetActor sets the actor in the session, or removes it if actor == nil. If no session exists, a

internal/session/session_test.go

@@ -11,6 +11,8 @@ import (
 	"time"
 
 	"github.com/sourcegraph/log/logtest"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
 
 	"github.com/sourcegraph/sourcegraph/internal/actor"
 	"github.com/sourcegraph/sourcegraph/internal/database"
@@ -565,3 +567,26 @@ func TestExpiredLicenseOnlyAllowsAdmins(t *testing.T) {
 		})
 	}
 }
+
+func TestSetActorFromUser(t *testing.T) {
+	cleanup := ResetMockSessionStore(t)
+	t.Cleanup(cleanup)
+
+	user := &types.User{
+		ID:        1,
+		SiteAdmin: true,
+		CreatedAt: time.Now(),
+	}
+
+	ctx := context.Background()
+	req, err := http.NewRequestWithContext(ctx, "GET", "/", nil)
+	require.NoError(t, err)
+	rr := httptest.NewRecorder()
+
+	ctx, err = SetActorFromUser(ctx, rr, req, user, 0)
+	assert.NoError(t, err)
+
+	actor := actor.FromContext(ctx)
+	assert.NotNil(t, actor)
+	assert.Equal(t, actor.UID, user.ID)
+}

internal/telemetry/events.go

@@ -16,12 +16,6 @@ type eventFeature string
 const (
 	// FeatureExample is a value for testing - do not use.
 	FeatureExample eventFeature = "exampleFeature"
-
-	// FeatureSignIn, FeatureSignOut, and FeatureSignUp are added here as telemetry
-	// examples - most callsites can directly provide the relevant feature.
-	FeatureSignIn  eventFeature = "signIn"
-	FeatureSignOut eventFeature = "signOut"
-	FeatureSignUp  eventFeature = "signUp"
 )
 
 // eventAction defines the action associated with an event. Values should