Skip to content

Commit 2c1cbbb

Browse files
ammarioammario
committed
Address spike + mathias + cian comments
1 parent 4b00f45 commit 2c1cbbb

File tree

13 files changed

+173
-96
lines changed

13 files changed

+173
-96
lines changed

‎coderd/database/dbauthz/dbauthz.go

+1-3
Original file line numberDiff line numberDiff line change
@@ -1759,9 +1759,7 @@ func (q *querier) GetUserWorkspaceBuildParameters(ctx context.Context, params da
17591759
if err != nil {
17601760
return nil, err
17611761
}
1762-
// The ability to update the user implies either the user themselves or someone
1763-
// with complete admin access to the user account.
1764-
if err := q.authorizeContext(ctx, rbac.ActionUpdate, u.UserDataRBACObject()); err != nil {
1762+
if err := q.authorizeContext(ctx, rbac.ActionRead, u.UserWorkspaceBuildParametersObject()); err != nil {
17651763
return nil, err
17661764
}
17671765
return q.db.GetUserWorkspaceBuildParameters(ctx, params)

‎coderd/database/dbmem/dbmem.go

+37-4
Original file line numberDiff line numberDiff line change
@@ -3773,15 +3773,48 @@ func (q *FakeQuerier) GetUserWorkspaceBuildParameters(_ context.Context, params
37733773
userWorkspaceIDs[ws.ID] = struct{}{}
37743774
}
37753775

3776-
userWorkspaceBuildParameters := make([]database.GetUserWorkspaceBuildParametersRow, 0)
3776+
userWorkspaceBuilds := make(map[uuid.UUID]struct{})
3777+
for _, wb := range q.workspaceBuilds {
3778+
if _, ok := userWorkspaceIDs[wb.WorkspaceID]; !ok {
3779+
continue
3780+
}
3781+
userWorkspaceBuilds[wb.ID] = struct{}{}
3782+
}
3783+
3784+
templateVersions := make(map[uuid.UUID]struct{})
3785+
for _, tv := range q.templateVersions {
3786+
if tv.TemplateID.UUID != params.TemplateID {
3787+
continue
3788+
}
3789+
templateVersions[tv.ID] = struct{}{}
3790+
}
3791+
3792+
tvps := make(map[string]struct{})
3793+
for _, tvp := range q.templateVersionParameters {
3794+
if _, ok := templateVersions[tvp.TemplateVersionID]; !ok {
3795+
continue
3796+
}
3797+
3798+
if _, ok := tvps[tvp.Name]; !ok && !tvp.Ephemeral {
3799+
tvps[tvp.Name] = struct{}{}
3800+
}
3801+
}
3802+
3803+
userWorkspaceBuildParameters := make(map[string]database.GetUserWorkspaceBuildParametersRow)
37773804
for _, wbp := range q.workspaceBuildParameters {
3778-
userWorkspaceBuildParameters = append(userWorkspaceBuildParameters, database.GetUserWorkspaceBuildParametersRow{
3805+
if _, ok := userWorkspaceBuilds[wbp.WorkspaceBuildID]; !ok {
3806+
continue
3807+
}
3808+
if _, ok := tvps[wbp.Name]; !ok {
3809+
continue
3810+
}
3811+
userWorkspaceBuildParameters[wbp.Name] = database.GetUserWorkspaceBuildParametersRow{
37793812
Name: wbp.Name,
37803813
Value: wbp.Value,
3781-
})
3814+
}
37823815
}
37833816

3784-
return userWorkspaceBuildParameters, nil
3817+
return maps.Values(userWorkspaceBuildParameters), nil
37853818
}
37863819

37873820
func (q *FakeQuerier) GetUsers(_ context.Context, params database.GetUsersParams) ([]database.GetUsersRow, error) {

‎coderd/database/modelmethods.go

+4
Original file line numberDiff line numberDiff line change
@@ -255,6 +255,10 @@ func (u User) UserDataRBACObject() rbac.Object {
255255
return rbac.ResourceUserData.WithID(u.ID).WithOwner(u.ID.String())
256256
}
257257

258+
func (u User) UserWorkspaceBuildParametersObject() rbac.Object {
259+
return rbac.ResourceUserWorkspaceBuildParameters.WithID(u.ID).WithOwner(u.ID.String())
260+
}
261+
258262
func (u GetUsersRow) RBACObject() rbac.Object {
259263
return rbac.ResourceUserObject(u.ID)
260264
}

‎coderd/database/queries/workspacebuildparameters.sql

+1
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ FROM (
3838
AND wb.transition = 'start'
3939
AND w.template_id = $2
4040
AND tvp.ephemeral = false
41+
AND tvp.name = wbp.name
4142
) sub
4243
WHERE
4344
sub.rn = 1

‎coderd/rbac/object.go

+6
Original file line numberDiff line numberDiff line change
@@ -148,6 +148,12 @@ var (
148148
Type: "user_data",
149149
}
150150

151+
// ResourceUserWorkspaceBuildParameters is the user's workspace build
152+
// parameter history.
153+
ResourceUserWorkspaceBuildParameters = Object{
154+
Type: "user_workspace_build_parameters",
155+
}
156+
151157
// ResourceOrganizationMember is a user's membership in an organization.
152158
// Has ONLY an organization owner.
153159
// create/delete = Create/delete member from org.

‎coderd/rbac/object_gen.go

+1
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

‎coderd/rbac/roles.go

+6-4
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,8 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
154154
Permissions(map[string][]Action{
155155
// Users cannot do create/update/delete on themselves, but they
156156
// can read their own details.
157-
ResourceUser.Type: {ActionRead},
157+
ResourceUser.Type: {ActionRead},
158+
ResourceUserWorkspaceBuildParameters.Type: {ActionRead},
158159
// Users can create provisioner daemons scoped to themselves.
159160
ResourceProvisionerDaemon.Type: {ActionCreate, ActionRead, ActionUpdate},
160161
})...,
@@ -209,9 +210,10 @@ func ReloadBuiltinRoles(opts *RoleOptions) {
209210
Name: userAdmin,
210211
DisplayName: "User Admin",
211212
Site: Permissions(map[string][]Action{
212-
ResourceRoleAssignment.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
213-
ResourceUser.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
214-
ResourceUserData.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
213+
ResourceRoleAssignment.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
214+
ResourceUser.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
215+
ResourceUserData.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
216+
ResourceUserWorkspaceBuildParameters.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
215217
// Full perms to manage org members
216218
ResourceOrganizationMember.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},
217219
ResourceGroup.Type: {ActionCreate, ActionRead, ActionUpdate, ActionDelete},

‎coderd/users.go

+8-23
Original file line numberDiff line numberDiff line change
@@ -581,30 +581,15 @@ func (api *API) userByName(rw http.ResponseWriter, r *http.Request) {
581581
// @Success 200 {array} codersdk.UserParameter
582582
// @Router /users/{user}/autofill-parameters [get]
583583
func (api *API) userAutofillParameters(rw http.ResponseWriter, r *http.Request) {
584-
var (
585-
apiKey = httpmw.APIKey(r)
586-
user = httpmw.UserParam(r)
587-
)
588-
589-
if apiKey.UserID != user.ID {
590-
httpapi.Write(r.Context(), rw, http.StatusForbidden, codersdk.Response{
591-
Message: "You are not authorized to view this user's parameters.",
592-
})
593-
return
594-
}
595-
596-
templateIDStr := r.URL.Query().Get("template_id")
597-
if templateIDStr == "" {
598-
httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{
599-
Message: "template_id is required.",
600-
})
601-
return
602-
}
584+
user := httpmw.UserParam(r)
603585

604-
templateID, err := uuid.Parse(templateIDStr)
605-
if err != nil {
586+
p := httpapi.NewQueryParamParser().Required("template_id")
587+
templateID := p.UUID(r.URL.Query(), uuid.UUID{}, "template_id")
588+
p.ErrorExcessParams(r.URL.Query())
589+
if len(p.Errors) > 0 {
606590
httpapi.Write(r.Context(), rw, http.StatusBadRequest, codersdk.Response{
607-
Message: "template_id must be a valid UUID.",
591+
Message: "Invalid query parameters.",
592+
Validations: p.Errors,
608593
})
609594
return
610595
}
@@ -616,7 +601,7 @@ func (api *API) userAutofillParameters(rw http.ResponseWriter, r *http.Request)
616601
TemplateID: templateID,
617602
},
618603
)
619-
if err != nil {
604+
if err != nil && !errors.Is(err, sql.ErrNoRows) {
620605
httpapi.Write(r.Context(), rw, http.StatusInternalServerError, codersdk.Response{
621606
Message: "Internal error fetching user's parameters.",
622607
Detail: err.Error(),

‎coderd/users_test.go

+68-24
Original file line numberDiff line numberDiff line change
@@ -1726,11 +1726,11 @@ func TestUserAutofillParameters(t *testing.T) {
17261726
t.Parallel()
17271727
t.Run("NotSelf", func(t *testing.T) {
17281728
t.Parallel()
1729-
client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
1729+
client1, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
17301730

1731-
u1 := coderdtest.CreateFirstUser(t, client)
1731+
u1 := coderdtest.CreateFirstUser(t, client1)
17321732

1733-
_, u2 := coderdtest.CreateAnotherUser(t, client, u1.OrganizationID)
1733+
client2, u2 := coderdtest.CreateAnotherUser(t, client1, u1.OrganizationID)
17341734

17351735
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
17361736
defer cancel()
@@ -1745,25 +1745,32 @@ func TestUserAutofillParameters(t *testing.T) {
17451745
Required: true,
17461746
}).Do()
17471747

1748-
_, err := client.UserAutofillParameters(
1748+
_, err := client2.UserAutofillParameters(
17491749
ctx,
1750-
u2.ID.String(),
1751-
version.Template.ID.String(),
1750+
u1.UserID.String(),
1751+
version.Template.ID,
17521752
)
17531753

17541754
var apiErr *codersdk.Error
17551755
require.ErrorAs(t, err, &apiErr)
1756-
require.Equal(t, http.StatusForbidden, apiErr.StatusCode())
1756+
require.Equal(t, http.StatusBadRequest, apiErr.StatusCode())
1757+
1758+
// u1 should be able to read u2's parameters as u1 is site admin.
1759+
_, err = client1.UserAutofillParameters(
1760+
ctx,
1761+
u2.ID.String(),
1762+
version.Template.ID,
1763+
)
1764+
require.NoError(t, err)
17571765
})
17581766

17591767
t.Run("FindsParameters", func(t *testing.T) {
17601768
t.Parallel()
1761-
client, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
1769+
client1, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{})
17621770

1763-
u1 := coderdtest.CreateFirstUser(t, client)
1771+
u1 := coderdtest.CreateFirstUser(t, client1)
17641772

1765-
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1766-
defer cancel()
1773+
client2, u2 := coderdtest.CreateAnotherUser(t, client1, u1.OrganizationID)
17671774

17681775
db := api.Database
17691776

@@ -1773,29 +1780,66 @@ func TestUserAutofillParameters(t *testing.T) {
17731780
}).Params(database.TemplateVersionParameter{
17741781
Name: "param",
17751782
Required: true,
1776-
}).Do()
1783+
},
1784+
database.TemplateVersionParameter{
1785+
Name: "param2",
1786+
Ephemeral: true,
1787+
},
1788+
).Do()
1789+
1790+
dbfake.WorkspaceBuild(t, db, database.Workspace{
1791+
OwnerID: u2.ID,
1792+
TemplateID: version.Template.ID,
1793+
}).Params(
1794+
database.WorkspaceBuildParameter{
1795+
Name: "param",
1796+
Value: "foo",
1797+
},
1798+
database.WorkspaceBuildParameter{
1799+
Name: "param2",
1800+
Value: "bar",
1801+
},
1802+
).Do()
17771803

1778-
coderdtest.CreateWorkspace(t, client, u1.OrganizationID, version.Template.ID,
1779-
func(cwr *codersdk.CreateWorkspaceRequest) {
1780-
cwr.RichParameterValues = []codersdk.WorkspaceBuildParameter{
1781-
{
1782-
Name: "param",
1783-
Value: "foo",
1784-
},
1785-
}
1786-
})
1804+
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
1805+
defer cancel()
17871806

1788-
params, err := client.UserAutofillParameters(
1807+
// Use client2 since client1 is site admin, so
1808+
// we don't get good coverage on RBAC working.
1809+
params, err := client2.UserAutofillParameters(
17891810
ctx,
1790-
u1.UserID.String(),
1791-
version.Template.ID.String(),
1811+
u2.ID.String(),
1812+
version.Template.ID,
17921813
)
17931814
require.NoError(t, err)
17941815

17951816
require.Equal(t, 1, len(params))
17961817

17971818
require.Equal(t, "param", params[0].Name)
17981819
require.Equal(t, "foo", params[0].Value)
1820+
1821+
// Verify that latest parameter value is returned.
1822+
dbfake.WorkspaceBuild(t, db, database.Workspace{
1823+
OwnerID: u2.ID,
1824+
TemplateID: version.Template.ID,
1825+
}).Params(
1826+
database.WorkspaceBuildParameter{
1827+
Name: "param",
1828+
Value: "foo_new",
1829+
},
1830+
).Do()
1831+
1832+
params, err = client2.UserAutofillParameters(
1833+
ctx,
1834+
u2.ID.String(),
1835+
version.Template.ID,
1836+
)
1837+
require.NoError(t, err)
1838+
1839+
require.Equal(t, 1, len(params))
1840+
1841+
require.Equal(t, "param", params[0].Name)
1842+
require.Equal(t, "foo_new", params[0].Value)
17991843
})
18001844
}
18011845

‎codersdk/rbacresources.go

+25-23
Original file line numberDiff line numberDiff line change
@@ -3,29 +3,30 @@ package codersdk
33
type RBACResource string
44

55
const (
6-
ResourceWorkspace RBACResource = "workspace"
7-
ResourceWorkspaceProxy RBACResource = "workspace_proxy"
8-
ResourceWorkspaceExecution RBACResource = "workspace_execution"
9-
ResourceWorkspaceApplicationConnect RBACResource = "application_connect"
10-
ResourceAuditLog RBACResource = "audit_log"
11-
ResourceTemplate RBACResource = "template"
12-
ResourceGroup RBACResource = "group"
13-
ResourceFile RBACResource = "file"
14-
ResourceProvisionerDaemon RBACResource = "provisioner_daemon"
15-
ResourceOrganization RBACResource = "organization"
16-
ResourceRoleAssignment RBACResource = "assign_role"
17-
ResourceOrgRoleAssignment RBACResource = "assign_org_role"
18-
ResourceAPIKey RBACResource = "api_key"
19-
ResourceUser RBACResource = "user"
20-
ResourceUserData RBACResource = "user_data"
21-
ResourceOrganizationMember RBACResource = "organization_member"
22-
ResourceLicense RBACResource = "license"
23-
ResourceDeploymentValues RBACResource = "deployment_config"
24-
ResourceDeploymentStats RBACResource = "deployment_stats"
25-
ResourceReplicas RBACResource = "replicas"
26-
ResourceDebugInfo RBACResource = "debug_info"
27-
ResourceSystem RBACResource = "system"
28-
ResourceTemplateInsights RBACResource = "template_insights"
6+
ResourceWorkspace RBACResource = "workspace"
7+
ResourceWorkspaceProxy RBACResource = "workspace_proxy"
8+
ResourceWorkspaceExecution RBACResource = "workspace_execution"
9+
ResourceWorkspaceApplicationConnect RBACResource = "application_connect"
10+
ResourceAuditLog RBACResource = "audit_log"
11+
ResourceTemplate RBACResource = "template"
12+
ResourceGroup RBACResource = "group"
13+
ResourceFile RBACResource = "file"
14+
ResourceProvisionerDaemon RBACResource = "provisioner_daemon"
15+
ResourceOrganization RBACResource = "organization"
16+
ResourceRoleAssignment RBACResource = "assign_role"
17+
ResourceOrgRoleAssignment RBACResource = "assign_org_role"
18+
ResourceAPIKey RBACResource = "api_key"
19+
ResourceUser RBACResource = "user"
20+
ResourceUserData RBACResource = "user_data"
21+
ResourceUserWorkspaceBuildParameters RBACResource = "user_workspace_build_parameters"
22+
ResourceOrganizationMember RBACResource = "organization_member"
23+
ResourceLicense RBACResource = "license"
24+
ResourceDeploymentValues RBACResource = "deployment_config"
25+
ResourceDeploymentStats RBACResource = "deployment_stats"
26+
ResourceReplicas RBACResource = "replicas"
27+
ResourceDebugInfo RBACResource = "debug_info"
28+
ResourceSystem RBACResource = "system"
29+
ResourceTemplateInsights RBACResource = "template_insights"
2930
)
3031

3132
const (
@@ -52,6 +53,7 @@ var (
5253
ResourceAPIKey,
5354
ResourceUser,
5455
ResourceUserData,
56+
ResourceUserWorkspaceBuildParameters,
5557
ResourceOrganizationMember,
5658
ResourceLicense,
5759
ResourceDeploymentValues,

‎codersdk/users.go

+1-1
Original file line numberDiff line numberDiff line change
@@ -227,7 +227,7 @@ type UserParameter struct {
227227
}
228228

229229
// UserAutofillParameters returns all recently used parameters for the given user.
230-
func (c *Client) UserAutofillParameters(ctx context.Context, user string, templateID string) ([]UserParameter, error) {
230+
func (c *Client) UserAutofillParameters(ctx context.Context, user string, templateID uuid.UUID) ([]UserParameter, error) {
231231
res, err := c.Request(ctx, http.MethodGet, fmt.Sprintf("/api/v2/users/%s/autofill-parameters?template_id=%s", user, templateID), nil)
232232
if err != nil {
233233
return nil, err

0 commit comments

Comments
 (0)