explicitly check viewer access to settings in GraphQL API (#63945)

Previously, a user's or org's settings were protected from unauthorized
access in the GraphQL API by access checks far from the actual
`SettingCascade` and `LatestSettings` implementations in most cases.
This did not present a security issue because on Sourcegraph.com we
prevented users from getting a reference to an org they aren't in, and
user settings had the right manual access checks.

But the access checks are too far away from the actual resolver methods
(so it'd be easy to make a mistake) and were not consistently
implemented. **Now, all checks for view-settings-of-subject access go
through the same function, `settingsSubjectForNodeAndCheckAccess`.**

One substantive security change is that now site admins may NOT view the
settings of an org they are not a member of. This is in line with site
admins on dotcom not being able to see user settings. This is important
because settings might contain secrets in the future (e.g., OpenCtx
provider config). Site admins may still add themselves to the org and
then view the settings, but that creates more of an audit trail and and
we may lock down that as well.



## Test plan

Unit tests, also browse around the UI and ensure there are no code paths
where our assertion triggers.

Author: sqs

cmd/frontend/graphqlbackend/BUILD.bazel

@@ -450,6 +450,7 @@ go_test(
         "search_test.go",
         "settings_cascade_test.go",
         "settings_mutation_test.go",
+        "settings_subject_test.go",
         "site_admin_test.go",
         "site_alerts_test.go",
         "site_config_change_connection_test.go",

cmd/frontend/graphqlbackend/default_settings.go

@@ -30,12 +30,18 @@ func marshalDefaultSettingsGQLID(defaultSettingsID string) graphql.ID {
 
 func (r *defaultSettingsResolver) ID() graphql.ID { return marshalDefaultSettingsGQLID(r.gqlID) }
 
-func (r *defaultSettingsResolver) LatestSettings(_ context.Context) (*settingsResolver, error) {
+func (r *defaultSettingsResolver) LatestSettings(ctx context.Context) (*settingsResolver, error) {
+	// 🚨 SECURITY: Check that the viewer can access these settings.
+	subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r)
+	if err != nil {
+		return nil, err
+	}
+
 	settings := &api.Settings{
 		Subject:  api.SettingsSubject{Default: true},
 		Contents: `{"experimentalFeatures": {}}`,
 	}
-	return &settingsResolver{db: r.db, subject: &settingsSubjectResolver{defaultSettings: r}, settings: settings}, nil
+	return &settingsResolver{db: r.db, subject: subject, settings: settings}, nil
 }
 
 func (r *defaultSettingsResolver) SettingsURL() *string { return nil }
@@ -44,8 +50,15 @@ func (r *defaultSettingsResolver) ViewerCanAdminister(_ context.Context) (bool,
 	return false, nil
 }
 
-func (r *defaultSettingsResolver) SettingsCascade() *settingsCascade {
-	return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{defaultSettings: r}}
+func (r *defaultSettingsResolver) SettingsCascade(ctx context.Context) (*settingsCascade, error) {
+	// 🚨 SECURITY: Check that the viewer can access these settings.
+	subject, err := settingsSubjectForNodeAndCheckAccess(ctx, r)
+	if err != nil {
+		return nil, err
+	}
+	return &settingsCascade{db: r.db, subject: subject}, nil
 }
 
-func (r *defaultSettingsResolver) ConfigurationCascade() *settingsCascade { return r.SettingsCascade() }
+func (r *defaultSettingsResolver) ConfigurationCascade(ctx context.Context) (*settingsCascade, error) {
+	return r.SettingsCascade(ctx)
+}

cmd/frontend/graphqlbackend/org.go

@@ -11,7 +11,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/cmd/frontend/graphqlbackend/graphqlutil"
 	"github.com/sourcegraph/sourcegraph/internal/actor"
 	sgactor "github.com/sourcegraph/sourcegraph/internal/actor"
-	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/auth"
 	"github.com/sourcegraph/sourcegraph/internal/authz/permssync"
 	"github.com/sourcegraph/sourcegraph/internal/database"
@@ -234,33 +233,36 @@ func (s *membersConnectionStore) UnmarshalCursor(cursor string, _ database.Order
 	return []any{nodeID}, nil
 }
 
-func (o *OrgResolver) settingsSubject() api.SettingsSubject {
-	return api.SettingsSubject{Org: &o.org.ID}
-}
-
 func (o *OrgResolver) LatestSettings(ctx context.Context) (*settingsResolver, error) {
-	// 🚨 SECURITY: Only organization members and site admins (not on cloud) may access the settings,
-	// because they may contain secrets or other sensitive data.
-	if err := auth.CheckOrgAccessOrSiteAdmin(ctx, o.db, o.org.ID); err != nil {
+	// 🚨 SECURITY: Check that the viewer can access these settings.
+	subject, err := settingsSubjectForNodeAndCheckAccess(ctx, o)
+	if err != nil {
 		return nil, err
 	}
 
-	settings, err := o.db.Settings().GetLatest(ctx, o.settingsSubject())
+	settings, err := o.db.Settings().GetLatest(ctx, subject.toSubject())
 	if err != nil {
 		return nil, err
 	}
 	if settings == nil {
 		return nil, nil
 	}
 
-	return &settingsResolver{db: o.db, subject: &settingsSubjectResolver{org: o}, settings: settings}, nil
+	return &settingsResolver{db: o.db, subject: subject, settings: settings}, nil
 }
 
-func (o *OrgResolver) SettingsCascade() *settingsCascade {
-	return &settingsCascade{db: o.db, subject: &settingsSubjectResolver{org: o}}
+func (o *OrgResolver) SettingsCascade(ctx context.Context) (*settingsCascade, error) {
+	// 🚨 SECURITY: Check that the viewer can access these settings.
+	subject, err := settingsSubjectForNodeAndCheckAccess(ctx, o)
+	if err != nil {
+		return nil, err
+	}
+	return &settingsCascade{db: o.db, subject: subject}, nil
 }
 
-func (o *OrgResolver) ConfigurationCascade() *settingsCascade { return o.SettingsCascade() }
+func (o *OrgResolver) ConfigurationCascade(ctx context.Context) (*settingsCascade, error) {
+	return o.SettingsCascade(ctx)
+}
 
 func (o *OrgResolver) ViewerPendingInvitation(ctx context.Context) (*organizationInvitationResolver, error) {
 	if actor := sgactor.FromContext(ctx); actor.IsAuthenticated() {

cmd/frontend/graphqlbackend/settings.go

@@ -64,6 +64,9 @@ var globalSettingsAllowEdits, _ = strconv.ParseBool(env.Get("GLOBAL_SETTINGS_ALL
 // like database.Settings.CreateIfUpToDate, except it handles notifying the
 // query-runner if any saved queries have changed.
 func settingsCreateIfUpToDate(ctx context.Context, db database.DB, subject *settingsSubjectResolver, lastID *int32, authorUserID int32, contents string) (latestSetting *api.Settings, err error) {
+	// 🚨 SECURITY: Ensure that we've already checked the viewer's access to the subject's settings.
+	subject.assertCheckedAccess()
+
 	if os.Getenv("GLOBAL_SETTINGS_FILE") != "" && subject.site != nil && !globalSettingsAllowEdits {
 		return nil, errors.New("Updating global settings not allowed when using GLOBAL_SETTINGS_FILE")
 	}

cmd/frontend/graphqlbackend/settings_cascade.go

@@ -25,6 +25,9 @@ type settingsCascade struct {
 }
 
 func (r *settingsCascade) Subjects(ctx context.Context) ([]*settingsSubjectResolver, error) {
+	// 🚨 SECURITY: Ensure that we've already checked the viewer's access to the subject's settings.
+	r.subject.assertCheckedAccess()
+
 	subjects, err := settings.RelevantSubjects(ctx, r.db, r.subject.toSubject())
 	if err != nil {
 		return nil, err
@@ -34,6 +37,9 @@ func (r *settingsCascade) Subjects(ctx context.Context) ([]*settingsSubjectResol
 }
 
 func (r *settingsCascade) Final(ctx context.Context) (string, error) {
+	// 🚨 SECURITY: Ensure that we've already checked the viewer's access to the subject's settings.
+	r.subject.assertCheckedAccess()
+
 	settingsTyped, err := settings.Final(ctx, r.db, r.subject.toSubject())
 	if err != nil {
 		return "", err
@@ -48,6 +54,9 @@ func (r *settingsCascade) Merged(ctx context.Context) (_ *configurationResolver,
 	tr, ctx := trace.New(ctx, "SettingsCascade.Merged")
 	defer tr.EndWithErr(&err)
 
+	// 🚨 SECURITY: Ensure that we've already checked the viewer's access to the subject's settings.
+	r.subject.assertCheckedAccess()
+
 	var messages []string
 	s, err := r.Final(ctx)
 	if err != nil {
@@ -61,10 +70,19 @@ func (r *schemaResolver) ViewerSettings(ctx context.Context) (*settingsCascade,
 	if err != nil {
 		return nil, err
 	}
-	if user == nil {
-		return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{site: NewSiteResolver(log.Scoped("settings"), r.db)}}, nil
+
+	var viewerNode Node
+	if user != nil {
+		viewerNode = user
+	} else {
+		viewerNode = NewSiteResolver(log.Scoped("settings"), r.db)
+	}
+
+	settingsSubject, err := settingsSubjectForNodeAndCheckAccess(ctx, viewerNode)
+	if err != nil {
+		return nil, err
 	}
-	return &settingsCascade{db: r.db, subject: &settingsSubjectResolver{user: user}}, nil
+	return &settingsCascade{db: r.db, subject: settingsSubject}, nil
 }
 
 // Deprecated: in the GraphQL API

cmd/frontend/graphqlbackend/settings_cascade_test.go

@@ -9,7 +9,9 @@ import (
 
 func TestSubjects(t *testing.T) {
 	t.Run("Default settings are included", func(t *testing.T) {
-		cascade := &settingsCascade{db: dbmocks.NewMockDB(), subject: &settingsSubjectResolver{site: NewSiteResolver(nil, nil)}}
+		subject := &settingsSubjectResolver{site: NewSiteResolver(nil, nil)}
+		subject.mockCheckedAccessForTest()
+		cascade := &settingsCascade{db: dbmocks.NewMockDB(), subject: subject}
 		subjects, err := cascade.Subjects(context.Background())
 		if err != nil {
 			t.Fatal(err)

cmd/frontend/graphqlbackend/settings_mutation.go

@@ -53,7 +53,7 @@ func (r *schemaResolver) SettingsMutation(ctx context.Context, args *settingsMut
 		return nil, err
 	}
 
-	subject, err := settingsSubjectForNode(ctx, n)
+	subject, err := settingsSubjectForNodeAndCheckAccess(ctx, n)
 	if err != nil {
 		return nil, err
 	}

cmd/frontend/graphqlbackend/settings_subject.go

@@ -20,7 +20,7 @@ func (r *schemaResolver) SettingsSubject(ctx context.Context, args *struct{ ID g
 		return nil, err
 	}
 
-	return settingsSubjectForNode(ctx, n)
+	return settingsSubjectForNodeAndCheckAccess(ctx, n)
 }
 
 var errUnknownSettingsSubject = errors.New("unknown settings subject")
@@ -31,29 +31,46 @@ type settingsSubjectResolver struct {
 	site            *siteResolver
 	org             *OrgResolver
 	user            *UserResolver
+
+	// 🚨 SECURITY: Only the settingsSubjectForNodeAndCheckAccess function can set this. It is used
+	// to ensure that access checks have been run on this value, so that we don't leak settings to
+	// an unauthorized viewer by an accidental bypass of access checks. This struct type is
+	// naturally constructed all over the place (because many types of nodes have settings), and it
+	// was too easy to bypass the access check accidentally.
+	checkedAccess_DO_NOT_SET_THIS_MANUALLY_OR_YOU_WILL_LEAK_SECRETS bool
+}
+
+func (r *settingsSubjectResolver) assertCheckedAccess() {
+	if !r.checkedAccess_DO_NOT_SET_THIS_MANUALLY_OR_YOU_WILL_LEAK_SECRETS {
+		panic("settingsSubjectResolver.assertCheckedAccess: access checks have not been run on this value")
+	}
 }
 
 func resolverForSubject(ctx context.Context, logger log.Logger, db database.DB, subject api.SettingsSubject) (*settingsSubjectResolver, error) {
-	switch {
-	case subject.Default:
+	if subject.Default {
 		return &settingsSubjectResolver{defaultSettings: newDefaultSettingsResolver(db)}, nil
+	}
+
+	var (
+		node Node
+		err  error
+	)
+	switch {
 	case subject.Site:
-		return &settingsSubjectResolver{site: NewSiteResolver(logger, db)}, nil
+		node = NewSiteResolver(logger, db)
 	case subject.Org != nil:
-		org, err := OrgByIDInt32(ctx, db, *subject.Org)
-		if err != nil {
-			return nil, err
-		}
-		return &settingsSubjectResolver{org: org}, nil
+		node, err = OrgByIDInt32(ctx, db, *subject.Org)
 	case subject.User != nil:
-		user, err := UserByIDInt32(ctx, db, *subject.User)
-		if err != nil {
-			return nil, err
-		}
-		return &settingsSubjectResolver{user: user}, nil
+		node, err = UserByIDInt32(ctx, db, *subject.User)
 	default:
-		return nil, errors.New("subject must have exactly one field set")
+		panic("subject must have exactly one field set")
+	}
+	if err != nil {
+		return nil, err
 	}
+
+	// 🚨 SECURITY: Call settingsSubjectForNode to reuse the security checks implemented there.
+	return settingsSubjectForNodeAndCheckAccess(ctx, node)
 }
 
 func resolversForSubjects(ctx context.Context, logger log.Logger, db database.DB, subjects []api.SettingsSubject) (_ []*settingsSubjectResolver, err error) {
@@ -67,12 +84,22 @@ func resolversForSubjects(ctx context.Context, logger log.Logger, db database.DB
 	return res, nil
 }
 
-// settingsSubjectForNode fetches the settings subject for the given Node. If
-// the node is not a valid settings subject, an error is returned.
-func settingsSubjectForNode(ctx context.Context, n Node) (*settingsSubjectResolver, error) {
+// settingsSubjectForNodeAndCheckAccess fetches the settings subject for the given Node. If the node
+// is not a valid settings subject, an error is returned.
+//
+// 🚨 SECURITY: This function also ensures that the actor is permitted to view the node's settings.
+// It is the ONLY place that the
+// (settingsSubjectResolver).checkedAccess_DO_NOT_SET_THIS_MANUALLY_OR_YOU_WILL_LEAK_SECRETS field
+// can be set.
+func settingsSubjectForNodeAndCheckAccess(ctx context.Context, n Node) (*settingsSubjectResolver, error) {
+	var subject settingsSubjectResolver
+
 	switch s := n.(type) {
+	case *defaultSettingsResolver:
+		subject.defaultSettings = s
+
 	case *siteResolver:
-		return &settingsSubjectResolver{site: s}, nil
+		subject.site = s
 
 	case *UserResolver:
 		// 🚨 SECURITY: Only the authenticated user can view their settings on
@@ -82,23 +109,35 @@ func settingsSubjectForNode(ctx context.Context, n Node) (*settingsSubjectResolv
 				return nil, err
 			}
 		} else {
-			// 🚨 SECURITY: Only the user and site admins are allowed to view the user's settings.
+			// 🚨 SECURITY: The user and site admins are allowed to view the user's settings otherwise.
 			if err := auth.CheckSiteAdminOrSameUser(ctx, s.db, s.user.ID); err != nil {
 				return nil, err
 			}
 		}
-		return &settingsSubjectResolver{user: s}, nil
+		subject.user = s
 
 	case *OrgResolver:
-		// 🚨 SECURITY: Check that the current user is a member of the org.
-		if err := auth.CheckOrgAccessOrSiteAdmin(ctx, s.db, s.org.ID); err != nil {
-			return nil, err
+		if dotcom.SourcegraphDotComMode() {
+			// 🚨 SECURITY: Only org members (not any site admin) can view org settings on Sourcegraph.com.
+			if err := auth.CheckOrgAccess(ctx, s.db, s.org.ID); err != nil {
+				return nil, err
+			}
+		} else {
+			// 🚨 SECURITY: Org members or site admins can view the org settings otherwise.
+			if err := auth.CheckOrgAccessOrSiteAdmin(ctx, s.db, s.org.ID); err != nil {
+				return nil, err
+			}
 		}
-		return &settingsSubjectResolver{org: s}, nil
+		subject.org = s
 
 	default:
 		return nil, errUnknownSettingsSubject
 	}
+
+	// 🚨 SECURITY: This is the ONLY place that this field can be set.
+	subject.checkedAccess_DO_NOT_SET_THIS_MANUALLY_OR_YOU_WILL_LEAK_SECRETS = true
+
+	return &subject, nil
 }
 
 func (s *settingsSubjectResolver) ToDefaultSettings() (*defaultSettingsResolver, bool) {
@@ -115,6 +154,8 @@ func (s *settingsSubjectResolver) ToUser() (*UserResolver, bool) { return s.user
 
 func (s *settingsSubjectResolver) toSubject() api.SettingsSubject {
 	switch {
+	case s.defaultSettings != nil:
+		return api.SettingsSubject{Default: true}
 	case s.site != nil:
 		return api.SettingsSubject{Site: true}
 	case s.org != nil:
@@ -186,21 +227,21 @@ func (s *settingsSubjectResolver) ViewerCanAdminister(ctx context.Context) (bool
 	}
 }
 
-func (s *settingsSubjectResolver) SettingsCascade() (*settingsCascade, error) {
+func (s *settingsSubjectResolver) SettingsCascade(ctx context.Context) (*settingsCascade, error) {
 	switch {
 	case s.defaultSettings != nil:
-		return s.defaultSettings.SettingsCascade(), nil
+		return s.defaultSettings.SettingsCascade(ctx)
 	case s.site != nil:
-		return s.site.SettingsCascade(), nil
+		return s.site.SettingsCascade(ctx)
 	case s.org != nil:
-		return s.org.SettingsCascade(), nil
+		return s.org.SettingsCascade(ctx)
 	case s.user != nil:
-		return s.user.SettingsCascade(), nil
+		return s.user.SettingsCascade(ctx)
 	default:
 		return nil, errUnknownSettingsSubject
 	}
 }
 
-func (s *settingsSubjectResolver) ConfigurationCascade() (*settingsCascade, error) {
-	return s.SettingsCascade()
+func (s *settingsSubjectResolver) ConfigurationCascade(ctx context.Context) (*settingsCascade, error) {
+	return s.SettingsCascade(ctx)
 }