Issue #179 Refactor and test handleJenkinsUIRequest() #198

kishansagathiya · 2018-03-16T07:05:31Z

Fixes #179

hferentschik · 2018-03-16T10:02:24Z

So that for now is just a variable rename?

kishansagathiya · 2018-03-16T10:09:40Z

@hferentschik
No,
The code from handleJenkinsUIRequest() has been transferred to processTokenJSON() and checkCookies().
Does this refactor seem right to you?
If this is the kind of refactor that you were expecting I can move to writing tests.

hferentschik · 2018-03-16T11:32:12Z

internal/proxy/proxy.go

-	if tj, ok := r.URL.Query()["token_json"]; ok { //If there is token_json in query, process it, find user info and login to Jenkins
-		if len(tj) < 1 {
-			p.HandleError(w, errors.New("could not read JWT token from URL"), requestLogEntry)
+	namespace, noProxy = p.processTokenJSON(responseWriter, request, requestLogEntry, redirectURL)


Ok, now I get it. It is a start, but I don't think there is a need for returning anything here. Especially look at p.checkCookies.

I guess as a first step this works, but the next is to eliminate the returns. Thale the last if statement for example if needsAuth. This could be pulled out into a function which then can get called.

Also, we tried to remove most of the used naked returns in the code. In most places we have done it already, but the code in proxy.go uses still a lot of them. The use of naked returns is really discouraged and should if ever be only used for very small methods. IMO in the current code it just makes it hard to understand. Let's get rid of them.

That said, I think we need to keep some conditional there. Pulling the conditionals into the methods obfuscates the code. Now it looks like, you are always processing the JSON token and you are always proecssing the cookies. There are several phases here which are not obvious anymore.

Leaving the outer if statements here will makes things clearer

hferentschik · 2018-03-16T11:45:03Z

internal/proxy/proxy.go

+func (p *Proxy) processTokenJSON(responseWriter http.ResponseWriter, request *http.Request, requestLogEntry *log.Entry, redirectURL *url.URL) (namespace string, noProxy bool) {
+
+	// Checks if token_json is present in the query
+	if tokenJSON, ok := request.URL.Query()["token_json"]; ok {


It might make sense to extract this into something like func isAuthenticatedRequest(request *http.Request) bool. If you then pull up the if statement again, the method name documents the alogrithm.

func (p *Proxy) handleJenkinsUIRequest(responseWriter http.ResponseWriter, request *http.Request, requestLogEntry *log.Entry) (cacheKey string, namespace string, noProxy bool) { if isAuthenticatedRequest(request) { namespace, noProxy = p.processAuthenticatedRequest(responseWriter, request, requestLogEntry, redirectURL) } else { return redirectToAuth(...) } if isWaitLoopRequest(request) { cacheKey, namespace, noProxy, needsAuth = p.checkCookies(responseWriter, request, requestLogEntry) } }

Not sure which code segment could be kept in isWaitLoopRequest(request)

hferentschik · 2018-03-16T11:46:49Z

internal/proxy/proxy.go

 	//redirect determines if we need to redirect to auth service
 	needsAuth := true
 	noProxy = true

-	//redirectURL is used for auth service as a target of successful auth redirect
-	redirectURL, err := url.ParseRequestURI(fmt.Sprintf("%s%s", strings.TrimRight(p.redirect, "/"), r.URL.Path))
+	// redirectURL is used for auth service as a target of successful auth redirect


I don't think we need to do that here. We can do that where it is needed. And most likely it can also happen in a dedicated properly named function

hferentschik · 2018-03-16T11:48:16Z

internal/proxy/proxy.go

 		return
 	}

 	//If the user provides OSO token, we can directly proxy
-	if _, ok := r.Header["Authorization"]; ok { //FIXME Do we need this?
+	if _, ok := request.Header["Authorization"]; ok { //FIXME Do we need this?


For the record, this can be removed. Needs to be tested though.

hferentschik · 2018-03-16T11:51:36Z

While working on this you might want to verify by running the Proxy locally whether it still works. There might be an issue with cookies in this case. I have not confirmed that and I successfully did a proxying using a local Proxy instance, but there is this issue which might prevent you to test the workflow locally #174. Might be worth following up.

Being able to test the workflow locally will help a lot, but remember the end game is to "codify" the flow into some tests.

hferentschik · 2018-03-21T08:13:01Z

Any more changes to this?

kishansagathiya · 2018-03-21T11:39:19Z

Is this(https://github.com/fabric8-services/fabric8-jenkins-proxy/blob/master/internal/proxy/proxy.go#L315) the function that waits until jenkins is unidled?

kishansagathiya · 2018-03-21T11:46:53Z

@chmouel If this seems right to you I can move forward with tests. Except for breaking the function this doesnt have much changed.

chmouel · 2018-03-26T05:28:07Z

@kishansagathiya there is a few concernces about this from @hferentschik what are about?

kishansagathiya · 2018-03-26T05:42:05Z

@chmouel I think they are all covered

No more naked returns
No more Authorization Header
redirectURL is in smaller functions
less returned values
conditions are out of the methods

chmouel · 2018-03-26T07:26:32Z

I am worried to merge something like this without any tests (i.e: no proxy_test.go) can we have unitests along this commit please?

kishansagathiya · 2018-03-26T08:09:19Z

@chmouel This is NOT ready for merge yet. It is WIP. We can merge this after tests. The reason for such temporary commits is to make sure that I am on the right path. If I get a review after tests, I will have change implementation and tests both.

kishansagathiya · 2018-03-28T15:13:18Z

@chmouel Have added some tests. It needs much more, which I keep pushing with this PR.
btw do I need to mock the database as well? not sure, what should I do? fabric8-auth doesn't mock db.

chmouel · 2018-03-28T15:41:10Z

look at the way it uses https://github.com/ory/dockertest for the database tests, there is already some example in the in db_store_test here

kishansagathiya · 2018-04-03T11:11:34Z

Tested this locally to be working using

http://localhost:8080/api/info/ksagathi-preview

It unidled the Jenkins and this is on running it again.

{"component":"proxy","header":"-H Accept-Encoding: gzip, deflate, br -H Accept-Language: en-US,en;q=0.9 -H Accept: text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8 -H Cache-Control: max-age=0 -H Connection: keep-alive -H Cookie: JenkinsIdled=<hidden> -H Upgrade-Insecure-Requests: 1 -H User-Agent: Mozilla/5.0 (X11; Fedora; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.132 Safari/537.36","level":"info","msg":"Handling incoming proxy request.","request":"GET /api/info/ksagathi-preview","request-hash":<hidden>,"time":"2018-04-03T16:37:41+05:30","type":"Jenkins UI"}
{"component":"proxy","level":"info","msg":"Accessing Jenkins route https://jenkins-ksagathi-preview-jenkins.b542.starter-us-east-2a.openshiftapps.com/securityRealm/commenceLogin?from=%2F","ns":"ksagathi-preview-jenkins","request-hash":<hidden>,"time":"2018-04-03T16:37:41+05:30"}
{"component":"proxy","level":"info","msg":"Found cookie <hidden>","ns":"ksagathi-preview-jenkins","request-hash":<hidden>,"time":"2018-04-03T16:37:42+05:30"}

chmouel

I think it's mostly fine, it's quite large so I may miss a few things there and there. The coverage is improving thanks for the tests, but there is still quite a bit not covered there see the coverage html file here https://drive.google.com/a/chmouel.com/uc?id=1cLNs5KLcsSaGqKdaOARADp23p8ubfuED&export=download

chmouel · 2018-04-05T05:58:26Z

internal/proxy/proxy_test.go

+	return "", errors.New("Could not get valid OSO Token")
+}
+
+type mockIdler struct {


It would be nice that the mock class are moved to their own package so this can be reused like we do on idler repo (and usually type/struct are top of the files as convention btw)

kishansagathiya · 2018-04-05T06:57:30Z

So far tests are only for handleJenkinsUIRequest. We can add other tests using a different PR and a different issue.

chmouel · 2018-04-05T07:01:52Z

@kishansagathiya fair enough, feel free to address the mock package move and i'll be 👍 with this,

kishansagathiya · 2018-04-05T07:38:34Z

@chmouel on it

…

On Thu 5 Apr, 2018, 12:31 PM Chmouel Boudjnah, ***@***.***> wrote: @kishansagathiya <https://github.com/kishansagathiya> fair enough, feel free to address the mock package move and i'll be 👍 with this, — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#198 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AJttD0zCMqwTqPqg7SRJlfoLSnMKn2BTks5tlcFhgaJpZM4StVbt> .

- Splitted handleJenkinsUIRequest() to smaller methods - Added error package, removed naked returns, decreased returned parameters - Added tests

kishansagathiya · 2018-04-09T08:38:47Z

@chmouel
Moving mocks to other package was very hairy

It creates import cycle
Removing import cycle would require us to move tests to a separate package say something like proxy_test, which would require us to export a lot of structs, interfaces, and methods
I requires to move other tests also like move db_store_test to separate package. I have those changes, but thought just moving mocks to different file would be a much better solution.

kishansagathiya changed the title ~~Issue #179 Refactor and test handleJenkinsUIRequest()~~ [WIP] Issue #179 Refactor and test handleJenkinsUIRequest() Mar 16, 2018

kishansagathiya force-pushed the issue_179 branch from 1eba87e to d60a2b3 Compare March 16, 2018 10:25

hferentschik suggested changes Mar 16, 2018

View reviewed changes

kishansagathiya force-pushed the issue_179 branch from b47b83e to 7bbcacc Compare April 3, 2018 09:37

kishansagathiya changed the title ~~[WIP] Issue #179 Refactor and test handleJenkinsUIRequest()~~ Issue #179 Refactor and test handleJenkinsUIRequest() Apr 3, 2018

chmouel reviewed Apr 5, 2018

View reviewed changes

Issue #179 Split handleJenkinsUIRequest() into smaller methods

9b2d795

- Splitted handleJenkinsUIRequest() to smaller methods - Added error package, removed naked returns, decreased returned parameters - Added tests

kishansagathiya force-pushed the issue_179 branch from 7bbcacc to 9b2d795 Compare April 9, 2018 08:33

chmouel merged commit a652445 into fabric8-services:master Apr 9, 2018

chmouel mentioned this pull request Apr 10, 2018

Issue #179 Refactor and test handleJenkinsUIRequest() (Revert) #226

Closed

chmouel added a commit that referenced this pull request Apr 10, 2018

Issue #198 Allow Revert word in validate_commit message

3abdc20

chmouel added a commit that referenced this pull request Apr 10, 2018

Issue #198 Allow Revert word in validate_commit message

b6442b1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #179 Refactor and test handleJenkinsUIRequest() #198

Issue #179 Refactor and test handleJenkinsUIRequest() #198

kishansagathiya commented Mar 16, 2018

hferentschik commented Mar 16, 2018

kishansagathiya commented Mar 16, 2018

hferentschik Mar 16, 2018

hferentschik Mar 16, 2018

hferentschik Mar 16, 2018

kishansagathiya Mar 21, 2018

hferentschik Mar 16, 2018

kishansagathiya Mar 26, 2018

hferentschik Mar 16, 2018

kishansagathiya Mar 26, 2018

hferentschik commented Mar 16, 2018

hferentschik commented Mar 21, 2018

kishansagathiya commented Mar 21, 2018

kishansagathiya commented Mar 21, 2018

chmouel commented Mar 26, 2018

kishansagathiya commented Mar 26, 2018

chmouel commented Mar 26, 2018

kishansagathiya commented Mar 26, 2018

kishansagathiya commented Mar 28, 2018

chmouel commented Mar 28, 2018

kishansagathiya commented Apr 3, 2018 •

edited

Loading

chmouel left a comment

chmouel Apr 5, 2018

kishansagathiya commented Apr 5, 2018

chmouel commented Apr 5, 2018

kishansagathiya commented Apr 5, 2018 via email

kishansagathiya commented Apr 9, 2018

Issue #179 Refactor and test handleJenkinsUIRequest() #198

Issue #179 Refactor and test handleJenkinsUIRequest() #198

Conversation

kishansagathiya commented Mar 16, 2018

hferentschik commented Mar 16, 2018

kishansagathiya commented Mar 16, 2018

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

hferentschik commented Mar 16, 2018

hferentschik commented Mar 21, 2018

kishansagathiya commented Mar 21, 2018

kishansagathiya commented Mar 21, 2018

chmouel commented Mar 26, 2018

kishansagathiya commented Mar 26, 2018

chmouel commented Mar 26, 2018

kishansagathiya commented Mar 26, 2018

kishansagathiya commented Mar 28, 2018

chmouel commented Mar 28, 2018

kishansagathiya commented Apr 3, 2018 • edited Loading

chmouel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kishansagathiya commented Apr 5, 2018

chmouel commented Apr 5, 2018

kishansagathiya commented Apr 5, 2018 via email

kishansagathiya commented Apr 9, 2018

kishansagathiya commented Apr 3, 2018 •

edited

Loading