Merge pull request #6591 from zhongwencool/api-key-update-unexpired

fix(api_key): set api_key unexpired when update expired_at=undefined

.github/workflows/run_static_checks.yaml

@@ -0,0 +1,23 @@
+name: Run static checks
+
+concurrency:
+  group: static-check-${{ github.event_name }}-${{ github.ref }}
+  cancel-in-progress: true
+
+on:
+  push:
+    tags:
+      - v*
+      - e*
+  pull_request:
+
+jobs:
+    run_static_analysis:
+      runs-on: self-hosted
+      container: "ghcr.io/emqx/emqx-builder/5.0-3:24.1.5-3-alpine3.14"
+      steps:
+      - uses: actions/checkout@v2
+      - name: xref
+        run: make xref
+      - name: dialyzer
+        run: make dialyzer

.github/workflows/run_test_cases.yaml

@@ -12,22 +12,6 @@ on:
   pull_request:
 
 jobs:
-    run_static_analysis:
-        strategy:
-          matrix:
-            emqx_builder:
-              - 5.0-3:24.1.5-3 # run dialyzer on latest OTP
-
-        runs-on: ubuntu-20.04
-        container: "ghcr.io/emqx/emqx-builder/${{ matrix.emqx_builder }}-ubuntu20.04"
-
-        steps:
-        - uses: actions/checkout@v2
-        - name: xref
-          run: make xref
-        - name: dialyzer
-          run: make dialyzer
-
     run_proper_test:
         strategy:
           matrix:

apps/emqx_management/src/emqx_mgmt_api_app.erl

@@ -139,13 +139,7 @@ api_key(post, #{body := App}) ->
         <<"desc">> := Desc0,
         <<"enable">> := Enable
     } = App,
-    %% undefined is never expired
-    ExpiredAt0 = maps:get(<<"expired_at">>, App, <<"undefined">>),
-    ExpiredAt =
-        case ExpiredAt0 of
-            <<"undefined">> -> undefined;
-            _ -> ExpiredAt0
-        end,
+    ExpiredAt = ensure_expired_at(App),
     Desc = unicode:characters_to_binary(Desc0, unicode),
     case emqx_mgmt_auth:create(Name, Enable, ExpiredAt, Desc) of
         {ok, NewApp} -> {200, format(NewApp)};
@@ -164,7 +158,7 @@ api_key_by_name(delete, #{bindings := #{name := Name}}) ->
     end;
 api_key_by_name(put, #{bindings := #{name := Name}, body := Body}) ->
     Enable = maps:get(<<"enable">>, Body, undefined),
-    ExpiredAt = maps:get(<<"expired_at">>, Body, undefined),
+    ExpiredAt = ensure_expired_at(Body),
     Desc = maps:get(<<"desc">>, Body, undefined),
     case emqx_mgmt_auth:update(Name, Enable, ExpiredAt, Desc) of
         {ok, App} -> {200, format(App)};
@@ -181,3 +175,6 @@ format(App = #{expired_at := ExpiredAt0, created_at := CreateAt}) ->
         expired_at => ExpiredAt,
         created_at => list_to_binary(calendar:system_time_to_rfc3339(CreateAt))
     }.
+
+ensure_expired_at(#{<<"expired_at">> := ExpiredAt})when is_integer(ExpiredAt) -> ExpiredAt;
+ensure_expired_at(_) -> undefined.

apps/emqx_management/src/emqx_mgmt_api_trace.erl

@@ -304,13 +304,16 @@ download_trace_log(get, #{bindings := #{name := Name}}) ->
             Zips = group_trace_file(ZipDir, TraceLog, TraceFiles),
             ZipFileName = ZipDir ++ binary_to_list(Name) ++ ".zip",
             {ok, ZipFile} = zip:zip(ZipFileName, Zips, [{cwd, ZipDir}]),
-            emqx_trace:delete_files_after_send(ZipFileName, Zips),
+            %% emqx_trace:delete_files_after_send(ZipFileName, Zips),
+            %% TODO use file replace file_binary.(delete file after send is not ready now).
+            {ok, Binary} = file:read_file(ZipFile),
+            ZipName = filename:basename(ZipFile),
+            _ = file:delete(ZipFile),
             Headers = #{
                 <<"content-type">> => <<"application/x-zip">>,
-                <<"content-disposition">> =>
-                iolist_to_binary("attachment; filename=" ++ filename:basename(ZipFile))
+                <<"content-disposition">> => iolist_to_binary("attachment; filename=" ++ ZipName)
             },
-            {200, Headers, {file, ZipFile}};
+            {200, Headers, {file_binary, ZipName, Binary}};
         {error, not_found} -> ?NOT_FOUND(Name)
     end.
 

apps/emqx_management/src/emqx_mgmt_auth.erl

@@ -68,11 +68,11 @@ update(Name, Enable, ExpiredAt, Desc) ->
     Fun = fun() ->
         case mnesia:read(?APP, Name, write) of
             [] -> mnesia:abort(not_found);
-            [App0 = #?APP{enable = Enable0, expired_at = ExpiredAt0, desc = Desc0}] ->
+            [App0 = #?APP{enable = Enable0, desc = Desc0}] ->
                 App =
                     App0#?APP{
+                        expired_at = ExpiredAt,
                         enable = ensure_not_undefined(Enable, Enable0),
-                        expired_at = ensure_not_undefined(ExpiredAt, ExpiredAt0),
                         desc = ensure_not_undefined(Desc, Desc0)
                     },
                 ok = mnesia:write(App),

apps/emqx_management/test/emqx_mgmt_auth_api_SUITE.erl

@@ -96,6 +96,13 @@ t_update(_Config) ->
     ?assertEqual(calendar:rfc3339_to_system_time(binary_to_list(ExpiredAt)),
         calendar:rfc3339_to_system_time(binary_to_list(maps:get(<<"expired_at">>, Update1)))
     ),
+    Unexpired1 = maps:without([expired_at], Change),
+    {ok, Update2} = update_app(Name, Unexpired1),
+    ?assertEqual(<<"undefined">>, maps:get(<<"expired_at">>, Update2)),
+    Unexpired2 = Change#{expired_at => <<"undefined">>},
+    {ok, Update3} = update_app(Name, Unexpired2),
+    ?assertEqual(<<"undefined">>, maps:get(<<"expired_at">>, Update3)),
+
     ?assertEqual({error, {"HTTP/1.1", 404, "Not Found"}}, update_app(<<"Not-Exist">>, Change)),
     ok.
 
@@ -137,6 +144,10 @@ t_authorize(_Config) ->
     },
     ?assertMatch({ok, #{<<"api_key">> := _, <<"enable">> := true}}, update_app(Name, Expired)),
     ?assertEqual(Unauthorized, emqx_mgmt_api_test_util:request_api(get, BanPath, BasicHeader)),
+    UnExpired = #{expired_at => undefined},
+    ?assertMatch({ok, #{<<"api_key">> := _, <<"expired_at">> := <<"undefined">>}},
+        update_app(Name, UnExpired)),
+    {ok, _Status1} = emqx_mgmt_api_test_util:request_api(get, BanPath, BasicHeader),
     ok.
 
 t_create_unexpired_app(_Config) ->

apps/emqx_statsd/src/emqx_statsd_api.erl

@@ -59,10 +59,12 @@ statsd(put, #{body := Body}) ->
                             Body,
                             #{rawconf_with_defaults => true, override_to => cluster}) of
         {ok, #{raw_config := NewConfig, config := Config}} ->
-            _ = emqx_statsd_sup:stop_child(?APP),
+            ok = emqx_statsd_sup:ensure_child_stopped(?APP),
             case maps:get(<<"enable">>, Body) of
-                true -> emqx_statsd_sup:start_child(?APP, maps:get(config, Config));
-                false -> ok
+                true ->
+                    ok = emqx_statsd_sup:ensure_child_started(?APP, maps:get(config, Config));
+                false ->
+                    ok
             end,
             {200, NewConfig};
         {error, Reason} ->

apps/emqx_statsd/src/emqx_statsd_app.erl

@@ -34,7 +34,7 @@ stop(_) ->
 maybe_enable_statsd() ->
     case emqx_conf:get([statsd, enable], false) of
         true ->
-            emqx_statsd_sup:start_child(?APP, emqx_conf:get([statsd], #{}));
+            emqx_statsd_sup:ensure_child_started(?APP, emqx_conf:get([statsd], #{}));
         false ->
             ok
     end.

apps/emqx_statsd/src/emqx_statsd_sup.erl

@@ -8,9 +8,9 @@
 -behaviour(supervisor).
 
 -export([ start_link/0
-        , start_child/1
-        , start_child/2
-        , stop_child/1
+        , ensure_child_started/1
+        , ensure_child_started/2
+        , ensure_child_stopped/1
         ]).
 
 -export([init/1]).
@@ -26,19 +26,24 @@
 start_link() ->
     supervisor:start_link({local, ?MODULE}, ?MODULE, []).
 
--spec start_child(supervisor:child_spec()) -> ok.
-start_child(ChildSpec) when is_map(ChildSpec) ->
+-spec ensure_child_started(supervisor:child_spec()) -> ok.
+ensure_child_started(ChildSpec) when is_map(ChildSpec) ->
     assert_started(supervisor:start_child(?MODULE, ChildSpec)).
 
--spec start_child(atom(), map()) -> ok.
-start_child(Mod, Opts) when is_atom(Mod) andalso is_map(Opts) ->
+-spec ensure_child_started(atom(), map()) -> ok.
+ensure_child_started(Mod, Opts) when is_atom(Mod) andalso is_map(Opts) ->
     assert_started(supervisor:start_child(?MODULE, ?CHILD(Mod, Opts))).
 
--spec(stop_child(any()) -> ok | {error, term()}).
-stop_child(ChildId) ->
+%% @doc Stop the child worker process.
+-spec ensure_child_stopped(any()) -> ok.
+ensure_child_stopped(ChildId) ->
     case supervisor:terminate_child(?MODULE, ChildId) of
-        ok -> supervisor:delete_child(?MODULE, ChildId);
-        Error -> Error
+        ok ->
+            %% with terminate_child/2 returned 'ok', it's not possible
+            %% for supervisor:delete_child/2 to return {error, Reason}
+            ok = supervisor:delete_child(?MODULE, ChildId);
+        {error, not_found} ->
+            ok
     end.
 
 init([]) ->
@@ -50,5 +55,5 @@ init([]) ->
 
 assert_started({ok, _Pid}) -> ok;
 assert_started({ok, _Pid, _Info}) -> ok;
-assert_started({error, {already_tarted, _Pid}}) -> ok;
+assert_started({error, {already_started, _Pid}}) -> ok;
 assert_started({error, Reason}) -> erlang:error(Reason).