Merge pull request #10056 from sstrigler/EMQX-9064-bridge-api-do-not-return-403-in-case-of-inconsistency-in-application-logic

Bridge API: do not return 403 in case of inconsistency in application logic

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")
             }
         }
@@ -383,7 +386,6 @@ schema("/bridges/:id/metrics/reset") ->
             parameters => [param_path_id()],
             responses => #{
                 204 => <<"Reset success">>,
-                400 => error_schema(['BAD_REQUEST'], "RPC Call Failed"),
                 404 => error_schema('NOT_FOUND', "Bridge not found")
             }
         }
@@ -438,8 +440,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 +518,11 @@ schema("/bridges_probe") ->
                     {ok, _} ->
                         204;
                     {error, {rules_deps_on_this_bridge, RuleIds}} ->
-                        %% [FIXME] this should be a 400 since '403' is about
+                        {400,
-                        %% authorization and not application logic.
-                        {403,
                             error_msg(
-                                'FORBIDDEN_REQUEST',
+                                'BAD_REQUEST',
-                                {<<"There're some rules dependent on this bridge">>, RuleIds}
+                                {<<"Can not delete bridge while active rules defined for this bridge">>,
+                                    RuleIds}
                             )};
                     {error, timeout} ->
                         {503, error_msg('SERVICE_UNAVAILABLE', <<"request timeout">>)};
@@ -539,13 +540,11 @@ schema("/bridges_probe") ->
 '/bridges/:id/metrics/reset'(put, #{bindings := #{id := Id}}) ->
     ?TRY_PARSE_ID(
         Id,
-        case
+        begin
-            emqx_bridge_resource:reset_metrics(
+            ok = emqx_bridge_resource:reset_metrics(
                 emqx_bridge_resource:resource_id(BridgeType, BridgeName)
-            )
+            ),
-        of
+            {204}
-            ok -> {204};
-            Reason -> {400, error_msg('BAD_REQUEST', Reason)}
         end
     ).
 
@@ -638,12 +637,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
+                        {400,
-                        %% logic.
-                        {403,
                             error_msg(
-                                'FORBIDDEN_REQUEST',
+                                'BAD_REQUEST',
-                                <<"forbidden operation: bridge disabled">>
+                                <<"Forbidden operation, bridge not enabled">>
                             )};
                     true ->
                         case emqx_misc:safe_to_existing_atom(Node, utf8) of

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
     ),
 

changes/ce/fix-10056.en.md

@@ -0,0 +1 @@
+`/bridges` API: return `400` instead of `403` in case of inconsistency in the application logic either because bridge is about to be deleted, but active rules still depend on it, or an operation (start|stop|restart) is called, but the bridge is not enabled.

changes/ce/fix-10056.zh.md

@@ -0,0 +1 @@
+`/bridges` API：在应用逻辑不一致的情况下，返回`400'而不是`403'，因为桥即将被删除，但活动规则仍然依赖于它，或者调用了一个操作（启动|停止|重新启动），但桥没有被启用。