With the recent update of the ACL module to 1.4, the format of $element['user_list']['#default_value'] has changed from serialize() to json_encode().
The Content Access module relies on checking the element's user_list in order to show the fieldset as expanded/collapsed. The change to the ACL module means that trying to unserialize #default_value will always return false, and therefore the fieldset will always be collapsed. This change only affects UX and is non-critical.
After upgrading to ACL 1.4, visit the Access Control tab. Under the User Access Control Lists fieldset, all sub-fieldsets will always be collapsed even if they contain a list of users.
Change how $form['acl'][$op]['user_list'] is checked.
Fortunately, the ACL module provides a helper function to simplify this change: acl_edit_form_get_user_list()
Both the forum_access and flexiaccess modules already contain examples of this change.
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | Screenshot from 2023-08-30 14-20-52.png | 95.18 KB | mikkmiggur |
| #8 | content_access-compatibility_with_acl-3383002-8-D7.patch | 2.47 KB | gresko8 |
| #3 | content_access-compatibility_with_acl-3383002-3-D7.patch | 884 bytes | hargobind |
Comments
Comment #2
hargobindAttaching a patch with this one-line change.
Comment #3
hargobindAccidentally included the ['user_list'] in the previous patch.
Comment #4
larynContent Access is specifically mentioned in the security advisory for ACL 1.4: https://www.drupal.org/sa-contrib-2023-034
I maintain the Backdrop version and what I've done there is along the lines of what ACL has done in switching from using `serialize` to `json_encode` -- so it requires an update hook to convert any Content Access settings saved in the database from serialization to json_encoding, as well.
Here's the relevant commit: https://github.com/backdrop-contrib/content_access/commit/4a45c548414df6...
Would you consider expanding this issue and patch to include that sort of change for security hardening?
Comment #5
hargobindUltimately that decision rests with the module maintainers. But this seems like a sensible change to me.
It would maintain a bit of feature parity as far as how data is encoded for ACL and content_access.
It's also safer from a security standpoint since serialize()/unserialize() is prone to remote code execution vulnerabilities.
@laryn would you like to contribute your patch or do you want me to?
Comment #6
gisleI am the module maintainer, and I intend to upgrade as soon as there is a RTBC patch that includes the changes mentioned in comment #4.
Comment #7
gisleChanging status.
Comment #8
gresko8 commentedI'm attaching my attempt at porting a patch from commit mentioned in comment #4.
Comment #9
gresko8 commentedComment #10
larynThanks @gresko8! The patch looks good and applies cleanly.
@hargobind, are you able to do some testing on your sites?
Comment #11
larynComment #12
mikkmiggur commentedI think that issue priority should be higher because without that patch access page is broken for every single node.
#8 patch seems to fix that issue.
Comment #14
gislePatch in comment #8 committed.
Proceeding to create a new release 7.x-1.3.
Comment #15
hargobindWonderful. Thank you all!
Comment #16
mikkmiggur commentedComment #17
gislePlease don't set status "Closed (fixed)" manually. An issue with the status "Fixed" will be automatically closed as fixed and removed from the list of open issues after 14 days. It should remain open for 14 days after being "Fixed" to cater for regressions and other unintended consequences of the fix.
For reference, please see item #11 on the list of things not to do in issues on this page: Issue Etiquette.