? Success

User tests: Successful: Unsuccessful:

avatar Hackwar
Hackwar
4 Nov 2014

PR #3972 implements access cache clearing in a very BAD way. This reverts that and implements a new method named clearAccessRights() to achieve a similar effect.

avatar Hackwar Hackwar - open - 4 Nov 2014
avatar jissues-bot jissues-bot - change - 4 Nov 2014
Labels Added: ?
avatar infograf768
infograf768 - comment - 6 Nov 2014

I used the code proposed on joomlacode;

$user            = JFactory::getUser();
$session         = JFactory::getSession();
$session_id      = $session->getId();
$session_handler = $app->getCfg('session_handler');
$session_handler = JSessionStorage::getInstance($session_handler);
//$session_data    = $session_handler->read($session_id);

echo '<pre>session_handler: ' . print_r($session_handler, true) . '</pre>';
echo '<pre>session_id: ' . print_r($session_id, true) . '</pre>';
//echo '<pre>session_data: ' . print_r($session_data, true) . '</pre>';
echo '<pre>user: ' . print_r($user, true) . '</pre>';

Logged in frontend with a user assigned to registered, author and Editor (group_ids: 2,3,4 in _user_usergroup_map

Therefore when login with that user and the code above I get:

[groups] => Array
        (
            [2] => 2
            [3] => 3
            [4] => 4
        )

Then I edited the user in back-end and deleted the group editor and author (2 and 3)
Reloaded the page in frontend: no change

avatar Hackwar
Hackwar - comment - 6 Nov 2014

That is expected. As I wrote above, the code that was proposed is not acceptable and thus the way that you can test this wont work either. The new method to get those changes into the system needs to be manually triggered by the extension that changes the permissions for that user by calling JUser::clearAccessRights(). This will also only work for that one single user, not for other users. If you change larger parts of the ACL system, all users have to log out and log back in again. There is no way around this.

avatar infograf768
infograf768 - comment - 6 Nov 2014

In that case, I suggest this PR to be limited to a revert.

avatar Hackwar
Hackwar - comment - 6 Nov 2014

the actual code that allows extensions to clear the cache is 4 lines of code that a maintainer can simply review by looking at it.

avatar Bakual
Bakual - comment - 6 Nov 2014

the actual code that allows extensions to clear the cache is 4 lines of code that a maintainer can simply review by looking at it.

I agree with JM here. If there is currently no way to test it, this will not get merged.
So it's better to just revert the previous PR and then create a new PR for the new feature, accompagned with testing instructions.

Merging a new feature just based on review is not something I want to do myself.

avatar Hackwar
Hackwar - comment - 6 Nov 2014

This is the code that would have to be reviewed.

$this->_authLevels = null;
$this->_authGroups = null;
$this->isRoot = null;
JAccess::clearStatics();

Apparently it was easy accepting code that breaks a large part of our userbase, but reverting that and reviewing 4 lines of code is absolutely difficult. If you wanna test this, do this:

  • Write a component com_test with the following test.php:
<?php
var_dump(JFactory::getUser());
if(JRequest::getVar('clear')) JFactory::getUser()->clearAccessRights();
  • Login to the frontend, call /index.php?option=com_test, see the output.
  • Go into the backend and change the ACL, reload the page in the frontend and see that nothing changed.
  • Open /index.php?option=com_test&clear=1 and then /index.php?option=com_test and see that the new changes have taken effect.

I refuse to change this PR further. Either take it or I'll close this PR.

avatar Bakual
Bakual - comment - 7 Nov 2014

Apparently it was easy accepting code that breaks a large part of our userbase, but reverting that and reviewing 4 lines of code is absolutely difficult.

Aren't you a bit exaggerating here? From what I see when I did some tests with a standard installation, we're talking about two additional queries run, giving a total of around 30 queries for a page with some article and modules. The queries itself are lightweight actually and run fast (around a half milisecond each).
One is

SELECT b.id
  FROM #__user_usergroup_map AS map
  LEFT JOIN #__usergroups AS a 
  ON a.id = map.group_id
  LEFT JOIN #__usergroups AS b 
  ON b.lft <= a.lft 
  AND b.rgt >= a.rgt
  WHERE map.user_id = 123

the other

SELECT id, rules
  FROM `#__viewlevels`

I probably just fail to see how that will "break a large part of our userbase".
It slows down the site by some miliseconds and it may not be worth the added gain of instantely refreshing the permissions. It may also be worse if there are a lot of access levels and user groups.
But it will not break anything and not for a large part of the users imho.

So I agree with the revert in principle as the solution also brings troubles and the gain isn't that big to justify that. But I don't agree with the tone and pressure expressed here.

Imho there is no reason why we can't follow our rules of testing things prior to merge.

avatar Hackwar
Hackwar - comment - 7 Nov 2014

thing is, that those queries are executed each time those methods are called.

avatar infograf768 infograf768 - close - 7 Nov 2014
avatar infograf768 infograf768 - change - 7 Nov 2014
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2014-11-07 10:31:50
avatar infograf768
infograf768 - comment - 7 Nov 2014

I merged the revert part of this PR.
Thanks Hannes for the head up on this issue.
Please propose a new PR for your solution.

Add a Comment

Login with GitHub to post a comment