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.

Sign up for GitHub

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

feat: revamp user storage encryption process #4981

Merged
merged 15 commits into from
Dec 3, 2024
Merged

feat: revamp user storage encryption process #4981

Show file tree
Hide file tree
Changes from 14 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
5042227
feat: re-encrypt batchGet results when they have different salts
mathieuartu Nov 26, 2024
72a99e3
add empty salt logic, keep backwards compatibility, add re-encryption…
mathieuartu Nov 29, 2024
fe830c4
Merge branch 'main' into feat/re_encrypt_data_if_salts_different
mathieuartu Nov 29, 2024
ac4dbda
fix: remove unused import
mathieuartu Nov 29, 2024
2c3923b
fix: remove unused param
mathieuartu Nov 29, 2024
97c860b
fix: new method in order to batch upsert already hashed and encrypted…
mathieuartu Nov 29, 2024
e6004fd
fix: PR feedbacks & improvements
mathieuartu Nov 29, 2024
e67adad
fix: use a shared salt instead of an empty one
mathieuartu Nov 29, 2024
4a483c5
Merge branch 'main' into feat/re_encrypt_data_if_salts_different
mathieuartu Dec 2, 2024
9f5ef50
un-1337 the UInt8Array construction
mathieuartu Dec 2, 2024
7d3a5ee
fix: salt comparison condition
mathieuartu Dec 2, 2024
1c8a4aa
Merge branch 'main' into feat/re_encrypt_data_if_salts_different
mathieuartu Dec 3, 2024
32621e1
feat: delete entries from user storage if un-decryptable
mathieuartu Dec 3, 2024
c7a10b9
Merge branch 'main' into feat/re_encrypt_data_if_salts_different
mathieuartu Dec 3, 2024
035dc21
Revert "feat: delete entries from user storage if un-decryptable"
mathieuartu Dec 3, 2024
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
19 changes: 10 additions & 9 deletions packages/profile-sync-controller/src/controllers/user-storage/__fixtures__/mockResponses.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,15 +58,16 @@ export async function createMockGetStorageResponse(
export async function createMockAllFeatureEntriesResponse(
dataArr: string[] = [MOCK_STORAGE_DATA],
): Promise<GetUserStorageAllFeatureEntriesResponse> {
return await Promise.all(
dataArr.map(async function (d) {
const encryptedData = await MOCK_ENCRYPTED_STORAGE_DATA(d);
return {
HashedKey: 'HASHED_KEY',
Data: encryptedData,
};
}),
);
const decryptedData = [];

for (const data of dataArr) {
decryptedData.push({
HashedKey: 'HASHED_KEY',
Data: await MOCK_ENCRYPTED_STORAGE_DATA(data),
});
}

return decryptedData;
}

/**
Expand Down
326 changes: 323 additions & 3 deletions packages/profile-sync-controller/src/controllers/user-storage/services.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
import encryption, { createSHA256Hash } from '../../shared/encryption';
import { SHARED_SALT } from '../../shared/encryption/constants';
import type { UserStorageFeatureKeys } from '../../shared/storage-schema';
import { USER_STORAGE_FEATURE_NAMES } from '../../shared/storage-schema';
import { createMockGetStorageResponse } from './__fixtures__';
import {
mockEndpointGetUserStorage,
mockEndpointUpsertUserStorage,
Expand All @@ -23,6 +25,8 @@ import {
upsertUserStorage,
deleteUserStorageAllFeatureEntries,
deleteUserStorage,
batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries,
batchDeleteUserStorageWithAlreadyHashedEntries,
} from './services';

describe('user-storage/services.ts - getUserStorage() tests', () => {
Expand Down Expand Up @@ -81,6 +85,70 @@ describe('user-storage/services.ts - getUserStorage() tests', () => {
mockGetUserStorage.done();
expect(result).toBeNull();
});

it('re-encrypts data if received entry was encrypted with a random salt, and saves it back to user storage', async () => {
const DECRYPED_DATA = 'data1';
const INITIAL_ENCRYPTED_DATA = {
HashedKey: 'entry1',
Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
};
// Encrypted with a random salt
const mockResponse = INITIAL_ENCRYPTED_DATA;

const mockGetUserStorage = await mockEndpointGetUserStorage(
`${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
{
status: 200,
body: JSON.stringify(mockResponse),
},
);

const mockUpsertUserStorage = mockEndpointUpsertUserStorage(
`${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
undefined,
async (requestBody) => {
if (typeof requestBody === 'string') {
return;
}

const isEncryptedUsingSharedSalt =
encryption.getSalt(requestBody.data).toString() ===
SHARED_SALT.toString();

expect(isEncryptedUsingSharedSalt).toBe(true);
},
);

const result = await actCallGetUserStorage();

mockGetUserStorage.done();
mockUpsertUserStorage.done();
expect(result).toBe(DECRYPED_DATA);
});

it('deletes entry if unable to decrypt data', async () => {
const badResponseData: GetUserStorageResponse = {
HashedKey: 'MOCK_HASH',
Data: 'Bad Encrypted Data',
};
const mockGetUserStorage = await mockEndpointGetUserStorage(
`${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
{
status: 200,
body: badResponseData,
},
);

const mockDeleteUserStorage = mockEndpointDeleteUserStorage(
`${USER_STORAGE_FEATURE_NAMES.notifications}.notification_settings`,
);

const result = await actCallGetUserStorage();

mockGetUserStorage.done();
expect(mockDeleteUserStorage.isDone()).toBe(true);
expect(result).toBeNull();
});
});

describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', () => {
Expand All @@ -103,6 +171,123 @@ describe('user-storage/services.ts - getUserStorageAllFeatureEntries() tests', (
expect(result).toStrictEqual([MOCK_STORAGE_DATA]);
});

it('re-encrypts data if received entries were encrypted with random salts, and saves it back to user storage', async () => {
// This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', '{ "hello": "world" }']]
// Each entry has been encrypted with a random salt, except for the last entry
// The last entry is used to test if the function can handle entries with both random salts and the shared salt
const mockResponse = [
{
HashedKey: 'entry1',
Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
},
{
HashedKey: 'entry2',
Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
},
await createMockGetStorageResponse(),
];

const mockGetUserStorageAllFeatureEntries =
await mockEndpointGetUserStorageAllFeatureEntries(
USER_STORAGE_FEATURE_NAMES.notifications,
{
status: 200,
body: JSON.stringify(mockResponse),
},
);

const mockBatchUpsertUserStorage = mockEndpointBatchUpsertUserStorage(
USER_STORAGE_FEATURE_NAMES.notifications,
undefined,
async (_uri, requestBody) => {
if (typeof requestBody === 'string') {
return;
}

const doEntriesHaveDifferentSalts =
encryption.getIfEntriesHaveDifferentSalts(
Object.entries(requestBody.data).map((entry) => entry[1] as string),
);

expect(doEntriesHaveDifferentSalts).toBe(false);

const doEntriesUseSharedSalt = Object.entries(requestBody.data).every(
([_entryKey, entryValue]) =>
encryption.getSalt(entryValue as string).toString() ===
SHARED_SALT.toString(),
);

expect(doEntriesUseSharedSalt).toBe(true);

const wereOnlyNonEmptySaltEntriesUploaded =
Object.entries(requestBody.data).length === 2;

expect(wereOnlyNonEmptySaltEntriesUploaded).toBe(true);
},
);

const result = await actCallGetUserStorageAllFeatureEntries();

mockGetUserStorageAllFeatureEntries.done();
mockBatchUpsertUserStorage.done();
expect(result).toStrictEqual(['data1', 'data2', MOCK_STORAGE_DATA]);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FUTURE - this tests have a lot of repetition, it would be nice to clean up these up so the tests clearly show us what we are testing instead of needing to read all this setup/acting.

});

it('deletes entries if unable to decrypt data', async () => {
// This corresponds to [['entry1', 'data1'], ['entry2', 'data2'], ['HASHED_KEY', 'Bad Encrypted Data']]
// Each entry has been encrypted with a random salt, except for the last entry
// The last entry is used to test if the function can handle entries with both random salts and the shared salt, mixed with bad data
const mockResponse = [
{
HashedKey: 'entry1',
Data: '{"v":"1","t":"scrypt","d":"HIu+WgFBCtKo6rEGy0R8h8t/JgXhzC2a3AF6epahGY2h6GibXDKxSBf6ppxM099Gmg==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
},
{
HashedKey: 'entry2',
Data: '{"v":"1","t":"scrypt","d":"3ioo9bxhjDjTmJWIGQMnOlnfa4ysuUNeLYTTmJ+qrq7gwI6hURH3ooUcBldJkHtvuQ==","o":{"N":131072,"r":8,"p":1,"dkLen":16},"saltLen":16}',
},
{
HashedKey: 'HASHED_KEY',
Data: 'Bad Encrypted Data',
},
];

const mockGetUserStorageAllFeatureEntries =
await mockEndpointGetUserStorageAllFeatureEntries(
USER_STORAGE_FEATURE_NAMES.notifications,
{
status: 200,
body: JSON.stringify(mockResponse),
},
);

const mockBatchUpsertUserStorage = mockEndpointBatchUpsertUserStorage(
USER_STORAGE_FEATURE_NAMES.notifications,
);

const mockBatchDeleteUserStorage = mockEndpointBatchDeleteUserStorage(
USER_STORAGE_FEATURE_NAMES.notifications,
undefined,
async (_uri, requestBody) => {
console.log('requestBody', requestBody);
if (typeof requestBody === 'string') {
return;
}

const expectedBody = ['HASHED_KEY'];

expect(requestBody.batch_delete).toStrictEqual(expectedBody);
},
);

const result = await actCallGetUserStorageAllFeatureEntries();

mockGetUserStorageAllFeatureEntries.done();
expect(mockBatchUpsertUserStorage.isDone()).toBe(true);
expect(mockBatchDeleteUserStorage.isDone()).toBe(true);
expect(result).toStrictEqual(['data1', 'data2']);
});

it('returns null if endpoint does not have entry', async () => {
const mockGetUserStorage =
await mockEndpointGetUserStorageAllFeatureEntries(
Expand Down Expand Up @@ -275,6 +460,79 @@ describe('user-storage/services.ts - batchUpsertUserStorage() tests', () => {
});
});

describe('user-storage/services.ts - batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries() tests', () => {
let dataToStore: [string, string][];
const getDataToStore = async (): Promise<[string, string][]> =>
(dataToStore ??= [
[
createSHA256Hash(`0x123${MOCK_STORAGE_KEY}`),
await encryption.encryptString(MOCK_STORAGE_DATA, MOCK_STORAGE_KEY),
],
[
createSHA256Hash(`0x456${MOCK_STORAGE_KEY}`),
await encryption.encryptString(MOCK_STORAGE_DATA, MOCK_STORAGE_KEY),
],
]);

const actCallBatchUpsertUserStorage = async () => {
return await batchUpsertUserStorageWithAlreadyHashedAndEncryptedEntries(
await getDataToStore(),
{
bearerToken: 'MOCK_BEARER_TOKEN',
path: USER_STORAGE_FEATURE_NAMES.accounts,
storageKey: MOCK_STORAGE_KEY,
},
);
};

it('invokes upsert endpoint with no errors', async () => {
const mockUpsertUserStorage = mockEndpointBatchUpsertUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
undefined,
async (_uri, requestBody) => {
if (typeof requestBody === 'string') {
return;
}

const expectedBody = Object.fromEntries(await getDataToStore());

expect(requestBody.data).toStrictEqual(expectedBody);
},
);

await actCallBatchUpsertUserStorage();

expect(mockUpsertUserStorage.isDone()).toBe(true);
});

it('throws error if unable to upsert user storage', async () => {
const mockUpsertUserStorage = mockEndpointBatchUpsertUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
{
status: 500,
},
);

await expect(actCallBatchUpsertUserStorage()).rejects.toThrow(
expect.any(Error),
);
mockUpsertUserStorage.done();
});

it('does nothing if empty data is provided', async () => {
const mockUpsertUserStorage =
mockEndpointBatchUpsertUserStorage('accounts_v2');

await batchUpsertUserStorage([], {
bearerToken: 'MOCK_BEARER_TOKEN',
path: 'accounts_v2',
storageKey: MOCK_STORAGE_KEY,
});

expect(mockUpsertUserStorage.isDone()).toBe(false);
});
});

describe('user-storage/services.ts - deleteUserStorage() tests', () => {
const actCallDeleteUserStorage = async () => {
return await deleteUserStorage({
Expand Down Expand Up @@ -446,12 +704,74 @@ describe('user-storage/services.ts - batchDeleteUserStorage() tests', () => {
});

it('does nothing if empty data is provided', async () => {
const mockDeleteUserStorage =
mockEndpointBatchDeleteUserStorage('accounts_v2');
const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
);

await batchDeleteUserStorage([], {
bearerToken: 'MOCK_BEARER_TOKEN',
path: 'accounts_v2',
path: USER_STORAGE_FEATURE_NAMES.accounts,
storageKey: MOCK_STORAGE_KEY,
});

expect(mockDeleteUserStorage.isDone()).toBe(false);
});
});

describe('user-storage/services.ts - batchDeleteUserStorageWithAlreadyHashedEntries() tests', () => {
const keysToDelete = [
createSHA256Hash(`0x123${MOCK_STORAGE_KEY}`),
createSHA256Hash(`0x456${MOCK_STORAGE_KEY}`),
];

const actCallBatchDeleteUserStorage = async () => {
return await batchDeleteUserStorageWithAlreadyHashedEntries(keysToDelete, {
bearerToken: 'MOCK_BEARER_TOKEN',
path: USER_STORAGE_FEATURE_NAMES.accounts,
storageKey: MOCK_STORAGE_KEY,
});
};

it('invokes upsert endpoint with no errors', async () => {
const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
undefined,
async (_uri, requestBody) => {
if (typeof requestBody === 'string') {
return;
}

expect(requestBody.batch_delete).toStrictEqual(keysToDelete);
},
);

await actCallBatchDeleteUserStorage();

expect(mockDeleteUserStorage.isDone()).toBe(true);
});

it('throws error if unable to upsert user storage', async () => {
const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
{
status: 500,
},
);

await expect(actCallBatchDeleteUserStorage()).rejects.toThrow(
expect.any(Error),
);
mockDeleteUserStorage.done();
});

it('does nothing if empty data is provided', async () => {
const mockDeleteUserStorage = mockEndpointBatchDeleteUserStorage(
USER_STORAGE_FEATURE_NAMES.accounts,
);

await batchDeleteUserStorage([], {
bearerToken: 'MOCK_BEARER_TOKEN',
path: USER_STORAGE_FEATURE_NAMES.accounts,
storageKey: MOCK_STORAGE_KEY,
});

Expand Down
Loading
Loading