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

Refactor Supabase plugin to avoid upsert operations to play better with RLS policies #329

Merged

Conversation

warrenbhw
Copy link
Contributor

@warrenbhw warrenbhw commented Jul 5, 2024

The PostgREST upsert endpoint (used by the supabase-js upsert function) interacts weirdly with some RLS policies.

Here's a simplified example of where things can go wrong:

I have a table notes with these columns:

  • id: uuid
  • content: text
  • workspace_id: uuid
  • deleted: boolean

I have an RLS policy that looks like this:

-- allow workspace members to select, insert, update, delete notes
create policy "Allow CRUD for workspace members" on public.notes
as permissive for all to authenticated
using ((workspace_id IN
    ( SELECT public.current_user_workspaces() AS current_user_workspaces)
))
with check ((workspace_id IN 
    ( SELECT public.current_user_workspaces() AS current_user_workspaces)
)));

if I do an upsert with this body:

{
  "id": "my-note-uuid",
  "deleted": true,
}

The RLS policy will fail, presumably because the logic in PostgREST leads to an attempt to check that the request meets RLS policy for both create AND update. This request doesn't pass the create branch of the check, because there is no workspace_id provided in the request.

The postgres plugin handles updates fine, because in that case, we send the entire record when we call upsert. However, on soft-deletes, we only send the id field and the deleted field, leading to this issue.

The two possible solutions are sending the entire record on delete, or splitting out creates and updates.

This seemed the cleaner and less error-prone avenue.

@warrenbhw
Copy link
Contributor Author

cc @jmeistrich - lmk your thoughts here. Should be a quick review, if you're on board with this.

@jmeistrich
Copy link
Contributor

@warrenbhw Yes, that looks perfect. I wasn't aware of that detail of RLS policies, very interesting! This is exactly the perfect fix, thank you!

Just one thing: the changes include bun.lockb which shouldn't have needed to change. Maybe it's just a difference in bun versions or something... I diffed them and it seems unchanged? But can you remove that file from the PR just to be safe?

@warrenbhw
Copy link
Contributor Author

@jmeistrich sure, will do

@warrenbhw
Copy link
Contributor Author

@warrenbhw Yes, that looks perfect. I wasn't aware of that detail of RLS policies, very interesting! This is exactly the perfect fix, thank you!

Just one thing: the changes include bun.lockb which shouldn't have needed to change. Maybe it's just a difference in bun versions or something... I diffed them and it seems unchanged? But can you remove that file from the PR just to be safe?

@jmeistrich lockfile change reverted. Mind approving the workflow and approving the PR? would love to land this today so I can start using it :)

@jmeistrich jmeistrich merged commit bea1e53 into LegendApp:main Jul 5, 2024
3 checks passed
@jmeistrich
Copy link
Contributor

@warrenbhw Released in alpha.25

@warrenbhw
Copy link
Contributor Author

sweet, thanks!

@warrenbhw warrenbhw deleted the ben/supabase-plugin-avoid-upsert branch July 5, 2024 20:14
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

Successfully merging this pull request may close these issues.

2 participants