When looking up a role in the database, it requires aname andorganization_id. Before this change I was usingname[:<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 thisrolestore.Expand():

coder/coderd/rbac/rolestore/rolestore.go

Lines 89 to 102 ina1a42b8

	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 rightValue() function to return a tuple literal (go does not have native tuples).

coder/coderd/database/types.go

Lines 147 to 172 ina1a42b8

	// NameOrganizationPair is used as a lookup tuple for custom role rows.
	typeNameOrganizationPairstruct {
	Namestring`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 {
	returnxerrors.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 (aNameOrganizationPair)Value() (driver.Value,error) {
	returnfmt.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 ina1a42b8

CREATETYPEname_organization_pairAS (nametext, organization_id uuid);

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

coder/coderd/database/sqlc.yaml

Lines 32 to 34 ina1a42b8

	-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 ina1a42b8

	typeCustomRolesParamsstruct {
	LookupRoles []NameOrganizationPair`db:"lookup_roles" json:"lookup_roles"`
	ExcludeOrgRolesbool`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 ina1a42b8

funcTestReadCustomRoles(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.

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

8e98cea

github-actionsbot assignedEmyrk

Jun 5, 2024

Emyrk added6 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

coderd/database/queries/roles.sql

Comment on lines +12 to +13

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

Copy link

MemberAuthor

EmyrkJun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

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 frommafredri andcoadler

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

Copy link

Contributor

coadler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

coderd/database/types.go

		//
		//SELECT ARRAY[('customrole'::text,'ece79dac-926e-44ca-9790-2ff7c5eb6e0c'::uuid)];
		func (aNameOrganizationPair)Value() (driver.Value,error) {
		returnfmt.Sprintf(`(%s,%s)`,a.Name,a.OrganizationID.String()),nil

Copy link

Contributor

coadlerJun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

Copy link

MemberAuthor

EmyrkJun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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?

Copy link

Contributor

coadlerJun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

Ah, good point actually. This is fine, sry

Copy link

MemberAuthor

EmyrkJun 6, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others.Learn more.

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

ThedefaultValueConverter 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. 👍

Emyrk merged commite2b330f intomain

Jun 6, 2024

Emyrk deleted the stevenmasley/org_roles_query_args branch

June 6, 2024 20:36

github-actionsbot locked and limited conversation to collaborators

Jun 6, 2024

Labels

None yet

2 participants

Movatterモバイル変換

Navigation Menu

Search code, repositories, users, issues, pull requests...

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

Uh oh!

Conversation

Emyrk commentedJun 5, 2024•
edited
Loading

Uh oh!

What this does

How this is done in sqlc

Tested

DB Logging

Uh oh!

EmyrkJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

coadler left a comment

Choose a reason for hiding this comment

Uh oh!

coadlerJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

EmyrkJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

coadlerJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

EmyrkJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Movatterモバイル変換

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

Uh oh!

Conversation

Emyrk commentedJun 5, 2024• editedLoading Uh oh!There was an error while loading.Please reload this page.

Uh oh!

What this does

How this is done in sqlc

Tested

DB Logging

Uh oh!

EmyrkJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

coadler left a comment

Choose a reason for hiding this comment

Uh oh!

coadlerJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

EmyrkJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

coadlerJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

EmyrkJun 6, 2024

Choose a reason for hiding this comment

Uh oh!

Uh oh!

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 commentedJun 5, 2024•
edited
Loading