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

Change GK account authentication to no longer use the VS Code authentication system #3524

Merged
merged 2 commits into from
Sep 11, 2024

Conversation

sergeibbb
Copy link
Member

@sergeibbb sergeibbb commented Sep 11, 2024

Currently we leverage the VS Code authentication system for our GK authentication, but this causes unnecessary prompts and limits our control over the experience.

We already implement both sides of the authentication process within GitLens, so we just need to avoid using VS Code as the middleman

Roadmap

  • stop registering AccountAuthenticationProvider in VSCode
  • fix get-or-create behavior and honoring createIfNeeded and silent flags.
  • probably stop implementing AuthenticationProvider ➡️ probably but in a follow-up
  • stop relying on authentication events, e.g. stop handling them and make sure that we loos nothing by doing that (or fix the cases that are broken) I've checked that don't relying on such events for GK authentication.

Testing notes

  • Be unauthenticated before the update:
    • general behavior
      • login
      • restart VS Code: you should keep your session
      • logout
      • login again
      • logout
      • restart VS Code: you should keep being unauthenticated.
    • integrations
      • you're able to connect-disconnect rich GitHub integration,
      • they survive restarting the application.
      • disconect GitHub, then go to the Account View, press "Integrations" button, come back to VS Code. Integrations become connected.
  • Be authenticated before the update
    • check the integrations behavior from the previous section
    • check the general behavior from the previous section
  • Test both "sign-in" and "sign-up" flows, when you're authenticated and unauthenticated in your browser

@eamodio eamodio added the feature New feature or request label Sep 6, 2024
@eamodio eamodio added this to the 15.5 milestone Sep 6, 2024
@sergeibbb
Copy link
Member

hi @eamodio

If I understand the task correctly it looks like we should:

  1. stop registering AccountAuthenticationProvider in VSCode
  2. probably stop implementing AuthenticationProvider because there is no need of that. (I'm not sure yet whether it's true or false).
  3. therefore stop relying on authentication events, e.g. stop handling them and make sure that we loos nothing by doing that (or fix the cases that are broken)

We already save sessions to the secret storage, so probably nothing new should appear that would be related to the session management.

Is this understanding correct in general?

@sergeibbb
Copy link
Member

sergeibbb commented Sep 11, 2024

Hi @eamodio

Here is updated version of the question above:

#1 - I've stop registering AccountAuthenticationProvider in VSCode
#3 - I've checked that don't relying on such events for GK authentication

So, now everything works as expected.

The rest question is #2: should I stop implementing AuthenticationProvider? Now it's left without changes and everything looks working.

And one more question will be in a comment related to the code: #3524 (comment)

cc @axosoft-ramint

@sergeibbb sergeibbb changed the title Change GK account authentication to no longer use the VS Code authentication system Change GK account authentication to no longer use the VS Code authentication system. Sep 11, 2024
@sergeibbb sergeibbb changed the title Change GK account authentication to no longer use the VS Code authentication system. Change GK account authentication to no longer use the VS Code authentication system Sep 11, 2024
scopes: string[],
createIfNeeded: boolean,
): Promise<AuthenticationSession | undefined> {
const session = (await this.getSessions(scopes))[0];
Copy link
Member Author

Choose a reason for hiding this comment

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

@axosoft-ramint @eamodio

I'm not sure that this is the right way of accessing the session: retrieving an array of matching sessions and taking the first one. Can you confirm or suggest a better way?

Copy link
Contributor

@axosoft-ramint axosoft-ramint left a comment

Choose a reason for hiding this comment

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

LGTM

@axosoft-ramint axosoft-ramint merged commit cf6766d into main Sep 11, 2024
2 checks passed
@axosoft-ramint axosoft-ramint deleted the chore/3524-gk-account-authentication branch September 11, 2024 23:44
@eamodio eamodio modified the milestones: 16.0, 15.4-patch Sep 12, 2024
@d13 d13 added pending-release Resolved but not yet released to the stable edition verified ✔ Verified labels Sep 12, 2024
@axosoft-ramint axosoft-ramint removed the pending-release Resolved but not yet released to the stable edition label Sep 12, 2024
@sergeibbb
Copy link
Member

sergeibbb commented Sep 13, 2024

Hi @eamodio @axosoft-ramint

I've created a follow up ticket: #3574
I suppose, it needs to be prioritized, milestonized, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature New feature or request verified ✔ Verified
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants