From 7e46d24b410498eeb8917d3c463377ce48f7691a Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Wed, 21 May 2025 04:03:05 +0000 Subject: [PATCH 1/2] chore: avoid depending on rbac in slim builds --- .../coder_users_edit-roles_--help.golden | 3 +- cli/usereditroles.go | 29 ++++++++----------- coderd/httpapi/authz.go | 28 ++++++++++++++++++ coderd/httpapi/authz_slim.go | 13 +++++++++ coderd/httpapi/httpapi.go | 19 ++---------- coderd/httpmw/authz.go | 2 ++ coderd/rbac/no_slim.go | 8 +++++ coderd/rbac/no_slim_slim.go | 14 +++++++++ docs/reference/cli/users_edit-roles.md | 2 +- 9 files changed, 81 insertions(+), 37 deletions(-) create mode 100644 coderd/httpapi/authz.go create mode 100644 coderd/httpapi/authz_slim.go create mode 100644 coderd/rbac/no_slim.go create mode 100644 coderd/rbac/no_slim_slim.go diff --git a/cli/testdata/coder_users_edit-roles_--help.golden b/cli/testdata/coder_users_edit-roles_--help.golden index 02dd9155b4d4e..5a21c152e63fc 100644 --- a/cli/testdata/coder_users_edit-roles_--help.golden +++ b/cli/testdata/coder_users_edit-roles_--help.golden @@ -8,8 +8,7 @@ USAGE: OPTIONS: --roles string-array A list of roles to give to the user. This removes any existing roles - the user may have. The available roles are: auditor, member, owner, - template-admin, user-admin. + the user may have. -y, --yes bool Bypass prompts. diff --git a/cli/usereditroles.go b/cli/usereditroles.go index 815d8f47dc186..5bdad7a66863b 100644 --- a/cli/usereditroles.go +++ b/cli/usereditroles.go @@ -1,32 +1,19 @@ package cli import ( - "fmt" "slices" - "sort" "strings" "golang.org/x/xerrors" "github.com/coder/coder/v2/cli/cliui" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/codersdk" "github.com/coder/serpent" ) func (r *RootCmd) userEditRoles() *serpent.Command { client := new(codersdk.Client) - - roles := rbac.SiteRoles() - - siteRoles := make([]string, 0) - for _, role := range roles { - siteRoles = append(siteRoles, role.Identifier.Name) - } - sort.Strings(siteRoles) - var givenRoles []string - cmd := &serpent.Command{ Use: "edit-roles ", Short: "Edit a user's roles by username or id", @@ -34,7 +21,7 @@ func (r *RootCmd) userEditRoles() *serpent.Command { cliui.SkipPromptOption(), { Name: "roles", - Description: fmt.Sprintf("A list of roles to give to the user. This removes any existing roles the user may have. The available roles are: %s.", strings.Join(siteRoles, ", ")), + Description: "A list of roles to give to the user. This removes any existing roles the user may have.", Flag: "roles", Value: serpent.StringArrayOf(&givenRoles), }, @@ -52,13 +39,21 @@ func (r *RootCmd) userEditRoles() *serpent.Command { if err != nil { return xerrors.Errorf("fetch user roles: %w", err) } + siteRoles, err := client.ListSiteRoles(ctx) + if err != nil { + return xerrors.Errorf("fetch site roles: %w", err) + } + siteRoleNames := make([]string, 0, len(siteRoles)) + for _, role := range siteRoles { + siteRoleNames = append(siteRoleNames, role.Name) + } var selectedRoles []string if len(givenRoles) > 0 { // Make sure all of the given roles are valid site roles for _, givenRole := range givenRoles { - if !slices.Contains(siteRoles, givenRole) { - siteRolesPretty := strings.Join(siteRoles, ", ") + if !slices.Contains(siteRoleNames, givenRole) { + siteRolesPretty := strings.Join(siteRoleNames, ", ") return xerrors.Errorf("The role %s is not valid. Please use one or more of the following roles: %s\n", givenRole, siteRolesPretty) } } @@ -67,7 +62,7 @@ func (r *RootCmd) userEditRoles() *serpent.Command { } else { selectedRoles, err = cliui.MultiSelect(inv, cliui.MultiSelectOptions{ Message: "Select the roles you'd like to assign to the user", - Options: siteRoles, + Options: siteRoleNames, Defaults: userRoles.Roles, }) if err != nil { diff --git a/coderd/httpapi/authz.go b/coderd/httpapi/authz.go new file mode 100644 index 0000000000000..f0f208d31b937 --- /dev/null +++ b/coderd/httpapi/authz.go @@ -0,0 +1,28 @@ +//go:build !slim + +package httpapi + +import ( + "context" + "net/http" + + "github.com/coder/coder/v2/coderd/rbac" +) + +// This is defined separately in slim builds to avoid importing the rbac +// package, which is a large dependency. +func SetAuthzCheckRecorderHeader(ctx context.Context, rw http.ResponseWriter) { + if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { + // If you're here because you saw this header in a response, and you're + // trying to investigate the code, here are a couple of notable things + // for you to know: + // - If any of the checks are `false`, they might not represent the whole + // picture. There could be additional checks that weren't performed, + // because processing stopped after the failure. + // - The checks are recorded by the `authzRecorder` type, which is + // configured on server startup for development and testing builds. + // - If this header is missing from a response, make sure the response is + // being written by calling `httpapi.Write`! + rw.Header().Set("x-authz-checks", rec.String()) + } +} diff --git a/coderd/httpapi/authz_slim.go b/coderd/httpapi/authz_slim.go new file mode 100644 index 0000000000000..0ebe7ca01aa86 --- /dev/null +++ b/coderd/httpapi/authz_slim.go @@ -0,0 +1,13 @@ +//go:build slim + +package httpapi + +import ( + "context" + "net/http" +) + +func SetAuthzCheckRecorderHeader(ctx context.Context, rw http.ResponseWriter) { + // There's no RBAC on the agent API, so this is separately defined to + // avoid importing the RBAC package, which is a large dependency. +} diff --git a/coderd/httpapi/httpapi.go b/coderd/httpapi/httpapi.go index 5c5c623474a47..466d45de82e5d 100644 --- a/coderd/httpapi/httpapi.go +++ b/coderd/httpapi/httpapi.go @@ -20,7 +20,6 @@ import ( "github.com/coder/websocket/wsjson" "github.com/coder/coder/v2/coderd/httpapi/httpapiconstraints" - "github.com/coder/coder/v2/coderd/rbac" "github.com/coder/coder/v2/coderd/tracing" "github.com/coder/coder/v2/codersdk" ) @@ -199,19 +198,7 @@ func Write(ctx context.Context, rw http.ResponseWriter, status int, response int _, span := tracing.StartSpan(ctx) defer span.End() - if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { - // If you're here because you saw this header in a response, and you're - // trying to investigate the code, here are a couple of notable things - // for you to know: - // - If any of the checks are `false`, they might not represent the whole - // picture. There could be additional checks that weren't performed, - // because processing stopped after the failure. - // - The checks are recorded by the `authzRecorder` type, which is - // configured on server startup for development and testing builds. - // - If this header is missing from a response, make sure the response is - // being written by calling `httpapi.Write`! - rw.Header().Set("x-authz-checks", rec.String()) - } + SetAuthzCheckRecorderHeader(ctx, rw) rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(status) @@ -228,9 +215,7 @@ func WriteIndent(ctx context.Context, rw http.ResponseWriter, status int, respon _, span := tracing.StartSpan(ctx) defer span.End() - if rec, ok := rbac.GetAuthzCheckRecorder(ctx); ok { - rw.Header().Set("x-authz-checks", rec.String()) - } + SetAuthzCheckRecorderHeader(ctx, rw) rw.Header().Set("Content-Type", "application/json; charset=utf-8") rw.WriteHeader(status) diff --git a/coderd/httpmw/authz.go b/coderd/httpmw/authz.go index 53aadb6cb7a57..9f1f397c858e0 100644 --- a/coderd/httpmw/authz.go +++ b/coderd/httpmw/authz.go @@ -1,3 +1,5 @@ +//go:build !slim + package httpmw import ( diff --git a/coderd/rbac/no_slim.go b/coderd/rbac/no_slim.go new file mode 100644 index 0000000000000..d44a3489c62c9 --- /dev/null +++ b/coderd/rbac/no_slim.go @@ -0,0 +1,8 @@ +package rbac + +const ( + // This declaration protects against imports in slim builds, see + // no_slim_slim.go. + //nolint:revive,unused + _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" +) diff --git a/coderd/rbac/no_slim_slim.go b/coderd/rbac/no_slim_slim.go new file mode 100644 index 0000000000000..62848057857bd --- /dev/null +++ b/coderd/rbac/no_slim_slim.go @@ -0,0 +1,14 @@ +//go:build slim + +package rbac + +const ( + // This re-declaration will result in a compilation error and is present to + // prevent increasing the slim binary size by importing this package, + // directly or indirectly. + // + // no_slim_slim.go:7:2: _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS redeclared in this block + // no_slim.go:4:2: other declaration of _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS + //nolint:revive,unused + _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" +) diff --git a/docs/reference/cli/users_edit-roles.md b/docs/reference/cli/users_edit-roles.md index 23e0baa42afff..04f12ce701584 100644 --- a/docs/reference/cli/users_edit-roles.md +++ b/docs/reference/cli/users_edit-roles.md @@ -25,4 +25,4 @@ Bypass prompts. |------|---------------------------| | Type | string-array | -A list of roles to give to the user. This removes any existing roles the user may have. The available roles are: auditor, member, owner, template-admin, user-admin. +A list of roles to give to the user. This removes any existing roles the user may have. From 953816ad87cb9ccad95036eba48187141d4f3903 Mon Sep 17 00:00:00 2001 From: Ethan Dickson Date: Thu, 22 May 2025 03:51:30 +0000 Subject: [PATCH 2/2] single file compile guard, SiteRoles -> SiteBuiltInRoles --- coderd/database/no_slim.go | 9 +++++---- coderd/database/no_slim_slim.go | 14 -------------- coderd/rbac/no_slim.go | 9 +++++---- coderd/rbac/no_slim_slim.go | 14 -------------- coderd/rbac/roles.go | 4 ++-- coderd/rbac/roles_test.go | 4 ++-- coderd/roles.go | 2 +- 7 files changed, 15 insertions(+), 41 deletions(-) delete mode 100644 coderd/database/no_slim_slim.go delete mode 100644 coderd/rbac/no_slim_slim.go diff --git a/coderd/database/no_slim.go b/coderd/database/no_slim.go index 561466490f53e..edb81e23ad1c7 100644 --- a/coderd/database/no_slim.go +++ b/coderd/database/no_slim.go @@ -1,8 +1,9 @@ +//go:build slim + package database const ( - // This declaration protects against imports in slim builds, see - // no_slim_slim.go. - //nolint:revive,unused - _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" + // This line fails to compile, preventing this package from being imported + // in slim builds. + _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS ) diff --git a/coderd/database/no_slim_slim.go b/coderd/database/no_slim_slim.go deleted file mode 100644 index 845ac0df77942..0000000000000 --- a/coderd/database/no_slim_slim.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build slim - -package database - -const ( - // This re-declaration will result in a compilation error and is present to - // prevent increasing the slim binary size by importing this package, - // directly or indirectly. - // - // no_slim_slim.go:7:2: _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS redeclared in this block - // no_slim.go:4:2: other declaration of _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS - //nolint:revive,unused - _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" -) diff --git a/coderd/rbac/no_slim.go b/coderd/rbac/no_slim.go index d44a3489c62c9..d1baaeade4108 100644 --- a/coderd/rbac/no_slim.go +++ b/coderd/rbac/no_slim.go @@ -1,8 +1,9 @@ +//go:build slim + package rbac const ( - // This declaration protects against imports in slim builds, see - // no_slim_slim.go. - //nolint:revive,unused - _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" + // This line fails to compile, preventing this package from being imported + // in slim builds. + _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS ) diff --git a/coderd/rbac/no_slim_slim.go b/coderd/rbac/no_slim_slim.go deleted file mode 100644 index 62848057857bd..0000000000000 --- a/coderd/rbac/no_slim_slim.go +++ /dev/null @@ -1,14 +0,0 @@ -//go:build slim - -package rbac - -const ( - // This re-declaration will result in a compilation error and is present to - // prevent increasing the slim binary size by importing this package, - // directly or indirectly. - // - // no_slim_slim.go:7:2: _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS redeclared in this block - // no_slim.go:4:2: other declaration of _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS - //nolint:revive,unused - _DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS = "DO_NOT_IMPORT_THIS_PACKAGE_IN_SLIM_BUILDS" -) diff --git a/coderd/rbac/roles.go b/coderd/rbac/roles.go index 89f86b567a48d..8b98f5f2f2bc7 100644 --- a/coderd/rbac/roles.go +++ b/coderd/rbac/roles.go @@ -798,12 +798,12 @@ func OrganizationRoles(organizationID uuid.UUID) []Role { return roles } -// SiteRoles lists all roles that can be applied to a user. +// SiteBuiltInRoles lists all roles that can be applied to a user. // This is the list of available roles, and not specific to a user // // This should be a list in a database, but until then we build // the list from the builtins. -func SiteRoles() []Role { +func SiteBuiltInRoles() []Role { var roles []Role for _, roleF := range builtInRoles { // Must provide some non-nil uuid to filter out org roles. diff --git a/coderd/rbac/roles_test.go b/coderd/rbac/roles_test.go index 4dfbc8fa2ab31..5738edfe8caa2 100644 --- a/coderd/rbac/roles_test.go +++ b/coderd/rbac/roles_test.go @@ -34,7 +34,7 @@ func (a authSubject) Subjects() []authSubject { return []authSubject{a} } // rules. If this is incorrect, that is a mistake. func TestBuiltInRoles(t *testing.T) { t.Parallel() - for _, r := range rbac.SiteRoles() { + for _, r := range rbac.SiteBuiltInRoles() { r := r t.Run(r.Identifier.String(), func(t *testing.T) { t.Parallel() @@ -997,7 +997,7 @@ func TestIsOrgRole(t *testing.T) { func TestListRoles(t *testing.T) { t.Parallel() - siteRoles := rbac.SiteRoles() + siteRoles := rbac.SiteBuiltInRoles() siteRoleNames := make([]string, 0, len(siteRoles)) for _, role := range siteRoles { siteRoleNames = append(siteRoleNames, role.Identifier.Name) diff --git a/coderd/roles.go b/coderd/roles.go index 89e6a964aba31..ed650f41fd6c9 100644 --- a/coderd/roles.go +++ b/coderd/roles.go @@ -43,7 +43,7 @@ func (api *API) AssignableSiteRoles(rw http.ResponseWriter, r *http.Request) { return } - httpapi.Write(ctx, rw, http.StatusOK, assignableRoles(actorRoles.Roles, rbac.SiteRoles(), dbCustomRoles)) + httpapi.Write(ctx, rw, http.StatusOK, assignableRoles(actorRoles.Roles, rbac.SiteBuiltInRoles(), dbCustomRoles)) } // assignableOrgRoles returns all org wide roles that can be assigned.