Skip to content

Commit

Permalink
simplify the "authed but Cody is not enabled" state (#5437)
Browse files Browse the repository at this point in the history
Previously, we had 2 fields on AuthState: `authenticated` and
`isLoggedIn`. These are synonyms but actually meant 2 different things:

- `authenticated`: whether the user is authenticated
- `isLoggedIn`: whether the user is authenticated AND Cody is enabled on
the instance AND the user has a verified email if the instance requires
such

Neither of the additional constraints for `isLoggedIn` were handled
cleanly in the UI. So, this change makes it so that users are prevented
from signing in if Cody is not enabled (and are shown an error message
during sign-in). The verified email issue is a much lesser issue now (it
was an issue when we had different signup methods ~11 months ago) and is
handled by the server returning an error. In general, we should
implement these checks on the server not the client since the auth flow
is on the server already (and that is how similar things would be
implemented today now that we have the nice non-raw-access-token flow).

## Test plan

Set `{"cody.enabled": false}` in site config and try signing in. Confirm
that an error message is shown.
  • Loading branch information
sqs authored Sep 2, 2024
1 parent 7b2c15b commit f8a1b72
Show file tree
Hide file tree
Showing 36 changed files with 85 additions and 124 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,11 @@ import com.google.gson.annotations.SerializedName;
data class AuthStatus(
val username: String,
val endpoint: EndpointEnum, // Oneof:
val isLoggedIn: Boolean,
val isFireworksTracingEnabled: Boolean,
val showInvalidAccessTokenError: Boolean,
val authenticated: Boolean,
val hasVerifiedEmail: Boolean,
val requiresVerifiedEmail: Boolean,
val siteHasCodyEnabled: Boolean,
val siteVersion: String,
val codyApiVersion: Long,
val configOverwrites: CodyLLMSiteConfiguration? = null,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ package com.sourcegraph.cody.agent.protocol_generated;
data class ServerInfo(
val name: String,
val authenticated: Boolean? = null,
val codyEnabled: Boolean? = null,
val codyVersion: String? = null,
val authStatus: AuthStatus? = null,
)
Expand Down
2 changes: 1 addition & 1 deletion agent/src/TestClient.ts
Original file line number Diff line number Diff line change
Expand Up @@ -900,7 +900,7 @@ ${patch}`

public async beforeAll(additionalConfig?: Partial<ExtensionConfiguration>) {
const info = await this.initialize(additionalConfig)
if (!info.authStatus?.isLoggedIn) {
if (!info.authStatus?.authenticated) {
throw new Error('Could not log in')
}
}
Expand Down
1 change: 0 additions & 1 deletion agent/src/agent.ts
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,6 @@ export class Agent extends MessageHandler implements ExtensionClient {
return {
name: 'cody-agent',
authenticated: authStatus?.authenticated,
codyEnabled: authStatus?.siteHasCodyEnabled,
codyVersion: authStatus?.siteVersion,
authStatus,
}
Expand Down
4 changes: 2 additions & 2 deletions agent/src/cli/command-auth/command-accounts.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ import ora from 'ora'
import { AuthenticatedAccount } from './AuthenticatedAccount'
import { booleanToText } from './command-auth'
import { type AuthenticationOptions, accessTokenOption, endpointOption } from './command-login'
import { notLoggedIn, unknownErrorSpinner } from './messages'
import { notAuthenticated, unknownErrorSpinner } from './messages'
import { loadUserSettings } from './settings'

export const accountsCommand = new Command('accounts')
Expand All @@ -18,7 +18,7 @@ export const accountsCommand = new Command('accounts')
try {
const settings = loadUserSettings()
if (!settings?.accounts || settings.accounts.length === 0) {
notLoggedIn(spinner)
notAuthenticated(spinner)
process.exit(1)
}
const t = new Table()
Expand Down
4 changes: 2 additions & 2 deletions agent/src/cli/command-auth/command-whoami.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import { isError } from 'lodash'
import ora from 'ora'
import { AuthenticatedAccount } from './AuthenticatedAccount'
import { type AuthenticationOptions, accessTokenOption, endpointOption } from './command-login'
import { errorSpinner, notLoggedIn, unknownErrorSpinner } from './messages'
import { errorSpinner, notAuthenticated, unknownErrorSpinner } from './messages'

export const whoamiCommand = new Command('whoami')
.description('Print the active authenticated account')
Expand All @@ -18,7 +18,7 @@ export const whoamiCommand = new Command('whoami')
process.exit(1)
}
if (!account?.username) {
notLoggedIn(spinner)
notAuthenticated(spinner)
process.exit(1)
}

Expand Down
2 changes: 1 addition & 1 deletion agent/src/cli/command-auth/messages.ts
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
import type { Ora } from 'ora'
import type { AuthenticationOptions } from './command-login'

export function notLoggedIn(spinner: Ora): void {
export function notAuthenticated(spinner: Ora): void {
if (!spinner.isSpinning) {
return
}
Expand Down
8 changes: 4 additions & 4 deletions agent/src/cli/command-chat.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import {
accessTokenOption,
endpointOption,
} from './command-auth/command-login'
import { errorSpinner, notLoggedIn } from './command-auth/messages'
import { errorSpinner, notAuthenticated } from './command-auth/messages'
import { isNonEmptyArray } from './isNonEmptyArray'

declare const process: { pkg: { entrypoint: string } } & NodeJS.Process
Expand Down Expand Up @@ -89,7 +89,7 @@ Enterprise Only:
process.exit(1)
}
if (!account?.username) {
notLoggedIn(spinner)
notAuthenticated(spinner)
process.exit(1)
}
options.accessToken = account.accessToken
Expand Down Expand Up @@ -143,8 +143,8 @@ export async function chatAction(options: ChatOptions): Promise<number> {
}
spinner.text = 'Initializing...'
const { serverInfo, client, messageHandler } = await newEmbeddedAgentClient(clientInfo, activate)
if (!serverInfo.authStatus?.isLoggedIn) {
notLoggedIn(spinner)
if (!serverInfo.authStatus?.authenticated) {
notAuthenticated(spinner)
return 1
}

Expand Down
2 changes: 1 addition & 1 deletion agent/src/enterprise-demo.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ describe('Enterprise', () => {
beforeAll(async () => {
const serverInfo = await demoEnterpriseClient.initialize()

expect(serverInfo.authStatus?.isLoggedIn).toBeTruthy()
expect(serverInfo.authStatus?.authenticated).toBeTruthy()
expect(serverInfo.authStatus?.username).toStrictEqual('codytesting')
}, 10_000)

Expand Down
2 changes: 1 addition & 1 deletion agent/src/enterprise-s2.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ describe('Enterprise - S2 (close main branch)', () => {
autocompleteAdvancedProvider: 'fireworks',
})

expect(serverInfo.authStatus?.isLoggedIn).toBeTruthy()
expect(serverInfo.authStatus?.authenticated).toBeTruthy()
expect(serverInfo.authStatus?.username).toStrictEqual('codytesting')
}, 10_000)

Expand Down
10 changes: 5 additions & 5 deletions agent/src/index.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,7 @@ describe('Agent', () => {
// with a new access token.
accessToken: 'sgp_INVALIDACCESSTOK_ENTHISSHOULDFAILEEEEEEEEEEEEEEEEEEEEEEE2',
})
expect(serverInfo?.authStatus?.isLoggedIn).toBeFalsy()
expect(serverInfo?.authStatus?.authenticated).toBeFalsy()

// Log in so test cases are authenticated by default
const valid = await client.request('extensionConfiguration/change', {
Expand All @@ -73,7 +73,7 @@ describe('Agent', () => {
serverEndpoint: client.info.extensionConfiguration?.serverEndpoint ?? DOTCOM_URL.toString(),
customHeaders: {},
})
expect(valid?.isLoggedIn).toBeTruthy()
expect(valid?.authenticated).toBeTruthy()

for (const name of [
'src/animal.ts',
Expand Down Expand Up @@ -166,7 +166,7 @@ describe('Agent', () => {
serverEndpoint: 'https://sourcegraph.com/',
customHeaders: {},
})
expect(invalid?.isLoggedIn).toBeFalsy()
expect(invalid?.authenticated).toBeFalsy()
const invalidModels = await client.request('chat/models', { modelUsage: ModelUsage.Chat })
const remoteInvalidModels = invalidModels.models.filter(model => model.provider !== 'Ollama')
expect(remoteInvalidModels).toStrictEqual([])
Expand All @@ -178,7 +178,7 @@ describe('Agent', () => {
serverEndpoint: client.info.extensionConfiguration?.serverEndpoint ?? DOTCOM_URL.toString(),
customHeaders: {},
})
expect(valid?.isLoggedIn).toBeTruthy()
expect(valid?.authenticated).toBeTruthy()

const reauthenticatedModels = await client.request('chat/models', {
modelUsage: ModelUsage.Chat,
Expand Down Expand Up @@ -1234,7 +1234,7 @@ describe('Agent', () => {
beforeAll(async () => {
const serverInfo = await rateLimitedClient.initialize()

expect(serverInfo.authStatus?.isLoggedIn).toBeTruthy()
expect(serverInfo.authStatus?.authenticated).toBeTruthy()
expect(serverInfo.authStatus?.username).toStrictEqual('sourcegraphcodyclients-1-efapb')
}, 10_000)

Expand Down
10 changes: 0 additions & 10 deletions lib/shared/src/auth/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import type { CodyLLMSiteConfiguration } from '../sourcegraph-api/graphql/client
export interface AuthStatus {
username: string
endpoint: string
isLoggedIn: boolean
/**
* Used to enable Fireworks tracing for Sourcegraph teammates on DotCom.
* https://readme.fireworks.ai/docs/enabling-tracing
Expand All @@ -18,7 +17,6 @@ export interface AuthStatus {
authenticated: boolean
hasVerifiedEmail: boolean
requiresVerifiedEmail: boolean
siteHasCodyEnabled: boolean
siteVersion: string
codyApiVersion: number
configOverwrites?: CodyLLMSiteConfiguration
Expand All @@ -44,13 +42,11 @@ export interface AuthStatusProvider {

export const defaultAuthStatus: AuthStatus = {
endpoint: '',
isLoggedIn: false,
isFireworksTracingEnabled: false,
showInvalidAccessTokenError: false,
authenticated: false,
hasVerifiedEmail: false,
requiresVerifiedEmail: false,
siteHasCodyEnabled: false,
siteVersion: '',
userCanUpgrade: false,
username: '',
Expand All @@ -59,13 +55,11 @@ export const defaultAuthStatus: AuthStatus = {

export const unauthenticatedStatus: AuthStatus = {
endpoint: '',
isLoggedIn: false,
isFireworksTracingEnabled: false,
showInvalidAccessTokenError: true,
authenticated: false,
hasVerifiedEmail: false,
requiresVerifiedEmail: false,
siteHasCodyEnabled: false,
siteVersion: '',
userCanUpgrade: false,
username: '',
Expand All @@ -75,27 +69,23 @@ export const unauthenticatedStatus: AuthStatus = {
export const networkErrorAuthStatus: Omit<AuthStatus, 'endpoint'> = {
showInvalidAccessTokenError: false,
authenticated: false,
isLoggedIn: false,
isFireworksTracingEnabled: false,
hasVerifiedEmail: false,
showNetworkError: true,
requiresVerifiedEmail: false,
siteHasCodyEnabled: false,
siteVersion: '',
userCanUpgrade: false,
username: '',
codyApiVersion: 0,
}

export const offlineModeAuthStatus: Omit<AuthStatus, 'endpoint'> = {
isLoggedIn: true,
isOfflineMode: true,
isFireworksTracingEnabled: false,
showInvalidAccessTokenError: false,
authenticated: true,
hasVerifiedEmail: true,
requiresVerifiedEmail: true,
siteHasCodyEnabled: true,
siteVersion: '',
userCanUpgrade: false,
username: 'offline',
Expand Down
6 changes: 1 addition & 5 deletions lib/shared/src/sourcegraph-api/graphql/client.ts
Original file line number Diff line number Diff line change
Expand Up @@ -178,9 +178,6 @@ interface CurrentUserInfoResponse {
//
// For the canonical type definition, see https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/internal/clientconfig/types.go
interface CodyClientConfig {
// Whether the site admin allows this user to make use of Cody at all.
codyEnabled: boolean

// Whether the site admin allows this user to make use of the Cody chat feature.
chatEnabled: boolean

Expand Down Expand Up @@ -1542,7 +1539,7 @@ export class ClientConfigSingleton {
}

public async setAuthStatus(authStatus: AuthStatus): Promise<void> {
this.isSignedIn = authStatus.authenticated && authStatus.isLoggedIn
this.isSignedIn = authStatus.authenticated
if (this.isSignedIn) {
await this.refreshConfig()
} else {
Expand Down Expand Up @@ -1663,7 +1660,6 @@ export class ClientConfigSingleton {
const features = await this.fetchConfigFeaturesLegacy(this.featuresLegacy)

return graphqlClient.isCodyEnabled().then(isCodyEnabled => ({
codyEnabled: isCodyEnabled.enabled,
chatEnabled: features.chat,
autoCompleteEnabled: features.autoComplete,
customCommandsEnabled: features.commands,
Expand Down
12 changes: 6 additions & 6 deletions vscode/src/auth/auth.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export async function showSignInMenu(
uri?: string
): Promise<void> {
const authStatus = authProvider.instance!.status
const mode = authStatus.isLoggedIn ? 'switch' : 'signin'
const mode = authStatus.authenticated ? 'switch' : 'signin'
logDebug('AuthProvider:signinMenu', mode)
telemetryRecorder.recordEvent('cody.auth.login', 'clicked')
const item = await showAuthMenu(mode)
Expand Down Expand Up @@ -57,7 +57,7 @@ export async function showSignInMenu(
endpoint: selectedEndpoint,
token: token || null,
})
if (!authStatus?.isLoggedIn) {
if (!authStatus?.authenticated) {
const newToken = await showAccessTokenInputBox(item.uri)
if (!newToken) {
return
Expand Down Expand Up @@ -213,7 +213,7 @@ async function signinMenuForInstanceUrl(instanceUrl: string): Promise<void> {
})
telemetryRecorder.recordEvent('cody.auth.signin.token', 'clicked', {
metadata: {
success: authState.isLoggedIn ? 1 : 0,
success: authState.authenticated ? 1 : 0,
},
})
await showAuthResultMessage(instanceUrl, authState)
Expand Down Expand Up @@ -246,7 +246,7 @@ async function showAuthResultMessage(
endpoint: string,
authStatus: AuthStatus | undefined
): Promise<void> {
if (authStatus?.isLoggedIn) {
if (authStatus?.authenticated) {
const authority = vscode.Uri.parse(endpoint).authority
await vscode.window.showInformationMessage(`Signed in to ${authority || endpoint}`)
} else {
Expand Down Expand Up @@ -280,10 +280,10 @@ export async function tokenCallbackHandler(
const authState = await authProvider.instance!.auth({ endpoint, token, customHeaders })
telemetryRecorder.recordEvent('cody.auth.fromCallback.web', 'succeeded', {
metadata: {
success: authState?.isLoggedIn ? 1 : 0,
success: authState?.authenticated ? 1 : 0,
},
})
if (authState?.isLoggedIn) {
if (authState?.authenticated) {
await vscode.window.showInformationMessage(`Signed in to ${endpoint}`)
} else {
await showAuthFailureMessage(endpoint)
Expand Down
10 changes: 5 additions & 5 deletions vscode/src/chat/chat-view/ChatController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -457,11 +457,11 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
'succeeded',
{
metadata: {
success: authStatus?.isLoggedIn ? 1 : 0,
success: authStatus?.authenticated ? 1 : 0,
},
}
)
if (!authStatus?.isLoggedIn) {
if (!authStatus?.authenticated) {
void vscode.window.showErrorMessage(
'Authentication failed. Please check your token and try again.'
)
Expand Down Expand Up @@ -508,7 +508,7 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
endpoint: DOTCOM_URL.href,
token,
})
if (!authStatus?.isLoggedIn) {
if (!authStatus?.authenticated) {
void vscode.window.showErrorMessage(
'Authentication failed. Please check your token and try again.'
)
Expand All @@ -523,7 +523,7 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
const nextAuth = authProvider.instance!.status
telemetryRecorder.recordEvent('cody.troubleshoot', 'reloadAuth', {
metadata: {
success: nextAuth.isLoggedIn ? 1 : 0,
success: nextAuth.authenticated ? 1 : 0,
},
})
break
Expand Down Expand Up @@ -566,7 +566,7 @@ export class ChatController implements vscode.Disposable, vscode.WebviewViewProv
void this.sendConfig()

// Get the latest model list available to the current user to update the ChatModel.
if (status.isLoggedIn) {
if (status.authenticated) {
this.chatModel.updateModel(getDefaultModelID())
}
}
Expand Down
4 changes: 2 additions & 2 deletions vscode/src/chat/chat-view/ChatsController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export class ChatsController implements vscode.Disposable {
}

private async setAuthStatus(authStatus: AuthStatus): Promise<void> {
const hasLoggedOut = !authStatus.isLoggedIn
const hasLoggedOut = !authStatus.authenticated
const hasSwitchedAccount =
this.currentAuthAccount && this.currentAuthAccount.endpoint !== authStatus.endpoint
if (hasLoggedOut || hasSwitchedAccount) {
Expand Down Expand Up @@ -350,7 +350,7 @@ export class ChatsController implements vscode.Disposable {
private async exportHistory(): Promise<void> {
telemetryRecorder.recordEvent('cody.exportChatHistoryButton', 'clicked')
const authStatus = authProvider.instance!.status
if (authStatus.isLoggedIn) {
if (authStatus.authenticated) {
try {
const historyJson = chatHistory.getLocalHistory(authStatus)
const exportPath = await vscode.window.showSaveDialog({
Expand Down
Loading

0 comments on commit f8a1b72

Please sign in to comment.