Issue #103 Refactor Handle function #109
Conversation
proxy/proxy.go
Outdated
| } | ||
| //If Jenkins is up, we can simply proxy through |
There was a problem hiding this comment.
I was hoping we could also split here - https://github.com/fabric8-services/fabric8-jenkins-proxy/pull/109/files#diff-3ac192716a1df66bbd1008d327af8792R112, almost splitting the method in half. Something like this:
if isGH {
handleGitHubRequest(w, r)
} else {
handleDirectJenkinsUIRequest(w,r)
}This way the existing comments on this if/else branch become obsolete, since the code gives you the same information.
There was a problem hiding this comment.
Similar to the above comment - https://github.com/fabric8-services/fabric8-jenkins-proxy/pull/109/files#diff-3ac192716a1df66bbd1008d327af8792R153
Here we could extract everything into if isIdle into a method sendStatusAcceptedToClient.
There was a problem hiding this comment.
I wonder whether the redirect flag would not be better named as needsAuth or if you want to keep the redirect part needsAuthRedirect.
There was a problem hiding this comment.
What's about extracting a processCookies and processJSONToken as well?
There was a problem hiding this comment.
Thanks! Will look into it
vpavlin
force-pushed
the
feature/refactor-proxy-func
branch
from
8df8b7a
to
de74eac
Compare
|
@hferentschik I've refactored more and tried to remove some complexity where it was possible. |
|
retest this please |
hferentschik
changed the title
| var ns string | ||
| var cacheKey string | ||
| var reqURL url.URL | ||
| var noProxy bool |
There was a problem hiding this comment.
https://github.com/fabric8-services/fabric8-jenkins-proxy/pull/109/files#diff-3ac192716a1df66bbd1008d327af8792R102 - I would think the best is to create an issue as well on the GitHub issue tracker. This can then be a place for further discussion as well.
| body, err = ioutil.ReadAll(r.Body) | ||
| if err != nil { | ||
| p.HandleError(w, err) | ||
| ns, noProxy = p.handleGitHubRequest(w, r) |
| return | ||
| } | ||
| } | ||
| //Write usage stats to DB, run in a goroutine to not slow down the proxy | ||
| go func() { |
| needsAuth = false | ||
| } | ||
|
|
||
| if len(r.Cookies()) > 0 { //Check cookies and proxy cache to find user info |
There was a problem hiding this comment.
I think here we could further split up, but we can do this in increments.
| } | ||
| http.SetCookie(w, cookie) |
There was a problem hiding this comment.
So just to sort this out in my mind, the cookie is just an optimisation which allows for short circuit JWT decoding, tenant lookup, etc. In theory, things would work without the cookie, correct?
There was a problem hiding this comment.
The flow is
- redirect to auth to get JWT token
- Get user info and OSO token
- Login to Jenkins with OSO token
- Find the newly generated JSESSION cookie and pass it back to user - Jenkins uses it to keep the user's session and not to force him re-login for every request, afaiu - which is why we need to tak the cookie from Proxy's login request to Jenkins and pass it back to user so that it gets stored in the browser - as if the user himself logged into Jenkins
|
Merged, thanks. |
Split out couple blocks of code, happy to do more if you see anything else.
Addresses issue #103