fix(registry): don't clobber $GLOBALS['injector'] on modern requests

[jcdelepine]

Horde_Registry::__construct() unconditionally overwrites $GLOBALS['injector'] with a brand new Horde_Injector, with none of the application's bindings registered on it. This is harmless on the legacy bootstrap (Horde_Registry::appInit() is the one populating that global in the first place), but on the modern PSR-15 stack (RampageBootstrap), $GLOBALS['injector'] is never set. Any service resolved by the modern container that happens to auto-wire a fresh Horde_Registry (e.g. LoginService's $registry property, since Horde_Registry::class has no explicit binding) ends up creating a third, disconnected injector as a side effect — distinct from both RampageBootstrap's own $injector and any other legacy caller's copy.

Concretely: LoginService::performLogout() calls
$this->registry->clearAuth(), which resolves SessionLifecycle via $GLOBALS['injector'] — now pointing at this disconnected injector — and destroys the session there. performLogout() then calls $this->sessionLifecycle->setup() on its own (correctly wired) SessionLifecycle instance, whose internal active/setupApplied state never learned about the destroy. setup() skips re-initialising session_set_cookie_params()/session_name()/etc (setupApplied still true) and skips restarting the session (active still true), while the underlying native PHP session has already been torn down. session_write_close() at shutdown then fails against this inconsistent state, silently logged as 'Failed to write session data'.

Observed concretely via OIDC back-channel logout followed by an idle browser: the automatic silent re-login after SLO runs through this exact path with no user interaction, leaving the session broken. Interactive logouts were unaffected, which is why this had gone unnoticed.

Fix: publish RampageBootstrap's injector into $GLOBALS['injector'] immediately after creating it, and make Horde_Registry's own fallback creation of that global idempotent ($GLOBALS['injector'] ??= new ...) instead of unconditional — so legacy bootstraps keep their existing behaviour, and modern requests share one consistent injector end to end.

Fixes #218.

[jcdelepine]

@ralflang I forgot to indentify you.

[ralflang]

This part is not controversial and immediately looks like a win.

[ralflang]

This part I need to re-review tomorrow with the full context. I don't want to make the rampage bootstrap publish any globals.

For the specific case of creating Horde_Registry from DI (rather than from ::appInit) we may be better off with a factory or a lazy proxy registered in the injector.

[ralflang]

I have thought about this a bit over the morning coffee.

The RampageBootstrap is designed to not force all Horde 5 assumptions, globals and conventions onto the user.
However many (most?) routes depend on globals-heavy parts of the ecosystem.

Horde\Core\Middleware\DefaultStack with Horde\Core\Middleware\HordeCore does this initialization by running

         Horde_Registry::appInit('horde', ['authentication' => 'none']);
        $injector = $GLOBALS['injector'];

and then some more setup.

We can easily just setup

        $GLOBALS['injector'] = $injector;
         Horde_Registry::appInit('horde', ['authentication' => 'none']);

or possibly even

         // Let Horde_Registry setup the global from a passed variable
         Horde_Registry::appInit('horde', ['authentication' => 'none'], $injector);

This would hand back control to the developer and keeps the RampageBootstrap clean.
Developers who want/need the legacy globals will use a stack which includes HordeCore middleware.
Developers who want a minimal/clean environment don't.

[ralflang]

I will probably merge this as-is and then revert the RampageBootstrap line in the next commit in favor of my described path.

[jcdelepine]

Hy, Quoting Ralf Lang ***@***.***>:

ralflang left a comment (horde/Core#227) I will probably merge this as-is and then revert the RampageBootstrap line in the next commit in favor of my described path.

If you let me some time I will certainely be able to follow your directives. I have something, after your first message, but not tested yet. Sincelry, Jean Charles Delépine

…

-- Service systèmes et réseaux - DISI Université de Picardie Jules Verne 5, rue du moulin neuf - 80000 Amiens

[ralflang]

In this case I am glad to accept your contribution. Take your time.

[jcdelepine]

Hi Ralf,

Thank you for the flexibility. Given that, I'd actually prefer you merge this as-is now rather than wait on me — I have OIDC work depending on this fix and would like to move forward with it.

I did spend some time trying to find a cleaner alternative to the RampageBootstrap line (a DI factory for Horde_Registry, plus pinning SessionLifecycle as a top-level singleton), since I understood you wanted to avoid publishing globals from Rampage. It turned up a deeper issue: SessionLifecycle can end up resolved twice within the same request through certain nested dependency-resolution paths (a middleware's constructor parameter pulling in Horde_Registry, itself instantiated deep inside the injector's own annotated-setter resolution), and pinning at the top level doesn't seem to propagate through that path. That felt like injector-internals territory rather than something I could safely fix from the application side, so I'll leave it to your judgement whenever you get to the RampageBootstrap follow-up.

Happy to share the instrumented logs/backtraces from that investigation if they're useful context for your fix.

Sincerely,
Jean Charles Delépine

[ralflang]

Hello @jcdelepine in this case I will move on with the merge and fix strategy as proposed. I think we need to be very careful to make the "classic" bootstrap more predictable and avoid concurrect SessionAccess/SessionLifecycle instances or multiple registry instances.

Ultimately we want to move away from the fat and monolithic older registry architecture, allowing each request to only pull in the sub systems it really needs.

For your OIDC related work, I some headsup:

• I am refactoring IMP to migrate off the old horde/imap_client lib/ to the modern horde/imap_client src/ implementation
• As an intended side effect this disables IMP's "application authentication" in favor of per-mailbox authentication handling.

If you like to share your intended integration architecture (email?) I would try to prioritize bits which are needed to accomodate you.

Thank you for investigating this!

[jcdelepine]

Hi Ralf, Following up on your heads-up in Core#227 — I looked at the new `ImapClient` (src/, FRAMEWORK_6_0) before answering. Given its constructor (per-instance `Credentials`, via `horde/sasl` and `TokenCredentials`), imp#65 as it stands doesn't fit your model anymore: `IMP_Application::authValidate()` validates a single token at the application level, for "the" active `server_key` — whereas your design allows several concurrent `ImapClient` instances, each with its own credentials. Four concrete questions so I'm not working blind: 1. The integration point for supplying a `TokenCredentials` (instead of a classic password) when each per-mailbox `ImapClient` is constructed — is that already planned somewhere in your refactor, or still open on IMP's side? 2. I also noticed ***@***.*** (generalizing `BackendConfigLoader` to accept any variable name, explicitly citing IMP's `$servers`) — is that the foundation `ImapClient` will read backend config through generically? If so, should the `oauth` backend key move onto that generic loader instead of IMP's current ad-hoc `IMP_Imap_Config` parsing? 3. What's the expected timeline for this migration — rough order of magnitude (weeks, months)? 4. Given that timeline: do I leave imp#65 as-is for now, or would you rather I start moving the validation/refresh logic from session-level to connection-level right away, adjusting once your refactor lands? Thanks, Jean Charles -- Service systèmes et réseaux - DISI Université de Picardie Jules Verne 5, rue du moulin neuf - 80000 Amiens

[ralflang]

Hi Ralf,

Following up on your heads-up in Core#227 — I looked at the new
ImapClient (src/, FRAMEWORK_6_0) before answering.

Given its constructor (per-instance Credentials, via horde/sasl
and TokenCredentials), imp#65 as it stands doesn't fit your model
anymore: IMP_Application::authValidate() validates a single token at
the application level, for "the" active server_key — whereas your
design allows several concurrent ImapClient instances, each with its
own credentials.

Four concrete questions so I'm not working blind:

1. The integration point for supplying a TokenCredentials (instead
   of a classic password) when each per-mailbox ImapClient is
   constructed — is that already planned somewhere in your refactor, or
   still open on IMP's side?
2. I also noticed @.*** (generalizing BackendConfigLoader to
   accept any variable name, explicitly citing IMP's $servers) — is
   that the foundation ImapClient will read backend config through
   generically? If so, should the oauth backend key move onto that
   generic loader instead of IMP's current ad-hoc IMP_Imap_Config
   parsing?
3. What's the expected timeline for this migration — rough order of
   magnitude (weeks, months)?
4. Given that timeline: do I leave imp#65 as-is for now, or would you
   rather I start moving the validation/refresh logic from session-level
   to connection-level right away, adjusting once your refactor lands?

Thanks,
Jean Charles

Service systèmes et réseaux - DISI

Université de Picardie Jules Verne
5, rue du moulin neuf - 80000 Amiens

Hi Charles,

tentative timeline for imp modernization is "somewhere in october" - I will try to outline the supporting infrastructure and integration points this week.

For the first round I intend to only support

session password (+traditional hooks)
password from traditional prefs "additional accounts" source
OIDC/Oauth 2.0 credential

A later round would add service passwords delegated to a secure store interface.

More details as soon as I can provide.