fix: creating workspaces should not require site wide permissions

Creating a workspace requires `read` on site wide `user`. Added unit tests to assert this
coder · Emyrk · Apr 9, 2025 · Apr 8, 2025 · Apr 8, 2025 · Apr 8, 2025
commit 38124bdd8d853f394157de1d4c47fa38d8eebd39
diff --git a/coderd/coderd.go b/coderd/coderd.go
@@ -1147,64 +1147,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.ExtractUserParam(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)
 					})
 				})
 			})

diff --git a/coderd/coderdtest/authorize.go b/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,9 +140,27 @@ func (a RBACAsserter) Reset() RBACAsserter {
 	return a
 }
 
+type AuthCalls []AuthCall
+
+func (c AuthCalls) Mutate(mut func(c AuthCall) AuthCall) AuthCalls {
+	for i := range c {
+		c[i] = mut(c[i])
+	}
+	return c
+}
+
+func (c AuthCalls) String() string {
+	var str strings.Builder
+	for _, call := range c {
+		str.WriteString(fmt.Sprintf("%s: %s.%s\n", call.Actor.FriendlyName, call.Action, call.Object.Type))
+	}
+	return str.String()
+}
+
 type AuthCall struct {
 	rbac.AuthCall
 
+	err      error
 	asserted bool
 	// callers is a small stack trace for debugging.
 	callers []string
@@ -252,7 +270,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, err error) {
 	r.Lock()
 	defer r.Unlock()
 
@@ -262,6 +280,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic
 			Action: action,
 			Object: object,
 		},
+		err: err,
 		callers: []string{
 			// This is a decent stack trace for debugging.
 			// Some dbauthz calls are a bit nested, so we skip a few.
@@ -288,11 +307,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)
+	err := r.Wrapped.Authorize(ctx, subject, action, object)
+	r.recordAuthorize(subject, action, object, err)
+	return err
 }
 
 func (r *RecordingAuthorizer) Prepare(ctx context.Context, subject rbac.Subject, action policy.Action, objectType string) (rbac.PreparedAuthorized, error) {
@@ -339,10 +359,11 @@ func (s *PreparedRecorder) Authorize(ctx context.Context, object rbac.Object) er
 	s.rw.Lock()
 	defer s.rw.Unlock()
 
+	err := s.prepped.Authorize(ctx, object)
 	if !s.usingSQL {
-		s.rec.recordAuthorize(s.subject, s.action, object)
+		s.rec.recordAuthorize(s.subject, s.action, object, err)
 	}
-	return s.prepped.Authorize(ctx, object)
+	return err
 }
 
 func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) {

diff --git a/coderd/rbac/object.go b/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]

diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go
@@ -423,6 +423,42 @@ func TestWorkspace(t *testing.T) {
 		require.ErrorAs(t, err, &apiError)
 		require.Equal(t, http.StatusForbidden, apiError.StatusCode())
 	})
+
+	// Asserting some authz calls when creating a workspace.
+	t.Run("AuthzStory", func(t *testing.T) {
+		t.Parallel()
+		owner, _, api := coderdtest.NewWithAPI(t, &coderdtest.Options{IncludeProvisionerDaemon: true})
+		first := coderdtest.CreateFirstUser(t, owner)
+		authz := coderdtest.AssertRBAC(t, api, owner)
+
+		version := coderdtest.CreateTemplateVersion(t, owner, first.OrganizationID, nil)
+		coderdtest.AwaitTemplateVersionJobCompleted(t, owner, version.ID)
+		template := coderdtest.CreateTemplate(t, owner, first.OrganizationID, version.ID)
+		_, userID := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID)
+
+		ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong)
+		defer cancel()
+
+		// Create a workspace with the current api.
+		authz.Reset() // Reset all previous checks done in setup.
+		_, err := owner.CreateUserWorkspace(ctx, userID.ID.String(), codersdk.CreateWorkspaceRequest{
+			TemplateID: template.ID,
+			Name:       "test-user",
+		})
+		require.NoError(t, err)
+
+		// Assert all authz properties
+		t.Run("OnlyOrganizationAuthzCalls", func(t *testing.T) {
+			// Creating workspaces is an organization action. So organization
+			// permissions should be sufficient to complete the action.
+			for _, call := range authz.AllCalls() {
+				assert.Falsef(t, call.Object.OrgID == "",
+					"call %q for object %q has no organization set. Site authz calls not expected here",
+					call.Action, call.Object.String(),
+				)
+			}
+		})
+	})
 }
 
 func TestResolveAutostart(t *testing.T) {

diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go
@@ -245,6 +245,55 @@ func TestCreateWorkspace(t *testing.T) {
 func TestCreateUserWorkspace(t *testing.T) {
 	t.Parallel()
 
+	// Create a custom role that can create workspaces for another user.
+	t.Run("ForAnotherUser", func(t *testing.T) {
+		t.Parallel()
+
+		owner, first := coderdenttest.New(t, &coderdenttest.Options{
+			Options: &coderdtest.Options{
+				IncludeProvisionerDaemon: true,
+			},
+			LicenseOptions: &coderdenttest.LicenseOptions{
+				Features: license.Features{
+					codersdk.FeatureCustomRoles:  1,
+					codersdk.FeatureTemplateRBAC: 1,
+				},
+			},
+		})
+		ctx := testutil.Context(t, testutil.WaitShort)
+		r, err := owner.CreateOrganizationRole(ctx, codersdk.Role{
+			Name:           "creator",
+			OrganizationID: first.OrganizationID.String(),
+			DisplayName:    "Creator",
+			OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{
+				codersdk.ResourceWorkspace: {codersdk.ActionCreate},
+			}),
+		})
+		require.NoError(t, err)
+
+		// use admin for setting up test
+		admin, adminID := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleTemplateAdmin())
+
+		// try the test action with this user & custom role
+		creator, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleMember(), rbac.RoleIdentifier{
+			Name:           r.Name,
+			OrganizationID: first.OrganizationID,
+		})
+
+		version := coderdtest.CreateTemplateVersion(t, admin, first.OrganizationID, nil)
+		coderdtest.AwaitTemplateVersionJobCompleted(t, admin, version.ID)
+		template := coderdtest.CreateTemplate(t, admin, first.OrganizationID, version.ID)
+
+		ctx = testutil.Context(t, testutil.WaitLong*1000) // Reset the context to avoid timeouts.
+
+		var _ = creator
+		_, err = owner.CreateUserWorkspace(ctx, adminID.ID.String(), codersdk.CreateWorkspaceRequest{
+			TemplateID: template.ID,
+			Name:       "workspace",
+		})
+		require.NoError(t, err)
+	})
+
 	t.Run("NoTemplateAccess", func(t *testing.T) {
 		t.Parallel()