Comments (12)
we also have another failing test that is related to this change that broke:
Oh, that's bad. Fixed (and added a missing test so I won't break it again π).
Adding #build_authorization_context adds a new action that is unreachable, thus failing CI.
Made private.
Both released in 0.6.3.
from action_policy.
I totally understand that assumption. For my app, sometimes it has intermediary authorization contexts that build up to a final authorization context (i.e. when scoping nested resources), so that assumption doesn't always hold true.
I'm fine using the monkey patch for now. No rush. Just wanted to start a discussion.
p.s. I really dig this gem! I was on my way to implement a similar system on top of Pundit when I stumbed upon Action Policy. Rather than continue adding abstractions on top of Pundit (allow!
, deny!
, authorization contexts, etc.), I decided to rip all that out and migrate to Action Policy. Still lots of work to do on that front, but so far it has helped clean up a lot of technical debt, and has made policies much easier to reason about.
p.p.s. Sponsored the gem on behalf of @keygen-sh. π
from action_policy.
the memoization by authorized_scope is rather unexpected and took awhile to debug. As far as I can tell, this behavior by authorized_scope isn't documented.
All helper methods (authorize!
, allowed_to?
and authorized_scope
) uses policy_for
under the hood, which is responsible for memoization/caching of policies and contexts.
I see several possible solutions to the problem:
- Add an API to reset authorization context:
reset_authorization_context!
- Add an option to use a fresh authorization context without caching/memoization:
authorized_scope(Post.all, isolate_context: true)
- Set context explicitly:
@post = authorized_scope(Post.all).find(params[:post_id])
authorization_context[:post] = @post
I would prefer the latter one. Adding 1) could be also useful. The 2) doesn't look good to me.
from action_policy.
Thanks for the consideration. I feel like a configuration option may be even better.
config.action_policy.memoize_authorization_context = false
from action_policy.
I think, we can proceed with one of the application-level options:
- Updating the context manually:
authorization_context[:post] = @post
. That, IMO, the best way to deal with it, explicit and performant. - Overriding the
authorization_context
method:
class ApplicationController
def authorization_context
{
user: current_user
}
end
end
class Posts::CommentsController < ApplicationController
def authorization_context
super.merge(post: post)
end
end
We already have at least two pure Ruby options, hence adding configuration/API complexity to the library is not necessary.
from action_policy.
Those solutions are fine, and seem to work. I think def authorization_context = super.merge(post:)
is the best option out of those. But I still think it's confusing that authorized_scope
memoizes the authorization context, excluding the later set post
. Especially because the controller explicitly calls authorize :post
. I expected the post
to be in the authorization context after it had been set, without additional work.
That behavior was not expected and took awhile to debug, having to dig into the internals of Active Policy to figure out what was going on. I'd expect any changes to the authorization context and its records be reflected in future authorizations.
Just curious β is there a particular reason the authorization_context
method is memoized? I can't see how a handful of, likely memoized themselves, method calls e.g. current_account
, current_user
, would be noticeably slower than a memoized authorization_context
call.
I looked through the method's history and I see it's always been this way, but I'm kind of curious if there's a reasoning here. To me, it seems like the memoization here wouldn't do much for performance.
from action_policy.
I'd expect any changes to the authorization context and its records be reflected in future authorizations.
I'm kind of curious if there's a reasoning here
The library was designed with the assumption that the context is defined once and never changes during the execution of a request (or another unit of work). Which doesn't seem to cover all the use cases. "A great abstraction will be used in ways you never anticipated" π
I think, getting rid of the memoization could be added to the v1 backlog.
For now, I suggest the following: split the current #authorization_context
method into two: #build_authorization_context
(building a Hash from the configured targets) and #authorization_context
(adds memoization). So you can "disable" memoization by adding an alias: alias authorization_context build_authorization_context
. WDYT?
from action_policy.
Sounds great!
Please, let me know if you some ideas of what could be great to have in the library itself.
P.S. Thanks for sponsoring
from action_policy.
@palkan this actually breaks our builds because we use traceroute
gem to check for unused routes. Is it possible to just make it these two methods private instead? π€ Happy to make the PR for it if needed.
from action_policy.
@corroded Oh, that's interesting. Could you provide more details? Sure, we can make #build_authorization_context
private (but not #authorization_context
, it was always public).
from action_policy.
@corroded Oh, that's interesting. Could you provide more details? Sure, we can make
#build_authorization_context
private (but not#authorization_context
, it was always public).
Should I provide this here or in another issue? Anyway, we have an ActiveSupport concern that includes ActionPolicy::Controller:
app/controllers/api/concerns/authorization.rb
module API::Concerns::Authorization
extend ActiveSupport::Concern
include ActionPolicy::Controller
...
this is then included in all controllers via:
class API::SomeController < ApplicationController
include API::Concerns::Authorization
I think the traceroute
gem basically audits all our controllers and checks if there are actions that are unreachable - meaning there are no routes against it. Adding #build_authorization_context
adds a new action that is unreachable, thus failing CI.
THAT SAID, I can just add it to our ignore file (which I have done). Just thought it would be a good idea to make the method private since it is not a public API? WDYT?
Additionally, we also have another failing test that is related to this change that broke:
this one that checks if ActiveRecord is being hit π€ interesting though as I don't see the specific change affecting this but maybe you might have a clue.
# This is a regression test for a performance problem that was fixed
# upstream in the `action_policy` gem.
# See: https://github.com/palkan/action_policy/commit/ddb7e635550f96e6a7b4f3a639a68c602a8f0700
it "does not actually execute the scoped query" do
allow(ActiveRecord::Base.connection).to receive(:exec_query).and_call_original
controller_class.action(:index).call(fake_request)
expect(ActiveRecord::Base.connection).not_to have_received(:exec_query)
.with(/FROM "webhook_deliveries"/, anything, anything, anything)
end
from action_policy.
many thanks!
from action_policy.
Related Issues (20)
- Add ability to authorize nil records if :with option is provided HOT 2
- NoMethodError: undefined method `params_filter' for MyPolicy:Class in tests HOT 1
- Authorizing fields based on params_filter HOT 1
- Unknown policy scope type :active_record_relation HOT 3
- Policy-generator not working with Ruby 3.2 HOT 1
- uninitialized constant ActionController::Parameters HOT 4
- I18n does not seem to work with I18n Active Record HOT 1
- How Do I Test Resource-less Authorize? HOT 1
- Add --parent option to policy generator HOT 2
- Update a documentation about #be_an_alias_of matcher HOT 2
- Documentation Contrast HOT 3
- Can't alias `create?` to `manage?` HOT 1
- Cannot use `controller_authorize_current_user` with `ActionPolicy::Base` HOT 1
- Rspec fails with v0.6.6 when `eager_load` is set to `true` HOT 13
- 0.6.7 breaks wrap_parameters HOT 3
- Policy lookup for authorized_scope returns default policy instead of using implicit authorization target HOT 3
- Add `with_context` qualifier to `have_authorized_scope` matcher. HOT 1
- Allow using callable objects as scopes HOT 1
- Migrate pretty print to Prism
- Allow to reset authorization context HOT 7
Recommend Projects
-
React
A declarative, efficient, and flexible JavaScript library for building user interfaces.
-
Vue.js
π Vue.js is a progressive, incrementally-adoptable JavaScript framework for building UI on the web.
-
Typescript
TypeScript is a superset of JavaScript that compiles to clean JavaScript output.
-
TensorFlow
An Open Source Machine Learning Framework for Everyone
-
Django
The Web framework for perfectionists with deadlines.
-
Laravel
A PHP framework for web artisans
-
D3
Bring data to life with SVG, Canvas and HTML. πππ
-
Recommend Topics
-
javascript
JavaScript (JS) is a lightweight interpreted programming language with first-class functions.
-
web
Some thing interesting about web. New door for the world.
-
server
A server is a program made to process requests and deliver data to clients.
-
Machine learning
Machine learning is a way of modeling and interpreting data that allows a piece of software to respond intelligently.
-
Visualization
Some thing interesting about visualization, use data art
-
Game
Some thing interesting about game, make everyone happy.
Recommend Org
-
Facebook
We are working to build community through open source technology. NB: members must have two-factor auth.
-
Microsoft
Open source projects and samples from Microsoft.
-
Google
Google β€οΈ Open Source for everyone.
-
Alibaba
Alibaba Open Source for everyone
-
D3
Data-Driven Documents codes.
-
Tencent
China tencent open source team.
from action_policy.