From 8825581d987bd0e80f8f083e666096698967582f Mon Sep 17 00:00:00 2001 From: David Taylor Date: Fri, 8 Sep 2017 21:06:27 +0100 Subject: [PATCH] Discourse-presence improvements MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - 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 --- .../composer-presence-display.js.es6 | 128 ++++++++++++++++- .../composer-controller-presence.js.es6 | 129 ------------------ .../connectors/composer-fields/presence.hbs | 7 +- .../discourse-presence/config/settings.yml | 2 +- plugins/discourse-presence/plugin.rb | 41 +++++- .../spec/presence_controller_spec.rb | 40 ++++-- .../spec/presence_manager_spec.rb | 88 ++++++++---- 7 files changed, 264 insertions(+), 171 deletions(-) delete mode 100644 plugins/discourse-presence/assets/javascripts/discourse/initializers/composer-controller-presence.js.es6 diff --git a/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 index dd50d3609d5..bbaa9f2166f 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/components/composer-presence-display.js.es6 +++ b/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'; }, diff --git a/plugins/discourse-presence/assets/javascripts/discourse/initializers/composer-controller-presence.js.es6 b/plugins/discourse-presence/assets/javascripts/discourse/initializers/composer-controller-presence.js.es6 deleted file mode 100644 index 3a98851d70f..00000000000 --- a/plugins/discourse-presence/assets/javascripts/discourse/initializers/composer-controller-presence.js.es6 +++ /dev/null @@ -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); - } -}; diff --git a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs b/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs index 127cb33575e..87021d261fc 100644 --- a/plugins/discourse-presence/assets/javascripts/discourse/templates/connectors/composer-fields/presence.hbs +++ b/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 + }} diff --git a/plugins/discourse-presence/config/settings.yml b/plugins/discourse-presence/config/settings.yml index f988b3118c5..56341eeea26 100644 --- a/plugins/discourse-presence/config/settings.yml +++ b/plugins/discourse-presence/config/settings.yml @@ -1,4 +1,4 @@ plugins: presence_enabled: - default: false + default: true client: true diff --git a/plugins/discourse-presence/plugin.rb b/plugins/discourse-presence/plugin.rb index aff34aa27ea..7c59e0e343e 100644 --- a/plugins/discourse-presence/plugin.rb +++ b/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) diff --git a/plugins/discourse-presence/spec/presence_controller_spec.rb b/plugins/discourse-presence/spec/presence_controller_spec.rb index c15263714e2..e61531beedc 100644 --- a/plugins/discourse-presence/spec/presence_controller_spec.rb +++ b/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 diff --git a/plugins/discourse-presence/spec/presence_manager_spec.rb b/plugins/discourse-presence/spec/presence_manager_spec.rb index 0c98197a474..4f8cb48b02f 100644 --- a/plugins/discourse-presence/spec/presence_manager_spec.rb +++ b/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