From 4c23ab097da5cdbf8ec624c88b82f10535b14bd9 Mon Sep 17 00:00:00 2001
From: Stefan Strigler <stefan.strigler@emqx.io>
Date: Thu, 2 Mar 2023 09:54:29 +0100
Subject: [PATCH] fix(emqx_bridge): return 400 if operation not possible

---
 apps/emqx_bridge/src/emqx_bridge_api.erl      | 24 +++++++++----------
 .../test/emqx_bridge_api_SUITE.erl            |  6 ++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl
index 8259e4ee5..708c3ac0b 100644
--- a/apps/emqx_bridge/src/emqx_bridge_api.erl
+++ b/apps/emqx_bridge/src/emqx_bridge_api.erl
@@ -353,8 +353,11 @@ schema("/bridges/:id") ->
             parameters => [param_path_id()],
             responses => #{
                 204 => <<"Bridge deleted">>,
+                400 => error_schema(
+                    'BAD_REQUEST',
+                    "Can not delete bridge while active rules defined for this bridge"
+                ),
                 404 => error_schema('NOT_FOUND', "Bridge not found"),
-                403 => error_schema('FORBIDDEN_REQUEST', "Forbidden operation"),
                 503 => error_schema('SERVICE_UNAVAILABLE', "Service unavailable")
             }
         }
@@ -438,8 +441,8 @@ schema("/nodes/:node/bridges/:id/:operation") ->
             ],
             responses => #{
                 204 => <<"Operation success">>,
+                400 => error_schema('BAD_REQUEST', "Forbidden operation, bridge not enabled"),
                 404 => error_schema('NOT_FOUND', "Bridge not found or invalid operation"),
-                403 => error_schema('FORBIDDEN_REQUEST', "forbidden operation"),
                 501 => error_schema('NOT_IMPLEMENTED', "Not Implemented"),
                 503 => error_schema('SERVICE_UNAVAILABLE', "Service unavailable")
             }
@@ -516,12 +519,11 @@ schema("/bridges_probe") ->
                     {ok, _} ->
                         204;
                     {error, {rules_deps_on_this_bridge, RuleIds}} ->
-                        %% [FIXME] this should be a 400 since '403' is about
-                        %% authorization and not application logic.
-                        {403,
+                        {400,
                             error_msg(
-                                'FORBIDDEN_REQUEST',
-                                {<<"There're some rules dependent on this bridge">>, RuleIds}
+                                'BAD_REQUEST',
+                                {<<"Can not delete bridge while active rules defined for this bridge">>,
+                                    RuleIds}
                             )};
                     {error, timeout} ->
                         {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
@@ -638,12 +640,10 @@ lookup_from_local_node(BridgeType, BridgeName) ->
                 ConfMap = emqx:get_config([bridges, BridgeType, BridgeName]),
                 case maps:get(enable, ConfMap, false) of
                     false ->
-                        %% [FIXME] `403` is about authorization not application
-                        %% logic.
-                        {403,
+                        {400,
                             error_msg(
-                                'FORBIDDEN_REQUEST',
-                                <<"forbidden operation: bridge disabled">>
+                                'BAD_REQUEST',
+                                <<"Forbidden operation, bridge not enabled">>
                             )};
                     true ->
                         case emqx_misc:safe_to_existing_atom(Node, utf8) of
diff --git a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl
index 14a7aafb8..727764d73 100644
--- a/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl
+++ b/apps/emqx_bridge/test/emqx_bridge_api_SUITE.erl
@@ -403,7 +403,7 @@ t_check_dependent_actions_on_delete(Config) ->
     ),
     #{<<"id">> := RuleId} = jsx:decode(Rule),
     %% delete the bridge should fail because there is a rule depenents on it
-    {ok, 403, _} = request(delete, uri(["bridges", BridgeID]), []),
+    {ok, 400, _} = request(delete, uri(["bridges", BridgeID]), []),
     %% delete the rule first
     {ok, 204, <<>>} = request(delete, uri(["rules", RuleId]), []),
     %% then delete the bridge is OK
@@ -601,9 +601,9 @@ t_enable_disable_bridges(Config) ->
     %% disable it again
     {ok, 204, <<>>} = request(put, enable_path(false, BridgeID), <<"">>),
 
-    {ok, 403, Res} = request(post, operation_path(node, restart, BridgeID), <<"">>),
+    {ok, 400, Res} = request(post, operation_path(node, restart, BridgeID), <<"">>),
     ?assertEqual(
-        <<"{\"code\":\"FORBIDDEN_REQUEST\",\"message\":\"forbidden operation: bridge disabled\"}">>,
+        <<"{\"code\":\"BAD_REQUEST\",\"message\":\"Forbidden operation, bridge not enabled\"}">>,
         Res
     ),