Skip to content

Commit dbdf49f

Browse files
EmyrkEmyrk
committed
chore: fixup permission assertions and testing
1 parent 110809e commit dbdf49f

File tree

7 files changed

+201
-114
lines changed

7 files changed

+201
-114
lines changed

‎coderd/coderd.go

+1
Original file line numberDiff line numberDiff line change
@@ -1148,6 +1148,7 @@ func New(options *Options) *API {
11481148
})
11491149
r.Route("/{user}", func(r chi.Router) {
11501150
r.Group(func(r chi.Router) {
1151+
r.Use(httpmw.ExtractUserParamOptional(options.Database))
11511152
// Creating workspaces does not require permissions on the user, only the
11521153
// organization member. This endpoint should match the authz story of
11531154
// postWorkspacesByOrganization

‎coderd/coderdtest/authorize.go

+2-2
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,8 @@ type AuthCalls []AuthCall
144144

145145
type AuthCall struct {
146146
rbac.AuthCall
147+
Err error
147148

148-
err error
149149
asserted bool
150150
// callers is a small stack trace for debugging.
151151
callers []string
@@ -265,7 +265,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic
265265
Action: action,
266266
Object: object,
267267
},
268-
err: err,
268+
Err: err,
269269
callers: []string{
270270
// This is a decent stack trace for debugging.
271271
// Some dbauthz calls are a bit nested, so we skip a few.

‎coderd/httpmw/organizationparam.go

+40-50
Original file line numberDiff line numberDiff line change
@@ -110,61 +110,51 @@ type OrganizationMember struct {
110110
func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.Handler {
111111
return func(next http.Handler) http.Handler {
112112
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
113-
organization := OrganizationParam(r)
114-
organizationMember, ok := ExtractOrganizationMemberContext(rw, r, db, organization.ID)
113+
ctx := r.Context()
114+
// We need to resolve the `{user}` URL parameter so that we can get the userID and
115+
// username. We do this as SystemRestricted since the caller might have permission
116+
// to access the OrganizationMember object, but *not* the User object. So, it is
117+
// very important that we do not add the User object to the request context or otherwise
118+
// leak it to the API handler.
119+
// nolint:gocritic
120+
user, ok := ExtractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
115121
if !ok {
116122
return
117123
}
124+
organization := OrganizationParam(r)
118125

119-
ctx := r.Context()
120-
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember)
121-
next.ServeHTTP(rw, r.WithContext(ctx))
122-
})
123-
}
124-
}
125-
126-
func ExtractOrganizationMemberContext(rw http.ResponseWriter, r *http.Request, db database.Store, orgID uuid.UUID) (OrganizationMember, bool) {
127-
ctx := r.Context()
128-
129-
// We need to resolve the `{user}` URL parameter so that we can get the userID and
130-
// username. We do this as SystemRestricted since the caller might have permission
131-
// to access the OrganizationMember object, but *not* the User object. So, it is
132-
// very important that we do not add the User object to the request context or otherwise
133-
// leak it to the API handler.
134-
// nolint:gocritic
135-
user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
136-
if !ok {
137-
return OrganizationMember{}, false
138-
}
126+
organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{
127+
OrganizationID: organization.ID,
128+
UserID: user.ID,
129+
IncludeSystem: false,
130+
}))
131+
if httpapi.Is404Error(err) {
132+
httpapi.ResourceNotFound(rw)
133+
return
134+
}
135+
if err != nil {
136+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
137+
Message: "Internal error fetching organization member.",
138+
Detail: err.Error(),
139+
})
140+
return
141+
}
139142

140-
organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{
141-
OrganizationID: orgID,
142-
UserID: user.ID,
143-
IncludeSystem: false,
144-
}))
145-
if httpapi.Is404Error(err) {
146-
httpapi.ResourceNotFound(rw)
147-
return OrganizationMember{}, false
148-
}
149-
if err != nil {
150-
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
151-
Message: "Internal error fetching organization member.",
152-
Detail: err.Error(),
143+
ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{
144+
OrganizationMember: organizationMember.OrganizationMember,
145+
// Here we're making two exceptions to the rule about not leaking data about the user
146+
// to the API handler, which is to include the username and avatar URL.
147+
// If the caller has permission to read the OrganizationMember, then we're explicitly
148+
// saying here that they also have permission to see the member's username and avatar.
149+
// This is OK!
150+
//
151+
// API handlers need this information for audit logging and returning the owner's
152+
// username in response to creating a workspace. Additionally, the frontend consumes
153+
// the Avatar URL and this allows the FE to avoid an extra request.
154+
Username: user.Username,
155+
AvatarURL: user.AvatarURL,
156+
})
157+
next.ServeHTTP(rw, r.WithContext(ctx))
153158
})
154-
return OrganizationMember{}, false
155159
}
156-
return OrganizationMember{
157-
OrganizationMember: organizationMember.OrganizationMember,
158-
// Here we're making two exceptions to the rule about not leaking data about the user
159-
// to the API handler, which is to include the username and avatar URL.
160-
// If the caller has permission to read the OrganizationMember, then we're explicitly
161-
// saying here that they also have permission to see the member's username and avatar.
162-
// This is OK!
163-
//
164-
// API handlers need this information for audit logging and returning the owner's
165-
// username in response to creating a workspace. Additionally, the frontend consumes
166-
// the Avatar URL and this allows the FE to avoid an extra request.
167-
Username: user.Username,
168-
AvatarURL: user.AvatarURL,
169-
}, true
170160
}

‎coderd/httpmw/userparam.go

+24-3
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,18 @@ func UserParam(r *http.Request) database.User {
3131
return user
3232
}
3333

34+
func UserParamOptional(r *http.Request) (database.User, bool) {
35+
user, ok := r.Context().Value(userParamContextKey{}).(database.User)
36+
return user, ok
37+
}
38+
3439
// ExtractUserParam extracts a user from an ID/username in the {user} URL
3540
// parameter.
3641
func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
3742
return func(next http.Handler) http.Handler {
3843
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
3944
ctx := r.Context()
40-
user, ok := extractUserContext(ctx, db, rw, r)
45+
user, ok := ExtractUserContext(ctx, db, rw, r)
4146
if !ok {
4247
// response already handled
4348
return
@@ -48,8 +53,24 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
4853
}
4954
}
5055

51-
// extractUserContext queries the database for the parameterized `{user}` from the request URL.
52-
func extractUserContext(ctx context.Context, db database.Store, rw http.ResponseWriter, r *http.Request) (user database.User, ok bool) {
56+
// ExtractUserParamOptional does not fail if no user is present.
57+
func ExtractUserParamOptional(db database.Store) func(http.Handler) http.Handler {
58+
return func(next http.Handler) http.Handler {
59+
return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
60+
ctx := r.Context()
61+
62+
user, ok := ExtractUserContext(ctx, db, &noopResponseWriter{}, r)
63+
if ok {
64+
ctx = context.WithValue(ctx, userParamContextKey{}, user)
65+
}
66+
67+
next.ServeHTTP(rw, r.WithContext(ctx))
68+
})
69+
}
70+
}
71+
72+
// ExtractUserContext queries the database for the parameterized `{user}` from the request URL.
73+
func ExtractUserContext(ctx context.Context, db database.Store, rw http.ResponseWriter, r *http.Request) (user database.User, ok bool) {
5374
// userQuery is either a uuid, a username, or 'me'
5475
userQuery := chi.URLParam(r, "user")
5576
if userQuery == "" {

‎coderd/workspaces.go

+57-20
Original file line numberDiff line numberDiff line change
@@ -413,21 +413,64 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) {
413413
return
414414
}
415415

416-
// No middleware exists to fetch the user from. This endpoint needs to fetch
417-
// the organization member, which requires the organization. Which can be
418-
// sourced from the template.
416+
var owner workspaceOwner
417+
// This user fetch is an optimization path for the most common case of creating a
418+
// workspace for 'Me'.
419419
//
420-
// TODO: This code gets called twice for each workspace build request.
421-
// This is inefficient and costs at most 2 extra RTTs to the DB.
422-
// This can be optimized. It exists as it is now for code simplicity.
423-
template, ok := requestTemplate(ctx, rw, req, api.Database)
424-
if !ok {
425-
return
426-
}
420+
// This is also required to allow `owners` to create workspaces for users
421+
// that are not in an organization.
422+
user, ok := httpmw.UserParamOptional(r)
423+
if ok {
424+
owner = workspaceOwner{
425+
ID: user.ID,
426+
Username: user.Username,
427+
AvatarURL: user.AvatarURL,
428+
}
429+
} else {
430+
// A workspace can still be created if the caller can read the organization
431+
// member. The organization is required, which can be sourced from the
432+
// template.
433+
//
434+
// TODO: This code gets called twice for each workspace build request.
435+
// This is inefficient and costs at most 2 extra RTTs to the DB.
436+
// This can be optimized. It exists as it is now for code simplicity.
437+
// The most common case is to create a workspace for 'Me'. Which does
438+
// not enter this code branch.
439+
template, ok := requestTemplate(ctx, rw, req, api.Database)
440+
if !ok {
441+
return
442+
}
427443

428-
member, ok := httpmw.ExtractOrganizationMemberContext(rw, r, api.Database, template.OrganizationID)
429-
if !ok {
430-
return
444+
// We need to fetch the original user as a system user to fetch the
445+
// user_id. 'ExtractUserContext' handles all cases like usernames,
446+
// 'Me', etc.
447+
// nolint:gocritic // The user_id needs to be fetched. This handles all those cases.
448+
user, ok := httpmw.ExtractUserContext(dbauthz.AsSystemRestricted(ctx), api.Database, rw, r)
449+
if !ok {
450+
return
451+
}
452+
453+
organizationMember, err := database.ExpectOne(api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{
454+
OrganizationID: template.OrganizationID,
455+
UserID: user.ID,
456+
IncludeSystem: false,
457+
}))
458+
if httpapi.Is404Error(err) {
459+
httpapi.ResourceNotFound(rw)
460+
return
461+
}
462+
if err != nil {
463+
httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{
464+
Message: "Internal error fetching organization member.",
465+
Detail: err.Error(),
466+
})
467+
return
468+
}
469+
owner = workspaceOwner{
470+
ID: organizationMember.OrganizationMember.UserID,
471+
Username: organizationMember.Username,
472+
AvatarURL: organizationMember.AvatarURL,
473+
}
431474
}
432475

433476
aReq, commitAudit := audit.InitRequest[database.WorkspaceTable](rw, &audit.RequestParams{
@@ -436,17 +479,11 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) {
436479
Request: r,
437480
Action: database.AuditActionCreate,
438481
AdditionalFields: audit.AdditionalFields{
439-
WorkspaceOwner: member.Username,
482+
WorkspaceOwner: owner.Username,
440483
},
441484
})
442485

443486
defer commitAudit()
444-
445-
owner := workspaceOwner{
446-
ID: member.UserID,
447-
Username: member.Username,
448-
AvatarURL: member.AvatarURL,
449-
}
450487
createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r)
451488
}
452489

‎coderd/workspaces_test.go

-36
Original file line numberDiff line numberDiff line change
@@ -423,42 +423,6 @@ func TestWorkspace(t *testing.T) {
423423
require.ErrorAs(t, err, &apiError)
424424
require.Equal(t, http.StatusForbidden, apiError.StatusCode())
425425
})
426-
427-
// Asserting some authz calls when creating a workspace.
428-
t.Run("AuthzStory", func(t *testing.T) {
429-
t.Parallel()
430-
owner, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
431-
first := coderdtest.CreateFirstUser(t, owner)
432-
authz := coderdtest.AssertRBAC(t, api, owner)
433-
434-
version := coderdtest.CreateTemplateVersion(t, owner, first.OrganizationID, nil)
435-
coderdtest.AwaitTemplateVersionJobCompleted(t, owner, version.ID)
436-
template := coderdtest.CreateTemplate(t, owner, first.OrganizationID, version.ID)
437-
_, userID := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID)
438-
439-
ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
440-
defer cancel()
441-
442-
// Create a workspace with the current api.
443-
authz.Reset() // Reset all previous checks done in setup.
444-
_, err := owner.CreateUserWorkspace(ctx, userID.ID.String(), codersdk.CreateWorkspaceRequest{
445-
TemplateID: template.ID,
446-
Name: "test-user",
447-
})
448-
require.NoError(t, err)
449-
450-
// Assert all authz properties
451-
t.Run("OnlyOrganizationAuthzCalls", func(t *testing.T) {
452-
// Creating workspaces is an organization action. So organization
453-
// permissions should be sufficient to complete the action.
454-
for _, call := range authz.AllCalls() {
455-
assert.Falsef(t, call.Object.OrgID == "",
456-
"call %q for object %q has no organization set. Site authz calls not expected here",
457-
call.Action, call.Object.String(),
458-
)
459-
}
460-
})
461-
})
462426
}
463427

464428
func TestResolveAutostart(t *testing.T) {

0 commit comments

Comments
 (0)