googol42 commented 2 years ago

System info

SOMEDOMAIN is a nextcloud instance running on 24.0.3 which is as of August 4th the latest stable release
caldav version was v0.9.1 and later master (8d9a36d004d983b2423e8d33d756ecbc0022e8c5)
python version: 3.6.8

Problem

It seems that a wrong url is used in the principal and thus a wrong calendar url is used.

Details

Assume the following:

user1, has some calendars, some are shared with user2, some are not. user1 does not have a calendar called "cal".
user2, has a calendar called "cal" which is not shared. (user2 also shares some calendars with user1, but I think this does not matter here.)

In the following program user2 wants to do something with his callendar called "cal":

#!/usr/bin/env python3
# -*- coding: utf-8 -*-

import caldav
#from caldav_git import caldav

def debug(client):
    principal = client.principal()
    print(principal)
    calendar = principal.calendar('cal', 'cal')
    print(calendar)
    #print(len(calendar.todos()))

if __name__ == '__main__':
    user_password_tuple1 = ('user1', 'hidden')
    user_password_tuple2 = ('user2', 'hidden')
    user, password = user_password_tuple2

    try:
        with caldav.DAVClient(f'https://{user}:{password}@SOMEDOMAIN/remote.php/dav/') as client:
            debug(client)
    except AttributeError as _: # __enter__ is not present in old versions
        client = caldav.DAVClient(f'https://{user}:{password}@SOMEDOMAIN/remote.php/dav/')
        debug(client)

The test program should print out this:

https://SOMEDOMAIN:443/remote.php/dav/principals/users/user2/
https://SOMEDOMAIN:443/remote.php/dav/calendars/user2/cal/

but prints this:

https://SOMEDOMAIN:443/remote.php/dav/principals/users/user1/
https://SOMEDOMAIN:443/remote.php/dav/calendars/user1/cal/

and the following exception is thrown when you uncomment the commented line in the test program (I guess because user1 does not have access to the calendar "cal"):

Traceback (most recent call last):
  File "test.py", line 23, in <module>
    debug(client)
  File "test.py", line 12, in debug
    print(len(calendar.todos()))
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 872, in todos
    matches1 = self._fetch_todos(filters1)
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 845, in _fetch_todos
    return self.search(root, comp_class=Todo)
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 812, in search
    (response, objects) = self._request_report_build_resultlist(xml, comp_class)
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 780, in _request_report_build_resultlist
    results = response.expand_simple_props(props_)
  File "/home/someone/caldav-debug/caldav_git/caldav/davclient.py", line 284, in expand_simple_props
    self.find_objects_and_props()
  File "/home/someone/caldav-debug/caldav_git/caldav/davclient.py", line 207, in find_objects_and_props
    responses = self._strip_to_multistatus()
  File "/home/someone/caldav-debug/caldav_git/caldav/davclient.py", line 143, in _strip_to_multistatus
    if tree.tag == "xml" and tree[0].tag == dav.MultiStatus.tag:
AttributeError: 'NoneType' object has no attribute 'tag'

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "test.py", line 26, in <module>
    debug(client)
  File "test.py", line 12, in debug
    print(len(calendar.todos()))
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 872, in todos
    matches1 = self._fetch_todos(filters1)
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 845, in _fetch_todos
    return self.search(root, comp_class=Todo)
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 812, in search
    (response, objects) = self._request_report_build_resultlist(xml, comp_class)
  File "/home/someone/.local/lib/python3.6/site-packages/caldav/objects.py", line 780, in _request_report_build_resultlist
    results = response.expand_simple_props(props_)
  File "/home/someone/caldav-debug/caldav_git/caldav/davclient.py", line 284, in expand_simple_props
    self.find_objects_and_props()
  File "/home/someone/caldav-debug/caldav_git/caldav/davclient.py", line 207, in find_objects_and_props
    responses = self._strip_to_multistatus()
  File "/home/someone/caldav-debug/caldav_git/caldav/davclient.py", line 143, in _strip_to_multistatus
    if tree.tag == "xml" and tree[0].tag == dav.MultiStatus.tag:
AttributeError: 'NoneType' object has no attribute 'tag'

I did some more debugging:

$ # cd to the dir where the test program is located.
$ git clone git@github.com:python-caldav/caldav.git caldav_git
$ touch caldav_git/__init__.py
$ # replace the caldav import by "from caldav_git import caldav" in the test program

I then run git bisect and I identified commit 164f88d8551f7b768edc983a36f351dbd3075ec4 as first bad commit. See the bisect log below:

$ git bisect log
git bisect start
# bad: [8d9a36d004d983b2423e8d33d756ecbc0022e8c5] Allow requests connection timeout to be set
git bisect bad 8d9a36d004d983b2423e8d33d756ecbc0022e8c5
# good: [40d1c52f064d05d554bd9c6d916984a00d888a83] version number 0.8.2, with changelog
git bisect good 40d1c52f064d05d554bd9c6d916984a00d888a83
# good: [541daa244ef426bf9cb5587f8a53e3ffba6b1421] this is the complete (?) changelog for v0.8.1
git bisect good 541daa244ef426bf9cb5587f8a53e3ffba6b1421
# bad: [4628bbc566d5db676759e6529f03c50c237dd44a] I regret writing the testTodoDateSearch :-)  Tuned compatibility flags to let Zimbra pass the test
git bisect bad 4628bbc566d5db676759e6529f03c50c237dd44a
# bad: [cbca28592a4ccbf9f06601fdbbf2656e4afff17f] changelog
git bisect bad cbca28592a4ccbf9f06601fdbbf2656e4afff17f
# good: [eb708a95605bcda5b4db652bbd27e1461f0be61b] silly stupid bugfix: the textual representation of any error was hardcoded to AuthorizedError in the base class
git bisect good eb708a95605bcda5b4db652bbd27e1461f0be61b
# good: [29e2dd3073c70fe15f83355a6f95f10077bce317] make sure to remove the test calendars properly when doing testing
git bisect good 29e2dd3073c70fe15f83355a6f95f10077bce317
# good: [9328d1cccf30c8e661a150aa2630b284b0e075c0] new test for to_wire
git bisect good 9328d1cccf30c8e661a150aa2630b284b0e075c0
# bad: [164f88d8551f7b768edc983a36f351dbd3075ec4] complete rewrite for working around https://github.com/python-caldav/caldav/issues/158 and fixing the digestauth vs basicauth discovery
git bisect bad 164f88d8551f7b768edc983a36f351dbd3075ec4
# good: [08136b60d6c488ce94c1fd9f8f34425abb8af7d8] changelog
git bisect good 08136b60d6c488ce94c1fd9f8f34425abb8af7d8
# first bad commit: [164f88d8551f7b768edc983a36f351dbd3075ec4] complete rewrite for working around https://github.com/python-caldav/caldav/issues/158 and fixing the digestauth vs basicauth discovery

As it worked in older versions, so I assume it is not a problem with nextcloud. Or am I using the libary incorrectly?

tobixen commented 2 years ago

First of all, I would like you to print out principal.url and see if it's correct or not.

Thanks for the research - if it wasn't for the bisect, I would probably have dismissed it as something funky with Nextcloud or the user setup in Nextcloud. Well, probably it is something funky with (your) Nextcloud, but this is also a regression issue, hence I think it's important to get it working again - though I don't think I will have much capacity to look properly into it in August. If you could figure out of it and produce a pull request it would be awesome :-)

Simple workaround

The Principal object can be constructed, and an URL can be passed - the constructor looks like this:

    def __init__(self, client=None, url=None):
        """
        Returns a Principal.

        Parameters:
         * client: a DAVClient() oject
         * url: Deprecated - for backwards compatibility purposes only.

        If url is not given, deduct principal path as well as calendar home set
        path from doing propfinds.
        """

... so basically, just replace principal = client.principal() with principal = Principal(client=client, url=...)

Background and hypothesis

The digest auth vs basic auth vs other auths has caused me really lots of troubles, and I thought I had finally nailed it. Since this is on the ordinary http layer, I think the requests library ought to have taken care of it for me, but unfortunately not.

The old logic would first attempt to do a basic auth, then a digest auth if basic auth didn't work out, but this logic broke on one calendar server. Then I made some stupid workarounds to try to get it working on that server - but this workaround caused it to fail on other servers. In commit 164f88d8551f7b768edc983a36f351dbd3075ec4 I replaced the "stupid workaround" with a more proper refactoring. Unfortunately this refactoring again caused things to break at various servers, several bugfixes and adjustments have been done to fix issues on different calendar servers after that.

The new logic first attempts to do the request without auth, and fixes auth according to instructions from the server after getting a 401. My hypothesis is that this breaks on your NextCloud, that it grants you access as a guest user or something like that when doing a propfind without auth.

Suggestion for fix

I'm not quite sure on this one - and it's needed to be very careful here as what fixes your issue may break at other servers. At the other hand, this is a regression issue, if it was working as of 0.8.x, I want it to work without modifications on the client side in the next patch.

googol42 commented 2 years ago

Thanks for your feedback.

First of all, I would like you to print out principal.url and see if it's correct or not.

I already printed the urls in the test program above.

that it grants you access as a guest user or something like that when doing a propfind without auth.

I don't have guest accounts on this instance. The calendars are all private, but some are shared with the other user. And the principal url is the one for the other user.

I check the db and the oc_calendars table in the nextcloud db. And the principal uris are right. I quickly set up a test nextcloud instance and tried to reproduce it there, but I couldn't :-( So I guess I have to set up a new (prod) nextcloud instance and to migrate my data. And I don't consider the nextcloud caldav implementation very good, at least it causes problems from time to time. So I can imagine that this might be a nextcloud issue.

The old logic would first attempt to do a basic auth, then a digest auth if basic auth didn't work out, but this logic broke on one calendar server. Then I made some stupid workarounds to try to get it working on that server - but this workaround caused it to fail on other servers. In commit https://github.com/python-caldav/caldav/commit/164f88d8551f7b768edc983a36f351dbd3075ec4 I replaced the "stupid workaround" with a more proper refactoring. Unfortunately this refactoring again caused things to break at various servers, several bugfixes and adjustments have been done to fix issues on different calendar servers after that.

Doesn't sound like fun :-|

When I can provide any information you need, please let me know. If I don't here anything from you I'll set up a new instance some time (maybe at the end of August)

tobixen commented 2 years ago

Thanks for your feedback.

First of all, I would like you to print out principal.url and see if it's correct or not.

I already printed the urls in the test program above.

Right, sorry for that ... I only saw the calendar URL. So it actually considers that the "current principal" is user1, even if you're logged in as user2. That's kind of weird. Very weird indeed.

And I don't consider the nextcloud caldav implementation very good,

Much better than the one in Zimbra at least.

To be honest, I think the caldav protocol kind of sucks, and most servers fail at some point to adhere to the standard. I really didn't want to touch it, so I hoped that by using the python caldav library I wouldn't have to dive into the details of the standard, Unfortunately the library didn't work well enough for me, so I needed to fix some things, and somehow I got stuck with the maintainer role :p

at least it causes problems from time to time. So I can imagine that this might be a nextcloud issue.

Seems so. But still it's also a regression issue.

Doesn't sound like fun :-|

When I can provide any information you need, please let me know. If I don't here anything from you I'll set up a new instance some time (maybe at the end of August)

It may be that I can find some time to do research on this those days - but I would need access to the test accounts to look more into it.

googol42 commented 2 years ago

Do you cache connections/sessions/credentials somehow/somewhere? When I change the password for "user1" then everything works as expected. Which is strange because the test program never logins in with the credentials for "user1".

change the password for "user1"
remove all entries from oc_authtoken
run the test program: now everything works as expected
change the password back to the old one
remove all entries from oc_authtoken
run the test program. It does not work as expected
check oc_authtoken again: there is only one entry, but for the wrong user. (for user1 which I never logged in via the test program.)

I also checked the access log and your library does a PROPFIND for the wrong user. This is the entry with a fresh password:

SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:15:57:40 +0200] "PROPFIND /remote.php/dav/ HTTP/1.1" 401 381 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:05 +0200] "PROPFIND /remote.php/dav/ HTTP/1.1" 207 240 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:31 +0200] "PROPFIND /remote.php/dav/principals/users/test2/ HTTP/1.1" 207 288 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:31 +0200] "PROPFIND /remote.php/dav/calendars/test2/ HTTP/1.1" 207 638 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:31 +0200] "REPORT /remote.php/dav/calendars/test2/cal/ HTTP/1.1" 207 1781 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:31 +0200] "REPORT /remote.php/dav/calendars/test2/cal/ HTTP/1.1" 207 844 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:32 +0200] "REPORT /remote.php/dav/calendars/test2/cal2/ HTTP/1.1" 207 12008 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:32 +0200] "REPORT /remote.php/dav/calendars/test2/cal2/ HTTP/1.1" 207 2493 "-" "Mozilla/5.0"
....
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:32 +0200] "REPORT /remote.php/dav/calendars/test2/cal5_shared_by_test1/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:32 +0200] "REPORT /remote.php/dav/calendars/test2/cal5_shared_by_test1/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal7_shared_by_test1/ HTTP/1.1" 207 6751 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal7_shared_by_test1/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal8_shared_by_test1/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal8_shared_by_test1/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal09_shared_by_test1/ HTTP/1.1" 207 2261 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal09_shared_by_test1/ HTTP/1.1" 207 1936 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal10-shared_by_test1/ HTTP/1.1" 207 10519 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test2 [04/Aug/2022:15:58:33 +0200] "REPORT /remote.php/dav/calendars/test2/cal10-shared_by_test1/ HTTP/1.1" 207 4016 "-" "Mozilla/5.0"

This is the log entry, with the old password:

SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:46 +0200] "PROPFIND /remote.php/dav/ HTTP/1.1" 207 242 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:47 +0200] "PROPFIND /remote.php/dav/principals/users/test1/ HTTP/1.1" 207 292 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:47 +0200] "PROPFIND /remote.php/dav/calendars/test1/ HTTP/1.1" 207 618 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:47 +0200] "REPORT /remote.php/dav/calendars/test1/cal1/ HTTP/1.1" 207 2233 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:48 +0200] "REPORT /remote.php/dav/calendars/test1/cal1/ HTTP/1.1" 207 1894 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:48 +0200] "REPORT /remote.php/dav/calendars/test1/cal2/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:48 +0200] "REPORT /remote.php/dav/calendars/test1/cal2/ HTTP/1.1" 207 250 "-" "Mozilla/5.0"
...
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:49 +0200] "REPORT /remote.php/dav/calendars/test1/cal09/ HTTP/1.1" 207 10407 "-" "Mozilla/5.0"
SOMEDOMAIN:443 SOMEIP - test1 [04/Aug/2022:16:03:49 +0200] "REPORT /remote.php/dav/calendars/test1/cal09/ HTTP/1.1" 207 3946 "-" "Mozilla/5.0"

(The calender names have obviously been renamed by me and they don't match up anymore. So a calender shared by one use might be missing in the logs.)

So either you library is broken or my nextcloud instance is utterly broken. But I run

php -f occ integrity:check-app calendar
php -f occ integrity:check-app tasks
php -f occ integrity:check-core

and none of them reported an error.

googol42 commented 2 years ago

oh my goodness. I found it. Some time ago I created a netrc file to automatically backup calenders from nextcloud. And it interfered with python-caldav. I just had to rename .netrc to some other name. That was the reason I could not reproduce this on a test system. And this also the explanation why the test program works after changing the password (because then the password in netrc does not match and the login fails). 😅 Sorry for the noise.

tobixen commented 2 years ago

Do you cache connections/sessions/credentials somehow/somewhere?

The client object has a self.session variable

tobixen commented 2 years ago

oh my goodness. I found it. Some time ago I created a netrc file to automatically backup calenders from nextcloud. And it interfered with python-caldav

That's a bit strange. Also strange that your bisect could pinpoint which commit it was breaking stuff.

googol42 commented 2 years ago

That's a bit strange. Also strange that your bisect could pinpoint which commit it was breaking stuff.

You said that you changed the behaviour that you first don't pass any credentials. In such case the .netrc kicked in (I think) and then those credentials (respectively the returned cookie/token) are used. Does this make sense to you?

tobixen commented 2 years ago

You said that you changed the behaviour that you first don't pass any credentials. In such case the .netrc kicked in (I think) and then those credentials (respectively the returned cookie/token) are used. Does this make sense to you?

Seems to be correct, the requests library does support .netrc indeed.

I find it very weird that the requests library does support .netrc, but does not support passing username and password without explicitly telling it what kind of authentication mode to use. Or perhaps I've just missed something.

It's still some kind of regression issue, but it's also sort of a feature that username/password can be specified in .netrc. I should add some notes about that in the documentation.

tobixen commented 2 years ago

It is a feature that .netrc is used if no username or password is given. It's a bug that .netrc is honored when username and password is given.

googol42 commented 2 years ago

should I re-open the bug?

tobixen commented 2 years ago

I created another one instead.

python-caldav / caldav

Principal for nextcloud user is wrong #205

System info

Problem

Details

Simple workaround

Background and hypothesis

Suggestion for fix