Node access check missing from comment indexes

Good idea, thanks for the patch!
Aside from the test bot not succeeding in applying it, there are several things to still fix, though. But apart from those, if it works properly, I'd commit it to the project.

1. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   + * Contains the SearchAPiCommentAccess class.
   

   Wrong class name.

2. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   +   * Check whether this data-alter callback is applicable for a certain index.
   +   *
   +   * Returns TRUE only for indexes on nodes.
   +   *
   +   * @param SearchApiIndex $index
   +   *   The index to check for.
   +   *
   +   * @return boolean
   +   *   TRUE if the callback can run on the given index; FALSE otherwise.
   

   "nodes" in second line. Everything else should be replaced by "Overrides SearchApiAbstractAlterCallback::supportsIndex()." as the first line.

3. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   +   * Declare the properties that are (or can be) added to items with this callback.
   +   *
   +   * Adds the "search_api_comment_node_access" property.
   +   *
   +   * @see hook_entity_property_info()
   +   *
   +   * @return array
   +   *   Information about all additional properties, as specified by
   +   *   hook_entity_property_info() (only the inner "properties" array).
   

   Comment lines shouldn't exceed 80 characters, plus same as before with "Overrides …" plus the thing that changed compared to parent class.

4. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   +      'comment_node_access' => array(
   

   If you use the same field name as for nodes, search_api_search_api_query_alter() becomes a lot easier to implement. Also, this should in any case be properly namespaced.

5. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   +  /**
   +   * Alter items before indexing.
   +   *
   +   * Items which are removed from the array won't be indexed, but will be marked
   +   * as clean for future indexing. This could for instance be used to implement
   +   * some sort of access filter for security purposes (e.g., don't index
   +   * unpublished nodes or comments).
   +   *
   +   * @param array $items
   +   *   An array of items to be altered, keyed by item IDs.
   +   */
   

   This can just use @{inheritdoc}.

6. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   +    foreach ($items as $cid => &$item) {
   

   Since you always use $items[$cid] to manipulate the items, don't define $item as a reference.

7. +++ b/includes/callback_comment_access.inc
   @@ -0,0 +1,109 @@
   +      // Check whether all users have access to the node where the comment posted.
   

   Comment line length again.

8. +++ b/search_api.module
   @@ -1763,31 +1768,55 @@ function search_api_get_processors() {
   + * Adds access checks to the query, if enabled any.
   

   "… if any are enabled."

9. +++ b/search_api.module
   @@ -1763,31 +1768,55 @@ function search_api_get_processors() {
   +  if ($index->item_type == 'comment') {
   +    // Only add comment node access if the necessary fields are indexed in the index, and
   +    // unless disabled explicitly by the query.
   +    $fields = $index->options['fields'];
   +    if (!empty($fields['comment_node_access']) && !empty($fields['status']) && !empty($fields['author']) && !$query->getOption('search_api_bypass_access')) {
   +      $account = $query->getOption('search_api_access_account', $user);
   

   As said, use the same property name for both nodes and comments. I don't think the two branches are necessary in either case, though – 90% of the code is identical.

As said, you should really use the similarities between the two data alterations better, not just copy the whole code. Attached is a rewrite which uses as much common code as possible, thus significantly reducing the complexity. Please test/review whether it still works for you (best also node access).

Good to hear.
Committed.
Thanks again for your great work!

The patch removes the !empty($fields['search_api_access_node']) check in search_api_search_api_query_alter(). Is this deliberate?
After upgrading from 7.x-1.8 to 7.x-1.10, I get an error

SearchApiException: Required field search_api_access_node not indexed on index Default fuzzysearch index. Could not perform access checks. in _search_api_query_add_node_access()

Yes, I'd say that's deliberate. Silently failing to do node access sounds like a really, really, REALLY bad idea, don't you agree?
Either mark the field as indexed or disable the "Node access" data alteration. If you have the latter enabled, the module has to assume you want node access, and should not just ignore it if it can't add it.

But the "Node access" data alteration is already disabled. The $index->options['data_alter_callbacks'] for the default fuzzysearch index contains

   ["search_api_alter_node_access"]=>
    array(3) {
      ["status"]=>
      int(0)
      ["weight"]=>
      string(3) "-49"
      ["settings"]=>
      array(0) {
      }
    }

which is not empty. If I enable node access data alteration, it says

    array(3) {
      ["status"]=>
      int(1)
      ["weight"]=>
      string(3) "-49"
      ["settings"]=>
      array(0) {
      }
    }

On https://drupal.org/node/1254452 it says

Node access
Adds node access checks to searches on this index.

So if it's not checked, I'd expect that node access checks are not enabled and so I should not get this error.

OK, I'm definitely overworked. Another sloppy patch/review …
Thanks for spotting this! (You should have mentioned the disabled data alteration right away, though.)
The attached patch should fix this, it's a rather stupid mistake.

The patch seems to fix it. Thanks for the quick followup.

Just wanted to add a second confirming review for the patch in #22.

"Node Access" option is unchecked in:

admin/config/search/search_api/index/[index_name]/workflow

Without the patch we receive a barrage of "SearchApiException: Required field search_api_access_node not indexed..." errors. With the patch everything works happily again.

I'm not super familiar with all of the logic in the methods involved here, but from a look at the patch it appears pretty minor and just seems to address an oversight in a conditional check. Given this, and the fact that patch author is the maintainer, it seems safe to again mark this RTBC.

Thanks for the reviews, good to hear it works!
Committed.