fix: check authz's file rule before save to file

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,12 @@ 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(),
+    ok = check_acl_file_rules(AclPath, Rules),
-    {ok, _Filename} = write_file(Path, NRules),
+    ok = write_file(AclPath, Rules),
-    maps:without([<<"rules">>], Source#{<<"path">> => Path}).
+    Source1 = maps:remove(<<"rules">>, Source0),
+    maps:put(<<"path">>, AclPath, Source1).
 
 %% @doc where the acl.conf file is stored.
 acl_conf_file() ->
@@ -506,7 +512,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 +534,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(Path, Rules),
+        #{annotations := #{rules := _}} = emqx_authz_file:create(#{path => Path}),
+        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

@@ -54,7 +54,7 @@ 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,
     Source#{annotations => #{rules => Rules}}.
 

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/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,35 @@ 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}) ->
+            ?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))
+        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">>}])
     ).
 

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.