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

feature: add search_result_path and search? authorization method #1634

Merged
merged 5 commits into from
Mar 21, 2023
Merged

feature: add search_result_path and search? authorization method #1634

merged 5 commits into from
Mar 21, 2023

Conversation

glaucocustodio
Copy link
Contributor

Add two new class attributes to Avo::BaseResource to further customize the search behaviour:

  • search_policy_class: responsible for setting the policy used to authorize the search on the applied resource
  • search_result_url: responsible for setting the url to be used in the search results

Context

An Avo app contains the following classes:

  • ReportResource (used only by admin users to manage reports)
  • Avo::Customers::ReportsController (used by customer users to view reports)
  • ReportPolicy (allowing only admin users to manage ReportResource)
  • Customers::ReportPolicy (allowing customer users to view reports from the controller aforementioned)

When a customer searches for a report on the global search, Avo 2.28 will use ReportPolicy to authorize the search and it will deny since a customer can't manage ReportResource.

In order to fix that, I am introducing search_policy_class. This class attribute will allow people to set which policy class must be used to authorize seaches:

class ReportResource < Avo::BaseResource
  self.title = :title
  self.includes = []
  self.search_policy_class = Customers::ReportPolicy
end

Once the authorization is fixed with the search_policy_class above, another problem is found: Avo 2.28 will point to /avo/resources/reports/:report_id as url in the search results. Remember: customers can't manage reports, so, search results should point to /avo/customers/reports/:report_id instead, that's why search_result_url has been introduced:

class ReportResource < Avo::BaseResource
  self.title = :title
  self.includes = []
  self.search_policy_class = Customers::ReportPolicy
  self.search_result_url = ->(report, main_app, params) do
    if params[:global] == "true"
      main_app.customer_report_path(report)
    end
  end
end

In the example above, if the search is global (performed possibly by a customer), search results will point to customer_report_path, otherwise, if it's performed on the ReportResource index page (only by an admin) it will point to /avo/resources/reports/:report_id (sames as in Avo 2.28).

Checklist:

  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation -> will open a PR for that
  • I have added tests that prove my fix is effective or that my feature works

Manual review steps

  1. Create a resource class, eg: ReportResource
  2. Create a "tool" controller to read these resources
  3. Create two policy classes: ReportPolicy to authorize ReportResource and Customers::ReportPolicy to authorize the tool controller
  4. Perform a global search with an user that can only read the tool controller (not the ReportResource)

@codeclimate
Copy link

codeclimate bot commented Mar 18, 2023

Code Climate has analyzed commit 28d85ba and detected 0 issues on this pull request.

View more on Code Climate.

@@ -120,12 +121,18 @@ def apply_search_metadata(models, avo_resource)
models.map do |model|
resource = avo_resource.dup.hydrate(model: model).hydrate_fields(model: model)

if resource.search_result_url
url = resource.search_result_url.call(model, main_app, params)
Copy link
Contributor Author

@glaucocustodio glaucocustodio Mar 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had to pass main_app to be able to use url helpers in the lambda.
I had to pass params to be able to check if the search is global or not.
Is there a better way of doing that?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the Evaluation Host we will be able to delegate main_app to the view_context.
See this.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed to use the search host, but I think we still need to pass model since the evaluation host does not seem to have the resource model instance. What do you think?


write_in_search "São Paulo"
expect(page).to have_content "São Paulo"
# expect(page).to have_link(href: "/any/custom/#{city.id}")
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I couldn't get that commented assert passing, I've seen this url in the search response but for somehow I can't see it in the page html during test

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to regex it from the page.

Suggested change
# expect(page).to have_link(href: "/any/custom/#{city.id}")
# expect(page).to have_link(href: /\/any/custom/#{city.id}/)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The regex doesn't work also.

During the test, if I print url I can see /any/custom/1, but not in page.body, that's why the assert fail 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should I remove this commented line? Do you have any other suggestion on how to assert that?

Copy link
Collaborator

@adrianthedev adrianthedev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this @glaucocustodio! This is a good feature indeed!

Regarding search_policy_class; Instead of having another policy to work with that only has one method index?, how about we use the policy of that resource (which you can customize using authorization_policy) and add another method search? (which once can customize it using different-policy-methods).
This would DRY up the code IMHO.

The search_result_url is 👨‍🍳🤌 (chef's kiss). One thing I'd do is use an Evaluation host. That will allow us not to pass in variables to that block and for better future-proofing.
Let me know if I can help with that.

@@ -120,12 +121,18 @@ def apply_search_metadata(models, avo_resource)
models.map do |model|
resource = avo_resource.dup.hydrate(model: model).hydrate_fields(model: model)

if resource.search_result_url
url = resource.search_result_url.call(model, main_app, params)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we use the Evaluation Host we will be able to delegate main_app to the view_context.
See this.


write_in_search "São Paulo"
expect(page).to have_content "São Paulo"
# expect(page).to have_link(href: "/any/custom/#{city.id}")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You might need to regex it from the page.

Suggested change
# expect(page).to have_link(href: "/any/custom/#{city.id}")
# expect(page).to have_link(href: /\/any/custom/#{city.id}/)

@glaucocustodio
Copy link
Contributor Author

glaucocustodio commented Mar 19, 2023

Thanks for working on this @glaucocustodio! This is a good feature indeed!

Regarding search_policy_class; Instead of having another policy to work with that only has one method index?, how about we use the policy of that resource (which you can customize using authorization_policy) and add another method search? (which once can customize it using different-policy-methods). This would DRY up the code IMHO.

The search_result_url is 👨‍🍳🤌 (chef's kiss). One thing I'd do is use an Evaluation host. That will allow us not to pass in variables to that block and for better future-proofing. Let me know if I can help with that.

Sorry, I didn't give you the full context, I have the following policies:

class AdminPolicy < ApplicationPolicy
  def index?
    allowed?
  end

  def show?
    allowed?
  end

  def create?
    allowed?
  end

  def update?
    allowed?
  end

  def destroy?
    allowed?
  end

  def upload_attachments?
    allowed?
  end

  def download_attachments?
    allowed?
  end

  def delete_attachments?
    allowed?
  end

  class Scope < Scope
    def resolve
      scope.all
    end
  end

  private

  def allowed?
    user.admin?
  end
end

# used by the Resource
class ReportPolicy < AdminPolicy
end

# used by the tool controller
module Customers
  class ReportPolicy < ApplicationPolicy
    def index?
      true
    end

    def show?
      true
    end
  end
end

Customers::ReportPolicy is used to authorize two tool controller actions: index and show.

I would rather managing customer policies in Customers::ReportPolicy (including search which uses index?) and keep ReportPolicy for managing admin policies related to the resource CRUD, that's why I think would be nice having search_policy_class.

@adrianthedev
Copy link
Collaborator

I understand your usage. I just think it would fall more in-line with what we currently have which is all things related to the authorization of a resource (CRUD, associations, attachments) are controlled from one policy.
For example, for CityResource you have CityPolicy.
So adding search there makes sense IMHO.
You could still implement that allowed? pattern you used showed me with

def search?
  allowed?
end

Otherwise, everyone will have to add a new policy for search with just one method index?.

So it would be something like

class AdminPolicy < ApplicationPolicy
  def index?
    allowed?
  end

  def show?
    allowed?
  end

  def create?
    allowed?
  end

  ...


  def search?
    allowed?
  end
end

Does that make sense?

Regarding the evaluation host, apologies, I didn't provide an example for that. I'll push some code to show what I mean.

@adrianthedev
Copy link
Collaborator

Check out my latest commit with the ResourceRecordHost implemented.
Now we can have a block without parameters for the search_result_path.

  self.search_result_path = -> do
    # you have access to resource, record, view_context and more from ResourceRecordHost
    avo.resources_city_path record, custom: "yup"
  end

@glaucocustodio
Copy link
Contributor Author

glaucocustodio commented Mar 19, 2023

I understand your usage. I just think it would fall more in-line with what we currently have which is all things related to the authorization of a resource (CRUD, associations, attachments) are controlled from one policy. For example, for CityResource you have CityPolicy. So adding search there makes sense IMHO. You could still implement that allowed? pattern you used showed me with

def search?
  allowed?
end

Otherwise, everyone will have to add a new policy for search with just one method index?.

So it would be something like

class AdminPolicy < ApplicationPolicy
  def index?
    allowed?
  end

  def show?
    allowed?
  end

  def create?
    allowed?
  end

  ...


  def search?
    allowed?
  end
end

Does that make sense?

Regarding the evaluation host, apologies, I didn't provide an example for that. I'll push some code to show what I mean.

How would Avo look for search? instead of index? when authorising a search? if I override index like below would mess with other policies no?!

config.authorization_methods = {
    index: 'search?',
}

@adrianthedev
Copy link
Collaborator

It would be just search? by default.

config.authorization_methods = {
    search: 'search?',
}

So instead of thinking of search as something like the index method/action, it has it's own search? method where you can control only the search.

@glaucocustodio
Copy link
Contributor Author

search: 'search?',

But for that we need to replace next unless @authorization.set_record(resource.model_class).authorize_action(:index, raise_exception: false) by next unless @authorization.set_record(resource.model_class).authorize_action(:search, raise_exception: false) in app/controllers/avo/search_controller.rb, right?!

@adrianthedev
Copy link
Collaborator

Yes, correct.
And probably do something like this before so we don't send :search but the one configured in by the user.

# fetch the configured name or use the default :search
action = Avo.configuration.authorization_methods.stringify_keys['search'] || :search

next unless @authorization.set_record(resource.model_class).authorize_action(action, raise_exception: false)

@glaucocustodio
Copy link
Contributor Author

glaucocustodio commented Mar 19, 2023

We don't need this bit:

config.authorization_methods = {
    search: 'search?',
}

It will work out of the box now, thanks for the suggestion 🙃

Please let me know if you wanna change something else.

@adrianthedev
Copy link
Collaborator

@glaucocustodio I'll have a look and merge it tomorrow. Today was swamped for me.

@glaucocustodio
Copy link
Contributor Author

Today was swamped for me.

No worries, hopefully the tests will pass next time

@adrianthedev
Copy link
Collaborator

This looks good!
I pushed some code to hide the search box from the individual index pages too when un-authorized.

CleanShot 2023-03-21 at 13 27 46@2x

@adrianthedev
Copy link
Collaborator

Thank you for working on this @glaucocustodio!

You championed this feature from 0 to 1! Good job!

@adrianthedev adrianthedev merged commit c04ed9b into avo-hq:main Mar 21, 2023
@glaucocustodio
Copy link
Contributor Author

glaucocustodio commented Mar 21, 2023

Thank you for guiding me to the best solution @adrianthedev, I am proud to contribute back to Avo 💙

@glaucocustodio glaucocustodio changed the title feature: add search_result_url and search_policy_class feature: add search_result_path and search? authorization method Mar 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants