-
Notifications
You must be signed in to change notification settings - Fork 70
feat(oauth): use oauth device flow to authenticate with predefined src-cli OAuth client #1223
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
base: main
Are you sure you want to change the base?
Conversation
cmd/src/login.go
Outdated
| } | ||
|
|
||
| fmt.Fprintln(out) | ||
| fmt.Fprintf(out, "🔐 To authenticate, visit %s and enter the code: %s\n", authResp.VerificationURI, authResp.UserCode) |
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.
no emojis in UI please 😬 since claude, this has an AI vibe slop feeling to it :(
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.
valid :) I'll remove it
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.
Removed. We need to do a larger sweep of the other emojis too :|
| fmt.Fprintln(out) | ||
| fmt.Fprintf(out, "To use this access token, set the following environment variables in your terminal:\n\n") | ||
| fmt.Fprintf(out, " export SRC_ENDPOINT=%s\n", endpointArg) | ||
| fmt.Fprintf(out, " export SRC_ACCESS_TOKEN=%s\n", cfg.AccessToken) |
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.
what you get here is not a SG access token, it's an oauth token and it comes with an access token and refresh token (and expiry) and needs to regularly be refreshed.
I think we need to store the accesstoken/refreshtoken pair in secure storage and add some http Transport that refreshes the credential as needed.
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.
Will look at using https://github.com/99designs/keyring. We already use it with sg to store some secrets. It uses your OS keychain
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.
oh nice, cross OS too :)
465fe85 to
fd1668e
Compare
|
This change is part of the following stack: Change managed by git-spice. |
| fmt.Fprintf(out, "To authenticate, visit %s and enter the code: %s\n", authResp.VerificationURI, authResp.UserCode) | ||
| if authResp.VerificationURIComplete != "" { | ||
| fmt.Fprintln(out) | ||
| fmt.Fprintf(out, "Alternatively, you can open: %s\n", authResp.VerificationURIComplete) |
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.
Should:
- always try to open the browser
- always print the url
| Use OAuth device flow to authenticate: | ||
| $ src login --device-flow https://sourcegraph.com |
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.
Do other CLI's require a flag for this? If I remember they are normally interactive right? You could still interactively decide between creating an access token vs oauth flows right?
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.
No they don't. It just happens. The plan it to make it part of the normal flow if you don't have SRC_ACCESS_TOKEN set
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.
src login is a misnomer currently haha, it should probably be renamed to src whoami and then src login is the interactive flow always 😬
| Override the default client id used during device flow when authenticating: | ||
| $ src login --device-flow https://sourcegraph.com --client-id sgo_my_own_client_id |
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.
doesn't seem worth supporting a custom client-id given you shipped a predefined one. If a user still hasn't upgraded sourcegraph just fallback to the old flow?
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.
agreed. It's already removed just have to update
This adds the flag
--device-flowtologincommand which then starts the OAuth device authentication flow.ghdoes the same flow when you authenticate from the cli withgh auth login.internal/oauthdevice.well-known/openid-configurationI wanted to add
--client-idin case people wanted to override the default client that is used which can also be used for testing, but when I tried creating a client on S2 it doesn't have the correct configuration as set from the UI to be able to do this.Important
Seems like a lot of code but it's the tests that make up most of it
Test plan
Amp thread