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

DisableIS is broken on pma 4.9.2 with controluser configured #15608

Closed
thg2k opened this issue Dec 1, 2019 · 18 comments
Closed

DisableIS is broken on pma 4.9.2 with controluser configured #15608

thg2k opened this issue Dec 1, 2019 · 18 comments
Assignees
Labels
Bug A problem or regression with an existing feature has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete
Projects
Milestone

Comments

@thg2k
Copy link

thg2k commented Dec 1, 2019

Describe the bug

There is a problem when DisableIS cfg option is true, and there is a controluser configured.

After logging in as root and clicking on a database name on the navigation tree (not to the "+", directly to the name), it pops up the error "An error has occurred while loading the navigation display". At that point, refreshing the page causes all databases except information_schema, mysql, and performance_schema to disappear.

I tried further investigating the issue, it is probably hiding a more serious bug related to the management of links for control user and logged in user. In particular, the problem started to appear from commit b865011 (which was buggy but it doesn't matter for this issue), the relevant change is the behaviour of instance member _current_user, which is no longer cached on error. This means that while before it was always returning ["",""], now it starts returning ["root","%"] when clicking on the navigation tree, which in turns triggers the bug.

I'm afraid there is something wrong with the whole DatabaseInterface class, it seems it is mixing up the control user link and the logged in user.

Please let me know if you need further information as I spent several hours troubleshooting this.

Server configuration

  • Operating system: Fedora 31
  • Web server: Apache/2.4.41 (Fedora) OpenSSL/1.1.1d PHP/7.3.12
  • Database version: 10.3.18-MariaDB-log - MariaDB Server
  • PHP version: 7.3.12
  • phpMyAdmin version: 4.9.2
@williamdes
Copy link
Member

Thank you for the report @thg2k
can you send your config.inc.php file ?

@williamdes williamdes added Bug A problem or regression with an existing feature question Used when we need feedback from the submitter or when the issue is a question about PMA labels Dec 1, 2019
@williamdes williamdes added this to Needs triage in issues via automation Dec 1, 2019
@williamdes
Copy link
Member

Maybe grants of each user could be usefull

Please let me know if you need further information as I spent several hours troubleshooting this.

Thank you !

I'm afraid there is something wrong with the whole DatabaseInterface class, it seems it is mixing up the control user link and the logged in user.

Interesting, I need to reproduce the issue

@thg2k
Copy link
Author

thg2k commented Dec 1, 2019

@williamdes It is really easy to reproduce:

From an empty database, have root@% with password and phpmyadmin@% with database phpmyadmin and SELECT,INSERT,UPDATE,DELETE access (i don't think it really matters, i tried giving ALL privileges to phpmyadmin user, it's the same).

Config diff looks like this:

--- config.sample.inc.php       2019-12-01 19:43:47.179968547 +0000
+++ config.inc.php      2019-12-01 22:46:23.000000000 +0000
@@ -14,7 +14,7 @@
  * This is needed for cookie based authentication to encrypt password in
  * cookie. Needs to be 32 chars long.
  */
-$cfg['blowfish_secret'] = ''; /* YOU MUST FILL IN THIS FOR COOKIE AUTH! */
+$cfg['blowfish_secret'] = "REDACTED";
 
 /**
  * Servers configuration
@@ -31,6 +31,7 @@
 $cfg['Servers'][$i]['host'] = 'localhost';
 $cfg['Servers'][$i]['compress'] = false;
 $cfg['Servers'][$i]['AllowNoPassword'] = false;
+$cfg['Servers'][$i]['DisableIS'] = true;
 
 /**
  * phpMyAdmin configuration storage settings.
@@ -39,8 +40,8 @@
 /* User used to manipulate with storage */
 // $cfg['Servers'][$i]['controlhost'] = '';
 // $cfg['Servers'][$i]['controlport'] = '';
-// $cfg['Servers'][$i]['controluser'] = 'pma';
-// $cfg['Servers'][$i]['controlpass'] = 'pmapass';
+$cfg['Servers'][$i]['controluser'] = 'phpmyadmin';
+$cfg['Servers'][$i]['controlpass'] = "REDACTED";
 
 /* Storage database and tables */
 // $cfg['Servers'][$i]['pmadb'] = 'phpmyadmin';

@thg2k
Copy link
Author

thg2k commented Dec 1, 2019

The bug appears from the commit I mentioned till current QA_4_9 d8444c1

If you have problems triggering the bug I can give you access to my environment, just contact me privately

@williamdes
Copy link
Member

@thg2k okay, thank you so much
I will try to reproduce and fix it tomorrow if I find some time after work :)

@williamdes williamdes moved this from Needs triage to High priority in issues Dec 3, 2019
@thg2k
Copy link
Author

thg2k commented Dec 5, 2019

I suppose you managed to replicate the bug? Is it as bad as I thought, with the mix-up of the link/auth connection links?

@williamdes
Copy link
Member

@thg2k Trying now, thank you for reminding me :)

@williamdes
Copy link
Member

image
Seems like it ?

@williamdes
Copy link
Member

$i++;
$cfg['Servers'][$i]['verbose'] = 'SSL test';
$cfg['Servers'][$i]['DisableIS'] = true;
$cfg['Servers'][$i]['auth_type'] = 'config';
$cfg['Servers'][$i]['host'] = 'eu-cdbr-west-0w.cleardb.net';
$cfg['Servers'][$i]['user'] = 'x';
$cfg['Servers'][$i]['password'] = 'c';
$cfg['Servers'][$i]['ssl'] = true;
$cfg['Servers'][$i]['ssl_verify'] = false;
$cfg['Servers'][$i]['ssl_cert'] = '/mnt/Dev/!OSS/a-cert.pem';
$cfg['Servers'][$i]['ssl_key'] = '/mnt/Dev/!OSS/a-key.pem';
$cfg['Servers'][$i]['ssl_ca'] = '/mnt/Dev/!OSS/cleardb-ca.pem';
$cfg['Servers'][$i]['compress'] = false;
$cfg['Servers'][$i]['controlhost'] = 'x.y.eu-west-v.rds.amazonaws.com';
$cfg['Servers'][$i]['controlport'] = 3306;
$cfg['Servers'][$i]['controluser'] = 'bb';
$cfg['Servers'][$i]['controlpass'] = 'aa';
$cfg['Servers'][$i]['control_ssl'] = true;
$cfg['Servers'][$i]['control_ssl_verify'] = true;
$cfg['Servers'][$i]['control_ssl_ca'] = '/mnt/Dev/!OSS/rds-combined-ca-bundle.pem';
$cfg['Servers'][$i]['pmadb'] = 'xx';

@williamdes
Copy link
Member

if ($user === '@') {// Request did not succeed, please do not cache > if (false) {// Request did not succeed, please do not cache

Allows access, I need to find what is going wrong..

@williamdes williamdes removed the question Used when we need feedback from the submitter or when the issue is a question about PMA label Dec 5, 2019
@thg2k thg2k closed this as completed Dec 5, 2019
issues automation moved this from High priority to Closed Dec 5, 2019
@williamdes williamdes reopened this Dec 5, 2019
issues automation moved this from Closed to Needs triage Dec 5, 2019
@williamdes williamdes moved this from Needs triage to High priority in issues Dec 5, 2019
@thg2k
Copy link
Author

thg2k commented Dec 5, 2019

Sorry, typing from phone and messed up! I was going to say, i'm afraid it's more complicated than that.. i wanted to submit a patch but after 2 hours i had to give up. i'm happy to help you if you are still working on a patch, just hook me up

@williamdes
Copy link
Member

i'm happy to help you if you are still working on a patch, just hook me up

@thg2k I you have a patch or a start you can open a pull-request or post the diff here if you want to give your work to be finished

The issue seems to be in the caching solution, but the source is maybe somewhere else.

@thg2k
Copy link
Author

thg2k commented Dec 6, 2019

i'm happy to help you if you are still working on a patch, just hook me up

@thg2k I you have a patch or a start you can open a pull-request or post the diff here if you want to give your work to be finished

I don't have a patch to share, as i said i tried to come up with a fix but got stuck on some weird logic i found..

The issue seems to be in the caching solution, but the source is maybe somewhere else.

I will be out of office until next wednesday, after that i'm happy to help on this if you give me a crash course on the pma source code.. I'm very good in PHP coding but I'm not familiar with pma code base, and I have some questions on the caching layer. If you can spare 10 minute to answer my questions then i can work on a solution.

@williamdes
Copy link
Member

I'm very good in PHP coding but I'm not familiar with pma code base, and I have some questions on the caching layer. If you can spare 10 minute to answer my questions then i can work on a solution.

I will try another time to find out, If I can not find I will ask you
I can answer questions if needed

@williamdes
Copy link
Member

2019/12/09 21:24:29 [error] 6#6: *200 FastCGI sent in stderr: "PHP message: db[
    {
        "file": "\/mnt\/Dev\/@phpmyadmin\/theREALphpMyAdminREPO\/libraries\/check_user_privileges.inc.php",
        "line": 16,
        "function": "getCurrentUserAndHost",
        "class": "PhpMyAdmin\\DatabaseInterface",
        "object": {
            "types": {}
        },
        "type": "->",
        "args": []
    },
    {
        "file": "\/mnt\/Dev\/@phpmyadmin\/theREALphpMyAdminREPO\/libraries\/classes\/ListDatabase.php",
        "line": 13,
        "args": [
            "\/mnt\/Dev\/@phpmyadmin\/theREALphpMyAdminREPO\/libraries\/check_user_privileges.inc.php"
        ],
        "function": "require_once"
    },
    {
        "file": "\/mnt\/Dev\/@phpmyadmin\/theREALphpMyAdminREPO\/vendor\/composer\/ClassLoader.php",
        "line": 444,
        "args": [
            "\/mnt\/Dev\/@phpmyadmin\/theREALphpMyAdminREPO\/libraries\/classes\/ListDatabase.php"
        ],
        "function": "include"
    },

Seems like the class loader calls the code when id should not

@williamdes williamdes added this to the 4.9.3 milestone Dec 19, 2019
@williamdes williamdes added the has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete label Dec 21, 2019
@williamdes williamdes self-assigned this Dec 21, 2019
williamdes added a commit that referenced this issue Dec 21, 2019
…oluser configured

Fixes: #15608
Pull-request: #15681
Ref: 2bc8afe
Signed-off-by: William Desportes <williamdes@wdes.fr>
williamdes added a commit that referenced this issue Dec 21, 2019
Signed-off-by: William Desportes <williamdes@wdes.fr>
issues automation moved this from High priority to Closed Dec 21, 2019
@williamdes
Copy link
Member

@thg2k can you apply f402a8f and tell us if it works fine ?

@thg2k
Copy link
Author

thg2k commented Dec 22, 2019

@thg2k can you apply f402a8f and tell us if it works fine ?

Hi @williamdes,

I tested bfe41c0 (more recent), and it works fine. I still have doubts about the fix though, I see you basically invalidated the caching in getCurrentUserAndHost(), but I still wonder how the DatabaseInterface class is meant to work. Is it supposed to be instantiated once per database? Once per connection? Once per server?

Because I see two contradicting signals:

    # this one holds multiple connection links? so both control and user... ok
    private $_links;

    # the following seems to use a cache entry 'mysql_cur_user', but for which link?
    public function getCurrentUser()
    {
        if (Util::cacheExists('mysql_cur_user')) {
            return Util::cacheGet('mysql_cur_user');
        }
        $user = $this->fetchValue('SELECT CURRENT_USER();');
        if ($user !== false) {
            Util::cacheSet('mysql_cur_user', $user);
            return $user;
        }
        return '@';
    }

So just an heads up.. it works now but if you stumble on other problems on this class consider it might need some major refactoring.

Thank you and kind regards
GG

@williamdes
Copy link
Member

@thg2k I agree with you, on 4.9 branch it is too complicated to do refactoring
We only do refactoring on master branch
Maybe we should clarify the code around what created 2 issues in total :)

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Bug A problem or regression with an existing feature has-pr An issue that has a pull request pending that may fix this issue. The pull request may be incomplete
Projects
issues
  
Closed
Development

No branches or pull requests

2 participants