diff --git a/caldav/base_client.py b/caldav/base_client.py index ac51c26b..de0bd703 100644 --- a/caldav/base_client.py +++ b/caldav/base_client.py @@ -446,7 +446,8 @@ def _raise_authorization_error(self, url_str: str, reason_source: Any) -> NoRetu precondition elements parsed out (issue #738) - a 403 with ``DAV:valid-sync-token`` means "resync", not "forbidden". ``reason_source`` is the raw HTTP response on both clients; httpx - names the reason ``reason_phrase``. + names the reason ``reason_phrase``. Its status is kept too, so a + caller can tell 401 (credentials) from 403 (permission). """ self._unwind_unprompted_basic() reason = getattr(reason_source, "reason", None) or getattr( @@ -462,6 +463,7 @@ def _raise_authorization_error(self, url_str: str, reason_source: Any) -> NoRetu reason=reason, preconditions=error.parse_preconditions(body), body=body, + status=error._status_of(reason_source), ) # ── Rate-limit handling ───────────────────────────────────────────────── diff --git a/caldav/calendarobjectresource.py b/caldav/calendarobjectresource.py index 20fbe91b..71aefa2d 100644 --- a/caldav/calendarobjectresource.py +++ b/caldav/calendarobjectresource.py @@ -1369,7 +1369,9 @@ def _raise_uid_conflict(e: error.DAVError, consistency: bool = False) -> None: ) else: cls = error.UID_CONFLICT_ERRORS.get(type(e), error.UidConflictError) - raise cls(e.url, e.reason, preconditions=e.preconditions, body=e.body) from e + raise cls( + e.url, e.reason, preconditions=e.preconditions, body=e.body, status=e.status + ) from e def _post_put( self, @@ -1896,6 +1898,13 @@ def _validate_save_constraints(self, existing, uid, no_overwrite, no_create): if not uid and no_create: raise error.ConsistencyError("no_create flag was set, but no ID given") if no_overwrite and existing: + ## The same UID under another URL is what the server's + ## CALDAV:no-uid-conflict precondition would refuse (issue #806) + target = self.url or (self._generate_url() if uid else None) + if existing.url and target and existing.url != target: + raise error.UidConflictConsistencyError( + "no_overwrite flag was set, but another object holds the UID" + ) raise error.ConsistencyError("no_overwrite flag was set, but object already exists") if no_create and not existing: raise error.ConsistencyError("no_create flag was set, but object does not exist") diff --git a/caldav/collection.py b/caldav/collection.py index a74335fe..9e40246f 100644 --- a/caldav/collection.py +++ b/caldav/collection.py @@ -1557,7 +1557,9 @@ def _post_multiget(self, results: Iterable[tuple[str, str, dict[str, str]]]) -> for url, data, tags in results ] - def multiget(self, event_urls: Iterable[URL], raise_notfound: bool = False) -> Iterable[_CC]: + def multiget( + self, event_urls: Iterable[URL], raise_notfound: bool = False + ) -> "list[_CC] | Coroutine[Any, Any, list[_CC]]": """ get multiple events' data TODO: Does it overlap the _request_report_build_resultlist method? diff --git a/caldav/davclient.py b/caldav/davclient.py index b82ef53e..7608d095 100644 --- a/caldav/davclient.py +++ b/caldav/davclient.py @@ -256,9 +256,6 @@ def __init__( self.auth = auth self.auth_type = auth_type - ## I had problems with passwords with non-ascii letters in it ... - if isinstance(self.password, str): - self.password = self.password.encode("utf-8") if auth and self.auth_type: logging.error( "both auth object and auth_type sent to DAVClient. The latter will be ignored." @@ -716,7 +713,7 @@ def build_auth_object(self, auth_types: list[str] | None = None) -> None: # Use shared selection logic auth_type = self._select_auth_type(auth_types) - # Decode password if it's bytes (HTTPDigestAuth needs string) + # A caller may still pass the password as bytes (HTTPDigestAuth needs a string) password = self.password if isinstance(password, bytes): password = password.decode("utf-8") diff --git a/caldav/elements/dav.py b/caldav/elements/dav.py index d6990e94..39126e23 100644 --- a/caldav/elements/dav.py +++ b/caldav/elements/dav.py @@ -97,6 +97,10 @@ class SupportedReportSet(BaseElement): tag = ns("D", "supported-report-set") +class CurrentUserPrivilegeSet(BaseElement): + tag: ClassVar[str] = ns("D", "current-user-privilege-set") + + class Response(BaseElement): tag: ClassVar[str] = ns("D", "response") diff --git a/caldav/lib/error.py b/caldav/lib/error.py index 1a2c4964..1dd26309 100644 --- a/caldav/lib/error.py +++ b/caldav/lib/error.py @@ -108,7 +108,8 @@ class DAVError(Exception): ``preconditions`` holds the Clark-notation tags of any precondition elements in a ``DAV:error`` response body (RFC 4918 section 16), and - ``body`` the raw response body (or None). A server may explain any + ``body`` the raw response body (or None), and ``status`` the HTTP + status code (or None) - e.g. 401 or 403 for an AuthorizationError. A server may explain any failure that way, whatever the status: e.g. ``["{DAV:}valid-sync-token"]`` for an expired sync token (RFC 6578 section 3.2), or ``["{urn:ietf:params:xml:ns:caldav}no-uid-conflict"]`` for a create whose @@ -124,6 +125,7 @@ def __init__( reason: str | None = None, preconditions: list[str] | None = None, body: bytes | str | None = None, + status: int | None = None, ) -> None: if url: self.url = url @@ -131,6 +133,7 @@ def __init__( self.reason = reason self.preconditions = list(preconditions) if preconditions else [] self.body = body + self.status = status def __str__(self) -> str: return "%s at '%s', reason %s" % ( @@ -191,9 +194,20 @@ def error_from_response( errmsg(response) if message is None else message, preconditions=parse_preconditions(body), body=body, + status=_status_of(response), ) +def _status_of(response: object) -> int | None: + """The HTTP status code of ``response``, which may be a ``DAVResponse`` + (``status``) or a raw requests/niquests/httpx response (``status_code``).""" + for attr in ("status", "status_code"): + status = getattr(response, attr, None) + if isinstance(status, int): + return status + return None + + class PropsetError(DAVError): pass @@ -312,8 +326,9 @@ def __init__( reason: str | None = None, retry_after: str | None = None, retry_after_seconds: float | None = None, + status: int | None = None, ) -> None: - super().__init__(url=url, reason=reason) + super().__init__(url=url, reason=reason, status=status) self.retry_after = retry_after self.retry_after_seconds = retry_after_seconds @@ -394,6 +409,7 @@ def raise_if_rate_limited( reason=f"Rate limited or service unavailable. Retry after: {retry_after_header}", retry_after=retry_after_header, retry_after_seconds=retry_seconds, + status=status_code, ) diff --git a/caldav/response.py b/caldav/response.py index 29821857..4c1b5a59 100644 --- a/caldav/response.py +++ b/caldav/response.py @@ -182,12 +182,24 @@ def _extract_properties(propstats: "list[_Element]") -> "dict[str, Any]": def _element_to_value(elem: _Element) -> Any: - """Convert a complex XML element to a Python value.""" - if len(elem) == 0: - return elem.text + """Convert a complex XML element to a Python value. + A property this does not know, with structure it cannot flatten into + a string or a list of strings (e.g. DAV:acl), is returned as the lxml + element itself. + """ tag = elem.tag + if tag in (dav.SupportedReportSet.tag, dav.CurrentUserPrivilegeSet.tag): + ## RFC 3253 section 3.1.5: supported-report/report/, and + ## RFC 3744 section 5.4: privilege/ - the names are wanted + return [ + leaf.tag for leaf in elem.iter(etree.Element) if len(leaf) == 0 and leaf is not elem + ] + + if len(elem) == 0: + return elem.text + if tag == cdav.SupportedCalendarComponentSet.tag: return [child.get("name") for child in elem if child.get("name")] diff --git a/changes/+no-overwrite-if-none-match.changed.md b/changes/+no-overwrite-if-none-match.changed.md index d0ec644e..7a7f09c3 100644 --- a/changes/+no-overwrite-if-none-match.changed.md +++ b/changes/+no-overwrite-if-none-match.changed.md @@ -1 +1 @@ -`save(no_overwrite=True)` and `save(no_create=True)` (and `add_event(..., no_overwrite=True)` and friends) have improved handling on RFC-compliant servers: the server enforces the flag at the same URL in the same request as the write, so another client can no longer slip in between the check and the PUT there. A refusal still raises `ConsistencyError`. +`save(no_overwrite=True)` and `save(no_create=True)` (and `add_event(..., no_overwrite=True)` and friends) have improved handling on RFC-compliant servers: the server enforces the flag at the same URL in the same request as the write, so another client can no longer slip in between the check and the PUT there. A refusal still raises `ConsistencyError`. When the refusal is because another object (at another URL) holds the UID, it is the subclass `UidConflictConsistencyError`, whether the server refused the UID or caldav's own UID search found it (https://github.com/python-caldav/caldav/issues/806). diff --git a/changes/795.fixed.md b/changes/795.fixed.md new file mode 100644 index 00000000..8f330771 --- /dev/null +++ b/changes/795.fixed.md @@ -0,0 +1 @@ +The `supported-report-set` and `current-user-privilege-set` properties from `propfind()` were the raw lxml element. They are now lists of the report and privilege names in Clark notation, e.g. `["{urn:ietf:params:xml:ns:caldav}calendar-query", "{DAV:}sync-collection"]`. Code that walked the returned element has to read the list instead. diff --git a/changes/799.changed.md b/changes/799.changed.md new file mode 100644 index 00000000..7b470f43 --- /dev/null +++ b/changes/799.changed.md @@ -0,0 +1 @@ +`DAVClient.password` is now the `str` that was passed in, as on `AsyncDAVClient`; it used to be encoded to UTF-8 `bytes`. The return type of `Calendar.multiget()` is annotated as a list, or a coroutine of one for an async client, like `search()`. diff --git a/changes/805.added.md b/changes/805.added.md new file mode 100644 index 00000000..81d4d627 --- /dev/null +++ b/changes/805.added.md @@ -0,0 +1 @@ +`DAVError` and its subclasses have a new `status` attribute with the HTTP status code of the failed response (or `None`), so an `AuthorizationError` tells a 401 (bad or missing credentials) from a 403 (authenticated, but not allowed). diff --git a/docs/design/TODO.md b/docs/design/TODO.md index 053ed6a6..2039fb8b 100644 --- a/docs/design/TODO.md +++ b/docs/design/TODO.md @@ -39,3 +39,11 @@ A response whose `Content-Type` says XML but whose body is not XML (an HTML erro ## A waiting session-lock claim blocks other threads `TestServer.claim_server()` in `tests/test_servers/base.py` polls for the flock (up to `PYTHON_CALDAV_TEST_SERVER_LOCK_TIMEOUT`, 30 minutes by default) while holding the module-wide `_session_locks_mutex`. A thread waiting for one busy server therefore blocks every other thread claiming a different server, which contradicts "runs against different servers proceed in parallel". Harmless for single-threaded pytest. Fix: hold the mutex only around the `_session_locks` checks and inserts, with a per-name lock against double claims. Found in the review of https://github.com/python-caldav/caldav/pull/776. + +## Config errors from extract_conn_params_from_section name no section + +`extract_conn_params_from_section()` in `caldav/config.py` passes the fixed name `"section"` to calendaring-config's `section_connection()`, so a failing `password_command` reads "password_command of section section exited with status 3" for plann users. Fix: an optional `section_name` parameter, passed on, and plann giving its section names. Found in the review of plann's switch to caldav 3.6, see https://github.com/python-caldav/caldav/issues/786. + +## calendar_multiget() fails on an async client + +`Calendar.calendar_multiget()` in `caldav/collection.py` does `list(self.multiget(...))`, but `multiget()` returns a coroutine for an async client, so the call raises `TypeError`. `calendar_multiget()` has no return annotation, so a type checker does not catch it. Fix: return `self.multiget(...)` for an async client (or annotate it like `multiget()`), with an async unit test. Found in the review of the fix for https://github.com/python-caldav/caldav/issues/799. diff --git a/tests/test_caldav_unit.py b/tests/test_caldav_unit.py index f35e75ef..e5a15fd9 100755 --- a/tests/test_caldav_unit.py +++ b/tests/test_caldav_unit.py @@ -3616,6 +3616,7 @@ def test_raise_if_rate_limited_429_with_header(self): error.raise_if_rate_limited(429, "http://x/", "30") assert exc_info.value.retry_after == "30" assert exc_info.value.retry_after_seconds == 30.0 + assert exc_info.value.status == 429 def test_raise_if_rate_limited_503_with_header(self): with pytest.raises(error.RateLimitError): @@ -3996,6 +3997,34 @@ def test_authorization_error_defaults(self): e = error.AuthorizationError(url="/", reason="Forbidden") assert e.preconditions == [] assert e.body is None + assert e.status is None + + @pytest.mark.parametrize("status", [401, 403]) + @mock.patch("caldav.davclient.requests.Session.request") + def test_status_is_kept(self, mocked, status): + """Issue https://github.com/python-caldav/caldav/issues/805: 401 (bad + or missing credentials) and 403 (not allowed) are both an + AuthorizationError, told apart by ``status``.""" + mocked.return_value = self._make_response(status) + client = DAVClient(url="https://cal.example.com/", auth=mock.MagicMock()) + with pytest.raises(error.AuthorizationError) as exc_info: + client.request("/") + assert exc_info.value.status == status + + def test_status_from_httpx_response(self): + """An httpx response names the reason ``reason_phrase``; the status + is ``status_code`` in all the HTTP libraries.""" + client = DAVClient(url="https://cal.example.com/") + r = mock.Mock(spec=["status_code", "reason_phrase", "content"]) + r.status_code, r.reason_phrase, r.content = 401, "Unauthorized", b"" + with pytest.raises(error.AuthorizationError) as exc_info: + client._raise_authorization_error("https://cal.example.com/", r) + assert exc_info.value.status == 401 + assert exc_info.value.reason == "Unauthorized" + + def test_error_from_response_keeps_status(self): + r = mock.Mock(status=404, reason="Not Found", raw="", _raw=b"") + assert error.error_from_response(error.NotFoundError, r).status == 404 def _calendar(self, report_error, sync_token_support=None): client = DAVClient(url="https://cal.example.com/") @@ -5592,7 +5621,7 @@ def test_url_with_user_but_no_password_does_not_crash(self): """DAVClient(url='https://user@host/', password='p') must not raise TypeError.""" client = DAVClient(url="https://user@cal.example.com/dav/", password="secret") assert client.username == "user" - assert client.password == b"secret" + assert client.password == "secret" def test_explicit_kwargs_take_precedence_over_url_credentials(self): """Explicit username/password kwargs must override credentials embedded in the URL.""" @@ -5602,7 +5631,7 @@ def test_explicit_kwargs_take_precedence_over_url_credentials(self): password="kwargpass", ) assert client.username == "kwarguser" - assert client.password == b"kwargpass" + assert client.password == "kwargpass" def test_explicit_username_does_not_borrow_url_password(self): """Gate finding F6: credentials are a pair, not two independent fields. @@ -5624,7 +5653,7 @@ def test_explicit_password_still_pairs_with_the_url_username(self): password="s3cret", ) assert client.username == "bob" - assert client.password == b"s3cret" + assert client.password == "s3cret" class TestRepeatedHrefCalendarData: @@ -7898,6 +7927,7 @@ def test_uid_conflict_is_also_consistency_error(self, base, status) -> None: self._run(self._event(client).save(no_overwrite=True)) assert isinstance(exc_info.value, error.ConsistencyError) assert exc_info.value.preconditions == ["{urn:ietf:params:xml:ns:caldav}no-uid-conflict"] + assert exc_info.value.status == status assert [m for m, _ in sent] == ["PUT"] def test_403_uid_conflict_is_also_consistency_error(self, base) -> None: @@ -7907,6 +7937,7 @@ def test_403_uid_conflict_is_also_consistency_error(self, base) -> None: reason="Forbidden", preconditions=[tag], body=UNIQUE_SCHEDULING_BODY, + status=403, ) client, _ = self._client(base, features=self.NO_UID_CONFLICT, exc=exc) with pytest.raises(error.UidConflictError) as exc_info: @@ -7914,6 +7945,26 @@ def test_403_uid_conflict_is_also_consistency_error(self, base) -> None: assert isinstance(exc_info.value, error.ConsistencyError) ## ... and still what earlier versions raised (issue #756) assert isinstance(exc_info.value, error.AuthorizationError) + assert exc_info.value.status == 403 + + def test_uid_found_elsewhere_by_search_is_uid_conflict(self, base) -> None: + """Issue https://github.com/python-caldav/caldav/issues/806: where + caldav searches for the UID itself, a hit under another URL is the + same UidConflictConsistencyError the server's precondition gives.""" + client, sent = self._client(base, report=EV1_MULTISTATUS) + with pytest.raises(error.UidConflictConsistencyError): + self._run(self._event(client).save(no_overwrite=True)) + assert [m for m, _ in sent] == ["REPORT"] + + def test_uid_found_at_own_url_by_search_is_plain_consistency_error(self, base) -> None: + """The object found is the one at the URL to save to: the name is + taken, as with a 412 to If-None-Match: *, not a UID conflict.""" + client, sent = self._client(base, report=EV1_MULTISTATUS) + event = self._event(client) + event.url = URL(self.URL + "ev1.ics") + with pytest.raises(error.ConsistencyError) as exc_info: + self._run(event.save(no_overwrite=True)) + assert not isinstance(exc_info.value, error.UidConflictError) def test_server_ignoring_if_none_match_keeps_search(self, base) -> None: """Where If-None-Match: * is known to overwrite (Zimbra, SOGo) the diff --git a/tests/test_protocol.py b/tests/test_protocol.py index e09d9220..c2503c49 100644 --- a/tests/test_protocol.py +++ b/tests/test_protocol.py @@ -329,6 +329,61 @@ def test_parse_sync_collection_generic_error(self): assert result.deleted == ["/cal/gone.ics"] assert result.changed == [] + def test_parse_nested_properties(self): + """Issue https://github.com/python-caldav/caldav/issues/795: properties + nested deeper than one level gave the raw lxml element.""" + xml = b""" + + + /calendars/user/calendar/ + + + + + + + + + + + + HTTP/1.1 200 OK + + + """ + + props = DAVResponse.from_bytes(xml).parse_propfind()[0].properties + + assert props["{DAV:}supported-report-set"] == [ + "{urn:ietf:params:xml:ns:caldav}calendar-query", + "{DAV:}sync-collection", + ] + assert props["{DAV:}current-user-privilege-set"] == [ + "{DAV:}read", + "{DAV:}write-content", + ] + + def test_parse_empty_nested_properties(self): + """No reports, no privileges: an empty list, not None.""" + xml = b""" + + + /calendars/user/calendar/ + + + + + + HTTP/1.1 200 OK + + + """ + + props = DAVResponse.from_bytes(xml).parse_propfind()[0].properties + + assert props["{DAV:}supported-report-set"] == [] + assert props["{DAV:}current-user-privilege-set"] == [] + def test_parse_complex_properties(self): """Parse complex properties like supported-calendar-component-set.""" xml = b"""