mirror of
https://github.com/coder/coder.git
synced 2026-09-24 15:04:27 +08:00
fix: use SQL-level auth filtering for chat listing (#23159)
## Problem The chat listing endpoint (`GetChatsByOwnerID`) was using `fetchWithPostFilter`, which fetches N rows from the database and then filters them in Go memory using RBAC checks. This causes a pagination bug: if the user requests `limit=25` but some rows fail the auth check, fewer than 25 rows are returned even though more authorized rows exist in the database. The client may incorrectly assume it has reached the end of the list. ## Solution Switch to the same pattern used by `GetWorkspaces`, `GetTemplates`, and `GetUsers`: `prepareSQLFilter` + `GetAuthorized*` variant. The RBAC filter is compiled to a SQL WHERE clause and injected into the query before `ORDER BY`/`LIMIT`, so the database returns exactly the requested number of authorized rows. Additionally, `GetChatsByOwnerID` is renamed to `GetChats` with `OwnerID` as an optional (nullable) filter parameter, matching the `GetWorkspaces` naming convention. ## Changes | File | Change | |------|--------| | `queries/chats.sql` | Renamed to `GetChats`, `owner_id` now optional via CASE/NULL, added `-- @authorize_filter` | | `queries.sql.go` | Renamed constant, params struct (`GetChatsParams`), and method | | `querier.go` | Interface method renamed | | `modelqueries.go` | Added `chatQuerier` interface + `GetAuthorizedChats` impl | | `dbauthz/dbauthz.go` | `GetChats` now uses `prepareSQLFilter` instead of `fetchWithPostFilter` | | `dbauthz/dbauthz_test.go` | Updated tests for SQL filter pattern | | `dbmock/dbmock.go` | Renamed + added mock for `GetAuthorizedChats` | | `dbmetrics/querymetrics.go` | Renamed + added metrics wrapper | | `rbac/regosql/configs.go` | Added `ChatConverter` (maps `org_owner` to empty string literal since `chats` has no `organization_id` column) | | `rbac/authz.go` | Added `ConfigChats()` | | `chats.go` | Handler uses renamed method with `uuid.NullUUID` | | `searchquery/search.go` | Updated return type | | `gitsync/worker.go` | Updated interface and call site | | Various test files | Updated for renamed types |
This commit is contained in:
Kyle Carberry
@@ -1235,6 +1235,230 @@ func TestGetAuthorizedWorkspacesAndAgentsByOwnerID(t *testing.T) {
|
||||
})
|
||||
}
|
||||
|
||||
func TestGetAuthorizedChats(t *testing.T) {
|
||||
t.Parallel()
|
||||
if testing.Short() {
|
||||
t.SkipNow()
|
||||
}
|
||||
|
||||
sqlDB := testSQLDB(t)
|
||||
err := migrations.Up(sqlDB)
|
||||
require.NoError(t, err)
|
||||
db := database.New(sqlDB)
|
||||
authorizer := rbac.NewStrictCachingAuthorizer(prometheus.NewRegistry())
|
||||
|
||||
// Create users with different roles.
|
||||
owner := dbgen.User(t, db, database.User{
|
||||
RBACRoles: []string{rbac.RoleOwner().String()},
|
||||
})
|
||||
member := dbgen.User(t, db, database.User{})
|
||||
secondMember := dbgen.User(t, db, database.User{})
|
||||
|
||||
// Create FK dependencies: a chat provider and model config.
|
||||
ctx := testutil.Context(t, testutil.WaitMedium)
|
||||
_, err = db.InsertChatProvider(ctx, database.InsertChatProviderParams{
|
||||
Provider: "openai",
|
||||
DisplayName: "OpenAI",
|
||||
APIKey: "test-key",
|
||||
Enabled: true,
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
modelCfg, err := db.InsertChatModelConfig(ctx, database.InsertChatModelConfigParams{
|
||||
Provider: "openai",
|
||||
Model: "test-model",
|
||||
DisplayName: "Test Model",
|
||||
CreatedBy: uuid.NullUUID{UUID: owner.ID, Valid: true},
|
||||
UpdatedBy: uuid.NullUUID{UUID: owner.ID, Valid: true},
|
||||
Enabled: true,
|
||||
IsDefault: true,
|
||||
ContextLimit: 128000,
|
||||
CompressionThreshold: 80,
|
||||
Options: json.RawMessage(`{}`),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
|
||||
// Create 3 chats owned by owner.
|
||||
for i := range 3 {
|
||||
_, err := db.InsertChat(ctx, database.InsertChatParams{
|
||||
OwnerID: owner.ID,
|
||||
LastModelConfigID: modelCfg.ID,
|
||||
Title: fmt.Sprintf("owner chat %d", i+1),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
// Create 2 chats owned by member.
|
||||
for i := range 2 {
|
||||
_, err := db.InsertChat(ctx, database.InsertChatParams{
|
||||
OwnerID: member.ID,
|
||||
LastModelConfigID: modelCfg.ID,
|
||||
Title: fmt.Sprintf("member chat %d", i+1),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
t.Run("sqlQuerier", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitMedium)
|
||||
|
||||
// Member should only see their own 2 chats.
|
||||
memberSubject, _, err := httpmw.UserRBACSubject(ctx, db, member.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
preparedMember, err := authorizer.Prepare(ctx, memberSubject, policy.ActionRead, rbac.ResourceChat.Type)
|
||||
require.NoError(t, err)
|
||||
memberRows, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{}, preparedMember)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, memberRows, 2)
|
||||
for _, row := range memberRows {
|
||||
require.Equal(t, member.ID, row.OwnerID, "member should only see own chats")
|
||||
}
|
||||
|
||||
// Owner should see at least the 5 pre-created chats (site-wide
|
||||
// access). Parallel subtests may add more, so use GreaterOrEqual.
|
||||
ownerSubject, _, err := httpmw.UserRBACSubject(ctx, db, owner.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
preparedOwner, err := authorizer.Prepare(ctx, ownerSubject, policy.ActionRead, rbac.ResourceChat.Type)
|
||||
require.NoError(t, err)
|
||||
ownerRows, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{}, preparedOwner)
|
||||
require.NoError(t, err)
|
||||
require.GreaterOrEqual(t, len(ownerRows), 5)
|
||||
|
||||
// secondMember has no chats and should see 0.
|
||||
secondSubject, _, err := httpmw.UserRBACSubject(ctx, db, secondMember.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
preparedSecond, err := authorizer.Prepare(ctx, secondSubject, policy.ActionRead, rbac.ResourceChat.Type)
|
||||
require.NoError(t, err)
|
||||
secondRows, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{}, preparedSecond)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, secondRows, 0)
|
||||
|
||||
// Org admin should NOT see other users' chats — chats are
|
||||
// not org-scoped resources.
|
||||
orgs, err := db.GetOrganizations(ctx, database.GetOrganizationsParams{})
|
||||
require.NoError(t, err)
|
||||
require.NotEmpty(t, orgs)
|
||||
orgAdmin := dbgen.User(t, db, database.User{})
|
||||
dbgen.OrganizationMember(t, db, database.OrganizationMember{
|
||||
UserID: orgAdmin.ID,
|
||||
OrganizationID: orgs[0].ID,
|
||||
Roles: []string{rbac.RoleOrgAdmin()},
|
||||
})
|
||||
orgAdminSubject, _, err := httpmw.UserRBACSubject(ctx, db, orgAdmin.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
preparedOrgAdmin, err := authorizer.Prepare(ctx, orgAdminSubject, policy.ActionRead, rbac.ResourceChat.Type)
|
||||
require.NoError(t, err)
|
||||
orgAdminRows, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{}, preparedOrgAdmin)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, orgAdminRows, 0, "org admin with no chats should see 0 chats")
|
||||
|
||||
// OwnerID filter: member queries their own chats.
|
||||
memberFilterSelf, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{
|
||||
OwnerID: member.ID,
|
||||
}, preparedMember)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, memberFilterSelf, 2)
|
||||
|
||||
// OwnerID filter: member queries owner's chats → sees 0.
|
||||
memberFilterOwner, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{
|
||||
OwnerID: owner.ID,
|
||||
}, preparedMember)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, memberFilterOwner, 0)
|
||||
})
|
||||
|
||||
t.Run("dbauthz", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitMedium)
|
||||
|
||||
authzdb := dbauthz.New(db, authorizer, slogtest.Make(t, &slogtest.Options{}), coderdtest.AccessControlStorePointer())
|
||||
|
||||
// As member: should see only own 2 chats.
|
||||
memberSubject, _, err := httpmw.UserRBACSubject(ctx, authzdb, member.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
memberCtx := dbauthz.As(ctx, memberSubject)
|
||||
memberRows, err := authzdb.GetChats(memberCtx, database.GetChatsParams{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, memberRows, 2)
|
||||
for _, row := range memberRows {
|
||||
require.Equal(t, member.ID, row.OwnerID, "member should only see own chats")
|
||||
}
|
||||
|
||||
// As owner: should see at least the 5 pre-created chats.
|
||||
ownerSubject, _, err := httpmw.UserRBACSubject(ctx, authzdb, owner.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
ownerCtx := dbauthz.As(ctx, ownerSubject)
|
||||
ownerRows, err := authzdb.GetChats(ownerCtx, database.GetChatsParams{})
|
||||
require.NoError(t, err)
|
||||
require.GreaterOrEqual(t, len(ownerRows), 5)
|
||||
|
||||
// As secondMember: should see 0 chats.
|
||||
secondSubject, _, err := httpmw.UserRBACSubject(ctx, authzdb, secondMember.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
secondCtx := dbauthz.As(ctx, secondSubject)
|
||||
secondRows, err := authzdb.GetChats(secondCtx, database.GetChatsParams{})
|
||||
require.NoError(t, err)
|
||||
require.Len(t, secondRows, 0)
|
||||
})
|
||||
|
||||
t.Run("pagination", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
ctx := testutil.Context(t, testutil.WaitMedium)
|
||||
|
||||
// Use a dedicated user for pagination to avoid interference
|
||||
// with the other parallel subtests.
|
||||
paginationUser := dbgen.User(t, db, database.User{})
|
||||
for i := range 7 {
|
||||
_, err := db.InsertChat(ctx, database.InsertChatParams{
|
||||
OwnerID: paginationUser.ID,
|
||||
LastModelConfigID: modelCfg.ID,
|
||||
Title: fmt.Sprintf("pagination chat %d", i+1),
|
||||
})
|
||||
require.NoError(t, err)
|
||||
}
|
||||
|
||||
pagUserSubject, _, err := httpmw.UserRBACSubject(ctx, db, paginationUser.ID, rbac.ExpandableScope(rbac.ScopeAll))
|
||||
require.NoError(t, err)
|
||||
preparedMember, err := authorizer.Prepare(ctx, pagUserSubject, policy.ActionRead, rbac.ResourceChat.Type)
|
||||
require.NoError(t, err)
|
||||
|
||||
// Fetch first page with limit=2.
|
||||
page1, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{
|
||||
LimitOpt: 2,
|
||||
}, preparedMember)
|
||||
require.NoError(t, err)
|
||||
require.Len(t, page1, 2)
|
||||
for _, row := range page1 {
|
||||
require.Equal(t, paginationUser.ID, row.OwnerID, "paginated results must belong to pagination user")
|
||||
}
|
||||
|
||||
// Fetch remaining pages and collect all chat IDs.
|
||||
allIDs := make(map[uuid.UUID]struct{})
|
||||
for _, row := range page1 {
|
||||
allIDs[row.ID] = struct{}{}
|
||||
}
|
||||
offset := int32(2)
|
||||
for {
|
||||
page, err := db.GetAuthorizedChats(ctx, database.GetChatsParams{
|
||||
LimitOpt: 2,
|
||||
OffsetOpt: offset,
|
||||
}, preparedMember)
|
||||
require.NoError(t, err)
|
||||
for _, row := range page {
|
||||
require.Equal(t, paginationUser.ID, row.OwnerID, "paginated results must belong to pagination user")
|
||||
allIDs[row.ID] = struct{}{}
|
||||
}
|
||||
if len(page) < 2 {
|
||||
break
|
||||
}
|
||||
offset += int32(len(page)) //nolint:gosec // Test code, pagination values are small.
|
||||
}
|
||||
|
||||
// All 7 member chats should be accounted for with no leakage.
|
||||
require.Len(t, allIDs, 7, "pagination should return all member chats exactly once")
|
||||
})
|
||||
}
|
||||
|
||||
func TestInsertWorkspaceAgentLogs(t *testing.T) {
|
||||
t.Parallel()
|
||||
if testing.Short() {
|
||||
|
||||
Reference in New Issue
Block a user