fix #38749, postgresql GRANT user's permission after createDatabase, …

[whlsxl]

…ensure database exist when GRANT

• Resolves: [Bug]: fresh install with postgresql, occ maintenance:install error #38749

[come-nc]

I’m not familiar enough with databases to review this

[kesselb]

Thank you very much for your pull 👍

Could you please provide us some more information about your change?

I'm not familiar with PostgreSQL. Grant create does not work, before creating the database because the schema public does not exist?

[kesselb]

I think we should remove the code to create database tables and users.

[whlsxl]

Thank you very much for your pull 👍

Could you please provide us some more information about your change?

I'm not familiar with PostgreSQL. Grant create does not work, before creating the database because the schema public does not exist?

Sorry, not familiar with PHP.

This is the docker-compose.yml to reproduce this issue.

version: "3.5"

services:
  postgres:
    container_name: postgres
    image: postgres:15.3-alpine
    restart: unless-stopped
    environment:
      - "POSTGRES_USER=postgres"
      - "POSTGRES_PASSWORD=U8PNOtHPefQ"
      - "POSTGRES_HOST_AUTH_METHOD=trust"
  nextcloud:
    container_name: nextcloud
    image: nextcloud:26.0.2-fpm-alpine
    restart: unless-stopped

After container up, enter the container docker exec -it nextcloud sh & install nextcloud with
php occ maintenance:install --database pgsql --database-host postgres:5432 --database-name nextcloud --database-user postgres --database-pass U8PNOtHPefQ --admin-user admin --admin-pass U8PNOtHPefQ --data-dir /var/www/html/data/

got error:
PostgreSQL username and/or password not valid

[whlsxl]

I traced this bug.
The pull request #34645 is to fix the #34617, but I tried many ways, can't reproduce the problem.

I think we should remove the code to create database tables and users.

In my opinion, create users & database & tables are very good functions, because most users are newbies, they don't have the ability to manage the database.

But this function should be refactor, for example

server/lib/private/Setup/PostgreSQL.php

Lines 122 to 143 in 63bf207

	private function createDatabase(Connection $connection) {
	if (!$this->databaseExists($connection)) {
	//The database does not exists... let's create it
	$query = $connection->prepare("CREATE DATABASE " . addslashes($this->dbName) . " OWNER \"" . addslashes($this->dbUser) . '"');
	try {
	$query->execute();
	} catch (DatabaseException $e) {
	$this->logger->error('Error while trying to create database', [
	'exception' => $e,
	]);
	}
	} else {
	$query = $connection->prepare("REVOKE ALL PRIVILEGES ON DATABASE " . addslashes($this->dbName) . " FROM PUBLIC");
	try {
	$query->execute();
	} catch (DatabaseException $e) {
	$this->logger->error('Error while trying to restrict database permissions', [
	'exception' => $e,
	]);
	}
	}
	}

if the database exists, the superuser should resign the owner to nextcloud database user and copy all the permission nextcloud database user.

I can do this job, but still try to figure out how to write the TEST & execute it.

[kesselb]

In my opinion, create users & database & tables are very good functions, because most users are newbies, they don't have the ability to manage the database.

I would prefer that newbies learn how to create a database and database user ;)

We have automated tests for PostgreSQL.

https://github.com/nextcloud/.github/blob/dc0440094e086858cb960e0054a29ca565c5fbbd/workflow-templates/phpunit-pgsql.yml#L49-L52

The notable difference:

• PostgreSQL 14 vs 15
• POSTGRES_DB (we ask the container to create a database named nextcloud)

That means in our automated tests, we do not run the code to create a database table. This code path is apparently broken.

Previous: Doctrine\DBAL\Exception: Failed to connect to the database: An exception occurred in the driver: SQLSTATE[08006] [7] FATAL:  database "nextcloud" does not exist
Trace: #0 /var/www/html/lib/private/Setup/PostgreSQL.php(114): OC\DB\Connection->connect()
#1 /var/www/html/lib/private/Setup.php(353): OC\Setup\PostgreSQL->setupDatabase('admin')

The stack trace is a bit misleading.

server/lib/private/Setup/PostgreSQL.php

Line 65 in eb7b682

$connectionMainDatabase = $this->connect();

Here is the problem. connect() uses this->dbName by default. That means we tell doctrine/pdo to connect to a non-existing database.

$connectionMainDatabase = $this->connect([
    'dbname' => 'postgres'
]);

We need to overwrite the dbname (like for $connection a few lines above) and hope that the postgres table will be there.

[whlsxl]

This bug still exists, wish someone could review it.

[whlsxl]

Can you please adjust your commits to comply with conventional commits? Thank you :)

Are you referring to the commit message? can you tell me more specifically?

[susnux]

Are you referring to the commit message? can you tell me more specifically?

Yes the commit message, we try to follow the conventional commits spec: https://www.conventionalcommits.org/

Some examples:

• fix: Something that was wrong
• fix(DB): Something that was wrong in database
• feat: Something new

[skjnldsv]

Rebased and reviewed!
this issue was a bit old and staled, but this should be simple enough to get merged 🤞

Thanks for the hard work @whlsxl 💪

[welcome]

Thanks for your first pull request and welcome to the community! Feel free to keep them coming! If you are looking for issues to tackle then have a look at this selection: https://github.com/nextcloud/server/issues?q=is%3Aopen+is%3Aissue+label%3A%22good+first+issue%22