From 830cdffe16c412aa487d585d69549698c277932f Mon Sep 17 00:00:00 2001 From: k32 <10274441+k32@users.noreply.github.com> Date: Mon, 29 Mar 2021 14:48:29 +0200 Subject: [PATCH 1/4] fix(emqx_auth_mnesia): add missing combinations of permissions Allow to define different access for pub and sub actions --- apps/emqx_auth_mnesia/src/emqx_acl_mnesia.erl | 1 + .../src/emqx_acl_mnesia_cli.erl | 30 +++++-- .../test/emqx_acl_mnesia_SUITE.erl | 2 + .../src/emqx_mgmt_data_backup.erl | 21 ++++- .../test/emqx_auth_mnesia_migration_SUITE.erl | 81 +++++++++++++++++++ .../v4.1.json | 48 +++++++++++ .../v4.2.json | 53 ++++++++++++ 7 files changed, 226 insertions(+), 10 deletions(-) create mode 100644 apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl create mode 100644 apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.1.json create mode 100644 apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.2.json diff --git a/apps/emqx_auth_mnesia/src/emqx_acl_mnesia.erl b/apps/emqx_auth_mnesia/src/emqx_acl_mnesia.erl index c657e54a0..ec8670a83 100644 --- a/apps/emqx_auth_mnesia/src/emqx_acl_mnesia.erl +++ b/apps/emqx_auth_mnesia/src/emqx_acl_mnesia.erl @@ -31,6 +31,7 @@ init() -> ok = ekka_mnesia:create_table(emqx_acl, [ + {type, bag}, {disc_copies, [node()]}, {attributes, record_info(fields, emqx_acl)}, {storage_properties, [{ets, [{read_concurrency, true}]}]}]), 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 ef852d04d..9d69402e5 100644 --- a/apps/emqx_auth_mnesia/src/emqx_acl_mnesia_cli.erl +++ b/apps/emqx_auth_mnesia/src/emqx_acl_mnesia_cli.erl @@ -39,13 +39,19 @@ -spec(add_acl(login() | all, emqx_topic:topic(), pub | sub | pubsub, allow | deny) -> ok | {error, any()}). add_acl(Login, Topic, Action, Access) -> - Acls = #?TABLE{ - filter = {Login, Topic}, - action = Action, - access = Access, - created_at = erlang:system_time(millisecond) - }, - ret(mnesia:transaction(fun mnesia:write/1, [Acls])). + Filter = {Login, Topic}, + Acl = #?TABLE{ + filter = Filter, + action = Action, + access = Access, + created_at = erlang:system_time(millisecond) + }, + ret(mnesia:transaction( + fun() -> + OldRecords = mnesia:wread({?TABLE, Filter}), + maybe_delete_shadowed_records(Action, OldRecords), + mnesia:write(Acl) + end)). %% @doc Lookup acl by login -spec(lookup_acl(login() | all) -> list()). @@ -233,3 +239,13 @@ print_acl({all, Topic, Action, Access, _}) -> "Acl($all topic = ~p action = ~p access = ~p)~n", [Topic, Action, Access] ). + +maybe_delete_shadowed_records(_, []) -> + ok; +maybe_delete_shadowed_records(Action1, [Rec = #emqx_acl{action = Action2} | Rest]) -> + if Action1 =:= Action2 orelse Action1 =:= pubsub -> + ok = mnesia:delete_object(Rec); + true -> + ok + end, + maybe_delete_shadowed_records(Action1, Rest). 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 f900fb91a..07fbf4211 100644 --- a/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl +++ b/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl @@ -124,6 +124,7 @@ t_acl_cli(_Config) -> ?assertEqual(0, length(emqx_acl_mnesia_cli:cli(["list"]))), + emqx_acl_mnesia_cli:cli(["add", "clientid", "test_clientid", "topic/A", "pub", "deny"]), emqx_acl_mnesia_cli:cli(["add", "clientid", "test_clientid", "topic/A", "pub", "allow"]), R1 = emqx_ctl:format("Acl(clientid = ~p topic = ~p action = ~p access = ~p)~n", [<<"test_clientid">>, <<"topic/A">>, pub, allow]), @@ -136,6 +137,7 @@ t_acl_cli(_Config) -> ?assertEqual([R2], emqx_acl_mnesia_cli:cli(["show", "username", "test_username"])), ?assertEqual([R2], emqx_acl_mnesia_cli:cli(["list", "username"])), + 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"]) diff --git a/apps/emqx_management/src/emqx_mgmt_data_backup.erl b/apps/emqx_management/src/emqx_mgmt_data_backup.erl index 76d16e89f..a455e18c8 100644 --- a/apps/emqx_management/src/emqx_mgmt_data_backup.erl +++ b/apps/emqx_management/src/emqx_mgmt_data_backup.erl @@ -441,9 +441,11 @@ import_acl_mnesia(Acls, _) -> do_import_acl_mnesia(Acls). -else. import_auth_mnesia(Auths, FromVersion) when FromVersion =:= "4.0" orelse - FromVersion =:= "4.1" orelse - FromVersion =:= "4.2" -> + FromVersion =:= "4.1" -> do_import_auth_mnesia_by_old_data(Auths); +import_auth_mnesia(Auths, "4.2") -> + %% 4.2 contains a bug where password is not base64-encoded + do_import_auth_mnesia_4_2(Auths); import_auth_mnesia(Auths, _) -> do_import_auth_mnesia(Auths). @@ -454,6 +456,17 @@ import_acl_mnesia(Acls, FromVersion) when FromVersion =:= "4.0" orelse import_acl_mnesia(Acls, _) -> do_import_acl_mnesia(Acls). + +do_import_auth_mnesia_4_2(Auths) -> + case ets:info(emqx_user) of + undefined -> ok; + _ -> + CreatedAt = erlang:system_time(millisecond), + lists:foreach(fun(#{<<"login">> := Login, + <<"password">> := Password}) -> + mnesia:dirty_write({emqx_user, {username, Login}, Password, CreatedAt}) + end, Auths) + end. -endif. do_import_auth_mnesia_by_old_data(Auths) -> @@ -466,6 +479,8 @@ do_import_auth_mnesia_by_old_data(Auths) -> mnesia:dirty_write({emqx_user, {username, Login}, base64:decode(Password), CreatedAt}) end, Auths) end. + + do_import_auth_mnesia(Auths) -> case ets:info(emqx_user) of undefined -> ok; @@ -648,4 +663,4 @@ covert_empty_headers(Headers) -> [] -> #{}; Other -> Other end. --endif. \ No newline at end of file +-endif. diff --git a/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl new file mode 100644 index 000000000..42ab0d907 --- /dev/null +++ b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl @@ -0,0 +1,81 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2021 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +-module(emqx_auth_mnesia_migration_SUITE). + +-compile(export_all). +-compile(nowarn_export_all). + +-include_lib("eunit/include/eunit.hrl"). + +-include_lib("emqx/include/emqx.hrl"). +-include_lib("emqx/include/emqx_mqtt.hrl"). +-include_lib("emqx_auth_mnesia/include/emqx_auth_mnesia.hrl"). + +all() -> + [t_import_4_2, t_import_4_1]. + +init_per_suite(Config) -> + emqx_ct_helpers:start_apps([emqx_management, emqx_dashboard, emqx_auth_mnesia]), + ekka_mnesia:start(), + emqx_mgmt_auth:mnesia(boot), + mnesia:clear_table(emqx_acl), + mnesia:clear_table(emqx_user), + Config. + +end_per_suite(_Config) -> + emqx_ct_helpers:stop_apps([emqx_modules, emqx_management, emqx_dashboard, emqx_management, emqx_auth_mnesia]), + ekka_mnesia:ensure_stopped(). + +init_per_testcase(_, Config) -> + Config. + +end_per_testcase(_, _Config) -> + mnesia:clear_table(emqx_acl), + mnesia:clear_table(emqx_user), + ok. + +t_import_4_2(Config) -> + test_import(Config, "v4.2.json"). + +t_import_4_1(Config) -> + test_import(Config, "v4.1.json"). + +test_import(Config, File) -> + Filename = filename:join(proplists:get_value(data_dir, Config), File), + ?assertMatch(ok, emqx_mgmt_data_backup:import(Filename)), + Records = lists:sort(ets:tab2list(emqx_acl)), + %% Check importing of records related to emqx_auth_mnesia + ?assertMatch([#emqx_acl{ + filter = {{username,<<"emqx_c">>}, <<"Topic/A">>}, + action = pub, + access = allow + }, + #emqx_acl{ + filter = {{username,<<"emqx_c">>}, <<"Topic/A">>}, + action = sub, + access = allow + }], + lists:sort(Records)), + ?assertMatch([#emqx_user{ + login = {username, <<"emqx_c">>} + }], ets:tab2list(emqx_user)), + Req = #{clientid => "blah", + username => <<"emqx_c">>, + password => "emqx_p" + }, + ?assertMatch({stop, #{auth_result := success}}, + emqx_auth_mnesia:check(Req, #{}, #{hash_type => sha256})). diff --git a/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.1.json b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.1.json new file mode 100644 index 000000000..04a7a273f --- /dev/null +++ b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.1.json @@ -0,0 +1,48 @@ +{ + "acl_mnesia": [ + { + "action": "sub", + "allow": true, + "login": "emqx_c", + "topic": "Topic/A" + }, + { + "action": "pub", + "allow": true, + "login": "emqx_c", + "topic": "Topic/A" + } + ], + "apps": [ + { + "desc": "Application user", + "expired": "undefined", + "id": "admin", + "name": "Default", + "secret": "public", + "status": true + } + ], + "auth_clientid": [], + "auth_mnesia": [ + { + "is_superuser": false, + "login": "emqx_c", + "password": "Y2ViNWU5MTdmNzkzMGFlOGYwZGMzY2ViNDk2YTQyOGY3ZTY0NDczNmVlYmNhMzZhMmI4ZjZiYmFjNzU2MTcxYQ==" + } + ], + "auth_username": [], + "blacklist": [], + "date": "2021-03-30 09:11:29", + "resources": [], + "rules": [], + "schemas": [], + "users": [ + { + "password": "t89PhgOb15rSCdpxm7Obp7QGcyY=", + "tags": "administrator", + "username": "admin" + } + ], + "version": "4.1" +} diff --git a/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.2.json b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.2.json new file mode 100644 index 000000000..57958aa58 --- /dev/null +++ b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE_data/v4.2.json @@ -0,0 +1,53 @@ +{ + "schemas": [], + "acl_mnesia": [ + { + "allow": true, + "action": "sub", + "topic": "Topic/A", + "login": "emqx_c" + }, + { + "allow": true, + "action": "pub", + "topic": "Topic/A", + "login": "emqx_c" + } + ], + "auth_mnesia": [ + { + "is_superuser": false, + "password": "ceb5e917f7930ae8f0dc3ceb496a428f7e644736eebca36a2b8f6bbac756171a", + "login": "emqx_c" + } + ], + "auth_username": [], + "auth_clientid": [], + "users": [ + { + "tags": "viewer", + "password": "oVqjR1wOi2u4DtsuXNctYt6+SKE=", + "username": "test" + }, + { + "tags": "administrator", + "password": "9SO4rEEZ6rNwA4vAwp3cnXgQsAM=", + "username": "admin" + } + ], + "apps": [ + { + "expired": "undefined", + "status": true, + "desc": "Application user", + "name": "Default", + "secret": "public", + "id": "admin" + } + ], + "blacklist": [], + "resources": [], + "rules": [], + "date": "2021-03-26 09:51:38", + "version": "4.2" +} From 2c029c0607a611efa12dd54b306343909954f8f3 Mon Sep 17 00:00:00 2001 From: k32 <10274441+k32@users.noreply.github.com> Date: Wed, 31 Mar 2021 14:45:11 +0200 Subject: [PATCH 2/4] fix(emqx_management): Allow to specify credential type during import --- .../src/emqx_mgmt_api_data.erl | 2 +- apps/emqx_management/src/emqx_mgmt_cli.erl | 9 +++-- .../src/emqx_mgmt_data_backup.erl | 39 ++++++++++++++++--- .../test/emqx_auth_mnesia_migration_SUITE.erl | 32 +++++++++++---- 4 files changed, 64 insertions(+), 18 deletions(-) diff --git a/apps/emqx_management/src/emqx_mgmt_api_data.erl b/apps/emqx_management/src/emqx_mgmt_api_data.erl index da551782d..855e09525 100644 --- a/apps/emqx_management/src/emqx_mgmt_api_data.erl +++ b/apps/emqx_management/src/emqx_mgmt_api_data.erl @@ -127,7 +127,7 @@ import(_Bindings, Params) -> do_import(Filename) -> FullFilename = filename:join([emqx:get_env(data_dir), Filename]), - emqx_mgmt_data_backup:import(FullFilename). + emqx_mgmt_data_backup:import(FullFilename, "{}"). download(#{filename := Filename}, _Params) -> FullFilename = filename:join([emqx:get_env(data_dir), Filename]), diff --git a/apps/emqx_management/src/emqx_mgmt_cli.erl b/apps/emqx_management/src/emqx_mgmt_cli.erl index e4f443cee..e342cf532 100644 --- a/apps/emqx_management/src/emqx_mgmt_cli.erl +++ b/apps/emqx_management/src/emqx_mgmt_cli.erl @@ -562,7 +562,9 @@ data(["export"]) -> end; data(["import", Filename]) -> - case emqx_mgmt_data_backup:import(Filename) of + data(["import", Filename, "--env", "{}"]); +data(["import", Filename, "--env", Env]) -> + case emqx_mgmt_data_backup:import(Filename, Env) of ok -> emqx_ctl:print("The emqx data has been imported successfully.~n"); {error, import_failed} -> @@ -574,8 +576,9 @@ data(["import", Filename]) -> end; data(_) -> - emqx_ctl:usage([{"data import ", "Import data from the specified file"}, - {"data export", "Export data"}]). + emqx_ctl:usage([{"data import [--env '']", + "Import data from the specified file, possibly with overrides"}, + {"data export", "Export data"}]). %%-------------------------------------------------------------------- %% @doc acl Command diff --git a/apps/emqx_management/src/emqx_mgmt_data_backup.erl b/apps/emqx_management/src/emqx_mgmt_data_backup.erl index a455e18c8..2d6447bde 100644 --- a/apps/emqx_management/src/emqx_mgmt_data_backup.erl +++ b/apps/emqx_management/src/emqx_mgmt_data_backup.erl @@ -52,7 +52,7 @@ ]). -export([ export/0 - , import/1 + , import/2 ]). %%-------------------------------------------------------------------- @@ -464,7 +464,7 @@ do_import_auth_mnesia_4_2(Auths) -> CreatedAt = erlang:system_time(millisecond), lists:foreach(fun(#{<<"login">> := Login, <<"password">> := Password}) -> - mnesia:dirty_write({emqx_user, {username, Login}, Password, CreatedAt}) + mnesia:dirty_write({emqx_user, {get_old_type(), Login}, Password, CreatedAt}) end, Auths) end. -endif. @@ -476,7 +476,7 @@ do_import_auth_mnesia_by_old_data(Auths) -> CreatedAt = erlang:system_time(millisecond), lists:foreach(fun(#{<<"login">> := Login, <<"password">> := Password}) -> - mnesia:dirty_write({emqx_user, {username, Login}, base64:decode(Password), CreatedAt}) + mnesia:dirty_write({emqx_user, {get_old_type(), Login}, base64:decode(Password), CreatedAt}) end, Auths) end. @@ -506,7 +506,7 @@ do_import_acl_mnesia_by_old_data(Acls) -> true -> allow; false -> deny end, - mnesia:dirty_write({emqx_acl, {{username, Login}, Topic}, any_to_atom(Action), Allow1, CreatedAt}) + mnesia:dirty_write({emqx_acl, {{get_old_type(), Login}, Topic}, any_to_atom(Action), Allow1, CreatedAt}) end, Acls) end. do_import_acl_mnesia(Acls) -> @@ -614,11 +614,14 @@ do_export_extra_data() -> do_export_extra_data() -> []. -endif. -import(Filename) -> +import(Filename, OverridesJson) -> case file:read_file(Filename) of {ok, Json} -> - Data = emqx_json:decode(Json, [return_maps]), + Imported = emqx_json:decode(Json, [return_maps]), + Overrides = emqx_json:decode(OverridesJson, [return_maps]), + Data = maps:merge(Imported, Overrides), Version = to_version(maps:get(<<"version">>, Data)), + read_global_auth_type(Data, Version), case lists:member(Version, ?VERSIONS) of true -> try @@ -664,3 +667,27 @@ covert_empty_headers(Headers) -> Other -> Other end. -endif. + +read_global_auth_type(Data, Version) when Version =:= "4.0" orelse + Version =:= "4.1" orelse + Version =:= "4.2" -> + case Data of + #{<<"auth.mnesia.as">> := <<"username">>} -> application:set_env(emqx_auth_mnesia, as, username); + #{<<"auth.mnesia.as">> := <<"clientid">>} -> application:set_env(emqx_auth_mnesia, as, clientid); + _ -> + logger:error("While importing data from EMQX versions prior to 4.3 " + "it is necessary to specify the value of \"auth.mnesia.as\" parameter " + "as it was configured in etc/plugins/emqx_auth_mnesia.conf.\n" + "Use the following command to import data:\n" + " $ emqx_ctl data import --env '{\"auth.mnesia.as\":\"username\"}'\n" + "or\n" + " $ emqx_ctl data import --env '{\"auth.mnesia.as\":\"clientid\"}'", + []), + error(import_failed) + end; +read_global_auth_type(_Data, _Version) -> + ok. + +get_old_type() -> + {ok, Type} = application:get_env(emqx_auth_mnesia, as), + Type. diff --git a/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl index 42ab0d907..39312cc40 100644 --- a/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl +++ b/apps/emqx_management/test/emqx_auth_mnesia_migration_SUITE.erl @@ -26,20 +26,32 @@ -include_lib("emqx_auth_mnesia/include/emqx_auth_mnesia.hrl"). all() -> + [{group, Id} || {Id, _, _} <- groups()]. + +groups() -> + [{username, [], cases()}, {clientid, [], cases()}]. + +cases() -> [t_import_4_2, t_import_4_1]. init_per_suite(Config) -> emqx_ct_helpers:start_apps([emqx_management, emqx_dashboard, emqx_auth_mnesia]), ekka_mnesia:start(), emqx_mgmt_auth:mnesia(boot), - mnesia:clear_table(emqx_acl), - mnesia:clear_table(emqx_user), Config. end_per_suite(_Config) -> emqx_ct_helpers:stop_apps([emqx_modules, emqx_management, emqx_dashboard, emqx_management, emqx_auth_mnesia]), ekka_mnesia:ensure_stopped(). +init_per_group(username, Config) -> + [{cred_type, username} | Config]; +init_per_group(clientid, Config) -> + [{cred_type, clientid} | Config]. + +end_per_group(_, Config) -> + Config. + init_per_testcase(_, Config) -> Config. @@ -55,26 +67,30 @@ t_import_4_1(Config) -> test_import(Config, "v4.1.json"). test_import(Config, File) -> + Type = proplists:get_value(cred_type, Config), + mnesia:clear_table(emqx_acl), + mnesia:clear_table(emqx_user), Filename = filename:join(proplists:get_value(data_dir, Config), File), - ?assertMatch(ok, emqx_mgmt_data_backup:import(Filename)), + Overrides = emqx_json:encode(#{<<"auth.mnesia.as">> => atom_to_binary(Type)}), + ?assertMatch(ok, emqx_mgmt_data_backup:import(Filename, Overrides)), Records = lists:sort(ets:tab2list(emqx_acl)), %% Check importing of records related to emqx_auth_mnesia ?assertMatch([#emqx_acl{ - filter = {{username,<<"emqx_c">>}, <<"Topic/A">>}, + filter = {{Type,<<"emqx_c">>}, <<"Topic/A">>}, action = pub, access = allow }, #emqx_acl{ - filter = {{username,<<"emqx_c">>}, <<"Topic/A">>}, + filter = {{Type,<<"emqx_c">>}, <<"Topic/A">>}, action = sub, access = allow }], lists:sort(Records)), ?assertMatch([#emqx_user{ - login = {username, <<"emqx_c">>} + login = {Type, <<"emqx_c">>} }], ets:tab2list(emqx_user)), - Req = #{clientid => "blah", - username => <<"emqx_c">>, + Req = #{clientid => <<"blah">>} + #{Type => <<"emqx_c">>, password => "emqx_p" }, ?assertMatch({stop, #{auth_result := success}}, From 61ad5d718f63486533500e3fd3048de2e18f366f Mon Sep 17 00:00:00 2001 From: k32 <10274441+k32@users.noreply.github.com> Date: Thu, 1 Apr 2021 15:08:10 +0200 Subject: [PATCH 3/4] fix(emqx_acl_mnesia): split pubsub into two different capabilities --- .../src/emqx_acl_mnesia_cli.erl | 25 ++++++++-- .../test/emqx_acl_mnesia_SUITE.erl | 46 +++++++++++++++---- 2 files changed, 60 insertions(+), 11 deletions(-) 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"]), From 017a07617deb6ad06aa236b137c95e9b5c85d643 Mon Sep 17 00:00:00 2001 From: k32 <10274441+k32@users.noreply.github.com> Date: Thu, 1 Apr 2021 17:44:50 +0200 Subject: [PATCH 4/4] fix(emqx_auth_mnesia): Add tests for migrating pubsub access --- .../test/emqx_acl_mnesia_SUITE.erl | 20 +++++++++++++++++++ 1 file changed, 20 insertions(+) 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 bfaea4230..e5e48cc93 100644 --- a/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl +++ b/apps/emqx_auth_mnesia/test/emqx_acl_mnesia_SUITE.erl @@ -131,6 +131,26 @@ t_management(_Config) -> deny = emqx_access_control:check_acl(User1, subscribe, <<"topic/mix">>), deny = emqx_access_control:check_acl(User1, publish, <<"topic/mix">>), + %% Test implicit migration of pubsub to pub and sub: + ok = emqx_acl_mnesia_cli:remove_acl({clientid, <<"test_clientid">>}, <<"topic/mix">>), + ok = mnesia:dirty_write(#emqx_acl{ + filter = {{clientid, <<"test_clientid">>}, <<"topic/mix">>}, + action = pubsub, + access = allow, + created_at = erlang:system_time(millisecond) + }), + 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">>, pub, deny), + timer:sleep(100), + allow = 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">>, sub, 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">>),