use exceptions, and let caller handle logging

bin/account-manager-client.pl

@@ -7,6 +7,7 @@ use lib qw(lib);
 use feature "switch";
 no warnings 'experimental::smartmatch';
 
+use English qw(-no_match_vars);
 use Getopt::Long qw(:config auto_help);
 use Pod::Usage;
 
@@ -150,19 +151,23 @@ sub list_test_accounts {
 sub parse_federation_metadata {
     my $federation_metadata = IdPAccountManager::SAMLMetadata->new();
 
-    die "unable to load federation metadata\n"
-        unless $federation_metadata->load(
+    eval {
+        $federation_metadata->load(
             federation_metadata_file_path =>
               $configuration->{federation_metadata_file_path}
         );
+    };
+    die "unable to load federation metadata: $EVAL_ERROR" if $EVAL_ERROR;
 
     my %args;
     if ($options{sp_entityid}) {
         $args{filter_entity_id} = $options{sp_entityid};
     }
 
-    die "unable to parse federation metadata\n"
-        unless $federation_metadata->parse(%args);
+    eval {
+        $federation_metadata->parse(%args);
+    };
+    die "unable to parse federation metadata: $EVAL_ERROR\n" if $EVAL_ERROR;
 
     printf "Document %s parsed\n",
       $configuration->{federation_metadata_file_path};

lib/IdPAccountManager/SAMLMetadata.pm

@@ -7,12 +7,11 @@ use English qw(-no_match_vars);
 use XML::LibXML;
 
 use IdPAccountManager::Tools;
-use IdPAccountManager::Logger;
 
 sub new {
     my ($pkg, %args) = @_;
 
-    my $self = { logger => $args{logger} };
+    my $self = {};
 
     bless $self, $pkg;
 
@@ -23,47 +22,23 @@ sub new {
 sub load {
     my ($self, %args) = @_;
 
-    unless ($args{federation_metadata_file_path}) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Missing parameter 'federation_metadata_file_path'"
-        );
-        return undef;
-    }
+    die "Missing parameter 'federation_metadata_file_path'"
+        unless $args{federation_metadata_file_path};
 
     $self->{federation_metadata_file_path} =
       $args{federation_metadata_file_path};
 
-    unless (-r $self->{federation_metadata_file_path}) {
-        $self->{logger}->log(
-            level => LOG_ERROR,
-            message =>
-              "Failed to read $args{federation_metadata_file_path} : $ERRNO"
-        );
-        return undef;
-    }
+    die "Failed to read $args{federation_metadata_file_path}: $ERRNO"
+        unless -r $self->{federation_metadata_file_path};
 
-    unless ($self->{federation_metadata_as_xml} =
-        $self->_get_xml_object($args{federation_metadata_file_path}))
-    {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Failed to parse file $args{metadata_file} : $ERRNO"
-        );
-        return undef;
-    }
+    die "Failed to parse file $args{metadata_file}: $ERRNO"
+        unless $self->{federation_metadata_as_xml} =
+            $self->_get_xml_object($args{federation_metadata_file_path});
 
     my $root = $self->{federation_metadata_as_xml}->documentElement();
-    unless ($root->nodeName() =~ /EntitiesDescriptor$/) {
-        $self->{logger}->(
-            level   => LOG_ERROR,
-            message => sprintf(
-                "Root element of file $args{federation_metadata_file_path} is of type '%s'; should be 'EntitiesDescriptor'",
-                $root->nodeName()
-            )
-        );
-        return undef;
-    }
+
+    die "Root element of file $args{federation_metadata_file_path} is of type '%s'; should be 'EntitiesDescriptor'", $root->nodeName()
+        unless $root->nodeName() =~ /EntitiesDescriptor$/;
 
     return 1;
 }
@@ -83,13 +58,9 @@ sub parse {
 
     $self->{federation_metadata_as_hashref} =
       $self->_parse_saml_metadata(%parser_args);
-    unless (defined $self->{federation_metadata_as_hashref}) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Failed to parse federation metadata"
-        );
-        return undef;
-    }
+
+    die "Failed to parse federation metadata"
+        unless defined $self->{federation_metadata_as_hashref};
 
     return 1;
 }
@@ -107,51 +78,25 @@ sub print {
 sub _get_xml_object {
     my ($self, $metadata_file) = @_;
 
-    unless (-f $metadata_file) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "File $metadata_file not found: $ERRNO"
-        );
-        return undef;
-    }
+    die "File $metadata_file not found: $ERRNO"
+        unless -f $metadata_file;
 
-    unless (open FH, $metadata_file) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Failed to open file $metadata_file: $ERRNO"
-        );
-        return undef;
-    }
+    die "Failed to open file $metadata_file: $ERRNO"
+        unless open FH, $metadata_file;
 
-    my $parser;
-    unless ($parser = XML::LibXML->new()) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Failed to initialize XML parser"
-        );
-        return undef;
-    }
+    my $parser = XML::LibXML->new();
+    die "Failed to initialize XML parser" unless $parser;
 
     $parser->line_numbers(1);
     my $doc;
 
     ## Eval() prevents the parsing from killing the main process
     eval { $doc = $parser->parse_fh(\*FH) };
-    if ($EVAL_ERROR) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Failed to parse file $metadata_file : $EVAL_ERROR"
-        );
-        return undef;
-    }
+    die "Failed to parse file $metadata_file : $EVAL_ERROR"
+        if $EVAL_ERROR;
 
-    unless ($doc) {
-        $self->{logger}->log(
-            level   => LOG_ERROR,
-            message => "Failed to parse file $metadata_file : $ERRNO"
-        );
-        return undef;
-    }
+    die "Failed to parse file $metadata_file : $EVAL_ERROR"
+        unless $doc;
 
     return $doc;
 }

lib/IdPAccountManager/WebRequest.pm

@@ -231,30 +231,31 @@ sub req_account_wizard {
     my ($self) = @_;
     $self->{logger}->log(level => LOG_INFO, message => "");
 
-    my $federation_metadata = IdPAccountManager::SAMLMetadata->new(
-        logger => $self->{logger}
-    );
+    my $federation_metadata = IdPAccountManager::SAMLMetadata->new();
 
-    unless (
+    eval {
         $federation_metadata->load(
             federation_metadata_file_path =>
               $self->{configuration}->{federation_metadata_file_path}
-        )
-      )
-    {
+        );
+    };
+    if ($EVAL_ERROR) {
         push @{ $self->{param_out}->{errors} }, "internal";
         $self->{logger}->log(
             level   => LOG_ERROR,
-            message => "Failed to load federation metadata : $ERRNO"
+            message => "Failed to load federation metadata: $EVAL_ERROR"
         );
         return undef;
     }
 
-    unless ($federation_metadata->parse()) {
+    eval {
+        $federation_metadata->parse());
+    };
+    if ($EVAL_ERROR) {
         push @{ $self->{param_out}->{errors} }, "internal";
         $self->{logger}->log(
             level   => LOG_ERROR,
-            message => "Failed to parse federation metadata : $ERRNO"
+            message => "Failed to parse federation metadata: $EVAL_ERROR"
         );
         return undef;
     }