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

[4.x] Postgres RLS support #1105

Closed
wants to merge 222 commits into from
Closed

[4.x] Postgres RLS support #1105

wants to merge 222 commits into from

Conversation

lukinovec
Copy link
Contributor

@lukinovec lukinovec commented Apr 21, 2023

(Note before merging: sync the Post/Comment/ScopedComment changes – use the dedicated model files.)

Implementation
To make Postgres RLS work with Tenancy, we create and use policies for scoping the queries performed on the tables of tenant-related models using the CreateRLSPoliciesForTenantTables command. The models are discovered in directories configured in tenancy.rls.model_directories (+ their subdirectories). Models are considered tenant-related if they belong to tenant directly (= they're using the BelongsToTenant trait), or indirectly (= they're using the BelongsToPrimaryModel).

To scope the queries by the current tenant using policies, the tenants also need Postgres users. We create them using the CreatePostgresUserForTenant job – the job also gives all the permissions to the user, but by changing the tenancy.rls.user_permissions config, you can choose the exact permissions you want to grant. Postgres users have the ID of the tenants they belong to as their names by default, e.g. a tenant with ID 'acme' will have a Postgres user named 'acme', or you can use a custom username by setting the internal db_username property on tenants (tenancy_db_username by default). The policies check if the tenant foreign key of the queried records (or their parent/primary models) is the same as the name of the current Postgres user.

The DeleteTenantsPostgresRole job deletes the tenant's Postgres user along with the user's permissions (Postgres user can't get deleted if he still has related objects, e.g. permissions).

Usage

Note: Your app has to use single-database tenancy

First, enable PostgresRLSBootstrapper. Then, uncomment the jobs in TenancyServiceProvider. After that, specify the directories of your models in the Tenancy config (tenancy.rls.model_directories) and make sure the DB is migrated. Finally, run php artisan tenants:create-rls-policies.

Note that tenants:create-rls-policies only creates policies for tables that are discovered in the configured directories and belong to tenants (directly, or through a primary model).

From now on, when tenants get created, we also create a Postgres user for them. You can use the tenants:create-postgres-user command to create Postgres users for the existing tenants.

To scope models using RLS, either:

  • set config('tenancy.rls.enabled') to true – queries of all tenant-related models will be scoped using RLS
  • make models that belong to tenants implement the RlsModel interface – queries of all models that use the BelongsToTenant and implement the RlsModel interface will get scoped using RLS

@codecov-commenter
Copy link

codecov-commenter commented Apr 21, 2023

Codecov Report

Merging #1105 (a664860) into master (bd9bbe8) will increase coverage by 0.25%.
The diff coverage is 94.89%.

❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more.

@@             Coverage Diff              @@
##             master    #1105      +/-   ##
============================================
+ Coverage     89.85%   90.11%   +0.25%     
- Complexity      585      617      +32     
============================================
  Files           139      145       +6     
  Lines          1794     1891      +97     
============================================
+ Hits           1612     1704      +92     
- Misses          182      187       +5     
Impacted Files Coverage Δ
src/Commands/CreatePostgresUserForTenants.php 0.00% <0.00%> (ø)
src/Tenancy.php 89.70% <ø> (ø)
src/Database/Concerns/DealsWithModels.php 94.44% <94.44%> (ø)
...strappers/Integrations/PostgresRLSBootstrapper.php 100.00% <100.00%> (ø)
src/Commands/CreateRLSPoliciesForTenantTables.php 100.00% <100.00%> (ø)
src/Database/Concerns/BelongsToTenant.php 100.00% <100.00%> (ø)
src/Jobs/CreatePostgresUserForTenant.php 100.00% <100.00%> (ø)
src/Jobs/DeleteTenantsPostgresUser.php 100.00% <100.00%> (ø)
src/TenancyServiceProvider.php 97.53% <100.00%> (+0.06%) ⬆️

@lukinovec lukinovec requested a review from stancl April 21, 2023 13:51
@stancl stancl linked an issue Apr 24, 2023 that may be closed by this pull request

/** @var TenantWithDatabase $tenant */
$this->config->set([
'database.connections.' . $centralConnection . '.username' => $tenant->database()->getUsername() ?? $tenant->getTenantKey(),
Copy link
Member

Choose a reason for hiding this comment

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

What happens when ?? $tenant->getTenantKey() is used?

Comment on lines 24 to 25
// If 'tenancy.rls.enabled' is true or this model implements RLSModel
// Scope queries using Postgres RLS instead of TenantScope
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// If 'tenancy.rls.enabled' is true or this model implements RLSModel
// Scope queries using Postgres RLS instead of TenantScope
// If 'tenancy.rls.enabled' is true or this model implements RLSModel
// Postgres RLS is used for scoping, so we don't enable the scope used with single-database tenancy.

Also just to confirm — this means that if tenancy.rls.enabled is used, individual models don't need to implement RLSModel, and RLS is used as the default "strategy" for scoping models that use this trait, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that's correct

Comment on lines 49 to 55
/**
* Filter all models retrieved by static::getModels() to get only the models that belong to tenants.
*/
public static function getTenantModels(): array
{
return array_filter(static::getModels(), fn (Model $model) => static::modelBelongsToTenant($model) || static::modelBelongsToTenantIndirectly($model));
}
Copy link
Member

Choose a reason for hiding this comment

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

Is this used only with RLS? If so, should it also check for whether models implement the interface RLS interface/whether RLS is used globally in config?

Copy link
Contributor Author

@lukinovec lukinovec Jan 31, 2024

Choose a reason for hiding this comment

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

It is used only with RLS (so far), but I don't think we have to make it specifically an RLS thing. The method just retrieves models that should belong to tenants, so I'd leave it as is

@stancl
Copy link
Member

stancl commented Feb 11, 2024

Resubmitted here https://github.com/tenancy-for-laravel/v4/pull/33

You can ignore the remaining reviews here, I'll resubmit all relevant ones in the new PR

@stancl stancl closed this Feb 11, 2024
@stancl stancl deleted the add-postgres-rls-support branch March 13, 2024 16:51
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.

Add support for Postgres RLS
4 participants