Julian Foad wrote:
> It's the "return SVN_NO_ERROR;" in the middle of the code above. I think
> it should return an SVN_ERR_AUTHN_FAILED error there.
>
> I looked at the callers of "first_creds" functions, and they pretty much
> all treat an error return the same as they treat a successful return
> with NULL for the credentials. There are two issues: (1) it seems to me
> like an error ought to be reported in this situation, and (2) if the
> function returns at this point with no error, the caller expects that
> '*credentials' has been set to some value, but it has not been set to
> anything.
>
> Agreed?
Julian, yes I agree with the above, which will lead to the following patch,
<patch>
Index: subversion/libsvn_auth_gnome_keyring/gnome_keyring.c
===================================================================
--- subversion/libsvn_auth_gnome_keyring/gnome_keyring.c (revision 39975)
+++ subversion/libsvn_auth_gnome_keyring/gnome_keyring.c (working copy)
@@ -387,7 +387,9 @@
/* If keyring is locked give up and try the next provider. */
if (! unlock_gnome_keyring(default_keyring, keyring_password, pool))
- return SVN_NO_ERROR;
+ return svn_error_create(SVN_ERR_AUTHN_CREDS_UNAVAILABLE, NULL,
+ _("GNOME Keyring is locked, giving up to "
+ "try next provider."));
}
}
else
@@ -442,7 +444,9 @@
/* If keyring is locked give up and try the next provider. */
if (! unlock_gnome_keyring(default_keyring, keyring_password, pool))
- return SVN_NO_ERROR;
+ return svn_error_create(SVN_ERR_AUTHN_CREDS_UNAVAILABLE, NULL,
+ _("GNOME Keyring is locked, giving up to "
+ "try next provider."));
}
}
else
@@ -549,7 +553,9 @@
/* If keyring is locked give up and try the next provider. */
if (! unlock_gnome_keyring(default_keyring, keyring_password, pool))
- return SVN_NO_ERROR;
+ return svn_error_create(SVN_ERR_AUTHN_CREDS_UNAVAILABLE, NULL,
+ "GNOME Keyring is locked, giving up to "
+ "try next provider.");
}
}
else
@@ -605,7 +611,9 @@
/* If keyring is locked give up and try the next provider. */
if (! unlock_gnome_keyring(default_keyring, keyring_password, pool))
- return SVN_NO_ERROR;
+ return svn_error_create(SVN_ERR_AUTHN_CREDS_UNAVAILABLE, NULL,
+ _("GNOME Keyring is locked, giving up to "
+ "try next provider."));
}
}
else
</patch>
but the problem with above kinda patch is, when we error out it is taken as an
authentication failure and do not check for the next provider here, thus we end
up with a rather misleading error message as follows:
<snip>
$ svn info http://localhost/svn/repos1
Password for 'default' GNOME keyring:
../subversion/svn/info-cmd.c:556: (apr_err=170001)
../subversion/libsvn_client/deprecated.c:1724: (apr_err=170001)
../subversion/libsvn_client/info.c:477: (apr_err=170001)
../subversion/libsvn_client/ra.c:421: (apr_err=170001)
../subversion/libsvn_ra/ra_loader.c:486: (apr_err=170001)
../subversion/libsvn_ra/ra_loader.c:486: (apr_err=170001)
../subversion/libsvn_ra_neon/util.c:613: (apr_err=170001)
svn: OPTIONS of 'http://localhost/svn/repos1': authorization failed: Could not
authenticate to server: rejected Basic challenge (http://localhost)
</snip>
On the other hand, when we don't error out, the following happens, in which
since the credentials are not populated yet, the next provider is contacted and
it moves on.
<snip>
$ svn info http://localhost/svn/repos1
Password for 'default' GNOME keyring:
Authentication realm: <http://localhost:80> Another SVN repository
Password for 'stylesen':
Password for 'default' GNOME keyring:
-----------------------------------------------------------------------
ATTENTION! Your password for authentication realm:
<http://localhost:80> Another SVN repository
can only be stored to disk unencrypted! You are advised to configure
your system so that Subversion can store passwords encrypted, if
possible. See the documentation for details.
You can avoid future appearances of this warning by setting the value
of the 'store-plaintext-passwords' option to either 'yes' or 'no' in
'/home/stylesen/.subversion/servers'.
-----------------------------------------------------------------------
Store password unencrypted (yes/no)? yes
Password for 'default' GNOME keyring:
Password for 'default' GNOME keyring:
Path: repos1
URL: http://localhost/svn/repos1
Repository Root: http://localhost/svn/repos1
Repository UUID: 0f6535f9-57e6-44d7-a8dc-9e67729ca394
Revision: 3
Node Kind: directory
Last Changed Author: stylesen
Last Changed Rev: 3
Last Changed Date: 2009-10-12 15:50:28 +0530 (Mon, 12 Oct 2009)
$ cat /home/stylesen/.subversion/auth/svn.simple/5970bcd6aca4ee59248689d98303d1e8
K 8
passtype
V 6
simple
K 8
password
V 8
password
K 15
svn:realmstring
V 44
<http://localhost:80> Another SVN repository
K 8
username
V 8
stylesen
END
</snip>
I looked into your "maybe_unlock_keyring" factored function, which should be
incorporated, except for the error return for reasons specified above.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
------------------------------------------------------
http://subversion.tigris.org/ds/viewMessage.do?dsForumId=462&dsMessageId=2406989
Received on 2009-10-13 23:14:36 CEST