Skip to content

Commit faff3fe

Browse files
thorstenthorsten
committed
fix: corrected review notes
1 parent 3179237 commit faff3fe

7 files changed

Lines changed: 72 additions & 19 deletions

File tree

‎phpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.php‎

Lines changed: 31 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -67,13 +67,35 @@ public function create(string $login, #[SensitiveParameter] string $password, st
6767
return false;
6868
}
6969

70+
try {
71+
$saved = $user->setUserData([
72+
'display_name' => $this->getDisplayName(),
73+
'email' => $this->getEmail(),
74+
'keycloak_sub' => $this->getSubject(),
75+
]);
76+
} catch (\Exception $exception) {
77+
$this->configuration
78+
->getLogger()
79+
->error(sprintf(
80+
'Keycloak user data persistence failed for "%s": %s',
81+
$this->redactIdentifier($login),
82+
$exception->getMessage(),
83+
));
84+
return false;
85+
}
86+
87+
if (!$saved) {
88+
$this->configuration
89+
->getLogger()
90+
->error(sprintf(
91+
'Keycloak user data persistence returned false for "%s"',
92+
$this->redactIdentifier($login),
93+
));
94+
return false;
95+
}
96+
7097
$user->setStatus('active');
7198
$user->setAuthSource(AuthenticationSourceType::AUTH_KEYCLOAK->value);
72-
$user->setUserData([
73-
'display_name' => $this->getDisplayName(),
74-
'email' => $this->getEmail(),
75-
'keycloak_sub' => $this->getSubject(),
76-
]);
7799

78100
if ($this->shouldAssignGroups()) {
79101
$this->assignUserToGroups($user->getUserId());
@@ -280,7 +302,10 @@ private function toBool(mixed $value): bool
280302

281303
private function shouldSynchronizeGroupsOnLogin(): bool
282304
{
283-
return $this->toBool($this->configuration->get(item: 'keycloak.groupSyncOnLogin'));
305+
return (
306+
$this->toBool($this->configuration->get(item: 'keycloak.groupSyncOnLogin'))
307+
&& $this->configuration->get(item: 'security.permLevel') === 'medium'
308+
);
284309
}
285310

286311
private function redactIdentifier(string $identifier): string

‎phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcIdTokenValidator.php‎

Lines changed: 8 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -151,12 +151,17 @@ private function validateClaims(
151151
if (!array_key_exists('exp', $claims) || !is_numeric($claims['exp'])) {
152152
throw new RuntimeException('OIDC id_token is missing the expiration claim');
153153
}
154-
if ((int) $claims['exp'] < $now) {
154+
if ($now >= (int) $claims['exp']) {
155155
throw new RuntimeException('OIDC id_token has expired');
156156
}
157157

158-
if (array_key_exists('nbf', $claims) && is_numeric($claims['nbf']) && (int) $claims['nbf'] > $now) {
159-
throw new RuntimeException('OIDC id_token is not valid yet');
158+
if (array_key_exists('nbf', $claims)) {
159+
if (!is_numeric($claims['nbf'])) {
160+
throw new RuntimeException('OIDC id_token has a non-numeric nbf claim');
161+
}
162+
if ($now < (int) $claims['nbf']) {
163+
throw new RuntimeException('OIDC id_token is not valid yet');
164+
}
160165
}
161166

162167
if (!array_key_exists('iat', $claims) || !is_numeric($claims['iat'])) {

‎phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php‎

Lines changed: 14 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -191,6 +191,10 @@ public function callback(Request $request): Response
191191
}
192192

193193
$login = $this->resolveLocalLogin($claims);
194+
if ($login === '') {
195+
$this->oidcSession->clearAuthorizationState();
196+
return $redirect;
197+
}
194198
$auth = new AuthKeycloak($this->configuration, $providerConfig, $claims, $login, $this->userFactory);
195199

196200
if (!$auth->isValidLogin($login)) {
@@ -289,7 +293,13 @@ private function resolveLocalLogin(array $claims): string
289293
return $email;
290294
}
291295

292-
return $subject;
296+
$this->configuration
297+
->getLogger()
298+
->warning(
299+
'Keycloak login rejected: claims are missing both preferred_username and email; refusing to auto-provision the sub.',
300+
);
301+
302+
return '';
293303
}
294304

295305
private function maskLogin(string $login): string
@@ -299,7 +309,9 @@ private function maskLogin(string $login): string
299309
return '<empty>';
300310
}
301311

302-
return 'sha256:' . substr(hash('sha256', $login), 0, 12);
312+
$secret = (string) $this->configuration->get('security.salt');
313+
314+
return 'hmac:' . substr(hash_hmac('sha256', $login, $secret), 0, 12);
303315
}
304316

305317
/** @param array<string, mixed> $claims */

‎phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php‎

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,8 +169,10 @@ protected function createIndex(string $table, string $indexName, string|array $c
169169

170170
if ($this->isSqlServer()) {
171171
return sprintf(
172-
"IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s') " . 'CREATE INDEX %s ON %s (%s)',
172+
"IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s' AND object_id = OBJECT_ID('%s')) "
173+
. 'CREATE INDEX %s ON %s (%s)',
173174
$indexName,
175+
$tableName,
174176
$indexName,
175177
$tableName,
176178
$columnList,
@@ -208,9 +210,10 @@ protected function createUniqueIndex(string $table, string $indexName, string|ar
208210
$col,
209211
), $columns));
210212
return sprintf(
211-
"IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s') "
213+
"IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s' AND object_id = OBJECT_ID('%s')) "
212214
. 'CREATE UNIQUE INDEX %s ON %s (%s) WHERE %s',
213215
$indexName,
216+
$tableName,
214217
$indexName,
215218
$tableName,
216219
$columnList,

‎phpmyfaq/src/phpMyFAQ/User/UserData.php‎

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -279,6 +279,11 @@ public function load(int $userId): bool
279279
*/
280280
public function save(): bool
281281
{
282+
$keycloakSubRaw = $this->data['keycloak_sub'] ?? null;
283+
$keycloakSubValue = is_string($keycloakSubRaw) && trim($keycloakSubRaw) !== ''
284+
? "'" . $this->configuration->getDb()->escape($keycloakSubRaw) . "'"
285+
: 'NULL';
286+
282287
$update = sprintf(
283288
"
284289
UPDATE
@@ -287,7 +292,7 @@ public function save(): bool
287292
last_modified = '%s',
288293
display_name = '%s',
289294
email = '%s',
290-
keycloak_sub = '%s',
295+
keycloak_sub = %s,
291296
is_visible = %d,
292297
twofactor_enabled = %d,
293298
secret = '%s'
@@ -297,7 +302,7 @@ public function save(): bool
297302
date(format: 'YmdHis', timestamp: Request::createFromGlobals()->server->get('REQUEST_TIME')),
298303
$this->configuration->getDb()->escape((string) ($this->data['display_name'] ?? '')),
299304
$this->configuration->getDb()->escape((string) ($this->data['email'] ?? '')),
300-
$this->configuration->getDb()->escape((string) ($this->data['keycloak_sub'] ?? '')),
305+
$keycloakSubValue,
301306
$this->data['is_visible'] ?? 0,
302307
$this->data['twofactor_enabled'] ?? 0,
303308
$this->data['secret'] ?? '',

‎tests/phpMyFAQ/Auth/AuthKeycloakTest.php‎

Lines changed: 6 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,7 @@ public function testCheckCredentialsSynchronizesGroupsForExistingUserWhenEnabled
4747
'keycloak.groupSyncOnLogin' => 'true',
4848
'keycloak.groupMapping' => '{"manage-users":"Administrators","faq-editors":"faq-editors"}',
4949
'keycloak.clientId' => 'phpmyfaq',
50+
'security.permLevel' => 'medium',
5051
default => null,
5152
});
5253
$configuration->method('getLogger')->willReturn($this->createMock(Logger::class));
@@ -125,7 +126,8 @@ public function testCheckCredentialsAutoProvisionsUserWhenEnabled(): void
125126
'display_name' => 'John Doe',
126127
'email' => 'john@example.com',
127128
'keycloak_sub' => 'subject-123',
128-
]);
129+
])
130+
->willReturn(true);
129131

130132
$auth = new AuthKeycloak(
131133
$configuration,
@@ -169,7 +171,8 @@ public function testCreateAssignsMappedGroupsFromKeycloakRoles(): void
169171
'display_name' => 'John Doe',
170172
'email' => 'john@example.com',
171173
'keycloak_sub' => 'subject-123',
172-
]);
174+
])
175+
->willReturn(true);
173176
$user->expects($this->once())->method('getUserId')->willReturn(42);
174177

175178
$permission = $this->createMock(MediumPermission::class);
@@ -230,7 +233,7 @@ public function testCreateSynchronizesMappedGroupsWhenEnabled(): void
230233
$user->expects($this->once())->method('createUser')->with('john', '', '')->willReturn(true);
231234
$user->expects($this->once())->method('setStatus')->with('active');
232235
$user->expects($this->once())->method('setAuthSource')->with(AuthenticationSourceType::AUTH_KEYCLOAK->value);
233-
$user->expects($this->once())->method('setUserData');
236+
$user->expects($this->once())->method('setUserData')->willReturn(true);
234237
$user->expects($this->once())->method('getUserId')->willReturn(42);
235238

236239
$permission = $this->createMock(MediumPermission::class);

‎tests/phpMyFAQ/Functional/ControllerWebTestCase.php‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ private function normalizeConfigurationValue(mixed $value): string
118118
}
119119

120120
if (is_array($value)) {
121-
return (string) json_encode($value);
121+
return json_encode($value, JSON_THROW_ON_ERROR);
122122
}
123123

124124
return (string) $value;

0 commit comments

Comments
 (0)