mirror of
https://github.com/coder/coder.git
synced 2026-10-16 14:48:56 +08:00
fix(coderd): require read and update chat permissions for delegated chat owners (#29684)
`POST /api/v2/chats` with `owner_id` (#29581) checks that the delegated owner holds `chat:create` before creating the chat under the owner's context. Creation also inserts the first message (`chat:update`) and reads the chat back (`chat:read`) inside the same transaction, so an owner whose custom organization role grants only `chat:create` passed the preflight and then failed inside the transaction with a generic 403 instead of the intended 400 `Chat owner does not have permission to use chats.` The transaction rolls back, so no chat is left behind; only the status code and message were wrong. The preflight now requires create, read, and update for the owner. Regular organization members hold all three through the member role, so this only changes the response for service accounts and custom roles with partial chat grants. Found by Codex on the 2.37 backport (#29683), which carries this same commit. The code is identical on main. ## Validation - `TestPostChats_OwnerID/OwnerWithCreateOnlyChatPermission`: red on the unfixed head (403 instead of 400), green with the fix. - `go test ./coderd/ -run 'TestPostChats|TestPatchChat' -count=1`: pass. - Pre-commit hook (gen, fmt, lint, slim build) passed on this commit. > Xum opened this PR on behalf of @ibetitsmike.
This commit is contained in:
+12
-7
@@ -1260,7 +1260,8 @@ func (api *API) postChats(rw http.ResponseWriter, r *http.Request) {
|
||||
// parsing because we need req.OrganizationID to scope the RBAC check
|
||||
// to the correct org. The request body is bounded by the ReadLimit above,
|
||||
// limiting the cost of parsing before rejection.
|
||||
if !api.Authorize(r, policy.ActionCreate, rbac.ResourceChat.WithOwner(ownerID.String()).InOrg(req.OrganizationID)) {
|
||||
chatObject := rbac.ResourceChat.WithOwner(ownerID.String()).InOrg(req.OrganizationID)
|
||||
if !api.Authorize(r, policy.ActionCreate, chatObject) {
|
||||
httpapi.Forbidden(rw)
|
||||
return
|
||||
}
|
||||
@@ -1306,12 +1307,16 @@ func (api *API) postChats(rw http.ResponseWriter, r *http.Request) {
|
||||
// must succeed for the owner, not merely for the caller.
|
||||
ownerCtx = dbauthz.As(ctx, owner)
|
||||
// Service accounts and custom roles may lack chat permissions
|
||||
// entirely; the owner could then never read the chat.
|
||||
if !api.HTTPAuth.AuthorizeContext(ownerCtx, policy.ActionCreate, rbac.ResourceChat.WithOwner(ownerID.String()).InOrg(req.OrganizationID)) {
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||
Message: "Chat owner does not have permission to use chats.",
|
||||
})
|
||||
return
|
||||
// entirely or hold only some of them. Creation inserts the first
|
||||
// message and reads the chat back under the owner, so a partial
|
||||
// grant would fail inside the transaction with a generic 403.
|
||||
for _, action := range []policy.Action{policy.ActionCreate, policy.ActionRead, policy.ActionUpdate} {
|
||||
if !api.HTTPAuth.AuthorizeContext(ownerCtx, action, chatObject) {
|
||||
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
|
||||
Message: "Chat owner does not have permission to use chats.",
|
||||
})
|
||||
return
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -2062,6 +2062,40 @@ func TestPostChats_OwnerID(t *testing.T) {
|
||||
sdkErr := requireSDKError(t, err, http.StatusBadRequest)
|
||||
require.Equal(t, "Chat owner does not have permission to use chats.", sdkErr.Message)
|
||||
})
|
||||
|
||||
t.Run("OwnerWithCreateOnlyChatPermission", func(t *testing.T) {
|
||||
t.Parallel()
|
||||
|
||||
ctx := testutil.Context(t, testutil.WaitLong)
|
||||
rawDB, pubsub := dbtestutil.NewDB(t)
|
||||
client, _ := newChatClientWithDatabase(t, func(opts *coderdtest.Options) {
|
||||
opts.Database = rawDB
|
||||
opts.Pubsub = pubsub
|
||||
})
|
||||
firstUser := coderdtest.CreateFirstUser(t, client.Client)
|
||||
_ = createChatModel(t, client)
|
||||
// A custom role can grant chat:create without read or update.
|
||||
role, err := rawDB.InsertCustomRole(ctx, database.InsertCustomRoleParams{
|
||||
Name: testutil.GetRandomName(t),
|
||||
DisplayName: "Chat Creator",
|
||||
OrganizationID: uuid.NullUUID{UUID: firstUser.OrganizationID, Valid: true},
|
||||
OrgPermissions: database.CustomRolePermissions{{
|
||||
ResourceType: rbac.ResourceChat.Type,
|
||||
Action: policy.ActionCreate,
|
||||
}},
|
||||
})
|
||||
require.NoError(t, err)
|
||||
serviceAccount := dbgen.User(t, rawDB, database.User{IsServiceAccount: true})
|
||||
dbgen.OrganizationMember(t, rawDB, database.OrganizationMember{
|
||||
OrganizationID: firstUser.OrganizationID,
|
||||
UserID: serviceAccount.ID,
|
||||
Roles: []string{role.Name},
|
||||
})
|
||||
|
||||
_, err = client.CreateChat(ctx, helloRequest(serviceAccount.ID, firstUser.OrganizationID))
|
||||
sdkErr := requireSDKError(t, err, http.StatusBadRequest)
|
||||
require.Equal(t, "Chat owner does not have permission to use chats.", sdkErr.Message)
|
||||
})
|
||||
}
|
||||
|
||||
// TestChats_ForceOnMCPServerEnforced is the endpoint-level regression
|
||||
|
||||
Reference in New Issue
Block a user