diff --git a/apps/emqx_auth_mnesia/src/emqx_acl_mnesia_cli.erl b/apps/emqx_auth_mnesia/src/emqx_acl_mnesia_cli.erl index 9d69402e5..ca1be1676 100644 --- a/apps/emqx_auth_mnesia/src/emqx_acl_mnesia_cli.erl +++ b/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), - mnesia:write(Acl) + case Action of + 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. diff --git a/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl b/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl index 07fbf4211..bfaea4230 100644 --- a/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl +++ b/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(5, length(emqx_acl_mnesia_cli:all_acls())), + ?assertEqual(2, length(emqx_acl_mnesia_cli:lookup_acl(all))), + ?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"], - emqx_acl_mnesia_cli:cli(["list", "_all"]) + ?assertMatch(["", + "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"]),