8000 fix: allow dynamic parameters without requiring org membership (#18531) · coder/coder@341b54e · GitHub
[go: up one dir, main page]
Skip to content
8000

Commit 341b54e

Browse files
EmyrkEmyrk
authored
fix: allow dynamic parameters without requiring org membership (#18531)
1 parent 5816455 commit 341b54e

File tree

2 files changed

+88
-24
lines changed

2 files changed

+88
-24
lines changed

‎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,

‎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 = < EDBE span class="pl-s1">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()

0 commit comments

Comments
 (0)
0