Movatterモバイル変換

[0]ホーム
URL:

Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

 Sign up
Appearance settings

Commitdbdf49f

Browse files
committed
chore: fixup permission assertions and testing
1 parent110809e commitdbdf49f

File tree

7 files changed

+201
-114
lines changed

7 files changed

+201
-114
lines changed

‎coderd/coderd.go

Lines changed: 1 addition & 0 deletions
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

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -144,8 +144,8 @@ type AuthCalls []AuthCall
144144

145145
typeAuthCallstruct {
146146
rbac.AuthCall
147+
Errerror
147148

148-
errerror
149149
assertedbool
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

Lines changed: 40 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -110,61 +110,51 @@ type OrganizationMember struct {
110110
funcExtractOrganizationMemberParam(db database.Store)func(http.Handler) http.Handler {
111111
returnfunc(next http.Handler) http.Handler {
112112
returnhttp.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-
funcExtractOrganizationMemberContext(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-
returnOrganizationMember{},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+
ifhttpapi.Is404Error(err) {
132+
httpapi.ResourceNotFound(rw)
133+
return
134+
}
135+
iferr!=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-
ifhttpapi.Is404Error(err) {
146-
httpapi.ResourceNotFound(rw)
147-
returnOrganizationMember{},false
148-
}
149-
iferr!=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-
returnOrganizationMember{},false
155159
}
156-
returnOrganizationMember{
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

Lines changed: 24 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -31,13 +31,18 @@ func UserParam(r *http.Request) database.User {
3131
returnuser
3232
}
3333

34+
funcUserParamOptional(r*http.Request) (database.User,bool) {
35+
user,ok:=r.Context().Value(userParamContextKey{}).(database.User)
36+
returnuser,ok
37+
}
38+
3439
// ExtractUserParam extracts a user from an ID/username in the {user} URL
3540
// parameter.
3641
funcExtractUserParam(db database.Store)func(http.Handler) http.Handler {
3742
returnfunc(next http.Handler) http.Handler {
3843
returnhttp.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-
funcextractUserContext(ctx context.Context,db database.Store,rw http.ResponseWriter,r*http.Request) (user database.User,okbool) {
56+
// ExtractUserParamOptional does not fail if no user is present.
57+
funcExtractUserParamOptional(db database.Store)func(http.Handler) http.Handler {
58+
returnfunc(next http.Handler) http.Handler {
59+
returnhttp.HandlerFunc(func(rw http.ResponseWriter,r*http.Request) {
60+
ctx:=r.Context()
61+
62+
user,ok:=ExtractUserContext(ctx,db,&noopResponseWriter{},r)
63+
ifok {
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+
funcExtractUserContext(ctx context.Context,db database.Store,rw http.ResponseWriter,r*http.Request) (user database.User,okbool) {
5374
// userQuery is either a uuid, a username, or 'me'
5475
userQuery:=chi.URLParam(r,"user")
5576
ifuserQuery=="" {

‎coderd/workspaces.go

Lines changed: 57 additions & 20 deletions
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 requirestheorganization. Which can be
418-
//sourced from the template.
416+
varownerworkspaceOwner
417+
//This user fetch is an optimization path forthemost 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+
ifok {
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+
ifhttpapi.Is404Error(err) {
459+
httpapi.ResourceNotFound(rw)
460+
return
461+
}
462+
iferr!=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
defercommitAudit()
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

Lines changed: 0 additions & 36 deletions
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-
defercancel()
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:=rangeauthz.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
funcTestResolveAutostart(t*testing.T) {

0 commit comments

Comments
 (0)
[8]ページ先頭
©2009-2025 Movatter.jp