Skip to content

Commit 0090108

Browse files
MauricioFauthMauricioFauth
authored
Merge pull request #20169 from MauricioFauth/Server-Privileges-getDataForChangeOrCopyUser
Refactor Privileges::getDataForChangeOrCopyUser()
2 parents 2354c6b + 61b0a72 commit 0090108

5 files changed

Lines changed: 82 additions & 89 deletions

File tree

‎phpstan-baseline.neon‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -11061,7 +11061,7 @@ parameters:
1106111061
-
1106211062
message: '#^Construct empty\(\) is not allowed\. Use more strict comparison\.$#'
1106311063
identifier: empty.notAllowed
11064-
count: 14
11064+
count: 13
1106511065
path: src/Server/Privileges.php
1106611066

1106711067
-

‎psalm-baseline.xml‎

Lines changed: 0 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -7533,7 +7533,6 @@
75337533
<RiskyTruthyFalsyComparison>
75347534
<code><![CDATA[empty($_POST[$currentGrant[0] . '_none'])]]></code>
75357535
<code><![CDATA[empty($_POST[$currentGrant[0] . '_none'])]]></code>
7536-
<code><![CDATA[empty($_POST['change_copy'])]]></code>
75377536
<code><![CDATA[empty($_POST['nopass'])]]></code>
75387537
<code><![CDATA[empty($_POST['pma_pw'])]]></code>
75397538
<code><![CDATA[empty($_POST['pma_pw2'])]]></code>
@@ -10519,9 +10518,6 @@
1051910518
<code><![CDATA[Config::getInstance()]]></code>
1052010519
<code><![CDATA[Config::getInstance()]]></code>
1052110520
</DeprecatedMethod>
10522-
<UnusedVariable>
10523-
<code><![CDATA[$password]]></code>
10524-
</UnusedVariable>
1052510521
</file>
1052610522
<file src="tests/unit/Server/SelectTest.php">
1052710523
<DeprecatedMethod>

‎src/Controllers/Server/PrivilegesController.php‎

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -108,12 +108,14 @@ public function __invoke(ServerRequest $request): Response
108108
)->getDisplay());
109109
}
110110

111+
$isChangeCopyUser = $request->hasBodyParam('change_copy');
112+
111113
/**
112114
* Checks if the user is using "Change Login Information / Copy User" dialog
113115
* only to update the password
114116
*/
115117
if (
116-
$request->hasBodyParam('change_copy')
118+
$isChangeCopyUser
117119
&& $serverPrivileges->username === $request->getParsedBodyParam('old_username')
118120
&& $serverPrivileges->hostname === $request->getParsedBodyParam('old_hostname')
119121
) {
@@ -131,13 +133,19 @@ public function __invoke(ServerRequest $request): Response
131133
return $this->response->response();
132134
}
133135

134-
/**
135-
* Changes / copies a user, part I
136-
*/
137-
$password = $serverPrivileges->getDataForChangeOrCopyUser(
138-
$request->getParsedBodyParamAsString('old_username', ''),
139-
$request->getParsedBodyParamAsString('old_hostname', ''),
140-
);
136+
$password = null;
137+
if ($isChangeCopyUser) {
138+
/** Changes / copies a user, part I */
139+
$password = $serverPrivileges->getDataForChangeOrCopyUser(
140+
$request->getParsedBodyParamAsString('old_username', ''),
141+
$request->getParsedBodyParamAsString('old_hostname', ''),
142+
);
143+
if ($password instanceof Message) {
144+
$this->response->addHTML($password->getDisplay());
145+
$password = null;
146+
$isChangeCopyUser = false;
147+
}
148+
}
141149

142150
/**
143151
* Adds a user
@@ -147,7 +155,7 @@ public function __invoke(ServerRequest $request): Response
147155
$queriesForDisplay = null;
148156
Current::$sqlQuery = '';
149157
$addUserError = false;
150-
if ($request->hasBodyParam('adduser_submit') || $request->hasBodyParam('change_copy')) {
158+
if ($request->hasBodyParam('adduser_submit') || $isChangeCopyUser) {
151159
$hostname = $serverPrivileges->getHostname(
152160
$request->getParsedBodyParamAsString('pred_hostname', ''),
153161
$serverPrivileges->hostname ?? '',
@@ -164,6 +172,7 @@ public function __invoke(ServerRequest $request): Response
164172
$hostname,
165173
$password,
166174
$relationParameters->configurableMenusFeature !== null,
175+
$isChangeCopyUser,
167176
);
168177
//update the old variables
169178
if ($retMessage !== null) {
@@ -176,7 +185,7 @@ public function __invoke(ServerRequest $request): Response
176185
* Changes / copies a user, part III
177186
*/
178187
if (
179-
$request->hasBodyParam('change_copy')
188+
$isChangeCopyUser
180189
&& $serverPrivileges->username !== null
181190
&& $serverPrivileges->hostname !== null
182191
) {
@@ -266,18 +275,18 @@ public function __invoke(ServerRequest $request): Response
266275
*/
267276
if (
268277
$request->hasBodyParam('delete')
269-
|| ($request->hasBodyParam('change_copy') && $request->getParsedBodyParam('mode') < 4)
278+
|| ($isChangeCopyUser && $request->getParsedBodyParam('mode') < 4)
270279
) {
271-
$queries = $serverPrivileges->getDataForDeleteUsers($queries);
272-
if (! $request->hasBodyParam('change_copy')) {
280+
$queries = $serverPrivileges->getDataForDeleteUsers($queries, $isChangeCopyUser);
281+
if (! $isChangeCopyUser) {
273282
[Current::$sqlQuery, Current::$message] = $serverPrivileges->deleteUser($queries);
274283
}
275284
}
276285

277286
/**
278287
* Changes / copies a user, part V
279288
*/
280-
if ($request->hasBodyParam('change_copy')) {
289+
if ($isChangeCopyUser) {
281290
$queries = $serverPrivileges->getDataForQueries($queries, $queriesForDisplay);
282291
Current::$message = Message::success();
283292
Current::$sqlQuery = implode("\n", $queries);
@@ -311,6 +320,7 @@ public function __invoke(ServerRequest $request): Response
311320
$serverPrivileges->hostname ?? '',
312321
$serverPrivileges->username ?? '',
313322
! is_array($databaseName) ? $databaseName : null,
323+
$isChangeCopyUser,
314324
);
315325

316326
if (Current::$message instanceof Message) {

‎src/Server/Privileges.php‎

Lines changed: 53 additions & 59 deletions
Original file line numberDiff line numberDiff line change
@@ -1367,6 +1367,7 @@ public function getExtraDataForAjaxBehavior(
13671367
string $hostname,
13681368
string $username,
13691369
string|null $dbname,
1370+
bool $isChangeCopyUser,
13701371
): array {
13711372
if ($dbname !== null) {
13721373
//if (preg_match('/\\\\(?:_|%)/i', $dbname)) {
@@ -1389,7 +1390,7 @@ public function getExtraDataForAjaxBehavior(
13891390
$extraData['sql_query'] = Generator::getMessage('', $sqlQuery);
13901391
}
13911392

1392-
if (isset($_POST['change_copy'])) {
1393+
if ($isChangeCopyUser) {
13931394
$user = [
13941395
'name' => $username,
13951396
'host' => $hostname,
@@ -2068,68 +2069,60 @@ private function generateQueriesForUpdatePrivileges(
20682069
/**
20692070
* Get List of information: Changes / copies a user
20702071
*/
2071-
public function getDataForChangeOrCopyUser(string $oldUsername, string $oldHostname): string|null
2072+
public function getDataForChangeOrCopyUser(string $oldUsername, string $oldHostname): Message|string|null
20722073
{
2073-
if (isset($_POST['change_copy'])) {
2074-
$userHostCondition = $this->getUserHostCondition($oldUsername, $oldHostname);
2075-
$row = $this->dbi->fetchSingleRow('SELECT * FROM `mysql`.`user` ' . $userHostCondition . ';');
2076-
if ($row === []) {
2077-
$response = ResponseRenderer::getInstance();
2078-
$response->addHTML(
2079-
Message::notice(__('No user found.'))->getDisplay(),
2080-
);
2081-
unset($_POST['change_copy']);
2082-
} else {
2083-
$this->sslType = $row['ssl_type'];
2084-
$this->sslCipher = $row['ssl_cipher'];
2085-
$this->x509Issuer = $row['x509_issuer'];
2086-
$this->x509Subject = $row['x509_subject'];
2087-
2088-
$serverVersion = $this->dbi->getVersion();
2089-
// Recent MySQL versions have the field "Password" in mysql.user,
2090-
// so the previous extract creates $row['Password'] but this script
2091-
// uses $password
2092-
if (! isset($row['password']) && isset($row['Password'])) {
2093-
$row['password'] = $row['Password'];
2094-
}
2074+
$userHostCondition = $this->getUserHostCondition($oldUsername, $oldHostname);
2075+
$row = $this->dbi->fetchSingleRow('SELECT * FROM `mysql`.`user` ' . $userHostCondition . ';');
2076+
if ($row === []) {
2077+
return Message::notice(__('No user found.'));
2078+
}
20952079

2096-
if (
2097-
Compatibility::isMySqlOrPerconaDb($this->dbi)
2098-
&& $serverVersion >= 50606
2099-
&& $serverVersion < 50706
2100-
&& ((isset($row['authentication_string'])
2101-
&& empty($row['password']))
2102-
|| (isset($row['plugin'])
2103-
&& $row['plugin'] === 'sha256_password'))
2104-
) {
2105-
$row['password'] = $row['authentication_string'];
2106-
}
2080+
$this->sslType = $row['ssl_type'];
2081+
$this->sslCipher = $row['ssl_cipher'];
2082+
$this->x509Issuer = $row['x509_issuer'];
2083+
$this->x509Subject = $row['x509_subject'];
21072084

2108-
if (
2109-
Compatibility::isMariaDb($this->dbi)
2110-
&& $serverVersion >= 50500
2111-
&& isset($row['authentication_string'])
2112-
&& empty($row['password'])
2113-
) {
2114-
$row['password'] = $row['authentication_string'];
2115-
}
2085+
$serverVersion = $this->dbi->getVersion();
2086+
// Recent MySQL versions have the field "Password" in mysql.user,
2087+
// so the previous extract creates $row['Password'] but this script
2088+
// uses $password
2089+
if (! isset($row['password']) && isset($row['Password'])) {
2090+
$row['password'] = $row['Password'];
2091+
}
21162092

2117-
// Always use 'authentication_string' column
2118-
// for MySQL 5.7.6+ since it does not have
2119-
// the 'password' column at all
2120-
if (
2121-
Compatibility::isMySqlOrPerconaDb($this->dbi)
2122-
&& $serverVersion >= 50706
2123-
&& isset($row['authentication_string'])
2124-
) {
2125-
$row['password'] = $row['authentication_string'];
2126-
}
2093+
if (
2094+
Compatibility::isMySqlOrPerconaDb($this->dbi)
2095+
&& $serverVersion >= 50606
2096+
&& $serverVersion < 50706
2097+
&& ((isset($row['authentication_string'])
2098+
&& empty($row['password']))
2099+
|| (isset($row['plugin'])
2100+
&& $row['plugin'] === 'sha256_password'))
2101+
) {
2102+
$row['password'] = $row['authentication_string'];
2103+
}
21272104

2128-
return $row['password'];
2129-
}
2105+
if (
2106+
Compatibility::isMariaDb($this->dbi)
2107+
&& $serverVersion >= 50500
2108+
&& isset($row['authentication_string'])
2109+
&& empty($row['password'])
2110+
) {
2111+
$row['password'] = $row['authentication_string'];
21302112
}
21312113

2132-
return null;
2114+
// Always use 'authentication_string' column
2115+
// for MySQL 5.7.6+ since it does not have
2116+
// the 'password' column at all
2117+
if (
2118+
Compatibility::isMySqlOrPerconaDb($this->dbi)
2119+
&& $serverVersion >= 50706
2120+
&& isset($row['authentication_string'])
2121+
) {
2122+
$row['password'] = $row['authentication_string'];
2123+
}
2124+
2125+
return $row['password'];
21332126
}
21342127

21352128
/**
@@ -2139,9 +2132,9 @@ public function getDataForChangeOrCopyUser(string $oldUsername, string $oldHostn
21392132
*
21402133
* @return mixed[]
21412134
*/
2142-
public function getDataForDeleteUsers(array $queries): array
2135+
public function getDataForDeleteUsers(array $queries, bool $isChangeCopyUser): array
21432136
{
2144-
if (isset($_POST['change_copy'])) {
2137+
if ($isChangeCopyUser) {
21452138
$selectedUsr = [$_POST['old_username'] . '&amp;#27;' . $_POST['old_hostname']];
21462139
} else {
21472140
// null happens when no user was selected
@@ -2244,6 +2237,7 @@ public function addUser(
22442237
string $hostname,
22452238
string|null $password,
22462239
bool $isMenuwork,
2240+
bool $isChangeCopyUser,
22472241
): array {
22482242
// Some reports were sent to the error reporting server with phpMyAdmin 5.1.0
22492243
// pred_username was reported to be not defined
@@ -2277,7 +2271,7 @@ public function addUser(
22772271
$alterSqlQuery,
22782272
] = $this->getSqlQueriesForDisplayAndAddUser($username, $hostname, $password ?? '');
22792273

2280-
if (empty($_POST['change_copy'])) {
2274+
if (! $isChangeCopyUser) {
22812275
$error = false;
22822276

22832277
if (! $this->dbi->tryQuery($createUserReal)) {

‎tests/unit/Server/PrivilegesTest.php‎

Lines changed: 4 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -282,14 +282,8 @@ public function testGetDataForChangeOrCopyUser(): void
282282

283283
$serverPrivileges = $this->getPrivileges($this->createDatabaseInterface($dummyDbi));
284284

285-
//$_POST['change_copy'] not set
286-
$password = $serverPrivileges->getDataForChangeOrCopyUser('', '');
287-
288-
//$_POST['change_copy'] is set
289-
$_POST['change_copy'] = true;
290285
$password = $serverPrivileges->getDataForChangeOrCopyUser('PMA_old_username', 'PMA_old_hostname');
291286
self::assertSame('pma_password', $password);
292-
unset($_POST['change_copy']);
293287
}
294288

295289
public function testGetExportUserDefinitionTextarea(): void
@@ -346,7 +340,7 @@ public function testAddUser(): void
346340
$retMessage,,,
347341
$sqlQuery,
348342
$addUserError,
349-
] = $serverPrivileges->addUser($dbname, $username, $hostname, $dbname, true);
343+
] = $serverPrivileges->addUser($dbname, $username, $hostname, $dbname, true, false);
350344
self::assertInstanceOf(Message::class, $retMessage);
351345
self::assertSame(
352346
'You have added a new user.',
@@ -394,7 +388,7 @@ public function testAddUserOld(): void
394388
$retMessage,,,
395389
$sqlQuery,
396390
$addUserError,
397-
] = $serverPrivileges->addUser($dbname, $username, $hostname, $dbname, true);
391+
] = $serverPrivileges->addUser($dbname, $username, $hostname, $dbname, true, false);
398392

399393
self::assertInstanceOf(Message::class, $retMessage);
400394
self::assertSame(
@@ -1179,7 +1173,6 @@ public function testGetExtraDataForAjaxBehavior(): void
11791173
$hostname = 'pma_hostname';
11801174
$dbname = 'pma_dbname';
11811175
$_POST['username'] = 'username';
1182-
$_POST['change_copy'] = 'change_copy';
11831176
$_GET['validate_username'] = 'validate_username';
11841177
$_GET['username'] = 'username';
11851178
$_POST['update_privs'] = 'update_privs';
@@ -1190,6 +1183,7 @@ public function testGetExtraDataForAjaxBehavior(): void
11901183
$hostname,
11911184
$username,
11921185
$dbname,
1186+
true,
11931187
);
11941188

11951189
//user_exists
@@ -1353,15 +1347,14 @@ public function testGetDataForDeleteUsers(): void
13531347
{
13541348
$serverPrivileges = $this->getPrivileges($this->createDatabaseInterface());
13551349

1356-
$_POST['change_copy'] = 'change_copy';
13571350
$_POST['old_hostname'] = 'old_hostname';
13581351
$_POST['old_username'] = 'old_username';
13591352
$relationParameters = RelationParameters::fromArray([]);
13601353
(new ReflectionProperty(Relation::class, 'cache'))->setValue(null, $relationParameters);
13611354

13621355
$queries = [];
13631356

1364-
$ret = $serverPrivileges->getDataForDeleteUsers($queries);
1357+
$ret = $serverPrivileges->getDataForDeleteUsers($queries, true);
13651358

13661359
$item = ["# Deleting 'old_username'@'old_hostname' ...", "DROP USER 'old_username'@'old_hostname';"];
13671360
self::assertSame($item, $ret);

0 commit comments

Comments
 (0)