? Failure

User tests: Successful: Unsuccessful:

avatar izharaazmi
izharaazmi
24 Dec 2015

The $_SESSION['__default']['registry'] is responsible for the userState
as in $app->setUserState('some.key.for.state', 'value');

Not migrating this value, as well as not even initializing this value as blank results in userState inaccessible until the user log-out and login again.

JApplicationCms::setUserState() requires the registry value set in order to be able to update user state.

Affected areas include sitewide pagination, filters, editing content etc.

This commit includes that key as a migratable key.

TESTING

  • Install Joomla 3.4.5 / 3.4.6 with sample data
  • Go to Article manager.
  • Filter the list using some filters, more that one filter would be better.
  • Navigate to any other page.
  • Go back to Article manager, the filters are still applied. (as expected)

  • Now update to Joomla 3.4.7 directly

  • Go back to Article manager, the filters are reset (not expected)
  • Apply some filters again. (Will be applied, but won't persist)

Repeat the process (starting with Joomla 3.4.5 / 3.4.6 again) but this time update with this patch included in the update package.

avatar izharaazmi izharaazmi - open - 24 Dec 2015
avatar izharaazmi izharaazmi - change - 24 Dec 2015
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 24 Dec 2015
Labels Added: ?
avatar izharaazmi
izharaazmi - comment - 26 Dec 2015

Please look at this one. From where I see this is really hot. I may be wrong, let me know in either case.
Also see #8777 (comment)

avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

My two possible solutions first, then the respective explanations with pros and cons follows.

Test and Patch as below

  1. First fresh install Joomla 3.4.6 and login to the administrator.
  2. Update to Joomla 3.4.7 from here
  3. Try editing an article. We all know it won't allow, so just try to be sure.
  4. DO NOT logout.
  5. Now apply any of the two patching options from below, to the file libraries/joomla/session/session.php:
  6. Then again, try to edit the articles. You should be able to do everything now.

Option 1

Replace the line ~595 which contains

$migratableKeys = array("user", "session.token", "session.counter", "session.timer.start", "session.timer.last", "session.timer.now");

with

$migratableKeys = array("user", "session.token", "session.counter", "session.timer.start", "session.timer.last", "session.timer.now", "registry");

Option 2

Find the end of the foreach loop

foreach ($migratableKeys as $migratableKey)
{
    // ... code for session migration goes here...
}

and add the two lines so that it becomes

foreach ($migratableKeys as $migratableKey)
{
    // ... code for session migration goes here...
}

$this->data->set('__default.registry', new Joomla\Registry\Registry);
unset($_SESSION['__default']);

How does this work?

Because the issue was not a permission migration issue. Permissions were migrated successfully with the JUser already with the J3.4.7 update.

The actual issue was that the JControllerLegacy::holdEditId() fails as it relies on JApplicationCms::setUserState() which in turn requires the registry to be Registry object in the session.

Since the old session is not destroyed completely during the session migration, we are able to migrate/reset the registry value. Afterwards we unset old session so that we do the reset only once.

Why the two options?

Option 1 does migrate all the old userState data by including registry as the migratableKeys which one may say to be not safe for security. However it retains all the components pagination, filters, edit id, form data etc.

Option 2 ignores all the old userState data and resets it to be a blank Registry object hence no security issue. It just lacks that all the components pagination, filters, edit id, form data etc. are discarded for once. But it is better than complete logout.

Who is affected and who is not?

When using this update,

  • Those who have or have not updated to J3.4.7 (but not J3.4.8) will get the benefit.
  • Those who already updated to J3.4.8 may not be able to see the difference as they are already logged out.
avatar photodude
photodude - comment - 1 Jan 2016

Looks like you have some Fatal errors that need to be addressed before this can be considered.

avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

@photodude Fatal error, where? If you are referring to the Travis result, I think that is not related to this PR. Please let me know otherwise.

avatar photodude
photodude - comment - 1 Jan 2016

I am refering to the travis-ci results. You made a change to the session and the session is failing, as such it looks like your change may have broken something (note: if you used 3.4.7 for your PR it will fail as there is a problem in that version).

From your title, it seems your targeting 3.4.7 which is a released version. I don't believe that the project will consider a PR that is against a version that is already released. To be considered, the PR should be introduced from the current staging branch. If/once accepted the patch would go into the next version 3.4.9+ .

avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

I am sure that the issue is not caused by this PR change. You can see https://travis-ci.org/joomla/joomla-cms/jobs/98701412#L963 Which shows that the JFactory::getUser() is called here. However, at that point the data property of the JSession was not initialized that's why the fatal error.

My change only comes into role when the session is being initialized.

I don't believe that the project will consider a PR that is against a version that is already released. To be considered, the PR should be introduced from the current staging branch.

I understand that this may be the case. Still I'd like people to test that if we apply this patch directly to J3.4.7 then it works or not.

If not so, then I'd quit.

If yes, then I suggest revert J3.4.8 (unless it does something more, apparently not the case) plus apply one of these proposed patch.

IMO J3.4.8 only targets to fix not allow to access that page directly. (XX) error. Does it fix anything else too?

avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

@nikosdion You were involved in the 3.4.8 fix. Can you please correct me in the assumption that J3.4.8 only targets to fix You are not allowed to access that page directly (XXX) error that occurred after update?

What were the other things it fixed?

avatar izharaazmi izharaazmi - change - 1 Jan 2016
Title
Include 'registry' in the session migratable keys during J3.4.7 update
Include missing 'registry' in the session migratable keys during J3.4.7 update
avatar photodude
photodude - comment - 1 Jan 2016

Joomla's versioning doesn't work that way. They use Semantic Versioning MAJOR.MINOR.PATCH .
Once released, you can't patch a patch; and you can't revert a released version. 3.4.7 is locked the way that it is, the fix is 3.4.8, and additional changes will go into 3.4.9.

avatar photodude
photodude - comment - 1 Jan 2016

@izharaazmi the release notes are listed on the Joomla website with the 3.4.8 announcment. https://www.joomla.org/announcements/release-news/5644-joomla-3-4-8-released.html

avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

@photodude When I said revert the release, I meant revert the changes made in that plus apply this change.
And yes I know any further change only affects next major/minor/revision version. That's how all versioning works, not just Joomla.

avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

@photodude Thanks for reminding me with the release announcement page. I will just cross verify my changes against those issues.

avatar izharaazmi izharaazmi - change - 1 Jan 2016
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2016-01-01 17:21:44
Closed_By izharaazmi
avatar izharaazmi izharaazmi - close - 1 Jan 2016
avatar izharaazmi izharaazmi - close - 1 Jan 2016
avatar izharaazmi izharaazmi - head_ref_deleted - 1 Jan 2016
avatar izharaazmi izharaazmi - head_ref_restored - 1 Jan 2016
avatar izharaazmi
izharaazmi - comment - 1 Jan 2016

@photodude @nikosdion Can you see the updated PR #8823 now that is now against staging branch.

avatar izharaazmi izharaazmi - head_ref_deleted - 1 Jan 2016

Add a Comment

Login with GitHub to post a comment