Merge pull request #11874 from zmstone/1103-validate-bridge-type-and-name-at-entry

fix(bridge): do not allow dot in bridge name

apps/emqx_bridge/src/emqx_bridge_resource.erl

@@ -102,21 +102,15 @@ bridge_id(BridgeType, BridgeName) ->
     <<Type/binary, ":", Name/binary>>.
 
 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),

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

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

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

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(<<?TEST_ID_PREFIX, ConnectorId/binary>>, 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).

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

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

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.