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

Handle customer proxy re-auth response by retrying, not prompting user for different token #6652

Draft
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

sqs
Copy link
Member

@sqs sqs commented Jan 15, 2025

A customer has a proxy that enforces recent 2fa on their machines to access the Sourcegraph instance. After 24h, the user needs to tap their 2fa key to make the network connection to Sourcegraph be reauthed and start working again. The responses from the proxy after the 24h period expires are being treated as "invalid token" and causing users to need to re-auth into Cody. The response looks like:

{"auth_url":"https://example.com","error":"request not authenticated","requested_url":"example.com/.api/client-config"}

with an HTTP header of the form X-${CUSTOMER}-U2f-Challenge: true.

  • Send Accept: application/json headers to ensure we get that JSON response and not an HTTP redirect from the proxy when the 24h period expires
  • Interpret that (or that kind of) error response in our auth code as a temporary network error and do not tell the user that token is invalid and ask for another token.
  • In general, distinguish between ephemeral network/availability errors and invalid access token errors. Do not ask the user to provide a different token for the former kind of errors.
  • Improve look and feel of the UI authentication error banner, and add a Try Again button.
  • Automatically refresh periodically (currently every 2.5s) when authentication failed awaiting an auth challenge.

Fixed issues (by @pkukielka ):

  • failed autocomplete triggers change to the status bar; if user forces autocomplete popup is additionally show
  • periodic refresh causes UI jitter in the status bar fas been fixed
  • prompt library dropdown error handling has been fixed (by the way of general improvements to the error handling)
  • fixed cody ignore logic to not mask network or auth erros as policy errors
  • fixed some incorrect caching of the ignore API requests
  • completions can now report/reset connection errors shown in a cody status icon

Fixes https://linear.app/sourcegraph/issue/CODY-4695/handle-customer-proxy-re-auth-response-by-retrying-not-prompting-user

Screenshots

When the user needs to complete the auth challenge, they will see the following errors, depending on what they're doing.

If the extension hasn't successfully authenticated since the editor was restarted:

image

In chat:

image

After failed completion:
image

Test plan

TBD

Changelog

  • Cody better distinguishes between ephemeral network/availability errors and invalid access token errors. You are no longer asked to provide a different token if authentication fails due to a network error.
  • For customers using an authentication challenge proxy that wraps the Sourcegraph instance, if it returns HTTP 401 with an HTTP header whose name matches /^X-.*-U2f-Challenge$/i and whose value is true, the user is asked to complete the authentication challenge on their device ("Tap YubiKey to Authenticate...") and is not prompted for a different access token. When the device's authentication challenge is successful, the extension will automatically proceeed to sign the user in using their stored access token (if any).

@sqs sqs marked this pull request as draft January 15, 2025 18:35
@sqs sqs changed the title add narrower Accept: application/json header to outgoing HTTP requests Handle customer proxy re-auth response by retrying, not prompting user for different token Jan 15, 2025
@sqs sqs force-pushed the sqs/client-config-accept branch 5 times, most recently from c77e517 to e7ed90b Compare January 16, 2025 07:51
Comment on lines 1723 to 1728
///////////////////////////////////
// TODO!(sqs): remove
///////////////////////////////////
if (require('node:fs').existsSync('/tmp/is-custom-auth-challenge-error')) {
return true
}
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: this is for local testing and will be removed before merging.

Copy link
Contributor

Choose a reason for hiding this comment

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

FYI I added https://github.com/sourcegraph/cody/blob/main/agent/scripts/reverse-proxy.py which is very simplistic for now but could be easily extended to simulate that kind of proxy response.

@sqs sqs force-pushed the sqs/client-config-accept branch from e7ed90b to 1e632ce Compare January 16, 2025 08:16
@pkukielka pkukielka force-pushed the sqs/client-config-accept branch 2 times, most recently from d771964 to a8e0929 Compare January 22, 2025 14:35
@pkukielka pkukielka force-pushed the sqs/client-config-accept branch from a8e0929 to 70a27b7 Compare January 23, 2025 14:53
@pkukielka pkukielka force-pushed the sqs/client-config-accept branch from 1daa94c to c08e156 Compare January 27, 2025 10:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants