all lists on lists.proxmox.com
 help / color / mirror / Atom feed
* [pve-devel] [PATCH storage] cifs: fix adding storage without username in the GUI
@ 2021-07-05 18:51 Stoiko Ivanov
  2021-07-06  5:45 ` Thomas Lamprecht
  0 siblings, 1 reply; 2+ messages in thread
From: Stoiko Ivanov @ 2021-07-05 18:51 UTC (permalink / raw)
  To: pve-devel

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.

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}) {
 	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}) {
 	cifs_set_credentials($param{password}, $storeid);
 	if (!exists($scfg->{username})) {
 	    warn "ignoring password parameter\n";
-- 
2.30.2





^ permalink raw reply	[flat|nested] 2+ messages in thread

* Re: [pve-devel] [PATCH storage] cifs: fix adding storage without username in the GUI
  2021-07-05 18:51 [pve-devel] [PATCH storage] cifs: fix adding storage without username in the GUI Stoiko Ivanov
@ 2021-07-06  5:45 ` Thomas Lamprecht
  0 siblings, 0 replies; 2+ messages in thread
From: Thomas Lamprecht @ 2021-07-06  5:45 UTC (permalink / raw)
  To: Proxmox VE development discussion, Stoiko Ivanov

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;
 




^ permalink raw reply	[flat|nested] 2+ messages in thread

end of thread, other threads:[~2021-07-06  5:46 UTC | newest]

Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-05 18:51 [pve-devel] [PATCH storage] cifs: fix adding storage without username in the GUI Stoiko Ivanov
2021-07-06  5:45 ` Thomas Lamprecht

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal