chore: change sql parameter for custom roles to be a (name,org_id) tuple (#13480)

* chore: sql parameter to custom roles to be a (name,org) tuple

CustomRole lookup takes (name,org_id) tuples as the search criteria.

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)
 			}
 		})
 	}

coderd/database/dbmem/dbmem.go

@@ -1187,12 +1187,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
 			}

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)

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',

coderd/database/migrations/000216_custom_role_pair_parameter.down.sql

@@ -0,0 +1 @@
+DROP TYPE name_organization_pair;

coderd/database/migrations/000216_custom_role_pair_parameter.up.sql

@@ -0,0 +1 @@
+CREATE TYPE name_organization_pair AS (name text, organization_id uuid);

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

coderd/database/queries.sql.go

@@ -5623,20 +5623,20 @@ FROM
 	custom_roles
 WHERE
   true
-  -- Lookup roles filter expects the role names to be in the rbac package
-  -- format. Eg: name[:<organization_id>]
-  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
@@ -5644,9 +5644,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) {

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[:<organization_id>]
-  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 (

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"

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
+}