diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index 8259e4ee5..871def0d6 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") } } @@ -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 - %% 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">>)}; @@ -539,13 +540,11 @@ schema("/bridges_probe") -> '/bridges/:id/metrics/reset'(put, #{bindings := #{id := Id}}) -> ?TRY_PARSE_ID( Id, - case - emqx_bridge_resource:reset_metrics( + begin + ok = emqx_bridge_resource:reset_metrics( emqx_bridge_resource:resource_id(BridgeType, BridgeName) - ) - of - ok -> {204}; - Reason -> {400, error_msg('BAD_REQUEST', Reason)} + ), + {204} 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 - %% 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 ), diff --git a/changes/ce/fix-10056.en.md b/changes/ce/fix-10056.en.md new file mode 100644 index 000000000..ab9b980e8 --- /dev/null +++ b/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. diff --git a/changes/ce/fix-10056.zh.md b/changes/ce/fix-10056.zh.md new file mode 100644 index 000000000..4d3317165 --- /dev/null +++ b/changes/ce/fix-10056.zh.md @@ -0,0 +1 @@ +`/bridges` API:在应用逻辑不一致的情况下,返回`400'而不是`403',因为桥即将被删除,但活动规则仍然依赖于它,或者调用了一个操作(启动|停止|重新启动),但桥没有被启用。