feat: remove site wide perms from creating a workspace (coder#17296)

Creating a workspace required `read` on site wide `user`. 
Only organization permissions should be required.

Author: Emyrk

coderd/coderd.go

@@ -1146,64 +1146,74 @@ func New(options *Options) *API {
 					r.Get("/", api.AssignableSiteRoles)
 				})
 				r.Route("/{user}", func(r chi.Router) {
-					r.Use(httpmw.ExtractUserParam(options.Database))
-					r.Post("/convert-login", api.postConvertLoginType)
-					r.Delete("/", api.deleteUser)
-					r.Get("/", api.userByName)
-					r.Get("/autofill-parameters", api.userAutofillParameters)
-					r.Get("/login-type", api.userLoginType)
-					r.Put("/profile", api.putUserProfile)
-					r.Route("/status", func(r chi.Router) {
-						r.Put("/suspend", api.putSuspendUserAccount())
-						r.Put("/activate", api.putActivateUserAccount())
+					r.Group(func(r chi.Router) {
+						r.Use(httpmw.ExtractUserParamOptional(options.Database))
+						// Creating workspaces does not require permissions on the user, only the
+						// organization member. This endpoint should match the authz story of
+						// postWorkspacesByOrganization
+						r.Post("/workspaces", api.postUserWorkspaces)
 					})
-					r.Get("/appearance", api.userAppearanceSettings)
-					r.Put("/appearance", api.putUserAppearanceSettings)
-					r.Route("/password", func(r chi.Router) {
-						r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
-						r.Put("/", api.putUserPassword)
-					})
-					// These roles apply to the site wide permissions.
-					r.Put("/roles", api.putUserRoles)
-					r.Get("/roles", api.userRoles)
-
-					r.Route("/keys", func(r chi.Router) {
-						r.Post("/", api.postAPIKey)
-						r.Route("/tokens", func(r chi.Router) {
-							r.Post("/", api.postToken)
-							r.Get("/", api.tokens)
-							r.Get("/tokenconfig", api.tokenConfig)
-							r.Route("/{keyname}", func(r chi.Router) {
-								r.Get("/", api.apiKeyByName)
-							})
+
+					r.Group(func(r chi.Router) {
+						r.Use(httpmw.ExtractUserParam(options.Database))
+
+						r.Post("/convert-login", api.postConvertLoginType)
+						r.Delete("/", api.deleteUser)
+						r.Get("/", api.userByName)
+						r.Get("/autofill-parameters", api.userAutofillParameters)
+						r.Get("/login-type", api.userLoginType)
+						r.Put("/profile", api.putUserProfile)
+						r.Route("/status", func(r chi.Router) {
+							r.Put("/suspend", api.putSuspendUserAccount())
+							r.Put("/activate", api.putActivateUserAccount())
 						})
-						r.Route("/{keyid}", func(r chi.Router) {
-							r.Get("/", api.apiKeyByID)
-							r.Delete("/", api.deleteAPIKey)
+						r.Get("/appearance", api.userAppearanceSettings)
+						r.Put("/appearance", api.putUserAppearanceSettings)
+						r.Route("/password", func(r chi.Router) {
+							r.Use(httpmw.RateLimit(options.LoginRateLimit, time.Minute))
+							r.Put("/", api.putUserPassword)
+						})
+						// These roles apply to the site wide permissions.
+						r.Put("/roles", api.putUserRoles)
+						r.Get("/roles", api.userRoles)
+
+						r.Route("/keys", func(r chi.Router) {
+							r.Post("/", api.postAPIKey)
+							r.Route("/tokens", func(r chi.Router) {
+								r.Post("/", api.postToken)
+								r.Get("/", api.tokens)
+								r.Get("/tokenconfig", api.tokenConfig)
+								r.Route("/{keyname}", func(r chi.Router) {
+									r.Get("/", api.apiKeyByName)
+								})
+							})
+							r.Route("/{keyid}", func(r chi.Router) {
+								r.Get("/", api.apiKeyByID)
+								r.Delete("/", api.deleteAPIKey)
+							})
 						})
-					})
 
-					r.Route("/organizations", func(r chi.Router) {
-						r.Get("/", api.organizationsByUser)
-						r.Get("/{organizationname}", api.organizationByUserAndName)
-					})
-					r.Post("/workspaces", api.postUserWorkspaces)
-					r.Route("/workspace/{workspacename}", func(r chi.Router) {
-						r.Get("/", api.workspaceByOwnerAndName)
-						r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
-					})
-					r.Get("/gitsshkey", api.gitSSHKey)
-					r.Put("/gitsshkey", api.regenerateGitSSHKey)
-					r.Route("/notifications", func(r chi.Router) {
-						r.Route("/preferences", func(r chi.Router) {
-							r.Get("/", api.userNotificationPreferences)
-							r.Put("/", api.putUserNotificationPreferences)
+						r.Route("/organizations", func(r chi.Router) {
+							r.Get("/", api.organizationsByUser)
+							r.Get("/{organizationname}", api.organizationByUserAndName)
+						})
+						r.Route("/workspace/{workspacename}", func(r chi.Router) {
+							r.Get("/", api.workspaceByOwnerAndName)
+							r.Get("/builds/{buildnumber}", api.workspaceBuildByBuildNumber)
+						})
+						r.Get("/gitsshkey", api.gitSSHKey)
+						r.Put("/gitsshkey", api.regenerateGitSSHKey)
+						r.Route("/notifications", func(r chi.Router) {
+							r.Route("/preferences", func(r chi.Router) {
+								r.Get("/", api.userNotificationPreferences)
+								r.Put("/", api.putUserNotificationPreferences)
+							})
+						})
+						r.Route("/webpush", func(r chi.Router) {
+							r.Post("/subscription", api.postUserWebpushSubscription)
+							r.Delete("/subscription", api.deleteUserWebpushSubscription)
+							r.Post("/test", api.postUserPushNotificationTest)
 						})
-					})
-					r.Route("/webpush", func(r chi.Router) {
-						r.Post("/subscription", api.postUserWebpushSubscription)
-						r.Delete("/subscription", api.deleteUserWebpushSubscription)
-						r.Post("/test", api.postUserPushNotificationTest)
 					})
 				})
 			})

coderd/coderdtest/authorize.go

@@ -81,7 +81,7 @@ func AssertRBAC(t *testing.T, api *coderd.API, client *codersdk.Client) RBACAsse
 // Note that duplicate rbac calls are handled by the rbac.Cacher(), but
 // will be recorded twice. So AllCalls() returns calls regardless if they
 // were returned from the cached or not.
-func (a RBACAsserter) AllCalls() []AuthCall {
+func (a RBACAsserter) AllCalls() AuthCalls {
 	return a.Recorder.AllCalls(&a.Subject)
 }
 
@@ -140,8 +140,11 @@ func (a RBACAsserter) Reset() RBACAsserter {
 	return a
 }
 
+type AuthCalls []AuthCall
+
 type AuthCall struct {
 	rbac.AuthCall
+	Err error
 
 	asserted bool
 	// callers is a small stack trace for debugging.
@@ -252,7 +255,7 @@ func (r *RecordingAuthorizer) AssertActor(t *testing.T, actor rbac.Subject, did
 }
 
 // recordAuthorize is the internal method that records the Authorize() call.
-func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object) {
+func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object, authzErr error) {
 	r.Lock()
 	defer r.Unlock()
 
@@ -262,6 +265,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic
 			Action: action,
 			Object: object,
 		},
+		Err: authzErr,
 		callers: []string{
 			// This is a decent stack trace for debugging.
 			// Some dbauthz calls are a bit nested, so we skip a few.
@@ -288,11 +292,12 @@ func caller(skip int) string {
 }
 
 func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subject, action policy.Action, object rbac.Object) error {
-	r.recordAuthorize(subject, action, object)
 	if r.Wrapped == nil {
 		panic("Developer error: RecordingAuthorizer.Wrapped is nil")
 	}
-	return r.Wrapped.Authorize(ctx, subject, action, object)
+	authzErr := r.Wrapped.Authorize(ctx, subject, action, object)
+	r.recordAuthorize(subject, action, object, authzErr)
+	return authzErr
 }
 
 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
 	s.rw.Lock()
 	defer s.rw.Unlock()
 
+	authzErr := s.prepped.Authorize(ctx, object)
 	if !s.usingSQL {
-		s.rec.recordAuthorize(s.subject, s.action, object)
+		s.rec.recordAuthorize(s.subject, s.action, object, authzErr)
 	}
-	return s.prepped.Authorize(ctx, object)
+	return authzErr
 }
 
 func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) {

coderd/httpapi/noop.go

@@ -0,0 +1,10 @@
+package httpapi
+
+import "net/http"
+
+// NoopResponseWriter is a response writer that does nothing.
+type NoopResponseWriter struct{}
+
+func (NoopResponseWriter) Header() http.Header         { return http.Header{} }
+func (NoopResponseWriter) Write(p []byte) (int, error) { return len(p), nil }
+func (NoopResponseWriter) WriteHeader(int)             {}

coderd/httpmw/organizationparam.go

@@ -117,7 +117,7 @@ func ExtractOrganizationMemberParam(db database.Store) func(http.Handler) http.H
 			// very important that we do not add the User object to the request context or otherwise
 			// leak it to the API handler.
 			// nolint:gocritic
-			user, ok := extractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
+			user, ok := ExtractUserContext(dbauthz.AsSystemRestricted(ctx), db, rw, r)
 			if !ok {
 				return
 			}

coderd/httpmw/userparam.go

@@ -31,13 +31,18 @@ func UserParam(r *http.Request) database.User {
 	return user
 }
 
+func UserParamOptional(r *http.Request) (database.User, bool) {
+	user, ok := r.Context().Value(userParamContextKey{}).(database.User)
+	return user, ok
+}
+
 // ExtractUserParam extracts a user from an ID/username in the {user} URL
 // parameter.
 func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
 	return func(next http.Handler) http.Handler {
 		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
 			ctx := r.Context()
-			user, ok := extractUserContext(ctx, db, rw, r)
+			user, ok := ExtractUserContext(ctx, db, rw, r)
 			if !ok {
 				// response already handled
 				return
@@ -48,15 +53,31 @@ func ExtractUserParam(db database.Store) func(http.Handler) http.Handler {
 	}
 }
 
-// extractUserContext queries the database for the parameterized `{user}` from the request URL.
-func extractUserContext(ctx context.Context, db database.Store, rw http.ResponseWriter, r *http.Request) (user database.User, ok bool) {
+// ExtractUserParamOptional does not fail if no user is present.
+func ExtractUserParamOptional(db database.Store) func(http.Handler) http.Handler {
+	return func(next http.Handler) http.Handler {
+		return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) {
+			ctx := r.Context()
+
+			user, ok := ExtractUserContext(ctx, db, &httpapi.NoopResponseWriter{}, r)
+			if ok {
+				ctx = context.WithValue(ctx, userParamContextKey{}, user)
+			}
+
+			next.ServeHTTP(rw, r.WithContext(ctx))
+		})
+	}
+}
+
+// ExtractUserContext queries the database for the parameterized `{user}` from the request URL.
+func ExtractUserContext(ctx context.Context, db database.Store, rw http.ResponseWriter, r *http.Request) (user database.User, ok bool) {
 	// userQuery is either a uuid, a username, or 'me'
 	userQuery := chi.URLParam(r, "user")
 	if userQuery == "" {
 		httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{
 			Message: "\"user\" must be provided.",
 		})
-		return database.User{}, true
+		return database.User{}, false
 	}
 
 	if userQuery == "me" {

coderd/rbac/object.go

@@ -1,10 +1,14 @@
 package rbac
 
 import (
+	"fmt"
+	"strings"
+
 	"github.com/google/uuid"
 	"golang.org/x/xerrors"
 
 	"github.com/coder/coder/v2/coderd/rbac/policy"
+	cstrings "github.com/coder/coder/v2/coderd/util/strings"
 )
 
 // ResourceUserObject is a helper function to create a user object for authz checks.
@@ -37,6 +41,25 @@ type Object struct {
 	ACLGroupList map[string][]policy.Action ` json:"acl_group_list"`
 }
 
+// String is not perfect, but decent enough for human display
+func (z Object) String() string {
+	var parts []string
+	if z.OrgID != "" {
+		parts = append(parts, fmt.Sprintf("org:%s", cstrings.Truncate(z.OrgID, 4)))
+	}
+	if z.Owner != "" {
+		parts = append(parts, fmt.Sprintf("owner:%s", cstrings.Truncate(z.Owner, 4)))
+	}
+	parts = append(parts, z.Type)
+	if z.ID != "" {
+		parts = append(parts, fmt.Sprintf("id:%s", cstrings.Truncate(z.ID, 4)))
+	}
+	if len(z.ACLGroupList) > 0 || len(z.ACLUserList) > 0 {
+		parts = append(parts, fmt.Sprintf("acl:%d", len(z.ACLUserList)+len(z.ACLGroupList)))
+	}
+	return strings.Join(parts, ".")
+}
+
 // ValidAction checks if the action is valid for the given object type.
 func (z Object) ValidAction(action policy.Action) error {
 	perms, ok := policy.RBACPermissions[z.Type]