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

Allow configuration of multiple moderator users to be invited to all rooms. #234

Merged
merged 4 commits into from
Dec 17, 2024
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
8 changes: 4 additions & 4 deletions config/default.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -26,10 +26,10 @@ idServerDomain: "vector.im"
# result in emails or other communications to the user.
idServerBrand: "vector-im"

# This is the user ID of the moderator for all public-facing rooms the bot creates.
# Note that this user will be granted power level 100 (the highest) in every room
# These are the user IDs of the moderators for all public-facing rooms the bot creates.
# Note that these users will be granted power level 100 (the highest) in every room
# and be invited.
moderatorUserId: "@moderator:example.org"
moderatorUserIds: ["@moderator:example.org"]

# Settings for how the bot should represent livestreams.
livestream:
Expand Down Expand Up @@ -337,4 +337,4 @@ metrics:
address: 127.0.0.1

# if set to `true` will prevent the conf-bot from sending live invites to email/matrix_ids
dry_run_enabled: false
dry_run_enabled: false
183 changes: 140 additions & 43 deletions spec/basic.spec.ts
Original file line number Diff line number Diff line change
@@ -1,49 +1,146 @@
import { E2ESetupTestTimeout, E2ETestEnv } from "./util/e2e-test";
import { describe, it, beforeEach, afterEach, expect } from "@jest/globals";

describe('Basic test setup', () => {
let testEnv: E2ETestEnv;
beforeEach(async () => {
testEnv = await E2ETestEnv.createTestEnv({
fixture: 'basic-conference',
});
const welcomeMsg = testEnv.waitForMessage();
await testEnv.setUp();
console.log((await welcomeMsg).event.content.body.startsWith('WECOME!'));
}, E2ESetupTestTimeout);
afterEach(() => {
return testEnv?.tearDown();
});
it('should start up successfully', async () => {
const { event } = await testEnv.sendAdminCommand('!conference status');
console.log(event.content.body);
// Check that we're generally okay.
expect(event.content.body).toMatch('Scheduled tasks yet to run: 0');
expect(event.content.body).toMatch('Schedule source healthy: true');
async function buildConference(testEnv: E2ETestEnv): Promise<void> {
let spaceBuilt,
supportRoomsBuilt,
conferenceBuilt = false;
const waitForFinish = new Promise<void>((resolve, reject) => {
const timeout = setTimeout(
() =>
reject(
new Error(
`Build incomplete. spaceBuild: ${spaceBuilt}, supportRoomsBuilt: ${supportRoomsBuilt}, conferenceBuilt: ${conferenceBuilt}`
)
),
30000
);
testEnv.adminClient.on("room.message", (_, event) => {
if (event.content.body.includes("Your conference's space is at")) {
spaceBuilt = true;
} else if (
event.content.body.includes("Support rooms have been created")
) {
supportRoomsBuilt = true;
} else if (event.content.body.includes("CONFERENCE BUILT")) {
conferenceBuilt = true;
}

if (spaceBuilt && supportRoomsBuilt && conferenceBuilt) {
resolve();
clearTimeout(timeout);
}
});
it('should be able to build successfully', async () => {
let spaceBuilt, supportRoomsBuilt, conferenceBuilt = false;
const waitForFinish = new Promise<void>((resolve, reject) => {
const timeout = setTimeout(() => reject(new Error(
`Build incomplete. spaceBuild: ${spaceBuilt}, supportRoomsBuilt: ${supportRoomsBuilt}, conferenceBuilt: ${conferenceBuilt}`
)), 30000);
testEnv.adminClient.on('room.message', (_, event) => {
if (event.content.body.includes("Your conference's space is at")) {
spaceBuilt = true;
} else if (event.content.body.includes("Support rooms have been created")) {
supportRoomsBuilt = true;
} else if (event.content.body.includes("CONFERENCE BUILT")) {
conferenceBuilt = true;
}

if (spaceBuilt && supportRoomsBuilt && conferenceBuilt) {
resolve();
clearTimeout(timeout);
}
})
});
await testEnv.sendAdminCommand('!conference build');
await waitForFinish;
// TODO: Now test that all the expected rooms are there.
});
await testEnv.sendAdminCommand("!conference build");
await waitForFinish;
}

function describeLocator(locator: any): string {
let out = `(${locator.conferenceId}) ${locator.kind}`;
for (let key of Object.keys(locator).sort()) {
if (key !== "conferenceId" && key !== "kind") {
out += ` ${key}=${locator[key]}`;
}
}
return out;
}

describe("Basic test setup", () => {
let testEnv: E2ETestEnv;
beforeEach(async () => {
testEnv = await E2ETestEnv.createTestEnv({
fixture: "basic-conference",
});
const welcomeMsg = testEnv.waitForMessage();
await testEnv.setUp();
console.log((await welcomeMsg).event.content.body.startsWith("WECOME!"));
}, E2ESetupTestTimeout);
afterEach(() => {
return testEnv?.tearDown();
});
it("should start up successfully", async () => {
const { event } = await testEnv.sendAdminCommand("!conference status");
console.log(event.content.body);
// Check that we're generally okay.
expect(event.content.body).toMatch("Scheduled tasks yet to run: 0");
expect(event.content.body).toMatch("Schedule source healthy: true");
});
it("should be able to build successfully", async () => {
await buildConference(testEnv);

// Now test that all the expected rooms are there.
// We will match against the 'locator' state events to identify the rooms.

const joinedRoomIds = await testEnv.confbotClient.getJoinedRooms();
console.debug("joined room IDs: ", joinedRoomIds);

const allLocators: string[] = [];
let roomsWithoutLocators = 0;

for (const joinedRoomId of joinedRoomIds) {
if (joinedRoomId == testEnv.opts.config?.managementRoom) {
// The management room is not interesting
continue;
}
try {
const roomLocator = await testEnv.confbotClient.getRoomStateEvent(
joinedRoomId,
"org.matrix.confbot.locator",
""
);
allLocators.push(describeLocator(roomLocator));
} catch (error) {
// This room doesn't have a locator
console.warn("room without locator: ", joinedRoomId);
roomsWithoutLocators += 1;
}
}

expect(allLocators.sort()).toMatchInlineSnapshot(`
[
"(test-conf) auditorium auditoriumId=main_stream",
"(test-conf) auditorium_backstage auditoriumId=main_stream",
"(test-conf) conference",
"(test-conf) conference_space",
"(test-conf) talk talkId=1",
]
`);
// TODO understand/explain why there are 6 rooms without locators
expect(roomsWithoutLocators).toBe(6);
});

it("should invite the moderator users to relevant rooms", async () => {
await buildConference(testEnv);

// List of rooms that we expect the moderator user to be invited to
const rooms = [
// `#test-conf:${testEnv.homeserver.domain}`, -- not invited to the root space
`#main_stream:${testEnv.homeserver.domain}`,
// `#main_stream-backstage:${testEnv.homeserver.domain}` -- not invited to the backstage,
`#talk-1:${testEnv.homeserver.domain}`,
];
const moderatorUserId = `@modbot:${testEnv.homeserver.domain}`;

for (let roomAlias of rooms) {
const roomId = await testEnv.confbotClient.resolveRoom(roomAlias);
let moderatorMembershipInRoom: any;
try {
moderatorMembershipInRoom =
await testEnv.confbotClient.getRoomStateEvent(
roomId,
"m.room.member",
moderatorUserId
);
} catch (err) {
const state = JSON.stringify(
await testEnv.confbotClient.getRoomState(roomId)
);
throw new Error(
`No m.room.member for ${moderatorUserId} in ${roomId} (${roomAlias}): ${state}`
);
}
expect(moderatorMembershipInRoom.membership).toBe("invite");
}
});
});
5 changes: 3 additions & 2 deletions spec/util/e2e-test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -203,7 +203,7 @@ export class E2ETestEnv {
}
},
},
moderatorUserId: `@modbot:${homeserver.domain}`,
moderatorUserIds: [`@modbot:${homeserver.domain}`],
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we attempt a test for this, should be easy to whip one up?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

wrote this now, a little fiddly but I think it's better than nothing? :-)

webserver: {
address: '0.0.0.0',
port: 0,
Expand All @@ -227,13 +227,14 @@ export class E2ETestEnv {
...providedConfig,
};
const conferenceBot = await ConferenceBot.start(config);
return new E2ETestEnv(homeserver, conferenceBot, adminUser.client, opts, tmpDir, config);
return new E2ETestEnv(homeserver, conferenceBot, adminUser.client, confBotOpts.client, opts, tmpDir, config);
}

private constructor(
public readonly homeserver: ComplementHomeServer,
public confBot: ConferenceBot,
public readonly adminClient: MatrixClient,
public readonly confbotClient: MatrixClient,
public readonly opts: Opts,
private readonly dataDir: string,
private readonly config: IConfig,
Expand Down
24 changes: 15 additions & 9 deletions src/Conference.ts
Original file line number Diff line number Diff line change
Expand Up @@ -394,7 +394,7 @@ export class Conference {
this.client,
mergeWithCreationTemplate(AUDITORIUM_BACKSTAGE_CREATION_TEMPLATE, {
room_alias_name: (new RoomAlias(alias)).localpart,
invite: [this.config.moderatorUserId],
invite: this.config.moderatorUserIds,
}),
);
await rootSpace.addChildRoom(roomId);
Expand Down Expand Up @@ -433,7 +433,7 @@ export class Conference {
subspace = await this.client.createSpace({
isPublic: true,
name: name,
invites: [this.config.moderatorUserId],
invites: this.config.moderatorUserIds,
});
this.subspaces[subspaceId] = subspace;

Expand All @@ -448,9 +448,11 @@ export class Conference {
roomId: subspace.roomId,
} as IStoredSubspace);

// Grants PL100 to the moderator in the subspace.
// Grants PL100 to the moderators in the subspace.
// We can't do this directly with `createSpace` unfortunately, as we could for plain rooms.
await this.client.setUserPowerLevel(this.config.moderatorUserId, subspace.roomId, 100);
for (let moderator of this.config.moderatorUserIds) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Technically we could convert createSpace to createRoom (with the necessary space state added in) and then just use the getSpace function to pull it as a space object, if we did prefer doing it that way?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I feel more happy leaving it as it is (in a 'known working' state) than messing TBH. It is a shame that these don't compose well though.

await this.client.setUserPowerLevel(moderator, subspace.roomId, 100);
}
} else {
subspace = this.subspaces[subspaceId];
}
Expand Down Expand Up @@ -481,7 +483,7 @@ export class Conference {
);
} else {
// Create a new interest room.
roomId = await safeCreateRoom(this.client, mergeWithCreationTemplate(SPECIAL_INTEREST_CREATION_TEMPLATE(this.config.moderatorUserId), {
roomId = await safeCreateRoom(this.client, mergeWithCreationTemplate(SPECIAL_INTEREST_CREATION_TEMPLATE(this.config.moderatorUserIds), {
creation_content: {
[RSC_CONFERENCE_ID]: this.id,
[RSC_SPECIAL_INTEREST_ID]: interestRoom.id,
Expand Down Expand Up @@ -571,7 +573,7 @@ export class Conference {

await parentSpace.addChildSpace(audSpace, { order: `auditorium-${auditorium.id}` });

const roomId = await safeCreateRoom(this.client, mergeWithCreationTemplate(AUDITORIUM_CREATION_TEMPLATE(this.config.moderatorUserId), {
const roomId = await safeCreateRoom(this.client, mergeWithCreationTemplate(AUDITORIUM_CREATION_TEMPLATE(this.config.moderatorUserIds), {
creation_content: {
[RSC_CONFERENCE_ID]: this.id,
[RSC_AUDITORIUM_ID]: auditorium.id,
Expand Down Expand Up @@ -629,7 +631,7 @@ export class Conference {
}

if (!this.talks[talk.id]) {
roomId = await safeCreateRoom(this.client, mergeWithCreationTemplate(TALK_CREATION_TEMPLATE(this.config.moderatorUserId), {
roomId = await safeCreateRoom(this.client, mergeWithCreationTemplate(TALK_CREATION_TEMPLATE(this.config.moderatorUserIds), {
name: talk.title,
creation_content: {
[RSC_CONFERENCE_ID]: this.id,
Expand Down Expand Up @@ -848,7 +850,9 @@ export class Conference {
// we'll be unable to do promotions/demotions in the future.
const pls = await this.client.getRoomStateEvent(roomId, "m.room.power_levels", "");
pls['users'][await this.client.getUserId()] = 100;
pls['users'][this.config.moderatorUserId] = 100;
for (let moderator of this.config.moderatorUserIds) {
pls['users'][moderator] = 100;
}
for (const userId of mxids) {
if (pls['users'][userId]) continue;
pls['users'][userId] = 50;
Expand Down Expand Up @@ -916,7 +920,9 @@ export class Conference {
this.membersInRooms[roomId] = joinedOrLeftMembers;
const total = new Set(Object.values(this.membersInRooms).flat());
total.delete(myUserId);
total.delete(this.config.moderatorUserId);
for (let moderator of this.config.moderatorUserIds) {
total.delete(moderator);
}
attendeeTotalGauge.set(total.size);
} catch (ex) {
LogService.warn("Conference", `Failed to recalculate room membership for ${roomId}`, ex);
Expand Down
7 changes: 6 additions & 1 deletion src/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,12 @@ export interface IConfig {
managementRoom: string;
idServerDomain?: string;
idServerBrand?: string;
moderatorUserId: string;

// Legacy option that causes a startup error when supplied.
// Removed in favour of `moderatorUserIds`.
moderatorUserId?: string;

moderatorUserIds: string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to still have the legacy moderatorUserId option, which we internally convert to moderatorUserIds at runtime just for anyone running an old config.

Or failing that, warn on startup that the old field is unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

refusing to start up seems like a reasonable option. I don't think we should force ourselves into too much backwards compat given we don't expect anyone else to be running this really. But refusing to start up sounds like a decent footgun-avoider.

livestream: {
auditoriumUrl: string;
talkUrl: string;
Expand Down
5 changes: 5 additions & 0 deletions src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,11 @@ export class ConferenceBot {
if (!RunMode[config.mode]) {
throw Error(`Incorrect mode '${config.mode}'`);
}

if (config.moderatorUserId !== undefined) {
throw new Error("The `moderatorUserId` config option has been replaced by `moderatorUserIds` that takes a list.");
}

const storage = new SimpleFsStorageProvider(path.join(config.dataPath, "bot.json"));
const client = await ConferenceMatrixClient.create(config, storage);
client.impersonateUserId(config.userId);
Expand Down
Loading
Loading