Closed alindeman closed 5 years ago
@alindeman thanks for this, I'll review it in detail a bit later today.
First question though - if the connector has no awareness of the prefix it's mounted at, how can it return working URLs from the body? e.g the webauthn connector is going to have a bunch of AJAX type stuff, if it say posts to /register
how will that get mapped to the specific connector? I think things like FileServer get away with this because the served content never has navigation.
First question though - if the connector has no awareness of the prefix it's mounted at, how can it return working URLs from the body? e.g the webauthn connector is going to have a bunch of AJAX type stuff, if it say posts to
/register
how will that get mapped to the specific connector? I think things like FileServer get away with this because the served content never has navigation.
I think a connector does need awareness of the path it's mounted at, and most connectors will need to be constructed with the return value from Server.ConnectorURL
. Something like:
server := New(...)
connector := &myConnector{
authenticator: server, // or server.Authenticator()
baseURL: server.ConnectorURL("connID"),
}
if err := server.AddConnector("connID", connector); err != nil {
// ...
}
The bit about StripPrefix
is mostly so that connectors can create muxes like Handle("/")
instead of Handle(prefix+"/")
or (in the case a mux is not used) have to parse the http.Request.URL
and try to remove prefix
at that time. In my experience, stripping the prefix in the place where the the http.Handler is mounted (e.g., main
) makes it easier to write and read an http.Handler because most of the path munging is in a single place.
If a connector needs to generate a link or redirect, it will have to prefix it with baseURL
(i.e., the return value from Server.ConnectorURL
it was passed). But this was necessary anyway.
Does that make sense or do you think I'm missing something?
@lstoll How do you feel about the changes in https://github.com/lstoll/deci/pull/28/commits/c96d3b1a226d3ca0602c1470ac559acdb48ab3e6?
I think I'm still leaning toward liking building a Connector fully before passing it to Server (i.e., passing it Server.Authenticator()
instead of Server
calling into [and mutating] the Connector later). But I'd also like your take.
I also think the main
(or whatever package creates Server
) handling mounting Connector.ServeHTTP
(if it's needed) is the right call, even if it's a bit more work for main
. But it has the most context about the HTTP server setup and it has to mount Server
anyway.
WDYT?
Merging for now to make progress. Happy to iterate on this over time as we get more info on how the code is being used and what's needed in client apps and connectors.
This is required for connectors that handle their own callbacks or have other needs to serve HTTP.
Serving HTTP introduced yet another bit of configuration that will need to be passed from the server to the connector: the URL and/or path where the
Connector
'shttp.Handler
will be mounted. In our case we've chosen/connector/:id
, but it's obviously not something we want connectors hard-coding. Connectors may also not have an awareness of their own ID. It's also possible that Deci itself is mounted in a path (e.g.,/deci
), so the full path may end up being something like/deci/connector/:id
. Most connectors will need to know this URL to properly generate links or HTTP redirects in its ownhttp.Handler
.My first tactic to support additional configuration options was to create a higher level
ConnectorBuilder
function that was passed a set ofConnectorOptions
and returned(Connector, error
). Something like:While I think this would solve the problem, it adds significant complexity to the constructor creation process. It also makes connectors more awkward to test on their own because a
ConnectorBuilder
must be used (which looks awkward in a test that otherwise doesn't care how theConnector
is built, because it's being tested independently ofServer
) or theConnectorBuilder
is left untested, which leaves important code untested.I settled instead on constructing a
Server
first, then addingConnector
instances later. TheServer
's public API becomes usable when building theConnector
, but smaller interfaces (e.g.,Authenticator
or astring
from the return value ofConnectorURL
) can be passed as constructor arguments or set directly onConnector
structs. This makes it more pleasant to test aConnector
on its own (withoutServer
) and avoids additional types likeConnectorBuilder
and such that look awkward in Go code.I've also ripped out the awareness of
URL.Path
inServer
's mux. This follows the convention of other Gohttp.Handler
(e.g.,FileServer
) which always serve relative to their own view of/
, but can be mounted at any path as long ashttp.StripPrefix
is used. This reduces the need to test non-/
paths and allows more flexiblility in mounting theServer
inmain
.