Problem/Motivation

The three functions used for access checking (_linkchecker_link_node_ids(), _linkchecker_link_comment_ids(), and _linkchecker_link_block_ids()) all return arrays. When the returned array is empty, which can happen if the user does not have access to content, the content is not being checked for links, or if the link is not in any content, the response is to always deny access. In some cases, like if the link is not in any content, that is the wrong response. A better method would be to return an explicit value indicating that the user does not have access, when that is the case, check the return value for that value, displaying the appropriate error notice, and displaying something different for an empty array.

Proposed resolution

Change the return value for actual permission checks to something other than an empty array, like FALSE or NULL, that can be used to determine if permission was actually denied.

Remaining tasks

  1. Fix the issue
  2. Create a patch
  3. Review the patch
  4. Commit the patch

User interface changes

Some links previously erroneously identified with a permission denied message will be visible.

API changes

The three functions will also return FALSE, in addition to an array.

Data model changes

None.

Original report by [username]

N/A

CommentFileSizeAuthor
#5 Screen Shot 2017-12-01 at 8.31.24 PM.png43.11 KBKristen Pol
#5 Screen Shot 2017-12-01 at 8.31.05 PM.png24.38 KBKristen Pol
#3 linkchecker_change-access-check-return-values_2923219_3.patch6.15 KBoadaeh
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

oadaeh created an issue. See original summary.

oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented

BTW, I did try applying the patch in #1489132-17: "Permission restrictions deny" message on many broken links when https securepages, but that did not address this problem.
It might also be that this is a duplicate of #2195429: Permission restrictions deny for links to deleted nodes, but when I read through that issue, it seemed like the problem was fundamentally different.

oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented
Assigned: oadaeh » Unassigned
Status: Active » Needs review
FileSize
linkchecker_change-access-check-return-values_2923219_3.patch6.15 KB

The attached patch attempts to address the issue raised here, though I'm not sure the return code for _linkchecker_link_block_ids() totally does it.

oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented
Issue summary: View changes
Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
FileSize
Screen Shot 2017-12-01 at 8.31.05 PM.png24.38 KB
Screen Shot 2017-12-01 at 8.31.24 PM.png43.11 KB

Thanks for the patch! It worked well for us. Before the patch we see permission denied errors like:

After the patch, it says that they aren't in content like:

I reviewed the code and didn't see anything obviously wrong and it's following Drupal coding standards so marking RTBC.

Kristen Pol’s picture

Kristen Pol
she/her
Primary language English
Location Santa Cruz, CA, USA
CreditAttribution: Kristen Pol at Hook 42 commented
Status: Needs review » Reviewed & tested by the community

Really RTBC'ing this time.

dpi’s picture

dpi
Location Perth, Australia
CreditAttribution: dpi at PreviousNext commented

+1 RTBC.

NancyDru’s picture

NancyDru
she/her/hers
Primary language English
Location Boston
CreditAttribution: NancyDru commented

I also see this problem with Google Docs Viewer when it returns a 204 code. I am not sure your patch will handle that.

I added:

    if (empty($nids) && empty($cids) && empty($bids)) {
      if ($link->code == '204') {
        $nid = db_query("SELECT nid FROM {linkchecker_node} WHERE lid = :lid ",
          array(':lid' => $link->lid))->fetchField();
        $rows[] = array(
          l(_filter_url_trim($link->url, 40), $link->url),
          check_plain($link->code),
          t('No Content'),
          l(t('View node ' . $nid, array()), '/node/' . $nid),
          );
      }
      else {
        $rows[] = array(array('data' => t('Permission restrictions deny you access to this broken link.'), 'colspan' => count($header)));
      }
      continue;
oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented

@NancyDru that does not seem like the right way to fix that. I'm wondering if maybe there's something else going on that might be causing that, but I'm not sure how you got there, so I'm not sure. Would you mind taking a few minutes to explain the set up you have and how Google Docs Viewer and Link checker are interacting with each other? Thanks.

NancyDru’s picture

NancyDru
she/her/hers
Primary language English
Location Boston
CreditAttribution: NancyDru commented

I agree that it is not ideal.

I did set up an environment for testing the patch and I am not seeing the permission denied message any more, so you can ignore me.

With the patch I am not seeing these errors any more. +1 for RTBC.

oadaeh’s picture

oadaeh CreditAttribution: oadaeh at Hook 42 commented
Issue summary: View changes
ckng’s picture

ckng
Primary language English
CreditAttribution: ckng at O8 commented

Tested patch #3, works great.

oraculi’s picture

oraculi CreditAttribution: oraculi commented

Also tested patch #3 and it solved the issue.

A bit of testing also seems to suggest that clicking the "Clear link data and analyze content for links" button in the Maintenance section of the Link Checker config screen ( /admin/config/content/linkchecker, then expand 'Maintenance' at the bottom of the page) also resolves the problem.

I initially encountered the permissions error only after changing the type of links to check to "Internal and external".

(edit to fix typo)

  • VladimirAus committed d431957d on 7.x-1.x authored by oadaeh 
    Issue #2923219 by oadaeh, Kristen Pol, NancyDru, oraculi, VladimirAus:...
VladimirAus’s picture

VladimirAus
Primary language English
Location Brisbane 🇦🇺
CreditAttribution: VladimirAus at Tomato Elephant Studio commented
Status: Reviewed & tested by the community » Fixed

Thank you for contributions. Committed! 🍺

Status: Fixed » Closed (fixed)

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