allow site admins to list org members & view user/org settings on dotcom (#63963)

Followup from https://github.com/sourcegraph/sourcegraph/pull/63941.

I forgot to make it so site admins could list org members in that PR.

As for the new ability for site admins to see user and org settings:
Site admins could already add themselves to orgs as members and see
settings that way. I considered still keeping it stricter, but it is
valuable for site admins to be able to view settings to help users
troubleshoot.


## Test plan

In dotcom mode, as a site admin, view a user or org (that the site admin
is not a member of). Confirm that the settings can be viewed.

Author: sqs

cmd/frontend/graphqlbackend/feature_flags.go

@@ -5,6 +5,7 @@ import (
 
 	"github.com/graph-gophers/graphql-go"
 	"github.com/graph-gophers/graphql-go/relay"
+
 	"github.com/sourcegraph/sourcegraph/internal/actor"
 	"github.com/sourcegraph/sourcegraph/internal/gqlutil"
 
@@ -172,7 +173,7 @@ func (r *schemaResolver) OrganizationFeatureFlagValue(ctx context.Context, args
 		return false, err
 	}
 	// same behavior as if the flag does not exist
-	if err := auth.CheckOrgAccess(ctx, r.db, org); err != nil {
+	if err := auth.CheckOrgAccessOrSiteAdmin(ctx, r.db, org); err != nil {
 		return false, nil
 	}
 

cmd/frontend/graphqlbackend/org.go

@@ -105,7 +105,7 @@ func (o *OrgResolver) Members(ctx context.Context, args struct {
 ) (*graphqlutil.ConnectionResolver[*UserResolver], error) {
 	// 🚨 SECURITY: On dotcom, only an org's members can list its members.
 	if dotcom.SourcegraphDotComMode() {
-		if err := auth.CheckOrgAccess(ctx, o.db, o.org.ID); err != nil {
+		if err := auth.CheckOrgAccessOrSiteAdmin(ctx, o.db, o.org.ID); err != nil {
 			return nil, err
 		}
 	}

cmd/frontend/graphqlbackend/org_test.go

@@ -123,9 +123,8 @@ func TestOrganizationMembers(t *testing.T) {
 	})
 
 	t.Run("non-members", func(t *testing.T) {
-		users.GetByCurrentAuthUserFunc.SetDefaultReturn(&types.User{Username: "xavier", ID: 10}, nil)
-
 		t.Run("can list members on non-dotcom", func(t *testing.T) {
+			users.GetByCurrentAuthUserFunc.SetDefaultReturn(&types.User{Username: "xavier", ID: 10}, nil)
 			dotcom.MockSourcegraphDotComMode(t, false)
 			RunTests(t, []*Test{
 				{
@@ -152,7 +151,36 @@ func TestOrganizationMembers(t *testing.T) {
 			})
 		})
 
-		t.Run("cannot list members on dotcom", func(t *testing.T) {
+		t.Run("who are site admin can list members on non-dotcom", func(t *testing.T) {
+			users.GetByCurrentAuthUserFunc.SetDefaultReturn(&types.User{Username: "xavier", ID: 10, SiteAdmin: true}, nil)
+			dotcom.MockSourcegraphDotComMode(t, true)
+			RunTests(t, []*Test{
+				{
+					Schema: mustParseGraphQLSchema(t, db),
+					Query: `
+						{
+							organization(name: "acme") {
+								members {
+									nodes { username }
+								}
+							}
+						}
+					`,
+					ExpectedResult: `
+						{
+							"organization": {
+								"members": {
+									"nodes": [{"username": "alice"}, {"username": "bob"}]
+								}
+							}
+						}
+					`,
+				},
+			})
+		})
+
+		t.Run("who are not site admin cannot list members on dotcom", func(t *testing.T) {
+			users.GetByCurrentAuthUserFunc.SetDefaultReturn(&types.User{Username: "xavier", ID: 10}, nil)
 			dotcom.MockSourcegraphDotComMode(t, true)
 			RunTests(t, []*Test{
 				{

cmd/frontend/graphqlbackend/settings_mutation_test.go

@@ -9,6 +9,7 @@ import (
 
 	"github.com/sourcegraph/sourcegraph/internal/actor"
 	"github.com/sourcegraph/sourcegraph/internal/api"
+	"github.com/sourcegraph/sourcegraph/internal/auth"
 	"github.com/sourcegraph/sourcegraph/internal/database/dbmocks"
 	"github.com/sourcegraph/sourcegraph/internal/dotcom"
 	"github.com/sourcegraph/sourcegraph/internal/gitserver"
@@ -142,15 +143,6 @@ func TestSettingsMutation(t *testing.T) {
 					})
 				},
 			},
-			{
-				name: "site admin",
-				ctx:  actor.WithActor(context.Background(), &actor.Actor{UID: 2}),
-				setup: func() {
-					users.GetByIDFunc.SetDefaultHook(func(ctx context.Context, id int32) (*types.User, error) {
-						return &types.User{ID: id, SiteAdmin: true}, nil
-					})
-				},
-			},
 		}
 		for _, test := range tests {
 			t.Run(test.name, func(t *testing.T) {
@@ -165,7 +157,7 @@ func TestSettingsMutation(t *testing.T) {
 					},
 				)
 				got := fmt.Sprintf("%v", err)
-				want := "must be authenticated as user with id 1"
+				want := auth.ErrMustBeSiteAdminOrSameUser.Error()
 				assert.Equal(t, want, got)
 			})
 		}

cmd/frontend/graphqlbackend/settings_subject.go

@@ -10,7 +10,6 @@ import (
 	"github.com/sourcegraph/sourcegraph/internal/api"
 	"github.com/sourcegraph/sourcegraph/internal/auth"
 	"github.com/sourcegraph/sourcegraph/internal/database"
-	"github.com/sourcegraph/sourcegraph/internal/dotcom"
 	"github.com/sourcegraph/sourcegraph/lib/errors"
 )
 
@@ -102,31 +101,16 @@ func settingsSubjectForNodeAndCheckAccess(ctx context.Context, n Node) (*setting
 		subject.site = s
 
 	case *UserResolver:
-		// 🚨 SECURITY: Only the authenticated user can view their settings on
-		// Sourcegraph.com.
-		if dotcom.SourcegraphDotComMode() {
-			if err := auth.CheckSameUser(ctx, s.user.ID); err != nil {
-				return nil, err
-			}
-		} else {
-			// 🚨 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
-			}
+		// 🚨 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
 		}
 		subject.user = s
 
 	case *OrgResolver:
-		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
-			}
+		// 🚨 SECURITY: Only org members or site admins can view the org settings.
+		if err := auth.CheckOrgAccessOrSiteAdmin(ctx, s.db, s.org.ID); err != nil {
+			return nil, err
 		}
 		subject.org = s
 

cmd/frontend/graphqlbackend/settings_subject_test.go

@@ -67,11 +67,11 @@ func TestSettingsSubjectForNodeAndCheckAccess(t *testing.T) {
 			wantSubject: &settingsSubjectResolver{user: &UserResolver{user: &types.User{ID: userID}, db: db}},
 		},
 		{
-			name:      "user settings - site admin on dotcom",
-			node:      &UserResolver{user: &types.User{ID: userID}, db: db},
-			actor:     actor.FromActualUser(&types.User{ID: otherUserID, SiteAdmin: true}),
-			isDotcom:  true,
-			wantError: &auth.InsufficientAuthorizationError{},
+			name:        "user settings - site admin on dotcom",
+			node:        &UserResolver{user: &types.User{ID: userID}, db: db},
+			actor:       actor.FromActualUser(&types.User{ID: otherUserID, SiteAdmin: true}),
+			isDotcom:    true,
+			wantSubject: &settingsSubjectResolver{user: &UserResolver{user: &types.User{ID: userID}, db: db}},
 		},
 		{
 			name:      "user settings - different user",
@@ -98,11 +98,11 @@ func TestSettingsSubjectForNodeAndCheckAccess(t *testing.T) {
 			wantSubject: &settingsSubjectResolver{org: &OrgResolver{org: &types.Org{ID: orgID}, db: db}},
 		},
 		{
-			name:      "org settings - non-member site admin on dotcom",
-			node:      &OrgResolver{org: &types.Org{ID: orgID}, db: db},
-			actor:     actor.FromActualUser(&types.User{ID: otherUserID, SiteAdmin: true}),
-			isDotcom:  true,
-			wantError: auth.ErrNotAnOrgMember,
+			name:        "org settings - non-member site admin on dotcom",
+			node:        &OrgResolver{org: &types.Org{ID: orgID}, db: db},
+			actor:       actor.FromActualUser(&types.User{ID: otherUserID, SiteAdmin: true}),
+			isDotcom:    true,
+			wantSubject: &settingsSubjectResolver{org: &OrgResolver{org: &types.Org{ID: orgID}, db: db}},
 		},
 		{
 			name:      "unknown node type",

cmd/frontend/graphqlbackend/user.go

@@ -516,7 +516,7 @@ func (r *schemaResolver) ChangeCodyPlan(ctx context.Context, args *changeCodyPla
 	}
 
 	// 🚨 SECURITY: Only the authenticated user can update their properties.
-	if err := auth.CheckSameUser(ctx, userID); err != nil {
+	if err := auth.CheckSiteAdminOrSameUser(ctx, r.db, userID); err != nil {
 		return nil, err
 	}
 

cmd/frontend/graphqlbackend/user_test.go

@@ -251,17 +251,6 @@ func TestUser_LatestSettings(t *testing.T) {
 				})
 			},
 		},
-		{
-			name:     "site admin on dotcom",
-			isDotcom: true,
-			ctx:      actor.WithActor(context.Background(), &actor.Actor{UID: 2}),
-			wantErr:  "must be authenticated as user with id 1",
-			setup: func() {
-				users.GetByIDFunc.SetDefaultHook(func(ctx context.Context, id int32) (*types.User, error) {
-					return &types.User{ID: id, SiteAdmin: true}, nil
-				})
-			},
-		},
 	}
 	for _, test := range tests {
 		t.Run(test.name, func(t *testing.T) {

internal/auth/orgs.go

@@ -21,15 +21,6 @@ func CheckOrgAccessOrSiteAdmin(ctx context.Context, db database.DB, orgID int32)
 	return checkOrgAccess(ctx, db, orgID, true)
 }
 
-// CheckOrgAccess returns an error if the user is not a member of the
-// organization with the specified ID.
-//
-// It is used when an action on an org can be performed by the organization's
-// members, but nobody else.
-func CheckOrgAccess(ctx context.Context, db database.DB, orgID int32) error {
-	return checkOrgAccess(ctx, db, orgID, false)
-}
-
 // checkOrgAccess is a helper method used above which allows optionally allowing
 // site admins to access all organizations.
 func checkOrgAccess(ctx context.Context, db database.DB, orgID int32, allowAdmin bool) error {

internal/auth/site_admin.go

@@ -72,8 +72,9 @@ func CheckSiteAdminOrSameUser(ctx context.Context, db database.DB, subjectUserID
 	return CheckSiteAdminOrSameUserFromActor(a, db, subjectUserID)
 }
 
-// CheckSameUser returns an error if the user is not the user specified by
-// subjectUserID.
+// CheckSameUser returns an error if the user is not the user specified by subjectUserID. It is used
+// for actions that can ONLY be performed by that user. In most cases, site admins should also be
+// able to perform the action, and you should use CheckSiteAdminOrSameUser instead.
 func CheckSameUser(ctx context.Context, subjectUserID int32) error {
 	a := actor.FromContext(ctx)
 	if a.IsInternal() || (a.IsAuthenticated() && a.UID == subjectUserID) {