feat: add chat sharing API by DanielleMaywood · Pull Request #24968 · coder/coder

DanielleMaywood · 2026-05-05T13:40:52Z

Note

🤖 This PR was written by Coder Agent on behalf of Danielle Maywood

Builds on the merged chat sharing foundation (#25041).

Adds the public experimental chat sharing API on top of the foundation PR. This includes chat ACL read/update handlers, SDK request/response types and client methods, ACL lifecycle coverage, and generated API/TypeScript updates.

Chat responses also expose owner username/name so the frontend can identify the chat owner without requiring separate user read access.

Implementation plan

# Alternative split plan for chat ACL foundation

## Question

Consider splitting current PR #25041 into two stacked PRs:

1. Database-layer foundation.
2. Actual ACL, RBAC, dbauthz, and handler behavior.

## Recommendation

This split is feasible and probably cleaner than keeping everything in one
foundation PR if we also accept the SQL view refactor from DEREM-3.

The split should be:

1. `dm/chat-acl-database-foundation`, base `main`.
2. `dm/chat-sharing-foundation`, base `dm/chat-acl-database-foundation`.
3. `dm/chat-sharing`, base `dm/chat-sharing-foundation`.
4. `dm/chat-sharing-frontend`, base `dm/chat-sharing`.

## PR 1: database-layer foundation

Purpose: add inert persistence and effective-read query infrastructure without
making chat sharing usable.

Include:

- `coderd/database/migrations/000489_chat_acl_sharing.*.sql`.
- `coderd/database/dump.sql` schema updates.
- `coderd/database/types.go` `ChatACL` type.
- `coderd/database/models.go` generated chat ACL fields.
- `coderd/database/sqlc.yaml` overrides for chat ACL columns and any view
  columns.
- `coderd/database/queries/chats.sql` raw ACL persistence queries:
  - `GetChatACLByID`.
  - `UpdateChatACLByID`.
- `coderd/database/queries/chats.sql` effective chat read view or equivalent
  query changes.
- Generated database files from `make gen`.
- `enterprise/audit/table.go` ACL tracking.
- Database tests for:
  - `ChatACL` scan/value/RBAC conversion.
  - effective ACL inheritance through the view/query path.
  - auto-archive ACL audit mapping, if fixed in this layer.

Do not include:

- dbauthz authorization wrappers for chat ACL behavior.
- `ChatConverter()` SQL authz mapping.
- `DisableChatSharing` deployment option.
- RBAC `ActionShare` exposure for chats.
- HTTP handler authorization changes.

SQL view refactor belongs here if we do it. This PR can define something like
`chats_effective_acl` or `chats_with_effective_acl`, then use it for read
queries that should return inherited ACLs. The raw table remains available for
writes and raw ACL reads.

## PR 2: behavior foundation

Purpose: turn the database foundation into enforced ACL behavior.

Include:

- dbauthz wrappers:
  - `GetChatACLByID`.
  - `UpdateChatACLByID`.
  - chat file linked-chat fallback behavior.
  - `GetChatsByWorkspaceIDs` post-filter behavior using effective ACLs.
- `coderd/rbac/regosql/configs.go` `ChatConverter()`.
- chat ACL kill switch behavior.
- `policy.ActionShare` and generated RBAC/API scope files.
- `agents-access` role decision if regular users should share their own chats.
- handler `ActionUpdate` hardening in `coderd/exp_chats.go`.
- dbauthz and handler tests for actual authorization behavior.

## Cost

The split adds branch work and at least one more draft PR, but reduces review
complexity.

Extra cost:

- 2 to 4 hours for branch surgery, PR creation, and rebase handling.
- One more `make gen` and `make pre-commit` cycle.
- Possible sqlc iteration if view columns do not preserve `database.Chat`
  shapes cleanly.
- Existing stacked PRs #24968 and #24987 need base updates after the split.

Reduced cost:

- Reviewers can approve schema/query mechanics separately from authorization
  behavior.
- The SQL view refactor becomes easier to justify because it lives in the
  database-only PR.
- Review feedback like DEREM-3 and DEREM-5 can be resolved in the lower PR
  without mixing in RBAC and handler changes.

## Risks

- sqlc may generate view-backed structs or raw JSON types unless the view and
  overrides are shaped carefully.
- Query names returning `database.Chat` must keep the same Go signatures or the
  upper PR will churn.
- Any read query using effective ACLs must be explicit. Raw write/locking paths
  should continue using the `chats` table.
- More stack management: every upper PR must be rebased after rewriting the
  lower PRs.

## Decision point

If we accept this split, revise the current feedback plan:

- Move C005 and C022 into PR 1.
- Move C023 into PR 1 if we treat audit mapping as database-layer data shaping.
- Keep C003, C004, C007, C008, C009, C010, C012, C013, C016, C017, C018,
  C019, C020, C024, and C027 in PR 2 unless they directly depend on generated
  database output.

Recommendation: do the split if we want to accept the SQL view refactor now.
Do not do it just for size reduction, because the current PR is already stacked
and another layer increases coordination cost.

github-actions · 2026-05-05T13:41:03Z

Docs preview

📖 View docs preview for docs/admin/security/audit-logs.md

DanielleMaywood · 2026-05-08T09:16:33Z

/coder-agents-review

coder-agents-review

Solid ACL implementation that correctly mirrors the workspace sharing model with proper transaction handling, defense-in-depth authorization, and thorough test coverage. The sub-chat TOCTOU protection (outer check + inner FOR UPDATE re-check) is well done. The generic UpdateValidator extension to support ChatRole is clean. Test coverage is comprehensive: lifecycle, sub-chat inheritance, validation, kill switch, stream access, and list exclusion.

Two P2 concerns, six P3 concerns, one P4, and five nits.

The headline issue is the group member PII leak: GET /acl returns full ReducedUser records (emails, login types, statuses) for every group member to any shared reader, while the comment claims only "member counts" are needed. Eight reviewers independently converged on this. The workspace ACL handler acknowledges the same pattern as security debt with a TODO; the chat handler copies the pattern without the acknowledgment. This is the single highest-value fix in this review.

The pubsub contract split is the second concern: three call sites still use db2sdk.Chat() which leaves OwnerUsername as "" in serialized events, while the SDK type declares it as a required string. Currently benign (frontend merge is correct, events go to owner only), but structurally wrong for any future consumer. Adding omitempty to OwnerUsername would at least align the serialization contract with OwnerName.

"The whole point is fresh eyes from a reviewer whose triggers did NOT match" was validated by Razor, who caught the dead sentinel panic path that the triggered reviewers all walked past.

coderd/exp_chats.go:123

P2 [DEREM-4] publishChatTitleChange emits Chat with empty OwnerUsername/OwnerName because it uses db2sdk.Chat() instead of ChatWithOwner.

Three pubsub producers were not updated to the new hydrated converter: publishChatTitleChange (here), publishChatPubsubEvent (chatd.go:4889), and publishChatActionRequired (chatd.go:4936). The Chat.OwnerUsername field has no omitempty tag, so the JSON always includes "owner_username": "". Meanwhile OwnerName has omitempty and is cleanly absent. This asymmetry means a consumer cannot distinguish "not provided" from "empty" for OwnerUsername but can for OwnerName.

Currently benign: the frontend spreads ...cachedChat as base so empty fields from events don't overwrite cached values. But any new consumer that trusts the event as a complete snapshot will silently lose owner identity.

Fix either:

Add omitempty to OwnerUsername so both owner fields have consistent absent-when-empty semantics, or
Hydrate owner info in the pubsub paths (the database.Chat already carries OwnerID).

(Hisoka P2, Mafuuu P2, Meruem P3, Ryosuke P3, Zoro P3, Chopper P3)

🤖

coderd/rbac/regosql/configs.go:53

P3 [DEREM-13] The ChatConverter inherits resourceIDMatcher() which generates unqualified id :: text. With the new JOIN users in GetChats, both tables expose an id column. If a future RBAC policy emits an id-based filter clause, PostgreSQL rejects the query with "column reference 'id' is ambiguous." Current policies don't emit id clauses, so this is latent. Fix: override with sqltypes.StringVarMatcher("chats.id :: text", []string{"input", "object", "id"}) in ChatConverter. (Knuckle P3, Hisoka Note)

🤖

🤖 This review was automatically generated with Coder Agents.

DanielleMaywood · 2026-05-13T15:35:37Z

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

C013 (DEREM-4): Partially fixed in 153c0018. owner_username now uses omitempty, matching owner_name, so empty owner fields are not serialized in watch payloads. I did not add publisher-side owner hydration here because that affects multiple event paths and needs a broader payload/query design.

DanielleMaywood · 2026-05-13T15:35:38Z

🤖 This comment was written by Coder Agent on behalf of Danielle Maywood 🤖

C014 (DEREM-13): Fixed in 153c0018. The chat ID matcher now qualifies input.object.id as chats_expanded.id :: text, which matches the current authorization filter query context.

DanielleMaywood · 2026-05-13T16:39:23Z

/coder-agents-review

DanielleMaywood · 2026-05-19T10:43:47Z

/coder-agents-review

coder-agents-review

Post-human-review verification complete. The R6 structural changes (removal of canonicalChatACLKeys, removal of writeChatACLSubChatError abstraction, comment cleanups) introduce no regressions.

Mafuuu re-raised the canonicalization root cause: without normalization, non-canonical UUID keys from non-standard API clients create phantom ACL entries that appear with wrong roles and cannot be deleted. The human reviewer explicitly chose this direction ("We have control over the client. See what we do with workspace ACL."), making chat ACL consistent with workspace ACL. The consequence is documented in the inventory as a human-decided trade-off (DEREM-24, Note).

The simplified handlers are cleaner: sub-chat checks write 400 directly instead of routing through a panic-guarded helper. Authorization boundaries remain correct across the simplification.

🤖 This review was automatically generated with Coder Agents.

johnstcn · 2026-05-19T12:11:23Z

+		if chat.RootChatID.Valid {
+			resp.Detail = "Target the root chat (id: " + chat.RootChatID.UUID.String() + ") instead."
+		}


For other readers: chat.RootChatID could be zero if it's a third-level-deep subchat.

We should probably track the owning chat even on Nth depth?

mafredri

Other than system restricted, LGTM 👍🏻

mafredri · 2026-05-19T12:40:59Z

+		if chat.RootChatID.Valid {
+			resp.Detail = "Target the root chat (id: " + chat.RootChatID.UUID.String() + ") instead."
+		}


We should probably track the owning chat even on Nth depth?

mafredri · 2026-05-19T14:32:50Z

+		// may not otherwise be readable by the caller.
+		var err error
+		//nolint:gocritic
+		dbGroups, err = api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs})


Why do we need as system restricted here? The gocritic nolint also has no explanation. We should try to avoid it if we can (e.g. as chatd).

Same justification as for workspaces

coder/coderd/workspaces.go

Lines 2386 to 2421 in 51b531f

// `GetGroups` returns all groups if `GroupIds` is empty so we check the length

// before making the DB call.

dbGroups := make([]database.GetGroupsRow, 0)

if len(groupIDs) > 0 {

// For context see https://github.com/coder/coder/pull/19375

// nolint:gocritic

dbGroups, err = api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs})

if err != nil && !xerrors.Is(err, sql.ErrNoRows) {

httpapi.InternalServerError(rw, err)

return

}

}

groups := make([]codersdk.WorkspaceGroup, 0, len(dbGroups))

for _, it := range dbGroups {

var members []database.GroupMember

// For context see https://github.com/coder/coder/pull/19375

// nolint:gocritic

members, err = api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{

GroupID: it.Group.ID,

IncludeSystem: false,

})

if err != nil {

httpapi.InternalServerError(rw, err)

return

}

groups = append(groups, codersdk.WorkspaceGroup{

Group: db2sdk.Group(database.GetGroupsRow{

Group: it.Group,

OrganizationName: it.OrganizationName,

OrganizationDisplayName: it.OrganizationDisplayName,

}, members, len(members)),

Role: convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions),

})

}

#19375

Happy to update the comment to make it clearer if you want?

Gotcha. Let's at least attach reasoning to the nolint line: //nolint:gocritic // [Reason why we're disabling it.]. It should state why, not just link to context (the linked PR suffers from the same issue). Adding the reason to the nolint line ensures it never gets disconnected.

github-actions Bot assigned DanielleMaywood May 5, 2026

DanielleMaywood force-pushed the dm/chat-sharing branch 2 times, most recently from 100cc49 to a001ff0 Compare May 6, 2026 18:00

DanielleMaywood mentioned this pull request May 7, 2026

feat: add chat sharing foundation #25041

Merged

DanielleMaywood force-pushed the dm/chat-sharing branch from a001ff0 to baeaf16 Compare May 7, 2026 14:36

DanielleMaywood changed the title ~~feat: add chat sharing~~ feat: add chat sharing API May 7, 2026

DanielleMaywood changed the base branch from main to dm/chat-sharing-foundation May 7, 2026 14:36

DanielleMaywood mentioned this pull request May 7, 2026

feat: enable chat sharing #24987

Merged

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from be8d940 to 1f00c89 Compare May 7, 2026 14:44

DanielleMaywood force-pushed the dm/chat-sharing branch from baeaf16 to 806e74d Compare May 7, 2026 14:46

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from 1f00c89 to 57c2c31 Compare May 7, 2026 16:00

DanielleMaywood force-pushed the dm/chat-sharing branch from 806e74d to f77ce50 Compare May 7, 2026 16:00

coder-agents-review Bot suggested changes May 8, 2026

View reviewed changes

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from 57c2c31 to 32efc17 Compare May 8, 2026 14:52

DanielleMaywood force-pushed the dm/chat-sharing branch 2 times, most recently from 7f45110 to fbb5847 Compare May 11, 2026 09:43

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from 32efc17 to d43fb9f Compare May 11, 2026 09:43

DanielleMaywood force-pushed the dm/chat-sharing branch from fbb5847 to 52f8308 Compare May 11, 2026 10:37

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch 2 times, most recently from 295dbe5 to d00ecb3 Compare May 11, 2026 12:55

DanielleMaywood force-pushed the dm/chat-sharing branch from 52f8308 to 8f9483a Compare May 11, 2026 12:55

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from d00ecb3 to ac20edb Compare May 12, 2026 12:28

DanielleMaywood force-pushed the dm/chat-sharing branch from 8f9483a to bdfd540 Compare May 12, 2026 12:28

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from f01b1d5 to dfc4533 Compare May 13, 2026 10:46

DanielleMaywood force-pushed the dm/chat-sharing branch from bdfd540 to ae29c15 Compare May 13, 2026 14:15

DanielleMaywood force-pushed the dm/chat-sharing-foundation branch from 7e8ecf0 to c544179 Compare May 18, 2026 21:15

Base automatically changed from dm/chat-sharing-foundation to main May 18, 2026 21:32

DanielleMaywood added 10 commits May 18, 2026 21:36

feat(coderd): add chat sharing API

f0b2603

fix: address chat sharing API feedback

4d659da

fix(coderd/database): cover chat owner fields

018d462

fix(coderd): address chat ACL review feedback

6f6d8c3

fix(coderd): reject duplicate chat ACL keys

0b8c7b2

Apply suggestion from @DanielleMaywood

9d73c9a

Apply suggestion from @DanielleMaywood

d86931e

fix(coderd): simplify chat ACL handling

0e9faf6

chore(docs/reference/api): regenerate chat API docs

7a8f45e

fix(coderd): align chat ACL tests with shared write denials

4161734

DanielleMaywood force-pushed the dm/chat-sharing branch from e65ee30 to 4161734 Compare May 18, 2026 21:46

DanielleMaywood added 2 commits May 19, 2026 10:08

fix(coderd): trim chat sharing API surface

cc515a0

fix(coderd): restore chat owner identity fields

949f616

coder-agents-review Bot approved these changes May 19, 2026

View reviewed changes

chore(coderd): clarify chat ACL tests

050ea00

DanielleMaywood marked this pull request as ready for review May 19, 2026 11:59

DanielleMaywood requested a review from Emyrk as a code owner May 19, 2026 11:59

DanielleMaywood requested review from johnstcn and mafredri May 19, 2026 12:00

johnstcn reviewed May 19, 2026

View reviewed changes

Comment thread coderd/exp_chats_acl.go

johnstcn approved these changes May 19, 2026

View reviewed changes

mafredri approved these changes May 19, 2026

View reviewed changes

DanielleMaywood added 2 commits May 20, 2026 09:11

fix(coderd): document chat ACL authz nolints

ceb9cf9

fix(coderd): clarify chat ACL authz nolints

52774b8

DanielleMaywood merged commit 96e3c49 into main May 20, 2026
31 checks passed

DanielleMaywood deleted the dm/chat-sharing branch May 20, 2026 09:46

	// `GetGroups` returns all groups if `GroupIds` is empty so we check the length
	// before making the DB call.
	dbGroups := make([]database.GetGroupsRow, 0)
	if len(groupIDs) > 0 {
	// For context see https://github.com/coder/coder/pull/19375
	// nolint:gocritic
	dbGroups, err = api.Database.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{GroupIds: groupIDs})
	if err != nil && !xerrors.Is(err, sql.ErrNoRows) {
	httpapi.InternalServerError(rw, err)
	return
	}
	}

	groups := make([]codersdk.WorkspaceGroup, 0, len(dbGroups))
	for _, it := range dbGroups {
	var members []database.GroupMember
	// For context see https://github.com/coder/coder/pull/19375
	// nolint:gocritic
	members, err = api.Database.GetGroupMembersByGroupID(dbauthz.AsSystemRestricted(ctx), database.GetGroupMembersByGroupIDParams{
	GroupID: it.Group.ID,
	IncludeSystem: false,
	})
	if err != nil {
	httpapi.InternalServerError(rw, err)
	return
	}
	groups = append(groups, codersdk.WorkspaceGroup{
	Group: db2sdk.Group(database.GetGroupsRow{
	Group: it.Group,
	OrganizationName: it.OrganizationName,
	OrganizationDisplayName: it.OrganizationDisplayName,
	}, members, len(members)),
	Role: convertToWorkspaceRole(workspaceACL.Groups[it.Group.ID.String()].Permissions),
	})
	}

Conversation

DanielleMaywood commented May 5, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented May 5, 2026

Docs preview

Uh oh!

DanielleMaywood commented May 8, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

DanielleMaywood commented May 13, 2026

Uh oh!

DanielleMaywood commented May 13, 2026

Uh oh!

DanielleMaywood commented May 13, 2026

Uh oh!

DanielleMaywood commented May 19, 2026

Uh oh!

coder-agents-review Bot left a comment

Choose a reason for hiding this comment

Uh oh!

johnstcn May 19, 2026

Choose a reason for hiding this comment

Uh oh!

mafredri May 19, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mafredri left a comment

Choose a reason for hiding this comment

Uh oh!

mafredri May 19, 2026

Choose a reason for hiding this comment

Uh oh!

mafredri May 19, 2026

Choose a reason for hiding this comment

Uh oh!

DanielleMaywood May 19, 2026

Choose a reason for hiding this comment

Uh oh!

mafredri May 20, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

DanielleMaywood commented May 5, 2026 •

edited

Loading