-
Notifications
You must be signed in to change notification settings - Fork 69
refactor: validate and parse the endpoint and proxy at program load #1267
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?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -5,6 +5,7 @@ import ( | |||||
| "flag" | ||||||
| "fmt" | ||||||
| "io" | ||||||
| "net/url" | ||||||
| "os" | ||||||
|
|
||||||
| "github.com/sourcegraph/src-cli/internal/api" | ||||||
|
|
@@ -48,23 +49,26 @@ Examples: | |||||
| if err := flagSet.Parse(args); err != nil { | ||||||
| return err | ||||||
| } | ||||||
| endpoint := cfg.Endpoint | ||||||
|
|
||||||
| var loginEndpointURL *url.URL | ||||||
| if flagSet.NArg() >= 1 { | ||||||
| endpoint = flagSet.Arg(0) | ||||||
| } | ||||||
| if endpoint == "" { | ||||||
| return cmderrors.Usage("expected exactly one argument: the Sourcegraph URL, or SRC_ENDPOINT to be set") | ||||||
| arg := flagSet.Arg(0) | ||||||
| u, err := parseEndpoint(arg) | ||||||
| if err != nil { | ||||||
| return cmderrors.Usage(fmt.Sprintf("invalid endpoint URL: %s", arg)) | ||||||
| } | ||||||
| loginEndpointURL = u | ||||||
| } | ||||||
|
|
||||||
| client := cfg.apiClient(apiFlags, io.Discard) | ||||||
|
|
||||||
| return loginCmd(context.Background(), loginParams{ | ||||||
| cfg: cfg, | ||||||
| client: client, | ||||||
| endpoint: endpoint, | ||||||
| out: os.Stdout, | ||||||
| apiFlags: apiFlags, | ||||||
| oauthClient: oauth.NewClient(oauth.DefaultClientID), | ||||||
| cfg: cfg, | ||||||
| client: client, | ||||||
| out: os.Stdout, | ||||||
| apiFlags: apiFlags, | ||||||
| oauthClient: oauth.NewClient(oauth.DefaultClientID), | ||||||
| loginEndpointURL: loginEndpointURL, | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
| }) | ||||||
| } | ||||||
|
|
||||||
|
|
@@ -76,12 +80,12 @@ Examples: | |||||
| } | ||||||
|
|
||||||
| type loginParams struct { | ||||||
| cfg *config | ||||||
| client api.Client | ||||||
| endpoint string | ||||||
| out io.Writer | ||||||
| apiFlags *api.Flags | ||||||
| oauthClient oauth.Client | ||||||
| cfg *config | ||||||
| client api.Client | ||||||
| out io.Writer | ||||||
| apiFlags *api.Flags | ||||||
| oauthClient oauth.Client | ||||||
| loginEndpointURL *url.URL | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We're using
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That'll be the intent of a follow up PR |
||||||
| } | ||||||
|
|
||||||
| type loginFlow func(context.Context, loginParams) error | ||||||
|
|
@@ -96,9 +100,9 @@ const ( | |||||
| ) | ||||||
|
|
||||||
| func loginCmd(ctx context.Context, p loginParams) error { | ||||||
| if p.cfg.ConfigFilePath != "" { | ||||||
| if p.cfg.configFilePath != "" { | ||||||
| fmt.Fprintln(p.out) | ||||||
| fmt.Fprintf(p.out, "⚠️ Warning: Configuring src with a JSON file is deprecated. Please migrate to using the env vars SRC_ENDPOINT, SRC_ACCESS_TOKEN, and SRC_PROXY instead, and then remove %s. See https://github.com/sourcegraph/src-cli#readme for more information.\n", p.cfg.ConfigFilePath) | ||||||
| fmt.Fprintf(p.out, "⚠️ Warning: Configuring src with a JSON file is deprecated. Please migrate to using the env vars SRC_ENDPOINT, SRC_ACCESS_TOKEN, and SRC_PROXY instead, and then remove %s. See https://github.com/sourcegraph/src-cli#readme for more information.\n", p.cfg.configFilePath) | ||||||
| } | ||||||
|
|
||||||
| _, flow := selectLoginFlow(p) | ||||||
|
|
@@ -107,15 +111,13 @@ func loginCmd(ctx context.Context, p loginParams) error { | |||||
|
|
||||||
| // selectLoginFlow decides what login flow to run based on configured AuthMode. | ||||||
| func selectLoginFlow(p loginParams) (loginFlowKind, loginFlow) { | ||||||
| endpointArg := cleanEndpoint(p.endpoint) | ||||||
|
|
||||||
| if p.loginEndpointURL != nil && p.loginEndpointURL.String() != p.cfg.endpointURL.String() { | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we just make cfg the source of truth and the cli endpoint arg just overrides it we don't have to do this check at all.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but that would break current behavior. As confusing as the current behavior is, I was trying to keep that behavior, changing it in a followup PR. |
||||||
| return loginFlowEndpointConflict, runEndpointConflictLogin | ||||||
| } | ||||||
| switch p.cfg.AuthMode() { | ||||||
| case AuthModeOAuth: | ||||||
| return loginFlowOAuth, runOAuthLogin | ||||||
| case AuthModeAccessToken: | ||||||
| if endpointArg != p.cfg.Endpoint { | ||||||
| return loginFlowEndpointConflict, runEndpointConflictLogin | ||||||
| } | ||||||
burmudar marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| return loginFlowValidate, runValidatedLogin | ||||||
| default: | ||||||
| return loginFlowMissingAuth, runMissingAuthLogin | ||||||
|
|
@@ -126,7 +128,7 @@ func printLoginProblem(out io.Writer, problem string) { | |||||
| fmt.Fprintf(out, "❌ Problem: %s\n", problem) | ||||||
| } | ||||||
|
|
||||||
| func loginAccessTokenMessage(endpoint string) string { | ||||||
| func loginAccessTokenMessage(endpointURL *url.URL) string { | ||||||
| return fmt.Sprintf("\n"+`🛠 To fix: Create an access token by going to %s/user/settings/tokens, then set the following environment variables in your terminal: | ||||||
| export SRC_ENDPOINT=%s | ||||||
|
|
@@ -135,5 +137,5 @@ func loginAccessTokenMessage(endpoint string) string { | |||||
| To verify that it's working, run the login command again. | ||||||
| Alternatively, you can try logging in interactively by running: src login %s | ||||||
| `, endpoint, endpoint, endpoint) | ||||||
| `, endpointURL, endpointURL, endpointURL) | ||||||
| } | ||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,15 +18,14 @@ import ( | |
| var loadStoredOAuthToken = oauth.LoadToken | ||
|
|
||
| func runOAuthLogin(ctx context.Context, p loginParams) error { | ||
| endpointArg := cleanEndpoint(p.endpoint) | ||
| client, err := oauthLoginClient(ctx, p, endpointArg) | ||
| client, err := oauthLoginClient(ctx, p) | ||
| if err != nil { | ||
| printLoginProblem(p.out, fmt.Sprintf("OAuth Device flow authentication failed: %s", err)) | ||
| fmt.Fprintln(p.out, loginAccessTokenMessage(endpointArg)) | ||
| fmt.Fprintln(p.out, loginAccessTokenMessage(p.cfg.endpointURL)) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Note that we are preferring the configured endpointURL over the provided argument endpoint here |
||
| return cmderrors.ExitCode1 | ||
| } | ||
|
|
||
| if err := validateCurrentUser(ctx, client, p.out, endpointArg); err != nil { | ||
| if err := validateCurrentUser(ctx, client, p.out, p.cfg.endpointURL); err != nil { | ||
| return err | ||
| } | ||
|
|
||
|
|
@@ -39,13 +38,13 @@ func runOAuthLogin(ctx context.Context, p loginParams) error { | |
| // oauthLoginClient returns a api.Client with the OAuth token set. It will check secret storage for a token | ||
| // and use it if one is present. | ||
| // If no token is found, it will start a OAuth Device flow to get a token and storage in secret storage. | ||
| func oauthLoginClient(ctx context.Context, p loginParams, endpoint string) (api.Client, error) { | ||
| func oauthLoginClient(ctx context.Context, p loginParams) (api.Client, error) { | ||
| // if we have a stored token, used it. Otherwise run the device flow | ||
| if token, err := loadStoredOAuthToken(ctx, endpoint); err == nil { | ||
| return newOAuthAPIClient(p, endpoint, token), nil | ||
| if token, err := loadStoredOAuthToken(ctx, p.cfg.endpointURL); err == nil { | ||
| return newOAuthAPIClient(p, token), nil | ||
| } | ||
|
|
||
| token, err := runOAuthDeviceFlow(ctx, endpoint, p.out, p.oauthClient) | ||
| token, err := runOAuthDeviceFlow(ctx, p.cfg.endpointURL, p.out, p.oauthClient) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -55,23 +54,23 @@ func oauthLoginClient(ctx context.Context, p loginParams, endpoint string) (api. | |
| fmt.Fprintf(p.out, "⚠️ Warning: Failed to store token in keyring store: %q. Continuing with this session only.\n", err) | ||
| } | ||
|
|
||
| return newOAuthAPIClient(p, endpoint, token), nil | ||
| return newOAuthAPIClient(p, token), nil | ||
| } | ||
|
|
||
| func newOAuthAPIClient(p loginParams, endpoint string, token *oauth.Token) api.Client { | ||
| func newOAuthAPIClient(p loginParams, token *oauth.Token) api.Client { | ||
| return api.NewClient(api.ClientOpts{ | ||
| Endpoint: endpoint, | ||
| AdditionalHeaders: p.cfg.AdditionalHeaders, | ||
| EndpointURL: p.cfg.endpointURL, | ||
| AdditionalHeaders: p.cfg.additionalHeaders, | ||
| Flags: p.apiFlags, | ||
| Out: p.out, | ||
| ProxyURL: p.cfg.ProxyURL, | ||
| ProxyPath: p.cfg.ProxyPath, | ||
| ProxyURL: p.cfg.proxyURL, | ||
| ProxyPath: p.cfg.proxyPath, | ||
| OAuthToken: token, | ||
| }) | ||
| } | ||
|
|
||
| func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, client oauth.Client) (*oauth.Token, error) { | ||
| authResp, err := client.Start(ctx, endpoint, nil) | ||
| func runOAuthDeviceFlow(ctx context.Context, endpointURL *url.URL, out io.Writer, client oauth.Client) (*oauth.Token, error) { | ||
| authResp, err := client.Start(ctx, endpointURL, nil) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
@@ -95,12 +94,12 @@ func runOAuthDeviceFlow(ctx context.Context, endpoint string, out io.Writer, cli | |
| interval = 5 * time.Second | ||
| } | ||
|
|
||
| resp, err := client.Poll(ctx, endpoint, authResp.DeviceCode, interval, authResp.ExpiresIn) | ||
| resp, err := client.Poll(ctx, endpointURL, authResp.DeviceCode, interval, authResp.ExpiresIn) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| token := resp.Token(endpoint) | ||
| token := resp.Token(endpointURL) | ||
| token.ClientID = client.ClientID() | ||
| return token, nil | ||
| } | ||
|
|
||
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 think after successful parse, we should override whatever is configured in cfg for the endpoint.
So cfg should be source of truth hence forth.
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.
Indeed it should, and after a followup PR, it will be!