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

[WIP] feat/cody: Prevent Enterprise users from logging into dotcom/PLG (CODY-4295) #6182

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

julialeex
Copy link

@julialeex julialeex commented Nov 23, 2024

This PR does Create mechanism to kick PLG logins/signup for user with email domains that below to enterprise customers.

Problem: We want to prevent enterprise customers from logging into PLG/dotcom.

This PR checks whether the user is an enterprise customer using UserShouldUseEnterprise, and whether the user tries to login into PLG/dotom through isDotCom, and logs the enterprise user out of PLG logins.

Things this PR does:

  • Add a new field userShouldUseEnterprise: boolean.
  • If an enterprise user tries to sign in, auth validation flow will detect this and the sign in process would fail with an error message.
  • Sign out and print an error message if the user should use enterprise and is already logged into PLG.
  • Tests.

Test plan

Test with debugger

  1. An enterprise user gets logged out for PLG/dotcom logins.
  2. An enterprise user that is already logged into PLG/dotcom gets logged out.

E2E test
Added test test enterprise customers should get logged out for dotcomUrl in auth.test.ts.

Changelog

  • feat(cody): prevent PLG login methods for enterprise users

Copy link
Contributor

@jamesmcnamara jamesmcnamara left a comment

Choose a reason for hiding this comment

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

Yeah I definitely gave you some bad advice here :) I left some comments in line but basically, we should:

  • Add a direct call to check the client config singleton from the auth flow.
  • Show an error notification when the flag is false
  • Fix the tests so that we only set userShouldUseEnterprise to true in tests where we want it

lib/shared/src/sourcegraph-api/clientConfig.ts Outdated Show resolved Hide resolved
Comment on lines 556 to 558
if (clientConfig?.userShouldUseEnterprise && isDotCom(authStatus.endpoint)) {
signOut(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.

I take your point on this now that I've seen it in action, and I think I gave you bad advice. The flash of the logged in screen before we're logged out looks janky and this code path is hard to follow for the next developer.

I think you're right that we should directly refresh this client config as part of the auth validation flow and stop the login with an error notification. I expect you'll need to add some methods to ClientConfigSingleton to achieve this.

Copy link
Author

@julialeex julialeex Nov 26, 2024

Choose a reason for hiding this comment

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

  1. To summarize, we want to add the logic to the auth validation flow, before we log in (like how we were checking userInfo in the auth validation flow before)?

The flash of the logged in screen before we're logged out looks janky

  1. I thought we have to log in first because of this comment on the ticket

Copy link
Contributor

Choose a reason for hiding this comment

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

  1. Yes exactly.
  2. I think that's more trying to capture that if a user is already logged in, and we refresh the client config and they are now supposed to be on an enterprise account, we should log them out. Which is a good point that that is another TODO.

Copy link
Author

Choose a reason for hiding this comment

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

Got it, thanks for the clarification! Will update the code

Copy link
Author

Choose a reason for hiding this comment

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

  1. I read the code for a bit more yesterday, and found that clientConfig is not populated until the user is authenticated (see code1 and code2)
  2. Because of what I found in 1), I double checked the feature requirements in the ticket and it turns out we do want to log in first and then log the user out. If this is true, I have a TODO of adding the error message after we log the user out

Copy link
Contributor

Choose a reason for hiding this comment

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

To me this sounds like a bad user experience. Sure the code currently expects us to be authenticated, but the only requirement an authentication token, which by that point in the login process we will have. Thus you could either:

  1. modify the client config class to have the ability to fetch the config on demand given a token, or
  2. complete the authentication flow as today, but stop the UI from actually displaying the logged in view until we've checked that value from client config and logging out if we need to (all from the auth flow so that the code is relatively linear)

I'll clarify on the ticket whether this behavior is expected

vscode/test/e2e/auth.test.ts Outdated Show resolved Hide resolved
julialeex and others added 3 commits November 25, 2024 16:37
Co-authored-by: James McNamara <jamesscottmcnamara@gmail.com>
Co-authored-by: James McNamara <jamesscottmcnamara@gmail.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