Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix(search): VSCode Search extension: remove auth provider#63262

Merged
peterguy merged 16 commits into
mainfrom
peterguy/vscode-remove-auth-provider
Jun 19, 2024
Merged

fix(search): VSCode Search extension: remove auth provider#63262
peterguy merged 16 commits into
mainfrom
peterguy/vscode-remove-auth-provider

Conversation

@peterguy

Copy link
Copy Markdown
Contributor

VSCode extensions can use authentication providers to enable authentication actions from the Accounts menu, and to allow for federated authentication. The Sourcegraph Search extension needs neither of those - it can meet all of its authentication needs with the existing actions in is side panel.

On top of that, the authentication provider isn't implemented quite correctly, which leads to issues like #43608.

This PR removes the authentication provider.

It retains all of the expected behavior of the extension: the user can still authenticate against sourcegraph.com or private instances using an access token, and log out.

Test plan

First

Build and run locally.

git switch peterguy/vscode-remove-auth-provider
cd client/vscode
pnpm run build

Then

  • Launch extension in VSCode: open the Run and Debug sidebar view in VS Code, then select Launch VS Code Extension from the dropdown menu.
  • Note that the Accounts icon does not display a "1" badge and "Sign in with SOURCEGRAPH_AUTH" is not present in the menu.
  • Click on Have an account? to open the login dialog.
  • Enter an access token and the URL of the Sourcegraph instance to which you would like to connect.
  • Click Authenticate account.
  • Check the Accounts icon and menu - should not be anything there that's a result of Sourcegraph Search.
  • In the Help and Feedback section, click your username to open the logout panel, then log out.
  • Note again, the lack of modifications to the Accounts icon and menu from the Sourcegraph Search extension.

Changelog

  • Remove interaction between the Sourcegraph Search extension and the Accounts menu.

peterguy added 15 commits June 13, 2024 15:20
because it's used only there (and will probably be removed entirely soon)
It uses the exported context from `extension.ts`, simiar to `endpointSetting.ts`.
Doesn't require passing in the secret storage.
instead of the authentication session.
because that's what's returned from `getAccessToken()` now.
Don't update the auth provider when changing the endpoint URL.
Instead, get the access token from secret storage.
All usages of it have been removed.
Instead of needing to call `endpointSetting()` in the constructor.

Simplifies code.
Because the only thing it was doing was comparing to a new endpoint URL before calling `setEndpoint()`.
But `setEndpoint()` does its own check against the stored endpoint before storing it, so there's no need to check in `SourcegraphAuthActions`.
@cla-bot cla-bot Bot added the cla-signed label Jun 14, 2024
@peterguy peterguy requested review from a team June 14, 2024 04:39
@peterguy

Copy link
Copy Markdown
Contributor Author

Flagging @sourcegraph/security-code-review and @shivasurya because this is a PR that interacts with authentication. I do not have any particular worries, but removing the auth providers does change how it interacts with VSCode's Accounts, and a glance by a set of security-minded eyeballs would not be amiss.

@peterguy peterguy linked an issue Jun 14, 2024 that may be closed by this pull request

@camdencheek camdencheek left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks good to me! Thanks for the cleanup!

@willdollman willdollman left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Changes lgtm and I don't have any concerns - removing an auth provider, token storage still seems fine and uses the built-in SecretStorage

@peterguy peterguy merged commit 5ee5e88 into main Jun 19, 2024
@peterguy peterguy deleted the peterguy/vscode-remove-auth-provider branch June 19, 2024 18:23
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

VS Code: Extension breaks VS Code's Account menu in a few ways

3 participants