Adding system event OnBeforeInitSession #15944

sdrenth · 2021-12-24T09:39:17Z

What does it do?

I've added a new system event called "OnBeforeInitSession" which is called prior to starting the session.

I also added some additional checks to verify if the lexicon service has been loaded yet in the modMediaSource class. This was causing conflicts when invoking an event before _initCulture was called (which loads the lexicon service) and when listening to the event using a static plugin file.

Why is it needed?

We use a plugin to switch between contexts which listens to the OnMODXInit system event, but because this event is triggered after the _initSession method, the session is already set for context "web" and this causes conflicts with sessions. For example, not being able to login in the frontend for NON-web contexts.

Until this moment we switch the context in index.php, but it would be cleaner if we could do this by using plugins.

To resolve this we added this new system event which is triggered before initialising the sessions, allowing the use of plugins to switch context before the session is initialised.

How to test

Create another context
Create a login form on page (for example by using Login extra) on context which is not "web"
Set up static plugin which switches the context which listens to system event "OnBeforeInitSession"

Now you should be able to login on the non-web context.

Related issue(s)/PR(s)

#14962

core/src/Revolution/modX.php

opengeek · 2021-12-24T16:13:01Z

I'm not sure I follow the problem being addressed by this. A session is determined by the domain, not by a context. Can you describe the issue you are trying to solve in more detail?

sdrenth · 2021-12-24T16:32:26Z

@opengeek

Think of the following situation:

MODX initialises default context "web" in index.php using $modx->initialize('web');
A plugin handling a context switch needs to be triggered as soon as possible, before this PR that would be using the "OnMODXInit" system event
The plugin uses $modx->switchContext('de'); to switch to a different context

When you look at the code below, it first initialises the session based on the current context and after that the OnMODXInit event is invoked. By invoking a new event before initialising the session, the context can be switched before initialising the session, so now it is using the correct (switched) context.

public function initialize($contextKey= 'web', $options = null) {
    if (!$this->_initialized) {
        if (!$this->startTime) {
            $this->startTime= microtime(true);
        }

        $this->getCacheManager();
        $this->getConfig();
        $this->_initContext($contextKey, false, $options);
        $this->_loadExtensionPackages($options);
        $this->_initSession($options);
        $this->_initErrorHandler($options);
        $this->_initCulture($options);

        $this->getService('registry', 'registry.modRegistry');

        $this->invokeEvent(
            'OnMODXInit',
            array(
                 'contextKey' => $contextKey,
                 'options' => $options
            )
        );

        if (is_array ($this->config)) {
            $this->setPlaceholders($this->config, '+');
        }

        $this->_initialized= true;
    }
    return $this->_initialized;
}

Hopefully this clarifies things a bit :)

opengeek · 2021-12-24T16:40:52Z

The problem is that a session is based on domain, so there should be no conflict on contexts representing different domains (or subdomains as long as the session cookie is subdomain-specific). If you are trying to use a single domain for multiple contexts and allow separate logins for each, then, yes, this would be an "issue." But I'm not sure this addresses all of the potential issues of allowing separate login control within a single session cookie.

sdrenth · 2021-12-27T07:29:39Z

@opengeek

If you are trying to use a single domain for multiple contexts and allow separate logins for each, then, yes, this would be an "issue." But I'm not sure this addresses all of the potential issues of allowing separate login control within a single session cookie.

This really is an issue, I'm actually talking about a set up containing multiple contexts using different domains.

So I'm actually talking about trying to log in (using Login extra) in the frontend and switch context using OnMODXInit event:

domain.nl - Context key: web
domain.com - Context key: en
domain.de - Context key: de

Now I can only login in the web context. When on contexts "en" and "de", the login does not work. However, when I initialise the proper context immediately in index.php it does work in all contexts. Using the newly added system event "OnBeforeInitSession" I'm now also able to login for all contexts.

opengeek · 2021-12-28T02:14:43Z

Understood. We are past the feature freeze for the 3.0 release. However, before just pushing off until 3.1, I'd like to solicit input from others here.

opengeek · 2022-01-07T20:47:24Z

Circling back to this, since there has been no feedback, I'm still unsure how, if each context has its own session, there is a conflict in sessions here. You say the Login extra does not work. Can you provide specifics on how it is not working? When logging in, it should add the context you are logging in to to the existing session so when the next request loads it would be authenticated for that context. I'll try to set up some tests this weekend to explore this in more detail if you can't provide specific errors or details about the conflict you are having.

JoshuaLuckers · 2022-01-23T08:37:08Z

@sdrenth would it be possible to have a simple test case to demonstrate this issue?

sdrenth · 2022-01-25T13:35:59Z

@JoshuaLuckers Please see my comment previous comment on how to reproduce:
#15944 (comment)

JoshuaLuckers · 2022-01-25T14:24:37Z

@JoshuaLuckers Please see my comment previous comment on how to reproduce:
#15944 (comment)

I meant to refer to @opengeek's comment/request:

I'm still unsure how, if each context has its own session, there is a conflict in sessions here. You say the Login extra does not work. Can you provide specifics on how it is not working?

opengeek · 2022-01-25T16:52:16Z

I have certainly not been able to reproduce the issue. It would be helpful to have details on the site configuration so it is possible.

JoshuaLuckers · 2022-02-01T09:13:23Z

@opengeek wouldn't it be still useful to have this event regardless of the use case?

opengeek · 2022-02-01T14:06:00Z

@JoshuaLuckers — I don't like just adding more events, especially for something that has never been an issue before. It seems like there is some unique situation here but I am unable to know what that is without more details. Either way, this is not going to happen before 3.1 anyway.

sdrenth · 2022-02-01T14:29:58Z

@opengeek @JoshuaLuckers

This should be enough to be able to reproduce the issue without having this fix.

@opengeek

If you are trying to use a single domain for multiple contexts and allow separate logins for each, then, yes, this would be an "issue." But I'm not sure this addresses all of the potential issues of allowing separate login control within a single session cookie.

This really is an issue, I'm actually talking about a set up containing multiple contexts using different domains.

So I'm actually talking about trying to log in (using Login extra) in the frontend and switch context using OnMODXInit event:

domain.nl - Context key: web

domain.com - Context key: en

domain.de - Context key: de

Now I can only login in the web context. When on contexts "en" and "de", the login does not work. However, when I initialise the proper context immediately in index.php it does work in all contexts. Using the newly added system event "OnBeforeInitSession" I'm now also able to login for all contexts.

If you can't reproduce the issue, can you tell me how you've tested this?

Please have a look at this method in the MODX class:

        if (!$this->_initialized) {
            if (!$this->startTime) {
                $this->startTime= microtime(true);
            }

            $this->getCacheManager();
            $this->getConfig();
            $this->_initContext($contextKey, false, $options);
            $this->_loadExtensionPackages($options);
            $this->_initSession($options);
            $this->_initErrorHandler($options);
            $this->_initCulture($options);

            $this->getService('registry', 'registry.modRegistry');

            $this->invokeEvent(
                'OnMODXInit',
                array(
                     'contextKey' => $contextKey,
                     'options' => $options
                )
            );

            if (is_array ($this->config)) {
                $c = $this->config;
                if ((bool)$this->getOption('filter_config_placeholders', null, true)) {
                    unset($c['password'], $c['username'], $c['mail_smtp_pass'], $c['mail_smtp_user'], $c['proxy_password'], $c['proxy_username'], $c['connections'], $c['connection_init'], $c['connection_mutable'], $c['dbname'], $c['database'], $c['table_prefix'], $c['driverOptions'], $c['dsn'], $c['session_name'], $c['cache_path'], $c['connectors_path'], $c['friendly_alias_translit_class_path'], $c['manager_path'], $c['processors_path']);
                }
                $this->setPlaceholders($c, '+');
            }

            $this->_initialized= true;
        }
        return $this->_initialized;
    }

As you can see OnMODXInit is called after _initSession(), so changing the context using the OnMODXInit system event has no effect on the sessions, because those have been initialised already.

Looking at _initSession() you'll see that it's initialising sessions using the key of the current context. So when changing contexts using the OnMODXInit system event, it has still initialised the sessions using the web context. So in order to switch contexts and to properly initialise the session, the context switching has to be done before the sessions are initialised.

/**
     * Loads the session handler and starts the session.
     *
     * @param array|null $options Options to override Settings explicitly.
     */
    protected function _initSession($options = null) {
        $contextKey= $this->context instanceof modContext ? $this->context->get('key') : null;
        if ($this->getOption('session_enabled', $options, true) || isset($_GET['preview'])) {
            if (!in_array($this->getSessionState(), array(modX::SESSION_STATE_INITIALIZED, modX::SESSION_STATE_EXTERNAL, modX::SESSION_STATE_UNAVAILABLE), true)) {
                $sh = false;
                if ($sessionHandlerClass = $this->getOption('session_handler_class', $options)) {
                    if ($shClass = $this->loadClass($sessionHandlerClass, '', false, true)) {
                        if ($sh = new $shClass($this)) {
                            session_set_save_handler(
                                array (& $sh, 'open'),
                                array (& $sh, 'close'),
                                array (& $sh, 'read'),
                                array (& $sh, 'write'),
                                array (& $sh, 'destroy'),
                                array (& $sh, 'gc')
                            );
                        }
                    }
                }
                if (
                    (is_string($sessionHandlerClass) && !$sh instanceof $sessionHandlerClass) ||
                    !is_string($sessionHandlerClass)
                ) {
                    $sessionSavePath = $this->getOption('session_save_path', $options);
                    if ($sessionSavePath && is_writable($sessionSavePath)) {
                        session_save_path($sessionSavePath);
                    }
                }
                $cookieDomain= $this->getOption('session_cookie_domain', $options, '');
                $cookiePath= $this->getOption('session_cookie_path', $options, MODX_BASE_URL);
                if (empty($cookiePath)) $cookiePath = $this->getOption('base_url', $options, MODX_BASE_URL);
                $cookieSecure= (boolean) $this->getOption('session_cookie_secure', $options, false);
                $cookieHttpOnly= (boolean) $this->getOption('session_cookie_httponly', $options, true);
                $cookieLifetime= (integer) $this->getOption('session_cookie_lifetime', $options, 0);
                $gcMaxlifetime = (integer) $this->getOption('session_gc_maxlifetime', $options, $cookieLifetime);
                if ($gcMaxlifetime > 0) {
                    ini_set('session.gc_maxlifetime', $gcMaxlifetime);
                }
                $site_sessionname = $this->getOption('session_name', $options, '');
                if (!empty($site_sessionname)) session_name($site_sessionname);
                session_set_cookie_params($cookieLifetime, $cookiePath, $cookieDomain, $cookieSecure, $cookieHttpOnly);
                if ($this->getOption('anonymous_sessions', $options, true) || isset($_COOKIE[session_name()])) {
                    if (!$this->startSession()) {
                        $this->log(modX::LOG_LEVEL_ERROR, 'Unable to initialize a session', '', __METHOD__, __FILE__, __LINE__);
                        $this->getUser($contextKey);
                        return;
                    }
                    $this->getUser($contextKey);
                    $cookieExpiration = 0;
                    if (isset ($_SESSION['modx.' . $contextKey . '.session.cookie.lifetime'])) {
                        $sessionCookieLifetime = (integer)$_SESSION['modx.' . $contextKey . '.session.cookie.lifetime'];
                        if ($sessionCookieLifetime !== $cookieLifetime) {
                            if ($sessionCookieLifetime) {
                                $cookieExpiration = time() + $sessionCookieLifetime;
                            }
                            setcookie(session_name(), session_id(), $cookieExpiration, $cookiePath, $cookieDomain,
                                $cookieSecure, $cookieHttpOnly);
                        }
                    }
                } else {
                    $this->getUser($contextKey);
                }
            } else {
                $this->getUser($contextKey);
            }
        } else {
            $this->getUser($contextKey);
        }
    }

opengeek · 2022-02-01T15:35:02Z

The problem here is you need to initialize the proper context when calling initialize. I do not think adding an event for the session is the way to address that. This needs something before initialize is called or an event that is triggered at the very start of initialize which will handle the logic for determining which context to initialize in the first place.

sdrenth · 2022-02-01T16:17:52Z

@opengeek I understand what you're saying and I also thought about something like that but I think that's going to be tricky with the way MODX works and would require quite a lot of work rebuilding some core parts of MODX. Mainly because when working with a file based plugin, the extension packages will need to be loaded first so it can load the third party file based plugin which handles the context switching.

I think, at least for now, this is the best and quickest solution to resolve this.

opengeek · 2022-02-01T16:30:34Z

The recommended way to do this previously was to modify index.php to add the logic to determine the context. I would think that just allowing a simple include from the index.php, if it exists, could solve this issue without worrying about overwriting your index.php on upgrades.

sdrenth · 2022-02-08T10:09:32Z

@opengeek So something like this?

$contextKey = 'web';
if (file_exists(MODX_CORE_PATH . '/config/context_switch.php')) {
    require MODX_CORE_PATH . '/config/context_switch.php';
}

/* Set the actual start time */
$modx->startTime= $tstart;

/* Initialize the default 'web' context */
$modx->initialize($contextKey');

context_switch.php:

$contextKey = 'en';

opengeek · 2022-02-08T13:50:19Z

@opengeek So something like this?

$contextKey = 'web';
if (file_exists(MODX_CORE_PATH . '/config/context_switch.php')) {
    require MODX_CORE_PATH . '/config/context_switch.php';
}

/* Set the actual start time */
$modx->startTime= $tstart;

/* Initialize the default 'web' context */
$modx->initialize($contextKey');

context_switch.php:

$contextKey = 'en';

Along those lines, yes. I was thinking config.context.php to stay with the config.core.php naming convention, and I think it would be fine in the same directory with the index file. I'd also rather see it return the context key value rather than setting a variable.

sdrenth · 2022-02-08T14:02:58Z

@opengeek That would work for me as well. What do you think @JoshuaLuckers?

Only downside of this resolution I can think of is that it is not as flexible as when switching context using a system event, so plugins are not able to hook into this process. This will fix the issue for my use-case though, so I'm fine either way.

opengeek · 2022-02-08T14:11:10Z

@opengeek That would work for me as well. What do you think @JoshuaLuckers?

Only downside of this resolution I can think of is that it is not as flexible as when switching context using a system event, so plugins are not able to hook into this process. This will fix the issue for my use-case though, so I'm fine either way.

We can still consider the event for 3.1. I'll add this simple optional require for 3.0 final.

JoshuaLuckers · 2022-12-26T11:56:47Z

For now I don't see any need for adding this event, especially since we now have the solution from PR #16045

Feel free to close this issue if you agree @sdrenth

Adding system event OnBeforeInitSession

04362c4

sdrenth requested review from Mark-H and opengeek as code owners December 24, 2021 09:39

cla-bot bot added the cla-signed CLA confirmed for contributors to this PR. label Dec 24, 2021

Ibochkarev added the pr/review-needed Pull request requires review and testing. label Dec 24, 2021

JoshuaLuckers reviewed Dec 24, 2021

View reviewed changes

core/src/Revolution/modX.php Outdated Show resolved Hide resolved

sdrenth added 2 commits December 24, 2021 14:30

Add options as second param to invokeEvent

5385fdf

Pass options param as array

ba89307

This was referenced Dec 24, 2021

Feature/add onbeforeinitsession event #15945

Closed

Add OnBeforeInitSession event Sterc/revolution#53

Open

opengeek added this to the v3.1.0 milestone Dec 24, 2021

opengeek mentioned this pull request Feb 8, 2022

Optionally determine context from config.context.php #16045

Merged

sdrenth closed this Dec 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding system event OnBeforeInitSession #15944

Adding system event OnBeforeInitSession #15944

sdrenth commented Dec 24, 2021

opengeek commented Dec 24, 2021

sdrenth commented Dec 24, 2021

opengeek commented Dec 24, 2021

sdrenth commented Dec 27, 2021

opengeek commented Dec 28, 2021

opengeek commented Jan 7, 2022

JoshuaLuckers commented Jan 23, 2022

sdrenth commented Jan 25, 2022

JoshuaLuckers commented Jan 25, 2022

opengeek commented Jan 25, 2022

JoshuaLuckers commented Feb 1, 2022

opengeek commented Feb 1, 2022

sdrenth commented Feb 1, 2022

opengeek commented Feb 1, 2022

sdrenth commented Feb 1, 2022

opengeek commented Feb 1, 2022

sdrenth commented Feb 8, 2022

opengeek commented Feb 8, 2022

sdrenth commented Feb 8, 2022

opengeek commented Feb 8, 2022

JoshuaLuckers commented Dec 26, 2022

Adding system event OnBeforeInitSession #15944

Adding system event OnBeforeInitSession #15944

Conversation

sdrenth commented Dec 24, 2021

What does it do?

Why is it needed?

How to test

Related issue(s)/PR(s)

opengeek commented Dec 24, 2021

sdrenth commented Dec 24, 2021

opengeek commented Dec 24, 2021

sdrenth commented Dec 27, 2021

opengeek commented Dec 28, 2021

opengeek commented Jan 7, 2022

JoshuaLuckers commented Jan 23, 2022

sdrenth commented Jan 25, 2022

JoshuaLuckers commented Jan 25, 2022

opengeek commented Jan 25, 2022

JoshuaLuckers commented Feb 1, 2022

opengeek commented Feb 1, 2022

sdrenth commented Feb 1, 2022

opengeek commented Feb 1, 2022

sdrenth commented Feb 1, 2022

opengeek commented Feb 1, 2022

sdrenth commented Feb 8, 2022

opengeek commented Feb 8, 2022

sdrenth commented Feb 8, 2022

opengeek commented Feb 8, 2022

JoshuaLuckers commented Dec 26, 2022