Skip to content

Commit 0b58798

Browse files
EmyrkEmyrk
authored
feat: remove site wide perms from creating a workspace (#17296)
Creating a workspace required `read` on site wide `user`. Only organization permissions should be required.
1 parent a03a54d commit 0b58798

File tree

8 files changed

+393
-136
lines changed

8 files changed

+393
-136
lines changed

‎coderd/coderd.go‎

Lines changed: 63 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -1146,64 +1146,74 @@ func New(options *Options) *API {
11461146
r.Get("/", api.AssignableSiteRoles)
11471147
})
11481148
r.Route("/{user}", func(r chi.Router) {
1149-
r.Use(httpmw.ExtractUserParam(options.Database))
1150-
r.Post("/convert-login", api.postConvertLoginType)
1151-
r.Delete("/", api.deleteUser)
1152-
r.Get("/", api.userByName)
1153-
r.Get("/autofill-parameters", api.userAutofillParameters)
1154-
r.Get("/login-type", api.userLoginType)
1155-
r.Put("/profile", api.putUserProfile)
1156-
r.Route("/status", func(r chi.Router) {
1157-
r.Put("/suspend", api.putSuspendUserAccount())
1158-
r.Put("/activate", api.putActivateUserAccount())
1149+
r.Group(func(r chi.Router) {
1150+
r.Use(httpmw.ExtractUserParamOptional(options.Database))
1151+
// Creating workspaces does not require permissions on the user, only the
1152+
// organization member. This endpoint should match the authz story of
1153+
// postWorkspacesByOrganization
1154+
r.Post("/workspaces", api.postUserWorkspaces)
11591155
})
1160-
r.Get("/appearance", api.userAppearanceSettings)
1161-
r.Put("/appearance", api.putUserAppearanceSettings)
1162-
r.Route("/password", func(r chi.Router) {
1163-
r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
1164-
r.Put("/", api.putUserPassword)
1165-
})
1166-
// These roles apply to the site wide permissions.
1167-
r.Put("/roles", api.putUserRoles)
1168-
r.Get("/roles", api.userRoles)
1169-
1170-
r.Route("/keys", func(r chi.Router) {
1171-
r.Post("/", api.postAPIKey)
1172-
r.Route("/tokens", func(r chi.Router) {
1173-
r.Post("/", api.postToken)
1174-
r.Get("/", api.tokens)
1175-
r.Get("/tokenconfig", api.tokenConfig)
1176-
r.Route("/{keyname}", func(r chi.Router) {
1177-
r.Get("/", api.apiKeyByName)
1178-
})
1156+
1157+
r.Group(func(r chi.Router) {
1158+
r.Use(httpmw.ExtractUserParam(options.Database))
1159+
1160+
r.Post("/convert-login", api.postConvertLoginType)
1161+
r.Delete("/", api.deleteUser)
1162+
r.Get("/", api.userByName)
1163+
r.Get("/autofill-parameters", api.userAutofillParameters)
1164+
r.Get("/login-type", api.userLoginType)
1165+
r.Put("/profile", api.putUserProfile)
1166+
r.Route("/status", func(r chi.Router) {
1167+
r.Put("/suspend", api.putSuspendUserAccount())
1168+
r.Put("/activate", api.putActivateUserAccount())
11791169
})
1180-
r.Route("/{keyid}", func(r chi.Router) {
1181-
r.Get("/", api.apiKeyByID)
1182-
r.Delete("/", api.deleteAPIKey)
1170+
r.Get("/appearance", api.userAppearanceSettings)
1171+
r.Put("/appearance", api.putUserAppearanceSettings)
1172+
r.Route("/password", func(r chi.Router) {
1173+
r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
1174+
r.Put("/", api.putUserPassword)
1175+
})
1176+
// These roles apply to the site wide permissions.
1177+
r.Put("/roles", api.putUserRoles)
1178+
r.Get("/roles", api.userRoles)
1179+
1180+
r.Route("/keys", func(r chi.Router) {
1181+
r.Post("/", api.postAPIKey)
1182+
r.Route("/tokens", func(r chi.Router) {
1183+
r.Post("/", api.postToken)
1184+
r.Get("/", api.tokens)
1185+
r.Get("/tokenconfig", api.tokenConfig)
1186+
r.Route("/{keyname}", func(r chi.Router) {
1187+
r.Get("/", api.apiKeyByName)
1188+
})
1189+
})
1190+
r.Route("/{keyid}", func(r chi.Router) {
1191+
r.Get("/", api.apiKeyByID)
1192+
r.Delete("/", api.deleteAPIKey)
1193+
})
11831194
})
1184-
})
11851195

1186-
r.Route("/organizations", func(r chi.Router) {
1187-
r.Get("/", api.organizationsByUser)
1188-
r.Get("/{organizationname}", api.organizationByUserAndName)
1189-
})
1190-
r.Post("/workspaces", api.postUserWorkspaces)
1191-
r.Route("/workspace/{workspacename}", func(r chi.Router) {
1192-
r.Get("/", api.workspaceByOwnerAndName)
1193-
r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
1194-
})
1195-
r.Get("/gitsshkey", api.gitSSHKey)
1196-
r.Put("/gitsshkey", api.regenerateGitSSHKey)
1197-
r.Route("/notifications", func(r chi.Router) {
1198-
r.Route("/preferences", func(r chi.Router) {
1199-
r.Get("/", api.userNotificationPreferences)
1200-
r.Put("/", api.putUserNotificationPreferences)
1196+
r.Route("/organizations", func(r chi.Router) {
1197+
r.Get("/", api.organizationsByUser)
1198+
r.Get("/{organizationname}", api.organizationByUserAndName)
1199+
})
1200+
r.Route("/workspace/{workspacename}", func(r chi.Router) {
1201+
r.Get("/", api.workspaceByOwnerAndName)
1202+
r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
1203+
})
1204+
r.Get("/gitsshkey", api.gitSSHKey)
1205+
r.Put("/gitsshkey", api.regenerateGitSSHKey)
1206+
r.Route("/notifications", func(r chi.Router) {
1207+
r.Route("/preferences", func(r chi.Router) {
1208+
r.Get("/", api.userNotificationPreferences)
1209+
r.Put("/", api.putUserNotificationPreferences)
1210+
})
1211+
})
1212+
r.Route("/webpush", func(r chi.Router) {
1213+
r.Post("/subscription", api.postUserWebpushSubscription)
1214+
r.Delete("/subscription", api.deleteUserWebpushSubscription)
1215+
r.Post("/test", api.postUserPushNotificationTest)
12011216
})
1202-
})
1203-
r.Route("/webpush", func(r chi.Router) {
1204-
r.Post("/subscription", api.postUserWebpushSubscription)
1205-
r.Delete("/subscription", api.deleteUserWebpushSubscription)
1206-
r.Post("/test", api.postUserPushNotificationTest)
12071217
})
12081218
})
12091219
})

‎coderd/coderdtest/authorize.go‎

Lines changed: 12 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -81,7 +81,7 @@ func AssertRBAC(t *testing.T, api *coderd.API, client *codersdk.Client) RBACAsse
8181
// Note that duplicate rbac calls are handled by the rbac.Cacher(), but
8282
// will be recorded twice. So AllCalls() returns calls regardless if they
8383
// were returned from the cached or not.
84-
func (a RBACAsserter) AllCalls() []AuthCall {
84+
func (a RBACAsserter) AllCalls() AuthCalls {
8585
return a.Recorder.AllCalls(&a.Subject)
8686
}
8787

@@ -140,8 +140,11 @@ func (a RBACAsserter) Reset() RBACAsserter {
140140
return a
141141
}
142142

143+
type AuthCalls []AuthCall
144+
143145
type AuthCall struct {
144146
rbac.AuthCall
147+
Err error
145148

146149
asserted bool
147150
// callers is a small stack trace for debugging.
@@ -252,7 +255,7 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did
252255
}
253256

254257
// recordAuthorize is the internal method that records the Authorize() call.
255-
func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object) {
258+
func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object, authzErr error) {
256259
r.Lock()
257260
defer r.Unlock()
258261

@@ -262,6 +265,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic
262265
Action: action,
263266
Object: object,
264267
},
268+
Err: authzErr,
265269
callers: []string{
266270
// This is a decent stack trace for debugging.
267271
// Some dbauthz calls are a bit nested, so we skip a few.
@@ -288,11 +292,12 @@ func caller(skip int) string {
288292
}
289293

290294
func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action policy.Action, object rbac.Object) error {
291-
r.recordAuthorize(subject, action, object)
292295
if r.Wrapped == nil {
293296
panic("Developer error: RecordingAuthorizer.Wrapped is nil")
294297
}
295-
return r.Wrapped.Authorize(ctx, subject, action, object)
298+
authzErr := r.Wrapped.Authorize(ctx, subject, action, object)
299+
r.recordAuthorize(subject, action, object, authzErr)
300+
return authzErr
296301
}
297302

298303
func (r *RecordingAuthorizer) Prepare(ctx context.Context, subject rbac.Subject, action policy.Action, objectType string) (rbac.PreparedAuthorized, error) {
@@ -339,10 +344,11 @@ func (s *PreparedRecorder) Authorize(ctx context.Context, object rbac.Object) er
339344
s.rw.Lock()
340345
defer s.rw.Unlock()
341346

347+
authzErr := s.prepped.Authorize(ctx, object)
342348
if !s.usingSQL {
343-
s.rec.recordAuthorize(s.subject, s.action, object)
349+
s.rec.recordAuthorize(s.subject, s.action, object, authzErr)
344350
}
345-
return s.prepped.Authorize(ctx, object)
351+
return authzErr
346352
}
347353

348354
func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) {

‎coderd/httpapi/noop.go‎

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,10 @@
1+
package httpapi
2+
3+
import "net/http"
4+
5+
// NoopResponseWriter is a response writer that does nothing.
6+
type NoopResponseWriter struct{}
7+
8+
func (NoopResponseWriter) Header() http.Header { return http.Header{} }
9+
func (NoopResponseWriter) Write(p []byte) (int, error) { return len(p), nil }
10+
func (NoopResponseWriter) WriteHeader(int) {}

‎coderd/httpmw/organizationparam.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -117,7 +117,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
117117
// very important that we do not add the User object to the request context or otherwise
118118
// leak it to the API handler.
119119
// nolint:gocritic
120-
user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
120+
user, ok := ExtractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
121121
if !ok {
122122
return
123123
}

‎coderd/httpmw/userparam.go‎

Lines changed: 25 additions & 4 deletions
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,15 +53,31 @@ 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, &httpapi.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 == "" {
5677
httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
5778
Message: "\"user\" must be provided.",
5879
})
59-
return database.User{}, true
80+
return database.User{}, false
6081
}
6182

6283
if userQuery == "me" {

‎coderd/rbac/object.go‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,10 +1,14 @@
11
package rbac
22

33
import (
4+
"fmt"
5+
"strings"
6+
47
"github.com/google/uuid"
58
"golang.org/x/xerrors"
69

710
"github.com/coder/coder/v2/coderd/rbac/policy"
11+
cstrings "github.com/coder/coder/v2/coderd/util/strings"
812
)
913

1014
// ResourceUserObject is a helper function to create a user object for authz checks.
@@ -37,6 +41,25 @@ type Object struct {
3741
ACLGroupList map[string][]policy.Action ` json:"acl_group_list"`
3842
}
3943

44+
// String is not perfect, but decent enough for human display
45+
func (z Object) String() string {
46+
var parts []string
47+
if z.OrgID != "" {
48+
parts = append(parts, fmt.Sprintf("org:%s", cstrings.Truncate(z.OrgID, 4)))
49+
}
50+
if z.Owner != "" {
51+
parts = append(parts, fmt.Sprintf("owner:%s", cstrings.Truncate(z.Owner, 4)))
52+
}
53+
parts = append(parts, z.Type)
54+
if z.ID != "" {
55+
parts = append(parts, fmt.Sprintf("id:%s", cstrings.Truncate(z.ID, 4)))
56+
}
57+
if len(z.ACLGroupList) > 0 || len(z.ACLUserList) > 0 {
58+
parts = append(parts, fmt.Sprintf("acl:%d", len(z.ACLUserList)+len(z.ACLGroupList)))
59+
}
60+
return strings.Join(parts, ".")
61+
}
62+
4063
// ValidAction checks if the action is valid for the given object type.
4164
func (z Object) ValidAction(action policy.Action) error {
4265
perms, ok := policy.RBACPermissions[z.Type]

0 commit comments

Comments
 (0)