From 96e7005de87ca0f42dfdc1aab4cf8cf021cf57c6 Mon Sep 17 00:00:00 2001
From: Zhongwen Deng <zhongwencool@gmail.com>
Date: Mon, 22 May 2023 11:03:23 +0800
Subject: [PATCH] refactor: add emqx_authz_file validate function

---
 apps/emqx_authz/src/emqx_authz.erl        |  5 +++--
 apps/emqx_authz/src/emqx_authz_file.erl   |  7 ++++++-
 apps/emqx_authz/src/emqx_authz_schema.erl | 12 ++----------
 apps/emqx_authz/test/emqx_authz_SUITE.erl |  7 ++++++-
 4 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/apps/emqx_authz/src/emqx_authz.erl b/apps/emqx_authz/src/emqx_authz.erl
index e7f59cb71..c7db65992 100644
--- a/apps/emqx_authz/src/emqx_authz.erl
+++ b/apps/emqx_authz/src/emqx_authz.erl
@@ -539,8 +539,9 @@ update_authz_chain(Actions) ->
 check_acl_file_rules(Path, Rules) ->
     TmpPath = Path ++ ".tmp",
     try
-        ok = write_file(Path, Rules),
-        emqx_authz_schema:validate_file_rules(Path)
+        ok = write_file(TmpPath, Rules),
+        {ok, _} = emqx_authz_file:validate(TmpPath),
+        ok
     catch
         throw:Reason -> throw(Reason)
     after
diff --git a/apps/emqx_authz/src/emqx_authz_file.erl b/apps/emqx_authz/src/emqx_authz_file.erl
index dfd28f0c0..317395a45 100644
--- a/apps/emqx_authz/src/emqx_authz_file.erl
+++ b/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
@@ -56,6 +57,10 @@ create(#{path := Path0} = Source) ->
                 ?SLOG(alert, #{msg => bad_acl_file_content, path => Path, reason => Reason}),
                 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) ->
diff --git a/apps/emqx_authz/src/emqx_authz_schema.erl b/apps/emqx_authz/src/emqx_authz_schema.erl
index 5f06f757c..8e847b93e 100644
--- a/apps/emqx_authz/src/emqx_authz_schema.erl
+++ b/apps/emqx_authz/src/emqx_authz_schema.erl
@@ -42,8 +42,7 @@
 
 -export([
     headers_no_content_type/1,
-    headers/1,
-    validate_file_rules/1
+    headers/1
 ]).
 
 %%--------------------------------------------------------------------
@@ -85,7 +84,7 @@ fields(file) ->
                     string(),
                     #{
                         required => true,
-                        validator => fun ?MODULE:validate_file_rules/1,
+                        validator => fun(Path) -> element(1, emqx_authz_file:validate(Path)) end,
                         desc => ?DESC(path)
                     }
                 )}
@@ -518,10 +517,3 @@ default_authz() ->
         <<"enable">> => true,
         <<"path">> => <<"${EMQX_ETC_DIR}/acl.conf">>
     }.
-
-validate_file_rules(Path) ->
-    %% Don't need assert the create result here, all error is thrown
-    %% some test mock the create function
-    %% #{annotations := #{rules := _}}
-    _ = emqx_authz_file:create(#{path => Path}),
-    ok.
diff --git a/apps/emqx_authz/test/emqx_authz_SUITE.erl b/apps/emqx_authz/test/emqx_authz_SUITE.erl
index 2c57d807a..9c1b7fd51 100644
--- a/apps/emqx_authz/test/emqx_authz_SUITE.erl
+++ b/apps/emqx_authz/test/emqx_authz_SUITE.erl
@@ -205,9 +205,14 @@ t_bad_file_source(_) ->
     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))
+            ?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},