Fix secret storing logic for small passwords on Windows (#175817)

On Windows, we still stored passwords less than the CredMan limit as they were (without the ChunkedPassword type wrapper).

This brings that behavior back.

Additionally, this moves all the event firing to the non-do functions which I think looks nicer.
This commit is contained in:
Tyler James Leonhardt 2023-03-01 16:21:01 -08:00 committed by GitHub
parent 5fb19ca5b4
commit ce3221ef8e
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23

View file

@ -58,7 +58,7 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
return null;
}
return this._sequencer.queue(service + account, () => this.doGetPassword(keytar, service, account));
return await this._sequencer.queue(service + account, () => this.doGetPassword(keytar, service, account));
}
private async doGetPassword(keytar: KeytarModule, service: string, account: string): Promise<string | null> {
@ -113,22 +113,30 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
}
await this._sequencer.queue(service + account, () => this.doSetPassword(keytar, service, account, password));
this._onDidChangePassword.fire({ service, account });
}
private async doSetPassword(keytar: KeytarModule, service: string, account: string, password: string): Promise<void> {
if (!isWindows) {
await retry(() => keytar.setPassword(service, account, password), 50, 3);
this.logService.trace('Set password from keytar for account:', account);
this._onDidChangePassword.fire({ service, account });
return;
}
// On Windows, we have to chunk the password because the Windows Credential Manager only allows passwords of a max length.
// So to make sure we can store passwords of any length, we chunk the password and store it as multiple passwords.
// To ensure we store an password correctly, we first delete any existing password chunks and then store the new ones.
// On Windows, we sometimes have to chunk the password because the Windows Credential Manager only allows passwords of a max length.
// So to make sure we can store passwords of any length, we chunk the longer passwords and store it as multiple passwords.
// To ensure we store any password correctly, we first delete any existing password, chunks and all, and then store the new ones.
await this.doDeletePassword(keytar, service, account);
// if it's a short password, just store it
if (password.length <= BaseCredentialsMainService.PASSWORD_CHUNK_SIZE) {
await retry(() => keytar.setPassword(service, account, password), 50, 3);
this.logService.trace('Set password from keytar for account:', account);
return;
}
// otherwise, chunk it and store it
let index = 0;
let chunk = 0;
let hasNextChunk = true;
@ -149,7 +157,6 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
await Promise.all(promises);
this.logService.trace(`Set${chunk ? ` ${chunk}-chunked` : ''} password from keytar for account:`, account);
this._onDidChangePassword.fire({ service, account });
}
async deletePassword(service: string, account: string): Promise<boolean> {
@ -162,7 +169,11 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
throw e;
}
return await this._sequencer.queue(service + account, () => this.doDeletePassword(keytar, service, account));
const result = await this._sequencer.queue(service + account, () => this.doDeletePassword(keytar, service, account));
if (result) {
this._onDidChangePassword.fire({ service, account });
}
return result;
}
private async doDeletePassword(keytar: KeytarModule, service: string, account: string): Promise<boolean> {
@ -208,7 +219,6 @@ export abstract class BaseCredentialsMainService extends Disposable implements I
// Delete the first account to determine deletion success
if (await keytar.deletePassword(service, account)) {
this._onDidChangePassword.fire({ service, account });
this.logService.trace(`Deleted${index ? ` ${index}-chunked` : ''} password from keytar for account:`, account);
return true;
}