fix(emqx_acl_mnesia): split pubsub into two different capabilities

apps/emqx_auth_mnesia/src/emqx_acl_mnesia_cli.erl

@@ -49,8 +49,13 @@ add_acl(Login, Topic, Action, Access) ->
     ret(mnesia:transaction(
           fun() ->
                   OldRecords = mnesia:wread({?TABLE, Filter}),
-                  maybe_delete_shadowed_records(Action, OldRecords),
+                  case Action of
-                  mnesia:write(Acl)
+                      pubsub ->
+                          update_permission(pub, Acl, OldRecords),
+                          update_permission(sub, Acl, OldRecords);
+                      _ ->
+                          update_permission(Action, Acl, OldRecords)
+                  end
           end)).
 
 %% @doc Lookup acl by login
@@ -240,12 +245,26 @@ print_acl({all, Topic, Action, Access, _}) ->
         [Topic, Action, Access]
      ).
 
+update_permission(Action, Acl0, OldRecords) ->
+    Acl = Acl0 #?TABLE{action = Action},
+    maybe_delete_shadowed_records(Action, OldRecords),
+    mnesia:write(Acl).
+
 maybe_delete_shadowed_records(_, []) ->
     ok;
 maybe_delete_shadowed_records(Action1, [Rec = #emqx_acl{action = Action2} | Rest]) ->
-    if Action1 =:= Action2 orelse Action1 =:= pubsub ->
+    if Action1 =:= Action2 ->
             ok = mnesia:delete_object(Rec);
+       Action2 =:= pubsub ->
+            %% Perform migration from the old data format on the
+            %% fly. This is needed only for the enterprise version,
+            %% delete this branch on 5.0
+            mnesia:delete_object(Rec),
+            mnesia:write(Rec#?TABLE{action = other_action(Action1)});
        true ->
             ok
     end,
     maybe_delete_shadowed_records(Action1, Rest).
+
+other_action(pub) -> sub;
+other_action(sub) -> pub.

apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl

@@ -86,11 +86,15 @@ t_management(_Config) ->
     ok = emqx_acl_mnesia_cli:add_acl({username, <<"test_username">>}, <<"topic/%u">>, sub, deny),
     ok = emqx_acl_mnesia_cli:add_acl({username, <<"test_username">>}, <<"topic/+">>, pub, allow),
     ok = emqx_acl_mnesia_cli:add_acl(all, <<"#">>, pubsub, deny),
+    %% Sleeps below are needed to hide the race condition between
+    %% mnesia and ets dirty select in check_acl, that make this test
+    %% flaky
+    timer:sleep(100),
 
     ?assertEqual(2, length(emqx_acl_mnesia_cli:lookup_acl({clientid, <<"test_clientid">>}))),
     ?assertEqual(2, length(emqx_acl_mnesia_cli:lookup_acl({username, <<"test_username">>}))),
-    ?assertEqual(1, length(emqx_acl_mnesia_cli:lookup_acl(all))),
+    ?assertEqual(2, length(emqx_acl_mnesia_cli:lookup_acl(all))),
-    ?assertEqual(5, length(emqx_acl_mnesia_cli:all_acls())),
+    ?assertEqual(6, length(emqx_acl_mnesia_cli:all_acls())),
 
     User1 = #{zone => external, clientid => <<"test_clientid">>},
     User2 = #{zone => external, clientid => <<"no_exist">>, username => <<"test_username">>},
@@ -105,11 +109,35 @@ t_management(_Config) ->
     deny  = emqx_access_control:check_acl(User3, subscribe, <<"topic/A/B">>),
     deny  = emqx_access_control:check_acl(User3, publish,   <<"topic/A/B">>),
 
+    %% Test merging of pubsub capability:
+    ok = emqx_acl_mnesia_cli:add_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>, pubsub, deny),
+    timer:sleep(100),
+    deny  = emqx_access_control:check_acl(User1, subscribe,   <<"topic/mix">>),
+    deny  = emqx_access_control:check_acl(User1, publish,     <<"topic/mix">>),
+    ok = emqx_acl_mnesia_cli:add_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>, pub, allow),
+    timer:sleep(100),
+    deny  = emqx_access_control:check_acl(User1, subscribe,   <<"topic/mix">>),
+    allow = emqx_access_control:check_acl(User1, publish,     <<"topic/mix">>),
+    ok = emqx_acl_mnesia_cli:add_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>, pubsub, allow),
+    timer:sleep(100),
+    allow = emqx_access_control:check_acl(User1, subscribe,   <<"topic/mix">>),
+    allow = emqx_access_control:check_acl(User1, publish,     <<"topic/mix">>),
+    ok = emqx_acl_mnesia_cli:add_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>, sub, deny),
+    timer:sleep(100),
+    deny  = emqx_access_control:check_acl(User1, subscribe,   <<"topic/mix">>),
+    allow = emqx_access_control:check_acl(User1, publish,     <<"topic/mix">>),
+    ok = emqx_acl_mnesia_cli:add_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>, pub, deny),
+    timer:sleep(100),
+    deny  = emqx_access_control:check_acl(User1, subscribe,   <<"topic/mix">>),
+    deny  = emqx_access_control:check_acl(User1, publish,     <<"topic/mix">>),
+
     ok = emqx_acl_mnesia_cli:remove_acl({clientid, <<"test_clientid">>}, <<"topic/%c">>),
     ok = emqx_acl_mnesia_cli:remove_acl({clientid, <<"test_clientid">>}, <<"topic/+">>),
+    ok = emqx_acl_mnesia_cli:remove_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>),
     ok = emqx_acl_mnesia_cli:remove_acl({username, <<"test_username">>}, <<"topic/%u">>),
     ok = emqx_acl_mnesia_cli:remove_acl({username, <<"test_username">>}, <<"topic/+">>),
     ok = emqx_acl_mnesia_cli:remove_acl(all, <<"#">>),
+    timer:sleep(100),
 
     ?assertEqual([], emqx_acl_mnesia_cli:all_acls()).
 
@@ -139,10 +167,12 @@ t_acl_cli(_Config) ->
 
     emqx_acl_mnesia_cli:cli(["add", "_all", "#", "pub", "allow"]),
     emqx_acl_mnesia_cli:cli(["add", "_all", "#", "pubsub", "deny"]),
-    ?assertMatch(["Acl($all topic = <<\"#\">> action = pubsub access = deny)\n"],
+    ?assertMatch(["",
-                 emqx_acl_mnesia_cli:cli(["list", "_all"])
+                  "Acl($all topic = <<\"#\">> action = pub access = deny)",
+                  "Acl($all topic = <<\"#\">> action = sub access = deny)"],
+                 lists:sort(string:split(emqx_acl_mnesia_cli:cli(["list", "_all"]), "\n", all))
                 ),
-    ?assertEqual(3, length(emqx_acl_mnesia_cli:cli(["list"]))),
+    ?assertEqual(4, length(emqx_acl_mnesia_cli:cli(["list"]))),
 
     emqx_acl_mnesia_cli:cli(["del", "clientid", "test_clientid", "topic/A"]),
     emqx_acl_mnesia_cli:cli(["del", "username", "test_username", "topic/B"]),
@@ -171,7 +201,7 @@ t_rest_api(_Config) ->
                 }],
     {ok, _} = request_http_rest_add([], Params1),
     {ok, Re1} = request_http_rest_list(["clientid", "test_clientid"]),
-    ?assertMatch(3, length(get_http_data(Re1))),
+    ?assertMatch(4, length(get_http_data(Re1))),
     {ok, _} = request_http_rest_delete(["clientid", "test_clientid", "topic", "topic/A"]),
     {ok, _} = request_http_rest_delete(["clientid", "test_clientid", "topic", "topic/B"]),
     {ok, _} = request_http_rest_delete(["clientid", "test_clientid", "topic", "topic/C"]),
@@ -195,7 +225,7 @@ t_rest_api(_Config) ->
                 }],
     {ok, _} = request_http_rest_add([], Params2),
     {ok, Re2} = request_http_rest_list(["username", "test_username"]),
-    ?assertMatch(3, length(get_http_data(Re2))),
+    ?assertMatch(4, length(get_http_data(Re2))),
     {ok, _} = request_http_rest_delete(["username", "test_username", "topic", "topic/A"]),
     {ok, _} = request_http_rest_delete(["username", "test_username", "topic", "topic/B"]),
     {ok, _} = request_http_rest_delete(["username", "test_username", "topic", "topic/C"]),
@@ -216,7 +246,7 @@ t_rest_api(_Config) ->
                 }],
     {ok, _} = request_http_rest_add([], Params3),
     {ok, Re3} = request_http_rest_list(["$all"]),
-    ?assertMatch(3, length(get_http_data(Re3))),
+    ?assertMatch(4, length(get_http_data(Re3))),
     {ok, _} = request_http_rest_delete(["$all", "topic", "topic/A"]),
     {ok, _} = request_http_rest_delete(["$all", "topic", "topic/B"]),
     {ok, _} = request_http_rest_delete(["$all", "topic", "topic/C"]),