Skip to content

Commit c1df85f

Browse files
thorstenthorsten
committed
fix: corrected review notes
1 parent 232b63a commit c1df85f

16 files changed

Lines changed: 511 additions & 62 deletions

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

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -48,15 +48,18 @@ public function __construct(
4848
*/
4949
public function create(string $login, #[SensitiveParameter] string $password, string $domain = ''): bool
5050
{
51-
$result = false;
5251
$user = $this->createUser();
5352

5453
try {
5554
$result = $user->createUser($login, '', $domain);
5655
} catch (\Exception $exception) {
5756
$this->configuration
5857
->getLogger()
59-
->error(sprintf('Keycloak user creation failed for "%s": %s', $login, $exception->getMessage()));
58+
->error(sprintf(
59+
'Keycloak user creation failed for "%s": %s',
60+
$this->redactIdentifier($login),
61+
$exception->getMessage(),
62+
));
6063
return false;
6164
}
6265

@@ -76,7 +79,7 @@ public function create(string $login, #[SensitiveParameter] string $password, st
7679
$this->assignUserToGroups($user->getUserId());
7780
}
7881

79-
return $result;
82+
return true;
8083
}
8184

8285
public function update(string $login, #[SensitiveParameter] string $password): bool
@@ -89,6 +92,9 @@ public function delete(string $login): bool
8992
return true;
9093
}
9194

95+
/**
96+
* @throws Exception
97+
*/
9298
public function checkCredentials(
9399
string $login,
94100
#[SensitiveParameter]
@@ -99,7 +105,12 @@ public function checkCredentials(
99105
return false;
100106
}
101107

102-
if ($this->userExists($login)) {
108+
$existingUser = $this->findUser($login);
109+
if ($existingUser instanceof User) {
110+
if ($this->shouldSynchronizeGroupsOnLogin()) {
111+
$this->assignUserToGroups($existingUser->getUserId());
112+
}
113+
103114
return true;
104115
}
105116

@@ -140,10 +151,10 @@ private function getSubject(): string
140151
return trim((string) ($this->claims['sub'] ?? ''));
141152
}
142153

143-
private function userExists(string $login): bool
154+
private function findUser(string $login): ?User
144155
{
145156
$user = $this->createUser();
146-
return $user->getUserByLogin($login, false);
157+
return $user->getUserByLogin($login, false) ? $user : null;
147158
}
148159

149160
private function shouldAssignGroups(): bool
@@ -272,6 +283,23 @@ private function shouldSynchronizeGroupsOnLogin(): bool
272283
return $this->toBool($this->configuration->get(item: 'keycloak.groupSyncOnLogin'));
273284
}
274285

286+
private function redactIdentifier(string $identifier): string
287+
{
288+
if (str_contains($identifier, '@')) {
289+
[$local, $domain] = explode('@', $identifier, 2);
290+
return $local[0] . '***@' . $domain;
291+
}
292+
293+
if (mb_strlen($identifier) <= 3) {
294+
return str_repeat('*', mb_strlen($identifier));
295+
}
296+
297+
return mb_substr($identifier, 0, 3) . '…';
298+
}
299+
300+
/**
301+
* @throws Exception
302+
*/
275303
private function createUser(): User
276304
{
277305
if ($this->userFactory instanceof Closure) {

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

Lines changed: 56 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -45,8 +45,8 @@ public function validate(
4545
string $expectedAudience,
4646
string $expectedNonce,
4747
): array {
48-
if ($idToken === '') {
49-
return [];
48+
if (trim($idToken) === '') {
49+
throw new RuntimeException('OIDC id_token is missing');
5050
}
5151

5252
[$encodedHeader, $encodedPayload, $encodedSignature] = $this->splitToken($idToken);
@@ -106,13 +106,25 @@ private function validateClaims(
106106
string $expectedAudience,
107107
string $expectedNonce,
108108
): void {
109-
$issuer = trim((string) ($claims['iss'] ?? ''));
110-
if ($issuer !== '' && $issuer !== $expectedIssuer) {
109+
if (!array_key_exists('iss', $claims) || !is_string($claims['iss']) || trim($claims['iss']) === '') {
110+
throw new RuntimeException('OIDC id_token is missing the issuer claim');
111+
}
112+
if (trim($claims['iss']) !== $expectedIssuer) {
111113
throw new RuntimeException('OIDC issuer mismatch in id_token');
112114
}
113115

114-
$audience = $claims['aud'] ?? null;
115-
if ($audience !== null && !$this->audienceMatches($audience, $expectedAudience)) {
116+
if (!array_key_exists('sub', $claims) || !is_string($claims['sub']) || trim($claims['sub']) === '') {
117+
throw new RuntimeException('OIDC id_token is missing the subject claim');
118+
}
119+
120+
if (!array_key_exists('aud', $claims)) {
121+
throw new RuntimeException('OIDC id_token is missing the audience claim');
122+
}
123+
$audience = $claims['aud'];
124+
if (!$this->isValidAudience($audience)) {
125+
throw new RuntimeException('OIDC id_token audience claim is invalid');
126+
}
127+
if (!$this->audienceMatches($audience, $expectedAudience)) {
116128
throw new RuntimeException('OIDC audience mismatch in id_token');
117129
}
118130

@@ -125,24 +137,57 @@ private function validateClaims(
125137
throw new RuntimeException('OIDC authorized party missing in id_token');
126138
}
127139

128-
$nonce = trim((string) ($claims['nonce'] ?? ''));
129-
if ($expectedNonce !== '' && $nonce !== '' && !hash_equals($expectedNonce, $nonce)) {
130-
throw new RuntimeException('OIDC nonce mismatch in id_token');
140+
if ($expectedNonce !== '') {
141+
if (!array_key_exists('nonce', $claims) || !is_string($claims['nonce']) || $claims['nonce'] === '') {
142+
throw new RuntimeException('OIDC id_token is missing the nonce claim');
143+
}
144+
if (!hash_equals($expectedNonce, $claims['nonce'])) {
145+
throw new RuntimeException('OIDC nonce mismatch in id_token');
146+
}
131147
}
132148

133149
$now = $this->getCurrentTimestamp();
134150

135-
if (array_key_exists('exp', $claims) && is_numeric($claims['exp']) && (int) $claims['exp'] < $now) {
151+
if (!array_key_exists('exp', $claims) || !is_numeric($claims['exp'])) {
152+
throw new RuntimeException('OIDC id_token is missing the expiration claim');
153+
}
154+
if ((int) $claims['exp'] < $now) {
136155
throw new RuntimeException('OIDC id_token has expired');
137156
}
138157

139158
if (array_key_exists('nbf', $claims) && is_numeric($claims['nbf']) && (int) $claims['nbf'] > $now) {
140159
throw new RuntimeException('OIDC id_token is not valid yet');
141160
}
142161

143-
if (array_key_exists('iat', $claims) && is_numeric($claims['iat']) && (int) $claims['iat'] > ($now + 60)) {
162+
if (!array_key_exists('iat', $claims) || !is_numeric($claims['iat'])) {
163+
throw new RuntimeException('OIDC id_token is missing the issued-at claim');
164+
}
165+
$issuedAt = (int) $claims['iat'];
166+
if ($issuedAt > ($now + 60)) {
144167
throw new RuntimeException('OIDC id_token issued-at time is in the future');
145168
}
169+
if ($issuedAt < ($now - 86400)) {
170+
throw new RuntimeException('OIDC id_token issued-at time is too far in the past');
171+
}
172+
}
173+
174+
private function isValidAudience(mixed $audience): bool
175+
{
176+
if (is_string($audience)) {
177+
return trim($audience) !== '';
178+
}
179+
180+
if (!is_array($audience) || $audience === []) {
181+
return false;
182+
}
183+
184+
foreach ($audience as $entry) {
185+
if (!is_string($entry) || trim($entry) === '') {
186+
return false;
187+
}
188+
}
189+
190+
return true;
146191
}
147192

148193
/**

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

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -90,10 +90,16 @@ public function callback(Request $request): Response
9090
[$auth, $oAuth, $entraIdSession] = $this->buildAuthContext();
9191

9292
$code = Filter::filterVar($request->query->get('code'), FILTER_SANITIZE_SPECIAL_CHARS, '');
93+
$errorParam = Filter::filterVar($request->query->get('error'), FILTER_SANITIZE_SPECIAL_CHARS, '');
9394
$error = Filter::filterVar($request->query->get('error_description'), FILTER_SANITIZE_SPECIAL_CHARS, '');
9495

95-
if ($error !== '') {
96-
$this->configuration->getLogger()->warning(sprintf('Azure callback error: %s', $error));
96+
if ($errorParam !== '' || $error !== '') {
97+
$this->configuration
98+
->getLogger()
99+
->warning(sprintf(
100+
'Azure callback error: %s',
101+
trim($errorParam . ($error !== '' ? ': ' . $error : '')),
102+
));
97103
return new RedirectResponse($this->configuration->getDefaultUrl());
98104
}
99105

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

Lines changed: 35 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -111,21 +111,22 @@ public function logout(): RedirectResponse
111111

112112
try {
113113
$providerConfig = $this->providerConfigFactory->create();
114-
if (!$providerConfig->enabled || $providerConfig->discoveryUrl === '') {
115-
return new RedirectResponse($this->configuration->getDefaultUrl());
116-
}
117-
118114
$idToken = $this->resolveLogoutIdToken($user);
119115

120116
$user->deleteFromSession();
121117
$this->oidcSession->clearIdToken();
122118

119+
if (!$providerConfig->enabled || $providerConfig->discoveryUrl === '') {
120+
return new RedirectResponse($this->configuration->getDefaultUrl());
121+
}
122+
123123
$discoveryDocument = $this->discoveryService->discover($providerConfig);
124124
$logoutUrl = $this->oidcClient->buildLogoutUrl($providerConfig, $discoveryDocument, $idToken);
125125

126126
return new RedirectResponse($logoutUrl ?? $this->configuration->getDefaultUrl());
127127
} catch (Exception) {
128128
$user->deleteFromSession();
129+
$this->oidcSession->clearIdToken();
129130
return new RedirectResponse($this->configuration->getDefaultUrl());
130131
}
131132
}
@@ -171,26 +172,39 @@ public function callback(Request $request): Response
171172
$code,
172173
$authorizationState['verifier'],
173174
);
174-
$this->idTokenValidator->validate(
175+
$idTokenClaims = $this->idTokenValidator->validate(
175176
(string) ($token['id_token'] ?? ''),
176177
$discoveryDocument,
177178
$providerConfig->client->clientId,
178179
$authorizationState['nonce'],
179180
);
180181
$claims = $this->oidcClient->fetchUserInfo($discoveryDocument, (string) $token['access_token']);
182+
183+
$idTokenSub = (string) ($idTokenClaims['sub'] ?? '');
184+
$userInfoSub = (string) ($claims['sub'] ?? '');
185+
if ($idTokenSub === '' || $userInfoSub === '' || !hash_equals($idTokenSub, $userInfoSub)) {
186+
$this->configuration
187+
->getLogger()
188+
->warning('Keycloak subject mismatch between ID token and UserInfo; aborting login.');
189+
$this->oidcSession->clearAuthorizationState();
190+
return $redirect;
191+
}
192+
181193
$login = $this->resolveLocalLogin($claims);
182194
$auth = new AuthKeycloak($this->configuration, $providerConfig, $claims, $login, $this->userFactory);
183195

184196
if (!$auth->isValidLogin($login)) {
185-
$this->configuration->getLogger()->warning(sprintf('Keycloak login not valid for user: %s', $login));
197+
$this->configuration
198+
->getLogger()
199+
->warning(sprintf('Keycloak login not valid for user: %s', $this->maskLogin($login)));
186200
$this->oidcSession->clearAuthorizationState();
187201
return $redirect;
188202
}
189203

190204
if (!$auth->checkCredentials($login, '')) {
191205
$this->configuration
192206
->getLogger()
193-
->warning(sprintf('Keycloak credentials not valid for user: %s', $login));
207+
->warning(sprintf('Keycloak credentials not valid for user: %s', $this->maskLogin($login)));
194208
$this->oidcSession->clearAuthorizationState();
195209
return $redirect;
196210
}
@@ -199,13 +213,15 @@ public function callback(Request $request): Response
199213
if (!$user->getUserByLogin($login)) {
200214
$this->configuration
201215
->getLogger()
202-
->warning(sprintf('Keycloak user lookup failed for login: %s', $login));
216+
->warning(sprintf('Keycloak user lookup failed for login: %s', $this->maskLogin($login)));
203217
$this->oidcSession->clearAuthorizationState();
204218
return $redirect;
205219
}
206220

207221
if (!$this->synchronizeKeycloakSubject($user, $claims)) {
208-
$this->configuration->getLogger()->warning(sprintf('Keycloak subject mismatch for user: %s', $login));
222+
$this->configuration
223+
->getLogger()
224+
->warning(sprintf('Keycloak subject mismatch for user: %s', $this->maskLogin($login)));
209225
$this->oidcSession->clearAuthorizationState();
210226
return $redirect;
211227
}
@@ -276,6 +292,16 @@ private function resolveLocalLogin(array $claims): string
276292
return $subject;
277293
}
278294

295+
private function maskLogin(string $login): string
296+
{
297+
$login = trim($login);
298+
if ($login === '') {
299+
return '<empty>';
300+
}
301+
302+
return 'sha256:' . substr(hash('sha256', $login), 0, 12);
303+
}
304+
279305
/** @param array<string, mixed> $claims */
280306
private function synchronizeKeycloakSubject(CurrentUser $user, array $claims): bool
281307
{

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -187,6 +187,44 @@ protected function createIndex(string $table, string $indexName, string|array $c
187187
return sprintf('CREATE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName, $columnList);
188188
}
189189

190+
/**
191+
* Helper to create a UNIQUE index.
192+
*
193+
* On SQL Server, NULLs are treated as equal in unique indexes, so a filtered
194+
* index is emitted to allow multiple NULL values. MySQL/MariaDB, PostgreSQL
195+
* and SQLite already treat NULLs as distinct in unique indexes.
196+
*
197+
* @param string|string[] $columns
198+
*/
199+
protected function createUniqueIndex(string $table, string $indexName, string|array $columns): string
200+
{
201+
$tableName = $this->table($table);
202+
$columns = (array) $columns;
203+
$columnList = implode(', ', $columns);
204+
205+
if ($this->isSqlServer()) {
206+
$whereClause = implode(' AND ', array_map(static fn(string $col): string => sprintf(
207+
'%s IS NOT NULL',
208+
$col,
209+
), $columns));
210+
return sprintf(
211+
"IF NOT EXISTS (SELECT * FROM sys.indexes WHERE name = '%s') "
212+
. 'CREATE UNIQUE INDEX %s ON %s (%s) WHERE %s',
213+
$indexName,
214+
$indexName,
215+
$tableName,
216+
$columnList,
217+
$whereClause,
218+
);
219+
}
220+
221+
if ($this->isMySql()) {
222+
return sprintf('CREATE UNIQUE INDEX %s ON %s (%s)', $indexName, $tableName, $columnList);
223+
}
224+
225+
return sprintf('CREATE UNIQUE INDEX IF NOT EXISTS %s ON %s (%s)', $indexName, $tableName, $columnList);
226+
}
227+
190228
/**
191229
* Helper to check if an index exists.
192230
* Returns a SQL query that checks for index existence.

‎phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -228,8 +228,8 @@ public function up(OperationRecorder $recorder): void
228228
);
229229

230230
$recorder->addSql(
231-
$this->createIndex('faquserdata', 'idx_faquserdata_keycloak_sub', 'keycloak_sub'),
232-
'Create keycloak_sub index on faquserdata',
231+
$this->createUniqueIndex('faquserdata', 'idx_faquserdata_keycloak_sub', 'keycloak_sub'),
232+
'Create unique keycloak_sub index on faquserdata',
233233
);
234234
$recorder->addConfig('api.rateLimit.interval', '3600');
235235
$recorder->addConfig('queue.transport', 'database');

‎phpmyfaq/src/phpMyFAQ/User.php‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -821,7 +821,11 @@ public function getUserIdByKeycloakSub(string $keycloakSub): int
821821

822822
$userData = $this->userdata->fetchAll('keycloak_sub', $keycloakSub);
823823

824-
return $userData['user_id'];
824+
if (!is_array($userData) || !isset($userData['user_id'])) {
825+
return 0;
826+
}
827+
828+
return (int) $userData['user_id'];
825829
}
826830

827831
/**

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -311,6 +311,10 @@ public function save(): bool
311311
}
312312

313313
if ($res === false) {
314+
if (array_key_exists('keycloak_sub', $this->data)) {
315+
return false;
316+
}
317+
314318
$update = sprintf(
315319
"
316320
UPDATE

0 commit comments

Comments
 (0)