From 38124bdd8d853f394157de1d4c47fa38d8eebd39 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 10:07:26 -0500 Subject: [PATCH 1/9] fix: creating workspaces should not require site wide permissions Creating a workspace requires `read` on site wide `user`. Added unit tests to assert this --- coderd/coderd.go | 116 +++++++++++++++------------ coderd/coderdtest/authorize.go | 33 ++++++-- coderd/rbac/object.go | 23 ++++++ coderd/workspaces_test.go | 36 +++++++++ enterprise/coderd/workspaces_test.go | 49 +++++++++++ 5 files changed, 198 insertions(+), 59 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 1eefd15a8d655..5e9312c1e8a5c 100644 --- 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 index af52f7fc70f53..268b0fa1f05e8 100644 --- 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 index 4f42de94a4c52..9beef03dd8f9a 100644 --- 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 index 76e85b0716181..43ec8891f5e89 100644 --- 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 index eedd6f1bcfa1c..a6cd7efad7534 100644 --- 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() From bf891a43181e8a84b799cfd4570f739d90ba16f1 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 10:32:20 -0500 Subject: [PATCH 2/9] feat: remove site wide perms from creating a workspace --- coderd/coderd.go | 1 - coderd/httpmw/organizationparam.go | 90 +++++++++------- coderd/httpmw/userparam.go | 2 +- coderd/workspaces.go | 163 +++++++++++++++++------------ 4 files changed, 145 insertions(+), 111 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index 5e9312c1e8a5c..ed9b3ec338e1f 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1148,7 +1148,6 @@ func New(options *Options) *API { }) r.Route("/{user}", func(r chi.Router) { 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 diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index 18938ec1e792d..f76dd77d72379 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -110,51 +110,61 @@ type OrganizationMember struct { func ExtractOrganizationMemberParam(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() - // We need to resolve the `{user}` URL parameter so that we can get the userID and - // username. We do this as SystemRestricted since the caller might have permission - // to access the OrganizationMember object, but *not* the User object. So, it is - // 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) - if !ok { - return - } organization := OrganizationParam(r) - - organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ - OrganizationID: organization.ID, - UserID: user.ID, - IncludeSystem: false, - })) - if httpapi.Is404Error(err) { - httpapi.ResourceNotFound(rw) - return - } - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching organization member.", - Detail: err.Error(), - }) + organizationMember, ok := ExtractOrganizationMemberContext(rw, r, db, organization.ID) + if !ok { return } - ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{ - OrganizationMember: organizationMember.OrganizationMember, - // Here we're making two exceptions to the rule about not leaking data about the user - // to the API handler, which is to include the username and avatar URL. - // If the caller has permission to read the OrganizationMember, then we're explicitly - // saying here that they also have permission to see the member's username and avatar. - // This is OK! - // - // API handlers need this information for audit logging and returning the owner's - // username in response to creating a workspace. Additionally, the frontend consumes - // the Avatar URL and this allows the FE to avoid an extra request. - Username: user.Username, - AvatarURL: user.AvatarURL, - }) + ctx := r.Context() + ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember) next.ServeHTTP(rw, r.WithContext(ctx)) }) } } + +func ExtractOrganizationMemberContext(rw http.ResponseWriter, r *http.Request, db database.Store, orgID uuid.UUID) (OrganizationMember, bool) { + ctx := r.Context() + + // We need to resolve the `{user}` URL parameter so that we can get the userID and + // username. We do this as SystemRestricted since the caller might have permission + // to access the OrganizationMember object, but *not* the User object. So, it is + // 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) + if !ok { + return OrganizationMember{}, false + } + + organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: orgID, + UserID: user.ID, + IncludeSystem: false, + })) + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return OrganizationMember{}, false + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organization member.", + Detail: err.Error(), + }) + return OrganizationMember{}, false + } + return OrganizationMember{ + OrganizationMember: organizationMember.OrganizationMember, + // Here we're making two exceptions to the rule about not leaking data about the user + // to the API handler, which is to include the username and avatar URL. + // If the caller has permission to read the OrganizationMember, then we're explicitly + // saying here that they also have permission to see the member's username and avatar. + // This is OK! + // + // API handlers need this information for audit logging and returning the owner's + // username in response to creating a workspace. Additionally, the frontend consumes + // the Avatar URL and this allows the FE to avoid an extra request. + Username: user.Username, + AvatarURL: user.AvatarURL, + }, true +} diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 03bff9bbb5596..6f845986c1fde 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -56,7 +56,7 @@ func extractUserContext(ctx context.Context, db database.Store, rw http.Response httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ Message: "\"user\" must be provided.", }) - return database.User{}, true + return database.User{}, false } if userQuery == "me" { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 6b010b53020a3..8d3440babfd76 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -406,30 +406,46 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { ctx = r.Context() apiKey = httpmw.APIKey(r) auditor = api.Auditor.Load() - user = httpmw.UserParam(r) ) + var req codersdk.CreateWorkspaceRequest + if !httpapi.Read(ctx, rw, r, &req) { + return + } + + // No middleware exists to fetch the user from. This endpoint needs to fetch + // the organization member, which requires the organization. Which can be + // sourced from the template. + // + // TODO: This code gets called twice for each workspace build request. + // This is inefficient and costs at most 2 extra RTTs to the DB. + // This can be optimized. It exists as it is now for code simplicity. + template, ok := requestTemplate(ctx, rw, req, api.Database) + if !ok { + return + } + + member, ok := httpmw.ExtractOrganizationMemberContext(rw, r, api.Database, template.OrganizationID) + if !ok { + return + } + aReq, commitAudit := audit.InitRequest[database.WorkspaceTable](rw, &audit.RequestParams{ Audit: *auditor, Log: api.Logger, Request: r, Action: database.AuditActionCreate, AdditionalFields: audit.AdditionalFields{ - WorkspaceOwner: user.Username, + WorkspaceOwner: member.Username, }, }) defer commitAudit() - var req codersdk.CreateWorkspaceRequest - if !httpapi.Read(ctx, rw, r, &req) { - return - } - owner := workspaceOwner{ - ID: user.ID, - Username: user.Username, - AvatarURL: user.AvatarURL, + ID: member.UserID, + Username: member.Username, + AvatarURL: member.AvatarURL, } createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r) } @@ -450,65 +466,8 @@ func createWorkspace( rw http.ResponseWriter, r *http.Request, ) { - // If we were given a `TemplateVersionID`, we need to determine the `TemplateID` from it. - templateID := req.TemplateID - if templateID == uuid.Nil { - templateVersion, err := api.Database.GetTemplateVersionByID(ctx, req.TemplateVersionID) - if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Template version %q doesn't exist.", templateID.String()), - Validations: []codersdk.ValidationError{{ - Field: "template_version_id", - Detail: "template not found", - }}, - }) - return - } - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching template version.", - Detail: err.Error(), - }) - return - } - if templateVersion.Archived { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Archived template versions cannot be used to make a workspace.", - Validations: []codersdk.ValidationError{ - { - Field: "template_version_id", - Detail: "template version archived", - }, - }, - }) - return - } - - templateID = templateVersion.TemplateID.UUID - } - - template, err := api.Database.GetTemplateByID(ctx, templateID) - if httpapi.Is404Error(err) { - httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ - Message: fmt.Sprintf("Template %q doesn't exist.", templateID.String()), - Validations: []codersdk.ValidationError{{ - Field: "template_id", - Detail: "template not found", - }}, - }) - return - } - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching template.", - Detail: err.Error(), - }) - return - } - if template.Deleted { - httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ - Message: fmt.Sprintf("Template %q has been deleted!", template.Name), - }) + template, ok := requestTemplate(ctx, rw, req, api.Database) + if !ok { return } @@ -776,6 +735,72 @@ func createWorkspace( httpapi.Write(ctx, rw, http.StatusCreated, w) } +func requestTemplate(ctx context.Context, rw http.ResponseWriter, req codersdk.CreateWorkspaceRequest, db database.Store) (database.Template, bool) { + // If we were given a `TemplateVersionID`, we need to determine the `TemplateID` from it. + templateID := req.TemplateID + + if templateID == uuid.Nil { + templateVersion, err := db.GetTemplateVersionByID(ctx, req.TemplateVersionID) + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Template version %q doesn't exist.", req.TemplateVersionID), + Validations: []codersdk.ValidationError{{ + Field: "template_version_id", + Detail: "template not found", + }}, + }) + return database.Template{}, false + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template version.", + Detail: err.Error(), + }) + return database.Template{}, false + } + if templateVersion.Archived { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Archived template versions cannot be used to make a workspace.", + Validations: []codersdk.ValidationError{ + { + Field: "template_version_id", + Detail: "template version archived", + }, + }, + }) + return database.Template{}, false + } + + templateID = templateVersion.TemplateID.UUID + } + + template, err := db.GetTemplateByID(ctx, templateID) + if httpapi.Is404Error(err) { + httpapi.Write(ctx, rw, http.StatusBadRequest, codersdk.Response{ + Message: fmt.Sprintf("Template %q doesn't exist.", templateID), + Validations: []codersdk.ValidationError{{ + Field: "template_id", + Detail: "template not found", + }}, + }) + return database.Template{}, false + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching template.", + Detail: err.Error(), + }) + return database.Template{}, false + } + if template.Deleted { + httpapi.Write(ctx, rw, http.StatusNotFound, codersdk.Response{ + Message: fmt.Sprintf("Template %q has been deleted!", template.Name), + }) + return database.Template{}, false + } + return template, true +} + func (api *API) notifyWorkspaceCreated( ctx context.Context, receiverID uuid.UUID, From 110809ef007e2fafe45c48a1fda4df1b9f28b4ff Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 10:34:07 -0500 Subject: [PATCH 3/9] remove dead code --- coderd/coderdtest/authorize.go | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index 268b0fa1f05e8..b8aa52f537ba7 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -142,21 +142,6 @@ func (a RBACAsserter) Reset() RBACAsserter { 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 From dbdf49f6ec856da1fa143433ae12b1fb5782588f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 11:29:24 -0500 Subject: [PATCH 4/9] chore: fixup permission assertions and testing --- coderd/coderd.go | 1 + coderd/coderdtest/authorize.go | 4 +- coderd/httpmw/organizationparam.go | 90 +++++++++++++--------------- coderd/httpmw/userparam.go | 27 ++++++++- coderd/workspaces.go | 77 +++++++++++++++++------- coderd/workspaces_test.go | 36 ----------- enterprise/coderd/workspaces_test.go | 80 ++++++++++++++++++++++++- 7 files changed, 201 insertions(+), 114 deletions(-) diff --git a/coderd/coderd.go b/coderd/coderd.go index ed9b3ec338e1f..a4883883790a5 100644 --- a/coderd/coderd.go +++ b/coderd/coderd.go @@ -1148,6 +1148,7 @@ func New(options *Options) *API { }) r.Route("/{user}", func(r chi.Router) { 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 diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index b8aa52f537ba7..587c0527fb039 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -144,8 +144,8 @@ type AuthCalls []AuthCall type AuthCall struct { rbac.AuthCall + Err error - err error asserted bool // callers is a small stack trace for debugging. callers []string @@ -265,7 +265,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic Action: action, Object: object, }, - err: err, + Err: err, callers: []string{ // This is a decent stack trace for debugging. // Some dbauthz calls are a bit nested, so we skip a few. diff --git a/coderd/httpmw/organizationparam.go b/coderd/httpmw/organizationparam.go index f76dd77d72379..782a0d37e1985 100644 --- a/coderd/httpmw/organizationparam.go +++ b/coderd/httpmw/organizationparam.go @@ -110,61 +110,51 @@ type OrganizationMember struct { func ExtractOrganizationMemberParam(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) { - organization := OrganizationParam(r) - organizationMember, ok := ExtractOrganizationMemberContext(rw, r, db, organization.ID) + ctx := r.Context() + // We need to resolve the `{user}` URL parameter so that we can get the userID and + // username. We do this as SystemRestricted since the caller might have permission + // to access the OrganizationMember object, but *not* the User object. So, it is + // 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) if !ok { return } + organization := OrganizationParam(r) - ctx := r.Context() - ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, organizationMember) - next.ServeHTTP(rw, r.WithContext(ctx)) - }) - } -} - -func ExtractOrganizationMemberContext(rw http.ResponseWriter, r *http.Request, db database.Store, orgID uuid.UUID) (OrganizationMember, bool) { - ctx := r.Context() - - // We need to resolve the `{user}` URL parameter so that we can get the userID and - // username. We do this as SystemRestricted since the caller might have permission - // to access the OrganizationMember object, but *not* the User object. So, it is - // 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) - if !ok { - return OrganizationMember{}, false - } + organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: organization.ID, + UserID: user.ID, + IncludeSystem: false, + })) + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organization member.", + Detail: err.Error(), + }) + return + } - organizationMember, err := database.ExpectOne(db.OrganizationMembers(ctx, database.OrganizationMembersParams{ - OrganizationID: orgID, - UserID: user.ID, - IncludeSystem: false, - })) - if httpapi.Is404Error(err) { - httpapi.ResourceNotFound(rw) - return OrganizationMember{}, false - } - if err != nil { - httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ - Message: "Internal error fetching organization member.", - Detail: err.Error(), + ctx = context.WithValue(ctx, organizationMemberParamContextKey{}, OrganizationMember{ + OrganizationMember: organizationMember.OrganizationMember, + // Here we're making two exceptions to the rule about not leaking data about the user + // to the API handler, which is to include the username and avatar URL. + // If the caller has permission to read the OrganizationMember, then we're explicitly + // saying here that they also have permission to see the member's username and avatar. + // This is OK! + // + // API handlers need this information for audit logging and returning the owner's + // username in response to creating a workspace. Additionally, the frontend consumes + // the Avatar URL and this allows the FE to avoid an extra request. + Username: user.Username, + AvatarURL: user.AvatarURL, + }) + next.ServeHTTP(rw, r.WithContext(ctx)) }) - return OrganizationMember{}, false } - return OrganizationMember{ - OrganizationMember: organizationMember.OrganizationMember, - // Here we're making two exceptions to the rule about not leaking data about the user - // to the API handler, which is to include the username and avatar URL. - // If the caller has permission to read the OrganizationMember, then we're explicitly - // saying here that they also have permission to see the member's username and avatar. - // This is OK! - // - // API handlers need this information for audit logging and returning the owner's - // username in response to creating a workspace. Additionally, the frontend consumes - // the Avatar URL and this allows the FE to avoid an extra request. - Username: user.Username, - AvatarURL: user.AvatarURL, - }, true } diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 6f845986c1fde..6ae0785313b3e 100644 --- a/coderd/httpmw/userparam.go +++ b/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,8 +53,24 @@ 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, &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 == "" { diff --git a/coderd/workspaces.go b/coderd/workspaces.go index 8d3440babfd76..d49de2388af59 100644 --- a/coderd/workspaces.go +++ b/coderd/workspaces.go @@ -413,21 +413,64 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { return } - // No middleware exists to fetch the user from. This endpoint needs to fetch - // the organization member, which requires the organization. Which can be - // sourced from the template. + var owner workspaceOwner + // This user fetch is an optimization path for the most common case of creating a + // workspace for 'Me'. // - // TODO: This code gets called twice for each workspace build request. - // This is inefficient and costs at most 2 extra RTTs to the DB. - // This can be optimized. It exists as it is now for code simplicity. - template, ok := requestTemplate(ctx, rw, req, api.Database) - if !ok { - return - } + // This is also required to allow `owners` to create workspaces for users + // that are not in an organization. + user, ok := httpmw.UserParamOptional(r) + if ok { + owner = workspaceOwner{ + ID: user.ID, + Username: user.Username, + AvatarURL: user.AvatarURL, + } + } else { + // A workspace can still be created if the caller can read the organization + // member. The organization is required, which can be sourced from the + // template. + // + // TODO: This code gets called twice for each workspace build request. + // This is inefficient and costs at most 2 extra RTTs to the DB. + // This can be optimized. It exists as it is now for code simplicity. + // The most common case is to create a workspace for 'Me'. Which does + // not enter this code branch. + template, ok := requestTemplate(ctx, rw, req, api.Database) + if !ok { + return + } - member, ok := httpmw.ExtractOrganizationMemberContext(rw, r, api.Database, template.OrganizationID) - if !ok { - return + // We need to fetch the original user as a system user to fetch the + // user_id. 'ExtractUserContext' handles all cases like usernames, + // 'Me', etc. + // nolint:gocritic // The user_id needs to be fetched. This handles all those cases. + user, ok := httpmw.ExtractUserContext(dbauthz.AsSystemRestricted(ctx), api.Database, rw, r) + if !ok { + return + } + + organizationMember, err := database.ExpectOne(api.Database.OrganizationMembers(ctx, database.OrganizationMembersParams{ + OrganizationID: template.OrganizationID, + UserID: user.ID, + IncludeSystem: false, + })) + if httpapi.Is404Error(err) { + httpapi.ResourceNotFound(rw) + return + } + if err != nil { + httpapi.Write(ctx, rw, http.StatusInternalServerError, codersdk.Response{ + Message: "Internal error fetching organization member.", + Detail: err.Error(), + }) + return + } + owner = workspaceOwner{ + ID: organizationMember.OrganizationMember.UserID, + Username: organizationMember.Username, + AvatarURL: organizationMember.AvatarURL, + } } aReq, commitAudit := audit.InitRequest[database.WorkspaceTable](rw, &audit.RequestParams{ @@ -436,17 +479,11 @@ func (api *API) postUserWorkspaces(rw http.ResponseWriter, r *http.Request) { Request: r, Action: database.AuditActionCreate, AdditionalFields: audit.AdditionalFields{ - WorkspaceOwner: member.Username, + WorkspaceOwner: owner.Username, }, }) defer commitAudit() - - owner := workspaceOwner{ - ID: member.UserID, - Username: member.Username, - AvatarURL: member.AvatarURL, - } createWorkspace(ctx, aReq, apiKey.UserID, api, owner, req, rw, r) } diff --git a/coderd/workspaces_test.go b/coderd/workspaces_test.go index 43ec8891f5e89..76e85b0716181 100644 --- a/coderd/workspaces_test.go +++ b/coderd/workspaces_test.go @@ -423,42 +423,6 @@ 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 index a6cd7efad7534..c0c62daaac3ab 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -31,6 +31,7 @@ import ( "github.com/coder/coder/v2/coderd/httpmw" "github.com/coder/coder/v2/coderd/notifications" "github.com/coder/coder/v2/coderd/rbac" + "github.com/coder/coder/v2/coderd/rbac/policy" agplschedule "github.com/coder/coder/v2/coderd/schedule" "github.com/coder/coder/v2/coderd/schedule/cron" "github.com/coder/coder/v2/coderd/util/ptr" @@ -266,7 +267,8 @@ func TestCreateUserWorkspace(t *testing.T) { OrganizationID: first.OrganizationID.String(), DisplayName: "Creator", OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{ - codersdk.ResourceWorkspace: {codersdk.ActionCreate}, + codersdk.ResourceWorkspace: {codersdk.ActionCreate, codersdk.ActionWorkspaceStart, codersdk.ActionUpdate, codersdk.ActionRead}, + codersdk.ResourceOrganizationMember: {codersdk.ActionRead}, }), }) require.NoError(t, err) @@ -286,15 +288,87 @@ func TestCreateUserWorkspace(t *testing.T) { ctx = testutil.Context(t, testutil.WaitLong*1000) // Reset the context to avoid timeouts. - var _ = creator - _, err = owner.CreateUserWorkspace(ctx, adminID.ID.String(), codersdk.CreateWorkspaceRequest{ + _, err = creator.CreateUserWorkspace(ctx, adminID.ID.String(), codersdk.CreateWorkspaceRequest{ TemplateID: template.ID, Name: "workspace", }) require.NoError(t, err) }) + // Asserting some authz calls when creating a workspace. + t.Run("AuthzStory", func(t *testing.T) { + t.Parallel() + owner, _, api, first := coderdenttest.NewWithAPI(t, &coderdenttest.Options{ + Options: &coderdtest.Options{ + IncludeProvisionerDaemon: true, + }, + LicenseOptions: &coderdenttest.LicenseOptions{ + Features: license.Features{ + codersdk.FeatureCustomRoles: 1, + codersdk.FeatureTemplateRBAC: 1, + }, + }, + }) + + ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2000) + defer cancel() + + creatorRole, err := owner.CreateOrganizationRole(ctx, codersdk.Role{ + Name: "creator", + OrganizationID: first.OrganizationID.String(), + OrganizationPermissions: codersdk.CreatePermissions(map[codersdk.RBACResource][]codersdk.RBACAction{ + codersdk.ResourceWorkspace: {codersdk.ActionCreate, codersdk.ActionWorkspaceStart, codersdk.ActionUpdate, codersdk.ActionRead}, + codersdk.ResourceOrganizationMember: {codersdk.ActionRead}, + }), + }) + require.NoError(t, err) + + 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) + creator, _ := coderdtest.CreateAnotherUser(t, owner, first.OrganizationID, rbac.RoleIdentifier{ + Name: creatorRole.Name, + OrganizationID: first.OrganizationID, + }) + + // Create a workspace with the current api using an org admin. + authz := coderdtest.AssertRBAC(t, api.AGPL, creator) + authz.Reset() // Reset all previous checks done in setup. + _, err = creator.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() { + if call.Action == policy.ActionRead && + call.Object.Equal(rbac.ResourceUser.WithOwner(userID.ID.String()).WithID(userID.ID)) { + // User read checks are called. If they fail, ignore them. + if call.Err != nil { + continue + } + } + + if call.Object.Type == rbac.ResourceDeploymentConfig.Type { + continue // Ignore + } + + 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(), + ) + } + }) + }) + t.Run("NoTemplateAccess", func(t *testing.T) { + // NoTemplateAccess intentionally does not use provisioners. The template + // version will be stuck in 'pending' forever. t.Parallel() client, first := coderdenttest.New(t, &coderdenttest.Options{ From d40ec9cdf755ba443d7bf8dec19618b872aa332f Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 11:31:28 -0500 Subject: [PATCH 5/9] forgot to commit a file --- coderd/httpmw/noop.go | 10 ++++++++++ 1 file changed, 10 insertions(+) create mode 100644 coderd/httpmw/noop.go diff --git a/coderd/httpmw/noop.go b/coderd/httpmw/noop.go new file mode 100644 index 0000000000000..4128c4d331147 --- /dev/null +++ b/coderd/httpmw/noop.go @@ -0,0 +1,10 @@ +package httpmw + +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) {} From a339587925d68e5f91d427dec0935ea622e92533 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 13:04:58 -0500 Subject: [PATCH 6/9] linting --- enterprise/coderd/workspaces_test.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/enterprise/coderd/workspaces_test.go b/enterprise/coderd/workspaces_test.go index c0c62daaac3ab..72859c5460fa7 100644 --- a/enterprise/coderd/workspaces_test.go +++ b/enterprise/coderd/workspaces_test.go @@ -262,6 +262,7 @@ func TestCreateUserWorkspace(t *testing.T) { }, }) ctx := testutil.Context(t, testutil.WaitShort) + //nolint:gocritic // using owner to setup roles r, err := owner.CreateOrganizationRole(ctx, codersdk.Role{ Name: "creator", OrganizationID: first.OrganizationID.String(), @@ -313,6 +314,7 @@ func TestCreateUserWorkspace(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), testutil.WaitLong*2000) defer cancel() + //nolint:gocritic // using owner to setup roles creatorRole, err := owner.CreateOrganizationRole(ctx, codersdk.Role{ Name: "creator", OrganizationID: first.OrganizationID.String(), From 6ac8a927d94362edd64404375d74d86cdff21c09 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 16:19:04 -0500 Subject: [PATCH 7/9] rename authz err --- coderd/coderdtest/authorize.go | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index 587c0527fb039..c9579c150fe2a 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -295,9 +295,9 @@ func (r *RecordingAuthorizer) Authorize(ctx context.Context, subject rbac.Subjec if r.Wrapped == nil { panic("Developer error: RecordingAuthorizer.Wrapped is nil") } - err := r.Wrapped.Authorize(ctx, subject, action, object) - r.recordAuthorize(subject, action, object, err) - return err + 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) { @@ -344,11 +344,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) + authzErr := s.prepped.Authorize(ctx, object) if !s.usingSQL { - s.rec.recordAuthorize(s.subject, s.action, object, err) + s.rec.recordAuthorize(s.subject, s.action, object, authzErr) } - return err + return authzErr } func (s *PreparedRecorder) CompileToSQL(ctx context.Context, cfg regosql.ConvertConfig) (string, error) { From 0f26830fb9bbfa80364323b4508584b71a80271c Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Tue, 8 Apr 2025 16:21:51 -0500 Subject: [PATCH 8/9] move noop response writer --- coderd/httpapi/noop.go | 10 ++++++++++ coderd/httpmw/noop.go | 10 ---------- coderd/httpmw/userparam.go | 2 +- 3 files changed, 11 insertions(+), 11 deletions(-) create mode 100644 coderd/httpapi/noop.go delete mode 100644 coderd/httpmw/noop.go diff --git a/coderd/httpapi/noop.go b/coderd/httpapi/noop.go new file mode 100644 index 0000000000000..52a0f5dd4d8a4 --- /dev/null +++ b/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) {} diff --git a/coderd/httpmw/noop.go b/coderd/httpmw/noop.go deleted file mode 100644 index 4128c4d331147..0000000000000 --- a/coderd/httpmw/noop.go +++ /dev/null @@ -1,10 +0,0 @@ -package httpmw - -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) {} diff --git a/coderd/httpmw/userparam.go b/coderd/httpmw/userparam.go index 6ae0785313b3e..2fbcc458489f9 100644 --- a/coderd/httpmw/userparam.go +++ b/coderd/httpmw/userparam.go @@ -59,7 +59,7 @@ func ExtractUserParamOptional(db database.Store) func(http.Handler) http.Handler return http.HandlerFunc(func(rw http.ResponseWriter, r *http.Request) { ctx := r.Context() - user, ok := ExtractUserContext(ctx, db, &noopResponseWriter{}, r) + user, ok := ExtractUserContext(ctx, db, &httpapi.NoopResponseWriter{}, r) if ok { ctx = context.WithValue(ctx, userParamContextKey{}, user) } From 1f462646eba18476b029658e312ab5da14aeb791 Mon Sep 17 00:00:00 2001 From: Steven Masley Date: Wed, 9 Apr 2025 07:30:03 -0500 Subject: [PATCH 9/9] rename err --- coderd/coderdtest/authorize.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/coderd/coderdtest/authorize.go b/coderd/coderdtest/authorize.go index c9579c150fe2a..279405c4e6a21 100644 --- a/coderd/coderdtest/authorize.go +++ b/coderd/coderdtest/authorize.go @@ -255,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, err error) { +func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action policy.Action, object rbac.Object, authzErr error) { r.Lock() defer r.Unlock() @@ -265,7 +265,7 @@ func (r *RecordingAuthorizer) recordAuthorize(subject rbac.Subject, action polic Action: action, Object: object, }, - Err: err, + Err: authzErr, callers: []string{ // This is a decent stack trace for debugging. // Some dbauthz calls are a bit nested, so we skip a few.