Refactor external auth providers to re-generate headers on demand #6687

pkukielka · 2025-01-17T13:31:04Z

Changes

That PR eliminates annoying chat reload upon an token refresh if external auth provider is used.
It does so by splitting external auth provider evaluation into two steps which happens at different stages: config resolution and http request building.

At config resolution we only evaluate config itself and prepare a function which will be later used to generate auth headers later. That function (getHeaders) is then used to obtain auth headers every time we are doing an authorised http request. Normally headers are cached, but if they expire getHeaders should internally refresh the cache and return an updated headers. If updated headers are still expired error will be shown to the user.

In opposition to the previous solution errors in executing external provider command, or token expiration, does not lead
to the config invalidation - config always remains the same unless changed by the user. The only thing which changes is result of getHeaders functions.
Errors from getHeaders are processed and handled in the same way as e.g. Network Error or Invalid Access Token, so we do not need custom code to handle them.

Test plan

Setup

Set cody.auth.externalProviders config to use provider which support credentials expiration.
You can use configuration shown bellow.
Please also make sure you have proxy running and your sourcegraph server have http auth proxy enabled, as described in #6526.

{
    "cody.auth.externalProviders": [
        {
            "endpoint": "http://localhost:7777",
            "executable": {
                "commandLine": ["/Users/pkukielka/Work/sourcegraph/cody2/agent/scripts/simple-external-auth-provider.py"],
                "shell": "/bin/bash",
            }
        }
    ],
    
    "cody.override.serverEndpoint": "http://localhost:7777",
}

Scenario 1:

Start cody, if you used simple-external-auth-provider.py should be successfully signed-in as someuser
Test chat and autocompletions - everything should work
Open simple-external-auth-provider.py and change current_epoch = int(time.time()) + 30 to current_epoch = int(time.time()) - 30
Wait 30 sec and do some Cody action, e.g. ask a chat question
You should get an error, but your conversation should be preserved
Revert your changes to simple-external-auth-provider.py
Ask the question again - chat should be working again

Scenario 2:

Start cody, if you used simple-external-auth-provider.py should be successfully signed-in as someuser
Test chat and autocompletions - everything should work
Open simple-external-auth-provider.py and change current_epoch = int(time.time()) + 30 to current_epoch = int(time.time()) - 30
Quickly do some Cody actions before 30 sec will pass.
They should be successful, but when 30 sec will pas since last action before the simple-external-auth-provider.py edit, you should get an credential expiration error
Revert your changes to simple-external-auth-provider.py
Make sure Cody works fine again

Scenario 3:

Open simple-external-auth-provider.py and change current_epoch = int(time.time()) + 30 to current_epoch = int(time.time()) - 30
Start Cody
You should see a credential expiration error
Click 'Sign In' - it should try to sign you in but eventually fail
Revert changes to simple-external-auth-provider.py
Click 'Sign In' - it should succeed
Make sure Cody chat and autocompletions works

pkukielka · 2025-01-17T13:37:52Z

lib/shared/src/configuration.ts

@@ -23,8 +23,7 @@ export interface AuthCredentials {

 export interface HeaderCredential {
    // We use function instead of property to prevent accidential top level serialization - we never want to store this data
-    getHeaders(): Record<string, string>
-    expiration: number | undefined


Exposing it in the API only complicates the code, we don't need it there.

pkukielka · 2025-01-17T13:41:33Z

vscode/src/services/open-telemetry/OpenTelemetryService.node.ts

@@ -33,28 +37,43 @@ export class OpenTelemetryService {

    constructor() {
        this.configSubscription = combineLatest(
+            externalAuthRefresh,


A little hack, as CodyTraceExport does not expose API which would allow us to add custom headers per request, so we need to create new instance of it every time headers changes.

Changes are simple though, diff is big due to indentation change.

Hmm, this is not watertight. We could leak the credentials from site X to trace provider for site Y if the external auth refresh and resolved config timings are out of step.

Also because the external auth refresh pings for changes but doesn't carry the value (which seems good, it will catch up immediately) if there's some rapid changes to auth, maybe things can get out of step more easily.

Could you drop a TODO here and point to https://linear.app/sourcegraph/project/cody-auth-state-rewritecleanup-7ac1409cc579/overview or a task within it, that this state should be joined?

Good point, I will add a TODO!
On a good side I do not think we would leak anything either way because I added safeguard to the function sending headers:

export function addAuthHeaders(auth: AuthCredentials, headers: Headers, url: URL): void { // We want to be sure we sent authorization headers only to the valid endpoint if (auth.credentials && url.host === new URL(auth.serverEndpoint).host) {

So in worst case request won't pass because of missing credentials, but it will not leak them.
But I agree it's a problem regardless.

Oh nice, I missed that guard. 👍

dominiccooney

Some feedback inline.

dominiccooney · 2025-01-21T00:51:34Z

lib/shared/src/configuration/auth-resolver.ts

+    return credentials
+}
+
+async function createTokenCredentails(


Suggested change

async function createTokenCredentails(

async function createTokenCredentials(

dominiccooney · 2025-01-21T00:52:14Z

lib/shared/src/configuration/auth-resolver.ts


-    return undefined
+function createHeaderCredentails(


Suggested change

function createHeaderCredentails(

function createHeaderCredentials(

dominiccooney · 2025-01-21T00:57:30Z

lib/shared/src/sourcegraph-api/errors.ts

+
+export class ExternalProviderAuthError extends Error {}
+
+export function isExternalProviderAuthError(error: unknown): boolean {


Use a type predicate so the result of this check flows into TypeScript type inference, see https://www.typescriptlang.org/docs/handbook/2/narrowing.html#using-type-predicates

Suggested change

export function isExternalProviderAuthError(error: unknown): boolean {

export function isExternalProviderAuthError(error: unknown): error is ExternalAuthProvider {

dominiccooney · 2025-01-21T01:01:41Z

vscode/src/services/open-telemetry/OpenTelemetryService.node.ts

@@ -33,28 +37,43 @@ export class OpenTelemetryService {

    constructor() {
        this.configSubscription = combineLatest(
+            externalAuthRefresh,


Hmm, this is not watertight. We could leak the credentials from site X to trace provider for site Y if the external auth refresh and resolved config timings are out of step.

Also because the external auth refresh pings for changes but doesn't carry the value (which seems good, it will catch up immediately) if there's some rapid changes to auth, maybe things can get out of step more easily.

Could you drop a TODO here and point to https://linear.app/sourcegraph/project/cody-auth-state-rewritecleanup-7ac1409cc579/overview or a task within it, that this state should be joined?

dominiccooney · 2025-01-21T01:19:47Z

lib/shared/src/configuration/auth-resolver.ts

+            async getHeaders() {
+                try {
+                    if (!_headersCache || hasExpired((await _headersCache)?.expiration)) {
+                        _headersCache = getExternalProviderHeaders(externalProvider)


This isn't right. You can have two race in here and both call getExternalProviderHeaders one after the other.

Typical pattern would be to check the Promise you awaited is the one you're resetting, something like:

while (true) { const observed = _headersCache; if (!observed || hasExpired(await observed)?.expiration) { if (observed !== _headersCache) { continue; // retry } observed = _headersCache = getExternalProviderHeaders(externalProvider); } return (await observed).headers; }

dominiccooney · 2025-01-21T01:24:17Z

lib/shared/src/configuration/auth-resolver.ts

+        throw new Error(`Output of the external auth command is invalid: ${result}`)
+    }
+
+    if (credentials.expiration && hasExpired(credentials.expiration)) {


hasExpired already handles undefined as "not expired" so you can simplify this to skip credentials.expiration &&

dominiccooney

Splendid!

dominiccooney · 2025-01-21T13:15:27Z

vscode/src/services/open-telemetry/OpenTelemetryService.node.ts

@@ -33,28 +37,43 @@ export class OpenTelemetryService {

    constructor() {
        this.configSubscription = combineLatest(
+            externalAuthRefresh,


Oh nice, I missed that guard. 👍

…mand (#6687)" This reverts commit 03c93f9.

pkukielka commented Jan 17, 2025

View reviewed changes

pkukielka force-pushed the pkukielka/refactor-ext-auth-provider branch 2 times, most recently from c9d62f0 to 1027505 Compare January 20, 2025 08:52

dominiccooney requested changes Jan 21, 2025

View reviewed changes

dominiccooney reviewed Jan 21, 2025

View reviewed changes

Refactor external auth providers to re-generate headers on demand

da075df

pkukielka force-pushed the pkukielka/refactor-ext-auth-provider branch from 1027505 to f089485 Compare January 21, 2025 09:52

Fixes for review

9831df6

pkukielka force-pushed the pkukielka/refactor-ext-auth-provider branch from f089485 to 9831df6 Compare January 21, 2025 09:54

dominiccooney approved these changes Jan 21, 2025

View reviewed changes

pkukielka merged commit 03c93f9 into main Jan 21, 2025
20 of 21 checks passed

pkukielka deleted the pkukielka/refactor-ext-auth-provider branch January 21, 2025 13:28

umpox added a commit that referenced this pull request Jan 24, 2025

Revert "Refactor external auth providers to re-generate headers on de…

b001a7c

…mand (#6687)" This reverts commit 03c93f9.

umpox added a commit that referenced this pull request Jan 24, 2025

Revert "Refactor external auth providers to re-generate headers on de…

95b0e79

…mand (#6687)" This reverts commit 03c93f9.

umpox mentioned this pull request Jan 24, 2025

Revert "Refactor external auth providers to re-generate headers on de… #6792

Closed

umpox added a commit that referenced this pull request Jan 27, 2025

Revert "Refactor external auth providers to re-generate headers on de…

76f7d78

…mand (#6687)" This reverts commit 03c93f9.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor external auth providers to re-generate headers on demand #6687

Refactor external auth providers to re-generate headers on demand #6687

pkukielka commented Jan 17, 2025 •

edited

Loading

pkukielka Jan 17, 2025

pkukielka Jan 17, 2025 •

edited

Loading

dominiccooney Jan 21, 2025

pkukielka Jan 21, 2025 •

edited

Loading

dominiccooney Jan 21, 2025

dominiccooney left a comment

dominiccooney Jan 21, 2025

dominiccooney Jan 21, 2025

dominiccooney Jan 21, 2025

dominiccooney Jan 21, 2025

dominiccooney Jan 21, 2025

dominiccooney Jan 21, 2025

dominiccooney left a comment

dominiccooney Jan 21, 2025

	async function createTokenCredentails(
	async function createTokenCredentials(

	function createHeaderCredentails(
	function createHeaderCredentials(


		export class ExternalProviderAuthError extends Error {}

		export function isExternalProviderAuthError(error: unknown): boolean {

Refactor external auth providers to re-generate headers on demand #6687

Refactor external auth providers to re-generate headers on demand #6687

Conversation

pkukielka commented Jan 17, 2025 • edited Loading

Changes

Test plan

Choose a reason for hiding this comment

pkukielka Jan 17, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkukielka Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominiccooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dominiccooney left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pkukielka commented Jan 17, 2025 •

edited

Loading

pkukielka Jan 17, 2025 •

edited

Loading

pkukielka Jan 21, 2025 •

edited

Loading