Dancer::Plugin::Auth::Extensible - removing sub attributes - ready to try out
Hi all, Following the recent discussion on the list regarding DPAE's use of subroutine attributes to declare which routes need auth/specific roles and a before handler to check for them, the general consensus seems to be that, whilst the attributes-based approach is quite clean, it's not necessary solid enough to rely upon. (Multiple people whose opinions I respect advised me that there are likely to be issues with it - some potential problems include thread safety, and problems I've seen at work when running under the debugger / Devel::Cover etc.) For that reason, whilst DPAE is still young and not widely used (as far as I'm aware :) ), I've decided it would make sense to take the approach recommended by multiple people, and replace sub attributes with keywords which wrap the route handler coderef and perform the required checks before executing it. The changes are in a remove_attributes branch - the refactor commit is: https://github.com/bigpresh/Dancer-Plugin-Auth-Extensible/commit/32844b7c3cc... From a user's point of view, the changes are in how you define that a route needs login / specific roles. Previously, you would say: get '/secret' => sub :RequiresLogin { ... }; Now, you'd say: get '/secret' => requires_login sub { ... }; And to require specific roles: get '/beer' => requires_role BeerDrinker => sub { ... }; Opinions on this would be warmly welcomed. Also, if you've been using DPAE and have a moment to try out your code with these changes, that would be awesome. DPAE passes all its tests after the changes, so I'm reasonably convinced that it's working correctly, but will perform more testing before this hits CPAN. Cheers Dave P -- 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
On Sat, Dec 15, 2012 at 11:40 AM, David Precious <davidp@preshweb.co.uk> wrote:
And to require specific roles:
get '/beer' => requires_role BeerDrinker => sub { ... };
As implemented, requires_role() only allows a single role. I think you should do something along the lines of: sub requires_role { my $coderef = pop; my @roles = @_; ... } And then s/requires_role/requires_roles/ perhaps. David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On Sat, 15 Dec 2012 12:35:15 -0500 David Golden <xdg@xdg.me> wrote:
On Sat, Dec 15, 2012 at 11:40 AM, David Precious <davidp@preshweb.co.uk> wrote:
And to require specific roles:
get '/beer' => requires_role BeerDrinker => sub { ... };
As implemented, requires_role() only allows a single role. I think you should do something along the lines of:
sub requires_role { my $coderef = pop; my @roles = @_; ... }
And then s/requires_role/requires_roles/ perhaps.
Yeah, that's true. I was thinking that require_role could accept an arrayref of roles instead of a straightforward role, if desired, so you could also say e.g.: get '/foo' => requires_role ['Foo','Bar'] => sub { ... }; (requires_roles could be added as an alias, so code could read better.) I imagine the common requirement will be to say "any of these roles", not "all of these roles". I was considering whether requires_role should be for "must have this role" or "must have all of these roles", and e.g. a new requires_any_role keyword would be added to ensure a user had all the specified roles; I'm not sure how valuable that would be, though. -- 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
On Sat, Dec 15, 2012 at 2:20 PM, David Precious <davidp@preshweb.co.uk> wrote:
Yeah, that's true. I was thinking that require_role could accept an arrayref of roles instead of a straightforward role, if desired, so you could also say e.g.:
get '/foo' => requires_role ['Foo','Bar'] => sub { ... };
(requires_roles could be added as an alias, so code could read better.)
I imagine the common requirement will be to say "any of these roles", not "all of these roles". I was considering whether requires_role should be for "must have this role" or "must have all of these roles", and e.g. a new requires_any_role keyword would be added to ensure a user had all the specified roles; I'm not sure how valuable that would be, though.
OK, I see. In that case, I think you'd be better off keeping the API as it is (taking a single "comparator") and letting users use Syntax::Keyword::Junction to be clear about their intent. All you really need to say is that the first argument to 'require_role' must be comparable with the "eq" operator and then give an example of using Syntax::Keyword::Junction for any more complex logic. use Syntax::Keyword::Junction qw/any all/; get '/foo' => requires_role any(qw/Foo Bar/) => sub { ... }; get '/bar' => requires_role all(qw/Foo Bar/) => sub { ... }; I think that might work as-is. (But you should test it, of course.) David -- David Golden <xdg@xdg.me> Take back your inbox! → http://www.bunchmail.com/ Twitter/IRC: @xdg
On Sat, 15 Dec 2012 14:35:18 -0500 David Golden <xdg@xdg.me> wrote:
On Sat, Dec 15, 2012 at 2:20 PM, David Precious <davidp@preshweb.co.uk> wrote:
Yeah, that's true. I was thinking that require_role could accept an arrayref of roles instead of a straightforward role, if desired, so you could also say e.g.:
get '/foo' => requires_role ['Foo','Bar'] => sub { ... };
(requires_roles could be added as an alias, so code could read better.)
I imagine the common requirement will be to say "any of these roles", not "all of these roles". I was considering whether requires_role should be for "must have this role" or "must have all of these roles", and e.g. a new requires_any_role keyword would be added to ensure a user had all the specified roles; I'm not sure how valuable that would be, though.
OK, I see.
In that case, I think you'd be better off keeping the API as it is (taking a single "comparator") and letting users use Syntax::Keyword::Junction to be clear about their intent.
All you really need to say is that the first argument to 'require_role' must be comparable with the "eq" operator and then give an example of using Syntax::Keyword::Junction for any more complex logic.
use Syntax::Keyword::Junction qw/any all/; get '/foo' => requires_role any(qw/Foo Bar/) => sub { ... }; get '/bar' => requires_role all(qw/Foo Bar/) => sub { ... };
Hmm, that would be nice, but any() is already a Dancer keyword; I could import any() as a different name, I believe, courtesy of Sub::Exporter, but that involves a fairly big dependency chain. For the time being, I've added two new keywords to handle matching multiple roles - requires_any_role and requires_all_roles, which do as their names would suggest. I think that's nice and clear, and will capably handle most user's requirements easily. -- 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
On Sat, 15 Dec 2012 20:16:21 +0000 David Precious <davidp@preshweb.co.uk> wrote:
For the time being, I've added two new keywords to handle matching multiple roles - requires_any_role and requires_all_roles, which do as their names would suggest.
Or, rather, I've standardised on singular require_* (e.g. require_login, require_role, require_any_role, require_all_roles) for consistency, and because I think it reads better - it's a command, to require something before this route can execute. -- 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
On 15/12/2012 19:20, David Precious wrote:
get '/foo' => requires_role ['Foo','Bar'] => sub { ... };
(requires_roles could be added as an alias, so code could read better.)
I imagine the common requirement will be to say "any of these roles", not "all of these roles". I was considering whether requires_role should be for "must have this role" or "must have all of these roles", and e.g. a new requires_any_role keyword would be added to ensure a user had all the specified roles; I'm not sure how valuable that would be, though.
You definitely need to be able to support any and all. Which is the default doesn't really matter IMO. I suggest also letting the user supply their own authentication sub so that they can implement exotica like "must be cleared for Case Nightmare Green and be ranked Major or higher". -- David Cantrell | top google result for "internet beard fetish club" It's my experience that neither users nor customers can articulate what it is they want, nor can they evaluate it when they see it -- Alan Cooper
On Tue, 18 Dec 2012 16:09:47 +0000 David Cantrell <david@cantrell.org.uk> wrote:
On 15/12/2012 19:20, David Precious wrote:
get '/foo' => requires_role ['Foo','Bar'] => sub { ... };
(requires_roles could be added as an alias, so code could read better.)
I imagine the common requirement will be to say "any of these roles", not "all of these roles". I was considering whether requires_role should be for "must have this role" or "must have all of these roles", and e.g. a new requires_any_role keyword would be added to ensure a user had all the specified roles; I'm not sure how valuable that would be, though.
You definitely need to be able to support any and all. Which is the default doesn't really matter IMO.
Indeed - the overhauled version provides require_any_role and require_all_roles keywords.
I suggest also letting the user supply their own authentication sub so that they can implement exotica like "must be cleared for Case Nightmare Green and be ranked Major or higher".
Hmm - I could handle that with a hook that fires, and whose return value can indicate whether the request is OK, possibly. Or, I could add a require_custom_auth (or similarly-named) keyword which would take a coderef which is used to decide if the route should be allowed, so you could say e.g.: sub check_auth { my %has_role = map { $_ => 1 } user_roles(); return ($has_role{Drinker} && !$has_role{Lightweight}); }; get '/shot' => require_custom_auth \&check_auth, sub { ... }; Ta for the suggestion, oh bearded one. -- 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
On 19/12/2012, at 3:09 AM, David Cantrell wrote:
On 15/12/2012 19:20, David Precious wrote:
get '/foo' => requires_role ['Foo','Bar'] => sub { ... };
(requires_roles could be added as an alias, so code could read better.)
I imagine the common requirement will be to say "any of these roles", not "all of these roles". I was considering whether requires_role should be for "must have this role" or "must have all of these roles", and e.g. a new requires_any_role keyword would be added to ensure a user had all the specified roles; I'm not sure how valuable that would be, though.
You definitely need to be able to support any and all. Which is the default doesn't really matter IMO.
I'd go for the most restrictive option (all) being the default. This ensures that someone who mis-guesses or misinterprets the default behaviour is less likely to grant inappropriate access to their content. Accidental denial of access can be un-denied, but disclosed information cannot be un-disclosed.
On 19/12/2012, at 05:24, Adam Clarke <adam.clarke@strategicdata.com.au> wrote:
I'd go for the most restrictive option (all) being the default. This ensures that someone who mis-guesses or misinterprets the default behaviour is less likely to grant inappropriate access to their content. Accidental denial of access can be un-denied, but disclosed information cannot be un-disclosed.
+1
participants (5)
-
Adam Clarke -
David Cantrell -
David Golden -
David Precious -
Paulo A Ferreira