Read-only index should not be removed

Created on 1 September 2023, about 1 year ago
Updated 6 September 2023, about 1 year ago

Problem/Motivation

Search API indexes can be read-only, most of the read-only functionality is handled by the Search API module, except for removeIndex method, which is implemented by every backend.

Proposed resolution

A simple check if the index is flagged as read-only should be enough. It would probably be good to implement some tests for this case.

Remaining tasks

📌 Task
Status

Fixed

Version

1.0

Component

Code

Created by

Live updates comments and jobs are added and updated live.
Sign in to follow issues

Comments & Activities

  • Issue created by @admirlju
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    103 pass, 2 fail
  • @admirlju opened merge request.
  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    104 pass
  • Status changed to Needs review about 1 year ago
  • 🇸🇮Slovenia DeaOm

    This one is a little bit weird to me, if you have a read-only index and you delete it, the index on the search api overview page gets removed, but the index in meili search stays. If you then add new index with the same name as the read only "deleted" one, it gets "populated" by the one from meili search. So if read-only indexes should not actually be deleted/removed, than what's the point of having the "delete" option if that option only removes it from the overview page?
    If this is the way it needs to be, then some additional check is needed, that indexes do not have the same names. If it's expected to have one read-only index that is removed from search api overview and then an additional index (that by coincidence has the same name), that is not the read-only deleted one.
    I think it makes sense to either allow deletion of read-only indexes form overview (like it already is) and also remove it from meilisearch OR remove the delete option for read only indexes (or do not remove it from the overview and add a message, that read only indexes can't actually be removed).
    I'm leaving this on needs review so somebody else can share their opinion as maybe I'm just over complicating things, but it's just confusing to "delete" and index, and actually it not being removed.

  • So this is something @bcizej mentioned in one other issue, to add support for read-only, but later on, we noticed in the Search API modules code, that in the BackendPluginBase isReadOnly method is used in removeIndex method.

    I then checked how they implement the method for Search API DB module. Here they also make sure to remove the data if it's not read-only.

    What I assume is, that they expect the service that the backend is hooking in to handle the above-mentioned problems. Example for MeiliSearch, it has Master and API keys. With API keys, you can control what indexes it can access and what operations are allowed on them. This part (kinda) connects to a feature request Allow usage of API with dropped privileges (not only master key) Needs work . And from looking at the Meilisearch php library, instead of the master key you can supply it API key.

    I just thought that we should be consistent with the original implementation since admins can enable read-only checkbox on the index settings.

  • I've been testing what I mentioned above with Meilisearch API keys. So with the master key one can add keys that are limited to actions or indexes.
    Here is an example of creating (POST request to /keys) a key, that can get version, documents, indexes, tasks, stats, and search query:

    {
    	"name": "Test key",
    	"description": "Search, view documents and version",
    	"actions": [
    		"search",
    		"documents.get",
    		"indexes.get",
    		"tasks.get",
    		"stats.get",
    		"version"
    	],
    	"indexes": ["*"],
    	"expiresAt": null
    }
    

    This generated a key, that can be used in the master key field and will not allow the backend to modify the index at all.

  • 🇦🇹Austria tgoeg

    On a side note:
    The referenced issue regarding dropped privileges is by me :-)
    We actually use this in production, and it works out neatly, although it is currently not meant to be used like this (just add the dropped priv key as master key in the module's config). It is however crucial to have this option to protect indexes from getting deleted (accidentally), getting populated from other (multi) sites, or even for testing purposes (i.e. see how a different frontend implementation works with a given index). It also helps to confine potential security breaches.
    I am not really sure how to handle this, as I see similar problems regarding deletion and re-creation as you do, but I just wanted to state that having indexes read-only (by whichever means, be it by a search_api feature or by dropped API privileges) definitely has its uses.

  • 🇸🇮Slovenia bcizej

    The "read only" feature should work as it was designed by the search_api module. Theres no point at the moment making some custom logic for this.

    So the data in meilisearch must not be altered in any way if the index is set to read-only. That also means that if you are setting an index as read-only, you expect data to exist in meilisearch and is ready for use. The thing here is that the fields meilisearch documents must match with how we generate them by indexing items. So at least the attribute "search_api_id" must exist in documents to match documents with drupal entities. In short, connecting to an existing meilisearch index will work only if the index and documents were generated by some other drupal instance using this module.

    If we want to support connecting to indexes that were generated by 3rd party, then a custom datasource would need to be implemented. Solr does this by implementing solr_document and solr_multisite_document datasource plugins that allow you to configure field mapping ie. drupal field <-> solr field. But that is another feature that can be implemented in the future.

    As for the different api keys I already mentioned possible solution in Allow usage of API with dropped privileges (not only master key) Needs work . Feel free to contribute there.

  • Status changed to RTBC about 1 year ago
  • 🇸🇮Slovenia DeaOm

    If we leave the search api functionality overview as is (deleting the index from Drupal, but leaving the meilisearch intact), then the MR does just that, and tests are also passing, so marking it as RTBC.

  • Open in Jenkins → Open on Drupal.org →
    Core: 9.5.x + Environment: PHP 7.4 & MySQL 5.7
    last update about 1 year ago
    109 pass
  • Status changed to Fixed about 1 year ago
  • 🇸🇮Slovenia bcizej

    Merged, thanks.

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

Production build 0.71.5 2024