Skip to content

Force scope on default filters collection #7511

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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

deivid-rodriguez
Copy link
Member

In-repo clone of #6923.

When we use default filters, or filter of an existing relationship without providing a collection, we enforce the collection to be scope
It is useful when you are using authorization scope to define what resources your current user can access, even in filter values.

Fixes #6883.

@javierjulio
Copy link
Member

The tests are failing. Likely need some updates since this was last looked at.

@mgrunberg mgrunberg force-pushed the 6883-Scope-default-filter-collections branch 3 times, most recently from 82e1817 to 26e87ed Compare August 1, 2024 02:26
@mgrunberg
Copy link
Contributor

@javierjulio This is now a breaking change. scoped_collection is not raising PunditError anymore.

@mgrunberg mgrunberg requested a review from javierjulio August 1, 2024 02:37
Copy link

codecov bot commented Aug 1, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 99.11%. Comparing base (7117d59) to head (282f4f9).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #7511   +/-   ##
=======================================
  Coverage   99.11%   99.11%           
=======================================
  Files         141      141           
  Lines        4069     4087   +18     
=======================================
+ Hits         4033     4051   +18     
  Misses         36       36           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mgrunberg mgrunberg force-pushed the 6883-Scope-default-filter-collections branch 8 times, most recently from a04b6ba to e8be50f Compare August 1, 2024 04:17
this should work even if default policy is not defined or does not
define a default scope
@mgrunberg mgrunberg force-pushed the 6883-Scope-default-filter-collections branch from e8be50f to 0ffac23 Compare August 1, 2024 14:42
@mgrunberg
Copy link
Contributor

@javierjulio took me some time to build a proper spec. Now is ready for review.
For the record, tests were failing because the code assumes that we can always get a scoped collection. But the template app define a default scope nor a policy for Tagging class (which is fine). That makes scoped_collection to raise an error.
I didn't want to investigate why raising an error is the expected result. In the context of building filters, I find it good enough to fallback to "all" scope if this happens.

Copy link
Member

@javierjulio javierjulio left a comment

Choose a reason for hiding this comment

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

What is the benefit of adding this? If I understand right it only affects default filters, so if you set filters explicitly which is best, this would not apply. Or I guess it would since there is an update to add_filter method. I'm not convinced on this change.

@@ -19,6 +19,16 @@ def message
end
end

class ScopeNotDefined < StandardError
Copy link
Member

Choose a reason for hiding this comment

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

I'd like to include "Policy" in the error class name so it's clear since when I think "scopes" it's something else. Perhaps "PolicyScopeNotDefined" as the class name?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand your concern with the name. I'm open to use another one. I just want to avoid Policy because it's a Pundit strong word. I want to model something more abstract, the ActiveRecord::RecordNotFoundError for scope_collection method. I don't want to tailor the error for a specific AuthorizationAdapter implementation.

I renamed the class to ScopeAuthorizationError. Is it better?

@mgrunberg
Copy link
Contributor

What is the benefit of adding this? If I understand right it only affects default filters, so if you set filters explicitly which is best, this would not apply. Or I guess it would since there is an update to add_filter method. I'm not convinced on this change.

@javierjulio The change affects filters and default filters. I added more test scenarios to demonstrate the usage. I understand the previous commit was misleading.
If you use an implicit filter collection, we make sure it is accessible by the user. That's the main idea.

@mgrunberg mgrunberg requested a review from javierjulio August 5, 2024 00:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Filters values aren't using authorization scopes
4 participants