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

Emyrk · 2024-06-05T20:40:03Z

What this does

When looking up a role in the database, it requires a name and organization_id. Before this change I was using name[:<org_id] as a comparison string, but this is obviously not ideal.

I found a way to instead pass a slice of (name, org_id) tuples as the search filter. This allows 1 query to grab all org roles across all orgs, and site roles in 1 query.

This is used in APIKeyMW when pulling a user's roles via this rolestore.Expand():

coder/coderd/rbac/rolestore/rolestore.go

Lines 89 to 102 in a1a42b8

    
           	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:     lookupArgs, 
        
           	ExcludeOrgRoles: false, 
        
           	OrganizationID:  uuid.Nil, 
        
           })

How this is done in sqlc

Create a custom type with the right Value() function to return a tuple literal (go does not have native tuples).

coder/coderd/database/types.go

Lines 147 to 172 in a1a42b8

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

Define the type in a migration, so that SQLc and Postgres know the type:

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

Line 1 in a1a42b8

CREATE TYPE name_organization_pair AS (name text, organization_id uuid);

Do an override in sqlc.yaml because SQLc does not have the functionality to interpret these types:

coder/coderd/database/sqlc.yaml

Lines 32 to 34 in a1a42b8

    
           - db_type: "name_organization_pair" 
        
             go_type: 
        
               type: "NameOrganizationPair"

The result is a really nice auto-generated param

coder/coderd/database/queries.sql.go

Lines 5627 to 5631 in a1a42b8

    
           type CustomRolesParams struct { 
        
           	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"` 
        
           }

Tested

A series of tests to verify it all works:

coder/coderd/database/querier_test.go

Line 520 in a1a42b8

func TestReadCustomRoles(t *testing.T) {

DB Logging

DB logging was super helpful doing this: a305e70. I reverted the commit because it brings in an extra dependency. We should consider keeping something like this.

Emyrk · 2024-06-06T00:08:31Z

coderd/database/queries/roles.sql

+    -- 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[])


Maybe we can bring NULL back, but for the life of me I could not get it to work. I think it has to do with my understanding of representing NULL as a literal

coadler

Definitely seems like an improvement 👍, don't really have any issues

coadler · 2024-06-06T20:19:26Z

coderd/database/types.go

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


For the name here probably want to use https://pkg.go.dev/github.com/lib/pq#QuoteLiteral unless I'm misunderstanding. This seems like direct sql injection.

It cannot be quoted, I was trying that before. This is a parameter argument still, so it will replace ?#.

I had considered a sql injection, but can a sql injection happen at a parameter substitution?

Ah, good point actually. This is fine, sry

I just tried to send in some invalid sql with -- and ; as the names, and the query still works.

The defaultValueConverter which implments this function for all primitive types just does the literal string value without quotes or escapes: https://github.com/golang/go/blob/master/src/database/sql/driver/types.go#L291-L293

That is the function behavior I am overriding. 👍

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

8e98cea

github-actions bot assigned Emyrk Jun 5, 2024

Emyrk added 6 commits June 5, 2024 16:17

WIP

4356f53

got it working

7f37232

revert logging

a305e70

fixups

8da3601

fixup null

dc7fe77

fixup sqlc

a1a42b8

Emyrk commented Jun 6, 2024

View reviewed changes

Emyrk marked this pull request as ready for review June 6, 2024 00:08

Emyrk changed the title ~~chore: sql parameter to custom roles to be a (name,org) tuple~~ chore: change sql parameter for custom roles to be a (name,org) tuple Jun 6, 2024

Emyrk changed the title ~~chore: change sql parameter for custom roles to be a (name,org) tuple~~ chore: change sql parameter for custom roles to be a (name,org) tuple Jun 6, 2024

Emyrk requested review from mafredri and coadler June 6, 2024 00:13

Emyrk mentioned this pull request Jun 6, 2024

chore: implement audit log for custom role edits #13494

Merged

coadler approved these changes Jun 6, 2024

View reviewed changes

Emyrk merged commit e2b330f into main Jun 6, 2024
42 checks passed

Emyrk deleted the stevenmasley/org_roles_query_args branch June 6, 2024 20:36

github-actions bot locked and limited conversation to collaborators Jun 6, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

Emyrk commented Jun 5, 2024 •

edited

Loading

Emyrk Jun 6, 2024

coadler left a comment

coadler Jun 6, 2024

Emyrk Jun 6, 2024

coadler Jun 6, 2024

Emyrk Jun 6, 2024

	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: lookupArgs,
	ExcludeOrgRoles: false,
	OrganizationID: uuid.Nil,
	})

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

	- db_type: "name_organization_pair"
	go_type:
	type: "NameOrganizationPair"

	type CustomRolesParams struct {
	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"`
	}

		-- 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[])

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

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

Conversation

Emyrk commented Jun 5, 2024 • edited Loading

What this does

How this is done in sqlc

Tested

DB Logging

Emyrk Jun 6, 2024

Choose a reason for hiding this comment

coadler left a comment

Choose a reason for hiding this comment

coadler Jun 6, 2024

Choose a reason for hiding this comment

Emyrk Jun 6, 2024

Choose a reason for hiding this comment

coadler Jun 6, 2024

Choose a reason for hiding this comment

Emyrk Jun 6, 2024

Choose a reason for hiding this comment

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

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

Emyrk commented Jun 5, 2024 •

edited

Loading