Allow usage of API with dropped privileges (not only master key)

I think key management or access levels/permissions is of out of scope of this module but I do agree that master key should not be needed. What we can do here is change the title and add a description of what permissions the key must have to perform operations within Drupal to the configuration form field.

Perhaps we can add an extra optional field for a key that supports search operations only to be used with searches although I don't see any benefit from the security standpoint (both keys are stored in Drupal) apart from maybe when used in decoupled architecture so searches can be done directly from the frontend.

As for the example you provided if I understood correctly, the issue is that even if your key has "indexes.create" permission, you cannot access the newly created index because of "indexes": ["my_first_allowed_index", "my_second_allowed_index"] setting?
Looking at the documentation it does state that "indexes" field does support wildcards and can be granted access to all indexes with ["*"]. If that is too loose it can be narrowed down to indexes starting with specific prefix like ["my_*"].
That would mean that the user in Drupal with such a key could create indexes starting with "my_" and have access to the newly created index.
Keep in mind I did not test this and the docs are for the latest stable version which we are not using (yet).

Now that we are working with the latest stable version of Meilisearch. I wanted to test the wildcard option in the index parameter.

I created a new API key with this body:

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

This API key can do almost everything master_key can, except modify API keys and it can only do its operations on indexes that are prefixed with search_api_, this also means creating a new index (but the prefix limit is still there).

Implementation idea:

Knowing this I was thinking of how to implement the functionality to be as simple as possible. First I think this could be a submodule, so if someone wants a basic or doesn't have a multi-tenant setup, doesn't have to use it.

Now this submodule would modify the first-time configuration of a new server. We'd still keep the master key field, but it would be a one-time usage key, just used for the generation of the API key. Also, there would have to be an index prefix field. Having these two fields we could create a new API key with the actions I put in the upper JSON body and indexes set to ["prefix_*"]. At this point we would store the API key and the master_key would never be used again.

Then modify the index creation and any interaction with the index, to always prefix the index id. Now the simplest way to implement this would be to override the machine name of the index on index creation, this way we don't have to change any other functionality of the base module.

This would introduce the requested security and functionality, but I see some complications that would show up.
The first one is if there is already an existing active server with indexes. For this, I think we should just set the prefix to an empty string and leave the master_key as was set.
The second is removing of submodule. So if the API key stays there searching, modifying, and deleting existing indexes should not be a problem, but if someone tries to add a new index to the server it will only work if the admin adds the prefix that works with the API key when naming the index.

Now after that long text. I'd like to hear opinions about this implementation. Especially from @tgoeg since from what I saw in other issues you actively use Meilisearch and this module.

Seems I did not push save the last time I wrote an answer to this, sorry.

I didn't know about the wildcard option. But I like the idea very much. Do you know why? My indexes already have a prefix, the domain they are being used on :-)

So yes, this feels pretty natural to me and I guess it will to anyone having a multisite setup.

I am unsure about the consequences when deleting an index. If a wildcard API key exists, it will most likely continue to exist. I think I remember that API keys vanish when their index gets deleted (but I may be wrong). Creating an index should also be fine, as - as long as the prefix is correct - there is no additional key needed (which would otherwise be the case; a per-index key needs to exist to be able to use the index, and therefor creating a new index would not make too much sense).

I currently get an error "Index already exists" if drush cim a config with an existing index. Would this be "fixed" then? I do not think an index should then be deleted, even if that would be possible now.
But that's a different issue actually, which I think needs a little modification as well.

There might be applications where protecting an index from getting deleted might make sense. My current (manually-created-key-)setup does just that. Drupal may in fact only really add and remove documents, and search through them, which feels adequate for a production instance. No way to really destroy anything.
I think people who want just this can however still lock down keys manually. This module should probably not break if people do it, however.

This way, I think this could be added to the core functionality right away. It does not really seem to have the potential to bring disadvantages to anyone, and it would prevent people from using a master-key based setup. I think that's something that should never be possible. It feels like saving your root password into some configuration in plain text.

If this shall be a separate submodule then there should be a big fat warning that running without it is not at all recommended.

I might be a little biased but I think prepopulating the (editable) prefix with the transliterated domain of the page, e.g. my-website.co.at will become my_website_co_at_ as a prefix) could help beginners a lot to get a clean setup.

Regarding your questions:
> The first one is if there is already an existing active server with indexes. For this, I think we should just set the prefix to an empty string and leave the master_key as was set.
Yes and no. It will keep legacy setups running, yes. But I'd like to see an upgrade path for this. It would be great if one could provide a prefix (that's already in use) and a key, basically just like the new algorithm layed out above would do, just that the key would not be created, but an existing one could be entered.
If the algorithm would just fill in the prefix and the (generated) key upon clicking some "generate" button (that also needs to have the master API key supplied), then those fields could be editable as well (with a warning stating that the key and prefix either need to exist already (for upgrades) or one should better use the generator, if a new key/prefix (and probably index) is preferred.

> The second is removing of submodule. So if the API key stays there searching, modifying, and deleting existing indexes should not be a problem, but if someone tries to add a new index to the server it will only work if the admin adds the prefix that works with the API key when naming the index.
Yes. And a note for this would need to be added. Which makes part of this functionality appear in the main module again. I think this should go into the main module (did I already mention that?). But, as I stated above, using a submodule is a possibility as well. The big fat warning™ from above would need an additional info that exactly those problems will arise if anyone decides to remove the submodule. I can't think of any meaningful setup that would need this, though.

Here are two mockups and the HTML that generates them.
I am unsure whether this should probably better be put into a separate tab. The double "generate key" buttons are no good solution yet. Probably just add a link to the second tab and do the key generation there.
Alternatively, it could be fit right into the basic configuration page already as well, should not be too long.

As you can see my screenshots are partially German. I am from Austria, so I can provide translations as well (later on).

So from my understanding, you don't want Drupal to be capable of creating a new API key. You think people should do this by hand outside Drupal? They should make sure that the correct privileges are setup and the correct index prefix is setup.

No!
And sorry, I seem to not have been clear.

I DO think it's a good idea for Drupal to setup keys. Something along the workflow depicted in the mockups.
It's just that the usage of prefixes (which is a good design decision IMHO) will not always be backwards compatible with existing setups. Only in the case of existing indexes that do NOT conform to a prefixed namespace will an admin have to manually manage keys outside of Drupal.
And that will be a very small percentage of users, if they exist at all.
Even if people have existing indexes, I - for one - would advise to drop them, generate a key like in the mockups and create the indexes anew, now conforming to the prefixed setup.

I think what needs additional attention is the creation of new indexes.
When a prefix (and accompanying key) is setup, only indexes starting with the prefix can be created. So misconfiguration can technically be ruled out as wrong indexes will not be able to be created anyway.
However, it might be helpful if it were possible to restrict new index creation to names starting with the prefix.
If that's too complicated, the error message when trying to create an index with a different prefix should state state the index does not start with the prefix. Not just that index creation failed because of missing permissions.

Ah ok, now I understand.

When it comes to creating new indexes, we have full control over what the index is named. I was thinking that prefixing should just happen in the code, an admin when creating a new index would just type in the name without the prefix. Of course, we'd add a quick check if the admin already included the prefix by hand, just so there is no double prefixing.

Ok, I've been wondering if this is a good way to do the configs.

This is how the server config looks when you first open it and set the prefix.

When you select "Generate API Key":

Once you save the server configuration:

Of course, I'll add more explanation to the field description. Also, the Generate API key checkbox is grayed out if there is a set access key.

Still needs a lot of work. Since the currently published code creates a fully permissive API key "actions": ["*"]. I think it would be good to add an extra checkbox when confirming deletion, to also remove the generated API key.

The prefixing also works now. So if someone sets the prefix, all indexes created from this point will automatically prepend it.

I'd just like to know if the UI is logical the way I made it. If there are any other things that should be added or changed.

Yeah! That looks good to me as well! Probably re-use some of my description texts to make it clearer.

Would I be able to deliberately delete the key and re-activate "Generate API key"?
There might be situations where I want to have a new key when one is configured already. Maybe when a new/empty Meilisearch server gets used and I want to start anew.
Or probably do not deactivate the checkbox and only show a red warning when it is selected and a key is already configured?

Comments overlapped.
Yes, a confirmation and deletion of the generated key totally makes sense. I do already have a lot of unused keys from development tries. This would definitely make setups cleaner and mean less manual admin work on the Meilisearch backend.

At the moment if you delete the key, it will enable the Generate API key checkbox and you can generate a new key. That said I need to add a way for the key to actually be removed from Meilisearch.

I think for the most part I implemented how I think it should work. But still need to add error handling (at the moment a quiet failure happens) and to add some automated tests.

I made it so prefix can't be changed once set at index creation. Sadly how key management works, every time the prefix gets changed we'd have to generate a new key.
Now when editing an already created server you can click generate new API key and this will cause the old key to be deleted and the new one set. When deleting a server you now have a master key field as a confirmation to delete the key, might change this to a checkbox, that shows the master key field. We always need to prompt the admin to type in the master key for this task.

All of this sounds reasonable to me!
I think a warning should be added that keys will get deleted (haven't looked at the actual code yet, maybe it is like this already). Most of the time this will be desired, but there may be cases where an index+key may be used from multiple Drupal instances or even different frontends.
Taking care of every hypothetical setup should not be the aim here, however. I'd expect admins of complex setups to be able to handle them manually, for themselves. A warning/note however should protect from unexpected accidents.

> Sadly how key management works, every time the prefix gets changed we'd have to generate a new key.
Yes, that's something that irritated me as well already. Should we probably file a feature request for an UPDATE feature of the /keys endpoint to allow both changing the index and/or the key?

Yes, I made sure to use a warning banner, so it pops out and makes it harder for admins to miss the message. The banner explains that if an admin wishes to keep the key, they can create one manually and paste it, this will not cause key deletion.

With the default D9.5 admin theme, it looks like this:

We could fill a feature request, they have a specific page for that. But I think it will not be their priority, so it might take some time before it's added.

Ok, so I've noticed one more thing, that might cause pain to admins. Looking at the Meilisearch documentation, API keys are calculated using the master key and key uid. This means that if an admin changes their master key, Meilisearch will generate a new API key hash so the key that is saved on Drupal will not work anymore.

I don't know if this should be a warning somewhere.

OK, RTFM'ing sometimes really helps :-)

That key derivation first irritated me, but after reading the reasoning behind it, it totally makes sense.
This also means it's of no use to file a feature request, as API keys simply cannot be changed.

It also rules out situations where people might probably change API keys on the Meilisearch server but forget to keep it in sync with Drupal, which could be considered an advantage.

All of this might be too theoretical anyway. I think the way you did it right now makes for 99% of the setups. At that's what this issue was all about at first. To remove the master key from the config and have a convenient way of managing the keys from Drupal itself so one can have a secure setup.
This seems fulfilled to me.

Let's wait for other use cases if there are any.

I could think of one, btw :-)

This is an actual project that might see the light of day next year probably. I'd want to have one central Drupal instance to feed an index and have multiple web sites to be able to search through subsets of this index (based on given filter criteria).
It would mean one default setup with the routines you laid out to initially create the index on the central instance. Nothing fancy yet.
On the other instances, I would manually enter prefixes and keys, as I would very likely want them to not be able to delete the index, add or delete documents, etc. One could probably prepare for such a scenario by adding an additional checkbox "read-only access", which would create the API key with a reduced set of permissions.
I am still not sure how to get the actual content from the central Drupal site, then, if search_api can handle non-local content for indexed data at all and if this is a good idea. Or if this module needs to be the one that indexed the data itself to be able to use it again (maybe not anymore since the fix in 📌 Remove entity id mapping config Fixed ?)
But this would probably be one application for another kind of key generation.

I'll set the issue to needs review. It would be nice to test in different scenarios.

I'm not really sure what to make of this feature as it is now. I think this goes way above what the base module should provide as I explained in #2. If this is moved to a submodule as perhaps a separate backend provider (so user can choose standard connector or advanced connector with keys management, auto generate keys for specific indexes etc.) then I'm ok with it. But in the current form of this PR I do not want to merge it. For this reason I'm setting the status to back to "Needs work".

What I do agree here is that current connector should not use master key, but API and Search key fields should be configurable and used instead. But this is a breaking change as current installations would need to update configuration manually and a separate issue should be opened on how to best deal with this.

I'm OK with a separate module, though I feel this should be the standard (secure) way of setting things up (and installed by default, then?). Maybe adding a note on the module's front page that using the submodule providing key management is strongly recommended for those not seasoned with manual key/index setups can help.

You mentioned

API and Search key

.
Haven't thought of search keys at all until now. The concept sounds good - a search client (Drupal frontend) only gets the least privilege, i.e. only permissions for searching. However, as the backend lives on the same installation, it is kind of limited here. When ingesting/indexing content is done on one machine and searching only on another, that absolutely makes sense. But yeah, why not, it doesn't hurt. And in case there's some vulnerability in the search code only, the index's contents are not at risk at once.

It should not be too hard to solve the backward compatibility I think: Let's take this over to ✨ Change master key to API and search key for improved security Active

If this issue at hand is available as a submodule, I think I can offer testing. It's just that I am currently stuck on 1.x, as the upgrade to 2.x breaks my facetted searches, even when I add the new facetting submodule. No errors whatsoever, I just don't get any results, but that's a different story. Just so you know this might be delayed quite a bit.

If this issue at hand is available as a submodule, I think I can offer testing. It's just that I am currently stuck on 1.x, as the upgrade to 2.x breaks my facetted searches, even when I add the new facetting submodule. No errors whatsoever, I just don't get any results, but that's a different story.

Feel free to open an issue with as much info as possible, there might be some hidden issues we are not aware of.