[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