Skip to content

Commit 08eab7a

Browse files
personalizedrefrigeratorpersonalizedrefrigerator
authored
Desktop: Use Electron safeStorage for keychain support (#10535)
1 parent 8d8c91e commit 08eab7a

18 files changed

+351
-41
lines changed

‎.eslintignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1102,8 +1102,10 @@ packages/lib/services/interop/Module.js
11021102
packages/lib/services/interop/types.js
11031103
packages/lib/services/joplinCloudUtils.js
11041104
packages/lib/services/joplinServer/personalizedUserContentBaseUrl.js
1105+
packages/lib/services/keychain/KeychainService.test.js
11051106
packages/lib/services/keychain/KeychainService.js
11061107
packages/lib/services/keychain/KeychainServiceDriver.dummy.js
1108+
packages/lib/services/keychain/KeychainServiceDriver.electron.js
11071109
packages/lib/services/keychain/KeychainServiceDriver.mobile.js
11081110
packages/lib/services/keychain/KeychainServiceDriver.node.js
11091111
packages/lib/services/keychain/KeychainServiceDriverBase.js

‎.gitignore

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1080,8 +1080,10 @@ packages/lib/services/interop/Module.js
10801080
packages/lib/services/interop/types.js
10811081
packages/lib/services/joplinCloudUtils.js
10821082
packages/lib/services/joplinServer/personalizedUserContentBaseUrl.js
1083+
packages/lib/services/keychain/KeychainService.test.js
10831084
packages/lib/services/keychain/KeychainService.js
10841085
packages/lib/services/keychain/KeychainServiceDriver.dummy.js
1086+
packages/lib/services/keychain/KeychainServiceDriver.electron.js
10851087
packages/lib/services/keychain/KeychainServiceDriver.mobile.js
10861088
packages/lib/services/keychain/KeychainServiceDriver.node.js
10871089
packages/lib/services/keychain/KeychainServiceDriverBase.js

‎packages/app-desktop/bridge.ts

Lines changed: 16 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,7 @@
11
import ElectronAppWrapper from './ElectronAppWrapper';
22
import shim from '@joplin/lib/shim';
33
import { _, setLocale } from '@joplin/lib/locale';
4-
import { BrowserWindow, nativeTheme, nativeImage, shell, dialog, MessageBoxSyncOptions } from 'electron';
4+
import { BrowserWindow, nativeTheme, nativeImage, shell, dialog, MessageBoxSyncOptions, safeStorage } from 'electron';
55
import { dirname, toSystemSlashes } from '@joplin/lib/path-utils';
66
import { fileUriToPath } from '@joplin/utils/url';
77
import { urlDecode } from '@joplin/lib/string-utils';
@@ -485,6 +485,21 @@ export class Bridge {
485485
return nativeImage.createFromPath(path);
486486
}
487487

488+
public safeStorage = {
489+
isEncryptionAvailable() {
490+
return safeStorage.isEncryptionAvailable();
491+
},
492+
encryptString(data: string) {
493+
return safeStorage.encryptString(data).toString('base64');
494+
},
495+
decryptString(base64Data: string) {
496+
return safeStorage.decryptString(Buffer.from(base64Data, 'base64'));
497+
},
498+
499+
getSelectedStorageBackend() {
500+
return safeStorage.getSelectedStorageBackend();
501+
},
502+
};
488503
}
489504

490505
let bridge_: Bridge = null;

‎packages/app-desktop/main.js

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ const registerCustomProtocols = require('./utils/customProtocols/registerCustomP
1818
// our case it's a string like "@joplin/app-desktop". It's also supposed to
1919
// check the productName key but is not doing it, so here set the
2020
// application name to the right string.
21-
electronApp.name = packageInfo.name;
21+
electronApp.setName(packageInfo.name);
2222

2323
process.on('unhandledRejection', (reason, p) => {
2424
console.error('Unhandled promise rejection', p, 'reason:', reason);

‎packages/app-mobile/root.tsx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -624,7 +624,7 @@ async function initialize(dispatch: Dispatch) {
624624
reg.logger().info('Database is ready.');
625625
reg.logger().info('Loading settings...');
626626

627-
await loadKeychainServiceAndSettings(KeychainServiceDriverMobile);
627+
await loadKeychainServiceAndSettings([KeychainServiceDriverMobile]);
628628
await migrateMasterPassword();
629629

630630
if (!Setting.value('clientId')) Setting.setValue('clientId', uuid.create());

‎packages/lib/BaseApplication.ts

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -4,8 +4,8 @@ import shim from './shim';
44
const { setupProxySettings } = require('./shim-init-node');
55
import BaseService from './services/BaseService';
66
import reducer, { getNotesParent, serializeNotesParent, setStore, State } from './reducer';
7-
import KeychainServiceDriver from './services/keychain/KeychainServiceDriver.node';
8-
import KeychainServiceDriverDummy from './services/keychain/KeychainServiceDriver.dummy';
7+
import KeychainServiceDriverNode from './services/keychain/KeychainServiceDriver.node';
8+
import KeychainServiceDriverElectron from './services/keychain/KeychainServiceDriver.electron';
99
import { setLocale } from './locale';
1010
import KvStore from './services/KvStore';
1111
import SyncTargetJoplinServer from './SyncTargetJoplinServer';
@@ -754,10 +754,13 @@ export default class BaseApplication {
754754

755755
reg.setDb(this.database_);
756756
BaseModel.setDb(this.database_);
757+
KvStore.instance().setDb(reg.db());
757758

758759
setRSA(RSA);
759760

760-
await loadKeychainServiceAndSettings(options.keychainEnabled ? KeychainServiceDriver : KeychainServiceDriverDummy);
761+
await loadKeychainServiceAndSettings(
762+
options.keychainEnabled ? [KeychainServiceDriverElectron, KeychainServiceDriverNode] : [],
763+
);
761764
await migrateMasterPassword();
762765
await handleSyncStartupOperation();
763766

@@ -841,7 +844,6 @@ export default class BaseApplication {
841844

842845
BaseItem.revisionService_ = RevisionService.instance();
843846

844-
KvStore.instance().setDb(reg.db());
845847

846848
BaseItem.encryptionService_ = EncryptionService.instance();
847849
BaseItem.shareService_ = ShareService.instance();

‎packages/lib/models/Setting.ts

Lines changed: 2 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -142,7 +142,6 @@ export type SettingMetadataSection = {
142142
export type MetadataBySection = SettingMetadataSection[];
143143

144144
class Setting extends BaseModel {
145-
146145
public static schemaUrl = 'https://joplinapp.org/schema/settings.json';
147146

148147
// For backward compatibility
@@ -976,19 +975,10 @@ class Setting extends BaseModel {
976975
// Also we don't control what happens on the keychain - the values can be edited or deleted
977976
// outside the application. For that reason, we rewrite it every time the values are saved,
978977
// even if, internally, they haven't changed.
979-
// As an optimisation, we check if the value exists on the keychain before writing it again.
980978
try {
981979
const passwordName = `setting.${s.key}`;
982-
const currentValue = await this.keychainService().password(passwordName);
983-
if (currentValue !== valueAsString) {
984-
const wasSet = await this.keychainService().setPassword(passwordName, valueAsString);
985-
if (wasSet) continue;
986-
} else {
987-
// The value is already in the keychain - so nothing to do
988-
// Make sure to `continue` here otherwise it will save the password
989-
// in clear text in the database.
990-
continue;
991-
}
980+
const wasSet = await this.keychainService().setPassword(passwordName, valueAsString);
981+
if (wasSet) continue;
992982
} catch (error) {
993983
logger.error(`Could not set setting on the keychain. Will be saved to database instead: ${s.key}:`, error);
994984
}

‎packages/lib/models/settings/builtInMetadata.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -928,6 +928,7 @@ const builtInMetadata = (Setting: typeof SettingType) => {
928928
collapsedFolderIds: { value: [] as string[], type: SettingItemType.Array, public: false },
929929

930930
'keychain.supported': { value: -1, type: SettingItemType.Int, public: false },
931+
'keychain.lastAvailableDrivers': { value: [] as string[], type: SettingItemType.Array, public: false },
931932
'db.ftsEnabled': { value: -1, type: SettingItemType.Int, public: false },
932933
'db.fuzzySearchEnabled': { value: -1, type: SettingItemType.Int, public: false },
933934
'encryption.enabled': { value: false, type: SettingItemType.Bool, public: false },

‎packages/lib/services/SettingUtils.ts

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,9 @@ import KeychainService from './keychain/KeychainService';
44
import Setting from '../models/Setting';
55
import uuid from '../uuid';
66
import { migrateLocalSyncInfo } from './synchronizer/syncInfoUtils';
7+
import KeychainServiceDriverBase from './keychain/KeychainServiceDriverBase';
8+
9+
type KeychainServiceDriverConstructor = new (appId: string, clientId: string)=> KeychainServiceDriverBase;
710

811
// This function takes care of initialising both the keychain service and settings.
912
//
@@ -13,11 +16,12 @@ import { migrateLocalSyncInfo } from './synchronizer/syncInfoUtils';
1316
// In other words, it's not possible to load the settings without the KS service and it's not
1417
// possible to initialise the KS service without the settings.
1518
// The solution is to fetch just the client ID directly from the database.
16-
// eslint-disable-next-line @typescript-eslint/no-explicit-any -- Old code before rule was applied
17-
export async function loadKeychainServiceAndSettings(KeychainServiceDriver: any) {
19+
export async function loadKeychainServiceAndSettings(keychainServiceDrivers: KeychainServiceDriverConstructor[]) {
1820
const clientIdSetting = await Setting.loadOne('clientId');
1921
const clientId = clientIdSetting ? clientIdSetting.value : uuid.create();
20-
KeychainService.instance().initialize(new KeychainServiceDriver(Setting.value('appId'), clientId));
22+
await KeychainService.instance().initialize(
23+
keychainServiceDrivers.map(Driver => new Driver(Setting.value('appId'), clientId)),
24+
);
2125
Setting.setKeychainService(KeychainService.instance());
2226
await Setting.load();
2327

‎packages/lib/services/keychain/KeychainService.test.ts

Lines changed: 128 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,128 @@
1+
import Setting from '../../models/Setting';
2+
import shim from '../../shim';
3+
import { switchClient, setupDatabaseAndSynchronizer } from '../../testing/test-utils';
4+
import KeychainService from './KeychainService';
5+
import KeychainServiceDriverDummy from './KeychainServiceDriver.dummy';
6+
import KeychainServiceDriverElectron from './KeychainServiceDriver.electron';
7+
import KeychainServiceDriverNode from './KeychainServiceDriver.node';
8+
9+
interface SafeStorageMockOptions {
10+
isEncryptionAvailable?: ()=> boolean;
11+
encryptString?: (str: string)=> Promise<string|null>;
12+
decryptString?: (str: string)=> Promise<string|null>;
13+
}
14+
15+
const mockSafeStorage = ({ // Safe storage
16+
isEncryptionAvailable = jest.fn(() => true),
17+
encryptString = jest.fn(async s => (`e:${s}`)),
18+
decryptString = jest.fn(async s => s.substring(2)),
19+
}: SafeStorageMockOptions) => {
20+
shim.electronBridge = () => ({
21+
safeStorage: {
22+
isEncryptionAvailable,
23+
encryptString,
24+
decryptString,
25+
getSelectedStorageBackend: () => 'mock',
26+
},
27+
});
28+
};
29+
30+
const mockKeytar = () => {
31+
const storage = new Map<string, string>();
32+
33+
const keytarMock = {
34+
getPassword: jest.fn(async (key, client) => {
35+
return storage.get(`${client}--${key}`);
36+
}),
37+
setPassword: jest.fn(async (key, client, password) => {
38+
if (!password) throw new Error('Keytar doesn\'t support empty passwords.');
39+
storage.set(`${client}--${key}`, password);
40+
}),
41+
deletePassword: jest.fn(async (key, client) => {
42+
storage.delete(`${client}--${key}`);
43+
}),
44+
};
45+
shim.keytar = () => keytarMock;
46+
return keytarMock;
47+
};
48+
49+
const makeDrivers = () => [
50+
new KeychainServiceDriverElectron(Setting.value('appId'), Setting.value('clientId')),
51+
new KeychainServiceDriverNode(Setting.value('appId'), Setting.value('clientId')),
52+
];
53+
54+
describe('KeychainService', () => {
55+
beforeEach(async () => {
56+
await setupDatabaseAndSynchronizer(0);
57+
await switchClient(0);
58+
Setting.setValue('keychain.supported', 1);
59+
shim.electronBridge = null;
60+
shim.keytar = null;
61+
});
62+
63+
test('should copy keys from keytar to safeStorage', async () => {
64+
const keytarMock = mockKeytar();
65+
await KeychainService.instance().initialize(makeDrivers());
66+
67+
// Set a secure setting
68+
Setting.setValue('encryption.masterPassword', 'testing');
69+
await Setting.saveAll();
70+
71+
mockSafeStorage({});
72+
73+
await KeychainService.instance().initialize(makeDrivers());
74+
await Setting.load();
75+
expect(Setting.value('encryption.masterPassword')).toBe('testing');
76+
77+
await Setting.saveAll();
78+
79+
// For now, passwords should not be removed from old backends -- this allows
80+
// users to revert to an earlier version of Joplin without data loss.
81+
expect(keytarMock.deletePassword).not.toHaveBeenCalled();
82+
83+
expect(shim.electronBridge().safeStorage.encryptString).toHaveBeenCalled();
84+
expect(shim.electronBridge().safeStorage.encryptString).toHaveBeenCalledWith('testing');
85+
86+
await Setting.load();
87+
expect(Setting.value('encryption.masterPassword')).toBe('testing');
88+
});
89+
90+
test('should use keytar when safeStorage is unavailable', async () => {
91+
const keytarMock = mockKeytar();
92+
await KeychainService.instance().initialize(makeDrivers());
93+
94+
Setting.setValue('encryption.masterPassword', 'test-password');
95+
await Setting.saveAll();
96+
expect(keytarMock.setPassword).toHaveBeenCalledWith(
97+
`${Setting.value('appId')}.setting.encryption.masterPassword`,
98+
`${Setting.value('clientId')}@joplin`,
99+
'test-password',
100+
);
101+
102+
await Setting.load();
103+
expect(Setting.value('encryption.masterPassword')).toBe('test-password');
104+
});
105+
106+
test('should re-check for keychain support when a new driver is added', async () => {
107+
mockKeytar();
108+
mockSafeStorage({});
109+
Setting.setValue('keychain.supported', -1);
110+
111+
await KeychainService.instance().initialize([
112+
new KeychainServiceDriverDummy(Setting.value('appId'), Setting.value('clientId')),
113+
]);
114+
await KeychainService.instance().detectIfKeychainSupported();
115+
116+
expect(Setting.value('keychain.supported')).toBe(0);
117+
118+
// Should re-run the check after keytar and safeStorage are available.
119+
await KeychainService.instance().initialize(makeDrivers());
120+
await KeychainService.instance().detectIfKeychainSupported();
121+
expect(Setting.value('keychain.supported')).toBe(1);
122+
123+
// Should re-run the check if safeStorage and keytar are both no longer available.
124+
await KeychainService.instance().initialize([]);
125+
await KeychainService.instance().detectIfKeychainSupported();
126+
expect(Setting.value('keychain.supported')).toBe(0);
127+
});
128+
});

0 commit comments

Comments
 (0)