Fix davx5 missing e-tag error.

aharter commented 3 years ago

This PR should fix #3, #6 and #8.

I haven't checked the RFC, so I'm not sure if this is according to standard. Especially the filter functions. Which is why I opened it as a draft. Let's discuss.

HeikoTheissen commented 3 years ago

This fixes the two problems I reported in my comments.

sedenardi commented 3 years ago

Thanks for the PR, sorry for the delay in reviewing. A couple things:

Can you post the request and response bodies before and after this change so we can ensure they're still conforming to the RFC? I don't have an Android device to test it with so I have to rely on yall to show what is being requested and returned.
There are a few coding style changes that need to be made to make the PR code consistent with the rest of the repository.

Once those things are addressed, I'll review the changes and the examples provided, as well as test the changes to make sure they still work with the iOS and MacOS CalDAV implementations (which was the original intent of this library).

To link other issues (only #3 was linked in the first PR comment):

Fixes #6 Fixes #8

aharter commented 3 years ago

I made the requested style changes.

I also changed the eventFilter logic from (!end || end < eventStart) || (!start || start > eventEnd) to (!start || start <= eventEnd) && (!end || end >= eventStart) after re-reading the RFC.

Here is the before and after comparison of request/response: REQUEST PROPFIND /caldav/cal/user@ex.co/exampleCal1/

<?xml version='1.0' encoding='UTF-8' ?>
<CAL:calendar-query xmlns="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav">
  <prop>
    <getetag />
  </prop>
  <CAL:filter>
    <CAL:comp-filter name="VCALENDAR">
      <CAL:comp-filter name="VEVENT">
        <CAL:time-range start="20201004T154527Z" />
      </CAL:comp-filter>
    </CAL:comp-filter>
  </CAL:filter>
</CAL:calendar-query>

RESPONSE BEFORE

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:supported-report-set>
          <D:supported-report>
            <D:report>
              <CAL:calendar-query/>
            </D:report>
          </D:supported-report>
          <D:supported-report>
            <D:report>
              <CAL:calendar-multiget/>
            </D:report>
          </D:supported-report>
          <D:supported-report>
            <D:report>
              <CAL:sync-collection/>
            </D:report>
          </D:supported-report>
        </D:supported-report-set>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/41c6fcc0-aea7-304f-983a-a6fb7e568109.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/faf52014-372e-44ed-9eba-2fc1a2b8d82d.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/895a04b2-30df-4927-ba03-dfc55514f6e7.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/30946424-40f4-47e2-80b1-006fdebbeb25.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
</D:multistatus>

RESPONSE AFTER

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/41c6fcc0-aea7-304f-983a-a6fb7e568109.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/faf52014-372e-44ed-9eba-2fc1a2b8d82d.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/895a04b2-30df-4927-ba03-dfc55514f6e7.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/30946424-40f4-47e2-80b1-006fdebbeb25.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getetag>20201122T100456Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
</D:multistatus>

Note The missing parts in the before response (with <D:supported-report-set>) was generated by the previous request

sedenardi commented 3 years ago

Apologies for the delay in reviewing this, I'll take some time in the next few days to take a look and give my feedback.

sedenardi commented 3 years ago

@aharter When I use curl to send the request in your example, I can't reproduce your findings. The responses on master and your branch are identical.

curl request

curl \
  --request PROPFIND \
  -H "Content-Type: text/xml" \
  --compressed \
  --url "http://localhost/caldav/cal/user@ex.co/exampleCal1/" \
  -u "user@ex.co:pass" \
  --data-binary @- << EOF
<?xml version='1.0' encoding='UTF-8' ?>
<CAL:calendar-query xmlns="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav">
  <prop>
    <getetag />
  </prop>
  <CAL:filter>
    <CAL:comp-filter name="VCALENDAR">
      <CAL:comp-filter name="VEVENT">
        <CAL:time-range start="20201004T154527Z" />
      </CAL:comp-filter>
    </CAL:comp-filter>
  </CAL:filter>
</CAL:calendar-query>
EOF

Response

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 404 Not Found</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/41c6fcc0-aea7-304f-983a-a6fb7e568109.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/faf52014-372e-44ed-9eba-2fc1a2b8d82d.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal1/895a04b2-30df-4927-ba03-dfc55514f6e7.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
    </D:propstat>
  </D:response>
</D:multistatus>

A couple things I noticed in the request

<prop> should be <A:prop>, right? Did you write the request, or did it come from the app?
<getetag /> should be <A:getetag /> - same as above

but changing those doesn't seem to have any effect.

The getetag element isn't being parsed properly, because it's not in the element array when the XML body is parsed.

However, the etag is present in the response when I make this request:

curl request

curl \
  --request PROPFIND \
  -H "Content-Type: text/xml" \
  --compressed \
  --url "http://localhost:3001/caldav/cal/user@ex.co/exampleCal2/" \
  -u "user@ex.co:pass" \
  --data-binary @- << EOF
<?xml version="1.0" encoding="UTF-8"?>
<A:propfind xmlns:A="DAV:">
  <A:prop>
    <A:getcontenttype/>
    <A:getetag/>
  </A:prop>
</A:propfind>
EOF

response

<?xml version="1.0"?>
<D:multistatus xmlns:D="DAV:" xmlns:CAL="urn:ietf:params:xml:ns:caldav" xmlns:CS="http://calendarserver.org/ns/" xmlns:ICAL="http://apple.com/ns/ical/">
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/b5292608-2548-0041-8d7a-d7d6257d0b58.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
        <D:getetag>20210205T043707Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/a3f14917-7607-d94b-bcd8-1d0a9cf408f8.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
        <D:getetag>20210205T043707Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
  <D:response>
    <D:href>/caldav/cal/user@ex.co/exampleCal2/30946424-40f4-47e2-80b1-006fdebbeb25.ics</D:href>
    <D:propstat>
      <D:status>HTTP/1.1 200 OK</D:status>
      <D:prop>
        <D:getcontenttype>text/calendar; charset=utf-8; component=VEVENT</D:getcontenttype>
        <D:getetag>20210205T043707Z</D:getetag>
      </D:prop>
    </D:propstat>
  </D:response>
</D:multistatus>

So it's either I'm incorrectly implementing the RFC (by looking for prop elements only if they're nested in a propfind element), or whatever client you're using is sending improper requests.

To be clear, I largely wrote this library based on the requests and responses that the MacOS/iOS clients send, and used the RFC as as clarifying guide. I'll try to dig into the RFC later on and see whether I should be considering prop element requests when not nested inside of a propfind element request, but I encourage you to do so as well.

I also apologize again for the long delay. Work mixed with having to re-familiarize myself with the library, as I haven't done significant work on it since middle of 2019.

sedenardi commented 3 years ago

I've tested this PR with MacOS and iOS, and everything looks good. I made a small comment regarding code conventions, but other than that it looks ready to mark as "Ready for review". Once you've done that I'll do another pass and merge if accepted.

Thanks again for your help and patience.

aharter commented 3 years ago

Thanks for taking the time to review the PR. I resolved your comments, if you have further additions please let me know. Next step for me would be figuring out, whether I port my application to koa or create some PR support expressjs. I guess for the later your thoughts around it would be crucial.

EDIT: I also took the liberty to rebase my changes on top of the current master

sedenardi commented 3 years ago

@aharter @HeikoTheissen Thanks to you both for your contributions. I've just published a new version to npm at 4.2.0.

Next step for me would be figuring out, whether I port my application to koa or create some PR support expressjs. I guess for the later your thoughts around it would be crucial.

I originally wrote the library with intention to expand it with express middleware support. It wouldn't take too much work to generalize the route and method handler functions to take in an object with context/request fields (not exhaustive):

method
url
params (raw and parsed)
authenticated user

and I'd certainly support such an effort if you wanted to take it on.

forwardemail / caldav-adapter