Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Multi tenant, an elegant and easy way #14

Closed
frederikhors opened this issue May 30, 2021 · 12 comments
Closed

Multi tenant, an elegant and easy way #14

frederikhors opened this issue May 30, 2021 · 12 comments

Comments

@frederikhors
Copy link

frederikhors commented May 30, 2021

Now that you have refactored a lot of things in go-pg and Bun is born, I think it's the right time to ask you something that I have long wanted to solve easily with go-pg.

I'm talking about the filter with tenant_id for tenantable models.

Example:

If Player is tenantable (has Tenant field):

type Tenant struct {
  ID int
  Name string
}

type Player struct {
  Tenant
  name string
  age int
}

I would like to add on each query a where filter like:

SELECT * FROM players where name LIKE 'John'
AND where tenant_id = 123 -- this is automagically added

123 here comes from user's tenant_id column.

I saw that there are a lot of hooks available but not the one before a Select which is what we need here.

Can you suggest an elegant way?

Since you also worked on a solution here: https://github.com/elliotcourant/go-pg-multitenant, I would like to hear your opinion on Bun too, @elliotcourant.

@elliotcourant
Copy link
Contributor

I will definitely be moving a lot of my stuff over from go-pg to bun, I'll probably make another repo with the implementation that I use as a boilerplate.

@vmihailenco
Copy link
Member

Such an approach only works for a single struct, not a slice. Why should we support the first and ignore the latter?

Overall it is not clear why make tenant filter so special. I write filters like this and tenant filter is just one of many.

@frederikhors
Copy link
Author

Overall it is not clear why make tenant filter so special.

What we're asking you since go-pg is a "global" and "safe" method that does not allow human errors (nobody can forget to filter by tenant_id).

@elliotcourant
Copy link
Contributor

I mean to be fair, multi tenant implementations are so unique from application to application it's very hard for the ORM itself to properly implement such a system that adequately serves each use case; each use case being good and valid.

It is often better to implement multi-tenant safety at the database level with something like row level security. Or by building a layer on top of the ORM you are using and making that layer a common interface throughout your code.

@frederikhors
Copy link
Author

frederikhors commented Jun 1, 2021

@elliotcourant any idea on how to use row level security with go-pg or bun? Can we set tenant_id on each connection?

@elliotcourant
Copy link
Contributor

Any orm that is pooling connections is not a good mix for row level security in my opinion. The safest way I've seen it done is to authenticate postgres using a user that only has access to that user's rows. That can be an expensive operation for applications that need to create many sessions to serve data. It might be possible to set a variable's value at the beginning of each session and then enforcing the row level security based on that variable, but then you risk the same problem; if you forget to set the variable in one spot -> now someone is reading someone else's data. But you have significantly overcomplicated your code in the process.

Row level security is best implemented where a connection is for a single tenant and thus there is no risk of it being misused. The connection is established for that tenant's sole user within postgres, and closed at the end of the work so that it is not re-used by a different tenant.

@vmihailenco do you want to move this do discussions?

@frederikhors
Copy link
Author

Long time ago I asked this go-gorm/gorm#3832.

@adpande
Copy link

adpande commented Jun 4, 2021

To handle multi-tenancy using postgres RLS
I think, if we set a key value like ("pgenv": [map of env variables] ) that we need to set in postgres environment at runtime using "SET" keyword in the context itself (https://golang.org/src/context/context.go?s=16244:16304#L509) and then pass the context along, and then in driver.go exec function we check if "pgenv" key is present in the coming context, if yes then get the map iterate over it and set the values before actually executing the query.

pgEnv := map[string]string{"tenant_id":"tenant_1" , "user_role": "admin"}
ctx := context.WithValue(context.Background(), "pgenv", pgEnv)
err := db.NewSelect().Model(book).Column("title", "text").Where("id = ?", 1).Scan(ctx)

Then in https://github.com/uptrace/bun/blob/0c18420f6be6b7188849ed24cdcc486a324176c5/driver/pgdriver/driver.go

We can check if the context have a value named "pgenv", if present then iterate over it and run the SET statements just before the execution of query.

  • I am new to golang

@frederikhors
Copy link
Author

frederikhors commented Jun 4, 2021

@adpande I love you! ❤️

@vmihailenco, @elliotcourant what do you think about?

@vmihailenco
Copy link
Member

It is possible to add BeforeSelectQueryHook and AfterSelectQueryHook that can change queries based on data in context

type BeforeSelectQueryHook interface {
	BeforeSelectQuery(ctx context.Context, query *SelectQuery) error
}

type AfterSelectQueryHook interface {
	AfterSelectQuery(ctx context.Context, query *SelectQuery) error
}

I personally think that such design brings more problems than it solves but I can live with it.

I mean to be fair, multi tenant implementations are so unique from application to application it's very hard for the ORM itself to properly implement such a system that adequately serves each use case; each use case being good and valid.

👍 I am even more skeptical and believe that it is better to write a little bit more code and repeat yourself here and there rather than relying on hooks & hacks.

(nobody can forget to filter by tenant_id)

You are lucky if this is the only / top10 problem in your app. It is easy to spot and even easier to fix...

@vmihailenco
Copy link
Member

Hooks are re-worked in v0.2.0 - https://bun.uptrace.dev/guide/hooks.html#model-query-hooks. This example demonstrates how to modify query based on values in context.

@vmihailenco
Copy link
Member

Closing since I don't know what else can be done here. Also see this disclaimer regarding hooks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants