FIX: Don't publish message to intersection of `user_ids` and `group_ids`

FIX: Don’t publish message to intersection of user_ids and group_ids

The original design intent was to always publish messages to clients that are either in user_ids and group_ids. However, there is a long standing bug where we only published to the intersection of user_ids and group_ids which this commit seeks to resolve.

diff --git a/CHANGELOG b/CHANGELOG
index 68b1626..dda9f4c 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -1,5 +1,6 @@
 Unreleasd
   - Drop support for Ruby 2.3
+  - FIX: Don't publish message to intersection of `user_ids` and `group_ids`
 
 26-03-2020
 
diff --git a/README.md b/README.md
index 29a7657..e46294a 100644
--- a/README.md
+++ b/README.md
@@ -74,32 +74,40 @@ id = MessageBus.last_id("/channel")
 MessageBus.backlog "/channel", id
 `‍``
 
-`‍``ruby
-# messages can be targetted at particular users or groups
-MessageBus.publish "/channel", "hello", user_ids: [1,2,3], group_ids: [4,5,6]
+### Targetted messages
 
-# messages can be targetted at particular clients (using MessageBus.clientId)
-MessageBus.publish "/channel", "hello", client_ids: ["XXX","YYY"]
+Messages can be targetted to particular clients by supplying the `client_ids` option when publishing a message.
 
-# message bus determines the user ids and groups based on env
+`‍``ruby
+MessageBus.publish "/channel", "hello", client_ids: ["XXX", "YYY"] # (using MessageBus.clientId)
+`‍``
 
+By configuring the `user_id_lookup` and `group_ids_lookup` options with a Proc or Lambda which will be called with a [Rack specification environment](https://github.com/rack/rack/blob/master/SPEC.rdoc#the-environment-), messages can be targetted to particular clients users or groups by supplying either the `user_ids` or `group_ids` options when publishing a message.
+
+`‍``ruby
 MessageBus.configure(user_id_lookup: proc do |env|
   # this lookup occurs on JS-client poolings, so that server can retrieve backlog
   # for the client considering/matching/filtering user_ids set on published messages
   # if user_id is not set on publish time, any user_id returned here will receive the message
-
   # return the user id here
 end)
 
+# Target user_ids when publishing a message
+MessageBus.publish "/channel", "hello", user_ids: [1, 2, 3]
+
 MessageBus.configure(group_ids_lookup: proc do |env|
   # return the group ids the user belongs to
   # can be nil or []
 end)
 
-# example of message bus to set user_ids from an initializer in Rails and Devise:
+# Target group_ids when publishing a message
+MessageBus.publish "/channel", "hello", group_ids: [1, 2, 3]
+
+# example of MessageBus to set user_ids from an initializer in Rails and Devise:
 # config/inializers/message_bus.rb
 MessageBus.user_id_lookup do |env|
   req = Rack::Request.new(env)
+
   if req.session && req.session["warden.user.user.key"] && req.session["warden.user.user.key"][0][0]
     user = User.find(req.session["warden.user.user.key"][0][0])
     user.id
@@ -107,6 +115,22 @@ MessageBus.user_id_lookup do |env|
 end
 `‍``
 
+If both `user_ids` and `group_ids` options are supplied when publishing a message, the message will be targetted at clients with lookup return values that  matches on either the `user_ids` **or** the `group_ids` options.
+
+`‍``ruby
+MessageBus.publish "/channel", "hello", user_ids: [1, 2, 3], group_ids: [1, 2, 3]
+`‍``
+
+If the `client_ids` option is supplied with either the `user_ids` or `group_ids` options when publising a message, the `client_ids` option will be applied unconditionally and messages will be filtered further using `user_id` or `group_id` clauses.
+
+`‍``ruby
+MessageBus.publish "/channel", "hello", client_ids: ["XXX", "YYY"], user_ids: [1, 2, 3], group_ids: [1, 2, 3]
+`‍``
+
+Passing `nil` or `[]` to either `client_ids`, `user_ids` or `group_ids` is equivalent to allowing all values on each option.
+
+### Error handling
+
 `‍``ruby
 MessageBus.configure(on_middleware_error: proc do |env, e|
    # If you wish to add special handling based on error
diff --git a/lib/message_bus/client.rb b/lib/message_bus/client.rb
index 6eceb2c..dd59f9a 100644
--- a/lib/message_bus/client.rb
+++ b/lib/message_bus/client.rb
@@ -128,15 +128,26 @@ class MessageBus::Client
   # @return [Boolean] whether or not the client has permission to receive the
   #   passed message
   def allowed?(msg)
-    allowed = !msg.user_ids || msg.user_ids.include?(self.user_id)
-    allowed &&= !msg.client_ids || msg.client_ids.include?(self.client_id)
-    allowed && (
-      msg.group_ids.nil? ||
-      msg.group_ids.length == 0 ||
-      (
-        msg.group_ids - self.group_ids
+    client_allowed = !msg.client_ids || msg.client_ids.length == 0 || msg.client_ids.include?(self.client_id)
+
+    user_allowed = false
+    group_allowed = false
+
+    # this is an inconsistency we should fix anyway, publishing `user_ids: nil` should work same as groups
+    has_users = msg.user_ids && msg.user_ids.length > 0
+    has_groups = msg.group_ids && msg.group_ids.length > 0
+
+    if has_users
+      user_allowed = msg.user_ids.include?(self.user_id)
+    end
+
+    if has_groups
+      group_allowed = (
+        msg.group_ids - (self.group_ids || [])
       ).length < msg.group_ids.length
-    )
+    end
+
+    client_allowed && (user_allowed || group_allowed || (!has_users && !has_groups))
   end
 
   # @return [Array<MessageBus::Message>] the set of messages the client is due
diff --git a/spec/lib/message_bus/client_spec.rb b/spec/lib/message_bus/client_spec.rb
index bc533e3..de1bab3 100644
--- a/spec/lib/message_bus/client_spec.rb
+++ b/spec/lib/message_bus/client_spec.rb
@@ -169,37 +169,187 @@ describe MessageBus::Client do
       log[0].data.must_equal 'world'
     end
 
-    it "allows only client_id in list if message contains client_ids" do
-      @message = MessageBus::Message.new(1, 2, '/test', 'hello')
-      @message.client_ids = ["1", "2"]
-      @client.client_id = "2"
-      @client.allowed?(@message).must_equal true
-
-      @client.client_id = "3"
-      @client.allowed?(@message).must_equal false
-    end
-
-    describe "targetted at group" do
-      before do
+    describe '#allowed?' do
+      it "allows only client_id in list if message contains client_ids" do
         @message = MessageBus::Message.new(1, 2, '/test', 'hello')
-        @message.group_ids = [1, 2, 3]
-      end
+        @message.client_ids = ["1", "2"]
+        @client.client_id = "2"
+        @client.allowed?(@message).must_equal true
 
-      it "denies users that are not members of group" do
-        @client.group_ids = [77, 0, 10]
+        @client.client_id = "3"
         @client.allowed?(@message).must_equal false
-      end
 
-      it "allows users that are members of group" do
-        @client.group_ids = [1, 2, 3]
+        @message.client_ids = []
+
+        @client.client_id = "3"
         @client.allowed?(@message).must_equal true
-      end
 
-      it "allows all users if groups not set" do
-        @message.group_ids = nil
-        @client.group_ids = [77, 0, 10]
+        @message.client_ids = nil
+
+        @client.client_id = "3"
         @client.allowed?(@message).must_equal true
       end
+
+      describe 'targetted at user' do
+        before do
+          @message = MessageBus::Message.new(1, 2, '/test', 'hello')
+          @message.user_ids = [1, 2, 3]
+        end
+
+        it "allows client with user_id that is included in message's user_ids" do
+          @client.user_id = 1
+          @client.allowed?(@message).must_equal(true)
+        end
+
+        it "denies client with user_id that is not included in message's user_ids" do
+          @client.user_id = 4
+          @client.allowed?(@message).must_equal(false)
+        end
+
+        it "denies client with nil user_id" do
+          @client.user_id = nil
+
+          @client.allowed?(@message).must_equal(false)
+        end
+
+        it "allows client if message's user_ids is not set" do
+          @message.user_ids = nil
+          @client.user_id = 4
+          @client.allowed?(@message).must_equal(true)
+        end
+
+        it "allows client if message's user_ids is empty" do
+          @message.user_ids = []
+          @client.user_id = 4
+          @client.allowed?(@message).must_equal(true)
+        end
+
+        it "allows client with client_id that is included in message's client_ids" do

[... diff too long, it was truncated ...]

GitHub sha: 0d75e8a4

This commit appears in #219 which was approved by eviltrout. It was merged by SamSaffron.