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

Storage.objects table should not use fk constraint with owners column on auth.users id column #276

Closed
GaryAustin1 opened this issue Feb 7, 2023 · 15 comments
Labels
bug Something isn't working

Comments

@GaryAustin1
Copy link

GaryAustin1 commented Feb 7, 2023

Bug report

Describe the bug

Currently there is a foreign key relation on objects owner column to auth.users id column that is not needed and causes constant problems for users and questions in Discord and Github.

I'm classifying this as a bug, not just an improvement, as it is a feature that seems to serve no useful purpose and causes users to try and implement delete cascade on storage.objects, just delete rows in storage.objects, and/or waste time before reaching out for help when deleting a user with an error (violating fk relation on storage objects) that they know nothing about.

The crux of the issue is that you can't just add a cascade delete to the objects table as then files are orphaned in storage.
You can't delete a user if there are files associated with that user in objects.

It is better to allow the user be deleted without error, than to have a customer not understand deleting the objects row does not remove the file and ( forever?) orphans the file in s3 storage. When the customer realizes there are orphaned rows in the objects table (no owner in auth.users) they can address the issue. Worst case is they have extra object table rows, versus orphaned s3 storage.

I believe currently the only way to deal with this is to have a delete trigger function on any table associated with a file and use the http extension to make a storage API delete call. This does NOT depend on the fk relation as a trigger is involved.

Not having the fk relation would also allow users to run a cron task to clean up orphaned files in the object table.

Another option is for Supabase to put cascade delete on the constraint AND make sure if an object row is deleted the appropriate trigger function is applied to delete the file from s3 storage.

To Reproduce

Add a file with a signed in user.
Then delete the user.

Expected behavior

Do not have an fk constraint.

Screenshots

System information

Additional context

Sorry for classifying as bug, but this has caused many issues that are unnecessary for users, IMO.
Here is a long time issue on this: #65
But this comes up weekly in support forums.

@GaryAustin1 GaryAustin1 added the bug Something isn't working label Feb 7, 2023
@wdavidw
Copy link
Contributor

wdavidw commented Feb 7, 2023

Related to #65

@GaryAustin1
Copy link
Author

GaryAustin1 commented Feb 8, 2023

Here is an example of a user trying to use storage with another auth system. This MIGHT work if owner was not tied to auth.users with an fk.
https://discord.com/channels/839993398554656828/1072718211134533682

Got the actual error from the user and this actually shows removing the fk would not solve the use of objects for all jwt's as the sub still has to be a UUID type or you get a postgres error when storage tries to insert the non uuid sub claim into owner column.

@jacobcoro
Copy link

Ended up here as well. Please come up with a more user-friendly way of deleting users! Thanks for your hard work, supabase rocks!

@ericrav
Copy link

ericrav commented Mar 1, 2023

Here is an example of a user trying to use storage with another auth system. This MIGHT work if owner was not tied to auth.users with an fk.
https://discord.com/channels/839993398554656828/1072718211134533682

I got a similar error to this while trying to use storage with a JWT from a custom auth system. Except my sub was a valid UUID (but not in auth.users), so I got this cryptic 404 error "The parent resource is not found".

I had to remove sub from my JWT to make storage uploads work. Discussion here: https://discord.com/channels/839993398554656828/1006358244786196510

@Lewitje
Copy link

Lewitje commented Mar 1, 2023

Having the same issue, can't delete a user because they uploaded an image

@GaryAustin1
Copy link
Author

Having the same issue, can't delete a user because they uploaded an image

Well, if you don't want to leave their images around then you have to deal with that anyway in a trigger function or delete their images first as part of your delete user process.

If you don't mind their images staying then yes this fk prevents that. You could null all owner columns with their UUID in trigger function in that case.

@imagitama
Copy link

I also have a custom auth system with my own JWTs and my user IDs are not UUID and instead are TEXT (leftover from migrating from Firebase). To upload any files I had to remove the foreign key so I could change the owner column type from UUID => TEXT but this will break anytime you guys role out an update. :(

@magnusrodseth
Copy link

magnusrodseth commented Apr 8, 2023

@imagitama Could you specify exactly how you converted the owner column in storage.objects from uuid -> text? I use Clerk as my authentication provider, and I have the exact same issue.

I tried to run the following in Supabase's SQL editor:

alter table storage.objects alter column owner type text;

However, I got the following error:

Failed to run sql query: foreign key constraint "objects_owner_fkey" cannot be implemented

@imagitama
Copy link

@magnusrodseth You must delete that foreign key constraint. The constraint between the tables means both colums need the same type so you can't have it.

ALTER TABLE storage.objects DROP CONSTRAINT objects_owner_fkey;

Note that if/when Supabase perform a system update they may reinstate the column type and constraint again. 🤷

@Zhumatic
Copy link

Zhumatic commented May 5, 2023

When using next auth with supabase adapter. If supabase is created with passing authorization header instructed by the supabase adapter docs like this:
const { supabaseAccessToken } = session; supabaseRLS = createClient( process.env.NEXT_PUBLIC_SUPABASE_URL, process.env.NEXT_PUBLIC_SUPABASE_ANON_KEY, { global: { headers: { Authorization: Bearer ${supabaseAccessToken}, }, }, } );
Then using this client to perform storage upload resulted in error: insert or update on table "objects" violates foreign key constraint "objects_owner_fkey". Then I tried client without passing the hearders, upload function works again.

I guess it's the "constraint objects_owner_fkey foreign key (owner) references auth.users (id)" in objects table causing this error? as next_auth being used, auth.users cannot be accessed. Guessing modify this line to "constraint objects_owner_fkey foreign key (owner) references next_auth.users (id)" will work?

@GaryAustin1
Copy link
Author

@Zhumatic it is risky to modify the storage schema. Supabase can change it at any point with a migration.

@bnjmnt4n
Copy link
Contributor

The foreign key constraint was removed in #330, so this issue and #65 can be closed.

@Zhumatic
Copy link

@GaryAustin1 didn't know that, thanks for your reminder.

@bombillazo
Copy link

Can confirm there is no longer a FK constriant on the objects table.

@HamedMP
Copy link

HamedMP commented Aug 29, 2023

Funny enough I selected all FKs and the object fk was still there.

WITH foreign_keys AS (
  SELECT
    conname,
    conrelid,
    confrelid,
    unnest(conkey)  AS conkey,
    unnest(confkey) AS confkey
  FROM pg_constraint
  WHERE contype = 'f' -- AND confrelid::regclass = 'your_table'::regclass
)
SELECT
  fk.conname as constraint_name,
  fk.conrelid::regclass as referencing_table, a.attname as fkcol,
  fk.confrelid::regclass as referenced_table, af.attname as ukcol
FROM foreign_keys fk
JOIN pg_attribute af ON af.attnum = fk.confkey AND af.attrelid = fk.confrelid
JOIN pg_attribute a ON a.attnum = conkey AND a.attrelid = fk.conrelid
ORDER BY fk.conrelid, fk.conname;

image

My issue was on using Clerk instead of Supabase auth. (a bit different but still the same)
After hours of debugging and testing every possible way, this ended up working:

  1. Select Storage from Table Editor
    image

  2. Change the column type of owner to text instead of uuid.
    image

  3. Set correct RLS Policies

  4. Voila! 🥳

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