andig / carddav2fb

Download CardDAV VCards and upload as phonebook to AVM FRITZ!Box
63 stars 19 forks source link

Support for digest access authentication #55

Closed spandi closed 6 years ago

spandi commented 6 years ago

I had some issues with the script downloading carddav entries from a Baikal server (see #54 ) due to the fact that Baikal was configured using digest access authentication instead of basic authentication.

It would be nice if the script would support digest access authentication as well. Looking at http://docs.guzzlephp.org/en/stable/request-options.html#auth it seems that GuzzleHttp is supporting this already.

Actually I tried to modify CardDav/Backend.php according to the docs above by adding a CurlHandler (http://docs.guzzlephp.org/en/stable/handlers-and-middleware.html), but it did not work for me. The setup is correct as I'm using digest access authentication on Mac (Contacts) and Android successfully for years so far. As I'm not a php developer it could very well be that I did something wrong. It also looks like the current approach with using

$credentials = base64_encode($this->username.':'.$this->password);
$request = $request->withHeader('Authorization', 'Basic '.$credentials);

is not quite matching the doc above ( see CardDav/Backend.php:226).

If someone could give some advice or even implement this I would be happy do finally test it.

andig commented 6 years ago

It would be nice if the script would support digest access authentication as well.

Sounds like a valid request.

... is not quite matching the doc above

Not sure what you mean- basic auth seems to work fine when the server is configured to support it?

If someone could give some advice or even implement this I would be happy do finally test it.

Could you show what you've tried sofar?

After looking at http://docs.guzzlephp.org/en/stable/request-options.html#auth this should be simple- instead of building the headers manually (what you quoted above) use the intentended guzzle auth parameter for that purpose.

For a proper PR setAuth() should be extended to accept a type parameter that should be digest for this case and null for basic auth.

andig commented 6 years ago

See if https://github.com/andig/carddav2fb/pull/56 works for you- totally untested...

spandi commented 6 years ago

Thanks a lot for the fix. Unfortunately I get 401 for both methods (basic auth and digest access auth). Please also note the comment on http://docs.guzzlephp.org/en/stable/request-options.html#auth -> digest. Seems that this is only working with the CurlHandler. I adapted the code to use the CurlHandler when creating the client, but no luck. The basic auth should anyway work independent of this (tried with and without CurlHandler) so it seems that there is some general problem with the change. Maybe if the basic auth works the digest access auth will work as well?

Not sure what you mean- basic auth seems to work fine when the server is configured to support it? I was referring to the auth parameter being mentioned in the docs instead of using the Authorization header. So your change in the pull request is how I would expect it to work. Unfortunately I don't have a clue why that's not working as expected.

If I can test some more changes please let me know. Thx

andig commented 6 years ago

I've updated the PR to properly set options on client instead of header. I've verified that that works for basic auth. It does not however work for digest auth. Looks as if the underlying curl library is not able to perform digest auth and/or there is a bug in either guzzle or php. The server simply never receives the auth headers in case of digest auth (tested with PHP 7.1.8). For time being you'll have to resort to basic auth.

andig commented 6 years ago

@spandi this might just be because my server is not using digest auth. I've verified that guzzle actually sends the required headers.

Could you retry current PR?

spandi commented 6 years ago

@andig Yes, I have installed the curl module. With the latest commit I can confirm the both authentication methods work like a charm, also digest auth (verified with my Baikal server) 👍. Seems it's not needed to set the CurlHandler explicitly.

Thanks a lot for fixing this issue.