You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Daniel Shahaf <d....@daniel.shahaf.co.il> on 2008/06/26 05:39:22 UTC
Re: [PATCH] Fix issue #2242.
Good morning Senthil,
Senthil Kumaran S wrote on Fri, 16 May 2008 at 16:19 +0530:
> Hi,
>
Thanks for your patience with this patch. I'll look at it now.
> I am attaching a patch along with this email which fixes issue #2242.
>
> [[[
> Fix issue #2242.
>
> auth cache picking up password from wrong username entry.
>
> * subversion/libsvn_subr/simple_providers.c
> (simple_password_get): Validate the username for which we get the password.
>
> Patch by: stylesen
> ]]]
>
> Thank You.
Okay, I compiled with it and it fixes the issue for me over svn://. (I
first tested over file:// and almost said it was broken, when I realised
I should test over svn://.) Two questions:
> Index: subversion/libsvn_subr/simple_providers.c
> ===================================================================
> --- subversion/libsvn_subr/simple_providers.c (revision 31223)
> +++ subversion/libsvn_subr/simple_providers.c (working copy)
> @@ -97,12 +97,17 @@
> apr_pool_t *pool)
> {
> svn_string_t *str;
> - str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
> + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> APR_HASH_KEY_STRING);
> - if (str && str->data)
> + if (strcmp(str->data, username) == 0)
^
Can STR be NULL here? I prefer
str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
APR_HASH_KEY_STRING);
if (str && strcmp(str->data, username) == 0)
> {
> - *password = str->data;
> - return TRUE;
> + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
> + APR_HASH_KEY_STRING);
> + if (str && str->data)
> + {
> + *password = str->data;
> + return TRUE;
> + }
> }
> return FALSE;
> }
>
Can you write a regression test?
Thanks,
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Julian Foad wrote:
>>>> +# Issue 2242, auth cache picking up password from wrong username entry
>>>> +def basic_auth_test(sbox):
>>>> + "basic auth test"
>>>> +
>>>> + sbox.build(read_only = True)
>>> Creates a working copy.
>> Will add the comment.
>
> I don't think Daniel meant you should add a comment there. (It's obvious
> what it does so it would be a redundant comment.) I think he was just
> preparing the way for his next remark that contrasts with this one.
Thanks for your clarification :)
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Daniel Shahaf wrote:
> Senthil Kumaran S wrote on Wed, 2 Jul 2008 at 02:38 +0530:
>> I ve made the patch more clear. I think this updated patch solves all your
>> concerns.
>>
>
> It does, thank you.
Committed in r31983.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Senthil Kumaran S wrote on Wed, 2 Jul 2008 at 02:38 +0530:
> I ve made the patch more clear. I think this updated patch solves all your
> concerns.
>
It does, thank you.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Daniel Shahaf wrote:
>>>> Tries to checkout onto existing working copy. It happens to work, but
>>>> I'm not sure that we guarantee that 'checkout' will behave as 'update'
>>>> if its target already exists. In which case, we need s/checkout/update/
>>>> here.
>>> Yes checkout works like update here. It says checked out revision 'n'.
>
> I know that it "works". I question whether its working here is
> a feature that we can rely on, or an incident of implementation that may
> be declared a bug and removed. (Briefly: does our API guarantee that
> 'svn co URL dir; svn co URL dir;' will work as it currently does?)
I don't see a solid statement which says 'it will work', but I feel it does so
from the past releases.
>>>>> + exit_code, output, errput = svntest.main.run_command(
>>>>> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
>>>>> + '--username', 'jrandom', '--non-interactive')
>>>>> +
>>>> We need --config-dir here or we would be affected by the user's
>>>> ~/.subversion/, right? (where, possibly, the creds from a previous run of
>>>> the test are cached, etc.)
>>> Yes exactly, that is the intention of using '--non-interactive' where we intend
>>> to use ~/.subversion
>
> I'm sorry, but I don't follow. I think --config-dir should be added to
> all the run_command calls here; I don't understand how --non-interactive
> or its absence come into play.
Sorry for this entire confusion, I tested this patch with my custom
/home/stylesen/.subversion in home directory which had
'store-plaintext-passwords = yes', which gave me false alarms. After careful
examination I found all these commands do need '--config-dir' (also, I messed
up with run_svn, which has a default config_dir and run_command which does
not). Updated patch has this.
>>>> if (svn_cstring_casecmp(store_plaintext_passwords,
>>>> SVN_CONFIG_ASK) == 0)
>>>> {
>>>> if (non_interactive)
>>>> /* In non-interactive mode, the default behaviour is
>>>> * to not store the password, because it is usually
>>>> * passed on the command line. */
>>>> may_save_password = FALSE;
>>> Yes the above code holds true, where I register the new password for this new
>>> user in the auth cache for the following checkouts.
>>>
>
> Assuming I understood you correctly (I think what you just said means
> "Yes" ;)), I would prefer to see --non-interactive passed and
> store-plaintext-passwords=True set in the config directory. (Are you
> running with keyring or kwallet? I'm surprising the above run_command()
> works without --non-interactive.)
I ve made the patch more clear. I think this updated patch solves all your
concerns.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Julian Foad wrote on Tue, 1 Jul 2008 at 19:51 +0100:
> On Tue, 2008-07-01 at 23:39 +0530, Senthil Kumaran S wrote:
> > Daniel Shahaf wrote:
> > >> +# Issue 2242, auth cache picking up password from wrong username entry
> > >> +def basic_auth_test(sbox):
> > >> + "basic auth test"
> > >> +
> > >> + sbox.build(read_only = True)
> > >
> > > Creates a working copy.
> >
> > Will add the comment.
>
> I don't think Daniel meant you should add a comment there. (It's obvious
> what it does so it would be a redundant comment.) I think he was just
> preparing the way for his next remark that contrasts with this one.
>
Correct, thanks Julian.
(It's much easier when someone else writes my text for me and I just
have to say "Yes".)
> > >> + wc_dir = sbox.wc_dir
> > >> +
> > >> + # Checkout with jrandom
> > >> + exit_code, output, errput = svntest.main.run_command(
> > >> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> > >> + '--username', 'jrandom', '--password', 'rayjandom')
> > >> +
> > >
> > > Tries to checkout onto existing working copy. It happens to work, but
> > > I'm not sure that we guarantee that 'checkout' will behave as 'update'
> > > if its target already exists. In which case, we need s/checkout/update/
> > > here.
> >
> > Yes checkout works like update here. It says checked out revision 'n'.
I know that it "works". I question whether its working here is
a feature that we can rely on, or an incident of implementation that may
be declared a bug and removed. (Briefly: does our API guarantee that
'svn co URL dir; svn co URL dir;' will work as it currently does?)
> > The intention here is to check the auth. Even an 'svn ls' will do
> > but I chose this. The previous checkout is called with
> > '--no-auth-cache' as per 'run_svn' in main.py, so here we need to
> > authenticate again if we dont find an entry in ~/.subversion/auth.
> >
--username and --password override reading from the cache, so there
shouldn't be a problem.
> > > Though, actually, you can do without a working copy for this test, e.g.
> > > by doing s/checkout $URL/info $URL/.
> >
> > Yes could be, but I thought it will be comfortable to use a working copy here,
> > in case if we want to extend this test case in future.
> >
Makes sense.
> > >> + exit_code, output, errput = svntest.main.run_command(
> > >> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> > >> + '--username', 'jrandom', '--non-interactive')
> > >> +
> > >
> > > We need --config-dir here or we would be affected by the user's
> > > ~/.subversion/, right? (where, possibly, the creds from a previous run of
> > > the test are cached, etc.)
> >
> > Yes exactly, that is the intention of using '--non-interactive' where we intend
> > to use ~/.subversion
I'm sorry, but I don't follow. I think --config-dir should be added to
all the run_command calls here; I don't understand how --non-interactive
or its absence come into play.
> > and also I am not using 'run_svn' here because it passes
> > the password in command line as default according to:
> >
Agreed.
> > <snip>
> > def _with_auth(args):
> > assert '--password' not in args
> > args = args + ('--password', wc_passwd,
> > '--no-auth-cache' )
> > if '--username' in args:
> > return args
> > else:
> > return args + ('--username', wc_author )
> > </snip>
> >
> > >> + # Checkout with jconstant
> > >> + exit_code, output, errput = svntest.main.run_command(
> > >> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> > >> + '--username', 'jconstant', '--password', 'rayjandom')
> > >> +
> > >
> > > Why aren't you passing --non-interactive? Is it due to the plaintext
> > > passwords logic?
> > >
> > > if (svn_cstring_casecmp(store_plaintext_passwords,
> > > SVN_CONFIG_ASK) == 0)
> > > {
> > > if (non_interactive)
> > > /* In non-interactive mode, the default behaviour is
> > > * to not store the password, because it is usually
> > > * passed on the command line. */
> > > may_save_password = FALSE;
> >
> > Yes the above code holds true, where I register the new password for this new
> > user in the auth cache for the following checkouts.
> >
Assuming I understood you correctly (I think what you just said means
"Yes" ;)), I would prefer to see --non-interactive passed and
store-plaintext-passwords=True set in the config directory. (Are you
running with keyring or kwallet? I'm surprising the above run_command()
works without --non-interactive.)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Julian Foad <ju...@btopenworld.com>.
On Tue, 2008-07-01 at 23:39 +0530, Senthil Kumaran S wrote:
> Daniel Shahaf wrote:
> >> Test case for issue #2242 - auth cache picking up password from wrong
> >> username entry.
> >>
> >> * subversion/tests/cmdline/basic_tests.py
> >> (def basic_auth_test): New test.
> > ^
> > s/def basic_auth_test/basic_auth_test/
>
> Yes noted.
>
> >> +# Issue 2242, auth cache picking up password from wrong username entry
> >> +def basic_auth_test(sbox):
> >> + "basic auth test"
> >> +
> >> + sbox.build(read_only = True)
> >
> > Creates a working copy.
>
> Will add the comment.
I don't think Daniel meant you should add a comment there. (It's obvious
what it does so it would be a redundant comment.) I think he was just
preparing the way for his next remark that contrasts with this one.
> >> + wc_dir = sbox.wc_dir
> >> +
> >> + # Checkout with jrandom
> >> + exit_code, output, errput = svntest.main.run_command(
> >> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> >> + '--username', 'jrandom', '--password', 'rayjandom')
> >> +
> >
> > Tries to checkout onto existing working copy. It happens to work, but
> > I'm not sure that we guarantee that 'checkout' will behave as 'update'
> > if its target already exists. In which case, we need s/checkout/update/
> > here.
>
> Yes checkout works like update here. It says checked out revision 'n'. The
> intention here is to check the auth. Even an 'svn ls' will do but I chose this.
> The previous checkout is called with '--no-auth-cache' as per 'run_svn' in
> main.py, so here we need to authenticate again if we dont find an entry in
> ~/.subversion/auth.
>
> > Though, actually, you can do without a working copy for this test, e.g.
> > by doing s/checkout $URL/info $URL/.
>
> Yes could be, but I thought it will be comfortable to use a working copy here,
> in case if we want to extend this test case in future.
>
> >> + exit_code, output, errput = svntest.main.run_command(
> >> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> >> + '--username', 'jrandom', '--non-interactive')
> >> +
> >
> > We need --config-dir here or we would be affected by the user's
> > ~/.subversion/, right? (where, possibly, the creds from a previous run of
> > the test are cached, etc.)
>
> Yes exactly, that is the intention of using '--non-interactive' where we intend
> to use ~/.subversion and also I am not using 'run_svn' here because it passes
> the password in command line as default according to:
>
> <snip>
> def _with_auth(args):
> assert '--password' not in args
> args = args + ('--password', wc_passwd,
> '--no-auth-cache' )
> if '--username' in args:
> return args
> else:
> return args + ('--username', wc_author )
> </snip>
>
> >> + # Checkout with jconstant
> >> + exit_code, output, errput = svntest.main.run_command(
> >> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> >> + '--username', 'jconstant', '--password', 'rayjandom')
> >> +
> >
> > Why aren't you passing --non-interactive? Is it due to the plaintext
> > passwords logic?
> >
> > if (svn_cstring_casecmp(store_plaintext_passwords,
> > SVN_CONFIG_ASK) == 0)
> > {
> > if (non_interactive)
> > /* In non-interactive mode, the default behaviour is
> > * to not store the password, because it is usually
> > * passed on the command line. */
> > may_save_password = FALSE;
>
> Yes the above code holds true, where I register the new password for this new
> user in the auth cache for the following checkouts.
>
> Thank You.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Daniel Shahaf wrote:
>> Test case for issue #2242 - auth cache picking up password from wrong
>> username entry.
>>
>> * subversion/tests/cmdline/basic_tests.py
>> (def basic_auth_test): New test.
> ^
> s/def basic_auth_test/basic_auth_test/
Yes noted.
>> +# Issue 2242, auth cache picking up password from wrong username entry
>> +def basic_auth_test(sbox):
>> + "basic auth test"
>> +
>> + sbox.build(read_only = True)
>
> Creates a working copy.
Will add the comment.
>> + wc_dir = sbox.wc_dir
>> +
>> + # Checkout with jrandom
>> + exit_code, output, errput = svntest.main.run_command(
>> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
>> + '--username', 'jrandom', '--password', 'rayjandom')
>> +
>
> Tries to checkout onto existing working copy. It happens to work, but
> I'm not sure that we guarantee that 'checkout' will behave as 'update'
> if its target already exists. In which case, we need s/checkout/update/
> here.
Yes checkout works like update here. It says checked out revision 'n'. The
intention here is to check the auth. Even an 'svn ls' will do but I chose this.
The previous checkout is called with '--no-auth-cache' as per 'run_svn' in
main.py, so here we need to authenticate again if we dont find an entry in
~/.subversion/auth.
> Though, actually, you can do without a working copy for this test, e.g.
> by doing s/checkout $URL/info $URL/.
Yes could be, but I thought it will be comfortable to use a working copy here,
in case if we want to extend this test case in future.
>> + exit_code, output, errput = svntest.main.run_command(
>> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
>> + '--username', 'jrandom', '--non-interactive')
>> +
>
> We need --config-dir here or we would be affected by the user's
> ~/.subversion/, right? (where, possibly, the creds from a previous run of
> the test are cached, etc.)
Yes exactly, that is the intention of using '--non-interactive' where we intend
to use ~/.subversion and also I am not using 'run_svn' here because it passes
the password in command line as default according to:
<snip>
def _with_auth(args):
assert '--password' not in args
args = args + ('--password', wc_passwd,
'--no-auth-cache' )
if '--username' in args:
return args
else:
return args + ('--username', wc_author )
</snip>
>> + # Checkout with jconstant
>> + exit_code, output, errput = svntest.main.run_command(
>> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
>> + '--username', 'jconstant', '--password', 'rayjandom')
>> +
>
> Why aren't you passing --non-interactive? Is it due to the plaintext
> passwords logic?
>
> if (svn_cstring_casecmp(store_plaintext_passwords,
> SVN_CONFIG_ASK) == 0)
> {
> if (non_interactive)
> /* In non-interactive mode, the default behaviour is
> * to not store the password, because it is usually
> * passed on the command line. */
> may_save_password = FALSE;
Yes the above code holds true, where I register the new password for this new
user in the auth cache for the following checkouts.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Senthil Kumaran S wrote on Tue, 1 Jul 2008 at 17:06 +0530:
> Daniel Shahaf wrote:
> > Can you write a regression test?
>
> I am attaching a test case in order to test this issue. Would like to get your
> comments before I commit.
>
Sure.
> [[[
> Test case for issue #2242 - auth cache picking up password from wrong
> username entry.
>
> * subversion/tests/cmdline/basic_tests.py
> (def basic_auth_test): New test.
^
s/def basic_auth_test/basic_auth_test/
> (test_list): Add above test.
>
> Patch by: stylesen
> ]]]
>
>
> Index: subversion/tests/cmdline/basic_tests.py
> ===================================================================
> --- subversion/tests/cmdline/basic_tests.py (revision 31937)
> +++ subversion/tests/cmdline/basic_tests.py (working copy)
> @@ -2355,6 +2355,40 @@
> expected_output, [], 'ls', '-r3',
> '^//A/@3', iota_url)
>
> +
> +# Issue 2242, auth cache picking up password from wrong username entry
> +def basic_auth_test(sbox):
> + "basic auth test"
> +
> + sbox.build(read_only = True)
Creates a working copy.
> + wc_dir = sbox.wc_dir
> +
> + # Checkout with jrandom
> + exit_code, output, errput = svntest.main.run_command(
> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> + '--username', 'jrandom', '--password', 'rayjandom')
> +
Tries to checkout onto existing working copy. It happens to work, but
I'm not sure that we guarantee that 'checkout' will behave as 'update'
if its target already exists. In which case, we need s/checkout/update/
here.
Though, actually, you can do without a working copy for this test, e.g.
by doing s/checkout $URL/info $URL/.
> + exit_code, output, errput = svntest.main.run_command(
> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> + '--username', 'jrandom', '--non-interactive')
> +
We need --config-dir here or we would be affected by the user's
~/.subversion/, right? (where, possibly, the creds from a previous run of
the test are cached, etc.)
> + # Checkout with jconstant
> + exit_code, output, errput = svntest.main.run_command(
> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> + '--username', 'jconstant', '--password', 'rayjandom')
> +
Why aren't you passing --non-interactive? Is it due to the plaintext
passwords logic?
if (svn_cstring_casecmp(store_plaintext_passwords,
SVN_CONFIG_ASK) == 0)
{
if (non_interactive)
/* In non-interactive mode, the default behaviour is
* to not store the password, because it is usually
* passed on the command line. */
may_save_password = FALSE;
> + exit_code, output, errput = svntest.main.run_command(
> + svntest.main.svn_binary, None, 1, 'co', sbox.repo_url, wc_dir,
> + '--username', 'jconstant', '--non-interactive')
> +
> + # Checkout with jrandom which should fail since we do not provide
> + # a password and the above cached password belongs to jconstant
Exactly.
> + expected_err = ["authorization failed: Could not authenticate to server:"]
> + exit_code, output, errput = svntest.main.run_command(
> + svntest.main.svn_binary, expected_err, 1, 'co', sbox.repo_url, wc_dir,
> + '--username', 'jrandom', '--non-interactive')
> +
> +
> #----------------------------------------------------------------------
Thanks,
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Daniel,
Daniel Shahaf wrote:
>
> Can you write a regression test?
I am attaching a test case in order to test this issue. Would like to get your
comments before I commit.
[[[
Test case for issue #2242 - auth cache picking up password from wrong
username entry.
* subversion/tests/cmdline/basic_tests.py
(def basic_auth_test): New test.
(test_list): Add above test.
Patch by: stylesen
]]]
--
Senthil Kumaran S
http://www.stylesen.org/
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Senthil Kumaran S wrote on Thu, 26 Jun 2008 at 12:05 +0530:
> [[[
> Fix issue #2242.
>
> auth cache picking up password from wrong username entry.
One small thing, these two should be on the same line. e.g.
Fix issue #2242: auth cache picking up password from wrong username entry.
> * subversion/libsvn_subr/simple_providers.c
> (simple_password_get): Validate the username for which we get the password.
>
> Patch by: stylesen
> ]]]
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Daniel Shahaf wrote:
>> I am attaching an updated patch along with this email based on your comments
>> above.
>>
>> Thank You.
>>
>
> +1 to commit, assuming it passes 'make check'.
Committed in r31884. Yes it passes 'make *check'.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Senthil Kumaran S wrote on Thu, 26 Jun 2008 at 12:05 +0530:
> Hi Daniel,
>
> Daniel Shahaf wrote:
> > > - if (str && str->data)
> > > + if (strcmp(str->data, username) == 0)
> > ^
> >
> > Can STR be NULL here? I prefer
> >
> > str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> > APR_HASH_KEY_STRING);
> > if (str && strcmp(str->data, username) == 0)
> >
>
> I am attaching an updated patch along with this email based on your comments
> above.
>
> Thank You.
>
+1 to commit, assuming it passes 'make check'.
Thanks!
Daniel
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Daniel,
Daniel Shahaf wrote:
>> - if (str && str->data)
>> + if (strcmp(str->data, username) == 0)
> ^
>
> Can STR be NULL here? I prefer
>
> str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> APR_HASH_KEY_STRING);
> if (str && strcmp(str->data, username) == 0)
>
I am attaching an updated patch along with this email based on your comments above.
Thank You.
--
Senthil Kumaran S
http://www.stylesen.org/
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Hi Daniel,
Daniel Shahaf wrote:
> Thanks for your patience with this patch. I'll look at it now.
This is really a good news! I am really happy :)
>> apr_pool_t *pool)
>> {
>> svn_string_t *str;
>> - str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
>> + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
>> APR_HASH_KEY_STRING);
>> - if (str && str->data)
>> + if (strcmp(str->data, username) == 0)
> ^
>
> Can STR be NULL here? I prefer
>
> str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> APR_HASH_KEY_STRING);
> if (str && strcmp(str->data, username) == 0)
Yes a check for STR is nice here. I am +1 for this.
>> {
>> - *password = str->data;
>> - return TRUE;
>> + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
>> + APR_HASH_KEY_STRING);
>> + if (str && str->data)
>> + {
>> + *password = str->data;
>> + return TRUE;
>> + }
>> }
>> return FALSE;
>> }
>>
>
> Can you write a regression test?
With pleasure. I shall do it asap.
Once again thanks for your time in reviewing this patch.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Senthil Kumaran S wrote on Thu, 26 Jun 2008 at 11:48 +0530:
> Daniel Shahaf wrote:
> > > Can STR be NULL here? I prefer
> > >
> > > str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> > > APR_HASH_KEY_STRING);
> > > if (str && strcmp(str->data, username) == 0)
> >
> > Actually, the docstring for svn_auth__password_get_t says:
> >
> > It might be obtained directly from CREDS, or from an external store,
> > using REALMSTRING and USERNAME as keys.
> > So don't we need compare the realms as well?
>
> We need not compare the realms here. If you look at the structure of the files
> which we store into the auth area, inside svn.simple we create one file per
> realm, where the filename is an hashed value of the realm. When we query for a
> password for a particular realm, we get the creds_hash as follows:
>
> <snip>
> err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
> realmstring, config_dir, pool);
> </snip>
>
> So we need not take the realm into account here.
>
Right, all calls to simple_password_get go through
svn_auth__simple_first_creds_helper, which checks the realm for us. So we
don't need to check it here ourselves.
It doesn't make the code easier to follow, but it works.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Daniel Shahaf wrote:
>> Can STR be NULL here? I prefer
>>
>> str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
>> APR_HASH_KEY_STRING);
>> if (str && strcmp(str->data, username) == 0)
>
> Actually, the docstring for svn_auth__password_get_t says:
>
> It might be obtained directly from CREDS, or from an external store,
> using REALMSTRING and USERNAME as keys.
>
> So don't we need compare the realms as well?
We need not compare the realms here. If you look at the structure of the files
which we store into the auth area, inside svn.simple we create one file per
realm, where the filename is an hashed value of the realm. When we query for a
password for a particular realm, we get the creds_hash as follows:
<snip>
err = svn_config_read_auth_data(&creds_hash, SVN_AUTH_CRED_SIMPLE,
realmstring, config_dir, pool);
</snip>
So we need not take the realm into account here.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Daniel Shahaf <d....@daniel.shahaf.co.il>.
Daniel Shahaf wrote on Thu, 26 Jun 2008 at 08:39 +0300:
> Senthil Kumaran S wrote on Fri, 16 May 2008 at 16:19 +0530:
> > Index: subversion/libsvn_subr/simple_providers.c
> > ===================================================================
> > --- subversion/libsvn_subr/simple_providers.c (revision 31223)
> > +++ subversion/libsvn_subr/simple_providers.c (working copy)
> > @@ -97,12 +97,17 @@
> > apr_pool_t *pool)
> > {
> > svn_string_t *str;
> > - str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_PASSWORD_KEY,
> > + str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> > APR_HASH_KEY_STRING);
> > - if (str && str->data)
> > + if (strcmp(str->data, username) == 0)
> ^
>
> Can STR be NULL here? I prefer
>
> str = apr_hash_get(creds, SVN_AUTH__AUTHFILE_USERNAME_KEY,
> APR_HASH_KEY_STRING);
> if (str && strcmp(str->data, username) == 0)
Actually, the docstring for svn_auth__password_get_t says:
It might be obtained directly from CREDS, or from an external store,
using REALMSTRING and USERNAME as keys.
So don't we need compare the realms as well?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: [PATCH] Fix issue #2242.
Posted by Senthil Kumaran S <se...@collab.net>.
Daniel Shahaf wrote:
> Okay, I compiled with it and it fixes the issue for me over svn://. (I
> first tested over file:// and almost said it was broken, when I realised
> I should test over svn://.) Two questions:
You can reproduce this with http:// and https:// as well.
--
Senthil Kumaran S
http://www.stylesen.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org