In #2709565: Convert function hosting_get_servers() to use db_select(), we added a 'node_access' tag to the query. Unless I'm mistaken, that means we're now triggering hook_node_access() implementations, where we weren't before. I believe the goal of that change was to allow filtering of server lists in the UI. However, this function should also be safe to call where node access checks aren't relevant. This appears to have had some unfortunate side-effects, such as https://gitlab.com/aegir/hosting_https/issues/7.

I believe that we should add an additional parameter that would trigger the node access checks, that defaults to "false", so as to return to the previous behaviour. We can then selectively change how we call hosting_get_servers() from the front-end code.

CommentFileSizeAuthor
#5 remove_node_access-2824329-5.patch1.1 KBhelmo
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ergonlogic created an issue. See original summary.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented

I like the solution above. Let's go with that.

Neograph734’s picture

Neograph734
he/him
Location Netherlands
CreditAttribution: Neograph734 as a volunteer commented

I suppose this came as a side effect of SA-CONTRIB-2016-046. hook_node_access() was invoked but always returned true for all content types if the user had permission to view only one of them. This is most likely the cause of #2821425: Views relations prevent sites and platforms from listing data for clients as well.

colan’s picture

colan
he/him
Primary language English
Location Toronto 🇨🇦
CreditAttribution: colan at Colan Schwartz Consulting commented
Related issues: +#2821425: Views relations prevent sites and platforms from listing data for clients
helmo’s picture

helmo CreditAttribution: helmo as a volunteer and at Initfour websolutions for Aegir Cooperative commented
Status: Active » Needs review
FileSize
remove_node_access-2824329-5.patch1.1 KB

Adding an extra optional parameter to hosting_get_servers sounds like a good plan, but having node_access enabled as default feels safer.

It should only be disabled for call where we know it's checked elsewhere.

What about this patch?

ergonlogic’s picture

ergonlogic
he/him
Primary language English
Location Montréal, Québec 🇨🇦
CreditAttribution: ergonlogic at Ergon Logic Enterprises for Aegir Cooperative commented
Status: Needs review » Reviewed & tested by the community

I'm fine with the default to "TRUE" here.

  • helmo committed 2f9dc3d on 7.x-3.x 
    Issue #2824329: Make 'node_access' check optional in hosting_get_servers...
helmo’s picture

helmo CreditAttribution: helmo as a volunteer and at Initfour websolutions for Aegir Cooperative commented
Status: Reviewed & tested by the community » Fixed

ok, committed.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.