Skip to content

Conversation

kaylendog
Copy link
Contributor

@kaylendog kaylendog commented Sep 25, 2025

Checklist

@kaylendog
Copy link
Contributor Author

Two potential considerations:

  • What do clients without the flag enabled see when using a room with state event encryption enabled?
  • Can clients without the flag enabled still send encrypted state events if they are sent in a room with state encryption enabled?

@Half-Shot
Copy link
Member

Can clients without the flag enabled still send encrypted state events if they are sent in a room with state encryption enabled?

IMO it would make sense that clients with this flag enabled can create / enable rooms to have encrypted state, but all supporting clients regardless of features should be able to interact in these rooms. So, they should probably attempt to send encrypted state if the room requires it.

@kaylendog
Copy link
Contributor Author

Can clients without the flag enabled still send encrypted state events if they are sent in a room with state encryption enabled?

IMO it would make sense that clients with this flag enabled can create / enable rooms to have encrypted state, but all supporting clients regardless of features should be able to interact in these rooms. So, they should probably attempt to send encrypted state if the room requires it.

I agree! The last commit I made locks MatrixClient.enableEncryptedStateEvents to true, but the toggle switch on the create room dialogue is still locked behind the lab flag.

@kaylendog
Copy link
Contributor Author

It will be quite challenging to improve coverage on src/createRoom.ts, since it relies on an event listener attached to RoomState. There doesn't seem to be any existing infrastructure for mocking this behaviour.

Copy link
Member

@florianduros florianduros left a comment

Choose a reason for hiding this comment

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

First quick review :)

Comment on lines 351 to 355
const timeout = setTimeout(() => {
logger.warn("Timed out while waiting for room to enable encryption");
roomState.off(RoomStateEvent.Update, onRoomStateUpdate);
resolve();
}, 3000);
Copy link
Member

Choose a reason for hiding this comment

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

If we have to wait for this long, we need a spinner or some feedback to the user

Copy link
Contributor Author

@kaylendog kaylendog Oct 8, 2025

Choose a reason for hiding this comment

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

There is an existing spinner, I could force it to be shown If state event encryption is enabled?


const roomState = resolvedRoom.getLiveTimeline().getState(Direction.Forward)!;

// Soft fail, since the room will still be functional if the initial state is not encrypted.
Copy link
Member

Choose a reason for hiding this comment

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

Also if it fails, how do we inform the user?

Copy link
Member

@richvdh richvdh left a comment

Choose a reason for hiding this comment

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

a few drive-by comments; sorry, haven't done a full review

});
});

it("creates a private with encryption", async () => {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
it("creates a private with encryption", async () => {
it("creates a private room with encryption", async () => {

Comment on lines 373 to 383
// Set room avatar
if (opts.avatar) {
let url: string;
if (opts.avatar instanceof File) {
({ content_uri: url } = await client.uploadContent(opts.avatar));
} else {
url = opts.avatar;
}
await client.sendStateEvent(roomId, EventType.RoomAvatar, { url }, "");
}
}
Copy link
Member

Choose a reason for hiding this comment

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

looks like you could do with a test that hits this codepath

spinner?: boolean;
guestAccess?: boolean;
encryption?: boolean;
stateEncryption?: boolean;
Copy link
Member

Choose a reason for hiding this comment

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

new public field needs doc-comment, please

Comment on lines 343 to 366
await new Promise<void>((resolve, reject) => {
if (resolvedRoom.hasEncryptionStateEvent()) {
return resolve();
}

const roomState = resolvedRoom.getLiveTimeline().getState(Direction.Forward)!;

// Soft fail, since the room will still be functional if the initial state is not encrypted.
const timeout = setTimeout(() => {
logger.warn("Timed out while waiting for room to enable encryption");
roomState.off(RoomStateEvent.Update, onRoomStateUpdate);
resolve();
}, 3000);

const onRoomStateUpdate = (state: RoomState): void => {
if (state.getStateEvents(EventType.RoomEncryption, "")) {
roomState.off(RoomStateEvent.Update, onRoomStateUpdate);
clearTimeout(timeout);
resolve();
}
};

roomState.on(RoomStateEvent.Update, onRoomStateUpdate);
});
Copy link
Member

Choose a reason for hiding this comment

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

the createRoom function is way too long and complicated. Factor this bit (at least) out to a separate function?

});
}

let stateEncryptedOpts: ICreateRoomOpts | undefined;
Copy link
Member

Choose a reason for hiding this comment

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

having a separate copy of the create opts feels like a confusing way to do things.

I think really, it's incorrect to pass the name in the ICreateRoomOpts (which is explicitly "a list of options to pass to the /createRoom API."); instead we should add a new name property to IOpts. (We could also add a sanity-check that throws an exception if someone attempts to set ICreateRoomOpts.name.) Suggest pulling this out to a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pulled it out here: #30981

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants