Merge pull request #602 from emqtt/issue#599

Fix issue #599 - Reject empty clientId with non-clean session

rel/files/emqttd.test.config

@@ -46,7 +46,7 @@
         %% Authetication. Anonymous Default
         {auth, [
             %% Authentication with username, password
-            {username, [{test, "password"}, {"test1", "password1"}]},
+            %% {username, [{test, "password"}, {"test1", "password1"}]},
             
             %% Authentication with clientid
             %{clientid, [{password, no}, {file, "etc/clients.config"}]},
@@ -104,8 +104,8 @@
             %% Statistics Collection Interval(seconds)
             {collect_interval, 20},
 
-            %% Expired after 2 days
-            {expired_after, 48}
+            %% Expired after 2 day (unit: minute)
+            {expired_after, 2880}
 
         ]},
         %% Queue
@@ -173,14 +173,12 @@
 
         %% Subscribe topics automatically when client connected
         {subscription, [
-            %% Static subscriptions from backend
-            backend,
 
-            %% $u will be replaced with username
-            {"$queue/username/$u", 1},
+            %% $c will be replaced by clientid
+            %% {"$queue/clients/$c", 1},
 
-            %% $c will be replaced with clientid
-            {"$queue/clients/$c", 1}
+            %% Static subscriptions from backend
+            backend
         ]}
 
         %% Rewrite rules

src/emqttd_protocol.erl

@@ -336,6 +336,12 @@ validate_clientid(#mqtt_packet_connect{client_id = ClientId},
     when (size(ClientId) >= 1) andalso (size(ClientId) =< MaxLen) ->
     true;
 
+%% Issue#599: Null clientId and clean_sess = false
+validate_clientid(#mqtt_packet_connect{client_id  = ClientId,
+                                       clean_sess = CleanSess}, _ProtoState)
+    when size(ClientId) == 0 andalso (not CleanSess) ->
+    false;
+
 %% MQTT3.1.1 allow null clientId.
 validate_clientid(#mqtt_packet_connect{proto_ver =?MQTT_PROTO_V311,
                                        client_id = ClientId}, _ProtoState)

test/emqttd_SUITE.erl

@@ -23,7 +23,8 @@
 -include_lib("eunit/include/eunit.hrl").
 
 all() ->
-    [{group, pubsub},
+    [{group, protocol},
+     {group, pubsub},
      {group, router},
      {group, session},
      {group, retainer},
@@ -35,7 +36,9 @@ all() ->
      {group, cli}].
 
 groups() ->
-    [{pubsub, [sequence],
+    [{protocol, [sequence],
+      [mqtt_connect]},
+     {pubsub, [sequence],
       [create_topic,
        create_subscription,
        subscribe_unsubscribe,
@@ -88,6 +91,24 @@ end_per_suite(_Config) ->
     application:stop(gproc),
     emqttd_mnesia:ensure_stopped().
 
+%%--------------------------------------------------------------------
+%% Protocol Test
+%%--------------------------------------------------------------------
+
+mqtt_connect(_) ->
+    %% Issue #599
+    %% Empty clientId and clean_session = false
+    ?assertEqual(<<32,2,0,2>>, connect_broker_(<<16,12,0,4,77,81,84,84,4,0,0,90,0,0>>, 4)),
+    %% Empty clientId and clean_session = true
+    ?assertEqual(<<32,2,0,0>>, connect_broker_(<<16,12,0,4,77,81,84,84,4,2,0,90,0,0>>, 4)).
+
+connect_broker_(Packet, RecvSize) ->
+    {ok, Sock} = gen_tcp:connect({127,0,0,1}, 1883, [binary, {packet, raw}, {active, false}]),
+    gen_tcp:send(Sock, Packet),
+    {ok, Data} = gen_tcp:recv(Sock, RecvSize, 3000),
+    gen_tcp:close(Sock),
+    Data.
+
 %%--------------------------------------------------------------------
 %% PubSub Test
 %%--------------------------------------------------------------------
@@ -156,6 +177,7 @@ pubsub_queue(_) ->
     Self = self(), Q = <<"$queue/abc">>,
     SubFun = fun() ->
                emqttd:subscribe(Q),
+               timer:sleep(1),
                {ok, Msgs} = loop_recv(Q, 10),
                Self ! {recv, self(), Msgs}
              end,