Skip to content

Commit eea8dc6

Browse files
EmyrkEmyrk
authored
feat: Add rbac to templateversion+orgmember endpoints (#1713)
1 parent f8410de commit eea8dc6

17 files changed

+302
-66
lines changed

‎coderd/coderd.go

+3
Original file line numberDiff line numberDiff line change
@@ -170,6 +170,7 @@ func newRouter(options *Options, a *api) chi.Router {
170170
r.Route("/{user}", func(r chi.Router) {
171171
r.Use(
172172
httpmw.ExtractUserParam(options.Database),
173+
httpmw.ExtractOrganizationMemberParam(options.Database),
173174
)
174175
r.Put("/roles", a.putMemberRoles)
175176
})
@@ -201,6 +202,7 @@ func newRouter(options *Options, a *api) chi.Router {
201202
r.Route("/templateversions/{templateversion}", func(r chi.Router) {
202203
r.Use(
203204
apiKeyMiddleware,
205+
authRolesMiddleware,
204206
httpmw.ExtractTemplateVersionParam(options.Database),
205207
)
206208

@@ -289,6 +291,7 @@ func newRouter(options *Options, a *api) chi.Router {
289291
r.Route("/workspaceresources/{workspaceresource}", func(r chi.Router) {
290292
r.Use(
291293
apiKeyMiddleware,
294+
authRolesMiddleware,
292295
httpmw.ExtractWorkspaceResourceParam(options.Database),
293296
httpmw.ExtractWorkspaceParam(options.Database),
294297
)

‎coderd/coderd_test.go

+74-21
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,8 @@ import (
1717
"github.com/coder/coder/coderd/coderdtest"
1818
"github.com/coder/coder/coderd/rbac"
1919
"github.com/coder/coder/codersdk"
20+
"github.com/coder/coder/provisioner/echo"
21+
"github.com/coder/coder/provisionersdk/proto"
2022
)
2123

2224
func TestMain(m *testing.M) {
@@ -47,13 +49,32 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
4749
require.NoError(t, err, "fetch org")
4850

4951
// Setup some data in the database.
50-
version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil)
52+
version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, &echo.Responses{
53+
Parse: echo.ParseComplete,
54+
Provision: []*proto.Provision_Response{{
55+
Type: &proto.Provision_Response_Complete{
56+
Complete: &proto.Provision_Complete{
57+
// Return a workspace resource
58+
Resources: []*proto.Resource{{
59+
Name: "some",
60+
Type: "example",
61+
Agents: []*proto.Agent{{
62+
Id: "something",
63+
Auth: &proto.Agent_Token{},
64+
}},
65+
}},
66+
},
67+
},
68+
}},
69+
})
5170
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
5271
template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID)
5372
workspace := coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID)
5473
coderdtest.AwaitWorkspaceBuildJob(t, client, workspace.LatestBuild.ID)
5574
file, err := client.Upload(ctx, codersdk.ContentTypeTar, make([]byte, 1024))
5675
require.NoError(t, err, "upload file")
76+
workspaceResources, err := client.WorkspaceResourcesByBuild(ctx, workspace.LatestBuild.ID)
77+
require.NoError(t, err, "workspace resources")
5778

5879
// Always fail auth from this point forward
5980
authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil)
@@ -78,6 +99,9 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
7899
"POST:/api/v2/users/logout": {NoAuthorize: true},
79100
"GET:/api/v2/users/authmethods": {NoAuthorize: true},
80101

102+
// Has it's own auth
103+
"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
104+
81105
// All workspaceagents endpoints do not use rbac
82106
"POST:/api/v2/workspaceagents/aws-instance-identity": {NoAuthorize: true},
83107
"POST:/api/v2/workspaceagents/azure-instance-identity": {NoAuthorize: true},
@@ -94,11 +118,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
94118
"GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true},
95119

96120
// TODO: @emyrk these need to be fixed by adding authorize calls
97-
"GET:/api/v2/workspaceresources/{workspaceresource}": {NoAuthorize: true},
98-
99-
"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
100-
101-
"PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true},
102121
"GET:/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true},
103122
"GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true},
104123
"POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true},
@@ -108,17 +127,6 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
108127
"GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true},
109128
"DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true},
110129

111-
"GET:/api/v2/templates/{template}/versions": {NoAuthorize: true},
112-
"PATCH:/api/v2/templates/{template}/versions": {NoAuthorize: true},
113-
"GET:/api/v2/templates/{template}/versions/{templateversionname}": {NoAuthorize: true},
114-
115-
"GET:/api/v2/templateversions/{templateversion}": {NoAuthorize: true},
116-
"PATCH:/api/v2/templateversions/{templateversion}/cancel": {NoAuthorize: true},
117-
"GET:/api/v2/templateversions/{templateversion}/logs": {NoAuthorize: true},
118-
"GET:/api/v2/templateversions/{templateversion}/parameters": {NoAuthorize: true},
119-
"GET:/api/v2/templateversions/{templateversion}/resources": {NoAuthorize: true},
120-
"GET:/api/v2/templateversions/{templateversion}/schema": {NoAuthorize: true},
121-
122130
"POST:/api/v2/users/{user}/organizations": {NoAuthorize: true},
123131

124132
"GET:/api/v2/workspaces/{workspace}/watch": {NoAuthorize: true},
@@ -164,6 +172,10 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
164172
AssertAction: rbac.ActionUpdate,
165173
AssertObject: workspaceRBACObj,
166174
},
175+
"GET:/api/v2/workspaceresources/{workspaceresource}": {
176+
AssertAction: rbac.ActionRead,
177+
AssertObject: workspaceRBACObj,
178+
},
167179
"PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel": {
168180
AssertAction: rbac.ActionUpdate,
169181
AssertObject: workspaceRBACObj,
@@ -199,12 +211,51 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
199211
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
200212
},
201213
"POST:/api/v2/files": {AssertAction: rbac.ActionCreate, AssertObject: rbac.ResourceFile},
202-
"GET:/api/v2/files/{fileHash}": {AssertAction: rbac.ActionRead,
203-
AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash)},
214+
"GET:/api/v2/files/{fileHash}": {
215+
AssertAction: rbac.ActionRead,
216+
AssertObject: rbac.ResourceFile.WithOwner(admin.UserID.String()).WithID(file.Hash),
217+
},
218+
"GET:/api/v2/templates/{template}/versions": {
219+
AssertAction: rbac.ActionRead,
220+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
221+
},
222+
"PATCH:/api/v2/templates/{template}/versions": {
223+
AssertAction: rbac.ActionUpdate,
224+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
225+
},
226+
"GET:/api/v2/templates/{template}/versions/{templateversionname}": {
227+
AssertAction: rbac.ActionRead,
228+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
229+
},
230+
"GET:/api/v2/templateversions/{templateversion}": {
231+
AssertAction: rbac.ActionRead,
232+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
233+
},
234+
"PATCH:/api/v2/templateversions/{templateversion}/cancel": {
235+
AssertAction: rbac.ActionUpdate,
236+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
237+
},
238+
"GET:/api/v2/templateversions/{templateversion}/logs": {
239+
AssertAction: rbac.ActionRead,
240+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
241+
},
242+
"GET:/api/v2/templateversions/{templateversion}/parameters": {
243+
AssertAction: rbac.ActionRead,
244+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
245+
},
246+
"GET:/api/v2/templateversions/{templateversion}/resources": {
247+
AssertAction: rbac.ActionRead,
248+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
249+
},
250+
"GET:/api/v2/templateversions/{templateversion}/schema": {
251+
AssertAction: rbac.ActionRead,
252+
AssertObject: rbac.ResourceTemplate.InOrg(template.OrganizationID).WithID(template.ID.String()),
253+
},
204254

205255
// These endpoints need payloads to get to the auth part. Payloads will be required
206-
"PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
207-
"POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
256+
"PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
257+
"PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true},
258+
"POST:/api/v2/workspaces/{workspace}/builds": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
208259
}
209260

210261
for k, v := range assertRoute {
@@ -240,6 +291,8 @@ func TestAuthorizeAllEndpoints(t *testing.T) {
240291
route = strings.ReplaceAll(route, "{workspacebuildname}", workspace.LatestBuild.Name)
241292
route = strings.ReplaceAll(route, "{template}", template.ID.String())
242293
route = strings.ReplaceAll(route, "{hash}", file.Hash)
294+
route = strings.ReplaceAll(route, "{workspaceresource}", workspaceResources[0].ID.String())
295+
route = strings.ReplaceAll(route, "{templateversion}", version.ID.String())
243296

244297
resp, err := client.Request(context.Background(), method, route, nil)
245298
require.NoError(t, err, "do req")

‎coderd/coderdtest/coderdtest.go

+1-2
Original file line numberDiff line numberDiff line change
@@ -276,8 +276,7 @@ func CreateAnotherUser(t *testing.T, client *codersdk.Client, organizationID uui
276276
for orgID, roles := range orgRoles {
277277
organizationID, err := uuid.Parse(orgID)
278278
require.NoError(t, err, fmt.Sprintf("parse org id %q", orgID))
279-
// TODO: @Emyrk add the member to the organization if they do not already belong.
280-
_, err = other.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
279+
_, err = client.UpdateOrganizationMemberRoles(context.Background(), organizationID, user.ID.String(),
281280
codersdk.UpdateRoles{Roles: append(roles, rbac.RoleOrgMember(organizationID))})
282281
require.NoError(t, err, "update org membership roles")
283282
}

‎coderd/database/modelmethods.go

+5
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,11 @@ func (t Template) RBACObject() rbac.Object {
66
return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.ID.String())
77
}
88

9+
func (t TemplateVersion) RBACObject() rbac.Object {
10+
// Just use the parent template resource for controlling versions
11+
return rbac.ResourceTemplate.InOrg(t.OrganizationID).WithID(t.TemplateID.UUID.String())
12+
}
13+
914
func (w Workspace) RBACObject() rbac.Object {
1015
return rbac.ResourceWorkspace.InOrg(w.OrganizationID).WithID(w.ID.String()).WithOwner(w.OwnerID.String())
1116
}

‎coderd/httpmw/organizationparam.go

+17-6
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,12 @@ func OrganizationParam(r *http.Request) database.Organization {
2828
func OrganizationMemberParam(r *http.Request) database.OrganizationMember {
2929
organizationMember, ok := r.Context().Value(organizationMemberParamContextKey{}).(database.OrganizationMember)
3030
if !ok {
31-
panic("developer error: organization param middleware not provided")
31+
panic("developer error: organization member param middleware not provided")
3232
}
3333
return organizationMember
3434
}
3535

36-
// ExtractOrganizationParam grabs an organization and user membership from the "organization" URL parameter.
36+
// ExtractOrganizationParam grabs an organization from the "organization" URL parameter.
3737
// This middleware requires the API key middleware higher in the call stack for authentication.
3838
func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler {
3939
return func(next http.Handler) http.Handler {
@@ -56,11 +56,23 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
5656
})
5757
return
5858
}
59+
ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization)
60+
next.ServeHTTP(rw, r.WithContext(ctx))
61+
})
62+
}
63+
}
64+
65+
// ExtractOrganizationMemberParam grabs a user membership from the "organization" and "user" URL parameter.
66+
// This middleware requires the ExtractUser and ExtractOrganization middleware higher in the stack
67+
func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.Handler {
68+
return func(next http.Handler) http.Handler {
69+
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
70+
organization := OrganizationParam(r)
71+
user := UserParam(r)
5972

60-
apiKey := APIKey(r)
6173
organizationMember, err := db.GetOrganizationMemberByUserID(r.Context(), database.GetOrganizationMemberByUserIDParams{
6274
OrganizationID: organization.ID,
63-
UserID: apiKey.UserID,
75+
UserID: user.ID,
6476
})
6577
if errors.Is(err, sql.ErrNoRows) {
6678
httpapi.Write(rw, http.StatusForbidden, httpapi.Response{
@@ -74,9 +86,8 @@ func ExtractOrganizationParam(db database.Store) func(http.Handler) http.Handler
7486
})
7587
return
7688
}
89+
ctx := context.WithValue(r.Context(), organizationMemberParamContextKey{}, organizationMember)
7790

78-
ctx := context.WithValue(r.Context(), organizationParamContextKey{}, organization)
79-
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember)
8091
next.ServeHTTP(rw, r.WithContext(ctx))
8192
})
8293
}

‎coderd/httpmw/organizationparam_test.go

+7-1
Original file line numberDiff line numberDiff line change
@@ -122,7 +122,7 @@ func TestOrganizationParam(t *testing.T) {
122122
var (
123123
db = databasefake.New()
124124
rw = httptest.NewRecorder()
125-
r, _ = setupAuthentication(db)
125+
r, u = setupAuthentication(db)
126126
rtr = chi.NewRouter()
127127
)
128128
organization, err := db.InsertOrganization(r.Context(), database.InsertOrganizationParams{
@@ -133,9 +133,12 @@ func TestOrganizationParam(t *testing.T) {
133133
})
134134
require.NoError(t, err)
135135
chi.RouteContext(r.Context()).URLParams.Add("organization", organization.ID.String())
136+
chi.RouteContext(r.Context()).URLParams.Add("user", u.ID.String())
136137
rtr.Use(
137138
httpmw.ExtractAPIKey(db, nil),
139+
httpmw.ExtractUserParam(db),
138140
httpmw.ExtractOrganizationParam(db),
141+
httpmw.ExtractOrganizationMemberParam(db),
139142
)
140143
rtr.Get("/", nil)
141144
rtr.ServeHTTP(rw, r)
@@ -167,9 +170,12 @@ func TestOrganizationParam(t *testing.T) {
167170
})
168171
require.NoError(t, err)
169172
chi.RouteContext(r.Context()).URLParams.Add("organization", organization.ID.String())
173+
chi.RouteContext(r.Context()).URLParams.Add("user", user.ID.String())
170174
rtr.Use(
171175
httpmw.ExtractAPIKey(db, nil),
172176
httpmw.ExtractOrganizationParam(db),
177+
httpmw.ExtractUserParam(db),
178+
httpmw.ExtractOrganizationMemberParam(db),
173179
)
174180
rtr.Get("/", func(rw http.ResponseWriter, r *http.Request) {
175181
_ = httpmw.OrganizationParam(r)

‎coderd/members.go

+15-13
Original file line numberDiff line numberDiff line change
@@ -17,27 +17,29 @@ import (
1717
)
1818

1919
func (api *api) putMemberRoles(rw http.ResponseWriter, r *http.Request) {
20-
// User is the user to modify
21-
// TODO: Until rbac authorize is implemented, only be able to change your
22-
// own roles. This also means you can grant yourself whatever roles you want.
2320
user := httpmw.UserParam(r)
24-
apiKey := httpmw.APIKey(r)
2521
organization := httpmw.OrganizationParam(r)
26-
// TODO: @emyrk add proper `Authorize()` check here instead of a uuid match.
27-
// Proper authorize should check the granted roles are able to given within
28-
// the selected organization. Until then, allow anarchy
29-
if apiKey.UserID != user.ID {
30-
httpapi.Write(rw, http.StatusUnauthorized, httpapi.Response{
31-
Message: "modifying other users is not supported at this time",
32-
})
33-
return
34-
}
22+
member := httpmw.OrganizationMemberParam(r)
3523

3624
var params codersdk.UpdateRoles
3725
if !httpapi.Read(rw, r, &params) {
3826
return
3927
}
4028

29+
added, removed := rbac.ChangeRoleSet(member.Roles, params.Roles)
30+
for _, roleName := range added {
31+
// Assigning a role requires the create permission.
32+
if !api.Authorize(rw, r, rbac.ActionCreate, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
33+
return
34+
}
35+
}
36+
for _, roleName := range removed {
37+
// Removing a role requires the delete permission.
38+
if !api.Authorize(rw, r, rbac.ActionDelete, rbac.ResourceOrgRoleAssignment.WithID(roleName).InOrg(organization.ID)) {
39+
return
40+
}
41+
}
42+
4143
updatedUser, err := api.updateOrganizationMemberRoles(r.Context(), database.UpdateMemberRolesParams{
4244
GrantedRoles: params.Roles,
4345
UserID: user.ID,

‎coderd/rbac/authz.go

+1
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ package rbac
33
import (
44
"context"
55
_ "embed"
6+
67
"golang.org/x/xerrors"
78

89
"github.com/open-policy-agent/opa/rego"

‎coderd/rbac/builtin.go

+37
Original file line numberDiff line numberDiff line change
@@ -135,6 +135,12 @@ var (
135135
Action: ActionRead,
136136
ResourceID: "*",
137137
},
138+
{
139+
// Can read available roles.
140+
ResourceType: ResourceOrgRoleAssignment.Type,
141+
ResourceID: "*",
142+
Action: ActionRead,
143+
},
138144
},
139145
},
140146
}
@@ -217,6 +223,37 @@ func SiteRoles() []Role {
217223
return roles
218224
}
219225

226+
// ChangeRoleSet is a helper function that finds the difference of 2 sets of
227+
// roles. When setting a user's new roles, it is equivalent to adding and
228+
// removing roles. This set determines the changes, so that the appropriate
229+
// RBAC checks can be applied using "ActionCreate" and "ActionDelete" for
230+
// "added" and "removed" roles respectively.
231+
func ChangeRoleSet(from []string, to []string) (added []string, removed []string) {
232+
has := make(map[string]struct{})
233+
for _, exists := range from {
234+
has[exists] = struct{}{}
235+
}
236+
237+
for _, roleName := range to {
238+
// If the user already has the role assigned, we don't need to check the permission
239+
// to reassign it. Only run permission checks on the difference in the set of
240+
// roles.
241+
if _, ok := has[roleName]; ok {
242+
delete(has, roleName)
243+
continue
244+
}
245+
246+
added = append(added, roleName)
247+
}
248+
249+
// Remaining roles are the ones removed/deleted.
250+
for roleName := range has {
251+
removed = append(removed, roleName)
252+
}
253+
254+
return added, removed
255+
}
256+
220257
// roleName is a quick helper function to return
221258
// role_name:scopeID
222259
// If no scopeID is required, only 'role_name' is returned

0 commit comments

Comments
 (0)