[#28021] - Fix cache logic in UsergrouplistField field type
- Fixed in Code Base
- 4 Mar 2020
- Medium
- Build: staging
- # 28021
- Diff
- joomdonation:patch-1
- Pending Hound Hound is busy sniffing around... Details
User tests: Successful: Unsuccessful:
Pull Request for Issue #28013.
Summary of Changes
This PR fixes wrong cache logic implemented in UsergrouplistField field type. Quote from @mbabker:
Truthfully, that cache has some bad logic. The cache needs to account for the checksuperuser attribute and whether the current user is a super user and not just the element (whatever that actually is). Because THAT has more impact on whether the data should be different than whether the XML has a different structure.
So instead of caching field options base on the field xml definition, we will just cache base on data of the checksuperuser attribute and whether the current user is a super user and not just the element (personal, I think on a page load, just cache it base on checksuperuser attribute value should be enough)
Testing Instructions
-
Apply patch. Then go to Users -> Manage, click on Options button in the toolbar
-
Test case #1: Check and make sure the user groups displayed in New User Registration Group and Guest User Group is still OK (basically, same as before)
-
Test case #2: Test cache behavior:
-
Open the file libraries/src/Form/Field/UsergrouplistField.php, add command echo 'Run'; before
this line of code https://github.com/joomdonation/joomla-cms/blob/patch-1/libraries/src/Form/Field/UsergrouplistField.php#L84 -
Refresh the page, make sure you only see the word 'Run' one time.
-
Now, modify this line https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_users/config.xml#L24 , change 1 to 0 (so that checksuperusergroup attribute is different for two fields. Refresh the page, make sure you see the words Run two times.
Documentation Changes Required
No
| Status | New | ⇒ | Pending |
joomla-cms-bot
- | Category | ⇒ | Libraries |
@HLeithner I submitted this PR to staging because if this is accepted, it can be used to address the issue #27262 (security risk causes by wrong settings by Super Users who don't understand much about Joomla ACL).
BTW, there is a PR which tries to address that issue submitted for Joomla 3, too #27629
So please look at it and let me know if you still want to move this new attributesto Joomla 4 only.
I will check this with @joomla/security thx for the info
@Bakual It could be different thing, please see my comment here #28011 (comment)
| Labels |
Added:
?
|
||
So I changed the PR to only fixes the wrong cache logic, update the testing instructions. This PR is now ready for testing.
| Title |
|
||||||
I have tested this item
Like instructed.
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28021.
I have tested this item
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28021.
| Status | Pending | ⇒ | Ready to Commit |
RTC
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28021.
Thanks
| Status | Ready to Commit | ⇒ | Fixed in Code Base |
| Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2020-03-04 21:05:57 |
| Closed_By | ⇒ | HLeithner | |
| Labels |
Added:
?
|
||
Thanks for your pr @joomdonation but we don't add new features to j3, please remove the new option and only remove the caching.
If you can make a pr for j4 with the new attribute, I'm not really sure if this is a good attribute or way to do it because of the dynamic our acl has but maybe it's enough to solve simple requirements.