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

Add keysharing on invites to File Tree Spaces #1744

Merged
merged 2 commits into from
Jun 22, 2021
Merged
Show file tree
Hide file tree
Changes from all 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
59 changes: 55 additions & 4 deletions spec/unit/models/MSC3089TreeSpace.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -105,7 +105,7 @@ describe("MSC3089TreeSpace", () => {
return Promise.resolve();
});
client.invite = fn;
await tree.invite(target, false);
await tree.invite(target, false, false);
expect(fn).toHaveBeenCalledTimes(1);
});

Expand All @@ -118,7 +118,7 @@ describe("MSC3089TreeSpace", () => {
return Promise.resolve();
});
client.invite = fn;
await tree.invite(target, false);
await tree.invite(target, false, false);
expect(fn).toHaveBeenCalledTimes(2);
});

Expand All @@ -131,7 +131,7 @@ describe("MSC3089TreeSpace", () => {
});
client.invite = fn;
try {
await tree.invite(target, false);
await tree.invite(target, false, false);

// noinspection ExceptionCaughtLocallyJS
throw new Error("Failed to fail");
Expand Down Expand Up @@ -159,10 +159,61 @@ describe("MSC3089TreeSpace", () => {
{ invite: (userId) => fn(tree.roomId, userId) } as MSC3089TreeSpace,
];

await tree.invite(target, true);
await tree.invite(target, true, false);
expect(fn).toHaveBeenCalledTimes(4);
});

it('should share keys with invitees', async () => {
const target = targetUser;
const sendKeysFn = jest.fn().mockImplementation((inviteRoomId: string, userIds: string[]) => {
expect(inviteRoomId).toEqual(roomId);
expect(userIds).toMatchObject([target]);
return Promise.resolve();
});
client.invite = () => Promise.resolve(); // we're not testing this here - see other tests
client.sendSharedHistoryKeys = sendKeysFn;

// Mock the history check as best as possible
const historyVis = "shared";
const historyFn = jest.fn().mockImplementation((eventType: string, stateKey?: string) => {
// We're not expecting a super rigid test: the function that calls this internally isn't
// really being tested here.
expect(eventType).toEqual(EventType.RoomHistoryVisibility);
expect(stateKey).toEqual("");
return { getContent: () => ({ history_visibility: historyVis }) }; // eslint-disable-line camelcase
});
room.currentState.getStateEvents = historyFn;

// Note: inverse test is implicit from other tests, which disable the call stack of this
// test in order to pass.
await tree.invite(target, false, true);
expect(sendKeysFn).toHaveBeenCalledTimes(1);
expect(historyFn).toHaveBeenCalledTimes(1);
});

it('should not share keys with invitees if inappropriate history visibility', async () => {
const target = targetUser;
const sendKeysFn = jest.fn().mockImplementation((inviteRoomId: string, userIds: string[]) => {
expect(inviteRoomId).toEqual(roomId);
expect(userIds).toMatchObject([target]);
return Promise.resolve();
});
client.invite = () => Promise.resolve(); // we're not testing this here - see other tests
client.sendSharedHistoryKeys = sendKeysFn;

const historyVis = "joined"; // NOTE: Changed.
const historyFn = jest.fn().mockImplementation((eventType: string, stateKey?: string) => {
expect(eventType).toEqual(EventType.RoomHistoryVisibility);
expect(stateKey).toEqual("");
return { getContent: () => ({ history_visibility: historyVis }) }; // eslint-disable-line camelcase
});
room.currentState.getStateEvents = historyFn;

await tree.invite(target, false, true);
expect(sendKeysFn).toHaveBeenCalledTimes(0);
expect(historyFn).toHaveBeenCalledTimes(1);
});

async function evaluatePowerLevels(pls: any, role: TreePermissions, expectedPl: number) {
makePowerLevels(pls);
const fn = jest.fn()
Expand Down
2 changes: 1 addition & 1 deletion src/crypto/algorithms/megolm.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ import {
import { WITHHELD_MESSAGES } from '../OlmDevice';

// determine whether the key can be shared with invitees
function isRoomSharedHistory(room) {
export function isRoomSharedHistory(room) {
const visibilityEvent = room.currentState &&
room.currentState.getStateEvents("m.room.history_visibility", "");
// NOTE: if the room visibility is unset, it would normally default to
Expand Down
22 changes: 18 additions & 4 deletions src/models/MSC3089TreeSpace.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ import {
} from "../utils";
import { MSC3089Branch } from "./MSC3089Branch";
import promiseRetry from "p-retry";
import { isRoomSharedHistory } from "../crypto/algorithms/megolm";

/**
* The recommended defaults for a tree space's power levels. Note that this
Expand Down Expand Up @@ -120,15 +121,28 @@ export class MSC3089TreeSpace {
* @param {string} userId The user ID to invite.
* @param {boolean} andSubspaces True (default) to invite the user to all
* directories/subspaces too, recursively.
* @param {boolean} shareHistoryKeys True (default) to share encryption keys
* with the invited user. This will allow them to decrypt the events (files)
* in the tree. Keys will not be shared if the room is lacking appropriate
* history visibility (by default, history visibility is "shared" in trees,
* which is an appropriate visibility for these purposes).
* @returns {Promise<void>} Resolves when complete.
*/
public invite(userId: string, andSubspaces = true): Promise<void> {
// TODO: [@@TR] Share keys
public invite(userId: string, andSubspaces = true, shareHistoryKeys = true): Promise<void> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Perhaps consider an options object, instead of a growing sequence of booleans which all look the same at the call site... just a suggestion though. 😇

Copy link
Member Author

Choose a reason for hiding this comment

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

will keep it under advisement for the proof of concept :p

There might still be one more option...

const promises: Promise<void>[] = [this.retryInvite(userId)];
if (andSubspaces) {
promises.push(...this.getDirectories().map(d => d.invite(userId, andSubspaces)));
promises.push(...this.getDirectories().map(d => d.invite(userId, andSubspaces, shareHistoryKeys)));
}
return Promise.all(promises).then(); // .then() to coerce types
return Promise.all(promises).then(() => {
// Note: key sharing is default on because for file trees it is relatively important that the invite
// target can actually decrypt the files. The implied use case is that by inviting a user to the tree
// it means the sender would like the receiver to view/download the files contained within, much like
// sharing a folder in other circles.
if (shareHistoryKeys && isRoomSharedHistory(this.room)) {
// noinspection JSIgnoredPromiseFromCall - we aren't concerned as much if this fails.
this.client.sendSharedHistoryKeys(this.roomId, [userId]);
}
});
}

private retryInvite(userId: string): Promise<void> {
Expand Down