-
Notifications
You must be signed in to change notification settings - Fork 47
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
CanCan aware ms_search implementation #255
Comments
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. In fact, to do such an addition, I want both concerns to be assigned. Now about your last sentence:
No, you should index the fields and add them to your |
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:
In 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:
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:
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 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. 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 |
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: #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:
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?
In this case, it will work because the ruby model attributes will be indexed on the Meilisearch side so you can filter them.
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. Using pundit (and pagy), that's the easy solution we have in place currently.
|
@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. |
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?
The text was updated successfully, but these errors were encountered: