fix: graceful handling when user_saml is enabled but no IdP is configured#1138
Conversation
570b38e to
261d821
Compare
When user_saml is enabled (type=saml) but no IdP exists in the database, navigating to the login page caused a TypeError: array_key_exists(): Argument #2 ($array) must be of type array, null given in SAMLSettings.php line 147 Root cause: Application.php beforeware calls getListOfIdps() which sets LOADED_ALL on SAMLSettings. When SAMLController::login() then calls getOneLoginSettingsArray(1), ensureConfigurationsLoaded(1) returns early due to LOADED_ALL, leaving $this->configurations[1] undefined (null). The array_key_exists() call on null then throws a TypeError. Two fixes in SAMLSettings::getOneLoginSettingsArray(): - Replace the lone array_key_exists() call (line 156) with ?? '' to be consistent with every other field access in the method. - Add an early guard that throws InvalidArgumentException when the requested IdP is not in $configurations, converting a cryptic null-pointer into a clear error message. SAMLController::login() catches InvalidArgumentException and redirects to the existing genericError page with a translatable user-readable message instead of letting the exception propagate as a 500. Tests added: - SAMLSettingsTest: reproduces the exact LOADED_ALL bug scenario, tests the simple not-in-db case, happy path, and sp-entityId fallback. - SAMLControllerTest: verifies the controller redirects to genericError when getOneLoginSettingsArray() throws InvalidArgumentException. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
Adds a new occ command that support teams and administrators can run to get an immediate diagnosis of the user_saml configuration state: occ saml:config:validate Exit codes: 0 — all configured IdPs pass required-field validation 1 — SAML type not set (app is not active) 2 — active but one or more required fields are missing The command checks for the three fields required for a working SAML login: idp-entityId, idp-singleSignOnService.url, general-uid_mapping. Useful for support diagnostics and CI/monitoring health checks. Signed-off-by: Misha M.-Kupriyanov <kupriyanov@strato.de>
261d821 to
61d9a6e
Compare
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
Summary
user_samlis active (type=saml) but no IdP has been configuredgenericErrorpage with a translatable user-readable message instead of crashingocc saml:config:validatefor one-shot admin/support diagnostics (exit 0/1/2)Reproduction
user_samlappocc config:app:set user_saml type --value samlRoot Cause
The crash is a two-step interaction within the same request:
Application.phpbeforeware calls$samlSettings->getListOfIdps()→ensureConfigurationsLoaded(-1)loads from DB → DB is empty → setsconfigurationsLoadedState = LOADED_ALLSAMLController::login(1)callsgetOneLoginSettingsArray(1)→ensureConfigurationsLoaded(1)returns early (alreadyLOADED_ALL) →$this->configurations[1]is never set → remainsnullarray_key_exists('sp-entityId', $this->configurations[$idp])—array_key_exists()onnullthrows aTypeErrorEvery other field access in
getOneLoginSettingsArray()uses the safe?? ''null-coalesce pattern; line 156 was the one outlier usingarray_key_exists().Changes
lib/SAMLSettings.phparray_key_exists()call with?? ''(consistent with all other lines in the method)ensureConfigurationsLoaded()that throwsInvalidArgumentExceptionwhen the requested IdP is absent from$configurations, converting a cryptic null-pointer into a clear, actionable messagelib/Controller/SAMLController.phpInvalidArgumentExceptionfromgetOneLoginSettingsArray()inlogin()and redirect to the existinggenericErrorpage with a translatable user-readable message, instead of letting the exception propagate as a 500occ saml:config:validate(new command)Gives support teams and administrators a single command to diagnose the SAML configuration state without reading raw database rows:
Exit codes:
01typenot set — app is not active2Validated fields per IdP:
idp-entityId,idp-singleSignOnService.url,general-uid_mapping.New files:
lib/Command/ConfigValidate.php,tests/unit/Command/ConfigValidateTest.php, + one line inappinfo/info.xml.Tests
tests/unit/SAMLSettingsTest.php(new): reproduces the exactLOADED_ALLbug scenario; also covers the simple not-in-DB case, happy path, andsp-entityIdfallback to metadata URLtests/unit/Controller/SAMLControllerTest.php: addstestLoginWithUnconfiguredIdpRedirectsToGenericError()— verifies the controller redirects togenericErrorwhengetOneLoginSettingsArray()throwsInvalidArgumentExceptiontests/unit/Command/ConfigValidateTest.php(new): covers all four exit-code paths (type not set, no IdPs, OK, missing fields)After This Fix
/login— shows the normal Nextcloud login page (no crash; admins can still log in to fix the configuration)/apps/user_saml/saml/login?idp=1— redirects to the existinggenericErrorpage with a readable message: "SAML authentication is not configured. Please ask your administrator to complete the SAML setup in the admin panel."data/nextcloud.log— noarray_key_exists(): Argument #2 must be of type array, null given