Re: [pve-devel] [PATCH storage] cifs: fix adding storage without username in the GUI

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

On 05.07.21 20:51, Stoiko Ivanov wrote:
> Currently when trying to add a CIFS storage, without providing a
> username and passwort it fails while trying to mount the share, due to
> the provided 'username=' parameter (w/o actual username).
> 
> I think the issue was introduced in
> 72385de9e23df9f8e438d74ff783a8075f8d1560
> with the extracting of sensitive parameters we (rightly) switched to a
> definedness check instead of one for truthyness - but an empty
> username/password is defined.

so it either did not switch "rightly" over or you switched it right back by mistake..

> 
> Signed-off-by: Stoiko Ivanov <s.ivanov@proxmox.com>
> ---
>  PVE/Storage/CIFSPlugin.pm | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/PVE/Storage/CIFSPlugin.pm b/PVE/Storage/CIFSPlugin.pm
> index 9d69b01..f8e173d 100644
> --- a/PVE/Storage/CIFSPlugin.pm
> +++ b/PVE/Storage/CIFSPlugin.pm
> @@ -157,7 +157,7 @@ sub check_config {
>  sub on_add_hook {
>      my ($class, $storeid, $scfg, %param) = @_;
>  
> -    if (defined($param{password})) {
> +    if (defined($param{password}) && $param{password}) {

defined && truthiness check of the same variable is effectively just the truthiness
check, as there can never be the case where `$param{password}` would evaluate to true
but `defined($param{password}) would not.

So this makes it again wrong, as it also skips values like "0".

If, you'd need to check the length($param{password}) == 0, but I do not really see
the point here, that would be normally the job of the API and a result should be
a parameter exception.


>  	cifs_set_credentials($param{password}, $storeid);
>  	if (!exists($scfg->{username})) {
>  	    warn "ignoring password parameter\n";
> @@ -174,7 +174,7 @@ sub on_update_hook {
>  
>      return if !exists($param{password});
>  
> -    if (defined($param{password})) {
> +    if (defined($param{password}) && $param{password}) {

same here, you effectively transformed it back to `if ($param{password}) {`

>  	cifs_set_credentials($param{password}, $storeid);
>  	if (!exists($scfg->{username})) {
>  	    warn "ignoring password parameter\n";
> 

The backend wasn't really the problem here, the front end sent empty strings
when it should not, so I replaced your patch with the following in manage:

----8<----
diff --git a/www/manager6/storage/CIFSEdit.js b/www/manager6/storage/CIFSEdit.js
index 3992c477..71415401 100644
--- a/www/manager6/storage/CIFSEdit.js
+++ b/www/manager6/storage/CIFSEdit.js
@@ -120,6 +120,19 @@ Ext.define('PVE.storage.CIFSInputPanel', {
 
     onlineHelp: 'storage_cifs',
 
+    onGetValues: function(values) {
+       let me = this;
+
+       if (values.password?.length === 0) {
+           delete values.password;
+       }
+       if (values.username?.length === 0) {
+           delete values.username;
+       }
+
+       return me.callParent([values]);
+    },
+
     initComponent: function() {
        var me = this;