[pve-devel] [PATCH common v2] remove read_password_func from cli handler

Dominik Csapak d.csapak at proxmox.com
Wed Jun 13 10:16:01 CEST 2018


we have a param_mapping now, with which we can impement that

e.g. instead of using:

sub read_password {
    # read password
}

we can do:

sub param_mapping {
    my ($name) = @_;

    my $password_map = ['password', sub{
	# read password
    }, '<password', 1];

    my $mapping = {
	'apicall1' => [$password_map],
	'apicall2' => [$password_map],
	...
    };

    return $mapping->{$name};
}

this has the advantage that we can use different subs for different
api calls (e.g. pveum passwd vs pveum ticket)

Signed-off-by: Dominik Csapak <d.csapak at proxmox.com>
---
changes from v1:
* changed &$sub() to $sub->() where we touch lines
* removed if(mapdef && name eq password) check (unecessary because of code below it)
* fixed condiditional my statement
 src/PVE/CLIHandler.pm  | 18 ++++++++----------
 src/PVE/JSONSchema.pm  | 15 +--------------
 src/PVE/RESTHandler.pm | 32 +++++++++++++++-----------------
 3 files changed, 24 insertions(+), 41 deletions(-)

diff --git a/src/PVE/CLIHandler.pm b/src/PVE/CLIHandler.pm
index 9bbc156..cb51d92 100644
--- a/src/PVE/CLIHandler.pm
+++ b/src/PVE/CLIHandler.pm
@@ -127,7 +127,6 @@ sub generate_usage_str {
     $separator //= '';
     $indent //= '';
 
-    my $read_password_func = $cli_handler_class->can('read_password');
     my $param_mapping_func = $cli_handler_class->can('param_mapping') ||
 	$cli_handler_class->can('string_param_file_mapping');
 
@@ -150,7 +149,7 @@ sub generate_usage_str {
 		    $str .= $indent;
 		    $str .= $class->usage_str($name, "$prefix $cmd", $arg_param,
 		                              $fixed_param, $format,
-		                              $read_password_func, $param_mapping_func);
+		                              $param_mapping_func);
 		    $oldclass = $class;
 
 		} elsif (defined($def->{$cmd}->{alias}) && ($format eq 'asciidoc')) {
@@ -174,7 +173,7 @@ sub generate_usage_str {
 
 	    $str .= $indent;
 	    $str .= $class->usage_str($name, $prefix, $arg_param, $fixed_param, $format,
-	                              $read_password_func, $param_mapping_func);
+	                              $param_mapping_func);
 	}
 	return $str;
     };
@@ -466,7 +465,7 @@ sub setup_environment {
 }
 
 my $handle_cmd  = sub {
-    my ($args, $read_password_func, $preparefunc, $param_mapping_func) = @_;
+    my ($args, $preparefunc, $param_mapping_func) = @_;
 
     $cmddef->{help} = [ __PACKAGE__, 'help', ['extra-args'] ];
 
@@ -496,13 +495,13 @@ my $handle_cmd  = sub {
     my ($class, $name, $arg_param, $uri_param, $outsub) = @{$def || []};
     $abort->("unknown command '$cmd_str'") if !$class;
 
-    my $res = $class->cli_handler($cmd_str, $name, $cmd_args, $arg_param, $uri_param, $read_password_func, $param_mapping_func);
+    my $res = $class->cli_handler($cmd_str, $name, $cmd_args, $arg_param, $uri_param, $param_mapping_func);
 
     &$outsub($res) if $outsub;
 };
 
 my $handle_simple_cmd = sub {
-    my ($args, $read_password_func, $preparefunc, $param_mapping_func) = @_;
+    my ($args, $preparefunc, $param_mapping_func) = @_;
 
     my ($class, $name, $arg_param, $uri_param, $outsub) = @{$cmddef};
     die "no class specified" if !$class;
@@ -531,7 +530,7 @@ my $handle_simple_cmd = sub {
 
     &$preparefunc() if $preparefunc;
 
-    my $res = $class->cli_handler($name, $name, \@ARGV, $arg_param, $uri_param, $read_password_func, $param_mapping_func);
+    my $res = $class->cli_handler($name, $name, \@ARGV, $arg_param, $uri_param, $param_mapping_func);
 
     &$outsub($res) if $outsub;
 };
@@ -552,7 +551,6 @@ sub run_cli_handler {
 
     my $preparefunc = $params{prepare};
 
-    my $read_password_func = $class->can('read_password');
     my $param_mapping_func = $cli_handler_class->can('param_mapping') ||
 	$class->can('string_param_file_mapping');
 
@@ -564,9 +562,9 @@ sub run_cli_handler {
     $cmddef = ${"${class}::cmddef"};
 
     if (ref($cmddef) eq 'ARRAY') {
-	&$handle_simple_cmd(\@ARGV, $read_password_func, $preparefunc, $param_mapping_func);
+	$handle_simple_cmd->(\@ARGV, $preparefunc, $param_mapping_func);
     } else {
-	&$handle_cmd(\@ARGV, $read_password_func, $preparefunc, $param_mapping_func);
+	$handle_cmd->(\@ARGV, $preparefunc, $param_mapping_func);
     }
 
     exit 0;
diff --git a/src/PVE/JSONSchema.pm b/src/PVE/JSONSchema.pm
index f014dc3..1259195 100644
--- a/src/PVE/JSONSchema.pm
+++ b/src/PVE/JSONSchema.pm
@@ -1333,7 +1333,7 @@ sub method_get_child_link {
 # a way to parse command line parameters, using a 
 # schema to configure Getopt::Long
 sub get_options {
-    my ($schema, $args, $arg_param, $fixed_param, $pwcallback, $param_mapping_hash) = @_;
+    my ($schema, $args, $arg_param, $fixed_param, $param_mapping_hash) = @_;
 
     if (!$schema || !$schema->{properties}) {
 	raise("too many arguments\n", code => HTTP_BAD_REQUEST)
@@ -1362,11 +1362,6 @@ sub get_options {
 	    # optional and call the mapping function afterwards.
 	    push @getopt, "$prop:s";
 	    push @interactive, [$prop, $mapping->{func}];
-	} elsif ($prop eq 'password' && $pwcallback) {
-	    # we do not accept plain password on input line, instead
-	    # we turn this into a boolean option and ask for password below
-	    # using $pwcallback() (for security reasons).
-	    push @getopt, "$prop";
 	} elsif ($pd->{type} eq 'boolean') {
 	    push @getopt, "$prop:s";
 	} else {
@@ -1418,14 +1413,6 @@ sub get_options {
 	}
     }
 
-    if (my $pd = $schema->{properties}->{password}) {
-	if ($pd->{type} ne 'boolean' && $pwcallback) {
-	    if ($opts->{password} || !$pd->{optional}) {
-		$opts->{password} = &$pwcallback(); 
-	    }
-	}
-    }
-
     foreach my $entry (@interactive) {
 	my ($opt, $func) = @$entry;
 	my $pd = $schema->{properties}->{$opt};
diff --git a/src/PVE/RESTHandler.pm b/src/PVE/RESTHandler.pm
index 50c37c2..7732157 100644
--- a/src/PVE/RESTHandler.pm
+++ b/src/PVE/RESTHandler.pm
@@ -432,7 +432,7 @@ sub handle {
 # $style: 'config', 'config-sub', 'arg' or 'fixed'
 # $mapdef: parameter mapping ({ desc => XXX, func => sub {...} })
 my $get_property_description = sub {
-    my ($name, $style, $phash, $format, $hidepw, $mapdef) = @_;
+    my ($name, $style, $phash, $format, $mapdef) = @_;
 
     my $res = '';
 
@@ -449,10 +449,6 @@ my $get_property_description = sub {
 
     my $type_text = PVE::JSONSchema::schema_get_type_text($phash, $style);
 
-    if ($hidepw && $name eq 'password') {
-	$type_text = '';
-    }
-
     if ($mapdef && $phash->{type} eq 'string') {
 	$type_text = $mapdef->{desc};
     }
@@ -576,10 +572,9 @@ my $compute_param_mapping_hash = sub {
 #   'short'    ... command line only (text, one line)
 #   'full'     ... text, include description
 #   'asciidoc' ... generate asciidoc for man pages (like 'full')
-# $hidepw      ... hide password option (use this if you provide a read passwork callback)
 # $param_mapping_func ... mapping for string parameters to file path parameters
 sub usage_str {
-    my ($self, $name, $prefix, $arg_param, $fixed_param, $format, $hidepw, $param_mapping_func) = @_;
+    my ($self, $name, $prefix, $arg_param, $fixed_param, $format, $param_mapping_func) = @_;
 
     $format = 'long' if !$format;
 
@@ -612,7 +607,7 @@ sub usage_str {
     foreach my $k (@$arg_param) {
 	next if defined($fixed_param->{$k}); # just to be sure
 	next if !$prop->{$k}; # just to be sure
-	$argdescr .= &$get_property_description($k, 'fixed', $prop->{$k}, $format, 0);
+	$argdescr .= $get_property_description->($k, 'fixed', $prop->{$k}, $format);
     }
 
     my $idx_param = {}; # -vlan\d+ -scsi\d+
@@ -624,7 +619,12 @@ sub usage_str {
 
 	my $type_text = $prop->{$k}->{type} || 'string';
 
-	next if $hidepw && ($k eq 'password') && !$prop->{$k}->{optional};
+	my $param_mapping_hash = {};
+
+	if ($param_mapping_func) {
+	    $param_mapping_hash = $compute_param_mapping_hash->(&$param_mapping_func($name));
+	    next if $param_mapping_hash->{password} && ($k eq 'password') && !$prop->{$k}->{optional};
+	}
 
 	my $base = $k;
 	if ($k =~ m/^([a-z]+)(\d+)$/) {
@@ -636,11 +636,9 @@ sub usage_str {
 	    }
 	}
 
-	my $param_mapping_hash = $compute_param_mapping_hash->(&$param_mapping_func($name))
-	    if $param_mapping_func;
 
-	$opts .= &$get_property_description($base, 'arg', $prop->{$k}, $format,
-					    $hidepw, $param_mapping_hash->{$k});
+	$opts .= $get_property_description->($base, 'arg', $prop->{$k}, $format,
+					    $param_mapping_hash->{$k});
 
 	if (!$prop->{$k}->{optional}) {
 	    $args .= " " if $args;
@@ -703,7 +701,7 @@ sub dump_properties {
 	    }
 	}
 
-	$raw .= &$get_property_description($base, $style, $phash, $format, 0);
+	$raw .= $get_property_description->($base, $style, $phash, $format);
 
 	next if $style ne 'config';
 
@@ -736,14 +734,14 @@ my $replace_file_names_with_contents = sub {
 };
 
 sub cli_handler {
-    my ($self, $prefix, $name, $args, $arg_param, $fixed_param, $read_password_func, $param_mapping_func) = @_;
+    my ($self, $prefix, $name, $args, $arg_param, $fixed_param, $param_mapping_func) = @_;
 
     my $info = $self->map_method_by_name($name);
 
     my $res;
     eval {
 	my $param_mapping_hash = $compute_param_mapping_hash->($param_mapping_func->($name)) if $param_mapping_func;
-	my $param = PVE::JSONSchema::get_options($info->{parameters}, $args, $arg_param, $fixed_param, $read_password_func, $param_mapping_hash);
+	my $param = PVE::JSONSchema::get_options($info->{parameters}, $args, $arg_param, $fixed_param, $param_mapping_hash);
 
 	if (defined($param_mapping_hash)) {
 	    &$replace_file_names_with_contents($param, $param_mapping_hash);
@@ -756,7 +754,7 @@ sub cli_handler {
 
 	die $err if !$ec || $ec ne "PVE::Exception" || !$err->is_param_exc();
 	
-	$err->{usage} = $self->usage_str($name, $prefix, $arg_param, $fixed_param, 'short', $read_password_func, $param_mapping_func);
+	$err->{usage} = $self->usage_str($name, $prefix, $arg_param, $fixed_param, 'short',  $param_mapping_func);
 
 	die $err;
     }
-- 
2.11.0





More information about the pve-devel mailing list