Client http server for token #80
Conversation
Now the problem is tha API is wrong. Sigh...
Versions beyond 11.1 (and possibly a few releases before) use a different method for delivering tokens. They also have disabled version 3 of the api. These changes address that and add a debugging mode for the server that make it easier to debug issues like this in the future.
Add an auto_token client option to change the flow such that the token will be sent back to the client directly by the browser. There are a number of "TODO" messages in this commit which will hopefully be addressed in code review. I'm not sure if they're needed.
|
This is very pleasing. Nice work. Apologies for taking so long to review this. It's been a busy time.
|
cmd/cashier/main.go
Outdated
| listener = client.StartHTTPServer() | ||
| if listener != nil { | ||
| authURL = fmt.Sprintf("%s?auto_token=%s", | ||
| c.CA, url.PathEscape(listener.ReceiverURL)) |
There was a problem hiding this comment.
Does it make sense to allow a url here? What about just passing a port around instead of a full URL and assuming that the receiver will always be running on localhost? I fear that allowing a full URL could open this to some kind of abuse where a token can be sent somewhere else
server/auth/gitlab/gitlab.go
Outdated
| @@ -1,13 +1,18 @@ | |||
| package gitlab | |||
There was a problem hiding this comment.
This file looks unrelated to the change? Oh I see, this includes the gitlab changes too. Can you yank those from this PR easily? If not a rebase on this branch after merging the other PR might be the way to go
There was a problem hiding this comment.
I needed it in order to test as the current gitlab support is broken. I can break it out, yes.
There was a problem hiding this comment.
Actually could you just merge the gitlab PR in and then I'll merge those changes in to this?
| buffer.WriteString(scanner.Text()) | ||
| var token string | ||
| if listener != nil { | ||
| // TODO: Timeout? |
There was a problem hiding this comment.
I'm happy to let this run forever on someone's machine ¯_(ツ)_/¯
Yes, sorry, that was removed. |
Codecov Report
@@ Coverage Diff @@
## master #80 +/- ##
==========================================
- Coverage 45.82% 44.66% -1.16%
==========================================
Files 18 18
Lines 1089 1124 +35
==========================================
+ Hits 499 502 +3
- Misses 531 560 +29
- Partials 59 62 +3
Continue to review full report at Codecov.
|
|
OK, changes from master moved in. |
|
Finally tested this with all the recent changes. It works until you start to:
So I think what I'm going to do in the Not 100% sure on detecting the cashier client. Just looking for a Referer header might be enough. Also, my original idea was to show the regular template page but have an |
This passes back the token from the browser directly to the CLI. What will we all do with all those 5 seconds we save up?
There are a few lines marked
TODO. A few are config options we could add, but really, how many knobs do we want the user to muck with? A few are possible security bits. Randomised paths, Randomised starting ports (and now that I think of it, it could just randomly pick ports the whole time instead of incrementally).I also have a general Go question. The port check goroutine and the starting the http server goroutine both poke at the
listener.Srv.Addrvar (the former reads, the latter reads/writes). Is this allowed? Are there potential panics / race conditions this can cause? I can't really wrap these in a mutex since thehttp.ListenAndServefunction is a blackbox - hence all the shenanigans to find the random port and communicate it back.