This needs to be thoroughly tested to ensure it does not conflict with the
database session handler otherwise the time handling MUST be wrapped in a
check for the current handler.

This is also probably a bug in 3.x and should be fixed there, not only in
4.0.

On Sun, Feb 24, 2019 at 2:38 PM JoeforJoomla Boy notifications@github.com
wrote:

Fix MetadataManager to set correctly the current time when the session is
only updated and not created for the very first time
Summary of Changes

As of now the session time is updated inconsistently in the
updateSessionRecord function of the MetadataManager compared to the
DatabaseHandler class.

The MetadataManager at first update the session time using the timer start
value:
$session->get('session.timer.start')
when the session is closed and wrote in the DatabaseHandler class instead
the session time is updated again using correctly the current time
Testing Instructions

Start a session and check the time value in the database just after the
execution of the updateSessionRecord function of the MetadataManager, then
check it again after the session has been closed
Expected result

The time value is the current time in both cases
Actual result

The time value is the initial session time after the first stage in the
MetadataManager class, The time value is correctly the current time in the
DatabaseHandler class
Documentation Changes Required

This discrepancy in the start/end time of the session lifecycle brings
some side effects that i experimented on my localhost. When a lot of
concurrent requests are placed, for example ajax requests, it seems that
the server increases the time required to write and close a session, as a
result the time value in the database persists for several seconds with the
initial session start time.
Due to this gap if an application queries the 'time' database field it
gets an old and wrong value.

You can view, comment on, or merge this pull request online at:

#24002
Commit Summary

• [4.0] Fix MetadataManager to set correct time

File Changes

• M libraries/src/Session/MetadataManager.php
  https://github.com/joomla/joomla-cms/pull/24002/files#diff-0 (2)

Patch Links:

• https://github.com/joomla/joomla-cms/pull/24002.patch
• https://github.com/joomla/joomla-cms/pull/24002.diff

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#24002, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAWfoasbQjarI6KyIAtrPYvR8TvUuaxZks5vQvhQgaJpZM4bOwSM
.

--

• Michael Please pardon any errors, this message was sent from my iPhone.

I agree @mbabker however i tested 3.9 and it doesn't seem to be a problem given that the updateSessionRecord function of the MetadataManager is still not present.

And that method won't exist in 3.9. But, the same conceptual problem exists (and predates the manager class). The metadata manager only creates a record for a session if one doesn't exist, it doesn't do anything to update records in later requests.

Yes that's true in 3.9 but not i 4.0 where the metadata manager not only creates a record for a session if one doesn't exist, but it even updates records in later requests, from which the problem arises and i ignore if this wanted or unwanted behavior:

private function updateSessionRecord(SessionInterface $session, User $user)
	{
		$query = $this->db->getQuery(true);

		$time = $session->isNew() ? time() : $session->get('session.timer.start');

		$setValues = [
			$this->db->quoteName('guest') . ' = :guest',
			$this->db->quoteName('time') . ' = :time',
			$this->db->quoteName('userid') . ' = :user_id',
			$this->db->quoteName('username') . ' = :username',
		];

		// Bind query values
		$sessionId   = $session->getId();
		$userIsGuest = $user->guest;
		$userId      = $user->id;
		$username    = $user->username === null ? '' : $user->username;

		$query->bind(':session_id', $sessionId)
			->bind(':guest', $userIsGuest, ParameterType::BOOLEAN)
			->bind(':time', $time)
			->bind(':user_id', $userId, ParameterType::INTEGER)
			->bind(':username', $username);

		if ($this->app instanceof CMSApplication && !$this->app->get('shared_session', false))
		{
			$clientId = $this->app->getClientId();

			$setValues[] = $this->db->quoteName('client_id') . ' = :client_id';

			$query->bind(':client_id', $clientId, ParameterType::INTEGER);
		}

		$query->update($this->db->quoteName('#__session'))
			->set($setValues)
			->where($this->db->quoteName('session_id') . ' = :session_id');

		$this->db->setQuery($query);

		try
		{
			$this->db->execute();
		}
		catch (ExecutionFailureException $e)
		{
			// This failure isn't critical, we can go on without the metadata
		}
	}

I get that part, I wrote the darn class ?

Just making the point that this isn't a bug unique to 4.0 (though it's more exposed by having logic to run updates in place) and making sure that this doesn't cause issues when the database session handler is in use.

Ok :) Let me know about this PR, if needed or not. If you plan to fix this in another way in future feel free to close this one.

It looks fine for what it is.

I have tested this item ✅ successfully on 148a11c

To follow the test instructions, I extended a little "hello world" component with following lines, where a a session is created or updated and the corresponding session time is read out of the database.

As I could see, the old code gives sometimes a negative difference, the now code of course not.

It is therefore successfully tested.

private function getTimeOfUserZero( $db, $user ){
        $query = $db->getQuery(true)
                ->select($db->quoteName(['time'])) #
            ->from($db->quoteName('#__session'))
            ->where($db->quoteName('userid') . ' =  '.(int) $user->id)
        ;

        $timeresult = $db->setQuery($query)->loadResult();

        return $timeresult;
    }

	public function __construct(){
		parent::__construct();

        $session = Joomla\CMS\Factory::getApplication()->getSession();
        $db = JFactory::getDbo();
        $user = new User(331); // My current user
        $manager = new MetadataManager(Joomla\CMS\Factory::getApplication(), $db);

        $orgTime = $this->getTimeOfUserZero($db, $user);
        $manager->createOrUpdateRecord($session, $user);

        $updTime = $this->getTimeOfUserZero($db, $user);

        var_dump("We can see orgTime = $orgTime and updTime = $updTime with a difference of = ". ($updTime-$orgTime));

	}

I have tested this item ✅ successfully on 148a11c

I tested the item sucessfully as well.

Status "Ready To Commit", but 1 build failed.

Thanks!