Switch to webview-based sign-in flow #2382

pkukielka · 2024-10-03T15:02:01Z

Fixes CODY-4347

Test plan

Full QA, I will add more instructions later.

Depends on:

mkondratek

in general looks good - I will try it in the morning 🌅

mkondratek · 2024-10-03T21:18:20Z

src/main/kotlin/com/sourcegraph/cody/listeners/CodyDocumentListener.kt

 import com.sourcegraph.cody.telemetry.TelemetryV2
 import com.sourcegraph.cody.vscode.InlineCompletionTriggerKind
 import com.sourcegraph.utils.CodyEditorUtil

 class CodyDocumentListener(val project: Project) : BulkAwareDocumentListener {

+  // TODO: Looks like this functionality is broken after the migration to webview


let us have a linear issue for this one

mkondratek · 2024-10-03T21:21:20Z

src/main/kotlin/com/sourcegraph/cody/config/migration/AccountsMigration.kt

+    codyAccountManager.getAccounts().forEach { oldAccount ->
+      val token = codyAccountManager.getTokenForAccount(oldAccount)
+      if (token != null) {
+        CodyAccount(oldAccount.server).storeToken(token)


mkondratek · 2024-10-03T21:27:27Z

src/main/kotlin/com/sourcegraph/cody/api/SourcegraphApiRequests.kt

+        val id: String,
+        val username: String,
+        val displayName: String?,
+        val avatarURL: String?


we do not use avatarURL anymore - I think we can remove it

mkondratek · 2024-10-03T21:30:04Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+  @JsonRequest("secrets/get")
+  fun secrets_get(params: Secrets_GetParams): CompletableFuture<String?> {
+    return CompletableFuture.completedFuture(
+        CodyAccount(SourcegraphServerPath(params.key)).getToken())
+  }
+
+  @JsonRequest("secrets/store")
+  fun secrets_store(params: Secrets_StoreParams): CompletableFuture<Null?> {
+    CodyAccount(SourcegraphServerPath(params.key)).storeToken(params.value)
+    return CompletableFuture.completedFuture(null)
+  }
+
+  @JsonRequest("secrets/delete")
+  fun secrets_delete(params: Secrets_DeleteParams): CompletableFuture<Null?> {
+    CodyAccount(SourcegraphServerPath(params.key)).storeToken(null)
+    return CompletableFuture.completedFuture(null)
+  }
+


maybe a stupid question but... is it safe? are there any security issues? would the secrets be present in the trace logs?

We log the secrets in the agent trace, not the log. You have to set an extra environment variable to turn on that level of logging... I think it should be fine.

We already log tokens in that file today... the ExtensionConfiguration message from JetBrains to Agent has the token in it... see build/sourcegraph/cody-agent-trace.json after a debugging session.

It is a s Dominic said :)

mkondratek · 2024-10-03T21:33:17Z

src/main/java/com/sourcegraph/config/CodyAuthNotificationActivity.java

@@ -79,15 +64,4 @@ public void actionPerformed(@NotNull AnActionEvent anActionEvent) {
    notification.addAction(neverShowAgainAction);
    Notifications.Bus.notify(notification);
  }
-
-  private void showMissingTokenNotification() {


btw, I wonder... what if the token expires (or is removed)? 🤔 what will happen (with chat, with autocomplete)?

I will test this scenario today. In general I think we should then (if user try to call some action explicitly) show cody panel, and it should be showing login panel.

By the way, VSCode has various APIs for showing views, we don't implement them. If we need to show the view, we should see if the extension is trying to do it anyway. Maybe we just need to implement a couple of those methods/properties in the VSCode shim.

mkondratek · 2024-10-03T21:38:57Z

src/main/kotlin/com/sourcegraph/cody/chat/actions/ExportChatsAction.kt

Don't we want to keep this action? I know it is possible to export chats from History tab in the webview. Simply, this action is an actual jetbrains action and can be searched for and run from anywhere

Yeah, for now we should keep this, and Explain, etc.

It would be a nice cleanup project later to ingest the VSCode package.json and consider wiring up those commands automatically--or at least linting that we have them.

Ok, I reverted that changes.

dominiccooney · 2024-10-04T01:01:38Z

src/main/kotlin/com/sourcegraph/cody/CodyToolWindowContent.kt

-  private val cardPanel = JPanel(cardLayout)
-  val allContentPanel: JComponent = JPanel(GridLayout(1, 1))
+  val allContentPanel: JComponent = JPanel(CardLayout())
+  private val mainPanel = JPanel(GridBagLayout())


We were using the cardlayout to swap between the loading, login and onboarding components.

We can't put the webview in a CardLayout because JetBrains remote doesn't render it, so we are swapping the components.

But now there are only two components to swap: Loading and webview. So we don't need this panel anymore.

Let's just have allContentPanel with whatever trivial layout (GridLayout(1,1) etc.)

So this is weird, because I tried to simplify it at the start but it was not working without CardLayout.
But maybe it was error on my side, I will check again :)

dominiccooney · 2024-10-04T01:06:12Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+  }
+
+  @JsonRequest("secrets/store")
+  fun secrets_store(params: Secrets_StoreParams): CompletableFuture<Null?> {


A nullable null... Why not a nullable nullable null 🤔

dominiccooney · 2024-10-04T01:08:53Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+
+  @JsonNotification("window/didChangeContext")
+  fun window_didChangeContext(params: Window_DidChangeContextParams) {
+    println(params)


Let's not spam the console. Info logging it might be fine.

Is this, in fact, important for refreshing the status bar icon? "cody.activated" will be true when the extension thinks it is good to go.

Yeah, refreshing status icon bar is part I/ know I missed but I forget to add it at the end.
Thanks, I will add it now!

dominiccooney · 2024-10-04T01:12:11Z

src/main/kotlin/com/sourcegraph/cody/auth/depracated/DeprecatedCodyAccountManager.kt

@@ -0,0 +1,101 @@
+package com.sourcegraph.cody.auth.depracated


Spelling: deprecated

dominiccooney · 2024-10-04T01:19:39Z

src/main/kotlin/com/sourcegraph/cody/auth/depracated/DeprecatedCodyAccountManager.kt

+
+  @RequiresEdt
+  internal fun updateAccountToken(account: DeprecatedCodyAccount, newToken: String) {
+    service<DeprecatedCodyPersistentAccounts>().accounts = (getAccounts() - account) + account


This seems tricky, maybe the method should be called addOrUpdateAccountToken or something?

dominiccooney · 2024-10-04T01:21:36Z

src/main/kotlin/com/sourcegraph/cody/autocomplete/CodyAutocompleteManager.kt

@@ -172,7 +172,7 @@ class CodyAutocompleteManager {

    val textDocument: TextDocument = IntelliJTextDocument(editor, project)

-    if (isTriggeredExplicitly && CodyAuthenticationManager.getInstance().hasNoActiveAccount()) {
+    if (isTriggeredExplicitly && !CodyAccount.hasActiveAccount()) {
      HintManager.getInstance().showErrorHint(editor, "Cody: Sign in to use autocomplete")


Ugh, we should link to the sign in panel here! Maybe add a TODO?

Actually I think we should just show it.
I will do the changes, thanks!

dominiccooney · 2024-10-04T01:26:40Z

src/main/kotlin/com/sourcegraph/cody/chat/actions/SmellCommand.kt

@@ -1,8 +0,0 @@
-package com.sourcegraph.cody.chat.actions


This too, I believe we should keep these commands.

dominiccooney · 2024-10-04T01:27:23Z

src/main/kotlin/com/sourcegraph/cody/chat/actions/NewChatAction.kt

@@ -12,8 +12,7 @@ class NewChatAction : DumbAwareEDTAction() {
  }

  override fun update(event: AnActionEvent) {
-    val hasActiveAccount = CodyAuthenticationManager.getInstance().hasActiveAccount()
-    event.presentation.isEnabled = hasActiveAccount
+    event.presentation.isEnabled = CodyAccount.hasActiveAccount()


This CodyAccount.hasActiveAccount, we should be using the flag we got from window/didChangeContext "cody.activated" instead.

I agree in principle, but it is not as simple.
I need to also know account, in particular to be able to check if it's enterprise/pro/free, which in turn I need to properly render LLM selection in edit window.
But now I think maybe we should add additional flag to the model, and avoid this problem in the first place.
It would allow us to remove quite a bit of code on the jetbrains side.

OK. Let's work out a practical split to get this landed, because there's a lot of great cleanup here. And we can smooth out the details later.

I agree for chat/models returning the models with the flag sounds good.

I tend to disagree in the specific case of NewChatAction, all we need to know is whether we have an active account. But let's land this first and take a fresh look at account type dependencies after this change is done.

dominiccooney · 2024-10-04T01:29:33Z

src/main/kotlin/com/sourcegraph/cody/config/migration/SettingsMigration.kt

    RunOnceUtil.runOnceForProject(project, "CodyMigrateChatHistory-v2") {
      ChatHistoryMigration.migrate(project)
    }
+    RunOnceUtil.runOnceForProject(project, "AccountsToCodyMigration") {
+      AccountsMigration.migrate()


Just curious but are these guaranteed to run in this order? Is it important?

AFAIR their are, this code is synchronous.

dominiccooney

Feedback inline, please take a look.

Approving to unblock since you have multiple sides to land.

We can go further than this, I think, by listening to window/didChangeContext and hooking a notification to the AuthStatus observer, but we can work on those in follow up patches if it is easier.

dominiccooney · 2024-10-04T01:54:14Z

src/main/kotlin/com/sourcegraph/cody/agent/CodyAgentClient.kt

+    println(params)
+  }
+
+  @JsonNotification("globalStorage/didChange")


Let's not sniff global storage for this. We should be listening to the configuration observable and pushing a semantic, application-level message to the IDE.

src/main/java/com/sourcegraph/cody/CodyToolWindowFactory.java

mkondratek · 2024-10-17T10:42:56Z

src/main/kotlin/com/sourcegraph/cody/ui/web/WebUIHost.kt

-    if ((command == "auth" && decodedJson.get("authKind")?.asString == "signout") ||
-        (isCommand && id == "cody.auth.signout")) {
-      CodyAuthenticationManager.getInstance().setActiveAccount(null)
-    } else if (isCommand && id == "cody.auth.switchAccount") {


cody.auth.switchAccount - we do not handle this command now 😕

Because it shouldn't handled by JetBrains anymore.
JetBrains is now only responsible for storing the tokens in the secure store.
Other than that all account management is done by Cody and we just receive notifications about it.

mkondratek · 2024-10-17T10:44:10Z

src/main/kotlin/com/sourcegraph/cody/listeners/CodyDocumentListener.kt

 import com.sourcegraph.cody.telemetry.TelemetryV2
 import com.sourcegraph.cody.vscode.InlineCompletionTriggerKind
 import com.sourcegraph.utils.CodyEditorUtil

 class CodyDocumentListener(val project: Project) : BulkAwareDocumentListener {

+  // TODO: Looks like this functionality is broken after the migration to webview


is it still broken or did you manage fix it? if it need more attention can you pls create an issue for that? 🙏

It's still broken, fix will be a bit more involved, I linked an linear issue

@dominiccooney

## Changes I've added `cody.serverEndpoint` context variable, which on the clients side is received through `window/didChangeContext` notification. We need it on the jetbrains side in few places, e.g. when we are checking if current account is a dotcom account. I think it might be better idea to replace this with a dedicated endpoint, but I'm not sure if `cody.activated` is currently used anywhere? @dominiccooney @sqs WDYT? ## Test plan Full QA with sourcegraph/jetbrains#2382 is needed. It does not change anything on the Cody side.

mkondratek · 2024-11-25T12:58:32Z

🚀

## Changes That PR: 1. Fixes webview crashes when switching accounts with Account tab open 2. Add `Switch Account` button on the Accounts tab when `accountSwitchingInWebview` capability is enabled in client. ## Test plan Testing with jetbrains `main`: **Fixes https://linear.app/sourcegraph/issue/QA-150** 1. Open Accounts tab 3. Click Cody icon, then `Manage Accounts` 4. Switch the account 5. Account tab should re-render properly (after a 1-2s delay) showing new account details **Testing with jetbrains [migrated to webview account management](sourcegraph/jetbrains#2382 Account switching: 1. Open Accounts tab 2. Click Switch Account 3. Select account you want to switch to from the list 4. Account tab should re-render properly (after a 1-2s delay) showing new account details Signing out: 1. Open Accounts tab 2. Click Sign Out 3. You should be moved to the Sign In panel Account adding: 1. Open Accounts tab 2. Click 'Switch Account', and then 'Add another account' 3. Type url of your endpoint 4. You should be redirected to the web page to confirm adding the token 5. Account should be added and displayed as active Note: If wrong endpoint is provided web redirection will fail, but no error is displayed in the UI. This definitely can be improved. Account adding with token: 1. Open Accounts tab 2. Click 'Switch Account', and then 'Add another account' 3. Type url of your endpoint 4. Click 'Access Token (optional)' 5. Type incorrect access token and click 'Add and Switch' 6. You should get error saying `Invalid access token.` 7. Type correct access token and click 'Add and Switch' 9. Account should be added and displayed as active ![image](https://github.com/user-attachments/assets/cbd9e85d-8075-45ec-a4ba-2b3a68c07fdb) ![image](https://github.com/user-attachments/assets/10b6d94f-c15d-4fe3-ae70-5b968bb8d251) ![image](https://github.com/user-attachments/assets/0d617e8b-0cc4-48d2-98b7-8e5d548f3f6c) ![image](https://github.com/user-attachments/assets/0ad531f2-5880-4d7a-a66d-31abc2112a79)

pkukielka changed the title ~~Switch to webview-based sign-iin flow~~ Switch to webview-based sign-in flow Oct 3, 2024

pkukielka marked this pull request as draft October 3, 2024 15:02

pkukielka mentioned this pull request Oct 3, 2024

Add globalStorage/didChange agent endpoint sourcegraph/cody#5795

Closed

mkondratek reviewed Oct 3, 2024

View reviewed changes

dominiccooney self-requested a review October 4, 2024 00:41

dominiccooney reviewed Oct 4, 2024

View reviewed changes

dominiccooney approved these changes Oct 4, 2024

View reviewed changes

dominiccooney reviewed Oct 4, 2024

View reviewed changes

mkondratek mentioned this pull request Oct 7, 2024

JetBrains: User doesn't signout from cody even after removing all the account #2346

Closed

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch 2 times, most recently from 7a32f34 to a856cfd Compare October 16, 2024 14:34

pkukielka mentioned this pull request Oct 16, 2024

Add setContext notification for cody.serverEndpoint sourcegraph/cody#5918

Merged

pkukielka marked this pull request as ready for review October 16, 2024 15:15

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch 2 times, most recently from 1aaddb9 to 3a8922f Compare October 17, 2024 09:02

mkondratek reviewed Oct 17, 2024

View reviewed changes

src/main/java/com/sourcegraph/cody/CodyToolWindowFactory.java Show resolved Hide resolved

mkondratek reviewed Oct 17, 2024

View reviewed changes

mkondratek approved these changes Oct 17, 2024

View reviewed changes

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch from 1c108e3 to 4110d2a Compare November 20, 2024 10:09

pkukielka mentioned this pull request Nov 20, 2024

Add account switcher component in the Accounts webview tab sourcegraph/cody#6159

Merged

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch from 3f3603d to 5b24246 Compare November 25, 2024 12:50

mkondratek approved these changes Nov 25, 2024

View reviewed changes

pkukielka added 3 commits November 25, 2024 15:18

Switch to webview-based sign-in flow

2c0d2bf

Enable 'accountSwitchingInWebview' capability

6d08cad

Bump cody commit

39e8bce

pkukielka force-pushed the pkukielka/switch-to-webview-signin-panel branch from 5b24246 to 39e8bce Compare November 25, 2024 14:19

pkukielka merged commit e07fe78 into main Nov 25, 2024
6 of 7 checks passed

pkukielka deleted the pkukielka/switch-to-webview-signin-panel branch November 25, 2024 16:25

		@@ -0,0 +1,101 @@
		package com.sourcegraph.cody.auth.depracated

Switch to webview-based sign-in flow #2382

Switch to webview-based sign-in flow #2382

Conversation

pkukielka commented Oct 3, 2024 • edited Loading

Test plan

mkondratek left a comment

Choose a reason for hiding this comment

mkondratek Oct 3, 2024 • edited by pkukielka Loading

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

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

Choose a reason for hiding this comment

dominiccooney Oct 4, 2024 • edited Loading

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

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

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

mkondratek commented Nov 25, 2024

pkukielka commented Oct 3, 2024 •

edited

Loading

mkondratek Oct 3, 2024 •

edited by pkukielka

Loading

dominiccooney Oct 4, 2024 •

edited

Loading