From: Tomas Doran Date: Sat, 30 Jun 2012 10:46:30 +0000 (+0100) Subject: Use Try::Tiny rather than eval X-Git-Tag: 0.10021~3 X-Git-Url: http://git.shadowcat.co.uk/gitweb/gitweb.cgi?p=catagits%2FCatalyst-Plugin-Authentication.git;a=commitdiff_plain;h=8f57bf96b78b15ca9407e6ab5d2426745ac8a1f0 Use Try::Tiny rather than eval --- diff --git a/Makefile.PL b/Makefile.PL index b8cd666..865a31c 100644 --- a/Makefile.PL +++ b/Makefile.PL @@ -24,6 +24,7 @@ requires 'Moose'; requires 'MooseX::Emulate::Class::Accessor::Fast'; requires 'namespace::clean'; requires 'String::RewritePrefix'; +requires 'Try::Tiny'; test_requires 'Test::More' => '0.88'; test_requires 'Test::Exception'; diff --git a/lib/Catalyst/Authentication/Credential/Remote.pm b/lib/Catalyst/Authentication/Credential/Remote.pm index 211e89d..e7bfb56 100644 --- a/lib/Catalyst/Authentication/Credential/Remote.pm +++ b/lib/Catalyst/Authentication/Credential/Remote.pm @@ -2,6 +2,7 @@ package Catalyst::Authentication::Credential::Remote; use strict; use warnings; +use Try::Tiny qw/ try catch /; use base 'Class::Accessor::Fast'; @@ -17,21 +18,27 @@ sub new { bless $self, $class; # we are gonna compile regular expresions defined in config parameters - # and explicitly throw an exception saying what parameter was invalid - if (defined($config->{allow_regexp}) && ($config->{allow_regexp} ne "")) { - eval { $self->allow_re( qr/$config->{allow_regexp}/ ) }; - Catalyst::Exception->throw( "Invalid regular expression in ". - "'allow_regexp' configuration parameter") if $@; + # and explicitly throw an exception saying what parameter was invalid + if (defined($config->{allow_regexp}) && ($config->{allow_regexp} ne "")) { + try { $self->allow_re( qr/$config->{allow_regexp}/ ) } + catch { + Catalyst::Exception->throw( "Invalid regular expression in ". + "'allow_regexp' configuration parameter"); + }; } - if (defined($config->{deny_regexp}) && ($config->{deny_regexp} ne "")) { - eval { $self->deny_re( qr/$config->{deny_regexp}/ ) }; - Catalyst::Exception->throw( "Invalid regular expression in ". - "'deny_regexp' configuration parameter") if $@; + if (defined($config->{deny_regexp}) && ($config->{deny_regexp} ne "")) { + try { $self->deny_re( qr/$config->{deny_regexp}/ ) } + catch { + Catalyst::Exception->throw( "Invalid regular expression in ". + "'deny_regexp' configuration parameter"); + }; } - if (defined($config->{cutname_regexp}) && ($config->{cutname_regexp} ne "")) { - eval { $self->cutname_re( qr/$config->{cutname_regexp}/ ) }; - Catalyst::Exception->throw( "Invalid regular expression in ". - "'cutname_regexp' configuration parameter") if $@; + if (defined($config->{cutname_regexp}) && ($config->{cutname_regexp} ne "")) { + try { $self->cutname_re( qr/$config->{cutname_regexp}/ ) } + catch { + Catalyst::Exception->throw( "Invalid regular expression in ". + "'cutname_regexp' configuration parameter"); + }; } $self->source($config->{source} || 'REMOTE_USER'); $self->realm($realm); diff --git a/lib/Catalyst/Authentication/Realm.pm b/lib/Catalyst/Authentication/Realm.pm index 28ce7ea..c469f88 100644 --- a/lib/Catalyst/Authentication/Realm.pm +++ b/lib/Catalyst/Authentication/Realm.pm @@ -3,6 +3,7 @@ package Catalyst::Authentication::Realm; use strict; use warnings; use String::RewritePrefix; +use Try::Tiny qw/ try catch /; use base qw/Class::Accessor::Fast/; @@ -68,48 +69,46 @@ sub new { ## Note to self - catch second exception and bitch in detail? - eval { + try { Catalyst::Utils::ensure_class_loaded( $credentialclass ); - }; - - if ($@) { + } + catch { # If the file is missing, then try the old-style fallback, # but re-throw anything else for the user to deal with. - die unless $@ =~ /^Can't locate/; + die $_ unless /^Can't locate/; $app->log->warn( qq(Credential class "$credentialclass" not found, trying deprecated ::Plugin:: style naming. ) ); my $origcredentialclass = $credentialclass; $credentialclass =~ s/Catalyst::Authentication/Catalyst::Plugin::Authentication/; - eval { Catalyst::Utils::ensure_class_loaded( $credentialclass ); }; - if ($@) { + try { Catalyst::Utils::ensure_class_loaded( $credentialclass ); } + catch { # Likewise this croak is useful if the second exception is also "not found", # but would be confusing if it's anything else. - die unless $@ =~ /^Can't locate/; + die $_ unless /^Can't locate/; Carp::croak "Unable to load credential class, " . $origcredentialclass . " OR " . $credentialclass . " in realm " . $self->name; - } - } - - eval { - Catalyst::Utils::ensure_class_loaded( $storeclass ); + }; }; - if ($@) { + try { + Catalyst::Utils::ensure_class_loaded( $storeclass ); + } + catch { # If the file is missing, then try the old-style fallback, # but re-throw anything else for the user to deal with. - die unless $@ =~ /^Can't locate/; + die $_ unless /^Can't locate/; $app->log->warn( qq(Store class "$storeclass" not found, trying deprecated ::Plugin:: style naming. ) ); my $origstoreclass = $storeclass; $storeclass =~ s/Catalyst::Authentication/Catalyst::Plugin::Authentication/; - eval { Catalyst::Utils::ensure_class_loaded( $storeclass ); }; - if ($@) { + try { Catalyst::Utils::ensure_class_loaded( $storeclass ); } + catch { # Likewise this croak is useful if the second exception is also "not found", # but would be confusing if it's anything else. - die unless $@ =~ /^Can't locate/; + die $_ unless /^Can't locate/; Carp::croak "Unable to load store class, " . $origstoreclass . " OR " . $storeclass . " in realm " . $self->name; - } - } + }; + }; # BACKWARDS COMPATIBILITY - if the store class does not define find_user, we define it in terms # of get_user and add it to the class. this is because the auth routines use find_user, @@ -127,13 +126,15 @@ sub new { ## we'll remove this soon. if ($storeclass->can('new')) { $self->store($storeclass->new($config->{'store'}, $app, $self)); - } else { + } + else { $app->log->error("THIS IS DEPRECATED: $storeclass has no new() method - Attempting to use uninstantiated"); $self->store($storeclass); } if ($credentialclass->can('new')) { $self->credential($credentialclass->new($config->{'credential'}, $app, $self)); - } else { + } + else { $app->log->error("THIS IS DEPRECATED: $credentialclass has no new() method - Attempting to use uninstantiated"); $self->credential($credentialclass); } diff --git a/lib/Catalyst/Plugin/Authentication.pm b/lib/Catalyst/Plugin/Authentication.pm index 3408b75..46fbb3b 100644 --- a/lib/Catalyst/Plugin/Authentication.pm +++ b/lib/Catalyst/Plugin/Authentication.pm @@ -863,8 +863,8 @@ default realm is checked. Returns the currently logged in user, or undef if there is none. Normally the user is re-retrieved from the store. -For L the user is re-restored -using the primary key of the user table. +For L the user is re-restored +using the primary key of the user table. Thus B can throw an error even though B returned true. diff --git a/lib/Catalyst/Plugin/Authentication/Internals.pod b/lib/Catalyst/Plugin/Authentication/Internals.pod index 9649cd8..fdbbc64 100644 --- a/lib/Catalyst/Plugin/Authentication/Internals.pod +++ b/lib/Catalyst/Plugin/Authentication/Internals.pod @@ -32,7 +32,7 @@ is required to implement a credential or a store. There are two main entry points you need to be aware of when writing a store or credential module. The first is initialization and the second is during the -actual call to the Catalyst application's authenticate method. +actual call to the Catalyst application's authenticate method. A simplified description of the authentication process follows: @@ -70,7 +70,7 @@ C<< $c->authenticate( $userinfo, $realm ) >> called 2) Credential's authenticate() method called with authinfo and realm object for current realm -=over 4 +=over 4 The realm object and the authinfo hash are provided to the credential object's authenticate call. In most cases the credential object will attempt to @@ -107,14 +107,14 @@ B - Per-Request operations When any user-related activity occurs, and $c->authenticate has not yet been called, the Catalyst::Plugin::Authentication module will attempt to restore the persisted user (normally from the session if one is available). -There is only one step in this process: +There is only one step in this process: =over 4 1) Store object's from_session() is called -=back - +=back + The serialized data previously returned by the store's for_session() method is provided to the from_session() method. The from_session() method should return a valid user object. @@ -138,12 +138,12 @@ L instantiates both a new credential object and a new store object. See below for the details of how credentials and stores are instantiated. -B: The instances created will remain active throughout the entire -lifetime of the application, and so should be relatively lightweight. -Care should be taken to ensure that they do not grow, or retain -information per request, because they will be involved in each +B: The instances created will remain active throughout the entire +lifetime of the application, and so should be relatively lightweight. +Care should be taken to ensure that they do not grow, or retain +information per request, because they will be involved in each authentication request and could therefore substantially -hurt memory consumption over time. +hurt memory consumption over time. =head2 AUTHENTICATION @@ -153,7 +153,7 @@ C<$c-Eauthenticate()> takes two arguments. The first is a hash reference containing all the information available about the user. This will be used to locate the user in the store and verify the user's credentials. The second argument is the realm to authenticate against. If the second argument is -omitted, the default realm is assumed. +omitted, the default realm is assumed. The main authentication module then locates the credential and store objects for the realm specified and calls the credential object's C @@ -201,16 +201,16 @@ first argument, C<$config>, is a hash reference containing the configuration information for the store module. The second argument is a reference to the Catalyst application. -Note that when new() is called, Catalyst has not yet loaded +Note that when new() is called, Catalyst has not yet loaded the various controller and model classes, nor is it definite -that other plugins have been loaded, so your new() method -must not rely on any of those being present. If any of +that other plugins have been loaded, so your new() method +must not rely on any of those being present. If any of this is required for your store to function, you should -defer that part of initialization until the first method call. +defer that part of initialization until the first method call. The C method should return a blessed reference to your store object. -=item find_user( $authinfo, $c ) +=item find_user( $authinfo, $c ) This is the workhorse of any authentication store. It's job is to take the information provided to it via the C<$authinfo> hashref and locate the user @@ -219,10 +219,10 @@ of anything else is considered to mean no user was found that matched the information provided. How C accomplishes it's job is entirely up to you, the author, as -is what $authinfo is required to contain. Many stores will simply use a +is what $authinfo is required to contain. Many stores will simply use a username element in $authinfo to locate the user, but more advanced functionality is possible and you may bend the $authinfo to your needs. Be aware, however, that -both Credentials and Stores usually work with the same $authinfo hash, so take +both Credentials and Stores usually work with the same $authinfo hash, so take care to avoid overlapping element names. Please note that this routine may be called numerous times in various @@ -234,22 +234,22 @@ store object. =item for_session( $c, $user ) This method is responsible for preparing a user object for storage in the session. -It should return information that can be placed in the session and later used to +It should return information that can be placed in the session and later used to restore a user object (using the C method). It should therefore ensure that whatever information provided can be used by the C method to locate the unique user being saved. Note that there is no guarantee -that the same Catalyst instance will receive both the C and +that the same Catalyst instance will receive both the C and C calls. You should take care to provide information that can -be used to restore a user, regardless of the current state of the application. +be used to restore a user, regardless of the current state of the application. A good rule of thumb is that if C can revive the user with the -given information even if the Catalyst application has just started up, you are +given information even if the Catalyst application has just started up, you are in good shape. =item from_session( $c, $frozenuser ) -This method is called whenever a user is being restored from the session. +This method is called whenever a user is being restored from the session. C<$frozenuser> contains the information that was stored in the session for the user. -This will under normal circumstances be the exact data your store returned from +This will under normal circumstances be the exact data your store returned from the previous call to C. C should return a valid user object. @@ -260,9 +260,9 @@ underlying user object is capable of. This is pretty-well free-form and the main purpose is to allow graceful integration with credentials and applications that may provide advanced functionality based on whether the underlying user object can do certain things. In most cases you will want to -pass this directly to the underlying user class' C method. Note that -this is used as a B method against the user class and therefore must -be able to function without an instantiated user object. +pass this directly to the underlying user class' C method. Note that +this is used as a B method against the user class and therefore must +be able to function without an instantiated user object. =back @@ -307,19 +307,19 @@ should experience no problems. =item id( ) -The C method should return a unique id (scalar) that can be used to +The C method should return a unique id (scalar) that can be used to retreive this user from the store. Often this will be provided to the store's -C routine as C $user-Eid> so you should ensure that your -store's C can cope with that. +C routine as C $user-Eid> so you should ensure that your +store's C can cope with that. =item supports( $feature, $subfeature ... ) This method checks to see if the user class supports a particular feature. It -is implemented such that each argument provides a subfeature of the previous +is implemented such that each argument provides a subfeature of the previous argument. In other words, passing 'foo', 'bar' would return true if the user supported the 'foo' feature, and the 'bar' feature of 'foo'. This is implemented in Catalyst::Authentication::User, so if your class inherits from that, you -do not need to implement this and can instead implement supported_features(). +do not need to implement this and can instead implement supported_features(). B If you want the authentication module to be able to save your user in the session you must return true when presented with the feature 'session'. @@ -337,15 +337,15 @@ This method should return the value of the field matching fieldname provided, or undef if there is no field matching that fieldname. In most cases this will access the underlying storage mechanism for the user data and return the information. This is used as a standard method of accessing an authenticated -user's data, and MUST be implemented by all user objects. +user's data, and MUST be implemented by all user objects. -B: There is no equivalent 'set' method. Each user class is -likely to vary greatly in how data must be saved and it is -therefore impractical to try to provide a standard way of -accomplishing it. When an application developer needs to save -data, they should obtain the underlying object / data by +B: There is no equivalent 'set' method. Each user class is +likely to vary greatly in how data must be saved and it is +therefore impractical to try to provide a standard way of +accomplishing it. When an application developer needs to save +data, they should obtain the underlying object / data by calling get_object, and work with it directly. - + =item get_object( ) @@ -358,7 +358,7 @@ across user classes. If your object is not backed by another class, or you need to provide additional intermediate functionality, it is perfectly reasonable to return C<$self>. -=back +=back =head1 WRITING A CREDENTIAL @@ -368,25 +368,25 @@ one class to implement, and it consists of only two required routines. They are: new() - instantiates the credential object authenticate() - performs the authentication and returns a user object - + =head2 CREDENTIAL METHODS =over 4 =item new( $config, $app, $realm ) -Like the Store method of the same name, the C method is called only -once, during the setup process of +Like the Store method of the same name, the C method is called only +once, during the setup process of L. The first argument, C<$config>, is a hash reference containing the configuration -information for the credential module. The second argument is a reference +information for the credential module. The second argument is a reference to the Catalyst application. $realm is the instantiated Realm object, which you may use to access realm routines - such as find_user. -Again, when the credential's new() method is called, Catalyst -has not yet loaded the various controller and model classes. +Again, when the credential's new() method is called, Catalyst +has not yet loaded the various controller and model classes. -The new method should perform any necessary setup required and instantiate +The new method should perform any necessary setup required and instantiate your credential object. It should return your instantiated credential. =item authenticate( $c, $realm, $authinfo ) @@ -394,16 +394,16 @@ your credential object. It should return your instantiated credential. This is the workhorse of your credential. When $c->authenticate() is called the L module retrieves the realm object and passes it, along with the $authinfo hash -to your credential's authenticate method. Your module should use the -$authinfo hash to obtain the user from the realm passed, and then perform +to your credential's authenticate method. Your module should use the +$authinfo hash to obtain the user from the realm passed, and then perform any credential verification steps necessary to authenticate the user. This method should return the user object returned by the authentication store if -credential verification succeeded. It should return undef on failure. +credential verification succeeded. It should return undef on failure. How your credential module performs the credential verification is entirely up to you. In most cases, the credential will retrieve a user from the store -first (using the stores find_user() method), and then validate the user's -information. However, this does not have to be the case. +first (using the stores find_user() method), and then validate the user's +information. However, this does not have to be the case. It is perfectly acceptable for your credential to perform other tasks prior to attempting to retrieve the user from the store. It may also make sense for @@ -412,29 +412,29 @@ question, for example, finding a user id based on an encrypted token. In these scenarios, the $authinfo hash passed to find_user() can be different than that which is passed in to $c->authenticate(). Once again this is perfectly acceptable if it makes sense for your credential, -though you are strongly advised to note this behavior clearly in your -credential's documentation - as application authors are almost -certainly expecting the user to be found using the information provided +though you are strongly advised to note this behavior clearly in your +credential's documentation - as application authors are almost +certainly expecting the user to be found using the information provided to $c->authenticate(). Look at the L -module source to see this in action. In order to avoid possible -mismatches between the encrypted and unencrypted passwords, the password -credential actually removes the provided password from the authinfo -array. It does this because, in many cases, the store's password -field will be encrypted in some way, and the password passed to -$c->authenticate is almost certainly in plaintext. +module source to see this in action. In order to avoid possible +mismatches between the encrypted and unencrypted passwords, the password +credential actually removes the provided password from the authinfo +array. It does this because, in many cases, the store's password +field will be encrypted in some way, and the password passed to +$c->authenticate is almost certainly in plaintext. NOTE: You should always assume that a store is going to use all -the information passed to it to locate the user in question. +the information passed to it to locate the user in question. If there are fields in the $authinfo hash that you are sure -are specific to your credential, you may want to consider +are specific to your credential, you may want to consider removing them before user retrieval. A better solution is to -place those arguments that are specific to your credential +place those arguments that are specific to your credential within their own subhash named after your module. - + The L module does this -in order to encapsulate arguments intended specifically for +in order to encapsulate arguments intended specifically for that module. See the L source for details.