Support custom user principal and home set paths #62

bitfehler · 2022-03-23T10:13:46Z

Currently, the user principal path and the home set path are both
hardcoded to "/", for both CalDAV and CardDAV. This poses a challenge if
one wishes to run a CardDAV and CalDAV server in the same server.

This commit introduces the concept of a UserPrincipalBackend. This
backend must provide the locations for the user principal as well as the
home sets. The CardDAV and CalDAV servers act accordingly.

The individual servers will continue to work as before (including the
option of keeping everything at "/"). If one wishes to run CardDAV and
CalDAV in parallel, the new webdav.ServeUserPrincipal() can be used,
which will use the UserPrincipalBackend interface to serve a common user
principal for both servers (if so desired, can be used for a single one
also).

Note that the storage backend will have to know about these paths as
well. For any non-trivial use case, a storage backend should probably
have access to the same UserPrincipalBackend. That is, however, an
implementation detail and doesn't have to be reflected in the
interfaces.

bitfehler · 2022-03-23T10:20:15Z

With this, I have successfully created a single server on which Evolution finds both a calendar as well as an address book. I also tested that using e.g. a simple carddav.Handler by itself still works with a UserPrincipalBackend implementation that always returns /. Forgot to mention in commit message, but this should take care of #27.

I also managed to create a setup with dynamic user principal paths, so you could have e.g. /emersion/, I would have /bitfehler/. The storage backend must be able to handle this, of course.

emersion · 2022-03-23T11:07:20Z

server.go

+
+// ServeUserPrincipal replies to discovery requests. It returns false if the
+// request wasn't handled.
+func ServeUserPrincipal(w http.ResponseWriter, r *http.Request, backend UserPrincipalBackend) bool {


I wonder if we should just take a struct instead of an interface here. For instance:

func ServeUserPrincipal(w http.ResponseWriter, r *http.Request, options *ServeUserPrincipalOptions) bool type ServeUserPrincipalOptions struct { Path string HomeSets map[string]HomeSet }

Should be very cheap to compute for the backend I think?

Not sure about the struct name yet -- if it's used by other functions and not just ServeUserPrincipal we should pick a more general name.

I think it could be used elsewhere, though it wouldn't have to if it's inconvenient. I was actually at some point considering attaching such a struct to the context. Maybe even use a middleware to do this?

Actually, we also need the interface to pass on to the internal.Handler, which needs it (in general, not so much for this function), so I guess we might as well stick to it? I might also be misunderstanding what you are aiming for.

I guess what I'm trying to get to is that UserPrincipalBackend is a bit overkill for the purposes of ServeUserPrincipal. We don't actually need the complexity of an interface with multiple functions here, a simple ServeUserPrincipalOptions struct with the info in it would be enough.

That said, it's true that CardDAV and CalDAV backends also need to access this info, and embedding UserPrincipalBackend in their backends is a simple way to require that. If we use a struct instead of an interface, we'd have to add

UserPrincipal(context.Context) *ServeUserPrincipalOptions

to the CardDAV and CalDAV backend interfaces. At which point the name ServeUserPrincipalOptions is maybe not a good name, maybe something else like UserPrincipalOptions or UserPrincipalInfo would be more fitting.

Just thinking out loud here, not sure what the best solution is, and I'm probably thinking too much as I often do. :P

server.go

emersion · 2022-04-08T16:35:56Z

server.go

@@ -234,3 +237,128 @@ func (b *backend) Move(r *http.Request, dest *internal.Href, overwrite bool) (cr
 	}
 	return created, err
 }
+
+type HomeSet interface {


I'm not super happy about this interface, because it's pretty easy to implement it externally outside of go-webdav, and implement it wrong by missing the fact that it needs to be XML-marshalable. To alleviate this, I've been thinking about making it impossible for external users to implement it. Usually this is done by adding an unexported func in the interface, e.g.

type HomeSet interface { homeSet() }

that way only the current package can implement it. But this wouldn't work here because the type is shared between our own packages, and the carddav package won't be able to implement the type.

Then I thought about a bunch of different solutions, like moving it to internal/ and doing more tricks to ensure only us can implement it. But it's pretty ugly and not worth the trouble.

tl;dr I think it's fine and we just need to document the type here. Explain what it represents, explain that it needs to marshal to XML to describe the home set element, etc.

emersion · 2022-04-08T16:42:18Z

server.go

@@ -234,3 +237,128 @@ func (b *backend) Move(r *http.Request, dest *internal.Href, overwrite bool) (cr
 	}
 	return created, err
 }
+
+type HomeSet interface {


This interface looks like it won't be easily re-usable in the DAV client. It would be nicer to find a good name to reflect this.

emersion · 2022-04-08T16:45:07Z

caldav/server.go

@@ -24,7 +25,8 @@ type Backend interface {
 // Handler handles CalDAV HTTP requests. It can be used to create a CalDAV
 // server.
 type Handler struct {
-	Backend Backend
+	Backend              Backend
+	UserPrincipalBackend webdav.UserPrincipalBackend


We could just embed webdav.UserPrincipalBackend into Backend to avoid dealing with multiple backends.

emersion · 2022-04-08T16:47:29Z

server.go

+// RelativeResourcePath returns the path of a resource without the prefix of
+// the home set that the resource is in. If the resource is not in any known
+// home set, the original request path is returned.
+func RelativeResourcePath(r *http.Request, backend UserPrincipalBackend) (string, error) {


Can we move RelativeResourcePath and IsResourceRequest to internal/? Or do we have good reasons to expose them?

bitfehler · 2022-04-13T18:33:43Z

Ok, I think I addressed all of your comments. I also shuffled the interfaces a bit (user principal backend is only responsible for user principal URL, carddav/caldav backend are responsible for home set path). I think the implementation is reasonably clean, given the complexity of the matter. The application implementation would also be pretty straightforward.
I am not sure about the home set interface naming, I came up with BackendSuppliedHomeSet, but I'll use whatever else if you have a suggestion.

If you give a thumbs up I am also happy to add some more documentation.

emersion · 2022-04-28T14:55:26Z

server.go

+// ServeUserPrincipal replies to discovery requests. It returns false if the
+// request wasn't handled.
+func ServeUserPrincipal(w http.ResponseWriter, r *http.Request, options ServeUserPrincipalOptions) bool {
+	if r.URL.Path == "/.well-known/carddav" || r.URL.Path == "/.well-known/caldav" {


Hm. I'm not a fan of hardcoding carddav and caldav here. Can we leave that to the appropriate packages?

Please open an issue with more details. I don't really understand what you're talking about here.

server.go

emersion · 2022-04-28T15:00:31Z

carddav/server.go

+	}
+	if !strings.HasPrefix(r.URL.Path, homeSetPath) {
+		return nil, &internal.HTTPError{Code: http.StatusMethodNotAllowed}
+	}


I'm not sure it's a good idea to add this logic. Servers may want to allow users to fetch/edit calendars which aren't under the home set. Additionally, strings.HasPrefix is not appropriate here -- the check passes with the path /abcdef and the home set /abc.

Ok, so a few things 🙂

Servers may want to allow users to fetch/edit calendars which aren't under the home set

I agree that this won't work like this, but as things are right now, this is outright impossible anyways (due to the home set being /). I think part of why this PR turned a bit tedious might be that I cannot quite seem to gauge your expectations. I initially had the impression you'd actually prefer to only do the bare minimum required to get the two servers working side-by-side, which would have just been allowing a home set prefix. I guess it is on me that I brought the user principal URL into this.

However, with this comment, I get the impression that now we're talking about laying the foundations for all kinds of things that have been previously impossible, such as accessing other people's calendars. This is fine, I just need to understand where we want to go.

Through #70 , I also realized that the backends should behave more like a HTTP backend (as opposed to e.g. map lookup or such). As such, my suggestion would be:

we remove that check entirely

as discussed in carddav: return 404 if contact not found #70, we expose some appropriate internal errors (not found, access denied, etc)

we document that the backend can have absolutely zero expectations w/ regards to the path it is asked about and has to check it itself and return the appropriate errors

WDYT?

PS: on a side note, I am quite disappointed that Go doesn't have a proper path.HasPrefix(), which is why I initially wrote those custom functions. But the check really would have never been good enough anyways, and the backend will have to perform them. So just removing it from here really makes sense...

However, with this comment, I get the impression that now we're talking about laying the foundations for all kinds of things that have been previously impossible, such as accessing other people's calendars. This is fine, I just need to understand where we want to go.

From my PoV, I'm just trying not to add any more assumptions which we will regret later on. The HasPrefix checks are new here, and I don't think it's something we want to have long-term.

Through #70 , I also realized that the backends should behave more like a HTTP backend (as opposed to e.g. map lookup or such).

Right. It's mostly about being able to return more detailed errors than "something failed". Just like os.Open errors can be introspected for instance.

WDYT?

LGTM!

I am quite disappointed that Go doesn't have a proper path.HasPrefix(), which is why I initially wrote those custom functions.

Yeah, I agree this is disappointing, as this is a common thing to do and it can be error-prone. Something like this can be used in backends:

func pathHasPrefix(p, prefix string) bool { p = path.Clean(p) prefix = path.Clean(prefix) return p == prefix || strings.HasPrefix(p, prefix + "/") }

Removed the checks as discussed and also addressed your other comments.

emersion · 2022-04-28T15:02:18Z

carddav/server.go

@@ -330,6 +374,18 @@ func (b *backend) Propfind(r *http.Request, propfind *internal.Propfind, depth i
 	return internal.NewMultistatus(resps...), nil
 }

+func (b *backend) propfindCommon(propfind *internal.Propfind, path, principalPath, homeSetPath string) (*internal.Response, error) {
+	props := map[xml.Name]internal.PropfindFunc{
+		addressBookHomeSetName: func(*internal.RawXMLValue) (interface{}, error) {


This only needs to be defined on the user principal.

emersion · 2022-05-02T18:45:08Z

server.go

+// ServeUserPrincipal replies to discovery requests. It returns false if the
+// request wasn't handled.
+func ServeUserPrincipal(w http.ResponseWriter, r *http.Request, options ServeUserPrincipalOptions) bool {
+	if r.Method != "PROPFIND" {


Hmm. If r.URL.Path != b.options.UserPrincipalPath we shouldn't handle the request, right? Should we return false here in that case?

(In which case, maybe we can just let the caller perform the path check and drop UserPrincipalPath?)

I thought the idea was that this handles everything up to, including the user principal URL. I've observed at least Evolution sending PROPSTAT to / during discovery, which then returns the user principal URL.

That's also why the .well-know handling was in there.

Hm I'd rather just handle the principal stuff in here (as the function name suggests).

Ok, I rewrote that function based on #74 (feel free to add yourself as co-author 🙂 ). I didn't take everything from there to not blow this up any more, I hope that's fine. Happy to add more bits and pieces afterwards.

emersion · 2022-05-02T18:50:12Z

caldav/server.go

 	var resps []internal.Response
-	if r.URL.Path == "/" {
+
+	resp, err := b.propfindCommon(propfind, r.URL.Path, principalPath)


Hmm. This is a bit weird. It produces multiple <response> elements for the same href potentially.

Yeah, sorry, I think I refactored that once too often. The composability is a bit weird there. Are you okay with how it is now?

I think in the long run, if we add more basic WebDAV properties, it might make sense to pass around pre-filled maps. But I tried that, and it would require a map copy in some places, so it's a bit ugly also. For now, it seemed best to just follow the current pattern but add the additional property everywhere...

Are you okay with how it is now?

Yeah sounds fine, let's keep it simple. Though, CardDAV seems fixed but not CalDAV.

Sorry, that got lost in rebase-land 😕 fixed now

Currently, the user principal path and the home set path are both hardcoded to "/", for both CalDAV and CardDAV. This poses a challenge if one wishes to run a CardDAV and CalDAV server in the same server. This commit introduces the concept of a UserPrincipalBackend. This backend must provide the path of the current user's principal URL from the given request context. The CalDAV and CardDAV backends are extended to also function as UserPrincipalBackend. In addition, they are required to supply the path of the respective home set (`calendar-home-set` and `addressbook-home-set`). The CardDAV and CalDAV servers act accordingly. The individual servers will continue to work as before (including the option of keeping everything at "/"). If one wishes to run CardDAV and CalDAV in parallel, the new `webdav.ServeUserPrincipal()` can be used as a convenience function to serve a common user principal URL for both servers. The input for this function can be easily computed by the application by getting the home set paths from the backends and using `caldav.NewCalendarHomeSet()` and `carddav.NewAddressbookHomeSet()` to create the home sets. Note that the storage backend will have to know about these paths as well. For any non-trivial use case, a storage backend should probably have access to the same UserPrincipalBackend. That is, however, an implementation detail and doesn't have to be reflected in the interfaces.

bitfehler · 2022-05-06T07:27:45Z

For what it's worth, you can see how the usage of all this could look like in its current state here: https://git.sr.ht/~sircmpwn/tokidoki/tree/master/item/cmd/tokidoki/main.go

Note that I am using more stuff that I wrote that I have not yet created PRs for, as it would conflict with this one, you can find that here: https://github.com/bitfehler/go-webdav/commits/bitfehler/more-caldav I am constantly rebasing that branch on the latest developments here, so should be easy to merge the rest once we arrive at a solution.

emersion · 2022-05-06T15:05:21Z

server.go

+	if err := internal.DecodeXMLRequest(r, &propfind); err != nil {
+		return err
+	}
+	props := map[xml.Name]internal.PropfindFunc{


We're missing a few principal props marked in the RFC as "MUST support", but we were missing them prior to this PR too. Let's keep that in mind for the future.

emersion · 2022-05-11T09:12:07Z

Thanks!

emersion reviewed Mar 23, 2022

View reviewed changes

server.go Outdated Show resolved Hide resolved

bitfehler force-pushed the bitfehler/custom-endpoints branch 2 times, most recently from 5ef61b2 to bba482a Compare March 29, 2022 14:29

emersion reviewed Apr 8, 2022

View reviewed changes

bitfehler mentioned this pull request Apr 11, 2022

Support custom user principal and home set paths (v2) #66

Closed

bitfehler force-pushed the bitfehler/custom-endpoints branch 2 times, most recently from 4afef1c to 9790163 Compare April 13, 2022 18:22

bitfehler force-pushed the bitfehler/custom-endpoints branch from 9790163 to 2f3e249 Compare April 22, 2022 07:41

emersion reviewed Apr 28, 2022

View reviewed changes

server.go Outdated Show resolved Hide resolved

emersion reviewed Apr 28, 2022

View reviewed changes

bitfehler force-pushed the bitfehler/custom-endpoints branch from 2f3e249 to 4927d7a Compare May 2, 2022 18:33

emersion reviewed May 2, 2022

View reviewed changes

bitfehler force-pushed the bitfehler/custom-endpoints branch 4 times, most recently from fc42929 to 56f724d Compare May 3, 2022 20:19

bitfehler force-pushed the bitfehler/custom-endpoints branch from 56f724d to 6b29ae3 Compare May 5, 2022 12:16

emersion reviewed May 6, 2022

View reviewed changes

emersion merged commit 6887b6b into emersion:master May 11, 2022

bitfehler deleted the bitfehler/custom-endpoints branch May 12, 2022 12:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support custom user principal and home set paths #62

Support custom user principal and home set paths #62

bitfehler commented Mar 23, 2022

bitfehler commented Mar 23, 2022

emersion Mar 23, 2022 •

edited

Loading

bitfehler Mar 25, 2022

bitfehler Mar 28, 2022

emersion Apr 8, 2022

emersion Apr 8, 2022

emersion Apr 8, 2022

emersion Apr 8, 2022

emersion Apr 8, 2022

bitfehler commented Apr 13, 2022

emersion Apr 28, 2022

emersion Jul 5, 2023

emersion Apr 28, 2022

bitfehler Apr 29, 2022

emersion Apr 29, 2022

bitfehler May 2, 2022

emersion Apr 28, 2022

emersion May 2, 2022

emersion May 2, 2022

bitfehler May 2, 2022

bitfehler May 2, 2022

emersion May 3, 2022

bitfehler May 3, 2022

emersion May 2, 2022

bitfehler May 3, 2022

emersion May 3, 2022

bitfehler May 3, 2022

bitfehler commented May 6, 2022

emersion May 6, 2022

emersion commented May 11, 2022

Support custom user principal and home set paths #62

Support custom user principal and home set paths #62

Conversation

bitfehler commented Mar 23, 2022

bitfehler commented Mar 23, 2022

emersion Mar 23, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitfehler commented Apr 13, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitfehler commented May 6, 2022

Choose a reason for hiding this comment

emersion commented May 11, 2022

emersion Mar 23, 2022 •

edited

Loading