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.

Sign up for GitHub

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

Jump to bottom

Fix Schema Division Handling #955

Open
wants to merge 5 commits into
base: 5.x
Choose a base branch
from
Open

Fix Schema Division Handling #955

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
cf25621
Only create database schema if it doesn't already exist.
mikebronner Jul 28, 2020
0962897
Drop all tables from the schema automatically.
mikebronner Jul 28, 2020
b9524f9
Do not drop privileges when using schema.
mikebronner Jul 28, 2020
a505636
Update PostgreSQL.php
mikebronner Jul 28, 2020
7783adf
Merge branch '5.x' into patch-1
mikebronner Feb 14, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 7 additions & 2 deletions src/Generators/Webserver/Database/Drivers/PostgreSQL.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,6 +129,10 @@ protected function flushConnection(IlluminateConnection $connection, array $conf

protected function dropPriviliges(IlluminateConnection $connection, array $config)
{
if (config("tenancy.db.tenant-division-mode") === Connection::DIVISION_MODE_SEPARATE_SCHEMA) {
return true;
}
Comment on lines +132 to +134
Copy link
Member

Choose a reason for hiding this comment

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

Can you give a little insight into why we should not do this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my use-case I have only one database user, that is the main database user used on the System connection as well as all other Tenant connections. Removing privileges for a user would remove all the items they owned, regardless of schema they were in. That is dangerous, and in my case broke functionality.


if ($this->userExists($connection, $config['username'])) {
return $connection->statement("DROP OWNED BY \"{$config['username']}\"");
}
Expand All @@ -143,8 +147,9 @@ protected function dropDatabase(IlluminateConnection $connection, array $config)

protected function dropUser(IlluminateConnection $connection, array $config)
{
if (config('tenancy.db.auto-delete-tenant-database-user') && $this->userExists($connection,
$config['username'])) {
if (config('tenancy.db.auto-delete-tenant-database-user')
&& $this->userExists($connection, $config['username'])
) {
ArlonAntonius marked this conversation as resolved.
Show resolved Hide resolved
return $connection->statement("DROP USER \"{$config['username']}\"");
}

Expand Down
4 changes: 2 additions & 2 deletions src/Generators/Webserver/Database/Drivers/PostgresSchema.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ class PostgresSchema extends PostgreSQL
{
protected function createDatabase(IlluminateConnection $connection, array $config)
{
return $connection->statement("CREATE SCHEMA \"{$config['schema']}\"");
return $connection->statement("CREATE SCHEMA IF NOT EXISTS \"{$config['schema']}\"");
}

protected function grantPrivileges(IlluminateConnection $connection, array $config)
Expand Down Expand Up @@ -54,6 +54,6 @@ public function updated(Updated $event, array $config, Connection $connection):

protected function dropDatabase(IlluminateConnection $connection, array $config)
{
return $connection->statement("DROP SCHEMA IF EXISTS \"{$config['schema']}\"");
return $connection->statement("DROP SCHEMA IF EXISTS \"{$config['schema']}\" CASCADE");
Copy link
Member

Choose a reason for hiding this comment

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

Using CASCADE is quite a big danger it, as far as my PSQL knowledge goes, this would delete everything in the schema. I think it would be better to use RESTRICT if we should change this at all. I think RESTRICT is even the default.
If we change this, we could risk people ending up deleting entire schema's while that's something that they would not want.

Copy link
Contributor Author

@mikebronner mikebronner Jul 29, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

My thoughts on this method is that we shouldn't be executing this code if our intent is not to drop the schema. And if our intent is to do so, why wouldn't we follow through with it? The problem I ran into when not adding the CASCADE is that the schema would never be dropped automatically, because the tables it contained were never dropped, as I disabled blanked removal of privileges (see change above), which is very risky.

}
}