Skip to content

Commit 08467d2

Browse files
eamodioeamodio
committed
Embeds auth service into integration service
 - Limits/controls auth access to be through an integration (mainly)
1 parent 67fca82 commit 08467d2

File tree

10 files changed

+111
-101
lines changed

10 files changed

+111
-101
lines changed

‎src/commands/patches.ts

Lines changed: 1 addition & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -323,11 +323,7 @@ export class OpenCloudPatchCommand extends Command {
323323
return;
324324
}
325325

326-
const session = await this.container.integrationAuthentication.getSession(
327-
integration.id,
328-
integration.authProviderDescriptor,
329-
);
330-
326+
const session = await integration.getSession();
331327
if (session == null) {
332328
void window.showErrorMessage(`Cannot open ${type}; provider not connected.`);
333329
return;

‎src/constants.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -878,7 +878,7 @@ export type SupportedAIModels =
878878
| 'vscode';
879879

880880
export type SecretKeys =
881-
| `gitlens.integration.auth:${string}`
881+
| `gitlens.integration.auth:${IntegrationId}|${string}`
882882
| `gitlens.${AIProviders}.key`
883883
| `gitlens.plus.auth:${Environment}`;
884884

‎src/container.ts

Lines changed: 5 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -634,21 +634,14 @@ export class Container {
634634
return this._context.extension.id;
635635
}
636636

637-
private _integrationAuthentication: IntegrationAuthenticationService | undefined;
638-
get integrationAuthentication() {
639-
if (this._integrationAuthentication == null) {
640-
this._disposables.push(
641-
(this._integrationAuthentication = new IntegrationAuthenticationService(this, this._connection)),
642-
);
643-
}
644-
645-
return this._integrationAuthentication;
646-
}
647-
648637
private _integrations: IntegrationService | undefined;
649638
get integrations(): IntegrationService {
650639
if (this._integrations == null) {
651-
this._disposables.push((this._integrations = new IntegrationService(this)));
640+
const authenticationService = new IntegrationAuthenticationService(this, this._connection);
641+
this._disposables.push(
642+
authenticationService,
643+
(this._integrations = new IntegrationService(this, authenticationService)),
644+
);
652645
}
653646
return this._integrations;
654647
}

‎src/plus/drafts/draftsService.ts

Lines changed: 18 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,7 @@ export class DraftService implements Disposable {
113113
};
114114

115115
const repo = patchRequests[0].repository;
116-
const providerAuth = await this.getProviderAuthFromRepository(repo);
116+
const providerAuth = await this.getProviderAuthFromRepoOrIntegrationId(repo);
117117
if (providerAuth == null) {
118118
throw new Error('No provider integration found');
119119
}
@@ -759,17 +759,24 @@ export class DraftService implements Disposable {
759759
};
760760
}
761761

762-
async getProviderAuthFromRepository(repository: Repository): Promise<ProviderAuth | undefined> {
763-
const remoteProvider = await repository.getBestRemoteWithIntegration();
764-
if (remoteProvider == null) return undefined;
762+
async getProviderAuthFromRepoOrIntegrationId(
763+
repoOrIntegrationId: Repository | IntegrationId,
764+
): Promise<ProviderAuth | undefined> {
765+
let integration;
766+
if (isRepository(repoOrIntegrationId)) {
767+
const remoteProvider = await repoOrIntegrationId.getBestRemoteWithIntegration();
768+
if (remoteProvider == null) return undefined;
765769

766-
const integration = await remoteProvider.getIntegration();
770+
integration = await remoteProvider.getIntegration();
771+
} else {
772+
const metadata = providersMetadata[repoOrIntegrationId];
773+
if (metadata == null) return undefined;
774+
775+
integration = await this.container.integrations.get(repoOrIntegrationId, metadata.domain);
776+
}
767777
if (integration == null) return undefined;
768778

769-
const session = await this.container.integrationAuthentication.getSession(
770-
integration.id,
771-
integration.authProviderDescriptor,
772-
);
779+
const session = await integration.getSession();
773780
if (session == null) return undefined;
774781

775782
return {
@@ -778,21 +785,6 @@ export class DraftService implements Disposable {
778785
};
779786
}
780787

781-
async getProviderAuthForIntegration(integrationId: IntegrationId): Promise<ProviderAuth | undefined> {
782-
const metadata = providersMetadata[integrationId];
783-
if (metadata == null) return undefined;
784-
const session = await this.container.integrationAuthentication.getSession(integrationId, {
785-
domain: metadata.domain,
786-
scopes: metadata.scopes,
787-
});
788-
if (session == null) return undefined;
789-
790-
return {
791-
provider: integrationId,
792-
token: session.accessToken,
793-
};
794-
}
795-
796788
async getProviderAuthForDraft(draft: Draft): Promise<ProviderAuth | undefined> {
797789
if (draft.changesets == null || draft.changesets.length === 0) {
798790
return undefined;
@@ -829,7 +821,7 @@ export class DraftService implements Disposable {
829821
repo = repositoryOrIdentity;
830822
}
831823

832-
return this.getProviderAuthFromRepository(repo);
824+
return this.getProviderAuthFromRepoOrIntegrationId(repo);
833825
}
834826

835827
async getCodeSuggestions(
@@ -850,9 +842,7 @@ export class DraftService implements Disposable {
850842
): Promise<Draft[]> {
851843
const entityIdentifier = getEntityIdentifierInput(item);
852844
const prEntityId = EntityIdentifierUtils.encode(entityIdentifier);
853-
const providerAuth = isRepository(repositoryOrIntegrationId)
854-
? await this.getProviderAuthFromRepository(repositoryOrIntegrationId)
855-
: await this.getProviderAuthForIntegration(repositoryOrIntegrationId);
845+
const providerAuth = await this.getProviderAuthFromRepoOrIntegrationId(repositoryOrIntegrationId);
856846

857847
// swallowing this error as we don't need to fail here
858848
try {

‎src/plus/integrations/authentication/integrationAuthentication.ts

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -143,7 +143,7 @@ export class IntegrationAuthenticationService implements Disposable {
143143
}
144144
}
145145

146-
private getSecretKey(providerId: string, id: string): `gitlens.integration.auth:${string}` {
146+
private getSecretKey(providerId: IntegrationId, id: string): `gitlens.integration.auth:${IntegrationId}|${string}` {
147147
return `gitlens.integration.auth:${providerId}|${id}`;
148148
}
149149

‎src/plus/integrations/integration.ts

Lines changed: 39 additions & 39 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@ import type { LogScope } from '../../system/logger.scope';
2424
import { getLogScope } from '../../system/logger.scope';
2525
import type {
2626
IntegrationAuthenticationProviderDescriptor,
27+
IntegrationAuthenticationService,
2728
IntegrationAuthenticationSessionDescriptor,
2829
} from './authentication/integrationAuthentication';
2930
import type { ProviderAuthenticationSession } from './authentication/models';
@@ -88,6 +89,7 @@ export abstract class IntegrationBase<
8889

8990
constructor(
9091
protected readonly container: Container,
92+
protected readonly authenticationService: IntegrationAuthenticationService,
9193
protected readonly getProvidersApi: () => Promise<ProvidersApi>,
9294
) {}
9395

@@ -125,7 +127,7 @@ export abstract class IntegrationBase<
125127
}
126128

127129
protected _session: ProviderAuthenticationSession | null | undefined;
128-
protected session() {
130+
getSession() {
129131
if (this._session === undefined) {
130132
return this.ensureSession(false);
131133
}
@@ -151,40 +153,38 @@ export abstract class IntegrationBase<
151153

152154
const connected = this._session != null;
153155

154-
if (connected && !options?.silent) {
155-
if (options?.currentSessionOnly) {
156-
void showIntegrationDisconnectedTooManyFailedRequestsWarningMessage(this.name);
156+
let signOut = !options?.currentSessionOnly;
157+
158+
if (connected && !options?.currentSessionOnly && !options?.silent) {
159+
const disable = { title: 'Disable' };
160+
const disableAndSignOut = { title: 'Disable & Sign Out' };
161+
const cancel = { title: 'Cancel', isCloseAffordance: true };
162+
163+
let result: MessageItem | undefined;
164+
if (this.authenticationService.supports(this.authProvider.id)) {
165+
result = await window.showWarningMessage(
166+
`Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`,
167+
{ modal: true },
168+
disable,
169+
disableAndSignOut,
170+
cancel,
171+
);
157172
} else {
158-
const disable = { title: 'Disable' };
159-
const signout = { title: 'Disable & Sign Out' };
160-
const cancel = { title: 'Cancel', isCloseAffordance: true };
161-
162-
let result: MessageItem | undefined;
163-
if (this.container.integrationAuthentication.supports(this.authProvider.id)) {
164-
result = await window.showWarningMessage(
165-
`Are you sure you want to disable the rich integration with ${this.name}?\n\nNote: signing out clears the saved authentication.`,
166-
{ modal: true },
167-
disable,
168-
signout,
169-
cancel,
170-
);
171-
} else {
172-
result = await window.showWarningMessage(
173-
`Are you sure you want to disable the rich integration with ${this.name}?`,
174-
{ modal: true },
175-
disable,
176-
cancel,
177-
);
178-
}
179-
180-
if (result == null || result === cancel) return;
181-
if (result === signout) {
182-
void this.container.integrationAuthentication.deleteSession(
183-
this.authProvider.id,
184-
this.authProviderDescriptor,
185-
);
186-
}
173+
result = await window.showWarningMessage(
174+
`Are you sure you want to disable the rich integration with ${this.name}?`,
175+
{ modal: true },
176+
disable,
177+
cancel,
178+
);
187179
}
180+
181+
if (result == null || result === cancel) return;
182+
183+
signOut = result === disableAndSignOut;
184+
}
185+
186+
if (signOut) {
187+
void this.authenticationService.deleteSession(this.authProvider.id, this.authProviderDescriptor);
188188
}
189189

190190
this.resetRequestExceptionCount();
@@ -241,14 +241,15 @@ export abstract class IntegrationBase<
241241
this.requestExceptionCount++;
242242

243243
if (this.requestExceptionCount >= 5 && this._session !== null) {
244+
void showIntegrationDisconnectedTooManyFailedRequestsWarningMessage(this.name);
244245
void this.disconnect({ currentSessionOnly: true });
245246
}
246247
}
247248

248249
@gate()
249250
@debug({ exit: true })
250251
async isConnected(): Promise<boolean> {
251-
return (await this.session()) != null;
252+
return (await this.getSession()) != null;
252253
}
253254

254255
@gate()
@@ -267,11 +268,10 @@ export abstract class IntegrationBase<
267268

268269
let session: ProviderAuthenticationSession | undefined | null;
269270
try {
270-
session = await this.container.integrationAuthentication.getSession(
271-
this.authProvider.id,
272-
this.authProviderDescriptor,
273-
{ createIfNeeded: createIfNeeded, forceNewSession: forceNewSession },
274-
);
271+
session = await this.authenticationService.getSession(this.authProvider.id, this.authProviderDescriptor, {
272+
createIfNeeded: createIfNeeded,
273+
forceNewSession: forceNewSession,
274+
});
275275
} catch (ex) {
276276
await this.container.storage.deleteWorkspace(this.connectedKey);
277277

‎src/plus/integrations/integrationService.ts

Lines changed: 28 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import { debug, log } from '../../system/decorators/log';
1212
import { take } from '../../system/event';
1313
import { filterMap, flatten } from '../../system/iterable';
1414
import type { SubscriptionChangeEvent } from '../gk/account/subscriptionService';
15+
import type { IntegrationAuthenticationService } from './authentication/integrationAuthentication';
1516
import { supportedCloudIntegrationIds, toIntegrationId } from './authentication/models';
1617
import type {
1718
HostingIntegration,
@@ -51,7 +52,10 @@ export class IntegrationService implements Disposable {
5152
private readonly _disposable: Disposable;
5253
private _integrations = new Map<IntegrationKey, Integration>();
5354

54-
constructor(private readonly container: Container) {
55+
constructor(
56+
private readonly container: Container,
57+
private readonly authenticationService: IntegrationAuthenticationService,
58+
) {
5559
this._disposable = Disposable.from(
5660
configuration.onDidChange(e => {
5761
if (configuration.changed(e, 'remotes')) {
@@ -222,39 +226,53 @@ export class IntegrationService implements Disposable {
222226
case HostingIntegrationId.GitHub:
223227
integration = new (
224228
await import(/* webpackChunkName: "integrations" */ './providers/github')
225-
).GitHubIntegration(this.container, this.getProvidersApi.bind(this));
229+
).GitHubIntegration(this.container, this.authenticationService, this.getProvidersApi.bind(this));
226230
break;
227231
case SelfHostedIntegrationId.GitHubEnterprise:
228232
if (domain == null) throw new Error(`Domain is required for '${id}' integration`);
229233
integration = new (
230234
await import(/* webpackChunkName: "integrations" */ './providers/github')
231-
).GitHubEnterpriseIntegration(this.container, this.getProvidersApi.bind(this), domain);
235+
).GitHubEnterpriseIntegration(
236+
this.container,
237+
this.authenticationService,
238+
this.getProvidersApi.bind(this),
239+
domain,
240+
);
232241
break;
233242
case HostingIntegrationId.GitLab:
234243
integration = new (
235244
await import(/* webpackChunkName: "integrations" */ './providers/gitlab')
236-
).GitLabIntegration(this.container, this.getProvidersApi.bind(this));
245+
).GitLabIntegration(this.container, this.authenticationService, this.getProvidersApi.bind(this));
237246
break;
238247
case SelfHostedIntegrationId.GitLabSelfHosted:
239248
if (domain == null) throw new Error(`Domain is required for '${id}' integration`);
240249
integration = new (
241250
await import(/* webpackChunkName: "integrations" */ './providers/gitlab')
242-
).GitLabSelfHostedIntegration(this.container, this.getProvidersApi.bind(this), domain);
251+
).GitLabSelfHostedIntegration(
252+
this.container,
253+
this.authenticationService,
254+
this.getProvidersApi.bind(this),
255+
domain,
256+
);
243257
break;
244258
case HostingIntegrationId.Bitbucket:
245259
integration = new (
246260
await import(/* webpackChunkName: "integrations" */ './providers/bitbucket')
247-
).BitbucketIntegration(this.container, this.getProvidersApi.bind(this));
261+
).BitbucketIntegration(this.container, this.authenticationService, this.getProvidersApi.bind(this));
248262
break;
249263
case HostingIntegrationId.AzureDevOps:
250264
integration = new (
251265
await import(/* webpackChunkName: "integrations" */ './providers/azureDevOps')
252-
).AzureDevOpsIntegration(this.container, this.getProvidersApi.bind(this));
266+
).AzureDevOpsIntegration(
267+
this.container,
268+
this.authenticationService,
269+
this.getProvidersApi.bind(this),
270+
);
253271
break;
254272
case IssueIntegrationId.Jira:
255273
integration = new (
256274
await import(/* webpackChunkName: "integrations" */ './providers/jira')
257-
).JiraIntegration(this.container, this.getProvidersApi.bind(this));
275+
).JiraIntegration(this.container, this.authenticationService, this.getProvidersApi.bind(this));
258276
break;
259277
default:
260278
throw new Error(`Integration with '${id}' is not supported`);
@@ -269,10 +287,11 @@ export class IntegrationService implements Disposable {
269287
private async getProvidersApi() {
270288
if (this._providersApi == null) {
271289
const container = this.container;
290+
const authenticationService = this.authenticationService;
272291
async function load() {
273292
return new (
274293
await import(/* webpackChunkName: "integrations-api" */ './providers/providersApi')
275-
).ProvidersApi(container);
294+
).ProvidersApi(container, authenticationService);
276295
}
277296

278297
this._providersApi = load();

‎src/plus/integrations/providers/github.ts

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,7 +9,10 @@ import { PullRequest } from '../../../git/models/pullRequest';
99
import type { RepositoryMetadata } from '../../../git/models/repositoryMetadata';
1010
import { log } from '../../../system/decorators/log';
1111
import { ensurePaidPlan } from '../../utils';
12-
import type { IntegrationAuthenticationProviderDescriptor } from '../authentication/integrationAuthentication';
12+
import type {
13+
IntegrationAuthenticationProviderDescriptor,
14+
IntegrationAuthenticationService,
15+
} from '../authentication/integrationAuthentication';
1316
import type { SupportedIntegrationIds } from '../integration';
1417
import { HostingIntegration } from '../integration';
1518
import { HostingIntegrationId, providersMetadata, SelfHostedIntegrationId } from './models';
@@ -282,10 +285,11 @@ export class GitHubEnterpriseIntegration extends GitHubIntegrationBase<SelfHoste
282285

283286
constructor(
284287
container: Container,
288+
authenticationService: IntegrationAuthenticationService,
285289
getProvidersApi: () => Promise<ProvidersApi>,
286290
private readonly _domain: string,
287291
) {
288-
super(container, getProvidersApi);
292+
super(container, authenticationService, getProvidersApi);
289293
}
290294

291295
@log()

0 commit comments

Comments
 (0)