From ae1346e3f0cb3f9ed5e664d49b547b8e76f5c02c Mon Sep 17 00:00:00 2001
From: JianBo He <heeejianbo@gmail.com>
Date: Thu, 16 Jun 2022 20:02:34 +0800
Subject: [PATCH] chore(gw): throw authn creation errors

---
 apps/emqx_gateway/src/emqx_gateway_http.erl   |  3 +
 .../src/emqx_gateway_insta_sup.erl            | 68 +++++++++++++------
 2 files changed, 51 insertions(+), 20 deletions(-)

diff --git a/apps/emqx_gateway/src/emqx_gateway_http.erl b/apps/emqx_gateway/src/emqx_gateway_http.erl
index fa5867962..07b0d0e36 100644
--- a/apps/emqx_gateway/src/emqx_gateway_http.erl
+++ b/apps/emqx_gateway/src/emqx_gateway_http.erl
@@ -556,6 +556,9 @@ with_gateway(GwName0, Fun) ->
             return_http_error(404, "Resource not found. path: " ++ Path);
         error:{badmatch, {error, einval}} ->
             return_http_error(400, "Invalid bind address");
+        error:{badauth, Reason} ->
+            Reason1 = emqx_gateway_utils:stringfy(Reason),
+            return_http_error(400, ["Bad authentication config: ", Reason1]);
         Class:Reason:Stk ->
             ?SLOG(error, #{
                 msg => "uncaught_exception",
diff --git a/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl b/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl
index c4b088db2..035c2d10f 100644
--- a/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl
+++ b/apps/emqx_gateway/src/emqx_gateway_insta_sup.erl
@@ -141,12 +141,16 @@ handle_call(disable, _From, State = #state{status = Status}) ->
 handle_call(enable, _From, State = #state{status = Status}) ->
     case Status of
         stopped ->
-            ok = ensure_authn_running(State),
-            case cb_gateway_load(State) of
+            case ensure_authn_running(State) of
+                ok ->
+                    case cb_gateway_load(State) of
+                        {error, Reason} ->
+                            {reply, {error, Reason}, State};
+                        {ok, NState1} ->
+                            {reply, ok, NState1}
+                    end;
                 {error, Reason} ->
-                    {reply, {error, Reason}, State};
-                {ok, NState1} ->
-                    {reply, ok, NState1}
+                    {reply, {error, Reason}, State}
             end;
         _ ->
             {reply, {error, already_started}, State}
@@ -238,12 +242,22 @@ detailed_gateway_info(State) ->
 %%--------------------------------------------------------------------
 %% Authn resources managing funcs
 
+pipeline(_, []) ->
+    ok;
+pipeline(Fun, [Args | More]) ->
+    case Fun(Args) of
+        ok ->
+            pipeline(Fun, More);
+        {error, Reason} ->
+            {error, Reason}
+    end.
+
 %% ensure authentication chain, authenticator created and keep its configured
 %% status
 ensure_authn_running(#state{name = GwName, config = Config}) ->
-    lists:foreach(
+    pipeline(
         fun({ChainName, AuthConf}) ->
-            ok = ensure_authenticator_created(ChainName, AuthConf)
+            ensure_authenticator_created(ChainName, AuthConf)
         end,
         authns(GwName, Config)
     ).
@@ -251,9 +265,9 @@ ensure_authn_running(#state{name = GwName, config = Config}) ->
 %% ensure authentication chain, authenticator created and keep its status
 %% as given
 ensure_authn_running(#state{name = GwName, config = Config}, Enable) ->
-    lists:foreach(
+    pipeline(
         fun({ChainName, AuthConf}) ->
-            ok = ensure_authenticator_created(ChainName, AuthConf#{enable => Enable})
+            ensure_authenticator_created(ChainName, AuthConf#{enable => Enable})
         end,
         authns(GwName, Config)
     ).
@@ -285,12 +299,14 @@ remove_all_authns(#state{name = GwName, config = Config}) ->
 ensure_authenticator_created(ChainName, Confs) ->
     case emqx_authentication:list_authenticators(ChainName) of
         {ok, [#{id := AuthenticatorId}]} ->
-            {ok, _} = emqx_authentication:update_authenticator(ChainName, AuthenticatorId, Confs),
-            ok;
+            case emqx_authentication:update_authenticator(ChainName, AuthenticatorId, Confs) of
+                {ok, _} -> ok;
+                {error, Reason} -> {error, {badauth, Reason}}
+            end;
         {ok, []} ->
-            ok = do_create_authenticator(ChainName, Confs);
+            do_create_authenticator(ChainName, Confs);
         {error, {not_found, {chain, _}}} ->
-            ok = do_create_authenticator(ChainName, Confs)
+            do_create_authenticator(ChainName, Confs)
     end.
 
 authns(GwName, Config) ->
@@ -328,7 +344,7 @@ do_create_authenticator(ChainName, AuthConf) ->
                 reason => Reason,
                 config => AuthConf
             }),
-            throw({badauth, Reason})
+            {error, {badauth, Reason}}
     end.
 
 do_update_one_by_one(
@@ -348,15 +364,27 @@ do_update_one_by_one(
 
     case {Status, NEnable} of
         {stopped, true} ->
-            ok = ensure_authn_running(State#state{config = NCfg}),
-            cb_gateway_load(State#state{config = NCfg});
+            case ensure_authn_running(State#state{config = NCfg}) of
+                ok ->
+                    cb_gateway_load(State#state{config = NCfg});
+                {error, Reason} ->
+                    {error, Reason}
+            end;
         {stopped, false} ->
-            ok = disable_authns(State#state{config = NCfg}),
-            {ok, State#state{config = NCfg}};
+            case disable_authns(State#state{config = NCfg}) of
+                ok ->
+                    {ok, State#state{config = NCfg}};
+                {error, Reason} ->
+                    {error, Reason}
+            end;
         {running, true} ->
             %% FIXME: minimum impact update
-            ok = ensure_authn_running(State#state{config = NCfg}),
-            cb_gateway_update(NCfg, State);
+            case ensure_authn_running(State#state{config = NCfg}) of
+                ok ->
+                    cb_gateway_update(NCfg, State);
+                {error, Reason} ->
+                    {error, Reason}
+            end;
         {running, false} ->
             case cb_gateway_unload(State) of
                 {ok, NState} ->