Created attachment 293541 [details] [review]
smb: Handle the anonymous flag when calling AskPassword

Previously, the smb backend would try logging in anonymously first if a
user is not specified making it difficult to mount a share as a specific
user from Nautilus.  Instead, only try an anonymous login if the
anonymous flag is TRUE after calling AskPassword.

Created attachment 293542 [details] [review]
gvfs-mount: Allow mounting as an anonymous user

The attached patches should fix the problem. If the patch is OK, I'll do the same for the smb-browse backend which has the same authentication code.

This has got to be the fastest turnaround time ever, thanks. I'm about to test the patches using gvfs-mount, should I also test using nautilus? What's the difference between the smb and smbbrowse backends?

Ok, I cherry-picked the patches against 1.20.2 and rebuilt the gvfs* ubuntu packages, and.... it's awesome!

Tests:
1] share requires authentication:
nautilus: offers to connect as anonymous user (which fails).
gvfs-mount (-a: without): requires credentials. prompts until valid credentials given or ^C.
gvfs-mount (-a: with): infinite loop of "Password required for share downloads on sangria".

2] share requires guest access (only)
nautilus: offers to connect as registered user. The again, despite clearly being a guest-only share (guest only = yes), samba would nonetheless allow access. So, meh.
gvfs-mount (-a: without): requires credentials (doesn't detect the share is guest only). But as with nautilus, valid credentials are accepted. So, once again, meh.
gvfs-mount (-a: with): mounts without requiring any input. However does output the following message: "Password required for share ... on ...".

3] share allows both (guest or authenticated)
nautilus: offers to connect as anonymous or registered. "If forget password immediately" is selected, nautilus will prompt again (as expected) on remount.
gvfs-mount (-a: without): requires credentials (doesnt detect the share accepts anonymous access).
gvfs-mount (-a: with): mounts without requiring any input. However does output the following message: "Password required for share ... on ...".


Additional Problems:
1] If the authentication fails and the password-input dialog is spawned too many times, it will be displayed with all input elements permanently disabled (greyed-out), so the user would have to select cancel and restart the process. However I'm pretty sure that it's unrelated to this bug report or the smb backend. 

2] If the password-input dialog is cancelled, it will be redisplayed once, ie:
 * accidentally select incorrect share
 * password-input dialog appears
 * select cancel
 * password-input dialog appears
 * select cancel (after some short-lived confusion)
 * select correct share

It is possible either of these problems may have been fixed in more recent releases.


Summary:
I don't know if all the behaviour I demonstrated in the tests above is intended, or even if it is possible for the smb backend to behave any other way. There are definitely at least two miniature bugs (unrelated output message and infinite loop), though in any case the extra functionality is great.

(In reply to comment #4)
> This has got to be the fastest turnaround time ever, thanks. I'm about to test
> the patches using gvfs-mount, should I also test using nautilus? What's the
> difference between the smb and smbbrowse backends?

The smb-browse backend lists servers and share, the smb backend is for accessing a particular share.

(In reply to comment #5)
> Ok, I cherry-picked the patches against 1.20.2 and rebuilt the gvfs* ubuntu
> packages, and.... it's awesome!
> 
> Tests:
> 1] share requires authentication:
> nautilus: offers to connect as anonymous user (which fails).
> gvfs-mount (-a: without): requires credentials. prompts until valid credentials
> given or ^C.
> gvfs-mount (-a: with): infinite loop of "Password required for share downloads
> on sangria".
> 
> 2] share requires guest access (only)
> nautilus: offers to connect as registered user. The again, despite clearly
> being a guest-only share (guest only = yes), samba would nonetheless allow
> access. So, meh.
> gvfs-mount (-a: without): requires credentials (doesn't detect the share is
> guest only). But as with nautilus, valid credentials are accepted. So, once
> again, meh.
> gvfs-mount (-a: with): mounts without requiring any input. However does output
> the following message: "Password required for share ... on ...".
> 
> 3] share allows both (guest or authenticated)
> nautilus: offers to connect as anonymous or registered. "If forget password
> immediately" is selected, nautilus will prompt again (as expected) on remount.
> gvfs-mount (-a: without): requires credentials (doesnt detect the share accepts
> anonymous access).
> gvfs-mount (-a: with): mounts without requiring any input. However does output
> the following message: "Password required for share ... on ...".
> 
> 
> Additional Problems:
> 1] If the authentication fails and the password-input dialog is spawned too
> many times, it will be displayed with all input elements permanently disabled
> (greyed-out), so the user would have to select cancel and restart the process.
> However I'm pretty sure that it's unrelated to this bug report or the smb
> backend. 

This is probably unrelated to gvfs.

> 
> 2] If the password-input dialog is cancelled, it will be redisplayed once, ie:
>  * accidentally select incorrect share
>  * password-input dialog appears
>  * select cancel
>  * password-input dialog appears
>  * select cancel (after some short-lived confusion)
>  * select correct share

This is probably a Nautilus bug that's been fixed, I can't reproduce it.

> 
> It is possible either of these problems may have been fixed in more recent
> releases.
> 
> 
> Summary:
> I don't know if all the behaviour I demonstrated in the tests above is
> intended, or even if it is possible for the smb backend to behave any other
> way. There are definitely at least two miniature bugs (unrelated output message
> and infinite loop), though in any case the extra functionality is great.

I will attach an updated patch that fixes these two bugs. Apart from the two unrelated problems above, everything else is as expected.  I don't think it is possible to detect whether a share supports anonymous access or authenticated access so both types are always offered.

Created attachment 293599 [details] [review]
gvfs-mount: Allow mounting as an anonymous user

Looks good.

The error message when attempting to mount a share requiring authentication as an anonymous user with gvfs-mount is slightly misleading:
"Error mounting location: Password dialog cancelled"

But I have a feeling the work required to fix that is too involved with far-reaching implications that would need extensive testing. IMO, not worth it.

(In reply to comment #0)
> The real problem:
> Well, I can work around around this by specifying a username
> gvfs-mount smb://username@server/share
> 
> The real problem is there is no equivalent workaround for GUI (nautilus)

You can also specify smb://username@server/share uri in "Connect to server" dialog in Nautilus. It works for me...

(In reply to comment #8)
> Looks good.
> 
> The error message when attempting to mount a share requiring authentication as
> an anonymous user with gvfs-mount is slightly misleading:
> "Error mounting location: Password dialog cancelled"
> 
> But I have a feeling the work required to fix that is too involved with
> far-reaching implications that would need extensive testing. IMO, not worth it.

We should to improve this case, when anonymous isn't allowed and we want it, because it is broken in Nautilus also. The dialog isn't sensitive after clicking to "Connect" and we can't select "Registered user" instead... Not sure what should happen with the dialog in case of failure, because I have seen this dialog first time, maybe it needs some fixes in gtk or where the dialog lives... I think the dialog should inform the user that anonymous login failed and that he should try password login instead...

(In reply to comment #9)
> (In reply to comment #0)
> > The real problem:
> > Well, I can work around around this by specifying a username
> > gvfs-mount smb://username@server/share
> > 
> > The real problem is there is no equivalent workaround for GUI (nautilus)
> 
> You can also specify smb://username@server/share uri in "Connect to server"
> dialog in Nautilus. It works for me...

The issue is when accessing a share via "Browse Network".

Created attachment 294045 [details] [review]
gvfs-mount: Allow mounting as an anonymous user

(In reply to comment #10)
> (In reply to comment #8)
> > Looks good.
> > 
> > The error message when attempting to mount a share requiring authentication as
> > an anonymous user with gvfs-mount is slightly misleading:
> > "Error mounting location: Password dialog cancelled"
> > 
> > But I have a feeling the work required to fix that is too involved with
> > far-reaching implications that would need extensive testing. IMO, not worth it.

The updated patch fixes this issue.

> 
> We should to improve this case, when anonymous isn't allowed and we want it,
> because it is broken in Nautilus also. The dialog isn't sensitive after
> clicking to "Connect" and we can't select "Registered user" instead... Not sure
> what should happen with the dialog in case of failure, because I have seen this
> dialog first time, maybe it needs some fixes in gtk or where the dialog
> lives... I think the dialog should inform the user that anonymous login failed
> and that he should try password login instead...

The code is in gtk+/gtk/gtkmountoperation.c

Yeah, perhaps we can get these patches committed and then assign the bug to gtk?

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #0)
> > > The real problem:
> > > Well, I can work around around this by specifying a username
> > > gvfs-mount smb://username@server/share
> > > 
> > > The real problem is there is no equivalent workaround for GUI (nautilus)
> > 
> > You can also specify smb://username@server/share uri in "Connect to server"
> > dialog in Nautilus. It works for me...
> 
> The issue is when accessing a share via "Browse Network".

i.e. You can't access a share as a registered in user when accessing it via Browse Network which would be the typical way of accessing a share, particularly if you're from a Windows background. Typing out the URI with an embedded username is a workaround which would not be clear to most people using Nautilus/gvfs.

(In reply to comment #11)
> The issue is when accessing a share via "Browse Network".

Aha, I'm not used to use this tool, but you are right...

(In reply to comment #13)
> The updated patch fixes this issue.

The changes of smb backend are done similarly as it is in ftp backend, however shouldn't be this fix done in the backend directly (also for ftp backend, maybe afp)? We should to return correct error message from the backend...

> Yeah, perhaps we can get these patches committed and then assign the bug to
> gtk?

We can do this since it is broken longer time for ftp probably...

(In reply to comment #15)
> (In reply to comment #11)
> > The issue is when accessing a share via "Browse Network".
> 
> Aha, I'm not used to use this tool, but you are right...
> 
> (In reply to comment #13)
> > The updated patch fixes this issue.
> 
> The changes of smb backend are done similarly as it is in ftp backend, however
> shouldn't be this fix done in the backend directly (also for ftp backend, maybe
> afp)? We should to return correct error message from the backend...
> 

All the backends continue to prompt the user for a password until it is correct or the user aborts... I believe this is the expected behavior. Changing it would mean if you get the password wrong, you have to restart the mount operation which would involve the user starting a new mount operation, reconnecting to the server, etc.

If anonymous access is requested at the gvfs-mount command-line, the mount operation is aborted after the first attempt fails to prevent an endless loop in gvfs-mount. Therefore special handling for the error message is needed.

Right, I didn't notice before gvfs-mount aborts the dialog, therefore backend returns correct error message...

Review of attachment 294045 [details] [review]:

Thanks, looks good! Just an idea...

::: programs/gvfs-mount.c
@@ +43,3 @@
+  MOUNT_OP_ASKED,
+  MOUNT_OP_ABORTED
+} MountOpState;

Isn't this too complicated? Maybe I'd rather to see global variable like:
static gboolean aborted = FALSE;

@@ +172,3 @@
     }
 
+  /* Only try anonymous access once. */

static gboolean asked = FALSE;

@@ +174,3 @@
+  /* Only try anonymous access once. */
+  if (anonymous &&
+      GPOINTER_TO_INT (g_object_get_data (G_OBJECT (op), "state")) == MOUNT_OP_ASKED)

anonymous && asked

@@ +181,3 @@
+  else
+    {
+      g_object_set_data (G_OBJECT (op), "state", GINT_TO_POINTER (MOUNT_OP_ASKED));

asked = TRUE

@@ +230,2 @@
       success = FALSE;
+      if (GPOINTER_TO_INT (g_object_get_data (G_OBJECT (op), "state")) == MOUNT_OP_ABORTED)

anonymous && aborted

Review of attachment 293541 [details] [review]:

Looks good!

::: daemon/gvfsbackendsmb.c
@@ +264,3 @@
 	    flags |= G_ASK_PASSWORD_NEED_USERNAME;
+          if (backend->user == NULL && backend->domain == NULL)
+	    flags |= G_ASK_PASSWORD_ANONYMOUS_SUPPORTED;

JFYI: If you unset the flag after unsuccessful anonymous login, the dialog in nautilus works properly. Maybe it would be good workaround...

(In reply to comment #18)
> Review of attachment 294045 [details] [review]:
> 
> Thanks, looks good! Just an idea...
> 
> ::: programs/gvfs-mount.c
> @@ +43,3 @@
> +  MOUNT_OP_ASKED,
> +  MOUNT_OP_ABORTED
> +} MountOpState;
> 
> Isn't this too complicated? Maybe I'd rather to see global variable like:
> static gboolean aborted = FALSE;
> 
> @@ +172,3 @@
>      }
> 
> +  /* Only try anonymous access once. */
> 
> static gboolean asked = FALSE;
> 
> @@ +174,3 @@
> +  /* Only try anonymous access once. */
> +  if (anonymous &&
> +      GPOINTER_TO_INT (g_object_get_data (G_OBJECT (op), "state")) ==
> MOUNT_OP_ASKED)
> 
> anonymous && asked
> 
> @@ +181,3 @@
> +  else
> +    {
> +      g_object_set_data (G_OBJECT (op), "state", GINT_TO_POINTER
> (MOUNT_OP_ASKED));
> 
> asked = TRUE
> 
> @@ +230,2 @@
>        success = FALSE;
> +      if (GPOINTER_TO_INT (g_object_get_data (G_OBJECT (op), "state")) ==
> MOUNT_OP_ABORTED)
> 
> anonymous && aborted

gvfs-mount accepts multiple mount points that can be mounted with a single invocation. Using a global variable wouldn't work with:
gvfs-mount -a smb://host/share smb://host2/share2

So ok, please push it both :-)

I've pushed both for now, but leaving the bug open to decide if anything needs to be done for smb-browse. Thanks

Created attachment 295512 [details] [review]
tests: improve tests with anonymous login

The test for smb anonymous is broken currently, because of the attachment 293541 [details] [review]. There is a patch to change the tests to use "-a" option or "anonymous" property. Unfortunately the test for samba still fails and I'm not pretty sure why :-(

Review of attachment 295512 [details] [review]:

Looks good otherwise.

::: test/gvfs-test
@@ +580,3 @@
 
+        uri = 'ftp://localhost:2121'
+        subprocess.check_call(['gvfs-mount', '-a', uri])

Instead of changing the test case, a new test should be added so that it tests both methods.

@@ +641,3 @@
         '''ftp:// anonymous (API)'''
 
+        uri = 'ftp://localhost:2121'

Again, instead of changing the test case, a new test should be added so that it tests both methods.

Created attachment 295907 [details] [review]
test: Fix running smb tests outside of testbed

Remove the 'interactive' flag which causes smb to only run a single
iteration before exiting.  This caues problems because of the way that
gvfsd-smb tries multiple different authentication methods (e.g. kerberos
first).

Also increase the time to wait for smbd to start (for slow rotational
disks).

smb works for me now with both patches.

Review of attachment 295907 [details] [review]:

Good point! It seemed to me it is trying only one iteration, but didn't know why :-/

::: test/gvfs-test
@@ +762,3 @@
                                      universal_newlines=True,
                                      stdout=subprocess.PIPE)
+        time.sleep(5)

This never fails for me, but it is good idea to increase it, but we should do some kind of dynamical checking (greping the log?, or using smbclient?) as it is done in all other cases, because we can't block the tests for 5 seconds everytime...

Created attachment 295943 [details] [review]
tests: add and fix tests with anonymous login

New tests added instead of changing them...

Review of attachment 295943 [details] [review]:

Looks good.

Created attachment 295984 [details] [review]
test: Wait for smbd to open its port before starting the test

Use ss -ltn and look for port 1445 to wait for smbd to be ready before
proceeding with the test.  This fixes test failures on slow machines
where smbd takes a while to start.

Comment on attachment 295907 [details] [review]
test: Fix running smb tests outside of testbed

I committed the first part and added a new patch for the second part.

Comment on attachment 295943 [details] [review]
tests: add and fix tests with anonymous login

commit c83eddf837b318c41703b16ca9bb5f2c39758fd9

Comment on attachment 295984 [details] [review]
test: Wait for smbd to open its port before starting the test

Thanks! Looks good :-)

Comment on attachment 295984 [details] [review]
test: Wait for smbd to open its port before starting the test

Pushed to master as 221378f58f50bdf6e780e668539e875bac581826.

*** Bug 529365 has been marked as a duplicate of this bug. ***

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/gvfs/issues/243.