-1

I am implementing the Publish-Subscribe pattern.

I have class Broker and interfaces IPublisher, ISubscriber.

interface IPublisher {
    broker: Broker;
    publish(channel: Channel, data: object): void;
}

interface ISubscriber {
    notify(channel: Channel, data: object): void;
}

class Broker {
    channels: Map<Channel, Set<ISubscriber>>

    constructor() {
        this.channels = new Map();
    }

    subscribe(channel: Channel, subscriber: ISubscriber) {
        if (!this.channels.has(channel)) {
            this.channels.set(channel, new Set());
        }
        this.channels.get(channel).add(subscriber);
    }
    unsubscribe(channel: Channel, subscriber: ISubscriber) {
        if (!this.channels.has(channel)) {
            return;
        }
        this.channels.get(channel).delete(subscriber);
    }
    broadcast(channel: Channel, data: object) {
        if (!this.channels.has(channel)) {
            return;
        }
        this.channels.get(channel).forEach(subscriber => subscriber.notify(channel, data));
    }
}

I am wondering about security concerns. This design currently technically allows for publishers to publish to any channel in any Broker that they have a reference to without that Broker's authorization, and for the Broker to subscribe any subscriber to itself and notify them of any channel without that subscriber's permission.

is this a problem? I've considered adding a register(channel: Channel, publisher: IPublisher) method to Broker and a subscribe(broker: Broker, channel: Channel) method to ISubscriber, splitting Broker.channels into two independent maps .subscribers, publishers, with an error thrown if the Broker attempts to notify a subscriber that has not subscribed to it or if the publisher attempts to publish to a channel that has not been registered with the Broker. This will require changing ISubscriber.notify() and IPublisher.publish() to pass themselves to the Broker as well.

interface IPublisher {
    broker: Broker;
    publish(self: IPubllisher, channel: Channel, data: object): void;
}

interface ISubscriber {
    subscribe(broker: Broker, channel: Channel): void;
    notify(broker: Broker, channel: Channel, data: object): void;
}

class Broker {
    subscribers: Map<Channel, Set<ISubscriber>>
    publishers: Map<Channel, Set<IPublisher>>

    constructor() {
        this.subscribers = new Map();
        this.publishers = new Map();
    }
    register(channel: Channel, publisher: IPublisher) {
        if (!this.publishers.has(channel)) {
            this.publishers.set(channel, new Set());
        }
        this.publishers.get(channel).add(publisher);
    }
    subscribe(channel: Channel, subscriber: ISubscriber) {
        if (!this.subscribers.has(channel)) {
            this.subscribers.set(channel, new Set());
        }
        this.subscribers.get(channel).add(subscriber);
    }
    unsubscribe(channel: Channel, subscriber: ISubscriber) {
        if (!this.subscribers.has(channel)) {
            return;
        }
        this.subscribers.get(channel).delete(subscriber);
    }
    broadcast(publisher: IPublisher, channel: Channel, data: object) {
        if (!this.publishers.has(channel)) {
            return;
        }
        if (!this.publishers.get(channel).has(publisher)) {
            throw new Error("Publisher not registered for channel");
        }
        this.subscribers.get(channel).forEach(subscriber => subscriber.notify(this, channel, data));
    }
}

Is this worth it? Are there better approaches? Are there any other security concerns I should be aware of? Should I restrict access at the channel level as well, or allow any authorized entity to access any channel?

1 Answer 1

4

You should mainly handle security at the perimeter, not between entities of your code. The scheme you propose would add complexity without actually protecting against external attackers. It has a slight chance of catching some coding errors, but protecting against such errors should only be done where they would result in execution of malicious code with resulting privilege escalation.

0

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

Not the answer you're looking for? Browse other questions tagged or ask your own question.