Skip to content

feature: Add CRUD permissions #491

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

Closed
wants to merge 21 commits into from
Closed

feature: Add CRUD permissions #491

wants to merge 21 commits into from

Conversation

ruggi99
Copy link
Contributor

@ruggi99 ruggi99 commented Jan 29, 2023

What kind of change does this PR introduce?

Feature.
Add CRUD table and column permission to postgres-meta.
Should allow an admin to list, grant and revoke permissions on single tables and columns of a table.
Needed to add in future column permission to Supabase Dashboard.
This requires a little discussion on variables naming, routes and so on.

I took the sql from this SELECT definition FROM pg_views WHERE viewname = 'column_privileges' and adapted it.

TODO list:

  • Add CRUD table permission to lib
  • Add column permission to lib
  • Add CRUD table permission to server
  • Add column permission to server
  • Eventually add read-only merged permissions

Waiting for feedback

What is the current behavior?

Please link any relevant issues here.

What is the new behavior?

Feel free to include screenshots if it includes visual changes.

Additional context

Closes #489

@ruggi99
Copy link
Contributor Author

ruggi99 commented Jan 31, 2023

@sweatybridge @soedirgo what are your initial thoughts?

@ruggi99 ruggi99 marked this pull request as ready for review February 4, 2023 08:10
@ruggi99 ruggi99 requested a review from a team as a code owner February 4, 2023 08:10
@soedirgo
Copy link
Member

soedirgo commented Feb 7, 2023

Sorry @ruggi99, currently occupied with other stuff - will come back to this once I have some bandwidth.

@ruggi99
Copy link
Contributor Author

ruggi99 commented Feb 7, 2023

Yeah thanks.
I think it needs some other work that I will do in my free time

@ruggi99 ruggi99 changed the title feature: Add column permission feature: Add CRUD permissions Feb 9, 2023
@ruggi99
Copy link
Contributor Author

ruggi99 commented Feb 15, 2023

@soedirgo any ETA on this for at least a first feedback?
I don't like having opened PRs that are pending due to others bandwidth constraints or other problems related.
If not I will temporary close this PR.

@ruggi99 ruggi99 closed this Feb 16, 2023
@kiwicopple kiwicopple reopened this Feb 16, 2023
@kiwicopple
Copy link
Member

hey @ruggi99 - this is a great PR, so I'm going to re-open it. We're a bit blocked, but we definitely need this functionality

If you prefer, I can migrate the PR over to myself

@kiwicopple
Copy link
Member

one thing that could be useful is to add tests. this repo is well-tested, and we'd love to keep the coverage high given the function this is serving

@ruggi99
Copy link
Contributor Author

ruggi99 commented Feb 16, 2023

Hi @kiwicopple ,
no problem in keeping it open, at least I know it will be reviewed one day.

Yeah I know that this PR is lacking tests but I'm not so good at writing them.
At least I can try in the spare time and push if I reach a good point.
But I think one of the team should take care of writing tests when reviewing this.
Sorry about that.

Copy link
Member

not so good at writing them

Use ChatGPT 😛 . Seriously though - no obligation, it's awesome that you've gone to the effort of doing this.

We'll merge this if after we have some passing tests 👍

@jdgamble555
Copy link

Hey guys, anyone looking at accepting this? This would be a huge feature to help with security!

Thanks!

J

@alaister
Copy link
Member

alaister commented Apr 7, 2023

@jdgamble555 yep, these are features that are great for pg-meta! We'll aim to get this in after launch week.

@MentalGear
Copy link

This is a much needed feature, e.g. for letting users update certain aspects of their table row, but not all.
ATM I'm not sure how to disallow a user to update his "role" with RLS/supabase only - is this something that can just be done in middleware/custom backend code? (maybe there is a supabase guide no this, but searching the docs there isn't)

@jdgamble555
Copy link

Hey @alaister, hopefully you guys are working on it this week 😃 I was thinking due to the way the grants will be, it is probably best to release this so that it works on local and cloud.

J

@jdgamble555
Copy link

Hey @alaister, are you guys able to get to this this week?

Thanks!

J

@ibanks42
Copy link

Any update on this?

@ruggi99 ruggi99 closed this Jul 1, 2023
@daKmoR
Copy link

daKmoR commented Jul 3, 2023

wooowwww I never knew I needed this 😅

just saw https://dev.to/jdgamble555/supabase-needs-column-level-security-170a and now I want it 🤣

@ruggi99
Copy link
Contributor Author

ruggi99 commented Jul 3, 2023

Hi all,
I simply closed (again) this PR because there's no interest from Supabase team in reviewing this PR, even the first time. No discussion, no asking, no review.
I only heard things like promises but not even a useful comment by Supabase team.
One could argue that there were conflicts to be resolved but I did not resolve them because it would have been a waste of my time.
There are open PRs that are trying to do what I've done. Hope they review and merge them in the next few years (or more?)

@jdgamble555 sorry for not being active on comments but now you know why I did not interact with this PR too much

@jdgamble555
Copy link

jdgamble555 commented Jul 3, 2023

Got it. Sorry about that. I updated the article. I didn't realize you weren't from the Supabase Team. Hopefully they still add this feature in one form or another.

J

@soedirgo
Copy link
Member

soedirgo commented Jul 5, 2023

Sorry @ruggi99, we should've got back to you on this PR. We still want the feature, but there are differences on what we want the API to look like & the SQL queries used.

As you said, there are open PRs now that implement this, which are currently being reviewed:

@ruggi99
Copy link
Contributor Author

ruggi99 commented Jul 5, 2023

And that's why I'm not contributing back to Supabase anymore.
At least until you change the bad approach you have with external open-source contributors.

@ruggi99 ruggi99 deleted the add-column-permission branch July 5, 2023 07:26
@kiwicopple
Copy link
Member

Hey @ruggi99 - sorry for this, it looks like a comms problem from our side. As @soedirgo mentioned, we want this and we're using your PR as a baseline to implement the functionality

As you mentioned above, the supabase team still had some work to do with creating tests ("one of the team should take care of writing tests when reviewing this"). That's 100% understandable, but please also be understanding if we create separate PRs to implement the missing functionality or decide to make some tweaks along the way

This is a great contribution and we're appreciate, even if we did a poor job communicating that.

@ruggi99
Copy link
Contributor Author

ruggi99 commented Jul 7, 2023

Agree on what you've said. I'm pleased that you are taking my work to continue working on that and moving forward, but as stated now it's better to continue on separate PRs that include tests and other stuff.

There are open PRs that are trying to do what I've done

Written like this may sound a little bad but I only wanted to let know people that Supabase Team has taken in account the task by creating PRs that implement the same thing I wanted to implement. I'm not saying that you are stealing my code, ideas, ecc No problem on my side

@lt692
Copy link

lt692 commented Jan 4, 2024

I wonder what's the progress with this feature and when is it planned to be released? 🤔

@soedirgo
Copy link
Member

soedirgo commented Feb 1, 2024

Hey @lt692! This is now available as a feature preview: supabase/supabase#13745 (comment)

@lt692
Copy link

lt692 commented Feb 1, 2024

Already trying it out! 🫡 People will love it!

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.

Feature request: Add endpoint to CRUD column privileges
9 participants