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

[PM-11941] Migrate TOTP Generator to use SDK #12987

Open
wants to merge 16 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 7 additions & 7 deletions apps/browser/src/autofill/background/overlay.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -692,13 +692,12 @@ export class OverlayBackground implements OverlayBackgroundInterface {
};

if (cipher.type === CipherType.Login) {
const totpCode = await this.totpService.getCode(cipher.login?.totp);
const totpCodeTimeInterval = this.totpService.getTimeInterval(cipher.login?.totp);
const totpResponse = await this.totpService.getCode(cipher.login?.totp);
inlineMenuData.login = {
username: cipher.login.username,
totp: totpCode,
totp: totpResponse?.code,
totpField: this.isTotpFieldForCurrentField(),
totpCodeTimeInterval: totpCodeTimeInterval,
totpCodeTimeInterval: totpResponse?.period,
passkey: hasPasskey
? {
rpName: cipher.login.fido2Credentials[0].rpName,
Expand Down Expand Up @@ -1116,9 +1115,10 @@ export class OverlayBackground implements OverlayBackgroundInterface {
this.updateLastUsedInlineMenuCipher(inlineMenuCipherId, cipher);

if (cipher.login?.totp) {
this.platformUtilsService.copyToClipboard(
await this.totpService.getCode(cipher.login.totp),
);
const totpResponse = await this.totpService.getCode(cipher.login.totp);
if (totpResponse) {
this.platformUtilsService.copyToClipboard(totpResponse.code);
}
}
return;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,10 +159,16 @@ describe("ContextMenuClickedHandler", () => {

totpService.getCode.mockImplementation((seed) => {
if (seed === "TEST_TOTP_SEED") {
return Promise.resolve("123456");
return Promise.resolve({
code: "123456",
period: 30,
});
}

return Promise.resolve("654321");
return Promise.resolve({
code: "654321",
period: 30,
});
});

await sut.run(createData(`${COPY_VERIFICATION_CODE_ID}_1`, COPY_VERIFICATION_CODE_ID), {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -199,10 +199,13 @@ export class ContextMenuClickedHandler {
action: COPY_VERIFICATION_CODE_ID,
});
} else {
this.copyToClipboard({
text: await this.totpService.getCode(cipher.login.totp),
tab: tab,
});
const totpResponse = await this.totpService.getCode(cipher.login.totp);
if (totpResponse?.code) {
this.copyToClipboard({
text: totpResponse.code,
tab: tab,
});
}
coroiu marked this conversation as resolved.
Show resolved Hide resolved
}

break;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -918,7 +918,7 @@ describe("AutofillService", () => {
.spyOn(billingAccountProfileStateService, "hasPremiumFromAnySource$")
.mockImplementation(() => of(true));
jest.spyOn(autofillService, "getShouldAutoCopyTotp").mockResolvedValue(true);
jest.spyOn(totpService, "getCode").mockResolvedValue(totpCode);
jest.spyOn(totpService, "getCode").mockResolvedValue({ code: totpCode, period: 30 });

const autofillResult = await autofillService.doAutoFill(autofillOptions);

Expand Down
4 changes: 2 additions & 2 deletions apps/browser/src/autofill/services/autofill.service.ts
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ export default class AutofillService implements AutofillServiceInterface {
const shouldAutoCopyTotp = await this.getShouldAutoCopyTotp();

totp = shouldAutoCopyTotp
? await this.totpService.getCode(options.cipher.login.totp)
? (await this.totpService.getCode(options.cipher.login.totp))?.code
: null;
}),
);
Expand Down Expand Up @@ -972,7 +972,7 @@ export default class AutofillService implements AutofillServiceInterface {
}

filledFields[t.opid] = t;
let totpValue = await this.totpService.getCode(login.totp);
let totpValue = (await this.totpService.getCode(login.totp))?.code;
if (totpValue.length == totps.length) {
totpValue = totpValue.charAt(i);
}
Expand Down
2 changes: 1 addition & 1 deletion apps/browser/src/background/main.background.ts
Original file line number Diff line number Diff line change
Expand Up @@ -963,7 +963,7 @@ export default class MainBackground {
this.authService,
this.accountService,
);
this.totpService = new TotpService(this.cryptoFunctionService, this.logService);
this.totpService = new TotpService(this.sdkService);

this.scriptInjectorService = new BrowserScriptInjectorService(
this.domainSettingsService,
Expand Down
3 changes: 2 additions & 1 deletion apps/browser/src/popup/services/services.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,7 @@ import { LogService } from "@bitwarden/common/platform/abstractions/log.service"
import { MessagingService as MessagingServiceAbstraction } from "@bitwarden/common/platform/abstractions/messaging.service";
import { PlatformUtilsService } from "@bitwarden/common/platform/abstractions/platform-utils.service";
import { SdkClientFactory } from "@bitwarden/common/platform/abstractions/sdk/sdk-client-factory";
import { SdkService } from "@bitwarden/common/platform/abstractions/sdk/sdk.service";
import { StateService } from "@bitwarden/common/platform/abstractions/state.service";
import {
AbstractStorageService,
Expand Down Expand Up @@ -266,7 +267,7 @@ const safeProviders: SafeProvider[] = [
safeProvider({
provide: TotpServiceAbstraction,
useClass: TotpService,
deps: [CryptoFunctionService, LogService],
deps: [SdkService],
}),
safeProvider({
provide: OffscreenDocumentService,
Expand Down
2 changes: 1 addition & 1 deletion apps/cli/src/commands/get.command.ts
Original file line number Diff line number Diff line change
Expand Up @@ -255,7 +255,7 @@ export class GetCommand extends DownloadCommand {
return Response.error("No TOTP available for this login.");
}

const totp = await this.totpService.getCode(cipher.login.totp);
const totp = (await this.totpService.getCode(cipher.login.totp))?.code;
if (totp == null) {
return Response.error("Couldn't generate TOTP code.");
}
Expand Down
2 changes: 1 addition & 1 deletion apps/cli/src/service-container/service-container.ts
Original file line number Diff line number Diff line change
Expand Up @@ -750,7 +750,7 @@ export class ServiceContainer {
this.stateProvider,
);

this.totpService = new TotpService(this.cryptoFunctionService, this.logService);
this.totpService = new TotpService(this.sdkService);

this.importApiService = new ImportApiService(this.apiService);

Expand Down
4 changes: 2 additions & 2 deletions apps/desktop/src/vault/app/vault/vault.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -213,7 +213,7 @@ export class VaultComponent implements OnInit, OnDestroy {
tCipher.login.hasTotp &&
this.userHasPremiumAccess
) {
const value = await this.totpService.getCode(tCipher.login.totp);
const value = (await this.totpService.getCode(tCipher.login.totp))?.code;
this.copyValue(tCipher, value, "verificationCodeTotp", "TOTP");
}
break;
Expand Down Expand Up @@ -390,7 +390,7 @@ export class VaultComponent implements OnInit, OnDestroy {
menu.push({
label: this.i18nService.t("copyVerificationCodeTotp"),
click: async () => {
const value = await this.totpService.getCode(cipher.login.totp);
const value = (await this.totpService.getCode(cipher.login.totp))?.code;
this.copyValue(cipher, value, "verificationCodeTotp", "TOTP");
},
});
Expand Down
15 changes: 9 additions & 6 deletions apps/web/src/app/vault/individual-vault/add-edit.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -135,12 +135,15 @@ export class AddEditComponent extends BaseAddEditComponent implements OnInit, On

if (this.showTotp()) {
await this.totpUpdateCode();
const interval = this.totpService.getTimeInterval(this.cipher.login.totp);
await this.totpTick(interval);

this.totpInterval = window.setInterval(async () => {
const totpResponse = await this.totpService.getCode(this.cipher.login.totp);
if (totpResponse) {
const interval = totpResponse.period;
await this.totpTick(interval);
}, 1000);

this.totpInterval = window.setInterval(async () => {
await this.totpTick(interval);
}, 1000);
coroiu marked this conversation as resolved.
Show resolved Hide resolved
}
}

const extensionRefreshEnabled = await firstValueFrom(
Expand Down Expand Up @@ -277,7 +280,7 @@ export class AddEditComponent extends BaseAddEditComponent implements OnInit, On
return;
}

this.totpCode = await this.totpService.getCode(this.cipher.login.totp);
this.totpCode = (await this.totpService.getCode(this.cipher.login.totp))?.code;
if (this.totpCode != null) {
if (this.totpCode.length > 4) {
const half = Math.floor(this.totpCode.length / 2);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -1263,7 +1263,7 @@ export class VaultComponent implements OnInit, OnDestroy {
typeI18nKey = "password";
} else if (field === "totp") {
aType = "TOTP";
value = await this.totpService.getCode(cipher.login.totp);
value = (await this.totpService.getCode(cipher.login.totp))?.code;
typeI18nKey = "verificationCodeTotp";
} else {
this.toastService.showToast({
Expand Down
2 changes: 1 addition & 1 deletion apps/web/src/app/vault/org-vault/vault.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1268,7 +1268,7 @@ export class VaultComponent implements OnInit, OnDestroy {
typeI18nKey = "password";
} else if (field === "totp") {
aType = "TOTP";
value = await this.totpService.getCode(cipher.login.totp);
value = (await this.totpService.getCode(cipher.login.totp))?.code;
typeI18nKey = "verificationCodeTotp";
} else {
this.toastService.showToast({
Expand Down
2 changes: 1 addition & 1 deletion libs/angular/src/services/jslib-services.module.ts
Original file line number Diff line number Diff line change
Expand Up @@ -593,7 +593,7 @@ const safeProviders: SafeProvider[] = [
safeProvider({
provide: TotpServiceAbstraction,
useClass: TotpService,
deps: [CryptoFunctionServiceAbstraction, LogService],
deps: [SdkService],
}),
safeProvider({
provide: TokenServiceAbstraction,
Expand Down
15 changes: 9 additions & 6 deletions libs/angular/src/vault/components/view.component.ts
Original file line number Diff line number Diff line change
Expand Up @@ -173,12 +173,15 @@ export class ViewComponent implements OnDestroy, OnInit {

if (this.cipher.type === CipherType.Login && this.cipher.login.totp && canGenerateTotp) {
await this.totpUpdateCode();
const interval = this.totpService.getTimeInterval(this.cipher.login.totp);
await this.totpTick(interval);

this.totpInterval = setInterval(async () => {
const totpResponse = await this.totpService.getCode(this.cipher.login.totp);
if (totpResponse) {
const interval = totpResponse.period;
await this.totpTick(interval);
}, 1000);

this.totpInterval = setInterval(async () => {
await this.totpTick(interval);
}, 1000);
coroiu marked this conversation as resolved.
Show resolved Hide resolved
}
}

if (this.previousCipherId !== this.cipherId) {
Expand Down Expand Up @@ -537,7 +540,7 @@ export class ViewComponent implements OnDestroy, OnInit {
return;
}

this.totpCode = await this.totpService.getCode(this.cipher.login.totp);
this.totpCode = (await this.totpService.getCode(this.cipher.login.totp))?.code;
if (this.totpCode != null) {
if (this.totpCode.length > 4) {
const half = Math.floor(this.totpCode.length / 2);
Expand Down
7 changes: 3 additions & 4 deletions libs/common/src/vault/abstractions/totp.service.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
// FIXME: Update this file to be type safe and remove this and next line
// @ts-strict-ignore
import { TotpResponse } from "@bitwarden/sdk-internal";

export abstract class TotpService {
getCode: (key: string) => Promise<string>;
getTimeInterval: (key: string) => number;
abstract getCode(key: string | undefined): Promise<TotpResponse | undefined>;
}
60 changes: 26 additions & 34 deletions libs/common/src/vault/services/totp.service.spec.ts
Original file line number Diff line number Diff line change
@@ -1,17 +1,31 @@
import { mock } from "jest-mock-extended";
import { of } from "rxjs";

import { LogService } from "../../platform/abstractions/log.service";
import { WebCryptoFunctionService } from "../../platform/services/web-crypto-function.service";
import { BitwardenClient } from "@bitwarden/sdk-internal";

import { SdkService } from "../../platform/abstractions/sdk/sdk.service";

import { TotpService } from "./totp.service";

describe("TotpService", () => {
let totpService: TotpService;

const logService = mock<LogService>();
const sdkService = mock<SdkService>();
const mockBitwardenClient = {
vault: () => ({
totp: () => ({
generate_totp: jest.fn().mockResolvedValue({
code: "123456",
period: 30,
}),
}),
}),
};

beforeEach(() => {
totpService = new TotpService(new WebCryptoFunctionService(global), logService);
sdkService.client$ = of(mockBitwardenClient as unknown as BitwardenClient);

totpService = new TotpService(sdkService);

// TOTP is time-based, so we need to mock the current time
jest.useFakeTimers({
Expand All @@ -24,40 +38,18 @@ describe("TotpService", () => {
jest.useRealTimers();
});

it("should return null if key is null", async () => {
const result = await totpService.getCode(null);
expect(result).toBeNull();
it("should return undefined if key is undefined", async () => {
const result = await totpService.getCode(undefined);
expect(result).toBeUndefined();
});

it("should return a code if key is not null", async () => {
it("should return TOTP response when is provided", async () => {
const result = await totpService.getCode("WQIQ25BRKZYCJVYP");
expect(result).toBe("194506");
});

it("should handle otpauth keys", async () => {
const key = "otpauth://totp/test-account?secret=WQIQ25BRKZYCJVYP";
const result = await totpService.getCode(key);
expect(result).toBe("194506");

const period = totpService.getTimeInterval(key);
expect(period).toBe(30);
expect(result).toEqual({ code: "123456", period: 30 });
});

it("should handle otpauth different period", async () => {
const key = "otpauth://totp/test-account?secret=WQIQ25BRKZYCJVYP&period=60";
const result = await totpService.getCode(key);
expect(result).toBe("730364");

const period = totpService.getTimeInterval(key);
expect(period).toBe(60);
});

it("should handle steam keys", async () => {
const key = "steam://HXDMVJECJJWSRB3HWIZR4IFUGFTMXBOZ";
const result = await totpService.getCode(key);
expect(result).toBe("7W6CJ");

const period = totpService.getTimeInterval(key);
expect(period).toBe(30);
it("should throw error when SDK is undefined", async () => {
sdkService.client$ = of(undefined);
await expect(totpService.getCode("WQIQ25BRKZYCJVYP")).rejects.toThrow("SDK is undefined");
});
});
Loading
Loading