Discourse-presence improvements

- Added guardian checks to endpoint
- Added security to messagebus publishing
- Added specs for security measures
- Moved all logic into component
- Stop sending ‘keepAlive’ messages if the user stops editing for more then 2 minutes

- Enable plugin by default

plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6

@@ -1,14 +1,138 @@
+import { ajax } from 'discourse/lib/ajax';
+import { observes, on }  from 'ember-addons/ember-computed-decorators';
 import computed from 'ember-addons/ember-computed-decorators';
+import pageVisible from 'discourse/lib/page-visible';
 
 export default Ember.Component.extend({
   composer: Ember.inject.controller(),
 
-  @computed('composer.presenceUsers', 'currentUser.id')
+  // Passed in variables
+  action: null,
+  post: null,
+  topic: null,
+  reply: null,
+
+  // Internal variables
+  oldPresenceState: null,
+  presenceState: null,
+  keepAliveTimer: null,
+  messageBusChannel: null,
+  presenceUsers: null,
+
+  @on('didInsertElement')
+  composerOpened(){
+    this.updateStateObject();
+  },
+
+  @on('willDestroyElement')
+  composerClosing(){
+    this.updateStateObject(true);
+  },
+
+  @observes('action', 'post', 'topic')
+  composerStateChanged(){
+    Ember.run.once(this, 'updateStateObject');
+  },
+
+  updateStateObject(isClosing = false){
+    var stateObject = null;
+
+    if(!isClosing && this.shouldSharePresence(this.get('action'))){
+      stateObject = {};
+
+      stateObject.action = this.get('action');
+
+      // Add some context if we're editing or replying
+      switch(stateObject.action){
+        case 'edit':
+          stateObject.post_id = this.get('post.id');
+          break;
+        case 'reply':
+          stateObject.topic_id = this.get('topic.id');
+          break;
+        default:
+          break; // createTopic or privateMessage
+      }
+    }
+
+    this.set('oldPresenceState', this.get('presenceState'));
+    this.set('presenceState', stateObject);
+  },
+
+  shouldSharePresence(action){
+    return ['edit','reply'].includes(action);
+  },
+
+  @observes('presenceState')
+  presenceStateChanged(){
+    if(this.get('messageBusChannel')){
+      this.messageBus.unsubscribe(this.get('messageBusChannel'));
+      this.set('messageBusChannel', null);
+    }
+
+    this.set('presenceUsers', []);
+
+    ajax('/presence/publish', {
+      type: 'POST',
+      data: {
+        response_needed: true,
+        previous: this.get('oldPresenceState'),
+        current: this.get('presenceState')
+      }
+    }).then((data) => {
+      const messageBusChannel = data['messagebus_channel'];
+      if(messageBusChannel){
+        const users = data['users'];
+        const messageBusId = data['messagebus_id'];
+        this.set('presenceUsers', users);
+        this.set('messageBusChannel', messageBusChannel);
+        this.messageBus.subscribe(messageBusChannel, message => {
+          this.set('presenceUsers', message['users']);
+        }, messageBusId);
+      }
+    }).catch((error) => {
+      // This isn't a critical failure, so don't disturb the user
+      console.error("Error publishing composer status", error);
+    });
+
+    Ember.run.cancel(this.get('keepAliveTimer'));
+    if(this.shouldSharePresence(this.get('presenceState.action'))){
+      // Send presence data every 10 seconds
+      this.set('keepAliveTimer', Ember.run.later(this, 'keepPresenceAlive', 10000));
+    }
+  },
+
+  keepPresenceAlive(){
+    // If we're not replying or editing,
+    // don't update anything, and don't schedule this task again
+    if(!this.shouldSharePresence(this.get('presenceState.action'))){
+      return;
+    }
+
+    const browserInFocus = pageVisible();
+
+    // Only send the keepalive message if the browser has focus
+    if(browserInFocus){
+      ajax('/presence/publish', {
+        type: 'POST',
+        data: { current: this.get('presenceState') }
+      }).catch((error) => {
+        // This isn't a critical failure, so don't disturb the user
+        console.error("Error publishing composer status", error);
+      });
+    }
+
+    // Schedule again in another 10 seconds
+    Ember.run.cancel(this.get('keepAliveTimer'));
+    this.set('keepAliveTimer', Ember.run.later(this, 'keepPresenceAlive', 10000));
+  },
+
+  @computed('presenceUsers', 'currentUser.id')
   users(presenceUsers, currentUser_id){
     return (presenceUsers || []).filter(user => user.id !== currentUser_id);
   },
 
-  @computed('composer.presenceState.action')
+  @computed('presenceState.action')
   isReply(action){
     return action === 'reply';
   },

plugins/discourse-presence/assets/javascripts/discourse/initializers/composer-controller-presence.js.es6

@@ -1,129 +0,0 @@
-import { ajax } from 'discourse/lib/ajax';
-import { observes}  from 'ember-addons/ember-computed-decorators';
-import { withPluginApi } from 'discourse/lib/plugin-api';
-import pageVisible from 'discourse/lib/page-visible';
-
-function initialize(api) {
-  api.modifyClass('controller:composer', {
-
-    oldPresenceState: { compose_state: 'closed' },
-    presenceState: { compose_state: 'closed' },
-    keepAliveTimer: null,
-    messageBusChannel: null,
-    presenceUsers: null,
-
-    @observes('model.composeState', 'model.action', 'model.post', 'model.topic')
-    openStatusChanged(){
-      Ember.run.once(this, 'updateStateObject');
-    },
-
-    updateStateObject(){
-      const composeState = this.get('model.composeState');
-
-      const stateObject = {
-        compose_state: composeState ? composeState : 'closed'
-      };
-
-      if(stateObject.compose_state === 'open'){
-        stateObject.action = this.get('model.action');
-
-        // Add some context if we're editing or replying
-        switch(stateObject.action){
-          case 'edit':
-            stateObject.post_id = this.get('model.post.id');
-            break;
-          case 'reply':
-            stateObject.topic_id = this.get('model.topic.id');
-            break;
-          default:
-            break; // createTopic or privateMessage
-        }
-      }
-
-      this.set('oldPresenceState', this.get('presenceState'));
-      this.set('presenceState', stateObject);
-    },
-
-    shouldSharePresence(){
-      const isOpen = this.get('presenceState.compose_state') !== 'open';
-      const isEditing = ['edit','reply'].includes(this.get('presenceState.action'));
-      return isOpen && isEditing;
-    },
-
-    @observes('presenceState')
-    presenceStateChanged(){
-      if(this.get('messageBusChannel')){
-        this.messageBus.unsubscribe(this.get('messageBusChannel'));
-        this.set('messageBusChannel', null);
-      }
-
-      this.set('presenceUsers', []);
-
-      ajax('/presence/publish/', {
-        type: 'POST',
-        data: {
-          response_needed: true,
-          previous: this.get('oldPresenceState'),
-          current: this.get('presenceState')
-        }
-      }).then((data) => {
-        const messageBusChannel = data['messagebus_channel'];
-        if(messageBusChannel){
-          const users = data['users'];
-          const messageBusId = data['messagebus_id'];
-          this.set('presenceUsers', users);
-          this.set('messageBusChannel', messageBusChannel);
-          this.messageBus.subscribe(messageBusChannel, message => {
-            this.set('presenceUsers', message['users']);
-          }, messageBusId);
-        }
-      }).catch((error) => {
-        // This isn't a critical failure, so don't disturb the user
-        console.error("Error publishing composer status", error);
-      });
-
-
-      Ember.run.cancel(this.get('keepAliveTimer'));
-      if(this.shouldSharePresence()){
-        // Send presence data every 10 seconds
-        this.set('keepAliveTimer', Ember.run.later(this, 'keepPresenceAlive', 10000));
-      }
-    },
-
-
-
-    keepPresenceAlive(){
-      // If the composer isn't open, or we're not editing,
-      // don't update anything, and don't schedule this task again
-      if(!this.shouldSharePresence()){
-        return;
-      }
-
-      // Only send the keepalive message if the browser has focus
-      if(pageVisible()){
-        ajax('/presence/publish/', {
-          type: 'POST',
-          data: { current: this.get('presenceState') }
-        }).catch((error) => {
-          // This isn't a critical failure, so don't disturb the user
-          console.error("Error publishing composer status", error);
-        });
-      }
-
-      // Schedule again in another 30 seconds
-      Ember.run.cancel(this.get('keepAliveTimer'));
-      this.set('keepAliveTimer', Ember.run.later(this, 'keepPresenceAlive', 10000));
-    }
-
-  });
-}
-
-export default {
-  name: "composer-controller-presence",
-  after: "message-bus",
-
-  initialize(container) {
-    const siteSettings = container.lookup('site-settings:main');
-    if (siteSettings.presence_enabled) withPluginApi('0.8.9', initialize);
-  }
-};

plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs

@@ -1 +1,6 @@
-{{composer-presence-display}}
+{{composer-presence-display 
+      action=model.action
+      post=model.post
+      topic=model.topic
+      reply=model.reply
+    }}

plugins/discourse-presence/config/settings.yml

@@ -1,4 +1,4 @@
 plugins:
   presence_enabled:
-    default: false
+    default: true
     client: true

plugins/discourse-presence/plugin.rb

@@ -50,12 +50,27 @@ after_initialize do
     end
 
     def self.publish(type, id)
+      topic =
+          if type == 'post'
+            Post.find_by(id: id).topic
+          else
+            Topic.find_by(id: id)
+          end
+
       users = get_users(type, id)
       serialized_users = users.map { |u| BasicUserSerializer.new(u, root: false) }
       message = {
         users: serialized_users
       }
-      MessageBus.publish(get_messagebus_channel(type, id), message.as_json)
+
+      messagebus_channel = get_messagebus_channel(type, id)
+      if topic.archetype == Archetype.private_message
+        user_ids = User.where('admin or moderator').pluck(:id)
+        user_ids += topic.allowed_users.pluck(:id)
+        MessageBus.publish(messagebus_channel, message.as_json, user_ids: user_ids)
+      else
+        MessageBus.publish(messagebus_channel, message.as_json, group_ids: topic.secure_group_ids)
+      end
 
       users
     end
@@ -86,17 +101,25 @@ after_initialize do
 
     def publish
       data = params.permit(:response_needed,
-              current: [:compose_state, :action, :topic_id, :post_id],
-              previous: [:compose_state, :action, :topic_id, :post_id]
+              current: [:action, :topic_id, :post_id],
+              previous: [:action, :topic_id, :post_id]
               )
 
       if data[:previous] &&
-          data[:previous][:compose_state] == 'open' &&
           data[:previous][:action].in?(['edit', 'reply'])
 
         type = data[:previous][:post_id] ? 'post' : 'topic'
         id = data[:previous][:post_id] ? data[:previous][:post_id] : data[:previous][:topic_id]
 
+        topic =
+          if type == 'post'
+            Post.find_by(id: id).topic
+          else
+            Topic.find_by(id: id)
+          end
+
+        guardian.ensure_can_see!(topic)
+
         any_changes = false
         any_changes ||= Presence::PresenceManager.remove(type, id, current_user.id)
         any_changes ||= Presence::PresenceManager.cleanup(type, id)
@@ -105,12 +128,20 @@ after_initialize do
       end
 
       if data[:current] &&
-          data[:current][:compose_state] == 'open' &&
           data[:current][:action].in?(['edit', 'reply'])
 
         type = data[:current][:post_id] ? 'post' : 'topic'
         id = data[:current][:post_id] ? data[:current][:post_id] : data[:current][:topic_id]
 
+        topic =
+          if type == 'post'
+            Post.find_by!(id: id).topic
+          else
+            Topic.find_by!(id: id)
+          end
+
+        guardian.ensure_can_see!(topic)
+
         any_changes = false
         any_changes ||= Presence::PresenceManager.add(type, id, current_user.id)
         any_changes ||= Presence::PresenceManager.cleanup(type, id)

plugins/discourse-presence/spec/presence_controller_spec.rb

@@ -10,9 +10,14 @@ describe ::Presence::PresencesController, type: :request do
   let(:user2) { Fabricate(:user) }
   let(:user3) { Fabricate(:user) }
 
+  let(:post1) { Fabricate(:post) }
+  let(:post2) { Fabricate(:post) }
+
   after(:each) do
-    $redis.del('presence:post:22')
-    $redis.del('presence:post:11')
+    $redis.del("presence:topic:#{post1.topic.id}")
+    $redis.del("presence:topic:#{post2.topic.id}")
+    $redis.del("presence:post:#{post1.id}")
+    $redis.del("presence:post:#{post2.id}")
   end
 
   context 'when not logged in' do
@@ -30,23 +35,38 @@ describe ::Presence::PresencesController, type: :request do
       expect { post '/presence/publish.json' }.not_to raise_error
     end
 
+    it "uses guardian to secure endpoint" do
+      # Private message
+      private_post = Fabricate(:private_message_post)
+      post '/presence/publish.json', current: { action: 'edit', post_id: private_post.id }
+      expect(response.code.to_i).to eq(403)
+
+      # Secure category
+      group = Fabricate(:group)
+      category = Fabricate(:private_category, group: group)
+      private_topic = Fabricate(:topic, category: category)
+
+      post '/presence/publish.json', current: { action: 'edit', topic_id: private_topic.id }
+      expect(response.code.to_i).to eq(403)
+    end
+
     it "returns a response when requested" do
       messages = MessageBus.track_publish do
-        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: 22 }, response_needed: true
+        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: post1.id }, response_needed: true
       end
 
       expect(messages.count).to eq (1)
 
       data = JSON.parse(response.body)
 
-      expect(data['messagebus_channel']).to eq('/presence/post/22')
-      expect(data['messagebus_id']).to eq(MessageBus.last_id('/presence/post/22'))
+      expect(data['messagebus_channel']).to eq("/presence/post/#{post1.id}")
+      expect(data['messagebus_id']).to eq(MessageBus.last_id("/presence/post/#{post1.id}"))
       expect(data['users'][0]["id"]).to eq(user1.id)
     end
 
     it "doesn't return a response when not requested" do
       messages = MessageBus.track_publish do
-        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: 22 }
+        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: post1.id }
       end
 
       expect(messages.count).to eq (1)
@@ -57,20 +77,20 @@ describe ::Presence::PresencesController, type: :request do
 
     it "doesn't send duplicate messagebus messages" do
       messages = MessageBus.track_publish do
-        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: 22 }
+        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: post1.id }
       end
       expect(messages.count).to eq (1)
 
       messages = MessageBus.track_publish do
-        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: 22 }
+        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: post1.id }
       end
       expect(messages.count).to eq (0)
     end
 
     it "clears 'previous' state when supplied" do
       messages = MessageBus.track_publish do
-        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: 22 }
-        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: 11 }, previous: { compose_state: 'open', action: 'edit', post_id: 22 }
+        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: post1.id }
+        post '/presence/publish.json', current: { compose_state: 'open', action: 'edit', post_id: post2.id }, previous: { compose_state: 'open', action: 'edit', post_id: post1.id }
       end
       expect(messages.count).to eq (3)
     end

plugins/discourse-presence/spec/presence_manager_spec.rb

@@ -7,58 +7,100 @@ describe ::Presence::PresenceManager do
   let(:user3) { Fabricate(:user) }
   let(:manager) { ::Presence::PresenceManager }
 
+  let(:post1) { Fabricate(:post) }
+  let(:post2) { Fabricate(:post) }
+
   after(:each) do
-    $redis.del('presence:post:22')
-    $redis.del('presence:post:11')
+    $redis.del("presence:topic:#{post1.topic.id}")
+    $redis.del("presence:topic:#{post2.topic.id}")
+    $redis.del("presence:post:#{post1.id}")
+    $redis.del("presence:post:#{post2.id}")
   end
 
   it 'adds, removes and lists users correctly' do
-    expect(manager.get_users('post', 22).count).to eq(0)
+    expect(manager.get_users('post', post1.id).count).to eq(0)
 
-    expect(manager.add('post', 22, user1.id)).to be true
-    expect(manager.add('post', 22, user2.id)).to be true
-    expect(manager.add('post', 11, user3.id)).to be true
+    expect(manager.add('post', post1.id, user1.id)).to be true
+    expect(manager.add('post', post1.id, user2.id)).to be true
+    expect(manager.add('post', post2.id, user3.id)).to be true
 
-    expect(manager.get_users('post', 22).count).to eq(2)
-    expect(manager.get_users('post', 11).count).to eq(1)
+    expect(manager.get_users('post', post1.id).count).to eq(2)
+    expect(manager.get_users('post', post2.id).count).to eq(1)
 
-    expect(manager.get_users('post', 22)).to contain_exactly(user1, user2)
-    expect(manager.get_users('post', 11)).to contain_exactly(user3)
+    expect(manager.get_users('post', post1.id)).to contain_exactly(user1, user2)
+    expect(manager.get_users('post', post2.id)).to contain_exactly(user3)
 
-    expect(manager.remove('post', 22, user1.id)).to be true
-    expect(manager.get_users('post', 22).count).to eq(1)
-    expect(manager.get_users('post', 22)).to contain_exactly(user2)
+    expect(manager.remove('post', post1.id, user1.id)).to be true
+    expect(manager.get_users('post', post1.id).count).to eq(1)
+    expect(manager.get_users('post', post1.id)).to contain_exactly(user2)
   end
 
   it 'publishes correctly' do
-    expect(manager.get_users('post', 22).count).to eq(0)
+    expect(manager.get_users('post', post1.id).count).to eq(0)
 
-    manager.add('post', 22, user1.id)
-    manager.add('post', 22, user2.id)
+    manager.add('post', post1.id, user1.id)
+    manager.add('post', post1.id, user2.id)
 
     messages = MessageBus.track_publish do
-      manager.publish('post', 22)
+      manager.publish('post', post1.id)
     end
 
     expect(messages.count).to eq (1)
     message = messages.first
 
-    expect(message.channel).to eq('/presence/post/22')
+    expect(message.channel).to eq("/presence/post/#{post1.id}")
 
     expect(message.data["users"].map { |u| u[:id] }).to contain_exactly(user1.id, user2.id)
   end
 
+  it 'publishes private message securely' do
+    private_post = Fabricate(:private_message_post)
+    manager.add('post', private_post.id, user2.id)
+
+    messages = MessageBus.track_publish do
+      manager.publish('post', private_post.id)
+    end
+
+    expect(messages.count).to eq (1)
+    message = messages.first
+
+    expect(message.channel).to eq("/presence/post/#{private_post.id}")
+
+    user_ids = User.where('admin or moderator').pluck(:id)
+    user_ids += private_post.topic.allowed_users.pluck(:id)
+    expect(message.user_ids).to contain_exactly(*user_ids)
+  end
+
+  it 'publishes private category securely' do
+    group = Fabricate(:group)
+    category = Fabricate(:private_category, group: group)
+    private_topic = Fabricate(:topic, category: category)
+
+    manager.add('topic', private_topic.id, user2.id)
+
+    messages = MessageBus.track_publish do
+      manager.publish('topic', private_topic.id)
+    end
+
+    expect(messages.count).to eq (1)
+    message = messages.first
+
+    expect(message.channel).to eq("/presence/topic/#{private_topic.id}")
+
+    expect(message.group_ids).to contain_exactly(*private_topic.secure_group_ids)
+  end
+
   it 'cleans up correctly' do
     freeze_time Time.zone.now do
-      expect(manager.add('post', 22, user1.id)).to be true
-      expect(manager.cleanup('post', 22)).to be false # Nothing to cleanup
-      expect(manager.get_users('post', 22).count).to eq(1)
+      expect(manager.add('post', post1.id, user1.id)).to be true
+      expect(manager.cleanup('post', post1.id)).to be false # Nothing to cleanup
+      expect(manager.get_users('post', post1.id).count).to eq(1)
     end
 
     # Anything older than 20 seconds should be cleaned up
     freeze_time 30.seconds.from_now do
-      expect(manager.cleanup('post', 22)).to be true
-      expect(manager.get_users('post', 22).count).to eq(0)
+      expect(manager.cleanup('post', post1.id)).to be true
+      expect(manager.get_users('post', post1.id).count).to eq(0)
     end
   end
 end