Fix: Fixing all the issues related to assigning privileges to databases with '_' #17015

iifawzi · 2021-07-15T22:49:28Z

Signed-off-by: Fawzi E. Abdulfattah iifawzie@gmail.com

Description

This PR fixes all the bugs discussed at #17010 and the bug mentioned in #16994.

Fixes:

The grant/revoke queries will be escaped once - Previously, when single db is selected, no escaping at all, and when multiple db are selected got escaped twice -
All the UI that's responsible to show the databases names are unescaped for the user to see the actual names.
All the links to edit/revoke are fixed.
Multiple select list will only show the databases that aren't already granted privileges.

williamdes · 2021-07-16T00:46:18Z

Hi @iifawzi
Your work is great, that said there is something wrong
Let me explain: you need to show the escaping if there is some, but not escape the escaping as we can see when granting permission

Currently you removed all escaping, this is not right because as you can see on my example a_% is a wildcard example. And using your PR it would prevent distinguishing between literal char and escaped

So you need to add back escaping on DB list, edit page. That said the queries look good !

I have a theory about why there is two \ used, maybe it is to escape the \ at the insert moment, that would explain the actual GUI

mysql.db

Show dbs before

Show dbs after

iifawzi · 2021-07-16T05:35:21Z

I can see that in your mysql.db table, the a_% and a_hidden are not escaped, I think this's incorrect, and this's the actual bug at QA_5_1 #17010 (comment), right? if this screenshot is taken from using this PR implementation, there's something wrong then, but i'm not able to re-produce such a case, did you remove all the grants that were made before testing this PR?

a_% is a wildcard example. And using your PR it would prevent distinguishing between literal char and escaped

How can I encounter such an issue? what do you mean by distinguishing between literal char and escaped?

This PR implementation is suppose to have this:

a_hidden db, should be escaped in the query to be a\_hidden, added to the mysql.db table as the same as shown in the query a\_hidden, and shown in the GUIs as a_hidden
a_% db, should be escaped in the query to be a\_%, added to the mysql.db table as the same as shown in the query a\_%, and shown in the GUIs as a_%

are we ok with this? can you give me a case that might prevent this from happening or make a conflict with it in the GUI?

I'm asking because, if the queries look good, so it means the GUI looks good I think, because i'm just unescaping the name used in the query, and used it in the GUI, if the. GUI can be misleading in any case, so it means there will be something wrong in the queries I think. how can the query looks good, and the GUI have bugs?

It took too long I know, thank your for your patience, I just wanna be sure that I understood your points before adding back the escaping on DB list, edit page GUIs.

Thank you again.

williamdes · 2021-07-16T22:26:47Z

I can see that in your mysql.db table, the a_% and a_hidden are not escaped, I think this's incorrect,

No, it's indended and is a wildcard example

How can I encounter such an issue? what do you mean by distinguishing between literal char and escaped?

You can grant a user the use of a_foo% using the box after the list of tables, If I am right it will mean a any char foo any string

I am not sure if this is more clear, but there is a case where _ means one char and \_ means literal _

williamdes · 2021-07-16T22:27:48Z

So you only need to escape on database lists add to make _ chars escaped, but if the user enters _ in the box then keep it not escaped :)

iifawzi · 2021-07-16T22:43:32Z

You can grant a user the use of a_foo% using the box after the list of tables, If I am right it will mean a any char foo any string

Ohhh... I got it. Nice! I never thought that this's possible.

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>

iifawzi

I've tested the mentioned cases in the PR's description and it worked, and the GUI now is working as expected, Hopefully :d.

iifawzi · 2021-07-17T01:00:28Z

libraries/classes/Server/Privileges.php

@@ -514,8 +514,7 @@ public function getSqlQueryForDisplayPrivTable($db, $table, $username, $hostname
            return 'SELECT * FROM `mysql`.`db`'
                . " WHERE `User` = '" . $this->dbi->escapeString($username) . "'"
                . " AND `Host` = '" . $this->dbi->escapeString($hostname) . "'"
-                . " AND '" . $this->dbi->escapeString(Util::unescapeMysqlWildcards($db)) . "'"
-                . ' LIKE `Db`;';
+                . " AND `Db` = '" . $this->dbi->escapeString($db) . "'";


I've encountered another issue regarding the privileges table, if we have a privileges on a_% and a_hidden.
before, if we tried to edit the privileges on a_hidden, the privileges of a_% will be shown instead. (can be tested on https://demo.phpmyadmin.net/master/ ), I've fixed it by looking for an exact match instead of using LIKE, and removed the unescaping function, I've tested all the cases that crossed my mind they worked as expected, I hope that I'm not missing anything.

williamdes

This all looks very nice, I will test it !

williamdes

Awesome !
It fixes all bugs

williamdes · 2021-07-20T20:27:50Z

Just found something ( #16477 is back)

Missing a tab

iifawzi · 2021-07-21T01:50:16Z

Hmm, I think based on #16477 (comment), the issue was due to considering the underscore as wildcard - the bug mentioned here #17010 (comment) -
since this commit fixed #17010 (comment), so #16477 is fixed too, here's an example granting a permissions on db with underscore bar_1_prod

The issue you've mentioned is regarding granting a permissions on a wildcard example like a_% or bar_1_prod, logically it shouldn't be allowed I think, we have no actual tables. I've also tested in https://demo.phpmyadmin.net/master/index.php and it have thrown an error table doesn't exist, which make sense I think.

the mess comes from that when #16481 is introduced, this bug #17010 (comment) wasn't fixed. which leaded to allowing the table tab to appear even for wildcards DBs a_% or bar_1_prod, which's is incorrect as per my understand, I might be missing something though

williamdes · 2021-07-21T15:03:49Z

@sudo-robot deploy

williamdes · 2021-07-21T16:04:29Z

Hmm, you are right. This was probably a bug, I could not replicate it on the deployed server.
And yes it makes sense that you can not add DBs to a wildcard privilege but that you can on a normal one.
Probably I made a mistake in my testing, I will let you know ;)

iifawzi · 2021-07-21T16:08:58Z

let me know if you've found anything that could be fixed, and thank you for following up with this, your guidance, and your fast replies @williamdes, You're encouraging me to keep contributing

williamdes · 2021-07-21T16:26:23Z

let me know if you've found anything that could be fixed, and thank you for following up with this, your guidance, and your fast replies @williamdes, You're encouraging me to keep contributing

I am so happy to have so great and involved contributions ! ❤️

Conclusion: it works better when I use an escaped version 🤦🏻

Pull-request: #17015 Ref: #16994 Signed-off-by: William Desportes <williamdes@wdes.fr>

Signed-off-by: William Desportes <williamdes@wdes.fr>

williamdes · 2021-07-21T16:44:56Z

✅ Tests fixed: 25c2cfe
✅ And one new test: 6d5c5e6

ℹ️ You can run tests with ./vendor/bin/phpunit --no-coverage --exclude-group=selenium

williamdes self-assigned this Jul 16, 2021

williamdes added this to the 5.1.2 milestone Jul 16, 2021

williamdes added this to In progress in pull-requests via automation Jul 16, 2021

williamdes moved this from In progress to Review in progress in pull-requests Jul 16, 2021

Fixing all the issues related to assigning privileges

68981b7

Signed-off-by: Fawzi E. Abdulfattah <iifawzie@gmail.com>

iifawzi force-pushed the fix-privileges-issues branch from abb6e03 to 68981b7 Compare July 17, 2021 00:49

iifawzi commented Jul 17, 2021

View reviewed changes

williamdes approved these changes Jul 20, 2021

View reviewed changes

pull-requests automation moved this from Review in progress to Reviewer approved Jul 20, 2021

williamdes approved these changes Jul 20, 2021

View reviewed changes

williamdes added a commit that referenced this pull request Jul 21, 2021

Add ChangeLog entries for #16994

fe95e6e

Pull-request: #17015 Ref: #16994 Signed-off-by: William Desportes <williamdes@wdes.fr>

williamdes merged commit ad5aad6 into phpmyadmin:QA_5_1 Jul 21, 2021

pull-requests automation moved this from Reviewer approved to Done Jul 21, 2021

williamdes added a commit that referenced this pull request Jul 21, 2021

Fix tests after #17015

25c2cfe

Signed-off-by: William Desportes <williamdes@wdes.fr>

williamdes added a commit that referenced this pull request Jul 21, 2021

Add tests after #17015

6d5c5e6

Signed-off-by: William Desportes <williamdes@wdes.fr>

williamdes mentioned this pull request Jun 19, 2022

Backslash being added to database names when granting privileges to a user #17612

Open

phpmyadmin deleted a comment from sudo-robot Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix: Fixing all the issues related to assigning privileges to databases with '_' #17015

Fix: Fixing all the issues related to assigning privileges to databases with '_' #17015

iifawzi commented Jul 15, 2021 •

edited

williamdes commented Jul 16, 2021

iifawzi commented Jul 16, 2021 •

edited

williamdes commented Jul 16, 2021

williamdes commented Jul 16, 2021

iifawzi commented Jul 16, 2021

iifawzi left a comment

iifawzi Jul 17, 2021 •

edited

williamdes left a comment

williamdes left a comment

williamdes commented Jul 20, 2021 •

edited

iifawzi commented Jul 21, 2021

williamdes commented Jul 21, 2021

williamdes commented Jul 21, 2021

iifawzi commented Jul 21, 2021

williamdes commented Jul 21, 2021

williamdes commented Jul 21, 2021

Fix: Fixing all the issues related to assigning privileges to databases with '_' #17015

Fix: Fixing all the issues related to assigning privileges to databases with '_' #17015

Conversation

iifawzi commented Jul 15, 2021 • edited

Description

williamdes commented Jul 16, 2021

mysql.db

Show dbs before

Show dbs after

iifawzi commented Jul 16, 2021 • edited

williamdes commented Jul 16, 2021

williamdes commented Jul 16, 2021

iifawzi commented Jul 16, 2021

iifawzi left a comment

Choose a reason for hiding this comment

iifawzi Jul 17, 2021 • edited

Choose a reason for hiding this comment

williamdes left a comment

Choose a reason for hiding this comment

williamdes left a comment

Choose a reason for hiding this comment

williamdes commented Jul 20, 2021 • edited

iifawzi commented Jul 21, 2021

williamdes commented Jul 21, 2021

williamdes commented Jul 21, 2021

iifawzi commented Jul 21, 2021

williamdes commented Jul 21, 2021

williamdes commented Jul 21, 2021

iifawzi commented Jul 15, 2021 •

edited

iifawzi commented Jul 16, 2021 •

edited

iifawzi Jul 17, 2021 •

edited

williamdes commented Jul 20, 2021 •

edited