chore: add /groups endpoint to filter by organization and/or member (#14260)

* chore: merge get groups sql queries into 1

* Add endpoint for fetching groups with filters
* remove 2 ways to customizing a fake authorizer
This commit is contained in:
Steven Masley
2024-08-15 13:40:15 -05:00
committed by GitHub
parent 83ccdaa755
commit 7b09d98238
24 changed files with 539 additions and 289 deletions
+8 -11
View File
@@ -1491,19 +1491,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) {
+19 -12
View File
@@ -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{})
+27 -7
View File
@@ -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
+27 -32
View File
@@ -2599,16 +2599,7 @@ 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
@@ -2616,34 +2607,38 @@ func (q *FakeQuerier) GetGroupsByOrganizationAndUserID(_ context.Context, arg da
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{}{}
}
}
// Handle the everyone group
for _, orgMember := range q.organizationMembers {
if orgMember.UserID == arg.HasMemberID {
groupIDs[orgMember.OrganizationID] = struct{}{}
}
}
}
groups := []database.Group{}
filtered := make([]database.Group, 0)
for _, group := range q.groups {
if slices.Contains(groupIDs, group.ID) && group.OrganizationID == arg.OrganizationID {
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
}
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))
for _, group := range q.groups {
if group.OrganizationID == id {
groups = append(groups, group)
}
}
return groups, nil
return filtered, nil
}
func (q *FakeQuerier) GetHealthSettings(_ context.Context) (string, error) {
+2 -16
View File
@@ -662,27 +662,13 @@ func (m metricsStore) GetGroupMembersCountByGroupID(ctx context.Context, groupID
return r0, r1
}
func (m metricsStore) GetGroups(ctx context.Context) ([]database.Group, error) {
func (m metricsStore) GetGroups(ctx context.Context, arg database.GetGroupsParams) ([]database.Group, error) {
start := time.Now()
r0, r1 := m.s.GetGroups(ctx)
r0, r1 := m.s.GetGroups(ctx, arg)
m.queryLatencies.WithLabelValues("GetGroups").Observe(time.Since(start).Seconds())
return r0, r1
}
func (m metricsStore) GetGroupsByOrganizationAndUserID(ctx context.Context, arg database.GetGroupsByOrganizationAndUserIDParams) ([]database.Group, error) {
start := time.Now()
r0, r1 := m.s.GetGroupsByOrganizationAndUserID(ctx, arg)
m.queryLatencies.WithLabelValues("GetGroupsByOrganizationAndUserID").Observe(time.Since(start).Seconds())
return r0, r1
}
func (m metricsStore) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]database.Group, error) {
start := time.Now()
groups, err := m.s.GetGroupsByOrganizationID(ctx, organizationID)
m.queryLatencies.WithLabelValues("GetGroupsByOrganizationID").Observe(time.Since(start).Seconds())
return groups, err
}
func (m metricsStore) GetHealthSettings(ctx context.Context) (string, error) {
start := time.Now()
r0, r1 := m.s.GetHealthSettings(ctx)
+4 -34
View File
@@ -1315,48 +1315,18 @@ func (mr *MockStoreMockRecorder) GetGroupMembersCountByGroupID(arg0, arg1 any) *
}
// GetGroups mocks base method.
func (m *MockStore) GetGroups(arg0 context.Context) ([]database.Group, error) {
func (m *MockStore) GetGroups(arg0 context.Context, arg1 database.GetGroupsParams) ([]database.Group, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetGroups", arg0)
ret := m.ctrl.Call(m, "GetGroups", arg0, arg1)
ret0, _ := ret[0].([]database.Group)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetGroups indicates an expected call of GetGroups.
func (mr *MockStoreMockRecorder) GetGroups(arg0 any) *gomock.Call {
func (mr *MockStoreMockRecorder) GetGroups(arg0, arg1 any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroups", reflect.TypeOf((*MockStore)(nil).GetGroups), arg0)
}
// GetGroupsByOrganizationAndUserID mocks base method.
func (m *MockStore) GetGroupsByOrganizationAndUserID(arg0 context.Context, arg1 database.GetGroupsByOrganizationAndUserIDParams) ([]database.Group, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetGroupsByOrganizationAndUserID", arg0, arg1)
ret0, _ := ret[0].([]database.Group)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetGroupsByOrganizationAndUserID indicates an expected call of GetGroupsByOrganizationAndUserID.
func (mr *MockStoreMockRecorder) GetGroupsByOrganizationAndUserID(arg0, arg1 any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupsByOrganizationAndUserID", reflect.TypeOf((*MockStore)(nil).GetGroupsByOrganizationAndUserID), arg0, arg1)
}
// GetGroupsByOrganizationID mocks base method.
func (m *MockStore) GetGroupsByOrganizationID(arg0 context.Context, arg1 uuid.UUID) ([]database.Group, error) {
m.ctrl.T.Helper()
ret := m.ctrl.Call(m, "GetGroupsByOrganizationID", arg0, arg1)
ret0, _ := ret[0].([]database.Group)
ret1, _ := ret[1].(error)
return ret0, ret1
}
// GetGroupsByOrganizationID indicates an expected call of GetGroupsByOrganizationID.
func (mr *MockStoreMockRecorder) GetGroupsByOrganizationID(arg0, arg1 any) *gomock.Call {
mr.mock.ctrl.T.Helper()
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroupsByOrganizationID", reflect.TypeOf((*MockStore)(nil).GetGroupsByOrganizationID), arg0, arg1)
return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetGroups", reflect.TypeOf((*MockStore)(nil).GetGroups), arg0, arg1)
}
// GetHealthSettings mocks base method.
+1 -3
View File
@@ -151,9 +151,7 @@ type sqlcQuerier interface {
// count even if the caller does not have read access to ResourceGroupMember.
// They only need ResourceGroup read access.
GetGroupMembersCountByGroupID(ctx context.Context, groupID uuid.UUID) (int64, error)
GetGroups(ctx context.Context) ([]Group, error)
GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error)
GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error)
GetGroups(ctx context.Context, arg GetGroupsParams) ([]Group, error)
GetHealthSettings(ctx context.Context) (string, error)
GetHungProvisionerJobs(ctx context.Context, updatedAt time.Time) ([]ProvisionerJob, error)
GetJFrogXrayScanByWorkspaceAndAgentID(ctx context.Context, arg GetJFrogXrayScanByWorkspaceAndAgentIDParams) (JfrogXrayScan, error)
+33 -96
View File
@@ -1561,105 +1561,42 @@ func (q *sqlQuerier) GetGroupByOrgAndName(ctx context.Context, arg GetGroupByOrg
}
const getGroups = `-- name: GetGroups :many
SELECT id, name, organization_id, avatar_url, quota_allowance, display_name, source FROM groups
`
func (q *sqlQuerier) GetGroups(ctx context.Context) ([]Group, error) {
rows, err := q.db.QueryContext(ctx, getGroups)
if err != nil {
return nil, err
}
defer rows.Close()
var items []Group
for rows.Next() {
var i Group
if err := rows.Scan(
&i.ID,
&i.Name,
&i.OrganizationID,
&i.AvatarURL,
&i.QuotaAllowance,
&i.DisplayName,
&i.Source,
); err != nil {
return nil, err
}
items = append(items, i)
}
if err := rows.Close(); err != nil {
return nil, err
}
if err := rows.Err(); err != nil {
return nil, err
}
return items, nil
}
const getGroupsByOrganizationAndUserID = `-- name: GetGroupsByOrganizationAndUserID :many
SELECT
groups.id, groups.name, groups.organization_id, groups.avatar_url, groups.quota_allowance, groups.display_name, groups.source
id, name, organization_id, avatar_url, quota_allowance, display_name, source
FROM
groups
WHERE
groups.id IN (
SELECT
group_id
FROM
group_members_expanded gme
WHERE
gme.user_id = $1
AND
gme.organization_id = $2
)
true
AND CASE
WHEN $1:: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
groups.organization_id = $1
ELSE true
END
AND CASE
-- Filter to only include groups a user is a member of
WHEN $2::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
EXISTS (
SELECT
1
FROM
-- this view handles the 'everyone' group in orgs.
group_members_expanded
WHERE
group_members_expanded.group_id = groups.id
AND
group_members_expanded.user_id = $2
)
ELSE true
END
`
type GetGroupsByOrganizationAndUserIDParams struct {
UserID uuid.UUID `db:"user_id" json:"user_id"`
type GetGroupsParams struct {
OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"`
HasMemberID uuid.UUID `db:"has_member_id" json:"has_member_id"`
}
func (q *sqlQuerier) GetGroupsByOrganizationAndUserID(ctx context.Context, arg GetGroupsByOrganizationAndUserIDParams) ([]Group, error) {
rows, err := q.db.QueryContext(ctx, getGroupsByOrganizationAndUserID, arg.UserID, arg.OrganizationID)
if err != nil {
return nil, err
}
defer rows.Close()
var items []Group
for rows.Next() {
var i Group
if err := rows.Scan(
&i.ID,
&i.Name,
&i.OrganizationID,
&i.AvatarURL,
&i.QuotaAllowance,
&i.DisplayName,
&i.Source,
); err != nil {
return nil, err
}
items = append(items, i)
}
if err := rows.Close(); err != nil {
return nil, err
}
if err := rows.Err(); err != nil {
return nil, err
}
return items, nil
}
const getGroupsByOrganizationID = `-- name: GetGroupsByOrganizationID :many
SELECT
id, name, organization_id, avatar_url, quota_allowance, display_name, source
FROM
groups
WHERE
organization_id = $1
`
func (q *sqlQuerier) GetGroupsByOrganizationID(ctx context.Context, organizationID uuid.UUID) ([]Group, error) {
rows, err := q.db.QueryContext(ctx, getGroupsByOrganizationID, organizationID)
func (q *sqlQuerier) GetGroups(ctx context.Context, arg GetGroupsParams) ([]Group, error) {
rows, err := q.db.QueryContext(ctx, getGroups, arg.OrganizationID, arg.HasMemberID)
if err != nil {
return nil, err
}
@@ -1766,15 +1703,15 @@ INSERT INTO groups (
id,
name,
organization_id,
source
source
)
SELECT
gen_random_uuid(),
group_name,
$1,
$2
gen_random_uuid(),
group_name,
$1,
$2
FROM
UNNEST($3 :: text[]) AS group_name
UNNEST($3 :: text[]) AS group_name
ON CONFLICT DO NOTHING
RETURNING id, name, organization_id, avatar_url, quota_allowance, display_name, source
`
+31 -29
View File
@@ -1,6 +1,3 @@
-- name: GetGroups :many
SELECT * FROM groups;
-- name: GetGroupByID :one
SELECT
*
@@ -23,30 +20,35 @@ AND
LIMIT
1;
-- name: GetGroupsByOrganizationID :many
-- name: GetGroups :many
SELECT
*
FROM
groups
WHERE
organization_id = $1;
-- name: GetGroupsByOrganizationAndUserID :many
SELECT
groups.*
*
FROM
groups
WHERE
groups.id IN (
SELECT
group_id
FROM
group_members_expanded gme
WHERE
gme.user_id = @user_id
AND
gme.organization_id = @organization_id
);
true
AND CASE
WHEN @organization_id:: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
groups.organization_id = @organization_id
ELSE true
END
AND CASE
-- Filter to only include groups a user is a member of
WHEN @has_member_id::uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN
EXISTS (
SELECT
1
FROM
-- this view handles the 'everyone' group in orgs.
group_members_expanded
WHERE
group_members_expanded.group_id = groups.id
AND
group_members_expanded.user_id = @has_member_id
)
ELSE true
END
;
-- name: InsertGroup :one
INSERT INTO groups (
@@ -68,15 +70,15 @@ INSERT INTO groups (
id,
name,
organization_id,
source
source
)
SELECT
gen_random_uuid(),
group_name,
@organization_id,
@source
gen_random_uuid(),
group_name,
@organization_id,
@source
FROM
UNNEST(@group_names :: text[]) AS group_name
UNNEST(@group_names :: text[]) AS group_name
-- If the name conflicts, do nothing.
ON CONFLICT DO NOTHING
RETURNING *;