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

fix/accounts: Do not prefill the dotcom URL in the Enterprise login field #6418

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dominiccooney
Copy link
Contributor

@dominiccooney dominiccooney commented Dec 19, 2024

When you last logged into dot com, we pre-fill the enterprise login URL with https://sourcegraph.com. This is contrary to our efforts to get Enterprise users to log into the right endpoint.

You can still log in to dot com using the handy enterprise login method/hack. We just won't prefill that URL.

Fixes CODY-4534.

Test plan

  1. Sign out of all your accounts.
  2. Sign in to dotcom. This works with social login buttons or signing into https://sourcegraph.com as an "Enterprise" URL.
  3. Go to https://sourcegraph.com/user/settings/tokens and delete the token you are using.
  4. Reload VSCode, or wait for it to log you out.
  5. Go to sign in to an enterprise account. Verify that the URL field has a placeholder like https://instance. and NOT https://sourcegraph.com
  6. Sign in to s2.
  7. Go to https://sourcegraph.sourcegraph.com/user/settings/tokens and delete the token you are using.
  8. Reload VSCode, or wait for it to log you out.
  9. Go to sign in to an enterprise account. Verify that the URL field is prefilled with https://sourcegraph.sourcegraph.com
  10. Run a new instance of VSCode with --user-data-dir=/tmp/foo --profile-temp to simulate a fresh install.
  11. Go to sign in to an enterprise account. Verify that the URL field has a placeholder like https://instance. and NOT https://sourcegraph.com

Note, if you sign out (cmd-shift-p Cody: Sign Out) etc. then the field is not prefilled, we delete the record of that account. It is new installs, or when accounts are left over from expired tokens, that you get the prefilling.

Note: This is not easily e2e testable because Cody Web has fixed the dotcom override URL in the vite config, which is a compile-time static. To make this testable we would need to configure the dotcom override URL as the webview boots up. This is a big job because DOTCOM_URL is a module-level constant, etc.

@dominiccooney dominiccooney requested a review from a team December 19, 2024 04:06
@@ -280,7 +285,7 @@ const ClientSignInForm: React.FC<ClientSignInFormProps> = memo(
isSubmitting: false,
showAuthError: false,
formData: {
endpoint: authStatus?.endpoint ?? '',
endpoint: authStatus && !isDotCom(authStatus) ? authStatus.endpoint : '',
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
endpoint: authStatus && !isDotCom(authStatus) ? authStatus.endpoint : '',
endpoint: authStatus && !isDotCom(authStatus) ? authStatus.endpoint : '',

If the user has just signed out of dotcom or have never signed in before, would the endpoint in their authstatus set to DotCom by default?

maybe we could also check if the user's available endpoints (endpoint history) contain dotcom or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If the user has just signed out of dotcom or have never signed in before, would the endpoint in their authstatus set to DotCom by default?

These two cases might be different, but in the completely new setup case we do default the endpoint to dotcom. Today we display https://sourcegraph.com and this is effective at suppressing it as a default. I have updated the test plan to cover the "new install" case (which I'm simulating with a new user data dir & profile temp.)

maybe we could also check if the user's available endpoints (endpoint history) contain dotcom or not?

Yeah, great idea. Let's do it in a follow up.

@dominiccooney dominiccooney enabled auto-merge (squash) December 19, 2024 23:49
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