meilisearch / meilisearch-rails

Meilisearch integration for Ruby on Rails
https://www.meilisearch.com
MIT License
295 stars 48 forks source link

Support for tenant tokens #152

Open archonic opened 2 years ago

archonic commented 2 years ago

Description v0.26.0 introduced tenant tokens but it looks like there aren't any opinions on tenant tokens in this gem.

Basic example Specifics of a meilisearch tenant token integration are going to depend on how tenancy is handled but it would be nice to see examples that support acts_as_tenant and perhaps Apartment. It should be easy to figure out how to integrate with Apartment by looking at examples for acts_at_tenant.

I'm wondering how the client should be initiated when searching a tenanted resource. I have this index action for example:

def index
  @pagy, @documents = pagy_meilisearch(
    Document.includes(Document.search_includes).pagy_search(
      @search_terms,
      **index_search_options
    )
  )
end

In order to use a tenant token, would I have to initiate the client myself? Like this:

client = MeiliSearch::Client.new(ENV['MEILISEARCH_HOST'], current_account.ms_tenant_token)
Document.ms_search(...)

I'm also wondering about best practices for token refreshes. I currently generate a tenant token when the tenant is created and set the expiry to one year, and plan on creating a tenant key refresh job that runs once per month on keys that expire that month.

brunoocasali commented 2 years ago

Hi @archonic, thanks for your issue!

Yes, currently there is no easy way to change the Meilisearch API key, used to query the resources internally. Because the data used to create the client comes from the configuration defined in the configuration side MeiliSearch::Rails.configuration = {}.

You are able to change this code at runtime but probably may have some problems with concurrency (not thread-safe).

I'm not sure if we will provide an example like you want (meilisearch-rails + apartment) here in the README, because we will have to be aware of apartment changes in order to make our tutorial always up to date. But we definitely should add at least an easy way to change the apikey used to make the searches.

Do you have any idea about: "how the public API should look like"?

(disclaimer, I've never used the apartment gem before, the code below is just an idea for us to discuss)

Apartment::Tenant.switch('tenant_name') do
  MeiliSearch::Rails.change_api_key(tenant_data.current_user.meili_token) do
    # search code?...
  end
end

or

Apartment::Tenant.switch('tenant_name') do
  MeiliSearch::Rails.change_api_key(tenant_data.current_user.meili_token)

  # search code?...
end

About the expiry of your token I personally recommend you to have it as shorter as possible because, in case of a leak, the token will be sooner expired. I know that could be hard to implement but is definitely the best way (you can read more about it).

Anyway, let's discuss more this possible API, and then we can plan a way to make it real!

Thanks for this issue, we're really happy to see you around!

archonic commented 2 years ago

I realized that the attribute I wanted to filter by was not a user provided param. So as long as my app doesn't expose a Meilisearch JWT, I figure it's ok to scope by filter on the Rails side instead of using tenant tokens to scope. Still, I'm wondering how the gem can accommodate tenant tokens - I'd like to eventually have a front-end instant search feature. Especially if Actix can render the search results :metal:.

Apartment is no longer maintained so it may make more sense to have an example for acts_as_tenant or just pseudo code it.

In Apartment, as long and the request object is available and the "elevator" has populated the current_account, you don't need to explicitly switch tenants. Just being on one.example.com would switch into the one tenant for that request. If the request object isn't present then you'd need to explicitly switch into the tenant, such as in a migration, background job, setting up a test, or setting expectations for tenanted data.

Something like MeiliSearch::Rails.change_api_key(current_account.ms_tenant_token) { ... } works for me. I prefer the block to avoid bugs that result from forgetting to switch back. Apartment has Apartment::Tenant.switch which expects a block and Apartment::Tenant.switch! which will stay switched.

This may be an over complication though. Tenant tokens are specifically for scoped front-end searches, right? Perhaps the token should be provided to a search function directly like:

Document.ms_search_tenant(current_account.ms_tenant_token, @search_terms, { filter: "...", sort: [...] } )

That should prevent issues where you forget to switch back to an admin key before the app tries to create a new index, and it's more concise.

Happy to be here and thanks for making such an efficient and easy to use search service!

After thought: I wonder if I've misunderstood the purpose of a tenant token. Maybe this gem doesn't need tenant token integration if they're only designed for front-end implementations where Rails would not be answering requests?

brunoocasali commented 2 years ago

Sorry if I let you alone, we are in the release cycle and all of our efforts are to make all the integrations ready when the v0.28 comes out!

I like the block approach as well, I've implemented something like that to switch databases and it was the perfect solution, I think it will be the same here.

I like the idea of having the ability to provide a different token at search-time, I think both can live together without any problem.

About instant-meilisearch, yes, we advise people to make the search in the front-end environment because it is where Meilisearch stands out! But if you do search in the backend you will still have a good performance and DX (it is not wrong).

But the tenant token is meant to just scope the data in the search time out of the box without having to apply other scripts in the data after the search. You can read more if you want to about the feature here, directly in the specification or here at the docs.

So to answer this question Maybe this gem doesn't need tenant token integration if they're only designed for front-end implementations where Rails would not be answering requests?, I think this gem needs a rails-way to use the tenant-token feature!

archonic commented 2 years ago

I think this gem needs a rails-way to use the tenant-token feature!

I agree. The gem can't assume that someone would have to use a front-end implementation to use that feature. I'm especially looking forward to building a front-end federated tenant search feature.