From 4c23ab097da5cdbf8ec624c88b82f10535b14bd9 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Thu, 2 Mar 2023 09:54:29 +0100 Subject: [PATCH 1/4] 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 ), From 39e1cf95021ee196e5b5ec108a3a08276c186899 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Thu, 2 Mar 2023 09:55:38 +0100 Subject: [PATCH 2/4] fix(emqx_bridge): let it crash instead of 400 on failed reset --- apps/emqx_bridge/src/emqx_bridge_api.erl | 11 ++++------- 1 file changed, 4 insertions(+), 7 deletions(-) diff --git a/apps/emqx_bridge/src/emqx_bridge_api.erl b/apps/emqx_bridge/src/emqx_bridge_api.erl index 708c3ac0b..871def0d6 100644 --- a/apps/emqx_bridge/src/emqx_bridge_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_api.erl @@ -386,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") } } @@ -541,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 ). From 57ea098d90f7d74f2a63ce7232f62f206873895e Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Thu, 2 Mar 2023 11:30:38 +0100 Subject: [PATCH 3/4] chore: add changelog --- changes/ce/fix-10056.en.md | 1 + changes/ce/fix-10056.zh.md | 1 + 2 files changed, 2 insertions(+) create mode 100644 changes/ce/fix-10056.en.md create mode 100644 changes/ce/fix-10056.zh.md diff --git a/changes/ce/fix-10056.en.md b/changes/ce/fix-10056.en.md new file mode 100644 index 000000000..7a119df20 --- /dev/null +++ b/changes/ce/fix-10056.en.md @@ -0,0 +1 @@ +`/bridges` API: return `400` instead of `403` in case of inconsistency in 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'，因为桥即将被删除，但活动规则仍然依赖于它，或者调用了一个操作（启动|停止|重新启动），但桥没有被启用。 From 8bdef1300b60c4de1ad7466a1dcfd8dc4c427234 Mon Sep 17 00:00:00 2001 From: Stefan Strigler Date: Thu, 2 Mar 2023 12:16:17 +0100 Subject: [PATCH 4/4] style: fix punctuation changes/ce/fix-10056.en.md Co-authored-by: ieQu1 <99872536+ieQu1@users.noreply.github.com> --- changes/ce/fix-10056.en.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/changes/ce/fix-10056.en.md b/changes/ce/fix-10056.en.md index 7a119df20..ab9b980e8 100644 --- a/changes/ce/fix-10056.en.md +++ b/changes/ce/fix-10056.en.md @@ -1 +1 @@ -`/bridges` API: return `400` instead of `403` in case of inconsistency in 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. +`/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.