Allow proxy to path #9
Conversation
main.go
Outdated
| p.sites[p.auth.SiteTwoLevel] = p.auth.SiteTwo | ||
| p.sites[p.auth.SiteThreeLevel] = p.auth.SiteThree | ||
| p.sites = make(map[string]AuthSite) | ||
| p.sites[p.auth.SiteOneLevel], err = NewAuthSite(p.auth.SiteOne) |
There was a problem hiding this comment.
I'm wondering if instead of three separate SiteX type entries in a ProxyAuth, it can just be a map in there as well like p.sites. Then you can loop through them.
Will there always be exactly three sites?
There was a problem hiding this comment.
This was done mainly because of an issue with kelseyhightower/envconfig where he use Split instead of SplitN or Cut. So urls with ports cause errors. Given that the project hasn't been updated in a few years, I'm not holding my breath for that fix.
I could make a custom decoder and use that instead, it probably won't actually be much more work. Given that AuthSite is pretty much a custom decoder, I think it's worth doing.
| caddyhttp.SetVar(r.Context(), "upstream", to) | ||
| p.log.Info("setting upstream to " + to) | ||
|
|
||
| if upstream == p.api || upstream.Path == "" { |
There was a problem hiding this comment.
I don't quite understand this if-condition. It looks like it's saying, "If we're sending them to the authorization API, or if the upstream server we're sending them to doesn't define a specific URL path, then use whatever URL path they requested."
However, if we're sending them to the authorization API, then we only want them to go to a single, pre-defined URL path... the one that sets the JWT with the authorization data.
Would you mind clarifying this condition's purpose for me? I suspect I'm still missing/misunderstanding something about it.
There was a problem hiding this comment.
We want to redirect them back to wherever they were before, not just "/", though I'll probably need to adjust it some to use a query param (not sure if I can rewrite with a query param included or not, it didn't look like it when I tried at first, but I may have not been doing it correctly).
| // if no cookie, redirect to get new cookie | ||
| cookie, err := r.Cookie(p.auth.CookieName) | ||
| if err != nil { | ||
| return p.ManagementAPI, nil | ||
| return p.api, nil |
There was a problem hiding this comment.
Out of curiosity, why the change from "ManagementAPI" to "api"?
There was a problem hiding this comment.
mainly needed so that I could get around the reverse_proxy and paths. It's quite possible I can remove this if/when we switch to redirect instead of reverse proxy to management api.
| @@ -1,12 +1,6 @@ | |||
| # For proxy JWT (required) | |||
| AUTH_COOKIE_NAME= | |||
| AUTH_TOKEN_SECRET= | |||
| AUTH_SITES= | |||
There was a problem hiding this comment.
Maybe add a comment here with an example.
main.go
Outdated
| func (a *AuthSites) Decode(input string) error { | ||
| *a = make(AuthSites) | ||
| sites := strings.Split(input, ";") | ||
| for _, site := range sites { |
There was a problem hiding this comment.
Maybe make this for _, s := ...
Or, if you prefer, in line 44 change the first site to something else, so that you don't assign a new value to the for variable.
| Path: u.Path, | ||
| // Add protocol if it is missing (needed for url parse) | ||
| if !re.MatchString(input) { | ||
| input = "http://" + input |
There was a problem hiding this comment.
Is this safe enough or would it be better just to return an error and say that the protocol is required?
I'm just wondering if in situations where it should really be https that the resulting problem/failure will be hidden from us.
There was a problem hiding this comment.
The protocol is currently not used, it is just there so url.Parse can parse it. I just get the host and path.
No description provided.