feat(authn): improve apis of moving authenticators

apps/emqx_authn/src/emqx_authn.erl

@@ -49,7 +49,7 @@
         , update_or_create_authenticator/3
         , lookup_authenticator/2
         , list_authenticators/1
-        , move_authenticator_to_the_nth/3
+        , move_authenticator/3
         ]).
 
 -export([ import_users/3
@@ -108,6 +108,30 @@ pre_config_update({update_or_create_authenticator, ID, Config}, OldConfig) ->
                               false -> C
                           end
                       end, OldConfig)
+    end;
+pre_config_update({move, ID, Position}, OldConfig) ->
+    case lookup_authenticator(?CHAIN, ID) of
+        {error, Reason} -> error(Reason);
+        {ok, #{name := Name}} ->
+            {ok, Found, Part1, Part2} = split_by_name(Name, OldConfig),
+            case Position of
+                <<"top">> ->
+                    [Found | Part1] ++ Part2;
+                <<"bottom">> ->
+                    Part1 ++ Part2 ++ [Found];
+                Before ->
+                    case binary:split(Before, <<":">>, [global]) of
+                        [<<"before">>, ID0] ->
+                            case lookup_authenticator(?CHAIN, ID0) of
+                                {error, Reason} -> error(Reason);
+                                {ok, #{name := Name1}} ->
+                                    {ok, NFound, NPart1, NPart2} = split_by_name(Name1, Part1 + Part2),
+                                    NPart1 ++ [Found, NFound | NPart2]
+                            end;
+                        _ ->
+                            error({invalid_parameter, position})
+                    end
+            end
     end.
 
 post_config_update({enable, true}, _NewConfig, _OldConfig) ->
@@ -157,6 +181,22 @@ post_config_update({update_or_create_authenticator, ID, #{<<"name">> := Name}},
             end;
         [_Config | _] ->
             error(name_has_be_used)
+    end;
+post_config_update({move, ID, Position}, _NewConfig, _OldConfig) ->
+    NPosition = case Position of
+                    <<"top">> -> top;
+                    <<"bottom">> -> bottom;
+                    Before ->
+                        case binary:split(Before, <<":">>, [global]) of
+                            [<<"before">>, ID0] ->
+                                {before, ID0};
+                            _ ->
+                                error({invalid_parameter, position})
+                        end
+                end,
+    case move_authenticator(?CHAIN, ID, NPosition) of
+        ok -> ok;
+        {error, Reason} -> throw(Reason)
     end.
 
 update_config(Path, ConfigRequest) ->
@@ -255,8 +295,8 @@ list_authenticators(ChainID) ->
             {ok, serialize_authenticators(Authenticators)}
     end.
 
-move_authenticator_to_the_nth(ChainID, AuthenticatorID, N) ->
-    gen_server:call(?MODULE, {move_authenticator, ChainID, AuthenticatorID, N}).
+move_authenticator(ChainID, AuthenticatorID, Position) ->
+    gen_server:call(?MODULE, {move_authenticator, ChainID, AuthenticatorID, Position}).
 
 import_users(ChainID, AuthenticatorID, Filename) ->
     gen_server:call(?MODULE, {import_users, ChainID, AuthenticatorID, Filename}).
@@ -364,16 +404,16 @@ handle_call({update_or_create_authenticator, ChainID, AuthenticatorID, Config},
     Reply = update_or_create_authenticator(ChainID, AuthenticatorID, Config, true),
     reply(Reply, State);
 
-handle_call({move_authenticator, ChainID, AuthenticatorID, N}, _From, State) ->
+handle_call({move_authenticator, ChainID, AuthenticatorID, Position}, _From, State) ->
     UpdateFun = 
         fun(#chain{authenticators = Authenticators} = Chain) ->
-            case move_authenticator_to_the_nth_(AuthenticatorID, Authenticators, N) of
-                        {ok, NAuthenticators} ->
-                            true = ets:insert(?CHAIN_TAB, Chain#chain{authenticators = NAuthenticators}),
-                            ok;
-                        {error, Reason} ->
-                            {error, Reason}
-                    end
+            case do_move_authenticator(AuthenticatorID, Authenticators, Position) of
+                {ok, NAuthenticators} ->
+                    true = ets:insert(?CHAIN_TAB, Chain#chain{authenticators = NAuthenticators}),
+                    ok;
+                {error, Reason} ->
+                    {error, Reason}
+            end
         end,
     Reply = update_chain(ChainID, UpdateFun),
     reply(Reply, State);
@@ -458,6 +498,21 @@ switch_version(State = #{version := ?VER_2}) ->
 switch_version(State) ->
     State#{version => ?VER_1}.
 
+split_by_name(Name, Config) ->
+    {Part1, Part2, true} = lists:foldl(
+             fun(#{<<"name">> := N} = C, {P1, P2, F0}) ->
+                 F = case N =:= Name of
+                         true -> true;
+                         false -> F0
+                     end,
+                 case F of
+                     false -> {[C | P1], P2, F};
+                     true -> {P1, [C | P2], F}
+                 end
+             end, {[], [], false}, Config),
+    [Found | NPart2] = lists:reverse(Part2),
+    {ok, Found, lists:reverse(Part1), NPart2}.
+
 do_create_authenticator(ChainID, AuthenticatorID, #{name := Name} = Config) ->
     Provider = authenticator_provider(Config),
     Unique = <<ChainID/binary, "/", AuthenticatorID/binary, ":", ?VER_1/binary>>,
@@ -546,23 +601,27 @@ update_or_create_authenticator(ChainID, AuthenticatorID, #{name := NewName} = Co
 replace_authenticator(ID, #authenticator{name = Name} = Authenticator, Authenticators) ->
     lists:keyreplace(ID, 1, Authenticators, {ID, Name, Authenticator}).
 
-move_authenticator_to_the_nth_(AuthenticatorID, Authenticators, N)
-  when N =< length(Authenticators) andalso N > 0 ->
-    move_authenticator_to_the_nth_(AuthenticatorID, Authenticators, N, []);
-move_authenticator_to_the_nth_(_, _, _) ->
-    {error, out_of_range}.
-
-move_authenticator_to_the_nth_(AuthenticatorID, [], _, _) ->
-    {error, {not_found, {authenticator, AuthenticatorID}}};
-move_authenticator_to_the_nth_(AuthenticatorID, [{AuthenticatorID, _, _} = Authenticator | More], N, Passed)
-  when N =< length(Passed) ->
-    {L1, L2} = lists:split(N - 1, lists:reverse(Passed)),
-    {ok, L1 ++ [Authenticator] ++ L2 ++ More};
-move_authenticator_to_the_nth_(AuthenticatorID, [{AuthenticatorID, _, _} = Authenticator | More], N, Passed) ->
-    {L1, L2} = lists:split(N - length(Passed) - 1, More),
-    {ok, lists:reverse(Passed) ++ L1 ++ [Authenticator] ++ L2};
-move_authenticator_to_the_nth_(AuthenticatorID, [Authenticator | More], N, Passed) ->
-    move_authenticator_to_the_nth_(AuthenticatorID, More, N, [Authenticator | Passed]).
+do_move_authenticator(AuthenticatorID, Authenticators, Position) when is_binary(AuthenticatorID) ->
+    case lists:keytake(AuthenticatorID, 1, Authenticators) of
+        false ->
+            {error, {not_found, {authenticator, AuthenticatorID}}};
+        {value, Authenticator, NAuthenticators} ->
+            do_move_authenticator(Authenticator, NAuthenticators, Position)
+    end;
+
+do_move_authenticator(Authenticator, Authenticators, top) ->
+    {ok, [Authenticator | Authenticators]};
+do_move_authenticator(Authenticator, Authenticators, bottom) ->
+    {ok, Authenticators ++ [Authenticator]};
+do_move_authenticator(Authenticator, Authenticators, {before, ID}) ->
+    insert(Authenticator, Authenticators, ID, []).
+
+insert(_, [], ID, _) ->
+    {error, {not_found, {authenticator, ID}}};
+insert(Authenticator, [{ID, _, _} | _] = Authenticators, ID, Acc) ->
+    {ok, lists:reverse(Acc) ++ [Authenticator | Authenticators]};
+insert(Authenticator, [{_, _, _} = Authenticator0 | More], ID, Acc) ->
+    insert(Authenticator, More, ID, [Authenticator0 | Acc]).
 
 update_chain(ChainID, UpdateFun) ->
     case ets:lookup(?CHAIN_TAB, ChainID) of

apps/emqx_authn/src/emqx_authn_api.erl

@@ -24,7 +24,7 @@
         , authentication/2
         , authenticators/2
         , authenticators2/2
-        , position/2
+        , move/2
         , import_users/2
         , users/2
         , users2/2
@@ -109,7 +109,7 @@ api_spec() ->
     {[ authentication_api()
      , authenticators_api()
      , authenticators_api2()
-     , position_api()
+     , move_api()
      , import_users_api()
      , users_api()
      , users2_api()
@@ -405,10 +405,10 @@ authenticators_api2() ->
     },
     {"/authentication/authenticators/:id", Metadata, authenticators2}.
 
-position_api() ->
+move_api() ->
     Metadata = #{
         post => #{
-            description => "Change the order of authenticators",
+            description => "Move authenticator",
             parameters => [
                 #{
                     name => id,
@@ -423,14 +423,30 @@ position_api() ->
                 content => #{
                     'application/json' => #{
                         schema => #{
-                            type => object,
-                            required => [position],
-                            properties => #{
-                                position => #{
-                                    type => integer,
-                                    example => 1
+                            oneOf => [
+                                #{
+                                    type => object,
+                                    required => [position],
+                                    properties => #{
+                                        position => #{
+                                            type => string,
+                                            enum => [<<"top">>, <<"bottom">>],
+                                            example => <<"top">>
+                                        }
+                                    }
+                                },
+                                #{
+                                    type => object,
+                                    required => [position],
+                                    properties => #{
+                                        position => #{
+                                            type => string,
+                                            description => <<"before:<authenticator_id>">>,
+                                            example => <<"before:67e4c9d3">>
+                                        }
+                                    }
                                 }
-                            }
+                            ]
                         }
                     }
                 }
@@ -444,7 +460,7 @@ position_api() ->
             }
         }
     },
-    {"/authentication/authenticators/:id/position", Metadata, position}.
+    {"/authentication/authenticators/:id/move", Metadata, move}.
 
 import_users_api() ->
     Metadata = #{
@@ -1304,18 +1320,17 @@ authenticators2(delete, Request) ->
             serialize_error(Reason)
     end.
 
-position(post, Request) ->
+move(post, Request) ->
     AuthenticatorID = cowboy_req:binding(id, Request),
     {ok, Body, _} = cowboy_req:read_body(Request),
-    NBody = emqx_json:decode(Body, [return_maps]),
-    Config = hocon_schema:check_plain(emqx_authn_implied_schema, #{<<"position">> => NBody},
-                                      #{nullable => true}, ["position"]),
-    #{position := #{position := Position}} = emqx_map_lib:unsafe_atom_key_map(Config),
-    case emqx_authn:move_authenticator_to_the_nth(?CHAIN, AuthenticatorID, Position) of
-        ok ->
-            {204};
-        {error, Reason} ->
-            serialize_error(Reason)
+    case emqx_json:decode(Body, [return_maps]) of
+        #{<<"position">> := Position} ->
+            case emqx_authn:update_config([authentication, authenticators], {move_authenticator, AuthenticatorID, Position}) of
+                ok -> {204};
+                {error, Reason} -> serialize_error(Reason)
+            end;
+        _ ->
+            serialize_error({missing_parameter, position})
     end.
 
 import_users(post, Request) ->
@@ -1393,9 +1408,6 @@ serialize_error({not_found, {authenticator, ID}}) ->
 serialize_error(name_has_be_used) ->
     {409, #{code => <<"ALREADY_EXISTS">>,
             message => <<"Name has be used">>}};
-serialize_error(out_of_range) ->
-    {400, #{code => <<"OUT_OF_RANGE">>,
-            message => <<"Out of range">>}};
 serialize_error({missing_parameter, Name}) ->
     {400, #{code => <<"MISSING_PARAMETER">>,
             message => list_to_binary(

apps/emqx_authn/src/simple_authn/emqx_authn_implied_schema.erl

@@ -25,12 +25,10 @@
         , fields/1
         ]).
 
-structs() -> [ "filename", "position", "user_info", "new_user_info"].
+structs() -> [ "filename", "user_info", "new_user_info"].
 
 fields("filename") ->
     [ {filename, fun filename/1} ];
-fields("position") ->
-    [ {position, fun position/1} ];
 fields("user_info") ->
     [ {user_id, fun user_id/1}
     , {password, fun password/1}
@@ -43,11 +41,6 @@ filename(type) -> string();
 filename(nullable) -> false;
 filename(_) -> undefined.
 
-position(type) -> integer();
-position(validate) -> [fun (Position) -> Position > 0 end];
-position(nullable) -> false;
-position(_) -> undefined.
-
 user_id(type) -> binary();
 user_id(nullable) -> false;
 user_id(_) -> undefined.

apps/emqx_authn/test/emqx_authn_SUITE.erl

@@ -86,10 +86,18 @@ t_authenticator(_) ->
     ?assertMatch({ok, #{id := ?CHAIN, authenticators := [#{name := AuthenticatorName1}, #{name := AuthenticatorName2}]}}, ?AUTH:lookup_chain(?CHAIN)),
     ?assertMatch({ok, [#{name := AuthenticatorName1}, #{name := AuthenticatorName2}]}, ?AUTH:list_authenticators(?CHAIN)),
 
-    ?assertEqual(ok, ?AUTH:move_authenticator_to_the_nth(?CHAIN, ID2, 1)),
+    ?assertEqual(ok, ?AUTH:move_authenticator(?CHAIN, ID2, top)),
     ?assertMatch({ok, [#{name := AuthenticatorName2}, #{name := AuthenticatorName1}]}, ?AUTH:list_authenticators(?CHAIN)),
-    ?assertEqual({error, out_of_range}, ?AUTH:move_authenticator_to_the_nth(?CHAIN, ID2, 3)),
-    ?assertEqual({error, out_of_range}, ?AUTH:move_authenticator_to_the_nth(?CHAIN, ID2, 0)),
+
+    ?assertEqual(ok, ?AUTH:move_authenticator(?CHAIN, ID2, bottom)),
+    ?assertMatch({ok, [#{name := AuthenticatorName1}, #{name := AuthenticatorName2}]}, ?AUTH:list_authenticators(?CHAIN)),
+
+    ?assertEqual(ok, ?AUTH:move_authenticator(?CHAIN, ID2, {before, ID1})),
+    
+    ?assertMatch({ok, [#{name := AuthenticatorName2}, #{name := AuthenticatorName1}]}, ?AUTH:list_authenticators(?CHAIN)),
+
+    ?assertEqual({error, {not_found, {authenticator, <<"nonexistent">>}}}, ?AUTH:move_authenticator(?CHAIN, ID2, {before, <<"nonexistent">>})),
+
     ?assertEqual(ok, ?AUTH:delete_authenticator(?CHAIN, ID1)),
     ?assertEqual(ok, ?AUTH:delete_authenticator(?CHAIN, ID2)),
     ?assertEqual({ok, []}, ?AUTH:list_authenticators(?CHAIN)),

apps/emqx_authn/test/emqx_authn_mnesia_SUITE.erl

@@ -144,7 +144,7 @@ t_multi_mnesia_authenticator(_) ->
                     clientid => <<"myclient">>,
 			        password => <<"mypass1">>},
     ?assertEqual({stop, ok}, ?AUTH:authenticate(ClientInfo1, ok)),
-    ?assertEqual(ok, ?AUTH:move_authenticator_to_the_nth(?CHAIN, ID2, 1)),
+    ?assertEqual(ok, ?AUTH:move_authenticator(?CHAIN, ID2, top)),
 
     ?assertEqual({stop, {error, bad_username_or_password}}, ?AUTH:authenticate(ClientInfo1, ok)),
     ClientInfo2 = ClientInfo1#{password => <<"mypass2">>},