Skip to content

Commit 9c8f951

Browse files
johnstcnjohnstcn
authored
Merge branch 'main' into mafredri/feat-agent-enable-devcontainers-by-default-
2 parents cdd0f2f + f44969b commit 9c8f951

File tree

9 files changed

+143
-55
lines changed

9 files changed

+143
-55
lines changed

‎coderd/database/dbauthz/dbauthz.go

Lines changed: 14 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -151,26 +151,28 @@ func (q *querier) authorizeContext(ctx context.Context, action policy.Action, ob
151151

152152
// authorizePrebuiltWorkspace handles authorization for workspace resource types.
153153
// prebuilt_workspaces are a subset of workspaces, currently limited to
154-
// supporting delete operations. Therefore, if the action is delete or
155-
// update and the workspace is a prebuild, a prebuilt-specific authorization
156-
// is attempted first. If that fails, it falls back to normal workspace
157-
// authorization.
154+
// supporting delete operations. This function first attempts normal workspace
155+
// authorization. If that fails, the action is delete or update and the workspace
156+
// is a prebuild, a prebuilt-specific authorization is attempted.
158157
// Note: Delete operations of workspaces requires both update and delete
159158
// permissions.
160159
func (q *querier) authorizePrebuiltWorkspace(ctx context.Context, action policy.Action, workspace database.Workspace) error {
161-
var prebuiltErr error
162-
// Special handling for prebuilt_workspace deletion authorization check
160+
// Try default workspace authorization first
161+
var workspaceErr error
162+
if workspaceErr = q.authorizeContext(ctx, action, workspace); workspaceErr == nil {
163+
return nil
164+
}
165+
166+
// Special handling for prebuilt workspace deletion
163167
if (action == policy.ActionUpdate || action == policy.ActionDelete) && workspace.IsPrebuild() {
164-
// Try prebuilt-specific authorization first
168+
var prebuiltErr error
165169
if prebuiltErr = q.authorizeContext(ctx, action, workspace.AsPrebuild()); prebuiltErr == nil {
166170
return nil
167171
}
172+
return xerrors.Errorf("authorize context failed for workspace (%v) and prebuilt (%w)", workspaceErr, prebuiltErr)
168173
}
169-
// Fallback to normal workspace authorization check
170-
if err := q.authorizeContext(ctx, action, workspace); err != nil {
171-
return xerrors.Errorf("authorize context: %w", errors.Join(prebuiltErr, err))
172-
}
173-
return nil
174+
175+
return xerrors.Errorf("authorize context: %w", workspaceErr)
174176
}
175177

176178
type authContextKey struct{}

‎coderd/database/dbauthz/dbauthz_test.go

Lines changed: 22 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -5650,7 +5650,17 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() {
56505650
Reason: database.BuildReasonInitiator,
56515651
TemplateVersionID: tv.ID,
56525652
JobID: pj.ID,
5653-
}).Asserts(w.AsPrebuild(), policy.ActionDelete)
5653+
}).
5654+
// Simulate a fallback authorization flow:
5655+
// - First, the default workspace authorization fails (simulated by returning an error).
5656+
// - Then, authorization is retried using the prebuilt workspace object, which succeeds.
5657+
// The test asserts that both authorization attempts occur in the correct order.
5658+
WithSuccessAuthorizer(func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error {
5659+
if obj.Type == rbac.ResourceWorkspace.Type {
5660+
return xerrors.Errorf("not authorized for workspace type")
5661+
}
5662+
return nil
5663+
}).Asserts(w, policy.ActionDelete, w.AsPrebuild(), policy.ActionDelete)
56545664
}))
56555665
s.Run("PrebuildUpdate/InsertWorkspaceBuildParameters", s.Subtest(func(db database.Store, check *expects) {
56565666
u := dbgen.User(s.T(), db, database.User{})
@@ -5679,6 +5689,16 @@ func (s *MethodTestSuite) TestAuthorizePrebuiltWorkspace() {
56795689
})
56805690
check.Args(database.InsertWorkspaceBuildParametersParams{
56815691
WorkspaceBuildID: wb.ID,
5682-
}).Asserts(w.AsPrebuild(), policy.ActionUpdate)
5692+
}).
5693+
// Simulate a fallback authorization flow:
5694+
// - First, the default workspace authorization fails (simulated by returning an error).
5695+
// - Then, authorization is retried using the prebuilt workspace object, which succeeds.
5696+
// The test asserts that both authorization attempts occur in the correct order.
5697+
WithSuccessAuthorizer(func(ctx context.Context, subject rbac.Subject, action policy.Action, obj rbac.Object) error {
5698+
if obj.Type == rbac.ResourceWorkspace.Type {
5699+
return xerrors.Errorf("not authorized for workspace type")
5700+
}
5701+
return nil
5702+
}).Asserts(w, policy.ActionUpdate, w.AsPrebuild(), policy.ActionUpdate)
56835703
}))
56845704
}

‎coderd/database/modelmethods.go

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -199,6 +199,13 @@ func (gm GroupMember) RBACObject() rbac.Object {
199199
return rbac.ResourceGroupMember.WithID(gm.UserID).InOrg(gm.OrganizationID).WithOwner(gm.UserID.String())
200200
}
201201

202+
// PrebuiltWorkspaceResource defines the interface for types that can be identified as prebuilt workspaces
203+
// and converted to their corresponding prebuilt workspace RBAC object.
204+
type PrebuiltWorkspaceResource interface {
205+
IsPrebuild() bool
206+
AsPrebuild() rbac.Object
207+
}
208+
202209
// WorkspaceTable converts a Workspace to it's reduced version.
203210
// A more generalized solution is to use json marshaling to
204211
// consistently keep these two structs in sync.

‎coderd/dynamicparameters/render.go

Lines changed: 26 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -243,24 +243,30 @@ func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
243243
return nil // already fetched
244244
}
245245

246-
// You only need to be able to read the organization member to get the owner
247-
// data. Only the terraform files can therefore leak more information than the
248-
// caller should have access to. All this info should be public assuming you can
249-
// read the user though.
250-
mem, err := database.ExpectOne(r.db.OrganizationMembers(ctx, database.OrganizationMembersParams{
251-
OrganizationID: r.data.templateVersion.OrganizationID,
252-
UserID: ownerID,
253-
IncludeSystem: true,
254-
}))
246+
user, err := r.db.GetUserByID(ctx, ownerID)
255247
if err != nil {
256-
return err
257-
}
248+
// If the user failed to read, we also try to read the user from their
249+
// organization member. You only need to be able to read the organization member
250+
// to get the owner data.
251+
//
252+
// Only the terraform files can therefore leak more information than the
253+
// caller should have access to. All this info should be public assuming you can
254+
// read the user though.
255+
mem, err := database.ExpectOne(r.db.OrganizationMembers(ctx, database.OrganizationMembersParams{
256+
OrganizationID: r.data.templateVersion.OrganizationID,
257+
UserID: ownerID,
258+
IncludeSystem: true,
259+
}))
260+
if err != nil {
261+
return xerrors.Errorf("fetch user: %w", err)
262+
}
258263

259-
// User data is required for the form. Org member is checked above
260-
// nolint:gocritic
261-
user, err := r.db.GetUserByID(dbauthz.AsProvisionerd(ctx), mem.OrganizationMember.UserID)
262-
if err != nil {
263-
return xerrors.Errorf("fetch user: %w", err)
264+
// Org member fetched, so use the provisioner context to fetch the user.
265+
//nolint:gocritic // Has the correct permissions, and matches the provisioning flow.
266+
user, err = r.db.GetUserByID(dbauthz.AsProvisionerd(ctx), mem.OrganizationMember.UserID)
267+
if err != nil {
268+
return xerrors.Errorf("fetch user: %w", err)
269+
}
264270
}
265271

266272
// nolint:gocritic // This is kind of the wrong query to use here, but it
@@ -314,10 +320,10 @@ func (r *dynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
314320
}
315321

316322
r.currentOwner = &previewtypes.WorkspaceOwner{
317-
ID: mem.OrganizationMember.UserID.String(),
318-
Name: mem.Username,
319-
FullName: mem.Name,
320-
Email: mem.Email,
323+
ID: user.ID.String(),
324+
Name: user.Username,
325+
FullName: user.Name,
326+
Email: user.Email,
321327
LoginType: string(user.LoginType),
322328
RBACRoles: ownerRoles,
323329
SSHPublicKey: key.PublicKey,

‎coderd/workspacebuilds.go

Lines changed: 7 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -391,17 +391,16 @@ func (api *API) postWorkspaceBuilds(rw http.ResponseWriter, r *http.Request) {
391391
tx,
392392
api.FileCache,
393393
func(action policy.Action, object rbac.Objecter) bool {
394+
if auth := api.Authorize(r, action, object); auth {
395+
return true
396+
}
394397
// Special handling for prebuilt workspace deletion
395-
if object.RBACObject().Type == rbac.ResourceWorkspace.Type && action == policy.ActionDelete {
396-
if workspaceObj, ok := object.(database.Workspace); ok {
397-
// Try prebuilt-specific authorization first
398-
if auth := api.Authorize(r, action, workspaceObj.AsPrebuild()); auth {
399-
return auth
400-
}
398+
if action == policy.ActionDelete {
399+
if workspaceObj, ok := object.(database.PrebuiltWorkspaceResource); ok && workspaceObj.IsPrebuild() {
400+
return api.Authorize(r, action, workspaceObj.AsPrebuild())
401401
}
402402
}
403-
// Fallback to default authorization
404-
return api.Authorize(r, action, object)
403+
return false
405404
},
406405
audit.WorkspaceBuildBaggageFromRequest(r),
407406
)

‎coderd/wsbuilder/wsbuilder.go

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1049,14 +1049,12 @@ func (b *Builder) authorize(authFunc func(action policy.Action, object rbac.Obje
10491049
return BuildError{http.StatusBadRequest, msg, xerrors.New(msg)}
10501050
}
10511051

1052+
// Try default workspace authorization first
1053+
authorized := authFunc(action, b.workspace)
1054+
10521055
// Special handling for prebuilt workspace deletion
1053-
authorized := false
1054-
if action == policy.ActionDelete && b.workspace.IsPrebuild() && authFunc(action, b.workspace.AsPrebuild()) {
1055-
authorized = true
1056-
}
1057-
// Fallback to default authorization
1058-
if !authorized && authFunc(action, b.workspace) {
1059-
authorized = true
1056+
if !authorized && action == policy.ActionDelete && b.workspace.IsPrebuild() {
1057+
authorized = authFunc(action, b.workspace.AsPrebuild())
10601058
}
10611059

10621060
if !authorized {

‎codersdk/deployment.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3380,7 +3380,6 @@ var ExperimentsKnown = Experiments{
33803380
ExperimentWebPush,
33813381
ExperimentWorkspacePrebuilds,
33823382
ExperimentAgenticChat,
3383-
ExperimentAITasks,
33843383
}
33853384

33863385
// ExperimentsSafe should include all experiments that are safe for

‎enterprise/coderd/workspaces_test.go

Lines changed: 62 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -287,11 +287,9 @@ func TestCreateUserWorkspace(t *testing.T) {
287287
OrganizationID: first.OrganizationID,
288288
})
289289

290-
version := coderdtest.CreateTemplateVersion(t, admin, first.OrganizationID, nil)
291-
coderdtest.AwaitTemplateVersionJobCompleted(t, admin, version.ID)
292-
template := coderdtest.CreateTemplate(t, admin, first.OrganizationID, version.ID)
290+
template, _ := coderdtest.DynamicParameterTemplate(t, admin, first.OrganizationID, coderdtest.DynamicParameterTemplateParams{})
293291

294-
ctx = testutil.Context(t, testutil.WaitLong*1000) // Reset the context to avoid timeouts.
292+
ctx = testutil.Context(t, testutil.WaitLong)
295293

296294
wrk, err := creator.CreateUserWorkspace(ctx, adminID.ID.String(), codersdk.CreateWorkspaceRequest{
297295
TemplateID: template.ID,
@@ -306,6 +304,66 @@ func TestCreateUserWorkspace(t *testing.T) {
306304
require.NoError(t, err)
307305
})
308306

307+
t.Run("ForANonOrgMember", func(t *testing.T) {
308+
t.Parallel()
309+
310+
owner, first := coderdenttest.New(t, &coderdenttest.Options{
311+
Options: &coderdtest.Options{
312+
IncludeProvisionerDaemon: true,
313+
},
314+
LicenseOptions: &coderdenttest.LicenseOptions{
315+
Features: license.Features{
316+
codersdk.FeatureCustomRoles: 1,
317+
codersdk.FeatureTemplateRBAC: 1,
318+
codersdk.FeatureMultipleOrganizations: 1,
319+
},
320+
},
321+
})
322+
ctx := testutil.Context(t, testutil.WaitShort)
323+
//nolint:gocritic // using owner to setup roles
324+
r, err := owner.CreateOrganizationRole(ctx, codersdk.Role{
325+
Name: "creator",
326+
OrganizationID: first.OrganizationID.String(),
327+
DisplayName: "Creator",
328+
OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
329+
codersdk.ResourceWorkspace: {codersdk.ActionCreate, codersdk.ActionWorkspaceStart, codersdk.ActionUpdate, codersdk.ActionRead},
330+
codersdk.ResourceOrganizationMember: {codersdk.ActionRead},
331+
}),
332+
})
333+
require.NoError(t, err)
334+
335+
// user to make the workspace for, **note** the user is not a member of the first org.
336+
// This is strange, but technically valid. The creator can create a workspace for
337+
// this user in this org, even though the user cannot access the workspace.
338+
secondOrg := coderdenttest.CreateOrganization(t, owner, coderdenttest.CreateOrganizationOptions{})
339+
_, forUser := coderdtest.CreateAnotherUser(t, owner, secondOrg.ID)
340+
341+
// try the test action with this user & custom role
342+
creator, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleMember(),
343+
rbac.RoleTemplateAdmin(), // Need site wide access to make workspace for non-org
344+
rbac.RoleIdentifier{
345+
Name: r.Name,
346+
OrganizationID: first.OrganizationID,
347+
},
348+
)
349+
350+
template, _ := coderdtest.DynamicParameterTemplate(t, creator, first.OrganizationID, coderdtest.DynamicParameterTemplateParams{})
351+
352+
ctx = testutil.Context(t, testutil.WaitLong)
353+
354+
wrk, err := creator.CreateUserWorkspace(ctx, forUser.ID.String(), codersdk.CreateWorkspaceRequest{
355+
TemplateID: template.ID,
356+
Name: "workspace",
357+
})
358+
require.NoError(t, err)
359+
coderdtest.AwaitWorkspaceBuildJobCompleted(t, creator, wrk.LatestBuild.ID)
360+
361+
_, err = creator.WorkspaceByOwnerAndName(ctx, forUser.Username, wrk.Name, codersdk.WorkspaceOptions{
362+
IncludeDeleted: false,
363+
})
364+
require.NoError(t, err)
365+
})
366+
309367
// Asserting some authz calls when creating a workspace.
310368
t.Run("AuthzStory", func(t *testing.T) {
311369
t.Parallel()

‎site/src/api/typesGenerated.ts

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

0 commit comments

Comments
 (0)