fix(auth): IMP server selector missing on login page unless auth driver is 'application'

[jcdelepine]

Bug

When multiple IMAP backends are configured in backends.php with $conf['server']['server_list'] = 'shown' (imp), the server selector dropdown never appears on the login page — unless the Horde auth driver is specifically application pointed at imp.

Root cause

LoginService::buildLoginFormData() (and login.php's equivalent inline code) resolves extra login form fields via $auth->getLoginParams(), using the auth driver instance for the 'horde' app only. IMP_Application::authLoginParams() — which builds the server selector — is only reachable through this path when $conf['auth']['driver'] = 'application' with params.app = 'imp', since in that configuration 'horde' auth delegates directly to 'imp'.

With any other driver (LDAP, SQL, etc.), IMP's login params are never consulted at all, silently dropping the server selector.

Fix

Add a 'loginparams' auth capability, declared by IMP_Application::$auth (mirroring the existing 'add', 'transparent', etc. capabilities). LoginService/login.php now additionally resolve IMP's login params via this capability, independently of which driver authenticates the user to Horde itself.

Depends on the companion change in horde/imp declaring the 'loginparams' capability.

Testing

Verified the server selector now appears on both /login.php and the responsive /auth/login route with auth.driver = ldap and multiple non-disabled backends in backends.php.

[TDannhauer]

Thanks for the PR and the detailed root-cause analysis! I reviewed this together with the companion horde/imp#91. I agree there is a real bug here, but I see two problems with the approach, and I believe the actual root cause is a one-line omission elsewhere.

1. The selector renders, but the selection is ignored

This PR patches only the form-display paths (login.php around line 368, LoginService::buildLoginFormData()). The POST-collection paths are unchanged:

• login.php (~line 205) and LoginService::processLogin() (~line 242) collect credentials only from the Horde driver's getLoginParams(), so with an LDAP/SQL driver the posted imp_server_key is never picked up.
• Even if it were, the value is only consumed in IMP_Application::authAuthenticate(), which only runs when IMP's own auth driver authenticates — exactly the driver = 'application' → imp configuration that already worked.
• When IMP is opened after a Horde-level login, IMP_Auth::transparent() → _canAutoLogin() → getAutoLoginServer() picks the first/preferred backend and never looks at any user selection.

So in the configurations this PR targets, a user would pick "Server B" on the login screen and silently be connected to Server A. That is arguably worse than not showing the selector.

2. Hardcoding imp in horde/base

The framework base special-casing one application is a layering problem — Gollem, for example, also implements authLoginParams(). If the capability approach were kept, this should iterate over registered apps declaring the capability rather than naming imp.

Likely actual root cause

The framework already has a generic flow for this case: when transparent auth to IMP fails (e.g. hordeauth disabled on the backend), the user is redirected to the login page with ?app=imp. Legacy login.php builds the auth object per-app:

$auth = $injector->getInstance('Horde_Core_Factory_Auth')->create(($is_auth && $vars->app) ? $vars->app : null);

In that flow the selector appears and the selection is honored via authAuthenticate(). The responsive route broke exactly this half: LoginService::buildLoginFormData() creates the auth instance without the app parameter (create() with no argument), even though it reads $queryParams['app'] a few lines later — while processLogin() does pass the app. So on /auth/login?app=imp the selector never renders even in the legitimate app-auth case.

Suggestion

1. Fix buildLoginFormData() to create the auth instance for the requested app, mirroring login.php and processLogin(). That restores the existing generic flow with no new capability and no imp special-case in base.
2. If a server selector on the initial Horde login (with a non-imp driver) is genuinely desired, the change additionally needs to (a) collect imp_server_key in both POST paths and (b) make IMP_Auth::transparent() / _canAutoLogin() honor the stored selection — otherwise the dropdown is cosmetic.

Happy to discuss!

[ralflang]

I think the larger issue here is that "application auth" is not a good idiom for imp's potentially multiple imap backends. But this is not something to fix on the fly and the observable failure must be solved regardless.

[ralflang]

Right analysis, wrong approach. The solution sketched here works specifically for IMP (but probably not for other apps which provide login forms such as gollem) and nothing of this belongs into base.

[ralflang]

Nothing IMP specific belongs here.

[ralflang]

Nothing IMP specific belongs here

[ralflang]

Catching Throwable is way too broad.

[ralflang]

Something like Horde_Exception|Horde\Exception\Throwable

[ralflang]

Generalize to something like:

foreach ($this->registry->listApps() as $app) {
if ($app === 'horde') continue;

try { $appAuth = $this->injector->getInstance('Horde_Core_Factory_Auth')->create($app);
if (!$appAuth->hasCapability('loginparams')) continue;
$result = $appAuth->getLoginParams();
} catch (Horde_Exception|Horde\Exception\Throwable) $e) {}

[jcdelepine]

Pushed a new commit addressing both reviews — thanks for the thorough feedback, it caught real gaps in the original approach.

@ralflang — generalized to iterate every app declaring loginparams (registry->listApps()), no more hardcoded imp.
@TDannhauer — the selection is now actually collected and honored: posted values are stored in the session on login, and the companion horde/imp change (getAutoLoginServer()) reads it instead of always falling back to preferred/first backend.

Commit messages on both base and the companion imp commit have the details.

[ralflang]

I will merge this as-is but I need to follow-up and make the same enhancement in LoginService class.

[jcdelepine]

@ralflang The LoginService follow-up is indeed needed — I forgot to carry that part over when reworking the fix. Happy to send that PR too, reusing the same hasCapability('loginparams') approach, unless you'd rather take care of it yourself?