Skip to content
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

Open
AliOsm opened this issue Apr 30, 2023 · 10 comments
Open

CanCan aware ms_search implementation #255

AliOsm opened this issue Apr 30, 2023 · 10 comments
Labels
question Further information is requested stale Pull request or issue that has recieve no activity for a long time.

Comments

@AliOsm
Copy link

AliOsm commented Apr 30, 2023

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 a content:text and a hidden: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:

class Ability
  include CanCan::Ability

  def initialize(user, controller_namespace)
    can :read, Cue, hidden: false

    return if user.blank?

    can :manage, :all if user.admin?
  end
end

To retrieve the record accessible by guest users, we can do the following:

Cue.accessible_by(current_ability)

The issue happens when we try to do ms_search on the model. The ms_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:

cue_conditions = current_ability.model_adapter(Cue, :show).conditions

Then I converted them to MeiliSearch filter string like this:

def ability_conditions_to_meilisearch_filter(condition)
  condition.map do |key, value|
    case value
    when Array then "#{key} IN [#{value.join(',')}]"
    when NilClass then "#{key} NOT EXISTS"
    when TrueClass, FalseClass then "#{key} = #{value}"
    when Hash then hash_condition_to_filter(key, value)
    else "#{key} = '#{value}'"
    end
  end.join(' AND ')
end

def hash_condition_to_filter(key, value)
  case value.keys[0]
  when :gt then "#{key} > #{value[:gt]}"
  when :gte then "#{key} >= #{value[:gte]}"
  when :lt then "#{key} < #{value[:lt]}"
  when :lte then "#{key} <= #{value[:lte]}"
  when :to then "#{key} #{value[:to].first} TO #{key} #{value[:to].last}"
  end
end

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?

@brunoocasali
Copy link
Member

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.

@brunoocasali brunoocasali added the question Further information is requested label May 2, 2023
@AliOsm
Copy link
Author

AliOsm commented May 3, 2023

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:

  • Create a generic adapter interface that has one public method convert, which accepts "authorization gem" conditions object, this object can be of any type, and returns MeiliSearch filter string
  • Implement this adapter for CanCan & Pundit gems (I can handle CanCan, but I don't have any experience with Pundit)
  • Inject this interface inside ms_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 method

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:

  • 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?
  • 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?
  • Based on the previous point, should we allow the user to skip conditions of one of the models in this case?

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 :)

@AliOsm
Copy link
Author

AliOsm commented May 3, 2023

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.

@brunoocasali
Copy link
Member

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.

@AliOsm
Copy link
Author

AliOsm commented May 3, 2023

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?

@brunoocasali
Copy link
Member

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.

@AliOsm
Copy link
Author

AliOsm commented May 4, 2023

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.

@brunoocasali
Copy link
Member

@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!

@brunoocasali brunoocasali added the stale Pull request or issue that has recieve no activity for a long time. label Aug 21, 2023
@Menelle
Copy link

Menelle commented Aug 27, 2023

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

@AliOsm
Copy link
Author

AliOsm commented Aug 27, 2023

@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 :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
question Further information is requested stale Pull request or issue that has recieve no activity for a long time.
Projects
None yet
Development

No branches or pull requests

3 participants