-
Notifications
You must be signed in to change notification settings - Fork 294
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
Remove auth steps #3244
Remove auth steps #3244
Conversation
@taylorsperry @chenkc805 In terms of logging, this is not using our regular experiment setup (because we need to be able to track assignment even when the user is not signed in yet). So instead this reuses the setup that @dominiccooney wrote for the simplified onboarding experiments. It will log a V1 event with the name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left a comment and question inline. Would be nice to have am e2e test cover the redirect machinic (if possible, doesn't have to be in this PR).
Tried to login via GitHub and also login as enterprise with redirect, works magically on both!
I did notice the CodyVSCodeExtension:Auth:connected
was logged in twice though, is that expected?
Ah good question I guess you see both the redirect and the token post logging that. I will see if we can fix this somehow 🙇 Thanks for the review! |
I think it's reasonable for this event to be logged twice I did add a specific event for when the token receiver server receives the token, though. This way we know exactly how the client receives the token (if the user follows the redirect AND posts to the webserver, we log two events but this seems reasonable?) We currently have no event though when a new auth provider is added the first time (this seems unrelated to this PR though) |
Closes #3216 As part of the [remove authentication steps](https://docs.google.com/document/d/1AubJ226dUhaV0zC8WpA_19vzoXXtQ8AUmNqSHPaWKWQ/edit#heading=h.ftqde5tlfi9g) PRD we want to simplify the login chain a bit. Right now, when the VS Code Cody client starts an auth processes, it will not know about the newly generated auth token until the user navigates back to VS Code. In user testing, this proofed to be confusing an some users might tab away before going through with the final redirect. We are working on an experiment that simplifies this. The trick? When an auth process is started, start a local http server (listening on a free port on `127.0.0.1`) and forward that to the web browser. Then, when a token is created, we can `POST` the new token to that endpoint in the hope that the VS Code client. ## Test plan https://github.com/sourcegraph/cody/assets/458591/f2ec6977-7859-4bda-be46-8d9ba2704a67 - Opt into the experiment by setting `"cody.testing.removeAuthenticationStep": true,` - Run a local version of Cody with this PR (The accompanying dotcom change is already rollout so no other change is necessary) - In the dev build, click one of the big sign in buttons - Notice that you do not have to wait for the redirect-back anymore
Closes #3216
As part of the remove authentication steps PRD we want to simplify the login chain a bit.
Right now, when the VS Code Cody client starts an auth processes, it will not know about the newly generated auth token until the user navigates back to VS Code. In user testing, this proofed to be confusing an some users might tab away before going through with the final redirect.
We are working on an experiment that simplifies this. The trick? When an auth process is started, start a local http server (listening on a free port on
127.0.0.1
) and forward that to the web browser. Then, when a token is created, we canPOST
the new token to that endpoint in the hope that the VS Code client.Test plan
Screen.Recording.2024-03-04.at.15.26.20.mov
"cody.testing.removeAuthenticationStep": true,