Merge pull request #10765 from zhongwencool/port-10742-to-release-50

Port 10742 to release 50

apps/emqx/test/emqx_common_test_helpers.erl

@@ -238,6 +238,8 @@ render_and_load_app_config(App, Opts) ->
     end.
 
 do_render_app_config(App, Schema, ConfigFile, Opts) ->
+    %% copy acl_conf must run before read_schema_configs
+    copy_acl_conf(),
     Vars = mustache_vars(App, Opts),
     RenderedConfigFile = render_config_file(ConfigFile, Vars),
     read_schema_configs(Schema, RenderedConfigFile),
@@ -497,6 +499,16 @@ copy_certs(emqx_conf, Dest0) ->
 copy_certs(_, _) ->
     ok.
 
+copy_acl_conf() ->
+    Dest = filename:join([code:lib_dir(emqx), "etc/acl.conf"]),
+    case code:lib_dir(emqx_authz) of
+        {error, bad_name} ->
+            (not filelib:is_regular(Dest)) andalso file:write_file(Dest, <<"">>);
+        _ ->
+            {ok, _} = file:copy(deps_path(emqx_authz, "etc/acl.conf"), Dest)
+    end,
+    ok.
+
 load_config(SchemaModule, Config) ->
     ConfigBin =
         case is_map(Config) of

apps/emqx_authz/src/emqx_authz.app.src

@@ -1,7 +1,7 @@
 %% -*- mode: erlang -*-
 {application, emqx_authz, [
     {description, "An OTP application"},
-    {vsn, "0.1.19"},
+    {vsn, "0.1.20"},
     {registered, []},
     {mod, {emqx_authz_app, []}},
     {applications, [

apps/emqx_authz/src/emqx_authz.erl

@@ -140,7 +140,12 @@ update(Cmd, Sources) ->
     emqx_authz_utils:update_config(?CONF_KEY_PATH, {Cmd, Sources}).
 
 pre_config_update(_, Cmd, Sources) ->
-    {ok, do_pre_config_update(Cmd, Sources)}.
+    try do_pre_config_update(Cmd, Sources) of
+        {error, Reason} -> {error, Reason};
+        NSources -> {ok, NSources}
+    catch
+        _:Reason -> {error, Reason}
+    end.
 
 do_pre_config_update({?CMD_MOVE, _, _} = Cmd, Sources) ->
     do_move(Cmd, Sources);
@@ -475,11 +480,14 @@ maybe_write_files(#{<<"type">> := <<"file">>} = Source) ->
 maybe_write_files(NewSource) ->
     maybe_write_certs(NewSource).
 
-write_acl_file(#{<<"rules">> := Rules} = Source) ->
+write_acl_file(#{<<"rules">> := Rules} = Source0) ->
-    NRules = check_acl_file_rules(Rules),
+    AclPath = ?MODULE:acl_conf_file(),
-    Path = ?MODULE:acl_conf_file(),
+    %% Always check if the rules are valid before writing to the file
-    {ok, _Filename} = write_file(Path, NRules),
+    %% If the rules are invalid, the old file will be kept
-    maps:without([<<"rules">>], Source#{<<"path">> => Path}).
+    ok = check_acl_file_rules(AclPath, Rules),
+    ok = write_file(AclPath, Rules),
+    Source1 = maps:remove(<<"rules">>, Source0),
+    maps:put(<<"path">>, AclPath, Source1).
 
 %% @doc where the acl.conf file is stored.
 acl_conf_file() ->
@@ -506,7 +514,7 @@ write_file(Filename, Bytes) ->
     ok = filelib:ensure_dir(Filename),
     case file:write_file(Filename, Bytes) of
         ok ->
-            {ok, iolist_to_binary(Filename)};
+            ok;
         {error, Reason} ->
             ?SLOG(error, #{filename => Filename, msg => "write_file_error", reason => Reason}),
             throw(Reason)
@@ -528,6 +536,14 @@ get_source_by_type(Type, Sources) ->
 update_authz_chain(Actions) ->
     emqx_hooks:put('client.authorize', {?MODULE, authorize, [Actions]}, ?HP_AUTHZ).
 
-check_acl_file_rules(RawRules) ->
+check_acl_file_rules(Path, Rules) ->
-    %% TODO: make sure the bin rules checked
+    TmpPath = Path ++ ".tmp",
-    RawRules.
+    try
+        ok = write_file(TmpPath, Rules),
+        {ok, _} = emqx_authz_file:validate(TmpPath),
+        ok
+    catch
+        throw:Reason -> throw(Reason)
+    after
+        _ = file:delete(TmpPath)
+    end.

apps/emqx_authz/src/emqx_authz_api_schema.erl

@@ -39,8 +39,10 @@ fields(file) ->
                 type => binary(),
                 required => true,
                 example =>
-                    <<"{allow,{username,\"^dashboard?\"},", "subscribe,[\"$SYS/#\"]}.\n",
+                    <<
-                        "{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}.">>,
+                        "{allow,{username,{re,\"^dashboard$\"}},subscribe,[\"$SYS/#\"]}.\n",
+                        "{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}."
+                    >>,
                 desc => ?DESC(rules)
             }}
         ];

apps/emqx_authz/src/emqx_authz_file.erl

@@ -33,13 +33,14 @@
     update/1,
     destroy/1,
     authorize/4,
+    validate/1,
     read_file/1
 ]).
 
 description() ->
     "AuthZ with static rules".
 
-create(#{path := Path0} = Source) ->
+validate(Path0) ->
     Path = filename(Path0),
     Rules =
         case file:consult(Path) of
@@ -54,8 +55,12 @@ create(#{path := Path0} = Source) ->
                 throw(failed_to_read_acl_file);
             {error, Reason} ->
                 ?SLOG(alert, #{msg => bad_acl_file_content, path => Path, reason => Reason}),
-                throw(bad_acl_file_content)
+                throw({bad_acl_file_content, Reason})
         end,
+    {ok, Rules}.
+
+create(#{path := Path} = Source) ->
+    {ok, Rules} = validate(Path),
     Source#{annotations => #{rules => Rules}}.
 
 update(#{path := _Path} = Source) ->

apps/emqx_authz/src/emqx_authz_rule.erl

@@ -68,7 +68,13 @@ compile({Permission, Who, Action, TopicFilters}) when
     {atom(Permission), compile_who(Who), atom(Action), [
         compile_topic(Topic)
      || Topic <- TopicFilters
-    ]}.
+    ]};
+compile({Permission, _Who, _Action, _TopicFilter}) when not ?ALLOW_DENY(Permission) ->
+    throw({invalid_authorization_permission, Permission});
+compile({_Permission, _Who, Action, _TopicFilter}) when not ?PUBSUB(Action) ->
+    throw({invalid_authorization_action, Action});
+compile(BadRule) ->
+    throw({invalid_authorization_rule, BadRule}).
 
 compile_who(all) ->
     all;

apps/emqx_authz/src/emqx_authz_schema.erl

@@ -78,7 +78,17 @@ fields("authorization") ->
     authz_fields();
 fields(file) ->
     authz_common_fields(file) ++
-        [{path, ?HOCON(string(), #{required => true, desc => ?DESC(path)})}];
+        [
+            {path,
+                ?HOCON(
+                    string(),
+                    #{
+                        required => true,
+                        validator => fun(Path) -> element(1, emqx_authz_file:validate(Path)) end,
+                        desc => ?DESC(path)
+                    }
+                )}
+        ];
 fields(http_get) ->
     authz_common_fields(http) ++
         http_common_fields() ++
@@ -496,7 +506,7 @@ authz_fields() ->
                     %% doc_lift is force a root level reference instead of nesting sub-structs
                     extra => #{doc_lift => true},
                     %% it is recommended to configure authz sources from dashboard
-                    %% hance the importance level for config is low
+                    %% hence the importance level for config is low
                     importance => ?IMPORTANCE_LOW
                 }
             )}

apps/emqx_authz/test/emqx_authz_SUITE.erl

@@ -155,22 +155,36 @@ set_special_configs(_App) ->
     <<"ssl">> => #{<<"enable">> => false},
     <<"cmd">> => <<"HGETALL mqtt_authz:", ?PH_USERNAME/binary>>
 }).
--define(SOURCE6, #{
+
+-define(FILE_SOURCE(Rules), #{
     <<"type">> => <<"file">>,
     <<"enable">> => true,
-    <<"rules">> =>
+    <<"rules">> => Rules
+}).
+
+-define(SOURCE6,
+    ?FILE_SOURCE(
         <<
             "{allow,{username,\"^dashboard?\"},subscribe,[\"$SYS/#\"]}."
             "\n{allow,{ipaddr,\"127.0.0.1\"},all,[\"$SYS/#\",\"#\"]}."
         >>
-}).
+    )
--define(SOURCE7, #{
+).
+-define(SOURCE7,
+    ?FILE_SOURCE(
+        <<
+            "{allow,{username,\"some_client\"},publish,[\"some_client/lwt\"]}.\n"
+            "{deny, all}."
+        >>
+    )
+).
+
+-define(BAD_FILE_SOURCE2, #{
     <<"type">> => <<"file">>,
     <<"enable">> => true,
     <<"rules">> =>
         <<
-            "{allow,{username,\"some_client\"},publish,[\"some_client/lwt\"]}.\n"
+            "{not_allow,{username,\"some_client\"},publish,[\"some_client/lwt\"]}."
-            "{deny, all}."
         >>
 }).
 
@@ -178,6 +192,40 @@ set_special_configs(_App) ->
 %% Testcases
 %%------------------------------------------------------------------------------
 
+-define(UPDATE_ERROR(Err), {error, {pre_config_update, emqx_authz, Err}}).
+
+t_bad_file_source(_) ->
+    BadContent = ?FILE_SOURCE(<<"{allow,{username,\"bar\"}, publish, [\"test\"]}">>),
+    BadContentErr = {bad_acl_file_content, {1, erl_parse, ["syntax error before: ", []]}},
+    BadRule = ?FILE_SOURCE(<<"{allow,{username,\"bar\"},publish}.">>),
+    BadRuleErr = {invalid_authorization_rule, {allow, {username, "bar"}, publish}},
+    BadPermission = ?FILE_SOURCE(<<"{not_allow,{username,\"bar\"},publish,[\"test\"]}.">>),
+    BadPermissionErr = {invalid_authorization_permission, not_allow},
+    BadAction = ?FILE_SOURCE(<<"{allow,{username,\"bar\"},pubsub,[\"test\"]}.">>),
+    BadActionErr = {invalid_authorization_action, pubsub},
+    lists:foreach(
+        fun({Source, Error}) ->
+            File = emqx_authz:acl_conf_file(),
+            {ok, Bin1} = file:read_file(File),
+            ?assertEqual(?UPDATE_ERROR(Error), emqx_authz:update(?CMD_REPLACE, [Source])),
+            ?assertEqual(?UPDATE_ERROR(Error), emqx_authz:update(?CMD_PREPEND, Source)),
+            ?assertEqual(?UPDATE_ERROR(Error), emqx_authz:update(?CMD_APPEND, Source)),
+            %% Check file content not changed if update failed
+            {ok, Bin2} = file:read_file(File),
+            ?assertEqual(Bin1, Bin2)
+        end,
+        [
+            {BadContent, BadContentErr},
+            {BadRule, BadRuleErr},
+            {BadPermission, BadPermissionErr},
+            {BadAction, BadActionErr}
+        ]
+    ),
+    ?assertMatch(
+        [],
+        emqx_conf:get([authorization, sources], [])
+    ).
+
 t_update_source(_) ->
     %% replace all
     {ok, _} = emqx_authz:update(?CMD_REPLACE, [?SOURCE3]),

apps/emqx_authz/test/emqx_authz_file_SUITE.erl

@@ -120,7 +120,9 @@ t_superuser(_Config) ->
 
 t_invalid_file(_Config) ->
     ?assertMatch(
-        {error, bad_acl_file_content},
+        {error,
+            {pre_config_update, emqx_authz,
+                {bad_acl_file_content, {1, erl_parse, ["syntax error before: ", "term"]}}}},
         emqx_authz:update(?CMD_REPLACE, [?RAW_SOURCE#{<<"rules">> => <<"{{invalid term">>}])
     ).
 

apps/emqx_conf/test/emqx_conf_schema_tests.erl

@@ -23,6 +23,7 @@
     """).
 
 array_nodes_test() ->
+    ensure_acl_conf(),
     ExpectNodes = ['emqx1@127.0.0.1', 'emqx2@127.0.0.1'],
     lists:foreach(
         fun(Nodes) ->
@@ -79,6 +80,7 @@ array_nodes_test() ->
 ).
 
 authn_validations_test() ->
+    ensure_acl_conf(),
     BaseConf = to_bin(?BASE_CONF, ["emqx1@127.0.0.1", "emqx1@127.0.0.1"]),
 
     OKHttps = to_bin(?BASE_AUTHN_ARRAY, [post, true, <<"https://127.0.0.1:8080">>]),
@@ -128,6 +130,7 @@ authn_validations_test() ->
 ).
 
 listeners_test() ->
+    ensure_acl_conf(),
     BaseConf = to_bin(?BASE_CONF, ["emqx1@127.0.0.1", "emqx1@127.0.0.1"]),
 
     Conf = <<BaseConf/binary, ?LISTENERS>>,
@@ -198,6 +201,7 @@ listeners_test() ->
     ok.
 
 doc_gen_test() ->
+    ensure_acl_conf(),
     %% the json file too large to encode.
     {
         timeout,
@@ -220,3 +224,11 @@ doc_gen_test() ->
 
 to_bin(Format, Args) ->
     iolist_to_binary(io_lib:format(Format, Args)).
+
+ensure_acl_conf() ->
+    File = emqx_schema:naive_env_interpolation(<<"${EMQX_ETC_DIR}/acl.conf">>),
+    ok = filelib:ensure_dir(filename:dirname(File)),
+    case filelib:is_regular(File) of
+        true -> ok;
+        false -> file:write_file(File, <<"">>)
+    end.

changes/ce/fix-10742.en.md

@@ -0,0 +1,2 @@
+Check the correctness of the rules before saving the authorization file source.
+Previously, Saving wrong rules could lead to restart failure.