Skip to content

Commit cd9638d

Browse files
EmyrkEmyrk
committed
fixup FileError test
1 parent f332bab commit cd9638d

File tree

3 files changed

+46
-27
lines changed

3 files changed

+46
-27
lines changed

‎coderd/dynamicparameters/render.go

Lines changed: 37 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -26,7 +26,7 @@ type Renderer interface {
2626
}
2727

2828
var (
29-
ErrorTemplateVersionNotReady error = xerrors.New("template version job not finished")
29+
ErrorTemplateVersionNotReady = xerrors.New("template version job not finished")
3030
)
3131

3232
// Loader is used to load the necessary coder objects for rendering a template
@@ -144,10 +144,11 @@ func (r *Loader) dynamicRenderer(ctx context.Context, db database.Store, cache *
144144
}
145145

146146
return &DynamicRenderer{
147-
data: r,
148-
templateFS: templateFS,
149-
db: db,
150-
plan: plan,
147+
data: r,
148+
templateFS: templateFS,
149+
db: db,
150+
plan: plan,
151+
failedOwners: make(map[uuid.UUID]error),
151152
close: func() {
152153
cache.Release(r.job.FileID)
153154
if moduleFilesFS != nil {
@@ -171,8 +172,14 @@ type DynamicRenderer struct {
171172
}
172173

173174
func (r *DynamicRenderer) Render(ctx context.Context, ownerID uuid.UUID, values map[string]string) (*preview.Output, hcl.Diagnostics) {
174-
err := r.getWorkspaceOwnerData(ctx, ownerID)
175-
if err != nil || r.currentOwner == nil {
175+
// Always start with the cached error, if we have one.
176+
ownerErr := r.failedOwners[ownerID]
177+
if ownerErr == nil {
178+
ownerErr = r.getWorkspaceOwnerData(ctx, ownerID)
179+
}
180+
181+
if ownerErr != nil || r.currentOwner == nil {
182+
r.failedOwners[ownerID] = ownerErr
176183
return nil, hcl.Diagnostics{
177184
{
178185
Severity: hcl.DiagError,
@@ -199,16 +206,24 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
199206
return nil // already fetched
200207
}
201208

202-
if r.failedOwners[ownerID] != nil {
203-
// previously failed, do not try again
204-
return r.failedOwners[ownerID]
205-
}
206-
207209
var g errgroup.Group
208210

209-
// TODO: @emyrk we should only need read access on the org member, not the
210-
// site wide user object. Figure out a better way to handle this.
211-
user, err := r.db.GetUserByID(ctx, ownerID)
211+
// You only need to be able to read the organization member to get the owner
212+
// data. Only the terraform files can therefore leak more information than the
213+
// caller should have access to. All this info should be public assuming you can
214+
// read the user though.
215+
mem, err := database.ExpectOne(r.db.OrganizationMembers(ctx, database.OrganizationMembersParams{
216+
OrganizationID: r.data.templateVersion.OrganizationID,
217+
UserID: ownerID,
218+
IncludeSystem: false,
219+
}))
220+
if err != nil {
221+
return err
222+
}
223+
224+
// User data is required for the form. Org member is checked above
225+
// nolint:gocritic
226+
user, err := r.db.GetUserByID(dbauthz.AsProvisionerd(ctx), mem.OrganizationMember.UserID)
212227
if err != nil {
213228
return xerrors.Errorf("fetch user: %w", err)
214229
}
@@ -218,7 +233,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
218233
// nolint:gocritic // This is kind of the wrong query to use here, but it
219234
// matches how the provisioner currently works. We should figure out
220235
// something that needs less escalation but has the correct behavior.
221-
row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsSystemRestricted(ctx), ownerID)
236+
row, err := r.db.GetAuthorizationUserRoles(dbauthz.AsProvisionerd(ctx), ownerID)
222237
if err != nil {
223238
return err
224239
}
@@ -248,7 +263,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
248263
// The correct public key has to be sent. This will not be leaked
249264
// unless the template leaks it.
250265
// nolint:gocritic
251-
key, err := r.db.GetGitSSHKey(dbauthz.AsSystemRestricted(ctx), ownerID)
266+
key, err := r.db.GetGitSSHKey(dbauthz.AsProvisionerd(ctx), ownerID)
252267
if err != nil {
253268
return err
254269
}
@@ -262,7 +277,7 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
262277
// user, unless the template does it through the parameters. Regardless, we need
263278
// the correct groups, and a user might not have read access.
264279
// nolint:gocritic
265-
groups, err := r.db.GetGroups(dbauthz.AsSystemRestricted(ctx), database.GetGroupsParams{
280+
groups, err := r.db.GetGroups(dbauthz.AsProvisionerd(ctx), database.GetGroupsParams{
266281
OrganizationID: r.data.templateVersion.OrganizationID,
267282
HasMemberID: ownerID,
268283
})
@@ -282,10 +297,10 @@ func (r *DynamicRenderer) getWorkspaceOwnerData(ctx context.Context, ownerID uui
282297
}
283298

284299
r.currentOwner = &previewtypes.WorkspaceOwner{
285-
ID: user.ID.String(),
286-
Name: user.Username,
287-
FullName: user.Name,
288-
Email: user.Email,
300+
ID: mem.OrganizationMember.UserID.String(),
301+
Name: mem.Username,
302+
FullName: mem.Name,
303+
Email: mem.Email,
289304
LoginType: string(user.LoginType),
290305
RBACRoles: ownerRoles,
291306
SSHPublicKey: publicKey,

‎coderd/parameters.go

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,6 @@ func (api *API) templateVersionDynamicParameters(listen bool, initial codersdk.D
112112
} else {
113113
api.handleParameterEvaluate(rw, r, initial, renderer)
114114
}
115-
116115
}
117116
}
118117

‎coderd/parameters_test.go

Lines changed: 9 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -203,11 +203,16 @@ func TestDynamicParametersWithTerraformValues(t *testing.T) {
203203
provisionerDaemonVersion: provProto.CurrentVersion.String(),
204204
mainTF: dynamicParametersTerraformSource,
205205
modulesArchive: modulesArchive,
206-
expectWebsocketError: true,
207206
})
208-
// This is checked in setupDynamicParamsTest. Just doing this in the
209-
// test to make it obvious what this test is doing.
210-
require.Zero(t, setup.api.FileCache.Count())
207+
208+
stream := setup.stream
209+
previews := stream.Chan()
210+
211+
// Assert the failed owner
212+
ctx := testutil.Context(t, testutil.WaitShort)
213+
preview := testutil.RequireReceive(ctx, t, previews)
214+
require.Len(t, preview.Diagnostics, 1)
215+
require.Equal(t, preview.Diagnostics[0].Summary, "Failed to fetch workspace owner")
211216
})
212217

213218
t.Run("RebuildParameters", func(t *testing.T) {

0 commit comments

Comments
 (0)