From fd4c685d1b30276365ef286bbf9afd0dbe75365a Mon Sep 17 00:00:00 2001
From: Guillaume Rousse <guillaume.rousse@renater.fr>
Date: Thu, 2 Nov 2017 11:39:56 +0100
Subject: [PATCH] more consistency in args passing

---
 lib/IdPAccountManager/AuthenticationToken.pm | 31 ++++++--------
 lib/IdPAccountManager/SAMLMetadata.pm        | 45 +++++++++-----------
 lib/IdPAccountManager/ServiceProvider.pm     | 11 +++--
 lib/IdPAccountManager/TestAccount.pm         | 24 +++++------
 lib/IdPAccountManager/Tools.pm               | 27 ++++++------
 lib/IdPAccountManager/WebRequest.pm          | 18 ++++----
 6 files changed, 72 insertions(+), 84 deletions(-)

diff --git a/lib/IdPAccountManager/AuthenticationToken.pm b/lib/IdPAccountManager/AuthenticationToken.pm
index 7660661..8155904 100644
--- a/lib/IdPAccountManager/AuthenticationToken.pm
+++ b/lib/IdPAccountManager/AuthenticationToken.pm
@@ -21,8 +21,7 @@ INIT {
 }
 
 sub new {
-    my ($pkg) = shift;
-    my %args = @_;
+    my ($pkg, %args) = @_;
 
     my $self = {};
 
@@ -42,26 +41,24 @@ sub new {
 
 ## Load an authentication token from DB
 sub load {
-    my $self = shift;
+    my ($self) = @_;
 
     return $self->{'persistent'}->load(speculative => 1);
 }
 
 ## Get object parameter
 sub get {
-    my $self           = shift;
-    my $attribute_name = shift;
+    my ($self, $parameter) = @_;
 
-    return $self->{'persistent'}->$attribute_name;
+    return $self->{'persistent'}->$parameter;
 }
 
 ## Set object parameters
 sub set {
-    my $self       = shift;
-    my %parameters = @_;
+    my ($self, %args) = @_;
 
-    foreach my $parameter_name (keys %parameters) {
-        $self->{'persistent'}->$parameter_name($parameters{$parameter_name});
+    foreach my $parameter (keys %args) {
+        $self->{'persistent'}->$parameter($args{$parameter});
     }
 
     return 1;
@@ -69,7 +66,7 @@ sub set {
 
 ## Save object to DB
 sub save {
-    my $self = shift;
+    my ($self) = @_;
 
     ## If no id is defined, it is a new account
     unless (defined $self->{'persistent'}->id) {
@@ -85,7 +82,7 @@ sub save {
 
 ## Delete a test account
 sub delete {
-    my $self = shift;
+    my ($self) = @_;
 
     unless ($self->{'persistent'}->delete()) {
         return undef;
@@ -94,8 +91,8 @@ sub delete {
 
 ## Print the content of a test account
 sub print {
-    my $self = shift;
-    my $fd = shift || \*STDOUT;
+    my ($self, $fd) = @_;
+    $fd = \*STDOUT unless $fd;
 
     printf $fd
 "AuthenticationToken ID=%s; token=%s; email_address=%s; sp_entityid=%s; creation_date=%s\n",
@@ -109,7 +106,7 @@ sub print {
 ## list all authentication tokens
 ## Class method
 sub list_authentication_tokens {
-    my %args = @_;
+    my (%args) = @_;
 
     my $persistent_tokens =
       IdPAccountManager::Data::AuthenticationToken::Manager
@@ -126,8 +123,8 @@ sub list_authentication_tokens {
 
 ## generate a random authentication token
 sub _generate_token {
-    my $salt = shift;
-    my $size = shift || 20;
+    my ($salt, $size) = @_;
+    $size = 20 unless $size;
 
     ## ID is based on time + PID
     return substr(Digest::MD5::md5_hex(time . $$ . $salt), -1 * $size);
diff --git a/lib/IdPAccountManager/SAMLMetadata.pm b/lib/IdPAccountManager/SAMLMetadata.pm
index ffb7a84..db8e00b 100644
--- a/lib/IdPAccountManager/SAMLMetadata.pm
+++ b/lib/IdPAccountManager/SAMLMetadata.pm
@@ -16,8 +16,7 @@ use XML::LibXML;
 use Carp;
 
 sub new {
-    my ($pkg) = shift;
-    my %args = @_;
+    my ($pkg, %args) = @_;
 
     my $self = { logger => $args{logger} };
 
@@ -28,10 +27,9 @@ sub new {
 
 ## Load metadata
 sub load {
-    my $self = shift;
-    my %in   = @_;
+    my ($self, %args) = @_;
 
-    unless ($in{'federation_metadata_file_path'}) {
+    unless ($args{'federation_metadata_file_path'}) {
         $self->{logger}->log(
             level   => LOG_ERROR,
             message => "Missing parameter 'federation_metadata_file_path'"
@@ -40,23 +38,23 @@ sub load {
     }
 
     $self->{'federation_metadata_file_path'} =
-      $in{'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 $in{'federation_metadata_file_path'} : $ERRNO"
+              "Failed to read $args{'federation_metadata_file_path'} : $ERRNO"
         );
         return undef;
     }
 
     unless ($self->{'federation_metadata_as_xml'} =
-        $self->_get_xml_object($in{'federation_metadata_file_path'}))
+        $self->_get_xml_object($args{'federation_metadata_file_path'}))
     {
         $self->{logger}->log(
             level   => LOG_ERROR,
-            message => "Failed to parse file $in{'metadata_file'} : $ERRNO"
+            message => "Failed to parse file $args{'metadata_file'} : $ERRNO"
         );
         return undef;
     }
@@ -66,7 +64,7 @@ sub load {
         $self->{logger}->(
             level   => LOG_ERROR,
             message => sprintf(
-                "Root element of file $in{'federation_metadata_file_path'} is of type '%s'; should be 'EntitiesDescriptor'",
+                "Root element of file $args{'federation_metadata_file_path'} is of type '%s'; should be 'EntitiesDescriptor'",
                 $root->nodeName()
             )
         );
@@ -78,16 +76,15 @@ sub load {
 
 ## Parse XML structure of metadata to fill a hashref
 sub parse {
-    my $self    = shift;
-    my %options = @_;
+    my ($self, %args) = @_;
 
     my %parser_args = (
         'metadata_as_xml'    => $self->{'federation_metadata_as_xml'},
         'filter_entity_type' => 'sp'
     );
 
-    if ($options{'filter_entity_id'}) {
-        $parser_args{'filter_entity_id'} = $options{'filter_entity_id'};
+    if ($args{'filter_entity_id'}) {
+        $parser_args{'filter_entity_id'} = $args{'filter_entity_id'};
     }
 
     $self->{'federation_metadata_as_hashref'} =
@@ -105,8 +102,8 @@ sub parse {
 
 ## Dumps the SAML metadata content
 sub print {
-    my $self = shift;
-    my $fd = shift || \*STDOUT;
+    my ($self, $fd) = @_;
+    $fd = \*STDOUT unless $fd;
 
     my $root = $self->{'federation_metadata_as_xml'}->documentElement();
     print $fd $root->toString();
@@ -116,8 +113,7 @@ sub print {
 
 ## returns a Lib::XML représenting an XML file
 sub _get_xml_object {
-    my $self = shift;
-    my $metadata_file = shift;
+    my ($self, $metadata_file) = @_;
     $self->{logger}->log(level => LOG_DEBUG, message => "");
 
     unless (-f $metadata_file) {
@@ -171,10 +167,9 @@ sub _get_xml_object {
 
 ## Parse a SAML federation metadata file
 sub _parse_saml_metadata {
-    my $self = shift;
-    my %options = @_;
+    my ($self, %args) = @_;
 
-    my $root = $options{'metadata_as_xml'};
+    my $root = $args{'metadata_as_xml'};
 
     my @extracted_array;
     foreach my $EntityDescriptor (
@@ -190,8 +185,8 @@ sub _parse_saml_metadata {
         }
 
         next
-          if ($options{'filter_entity_id'}
-            && ($options{'filter_entity_id'} ne $extracted_data->{'entityid'}));
+          if ($args{'filter_entity_id'}
+            && ($args{'filter_entity_id'} ne $extracted_data->{'entityid'}));
 
         $extracted_data->{'xml_md'} =
           IdPAccountManager::Tools::escape_xml($EntityDescriptor->toString());
@@ -394,8 +389,8 @@ sub _parse_saml_metadata {
 
         ## Filter entities based  on type
         next
-          if (defined $options{'filter_entity_type'}
-            && ($options{'filter_entity_type'} ne $extracted_data->{'type'}));
+          if (defined $args{'filter_entity_type'}
+            && ($args{'filter_entity_type'} ne $extracted_data->{'type'}));
 
         ## Merge domains in a single string
         my $domains = join(',', @{ $extracted_data->{'domain'} })
diff --git a/lib/IdPAccountManager/ServiceProvider.pm b/lib/IdPAccountManager/ServiceProvider.pm
index aafc696..21f8e92 100644
--- a/lib/IdPAccountManager/ServiceProvider.pm
+++ b/lib/IdPAccountManager/ServiceProvider.pm
@@ -24,8 +24,8 @@ INIT {
 
 ## Print the content of a test account
 sub print {
-    my $self = shift;
-    my $fd = shift || \*STDOUT;
+    my ($self, $fd) = @_;
+    $fd = \*STDOUT unless $fd;
 
     printf $fd
       "ServiceProvider ID=%s; entityid=%s; displayname=%s; contacts=%s\n",
@@ -36,7 +36,7 @@ sub print {
 
 ## list contacts for this SP, including those listed in conf.dev_sp_contact
 sub list_contacts_as_array {
-    my $self = shift;
+    my ($self) = @_;
 
     my %contact_list;
 
@@ -53,8 +53,7 @@ sub list_contacts_as_array {
 
 ## Check if email address is a known contact (or conf.dev_sp_contact)
 sub is_contact {
-    my $self  = shift;
-    my $email = shift;
+    my ($self, $email) = @_;
 
     foreach my $known_contact ($self->list_contacts_as_array()) {
         return 1 if (lc($email) eq lc($known_contact));
@@ -66,7 +65,7 @@ sub is_contact {
 ## list all test accounts
 ## Class method
 sub list_service_providers {
-    my %args = @_;
+    my (%args) = @_;
 
     my $persistent_accounts =
       IdPAccountManager::Data::ServiceProvider::Manager->get_serviceproviders(
diff --git a/lib/IdPAccountManager/TestAccount.pm b/lib/IdPAccountManager/TestAccount.pm
index a586a98..9406302 100644
--- a/lib/IdPAccountManager/TestAccount.pm
+++ b/lib/IdPAccountManager/TestAccount.pm
@@ -22,8 +22,7 @@ INIT {
 }
 
 sub new {
-    my ($pkg) = shift;
-    my %args = @_;
+    my ($pkg, %args) = @_;
 
     my $self = {};
 
@@ -42,20 +41,19 @@ sub new {
 }
 
 sub get {
-    my $self           = shift;
-    my $attribute_name = shift;
+    my ($self, $attribute) = @_;
 
     ## User password is not stored in DB
-    if ($attribute_name eq 'user_password') {
-        return $self->{$attribute_name};
+    if ($attribute eq 'user_password') {
+        return $self->{$attribute};
     } else {
-        return $self->{'persistent'}->$attribute_name;
+        return $self->{'persistent'}->$attribute;
 
     }
 }
 
 sub save {
-    my $self = shift;
+    my ($self) = @_;
 
     ## If no id is defined, it is a new account
     unless (defined $self->{'persistent'}->id) {
@@ -75,7 +73,7 @@ sub save {
 
 ## Delete a test account
 sub delete {
-    my $self = shift;
+    my ($self) = @_;
 
     unless ($self->{'persistent'}->delete()) {
         return undef;
@@ -84,8 +82,8 @@ sub delete {
 
 ## Print the content of a test account
 sub print {
-    my $self = shift;
-    my $fd = shift || \*STDOUT;
+    my ($self, $fd) = @_;
+    $fd = \*STDOUT unless $fd;
 
     printf $fd
 "Account ID=%s; password_hash=%s; sp_entityid=%s; account_profile=%s; creation_date=%s; expiration_date=%s\n",
@@ -100,7 +98,7 @@ sub print {
 ## list all test accounts
 ## Class method
 sub list_test_accounts {
-    my %args = @_;
+    my (%args) = @_;
 
     my $persistent_accounts =
       IdPAccountManager::Data::TestAccount::Manager->get_testaccounts(%args);
@@ -115,7 +113,7 @@ sub list_test_accounts {
 
 ## create test accounts for all active account profiles
 sub create_test_accounts_for_sp {
-    my %args = @_;
+    my (%args) = @_;
     my @test_accounts;
 
     unless ($args{'sp_entityid'}) {
diff --git a/lib/IdPAccountManager/Tools.pm b/lib/IdPAccountManager/Tools.pm
index 477ed34..dd718ba 100644
--- a/lib/IdPAccountManager/Tools.pm
+++ b/lib/IdPAccountManager/Tools.pm
@@ -16,15 +16,13 @@ use Encode;
 INIT {
     ## a TT2 virtual method to get a variable type
     $Template::Stash::LIST_OPS->{isa} = sub {
-        my $list = shift;
-        my $type = shift;
+        my ($list, $type) = @_;
 
         return 1 if ($type eq 'ARRAY');
         return 0;
     };
     $Template::Stash::SCALAR_OPS->{isa} = sub {
-        my $list = shift;
-        my $type = shift;
+        my ($list, $type) = @_;
 
         return 1 if ($type eq 'SCALAR');
         return 0;
@@ -33,7 +31,7 @@ INIT {
 
 # get SHA256 hash for a string
 sub sha256_hash {
-    my $s = shift;
+    my ($s) = @_;
 
     return Digest::SHA::sha256_base64($s);
 }
@@ -150,10 +148,10 @@ sub dump_var {
 ##   template : mail template file
 ##   data : data used by the TT2 parser
 sub mail_notice {
-    my %in        = @_;
-    my $tt2_file  = $in{'template'};
-    my $mail_data = $in{'data'};
-    my $logger    = $in{'logger'};
+    my (%args) = @_;
+    my $tt2_file  = $args{'template'};
+    my $mail_data = $args{'data'};
+    my $logger    = $args{'logger'};
 
     $mail_data->{'conf'} ||= \%Conf::global;
 
@@ -212,7 +210,7 @@ sub mail_notice {
 }
 
 sub qencode {
-    my $string = shift;
+    my ($string) = @_;
 
     # We are not able to determine the name of header field, so assume
     # longest (maybe) one.
@@ -226,14 +224,15 @@ sub qencode {
 
 ## usefull to pass parameters to TT2
 sub encode_utf8 ($) {
-    my $string = shift || '';
+    my ($string) = @_;
+    $string = '' unless $string;
 
     return Encode::encode('utf8', $string);
 }
 
 ## Escape characters that may interfer in an XML document
 sub escape_xml {
-    my $s = shift;
+    my ($s) = @_;
 
     $s =~ s/\&/&amp\;/gm;
     $s =~ s/\"/&quot\;/gm;
@@ -246,7 +245,7 @@ sub escape_xml {
 
 ## usefull to pass parameters to TT2
 sub escape_quotes {
-    my $string = shift;
+    my ($string) = @_;
 
     $string =~ s/\'/\\\'/g;
 
@@ -255,7 +254,7 @@ sub escape_quotes {
 
 ## returns an integer (0 or 1), given an input string ('true' or 'false')
 sub boolean2integer {
-    my $boolean = shift;
+    my ($boolean) = @_;
 
     if ($boolean eq 'true') {
         return 1;
diff --git a/lib/IdPAccountManager/WebRequest.pm b/lib/IdPAccountManager/WebRequest.pm
index 0666cb9..a4261e4 100755
--- a/lib/IdPAccountManager/WebRequest.pm
+++ b/lib/IdPAccountManager/WebRequest.pm
@@ -9,8 +9,8 @@ use Conf;
 
 ## New web request
 sub new {
-    my $pkg     = shift;
-    my %args    = shift;
+    my ($pkg, %args) = @_;
+
     my $self = {
         format  => $args{format},
         actions => $args{actions},
@@ -79,7 +79,7 @@ sub new {
 
 ## Execute a web request
 sub execute {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_DEBUG, message => "");
 
     my $status;
@@ -129,7 +129,7 @@ sub execute {
 
 ## Return HTML content
 sub respond {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_DEBUG, message => "");
 
     ## Automatic pass object entries to the output hash
@@ -205,7 +205,7 @@ sub respond {
 
 ## Return the list of known SPs first
 sub req_account_wizard {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_INFO, message => "");
 
     my $federation_metadata = IdPAccountManager::SAMLMetadata->new(
@@ -245,7 +245,7 @@ sub req_account_wizard {
 ## Select a Service Provider and return metadata sctucture for the SP
 ## Sample URL : https://dev-edugain.renater.fr/accountmanager?action=select_sp&sp_entityid=http%3A%2F%2Fsp.lat.csc.fi
 sub req_select_sp {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_INFO, message => "");
 
     unless ($self->{'param_in'}{'sp_entityid'}) {
@@ -361,7 +361,7 @@ sub req_select_sp {
 ## Generate an authentication token to validate an email address
 ## Sample call : dev-edugain.renater.fr/accountmanager?action=generate_token&style=nobanner&sp_entityid=https%3A%2F%2Fsourcesup.cru.fr%2Fshibboleth&email_address=support%40renater.fr
 sub req_generate_token {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_INFO, message => "");
 
     unless ($self->{'param_in'}{'sp_entityid'}) {
@@ -489,7 +489,7 @@ sub req_generate_token {
 ## Test accounts get created
 ## Sample call : dev-edugain.renater.fr/accountmanager?action=validate_token&style=nobanner&sp_entityid=https%3A%2F%2Fsourcesup.cru.fr%2Fshibboleth&authentication_token=c1cfecb51ea40d39a695
 sub req_validate_token {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_INFO, message => "");
 
     unless ($self->{'param_in'}{'sp_entityid'}) {
@@ -587,7 +587,7 @@ sub req_validate_token {
 
 ## Return the homepage of the service
 sub req_home {
-    my $self = shift;
+    my ($self) = @_;
     $self->{logger}->log(level => LOG_INFO, message => "");
 
     return 1;
-- 
GitLab