diff --git a/coderd/database/dbauthz/customroles_test.go b/coderd/database/dbauthz/customroles_test.go index a5077121c0629..b98af8fd23889 100644 --- a/coderd/database/dbauthz/customroles_test.go +++ b/coderd/database/dbauthz/customroles_test.go @@ -38,7 +38,7 @@ func TestUpsertCustomRoles(t *testing.T) { Name: "can-assign", DisplayName: "", Site: rbac.Permissions(map[string][]policy.Action{ - rbac.ResourceAssignRole.Type: {policy.ActionCreate}, + rbac.ResourceAssignRole.Type: {policy.ActionRead, policy.ActionCreate}, }), } @@ -243,6 +243,20 @@ func TestUpsertCustomRoles(t *testing.T) { require.ErrorContains(t, err, tc.errorContains) } else { require.NoError(t, err) + + // Verify we can fetch the role + roles, err := az.CustomRoles(ctx, database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{ + { + Name: "test-role", + OrganizationID: tc.organizationID.UUID, + }, + }, + ExcludeOrgRoles: false, + OrganizationID: uuid.UUID{}, + }) + require.NoError(t, err) + require.Len(t, roles, 1) } }) } diff --git a/coderd/database/dbmem/dbmem.go b/coderd/database/dbmem/dbmem.go index 2bfff39a949a9..727f76077b023 100644 --- a/coderd/database/dbmem/dbmem.go +++ b/coderd/database/dbmem/dbmem.go @@ -1186,12 +1186,17 @@ func (q *FakeQuerier) CustomRoles(_ context.Context, arg database.CustomRolesPar for _, role := range q.data.customRoles { role := role if len(arg.LookupRoles) > 0 { - if !slices.ContainsFunc(arg.LookupRoles, func(s string) bool { - roleName := rbac.RoleName(role.Name, "") - if role.OrganizationID.UUID != uuid.Nil { - roleName = rbac.RoleName(role.Name, role.OrganizationID.UUID.String()) + if !slices.ContainsFunc(arg.LookupRoles, func(pair database.NameOrganizationPair) bool { + if pair.Name != role.Name { + return false } - return strings.EqualFold(s, roleName) + + if role.OrganizationID.Valid { + // Expect org match + return role.OrganizationID.UUID == pair.OrganizationID + } + // Expect no org + return pair.OrganizationID == uuid.Nil }) { continue } diff --git a/coderd/database/dbtestutil/postgres.go b/coderd/database/dbtestutil/postgres.go index 33e0350821099..3a559778b6968 100644 --- a/coderd/database/dbtestutil/postgres.go +++ b/coderd/database/dbtestutil/postgres.go @@ -28,6 +28,7 @@ func Open() (string, func(), error) { if err != nil { return "", nil, xerrors.Errorf("connect to ci postgres: %w", err) } + defer db.Close() dbName, err := cryptorand.StringCharset(cryptorand.Lower, 10) diff --git a/coderd/database/dump.sql b/coderd/database/dump.sql index 28adc8a36b1f1..9ec634cf79a2d 100644 --- a/coderd/database/dump.sql +++ b/coderd/database/dump.sql @@ -73,6 +73,11 @@ CREATE TYPE login_type AS ENUM ( COMMENT ON TYPE login_type IS 'Specifies the method of authentication. "none" is a special case in which no authentication method is allowed.'; +CREATE TYPE name_organization_pair AS ( + name text, + organization_id uuid +); + CREATE TYPE parameter_destination_scheme AS ENUM ( 'none', 'environment_variable', diff --git a/coderd/database/migrations/000216_custom_role_pair_parameter.down.sql b/coderd/database/migrations/000216_custom_role_pair_parameter.down.sql new file mode 100644 index 0000000000000..7322a09ee26b8 --- /dev/null +++ b/coderd/database/migrations/000216_custom_role_pair_parameter.down.sql @@ -0,0 +1 @@ +DROP TYPE name_organization_pair; diff --git a/coderd/database/migrations/000216_custom_role_pair_parameter.up.sql b/coderd/database/migrations/000216_custom_role_pair_parameter.up.sql new file mode 100644 index 0000000000000..b131054fc8dfb --- /dev/null +++ b/coderd/database/migrations/000216_custom_role_pair_parameter.up.sql @@ -0,0 +1 @@ +CREATE TYPE name_organization_pair AS (name text, organization_id uuid); diff --git a/coderd/database/querier_test.go b/coderd/database/querier_test.go index c3e1f2e46b3db..0d523c25290e2 100644 --- a/coderd/database/querier_test.go +++ b/coderd/database/querier_test.go @@ -6,6 +6,7 @@ import ( "context" "database/sql" "encoding/json" + "fmt" "sort" "testing" "time" @@ -14,6 +15,7 @@ import ( "github.com/stretchr/testify/require" "github.com/coder/coder/v2/coderd/database" + "github.com/coder/coder/v2/coderd/database/db2sdk" "github.com/coder/coder/v2/coderd/database/dbgen" "github.com/coder/coder/v2/coderd/database/dbtime" "github.com/coder/coder/v2/coderd/database/migrations" @@ -514,6 +516,234 @@ func TestDefaultOrg(t *testing.T) { require.True(t, all[0].IsDefault, "first org should always be default") } +// TestReadCustomRoles tests the input params returns the correct set of roles. +func TestReadCustomRoles(t *testing.T) { + t.Parallel() + + if testing.Short() { + t.SkipNow() + } + + sqlDB := testSQLDB(t) + err := migrations.Up(sqlDB) + require.NoError(t, err) + + db := database.New(sqlDB) + ctx := testutil.Context(t, testutil.WaitLong) + + // Make a few site roles, and a few org roles + orgIDs := make([]uuid.UUID, 3) + for i := range orgIDs { + orgIDs[i] = uuid.New() + } + + allRoles := make([]database.CustomRole, 0) + siteRoles := make([]database.CustomRole, 0) + orgRoles := make([]database.CustomRole, 0) + for i := 0; i < 15; i++ { + orgID := uuid.NullUUID{ + UUID: orgIDs[i%len(orgIDs)], + Valid: true, + } + if i%4 == 0 { + // Some should be site wide + orgID = uuid.NullUUID{} + } + + role, err := db.UpsertCustomRole(ctx, database.UpsertCustomRoleParams{ + Name: fmt.Sprintf("role-%d", i), + OrganizationID: orgID, + }) + require.NoError(t, err) + allRoles = append(allRoles, role) + if orgID.Valid { + orgRoles = append(orgRoles, role) + } else { + siteRoles = append(siteRoles, role) + } + } + + // normalizedRoleName allows for the simple ElementsMatch to work properly. + normalizedRoleName := func(role database.CustomRole) string { + return role.Name + ":" + role.OrganizationID.UUID.String() + } + + roleToLookup := func(role database.CustomRole) database.NameOrganizationPair { + return database.NameOrganizationPair{ + Name: role.Name, + OrganizationID: role.OrganizationID.UUID, + } + } + + testCases := []struct { + Name string + Params database.CustomRolesParams + Match func(role database.CustomRole) bool + }{ + { + Name: "NilRoles", + Params: database.CustomRolesParams{ + LookupRoles: nil, + ExcludeOrgRoles: false, + OrganizationID: uuid.UUID{}, + }, + Match: func(role database.CustomRole) bool { + return true + }, + }, + { + // Empty params should return all roles + Name: "Empty", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{}, + ExcludeOrgRoles: false, + OrganizationID: uuid.UUID{}, + }, + Match: func(role database.CustomRole) bool { + return true + }, + }, + { + Name: "Organization", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{}, + ExcludeOrgRoles: false, + OrganizationID: orgIDs[1], + }, + Match: func(role database.CustomRole) bool { + return role.OrganizationID.UUID == orgIDs[1] + }, + }, + { + Name: "SpecificOrgRole", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{ + { + Name: orgRoles[0].Name, + OrganizationID: orgRoles[0].OrganizationID.UUID, + }, + }, + }, + Match: func(role database.CustomRole) bool { + return role.Name == orgRoles[0].Name && role.OrganizationID.UUID == orgRoles[0].OrganizationID.UUID + }, + }, + { + Name: "SpecificSiteRole", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{ + { + Name: siteRoles[0].Name, + OrganizationID: siteRoles[0].OrganizationID.UUID, + }, + }, + }, + Match: func(role database.CustomRole) bool { + return role.Name == siteRoles[0].Name && role.OrganizationID.UUID == siteRoles[0].OrganizationID.UUID + }, + }, + { + Name: "FewSpecificRoles", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{ + { + Name: orgRoles[0].Name, + OrganizationID: orgRoles[0].OrganizationID.UUID, + }, + { + Name: orgRoles[1].Name, + OrganizationID: orgRoles[1].OrganizationID.UUID, + }, + { + Name: siteRoles[0].Name, + OrganizationID: siteRoles[0].OrganizationID.UUID, + }, + }, + }, + Match: func(role database.CustomRole) bool { + return (role.Name == orgRoles[0].Name && role.OrganizationID.UUID == orgRoles[0].OrganizationID.UUID) || + (role.Name == orgRoles[1].Name && role.OrganizationID.UUID == orgRoles[1].OrganizationID.UUID) || + (role.Name == siteRoles[0].Name && role.OrganizationID.UUID == siteRoles[0].OrganizationID.UUID) + }, + }, + { + Name: "AllRolesByLookup", + Params: database.CustomRolesParams{ + LookupRoles: db2sdk.List(allRoles, roleToLookup), + }, + Match: func(role database.CustomRole) bool { + return true + }, + }, + { + Name: "NotExists", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{ + { + Name: "not-exists", + OrganizationID: uuid.New(), + }, + { + Name: "not-exists", + OrganizationID: uuid.Nil, + }, + }, + }, + Match: func(role database.CustomRole) bool { + return false + }, + }, + { + Name: "Mixed", + Params: database.CustomRolesParams{ + LookupRoles: []database.NameOrganizationPair{ + { + Name: "not-exists", + OrganizationID: uuid.New(), + }, + { + Name: "not-exists", + OrganizationID: uuid.Nil, + }, + { + Name: orgRoles[0].Name, + OrganizationID: orgRoles[0].OrganizationID.UUID, + }, + { + Name: siteRoles[0].Name, + }, + }, + }, + Match: func(role database.CustomRole) bool { + return (role.Name == orgRoles[0].Name && role.OrganizationID.UUID == orgRoles[0].OrganizationID.UUID) || + (role.Name == siteRoles[0].Name && role.OrganizationID.UUID == siteRoles[0].OrganizationID.UUID) + }, + }, + } + + for _, tc := range testCases { + tc := tc + + t.Run(tc.Name, func(t *testing.T) { + t.Parallel() + + ctx := testutil.Context(t, testutil.WaitLong) + found, err := db.CustomRoles(ctx, tc.Params) + require.NoError(t, err) + filtered := make([]database.CustomRole, 0) + for _, role := range allRoles { + if tc.Match(role) { + filtered = append(filtered, role) + } + } + + a := db2sdk.List(filtered, normalizedRoleName) + b := db2sdk.List(found, normalizedRoleName) + require.Equal(t, a, b) + }) + } +} + type tvArgs struct { Status database.ProvisionerJobStatus // CreateWorkspace is true if we should create a workspace for the template version diff --git a/coderd/database/queries.sql.go b/coderd/database/queries.sql.go index 677f972e734c3..b7e4ed58d35cb 100644 --- a/coderd/database/queries.sql.go +++ b/coderd/database/queries.sql.go @@ -5604,20 +5604,20 @@ FROM custom_roles WHERE true - -- Lookup roles filter expects the role names to be in the rbac package - -- format. Eg: name[:] - AND CASE WHEN array_length($1 :: text[], 1) > 0 THEN - -- Case insensitive lookup with org_id appended (if non-null). - -- This will return just the name if org_id is null. It'll append - -- the org_id if not null - concat(name, NULLIF(concat(':', organization_id), ':')) ILIKE ANY($1 :: text []) + -- @lookup_roles will filter for exact (role_name, org_id) pairs + -- To do this manually in SQL, you can construct an array and cast it: + -- cast(ARRAY[('customrole','ece79dac-926e-44ca-9790-2ff7c5eb6e0c')] AS name_organization_pair[]) + AND CASE WHEN array_length($1 :: name_organization_pair[], 1) > 0 THEN + -- Using 'coalesce' to avoid troubles with null literals being an empty string. + (name, coalesce(organization_id, '00000000-0000-0000-0000-000000000000' ::uuid)) = ANY ($1::name_organization_pair[]) ELSE true END - -- Org scoping filter, to only fetch site wide roles + -- This allows fetching all roles, or just site wide roles AND CASE WHEN $2 :: boolean THEN organization_id IS null ELSE true END + -- Allows fetching all roles to a particular organization AND CASE WHEN $3 :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN organization_id = $3 ELSE true @@ -5625,9 +5625,9 @@ WHERE ` type CustomRolesParams struct { - LookupRoles []string `db:"lookup_roles" json:"lookup_roles"` - ExcludeOrgRoles bool `db:"exclude_org_roles" json:"exclude_org_roles"` - OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` + LookupRoles []NameOrganizationPair `db:"lookup_roles" json:"lookup_roles"` + ExcludeOrgRoles bool `db:"exclude_org_roles" json:"exclude_org_roles"` + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` } func (q *sqlQuerier) CustomRoles(ctx context.Context, arg CustomRolesParams) ([]CustomRole, error) { diff --git a/coderd/database/queries/roles.sql b/coderd/database/queries/roles.sql index dd8816d40eecc..ec5566a3d0dbb 100644 --- a/coderd/database/queries/roles.sql +++ b/coderd/database/queries/roles.sql @@ -5,26 +5,27 @@ FROM custom_roles WHERE true - -- Lookup roles filter expects the role names to be in the rbac package - -- format. Eg: name[:] - AND CASE WHEN array_length(@lookup_roles :: text[], 1) > 0 THEN - -- Case insensitive lookup with org_id appended (if non-null). - -- This will return just the name if org_id is null. It'll append - -- the org_id if not null - concat(name, NULLIF(concat(':', organization_id), ':')) ILIKE ANY(@lookup_roles :: text []) + -- @lookup_roles will filter for exact (role_name, org_id) pairs + -- To do this manually in SQL, you can construct an array and cast it: + -- cast(ARRAY[('customrole','ece79dac-926e-44ca-9790-2ff7c5eb6e0c')] AS name_organization_pair[]) + AND CASE WHEN array_length(@lookup_roles :: name_organization_pair[], 1) > 0 THEN + -- Using 'coalesce' to avoid troubles with null literals being an empty string. + (name, coalesce(organization_id, '00000000-0000-0000-0000-000000000000' ::uuid)) = ANY (@lookup_roles::name_organization_pair[]) ELSE true END - -- Org scoping filter, to only fetch site wide roles + -- This allows fetching all roles, or just site wide roles AND CASE WHEN @exclude_org_roles :: boolean THEN organization_id IS null ELSE true END + -- Allows fetching all roles to a particular organization AND CASE WHEN @organization_id :: uuid != '00000000-0000-0000-0000-000000000000'::uuid THEN organization_id = @organization_id ELSE true END ; + -- name: UpsertCustomRole :one INSERT INTO custom_roles ( diff --git a/coderd/database/sqlc.yaml b/coderd/database/sqlc.yaml index ff8faf5f7704c..67d7e52b06b6d 100644 --- a/coderd/database/sqlc.yaml +++ b/coderd/database/sqlc.yaml @@ -28,6 +28,10 @@ sql: emit_enum_valid_method: true emit_all_enum_values: true overrides: + # Used in 'CustomRoles' query to filter by (name,organization_id) + - db_type: "name_organization_pair" + go_type: + type: "NameOrganizationPair" - column: "custom_roles.site_permissions" go_type: type: "CustomRolePermissions" diff --git a/coderd/database/types.go b/coderd/database/types.go index 5d0490d0c9020..fd7a2fed82300 100644 --- a/coderd/database/types.go +++ b/coderd/database/types.go @@ -3,6 +3,7 @@ package database import ( "database/sql/driver" "encoding/json" + "fmt" "time" "github.com/google/uuid" @@ -142,3 +143,30 @@ func (a CustomRolePermission) String() string { } return str } + +// NameOrganizationPair is used as a lookup tuple for custom role rows. +type NameOrganizationPair struct { + Name string `db:"name" json:"name"` + // OrganizationID if unset will assume a null column value + OrganizationID uuid.UUID `db:"organization_id" json:"organization_id"` +} + +func (*NameOrganizationPair) Scan(_ interface{}) error { + return xerrors.Errorf("this should never happen, type 'NameOrganizationPair' should only be used as a parameter") +} + +// Value returns the tuple **literal** +// To get the literal value to return, you can use the expression syntax in a psql +// shell. +// +// SELECT ('customrole'::text,'ece79dac-926e-44ca-9790-2ff7c5eb6e0c'::uuid); +// To see 'null' option. Using the nil uuid as null to avoid empty string literals for null. +// SELECT ('customrole',00000000-0000-0000-0000-000000000000); +// +// This value is usually used as an array, NameOrganizationPair[]. You can see +// what that literal is as well, with proper quoting. +// +// SELECT ARRAY[('customrole'::text,'ece79dac-926e-44ca-9790-2ff7c5eb6e0c'::uuid)]; +func (a NameOrganizationPair) Value() (driver.Value, error) { + return fmt.Sprintf(`(%s,%s)`, a.Name, a.OrganizationID.String()), nil +} diff --git a/coderd/rbac/rolestore/rolestore.go b/coderd/rbac/rolestore/rolestore.go index 083f03877aa83..80cbd1165073b 100644 --- a/coderd/rbac/rolestore/rolestore.go +++ b/coderd/rbac/rolestore/rolestore.go @@ -69,11 +69,34 @@ func Expand(ctx context.Context, db database.Store, names []string) (rbac.Roles, } if len(lookup) > 0 { + // The set of roles coming in are formatted as 'rolename[:]'. + // In the database, org roles are scoped with an organization column. + lookupArgs := make([]database.NameOrganizationPair, 0, len(lookup)) + for _, name := range lookup { + roleName, orgID, err := rbac.RoleSplit(name) + if err != nil { + continue + } + + parsedOrgID := uuid.Nil // Default to no org ID + if orgID != "" { + parsedOrgID, err = uuid.Parse(orgID) + if err != nil { + continue + } + } + + lookupArgs = append(lookupArgs, database.NameOrganizationPair{ + Name: roleName, + OrganizationID: parsedOrgID, + }) + } + // If some roles are missing from the database, they are omitted from // the expansion. These roles are no-ops. Should we raise some kind of // warning when this happens? dbroles, err := db.CustomRoles(ctx, database.CustomRolesParams{ - LookupRoles: lookup, + LookupRoles: lookupArgs, ExcludeOrgRoles: false, OrganizationID: uuid.Nil, })