User tests: Successful: Unsuccessful:
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
Go back to Article manager, the filters are still applied. (as expected)
Now update to Joomla 3.4.7 directly
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.
Status | New | ⇒ | Pending |
Labels |
Added:
?
|
My two possible solutions first, then the respective explanations with pros and cons follows.
libraries/joomla/session/session.php
: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");
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']);
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.
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.
When using this update,
Looks like you have some Fatal errors that need to be addressed before this can be considered.
@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.
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+ .
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?
@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?
Title |
|
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.
@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
@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.
@photodude Thanks for reminding me with the release announcement page. I will just cross verify my changes against those issues.
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2016-01-01 17:21:44 |
Closed_By | ⇒ | izharaazmi |
@photodude @nikosdion Can you see the updated PR #8823 now that is now against staging branch.
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)