Open AliOsm opened 1 year ago
Hi @AliOsm
I understand your issue, and it is a good addition to the gem since using pundit, cancancan, and other gems are common in the rails world. I wonder about making it simpler to integrate with any kind of authorization gem out there, and this would be my first requirement to allow that to be merged in this repo.
Also, my second concern is regarding the overhead a ms_search
would have after an implementation like this.
In fact, to do such an addition, I want both concerns to be assigned.
Now about your last sentence:
The approach mentioned above requires the index to have the fields used to authorize the users indexed inside it. Could we do it without indexing these fields?
No, you should index the fields and add them to your filterable_attributes
(the rails gem has a particular way to do it for you. Check the README ;)).
But you do have an alternative which is the tenant tokens, where based on the authentication JWT from the client, you can automatically apply a search_rule (eg. filter) so the results are automatically validated. Check this link for more info.
Thanks @brunoocasali for your reply, I understand your concerns. First of all, I'm very impressed by the tenant tokens solution, but again it suffers from the same CanCan conditions to MeiliSearch filter string conversion problem, right?
Based on this I propose to:
convert
, which accepts "authorization gem" conditions object, this object can be of any type, and returns MeiliSearch filter stringms_search
method and allow users to enable it based on an argument they pass to the method (authorized_search
or something like this) OR implement a new ms_authorized_search
methodIn this way, anyone can extend the component to support new gems, and we are not forcing/coupling users to use the authorized search.
Some open questions:
Speaker
, Playlist
, and Medium
), while the current ms_search
call happens on the Medium
model, should we assume that the authorization should happen based on the Medium
model conditions only?This is what I can think of currently, please let me know about what you think.
I'm interested in implementing something like this, but I will need some guidance as I'm not the Rails ninja that can do magic out of the box :)
One more thing, based on the proposed solution, we leave the responsibility of adding the fields used in the ability conditions to the index on the user. I don't like this, but I can't think of a simple way to automatically check and warn the user about them.
Before getting into details of your proposition, let me better understand the first part:
but again it suffers from the same CanCan conditions to MeiliSearch filter string conversion problem, right?
When you are using the tenant tokens, the search result will be already filtered for you, so you don't need to add anything in search time. It means all the records in your ms_search
result are going to be filtered.
In other words, the search_rules
were already provided by you when you created the tenant token (JWT), and now you just need to ensure your client is using that token.
uid = '85c3c2f9-bdd6-41f1-abd8-11fcf80e0f76'
api_key = 'B5KdX2MY2jV6EXfUs6scSfmC...'
expires_at = Time.new(2025, 12, 20).utc
search_rules = {
'my_index' => {
'filter' => 'hidden = true'
}
}
token = client.generate_tenant_token(uid, search_rules, api_key: api_key, expires_at: expires_at)
MeiliSearch::Rails.configuration = {
meilisearch_url: 'http://localhost:7700',
meilisearch_api_key: token,
}
Model.search('bar') # all the results are `hidden = true` automatically.
As you can see above, the search is filtered without handling it in memory (ruby side). Meilisearch took care of that for you.
But as you also can see, if you want to use the tenant tokens, you'll have to change the MeiliSearch::Rails.configuration
every time you need to call using a different key (which will cause concurrency issues) due to the global configuration.
But unfortunately, we don't provide a good DX with tenant tokens in this gem (but I'm open to contributions).
So, right now, without deep thinking, your use case could be handled by Meili using the tenants, but it would require an introduction of proper token handling in this repo.
Even with tenants, the issue is how to convert the ability conditions to tenant search rules? This is the part that needs to be solved.
Again, if I have an ability with conditions on some model like {hidden: false}
, how to tell the tenant to use these conditions (e.g. From CanCan) and convert them to Meili filter string automatically when I call ms_search
?
Ok, I got it!
I think this method could live inside meilisearch-rails or even meilisearch-ruby as some utility method. But I prefer to leave it as is (it would be an arbitrary method in the public API of the gem with a clear purpose). Well, at least I can find more "no-go" reasons than otherwise. You may have a different opinion. Let me know.
Knowing this, I would like to know if adding an authorization approach like the one you suggest is the best. Or we can redirect the efforts to make it easier to do this: https://github.com/meilisearch/meilisearch-rails/issues/152, which also could solve your use case.
From my side, I see them as two different features and they can't replace each other. Having easy to use tenant tokens will not solve the mentioned issue for authorization, and the authorization solution will not remove the need for the tenant tokens.
I already implemented the authorization solution in my project, if you are welling to help, I can start a PR to integrate it with MeiliSearch either in Ruby or Rails gems.
@AliOsm, I'm very curious to see the implementation, to be honest. If you want to do it, I will review it, and then we can discuss it. But I need to be transparent that I want to help you do it, and also, in case I don't see more significant value for other users, I reserve the right not to merge it.
Answering your previous questions:
- If the user doing authorized search on an index, should we extract the ability conditions for this index based on the associated models only? For example, if CanCan ability have conditions on 3 models (Speaker, Playlist, and Medium), while the current ms_search call happens on the Medium model, should we assume that the authorization should happen based on the Medium model conditions only?
Yes, because in Meilisearch, you can't use two different indexes simultaneously when searching; it is unlike SQL JOIN. Btw, should we have to raise an error to prevent misunderstandings?
- Based on the previous point, when doing authorized search on a custom index that is handling records from different models as mentioned here, should we apply conditions from both indexes if they appear in the ability conditions?
In this case, it will work because the ruby model attributes will be indexed on the Meilisearch side so you can filter them.
- Based on the previous point, should we allow the user to skip conditions of one of the models in this case?
What do you mean about skip conditions?
Looking forward to hearing from you!
First of all, thank you for the gem and for being open minded about extending the gem to work with other gems. I would be interested to see that feature implemented too. As you mentioned, authorization is common inside the rails world. I do think that doing the pundit/cancancan authorization call first then search with meilisearch filters based on authorizated ids is reducing the perf drastically. Tenant is nice but I would only use it with apps targeting a small range of users for the reasons you mentioned, not with a b2c store with user preferences or account status (gold, silver, ...) for example. From experience, race condition is pretty complex to debug also.
Using pundit (and pagy), that's the easy solution we have in place currently.
def index
authorize Product
options =
{}.tap { |options|
options[:sort] = ["release_at:desc"] if params[:q].to_s.blank?
options[:filter] = ["id IN #{ProductPolicy::Scope.new(current_user, Product).resolve.pluck(:id)}"]
}
q = Product.includes(:category_secondary, :gender).references(:category_secondary, :gender).pagy_search(params[:q], **options)
@pagy, @products = pagy_meilisearch(q)
end
@Menelle I'm using something similar as you can see in my comments above. I added a value in the index itself indicates the ability of the current user to show this record (e.g. hidden: true
) and as you are doing, building a filter string based on the current user CanCan ability. This approach is working, but requires more steps from the developer (me). I would love to have it implemented in the gem itself. I didn't have time to implement that and raise the PR unfortunately :)
Description
When I have a CanCan ability, I need to be able to rely on the ability to filter the MeiliSearch search results.
Basic example
Let's say that I have a
Cue
model, where it has acontent:text
and ahidden:bool
attribute.The guest users can search the non-hidden cues only, while the signed-in admin users can search both non-hidden and hidden cues.
To implement such simple authorization logic, we can use CanCanCan gem with the following ability:
To retrieve the record accessible by guest users, we can do the following:
The issue happens when we try to do
ms_search
on the model. Thems_search
method is un-aware about the authorization logic both in terms of the filtration fields and which fields to use with which user.To work around this issue, I extracted the CanCan ability conditions like this:
Then I converted them to MeiliSearch filter string like this:
The implementation is not perfect, but it fulfills my requirements for now.
I hope that this functionality is implemented directly inside
ms_search
method.Other
The approach mentioned above requires the index to have the fields used to authorize the users indexed inside it, could we do it without indexing these fields?