Skip to content

fix: grant AsAIBridged ResourceSystem.ActionCreate for UpsertAISeatState #24603

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
mtojek merged 1 commit into from
Apr 22, 2026
Merged

fix: grant AsAIBridged ResourceSystem.ActionCreate for UpsertAISeatState #24603

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions coderd/database/dbauthz/dbauthz.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -622,6 +622,7 @@ var (
},
rbac.ResourceApiKey.Type: {policy.ActionRead}, // Validate API keys.
rbac.ResourceAibridgeInterception.Type: {policy.ActionCreate, policy.ActionRead, policy.ActionUpdate, policy.ActionDelete},
rbac.ResourceSystem.Type: {policy.ActionCreate}, // Required for UpsertAISeatState.
Copy link
Copy Markdown
Contributor

@jaaydenh jaaydenh Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is much too broad of a permission to add to AI Bridge which will have unintended consequences. Something a bit more fine grained is needed instead.

Copy link
Copy Markdown
Member Author

@mtojek mtojek Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically agree with you, but as a quick first for seat calculation, this option is the cheapest 👍

EDIT:

It's still better than its counterpart in provisionerd:
rbac.ResourceSystem.Type: {policy.WildcardSymbol},

Copy link
Copy Markdown
Contributor

@jaaydenh jaaydenh Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't fully understand AI bridge but I was thinking because of how it proxies LLM requests, its important to be careful about what access is given here for security reasons

Copy link
Copy Markdown
Member Author

@mtojek mtojek Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fair point. ResourceSystem is a catch-all, so granting any action on it feels too broad on the surface. Here's why I think this is safe short-term, plus what I'd suggest long-term.

Short-term safety argument (for the nearest release):

ResourceSystem + ActionCreate unlocks some querier methods (InsertDBCryptKey, InsertDERPMeshKey, InsertReplica, UpsertRuntimeConfig, etc.). However, none of them are reachable from aibridged code paths. The AI Bridge daemon interacts with the DB exclusively through aibridgedserver.Server methods, which only call:

  • ResourceAibridgeInterception operations (CRUD)
  • ResourceUser (read)
  • ResourceApiKey (read)
  • SeatTracker.RecordUsage -> UpsertAISeatState (the broken one)

Long-term: dedicated ResourceAISeat

ResourceSystem is already deprecated (the comment in object_gen.go says so explicitly), and UpsertAISeatState should have its own resource. Happy to do that as a follow-up. For the immediate fix, the blast radius is zero in practice because there's no code path from AI Bridge to any of the other ResourceSystem + ActionCreate operations.

Copy link
Copy Markdown
Member Author

@mtojek mtojek Apr 22, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI here is a draft PR for the long-term version: #24613 , but we'll need RBAC subject-matter expert to review it thoroughly.

}),
User: []rbac.Permission{},
ByOrgID: map[string]rbac.OrgPermissions{},
Expand Down
37 changes: 37 additions & 0 deletions enterprise/aiseats/tracker_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,14 +5,19 @@ import (
"testing"
"time"

"github.com/prometheus/client_golang/prometheus"
"github.com/stretchr/testify/require"

"cdr.dev/slog/v3/sloggers/slogtest"
agplaiseats "github.com/coder/coder/v2/coderd/aiseats"
"github.com/coder/coder/v2/coderd/audit"
"github.com/coder/coder/v2/coderd/coderdtest"
"github.com/coder/coder/v2/coderd/database"
"github.com/coder/coder/v2/coderd/database/dbauthz"
"github.com/coder/coder/v2/coderd/database/dbgen"
"github.com/coder/coder/v2/coderd/database/dbtestutil"
"github.com/coder/coder/v2/coderd/database/dbtime"
"github.com/coder/coder/v2/coderd/rbac"
enterpriseaiseats "github.com/coder/coder/v2/enterprise/aiseats"
"github.com/coder/coder/v2/testutil"
"github.com/coder/quartz"
Expand All @@ -37,6 +42,38 @@ func TestSeatTrackerDB(t *testing.T) {
require.EqualValues(t, 1, count)
})

// Regression test for coder/internal#1444: UpsertAISeatState must
// succeed when called through the AsAIBridged RBAC subject. The
// aibridged daemon context was missing ResourceSystem.ActionCreate,
// which caused the very first RecordUsage call per user to fail
// with "unauthorized: rbac: forbidden".
t.Run("AsAIBridgedRBAC", func(t *testing.T) {
t.Parallel()

rawDB, _ := dbtestutil.NewDB(t)
authz := rbac.NewStrictAuthorizer(prometheus.NewRegistry())
authzDB := dbauthz.New(rawDB, authz, slogtest.Make(t, nil), coderdtest.AccessControlStorePointer())

ctx := testutil.Context(t, testutil.WaitShort)
clock := quartz.NewMock(t)
tracker := enterpriseaiseats.New(authzDB, testutil.Logger(t), clock, nil)

// Insert a user directly in the raw DB so it exists for the
// foreign key reference.
user := dbgen.User(t, rawDB, database.User{Status: database.UserStatusActive})

// Call RecordUsage with the AIBridged context, mirroring the
// production call path in aibridgedserver.RecordInterception.
aibridgedCtx := dbauthz.AsAIBridged(ctx)
tracker.RecordUsage(aibridgedCtx, user.ID, agplaiseats.ReasonAIBridge("provider=test, model=test"))

// Verify the seat was actually recorded. A count of 0 means
// the upsert was silently rejected by RBAC.
count, err := rawDB.GetActiveAISeatCount(ctx)
require.NoError(t, err)
require.EqualValues(t, 1, count, "AI seat should be recorded when using AsAIBridged context")
})

t.Run("InactiveUsersExcluded", func(t *testing.T) {
t.Parallel()

Expand Down
Loading