Merge pull request #3599 from esl/mod_keystore-map-config

Use maps for mod_keystore config

big_tests/tests/oauth_SUITE.erl

@@ -461,15 +461,14 @@ to_lower(B) when is_binary(B) ->
     list_to_binary(string:to_lower(binary_to_list(B))).
 
 required_modules() ->
-    KeyStoreOpts = [{keys, [
-                            {token_secret, ram},
-                            %% This is a hack for tests! As the name implies,
-                            %% a pre-shared key should be read from a file stored
-                            %% on disk. This way it can be shared with trusted 3rd
-                            %% parties who can use it to sign tokens for users
-                            %% to authenticate with and MongooseIM to verify.
-                            {provision_pre_shared, ram}
-                           ]}],
+    KeyOpts = #{keys => #{token_secret => ram,
+                         %% This is a hack for tests! As the name implies,
+                         %% a pre-shared key should be read from a file stored
+                         %% on disk. This way it can be shared with trusted 3rd
+                         %% parties who can use it to sign tokens for users
+                         %% to authenticate with and MongooseIM to verify.
+                         provision_pre_shared => ram}},
+    KeyStoreOpts = config_parser_helper:mod_config(mod_keystore, KeyOpts),
     [{mod_last, stopped},
      {mod_keystore, KeyStoreOpts},
      {mod_auth_token, auth_token_opts()}].

src/mod_keystore.erl

@@ -12,11 +12,8 @@
 %% Hook handlers
 -export([get_key/2]).
 
-%% Tests only!
--export([validate_opts/1]).
-
 -export([config_metrics/1]).
--export([process_keys/1]).
+-export([process_key/1]).
 
 %% Public types
 -export_type([key/0,
@@ -25,9 +22,7 @@
               key_name/0,
               raw_key/0]).
 
--ignore_xref([
-    behaviour_info/1, get_key/2, validate_opts/1
-]).
+-ignore_xref([get_key/2]).
 
 -include("mod_keystore.hrl").
 -include("mongoose.hrl").
@@ -54,9 +49,8 @@
 %% gen_mod callbacks
 %%
 
--spec start(mongooseim:host_type(), list()) -> ok.
+-spec start(mongooseim:host_type(), gen_mod:module_opts()) -> ok.
 start(HostType, Opts) ->
-    validate_opts(Opts),
     create_keystore_ets(),
     mod_keystore_backend:init(HostType, Opts),
     init_keys(HostType, Opts),
@@ -83,8 +77,12 @@ config_spec() ->
     #section{
         items = #{<<"ram_key_size">> => #option{type = integer,
                                                 validate = non_negative},
-                  <<"keys">> => #list{items = keys_spec()}
-        }
+                  <<"keys">> => #list{items = keys_spec(),
+                                      format_items = map}
+        },
+        defaults = #{<<"ram_key_size">> => ?DEFAULT_RAM_KEY_SIZE,
+                     <<"keys">> => #{}},
+        format_items = map
     }.
 
 keys_spec() ->
@@ -97,15 +95,14 @@ keys_spec() ->
                                         validate = filename}
         },
         required = [<<"name">>, <<"type">>],
-        process = fun ?MODULE:process_keys/1
+        format_items = map,
+        process = fun ?MODULE:process_key/1
     }.
 
-process_keys(KVs) ->
-    {[[{name, Name}], [{type, Type}]], PathOpts} = proplists:split(KVs, [name, type]),
-    process_key_opts(Name, Type, PathOpts).
-
-process_key_opts(Name, ram, []) -> {Name, ram};
-process_key_opts(Name, file, [{path, Path}]) -> {Name, {file, Path}}.
+process_key(#{name := Name, type := file, path := Path}) ->
+    {Name, {file, Path}};
+process_key(#{name := Name, type := ram}) ->
+    {Name, ram}.
 
 %%
 %% Hook handlers
@@ -165,16 +162,16 @@ clear_keystore_ets(HostType) ->
 does_table_exist(NameOrTID) ->
     ets:info(NameOrTID, name) /= undefined.
 
-init_keys(HostType, Opts) ->
-    [ init_key(K, HostType, Opts) || K <- proplists:get_value(keys, Opts, []) ].
+init_keys(HostType, Opts = #{keys := Keys}) ->
+    maps:map(fun(KeyName, KeyType) -> init_key({KeyName, KeyType}, HostType, Opts) end, Keys).
 
--spec init_key({key_name(), key_type()}, mongooseim:host_type(), list()) -> ok.
+-spec init_key({key_name(), key_type()}, mongooseim:host_type(), gen_mod:module_opts()) -> ok.
 init_key({KeyName, {file, Path}}, HostType, _Opts) ->
     {ok, Data} = file:read_file(Path),
     true = ets_store_key({KeyName, HostType}, Data),
     ok;
-init_key({KeyName, ram}, HostType, Opts) ->
-    ProposedKey = crypto:strong_rand_bytes(get_key_size(Opts)),
+init_key({KeyName, ram}, HostType, #{ram_key_size := KeySize}) ->
+    ProposedKey = crypto:strong_rand_bytes(KeySize),
     KeyRecord = #key{id = {KeyName, HostType},
                      key = ProposedKey},
     {ok, _ActualKey} = mod_keystore_backend:init_ram_key(HostType, KeyRecord),
@@ -187,23 +184,5 @@ ets_get_key(KeyID) ->
 ets_store_key(KeyID, RawKey) ->
     ets:insert(keystore, {KeyID, RawKey}).
 
-get_key_size(Opts) ->
-    case lists:keyfind(ram_key_size, 1, Opts) of
-        false -> ?DEFAULT_RAM_KEY_SIZE;
-        {ram_key_size, KeySize} -> KeySize
-    end.
-
-validate_opts(Opts) ->
-    validate_key_ids(proplists:get_value(keys, Opts, [])).
-
-validate_key_ids(KeySpecs) ->
-    KeyIDs = [ KeyID || {KeyID, _} <- KeySpecs ],
-    SortedAndUniqueKeyIDs = lists:usort(KeyIDs),
-    case KeyIDs -- SortedAndUniqueKeyIDs of
-        [] -> ok;
-        [_|_] -> error(non_unique_key_ids, KeySpecs)
-    end.
-
 config_metrics(Host) ->
-    OptsToReport = [{backend, mnesia}], %list of tuples {option, defualt_value}
-    mongoose_module_metrics:opts_for_module(Host, ?MODULE, OptsToReport).
+    mongoose_module_metrics:opts_for_module(Host, ?MODULE, [backend]).

test/common/config_parser_helper.erl

@@ -453,11 +453,10 @@ all_modules() ->
       mod_adhoc => #{iqdisc => one_queue, report_commands_node => true},
       mod_mam_rdbms_arch_async => default_config([modules, mod_mam_meta, async_writer]),
       mod_keystore =>
-          [{keys,
-            [{access_secret, ram},
-             {access_psk, {file, "priv/access_psk"}},
-             {provision_psk, {file, "priv/provision_psk"}}]},
-           {ram_key_size, 1000}],
+          mod_config(mod_keystore, #{keys => #{access_secret => ram,
+                                               access_psk => {file, "priv/access_psk"},
+                                               provision_psk => {file, "priv/provision_psk"}},
+                                     ram_key_size => 1000}),
       mod_global_distrib =>
           mod_config(mod_global_distrib,
                      #{global_host => <<"example.com">>,
@@ -877,6 +876,8 @@ default_mod_config(mod_inbox) ->
       remove_on_kicked => true,
       reset_markers => [<<"displayed">>],
       iqdisc => no_queue};
+default_mod_config(mod_keystore) ->
+    #{ram_key_size => 2048, keys => #{}};
 default_mod_config(mod_last) ->
     #{iqdisc => one_queue, backend => mnesia};
 default_mod_config(mod_mam) ->

test/config_parser_SUITE.erl

@@ -2102,30 +2102,36 @@ mod_jingle_sip(_Config) ->
     ?errh(T(#{<<"sdp_origin">> => <<"abc">>})).
 
 mod_keystore(_Config) ->
+    check_module_defaults(mod_keystore),
     T = fun(Opts) -> #{<<"modules">> => #{<<"mod_keystore">> => Opts}} end,
-    M = fun(Cfg) -> modopts(mod_keystore, Cfg) end,
-    ?cfgh(M([{ram_key_size, 1024}]),
+    P = [modules, mod_keystore],
+    ?cfgh(P ++ [ram_key_size], 1024,
           T(#{<<"ram_key_size">> => 1024})),
     ?errh(T(#{<<"ram_key_size">> => -1})).
 
 mod_keystore_keys(_Config) ->
     T = fun(Opts) -> #{<<"modules">> => #{<<"mod_keystore">> =>
                                               #{<<"keys">> => Opts}}}
         end,
-    M = fun(Cfg) -> modopts(mod_keystore, [{keys, Cfg}]) end,
+    P = [modules, mod_keystore, keys],
     RequiredOpts = #{<<"name">> => <<"access_secret">>,
                      <<"type">> => <<"ram">>},
-    ?cfgh(M([{access_secret, ram}]),
+    ?cfgh(P ++ [access_secret], ram,
           T([RequiredOpts])),
-    ?cfgh(M([{access_secret, {file, "priv/access_psk"}}]),
+    ?cfgh(P ++ [access_secret], {file, "priv/access_psk"},
           T([RequiredOpts#{<<"type">> => <<"file">>,
                            <<"path">> => <<"priv/access_psk">>}])),
     [?errh(T([maps:remove(Key, RequiredOpts)])) || Key <- maps:keys(RequiredOpts)],
     ?errh(T([RequiredOpts#{<<"name">> => <<>>}])),
     ?errh(T([RequiredOpts#{<<"type">> => <<"rampampam">>}])),
     ?errh(T([RequiredOpts#{<<"type">> => <<"file">>}])),
     ?errh(T([RequiredOpts#{<<"type">> => <<"file">>,
-                           <<"path">> => <<"does/not/exists">>}])).
+                           <<"path">> => <<"does/not/exists">>}])),
+    ?errh(T([#{<<"name">> => <<"same_name_twice">>,
+               <<"type">> => <<"ram">>},
+             #{<<"name">> => <<"same_name_twice">>,
+               <<"type">> => <<"file">>,
+               <<"path">> => <<"priv/access_psk">>}])).
 
 mod_last(_Config) ->
     check_iqdisc_map(mod_last),

test/keystore_SUITE.erl

@@ -1,6 +1,7 @@
 -module(keystore_SUITE).
 -include_lib("eunit/include/eunit.hrl").
 -compile([export_all, nowarn_export_all]).
+-import(config_parser_helper, [default_mod_config/1, mod_config/2]).
 
 -define(ae(Expected, Actual), ?assertEqual(Expected, Actual)).
 
@@ -11,7 +12,6 @@ all() ->
      module_startup_create_ram_key,
      module_startup_create_ram_key_of_given_size,
      module_startup_for_multiple_domains,
-     module_startup_non_unique_key_ids,
      multiple_domains_one_stopped
     ].
 
@@ -54,7 +54,7 @@ clean_after_testcase(C) ->
 %%
 
 module_startup_no_opts(_) ->
-    ok = mod_keystore:start(<<"localhost">>, []).
+    ok = mod_keystore:start(<<"localhost">>, default_mod_config(mod_keystore)).
 
 module_startup_read_key_from_file(_) ->
     %% given
@@ -103,18 +103,6 @@ module_startup_for_multiple_domains(_Config) ->
     ?ae([{{key_from_file, <<"second.com">>}, SecondKey}],
         get_key(<<"second.com">>, key_from_file)).
 
-module_startup_non_unique_key_ids(_) ->
-    %% given
-    NonUniqueKeyIDsOpts = [{keys, [{some_key, ram},
-                                   {some_key, {file, "some_key.dat"}}]}],
-    %% when
-    try
-        mod_keystore:start(<<"localhost">>, NonUniqueKeyIDsOpts)
-    %% then
-    catch
-        error:non_unique_key_ids -> ok
-    end.
-
 multiple_domains_one_stopped(_Config) ->
     % given
     [] = get_key(<<"first.com">>, key_from_file),
@@ -135,40 +123,19 @@ multiple_domains_one_stopped(_Config) ->
 %% Helpers
 %%
 
-start_async(M, F, A) ->
-    Self = self(),
-    P = spawn(fun () ->
-                      erlang:apply(M, F, A),
-                      Self ! started,
-                      helper_loop()
-              end),
-    receive
-        started ->
-            %ct:pal("started", []),
-            {ok, P}
-        after timer:seconds(1) ->
-            ct:fail("async start timeout")
-    end.
-
-helper_loop() ->
-    receive
-        stop -> exit(normal);
-        _    -> helper_loop()
-    end.
-
 key_at(Path, Data) ->
     ok = file:write_file(Path, Data),
     {ok, Path}.
 
 key_from_file(KeyFile) ->
-    [{keys, [{key_from_file, {file, KeyFile}}]}].
+    mod_config(mod_keystore, #{keys => #{key_from_file => {file, KeyFile}}}).
 
 ram_key() ->
-    [{keys, [{ram_key, ram}]}].
+    mod_config(mod_keystore, #{keys => #{ram_key => ram}}).
 
 sized_ram_key(Size) ->
-    [{keys, [{ram_key, ram}]},
-     {ram_key_size, Size}].
+    mod_config(mod_keystore, #{keys => #{ram_key => ram},
+                               ram_key_size => Size}).
 
 mock_mongoose_metrics() ->
     meck:new(mongoose_metrics, []),
@@ -183,7 +150,3 @@ mock_mongoose_metrics() ->
       Result :: mod_keystore:key_list().
 get_key(HostType, KeyName) ->
     mongoose_hooks:get_key(HostType, KeyName).
-
-%%{mod_keystore, [{keys, [{asdqwe_access_secret, ram},
-%%                        {asdqwe_access_psk,    {file, "priv/asdqwe_access_psk"}},
-%%                        {asdqwe_provision_psk, {file, "priv/asdqwe_access_psk"}}]}]},