From 70adef1783475d3ba37ce60d776f7995ef1fd636 Mon Sep 17 00:00:00 2001 From: Tom Ross Date: Fri, 24 Jan 2025 09:28:18 +0000 Subject: [PATCH] Revert "Improve reporting auth errors (#6639)" This reverts commit 1c16f3569c519c3c516a2b9f89cd489b7e5238da. --- .../protocol_generated/AuthenticationError.kt | 12 ---- .../CodyContextFilterItem.kt | 11 +-- .../agent/protocol_generated/Constants.kt | 2 - agent/scripts/reverse-proxy.py | 0 .../scripts/simple-external-auth-provider.py | 21 ------ .../scip-codegen/emitters/KotlinEmitter.ts | 2 +- lib/shared/src/auth/types.ts | 12 +--- lib/shared/src/configuration.ts | 1 - .../src/configuration/auth-resolver.test.ts | 69 +----------------- lib/shared/src/configuration/auth-resolver.ts | 72 +++++++------------ lib/shared/src/configuration/resolver.ts | 15 ++-- vscode/src/auth/auth.ts | 22 +----- 12 files changed, 45 insertions(+), 194 deletions(-) mode change 100755 => 100644 agent/scripts/reverse-proxy.py delete mode 100755 agent/scripts/simple-external-auth-provider.py diff --git a/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/AuthenticationError.kt b/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/AuthenticationError.kt index 1a9d1c571983..3535471ab4be 100644 --- a/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/AuthenticationError.kt +++ b/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/AuthenticationError.kt @@ -16,7 +16,6 @@ sealed class AuthenticationError { "network-error" -> context.deserialize(element, NetworkAuthError::class.java) "invalid-access-token" -> context.deserialize(element, InvalidAccessTokenError::class.java) "enterprise-user-logged-into-dotcom" -> context.deserialize(element, EnterpriseUserDotComError::class.java) - "auth-config-error" -> context.deserialize(element, AuthConfigError::class.java) else -> throw Exception("Unknown discriminator ${element}") } } @@ -51,14 +50,3 @@ data class EnterpriseUserDotComError( } } -data class AuthConfigError( - val title: String? = null, - val message: String, - val type: TypeEnum, // Oneof: auth-config-error -) : AuthenticationError() { - - enum class TypeEnum { - @SerializedName("auth-config-error") `Auth-config-error`, - } -} - diff --git a/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/CodyContextFilterItem.kt b/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/CodyContextFilterItem.kt index d54012c2afbb..471920ca5ab1 100644 --- a/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/CodyContextFilterItem.kt +++ b/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/CodyContextFilterItem.kt @@ -1,15 +1,8 @@ @file:Suppress("FunctionName", "ClassName", "unused", "EnumEntryName", "UnusedImport") package com.sourcegraph.cody.agent.protocol_generated; -import com.google.gson.annotations.SerializedName; - data class CodyContextFilterItem( - val repoNamePattern: RepoNamePatternEnum, // Oneof: .* + val repoNamePattern: String, val filePathPatterns: List? = null, -) { - - enum class RepoNamePatternEnum { - @SerializedName(".*") Wildcard, - } -} +) diff --git a/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/Constants.kt b/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/Constants.kt index 5e6294c2ffc5..515d5e785346 100644 --- a/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/Constants.kt +++ b/agent/bindings/kotlin/lib/src/main/kotlin/com/sourcegraph/cody/agent/protocol_generated/Constants.kt @@ -2,7 +2,6 @@ package com.sourcegraph.cody.agent.protocol_generated; object Constants { - const val Wildcard = ".*" const val Applied = "Applied" const val Applying = "Applying" const val Automatic = "Automatic" @@ -16,7 +15,6 @@ object Constants { const val agentic = "agentic" const val ask = "ask" const val assistant = "assistant" - const val `auth-config-error` = "auth-config-error" const val authenticated = "authenticated" const val autocomplete = "autocomplete" const val balanced = "balanced" diff --git a/agent/scripts/reverse-proxy.py b/agent/scripts/reverse-proxy.py old mode 100755 new mode 100644 diff --git a/agent/scripts/simple-external-auth-provider.py b/agent/scripts/simple-external-auth-provider.py deleted file mode 100755 index 11209b7ed4f5..000000000000 --- a/agent/scripts/simple-external-auth-provider.py +++ /dev/null @@ -1,21 +0,0 @@ -#!/usr/bin/env python3 -import json -import time -from datetime import datetime - -def generate_credentials(): - current_epoch = int(time.time()) + 100 - - credentials = { - "headers": { - "Authorization": "Bearer SomeUser", - "Expiration": current_epoch, - }, - "expiration": current_epoch - } - - # Print JSON to stdout - print(json.dumps(credentials)) - -if __name__ == "__main__": - generate_credentials() diff --git a/agent/src/cli/scip-codegen/emitters/KotlinEmitter.ts b/agent/src/cli/scip-codegen/emitters/KotlinEmitter.ts index e3d5b67b3d56..7814d66d849c 100644 --- a/agent/src/cli/scip-codegen/emitters/KotlinEmitter.ts +++ b/agent/src/cli/scip-codegen/emitters/KotlinEmitter.ts @@ -265,7 +265,7 @@ export class KotlinFormatter extends Formatter { } override formatFieldName(name: string): string { - const escaped = name.replace('.*', 'Wildcard').replace(':', '_').replace('/', '_') + const escaped = name.replace(':', '_').replace('/', '_') const isKeyword = this.options.reserved.has(escaped) const needsBacktick = isKeyword || !/^[a-zA-Z0-9_]+$/.test(escaped) // Replace all non-alphanumeric characters with underscores diff --git a/lib/shared/src/auth/types.ts b/lib/shared/src/auth/types.ts index cca6b6bd6d3a..1a5bc31babf9 100644 --- a/lib/shared/src/auth/types.ts +++ b/lib/shared/src/auth/types.ts @@ -62,15 +62,7 @@ export interface EnterpriseUserDotComError { enterprise: string } -export interface AuthConfigError extends AuthenticationErrorMessage { - type: 'auth-config-error' -} - -export type AuthenticationError = - | NetworkAuthError - | InvalidAccessTokenError - | EnterpriseUserDotComError - | AuthConfigError +export type AuthenticationError = NetworkAuthError | InvalidAccessTokenError | EnterpriseUserDotComError export interface AuthenticationErrorMessage { title?: string @@ -98,8 +90,6 @@ export function getAuthErrorMessage(error: AuthenticationError): AuthenticationE "in through your organization's enterprise instance instead. If you need assistance " + 'please contact your Sourcegraph admin.', } - case 'auth-config-error': - return error } } diff --git a/lib/shared/src/configuration.ts b/lib/shared/src/configuration.ts index 10397e508d46..3e43fc12f89d 100644 --- a/lib/shared/src/configuration.ts +++ b/lib/shared/src/configuration.ts @@ -18,7 +18,6 @@ export type TokenSource = 'redirect' | 'paste' export interface AuthCredentials { serverEndpoint: string credentials: HeaderCredential | TokenCredential | undefined - error?: any } export interface HeaderCredential { diff --git a/lib/shared/src/configuration/auth-resolver.test.ts b/lib/shared/src/configuration/auth-resolver.test.ts index 9b779fe707e8..52f69e429e38 100644 --- a/lib/shared/src/configuration/auth-resolver.test.ts +++ b/lib/shared/src/configuration/auth-resolver.test.ts @@ -61,10 +61,9 @@ describe('auth-resolver', () => { }) test('resolve custom auth provider', async () => { - const futureEpoch = Date.UTC(2050) / 1000 const credentialsJson = JSON.stringify({ headers: { Authorization: 'token X' }, - expiration: futureEpoch, + expiration: 1337, }) const auth = await resolveAuth( @@ -92,75 +91,11 @@ describe('auth-resolver', () => { expect(auth.serverEndpoint).toBe('https://my-server.com/') const headerCredential = auth.credentials as HeaderCredential - expect(headerCredential.expiration).toBe(futureEpoch) + expect(headerCredential.expiration).toBe(1337) expect(headerCredential.getHeaders()).toStrictEqual({ Authorization: 'token X', }) expect(JSON.stringify(headerCredential)).not.toContain('token X') }) - - test('resolve custom auth provider error handling - bad JSON', async () => { - const auth = await resolveAuth( - 'sourcegraph.com', - { - authExternalProviders: [ - { - endpoint: 'https://my-server.com', - executable: { - commandLine: ['echo x'], - shell: isWindows() ? process.env.ComSpec : '/bin/bash', - timeout: 5000, - windowsHide: true, - }, - }, - ], - overrideServerEndpoint: 'https://my-server.com', - overrideAuthToken: undefined, - }, - new TempClientSecrets(new Map()) - ) - - expect(auth.serverEndpoint).toBe('https://my-server.com/') - - expect(auth.credentials).toBe(undefined) - expect(auth.error.message).toContain('Failed to execute external auth command: Unexpected token') - }) - - test('resolve custom auth provider error handling - bad expiration', async () => { - const expiredEpoch = Date.UTC(2020) / 1000 - const credentialsJson = JSON.stringify({ - headers: { Authorization: 'token X' }, - expiration: expiredEpoch, - }) - - const auth = await resolveAuth( - 'sourcegraph.com', - { - authExternalProviders: [ - { - endpoint: 'https://my-server.com', - executable: { - commandLine: [ - isWindows() ? `echo ${credentialsJson}` : `echo '${credentialsJson}'`, - ], - shell: isWindows() ? process.env.ComSpec : '/bin/bash', - timeout: 5000, - windowsHide: true, - }, - }, - ], - overrideServerEndpoint: 'https://my-server.com', - overrideAuthToken: undefined, - }, - new TempClientSecrets(new Map()) - ) - - expect(auth.serverEndpoint).toBe('https://my-server.com/') - - expect(auth.credentials).toBe(undefined) - expect(auth.error.message).toContain( - 'Credentials expiration cannot be set to a date in the past' - ) - }) }) diff --git a/lib/shared/src/configuration/auth-resolver.ts b/lib/shared/src/configuration/auth-resolver.ts index 688d255c4e5c..414a21302262 100644 --- a/lib/shared/src/configuration/auth-resolver.ts +++ b/lib/shared/src/configuration/auth-resolver.ts @@ -64,56 +64,38 @@ export async function resolveAuth( const { authExternalProviders, overrideServerEndpoint, overrideAuthToken } = configuration const serverEndpoint = normalizeServerEndpointURL(overrideServerEndpoint || endpoint) - try { - if (overrideAuthToken) { - return { credentials: { token: overrideAuthToken }, serverEndpoint } - } + if (overrideAuthToken) { + return { credentials: { token: overrideAuthToken }, serverEndpoint } + } - const credentials = await getExternalProviderAuthResult( - serverEndpoint, - authExternalProviders - ).catch(error => { - throw new Error(`Failed to execute external auth command: ${error.message || error}`) - }) - - if (credentials) { - if (credentials?.expiration) { - const expirationMs = credentials?.expiration * 1000 - if (expirationMs < Date.now()) { - throw new Error( - 'Credentials expiration cannot be set to a date in the past: ' + - `${new Date(expirationMs)} (${credentials.expiration})` - ) - } - } - return { - credentials: { - expiration: credentials?.expiration, - getHeaders() { - return credentials.headers - }, - }, - serverEndpoint, - } + const credentials = await getExternalProviderAuthResult(serverEndpoint, authExternalProviders).catch( + error => { + throw new Error(`Failed to execute external auth command: ${error}`) } + ) - const token = await clientSecrets.getToken(serverEndpoint).catch(error => { - throw new Error( - `Failed to get access token for endpoint ${serverEndpoint}: ${error.message || error}` - ) - }) - - return { - credentials: token - ? { token, source: await clientSecrets.getTokenSource(serverEndpoint) } - : undefined, - serverEndpoint, - } - } catch (error) { + if (credentials) { return { - credentials: undefined, + credentials: { + expiration: credentials?.expiration, + getHeaders() { + return credentials.headers + }, + }, serverEndpoint, - error, } } + + const token = await clientSecrets.getToken(serverEndpoint).catch(error => { + throw new Error( + `Failed to get access token for endpoint ${serverEndpoint}: ${error.message || error}` + ) + }) + + return { + credentials: token + ? { token, source: await clientSecrets.getTokenSource(serverEndpoint) } + : undefined, + serverEndpoint, + } } diff --git a/lib/shared/src/configuration/resolver.ts b/lib/shared/src/configuration/resolver.ts index a6df637a102a..4cdc90d95e27 100644 --- a/lib/shared/src/configuration/resolver.ts +++ b/lib/shared/src/configuration/resolver.ts @@ -96,7 +96,14 @@ async function resolveConfiguration({ const auth = await resolveAuth(serverEndpoint, clientConfiguration, clientSecrets) const cred = auth.credentials if (cred !== undefined && 'expiration' in cred && cred.expiration !== undefined) { - const expireInMs = cred.expiration * 1000 - Date.now() + const expirationMs = cred.expiration * 1000 + const expireInMs = expirationMs - Date.now() + if (expireInMs < 0) { + throw new Error( + 'Credentials expiration cannot be se to the past date:' + + `${new Date(expirationMs)} (${cred.expiration})` + ) + } setInterval(() => _refreshConfigRequests.next(), expireInMs) } return { configuration: clientConfiguration, clientState, auth, isReinstall } @@ -104,11 +111,7 @@ async function resolveConfiguration({ // We don't want to throw here, because that would cause the observable to terminate and // all callers receiving no further config updates. logError('resolveConfiguration', `Error resolving configuration: ${error}`) - const auth = { - credentials: undefined, - serverEndpoint, - error: error, - } + const auth = { credentials: undefined, serverEndpoint } return { configuration: clientConfiguration, clientState, auth, isReinstall } } } diff --git a/vscode/src/auth/auth.ts b/vscode/src/auth/auth.ts index 414baef92c59..822614eb42c2 100644 --- a/vscode/src/auth/auth.ts +++ b/vscode/src/auth/auth.ts @@ -90,7 +90,9 @@ export async function showSignInMenu( const { configuration } = await currentResolvedConfig() const auth = await resolveAuth(selectedEndpoint, configuration, secretStorage) - let authStatus = await authProvider.validateAndStoreCredentials(auth, 'store-if-valid') + let authStatus = auth.credentials + ? await authProvider.validateAndStoreCredentials(auth, 'store-if-valid') + : undefined if (!authStatus?.authenticated) { const token = await showAccessTokenInputBox(selectedEndpoint) @@ -404,24 +406,6 @@ export async function validateCredentials( signal?: AbortSignal, clientConfig?: CodyClientConfig ): Promise { - if (config.auth.error !== undefined) { - logDebug( - 'auth', - `Failed to authenticate to ${config.auth.serverEndpoint} due to configuration error`, - config.auth.error - ) - return { - authenticated: false, - endpoint: config.auth.serverEndpoint, - pendingValidation: false, - error: { - type: 'auth-config-error', - title: 'Auth config error', - message: config.auth.error?.message ?? config.auth.error, - }, - } - } - // An access token is needed except for Cody Web, which uses cookies. if (!config.auth.credentials && !clientCapabilities().isCodyWeb) { return { authenticated: false, endpoint: config.auth.serverEndpoint, pendingValidation: false }