-
Notifications
You must be signed in to change notification settings - Fork 335
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
feat(auth): Allow workspace to pre-populate URL for quick sign-in #6653
base: main
Are you sure you want to change the base?
Changes from 1 commit
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 |
---|---|---|
|
@@ -19,6 +19,7 @@ import { | |
isDotCom, | ||
isError, | ||
isNetworkLikeError, | ||
isWorkspaceInstance, | ||
telemetryRecorder, | ||
} from '@sourcegraph/cody-shared' | ||
import { resolveAuth } from '@sourcegraph/cody-shared/src/configuration/auth-resolver' | ||
|
@@ -40,6 +41,28 @@ interface LoginMenuItem { | |
|
||
type AuthMenuType = 'signin' | 'switch' | ||
|
||
/** | ||
* Handles trying to directly sign-in or add to an enterprise instance. | ||
* First tries to sign in with the current token, if it's valid. Otherwise, | ||
* opens the sign-in flow and has user confirm. | ||
*/ | ||
async function showEnterpriseInstanceUrlFlow(endpoint: string): Promise<void> { | ||
const { configuration } = await currentResolvedConfig() | ||
const auth = await resolveAuth(endpoint, configuration, secretStorage) | ||
|
||
const authStatus = await authProvider.validateAndStoreCredentials(auth, 'store-if-valid') | ||
|
||
if (!authStatus?.authenticated) { | ||
const instanceUrl = await showInstanceURLInputBox(endpoint) | ||
if (!instanceUrl) { | ||
return | ||
} | ||
authProvider.setAuthPendingToEndpoint(instanceUrl) | ||
redirectToEndpointLogin(instanceUrl) | ||
} else { | ||
await showAuthResultMessage(endpoint, authStatus) | ||
} | ||
} | ||
/** | ||
* Show a quickpick to select the endpoint to sign into. | ||
*/ | ||
|
@@ -137,12 +160,12 @@ async function showAuthMenu(type: AuthMenuType): Promise<LoginMenuItem | null> { | |
/** | ||
* Show a VS Code input box to ask the user to enter a Sourcegraph instance URL. | ||
*/ | ||
async function showInstanceURLInputBox(title: string): Promise<string | undefined> { | ||
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. This was odd? This is either never passed in OR is a url. Seems the right thing to do for a URL is populate the url and not make that a title. 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. I think this is a component originally created for the second step of their sign in flow where user is expected to enter their endpoint before we do the redirection. Since we have moved out away from using native vs code component, I think this should be fine to change. |
||
async function showInstanceURLInputBox(url?: string): Promise<string | undefined> { | ||
const result = await vscode.window.showInputBox({ | ||
title, | ||
title: 'Connect to a Sourcegraph instance', | ||
prompt: 'Enter the URL of the Sourcegraph instance. For example, https://sourcegraph.example.com.', | ||
placeHolder: 'https://sourcegraph.example.com', | ||
value: 'https://', | ||
value: url ?? 'https://', | ||
password: false, | ||
ignoreFocusOut: true, | ||
// valide input to ensure the user is not entering a token as URL | ||
|
@@ -300,8 +323,22 @@ export async function tokenCallbackHandler(uri: vscode.Uri): Promise<void> { | |
closeAuthProgressIndicator() | ||
|
||
const params = new URLSearchParams(uri.query) | ||
|
||
const token = params.get('code') || params.get('token') | ||
const endpoint = currentAuthStatus().endpoint | ||
|
||
// If we were provided an instance URL then it means we are | ||
// request the user setup auth with a different sourcegraph instance | ||
// We want to prompt them to switch to this instance and if needed | ||
// start the auth flow | ||
const instanceHost = params.get('instance') | ||
const instanceUrl = instanceHost ? new URL(`https://${instanceHost}`) : undefined | ||
if (instanceUrl && isWorkspaceInstance(instanceUrl.toString())) { | ||
// Prompt the user to switch/setup with the new instance | ||
await showEnterpriseInstanceUrlFlow(instanceUrl.toString()) | ||
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. So all this does is populate the URL and the user still has to confirm and proceed right? That feels low risk to me and like something that enterprise would benefit from as well - making it easier to onboard to cody feels useful. Maybe we can remove the workspaces special casing even when @evict agrees? 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. Yea I put a screenshot in the description of what you see. In terms of restrictions it depends on how general we want to be. If we want an onprem instance to work with this, then it needs to have 0 restrictions. But if we just targeted MT and Cloud we can add the cloud domain. But I am not sure on risk of letting someone pre-fill customer domains. 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. Looks good to me. I don't see issues with this approach. 👍 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. Just brainstorming for after the launch, what if we:
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. Just brainstorming for after the launch, what if we:
|
||
return | ||
} | ||
|
||
if (!token || !endpoint) { | ||
return | ||
} | ||
|
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.
Nit: check for https protocol? Not sure if it'd help tho.