ash-project / ash-project/ash_postgres
Relationships are able to cross tenants with attribute strategy
- Dominant language
- Elixir
- Stars
- 189
- Forks
- 168
- Avg merge
- 13h 1m
- Merged PRs (30d)
- 22
Description
**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](https://github.com/rbino/ash_postgres/commit/d8f62872e94bb8e7e115d7e24ec715142ffe4142#diff-ee89cab7243923108e4ded2df70b2b46b17b4acbedfadf03af83c35944d4a04cR24) 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](https://hexdocs.pm/ecto/multi-tenancy-with-foreign-keys.html#working-with-multi-tenant-associations) 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 0adec1d1634387367bb818e40711e34ceb8cb3e4.
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.
Contributor guide
Research direction
Start by running the two cross-tenant relationship tests introduced in commit d8f62872e94bb8e7e115d7e24ec715142ffe4142, then compare the behavior with the change in 0adec1d1634387367bb818e40711e34ceb8cb3e4. Read the Ecto multi-tenancy foreign-key guidance and trace the migration generation involved. Done means primary-key relationships reject cross-tenant inserts at the database level without breaking the case addressed by #144.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- elixir, postgresql
- Domain
- databases
- Issue type
- Bug
- Difficulty
- 4/5
- Estimated time
- 3-5 days
- Activity status
- Quiet
- Clarity
- Mostly clear
- Newbie friendliness
- 48/100