Maintain a global cache of ACL verifiers in Wallet::ACL and reuse them

over the life of the process if we see another ACL line from the same
scheme, rather than only reusing ACL verifiers within a single ACL.

3 files changed, 40 insertions, 31 deletions

diff --git a/NEWS b/NEWS
index 327afcf..fa5e527 100644
--- a/NEWS
+++ b/NEWS
@@ -2,6 +2,10 @@
 
 wallet 0.4 (unreleased)
 
+    Maintain a global cache of ACL verifiers in Wallet::ACL and reuse them
+    over the life of the process if we see another ACL line from the same
+    scheme, rather than only reusing ACL verifiers within a single ACL.
+
     Add a subclass of the NetDB ACL verifier that requires the principal
     have an instance of "root" and strips that instance before checking
     NetDB roles.
diff --git a/TODO b/TODO
index fd8bf60..44ffb15 100644
--- a/TODO
+++ b/TODO
@@ -43,9 +43,6 @@ Release 1.0:
 * Log failures in the wallet-backend properly, which also requires
   catching all exceptions.
 
-* Implement default ACL policy to allow dynamic object creation on first
-  request for keytabs.
-
 * Implement special handling for keytabs in the wallet client.
 
 * Add support to the wallet client for getting Kerberos tickets, using the
@@ -170,9 +167,3 @@ May or may not be good ideas:
 
 * Remove the hard-coded ADMIN ACL in the server with something more
   configurable, perhaps a global ACL table or something.
-
-* The ACL implementation is currently rather inefficient for ACL
-  verifiers that need to maintain state (such as LDAP binds).  Now
-  they're created and destroyed when verifying each ACL.  It may be useful
-  to somehow return global verifiers, probably through a factory
-  implementation.  On the other hand, performance may not be worth it.
diff --git a/perl/Wallet/ACL.pm b/perl/Wallet/ACL.pm
index d654e68..3745074 100644
--- a/perl/Wallet/ACL.pm
+++ b/perl/Wallet/ACL.pm
@@ -349,15 +349,45 @@ sub history {
     return $output;
 }
 
+# Given a principal, a scheme, and an identifier, check whether that ACL
+# scheme and identifier grant access to that principal.  Return 1 if access
+# was granted, 0 if access was deined, and undef on some error.  On error, the
+# error message is also added to the check_errors variable.  This method is
+# internal to the class.
+#
+# Maintain ACL verifiers for all schemes we've seen in the local %verifier
+# hash so that we can optimize repeated ACL checks.
+{
+    my %verifier;
+    sub check_line {
+        my ($self, $principal, $scheme, $identifier) = @_;
+        unless ($verifier{$scheme}) {
+            my $class = $self->scheme_mapping ($scheme);
+            unless ($class) {
+                push (@{ $self->{check_errors} }, "unknown scheme $scheme");
+                return;
+            }
+            $verifier{$scheme} = $class->new;
+            unless (defined $verifier{$scheme}) {
+                push (@{ $self->{check_errors} }, "cannot verify $scheme");
+                return;
+            }
+        }
+        my $result = ($verifier{$scheme})->check ($principal, $identifier);
+        if (not defined $result) {
+            push (@{ $self->{check_errors} }, ($verifier{$scheme})->error);
+            return;
+        } else {
+            return $result;
+        }
+    }
+}
+
 # Given a principal, check whether it should be granted access according to
 # this ACL.  Returns 1 if access was granted, 0 if access was denied, and
 # undef on some error.  Errors from ACL verifiers do not cause an error
 # return, but are instead accumulated in the check_errors variable returned by
 # the check_errors() method.
-#
-# This routine is currently rather inefficient when it comes to instantiating
-# verifier objects.  They're created anew for each check.  Ideally, we should
-# globally cache verifiers in some way.
 sub check {
     my ($self, $principal) = @_;
     unless ($principal) {
@@ -370,24 +400,8 @@ sub check {
     $self->{check_errors} = [];
     for my $entry (@entries) {
         my ($scheme, $identifier) = @$entry;
-        unless ($verifier{$scheme}) {
-            my $class = $self->scheme_mapping ($scheme);
-            unless ($class) {
-                push (@{ $self->{check_errors} }, "unknown scheme $scheme");
-                next;
-            }
-            $verifier{$scheme} = $class->new;
-            unless (defined $verifier{$scheme}) {
-                push (@{ $self->{check_errors} }, "cannot verify $scheme");
-                next;
-            }
-        }
-        my $result = ($verifier{$scheme})->check ($principal, $identifier);
-        if (not defined $result) {
-            push (@{ $self->{check_errors} }, ($verifier{$scheme})->error);
-        } elsif ($result == 1) {
-            return 1;
-        }
+        my $result = $self->check_line ($principal, $scheme, $identifier);
+        return 1 if $result;
     }
     return 0;
 }