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

feat: add config to allow managing policies on tables #85

Merged
merged 6 commits into from
Apr 4, 2024

Conversation

soedirgo
Copy link
Member

@soedirgo soedirgo commented Apr 3, 2024

What kind of change does this PR introduce?

Feature

What is the current behavior?

Only table owners can create RLS policies. This is limiting because e.g. for Supabase Storage we want users to be able to create policies on certain Storage tables but not allow them to perform all DDL on them (which tables owners can do).

What is the new behavior?

Decouple the privileges for managing a table's policies from the table's ownership. With supautils.policy_grants, the privileged role can manage policies on a set of tables without being the table owner.

@soedirgo soedirgo requested a review from steve-chavez April 3, 2024 03:19
Copy link

@darora darora left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add docs on how this is going to work?
e.g. do we get to specify which tables and users are in-scope for this?

@steve-chavez
Copy link
Member

Who creates Storage tables on Supabase? Is it the supabase_admin?

Copy link

@soedirgo
Copy link
Member Author

soedirgo commented Apr 3, 2024

👍 Will add the docs.

do we get to specify which tables and users

Yup, we get to specify which tables are in the allowlist, similar to privileged_role_allowed_configs. Users (non-superusers) can't change it. The extra privileges to manage the policies is only applicable to the privileged_role (postgres in our case).

Who creates Storage tables on Supabase?

The tables are created by supabase_storage_admin. For Realtime (which has a similar requirement), they're created by supabase_admin.

src/utils.c Outdated Show resolved Hide resolved
src/supautils.c Outdated Show resolved Hide resolved
src/utils.c Outdated Show resolved Hide resolved
src/supautils.c Outdated Show resolved Hide resolved
src/supautils.c Show resolved Hide resolved
README.md Outdated
Comment on lines 182 to 197
## Managing Policies Without Table Ownership

In Postgres, only table owners can create RLS policies for a table. This can be
limiting if you need to allow certain roles to manage policies without allowing
them to perform other DDL (e.g. to prevent them from dropping the table).

With supautils, this can be done like so:

```
supautils.privileged_role = 'my_privileged_role'
supautils.privileged_role_allow_policies_on_tables = 'public.not_my_table, public.also_not_my_table'
```

This allows `my_privileged_role` to manage policies for `public.not_my_table`
and `public.also_not_my_table` without being an owner of these tables.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thinking more about this. What you really want is something like GRANT CREATE POLICY ON TABLE TO <role>. Maybe this feature can be done with a single config.

Copy link
Member Author

@soedirgo soedirgo Apr 3, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup exactly, all this isn't needed if POLICY is a table privilege, that way we can decouple it from table ownership.

This is already a single config though, the privileged_role is a config we already use for other things (e.g. privileged_role_allowed_configs)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, cool. So since supautils adds security mechanisms that aren't in postgres yet, I think the proper design for this is:

supautils.grant_create_policy = '{"role1": ["table1", "table2"]}'

This way it's a generic solution and not just Supabase-specific.

(I know it's not exactly GRANT CREATE POLICY since it's global but it's the best we can do without creating tables)

Also done this way later:

  • We won't cause a breaking change
  • We can use this flexibility if the need arises

@soedirgo
Copy link
Member Author

soedirgo commented Apr 3, 2024

Updated the PR to use a more generic supautils.policy_grants (since we also allow alter policy and drop policy)

nix/withTmpDb.sh.in Outdated Show resolved Hide resolved
@soedirgo soedirgo merged commit 7a546bd into master Apr 4, 2024
6 checks passed
@soedirgo soedirgo deleted the feat/privileged-role-allow-policies-on-tables branch April 4, 2024 17:19
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.

5 participants