Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix cache logic in UsergrouplistField field type #28021

Merged
merged 3 commits into from
Mar 4, 2020

Conversation

joomdonation
Copy link
Contributor

@joomdonation joomdonation commented Feb 22, 2020

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

  1. Apply patch. Then go to Users -> Manage, click on Options button in the toolbar

  2. Test case Test #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)

  3. Test case Test #2: Test cache behavior:

Documentation Changes Required

No

@HLeithner
Copy link
Member

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.

@joomdonation
Copy link
Contributor Author

@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.

@HLeithner
Copy link
Member

I will check this with @joomla/security thx for the info

@Bakual
Copy link
Contributor

Bakual commented Feb 22, 2020

Actually, #28011 is the correct fix for the issue. We don't need another way of excluding groups.

@joomdonation
Copy link
Contributor Author

@Bakual It could be different thing, please see my comment here #28011 (comment)

@joomdonation
Copy link
Contributor Author

So I changed the PR to only fixes the wrong cache logic, update the testing instructions. This PR is now ready for testing.

@joomdonation joomdonation changed the title Fix + Improve UsergrouplistField field type Fix cache logic in UsergrouplistField field type Feb 23, 2020
@ReLater
Copy link
Contributor

ReLater commented Feb 26, 2020

I have tested this item ✅ successfully on 79a71ef

Like instructed.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28021.

@Quy
Copy link
Contributor

Quy commented Feb 27, 2020

I have tested this item ✅ successfully on 79a71ef


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28021.

@Quy
Copy link
Contributor

Quy commented Feb 27, 2020

RTC


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/28021.

@joomla-cms-bot joomla-cms-bot added the RTC This Pull Request is Ready To Commit label Feb 27, 2020
@HLeithner HLeithner merged commit 5e7c899 into joomla:staging Mar 4, 2020
@HLeithner
Copy link
Member

Thanks

@HLeithner HLeithner added this to the Joomla! 3.9.16 milestone Mar 4, 2020
@joomla-cms-bot joomla-cms-bot removed the RTC This Pull Request is Ready To Commit label Mar 4, 2020
@joomdonation joomdonation deleted the patch-1 branch March 16, 2020 02:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants