Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.Problem/Motivation
In a facet API list the IDs for access-controlled entities may still appear. These should be hidden instead. Just to illustrate, if we have entities with IDs 1, 2 and 3 and labels "Bananas", "Secret", "Apples", with ID #2 being inaccessible for the user, the list of options will look like this:
- Bananas
- 2
- Apples
Other than being meaningless to the end user (what does it mean to filter on "2"?), this is also a security issue in the (theoretical?) case the IDs contain sensitive information.
An example is using the termaccess module, with a taxonomy facet and one of the terms being "unpublished/hidden".
Proposed resolution
The values that are being fed into _search_api_facetapi_facet_create_label() may contain access-controlled/unloadable entities. Make sure these are not displayed.
Remaining tasks
Review patch.
User interface changes
Removal of IDs of unaccessible entities from lists.
API changes
| Comment | File | Size | Author |
|---|---|---|---|
| #12 | 2479399-12--hide_unknown_entity_facets.patch | 1004 bytes | drunken monkey |
| |||
Comments
Comment #1
stefan.r CreditAttribution: stefan.r commentedComment #2
stefan.r CreditAttribution: stefan.r commentedWe have been testing this on a setup with term access control and this seemed fine.
Comment #3
drunken monkeyThanks for reporting, and already providing a patch!
Seems pretty reasonable to me, I can't really think of a case where that might lead to undesired results.
However, I think the code is a bit clearer in this revised patch. What do you say, does this work for you, too?
Comment #4
stefan.r CreditAttribution: stefan.r commentedThis should work the same, but as of the latest version of Search API both patches seem to be incomplete now -- the results count for the "inaccessible" facet shows up in the results in parenthesis even if the map value is set to FALSE.
Do we know which commit may have caused that?
Comment #5
stefan.r CreditAttribution: stefan.r commentedComment #6
stefan.r CreditAttribution: stefan.r commentedWould this be OK?
Comment #7
drunken monkeyAre you sure this worked before? I can see no relevant change in either the Search API Facets code nor in the Facet API since your last patch. The only two issues committed for the two modules in 2015 are #2450227: Ignore parent terms in facets when operator is OR (seems unrelated) and #2382697: Typos and Spelling - D7 (fixes only typos). Can you please tell me which versions of Search API and Facet API you were using before, where it worked?
Anyways, it would be very easy to get this to work as intended with a small change to the Facet API, see #2545130: Allow the map callback to exclude items. (We could also do the same thing in the Search API Facets code, though, by overriding the method – if Chris doesn't think this change should go in.)
Comment #8
stefan.r CreditAttribution: stefan.r commentedWe were using stable versions and did make screenshots of the before and after with the old patch and it didn't have the term count in the after.
In any case let's see if we can get your patch in, it seems like it would result in a cleaner fix.
Comment #9
drunken monkeyIt would be helpful if you could get a Facet API maintainer to respond in the other issue.
Otherwise, please bump this issue in a month or two and we'll add the workaround to this module instead.
Comment #10
Patrick R. CreditAttribution: Patrick R. at UEBERBIT GmbH commentedSlightly updated the last patch to reflect the change in expected value (NULL instead of FALSE) introduced in 9c8713f, see related issue #2545130.
Comment #11
stefan.r CreditAttribution: stefan.r commentedno luck with facet API yet, so re-marking for maintainers to have a look at this per #9
Comment #12
drunken monkeyWhat do you mean? The Facet API issue has been committed one and a half years ago.
@ Patrick R.: Thanks for the update. However, the changes in the
SearchApiFacetapiTermclass are both unnecessary and wrong, as far as I can see. The attached patch should be all that's needed to fix this now (with the latest Facet API dev version – you should maybe ask the maintainers to do a new release).Comment #13
stefan.r CreditAttribution: stefan.r commentedmy bad