Dancer::Plugin::Auth::Extensible - possible backwards-incompatible change
Hi all, Whilst I really like the (ab)use of subroutine attributes for denoting which routes require authentication/specific roles, some people (whose opinions I respect) have tried to convince me that this is a Bad Idea, and is likely to be fragile. One particularly good point made is that the current implementation stores the attributes for a given route handler by the refaddr, which could be problematic if run under threads (not sure if anyone really does that, though). Classes can provide a CLONE method to work around this, but I don't think that'll work in this case. One suggestion was to provide a new keyword, e.g. requires_auth, which would work something like: get '/secret' => requires_login(sub { .... }); get '/beer' => requires_role('BeerDrinker', sub { ... }); (Something along those lines, at least.) I'm certain how I would implement it, though - i.e. how requires_login/requires_role would store the fact that the provided sub requires auth, without the same thread safety issues of using refaddr. Perhaps detecting the use of threads and refusing to continue would be one way of dealing with it :) Opinions on this would be very welcome. -- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
Perhaps I'm missing something but... "how requires_login/requires_role would store the fact that the provided sub requires auth" Simple implementation: # given has_role is calculable sub requires_role { my ($role, $code, $handle_exception) = @_; return sub { if (has_role($role)){ $code->(@_); } else{ if (defined $handle_exception){ $handle_exception->($role, @_) } else { default_403_no_role($role, @_) } } } } Complex implementation: requires_login and requires_role return objects which have metadata about their requirements and exception handling which can be changed easily, but that overload as coderefs to something like the code above. From: David Precious <davidp@preshweb.co.uk> To: dancer-users@dancer.pm Date: 11/12/2012 11:25 Subject: [dancer-users] Dancer::Plugin::Auth::Extensible - possible backwards-incompatible change Sent by: dancer-users-bounces@dancer.pm Hi all, Whilst I really like the (ab)use of subroutine attributes for denoting which routes require authentication/specific roles, some people (whose opinions I respect) have tried to convince me that this is a Bad Idea, and is likely to be fragile. One particularly good point made is that the current implementation stores the attributes for a given route handler by the refaddr, which could be problematic if run under threads (not sure if anyone really does that, though). Classes can provide a CLONE method to work around this, but I don't think that'll work in this case. One suggestion was to provide a new keyword, e.g. requires_auth, which would work something like: get '/secret' => requires_login(sub { .... }); get '/beer' => requires_role('BeerDrinker', sub { ... }); (Something along those lines, at least.) I'm certain how I would implement it, though - i.e. how requires_login/requires_role would store the fact that the provided sub requires auth, without the same thread safety issues of using refaddr. Perhaps detecting the use of threads and refusing to continue would be one way of dealing with it :) Opinions on this would be very welcome. -- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github _______________________________________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/mailman/listinfo/dancer-users
On Tue, 11 Dec 2012 11:40:29 +0000 Daniel Perrett <dperrett@cambridge.org> wrote:
Perhaps I'm missing something but...
"how requires_login/requires_role would store the fact that the provided sub requires auth"
Simple implementation:
# given has_role is calculable
sub requires_role { my ($role, $code, $handle_exception) = @_; return sub { if (has_role($role)){ $code->(@_); } else{ if (defined $handle_exception){ $handle_exception->($role, @_) } else { default_403_no_role($role, @_) } } } }
Ah, yes - that's pretty clever. I think like that approach - it's simple and solid. Thanks! -- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
Hi, The only drawback is performance : there is an additional indirection, and code dereferencing might be costly. The optimization of the route caching stops wher your route code starts, so it won't be optimized. You don't get performance hits ( or less, at least I think) with attributes. Hm, it could be solved by calling got &$code actually. But be careful about argument passing. On 11 December 2012 13:39, David Precious <davidp@preshweb.co.uk> wrote:
On Tue, 11 Dec 2012 11:40:29 +0000 Daniel Perrett <dperrett@cambridge.org> wrote:
Perhaps I'm missing something but...
"how requires_login/requires_role would store the fact that the provided sub requires auth"
Simple implementation:
# given has_role is calculable
sub requires_role { my ($role, $code, $handle_exception) = @_; return sub { if (has_role($role)){ $code->(@_); } else{ if (defined $handle_exception){ $handle_exception->($role, @_) } else { default_403_no_role($role, @_) } } } }
Ah, yes - that's pretty clever. I think like that approach - it's simple and solid.
Thanks!
-- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
_______________________________________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/mailman/listinfo/dancer-users
On Tue, Dec 11, 2012 at 9:10 AM, damien krotkine <dkrotkine@gmail.com> wrote:
The only drawback is performance : there is an additional indirection, and code dereferencing might be costly. The optimization of the route caching stops wher your route code starts, so it won't be optimized. You don't get performance hits ( or less, at least I think) with attributes.
I'm not sure it's clear cut. Right now, there is a before hook that runs for *every* route that has to do this: * method call to get the coderef for the route * function call to retrieve attributes for coderef * logic on attributes returned If only a fraction of routes require authentication, then that's a lot of extra overhead compared to wrapping only the subs that need it. And even if all routes require auth, the indirection is one call compared to two. The two calls might be cacheable, though, but even still, a single extra sub call only on routes that need it isn't too bad. David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
Le mardi 11 décembre 2012 à 20:56, David Golden a écrit :
On Tue, Dec 11, 2012 at 9:10 AM, damien krotkine <dkrotkine@gmail.com (mailto:dkrotkine@gmail.com)> wrote:
The only drawback is performance : there is an additional indirection, and code dereferencing might be costly. The optimization of the route caching stops wher your route code starts, so it won't be optimized. You don't get performance hits ( or less, at least I think) with attributes.
I'm not sure it's clear cut. Right now, there is a before hook that runs for *every* route that has to do this:
* method call to get the coderef for the route * function call to retrieve attributes for coderef * logic on attributes returned
Hm you're right, I had not realized it'd be for *every* route. Indeed we need more benchmark here :) But I like the attribute style. If it can help in decision making, catalyst makes use of them as well, and it's rather elegant.
If only a fraction of routes require authentication, then that's a lot of extra overhead compared to wrapping only the subs that need it. And even if all routes require auth, the indirection is one call compared to two. The two calls might be cacheable, though, but even still, a single extra sub call only on routes that need it isn't too bad.
David
-- David Golden <xdg@xdg.me (mailto:xdg@xdg.me)> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg _______________________________________________ dancer-users mailing list dancer-users@dancer.pm (mailto:dancer-users@dancer.pm) http://lists.preshweb.co.uk/mailman/listinfo/dancer-users
Hm you're right, I had not realized it'd be for *every* route. Indeed we need more benchmark here :) But I like the attribute style. If it can help in decision making, catalyst makes use of them as well, and it's rather elegant.
Attribute style might be visually nice, but the implementation of attributes in Perl is really ugly hackery and many people (myself included) who have gotten into the weeds have concluded that it's just not worth the trouble. Personally, I would find the style below just as appealing if not more because it's guaranteed to follow Perl's grammar: get '/beer' => requires_role BeerDrinker => sub { ... }; get '/user/:user_id' => requires_role qw/Admin TeamLeader/ => sub { ... }; Plus, I can do this: my @power_users = qw/Admin TeamLeader/; get '/user/:user_id' => requires_role @power_users => sub { ... }; You can't as easily do that with attributes because attribute parameters are strings that have to be parsed -- typically split on whitespace. You *could* parse looking for "@\w+" and then eval() it, but (a) that's gross and (b) you've got timing issues because the attribute is processed at compile time and the array isn't populated until runtime. And consider if you want to put your role names in a config file instead of the source code. How do you do that with attributes? Have I convinced the doubters by now? :-) David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
Le mardi 11 décembre 2012 à 21:16, David Golden a écrit :
Hm you're right, I had not realized it'd be for *every* route. Indeed we need more benchmark here :) But I like the attribute style. If it can help in decision making, catalyst makes use of them as well, and it's rather elegant.
Attribute style might be visually nice, but the implementation of attributes in Perl is really ugly hackery and many people (myself included) who have gotten into the weeds have concluded that it's just not worth the trouble.
Personally, I would find the style below just as appealing if not more because it's guaranteed to follow Perl's grammar:
get '/beer' => requires_role BeerDrinker => sub { ... };
get '/user/:user_id' => requires_role qw/Admin TeamLeader/ => sub { ... };
Plus, I can do this:
my @power_users = qw/Admin TeamLeader/;
get '/user/:user_id' => requires_role @power_users => sub { ... };
You can't as easily do that with attributes because attribute parameters are strings that have to be parsed -- typically split on whitespace. You *could* parse looking for "@\w+" and then eval() it, but (a) that's gross and (b) you've got timing issues because the attribute is processed at compile time and the array isn't populated until runtime.
And consider if you want to put your role names in a config file instead of the source code. How do you do that with attributes?
Use a huge BEGIN for parsing the config file ? :) Just kidding
Have I convinced the doubters by now? :-)
I'm convinced :)
Hi, Maybe is a stupid suggestion that might not even be possible to do, but I think the syntax would look nicer if would be something like: get '/secret' => sub { requires_login; ... }; get '/beer' => sub { requires_role 'BeerDrinker'; ... }; get '/beer' => sub { requires_any_role [ 'BeerDrinker', 'VodkaDrinker' ]; ... }; --Octavian ----- Original Message ----- From: "David Precious" <davidp@preshweb.co.uk> To: <dancer-users@dancer.pm> Sent: Tuesday, December 11, 2012 1:25 PM Subject: [dancer-users] Dancer::Plugin::Auth::Extensible - possible backwards-incompatible change
Hi all,
Whilst I really like the (ab)use of subroutine attributes for denoting which routes require authentication/specific roles, some people (whose opinions I respect) have tried to convince me that this is a Bad Idea, and is likely to be fragile.
One particularly good point made is that the current implementation stores the attributes for a given route handler by the refaddr, which could be problematic if run under threads (not sure if anyone really does that, though). Classes can provide a CLONE method to work around this, but I don't think that'll work in this case.
One suggestion was to provide a new keyword, e.g. requires_auth, which would work something like:
get '/secret' => requires_login(sub { .... });
get '/beer' => requires_role('BeerDrinker', sub { ... });
(Something along those lines, at least.) I'm certain how I would implement it, though - i.e. how requires_login/requires_role would store the fact that the provided sub requires auth, without the same thread safety issues of using refaddr.
Perhaps detecting the use of threads and refusing to continue would be one way of dealing with it :)
Opinions on this would be very welcome.
-- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
_______________________________________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/mailman/listinfo/dancer-users
If the session is able to distinguish each subroutine context, it would be possible. I'm not sure if it's supported. On Tue, Dec 11, 2012 at 1:47 PM, Octavian Rasnita <orasnita@gmail.com>wrote:
Hi,
Maybe is a stupid suggestion that might not even be possible to do, but I think the syntax would look nicer if would be something like:
get '/secret' => sub { requires_login; ... };
get '/beer' => sub { requires_role 'BeerDrinker'; ... };
get '/beer' => sub { requires_any_role [ 'BeerDrinker', 'VodkaDrinker' ]; ... };
--Octavian
----- Original Message ----- From: "David Precious" <davidp@preshweb.co.uk
To: <dancer-users@dancer.pm> Sent: Tuesday, December 11, 2012 1:25 PM
Subject: [dancer-users] Dancer::Plugin::Auth::**Extensible - possible backwards-incompatible change
Hi all,
Whilst I really like the (ab)use of subroutine attributes for denoting which routes require authentication/specific roles, some people (whose opinions I respect) have tried to convince me that this is a Bad Idea, and is likely to be fragile.
One particularly good point made is that the current implementation stores the attributes for a given route handler by the refaddr, which could be problematic if run under threads (not sure if anyone really does that, though). Classes can provide a CLONE method to work around this, but I don't think that'll work in this case.
One suggestion was to provide a new keyword, e.g. requires_auth, which would work something like:
get '/secret' => requires_login(sub { .... });
get '/beer' => requires_role('BeerDrinker', sub { ... });
(Something along those lines, at least.) I'm certain how I would implement it, though - i.e. how requires_login/requires_role would store the fact that the provided sub requires auth, without the same thread safety issues of using refaddr.
Perhaps detecting the use of threads and refusing to continue would be one way of dealing with it :)
Opinions on this would be very welcome.
-- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
______________________________**_________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/**mailman/listinfo/dancer-users<http://lists.preshweb.co.uk/mailman/listinfo/dancer-users>
______________________________**_________________ dancer-users mailing list dancer-users@dancer.pm http://lists.preshweb.co.uk/**mailman/listinfo/dancer-users<http://lists.preshweb.co.uk/mailman/listinfo/dancer-users>
On Tue, 11 Dec 2012 13:47:28 +0200 "Octavian Rasnita" <orasnita@gmail.com> wrote:
Hi,
Maybe is a stupid suggestion that might not even be possible to do, but I think the syntax would look nicer if would be something like:
get '/secret' => sub { requires_login; ... };
get '/beer' => sub { requires_role 'BeerDrinker'; ... };
Should be possible! I'm not a big fan of that approach as it seems a little... manual; it feels only a step away from doing all the checking yourself :) My other objection is that it would be easy to forget to add the appropriate requires_* keyword call to a route, and thus have that route unprotected; one of the options I'd been planning with the current attributes-based approach was a setting to require all routes to have attributes set (and adding a new NoLoginNeeded one), so that you could optionally protect yourself against accidentally forgetting to protect newly-added routes. Having said that, though, it would be quite easy that way. -- David Precious ("bigpresh") <davidp@preshweb.co.uk> http://www.preshweb.co.uk/ www.preshweb.co.uk/twitter www.preshweb.co.uk/linkedin www.preshweb.co.uk/facebook www.preshweb.co.uk/cpan www.preshweb.co.uk/github
David Precious <davidp@preshweb.co.uk> wrote:
One particularly good point made is that the current implementation stores the attributes for a given route handler by the refaddr, which could be problematic if run under threads (not sure if anyone really does that, though).
Moreover, if the coderef is a closure, refaddr isn't stable between compile time (when the attribute is applied) and run time (when the closure is created). But I believe this offers a workaround: # Returns an arbitrary non-reference value that should be stable across all # closures which are clones of (the same thing as) $coderef. sub subroutine_identity { my $coderef = shift; die "Not a CODE reference\n" if ref $coderef ne 'CODE'; my $cv = svref_2object($coderef); die "Not a CV\n" if !$cv->isa('B::CV'); my $op = $cv->START; return $$op; } What's this doing? B::svref_2object takes an arbitrary Perl reference, and returns an instance of the appropriate B class (B::CV in this case), representing the underlying object in the perl VM. Then $cv->START returns (a reference to) an integer representing the address of the first op in the subroutine. So this extracts a stable value that's guaranteed to refer to the underlying code rather than any of its closures. See also this thread on the perl-qa list: http://www.nntp.perl.org/group/perl.qa/2012/11/msg13303.html I haven't tried this in a threaded Perl, but it should work fine, because, AIUI, optrees are shared across cloned threads (even for closures created in different threads). Personally, I do like the attribute-based API in D::P::A::E, so I'd like to put in a plea for sticking with it. Unless I'm wrong about this approach working well enough, of course. :-) -- Aaron Crane ** http://aaroncrane.co.uk/
On Tue, Dec 11, 2012 at 2:42 PM, Aaron Crane <perl@aaroncrane.co.uk> wrote:
subroutine_identity
I can't say whether this is necessary or not, but, if so, it adds another function call needed at runtime to look up every route's closure. Wrapping the coderef as Daniel suggests is looking better and better. David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On Tue, Dec 11, 2012 at 11:25:00AM +0000, David Precious wrote:
Whilst I really like the (ab)use of subroutine attributes for denoting which routes require authentication/specific roles, some people (whose opinions I respect) have tried to convince me that this is a Bad Idea, and is likely to be fragile.
One particularly good point made is that the current implementation stores the attributes for a given route handler by the refaddr, which could be problematic if run under threads ...
... or under the debugger, as David P and I know all too well :-)
One suggestion was to provide a new keyword, e.g. requires_auth, which would work something like:
get '/secret' => requires_login(sub { .... });
get '/beer' => requires_role('BeerDrinker', sub { ... });
(Something along those lines, at least.) I'm certain how I would implement it, though - i.e. how requires_login/requires_role would store the fact that the provided sub requires auth, without the same thread safety issues of using refaddr.
That's easy. requires_role() constructs a subroutine that does the authentication and then hands off to the supplied sub. Something like this: sub requires_role { my $role = shift; my $handler = shift; my $fail_handler = shift; return sub { if(currently_logged_in_as($role)) { return $handler->(); } else { return $fail_handler->(); } } } -- David Cantrell | top google result for "topless karaoke murders" "Cynical" is a word used by the naive to describe the experienced. George Hills, in uknot
participants (9)
-
Aaron Crane -
damien krotkine -
Damien Krotkine
-
Daniel Perrett -
David Cantrell -
David Golden -
David Precious -
Octavian Rasnita -
sawyer x