Check for grant permission before adding database user, fixes #13628

[thomasheller]

Fixes #13628

[MorrisJobke]

Test failures are unrelated.

[MorrisJobke]

Code change looks good 👍

[kesselb]

server/lib/private/Setup/MySQL.php

Lines 164 to 168 in 2e36069

	$this->logger->logException($ex, [
	'message' => 'Can not create a new MySQL user, will continue with the provided user.',
	'level' => ILogger::INFO,
	'app' => 'mysql.setup',
	]);

I use a user oc_admin with usage permission only and run into the above exception and no other user is created. What issue does this pr fix?

[kesselb]

Granted oc_admin the permission to create a user. Work as expected (still running into the exception).

Can reproduce your case if oc_admin has the permission to read mysql.user. This looks more like a configuration issue? Why should oc_admin read mysql.user?

[kesselb]

I'm not sure about this "fix". If you setup a user for nextcloud yourself (without global permissions) but grant the right to create a user and select permission for mysql.user you run into an invalid case (and this case is fixed by this pr). Not really related to this pr but it's questionable why there is some "create custom user, grant permission stuff" at all. Setup a database should be done before installing nextcloud. This is sysadmin stuff like setting memory limit, etc.

[thomasheller]

I referenced the bug description #13628 -- which includes a link to a post where another user ran into the problem -- in the PR description. Sorry I didn't make that more obvious. Please let me know if you need any additional information.

Rationale for the PR: "Home users" (contrasting with professional sysadmins) are one of Nextcloud's user groups mentioned specifically on the public website: https://nextcloud.com/athome/ So I'd say Nextcloud should be robust enough to handle environments set up by semi-professionals.

From a technical perspective, the current code is kind of deceptive: "this should be enough to check for admin rights in mysql" This simply doesn't hold true. Any database user can query the mysql.user table, if permissions are sufficient. That doesn't necessarily mean that the user is an "admin".

[kesselb]

Nextcloud should be robust enough to handle environments set up by semi-professionals.

No. Nextcloud should provide a good setup guide and advice semi-professionals howto setup a secure system. Magic is not the right approach here. There a many of these discussions here on github because nextcloud does sysadmin stuff (e.g. change memory limits, set headers, etc...)

From a technical perspective, the current code is kind of deceptive: "this should be enough to check for admin rights in mysql" This simply doesn't hold true. Any database user can query the mysql.user table, if permissions are sufficient. That doesn't necessarily mean that the user is an "admin".

What is the point of a setup like this? On a shared hosting select on mysql.user is bad. I'm not against this pr but for me it looks like a configuration issue. If you create a dedicated user for nextcloud without permission for mysql.user everything work as expected.

[thomasheller]

Nextcloud should provide a good setup guide and advice semi-professionals howto setup a secure system. Magic is not the right approach here.

Well, by that logic, that whole "Nextcloud magically creates its own database if it can, but uses the specified database if it can't" thing should go away entirely. Instead, Nextcloud would try to use the specified database, regardless of admin privileges. That would also be a clean way of doing it -- leaving the choice of running Nextcloud via a privileged database user or not to the hopefully informed user. So what I'm saying is that if Nextcloud tries to check permissions, it should do so in a thorough and robust way.

[kesselb]

Well, by that logic, that whole "Nextcloud magically creates its own database if it can, but uses the specified database if it can't" thing should go away entirely. Instead, Nextcloud would try to use the specified database, regardless of admin privileges.

That would be my preferred approach.

So what I'm saying is that if Nextcloud tries to check permissions, it should do so in a thorough and robust way.

I see your point but i doubt that your special edge case (a dedicated user for nextcloud with permission to read mysql.user) is true for a lot of people out there. Why would someone setup something like this?

Note: I'm not blocking this pr. Technically it works and for this edge case it makes sense.

[thomasheller]

That would be my preferred approach.

I agree that simplifying the database setup would have its pros. But I'm not sure if that should be discussed (and possibly changed) in the scope of this fix? Sure that would also remedy the original problem described in #13628, but I don't know if there are any other side effects to consider.

Why would someone setup something like this?

The point is, it happens, even if only by accident. It's actually described this way in some popular tutorials, just as an example: https://www.digitalocean.com/community/tutorials/how-to-create-a-new-user-and-grant-permissions-in-mysql (from 2012, but linked in newer tutorials, and ranking well on search engines -- maybe they'll change it now if someone reads this comment... 😉)

[ChristophWurst]

@thomasheller can we get this in? Otherwise I would suggest to close beacuse it's been since the last activity.