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

Agent: authentication capability - enable url handler and auth redirections #5325

Merged
merged 18 commits into from
Sep 4, 2024

Conversation

abeatrix
Copy link
Contributor

@abeatrix abeatrix commented Aug 25, 2024

Allows agent to set up HTTP server to listen for authentication redirections when agent capabilities for authentication are enabled.

TODO:

Follow-ups:

  • we should close the opened browser to refocus the webview if possible.

Test plan

Example of this feature working in Visual Studio:

vs_agent_auth.mp4

Updated onboarding view for non VS Code editors to support login with browser for enterprise:

image

Changelog

## Description

This PR adds an `AgentAuthHandler` class that sets up an Express server to handle authentication flows. The server listens on a local port and redirects the user to the Sourcegraph login page when the agent is started. Once the user authenticates, the server receives the token callback and notifies the agent.

The `vscode-shim` is also updated to register a URI handler that passes the callback URL to the `AgentAuthHandler` instance.

This feature is enabled when the client capabilities indicate that authentication is 'enabled'.

## Change Summary
- Add `express` dependency
- Implement `AgentAuthHandler` class to manage authentication flow
- Update `vscode-shim` to register URI handler for authentication callback
@abeatrix abeatrix changed the title WIP: Agent: authentication capability Agent: authentication capability Aug 28, 2024
@abeatrix abeatrix marked this pull request as ready for review August 28, 2024 15:43
@abeatrix abeatrix changed the title Agent: authentication capability Agent: authentication capability - enable url handler and auth redirections Aug 28, 2024
@abeatrix abeatrix requested review from olafurpg and a team August 28, 2024 16:04
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.

Love the direction. Can we polish this a bit...

  • Bind a dynamic port so we don't have port conflicts, assuming the site side supports different port numbers.
  • Bind the loopback interface, some firewalls may be more lenient about this.
  • Would be cool if the client requested focus when it has the token, although I guess that's a separate, per-client patch.
  • I guess we can't remove the old app link method until a certain Sourcegraph server support window passes. Can you put TODOs or something mentioning specific versions being sunset when we can remove that? Can we turn off the app link method for dotcom and any compatible Sourcegraph version?

agent/src/AgentAuthHandler.ts Outdated Show resolved Hide resolved
agent/src/AgentAuthHandler.ts Outdated Show resolved Hide resolved
agent/src/AgentAuthHandler.ts Outdated Show resolved Hide resolved
@dominiccooney
Copy link
Contributor

Ah... one more thing we need to consider. When the extension is running remotely, like SSH hosts, we should take that opportunity to have a login method that works (or at least helpfully fails... link to docs, way to manually set a token, etc.)

@abeatrix abeatrix requested review from dominiccooney and a team September 3, 2024 22:10
@abeatrix
Copy link
Contributor Author

abeatrix commented Sep 3, 2024

@dominiccooney thanks for the helpful review, I've updated the PR address some of your feedback.

  • Bind a dynamic port so we don't have port conflicts, assuming the site side supports different port numbers.
  • Bind the loopback interface, some firewalls may be more lenient about this.
  • Would be cool if the client requested focus when it has the token, although I guess that's a separate, per-client patch.
  • I guess we can't remove the old app link method until a certain Sourcegraph server support window passes. Can you put TODOs or something mentioning specific versions being sunset when we can remove that? Can we turn off the app link method for dotcom and any compatible Sourcegraph version?

For the last item, which old app link method are you referring to?

@dominiccooney
Copy link
Contributor

For the last item, which old app link method are you referring to?

The ones that read the querystring and redirect to URLs like this... vscode://sourcegraph.cody-ai etc.

High level question is, should we migrate to all http://localhost redirect logins? Or should we keep supporting the vscode:// etc. method forever. Simpler is better and localhost redirects mean we can ship new clients without updating old Sourcegraph instances.

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.

Great!

@abeatrix
Copy link
Contributor Author

abeatrix commented Sep 4, 2024

High level question is, should we migrate to all http://localhost/ redirect logins? Or should we keep supporting the vscode:// etc. method forever. Simpler is better and localhost redirects mean we can ship new clients without updating old Sourcegraph instances.

I agree that simpler is better. Right now, adding new IDE redirection support through the SG instance requires enterprise users to upgrade to the latest version of SG. Since that change would require a server-side change that is not covered by this PR, I will create a linear issue for it. Thanks for the review, Dom!

Update: https://linear.app/sourcegraph/issue/CODY-3629

@abeatrix abeatrix merged commit 9a7e712 into main Sep 4, 2024
19 checks passed
@abeatrix abeatrix deleted the bee/agent-auth branch September 4, 2024 23:16
abeatrix added a commit that referenced this pull request Sep 5, 2024
Follow #5325

This PR updates the current auth to replace getAuthReferralCode with the
new getCodyAuthReferralCode added in
#5325

## Test plan

<!-- Required. See
https://docs-legacy.sourcegraph.com/dev/background-information/testing_principles.
-->

All the current tests should pass to confirm this change works the same
as before.

## Changelog

<!-- OPTIONAL; info at
https://www.notion.so/sourcegraph/Writing-a-changelog-entry-dd997f411d524caabf0d8d38a24a878c
-->
abeatrix added a commit to sourcegraph/cody-vs that referenced this pull request Sep 6, 2024
CLOSE #72
https://linear.app/sourcegraph/issue/CODY-3619/implement-ivscredentialstorageservice-interface-for-storing-secrets
https://linear.app/sourcegraph/issue/CODY-3618/agent-api-for-secret-storage-capability
https://linear.app/sourcegraph/issue/CODY-3617/implement-agent-requests-for-secret-storage-capability

Try logout and then log back into Cody to confirm you can now use token
redirect and secret storage via Agent:


![image](https://github.com/user-attachments/assets/cf7c1838-11a5-44d0-b655-eeb639259abe)

This PR enables client capability for authentication (added in
sourcegraph/cody#5325) and secrets (added in
sourcegraph/cody#5348) that allows users to use
the native webview for authentication in Cody for Visual Studio.

The protocols for secret storage operations have also been implemented
and the secrets will be stored in
[IVsCredentialStorageService](https://learn.microsoft.com/en-us/dotnet/api/microsoft.visualstudio.shell.connected.credentialstorage?view=visualstudiosdk-2017).

Demo:


https://github.com/user-attachments/assets/d21c8c2f-2667-426a-9c4d-991b7645d2e5

---------

Co-authored-by: Piotr Karczmarz <piotr@karczmarz.com>
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