Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion lib/shared/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -404,5 +404,5 @@ export {
checkVersion,
} from './sourcegraph-api/siteVersion'
export { configOverwrites } from './models/configOverwrites'
export { isS2 } from './sourcegraph-api/environments'
export { isS2, isWorkspaceInstance } from './sourcegraph-api/environments'
export { createGitDiff } from './editor/create-git-diff'
22 changes: 22 additions & 0 deletions lib/shared/src/sourcegraph-api/environments.ts
Original file line number Diff line number Diff line change
Expand Up @@ -40,3 +40,25 @@ export function isS2(arg: Pick<AuthStatus, 'endpoint'> | undefined | string): bo

// TODO: Update to live link https://linear.app/sourcegraph/issue/CORE-535/cody-clients-migrate-ctas-to-live-links
export const DOTCOM_WORKSPACE_UPGRADE_URL = new URL('https://sourcegraph.com/cody/manage')

export const Workspaces_Host_Prod = '.sourcegraph.app'
export const Workspaces_Host_Dev = '.sourcegraphdev.app'

// 🚨 SECURITY: This is used to validate a set of URLs we will allow to be passed in
// to the editor in the URL handler.
export function isWorkspaceInstance(authStatus: Pick<AuthStatus, 'endpoint'> | undefined): boolean
export function isWorkspaceInstance(url: string): boolean
export function isWorkspaceInstance(arg: Pick<AuthStatus, 'endpoint'> | undefined | string): boolean {
const url = typeof arg === 'string' ? arg : arg?.endpoint
if (url === undefined) {
return false
}
try {
return (
new URL(url).host.endsWith(Workspaces_Host_Prod) ||
Copy link
Contributor

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.

new URL(url).host.endsWith(Workspaces_Host_Dev)
)
} catch {
return false
}
}
43 changes: 40 additions & 3 deletions vscode/src/auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@ import {
isDotCom,
isError,
isNetworkLikeError,
isWorkspaceInstance,
telemetryRecorder,
} from '@sourcegraph/cody-shared'
import { resolveAuth } from '@sourcegraph/cody-shared/src/configuration/auth-resolver'
Expand All @@ -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.
*/
Expand Down Expand Up @@ -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> {
Copy link
Author

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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
Expand Down Expand Up @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Author

@mmanela mmanela Jan 15, 2025

Choose a reason for hiding this comment

The 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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I don't see issues with this approach. 👍

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just brainstorming for after the launch, what if we:

  1. The browser send the instance url to the IDE
  2. Here the handler received the instance URL, and then instead of showing the input box for the show enterprise instance url form, we just open the link to the account token page in browser where user needs to manually click on the approve button to send the token back to the client

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just brainstorming for after the launch, what if we:

  1. The browser send the instance url to the IDE
  2. Here the handler received the instance URL, and then instead of showing the input box for the show enterprise instance url form, we just open the link to the account token page in browser where user needs to manually click on the approve button to send the token back to the client

return
}

if (!token || !endpoint) {
return
}
Expand Down
Loading