Skip to content

Force scope on default filters collection#7511

Open
deivid-rodriguez wants to merge 10 commits into
masterfrom
6883-Scope-default-filter-collections
Open

Force scope on default filters collection#7511
deivid-rodriguez wants to merge 10 commits into
masterfrom
6883-Scope-default-filter-collections

Conversation

@deivid-rodriguez

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
@codecov

codecov Bot commented Aug 1, 2024

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 99.11%. Comparing base (7117d59) to head (282f4f9).
⚠️ Report is 224 commits behind head on master.

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.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@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
Copy Markdown
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.

@javierjulio javierjulio left a comment

Copy link
Copy Markdown
Member

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.

Comment thread lib/active_admin/error.rb Outdated
end
end

class ScopeNotDefined < StandardError

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

3 participants