Conversation
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Matthias Sauer <sauerm@strato.de>
There was a problem hiding this comment.
Pull request overview
Fixes a login dead-end in the user_saml Nextcloud app when SAML is enabled but no IdP has been configured yet, by allowing /login to fall through to the normal Nextcloud login instead of forcing a broken SAML redirect.
Changes:
- Added
SAMLSettings::getListOfConfiguredIdps()to filter IdPs to those with bothidp-entityIdandidp-singleSignOnService.urlset. - Updated
Application::boot()to use the filtered IdP list and return early when none are configured.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| lib/SAMLSettings.php | Adds a filtered IdP list helper to distinguish minimally configured IdPs. |
| lib/AppInfo/Application.php | Uses the filtered list to avoid redirecting /login into a broken SAML flow when no IdP is configured. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| $configuredIdps = $samlSettings->getListOfConfiguredIdps(); | ||
| // If no IdP has the minimum required config (entityId + SSO URL), fall through to normal login | ||
| if (empty($configuredIdps)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
The new login-flow behavior (falling through to core login when SAML is enabled but no IdP is minimally configured) is security/availability sensitive and currently isn’t covered by automated tests. Please add a unit/integration test that asserts /login is redirected to the SAML endpoint when an IdP is configured, and is not redirected when no IdP has entityId+SSO URL.
There was a problem hiding this comment.
Added unit tests for SAMLSettings::getListOfConfiguredIdps() in tests/unit/SAMLSettingsTest.php (commit 6c8ed9c). The tests cover:
- No configurations → empty array (login falls through to Nextcloud)
- IdP with both
idp-entityId+idp-singleSignOnService.url→ returned (SAML redirect applies) - IdP with whitespace-only values → filtered out (login falls through)
- Mix of fully and partially configured IdPs → only fully configured ones returned
Since Application::boot() uses header() + exit() which can't be unit-tested in isolation, the tests are placed on getListOfConfiguredIdps() — the method that drives the redirect decision.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Matthias Sauer <151215545+matsaur@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> Signed-off-by: Matthias Sauer <151215545+matsaur@users.noreply.github.com>
Agent-Logs-Url: https://github.com/IONOS-Productivity/nc-user_saml/sessions/096a3167-5eda-4bd0-bc50-582afe712801 Co-authored-by: matsaur <151215545+matsaur@users.noreply.github.com>
|
Hello, I imported and adapted this PR as nextcloud#1091 on the main repo. |
|
@come-nc Thank you for your feedback, I'll close this PR 👍 |
Summary
When the
user_samlapp is enabled but no IdP has been configured yet(i.e.
idp-entityIdandidp-singleSignOnService.urlare empty), thelogin redirect in
Application::boot()would still intercept all/loginrequests and forward them to the SAML endpoint — making it impossible to
log in at all.
Changes
SAMLSettings::getListOfConfiguredIdps()which filters the IdPlist to only entries that have both
idp-entityIdandidp-singleSignOnService.urlset.Application::boot()now uses this method instead ofgetListOfIdps()and returns early (falling through to normal Nextcloud login) when no
fully configured IdP exists.
How to reproduce
user_samlappnow falls through to the standard login form.