Improve the error handling of the tenant token requests

[brunoocasali]

Following this comment: https://github.com/meilisearch/specifications/pull/89/files#r810014680

From: meilisearch/integration-guides#181 (comment)

During our internal @meilisearch/integration-team meeting, we talked about what we should do regarding the searchRules, since there are a lot of combinations we may not be able to check/validate the content in every aspect this is something it would be awesome to have but doesn't seem to be a responsibility of the SDKs.

So, if we could allow the users to create tokens with a wrong filter DSL, eg: "filter": "user_id ==+1== 1", "filter": "blabla<|>1" (you got the idea).
Because this problem will not be felt during the execution of the generateTenantToken method, but only after the user calls a search method with the generated token.
So, they could be misbehaved when they query the index using the searchRules filter.

Suppose some developer create a key with this filter: "filter": "user_id == 123".
And the token was generated successfully, and now ANOTHER dev will call the index search (using a GET without body).

{
    "message": "Attribute `user_id` is not filterable. Available filterable attributes are: ``.\n1:6 user_id = 123",
    "code": "invalid_filter",
    "type": "invalid_request",
    "link": "https://docs.meilisearch.com/errors#invalid_filter"
}

In this initial response, the dev could ask themself, WTF? I didn't put that here, my request is empty!
The message could be clearer to indicate this filter was injected by the tenant token.

{
    "message": "Tenant token searchRules used this attribute `user_id` but it is not filterable. Available filterable attributes are: ``.\n1:6 user_id = 123",
    "code": "invalid_filter_by_token",
    "source": "tenant_token",  // and when this come from the query we can use just "search_query" 
    "type": "invalid_request",
    "link": "https://docs.meilisearch.com/errors#invalid_filter_by_tokens"
}

And the same could happen when the attribute is filterable but there is a mistake in the searchRules:

"searchRules": {
     "*": {
       "filter": "title == 1"
    }
  }

The search query with the generated tenant token:

POST localhost:7700/indexes/Task/search, body: {"filter": "title = 123" }

{
    "message": "Was expecting a value but instead got nothing.\n8:8 title == 1",
    "code": "invalid_filter",
    "type": "invalid_request",
    "link": "https://docs.meilisearch.com/errors#invalid_filter"
}

The developer could have a hard time understanding what's going on here.

FYI: @curquiza

---

The idea of this discussion is to keep track of this possible problem that could happen in the future (after the v0.26 release).

Some ideas to handle the problem:

• Add new information in the error message specifying that the problem happened because of the injected tenant token.
• Add more information in the error message talking about the possibility that could be an error generated from the tenant token.
• Add more information in the docs section about the possibility that could be an error generated from the tenant token.
• Make a request to /search every time the user generates a new token.
  • This could be helpful, but we will be connecting one feature to other and taking risks when the /search changes.
• Using the same idea from the last item, one approach is to: core exposes the searchRules parser into the API (/verify-search-rules or something), and we use it to validate the generated token before returning it to the user.

Above we have some ideas to handle the problem, but we definitely can wait for the feedback regarding the usage of the feature in the following weeks after the release.

[alallema]

I think this is really an issue that we need to think about it and try to handle better. Because when implementing the token creation functions it was very difficult to understand where the problem was coming from when an error was returned. Mainly because the message suggests that it is the encryption that went wrong but there is no clue if the problem is in the payload or in a field of the payload or in the encryption.
And I think that's a problem that users will probably face as well see

[gmourier]

Thanks, @alallema and @brunoocasali!

To improve this aspect, I don't see this proposal but would it be possible to validate in the SDK methods that these conditions are met?

• The field searchRules has been specified.
• The field apiKeyPrefix has been specified and it is valid (8 chars length).
• The field exp is not mandatory but can be type-checked when specified.

There is still the possibility of an error if an operator is not compliant (I'm thinking in particular of filter. At this time, Meilisearch can't know if the tenant token body filter is invalid or if it's coming from the search request filter parameter. This is too "deep" in the core codebase when it happens) but this allows to eliminate some errors on the user side without having to make a particular exchange with Meilisearch.

And I think that's a problem that users will probably face as well meilisearch/meilisearch-python#412 (comment)

It seems to me that this specific error could have been resolved by checking that apiKeyPrefix was filled by the SDK helper method.

I'm not super alarmed by this at the moment. Here is why.

• I think the documentation will be a great help on this particular part. I expect that users will come across this feature after searching the documentation to solve their multi-tenancy needs and will follow the guide.
• I didn't find many times this topic on Algolia forums for example while they do strictly no SDK / server-side validation and don't give any additional information at the time of an erroneous search that is related to a secured API key body (This is their tenant token feature). More generally, Algolia doesn't even raise an error when a field in filters is not configured as such. I've done some tests. It's not a reason to not go further and have a better product, I'm well aware of that. 🤓

[brunoocasali]

Hi, @gmourier all of these ideas:

• The field searchRules has been specified.
• The field apiKeyPrefix has been specified and it is valid (8 chars length).
• The field exp is not mandatory but can be type-checked when specified.

Will be implemented in the SDKs :)

And regarding the possibility of the user sending an exp value already expired.

About the idea of checking the operators, I think that falls into the same problem we have already, have a deep validation that could be always outdated regarding the core validations. So I think is good to keep the core as the source of truth in this particular situation.
As you said, the docs could help us add a good explanation regarding the error handling.

[gmourier]

During a discussion with @ManyTheFish, we quickly evaluated what could be done in the Meilisearch API to ensure a better validation and help the user not generate invalid tenant tokens for a future iteration.

We could have an endpoint to generate a tenant token like POST /tenant-tokens or something like that.

It could allow verifying and returning a dedicated error message when:

• signingApiKey is missing.
• signingApiKey provided is the master-key.
• signingApiKey does not have the search action.
• searchRules is missing.
• searchRules payload is invalid because it tries to overrides the rights of the signing API key.
• filter syntax is invalid.
• exp field is missing.
• exp field is invalid.

It won't:

• Check that a valid filter syntax contains setted filterableAttributes.
• Store the generated tenant token.

Changes vs SDK method:

The http payload should require the full signing API Key key field value and not just the 8 first chars.

Further questions:

• Is this endpoint a new API Key action ?
• If not, is it only accessible to the master-key?
• Does it need to be secured at all?
• Could it just be a JWT validation endpoint?

[brunoocasali]

I'm totally in favor of this upgrade into this feature, doing this from the core side it will improve drastically the DX of this feature.

Regarding the further questions:

Is this endpoint a new API Key action?

In my opinion yes, tenant_tokens.create, tokens.create or something similar, since this is a good way for the user to have a better control regarding who has the rights to do that or not.

If not, is it only accessible to the master-key?

Since the idea is to not allow tokens being generated by the masterKey, I think we could have special handling when the user requests a new token with the masterKey, maybe a special error?.

Does it need to be secured at all?

I'm not sure if I understood the point, but if it is not secured mean that anyone can create tokens freely?

Could it just be a JWT validation endpoint?

If in the SDK's we just create the token and go to the core to validate them, it will not be the best case, because on the SDK's side you will still have all the code regarding creating a new JWT (duplicated in all the SDK's). So, in my opinion, it does not have a good value => but yes it is better than if we just don't have validations to that cases at all.

[gmourier]

Related to https://github.com/meilisearch/product/discussions/415

[bidoubiwa]

My bad I did not saw it. Would you like me to close it and put my opinion here?

[gmourier]

Yes! That would be great! Thanks!

[bidoubiwa]

For the moment, when a token is badly created the error thrown is Invalid API key. Which doesn't say a lot about the issue and makes debugging very hard.

After looking at the code with @irevoire we realised that some checks could be easily implemented (I don't know about the workloud in the spec's though!)

These are the errors that can be returned:

• Unsupported algorithm
• Missing searchRules field
• Wrong format for searchRules field
• Missing apiPrefix field
• apiPrefix field should be 8 characters long
• Signature is invalid
• Token expired

It would help a lot in creating the tokens when using the jwt libraries for example.

[derekperkins]

This is very frustrating to deal with. It also returns the same error for:

• requests to indexes that the key doesn't have access to, rather than a 403
• actions not supported by tenant tokens, like documents.get, rather than 400 or 405 for unsupported action