From 934ef04eb06cae12c16121ec5b17fa79bd3d3926 Mon Sep 17 00:00:00 2001 From: Eric Amodio Date: Fri, 4 Dec 2020 04:10:34 -0500 Subject: [PATCH] Fixes #1208 - refines connect to remote flow Adds a deny everywhere option Adds command to reset denials --- CHANGELOG.md | 1 + package.json | 9 ++ src/commands.ts | 1 + src/commands/common.ts | 1 + src/commands/resetRemoteConnectionAuthorization.ts | 17 ++++ src/constants.ts | 10 +- src/extension.ts | 31 +++--- src/git/gitService.ts | 23 ++++- src/git/models/repository.ts | 12 ++- src/git/remotes/provider.ts | 104 +++++++++++++-------- src/quickpicks/quickPicksItems.ts | 1 + 11 files changed, 149 insertions(+), 61 deletions(-) create mode 100644 src/commands/resetRemoteConnectionAuthorization.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index 484ba45..12ba64c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,7 @@ The format is based on [Keep a Changelog](http://keepachangelog.com/) and this p ### Fixed +- Fixes [#1208](https://github.com/eamodio/vscode-gitlens/issues/1208) - Connect to Github notification is noisy - Fixes [#526](https://github.com/eamodio/vscode-gitlens/issues/526) - FAILED in gitlens.outputLevel=verbose; likely due to regex not in quotes - Fixes [#1222](https://github.com/eamodio/vscode-gitlens/issues/1222) - GitLens: Open Associated Pull Request doesn't work - Fixes [#1223](https://github.com/eamodio/vscode-gitlens/issues/1223) - commit pane, ${tips} does not show tags diff --git a/package.json b/package.json index 0371ef4..25fb90d 100644 --- a/package.json +++ b/package.json @@ -4409,6 +4409,11 @@ "command": "gitlens.views.tags.setShowAvatarsOff", "title": "Hide Avatars", "category": "GitLens" + }, + { + "command": "gitlens.resetRemoteConnectionAuthorization", + "title": "Reset Remote Connection Authorization", + "category": "GitLens" } ], "menus": { @@ -5596,6 +5601,10 @@ { "command": "gitlens.views.tags.setShowAvatarsOff", "when": "false" + }, + { + "command": "gitlens.resetRemoteConnectionAuthorization", + "when": "gitlens:hasRichRemotes" } ], "editor/context": [ diff --git a/src/commands.ts b/src/commands.ts index 290b141..8742be3 100644 --- a/src/commands.ts +++ b/src/commands.ts @@ -39,6 +39,7 @@ export * from './commands/rebaseEditor'; export * from './commands/refreshHover'; export * from './commands/remoteProviders'; export * from './commands/repositories'; +export * from './commands/resetRemoteConnectionAuthorization'; export * from './commands/resetSuppressedWarnings'; export * from './commands/setViewsLayout'; export * from './commands/searchCommits'; diff --git a/src/commands/common.ts b/src/commands/common.ts index 2cdad75..6d7913f 100644 --- a/src/commands/common.ts +++ b/src/commands/common.ts @@ -92,6 +92,7 @@ export enum Commands { PushRepositories = 'gitlens.pushRepositories', GitCommands = 'gitlens.gitCommands', RefreshHover = 'gitlens.refreshHover', + ResetRemoteConnectionAuthorization = 'gitlens.resetRemoteConnectionAuthorization', ResetSuppressedWarnings = 'gitlens.resetSuppressedWarnings', RevealCommitInView = 'gitlens.revealCommitInView', SearchCommits = 'gitlens.showCommitSearch', diff --git a/src/commands/resetRemoteConnectionAuthorization.ts b/src/commands/resetRemoteConnectionAuthorization.ts new file mode 100644 index 0000000..e8217e4 --- /dev/null +++ b/src/commands/resetRemoteConnectionAuthorization.ts @@ -0,0 +1,17 @@ +'use strict'; +import { Container } from '../container'; +import { command, Command, Commands } from './common'; + +@command() +export class ResetRemoteConnectionAuthorizationCommand extends Command { + constructor() { + super(Commands.ResetRemoteConnectionAuthorization); + } + + async execute() { + for (const repo of await Container.git.getRepositories()) { + const remote = await Container.git.getRichRemoteProvider(repo.path, { includeDisconnected: true }); + await remote?.provider?.resetRemoteConnectionAuthorization(); + } + } +} diff --git a/src/constants.ts b/src/constants.ts index 4a46be1..3945409 100644 --- a/src/constants.ts +++ b/src/constants.ts @@ -31,8 +31,9 @@ export enum ContextKeys { DisabledToggleCodeLens = 'gitlens:disabledToggleCodeLens', Disabled = 'gitlens:disabled', Enabled = 'gitlens:enabled', - HasRemotes = 'gitlens:hasRemotes', HasConnectedRemotes = 'gitlens:hasConnectedRemotes', + HasRemotes = 'gitlens:hasRemotes', + HasRichRemotes = 'gitlens:hasRichRemotes', Key = 'gitlens:key', Readonly = 'gitlens:readonly', ViewsCanCompare = 'gitlens:views:canCompare', @@ -129,15 +130,17 @@ export enum GlyphChars { } export enum SyncedState { - Version = 'gitlens:synced:version', + DisallowConnectionPrefix = 'gitlens:disallow:connection:', UpdatesViewVisible = 'gitlens:views:updates:visible', + Version = 'gitlens:synced:version', WelcomeViewVisible = 'gitlens:views:welcome:visible', } export enum GlobalState { - DeprecatedVersion = 'gitlensVersion', Avatars = 'gitlens:avatars', Version = 'gitlens:version', + + Deprecated_Version = 'gitlensVersion', } export const ImageMimetypes: Record = { @@ -208,6 +211,7 @@ export interface StarredRepositories { export enum WorkspaceState { BranchComparisons = 'gitlens:branch:comparisons', + ConnectedPrefix = 'gitlens:connected:', DefaultRemote = 'gitlens:remote:default', DisallowConnectionPrefix = 'gitlens:disallow:connection:', StarredBranches = 'gitlens:starred:branches', diff --git a/src/extension.ts b/src/extension.ts index 5144394..395bb10 100644 --- a/src/extension.ts +++ b/src/extension.ts @@ -13,9 +13,13 @@ import { Messages } from './messages'; import { Strings, Versions } from './system'; import { ViewNode } from './views/nodes'; +let _context: ExtensionContext | undefined; + export async function activate(context: ExtensionContext) { const start = process.hrtime(); + _context = context; + let extensionId = 'eamodio.gitlens'; if (paths.basename(context.globalStorageUri.fsPath) === 'eamodio.gitlens-insiders') { extensionId = 'eamodio.gitlens-insiders'; @@ -34,11 +38,7 @@ export async function activate(context: ExtensionContext) { // Pretend we are enabled (until we know otherwise) and set the view contexts to reduce flashing on load void setContext(ContextKeys.Enabled, true); - context.globalState.setKeysForSync([ - SyncedState.Version, - SyncedState.UpdatesViewVisible, - SyncedState.WelcomeViewVisible, - ]); + setKeysForSync(); Logger.configure(context, configuration.get('outputLevel'), o => { if (GitUri.is(o)) { @@ -64,7 +64,7 @@ export async function activate(context: ExtensionContext) { const syncedVersion = context.globalState.get(SyncedState.Version); const previousVersion = context.globalState.get(GlobalState.Version) ?? - context.globalState.get(GlobalState.DeprecatedVersion) ?? + context.globalState.get(GlobalState.Deprecated_Version) ?? syncedVersion; if (Logger.level === TraceLevel.Debug || Logger.isDebugging) { @@ -157,10 +157,6 @@ export function deactivate() { // nothing to do } -export async function setEnabled(enabled: boolean): Promise { - await Promise.all([setContext(ContextKeys.Enabled, enabled), setContext(ContextKeys.Disabled, !enabled)]); -} - // async function migrateSettings(context: ExtensionContext, previousVersion: string | undefined) { // if (previousVersion === undefined) return; @@ -174,7 +170,20 @@ export async function setEnabled(enabled: boolean): Promise { // } // } -function notifyOnUnsupportedGitVersion(version: string) { +export async function setEnabled(enabled: boolean): Promise { + await Promise.all([setContext(ContextKeys.Enabled, enabled), setContext(ContextKeys.Disabled, !enabled)]); +} + +export function setKeysForSync(...keys: (SyncedState | string)[]) { + return _context?.globalState.setKeysForSync([ + ...keys, + SyncedState.UpdatesViewVisible, + SyncedState.Version, + SyncedState.WelcomeViewVisible, + ]); +} + +export function notifyOnUnsupportedGitVersion(version: string) { if (GitService.compareGitVersion('2.7.2') !== -1) return; // If git is less than v2.7.2 diff --git a/src/git/gitService.ts b/src/git/gitService.ts index 3cd1e1b..7c26aee 100644 --- a/src/git/gitService.ts +++ b/src/git/gitService.ts @@ -445,23 +445,36 @@ export class GitService implements Disposable { await setEnabled(hasRepository); let hasRemotes = false; + let hasRichRemotes = false; let hasConnectedRemotes = false; if (hasRepository) { for (const repo of repositoryTree.values()) { if (!hasConnectedRemotes) { - hasConnectedRemotes = await repo.hasConnectedRemotes(); + hasConnectedRemotes = await repo.hasConnectedRemote(); + + if (hasConnectedRemotes) { + hasRichRemotes = true; + hasRemotes = true; + } + } + + if (!hasRichRemotes) { + hasRichRemotes = await repo.hasRichRemote(); } if (!hasRemotes) { - hasRemotes = hasConnectedRemotes || (await repo.hasRemotes()); + hasRemotes = await repo.hasRemotes(); } - if (hasRemotes && hasConnectedRemotes) break; + if (hasRemotes && hasRichRemotes && hasConnectedRemotes) break; } } - await setContext(ContextKeys.HasRemotes, hasRemotes); - await setContext(ContextKeys.HasConnectedRemotes, hasConnectedRemotes); + await Promise.all([ + setContext(ContextKeys.HasRemotes, hasRemotes), + setContext(ContextKeys.HasRichRemotes, hasRichRemotes), + setContext(ContextKeys.HasConnectedRemotes, hasConnectedRemotes), + ]); // If we have no repositories setup a watcher in case one is initialized if (!hasRepository) { diff --git a/src/git/models/repository.ts b/src/git/models/repository.ts index 430e7f3..7a2f885 100644 --- a/src/git/models/repository.ts +++ b/src/git/models/repository.ts @@ -485,14 +485,20 @@ export class Repository implements Disposable { return Container.git.getTags(this.path, options); } + async hasConnectedRemote(): Promise { + const remote = await Container.git.getRichRemoteProvider(await this.getRemotes()); + return remote?.provider != null; + } + async hasRemotes(): Promise { const remotes = await this.getRemotes(); return remotes?.length > 0; } - async hasConnectedRemotes(): Promise { - const remotes = await this.getRemotes(); - const remote = await Container.git.getRichRemoteProvider(remotes); + async hasRichRemote(): Promise { + const remote = await Container.git.getRichRemoteProvider(await this.getRemotes(), { + includeDisconnected: true, + }); return remote?.provider != null; } diff --git a/src/git/remotes/provider.ts b/src/git/remotes/provider.ts index 4a46fba..5324e02 100644 --- a/src/git/remotes/provider.ts +++ b/src/git/remotes/provider.ts @@ -13,8 +13,9 @@ import { } from 'vscode'; import { DynamicAutolinkReference } from '../../annotations/autolinks'; import { AutolinkReference } from '../../config'; -import { WorkspaceState } from '../../constants'; +import { SyncedState, WorkspaceState } from '../../constants'; import { Container } from '../../container'; +import { setKeysForSync } from '../../extension'; import { Logger } from '../../logger'; import { Messages } from '../../messages'; import { Account, GitLogCommit, IssueOrPullRequest, PullRequest, PullRequestState, Repository } from '../models/models'; @@ -287,7 +288,15 @@ export abstract class RichRemoteProvider extends RemoteProvider { return this.custom ? `${this.name}:${this.domain}` : this.name; } - private get disallowConnectionKey() { + private get connectedKey() { + return `${WorkspaceState.ConnectedPrefix}${this.key}`; + } + + private get disallowSyncedGlobalConnectionKey() { + return `${SyncedState.DisallowConnectionPrefix}${this.key}`; + } + + private get disallowWorkspaceConnectionKey() { return `${WorkspaceState.DisallowConnectionPrefix}${this.key}`; } @@ -330,7 +339,8 @@ export abstract class RichRemoteProvider extends RemoteProvider { this._session = null; if (disconnected) { - void Container.context.workspaceState.update(this.disallowConnectionKey, true); + void Container.context.workspaceState.update(this.connectedKey, undefined); + void Container.context.workspaceState.update(this.disallowWorkspaceConnectionKey, true); this._onDidChange.fire(); if (!silent) { @@ -339,6 +349,15 @@ export abstract class RichRemoteProvider extends RemoteProvider { } } + @log() + async resetRemoteConnectionAuthorization() { + await Promise.all([ + Container.context.workspaceState.update(this.connectedKey, undefined), + Container.context.workspaceState.update(this.disallowWorkspaceConnectionKey, undefined), + Container.context.globalState.update(this.disallowSyncedGlobalConnectionKey, undefined), + ]); + } + @gate() @debug({ exit: connected => `returned ${connected}`, @@ -525,26 +544,36 @@ export abstract class RichRemoteProvider extends RemoteProvider { ): Promise; @gate() - private async ensureSession(createIfNone: boolean): Promise { + private async ensureSession(createIfNeeded: boolean): Promise { if (this._session != null) return this._session; - if (createIfNone) { - await Container.context.workspaceState.update(this.disallowConnectionKey, undefined); - } else if (Container.context.workspaceState.get(this.disallowConnectionKey)) { + if (createIfNeeded) { + await Promise.all([ + Container.context.workspaceState.update(this.connectedKey, undefined), + Container.context.workspaceState.update(this.disallowWorkspaceConnectionKey, undefined), + ]); + } else if ( + !Container.context.workspaceState.get(this.connectedKey) && + (Container.context.globalState.get(this.disallowSyncedGlobalConnectionKey) || + Container.context.workspaceState.get(this.disallowWorkspaceConnectionKey)) + ) { return undefined; } let session; try { session = await authentication.getSession(this.authProvider.id, this.authProvider.scopes, { - createIfNone: createIfNone, + createIfNone: createIfNeeded, }); } catch (ex) { - await Container.context.workspaceState.update(this.disallowConnectionKey, true); + await Promise.all([ + Container.context.workspaceState.update(this.connectedKey, undefined), + Container.context.workspaceState.update(this.disallowWorkspaceConnectionKey, true), + ]); if (ex instanceof Error && ex.message.includes('User did not consent')) { - if (!createIfNone) { - void this.promptToRetyConnection(); + if (!createIfNeeded) { + void this.promptToAllow(true); } return undefined; } @@ -552,19 +581,25 @@ export abstract class RichRemoteProvider extends RemoteProvider { session = null; } - if (session === undefined && !createIfNone) { - await Container.context.workspaceState.update(this.disallowConnectionKey, true); + if (session === undefined && !createIfNeeded) { + await Promise.all([ + Container.context.workspaceState.update(this.connectedKey, undefined), + Container.context.workspaceState.update(this.disallowWorkspaceConnectionKey, true), + ]); - void this.promptToConnect(); + void this.promptToAllow(); } this._session = session ?? null; this.invalidClientExceptionCount = 0; if (session != null) { - await Container.context.workspaceState.update(this.disallowConnectionKey, undefined); + await Promise.all([ + Container.context.workspaceState.update(this.connectedKey, true), + Container.context.workspaceState.update(this.disallowWorkspaceConnectionKey, undefined), + ]); - if (createIfNone) { + if (createIfNeeded) { this._onDidChange.fire(); _onDidChangeAuthentication.fire({ reason: 'connected', key: this.key }); } @@ -574,34 +609,25 @@ export abstract class RichRemoteProvider extends RemoteProvider { } @gate() - private async promptToConnect() { - const connect: MessageItem = { title: `Connect to ${this.name}` }; - const cancel: MessageItem = { title: 'Cancel', isCloseAffordance: true }; + private async promptToAllow(retry: boolean = false) { + const allow: MessageItem = { title: 'Allow' }; + const deny: MessageItem = { title: 'Deny' }; + const denyEverywhere: MessageItem = { title: 'Deny Everywhere' }; const result = await window.showInformationMessage( - `GitLens would like to connect to ${this.name} to provide a rich integration with pull requests, issues, avatars, and more. If you choose to connect, you will be prompted to authenticate with ${this.name}.`, - connect, - cancel, - ); - - if (result === connect) { - void this.connect(); - } - } - - @gate() - private async promptToRetyConnection() { - const connect: MessageItem = { title: `Connect to ${this.name}` }; - const cancel: MessageItem = { title: 'Cancel', isCloseAffordance: true }; - - const result = await window.showErrorMessage( - `GitLens wants to connect to ${this.name} to provide a rich integration with pull requests, issues, avatars, and more. If you choose to connect, you will be prompted again to allow authentication with ${this.name}.`, - connect, - cancel, + retry + ? `By allowing GitLens to connect to ${this.name}, it can provide a rich integration with pull requests, issues, avatars, and more.\n\nIf you choose 'Allow', you will again be prompted to authenticate with ${this.name}.` + : `Allow GitLens to connect to ${this.name} to provide a rich integration with pull requests, issues, avatars, and more.\n\nIf you choose 'Allow', you will be prompted to authenticate with ${this.name}.`, + allow, + deny, + denyEverywhere, ); - if (result === connect) { + if (result === allow) { void this.connect(); + } else if (result === denyEverywhere) { + setKeysForSync(this.disallowSyncedGlobalConnectionKey); + await Container.context.globalState.update(this.disallowSyncedGlobalConnectionKey, true); } } diff --git a/src/quickpicks/quickPicksItems.ts b/src/quickpicks/quickPicksItems.ts index 6066ac7..0af4176 100644 --- a/src/quickpicks/quickPicksItems.ts +++ b/src/quickpicks/quickPicksItems.ts @@ -6,6 +6,7 @@ import { GitReference, GitRevisionReference, GitStashCommit, SearchPattern } fro import { Keys } from '../keyboard'; declare module 'vscode' { + // eslint-disable-next-line @typescript-eslint/no-unused-vars interface QuickPickItem { onDidSelect?(): void; onDidPressKey?(key: Keys): Promise;