Bug #42877 for HTTP-DAV: Patch for HTTP::DAV fixing bug with recent libwww version

Thu Jan 29 03:19:35 2009 OPERA [...] cpan.org - Ticket created

CC:	pcollins [...] cpan.org
Subject:	Patch for HTTP::DAV fixing bug with recent libwww version

Hi there, I started using HTTP::DAV recently and faced some problems with the authentication. I always got an "Unauthorized" error although the credentials were set. I was able to track it down to the HTTP::HTTP::DAV::UserAgent->credentials() in HTTP::DAV::Comms.pm which behaves differently compared to LWP::UserAgent->credentials(). The attached patch fixes the problem, at least for me, and it seems to work fine now. I'm using: libwww-perl 5.820-1 (Debian package on sid) libhttp-dav-perl 0.35-1 (Build from the latest CPAN sources on Debian sid) Could you pls apply the patch to the mainstream version? Thanks a lot for the good work on this module Best regards -- Thorsten Klein <grobie@perlwizard.de>

Subject:

HTTP-DAV-Comms.patch.gz

Download HTTP-DAV-Comms.patch.gz
application/x-gzip 412b

Message body not shown because it is not plain text.

Thu Jan 29 03:59:48 2009 OPERA [...] cpan.org - Correspondence added 60 min

Hi Thorsten, I looked at the patch, and it's a bit weird. I see it's taken from LWP::UserAgent, where you are using "@$old" it's not the old user/password credentials really, because the reference used is the same before and after. I will review the patch, making sure that the behavior is the same as that of LWP::UserAgent. However, this might clash with usage inside HTTP::DAV itself. Patrick, do you have any indications about this? AFAIK, credentials() in HTTP::DAV::Comms and HTTP::DAV::UserAgent return the new credentials as return values, not the old ones like LWP::UserAgent does. Do we need this behavior somewhere or we can just conform to LWP::UserAgent? I tried to check this by myself, and it *seems* we can change this.

Thu Jan 29 03:59:49 2009 The RT System itself - Status changed from 'new' to 'open'

Thu Jan 29 04:52:58 2009 grobie [...] perlwizard.de - Correspondence added

On Thu Jan 29 03:59:48 2009, OPERA wrote: Show quoted text

> Hi Thorsten, > > I looked at the patch, and it's a bit weird. > I see it's taken from LWP::UserAgent, where you are using "@$old"

Yes, right. it's Show quoted text

> > AFAIK, credentials() in HTTP::DAV::Comms and HTTP::DAV::UserAgent > return the new credentials as return values, not the old ones like > LWP::UserAgent does.

In fact the old values are only returned if no new values where given to credentials. I noticed that there's a call in LWP::Authen::Basic which calls credentials w/o user name and password and expects the already set values as return values. From my point of view the big difference is that you can't unset username and password with the patched version, at least not by supplying empty values. IMHO the LWP::UserAgent approach could help here: not assigning all values at once but only $netloc and $realm and checking @_ later then.

Thu Jan 29 07:38:42 2009 grobie [...] perlwizard.de - Correspondence added

Hi there, I updated the patch to take your remarks into account. If you pass user and password to credentials() they will be updated and returned. AFAIK this is the current behaviour of credentials(). In case you don't even pass the parameter you simply get the current values. I have no clue why LWP changed this behaviour, because there's get_basic_crendtials() in place... HTH cu thorsten

Download HTTP-DAV-Comms.patch2.gz
application/x-gzip 516b

Message body not shown because it is not plain text.

Thu Jan 29 09:04:57 2009 OPERA [...] cpan.org - Correspondence added 60 min

RT-Send-CC:

pcollins [...] cpan.org

Hi Thorsten, thanks again for your revised patch. I tried to rework it in a way that matches my understanding of HTTP::DAV. Please don't take offense. It should fix another RT bug #19616, but can you please make sure your test case is working properly? I attached the new version to this ticket. Thanks!

Message body is not shown because it is too large.

Thu Jan 29 10:04:30 2009 grobie [...] perlwizard.de - Correspondence added

Hi Patrick, On Thu Jan 29 09:04:57 2009, OPERA wrote: Show quoted text

> Hi Thorsten, > > thanks again for your revised patch. > I tried to rework it in a way that matches my understanding of > HTTP::DAV. Please don't take offense. >

Not at all. I just wanted to offer a possible solution, however it's your code and it has to fit into your style. Show quoted text

> It should fix another RT bug #19616, but can you please make sure your > test case is working properly? > I attached the new version to this ticket. >

I've checked it out and found the following problems: - the first time you call credentials() from HTTP::DAV::Comms $self->{basic_authentication}->{$netloc}->{$realm} is undef and therefore $cred is not becoming a reference. - I've changed the return values, because $user and $pass could be undef while $cred is filled fine. if (! $cred) { return; } # User/password pair elsif (wantarray) { return ($cred->[0], $cred->[1]); } # 'user:password' return join(':', $cred->[0], $cred->[1]); HTH

Thu Jan 29 17:47:25 2009 OPERA [...] cpan.org - Correspondence added 60 min

On Gio. 29 Gen. 2009 10:04:30, grobie@perlwizard.de wrote: Show quoted text

> > On Thu Jan 29 09:04:57 2009, OPERA wrote:

> > thanks again for your revised patch. > > I tried to rework it in a way that matches my understanding of > > HTTP::DAV. [...] > > I attached the new version to this ticket.

Show quoted text

> I've checked it out and found the following problems: > > - the first time you call credentials() from HTTP::DAV::Comms > $self->{basic_authentication}->{$netloc}->{$realm} is undef and > therefore $cred is not becoming a reference.

Sure. Should be fixed now. Show quoted text

> - I've changed the return values, because $user and $pass could be

undef Show quoted text

> while $cred is filled fine.

Again, yes. I was too quick. Should work fine now. I have attached a new version. Can you give it another try, please?

Message body is not shown because it is too large.

Thu Jan 29 17:49:45 2009 OPERA [...] cpan.org - Correspondence added

Show quoted text

> Again, yes. I was too quick. Should work fine now. I have attached a > new version. Can you give it another try, please?

I thought to remove the "undef" case, because that's only meaningful if we want to return the old values, which doesn't seem appropriate here. Examining the rest of the code, it *might* be that we're relying on the give-me-the-current-values behavior. So credentials() should just distinguish between list or scalar context.

Fri Jan 30 03:42:12 2009 grobie [...] perlwizard.de - Correspondence added

On Thu Jan 29 17:49:45 2009, OPERA wrote: Show quoted text

> > Again, yes. I was too quick. Should work fine now. I have attached a > > new version. Can you give it another try, please?

>

I still face the same problem and get an error message as well. "Can't use an undefined value as an ARRAY reference at lib/HTTP/DAV/Comms.pm line 371." Show quoted text

> I thought to remove the "undef" case, because that's only meaningful if > we want to return the old values, which doesn't seem appropriate here. > Examining the rest of the code, it *might* be that we're relying on the > give-me-the-current-values behavior. > > So credentials() should just distinguish between list or scalar context.

I'm not sure if I got you right, so I add my 2 cents here. IMHO credentials() should behave the same as in LWP, because it's used by LWP e.g. in case of authorisation LWP::Authen::Basic::authenticate() (http://gitorious.org/projects/libwww-perl/repos/mainline/blobs/master/lib/LWP/Authen/Basic.pm#line28). The problem I see after debugging the code is that $cred isn't a reference to $self->{basic_authentication}->{$netloc} if you call credentials() for the very first time. I tried to add the following piece just before my $cred = ... if ( !defined $self->{basic_authentication}->{$netloc}->{$realm} ) { $self->{basic_authentication}->{$netloc}->{$realm} = []; } This solves the problem of "lost" credentials for me.

Fri Jan 30 03:50:00 2009 OPERA [...] cpan.org - Correspondence added 60 min

On Ven. 30 Gen. 2009 03:42:12, grobie@perlwizard.de wrote: Show quoted text

> On Thu Jan 29 17:49:45 2009, OPERA wrote:

> > > Again, yes. I was too quick. Should work fine now. I have attached

> a

> > > new version. Can you give it another try, please?

> >

> I still face the same problem and get an error message as well. > > "Can't use an undefined value as an ARRAY reference at > lib/HTTP/DAV/Comms.pm line 371."

Yes, sorry about this waste of time. Actually, now I wrote some test cases to appropriately test the whole thing without nagging you. However... Show quoted text

> I'm not sure if I got you right, so I add my 2 cents here. > IMHO credentials() should behave the same as in LWP, because it's used > by LWP e.g. in case of authorisation

It's here I'm not really sure. It's true that we must behave like LWP::UserAgent::credentials(), which returns the *old* values, not the current ones. But, until now, HTTP::DAV::UserAgent::credentials() has always returned the *current* values. And I seem to understand that HTTP::DAV relies on the latter for some inner workings (HTTP::DAV::Comms::credentials(), and probably other points in the code). Show quoted text

> if ( !defined $self->{basic_authentication}->{$netloc}->{$realm} ) { > $self->{basic_authentication}->{$netloc}->{$realm} = []; > }

That's correct. I added this already in our internal version after playing a bit with tests. I'll continue with the tests. If you have a test script that I can use, please send it along. And thanks for your help so far.

Fri Jan 30 05:18:53 2009 grobie [...] perlwizard.de - Correspondence added

On Fri Jan 30 03:50:00 2009, OPERA wrote: Show quoted text

> On Ven. 30 Gen. 2009 03:42:12, grobie@perlwizard.de wrote:

> > On Thu Jan 29 17:49:45 2009, OPERA wrote:

> > > > Again, yes. I was too quick. Should work fine now. I have attached

> > a

> > > > new version. Can you give it another try, please?

> > >

> > I still face the same problem and get an error message as well. > > > > "Can't use an undefined value as an ARRAY reference at > > lib/HTTP/DAV/Comms.pm line 371."

> > Yes, sorry about this waste of time.

No problem at all :) Show quoted text

>

> > I'm not sure if I got you right, so I add my 2 cents here. > > IMHO credentials() should behave the same as in LWP, because it's used > > by LWP e.g. in case of authorisation

> > It's here I'm not really sure. > It's true that we must behave like LWP::UserAgent::credentials(), which > returns the *old* values, not the current ones. > But, until now, HTTP::DAV::UserAgent::credentials() has always returned > the *current* values. And I seem to understand that HTTP::DAV relies on > the latter for some inner workings (HTTP::DAV::Comms::credentials(), > and probably other points in the code). >

I think that LWP::UserAgent::credentials() does the same. The difference is that H::D::C::credentials() is *always* called with 4 parameters including $user and $pass. So you apply new values and return them. L::U::credentials() is sometime called with 2 parameters only, so nothing new to apply then and it returns the "old" values which are the current ones in fact. Show quoted text

> > If you have a test script that I can use, please send it along. > And thanks for your help so far.

I attached a stripped down version of my script. You most likely have to change the constant DUMP_FILE (which is the local file). Usage: dav-test.pl -p passwd -f http://server/path/file.txt -u user HTH

Download dav-test.pl.gz
application/x-gzip 1.2k

Message body not shown because it is not plain text.

Tue Feb 24 18:19:47 2009 OPERA [...] cpan.org - Correspondence added 120 min

Version 0.36 is on its way to CPAN. I'd be glad if you could test it and confirm it works for you. I tried to add a test case as t/9_RT_42877.t.

Thu Feb 26 03:47:26 2009 grobie [...] perlwizard.de - Correspondence added

From:

grobie [...] perlwizard.de

On Tue Feb 24 18:19:47 2009, OPERA wrote: Show quoted text

> Version 0.36 is on its way to CPAN. > I'd be glad if you could test it and confirm it works for you. > I tried to add a test case as t/9_RT_42877.t.

I'd tested the new version and it works perfectly fine for me. I think this ticket can be closed.

Thu Feb 26 03:48:14 2009 grobie [...] perlwizard.de - Status changed from 'open' to 'resolved'

Thu Feb 26 03:48:14 2009 grobie [...] perlwizard.de - Fixed in 0.36 added