From 600747b7e584e51c7da069b0cab152c2b9311b2a Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Fri, 3 Nov 2023 20:44:57 +0100 Subject: [PATCH] fix(bridge): do not allow dot in bridge name also validate name at the API entry --- apps/emqx_bridge/src/emqx_bridge_resource.erl | 57 ++----------- apps/emqx_bridge/src/emqx_bridge_v2_api.erl | 16 ++++ .../test/emqx_bridge_v2_api_SUITE.erl | 3 +- .../test/emqx_bridge_v2_dummy_connector.erl | 31 +++++++ .../src/emqx_connector_resource.erl | 64 +++----------- .../test/emqx_connector_SUITE.erl | 2 +- .../test/emqx_connector_dummy_impl.erl | 31 +++++++ apps/emqx_resource/src/emqx_resource.erl | 83 +++++++++++++++++++ 8 files changed, 183 insertions(+), 104 deletions(-) create mode 100644 apps/emqx_bridge/test/emqx_bridge_v2_dummy_connector.erl create mode 100644 apps/emqx_connector/test/emqx_connector_dummy_impl.erl diff --git a/apps/emqx_bridge/src/emqx_bridge_resource.erl b/apps/emqx_bridge/src/emqx_bridge_resource.erl index 1c5365bc0..c7646faf4 100644 --- a/apps/emqx_bridge/src/emqx_bridge_resource.erl +++ b/apps/emqx_bridge/src/emqx_bridge_resource.erl @@ -102,21 +102,15 @@ bridge_id(BridgeType, BridgeName) -> <>. parse_bridge_id(BridgeId) -> - parse_bridge_id(BridgeId, #{atom_name => true}). + parse_bridge_id(bin(BridgeId), #{atom_name => true}). --spec parse_bridge_id(list() | binary() | atom(), #{atom_name => boolean()}) -> +-spec parse_bridge_id(binary() | atom(), #{atom_name => boolean()}) -> {atom(), atom() | binary()}. +parse_bridge_id(<<"bridge:", ID/binary>>, Opts) -> + parse_bridge_id(ID, Opts); parse_bridge_id(BridgeId, Opts) -> - case string:split(bin(BridgeId), ":", all) of - [Type, Name] -> - {to_type_atom(Type), validate_name(Name, Opts)}; - [Bridge, Type, Name] when Bridge =:= <<"bridge">>; Bridge =:= "bridge" -> - {to_type_atom(Type), validate_name(Name, Opts)}; - _ -> - invalid_data( - <<"should be of pattern {type}:{name}, but got ", BridgeId/binary>> - ) - end. + {Type, Name} = emqx_resource:parse_resource_id(BridgeId, Opts), + {emqx_bridge_lib:upgrade_type(Type), Name}. bridge_hookpoint(BridgeId) -> <<"$bridges/", (bin(BridgeId))/binary>>. @@ -126,48 +120,9 @@ bridge_hookpoint_to_bridge_id(?BRIDGE_HOOKPOINT(BridgeId)) -> bridge_hookpoint_to_bridge_id(_) -> {error, bad_bridge_hookpoint}. -validate_name(Name0, Opts) -> - Name = unicode:characters_to_list(Name0, utf8), - case is_list(Name) andalso Name =/= [] of - true -> - case lists:all(fun is_id_char/1, Name) of - true -> - case maps:get(atom_name, Opts, true) of - % NOTE - % Rule may be created before bridge, thus not `list_to_existing_atom/1`, - % also it is infrequent user input anyway. - true -> list_to_atom(Name); - false -> Name0 - end; - false -> - invalid_data(<<"bad name: ", Name0/binary>>) - end; - false -> - invalid_data(<<"only 0-9a-zA-Z_-. is allowed in name: ", Name0/binary>>) - end. - -spec invalid_data(binary()) -> no_return(). invalid_data(Reason) -> throw(#{kind => validation_error, reason => Reason}). -is_id_char(C) when C >= $0 andalso C =< $9 -> true; -is_id_char(C) when C >= $a andalso C =< $z -> true; -is_id_char(C) when C >= $A andalso C =< $Z -> true; -is_id_char($_) -> true; -is_id_char($-) -> true; -is_id_char($.) -> true; -is_id_char(_) -> false. - -to_type_atom(<<"kafka">>) -> - %% backward compatible - kafka_producer; -to_type_atom(Type) -> - try - erlang:binary_to_existing_atom(Type, utf8) - catch - _:_ -> - invalid_data(<<"unknown bridge type: ", Type/binary>>) - end. - reset_metrics(ResourceId) -> %% TODO we should not create atoms here {Type, Name} = parse_bridge_id(ResourceId), diff --git a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl index 0375b38bb..c3bfef644 100644 --- a/apps/emqx_bridge/src/emqx_bridge_v2_api.erl +++ b/apps/emqx_bridge/src/emqx_bridge_v2_api.erl @@ -742,6 +742,22 @@ update_bridge(BridgeType, BridgeName, Conf) -> create_or_update_bridge(BridgeType, BridgeName, Conf, 200). create_or_update_bridge(BridgeType, BridgeName, Conf, HttpStatusCode) -> + Check = + try + is_binary(BridgeType) andalso emqx_resource:validate_type(BridgeType), + ok = emqx_resource:validate_name(BridgeName) + catch + throw:Error -> + ?BAD_REQUEST(map_to_json(Error)) + end, + case Check of + ok -> + do_create_or_update_bridge(BridgeType, BridgeName, Conf, HttpStatusCode); + BadRequest -> + BadRequest + end. + +do_create_or_update_bridge(BridgeType, BridgeName, Conf, HttpStatusCode) -> case emqx_bridge_v2:create(BridgeType, BridgeName, Conf) of {ok, _} -> lookup_from_all_nodes(BridgeType, BridgeName, HttpStatusCode); diff --git a/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl b/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl index c7bcbf41a..592c2f96c 100644 --- a/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl +++ b/apps/emqx_bridge/test/emqx_bridge_v2_api_SUITE.erl @@ -258,7 +258,7 @@ end_per_testcase(_TestCase, Config) -> ok = emqx_common_test_helpers:call_janitor(), ok. --define(CONNECTOR_IMPL, dummy_connector_impl). +-define(CONNECTOR_IMPL, emqx_bridge_v2_dummy_connector). init_mocks() -> meck:new(emqx_connector_ee_schema, [passthrough, no_link]), meck:expect(emqx_connector_ee_schema, resource_type, 1, ?CONNECTOR_IMPL), @@ -534,6 +534,7 @@ t_bridges_lifecycle(Config) -> %% Try create bridge with bad characters as name {ok, 400, _} = request(post, uri([?ROOT]), ?KAFKA_BRIDGE(<<"隋达"/utf8>>), Config), + {ok, 400, _} = request(post, uri([?ROOT]), ?KAFKA_BRIDGE(<<"a.b">>), Config), ok. t_start_bridge_unknown_node(Config) -> diff --git a/apps/emqx_bridge/test/emqx_bridge_v2_dummy_connector.erl b/apps/emqx_bridge/test/emqx_bridge_v2_dummy_connector.erl new file mode 100644 index 000000000..c5ab48a85 --- /dev/null +++ b/apps/emqx_bridge/test/emqx_bridge_v2_dummy_connector.erl @@ -0,0 +1,31 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +%% this module is only intended to be mocked +-module(emqx_bridge_v2_dummy_connector). + +-export([ + callback_mode/0, + on_start/2, + on_stop/2, + on_add_channel/4, + on_get_channel_status/3 +]). + +callback_mode() -> error(unexpected). +on_start(_, _) -> error(unexpected). +on_stop(_, _) -> error(unexpected). +on_add_channel(_, _, _, _) -> error(unexpected). +on_get_channel_status(_, _, _) -> error(unexpected). diff --git a/apps/emqx_connector/src/emqx_connector_resource.erl b/apps/emqx_connector/src/emqx_connector_resource.erl index 0b48869fb..bc648a102 100644 --- a/apps/emqx_connector/src/emqx_connector_resource.erl +++ b/apps/emqx_connector/src/emqx_connector_resource.erl @@ -95,20 +95,14 @@ connector_id(ConnectorType, ConnectorName) -> parse_connector_id(ConnectorId) -> parse_connector_id(ConnectorId, #{atom_name => true}). --spec parse_connector_id(list() | binary() | atom(), #{atom_name => boolean()}) -> +-spec parse_connector_id(binary() | atom(), #{atom_name => boolean()}) -> {atom(), atom() | binary()}. +parse_connector_id(<<"connector:", ConnectorId/binary>>, Opts) -> + parse_connector_id(ConnectorId, Opts); +parse_connector_id(<>, Opts) -> + parse_connector_id(ConnectorId, Opts); parse_connector_id(ConnectorId, Opts) -> - case string:split(bin(ConnectorId), ":", all) of - [Type, Name] -> - {to_type_atom(Type), validate_name(Name, Opts)}; - [_, Type, Name] -> - {to_type_atom(Type), validate_name(Name, Opts)}; - _ -> - invalid_data( - <<"should be of pattern {type}:{name} or connector:{type}:{name}, but got ", - ConnectorId/binary>> - ) - end. + emqx_resource:parse_resource_id(ConnectorId, Opts). connector_hookpoint(ConnectorId) -> <<"$connectors/", (bin(ConnectorId))/binary>>. @@ -118,45 +112,6 @@ connector_hookpoint_to_connector_id(?BRIDGE_HOOKPOINT(ConnectorId)) -> connector_hookpoint_to_connector_id(_) -> {error, bad_connector_hookpoint}. -validate_name(Name0, Opts) -> - Name = unicode:characters_to_list(Name0, utf8), - case is_list(Name) andalso Name =/= [] of - true -> - case lists:all(fun is_id_char/1, Name) of - true -> - case maps:get(atom_name, Opts, true) of - % NOTE - % Rule may be created before connector, thus not `list_to_existing_atom/1`, - % also it is infrequent user input anyway. - true -> list_to_atom(Name); - false -> Name0 - end; - false -> - invalid_data(<<"bad name: ", Name0/binary>>) - end; - false -> - invalid_data(<<"only 0-9a-zA-Z_-. is allowed in name: ", Name0/binary>>) - end. - --spec invalid_data(binary()) -> no_return(). -invalid_data(Reason) -> throw(#{kind => validation_error, reason => Reason}). - -is_id_char(C) when C >= $0 andalso C =< $9 -> true; -is_id_char(C) when C >= $a andalso C =< $z -> true; -is_id_char(C) when C >= $A andalso C =< $Z -> true; -is_id_char($_) -> true; -is_id_char($-) -> true; -is_id_char($.) -> true; -is_id_char(_) -> false. - -to_type_atom(Type) -> - try - erlang:binary_to_existing_atom(Type, utf8) - catch - _:_ -> - invalid_data(<<"unknown connector type: ", Type/binary>>) - end. - restart(Type, Name) -> emqx_resource:restart(resource_id(Type, Name)). @@ -416,6 +371,13 @@ parse_url(Url) -> invalid_data(<<"Missing scheme in URL: ", Url/binary>>) end. +-spec invalid_data(binary()) -> no_return(). +invalid_data(Msg) -> + throw(#{ + kind => validation_error, + reason => Msg + }). + bin(Bin) when is_binary(Bin) -> Bin; bin(Str) when is_list(Str) -> list_to_binary(Str); bin(Atom) when is_atom(Atom) -> atom_to_binary(Atom, utf8). diff --git a/apps/emqx_connector/test/emqx_connector_SUITE.erl b/apps/emqx_connector/test/emqx_connector_SUITE.erl index ee1f7a46e..a62b5ed95 100644 --- a/apps/emqx_connector/test/emqx_connector_SUITE.erl +++ b/apps/emqx_connector/test/emqx_connector_SUITE.erl @@ -22,7 +22,7 @@ -include_lib("common_test/include/ct.hrl"). -define(START_APPS, [emqx, emqx_conf, emqx_connector]). --define(CONNECTOR, dummy_connector_impl). +-define(CONNECTOR, emqx_connector_dummy_impl). all() -> emqx_common_test_helpers:all(?MODULE). diff --git a/apps/emqx_connector/test/emqx_connector_dummy_impl.erl b/apps/emqx_connector/test/emqx_connector_dummy_impl.erl new file mode 100644 index 000000000..9dca42868 --- /dev/null +++ b/apps/emqx_connector/test/emqx_connector_dummy_impl.erl @@ -0,0 +1,31 @@ +%%-------------------------------------------------------------------- +%% Copyright (c) 2020-2023 EMQ Technologies Co., Ltd. All Rights Reserved. +%% +%% Licensed under the Apache License, Version 2.0 (the "License"); +%% you may not use this file except in compliance with the License. +%% You may obtain a copy of the License at +%% http://www.apache.org/licenses/LICENSE-2.0 +%% +%% Unless required by applicable law or agreed to in writing, software +%% distributed under the License is distributed on an "AS IS" BASIS, +%% WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +%% See the License for the specific language governing permissions and +%% limitations under the License. +%%-------------------------------------------------------------------- + +%% this module is only intended to be mocked +-module(emqx_connector_dummy_impl). + +-export([ + callback_mode/0, + on_start/2, + on_stop/2, + on_add_channel/4, + on_get_channel_status/3 +]). + +callback_mode() -> error(unexpected). +on_start(_, _) -> error(unexpected). +on_stop(_, _) -> error(unexpected). +on_add_channel(_, _, _, _) -> error(unexpected). +on_get_channel_status(_, _, _) -> error(unexpected). diff --git a/apps/emqx_resource/src/emqx_resource.erl b/apps/emqx_resource/src/emqx_resource.erl index cb3eb44fe..b0c0ed377 100644 --- a/apps/emqx_resource/src/emqx_resource.erl +++ b/apps/emqx_resource/src/emqx_resource.erl @@ -139,6 +139,13 @@ -export([apply_reply_fun/2]). +%% common validations +-export([ + parse_resource_id/2, + validate_type/1, + validate_name/1 +]). + -export_type([ query_mode/0, resource_id/0, @@ -776,3 +783,79 @@ clean_allocated_resources(ResourceId, ResourceMod) -> false -> ok end. + +%% @doc Split : separated resource id into type and name. +%% Type must be an existing atom. +%% Name is converted to atom if `atom_name` option is true. +-spec parse_resource_id(list() | binary(), #{atom_name => boolean()}) -> + {atom(), atom() | binary()}. +parse_resource_id(Id0, Opts) -> + Id = bin(Id0), + case string:split(bin(Id), ":", all) of + [Type, Name] -> + {to_type_atom(Type), validate_name(Name, Opts)}; + _ -> + invalid_data( + <<"should be of pattern {type}:{name}, but got: ", Id/binary>> + ) + end. + +to_type_atom(Type) when is_binary(Type) -> + try + erlang:binary_to_existing_atom(Type, utf8) + catch + _:_ -> + throw(#{ + kind => validation_error, + reason => <<"unknown resource type: ", Type/binary>> + }) + end. + +%% @doc Validate if type is valid. +%% Throws and JSON-map error if invalid. +-spec validate_type(binary()) -> ok. +validate_type(Type) -> + _ = to_type_atom(Type), + ok. + +bin(Bin) when is_binary(Bin) -> Bin; +bin(Str) when is_list(Str) -> list_to_binary(Str); +bin(Atom) when is_atom(Atom) -> atom_to_binary(Atom, utf8). + +%% @doc Validate if name is valid for bridge. +%% Throws and JSON-map error if invalid. +-spec validate_name(binary()) -> ok. +validate_name(Name) -> + _ = validate_name(Name, #{atom_name => false}), + ok. + +validate_name(<<>>, _Opts) -> + invalid_data("name cannot be empty string"); +validate_name(Name, _Opts) when size(Name) >= 255 -> + invalid_data("name length must be less than 255"); +validate_name(Name0, Opts) -> + Name = unicode:characters_to_list(Name0, utf8), + case lists:all(fun is_id_char/1, Name) of + true -> + case maps:get(atom_name, Opts, true) of + % NOTE + % Rule may be created before bridge, thus not `list_to_existing_atom/1`, + % also it is infrequent user input anyway. + true -> list_to_atom(Name); + false -> Name0 + end; + false -> + invalid_data( + <<"only 0-9a-zA-Z_- is allowed in resource name, got: ", Name0/binary>> + ) + end. + +-spec invalid_data(binary()) -> no_return(). +invalid_data(Reason) -> throw(#{kind => validation_error, reason => Reason}). + +is_id_char(C) when C >= $0 andalso C =< $9 -> true; +is_id_char(C) when C >= $a andalso C =< $z -> true; +is_id_char(C) when C >= $A andalso C =< $Z -> true; +is_id_char($_) -> true; +is_id_char($-) -> true; +is_id_char(_) -> false.