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

Adding the system table for row level security #1746

Merged
merged 6 commits into from
Oct 11, 2024
Merged

Conversation

mamcx
Copy link
Contributor

@mamcx mamcx commented Sep 26, 2024

Description of Changes

NOTE: The design changes after several meetings with the team to improve the ergonomics of module compilation.

Add a new system table st_row_level_security to store the SQL queries for row-level security.

This is a first step to add support for authorization. This PR doesn't validate that the queries are valid, that will done when #1602 is implemented.

Closes #1600.

Expected complexity level and risk

2: It adds a new system table and because we haven't yet migrated support for them, it needs a recreate of the db.

Testing

  • Add extra test for RLS
  • Because we don't have any RLS in the initial bootstrap I also add a test for reload after creating one
  • Check using the SQL cli the new table

@mamcx mamcx added abi-break A PR that makes an ABI breaking change release-1.0 labels Sep 26, 2024
@mamcx mamcx self-assigned this Sep 26, 2024
Copy link
Contributor

@kazimuth kazimuth left a comment

Choose a reason for hiding this comment

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

Looks great. We've ended up with a lot of boxes to check when adding stuff to the schema, good job navigating all that. Maybe we should document the process somewhere.

You may also want to add row-level-security to crates/schema/src/auto_migrate.rs. It seems to me that arbitrary changes to row-level security can be performed automatically.

@mamcx
Copy link
Contributor Author

mamcx commented Oct 2, 2024

It seems to me that arbitrary changes to row-level security can be performed automatically.

If we need to check the validity of the sql then probably not.

Copy link
Collaborator

@joshua-spacetime joshua-spacetime left a comment

Choose a reason for hiding this comment

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

My main questions are:

  1. Can we get rid of the name column? I believe we can.
  2. Do we need a primary key column? I believe we don't.

@mamcx mamcx force-pushed the mamcx/rls_system_table branch from 5c23955 to 3085324 Compare October 8, 2024 18:20
@mamcx mamcx force-pushed the mamcx/rls_system_table branch 2 times, most recently from fea437d to 44aec4f Compare October 8, 2024 18:38
@mamcx mamcx force-pushed the mamcx/rls_system_table branch from ef7b76a to 250da82 Compare October 9, 2024 18:35
@mamcx mamcx force-pushed the mamcx/rls_system_table branch from 250da82 to 89b1722 Compare October 10, 2024 15:01
@mamcx
Copy link
Contributor Author

mamcx commented Oct 11, 2024

My main questions are:

  1. Can we get rid of the name column? I believe we can.
  2. Do we need a primary key column? I believe we don't.

Yes, is already done

@mamcx mamcx added this pull request to the merge queue Oct 11, 2024
Merged via the queue into master with commit b1b58ac Oct 11, 2024
8 checks passed
@mamcx mamcx mentioned this pull request Oct 14, 2024
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
abi-break A PR that makes an ABI breaking change release-1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RLS: Store module defined RLS queries in system table
3 participants