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

Relationships are able to cross tenants with attribute strategy #192

Open
rbino opened this issue Jan 13, 2024 · 1 comment
Open

Relationships are able to cross tenants with attribute strategy #192

rbino opened this issue Jan 13, 2024 · 1 comment
Labels
bug Something isn't working

Comments

@rbino
Copy link
Contributor

rbino commented Jan 13, 2024

Describe the bug
Ash Postgres currently allows relating two resources from two different tenants when using attribute strategy and referencing the primary key of the other resource.

To Reproduce
Here is a commit that adds two tests that manage cross-tenant relationships.

The one that references the primary key is able to add a cross-tenant relationship, the other correctly fails (with a rather cryptic error message though, so I'm not sure if it's actually failing for the right reason).

Expected behavior
I would expect cross-tenant relationship not to be possible

Runtime

  • Elixir version: 1.15.4
  • Erlang version: 26.0.2
  • OS: Debian Testing
  • Ash Postgres version: main

Additional context
The Ecto guide covers this usecase by adding composite foreign keys to disallow inserting cross-tenant associations.

This is what happens for relationships referencing non-primary key attributes, and it was happening also for primary key attributes before 0adec1d.

That commit was made to fix the error shown in #144, but I think that the fix shouldn't have been dropping the composite foreign key, but rather adding the extra [:id, :org_id] unique index in the migration generator (should this be added for all multitenant-attribute resources or just as soon as their primary key gets referenced for the first time?), as is also shown in the Ecto guide.

Of course the cross-tenant relationship is correctly filtered out when reading the resource so it can't be retrieved, but I think it would be better to disallow its creation completely at the database level given there's the possibility to do so.

@rbino rbino added bug Something isn't working needs review labels Jan 13, 2024
@zachdaniel
Copy link
Contributor

Yeah, I think that is a good idea 👍Will be a breaking change though, so will need some level of configuration backing it and/or to wait for a major release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants