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.

Sign up for GitHub

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

Jump to bottom

Improve reporting auth errors #6639

Merged
merged 4 commits into from
Jan 15, 2025
Merged

Improve reporting auth errors #6639

merged 4 commits into from
Jan 15, 2025

Conversation

pkukielka
Copy link
Contributor

@pkukielka pkukielka commented Jan 14, 2025 •
edited
Loading

Fixes https://linear.app/sourcegraph/issue/CODY-4648

Changes

This PR improves error reporting if auth configuration is broken.

image

Test plan

  1. Configure custom auth provider as described in External Authentication Providers Support for Cody #6526
  2. Change config to be invalid, e.g. in the commandLine change echo to echox and save a config.
  3. You should get logged out and see a detailed error message describing what is wrong.
  4. Fix the issue and save the config, error should disappear and you should be logged in back.

@pkukielka pkukielka force-pushed the pkukielka/improve-auth-error-reporting branch from 5d457ce to 7c8704b Compare January 14, 2025 17:20
@pkukielka pkukielka force-pushed the pkukielka/improve-auth-error-reporting branch from c1e889f to 0f51f3a Compare January 14, 2025 17:28
dominiccooney
dominiccooney approved these changes Jan 15, 2025
View reviewed changes
Copy link
Contributor

@dominiccooney dominiccooney left a comment

Choose a reason for hiding this comment

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

On the whole, good.

BUT is it a configuration error if the external auth provider fails? Use a more descriptive name. What about ExternalAuthProviderError or something like that?

Some other feedback inline.

lib/shared/src/configuration/auth-resolver.test.ts Outdated
expect(auth.serverEndpoint).toBe('https://my-server.com/')

expect(auth.credentials).toBe(undefined)
expect(auth.error.message).toContain('Unexpected end of JSON input')
Copy link
Contributor

Choose a reason for hiding this comment

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

This isn't working on Windows. In cmd, echo with no arguments tells you whether echoing is on or off (and you can echo on, echo off to do some stdin redirection.)

In Windows to echo nothing, use echo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

lib/shared/src/configuration/auth-resolver.test.ts Outdated
expect(auth.serverEndpoint).toBe('https://my-server.com/')

expect(auth.credentials).toBe(undefined)
expect(auth.error.message).toContain('Credentials expiration cannot be se to the past date')
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
expect(auth.error.message).toContain('Credentials expiration cannot be se to the past date')
expect(auth.error.message).toContain('Credentials expiration cannot be set to a date in the past')

lib/shared/src/configuration/auth-resolver.ts Outdated
const expireInMs = expirationMs - Date.now()
if (expireInMs < 0) {
throw new Error(
'Credentials expiration cannot be se to the past date: ' +
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'Credentials expiration cannot be se to the past date: ' +
'Credentials expiration cannot be set to a date in the past: ' +

lib/shared/src/configuration/auth-resolver.ts Outdated
Comment on lines 82 to 83
const expireInMs = expirationMs - Date.now()
if (expireInMs < 0) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This may be more direct.

Suggested change
const expireInMs = expirationMs - Date.now()
if (expireInMs < 0) {
if (expirationMs < Date.now()) {

@pkukielka
Copy link
Contributor Author

On the whole, good.

BUT is it a configuration error if the external auth provider fails? Use a more descriptive name. What about ExternalAuthProviderError or something like that?

Some other feedback inline.

We cannot be sure it's external provider error.
It can also happen in https://github.com/sourcegraph/cody/pull/6639/files#diff-992197dfbbef5b4750d10ba6fc1809b946883f048685c4845676d7724f192521L114 and be any other error related to parsing config.

That might be showing some structural problem.
Maybe executing the configured command should not happen during config resolution but during auth validation.
But then AuthCredentials should really be AuthCredentialsConfig. It just happened that until now config was equal to credentials and was not requiring further processing.
To be honest I'm not sure which of those two would be better approach.

@pkukielka pkukielka force-pushed the pkukielka/improve-auth-error-reporting branch 2 times, most recently from 048060c to bba0195 Compare January 15, 2025 09:55
@pkukielka pkukielka force-pushed the pkukielka/improve-auth-error-reporting branch from bba0195 to 0eb8453 Compare January 15, 2025 10:39
@pkukielka pkukielka merged commit 1c16f35 into main Jan 15, 2025
21 of 22 checks passed
@pkukielka pkukielka deleted the pkukielka/improve-auth-error-reporting branch January 15, 2025 11:23
dominiccooney added a commit that referenced this pull request Jan 24, 2025
@dominiccooney
Revert "Improve reporting auth errors (#6639)"
2d0cb34 
This reverts commit 1c16f35.
dominiccooney added a commit that referenced this pull request Jan 24, 2025
@dominiccooney
Revert "Improve reporting auth errors (#6639)"
0db8df7 
This reverts commit 1c16f35.
umpox added a commit that referenced this pull request Jan 24, 2025
@umpox
Revert "Improve reporting auth errors (#6639)"
014ee6a 
This reverts commit 1c16f35.
umpox added a commit that referenced this pull request Jan 24, 2025
@umpox
Revert "Improve reporting auth errors (#6639)"
7839cb4 
This reverts commit 1c16f35.
umpox added a commit that referenced this pull request Jan 24, 2025
@umpox
Revert "Improve reporting auth errors (#6639)"
bcccdbb 
This reverts commit 1c16f35.
umpox added a commit that referenced this pull request Jan 24, 2025
@umpox
Revert "Improve reporting auth errors (#6639)"
70adef1 
This reverts commit 1c16f35.
umpox added a commit that referenced this pull request Jan 27, 2025
@umpox
Revert "Improve reporting auth errors (#6639)" (#6789)
f1da24b 
This reverts commit 1c16f35

## Test plan

Initial behaviour is restored. See
[#inc-365-jetbrains-and-vscode-auth-errors](https://sourcegraph.slack.com/archives/C089N0XTC0P)

This revert will be part of a series of reverts, we will validate that
the intended behaviour is restored.


<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->
umpox added a commit that referenced this pull request Jan 27, 2025
@umpox
Revert "Improve reporting auth errors (#6639)"
89d26c5 
This reverts commit 1c16f35.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominiccooney dominiccooney dominiccooney approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@pkukielka @dominiccooney