From 2a5b04e97ab30d88637e44d52f4988e901b82818 Mon Sep 17 00:00:00 2001 From: Joe Chen Date: Wed, 16 Nov 2022 16:57:21 +0800 Subject: [PATCH] event_logs: instrument SOAP-related event logs --- cmd/frontend/auth/user.go | 85 +++++++++++--- cmd/frontend/auth/user_test.go | 46 ++++++++ cmd/frontend/graphqlbackend/graphqlbackend.go | 59 +++++++++- cmd/frontend/graphqlbackend/search_test.go | 2 +- cmd/frontend/internal/cli/http.go | 6 +- cmd/frontend/internal/httpapi/auth.go | 105 ++++++++++++++++-- cmd/frontend/internal/httpapi/auth_test.go | 88 +++++++++++---- enterprise/cmd/frontend/internal/auth/init.go | 3 +- .../auth/sourcegraphoperator/config.go | 7 +- .../auth/sourcegraphoperator/middleware.go | 67 +++++++++-- .../sourcegraphoperator/middleware_test.go | 11 +- .../auth/sourcegraphoperator/provider.go | 11 +- .../internal/licensing/enforcement/users.go | 4 +- .../licensing/enforcement/users_test.go | 4 +- .../auth/sourcegraph_operator_cleaner.go | 11 +- .../auth/sourcegraph_operator_cleaner_test.go | 10 +- internal/actor/actor.go | 3 + internal/auth/const.go | 5 + internal/database/event_logs.go | 20 +++- internal/database/security_event_logs.go | 23 +++- 20 files changed, 483 insertions(+), 87 deletions(-) create mode 100644 internal/auth/const.go diff --git a/cmd/frontend/auth/user.go b/cmd/frontend/auth/user.go index 265b52041921..137a87322c17 100644 --- a/cmd/frontend/auth/user.go +++ b/cmd/frontend/auth/user.go @@ -5,9 +5,10 @@ import ( "encoding/json" "fmt" - "github.com/inconshreveable/log15" + sglog "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/internal/actor" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/authz" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/deviceid" @@ -62,14 +63,15 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (u return MockGetAndSaveUser(ctx, op) } - extacc := db.UserExternalAccounts() + externalAccountsStore := db.UserExternalAccounts() + logger := sglog.Scoped("authGetAndSaveUser", "get and save user authenticated by external providers") userID, userSaved, extAcctSaved, safeErrMsg, err := func() (int32, bool, bool, string, error) { if actor := actor.FromContext(ctx); actor.IsAuthenticated() { return actor.UID, false, false, "", nil } - uid, lookupByExternalErr := extacc.LookupUserAndSave(ctx, op.ExternalAccount, op.ExternalAccountData) + uid, lookupByExternalErr := externalAccountsStore.LookupUserAndSave(ctx, op.ExternalAccount, op.ExternalAccountData) if lookupByExternalErr == nil { return uid, false, true, "", nil } @@ -104,8 +106,18 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (u return 0, false, false, "It looks like this is your first time signing in with this external identity. Sourcegraph couldn't link it to an existing user, because no verified email was provided. Ask your site admin to configure the auth provider to include the user's verified email on sign-in.", lookupByExternalErr } - // If CreateIfNotExist is true, create the new user, regardless of whether the email was verified or not. - userID, err := extacc.CreateUserAndSave(ctx, op.UserProps, op.ExternalAccount, op.ExternalAccountData) + act := &actor.Actor{ + SourcegraphOperator: op.ExternalAccount.ServiceType == auth.SourcegraphOperatorProviderType, + } + + // If CreateIfNotExist is true, create the new user, regardless of whether the + // email was verified or not. + // + // NOTE: It is important to propagate the correct context that carries the + // information of the actor, especially whether the actor is a Sourcegraph + // operator or not. + ctx = actor.WithActor(ctx, act) + userID, err := externalAccountsStore.CreateUserAndSave(ctx, op.UserProps, op.ExternalAccount, op.ExternalAccountData) switch { case database.IsUsernameExists(err): return 0, false, false, fmt.Sprintf("Username %q already exists, but no verified email matched %q", op.UserProps.Username, op.UserProps.Email), err @@ -114,26 +126,68 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (u case err != nil: return 0, false, false, "Unable to create a new user account due to a unexpected error. Ask a site admin for help.", err } + act.UID = userID if err = db.Authz().GrantPendingPermissions(ctx, &database.GrantPendingPermissionsArgs{ UserID: userID, Perm: authz.Read, Type: authz.PermRepos, }); err != nil { - log15.Error("Failed to grant user pending permissions", "userID", userID, "error", err) + logger.Error( + "failed to grant user pending permissions", + sglog.Int32("userID", userID), + sglog.Error(err), + ) + // OK to continue, since this is a best-effort to improve the UX with some initial permissions available. } - serviceTypeArg := json.RawMessage(fmt.Sprintf(`{"serviceType": %q}`, op.ExternalAccount.ServiceType)) - if logErr := usagestats.LogBackendEvent(db, actor.FromContext(ctx).UID, deviceid.FromContext(ctx), "ExternalAuthSignupSucceeded", serviceTypeArg, serviceTypeArg, featureflag.GetEvaluatedFlagSet(ctx), nil); logErr != nil { - log15.Warn("Failed to log event ExternalAuthSignupSucceded", "error", logErr) + const eventName = "ExternalAuthSignupSucceeded" + args, err := json.Marshal(map[string]any{ + // NOTE: The conventional name should be "service_type", but keeping as-is for + // backwards capability. + "serviceType": op.ExternalAccount.ServiceType, + }) + if err != nil { + logger.Error( + "failed to marshal JSON for event log argument", + sglog.String("eventName", eventName), + sglog.Error(err), + ) + // OK to continue, we still want the event log to be created + } + + // NOTE: It is important to propagate the correct context that carries the + // information of the actor, especially whether the actor is a Sourcegraph + // operator or not. + err = usagestats.LogEvent( + ctx, + db, + usagestats.Event{ + EventName: eventName, + UserID: act.UID, + Argument: args, + Source: "BACKEND", + }, + ) + if err != nil { + logger.Error( + "failed to log event", + sglog.String("eventName", eventName), + sglog.Error(err), + ) } return userID, true, true, "", nil }() if err != nil { + const eventName = "ExternalAuthSignupFailed" serviceTypeArg := json.RawMessage(fmt.Sprintf(`{"serviceType": %q}`, op.ExternalAccount.ServiceType)) - if logErr := usagestats.LogBackendEvent(db, actor.FromContext(ctx).UID, deviceid.FromContext(ctx), "ExternalAuthSignupFailed", serviceTypeArg, serviceTypeArg, featureflag.GetEvaluatedFlagSet(ctx), nil); logErr != nil { - log15.Warn("Failed to log event ExternalAuthSignUpFailed", "error", logErr) + if logErr := usagestats.LogBackendEvent(db, actor.FromContext(ctx).UID, deviceid.FromContext(ctx), eventName, serviceTypeArg, serviceTypeArg, featureflag.GetEvaluatedFlagSet(ctx), nil); logErr != nil { + logger.Error( + "failed to log event", + sglog.String("eventName", eventName), + sglog.Error(err), + ) } return 0, safeErrMsg, err } @@ -162,7 +216,7 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (u // Create/update the external account and ensure it's associated with the user ID if !extAcctSaved { - err := extacc.AssociateUserAndSave(ctx, userID, op.ExternalAccount, op.ExternalAccountData) + err := externalAccountsStore.AssociateUserAndSave(ctx, userID, op.ExternalAccount, op.ExternalAccountData) if err != nil { return 0, "Unexpected error associating the external account with your Sourcegraph user. The most likely cause for this problem is that another Sourcegraph user is already linked with this external account. A site admin or the other user can unlink the account to fix this problem.", err } @@ -172,7 +226,12 @@ func GetAndSaveUser(ctx context.Context, db database.DB, op GetAndSaveUserOp) (u Perm: authz.Read, Type: authz.PermRepos, }); err != nil { - log15.Error("Failed to grant user pending permissions", "userID", userID, "error", err) + logger.Error( + "failed to grant user pending permissions", + sglog.Int32("userID", userID), + sglog.Error(err), + ) + // OK to continue, since this is a best-effort to improve the UX with some initial permissions available. } } diff --git a/cmd/frontend/auth/user_test.go b/cmd/frontend/auth/user_test.go index 880116c96efb..cc7b83af470e 100644 --- a/cmd/frontend/auth/user_test.go +++ b/cmd/frontend/auth/user_test.go @@ -7,9 +7,12 @@ import ( "testing" "github.com/davecgh/go-spew/spew" + mockrequire "github.com/derision-test/go-mockgen/testutil/require" "github.com/sergi/go-diff/diffmatchpatch" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/actor" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/errcode" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -419,6 +422,49 @@ func TestGetAndSaveUser(t *testing.T) { } }) } + + t.Run("Sourcegraph operator actor should be propagated", func(t *testing.T) { + ctx := context.Background() + + errNotFound := &errcode.Mock{ + IsNotFound: true, + } + usersStore := database.NewMockUserStore() + usersStore.GetByVerifiedEmailFunc.SetDefaultReturn(nil, errNotFound) + externalAccountsStore := database.NewMockUserExternalAccountsStore() + externalAccountsStore.LookupUserAndSaveFunc.SetDefaultReturn(0, errNotFound) + externalAccountsStore.CreateUserAndSaveFunc.SetDefaultHook(func(ctx context.Context, _ database.NewUser, _ extsvc.AccountSpec, _ extsvc.AccountData) (int32, error) { + require.True(t, actor.FromContext(ctx).SourcegraphOperator, "the actor should be a Sourcegraph operator") + return 1, nil + }) + eventLogsStore := database.NewMockEventLogStore() + eventLogsStore.BulkInsertFunc.SetDefaultHook(func(ctx context.Context, _ []*database.Event) error { + require.True(t, actor.FromContext(ctx).SourcegraphOperator, "the actor should be a Sourcegraph operator") + return nil + }) + db := database.NewMockDB() + db.UsersFunc.SetDefaultReturn(usersStore) + db.UserExternalAccountsFunc.SetDefaultReturn(externalAccountsStore) + db.AuthzFunc.SetDefaultReturn(database.NewMockAuthzStore()) + db.EventLogsFunc.SetDefaultReturn(eventLogsStore) + + _, _, err := GetAndSaveUser( + ctx, + db, + GetAndSaveUserOp{ + UserProps: database.NewUser{ + EmailIsVerified: true, + }, + ExternalAccount: extsvc.AccountSpec{ + ServiceType: auth.SourcegraphOperatorProviderType, + }, + ExternalAccountData: extsvc.AccountData{}, + CreateIfNotExist: true, + }, + ) + require.NoError(t, err) + mockrequire.Called(t, externalAccountsStore.CreateUserAndSaveFunc) + }) } type userInfo struct { diff --git a/cmd/frontend/graphqlbackend/graphqlbackend.go b/cmd/frontend/graphqlbackend/graphqlbackend.go index 3590bf253e01..0f61372022f7 100644 --- a/cmd/frontend/graphqlbackend/graphqlbackend.go +++ b/cmd/frontend/graphqlbackend/graphqlbackend.go @@ -32,6 +32,7 @@ import ( sgtrace "github.com/sourcegraph/sourcegraph/internal/trace" "github.com/sourcegraph/sourcegraph/internal/trace/policy" "github.com/sourcegraph/sourcegraph/internal/types" + "github.com/sourcegraph/sourcegraph/internal/usagestats" "github.com/sourcegraph/sourcegraph/lib/errors" ) @@ -43,11 +44,13 @@ var ( }, []string{"type", "field", "error", "source", "request_name"}) ) -type prometheusTracer struct { +type requestTracer struct { + DB database.DB tracer trace.OpenTracingTracer + logger sglog.Logger } -func (t *prometheusTracer) TraceQuery(ctx context.Context, queryString string, operationName string, variables map[string]any, varTypes map[string]*introspection.Type) (context.Context, trace.TraceQueryFinishFunc) { +func (t *requestTracer) TraceQuery(ctx context.Context, queryString string, operationName string, variables map[string]any, varTypes map[string]*introspection.Type) (context.Context, trace.TraceQueryFinishFunc) { start := time.Now() var finish trace.TraceQueryFinishFunc if policy.ShouldTrace(ctx) { @@ -64,6 +67,48 @@ func (t *prometheusTracer) TraceQuery(ctx context.Context, queryString string, o currentUserID = a.UID } + // 🚨 SECURITY: We want to log every single operation the Sourcegraph operator + // has done on the instance, so we need to do additional logging here. Sometimes + // we would end up having logging twice for the same operation (here and the web + // app), but we would not want to risk missing logging operations. Also in the + // future, we expect audit logging of Sourcegraph operators to live outside the + // instance, which makes this pattern less of a concern in terms of redundancy. + if a.SourcegraphOperator { + const eventName = "SourcegraphOperatorGraphQLRequest" + args, err := json.Marshal(map[string]any{ + "queryString": queryString, + "variables": variables, + }) + if err != nil { + t.logger.Error( + "failed to marshal JSON for event log argument", + sglog.String("eventName", eventName), + sglog.Error(err), + ) + } + + // NOTE: It is important to propagate the correct context that carries the + // information of the actor, especially whether the actor is a Sourcegraph + // operator or not. + err = usagestats.LogEvent( + ctx, + t.DB, + usagestats.Event{ + EventName: eventName, + UserID: a.UID, + Argument: args, + Source: "BACKEND", + }, + ) + if err != nil { + t.logger.Error( + "failed to log event", + sglog.String("eventName", eventName), + sglog.Error(err), + ) + } + } + // Requests made by our JS frontend and other internal things will have a concrete name attached to the // request which allows us to (softly) differentiate it from end-user API requests. For example, // /.api/graphql?Foobar where Foobar is the name of the request we make. If there is not a request name, @@ -96,7 +141,7 @@ VARIABLES } } -func (prometheusTracer) TraceField(ctx context.Context, label, typeName, fieldName string, trivial bool, args map[string]any) (context.Context, trace.TraceFieldFinishFunc) { +func (requestTracer) TraceField(ctx context.Context, label, typeName, fieldName string, trivial bool, args map[string]any) (context.Context, trace.TraceFieldFinishFunc) { // We don't call into t.OpenTracingTracer.TraceField since it generates too many spans which is really hard to read. start := time.Now() return ctx, func(err *gqlerrors.QueryError) { @@ -111,7 +156,7 @@ func (prometheusTracer) TraceField(ctx context.Context, label, typeName, fieldNa } } -func (t prometheusTracer) TraceValidation(ctx context.Context) trace.TraceValidationFinishFunc { +func (t requestTracer) TraceValidation(ctx context.Context) trace.TraceValidationFinishFunc { var finish trace.TraceValidationFinishFunc if policy.ShouldTrace(ctx) { finish = t.tracer.TraceValidation(ctx) @@ -458,10 +503,14 @@ func NewSchema( } } + logger := sglog.Scoped("GraphQL", "general GraphQL logging") return graphql.ParseSchema( strings.Join(schemas, "\n"), resolver, - graphql.Tracer(&prometheusTracer{}), + graphql.Tracer(&requestTracer{ + DB: db, + logger: logger, + }), graphql.UseStringDescriptions(), ) } diff --git a/cmd/frontend/graphqlbackend/search_test.go b/cmd/frontend/graphqlbackend/search_test.go index 2ac272c995a7..dd724080e62e 100644 --- a/cmd/frontend/graphqlbackend/search_test.go +++ b/cmd/frontend/graphqlbackend/search_test.go @@ -117,7 +117,7 @@ func TestSearch(t *testing.T) { gsClient.ResolveRevisionFunc.SetDefaultHook(tc.repoRevsMock) sr := newSchemaResolver(db, gsClient) - schema, err := graphql.ParseSchema(mainSchema, sr, graphql.Tracer(&prometheusTracer{})) + schema, err := graphql.ParseSchema(mainSchema, sr, graphql.Tracer(&requestTracer{})) if err != nil { t.Fatal(err) } diff --git a/cmd/frontend/internal/cli/http.go b/cmd/frontend/internal/cli/http.go index 06d3cd7ec90f..d343eb8240dd 100644 --- a/cmd/frontend/internal/cli/http.go +++ b/cmd/frontend/internal/cli/http.go @@ -63,7 +63,7 @@ func newExternalHTTPHandler( // 🚨 SECURITY: The HTTP API should not accept cookies as authentication, except from trusted // origins, to avoid CSRF attacks. See session.CookieMiddlewareWithCSRFSafety for details. apiHandler = session.CookieMiddlewareWithCSRFSafety(logger, db, apiHandler, corsAllowHeader, isTrustedOrigin) // API accepts cookies with special header - apiHandler = internalhttpapi.AccessTokenAuthMiddleware(db, apiHandler) // API accepts access tokens + apiHandler = internalhttpapi.AccessTokenAuthMiddleware(db, logger, apiHandler) // API accepts access tokens apiHandler = requestclient.HTTPMiddleware(apiHandler) apiHandler = gziphandler.GzipHandler(apiHandler) if envvar.SourcegraphDotComMode() { @@ -85,8 +85,8 @@ func newExternalHTTPHandler( appHandler = actor.AnonymousUIDMiddleware(appHandler) appHandler = authMiddlewares.App(appHandler) // 🚨 SECURITY: auth middleware appHandler = middleware.OpenGraphMetadataMiddleware(db.FeatureFlags(), appHandler) - appHandler = session.CookieMiddleware(logger, db, appHandler) // app accepts cookies - appHandler = internalhttpapi.AccessTokenAuthMiddleware(db, appHandler) // app accepts access tokens + appHandler = session.CookieMiddleware(logger, db, appHandler) // app accepts cookies + appHandler = internalhttpapi.AccessTokenAuthMiddleware(db, logger, appHandler) // app accepts access tokens appHandler = requestclient.HTTPMiddleware(appHandler) if envvar.SourcegraphDotComMode() { appHandler = deviceid.Middleware(appHandler) diff --git a/cmd/frontend/internal/httpapi/auth.go b/cmd/frontend/internal/httpapi/auth.go index 5622b14fb7cf..44061eacb909 100644 --- a/cmd/frontend/internal/httpapi/auth.go +++ b/cmd/frontend/internal/httpapi/auth.go @@ -1,9 +1,11 @@ package httpapi import ( + "encoding/json" "net/http" + "time" - "github.com/inconshreveable/log15" + "github.com/sourcegraph/log" "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/auth" @@ -16,7 +18,8 @@ import ( // AccessTokenAuthMiddleware authenticates the user based on the // token query parameter or the "Authorization" header. -func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { +func AccessTokenAuthMiddleware(db database.DB, logger log.Logger, next http.Handler) http.Handler { + logger = logger.Scoped("accessTokenAuth", "Access token authentication middleware") return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Add("Vary", "Authorization") @@ -38,7 +41,11 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { if err != nil { if authz.IsUnrecognizedScheme(err) { // Ignore Authorization headers that we don't handle. - log15.Warn("Ignoring unrecognized Authorization header.", "err", err, "value", headerValue) + logger.Warn( + "ignoring unrecognized Authorization header", + log.String("value", headerValue), + log.Error(err), + ) next.ServeHTTP(w, r) return } @@ -46,7 +53,7 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { // Report errors on malformed Authorization headers for schemes we do handle, to // make it clear to the client that their request is not proceeding with their // supplied credentials. - log15.Error("Invalid Authorization header.", "err", err) + logger.Error("invalid Authorization header", log.Error(err)) http.Error(w, "Invalid Authorization header.", http.StatusUnauthorized) return } @@ -72,16 +79,43 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { subjectUserID, err := db.AccessTokens().Lookup(r.Context(), token, requiredScope) if err != nil { if err == database.ErrAccessTokenNotFound || errors.HasType(err, database.InvalidTokenError{}) { - log15.Error("AccessTokenAuthMiddleware.invalidAccessToken", "token", token, "error", err) + logger.Error( + "invalid access token", + log.String("token", token), + log.Error(err), + ) http.Error(w, "Invalid access token.", http.StatusUnauthorized) return } - log15.Error("AccessTokenAuthMiddleware.lookingUpAccessToken.", "token", token, "error", err) + logger.Error( + "failed to look up access token", + log.String("token", token), + log.Error(err), + ) http.Error(w, err.Error(), http.StatusInternalServerError) return } + // FIXME: Can we find a way to do this only for SOAP users? + soapCount, err := db.UserExternalAccounts().Count( + r.Context(), + database.ExternalAccountsListOptions{ + UserID: subjectUserID, + ServiceType: auth.SourcegraphOperatorProviderType, + }, + ) + if err != nil { + logger.Error( + "failed to list user external accounts", + log.Int32("subjectUserID", subjectUserID), + log.Error(err), + ) + http.Error(w, err.Error(), http.StatusInternalServerError) + return + } + sourcegraphOperator := soapCount > 0 + // Determine the actor's user ID. var actorUserID int32 if sudoUser == "" { @@ -90,7 +124,11 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { // 🚨 SECURITY: Confirm that the sudo token's subject is still a site admin, to // prevent users from retaining site admin privileges after being demoted. if err := auth.CheckUserIsSiteAdmin(r.Context(), db, subjectUserID); err != nil { - log15.Error("Sudo access token's subject is not a site admin.", "subjectUserID", subjectUserID, "err", err) + logger.Error( + "sudo access token's subject is not a site admin", + log.Int32("subjectUserID", subjectUserID), + log.Error(err), + ) http.Error(w, "The subject user of a sudo access token must be a site admin.", http.StatusForbidden) return } @@ -99,7 +137,11 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { // the necessary scope in the Lookup call above. user, err := db.Users().GetByUsername(r.Context(), sudoUser) if err != nil { - log15.Error("Invalid username used with sudo access token.", "sudoUser", sudoUser, "err", err) + logger.Error( + "invalid username used with sudo access token", + log.String("sudoUser", sudoUser), + log.Error(err), + ) var message string if errcode.IsNotFound(err) { message = "Unable to sudo to nonexistent user." @@ -110,10 +152,53 @@ func AccessTokenAuthMiddleware(db database.DB, next http.Handler) http.Handler { return } actorUserID = user.ID - log15.Debug("HTTP request used sudo token.", "requestURI", r.URL.RequestURI(), "tokenSubjectUserID", subjectUserID, "actorUserID", actorUserID, "actorUsername", user.Username) + logger.Debug( + "HTTP request used sudo token", + log.String("requestURI", r.URL.RequestURI()), + log.Int32("tokenSubjectUserID", subjectUserID), + log.Int32("actorUserID", actorUserID), + log.String("actorUsername", user.Username), + ) + + args, err := json.Marshal(map[string]any{ + "sudo_user_id": actorUserID, + }) + if err != nil { + logger.Error( + "failed to marshal JSON for security event log argument", + log.String("eventName", string(database.SecurityEventAccessTokenImpersonated)), + log.String("sudoUser", sudoUser), + log.Error(err), + ) + // OK to continue, we still want the security event log to be created + } + db.SecurityEventLogs().LogEvent( + actor.WithActor( + r.Context(), + &actor.Actor{ + UID: subjectUserID, + SourcegraphOperator: sourcegraphOperator, + }, + ), + &database.SecurityEvent{ + Name: database.SecurityEventAccessTokenImpersonated, + UserID: uint32(subjectUserID), + Argument: args, + Source: "BACKEND", + Timestamp: time.Now(), + }, + ) } - r = r.WithContext(actor.WithActor(r.Context(), &actor.Actor{UID: actorUserID})) + r = r.WithContext( + actor.WithActor( + r.Context(), + &actor.Actor{ + UID: actorUserID, + SourcegraphOperator: sourcegraphOperator, + }, + ), + ) } next.ServeHTTP(w, r) diff --git a/cmd/frontend/internal/httpapi/auth_test.go b/cmd/frontend/internal/httpapi/auth_test.go index ffb1547f0dfe..50bf0c48203c 100644 --- a/cmd/frontend/internal/httpapi/auth_test.go +++ b/cmd/frontend/internal/httpapi/auth_test.go @@ -9,6 +9,8 @@ import ( "testing" mockrequire "github.com/derision-test/go-mockgen/testutil/require" + "github.com/sourcegraph/log/logtest" + "github.com/stretchr/testify/require" "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/authz" @@ -19,14 +21,17 @@ import ( func TestAccessTokenAuthMiddleware(t *testing.T) { newHandler := func(db database.DB) http.Handler { - return AccessTokenAuthMiddleware(db, http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - actor := actor.FromContext(r.Context()) - if actor.IsAuthenticated() { - fmt.Fprintf(w, "user %v", actor.UID) - } else { - fmt.Fprint(w, "no user") - } - })) + return AccessTokenAuthMiddleware( + db, + logtest.NoOp(t), + http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + actor := actor.FromContext(r.Context()) + if actor.IsAuthenticated() { + _, _ = fmt.Fprintf(w, "user %v", actor.UID) + } else { + _, _ = fmt.Fprint(w, "no user") + } + })) } checkHTTPResponse := func(t *testing.T, db database.DB, req *http.Request, wantStatusCode int, wantBody string) { @@ -40,9 +45,11 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { } } + db := database.NewMockDB() + db.UserExternalAccountsFunc.SetDefaultReturn(database.NewMockUserExternalAccountsStore()) t.Run("no header", func(t *testing.T) { req, _ := http.NewRequest("GET", "/", nil) - checkHTTPResponse(t, database.NewMockDB(), req, http.StatusOK, "no user") + checkHTTPResponse(t, db, req, http.StatusOK, "no user") }) // Test that the absence of an Authorization header doesn't unset the actor provided by a prior @@ -50,14 +57,14 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { t.Run("no header, actor present", func(t *testing.T) { req, _ := http.NewRequest("GET", "/", nil) req = req.WithContext(actor.WithActor(context.Background(), &actor.Actor{UID: 123})) - checkHTTPResponse(t, database.NewMockDB(), req, http.StatusOK, "user 123") + checkHTTPResponse(t, db, req, http.StatusOK, "user 123") }) for _, unrecognizedHeaderValue := range []string{"x", "x y", "Basic abcd"} { t.Run("unrecognized header "+unrecognizedHeaderValue, func(t *testing.T) { req, _ := http.NewRequest("GET", "/", nil) req.Header.Set("Authorization", unrecognizedHeaderValue) - checkHTTPResponse(t, database.NewMockDB(), req, http.StatusOK, "no user") + checkHTTPResponse(t, db, req, http.StatusOK, "no user") }) } @@ -65,7 +72,7 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { t.Run("invalid header "+invalidHeaderValue, func(t *testing.T) { req, _ := http.NewRequest("GET", "/", nil) req.Header.Set("Authorization", invalidHeaderValue) - checkHTTPResponse(t, database.NewMockDB(), req, http.StatusUnauthorized, "Invalid Authorization header.\n") + checkHTTPResponse(t, db, req, http.StatusUnauthorized, "Invalid Authorization header.\n") }) } @@ -75,7 +82,6 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { accessTokens := database.NewMockAccessTokenStore() accessTokens.LookupFunc.SetDefaultReturn(0, database.InvalidTokenError{}) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) checkHTTPResponse(t, db, req, http.StatusUnauthorized, "Invalid access token.\n") @@ -97,7 +103,6 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { } return 123, nil }) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) checkHTTPResponse(t, db, req, http.StatusOK, "user 123") @@ -121,7 +126,6 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { } return 123, nil }) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) checkHTTPResponse(t, db, req, http.StatusOK, "user 123") @@ -155,7 +159,6 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { } return 123, nil }) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) checkHTTPResponse(t, db, req, http.StatusOK, "user 123") @@ -192,14 +195,63 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { return &types.User{ID: 456, SiteAdmin: true}, nil }) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) db.UsersFunc.SetDefaultReturn(users) + db.SecurityEventLogsFunc.SetDefaultReturn(database.NewMockSecurityEventLogsStore()) + + checkHTTPResponse(t, db, req, http.StatusOK, "user 456") + mockrequire.Called(t, accessTokens.LookupFunc) + mockrequire.Called(t, users.GetByIDFunc) + mockrequire.Called(t, users.GetByUsernameFunc) + }) + + t.Run("valid sudo token as a Sourcegraph operator", func(t *testing.T) { + req, _ := http.NewRequest("GET", "/", nil) + req.Header.Set("Authorization", `token-sudo token="abcdef",user="alice"`) + + accessTokens := database.NewMockAccessTokenStore() + accessTokens.LookupFunc.SetDefaultHook(func(_ context.Context, tokenHexEncoded, requiredScope string) (subjectUserID int32, err error) { + if want := "abcdef"; tokenHexEncoded != want { + t.Errorf("got %q, want %q", tokenHexEncoded, want) + } + if want := authz.ScopeSiteAdminSudo; requiredScope != want { + t.Errorf("got %q, want %q", requiredScope, want) + } + return 123, nil + }) + + users := database.NewMockUserStore() + users.GetByIDFunc.SetDefaultHook(func(ctx context.Context, userID int32) (*types.User, error) { + if want := int32(123); userID != want { + t.Errorf("got %d, want %d", userID, want) + } + return &types.User{ID: userID, SiteAdmin: true}, nil + }) + users.GetByUsernameFunc.SetDefaultHook(func(ctx context.Context, username string) (*types.User, error) { + if want := "alice"; username != want { + t.Errorf("got %q, want %q", username, want) + } + return &types.User{ID: 456, SiteAdmin: true}, nil + }) + + userExternalAccountsStore := database.NewMockUserExternalAccountsStore() + userExternalAccountsStore.CountFunc.SetDefaultReturn(1, nil) + + securityEventLogsStore := database.NewMockSecurityEventLogsStore() + securityEventLogsStore.LogEventFunc.SetDefaultHook(func(ctx context.Context, _ *database.SecurityEvent) { + require.True(t, actor.FromContext(ctx).SourcegraphOperator, "the actor should be a Sourcegraph operator") + }) + + db.AccessTokensFunc.SetDefaultReturn(accessTokens) + db.UsersFunc.SetDefaultReturn(users) + db.UserExternalAccountsFunc.SetDefaultReturn(userExternalAccountsStore) + db.SecurityEventLogsFunc.SetDefaultReturn(securityEventLogsStore) checkHTTPResponse(t, db, req, http.StatusOK, "user 456") mockrequire.Called(t, accessTokens.LookupFunc) mockrequire.Called(t, users.GetByIDFunc) mockrequire.Called(t, users.GetByUsernameFunc) + mockrequire.Called(t, securityEventLogsStore.LogEventFunc) }) // Test that if a sudo token's subject user is not a site admin (which means they were demoted @@ -227,7 +279,6 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { return &types.User{ID: userID, SiteAdmin: false}, nil }) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) db.UsersFunc.SetDefaultReturn(users) @@ -265,7 +316,6 @@ func TestAccessTokenAuthMiddleware(t *testing.T) { return nil, &errcode.Mock{IsNotFound: true} }) - db := database.NewMockDB() db.AccessTokensFunc.SetDefaultReturn(accessTokens) db.UsersFunc.SetDefaultReturn(users) diff --git a/enterprise/cmd/frontend/internal/auth/init.go b/enterprise/cmd/frontend/internal/auth/init.go index fcf63714ac70..54043c8306ea 100644 --- a/enterprise/cmd/frontend/internal/auth/init.go +++ b/enterprise/cmd/frontend/internal/auth/init.go @@ -19,6 +19,7 @@ import ( "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/saml" "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/sourcegraphoperator" "github.com/sourcegraph/sourcegraph/enterprise/internal/licensing" + internalauth "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database" ) @@ -112,7 +113,7 @@ func ssoSignOutHandler(w http.ResponseWriter, r *http.Request) { } } - if p := sourcegraphoperator.GetOIDCProvider(sourcegraphoperator.ProviderType); p != nil { + if p := sourcegraphoperator.GetOIDCProvider(internalauth.SourcegraphOperatorProviderType); p != nil { _, err := openidconnect.SignOut( w, r, diff --git a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/config.go b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/config.go index bd94026f792c..6bb4db25e5d3 100644 --- a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/config.go +++ b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/config.go @@ -8,6 +8,7 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/auth/providers" "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/openidconnect" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/conf/conftypes" ) @@ -18,7 +19,7 @@ import ( func GetOIDCProvider(id string) *openidconnect.Provider { p, ok := providers.GetProviderByConfigID( providers.ConfigID{ - Type: ProviderType, + Type: auth.SourcegraphOperatorProviderType, ID: id, }, ).(*provider) @@ -37,13 +38,13 @@ func Init() { conf.ContributeValidator(validateConfig) p := NewProvider(*cloudSiteConfig.AuthProviders.SourcegraphOperator) - logger := log.Scoped(ProviderType, "Sourcegraph Operator config watch") + logger := log.Scoped(auth.SourcegraphOperatorProviderType, "Sourcegraph Operator config watch") go func() { if err := p.Refresh(context.Background()); err != nil { logger.Error("failed to fetch Sourcegraph Operator service provider metadata", log.Error(err)) } }() - providers.Update(ProviderType, []providers.Provider{p}) + providers.Update(auth.SourcegraphOperatorProviderType, []providers.Provider{p}) } func validateConfig(c conftypes.SiteConfigQuerier) (problems conf.Problems) { diff --git a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware.go b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware.go index f79abbc0c85c..c50985160a99 100644 --- a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware.go +++ b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware.go @@ -1,6 +1,7 @@ package sourcegraphoperator import ( + "encoding/json" "net/http" "strings" "time" @@ -12,12 +13,13 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/external/session" "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/openidconnect" "github.com/sourcegraph/sourcegraph/internal/actor" + internalauth "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/lib/errors" ) // All Sourcegraph Operator endpoints are under this path prefix. -const authPrefix = auth.AuthURLPrefix + "/" + ProviderType +const authPrefix = auth.AuthURLPrefix + "/" + internalauth.SourcegraphOperatorProviderType // Middleware is middleware for Sourcegraph Operator authentication, adding // endpoints under the auth path prefix ("/.auth") to enable the login flow and @@ -66,7 +68,7 @@ const ( ) func authHandler(db database.DB) func(w http.ResponseWriter, r *http.Request) { - logger := log.Scoped(ProviderType+".authHandler", "Sourcegraph Operator authentication handler") + logger := log.Scoped(internalauth.SourcegraphOperatorProviderType+".authHandler", "Sourcegraph Operator authentication handler") return func(w http.ResponseWriter, r *http.Request) { switch strings.TrimPrefix(r.URL.Path, authPrefix) { case "/login": // Endpoint that starts the Authentication Request Code Flow. @@ -89,12 +91,15 @@ func authHandler(db database.DB) func(w http.ResponseWriter, r *http.Request) { p, ok := providers.GetProviderByConfigID( providers.ConfigID{ - Type: ProviderType, - ID: ProviderType, + Type: internalauth.SourcegraphOperatorProviderType, + ID: internalauth.SourcegraphOperatorProviderType, }, ).(*provider) if !ok { - logger.Error("failed to get Sourcegraph Operator authentication provider", log.Error(errors.Errorf("no authentication provider found with ID %q", ProviderType))) + logger.Error( + "failed to get Sourcegraph Operator authentication provider", + log.Error(errors.Errorf("no authentication provider found with ID %q", internalauth.SourcegraphOperatorProviderType)), + ) http.Error(w, "Misconfigured authentication provider.", http.StatusInternalServerError) return } @@ -118,7 +123,7 @@ func authHandler(db database.DB) func(w http.ResponseWriter, r *http.Request) { // If the "sourcegraph-operator" is the only external account associated with the // user, that means the user is a pure Sourcegraph Operator which should have // designated and aggressive session expiry. - if len(extAccts) == 1 && extAccts[0].ServiceType == ProviderType { + if len(extAccts) == 1 && extAccts[0].ServiceType == internalauth.SourcegraphOperatorProviderType { // The user session will only live at most for the remaining duration from the // "users.created_at" compared to the current time. // @@ -130,16 +135,41 @@ func authHandler(db database.DB) func(w http.ResponseWriter, r *http.Request) { // the second session only lives for 50 minutes. expiry = time.Until(result.User.CreatedAt.Add(p.lifecycleDuration())) if expiry <= 0 { + // Let's do a proactive hard delete since the background worker hasn't caught up + + // Help exclude Sourcegraph operator related events from analytics + ctx := actor.WithActor( + r.Context(), + &actor.Actor{ + SourcegraphOperator: true, + }, + ) + err = db.Users().HardDelete(ctx, result.User.ID) + if err != nil { + logger.Error("failed to proactively clean up the expire user account", log.Error(err)) + } + http.Error(w, "The retrieved user account lifecycle has already expired, please re-authenticate.", http.StatusUnauthorized) return } } - if err = session.SetActor(w, r, actor.FromUser(result.User.ID), expiry, result.User.CreatedAt); err != nil { + + act := &actor.Actor{ + UID: result.User.ID, + SourcegraphOperator: true, + } + err = session.SetActor(w, r, act, expiry, result.User.CreatedAt) + if err != nil { logger.Error("failed to authenticate with Sourcegraph Operator", log.Error(errors.Wrap(err, "initiate session"))) http.Error(w, "Authentication failed. Try signing in again (and clearing cookies for the current site). The error was: could not initiate session.", http.StatusInternalServerError) return } + // NOTE: It is important to wrap the request context with the correct actor and + // use it onwards to be able to mark all generated event logs with + // `"sourcegraph_operator": true`. + ctx := actor.WithActor(r.Context(), act) + if err = session.SetData(w, r, SessionKey, result.SessionData); err != nil { // It's not fatal if this fails. It just means we won't be able to sign the user // out of the OP. @@ -148,10 +178,31 @@ func authHandler(db database.DB) func(w http.ResponseWriter, r *http.Request) { log.String("message", "The session is still secure, but Sourcegraph will be unable to revoke the user's token or redirect the user to the end-session endpoint after the user signs out of Sourcegraph."), log.Error(err), ) + } else { + args, err := json.Marshal(map[string]any{ + "session_expiry_seconds": int64(expiry.Seconds()), + }) + if err != nil { + logger.Error( + "failed to marshal JSON for security event log argument", + log.String("eventName", string(database.SecurityEventNameSignInSucceeded)), + log.Error(err), + ) + } + db.SecurityEventLogs().LogEvent( + ctx, + &database.SecurityEvent{ + Name: database.SecurityEventNameSignInSucceeded, + UserID: uint32(act.UID), + Argument: args, + Source: "BACKEND", + Timestamp: time.Now(), + }, + ) } if !result.User.SiteAdmin { - err = db.Users().SetIsSiteAdmin(r.Context(), result.User.ID, true) + err = db.Users().SetIsSiteAdmin(ctx, result.User.ID, true) if err != nil { logger.Error("failed to update Sourcegraph Operator as site admin", log.Error(err)) http.Error(w, "Authentication failed. Try signing in again (and clearing cookies for the current site). The error was: could not set as site admin.", http.StatusInternalServerError) diff --git a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware_test.go b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware_test.go index 0a7972206872..fe49b9ecb99f 100644 --- a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware_test.go +++ b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/middleware_test.go @@ -24,6 +24,7 @@ import ( "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/openidconnect" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" "github.com/sourcegraph/sourcegraph/internal/actor" + internalauth "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/extsvc" "github.com/sourcegraph/sourcegraph/internal/types" @@ -100,7 +101,7 @@ func newOIDCIDServer(t *testing.T, code string, providerConfig *cloud.SchemaAuth }) auth.MockGetAndSaveUser = func(ctx context.Context, op auth.GetAndSaveUserOp) (userID int32, safeErrMsg string, err error) { - if op.ExternalAccount.ServiceType == ProviderType && + if op.ExternalAccount.ServiceType == internalauth.SourcegraphOperatorProviderType && op.ExternalAccount.ServiceID == providerConfig.Issuer && op.ExternalAccount.ClientID == testClientID && op.ExternalAccount.AccountID == testOIDCUser { @@ -143,7 +144,7 @@ func TestMiddleware(t *testing.T) { []*extsvc.Account{ { AccountSpec: extsvc.AccountSpec{ - ServiceType: ProviderType, + ServiceType: internalauth.SourcegraphOperatorProviderType, }, }, }, @@ -152,6 +153,7 @@ func TestMiddleware(t *testing.T) { db := database.NewMockDB() db.UsersFunc.SetDefaultReturn(usersStore) db.UserExternalAccountsFunc.SetDefaultReturn(userExternalAccountsStore) + db.SecurityEventLogsFunc.SetDefaultReturn(database.NewMockSecurityEventLogsStore()) h := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {}) authedHandler := http.NewServeMux() @@ -320,6 +322,10 @@ func TestMiddleware(t *testing.T) { CreatedAt: time.Now().Add(-61 * time.Minute), }, nil }) + usersStore.HardDeleteFunc.SetDefaultHook(func(ctx context.Context, _ int32) error { + require.True(t, actor.FromContext(ctx).SourcegraphOperator, "the actor should be a Sourcegraph operator") + return nil + }) state := &openidconnect.AuthnState{ CSRFToken: "good", @@ -350,5 +356,6 @@ func TestMiddleware(t *testing.T) { body, err := io.ReadAll(resp.Body) require.NoError(t, err) assert.Contains(t, string(body), "The retrieved user account lifecycle has already expired") + mockrequire.Called(t, usersStore.HardDeleteFunc) }) } diff --git a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/provider.go b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/provider.go index ca07b1d76187..bc5efd86c19d 100644 --- a/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/provider.go +++ b/enterprise/cmd/frontend/internal/auth/sourcegraphoperator/provider.go @@ -6,13 +6,10 @@ import ( "github.com/sourcegraph/sourcegraph/cmd/frontend/auth/providers" "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/openidconnect" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/schema" ) -// ProviderType is the unique identifier of the Sourcegraph Operator -// authentication provider. -const ProviderType = "sourcegraph-operator" - // provider is an implementation of providers.Provider for the Sourcegraph // Operator authentication. type provider struct { @@ -31,11 +28,11 @@ func NewProvider(config cloud.SchemaAuthProviderSourcegraphOperator) providers.P AllowSignup: &allowSignUp, ClientID: config.ClientID, ClientSecret: config.ClientSecret, - ConfigID: ProviderType, + ConfigID: auth.SourcegraphOperatorProviderType, DisplayName: "Sourcegraph Operators", Issuer: config.Issuer, RequireEmailDomain: "sourcegraph.com", - Type: ProviderType, + Type: auth.SourcegraphOperatorProviderType, }, authPrefix, ).(*openidconnect.Provider), @@ -50,7 +47,7 @@ func (p *provider) Config() schema.AuthProviders { // non-Sourcegraph employees. return schema.AuthProviders{ Openidconnect: &schema.OpenIDConnectAuthProvider{ - ConfigID: ProviderType, + ConfigID: auth.SourcegraphOperatorProviderType, }, } } diff --git a/enterprise/cmd/frontend/internal/licensing/enforcement/users.go b/enterprise/cmd/frontend/internal/licensing/enforcement/users.go index 8e6c3e0346af..3a1a881d63cc 100644 --- a/enterprise/cmd/frontend/internal/licensing/enforcement/users.go +++ b/enterprise/cmd/frontend/internal/licensing/enforcement/users.go @@ -7,9 +7,9 @@ import ( "github.com/inconshreveable/log15" "github.com/sourcegraph/sourcegraph/cmd/frontend/envvar" - "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/sourcegraphoperator" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" "github.com/sourcegraph/sourcegraph/enterprise/internal/licensing" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/errcode" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -25,7 +25,7 @@ func NewBeforeCreateUserHook() func(context.Context, database.DB, *extsvc.Accoun // // NOTE: It is important to make sure the Sourcegraph Operator authentication // provider is actually enabled. - if spec != nil && spec.ServiceType == sourcegraphoperator.ProviderType && + if spec != nil && spec.ServiceType == auth.SourcegraphOperatorProviderType && cloud.SiteConfig().SourcegraphOperatorAuthProviderEnabled() { return nil } diff --git a/enterprise/cmd/frontend/internal/licensing/enforcement/users_test.go b/enterprise/cmd/frontend/internal/licensing/enforcement/users_test.go index c6a681dbf58a..a0dea8a116a6 100644 --- a/enterprise/cmd/frontend/internal/licensing/enforcement/users_test.go +++ b/enterprise/cmd/frontend/internal/licensing/enforcement/users_test.go @@ -10,10 +10,10 @@ import ( "github.com/stretchr/testify/assert" "github.com/sourcegraph/sourcegraph/cmd/frontend/envvar" - "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/sourcegraphoperator" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" "github.com/sourcegraph/sourcegraph/enterprise/internal/license" "github.com/sourcegraph/sourcegraph/enterprise/internal/licensing" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/basestore" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -97,7 +97,7 @@ func TestEnforcement_PreCreateUser(t *testing.T) { ) }, spec: &extsvc.AccountSpec{ - ServiceType: sourcegraphoperator.ProviderType, + ServiceType: auth.SourcegraphOperatorProviderType, }, }, } diff --git a/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner.go b/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner.go index 233653e7a4bb..bcfc2e60f35e 100644 --- a/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner.go +++ b/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner.go @@ -11,6 +11,8 @@ import ( workerdb "github.com/sourcegraph/sourcegraph/cmd/worker/shared/init/db" "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/sourcegraphoperator" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" + "github.com/sourcegraph/sourcegraph/internal/actor" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/basestore" "github.com/sourcegraph/sourcegraph/internal/env" @@ -82,7 +84,7 @@ WHERE AND users.created_at <= %s GROUP BY user_id HAVING COUNT(*) = 1 `, - sourcegraphoperator.ProviderType, + auth.SourcegraphOperatorProviderType, time.Now().Add(-1*h.lifecycleDuration), ) userIDs, err := basestore.ScanInt32s(h.db.QueryContext(ctx, q.Query(sqlf.PostgresBindVar), q.Args()...)) @@ -90,6 +92,13 @@ GROUP BY user_id HAVING COUNT(*) = 1 return errors.Wrap(err, "query user IDs") } + // Help exclude Sourcegraph operator related events from analytics + ctx = actor.WithActor( + ctx, + &actor.Actor{ + SourcegraphOperator: true, + }, + ) err = h.db.Users().HardDeleteList(ctx, userIDs) if err != nil && !errcode.IsNotFound(err) { return errors.Wrap(err, "hard delete users") diff --git a/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner_test.go b/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner_test.go index 18219a34365a..91d6da4f4c74 100644 --- a/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner_test.go +++ b/enterprise/cmd/frontend/worker/auth/sourcegraph_operator_cleaner_test.go @@ -9,8 +9,8 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - "github.com/sourcegraph/sourcegraph/enterprise/cmd/frontend/internal/auth/sourcegraphoperator" "github.com/sourcegraph/sourcegraph/enterprise/internal/cloud" + "github.com/sourcegraph/sourcegraph/internal/auth" "github.com/sourcegraph/sourcegraph/internal/database" "github.com/sourcegraph/sourcegraph/internal/database/dbtest" "github.com/sourcegraph/sourcegraph/internal/extsvc" @@ -33,7 +33,7 @@ func TestSourcegraphOperatorCleanHandler(t *testing.T) { ) ctx := context.Background() - logger := logtest.Scoped(t) + logger := logtest.NoOp(t) db := database.NewDB(logger, dbtest.NewDB(logger, t)) handler := sourcegraphOperatorCleanHandler{ db: db, @@ -64,7 +64,7 @@ func TestSourcegraphOperatorCleanHandler(t *testing.T) { Username: "morgan", }, extsvc.AccountSpec{ - ServiceType: sourcegraphoperator.ProviderType, + ServiceType: auth.SourcegraphOperatorProviderType, ServiceID: "https://sourcegraph.com", ClientID: "soap", AccountID: "morgan", @@ -93,7 +93,7 @@ func TestSourcegraphOperatorCleanHandler(t *testing.T) { Username: "jordan", }, extsvc.AccountSpec{ - ServiceType: sourcegraphoperator.ProviderType, + ServiceType: auth.SourcegraphOperatorProviderType, ServiceID: "https://sourcegraph.com", ClientID: "soap", AccountID: "jordan", @@ -108,7 +108,7 @@ func TestSourcegraphOperatorCleanHandler(t *testing.T) { Username: "riley", }, extsvc.AccountSpec{ - ServiceType: sourcegraphoperator.ProviderType, + ServiceType: auth.SourcegraphOperatorProviderType, ServiceID: "https://sourcegraph.com", ClientID: "soap", AccountID: "riley", diff --git a/internal/actor/actor.go b/internal/actor/actor.go index 6021a3888f02..1edd0bff44dc 100644 --- a/internal/actor/actor.go +++ b/internal/actor/actor.go @@ -36,6 +36,9 @@ type Actor struct { // not tied to a specific user). Internal bool `json:",omitempty"` + // SourcegraphOperator indicates whether the actor is a Sourcegraph operator user account. + SourcegraphOperator bool `json:",omitempty"` + // FromSessionCookie is whether a session cookie was used to authenticate the actor. It is used // to selectively display a logout link. (If the actor wasn't authenticated with a session // cookie, logout would be ineffective.) diff --git a/internal/auth/const.go b/internal/auth/const.go new file mode 100644 index 000000000000..9d89a6d2ca22 --- /dev/null +++ b/internal/auth/const.go @@ -0,0 +1,5 @@ +package auth + +// SourcegraphOperatorProviderType is the unique identifier of the Sourcegraph +// Operator authentication provider. +const SourcegraphOperatorProviderType = "sourcegraph-operator" diff --git a/internal/database/event_logs.go b/internal/database/event_logs.go index db984faf807d..34e97eb78214 100644 --- a/internal/database/event_logs.go +++ b/internal/database/event_logs.go @@ -10,16 +10,17 @@ import ( "time" "github.com/gofrs/uuid" - "github.com/keegancsmith/sqlf" "github.com/lib/pq" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/conf" "github.com/sourcegraph/sourcegraph/internal/database/basestore" "github.com/sourcegraph/sourcegraph/internal/database/batch" "github.com/sourcegraph/sourcegraph/internal/database/dbutil" "github.com/sourcegraph/sourcegraph/internal/eventlogger" "github.com/sourcegraph/sourcegraph/internal/featureflag" + "github.com/sourcegraph/sourcegraph/internal/jsonc" "github.com/sourcegraph/sourcegraph/internal/timeutil" "github.com/sourcegraph/sourcegraph/internal/types" "github.com/sourcegraph/sourcegraph/internal/version" @@ -216,6 +217,7 @@ func (l *eventLogStore) BulkInsert(ctx context.Context, events []*Event) error { return *in } + actor := actor.FromContext(ctx) rowValues := make(chan []any, len(events)) for _, event := range events { featureFlags, err := json.Marshal(event.EvaluatedFlagSet) @@ -223,6 +225,20 @@ func (l *eventLogStore) BulkInsert(ctx context.Context, events []*Event) error { return err } + // Add an attribution for Sourcegraph operator to be distinguished in our analytics pipelines + publicArgument := coalesce(event.PublicArgument) + if actor.SourcegraphOperator { + result, err := jsonc.Edit( + string(publicArgument), + true, + "sourcegraph_operator", + ) + publicArgument = json.RawMessage(result) + if err != nil { + return errors.Wrap(err, `edit "public_argument" for Sourcegraph operator`) + } + } + rowValues <- []any{ event.Name, // 🚨 SECURITY: It is important to sanitize event URL before @@ -233,7 +249,7 @@ func (l *eventLogStore) BulkInsert(ctx context.Context, events []*Event) error { event.AnonymousUserID, event.Source, coalesce(event.Argument), - coalesce(event.PublicArgument), + publicArgument, version.Version(), event.Timestamp.UTC(), featureFlags, diff --git a/internal/database/security_event_logs.go b/internal/database/security_event_logs.go index aac6b0351797..695a50e87bd2 100644 --- a/internal/database/security_event_logs.go +++ b/internal/database/security_event_logs.go @@ -9,8 +9,10 @@ import ( "github.com/keegancsmith/sqlf" "github.com/sourcegraph/log" + "github.com/sourcegraph/sourcegraph/internal/actor" "github.com/sourcegraph/sourcegraph/internal/audit" "github.com/sourcegraph/sourcegraph/internal/database/basestore" + "github.com/sourcegraph/sourcegraph/internal/jsonc" "github.com/sourcegraph/sourcegraph/internal/trace" "github.com/sourcegraph/sourcegraph/internal/version" "github.com/sourcegraph/sourcegraph/lib/errors" @@ -42,9 +44,10 @@ const ( SecurityEventNameAccessGranted SecurityEventName = "AccessGranted" - SecurityEventAccessTokenCreated SecurityEventName = "AccessTokenCreated" - SecurityEventAccessTokenDeleted SecurityEventName = "AccessTokenDeleted" - SecurityEventAccessTokenHardDeleted SecurityEventName = "AccessTokenHardDeleted" + SecurityEventAccessTokenCreated SecurityEventName = "AccessTokenCreated" + SecurityEventAccessTokenDeleted SecurityEventName = "AccessTokenDeleted" + SecurityEventAccessTokenHardDeleted SecurityEventName = "AccessTokenHardDeleted" + SecurityEventAccessTokenImpersonated SecurityEventName = "AccessTokenImpersonated" SecurityEventGitHubAuthSucceeded SecurityEventName = "GitHubAuthSucceeded" SecurityEventGitHubAuthFailed SecurityEventName = "GitHubAuthFailed" @@ -104,8 +107,22 @@ func (s *securityEventLogsStore) Insert(ctx context.Context, event *SecurityEven } func (s *securityEventLogsStore) InsertList(ctx context.Context, events []*SecurityEvent) error { + actor := actor.FromContext(ctx) vals := make([]*sqlf.Query, len(events)) for index, event := range events { + // Add an attribution for Sourcegraph operator to be distinguished in our analytics pipelines + if actor.SourcegraphOperator { + result, err := jsonc.Edit( + event.marshalArgumentAsJSON(), + true, + "sourcegraph_operator", + ) + event.Argument = json.RawMessage(result) + if err != nil { + return errors.Wrap(err, `edit "argument" for Sourcegraph operator`) + } + } + vals[index] = sqlf.Sprintf(`(%s, %s, %s, %s, %s, %s, %s, %s)`, event.Name, event.URL,