Skip to content

Commit 4ad5ac2

Browse files
EmyrkEmyrk
authored
feat: Rbac more coderd endpoints, unit test to confirm (#1437)
* feat: Enforce authorize call on all endpoints - Make 'request()' exported for running custom requests * Rbac users endpoints * 401 -> 403
1 parent 495c87b commit 4ad5ac2

40 files changed

+631
-319
lines changed

‎cli/autostart_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ func TestAutostart(t *testing.T) {
110110
clitest.SetupConfig(t, client, root)
111111

112112
err := cmd.Execute()
113-
require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
113+
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
114114
})
115115

116116
t.Run("Disable_NotFound", func(t *testing.T) {
@@ -128,7 +128,7 @@ func TestAutostart(t *testing.T) {
128128
clitest.SetupConfig(t, client, root)
129129

130130
err := cmd.Execute()
131-
require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
131+
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
132132
})
133133

134134
t.Run("Enable_DefaultSchedule", func(t *testing.T) {

‎cli/autostop_test.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -109,7 +109,7 @@ func TestAutostop(t *testing.T) {
109109
clitest.SetupConfig(t, client, root)
110110

111111
err := cmd.Execute()
112-
require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
112+
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
113113
})
114114

115115
t.Run("Disable_NotFound", func(t *testing.T) {
@@ -127,7 +127,7 @@ func TestAutostop(t *testing.T) {
127127
clitest.SetupConfig(t, client, root)
128128

129129
err := cmd.Execute()
130-
require.ErrorContains(t, err, "status code 404: no workspace found by name", "unexpected error")
130+
require.ErrorContains(t, err, "status code 403: forbidden", "unexpected error")
131131
})
132132

133133
t.Run("Enable_DefaultSchedule", func(t *testing.T) {

‎coderd/authorize.go

+43
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
package coderd
2+
3+
import (
4+
"net/http"
5+
6+
"golang.org/x/xerrors"
7+
8+
"cdr.dev/slog"
9+
10+
"github.com/coder/coder/coderd/httpapi"
11+
"github.com/coder/coder/coderd/httpmw"
12+
"github.com/coder/coder/coderd/rbac"
13+
)
14+
15+
func (api *api) Authorize(rw http.ResponseWriter, r *http.Request, action rbac.Action, object rbac.Object) bool {
16+
roles := httpmw.UserRoles(r)
17+
err := api.Authorizer.ByRoleName(r.Context(), roles.ID.String(), roles.Roles, action, object)
18+
if err != nil {
19+
httpapi.Write(rw, http.StatusForbidden, httpapi.Response{
20+
Message: err.Error(),
21+
})
22+
23+
// Log the errors for debugging
24+
internalError := new(rbac.UnauthorizedError)
25+
logger := api.Logger
26+
if xerrors.As(err, internalError) {
27+
logger = api.Logger.With(slog.F("internal", internalError.Internal()))
28+
}
29+
// Log information for debugging. This will be very helpful
30+
// in the early days
31+
logger.Warn(r.Context(), "unauthorized",
32+
slog.F("roles", roles.Roles),
33+
slog.F("user_id", roles.ID),
34+
slog.F("username", roles.Username),
35+
slog.F("route", r.URL.Path),
36+
slog.F("action", action),
37+
slog.F("object", object),
38+
)
39+
40+
return false
41+
}
42+
return true
43+
}

‎coderd/coderd.go

+5-13
Original file line numberDiff line numberDiff line change
@@ -50,7 +50,7 @@ type Options struct {
5050
SecureAuthCookie bool
5151
SSHKeygenAlgorithm gitsshkey.Algorithm
5252
TURNServer *turnconn.Server
53-
Authorizer *rbac.RegoAuthorizer
53+
Authorizer rbac.Authorizer
5454
}
5555

5656
// New constructs the Coder API into an HTTP handler.
@@ -83,10 +83,6 @@ func New(options *Options) (http.Handler, func()) {
8383
// TODO: @emyrk we should just move this into 'ExtractAPIKey'.
8484
authRolesMiddleware := httpmw.ExtractUserRoles(options.Database)
8585

86-
authorize := func(f http.HandlerFunc, actions rbac.Action) http.HandlerFunc {
87-
return httpmw.Authorize(api.Logger, api.Authorizer, actions)(f).ServeHTTP
88-
}
89-
9086
r := chi.NewRouter()
9187

9288
r.Use(
@@ -158,10 +154,7 @@ func New(options *Options) (http.Handler, func()) {
158154
})
159155
})
160156
r.Route("/members", func(r chi.Router) {
161-
r.Route("/roles", func(r chi.Router) {
162-
r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole))
163-
r.Get("/", authorize(api.assignableOrgRoles, rbac.ActionRead))
164-
})
157+
r.Get("/roles", api.assignableOrgRoles)
165158
r.Route("/{user}", func(r chi.Router) {
166159
r.Use(
167160
httpmw.ExtractUserParam(options.Database),
@@ -232,8 +225,7 @@ func New(options *Options) (http.Handler, func()) {
232225
r.Get("/", api.users)
233226
// These routes query information about site wide roles.
234227
r.Route("/roles", func(r chi.Router) {
235-
r.Use(httpmw.WithRBACObject(rbac.ResourceUserRole))
236-
r.Get("/", authorize(api.assignableSiteRoles, rbac.ActionRead))
228+
r.Get("/", api.assignableSiteRoles)
237229
})
238230
r.Route("/{user}", func(r chi.Router) {
239231
r.Use(httpmw.ExtractUserParam(options.Database))
@@ -244,8 +236,7 @@ func New(options *Options) (http.Handler, func()) {
244236
r.Put("/active", api.putUserStatus(database.UserStatusActive))
245237
})
246238
r.Route("/password", func(r chi.Router) {
247-
r.Use(httpmw.WithRBACObject(rbac.ResourceUserPasswordRole))
248-
r.Put("/", authorize(api.putUserPassword, rbac.ActionUpdate))
239+
r.Put("/", api.putUserPassword)
249240
})
250241
r.Get("/organizations", api.organizationsByUser)
251242
r.Post("/organizations", api.postOrganizationsByUser)
@@ -302,6 +293,7 @@ func New(options *Options) (http.Handler, func()) {
302293
r.Route("/workspaces/{workspace}", func(r chi.Router) {
303294
r.Use(
304295
apiKeyMiddleware,
296+
authRolesMiddleware,
305297
httpmw.ExtractWorkspaceParam(options.Database),
306298
)
307299
r.Get("/", api.workspace)

‎coderd/coderd_test.go

+201-2
Original file line numberDiff line numberDiff line change
@@ -2,14 +2,19 @@ package coderd_test
22

33
import (
44
"context"
5+
"net/http"
6+
"strings"
57
"testing"
68

7-
"go.uber.org/goleak"
8-
9+
"github.com/go-chi/chi/v5"
10+
"github.com/stretchr/testify/assert"
911
"github.com/stretchr/testify/require"
12+
"go.uber.org/goleak"
13+
"golang.org/x/xerrors"
1014

1115
"github.com/coder/coder/buildinfo"
1216
"github.com/coder/coder/coderd/coderdtest"
17+
"github.com/coder/coder/coderd/rbac"
1318
)
1419

1520
func TestMain(m *testing.M) {
@@ -24,3 +29,197 @@ func TestBuildInfo(t *testing.T) {
2429
require.Equal(t, buildinfo.ExternalURL(), buildInfo.ExternalURL, "external URL")
2530
require.Equal(t, buildinfo.Version(), buildInfo.Version, "version")
2631
}
32+
33+
// TestAuthorizeAllEndpoints will check `authorize` is called on every endpoint registered.
34+
func TestAuthorizeAllEndpoints(t *testing.T) {
35+
t.Parallel()
36+
37+
authorizer := &fakeAuthorizer{}
38+
srv, client := coderdtest.NewMemoryCoderd(t, &coderdtest.Options{
39+
Authorizer: authorizer,
40+
})
41+
admin := coderdtest.CreateFirstUser(t, client)
42+
organization, err := client.Organization(context.Background(), admin.OrganizationID)
43+
require.NoError(t, err, "fetch org")
44+
45+
// Setup some data in the database.
46+
coderdtest.NewProvisionerDaemon(t, client)
47+
version := coderdtest.CreateTemplateVersion(t, client, admin.OrganizationID, nil)
48+
coderdtest.AwaitTemplateVersionJob(t, client, version.ID)
49+
template := coderdtest.CreateTemplate(t, client, admin.OrganizationID, version.ID)
50+
workspace := coderdtest.CreateWorkspace(t, client, admin.OrganizationID, template.ID)
51+
52+
// Always fail auth from this point forward
53+
authorizer.AlwaysReturn = rbac.ForbiddenWithInternal(xerrors.New("fake implementation"), nil, nil)
54+
55+
// skipRoutes allows skipping routes from being checked.
56+
type routeCheck struct {
57+
NoAuthorize bool
58+
AssertObject rbac.Object
59+
StatusCode int
60+
}
61+
assertRoute := map[string]routeCheck{
62+
// These endpoints do not require auth
63+
"GET:/api/v2": {NoAuthorize: true},
64+
"GET:/api/v2/buildinfo": {NoAuthorize: true},
65+
"GET:/api/v2/users/first": {NoAuthorize: true},
66+
"POST:/api/v2/users/first": {NoAuthorize: true},
67+
"POST:/api/v2/users/login": {NoAuthorize: true},
68+
"POST:/api/v2/users/logout": {NoAuthorize: true},
69+
"GET:/api/v2/users/authmethods": {NoAuthorize: true},
70+
71+
// All workspaceagents endpoints do not use rbac
72+
"POST:/api/v2/workspaceagents/aws-instance-identity": {NoAuthorize: true},
73+
"POST:/api/v2/workspaceagents/azure-instance-identity": {NoAuthorize: true},
74+
"POST:/api/v2/workspaceagents/google-instance-identity": {NoAuthorize: true},
75+
"GET:/api/v2/workspaceagents/me/gitsshkey": {NoAuthorize: true},
76+
"GET:/api/v2/workspaceagents/me/iceservers": {NoAuthorize: true},
77+
"GET:/api/v2/workspaceagents/me/listen": {NoAuthorize: true},
78+
"GET:/api/v2/workspaceagents/me/metadata": {NoAuthorize: true},
79+
"GET:/api/v2/workspaceagents/me/turn": {NoAuthorize: true},
80+
"GET:/api/v2/workspaceagents/{workspaceagent}": {NoAuthorize: true},
81+
"GET:/api/v2/workspaceagents/{workspaceagent}/": {NoAuthorize: true},
82+
"GET:/api/v2/workspaceagents/{workspaceagent}/dial": {NoAuthorize: true},
83+
"GET:/api/v2/workspaceagents/{workspaceagent}/iceservers": {NoAuthorize: true},
84+
"GET:/api/v2/workspaceagents/{workspaceagent}/pty": {NoAuthorize: true},
85+
"GET:/api/v2/workspaceagents/{workspaceagent}/turn": {NoAuthorize: true},
86+
87+
// TODO: @emyrk these need to be fixed by adding authorize calls
88+
"GET:/api/v2/workspaceresources/{workspaceresource}": {NoAuthorize: true},
89+
"GET:/api/v2/workspacebuilds/{workspacebuild}": {NoAuthorize: true},
90+
"GET:/api/v2/workspacebuilds/{workspacebuild}/logs": {NoAuthorize: true},
91+
"GET:/api/v2/workspacebuilds/{workspacebuild}/resources": {NoAuthorize: true},
92+
"GET:/api/v2/workspacebuilds/{workspacebuild}/state": {NoAuthorize: true},
93+
"PATCH:/api/v2/workspacebuilds/{workspacebuild}/cancel": {NoAuthorize: true},
94+
"GET:/api/v2/workspaces/{workspace}/builds/{workspacebuildname}": {NoAuthorize: true},
95+
96+
"GET:/api/v2/users/oauth2/github/callback": {NoAuthorize: true},
97+
98+
"POST:/api/v2/users/{user}/organizations/": {NoAuthorize: true},
99+
"PUT:/api/v2/organizations/{organization}/members/{user}/roles": {NoAuthorize: true},
100+
"GET:/api/v2/organizations/{organization}/provisionerdaemons": {NoAuthorize: true},
101+
"POST:/api/v2/organizations/{organization}/templates": {NoAuthorize: true},
102+
"GET:/api/v2/organizations/{organization}/templates": {NoAuthorize: true},
103+
"GET:/api/v2/organizations/{organization}/templates/{templatename}": {NoAuthorize: true},
104+
"POST:/api/v2/organizations/{organization}/templateversions": {NoAuthorize: true},
105+
"POST:/api/v2/organizations/{organization}/workspaces": {NoAuthorize: true},
106+
107+
"POST:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true},
108+
"GET:/api/v2/parameters/{scope}/{id}": {NoAuthorize: true},
109+
"DELETE:/api/v2/parameters/{scope}/{id}/{name}": {NoAuthorize: true},
110+
111+
"GET:/api/v2/provisionerdaemons/me/listen": {NoAuthorize: true},
112+
113+
"DELETE:/api/v2/templates/{template}": {NoAuthorize: true},
114+
"GET:/api/v2/templates/{template}": {NoAuthorize: true},
115+
"GET:/api/v2/templates/{template}/versions": {NoAuthorize: true},
116+
"PATCH:/api/v2/templates/{template}/versions": {NoAuthorize: true},
117+
"GET:/api/v2/templates/{template}/versions/{templateversionname}": {NoAuthorize: true},
118+
119+
"GET:/api/v2/templateversions/{templateversion}": {NoAuthorize: true},
120+
"PATCH:/api/v2/templateversions/{templateversion}/cancel": {NoAuthorize: true},
121+
"GET:/api/v2/templateversions/{templateversion}/logs": {NoAuthorize: true},
122+
"GET:/api/v2/templateversions/{templateversion}/parameters": {NoAuthorize: true},
123+
"GET:/api/v2/templateversions/{templateversion}/resources": {NoAuthorize: true},
124+
"GET:/api/v2/templateversions/{templateversion}/schema": {NoAuthorize: true},
125+
126+
"POST:/api/v2/users/{user}/organizations": {NoAuthorize: true},
127+
128+
"GET:/api/v2/workspaces/{workspace}": {NoAuthorize: true},
129+
"PUT:/api/v2/workspaces/{workspace}/autostart": {NoAuthorize: true},
130+
"PUT:/api/v2/workspaces/{workspace}/autostop": {NoAuthorize: true},
131+
"GET:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true},
132+
"POST:/api/v2/workspaces/{workspace}/builds": {NoAuthorize: true},
133+
134+
"POST:/api/v2/files": {NoAuthorize: true},
135+
"GET:/api/v2/files/{hash}": {NoAuthorize: true},
136+
137+
// These endpoints have more assertions. This is good, add more endpoints to assert if you can!
138+
"GET:/api/v2/organizations/{organization}": {AssertObject: rbac.ResourceOrganization.InOrg(admin.OrganizationID)},
139+
"GET:/api/v2/users/{user}/organizations": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceOrganization},
140+
"GET:/api/v2/users/{user}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
141+
"GET:/api/v2/organizations/{organization}/workspaces/{user}": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
142+
"GET:/api/v2/organizations/{organization}/workspaces/{user}/{workspace}": {
143+
AssertObject: rbac.ResourceWorkspace.InOrg(organization.ID).WithID(workspace.ID.String()).WithOwner(workspace.OwnerID.String()),
144+
},
145+
"GET:/api/v2/organizations/{organization}/workspaces": {StatusCode: http.StatusOK, AssertObject: rbac.ResourceWorkspace},
146+
147+
// These endpoints need payloads to get to the auth part.
148+
"PUT:/api/v2/users/{user}/roles": {StatusCode: http.StatusBadRequest, NoAuthorize: true},
149+
}
150+
151+
c, _ := srv.Config.Handler.(*chi.Mux)
152+
err = chi.Walk(c, func(method string, route string, handler http.Handler, middlewares ...func(http.Handler) http.Handler) error {
153+
name := method + ":" + route
154+
t.Run(name, func(t *testing.T) {
155+
authorizer.reset()
156+
routeAssertions, ok := assertRoute[strings.TrimRight(name, "/")]
157+
if !ok {
158+
// By default, all omitted routes check for just "authorize" called
159+
routeAssertions = routeCheck{}
160+
}
161+
if routeAssertions.StatusCode == 0 {
162+
routeAssertions.StatusCode = http.StatusForbidden
163+
}
164+
165+
// Replace all url params with known values
166+
route = strings.ReplaceAll(route, "{organization}", admin.OrganizationID.String())
167+
route = strings.ReplaceAll(route, "{user}", admin.UserID.String())
168+
route = strings.ReplaceAll(route, "{organizationname}", organization.Name)
169+
route = strings.ReplaceAll(route, "{workspace}", workspace.Name)
170+
171+
resp, err := client.Request(context.Background(), method, route, nil)
172+
require.NoError(t, err, "do req")
173+
_ = resp.Body.Close()
174+
175+
if !routeAssertions.NoAuthorize {
176+
assert.NotNil(t, authorizer.Called, "authorizer expected")
177+
assert.Equal(t, routeAssertions.StatusCode, resp.StatusCode, "expect unauthorized")
178+
if authorizer.Called != nil {
179+
if routeAssertions.AssertObject.Type != "" {
180+
assert.Equal(t, routeAssertions.AssertObject.Type, authorizer.Called.Object.Type, "resource type")
181+
}
182+
if routeAssertions.AssertObject.Owner != "" {
183+
assert.Equal(t, routeAssertions.AssertObject.Owner, authorizer.Called.Object.Owner, "resource owner")
184+
}
185+
if routeAssertions.AssertObject.OrgID != "" {
186+
assert.Equal(t, routeAssertions.AssertObject.OrgID, authorizer.Called.Object.OrgID, "resource org")
187+
}
188+
if routeAssertions.AssertObject.ResourceID != "" {
189+
assert.Equal(t, routeAssertions.AssertObject.ResourceID, authorizer.Called.Object.ResourceID, "resource ID")
190+
}
191+
}
192+
} else {
193+
assert.Nil(t, authorizer.Called, "authorize not expected")
194+
}
195+
})
196+
return nil
197+
})
198+
require.NoError(t, err)
199+
}
200+
201+
type authCall struct {
202+
SubjectID string
203+
Roles []string
204+
Action rbac.Action
205+
Object rbac.Object
206+
}
207+
208+
type fakeAuthorizer struct {
209+
Called *authCall
210+
AlwaysReturn error
211+
}
212+
213+
func (f *fakeAuthorizer) ByRoleName(_ context.Context, subjectID string, roleNames []string, action rbac.Action, object rbac.Object) error {
214+
f.Called = &authCall{
215+
SubjectID: subjectID,
216+
Roles: roleNames,
217+
Action: action,
218+
Object: object,
219+
}
220+
return f.AlwaysReturn
221+
}
222+
223+
func (f *fakeAuthorizer) reset() {
224+
f.Called = nil
225+
}

0 commit comments

Comments
 (0)