[PATCH] AuthenticationConfig patch to provide expected keys to credential store plug-ins
Jean-Francois Roy
bahamut at macstorm.org
Tue Mar 10 18:31:26 GMT 2009
On Mar 9, 2009, at 5:47 PM, Vincent Ladeuil wrote:
>>>>>> "jfroy" == Jean-Francois Roy <bahamut at macstorm.org> writes:
>
> jfroy> In addition, the netrc credential store plug-in as well as
> jfroy> its tests are designed under the assumption the credentials
> jfroy> dictionary will contain a "host" key, when it fact this
> would
> jfroy> never be the case.
>
> This is the case for the default section only since all sections in
> .netrc are by host.
That is incorrect. The credentials dictionary passed to credential
stores would never contain a 'host' key, or any other key than the
username, authentication section name and encoded password, as shown
below:
credentials = dict(name=auth_def_name,
user=a_user,
password=auth_def.get('password', None),
verify_certificates=a_verify_certificates)
self.decode_password(credentials,
auth_def.get('password_encoding',
None))
And as shown by the modified netrc test which now create and use an
AuthenticationConfig object:
jfroy:bzr.dev bahamut$ ./bzr selftest
bzrlib.plugins.netrc_credential_store -Dauth
testing: /Volumes/Crossroads/bahamut/Documents/Source/bzr/bzr.dev/bzr
/Volumes/Crossroads/bahamut/Documents/Source/bzr/bzr.dev/bzrlib
(1.13dev python2.6.1)
ERROR:
bzrlib.plugins.netrc_credential_store.tests.test_netrc.TestNetrcCS.test_default_password
'host'
ERROR:
bzrlib.plugins.netrc_credential_store.tests.test_netrc.TestNetrcCS.test_default_password_without_user
'host'
ERROR:
bzrlib.plugins.netrc_credential_store.tests.test_netrc.TestNetrcCS.test_matching_user
'host'
ERROR:
bzrlib.plugins.netrc_credential_store.tests.test_netrc.TestNetrcCS.test_not_matching_user
'host'
Sample traceback for one of the above failures:
Traceback (most recent call last):
File "/Volumes/Crossroads/bahamut/Documents/Source/bzr/bzr.dev/
bzrlib/plugins/netrc_credential_store/tests/test_netrc.py", line 86,
in test_default_password
credentials = self.ac.get_credentials('scheme', 'other',
user='anonymous')
File "/Volumes/Crossroads/bahamut/Documents/Source/bzr/bzr.dev/
bzrlib/config.py", line 1065, in get_credentials
auth_def.get('password_encoding', None))
File "/Volumes/Crossroads/bahamut/Documents/Source/bzr/bzr.dev/
bzrlib/config.py", line 1184, in decode_password
credentials['password'] = cs.decode_password(credentials)
File "/Volumes/Crossroads/bahamut/Documents/Source/bzr/bzr.dev/
bzrlib/plugins/netrc_credential_store/__init__.py", line 47, in
decode_password
auth = self._netrc.authenticators(credentials['host'])
KeyError: 'host'
And with this patch:
jfroy:authconfig-provider-fix bahamut$ ./bzr selftest
bzrlib.plugins.netrc_credential_store -Dauth
testing: /Volumes/Crossroads/bahamut/Documents/Source/bzr/authconfig-
provider-fix/bzr
/Volumes/Crossroads/bahamut/Documents/Source/bzr/authconfig-
provider-fix/bzrlib (1.13dev python2.6.1)
[test 0/5] starting...
----------------------------------------------------------------------
Ran 5 tests in 0.083s
OK
tests passed
Also, for completeness:
jfroy:authconfig-provider-fix bahamut$ ./bzr selftest
bzrlib.tests.test_config.TestAuthenticationConfigFile
testing: /Volumes/Crossroads/bahamut/Documents/Source/bzr/authconfig-
provider-fix/bzr
/Volumes/Crossroads/bahamut/Documents/Source/bzr/authconfig-
provider-fix/bzrlib (1.13dev python2.6.1)
[test 0/14] starting...
----------------------------------------------------------------------
Ran 14 tests in 0.013s
OK
tests passed
>
> jfroy> In other words, without this patch, the netrc credential
> jfroy> store plug-in bundled with Bazaar is inoperative.
>
> I don't think so, but anyway, that's not the point :)
See above.
>
> jfroy> Relevant tests have been updated
>
> You missed the ones you pointed out above: the netrc_credential_store
> ones :-)
>
> Since they were the root cause of the problem you're addressing here
> (they test the lower level API, masking the problem to the upper
> levels), it would be best to update them by using an
> AuthenticationConfig object on top of the NetrcCredentialStore one and
> use get_password instead of decode_password directly.
>
> Or even better *add* a new single test that reproduce the problem
> without your patch and pass with your patch.
Done in the revised patch. All the tests now use an
AuthenticationConfig object, with the exception of
test_default_password_without_user (since AuthenticationConfig will
not allow a "no user" situation).
>
> <snip/>
>
> jfroy> === modified file 'NEWS'
> jfroy> --- NEWS 2009-03-05 09:12:17 +0000
> jfroy> +++ NEWS 2009-03-05 18:28:58 +0000
> jfroy> @@ -110,6 +110,12 @@
> jfroy> when needed.
> jfroy> (Vincent Ladeuil)
>
> jfroy> + * Corrected a disconnect between
> ``AuthenticationConfig`` and credential
> jfroy> + provider plug-ins where the credentials dictionary
> passed to
> jfroy> + providers would not contain expected keys, as
> tested by the netrc
> jfroy> + provider plug-in tests.
> jfroy> + (Jean-Francois Roy)
> jfroy> +
>
> Documenting what didn't work is good but documenting what is working
> *now* is better :)
>
> How about:
>
> authentication plugins now receive all the parameters from the request
> itself (aka host, port, realm, path, etc), only the parameters defined
> in the authentication section were provided previously.
Done.
>
> jfroy> === modified file 'bzrlib/config.py'
> jfroy> --- bzrlib/config.py 2009-01-17 01:30:58 +0000
> jfroy> +++ bzrlib/config.py 2009-03-05 18:15:54 +0000
>
> <snip/>
>
> jfroy> @@ -1057,12 +1060,26 @@
>
> <snip/>
>
> jfroy> + # Delete all extra keys from the credentials
> dictionary
> jfroy> + del credentials['scheme']
> jfroy> + del credentials['host']
> jfroy> + del credentials['port']
> jfroy> + del credentials['path']
> jfroy> + del credentials['realm']
>
> I understand you comply with the doc string here, I wonder if we
> should
> left these keys in place an update the doc string instead :)
I think it's fine to delete the keys. They're not required past the
decode password key and keeping them around would only increase memory
usage. Not changed in the updated patch.
>
> BB:tweak
>
> Note to other reviewer, I'd be happy to merge that patch myself and
> handle any needed tweak.
>
> Vincent
-------------- next part --------------
A non-text attachment was scrubbed...
Name: authconfig-provider-fix-2.patch
Type: application/octet-stream
Size: 15213 bytes
Desc: not available
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20090310/d0dc8159/attachment-0001.obj
More information about the bazaar
mailing list