-
Notifications
You must be signed in to change notification settings - Fork 885
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
Sign out of GitHub Copilot #25014
Sign out of GitHub Copilot #25014
Conversation
Pull Request Test Coverage Report for Build 7147208485Warning: This coverage report may be inaccurate.We've detected an issue with your CI configuration that might affect the accuracy of this pull request's coverage report.
💛 - Coveralls |
src/sql/workbench/services/accountManagement/browser/accountListRenderer.ts
Show resolved
Hide resolved
src/sql/workbench/services/accountManagement/browser/accountDialog.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/accountManagement/browser/accountDialog.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/accountManagement/browser/accountDialog.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/accountManagement/browser/accountDialog.ts
Outdated
Show resolved
Hide resolved
src/sql/workbench/services/accountManagement/browser/accountDialog.ts
Outdated
Show resolved
Hide resolved
@IAccountManagementService private _accountManagementService: IAccountManagementService, | ||
@ILogService private _logService: ILogService, | ||
@IAuthenticationService private readonly _authenticationService: IAuthenticationService | ||
) { | ||
// Create our event emitters | ||
this._addProviderEmitter = new Emitter<AccountProviderAddedEventParams>(); |
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.
These emitters are likely undisposed..
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.
Good catch, although I'd suggest going through and fixing those as a follow up PR to avoid cluttering up this PR even more
src/sql/workbench/services/accountManagement/browser/accountListRenderer.ts
Show resolved
Hide resolved
const providerId = this._account.key.providerId; | ||
const allSessions = await this.authenticationService.getSessions(providerId); | ||
const sessionsForAccount = allSessions.filter(s => s.account.label === this._account.displayInfo.userId); | ||
await this.authenticationService.removeAccountSessions(providerId, this._account.displayInfo.userId, sessionsForAccount); |
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.
Successfully signed out popup doesn't go away and requires user interaction, please make sure it goes away in 5-10 seconds.
const providerId = this._account.key.providerId; | ||
const allSessions = await this.authenticationService.getSessions(providerId); | ||
const sessionsForAccount = allSessions.filter(s => s.account.label === this._account.displayInfo.userId); | ||
await this.authenticationService.removeAccountSessions(providerId, this._account.displayInfo.userId, sessionsForAccount); |
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.
maybe ADS should be restarted to complete the sign out process to make sure the copilot extension sign in notification is triggered?
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.
Some tests would be fantastic as well - at least some focused on ensuring that the view model is updated correctly when accounts/sessions are added/removed.
@IAccountManagementService private _accountManagementService: IAccountManagementService, | ||
@ILogService private _logService: ILogService, | ||
@IAuthenticationService private readonly _authenticationService: IAuthenticationService | ||
) { | ||
// Create our event emitters | ||
this._addProviderEmitter = new Emitter<AccountProviderAddedEventParams>(); |
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.
Good catch, although I'd suggest going through and fixing those as a follow up PR to avoid cluttering up this PR even more
src/sql/workbench/services/accountManagement/browser/accountManagementService.ts
Show resolved
Hide resolved
src/sql/workbench/services/accountManagement/browser/accountManagementService.ts
Show resolved
Hide resolved
src/vs/workbench/services/authentication/browser/authenticationService.ts
Outdated
Show resolved
Hide resolved
* Sign out of GitHub Copilot * Code review changes * Remove unnecessary loop in action * Replace forEach call with map * Add hook to remove account after sign out * Revert "Add hook to remove account after sign out" This reverts commit 46b5f3c. * Revert "Revert "Add hook to remove account after sign out"" This reverts commit 5a401fa. * Comment out faulty code * Return empty array * Display provider name correctly * Add github logo for github accounts * Function argument rename * Remove account on sign out confirmation * Add missing semicolon * Code review changes * Implements mock auth service * Fix failing unit tests * Replaces callback hook with event handler * Remove updateAccountList hook
This PR closes #24478
The PR adds functionality to sign out of GitHub Copilot in Azure Data Studio.
The accounts section in ADS will show any active GitHub Copilot sessions that can be signed out of:
![image](https://private-user-images.githubusercontent.com/87730006/284683737-d3701699-eaa9-42a9-bc71-acfc55295e4e.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjI3MzA1NzIsIm5iZiI6MTcyMjczMDI3MiwicGF0aCI6Ii84NzczMDAwNi8yODQ2ODM3MzctZDM3MDE2OTktZWFhOS00MmE5LWJjNzEtYWNmYzU1Mjk1ZTRlLnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MDQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODA0VDAwMTExMlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTUxNzAwYTVjODBiMWY0MzQzOTU3N2M5YzZkZjY5MDAyZjc3ODYyNmRmNjZiOTM0YTBkYmIxYzEyYTNiYjA2YWUmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.Oe8fNZ1TTRaLCd-RG7Un8cVx7ScXPO8f0O4FjJnttVc)
Clicking on the 'X' will then sign the user out and GitHub Copilot will then show a dialog informing the user to log in to the GitHub Copilot extension:
![image](https://private-user-images.githubusercontent.com/87730006/284683041-8485146b-66a9-49be-9140-9a9d9cd02f39.png?jwt=eyJhbGciOiJIUzI1NiIsInR5cCI6IkpXVCJ9.eyJpc3MiOiJnaXRodWIuY29tIiwiYXVkIjoicmF3LmdpdGh1YnVzZXJjb250ZW50LmNvbSIsImtleSI6ImtleTUiLCJleHAiOjE3MjI3MzA1NzIsIm5iZiI6MTcyMjczMDI3MiwicGF0aCI6Ii84NzczMDAwNi8yODQ2ODMwNDEtODQ4NTE0NmItNjZhOS00OWJlLTkxNDAtOWE5ZDljZDAyZjM5LnBuZz9YLUFtei1BbGdvcml0aG09QVdTNC1ITUFDLVNIQTI1NiZYLUFtei1DcmVkZW50aWFsPUFLSUFWQ09EWUxTQTUzUFFLNFpBJTJGMjAyNDA4MDQlMkZ1cy1lYXN0LTElMkZzMyUyRmF3czRfcmVxdWVzdCZYLUFtei1EYXRlPTIwMjQwODA0VDAwMTExMlomWC1BbXotRXhwaXJlcz0zMDAmWC1BbXotU2lnbmF0dXJlPTQ5MGRhN2E2NGE1M2QzNjQyYjI0NDYyYjA3MTllNDEyYzM5MjcyNzQ1MGU0MTY1MmJhOTJiYzQ1OGIxODEyZjEmWC1BbXotU2lnbmVkSGVhZGVycz1ob3N0JmFjdG9yX2lkPTAma2V5X2lkPTAmcmVwb19pZD0wIn0.891cqbkJnoi-y7a8Z7S7Gsp3zIlkl7wwnhczKnIkfUg)