Merge pull request #13678 from zmstone/0823-fix-authenticator-delete-idempotence

fix(authn): allow deleting an authenticator which failed to initialize

apps/emqx_auth/src/emqx_authn/emqx_authn_api.erl

@@ -221,8 +221,7 @@ schema("/authentication/:id") ->
             description => ?DESC(authentication_id_delete),
             parameters => [param_auth_id()],
             responses => #{
-                204 => <<"Authenticator deleted">>,
-                404 => error_codes([?NOT_FOUND], <<"Not Found">>)
+                204 => <<"Authenticator deleted">>
             }
         }
     };

apps/emqx_auth/src/emqx_authn/emqx_authn_chains.erl

@@ -587,8 +587,9 @@ handle_delete_authenticator(Chain, AuthenticatorID) ->
         ID =:= AuthenticatorID
     end,
     case do_delete_authenticators(MatchFun, Chain) of
-        {[], _NewChain} ->
-            {error, {not_found, {authenticator, AuthenticatorID}}};
+        {[], NewChain} ->
+            %% Idempotence intended
+            {ok, ok, NewChain};
         {[AuthenticatorID], NewChain} ->
             {ok, ok, NewChain}
     end.

apps/emqx_auth/test/emqx_authn/emqx_authn_api_SUITE.erl

@@ -409,7 +409,8 @@ test_authenticator(PathPrefix) ->
         ValidConfig2
     ),
 
-    {ok, 404, _} = request(
+    %% allow deletion of unknown (not created) authenticator
+    {ok, 204, _} = request(
         delete,
         uri(PathPrefix ++ [?CONF_NS, "password_based:redis"])
     ),
@@ -632,6 +633,12 @@ ignore_switch_to_global_chain(_) ->
         delete,
         uri([listeners, "tcp:default", ?CONF_NS, "password_based:http"])
     ),
+    %% Delete unknown should retun 204
+    %% There is not even a name validation for it.
+    {ok, 204, _} = request(
+        delete,
+        uri([listeners, "tcp:default", ?CONF_NS, "password_based:httpx"])
+    ),
 
     %% Local chain is empty now and should be removed
     %% Listener user should not be OK

apps/emqx_auth/test/emqx_authn/emqx_authn_chains_SUITE.erl

@@ -191,7 +191,16 @@ t_authenticator(Config) when is_list(Config) ->
         ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1)
     ),
 
+    %% delete an unknown authenticator is allowed, do not epxect not_found
+    ?assertEqual(ok, ?AUTHN:delete_authenticator(ChainName, <<"password_based:http">>)),
+    %% the deletion of the last authenticator in the chain should result in
+    %% an implict deletion of the chain
     ?assertEqual(ok, ?AUTHN:delete_authenticator(ChainName, ID1)),
+    %% expected not_found for the chain
+    ?assertEqual(
+        {error, {not_found, {chain, test}}},
+        ?AUTHN:update_authenticator(ChainName, ID1, AuthenticatorConfig1)
+    ),
 
     ?assertEqual(
         {error, {not_found, {chain, test}}},

changes/ce/fix-13678.en.md

@@ -0,0 +1 @@
+Allow deletion of non-existing authenticator.