From 5a9192e41244f23904f73672229cdcf08e8ccac1 Mon Sep 17 00:00:00 2001
From: Guillaume Rousse <guillaume.rousse@renater.fr>
Date: Fri, 3 Nov 2017 12:13:01 +0100
Subject: [PATCH] use exceptions, and let caller handle logging
---
bin/account-manager-client.pl | 13 +++-
lib/IdPAccountManager/SAMLMetadata.pm | 103 ++++++--------------------
lib/IdPAccountManager/WebRequest.pm | 21 +++---
3 files changed, 44 insertions(+), 93 deletions(-)
diff --git a/bin/account-manager-client.pl b/bin/account-manager-client.pl
index aff71b4..da4bfd0 100755
--- a/bin/account-manager-client.pl
+++ b/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};
diff --git a/lib/IdPAccountManager/SAMLMetadata.pm b/lib/IdPAccountManager/SAMLMetadata.pm
index 0cf1884..de3a26f 100644
--- a/lib/IdPAccountManager/SAMLMetadata.pm
+++ b/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;
}
diff --git a/lib/IdPAccountManager/WebRequest.pm b/lib/IdPAccountManager/WebRequest.pm
index c0cdf03..0ec8686 100755
--- a/lib/IdPAccountManager/WebRequest.pm
+++ b/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;
}
--
GitLab