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

Use AS-specific room publication API #306

Merged
merged 13 commits into from
Dec 13, 2016
3 changes: 3 additions & 0 deletions config.sample.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,9 @@ ircService:
# A human-readable description string
# description: "Example.com IRC network"

# An ID for uniquely identifying this server amongst other servers being bridged.
# networkId: "example"

# The port to connect to. Optional.
port: 6697
# Whether to use SSL or not. Default: false.
Expand Down
2 changes: 1 addition & 1 deletion lib/DataStore.js
Original file line number Diff line number Diff line change
Expand Up @@ -170,7 +170,7 @@ DataStore.prototype.getAllChannelMappings = Promise.coroutine(function*() {
(e) => e.matrix_id === roomId
).map((e) => {
return {
domain: e.remote.domain,
networkId: this._serverMappings[e.remote.domain].getNetworkId(),
channel: e.remote.channel
}
});
Expand Down
1 change: 1 addition & 0 deletions lib/bridge/IrcBridge.js
Original file line number Diff line number Diff line change
Expand Up @@ -560,6 +560,7 @@ IrcBridge.prototype.getThirdPartyProtocol = function(protocol) {
},
instances: servers.map((server) => {
return {
network_id: server.getNetworkId(),
bot_user_id: this.getAppServiceUserId(),
desc: server.config.name || server.domain,
icon: server.config.icon,
Expand Down
4 changes: 3 additions & 1 deletion lib/bridge/IrcHandler.js
Original file line number Diff line number Diff line change
Expand Up @@ -571,9 +571,11 @@ IrcHandler.prototype.onMode = Promise.coroutine(function*(req, server, channel,
req.log.info("Not syncing publicity: shouldPublishRooms is false");
return Promise.resolve();
}
const key = this.ircBridge.publicitySyncer.getIRCVisMapKey(server.getNetworkId(), channel);

// Update the visibility for all rooms connected to this channel
return this.ircBridge.publicitySyncer.updateVisibilityMap(
true, server.domain + ' ' + channel, enabled
true, key, enabled
);
}

Expand Down
28 changes: 24 additions & 4 deletions lib/bridge/PublicitySyncer.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,10 @@ function PublicitySyncer(ircBridge) {
// should be resolved by keeping the matrix side as private as necessary
this._visibilityMap = {
mappings: {
//room_id: ['server #channel1', 'server channel2',...]
//room_id: ['funNetwork #channel1', 'funNetwork channel2',...]
},
channelIsSecret: {
// '$server $channel': true | false
// '$networkId $channel': true | false
},
roomVisibilities: {
// room_id: "private" | "public"
Expand Down Expand Up @@ -61,6 +61,22 @@ PublicitySyncer.prototype.initModes = Promise.coroutine(function*(server) {
});
});

/**
* Returns the key used when calling `updateVisibilityMap` for updating an IRC channel
* visibility mode (+s or -s).
* ```
* // Set channel on server to be +s
* const key = publicitySyncer.getIRCVisMapKey(server.getNetworkId(), channel);
* publicitySyncer.updateVisibilityMap(true, key, true);
* ```
* @param {string} networkId
* @param {string} channel
* @returns {string}
*/
PublicitySyncer.prototype.getIRCVisMapKey = function(networkId, channel) {
return networkId + ' ' + channel;
}

// This is used so that any updates to the visibility map will cause the syncer to
// reset a timer and begin counting down again to the eventual call to solve any
// inconsistencies in the visibility map.
Expand Down Expand Up @@ -125,7 +141,7 @@ PublicitySyncer.prototype._solveVisibility = Promise.coroutine(function*() {

roomIds.forEach((roomId) => {
this._visibilityMap.mappings[roomId] = mappings[roomId].map((mapping) => {
return mapping.domain + ' ' + mapping.channel
return getIRCVisMapKey(mapping.networkId, mapping.channel);
Copy link
Member

Choose a reason for hiding this comment

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

You forgot a this.

});
});

Expand Down Expand Up @@ -181,8 +197,12 @@ PublicitySyncer.prototype._solveVisibility = Promise.coroutine(function*() {
let currentState = this._visibilityMap.roomVisibilities[roomId];
let correctState = shouldBePrivate(roomId, []) ? 'private' : 'public';

// Use the server network ID of the first mapping
// 'funNetwork #channel1' => 'funNetwork'
const networkId = this._visibilityMap.mappings[roomId][0].split(' ')[0];
Copy link
Member

Choose a reason for hiding this comment

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

Is this._visibilityMap.mappings[roomId] guaranteed to be non-null? We seem to do a null guard on :140 but we don't here. But :127 makes me think that yes, it is guaranteed to be non-null. But then why the null guard on :140? Some clarification here would be good.

Copy link
Member

Choose a reason for hiding this comment

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

Also, ignoring the null guard issue, will there always be a mapped channel for this room ID? Now that I think is no, which would then NPE on this line.

Copy link
Contributor Author

@lukebarnard1 lukebarnard1 Dec 12, 2016

Choose a reason for hiding this comment

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

The null guard shouldn't be needed, you're right (it didn't originate for any particular reason, I checked in it's blame history). And yes the mappings are guaranteed to be set for a room, and contain at least one channel mapping otherwise that room wouldn't be in the RoomStore, and thus wouldn't be returned by getAllChannelMappings.

But I'd be happy to null guard just in case.


if (currentState !== correctState) {
return cli.setRoomDirectoryVisibility(roomId, correctState).then(
return cli.setRoomDirectoryVisibilityAppService(networkId, roomId, correctState).then(
Copy link
Member

Choose a reason for hiding this comment

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

How does this work for the existing Freenode rooms on matrix.org which are not in a networked list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Much in the same way it would work for those not in the main list.

() => {
// Update cache
this._visibilityMap.roomVisibilities[roomId] = correctState;
Expand Down
3 changes: 3 additions & 0 deletions lib/config/schema.yml
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,9 @@ properties:
type: "string"
description:
type: "string"
networkId:
type: "string"
pattern: "^[a-zA-Z0-9]+$"
icon:
type: "string"
quitDebounce:
Expand Down
9 changes: 9 additions & 0 deletions lib/irc/IrcServer.js
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,15 @@ IrcServer.prototype.getReadableName = function() {
return '';
}

/**
* Returns the network ID of this server, which should be unique across all
* IrcServers on the bridge. Defaults to the domain of this IrcServer.
* @return {string} this.config.networkId || this.domain
*/
Copy link
Member

Choose a reason for hiding this comment

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

JSDoc please.

IrcServer.prototype.getNetworkId = function() {
return this.config.networkId || this.domain;
}

/**
* Returns whether the server is configured to wait getQuitDebounceDelayMs before
* parting a user that has disconnected due to a net-split.
Expand Down