-
Notifications
You must be signed in to change notification settings - Fork 45
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Client http server for token #80
base: master
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep.
server/auth/gitlab/gitlab.go
Outdated
@@ -1,13 +1,18 @@ | |||
package gitlab |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually could you just merge the gitlab PR in and then I'll merge those changes in to this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Go for it
buffer.WriteString(scanner.Text()) | ||
var token string | ||
if listener != nil { | ||
// TODO: Timeout? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.Addr
var (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.ListenAndServe
function is a blackbox - hence all the shenanigans to find the random port and communicate it back.