feat(keycloak): added a dedicated Keycloak configuration (#4188)#4189
Merged
feat(keycloak): added a dedicated Keycloak configuration (#4188)#4189
Conversation
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (7)
📝 WalkthroughWalkthroughAdds Keycloak OpenID Connect SSO support: UI tabs and login buttons, OIDC client/discovery/PKCE/ID-token validation services, a Keycloak authentication driver and frontend controller, user-data and DB schema updates for Keycloak subject storage, DI/service wiring, extensive tests, and documentation. Changes
Sequence DiagramssequenceDiagram
participant User as Browser
participant App as phpMyFAQ
participant Session as SessionStore
participant KC as Keycloak
participant JWKS as JWKS
User->>App: GET /auth/keycloak/authorize
App->>App: gen state, nonce, pkce_verifier/challenge
App->>Session: store(state, nonce, verifier)
App->>User: 302 redirect -> Keycloak /authorize (client_id, state, nonce, code_challenge)
User->>KC: Authenticate → approve
KC->>User: 302 redirect -> /auth/keycloak/callback?code=...&state=...
User->>App: GET /auth/keycloak/callback?code&state
App->>Session: validate state == stored
App->>KC: POST /token (code, code_verifier, client credentials)
KC->>App: access_token, id_token, refresh_token
App->>JWKS: GET jwks_uri
JWKS->>App: JWKS keys
App->>App: validate id_token signature & claims (iss,aud,nonce,exp)
App->>KC: GET /userinfo (Authorization: Bearer ...)
KC->>App: user claims (sub,email,roles)
App->>App: resolve/provision local user, map roles→groups, sync groups
App->>Session: set logged-in session + store tokens
App->>User: Redirect to app home
sequenceDiagram
participant User as Browser
participant App as phpMyFAQ
participant Session as SessionStore
participant KC as Keycloak
User->>App: GET /logout
App->>Session: get auth source, id_token
alt auth source == keycloak and id_token present
App->>KC: redirect -> end_session_endpoint?client_id&post_logout_redirect_uri&id_token_hint
else
App->>User: redirect -> app default URL
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Auth/Keycloak/KeycloakProviderConfigFactory.php`:
- Around line 35-50: The code currently returns an enabled OidcProviderConfig
even when keycloak.enable is true but required discovery inputs are missing;
update the KeycloakProviderConfigFactory method to validate inputs after reading
$baseUrl and $realm: call
$this->toBool($this->configuration->get('keycloak.enable')) into a local
$enabled flag and if $enabled is true and either $baseUrl === '' or $realm ===
'' then throw a clear InvalidArgumentException (or RuntimeException) explaining
which field(s) are missing (e.g. "Keycloak enabled but missing baseUrl and/or
realm"), so the failure is raised immediately before calling buildDiscoveryUrl
and returning the OidcProviderConfig.
- Around line 52-53: The client secret is being trimmed which can corrupt valid
secrets; in KeycloakProviderConfigFactory where the provider config array is
built (the entries using $this->configuration->get('keycloak.clientId') and
$this->configuration->get('keycloak.clientSecret')), remove the trim() around
the clientSecret value and keep only the (string) cast so the secret is
preserved exactly, while leaving trim() on clientId intact if desired.
In `@phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcDiscoveryService.php`:
- Around line 50-57: The decoded JSON payload may be a scalar
(string/number/null) which will cause a TypeError when calling
OidcDiscoveryDocument::fromArray(array $data); add an explicit check after
json_decode to verify is_array($payload) and if not throw a RuntimeException
like "OIDC discovery response is not a JSON object/array" (include
gettype($payload) in the message for debugging) so callers get a controlled
error instead of an unhandled TypeError; ensure this happens in
OidcDiscoveryService right before the OidcDiscoveryDocument::fromArray call and
preserve the previous exception where appropriate.
In `@phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcPkceGenerator.php`:
- Around line 24-43: Validate PKCE verifier length and characters in both
generateVerifier(int $length = 128) and generateChallenge(string $verifier):
check that requested $length is between 43 and 128 inclusive and throw an
InvalidArgumentException for out-of-range lengths; in generateChallenge verify
the supplied $verifier is non-empty, its strlen is between 43 and 128 and
contains only the allowed characters (0-9a-zA-Z-._~), throwing
InvalidArgumentException on failure, then proceed with existing challenge
computation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f28181f7-1647-4438-b13a-7ce0e7f32ee2
📒 Files selected for processing (24)
phpmyfaq/admin/assets/src/configuration/configuration.test.tsphpmyfaq/admin/assets/src/configuration/configuration.tsphpmyfaq/assets/templates/admin/configuration/main.twigphpmyfaq/assets/templates/admin/configuration/tab-list.twigphpmyfaq/src/phpMyFAQ/Auth/Keycloak/KeycloakProviderConfigFactory.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcClientConfig.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcDiscoveryDocument.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcDiscoveryService.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcPkceGenerator.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcProviderConfig.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcSession.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationTabController.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.phpphpmyfaq/src/services.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Auth/Keycloak/KeycloakProviderConfigFactoryTest.phptests/phpMyFAQ/Auth/Oidc/OidcDiscoveryServiceTest.phptests/phpMyFAQ/Auth/Oidc/OidcPkceGeneratorTest.phptests/phpMyFAQ/Auth/Oidc/OidcSessionTest.phptests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.phptests/phpMyFAQ/Controller/Administration/ConfigurationControllerWebTest.phptests/phpMyFAQ/TranslationTest.php
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (9)
tests/phpMyFAQ/TranslationTest.php (1)
211-224: Assert the full Keycloak key set.This test currently checks the count plus only part of the expected keys. A missing expected key could be masked by an extra unexpected
keycloak.*entry while still passingassertCount(11).Proposed test tightening
public function testGetConfigurationItemsReturnsKeycloakSection(): void { $configurationItems = Translation::getConfigurationItems('keycloak.'); - $this->assertCount(11, $configurationItems); - $this->assertArrayHasKey('keycloak.enable', $configurationItems); + $expectedKeys = [ + 'keycloak.enable', + 'keycloak.baseUrl', + 'keycloak.realm', + 'keycloak.clientId', + 'keycloak.clientSecret', + 'keycloak.redirectUri', + 'keycloak.scopes', + 'keycloak.autoProvision', + 'keycloak.groupAutoAssign', + 'keycloak.groupMapping', + 'keycloak.logoutRedirectUrl', + ]; + + $this->assertSame($expectedKeys, array_keys($configurationItems)); $this->assertSame('checkbox', $configurationItems['keycloak.enable']['element']); - $this->assertArrayHasKey('keycloak.clientSecret', $configurationItems); $this->assertSame('password', $configurationItems['keycloak.clientSecret']['element']); - $this->assertArrayHasKey('keycloak.autoProvision', $configurationItems); - $this->assertArrayHasKey('keycloak.groupAutoAssign', $configurationItems); - $this->assertArrayHasKey('keycloak.groupMapping', $configurationItems); - $this->assertArrayHasKey('keycloak.logoutRedirectUrl', $configurationItems); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/TranslationTest.php` around lines 211 - 224, The test testGetConfigurationItemsReturnsKeycloakSection currently only checks count and a subset of keys; replace those partial assertions with a strict key-set assertion: call Translation::getConfigurationItems('keycloak.') and assert that array_keys($configurationItems) exactly match the expected list of Keycloak keys (e.g. 'keycloak.enable','keycloak.clientSecret','keycloak.autoProvision','keycloak.groupAutoAssign','keycloak.groupMapping','keycloak.logoutRedirectUrl', plus the other expected keys) so any missing or extra keycloak.* entries fail; keep the existing checks for element types (like 'checkbox' and 'password') but remove the loose assertCount/assertArrayHasKey pattern in testGetConfigurationItemsReturnsKeycloakSection to ensure the full set is validated.docs/administration.md (3)
889-889: Hyphenate "post-logout" for consistency with OAuth/OIDC terminology.The phrase "post logout redirect URI" should use a hyphen: "post-logout redirect URI". This aligns with standard OAuth 2.0 and OpenID Connect terminology.
📝 Proposed fix
-- Valid post logout redirect URI: `https://faq.example.com/` +- Valid post-logout redirect URI: `https://faq.example.com/`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/administration.md` at line 889, Update the phrase "Valid post logout redirect URI: `https://faq.example.com/`" to use the hyphenated form "post-logout" so it reads "Valid post-logout redirect URI: `https://faq.example.com/`"; locate the exact text "Valid post logout redirect URI" and insert the hyphen between "post" and "logout" to match OAuth/OIDC terminology.
916-916: Remove or clarify "in the current implementation" qualifier.The phrase "in the current implementation" suggests the behavior may change in future versions, which can create uncertainty for administrators. Consider either:
- Removing the qualifier if the additive behavior is the intended long-term design
- Adding a clear note about planned changes if a different model is coming
💡 Suggested revision
If the additive behavior is intended to stay:
-Group assignment is additive in the current implementation: +Group assignment is additive:If changes are planned, add a note:
Group assignment is additive in the current implementation: + +> [!NOTE] +> Future versions may support synchronized group removal. Check the changelog when upgrading.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/administration.md` at line 916, Edit the sentence "Group assignment is additive in the current implementation:" in docs/administration.md to either remove the qualifier or clarify intent: either change it to "Group assignment is additive." if additive behavior is the long-term design, or append a short note such as "Group assignment is additive; this behavior is expected to remain stable." or "Group assignment is additive; a different model is planned (see roadmap)." to explicitly state planned changes. Target the exact sentence "Group assignment is additive in the current implementation:" when making the change.
917-921: Consider adding a security note about automatic group assignment.Automatic group creation and assignment from Keycloak roles can have security implications if Keycloak role membership isn't tightly controlled. Administrators should understand that any Keycloak role (mapped or unmapped) can create phpMyFAQ groups and grant access.
📋 Suggested security note
Add after line 921:
> [!WARNING] > Automatic group assignment grants phpMyFAQ access based on Keycloak roles. Ensure your Keycloak role membership is properly controlled to prevent unintended privilege escalation. Review the group-to-permission mappings in phpMyFAQ after enabling this feature.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/administration.md` around lines 917 - 921, Add a security warning immediately after the paragraph/list that ends with "existing phpMyFAQ group memberships are not removed automatically": insert the provided warning block that explains automatic group assignment from Keycloak roles can grant phpMyFAQ access and advises administrators to tightly control Keycloak role membership and review group-to-permission mappings; ensure the warning uses the markdown admonition format shown (a [!WARNING] block) and is placed directly after the existing list about Keycloak role/group behavior.tests/phpMyFAQ/Auth/AuthKeycloakTest.php (1)
123-135: Assert both expected groups are assigned.The current matcher would still pass if
addToGroup()were called twice with group10and never with11. Capture the IDs and assert the exact set/order.Proposed test tightening
+ $addedGroupIds = []; $permission ->expects($this->exactly(2)) ->method('addToGroup') - ->with($this->equalTo(42), $this->logicalOr($this->equalTo(10), $this->equalTo(11))); + ->with( + $this->equalTo(42), + $this->callback(static function (int $groupId) use (&$addedGroupIds): bool { + $addedGroupIds[] = $groupId; + return in_array($groupId, [10, 11], true); + }), + ); @@ $this->assertTrue($auth->create('john', '')); + $this->assertSame([10, 11], $addedGroupIds);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Auth/AuthKeycloakTest.php` around lines 123 - 135, The test's current mock for MediumPermission allows addToGroup to be called twice with the same group ID (e.g., 10) and still pass; change the mock to record the group IDs passed to addToGroup and then assert the recorded values contain exactly the two expected IDs (10 and 11) (or assert the set equals {10,11}) instead of using the logicalOr matcher; specifically modify the MediumPermission mock behavior for addToGroup (and keep findOrCreateGroupByName callback) to push the second argument into a local array and after the exercise assert that array contains both 10 and 11 (and if order matters, assert the exact order).tests/phpMyFAQ/Controller/Frontend/AuthenticationControllerWebTest.php (1)
59-70: Keep positive coverage for WebAuthn rendering.This test now only proves registration renders when enabled. Since the PR decouples passkey rendering from other SSO buttons, add a separate positive assertion for
security.enableWebAuthnSupport.Proposed test addition
public function testLoginPageShowsRegistrationActionWhenEnabled(): void { $this->getConfiguration()->getAll(); $this->overrideConfigurationValues([ 'main.enableUserTracking' => false, @@ self::assertResponseIsSuccessful($response); self::assertResponseContains('href="user/register"', $response); } + + public function testLoginPageShowsPasskeyActionWhenWebAuthnIsEnabled(): void + { + $this->getConfiguration()->getAll(); + $this->overrideConfigurationValues([ + 'main.enableUserTracking' => false, + 'security.enableWebAuthnSupport' => true, + ]); + + $response = $this->requestPublic('GET', '/login'); + + self::assertResponseIsSuccessful($response); + self::assertResponseContains('./services/webauthn', $response); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Frontend/AuthenticationControllerWebTest.php` around lines 59 - 70, The test testLoginPageShowsRegistrationActionWhenEnabled currently only asserts registration rendering; enable security.enableWebAuthnSupport in the overridden configuration and add a positive assertion that the response contains the WebAuthn/passkey indicator (e.g. a WebAuthn-specific link or text) using the same requestPublic(...) response and assertResponseContains(...) helper so the test also verifies WebAuthn UI is rendered when security.enableWebAuthnSupport is true.tests/phpMyFAQ/Auth/Oidc/OidcClientTest.php (1)
44-51: Assert the authorization query structurally.Substring checks can miss malformed queries and do not cover required OIDC/PKCE parameters such as
response_type,redirect_uri, andcode_challenge_method.Proposed stronger assertions
$url = $client->buildAuthorizationUrl($config, $discoveryDocument, 'state-123', 'nonce-456', 'challenge-789'); $this->assertStringStartsWith('https://sso.example.test/auth?', $url); - $this->assertStringContainsString('client_id=phpmyfaq', $url); - $this->assertStringContainsString('state=state-123', $url); - $this->assertStringContainsString('nonce=nonce-456', $url); - $this->assertStringContainsString('code_challenge=challenge-789', $url); - $this->assertStringContainsString('scope=openid+profile+email', $url); + parse_str((string) parse_url($url, PHP_URL_QUERY), $query); + + $this->assertSame('code', $query['response_type']); + $this->assertSame('phpmyfaq', $query['client_id']); + $this->assertSame('https://faq.example.test/auth/keycloak/callback', $query['redirect_uri']); + $this->assertSame('state-123', $query['state']); + $this->assertSame('nonce-456', $query['nonce']); + $this->assertSame('challenge-789', $query['code_challenge']); + $this->assertSame('S256', $query['code_challenge_method']); + $this->assertSame('openid profile email', $query['scope']);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Auth/Oidc/OidcClientTest.php` around lines 44 - 51, The test currently only checks substrings of the authorization URL; update the OidcClientTest::test... that calls buildAuthorizationUrl to parse the generated URL (use PHP's parse_url and parse_str) and assert the query parameters structurally include response_type=code, client_id=phpmyfaq, redirect_uri matching the config value, state=state-123, nonce=nonce-456, code_challenge=challenge-789, code_challenge_method=S256 (or whatever the client should emit), and scope contains openid, profile, and email; also assert the base URL/host equals https://sso.example.test/auth to ensure the path is correct. Ensure you reference buildAuthorizationUrl and the $config/discoveryDocument variables when extracting expected values.tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php (1)
127-127: Couple the assertion to the enum to avoid silent drift.Nit: hard-coding
'keycloak'duplicates the source of truth inAuthenticationSourceType::AUTH_KEYCLOAK. PreferAuthenticationSourceType::AUTH_KEYCLOAK->valueso a future rename cannot break this test silently.- $currentUser->expects($this->once())->method('setAuthSource')->with('keycloak'); + $currentUser->expects($this->once())->method('setAuthSource') + ->with(AuthenticationSourceType::AUTH_KEYCLOAK->value);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php` at line 127, The test assertion hard-codes 'keycloak' which can drift; update the expectation on $currentUser->setAuthSource to use the enum value instead by replacing the literal with AuthenticationSourceType::AUTH_KEYCLOAK->value so the test references the single source of truth (keep the $currentUser->expects($this->once())->method('setAuthSource') call but pass the enum value).phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php (1)
78-88: Usewarning/errorlog level for auth failures.Authorization failures (unreachable discovery endpoint, cert errors, misconfiguration) are not informational — downgrading them to
infowill hide regressions in typical log configurations. Considerwarningfor user-facing misconfig anderrorfor exceptions.- $this->configuration - ->getLogger() - ->info(sprintf( + $this->configuration + ->getLogger() + ->error(sprintf( 'Keycloak login failed: %s at line %d at %s',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php` around lines 78 - 88, The catch block in KeycloakAuthenticationController currently logs auth exceptions at info level; change the logger call in the exception handler (the $this->configuration->getLogger()->info(...) inside the KeycloakAuthenticationController login flow) to a more appropriate severity — use $this->configuration->getLogger()->warning(...) for user-facing/misconfiguration cases or $this->configuration->getLogger()->error(...) for actual exceptions — keeping the same formatted message (exception message, line, file) so failures are surfaced in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/admin/assets/src/configuration/configuration.test.ts`:
- Around line 297-299: Add a test in configuration.test.ts that specifically
asserts Keycloak filtering/tab behavior: simulate entering the filter string
"keycloak" (or "client id") and verify that elements with
data-config-group="integrations" and data-config-label="Keycloak" (or the tab
anchor href="#keycloak") are visible while non-matching config items are hidden;
use the existing fixtures added around lines where Keycloak is introduced to
target the Keycloak tab/content and assert show/hide behavior rather than only
relying on fixture presence.
In `@phpmyfaq/assets/templates/default/login.twig`:
- Around line 77-82: The Keycloak sign-in button (inside the
useSignInWithKeycloak block, the <a> anchor rendering the Keycloak button) uses
btn-outline-warning which yields low contrast; change its classes to a
higher-contrast variant (for example replace btn-outline-warning with a filled
btn-warning or a darker variant like btn-dark and ensure w-100 py-2 mb-2 btn
rounded-3 remains) so the icon and label meet accessibility contrast
requirements; update the class string on that anchor in login.twig and verify
visual contrast on the white card footer.
In `@phpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.php`:
- Around line 54-69: The code proceeds to set status, auth source, user data and
assign groups even when createUser() fails or returns false; change the flow in
the createUser handling so that after attempting $result =
$user->createUser($login, '', $domain) you immediately return/stop further
processing if an exception is caught or if $result is falsy. Specifically,
inside the catch for createUser ensure you log the exception (using
$this->configuration->getLogger()) and then return early, and after the
try/catch add a check like if (!$result) return; before calling
$user->setStatus(...), $user->setAuthSource(...), $user->setUserData(...),
shouldAssignGroups() and assignUserToGroups($user->getUserId()).
- Around line 158-206: extractRoleNames currently pulls roles from all
resource_access entries and the group-assignment loop uses unmapped role names
to create groups and logs $this->resolvedLogin; restrict extraction to the
configured Keycloak client only (use the configured client id property, e.g.
$this->clientId) by reading resource_access[$this->clientId]['roles'] and realm
roles as before, and change the group-assignment logic (the foreach over
$roleNames that calls findOrCreateGroupByName/addToGroup) to only act on roles
that exist in the explicit $groupMapping (skip any role not mapped rather than
creating a new group); also remove or redact $this->resolvedLogin from the info
log (use $userId or omit personal identifiers) when calling the logger.
In `@phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcClient.php`:
- Around line 136-149: The decodeJsonResponse method currently decodes JSON but
doesn't verify the result is a JSON object/array; after json_decode in
OidcClient::decodeJsonResponse add an is_array() check (like the one in
OidcDiscoveryService) and if the payload is not an array throw a
RuntimeException indicating "OIDC {context} response is not a JSON object/array"
(include the $context in the message and chain the JsonException as previous
when applicable) so the method truly returns array<string,mixed>.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php`:
- Around line 153-191: Replace direct disclosure of exception internals and
plain-text responses by logging the full exception server-side and returning a
generic user-facing redirect/flash response; specifically, in
KeycloakAuthenticationController where you call $auth->isValidLogin($login) and
$auth->checkCredentials($login, ''), stop returning new Response('Login not
valid.') / new Response('Credentials not valid.') and instead clear
authorization state then redirect back to the login flow with a flash/error
message; in the catch(Exception $exception) block, remove $exception->getFile()
and $exception->getLine() from the HTTP response, call the server-side logger to
log $exception (e.g. $this->logger->error or error_log with the exception),
clear authorization state, and return a generic redirect or error page
consistent with authorize()/logout() behavior.
- Around line 163-168: getUserByLogin() is being called without checking its
result and the code unconditionally marks the user as logged in; change the
sequence in KeycloakAuthenticationController to capture and check the return
value of $this->getCurrentUserService()->getUserByLogin($login) (or assign the
result to a variable) and only call setLoggedIn(true),
setAuthSource(AuthenticationSourceType::AUTH_KEYCLOAK->value),
updateSessionId(true) and saveToSession() when the lookup/provisioning actually
succeeded; on failure, abort the login flow (e.g. log the failure and
return/error) so a non-existent or non-provisioned account is not marked as
logged in.
- Around line 91-107: The logout() action currently passes an empty id_token to
oidcClient->buildLogoutUrl and doesn't invalidate the local session; update
KeycloakAuthenticationController::logout to retrieve the stored id_token from
the current user (use the same session key or accessor used in the callback
flow) and pass that token instead of '' to
$this->oidcClient->buildLogoutUrl($providerConfig, $discoveryDocument,
$idToken); and before returning the RedirectResponse (and before calling
external IdP logout) call $this->currentUser->deleteFromSession() to destroy the
local session following the same pattern used in AuthenticationController so the
user is logged out locally even if IdP logout is cancelled.
- Around line 120-126: The error null-check logic in
KeycloakAuthenticationController incorrectly treats a missing error_description
as non-empty because Filter::filterVar(...) returns null by default; update the
Filter::filterVar call that sets $error to supply an explicit empty-string
default (e.g., third parameter '') so $error is '' when absent, then change the
if ($error !== '') branch to log the warning via
$this->configuration->getLogger()->warning(...) and return a RedirectResponse to
$this->configuration->getDefaultUrl() (instead of returning an empty
Response()); apply the same default-value fix to the other Filter::filterVar
calls (and mirror the same fix in AzureAuthenticationController.php) so
successful OIDC callbacks proceed normally.
In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php`:
- Around line 107-115: The test method
testCallbackReturnsErrorResponseWhenProviderErrorIsSet should be updated to
match the controller's new behavior: call
KeycloakAuthenticationController::callback with a Request containing
'error_description' and assert that the result is a RedirectResponse (not a
plain 200 Response), assert the status code is a redirect (e.g., 302), assert
the session/flash bag contains the error message ('Denied by provider') rather
than comparing Response::getContent(), and if a logger mock is injected into the
controller assert the logger was called with the error; update assertions in the
test accordingly (replace checks against Response::class and getContent() with
checks for RedirectResponse, flash message presence, redirect location and
logger invocation).
---
Nitpick comments:
In `@docs/administration.md`:
- Line 889: Update the phrase "Valid post logout redirect URI:
`https://faq.example.com/`" to use the hyphenated form "post-logout" so it reads
"Valid post-logout redirect URI: `https://faq.example.com/`"; locate the exact
text "Valid post logout redirect URI" and insert the hyphen between "post" and
"logout" to match OAuth/OIDC terminology.
- Line 916: Edit the sentence "Group assignment is additive in the current
implementation:" in docs/administration.md to either remove the qualifier or
clarify intent: either change it to "Group assignment is additive." if additive
behavior is the long-term design, or append a short note such as "Group
assignment is additive; this behavior is expected to remain stable." or "Group
assignment is additive; a different model is planned (see roadmap)." to
explicitly state planned changes. Target the exact sentence "Group assignment is
additive in the current implementation:" when making the change.
- Around line 917-921: Add a security warning immediately after the
paragraph/list that ends with "existing phpMyFAQ group memberships are not
removed automatically": insert the provided warning block that explains
automatic group assignment from Keycloak roles can grant phpMyFAQ access and
advises administrators to tightly control Keycloak role membership and review
group-to-permission mappings; ensure the warning uses the markdown admonition
format shown (a [!WARNING] block) and is placed directly after the existing list
about Keycloak role/group behavior.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php`:
- Around line 78-88: The catch block in KeycloakAuthenticationController
currently logs auth exceptions at info level; change the logger call in the
exception handler (the $this->configuration->getLogger()->info(...) inside the
KeycloakAuthenticationController login flow) to a more appropriate severity —
use $this->configuration->getLogger()->warning(...) for
user-facing/misconfiguration cases or
$this->configuration->getLogger()->error(...) for actual exceptions — keeping
the same formatted message (exception message, line, file) so failures are
surfaced in logs.
In `@tests/phpMyFAQ/Auth/AuthKeycloakTest.php`:
- Around line 123-135: The test's current mock for MediumPermission allows
addToGroup to be called twice with the same group ID (e.g., 10) and still pass;
change the mock to record the group IDs passed to addToGroup and then assert the
recorded values contain exactly the two expected IDs (10 and 11) (or assert the
set equals {10,11}) instead of using the logicalOr matcher; specifically modify
the MediumPermission mock behavior for addToGroup (and keep
findOrCreateGroupByName callback) to push the second argument into a local array
and after the exercise assert that array contains both 10 and 11 (and if order
matters, assert the exact order).
In `@tests/phpMyFAQ/Auth/Oidc/OidcClientTest.php`:
- Around line 44-51: The test currently only checks substrings of the
authorization URL; update the OidcClientTest::test... that calls
buildAuthorizationUrl to parse the generated URL (use PHP's parse_url and
parse_str) and assert the query parameters structurally include
response_type=code, client_id=phpmyfaq, redirect_uri matching the config value,
state=state-123, nonce=nonce-456, code_challenge=challenge-789,
code_challenge_method=S256 (or whatever the client should emit), and scope
contains openid, profile, and email; also assert the base URL/host equals
https://sso.example.test/auth to ensure the path is correct. Ensure you
reference buildAuthorizationUrl and the $config/discoveryDocument variables when
extracting expected values.
In `@tests/phpMyFAQ/Controller/Frontend/AuthenticationControllerWebTest.php`:
- Around line 59-70: The test testLoginPageShowsRegistrationActionWhenEnabled
currently only asserts registration rendering; enable
security.enableWebAuthnSupport in the overridden configuration and add a
positive assertion that the response contains the WebAuthn/passkey indicator
(e.g. a WebAuthn-specific link or text) using the same requestPublic(...)
response and assertResponseContains(...) helper so the test also verifies
WebAuthn UI is rendered when security.enableWebAuthnSupport is true.
In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php`:
- Line 127: The test assertion hard-codes 'keycloak' which can drift; update the
expectation on $currentUser->setAuthSource to use the enum value instead by
replacing the literal with AuthenticationSourceType::AUTH_KEYCLOAK->value so the
test references the single source of truth (keep the
$currentUser->expects($this->once())->method('setAuthSource') call but pass the
enum value).
In `@tests/phpMyFAQ/TranslationTest.php`:
- Around line 211-224: The test testGetConfigurationItemsReturnsKeycloakSection
currently only checks count and a subset of keys; replace those partial
assertions with a strict key-set assertion: call
Translation::getConfigurationItems('keycloak.') and assert that
array_keys($configurationItems) exactly match the expected list of Keycloak keys
(e.g.
'keycloak.enable','keycloak.clientSecret','keycloak.autoProvision','keycloak.groupAutoAssign','keycloak.groupMapping','keycloak.logoutRedirectUrl',
plus the other expected keys) so any missing or extra keycloak.* entries fail;
keep the existing checks for element types (like 'checkbox' and 'password') but
remove the loose assertCount/assertArrayHasKey pattern in
testGetConfigurationItemsReturnsKeycloakSection to ensure the full set is
validated.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f8c58077-5e0d-4333-8737-a5e39ffaaacc
📒 Files selected for processing (34)
docs/administration.mddocs/configuration.mddocs/index.mdphpmyfaq/admin/assets/src/configuration/configuration.test.tsphpmyfaq/assets/templates/admin/login.twigphpmyfaq/assets/templates/default/login.twigphpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.phpphpmyfaq/src/phpMyFAQ/Auth/Keycloak/KeycloakProviderConfigFactory.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcClient.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcDiscoveryService.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcPkceGenerator.phpphpmyfaq/src/phpMyFAQ/Configuration/SecuritySettings.phpphpmyfaq/src/phpMyFAQ/ConfigurationMethodsTrait.phpphpmyfaq/src/phpMyFAQ/Controller/AbstractController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationTabController.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/AuthenticationController.phpphpmyfaq/src/phpMyFAQ/Controller/Frontend/AuthenticationController.phpphpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.phpphpmyfaq/src/phpMyFAQ/Enums/AuthenticationSourceType.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.phpphpmyfaq/src/services.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Auth/AuthKeycloakTest.phptests/phpMyFAQ/Auth/Oidc/OidcClientTest.phptests/phpMyFAQ/Configuration/ConfigurationMethodsTraitTest.phptests/phpMyFAQ/Configuration/SettingsValueObjectsTest.phptests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.phptests/phpMyFAQ/Controller/Administration/AuthenticationControllerTest.phptests/phpMyFAQ/Controller/Frontend/AuthenticationControllerTest.phptests/phpMyFAQ/Controller/Frontend/AuthenticationControllerWebTest.phptests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.phptests/phpMyFAQ/TranslationTest.php
✅ Files skipped from review due to trivial changes (8)
- docs/index.md
- phpmyfaq/src/phpMyFAQ/Enums/AuthenticationSourceType.php
- phpmyfaq/src/phpMyFAQ/Controller/AbstractController.php
- phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php
- phpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.php
- docs/configuration.md
- phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcDiscoveryService.php
- phpmyfaq/src/phpMyFAQ/Auth/Keycloak/KeycloakProviderConfigFactory.php
🚧 Files skipped from review as they are similar to previous changes (5)
- phpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationTabController.php
- tests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.php
- phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcPkceGenerator.php
- phpmyfaq/translations/language_de.php
- phpmyfaq/translations/language_en.php
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php (1)
338-346:⚠️ Potential issue | 🟡 MinorAdd the Keycloak subject lookup index for MySQLi installs.
Fresh MySQLi installs should get the same
keycloak_subindex as migrated databases.Proposed fix
'faquserdata' => 'CREATE TABLE %sfaquserdata ( user_id INT(11) NOT NULL, last_modified VARCHAR(14) NULL, display_name VARCHAR(128) NULL, email VARCHAR(128) NULL, keycloak_sub VARCHAR(255) NULL, is_visible INT(1) NULL DEFAULT 0, twofactor_enabled INT(1) NULL DEFAULT 0, secret VARCHAR(128) NULL DEFAULT NULL)', + 'faquserdata_keycloak_sub_idx' => 'CREATE INDEX idx_faquserdata_keycloak_sub + ON %sfaquserdata (keycloak_sub)',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php` around lines 338 - 346, The MySQLi schema for the faquserdata table (array entry 'faquserdata' in class Mysqli) creates the keycloak_sub column but doesn't add the lookup index; add a non-unique index for keycloak_sub for fresh installs by including an index creation step tied to %sfaquserdata (e.g. either add an index definition in the table DDL or run "CREATE INDEX" / "ALTER TABLE ... ADD KEY keycloak_sub (keycloak_sub)" immediately after creating %sfaquserdata) so the new MySQLi installs match migrated DBs that use keycloak_sub lookups.phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php (1)
686-697:⚠️ Potential issue | 🟡 MinorAdd the
keycloak_sublookup index to the canonical schema.This is the fresh-install source of truth, so it should include the same
keycloak_subindex as the migration path.Proposed fix
->varchar('email', 128) ->varchar('keycloak_sub', 255) ->smallInteger('is_visible', true, 0) ->smallInteger('twofactor_enabled', true, 0) - ->varchar('secret', 128); + ->varchar('secret', 128) + ->index('idx_faquserdata_keycloak_sub', 'keycloak_sub');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php` around lines 686 - 697, The canonical schema for faquserdata is missing the keycloak_sub lookup index; update the faquserdata() TableBuilder chain in the DatabaseSchema class to add the same index used in the migration path for the keycloak_sub column (i.e., add an index/lookup entry for 'keycloak_sub' on the TableBuilder returned by faquserdata()) so fresh installs include the lookup index alongside the existing columns.phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php (1)
327-335:⚠️ Potential issue | 🟡 MinorAdd the missing Keycloak subject index to maintain schema consistency with migrations.
The migration (Migration420Alpha.php) creates both the
keycloak_subcolumn and an indexidx_faquserdata_keycloak_sub, but all fresh-install schemas only include the column. This divergence can cause performance degradation on fresh installations during Keycloak lookups. Add the index to align fresh installs with migrated databases.Proposed fix
'faquserdata' => 'CREATE TABLE %sfaquserdata ( user_id INTEGER NOT NULL, last_modified NVARCHAR(14) NULL, display_name NVARCHAR(128) NULL, email NVARCHAR(128) NULL, keycloak_sub NVARCHAR(255) NULL, is_visible INTEGER NULL DEFAULT 0, twofactor_enabled INTEGER NULL DEFAULT 0, secret NVARCHAR(128) NULL DEFAULT NULL)', + 'faquserdata_keycloak_sub_idx' => 'CREATE INDEX idx_faquserdata_keycloak_sub ON %sfaquserdata (keycloak_sub)',🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php` around lines 327 - 335, The faquserdata table schema in PdoSqlsrv.php is missing the Keycloak subject index; add a CREATE INDEX statement named idx_faquserdata_keycloak_sub for the keycloak_sub column to the schema definitions so fresh installs match Migration420Alpha.php (index name: idx_faquserdata_keycloak_sub, target table symbol %sfaquserdata, column keycloak_sub, SQL Server CREATE INDEX ... ON ... (keycloak_sub)). Ensure the index creation uses the same identifier and column as Migration420Alpha.php to keep schemas consistent.phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php (1)
337-345:⚠️ Potential issue | 🟡 MinorAdd the Keycloak subject index to fresh MySQL installs.
The migration creates
idx_faquserdata_keycloak_sub, butcreateTables()for new MySQL instances only adds the column. Fresh installs will miss the lookup index used by Keycloak subject resolution.Proposed schema parity fix
'faquserdata' => 'CREATE TABLE %sfaquserdata ( user_id INT(11) NOT NULL, last_modified VARCHAR(14) NULL, display_name VARCHAR(128) NULL, email VARCHAR(128) NULL, keycloak_sub VARCHAR(255) NULL, is_visible INT(1) NULL DEFAULT 0, twofactor_enabled INT(1) NULL DEFAULT 0, secret VARCHAR(128) NULL DEFAULT NULL)', + 'faquserdata_keycloak_sub_idx' => 'CREATE INDEX idx_faquserdata_keycloak_sub ON %sfaquserdata (keycloak_sub)', 'faquserlogin' => 'CREATE TABLE %sfaquserlogin (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php` around lines 337 - 345, The fresh-install CREATE TABLE for 'faquserdata' in PdoMysql::createTables() adds the keycloak_sub column but not the index used by migrations; update the CREATE TABLE statement for 'faquserdata' to include the idx_faquserdata_keycloak_sub index on keycloak_sub (matching the migration) so new MySQL installs get the same lookup index as upgrades.
🧹 Nitpick comments (8)
docs/keycloak.md (1)
28-28: Minor: hyphenation inconsistency for "post-logout".Line 28 uses "Valid post logout redirect URIs" while line 105 uses the hyphenated "post-logout redirect URI". Consider hyphenating here for consistency (the Keycloak admin console label itself is "Valid post logout redirect URIs" without hyphen, so either keep both unhyphenated to match the UI exactly, or hyphenate both — just align them).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/keycloak.md` at line 28, The phrase "Valid post logout redirect URIs" is inconsistent with "post-logout redirect URI" elsewhere; update the text that currently reads "Valid post logout redirect URIs" to use the hyphenated form "Valid post-logout redirect URIs" (or alternatively change the other occurrence to remove the hyphen) so both occurrences (search for the exact strings "Valid post logout redirect URIs" and "post-logout redirect URI") match consistently throughout the document..github/workflows/build.yml (2)
65-71: Pin MkDocs (and any plugins) for reproducible--strictbuilds.
pip install --upgrade pip mkdocspulls the latest MkDocs on every run. Since the build uses--strict, a new upstream release that tightens warnings (deprecated config keys, plugin changes, etc.) can turn green PRs red without any code change in this repo. Consider pinning to a known-good version, ideally via adocs/requirements.txt.♻️ Suggested change
- - name: Install MkDocs - run: python -m pip install --upgrade pip mkdocs + - name: Install MkDocs + run: | + python -m pip install --upgrade pip + python -m pip install "mkdocs==1.6.*"Or, preferably, track dependencies in
docs/requirements.txtandpip install -r docs/requirements.txtso any MkDocs plugins you add later are versioned alongside MkDocs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 65 - 71, The workflow currently installs MkDocs dynamically in the "Install MkDocs" step which can break reproducible `--strict` docs builds; change the step to install pinned versions instead by creating a docs/requirements.txt that pins mkdocs (and plugins) to known-good versions and updating the "Install MkDocs" step (the job named "Setup Python for docs build" and the step titled "Install MkDocs") to run pip install -r docs/requirements.txt (or explicitly pin mkdocs==X.Y.Z in the workflow if you prefer a quick fix).
65-103: Docs build runs 3× across the PHP matrix — consider extracting to a dedicated job.The documentation build has no PHP dependency, yet it will execute once per matrix entry (PHP 8.4, 8.5, 8.6). Splitting it into its own job (or gating with
if: matrix.php-versions == '8.4') shortens CI time, removes redundant Python setup on PHP-only runners, and makes docs failures easier to spot.♻️ Example: gate within the existing job
- name: Setup Python for docs build + if: matrix.php-versions == '8.4' uses: actions/setup-python@v6 with: python-version: '3.x' - name: Install MkDocs + if: matrix.php-versions == '8.4' run: python -m pip install --upgrade pip mkdocs ... - name: Build documentation + if: matrix.php-versions == '8.4' run: mkdocs build --strictA separate top-level
docsjob running in parallel is cleaner still.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build.yml around lines 65 - 103, The docs build is executed for every PHP matrix entry; extract the Python/pnpm/mkdocs-related steps (the steps with names "Setup Python for docs build", "Install MkDocs", "Setup pnpm", "Get pnpm store directory", "Setup pnpm cache", "Install Node.js dependencies", "Run ESLint check", "Run Vitest tests", "Build documentation") into a dedicated GitHub Actions job (e.g., job name "docs") that runs once (or alternatively add a gate like if: matrix.php-versions == '8.4' to the current job) so the docs pipeline runs independently and only once, moving the steps and necessary uses/with blocks into that new job and wiring any required cache keys/outputs accordingly.tests/phpMyFAQ/Controller/Frontend/AzureAuthenticationControllerTest.php (1)
122-137: Rename the test to match the redirect behavior.The assertions now verify a default redirect, not an error response.
Proposed rename
- public function testCallbackReturnsErrorResponseWhenProviderErrorIsSet(): void + public function testCallbackRedirectsToDefaultUrlWhenProviderErrorIsSet(): void🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Frontend/AzureAuthenticationControllerTest.php` around lines 122 - 137, Rename the test method testCallbackReturnsErrorResponseWhenProviderErrorIsSet to reflect that it expects a default redirect instead of an error response (e.g., testCallbackRedirectsToDefaultUrlWhenProviderErrorIsSet); update only the test method name in AzureAuthenticationControllerTest so it matches the assertions that callback(Request with 'error_description') returns a RedirectResponse whose Location equals $this->configuration->getDefaultUrl(), leaving the rest of the test (controller instantiation and assertions) unchanged.tests/phpMyFAQ/Permission/MediumPermissionRepositoryTest.php (1)
223-228: Add a positive removal-path assertion.This only covers guard clauses. Since Keycloak sync depends on membership removal, add a fixture-backed case that adds a user to a group, calls
removeFromGroup(), and verifies the membership is gone.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Permission/MediumPermissionRepositoryTest.php` around lines 223 - 228, Extend testRemoveFromGroup to include a positive case that uses test fixtures to create a real membership, calls $this->repository->removeFromGroup($userId, $groupId) and asserts it returns true, then verifies the membership no longer exists (e.g. via repository->isMember or a direct DB/fixture lookup). Locate the existing testRemoveFromGroup method and add steps to (1) ensure a user and group fixture exist, (2) add the user to the group (using whichever helper/setup method your tests provide), (3) call $this->repository->removeFromGroup($userId, $groupId) and assert true, and (4) assert the membership is removed by checking the repository or fixture state.tests/phpMyFAQ/UserTest.php (1)
585-598: Make the Keycloak lookup assertion verify the lookup key.Use an argument-aware mock so this fails if
getUserIdByKeycloakSub()does not querykeycloak_sub.Proposed mock tightening
$this->userData ->expects($this->exactly(3)) ->method('fetchAll') - ->willReturnOnConsecutiveCalls( - ['user_id' => 11, 'is_visible' => true], - ['user_id' => 12], - ['user_id' => 13], - ); + ->willReturnMap([ + ['email', 'thorsten@example.com', ['user_id' => 11, 'is_visible' => true]], + ['email', 'anonymous@example.com', ['user_id' => 12]], + ['keycloak_sub', 'keycloak-sub-123', ['user_id' => 13]], + ]);As per coding guidelines, "
**/*.test.{ts,tsx,php}: Always write tests for new features and bug fixes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/UserTest.php` around lines 585 - 598, Tighten the userData mock so the fetchAll call made by getUserIdByKeycloakSub('keycloak-sub-123') is asserted to include the key used for lookup; change the existing expectation on $this->userData->method('fetchAll') (currently using willReturnOnConsecutiveCalls) to an argument-aware expectation for the third invocation that verifies the query/params contain keycloak_sub => 'keycloak-sub-123' (e.g. using withConsecutive or a callback-based with assertion), while preserving the same return values for the three calls and keeping the test assertions on getUserData, setUserData, getUserIdByEmail, getUserVisibilityByEmail, and getUserIdByKeycloakSub unchanged.tests/phpMyFAQ/Permission/MediumPermissionTest.php (1)
435-451: Assert the membership before and after removal.Right now the test only checks the DELETE result; a no-op successful DELETE could still pass. Assert setup success and that the user no longer belongs to the group afterward.
Proposed test strengthening
- $this->mediumPermission->addGroup($groupData); - $this->mediumPermission->addToGroup(1, 1); + $groupId = $this->mediumPermission->addGroup($groupData); + $this->assertGreaterThan(0, $groupId); + $this->assertTrue($this->mediumPermission->addToGroup(1, $groupId)); + $this->assertContains($groupId, $this->mediumPermission->getUserGroups(1)); - $this->assertTrue($this->mediumPermission->removeFromGroup(1, 1)); + $this->assertTrue($this->mediumPermission->removeFromGroup(1, $groupId)); + $this->assertNotContains($groupId, $this->mediumPermission->getUserGroups(1)); // Cleanup - $this->mediumPermission->deleteGroup(1); + $this->mediumPermission->deleteGroup($groupId);As per coding guidelines, "
**/*.test.{ts,tsx,php}: Always write tests for new features and bug fixes".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php` around lines 435 - 451, The test should verify membership both before and after removal: after creating the group with addGroup(...) assert that addToGroup(1, 1) returned true (setup succeeded), then assert the user is actually a member (use the existing membership-check helper such as isUserInGroup(userId, groupId) or getGroupsForUser(userId) to assert groupId is present), then call removeFromGroup(1, 1) and assert it returned true and that the membership-check now shows the user is no longer in the group; keep the cleanup deleteGroup(1). Use the methods mediumPermission->addToGroup, mediumPermission->removeFromGroup, and the project’s membership-query method (e.g., mediumPermission->isUserInGroup or mediumPermission->getGroupsForUser) to locate the checks.tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.php (1)
137-140: Assert the stored OIDC session token, not only the local test variable.
$idTokenis assigned inside the mock callback, so this assertion can pass even ifcallback()stops persisting the token. Please assert$oidcSession->getIdToken()equals the generated token to cover the logout dependency.Test assertion tightening
self::assertResponseStatusCodeSame(302, $response); self::assertSame($configuration->getDefaultUrl(), $response->headers->get('Location')); -self::assertNotSame('', $idToken); +self::assertNotSame('', $idToken); +self::assertSame($idToken, $oidcSession->getIdToken());🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.php` around lines 137 - 140, The test currently only asserts the local $idToken variable (set in the mock callback) which can pass even if the callback stopped persisting the token; update the assertions to verify the stored OIDC session token by asserting $oidcSession->getIdToken() equals the generated token (and optionally assert it's not empty) after the controller action so you cover the persistence behavior of callback() and the logout dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.php`:
- Around line 102-104: The early return in AuthKeycloak->userExists($login)
prevents running assignUserToGroups(), so groupSyncOnLogin only runs for
auto-provisioned users; update the existing-user branch to call
assignUserToGroups($login) when the keycloak.groupSyncOnLogin setting is enabled
before returning true (i.e., move or add a call to assignUserToGroups inside the
if ($this->userExists($login)) block guarded by the keycloak.groupSyncOnLogin
config check) so existing users get their groups synced on login.
- Around line 54-60: In AuthKeycloak where createUser($login, '', $domain) is
called and the catch logs the failure via
$this->configuration->getLogger()->error(...), redact the user identifier before
logging: compute a redacted version of $login (e.g., mask an email by keeping
only the first character and the domain or otherwise show a short prefix +
ellipsis) and use that $redacted value in the error message while still
including the full exception message; implement the redaction as a small helper
or inline logic (e.g., redactIdentifier($login)) inside the AuthKeycloak class
and replace usages in the catch block so logs never contain the raw $login.
In `@phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcIdTokenValidator.php`:
- Around line 48-56: The validator currently accepts empty tokens and does not
enforce required OIDC claims; update the logic in OidcIdTokenValidator (around
where splitToken, decodeSegment and validateClaims are used) to reject empty
idToken outright, require presence and correct types for iss, sub, aud, exp, and
iat in the decoded payload, and throw on missing/invalid values; when
expectedNonce is provided, require the payload nonce to exist and match exactly;
validate that exp exists and is a unix timestamp in the future, and that iat
exists and is a unix timestamp not unreasonably far in the past (apply the same
stricter checks in the other validation path that covers the later block
referenced).
In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/AzureAuthenticationController.php`:
- Around line 92-97: The callback currently only inspects error_description
($error) but can miss a plain OAuth2/OIDC "error" parameter; in
AzureAuthenticationController update the request handling to also read and check
the 'error' query param (e.g. $errorParam =
Filter::filterVar($request->query->get('error'), ...)) and treat it the same as
$error_description: log a warning via
$this->configuration->getLogger()->warning(...) and return the default
RedirectResponse immediately (before using $code or performing the token
exchange). Ensure you check both $error and $error_description and short-circuit
the token exchange path when either is non-empty.
In
`@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php`:
- Around line 184-208: The warning logs currently include the raw $login (in the
blocks around isValidLogin, checkCredentials, getUserByLogin, and
synchronizeKeycloakSubject); replace those direct uses with a redacted value by
adding a helper (e.g., maskLogin or sanitizeLogin) that returns a
non-identifying string (masked/email-local-part or deterministic hash) and use
that helper in the sprintf calls passed to
$this->configuration->getLogger()->warning; ensure no other log or persistent
storage writes the raw $login in these paths and keep
oidcSession->clearAuthorizationState() calls unchanged.
- Around line 112-126: When Keycloak is disabled or discoveryUrl is empty the
controller returns early without clearing the local session; update
KeycloakAuthenticationController so you always resolve the logout id token (call
resolveLogoutIdToken($user) if needed) and call $user->deleteFromSession() and
$this->oidcSession->clearIdToken() before returning any RedirectResponse
fallback. Specifically, in the block that uses
$this->providerConfigFactory->create() and currently returns new
RedirectResponse($this->configuration->getDefaultUrl()), move or duplicate the
resolveLogoutIdToken(), $user->deleteFromSession(), and
$this->oidcSession->clearIdToken() calls so they run unconditionally prior to
any RedirectResponse (both the fallback and the logoutUrl redirect paths).
- Around line 174-181: Capture the decoded ID token claims returned by
idTokenValidator->validate(...) (using the same inputs:
(string)($token['id_token'] ?? ''), $discoveryDocument,
$providerConfig->client->clientId, $authorizationState['nonce']), then call
$this->oidcClient->fetchUserInfo(...) as before and compare the 'sub' value from
the decoded ID token to the 'sub' in the UserInfo $claims; if they match,
continue with $this->resolveLocalLogin($claims), otherwise abort the login flow
(throw or return an error/log and do not call resolveLocalLogin) to prevent
linking on mismatched subjects.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.php`:
- Around line 222-233: The migration currently adds a non-unique index for
keycloak_sub on faquserdata which allows duplicate non-empty Keycloak subject
values; change the migration so that the DB enforces uniqueness: modify the
recorder calls that operate on the keycloak_sub column (see
addColumn('faquserdata','keycloak_sub',...) and
createIndex('faquserdata','idx_faquserdata_keycloak_sub', 'keycloak_sub')) to
create a UNIQUE constraint/index instead (or add an explicit unique constraint
on faquserdata.keycloak_sub) so the database prevents duplicate keycloak_sub
values used by getUserIdByKeycloakSub().
In `@phpmyfaq/src/phpMyFAQ/User.php`:
- Around line 822-824: The code reads $userData =
$this->userdata->fetchAll('keycloak_sub', $keycloakSub) and immediately returns
$userData['user_id'] which can be undefined when no row exists; change this to
guard the not-found path by checking $userData (and isset($userData['user_id']))
before returning, and if missing either return a safe integer (e.g., 0) or throw
a clear exception (e.g., InvalidArgumentException/RuntimeException) so the
method respects its int return contract and avoids undefined-index warnings.
In `@phpmyfaq/src/phpMyFAQ/User/UserData.php`:
- Around line 307-340: The fallback UPDATE drops keycloak_sub and can return
true even after the first write failed; modify the logic in UserData (the method
performing the update in phpMyFAQ/User/UserData.php) to detect when the original
update includes keycloak_sub (e.g. when $this->data['keycloak_sub'] is set) and,
if the first query throws/returns false, do not run the reduced fallback or
return success — instead propagate failure (return false) or rethrow; ensure any
fallback UPDATE only runs when keycloak_sub was not part of the original payload
so you never silently lose/persist an IdP binding.
In
`@tests/phpMyFAQ/Controller/Administration/AuthenticationControllerWebTest.php`:
- Line 30: The test calls the non-static assertion method
assertResponseContains() using self:: which fails at runtime on PHP 8+; update
the call in AuthenticationControllerWebTest to invoke the instance method (use
$this->assertResponseContains(...)) so it matches the declaration in
ControllerWebTestCase and remove the static invocation.
In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php`:
- Around line 67-80: The test mutates the singleton Configuration::$config via
Reflection and doesn't restore it; capture the original config in setUp() (e.g.
use
ReflectionClass(Configuration::class)->getProperty('config')->getValue($this->configuration')
and store it in a private property like $originalConfig) and then restore it in
tearDown() by calling setValue($this->configuration, $this->originalConfig);
ensure both setUp() and tearDown() exist in KeycloakAuthenticationControllerTest
and reference the same Reflection property to revert changes after each test.
In
`@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.php`:
- Line 89: The mock callback passed to MockHttpClient in
KeycloakAuthenticationControllerWebTest currently has an unused $method
parameter; update the callback(s) to assert the HTTP method for each expected
request (e.g., assertEquals('GET', $method) or assertEquals('POST', $method)
according to the request sequence driven by $responseIndex) or otherwise use
$method in the logic so PHPMD warnings are eliminated; locate the anonymous
functions that capture $responseIndex, $idToken, $expectedNonce,
$expectedVerifier and add the appropriate method assertions for each branch of
the response sequence (or rename/suppress per test rules if assertion is not
practical).
In `@tests/phpMyFAQ/Functional/ControllerWebTestCase.php`:
- Around line 94-108: The reflected in-memory config is being merged with the
raw $values (line with $configProperty->setValue(... array_merge($latestConfig,
$values))), which skips the normalizeConfigurationValue conversions just stored
via configuration->add/update; fix by creating a normalized map of the incoming
$values (use normalizeConfigurationValue for each entry, casting keys to string
like you do when calling configuration->add/update), then use that normalized
map for the final array_merge and for any update paths so both persisted and
in-memory overrides remain normalized; refer to addedConfigurationKeys,
normalizeConfigurationValue, configuration->add/update, $latestConfig and
$configProperty->setValue to locate where to apply the change.
---
Outside diff comments:
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.php`:
- Around line 338-346: The MySQLi schema for the faquserdata table (array entry
'faquserdata' in class Mysqli) creates the keycloak_sub column but doesn't add
the lookup index; add a non-unique index for keycloak_sub for fresh installs by
including an index creation step tied to %sfaquserdata (e.g. either add an index
definition in the table DDL or run "CREATE INDEX" / "ALTER TABLE ... ADD KEY
keycloak_sub (keycloak_sub)" immediately after creating %sfaquserdata) so the
new MySQLi installs match migrated DBs that use keycloak_sub lookups.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.php`:
- Around line 337-345: The fresh-install CREATE TABLE for 'faquserdata' in
PdoMysql::createTables() adds the keycloak_sub column but not the index used by
migrations; update the CREATE TABLE statement for 'faquserdata' to include the
idx_faquserdata_keycloak_sub index on keycloak_sub (matching the migration) so
new MySQL installs get the same lookup index as upgrades.
In `@phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.php`:
- Around line 327-335: The faquserdata table schema in PdoSqlsrv.php is missing
the Keycloak subject index; add a CREATE INDEX statement named
idx_faquserdata_keycloak_sub for the keycloak_sub column to the schema
definitions so fresh installs match Migration420Alpha.php (index name:
idx_faquserdata_keycloak_sub, target table symbol %sfaquserdata, column
keycloak_sub, SQL Server CREATE INDEX ... ON ... (keycloak_sub)). Ensure the
index creation uses the same identifier and column as Migration420Alpha.php to
keep schemas consistent.
In `@phpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.php`:
- Around line 686-697: The canonical schema for faquserdata is missing the
keycloak_sub lookup index; update the faquserdata() TableBuilder chain in the
DatabaseSchema class to add the same index used in the migration path for the
keycloak_sub column (i.e., add an index/lookup entry for 'keycloak_sub' on the
TableBuilder returned by faquserdata()) so fresh installs include the lookup
index alongside the existing columns.
---
Nitpick comments:
In @.github/workflows/build.yml:
- Around line 65-71: The workflow currently installs MkDocs dynamically in the
"Install MkDocs" step which can break reproducible `--strict` docs builds;
change the step to install pinned versions instead by creating a
docs/requirements.txt that pins mkdocs (and plugins) to known-good versions and
updating the "Install MkDocs" step (the job named "Setup Python for docs build"
and the step titled "Install MkDocs") to run pip install -r
docs/requirements.txt (or explicitly pin mkdocs==X.Y.Z in the workflow if you
prefer a quick fix).
- Around line 65-103: The docs build is executed for every PHP matrix entry;
extract the Python/pnpm/mkdocs-related steps (the steps with names "Setup Python
for docs build", "Install MkDocs", "Setup pnpm", "Get pnpm store directory",
"Setup pnpm cache", "Install Node.js dependencies", "Run ESLint check", "Run
Vitest tests", "Build documentation") into a dedicated GitHub Actions job (e.g.,
job name "docs") that runs once (or alternatively add a gate like if:
matrix.php-versions == '8.4' to the current job) so the docs pipeline runs
independently and only once, moving the steps and necessary uses/with blocks
into that new job and wiring any required cache keys/outputs accordingly.
In `@docs/keycloak.md`:
- Line 28: The phrase "Valid post logout redirect URIs" is inconsistent with
"post-logout redirect URI" elsewhere; update the text that currently reads
"Valid post logout redirect URIs" to use the hyphenated form "Valid post-logout
redirect URIs" (or alternatively change the other occurrence to remove the
hyphen) so both occurrences (search for the exact strings "Valid post logout
redirect URIs" and "post-logout redirect URI") match consistently throughout the
document.
In `@tests/phpMyFAQ/Controller/Frontend/AzureAuthenticationControllerTest.php`:
- Around line 122-137: Rename the test method
testCallbackReturnsErrorResponseWhenProviderErrorIsSet to reflect that it
expects a default redirect instead of an error response (e.g.,
testCallbackRedirectsToDefaultUrlWhenProviderErrorIsSet); update only the test
method name in AzureAuthenticationControllerTest so it matches the assertions
that callback(Request with 'error_description') returns a RedirectResponse whose
Location equals $this->configuration->getDefaultUrl(), leaving the rest of the
test (controller instantiation and assertions) unchanged.
In
`@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.php`:
- Around line 137-140: The test currently only asserts the local $idToken
variable (set in the mock callback) which can pass even if the callback stopped
persisting the token; update the assertions to verify the stored OIDC session
token by asserting $oidcSession->getIdToken() equals the generated token (and
optionally assert it's not empty) after the controller action so you cover the
persistence behavior of callback() and the logout dependency.
In `@tests/phpMyFAQ/Permission/MediumPermissionRepositoryTest.php`:
- Around line 223-228: Extend testRemoveFromGroup to include a positive case
that uses test fixtures to create a real membership, calls
$this->repository->removeFromGroup($userId, $groupId) and asserts it returns
true, then verifies the membership no longer exists (e.g. via
repository->isMember or a direct DB/fixture lookup). Locate the existing
testRemoveFromGroup method and add steps to (1) ensure a user and group fixture
exist, (2) add the user to the group (using whichever helper/setup method your
tests provide), (3) call $this->repository->removeFromGroup($userId, $groupId)
and assert true, and (4) assert the membership is removed by checking the
repository or fixture state.
In `@tests/phpMyFAQ/Permission/MediumPermissionTest.php`:
- Around line 435-451: The test should verify membership both before and after
removal: after creating the group with addGroup(...) assert that addToGroup(1,
1) returned true (setup succeeded), then assert the user is actually a member
(use the existing membership-check helper such as isUserInGroup(userId, groupId)
or getGroupsForUser(userId) to assert groupId is present), then call
removeFromGroup(1, 1) and assert it returned true and that the membership-check
now shows the user is no longer in the group; keep the cleanup deleteGroup(1).
Use the methods mediumPermission->addToGroup, mediumPermission->removeFromGroup,
and the project’s membership-query method (e.g., mediumPermission->isUserInGroup
or mediumPermission->getGroupsForUser) to locate the checks.
In `@tests/phpMyFAQ/UserTest.php`:
- Around line 585-598: Tighten the userData mock so the fetchAll call made by
getUserIdByKeycloakSub('keycloak-sub-123') is asserted to include the key used
for lookup; change the existing expectation on
$this->userData->method('fetchAll') (currently using
willReturnOnConsecutiveCalls) to an argument-aware expectation for the third
invocation that verifies the query/params contain keycloak_sub =>
'keycloak-sub-123' (e.g. using withConsecutive or a callback-based with
assertion), while preserving the same return values for the three calls and
keeping the test assertions on getUserData, setUserData, getUserIdByEmail,
getUserVisibilityByEmail, and getUserIdByKeycloakSub unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 88af5faa-c708-48ae-ad2b-5a719dca9307
📒 Files selected for processing (55)
.github/workflows/build.yml.gitignore.prettierignoreCHANGELOG.mdcomposer.jsondocs/administration.mddocs/configuration.mddocs/keycloak.mdeslint.config.mjsmkdocs.ymlpackage.jsonphpmyfaq/admin/assets/src/configuration/configuration.test.tsphpmyfaq/assets/templates/default/login.twigphpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcClient.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcIdTokenValidator.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcSession.phpphpmyfaq/src/phpMyFAQ/Controller/Administration/Api/ConfigurationTabController.phpphpmyfaq/src/phpMyFAQ/Controller/Frontend/AzureAuthenticationController.phpphpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Mysqli.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoMysql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.phpphpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlsrv.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.phpphpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermission.phpphpmyfaq/src/phpMyFAQ/Permission/MediumPermissionRepository.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DatabaseSchema.phpphpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.phpphpmyfaq/src/phpMyFAQ/User.phpphpmyfaq/src/phpMyFAQ/User/UserData.phpphpmyfaq/src/services.phpphpmyfaq/translations/language_de.phpphpmyfaq/translations/language_en.phptests/phpMyFAQ/Auth/AuthKeycloakTest.phptests/phpMyFAQ/Auth/Oidc/OidcIdTokenValidatorTest.phptests/phpMyFAQ/Auth/Oidc/OidcSessionTest.phptests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.phptests/phpMyFAQ/Controller/Administration/AuthenticationControllerTest.phptests/phpMyFAQ/Controller/Administration/AuthenticationControllerWebTest.phptests/phpMyFAQ/Controller/Frontend/AuthenticationControllerWebTest.phptests/phpMyFAQ/Controller/Frontend/AzureAuthenticationControllerTest.phptests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.phptests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.phptests/phpMyFAQ/Functional/ControllerWebTestCase.phptests/phpMyFAQ/Permission/MediumPermissionRepositoryTest.phptests/phpMyFAQ/Permission/MediumPermissionTest.phptests/phpMyFAQ/TranslationTest.phptests/phpMyFAQ/User/UserDataTest.phptests/phpMyFAQ/UserTest.phpvite.config.ts
✅ Files skipped from review due to trivial changes (14)
- .gitignore
- .prettierignore
- phpmyfaq/src/phpMyFAQ/Instance/Database/PdoPgsql.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlite3.php
- eslint.config.mjs
- phpmyfaq/src/phpMyFAQ/Instance/Database/Pgsql.php
- tests/phpMyFAQ/User/UserDataTest.php
- phpmyfaq/src/phpMyFAQ/Instance/Database/PdoSqlite.php
- CHANGELOG.md
- package.json
- mkdocs.yml
- composer.json
- docs/configuration.md
- phpmyfaq/src/phpMyFAQ/Instance/Database/Sqlsrv.php
🚧 Files skipped from review as they are similar to previous changes (10)
- docs/administration.md
- phpmyfaq/assets/templates/default/login.twig
- phpmyfaq/src/phpMyFAQ/Setup/Installation/DefaultDataSeeder.php
- tests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.php
- tests/phpMyFAQ/Auth/Oidc/OidcSessionTest.php
- phpmyfaq/translations/language_en.php
- phpmyfaq/translations/language_de.php
- phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcSession.php
- tests/phpMyFAQ/Controller/Frontend/AuthenticationControllerWebTest.php
- tests/phpMyFAQ/TranslationTest.php
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/phpMyFAQ/Functional/ControllerWebTestCase.php (1)
234-242:⚠️ Potential issue | 🟠 MajorSnapshot
main.referenceURLbefore persisting the test override.Line 234 updates the backing configuration outside the restore tracking path. A test that only calls
request*()won't populate$originalConfigurations, somain.referenceURLcan leak into later tests. TheConfiguration::update()method persists directly to the database viaDatabaseConfigurationStore::updateConfigValue(), and cleanup only occurs for entries tracked in$originalConfigurations.Proposed fix if the persisted update is required
$configuration = $container->get('phpmyfaq.configuration'); self::assertInstanceOf(Configuration::class, $configuration); - $configuration->update(['main.referenceURL' => 'https://localhost/']); - $reflection = new ReflectionClass(Configuration::class); $configProperty = $reflection->getProperty('config'); $currentConfig = $configProperty->getValue($configuration); self::assertIsArray($currentConfig); + + if (!array_key_exists($context, $this->originalConfigurations)) { + $this->originalConfigurations[$context] = [ + 'configuration' => $configuration, + 'values' => $currentConfig, + ]; + } + + $configuration->update(['main.referenceURL' => 'https://localhost/']); $configProperty->setValue($configuration, array_merge($currentConfig, [ 'main.referenceURL' => 'https://localhost/', ]));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Functional/ControllerWebTestCase.php` around lines 234 - 242, The test calls Configuration::update('main.referenceURL', ...) which persists directly and can leak between tests; before calling Configuration::update on $configuration, snapshot the current value of 'main.referenceURL' by reading the config via ReflectionClass(Configuration::class) / $configProperty->getValue($configuration) and record it into the test's $originalConfigurations tracking (use the same key used for cleanup) so the teardown can restore it, or avoid persistence entirely by only setting the in-memory config property via the Reflection-set path you already have (i.e., remove the call to Configuration::update and keep the array_merge on the reflected config) depending on whether persistence is actually required.
🧹 Nitpick comments (2)
tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php (1)
82-90: Swallowing all errors aroundopenssl_pkey_newhides real failures.Installing
static fn(): bool => truesuppresses every PHP warning/notice (not just OpenSSL) for the duration of the block. Ifopenssl_pkey_new()fails due to a missing/misconfigured OpenSSL on the test host, the nextassertInstanceOfwill fail with a cryptic message instead of the actual OpenSSL error (e.g.,config file not found). Sinceset_error_handleris restricted to OpenSSL warnings anyway, consider either not suppressing (recent PHP + OpenSSL 3 is typically clean) or using a handler that only swallowsE_WARNINGwithOPENSSLin the message.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php` around lines 82 - 90, The current test globally silences all errors by calling set_error_handler(static fn(): bool => true) around openssl_pkey_new, which hides real OpenSSL failures; change the approach in KeycloakAuthenticationControllerTest so that you either remove the custom error handler entirely (rely on opensSL behavior) or register a targeted handler that only intercepts E_WARNING and inspects the message for "OpenSSL" (or "OPENSSL" / "config file") before returning true, then restore_error_handler as currently done; update the block around openssl_pkey_new and keep the following assertInstanceOf checks so any real OpenSSL errors surface in test output.phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php (1)
112-131:logout()catch block may re-invokedeleteFromSession()/clearIdToken().If the
trysucceeds past line 117 and thendiscoveryService->discover()orbuildLogoutUrl()throws (lines 123–124), the catch at 127–130 re-calls both methods. These need to be safely idempotent.CurrentUser::deleteFromSession()may destroy or re-destroy the session depending on implementation, and logging/side effects could fire twice. Consider guarding with a local boolean or narrowing thetryto wrap only the discovery/logout-URL section:♻️ Proposed narrowing
public function logout(): RedirectResponse { $user = $this->getCurrentUserService(); - try { - $providerConfig = $this->providerConfigFactory->create(); - $idToken = $this->resolveLogoutIdToken($user); - - $user->deleteFromSession(); - $this->oidcSession->clearIdToken(); - - if (!$providerConfig->enabled || $providerConfig->discoveryUrl === '') { - return new RedirectResponse($this->configuration->getDefaultUrl()); - } - - $discoveryDocument = $this->discoveryService->discover($providerConfig); - $logoutUrl = $this->oidcClient->buildLogoutUrl($providerConfig, $discoveryDocument, $idToken); - - return new RedirectResponse($logoutUrl ?? $this->configuration->getDefaultUrl()); - } catch (Exception) { - $user->deleteFromSession(); - $this->oidcSession->clearIdToken(); - return new RedirectResponse($this->configuration->getDefaultUrl()); - } + $providerConfig = $this->providerConfigFactory->create(); + $idToken = $this->resolveLogoutIdToken($user); + + $user->deleteFromSession(); + $this->oidcSession->clearIdToken(); + + if (!$providerConfig->enabled || $providerConfig->discoveryUrl === '') { + return new RedirectResponse($this->configuration->getDefaultUrl()); + } + + try { + $discoveryDocument = $this->discoveryService->discover($providerConfig); + $logoutUrl = $this->oidcClient->buildLogoutUrl($providerConfig, $discoveryDocument, $idToken); + return new RedirectResponse($logoutUrl ?? $this->configuration->getDefaultUrl()); + } catch (Exception) { + return new RedirectResponse($this->configuration->getDefaultUrl()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php` around lines 112 - 131, In logout(), avoid calling CurrentUser::deleteFromSession() and OidcSession::clearIdToken() twice by either narrowing the try block to only wrap discoveryService->discover() and oidcClient->buildLogoutUrl() (so deleteFromSession() and clearIdToken() are called once before the try), or add a local boolean (e.g. $sessionCleared) set when calling deleteFromSession()/clearIdToken() and check it in the catch before re-invoking; adjust references: logout(), resolveLogoutIdToken(), CurrentUser::deleteFromSession(), OidcSession::clearIdToken(), discoveryService->discover(), and oidcClient->buildLogoutUrl() accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@phpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.php`:
- Around line 110-112: The code calls
assignUserToGroups($existingUser->getUserId()) when
shouldSynchronizeGroupsOnLogin() is true regardless of current permission
backend; update both existing-user and other relevant paths (the block around
assignUserToGroups at lines shown and the similar block at 281-284) to guard the
call with the permission-level check used elsewhere (e.g., only call
assignUserToGroups when shouldSynchronizeGroupsOnLogin() &&
$this->isPermissionLevelMedium() or the equivalent method/property on
AuthKeycloak), so group synchronization only runs when security.permLevel is
medium.
- Around line 70-76: The code sets the user active and auth source then calls
setUserData which can fail; change the flow to attempt to persist the durable
binding first and fail provisioning if it cannot be saved: call setUserData(...)
and check its return value (or catch an exception) using the same symbols
($user->setUserData, $this->getDisplayName, $this->getEmail, $this->getSubject);
if setUserData fails, log/propagate the error and return false or throw so
provisioning aborts, and only after successful save set
$user->setStatus('active') and
$user->setAuthSource(AuthenticationSourceType::AUTH_KEYCLOAK->value).
In `@phpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcIdTokenValidator.php`:
- Around line 151-160: The token-time validation in OidcIdTokenValidator is
incorrect: change the expiration check to reject when the current time is
greater than or equal to exp (use <= comparison instead of <) so tokens at the
exp boundary fail, and enforce that when an nbf claim is present it must be
numeric (reject non-numeric nbf instead of ignoring it) and continue to reject
if current time < nbf; update the checks around $claims['exp'], $claims['nbf']
and $now in the validation routine accordingly (refer to variables $claims, $now
and the exp/nbf claim handling in OidcIdTokenValidator).
In
`@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php`:
- Around line 258-293: resolveLocalLogin() currently falls back to returning the
raw Keycloak "sub" (UUID) when preferred_username and email are empty; change
this to refuse the login instead of returning the UUID: in resolveLocalLogin(),
after computing $preferredUsername, $email and $subject, if both
$preferredUsername and $email are empty, log a clear warning and either throw a
descriptive exception (e.g. RuntimeException) or return an empty string to
indicate failure rather than returning $subject; ensure callers of
resolveLocalLogin() (e.g.
AuthKeycloak::isValidLogin/checkCredentials/CurrentUser::getUserByLogin) can
handle the failure path and avoid auto-provisioning a UUID as a login.
- Around line 295-303: maskLogin() currently emits an unsalted, truncated
SHA-256 which is reversible; change it to a deterministic keyed HMAC before
truncation so logs remain correlatable but not recoverable. Specifically, use a
stable per-install secret (e.g. the app Configuration secret or a Keycloak salt)
as the HMAC key and compute hash_hmac('sha256', $login, $serverSecret) in
maskLogin(), then truncate that HMAC output (same 12 hex chars if desired) and
preserve the '<empty>' behavior; reference the maskLogin method and obtain the
secret from the existing Configuration/secret provider used elsewhere in the
class rather than hardcoding a key.
In `@phpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.php`:
- Around line 205-218: The SQL Server existence check in isSqlServer() builds a
query using sys.indexes.name only, which can collide across tables; update the
returned string in the block that constructs
$whereClause/$indexName/$tableName/$columnList so the IF NOT EXISTS condition
also filters by the target table's object_id (e.g. add "AND object_id =
OBJECT_ID('<table>')" or join against sys.objects using the $tableName) to
ensure the index name is checked for the specific $tableName before issuing
CREATE UNIQUE INDEX.
In `@phpmyfaq/src/phpMyFAQ/User/UserData.php`:
- Around line 270-271: The code treats an absent Keycloak subject as an empty
string when persisting, which causes unique-index collisions; keep the read
fallback $this->data['keycloak_sub'] ??= '' but when writing the user record
ensure you persist NULL for an unlinked subject instead of ''. Update the
persistence paths in the UserData class that save $this->data (the insert/update
logic that writes keycloak_sub) to convert empty-string or missing
$this->data['keycloak_sub'] to NULL before binding/executing the DB statement,
and ensure any prepared-statement parameter types allow NULL for keycloak_sub.
In `@tests/phpMyFAQ/Functional/ControllerWebTestCase.php`:
- Around line 120-122: The current array-to-string conversion silently hides
json_encode failures by casting its false return to an empty string; in the
method in ControllerWebTestCase (the branch that checks is_array($value)) call
json_encode with the JSON_THROW_ON_ERROR flag so a JsonException is thrown on
failure (optionally catch and rethrow with a clearer message if you want
contextual test output), replacing the current cast-to-string behavior to make
encoding errors explicit.
---
Outside diff comments:
In `@tests/phpMyFAQ/Functional/ControllerWebTestCase.php`:
- Around line 234-242: The test calls Configuration::update('main.referenceURL',
...) which persists directly and can leak between tests; before calling
Configuration::update on $configuration, snapshot the current value of
'main.referenceURL' by reading the config via
ReflectionClass(Configuration::class) /
$configProperty->getValue($configuration) and record it into the test's
$originalConfigurations tracking (use the same key used for cleanup) so the
teardown can restore it, or avoid persistence entirely by only setting the
in-memory config property via the Reflection-set path you already have (i.e.,
remove the call to Configuration::update and keep the array_merge on the
reflected config) depending on whether persistence is actually required.
---
Nitpick comments:
In
`@phpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.php`:
- Around line 112-131: In logout(), avoid calling
CurrentUser::deleteFromSession() and OidcSession::clearIdToken() twice by either
narrowing the try block to only wrap discoveryService->discover() and
oidcClient->buildLogoutUrl() (so deleteFromSession() and clearIdToken() are
called once before the try), or add a local boolean (e.g. $sessionCleared) set
when calling deleteFromSession()/clearIdToken() and check it in the catch before
re-invoking; adjust references: logout(), resolveLogoutIdToken(),
CurrentUser::deleteFromSession(), OidcSession::clearIdToken(),
discoveryService->discover(), and oidcClient->buildLogoutUrl() accordingly.
In `@tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.php`:
- Around line 82-90: The current test globally silences all errors by calling
set_error_handler(static fn(): bool => true) around openssl_pkey_new, which
hides real OpenSSL failures; change the approach in
KeycloakAuthenticationControllerTest so that you either remove the custom error
handler entirely (rely on opensSL behavior) or register a targeted handler that
only intercepts E_WARNING and inspects the message for "OpenSSL" (or "OPENSSL" /
"config file") before returning true, then restore_error_handler as currently
done; update the block around openssl_pkey_new and keep the following
assertInstanceOf checks so any real OpenSSL errors surface in test output.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44966343-27fa-4cb9-ad44-646f779938f1
📒 Files selected for processing (16)
phpmyfaq/src/phpMyFAQ/Auth/AuthKeycloak.phpphpmyfaq/src/phpMyFAQ/Auth/Oidc/OidcIdTokenValidator.phpphpmyfaq/src/phpMyFAQ/Controller/Frontend/AzureAuthenticationController.phpphpmyfaq/src/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationController.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/AbstractMigration.phpphpmyfaq/src/phpMyFAQ/Setup/Migration/Versions/Migration420Alpha.phpphpmyfaq/src/phpMyFAQ/User.phpphpmyfaq/src/phpMyFAQ/User/UserData.phptests/phpMyFAQ/Auth/AuthKeycloakTest.phptests/phpMyFAQ/Auth/Oidc/OidcIdTokenValidatorTest.phptests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.phptests/phpMyFAQ/Controller/Administration/AuthenticationControllerWebTest.phptests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerTest.phptests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.phptests/phpMyFAQ/Functional/ControllerWebTestCase.phptests/phpMyFAQ/User/UserDataTest.php
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/phpMyFAQ/Controller/Administration/Api/ConfigurationTabControllerTest.php
- tests/phpMyFAQ/User/UserDataTest.php
- tests/phpMyFAQ/Controller/Administration/AuthenticationControllerWebTest.php
- tests/phpMyFAQ/Controller/Frontend/KeycloakAuthenticationControllerWebTest.php
- phpmyfaq/src/phpMyFAQ/Controller/Frontend/AzureAuthenticationController.php
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation