coder · Emyrk · Aug 15, 2024 · Aug 13, 2024 · Aug 13, 2024 · Aug 13, 2024
diff --git a/coderd/apidoc/docs.go b/coderd/apidoc/docs.go
diff --git a/coderd/apidoc/swagger.json b/coderd/apidoc/swagger.json
diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go
@@ -353,16 +353,28 @@ func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.Convert
 	return s.prepped.CompileToSQL(ctx, cfg)
 }
 
-// FakeAuthorizer is an Authorizer that always returns the same error.
+// FakeAuthorizer is an Authorizer that will return an error based on the
+// "ConditionalReturn" function. By default, **no error** is returned.
+// Meaning 'FakeAuthorizer' by default will never return "unauthorized".
 type FakeAuthorizer struct {
-	// AlwaysReturn is the error that will be returned by Authorize.
-	AlwaysReturn error
+	ConditionalReturn func(context.Context, rbac.Subject, policy.Action, rbac.Object) error
 }
 
 var _ rbac.Authorizer = (*FakeAuthorizer)(nil)
 
-func (d *FakeAuthorizer) Authorize(_ context.Context, _ rbac.Subject, _ policy.Action, _ rbac.Object) error {
-	return d.AlwaysReturn
+// AlwaysReturn is the error that will be returned by Authorize.
+func (d *FakeAuthorizer) AlwaysReturn(err error) *FakeAuthorizer {
+	d.ConditionalReturn = func(_ context.Context, _ rbac.Subject, _ policy.Action, _ rbac.Object) error {
+		return err
+	}
+	return d
+}
+
+func (d *FakeAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action policy.Action, object rbac.Object) error {
+	if d.ConditionalReturn != nil {
+		return d.ConditionalReturn(ctx, subject, action, object)
+	}
+	return nil
 }
 
 func (d *FakeAuthorizer) Prepare(_ context.Context, subject rbac.Subject, action policy.Action, _ string) (rbac.PreparedAuthorized, error) {

diff --git a/coderd/database/dbauthz/dbauthz.go b/coderd/database/dbauthz/dbauthz.go
@@ -1411,19 +1411,16 @@ func (q *querier) GetGroupMembersCountByGroupID(ctx context.Context, groupID uui
 	return memberCount, nil
 }
 
-func (q *querier) GetGroups(ctx context.Context) ([]database.Group, error) {
-	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err != nil {
-		return nil, err
+func (q *querier) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.Group, error) {
+	if err := q.authorizeContext(ctx, policy.ActionRead, rbac.ResourceSystem); err == nil {
+		// Optimize this query for system users as it is used in telemetry.
+		// Calling authz on all groups in a deployment for telemetry jobs is
+		// excessive. Most user calls should have some filtering applied to reduce
+		// the size of the set.
+		return q.db.GetGroups(ctx, arg)
 	}
-	return q.db.GetGroups(ctx)
-}
-
-func (q *querier) GetGroupsByOrganizationAndUserID(ctx context.Context, arg database.GetGroupsByOrganizationAndUserIDParams) ([]database.Group, error) {
-	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupsByOrganizationAndUserID)(ctx, arg)
-}
 
-func (q *querier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) {
-	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroupsByOrganizationID)(ctx, organizationID)
+	return fetchWithPostFilter(q.auth, policy.ActionRead, q.db.GetGroups)(ctx, arg)
 }
 
 func (q *querier) GetHealthSettings(ctx context.Context) (string, error) {

diff --git a/coderd/database/dbauthz/dbauthz_test.go b/coderd/database/dbauthz/dbauthz_test.go
@@ -81,7 +81,7 @@ func TestInTX(t *testing.T) {
 
 	db := dbmem.New()
 	q := dbauthz.New(db, &coderdtest.RecordingAuthorizer{
-		Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: xerrors.New("custom error")},
+		Wrapped: (&coderdtest.FakeAuthorizer{}).AlwaysReturn(xerrors.New("custom error")),
 	}, slog.Make(), coderdtest.AccessControlStorePointer())
 	actor := rbac.Subject{
 		ID:     uuid.NewString(),
@@ -110,7 +110,7 @@ func TestNew(t *testing.T) {
 		db  = dbmem.New()
 		exp = dbgen.Workspace(t, db, database.Workspace{})
 		rec = &coderdtest.RecordingAuthorizer{
-			Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
+			Wrapped: &coderdtest.FakeAuthorizer{},
 		}
 		subj = rbac.Subject{}
 		ctx  = dbauthz.As(context.Background(), rbac.Subject{})
@@ -135,7 +135,7 @@ func TestNew(t *testing.T) {
 func TestDBAuthzRecursive(t *testing.T) {
 	t.Parallel()
 	q := dbauthz.New(dbmem.New(), &coderdtest.RecordingAuthorizer{
-		Wrapped: &coderdtest.FakeAuthorizer{AlwaysReturn: nil},
+		Wrapped: &coderdtest.FakeAuthorizer{},
 	}, slog.Make(), coderdtest.AccessControlStorePointer())
 	actor := rbac.Subject{
 		ID:     uuid.NewString(),
@@ -342,18 +342,21 @@ func (s *MethodTestSuite) TestGroup() {
 		dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
 		check.Asserts(rbac.ResourceSystem, policy.ActionRead)
 	}))
-	s.Run("GetGroups", s.Subtest(func(db database.Store, check *expects) {
+	s.Run("System/GetGroups", s.Subtest(func(db database.Store, check *expects) {
 		_ = dbgen.Group(s.T(), db, database.Group{})
-		check.Asserts(rbac.ResourceSystem, policy.ActionRead)
+		check.Args(database.GetGroupsParams{}).
+			Asserts(rbac.ResourceSystem, policy.ActionRead)
 	}))
-	s.Run("GetGroupsByOrganizationAndUserID", s.Subtest(func(db database.Store, check *expects) {
+	s.Run("GetGroups", s.Subtest(func(db database.Store, check *expects) {
 		g := dbgen.Group(s.T(), db, database.Group{})
 		u := dbgen.User(s.T(), db, database.User{})
 		gm := dbgen.GroupMember(s.T(), db, database.GroupMemberTable{GroupID: g.ID, UserID: u.ID})
-		check.Args(database.GetGroupsByOrganizationAndUserIDParams{
+		check.Args(database.GetGroupsParams{
 			OrganizationID: g.OrganizationID,
-			UserID:         gm.UserID,
-		}).Asserts(g, policy.ActionRead)
+			HasMemberID:    gm.UserID,
+		}).Asserts(rbac.ResourceSystem, policy.ActionRead, g, policy.ActionRead).
+			// Fail the system resource skip
+			FailSystemObjectChecks()
 	}))
 	s.Run("InsertAllUsersGroup", s.Subtest(func(db database.Store, check *expects) {
 		o := dbgen.Organization(s.T(), db, database.Organization{})
@@ -597,12 +600,16 @@ func (s *MethodTestSuite) TestLicense() {
 }
 
 func (s *MethodTestSuite) TestOrganization() {
-	s.Run("GetGroupsByOrganizationID", s.Subtest(func(db database.Store, check *expects) {
+	s.Run("ByOrganization/GetGroups", s.Subtest(func(db database.Store, check *expects) {
 		o := dbgen.Organization(s.T(), db, database.Organization{})
 		a := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
 		b := dbgen.Group(s.T(), db, database.Group{OrganizationID: o.ID})
-		check.Args(o.ID).Asserts(a, policy.ActionRead, b, policy.ActionRead).
-			Returns([]database.Group{a, b})
+		check.Args(database.GetGroupsParams{
+			OrganizationID: o.ID,
+		}).Asserts(rbac.ResourceSystem, policy.ActionRead, a, policy.ActionRead, b, policy.ActionRead).
+			Returns([]database.Group{a, b}).
+			// Fail the system check shortcut
+			FailSystemObjectChecks()
 	}))
 	s.Run("GetOrganizationByID", s.Subtest(func(db database.Store, check *expects) {
 		o := dbgen.Organization(s.T(), db, database.Organization{})

diff --git a/coderd/database/dbauthz/setup_test.go b/coderd/database/dbauthz/setup_test.go
@@ -114,9 +114,7 @@ func (s *MethodTestSuite) Subtest(testCaseF func(db database.Store, check *expec
 		s.methodAccounting[methodName]++
 
 		db := dbmem.New()
-		fakeAuthorizer := &coderdtest.FakeAuthorizer{
-			AlwaysReturn: nil,
-		}
+		fakeAuthorizer := &coderdtest.FakeAuthorizer{}
 		rec := &coderdtest.RecordingAuthorizer{
 			Wrapped: fakeAuthorizer,
 		}
@@ -174,7 +172,11 @@ func (s *MethodTestSuite) Subtest(testCaseF func(db database.Store, check *expec
 		// Always run
 		s.Run("Success", func() {
 			rec.Reset()
-			fakeAuthorizer.AlwaysReturn = nil
+			if testCase.successAuthorizer != nil {
+				fakeAuthorizer.ConditionalReturn = testCase.successAuthorizer
+			} else {
+				fakeAuthorizer.AlwaysReturn(nil)
+			}
 
 			outputs, err := callMethod(ctx)
 			if testCase.err == nil {
@@ -232,7 +234,7 @@ func (s *MethodTestSuite) NoActorErrorTest(callMethod func(ctx context.Context)
 // Asserts that the error returned is a NotAuthorizedError.
 func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderdtest.FakeAuthorizer, testCase expects, callMethod func(ctx context.Context) ([]reflect.Value, error)) {
 	s.Run("NotAuthorized", func() {
-		az.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("Always fail authz"), rbac.Subject{}, "", rbac.Object{}, nil)
+		az.AlwaysReturn(rbac.ForbiddenWithInternal(xerrors.New("Always fail authz"), rbac.Subject{}, "", rbac.Object{}, nil))
 
 		// If we have assertions, that means the method should FAIL
 		// if RBAC will disallow the request. The returned error should
@@ -257,8 +259,8 @@ func (s *MethodTestSuite) NotAuthorizedErrorTest(ctx context.Context, az *coderd
 		// Pass in a canceled context
 		ctx, cancel := context.WithCancel(ctx)
 		cancel()
-		az.AlwaysReturn = rbac.ForbiddenWithInternal(&topdown.Error{Code: topdown.CancelErr},
-			rbac.Subject{}, "", rbac.Object{}, nil)
+		az.AlwaysReturn(rbac.ForbiddenWithInternal(&topdown.Error{Code: topdown.CancelErr},
+			rbac.Subject{}, "", rbac.Object{}, nil))
 
 		// If we have assertions, that means the method should FAIL
 		// if RBAC will disallow the request. The returned error should
@@ -324,6 +326,7 @@ type expects struct {
 	// instead.
 	notAuthorizedExpect string
 	cancelledCtxExpect  string
+	successAuthorizer   func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error
 }
 
 // Asserts is required. Asserts the RBAC authorize calls that should be made.
@@ -354,6 +357,23 @@ func (m *expects) Errors(err error) *expects {
 	return m
 }
 
+func (m *expects) FailSystemObjectChecks() *expects {
+	return m.WithSuccessAuthorizer(func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error {
+		if obj.Type == rbac.ResourceSystem.Type {
+			return xerrors.Errorf("hard coded system authz failed")
+		}
+		return nil
+	})
+}
+
+// WithSuccessAuthorizer is helpful when an optimization authz check is made
+// to skip some RBAC checks. This check in testing would prevent the ability
+// to assert the more nuanced RBAC checks.
+func (m *expects) WithSuccessAuthorizer(f func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error) *expects {
+	m.successAuthorizer = f
+	return m
+}
+
 func (m *expects) WithNotAuthorized(contains string) *expects {
 	m.notAuthorizedExpect = contains
 	return m

diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go
@@ -2599,51 +2599,46 @@ func (q *FakeQuerier) GetGroupMembersCountByGroupID(ctx context.Context, groupID
 	return int64(len(users)), nil
 }
 
-func (q *FakeQuerier) GetGroups(_ context.Context) ([]database.Group, error) {
-	q.mutex.RLock()
-	defer q.mutex.RUnlock()
-
-	out := make([]database.Group, len(q.groups))
-	copy(out, q.groups)
-	return out, nil
-}
-
-func (q *FakeQuerier) GetGroupsByOrganizationAndUserID(_ context.Context, arg database.GetGroupsByOrganizationAndUserIDParams) ([]database.Group, error) {
+func (q *FakeQuerier) GetGroups(_ context.Context, arg database.GetGroupsParams) ([]database.Group, error) {
 	err := validateDatabaseType(arg)
 	if err != nil {
 		return nil, err
 	}
 
 	q.mutex.RLock()
 	defer q.mutex.RUnlock()
-	var groupIDs []uuid.UUID
-	for _, member := range q.groupMembers {
-		if member.UserID == arg.UserID {
-			groupIDs = append(groupIDs, member.GroupID)
+
+	groupIDs := make(map[uuid.UUID]struct{})
+	if arg.HasMemberID != uuid.Nil {
+		for _, member := range q.groupMembers {
+			if member.UserID == arg.HasMemberID {
+				groupIDs[member.GroupID] = struct{}{}
+			}
 		}
-	}
-	groups := []database.Group{}
-	for _, group := range q.groups {
-		if slices.Contains(groupIDs, group.ID) && group.OrganizationID == arg.OrganizationID {
-			groups = append(groups, group)
+
+		// Handle the everyone group
+		for _, orgMember := range q.organizationMembers {
+			if orgMember.UserID == arg.HasMemberID {
+				groupIDs[orgMember.OrganizationID] = struct{}{}
+			}
 		}
 	}
 
-	return groups, nil
-}
-
-func (q *FakeQuerier) GetGroupsByOrganizationID(_ context.Context, id uuid.UUID) ([]database.Group, error) {
-	q.mutex.RLock()
-	defer q.mutex.RUnlock()
-
-	groups := make([]database.Group, 0, len(q.groups))
+	filtered := make([]database.Group, 0)
 	for _, group := range q.groups {
-		if group.OrganizationID == id {
-			groups = append(groups, group)
+		if arg.OrganizationID != uuid.Nil && group.OrganizationID != arg.OrganizationID {
+			continue
+		}
+
+		_, ok := groupIDs[group.ID]
+		if arg.HasMemberID != uuid.Nil && !ok {
+			continue
 		}
+
+		filtered = append(filtered, group)
 	}
 
-	return groups, nil
+	return filtered, nil
 }
 
 func (q *FakeQuerier) GetHealthSettings(_ context.Context) (string, error) {