Skip to content

Commit 634f4ca

Browse files
EmyrkEmyrk
committed
feat: push GetUsers filter to SQL
1 parent 24ec05b commit 634f4ca

File tree

9 files changed

+178
-14
lines changed

9 files changed

+178
-14
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 13 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -620,7 +620,6 @@ func (q *querier) GetAuthorizedUserCount(ctx context.Context, arg database.GetFi
620620
}
621621

622622
func (q *querier) GetUsersWithCount(ctx context.Context, arg database.GetUsersParams) ([]database.User, int64, error) {
623-
// TODO Implement this with a SQL filter. The count is incorrect without it.
624623
rowUsers, err := q.db.GetUsers(ctx, arg)
625624
if err != nil {
626625
return nil, -1, err
@@ -630,18 +629,8 @@ func (q *querier) GetUsersWithCount(ctx context.Context, arg database.GetUsersPa
630629
return []database.User{}, 0, nil
631630
}
632631

633-
act, ok := ActorFromContext(ctx)
634-
if !ok {
635-
return nil, -1, NoActorError
636-
}
637-
638632
// TODO: Is this correct? Should we return a restricted user?
639633
users := database.ConvertUserRows(rowUsers)
640-
users, err = rbac.Filter(ctx, q.auth, act, rbac.ActionRead, users)
641-
if err != nil {
642-
return nil, -1, err
643-
}
644-
645634
return users, rowUsers[0].Count, nil
646635
}
647636

@@ -699,6 +688,13 @@ func authorizedTemplateVersionFromJob(ctx context.Context, q *querier, job datab
699688
}
700689
}
701690

691+
// GetAuthorizedUsers is not required for dbauthz since GetUsers is already
692+
// authenticated.
693+
func (q *querier) GetAuthorizedUsers(ctx context.Context, arg database.GetUsersParams, _ rbac.PreparedAuthorized) ([]database.GetUsersRow, error) {
694+
// GetUsers is authenticated.
695+
return q.GetUsers(ctx, arg)
696+
}
697+
702698
func (q *querier) AcquireLock(ctx context.Context, id int64) error {
703699
return q.db.AcquireLock(ctx, id)
704700
}
@@ -1427,8 +1423,12 @@ func (q *querier) GetUserLinkByUserIDLoginType(ctx context.Context, arg database
14271423
}
14281424

14291425
func (q *querier) GetUsers(ctx context.Context, arg database.GetUsersParams) ([]database.GetUsersRow, error) {
1430-
// TODO: We should use GetUsersWithCount with a better method signature.
1431-
return fetchWithPostFilter(q.auth, q.db.GetUsers)(ctx, arg)
1426+
// This does the filtering in SQL.
1427+
prep, err := prepareSQLFilter(ctx, q.auth, rbac.ActionRead, rbac.ResourceUser.Type)
1428+
if err != nil {
1429+
return nil, xerrors.Errorf("(dev error) prepare sql filter: %w", err)
1430+
}
1431+
return q.db.GetAuthorizedUsers(ctx, arg, prep)
14321432
}
14331433

14341434
// GetUsersByIDs is only used for usernames on workspace return data.

‎coderd/database/dbfake/dbfake.go

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@ import (
2323
"github.com/coder/coder/coderd/database/db2sdk"
2424
"github.com/coder/coder/coderd/httpapi"
2525
"github.com/coder/coder/coderd/rbac"
26+
"github.com/coder/coder/coderd/rbac/regosql"
2627
"github.com/coder/coder/coderd/util/slice"
2728
"github.com/coder/coder/codersdk"
2829
)
@@ -5437,3 +5438,38 @@ func (*FakeQuerier) UpsertTailnetClient(context.Context, database.UpsertTailnetC
54375438
func (*FakeQuerier) UpsertTailnetCoordinator(context.Context, uuid.UUID) (database.TailnetCoordinator, error) {
54385439
return database.TailnetCoordinator{}, ErrUnimplemented
54395440
}
5441+
5442+
func (q *FakeQuerier) GetAuthorizedUsers(ctx context.Context, arg database.GetUsersParams, prepared rbac.PreparedAuthorized) ([]database.GetUsersRow, error) {
5443+
if err := validateDatabaseType(arg); err != nil {
5444+
return nil, err
5445+
}
5446+
5447+
// Call this to match the same function calls as the SQL implementation.
5448+
if prepared != nil {
5449+
_, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{
5450+
VariableConverter: regosql.UserConverter(),
5451+
})
5452+
if err != nil {
5453+
return nil, err
5454+
}
5455+
}
5456+
5457+
users, err := q.GetUsers(ctx, arg)
5458+
if err != nil {
5459+
return nil, err
5460+
}
5461+
5462+
q.mutex.RLock()
5463+
defer q.mutex.RUnlock()
5464+
5465+
filteredUsers := make([]database.GetUsersRow, 0, len(users))
5466+
for _, user := range users {
5467+
// If the filter exists, ensure the object is authorized.
5468+
if prepared != nil && prepared.Authorize(ctx, user.RBACObject()) != nil {
5469+
continue
5470+
}
5471+
5472+
filteredUsers = append(filteredUsers, user)
5473+
}
5474+
return filteredUsers, nil
5475+
}

‎coderd/database/dbmetrics/dbmetrics.go

Lines changed: 7 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/dbmock/dbmock.go

Lines changed: 15 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/modelqueries.go

Lines changed: 64 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -256,10 +256,73 @@ func (q *sqlQuerier) GetAuthorizedWorkspaces(ctx context.Context, arg GetWorkspa
256256

257257
type userQuerier interface {
258258
GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error)
259+
GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, prepared rbac.PreparedAuthorized) ([]GetUsersRow, error)
260+
}
261+
262+
func (q *sqlQuerier) GetAuthorizedUsers(ctx context.Context, arg GetUsersParams, prepared rbac.PreparedAuthorized) ([]GetUsersRow, error) {
263+
authorizedFilter, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{
264+
VariableConverter: regosql.UserConverter(),
265+
})
266+
267+
if err != nil {
268+
return nil, xerrors.Errorf("compile authorized filter: %w", err)
269+
}
270+
271+
filtered, err := insertAuthorizedFilter(getUsers, fmt.Sprintf(" AND %s", authorizedFilter))
272+
if err != nil {
273+
return nil, xerrors.Errorf("insert authorized filter: %w", err)
274+
}
275+
276+
query := fmt.Sprintf("-- name: GetAuthorizedUsers :many\n%s", filtered)
277+
rows, err := q.db.QueryContext(ctx, query,
278+
arg.AfterID,
279+
arg.Search,
280+
pq.Array(arg.Status),
281+
pq.Array(arg.RbacRole),
282+
arg.LastSeenBefore,
283+
arg.LastSeenAfter,
284+
arg.OffsetOpt,
285+
arg.LimitOpt,
286+
)
287+
if err != nil {
288+
return nil, err
289+
}
290+
defer rows.Close()
291+
var items []GetUsersRow
292+
for rows.Next() {
293+
var i GetUsersRow
294+
if err := rows.Scan(
295+
&i.ID,
296+
&i.Email,
297+
&i.Username,
298+
&i.HashedPassword,
299+
&i.CreatedAt,
300+
&i.UpdatedAt,
301+
&i.Status,
302+
&i.RBACRoles,
303+
&i.LoginType,
304+
&i.AvatarURL,
305+
&i.Deleted,
306+
&i.LastSeenAt,
307+
&i.Count,
308+
); err != nil {
309+
return nil, err
310+
}
311+
items = append(items, i)
312+
}
313+
if err := rows.Close(); err != nil {
314+
return nil, err
315+
}
316+
if err := rows.Err(); err != nil {
317+
return nil, err
318+
}
319+
return items, nil
259320
}
260321

261322
func (q *sqlQuerier) GetAuthorizedUserCount(ctx context.Context, arg GetFilteredUserCountParams, prepared rbac.PreparedAuthorized) (int64, error) {
262-
authorizedFilter, err := prepared.CompileToSQL(ctx, rbac.ConfigWithoutACL())
323+
authorizedFilter, err := prepared.CompileToSQL(ctx, regosql.ConvertConfig{
324+
VariableConverter: regosql.UserConverter(),
325+
})
263326
if err != nil {
264327
return -1, xerrors.Errorf("compile authorized filter: %w", err)
265328
}

‎coderd/database/queries.sql.go

Lines changed: 3 additions & 0 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/database/queries/users.sql

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -208,6 +208,9 @@ WHERE
208208
ELSE true
209209
END
210210
-- End of filters
211+
212+
-- Authorize Filter clause will be injected below in GetAuthorizedUsers
213+
-- @authorize_filter
211214
ORDER BY
212215
-- Deterministic and consistent ordering of all users. This is to ensure consistent pagination.
213216
LOWER(username) ASC OFFSET @offset_opt

‎coderd/rbac/regosql/compile_test.go

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -242,6 +242,26 @@ neq(input.object.owner, "");
242242
p("false")),
243243
VariableConverter: regosql.TemplateConverter(),
244244
},
245+
{
246+
Name: "UserNoOrgOwner",
247+
Queries: []string{
248+
`input.object.org_owner != ""`,
249+
},
250+
ExpectedSQL: p("'' != ''"),
251+
VariableConverter: regosql.UserConverter(),
252+
},
253+
{
254+
Name: "UserOwnsSelf",
255+
Queries: []string{
256+
`"10d03e62-7703-4df5-a358-4f76577d4e2f" = input.object.owner;
257+
input.object.owner != "";
258+
input.object.org_owner = ""`,
259+
},
260+
VariableConverter: regosql.UserConverter(),
261+
ExpectedSQL: p(
262+
p("'10d03e62-7703-4df5-a358-4f76577d4e2f' = ''") + " AND " + p("'' != ''") + " AND " + p("'' = ''"),
263+
),
264+
},
245265
}
246266

247267
for _, tc := range testCases {

‎coderd/rbac/regosql/configs.go

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,23 @@ func TemplateConverter() *sqltypes.VariableConverter {
3636
return matcher
3737
}
3838

39+
func UserConverter() *sqltypes.VariableConverter {
40+
matcher := sqltypes.NewVariableConverter().RegisterMatcher(
41+
resourceIDMatcher(),
42+
// Users are never owned by an organization, so always return the empty string
43+
// for the org owner.
44+
sqltypes.StringVarMatcher("''", []string{"input", "object", "org_owner"}),
45+
// Users never have an owner, and are only owned site wide.
46+
sqltypes.StringVarMatcher("''", []string{"input", "object", "owner"}),
47+
)
48+
matcher.RegisterMatcher(
49+
// No ACLs on the user type
50+
sqltypes.AlwaysFalse(groupACLMatcher(matcher)),
51+
sqltypes.AlwaysFalse(userACLMatcher(matcher)),
52+
)
53+
return matcher
54+
}
55+
3956
// NoACLConverter should be used when the target SQL table does not contain
4057
// group or user ACL columns.
4158
func NoACLConverter() *sqltypes.VariableConverter {

0 commit comments

Comments
 (0)