Merge pull request #3439 from esl/auth-password-config

Introduce defaults in the auth section

**Goal:** POC of defaults for nested options in an optional section to see how they would look like and decide if we want to proceed with this approach for the whole configuration file.

**Scope:** Defaults for `methods`, `sasl_mechanisms` and `sasl_external` in the `auth` section.

**Motivation** - advantages of defaults:
- Easy to find the default value in the code.
- Simpler code for fetching the values.
- No need to recompute larger values like the default list of SASL mechanisms every time they are fetched.

**Challenges:**
- Optional sections (like `auth`) need to be always included to make the defaults present. This cannot be a universal rule as most sections need to be included only when particular features are enabled.
- Small tests: nested values need to be checked selectively instead of comparing the whole configuration terms. However, this seems like a general improvement and the test cases look cleaner afterwards.
- Small tests: defaults need to be inserted into the expected options for the file tests. To fix this, the last commit changes the static `*.options` files into dynamic lists created by Erlang code. This seems to provide more flexibility, but you can check the last commit and see if you like the new, less verbose version more.

See the commit messages for more details.

**Comments**
> Hmm... don't know if it is too late for this idea, but the fact that we'd have defaults for entire sections seems to make some things more verbose 🤔 Didn't we consider setting these defaults per-option

It's a good idea and actually this **was** my initial attempt of the POC that I summarized at a grooming when I was introducing the whole concept of defaults.

However, implementation for the `general` section showed the following issues:
- The `default` would work if we are in a section, but would make no sense if we are not in a section, so you would always need to go one level up (which is tricky) to verify if it makes sense or not
- Sections have defaults only if they are not in `host_config` and having the defaults scattered around would mean a lot of extra code to have the second version for `host_config` without the defaults (see the code for the `general` section).
- Parser code was more complicated.
- Defaults should be defined at the same level as `required` keys.

include/mongoose_config_spec.hrl

@@ -8,9 +8,10 @@
                   validate = any :: mongoose_config_validator:section_validator(),
                   format_items = none :: mongoose_config_spec:format_items(),
                   process :: undefined | mongoose_config_parser_toml:list_processor(),
-                  wrap = default :: mongoose_config_spec:wrapper(),
                   defaults = #{} :: #{mongoose_config_parser_toml:toml_key() =>
-                                         mongoose_config_parser_toml:config_part()}}).
+                                         mongoose_config_parser_toml:config_part()},
+                  wrap = default :: mongoose_config_spec:wrapper(),
+                  include = when_present :: always | when_present}).
 
 -record(list, {items :: mongoose_config_spec:config_node(),
                validate = any :: mongoose_config_validator:list_validator(),

src/auth/ejabberd_auth.erl

@@ -417,7 +417,7 @@ auth_modules_for_host_type(HostType) ->
 
 -spec auth_methods(mongooseim:host_type()) -> [atom()].
 auth_methods(HostType) ->
-    mongoose_config:get_opt([{auth, HostType}, methods], []).
+    mongoose_config:get_opt([{auth, HostType}, methods]).
 
 -spec auth_method_to_module(atom()) -> authmodule().
 auth_method_to_module(Method) ->

src/config/mongoose_config_parser_toml.erl

@@ -108,7 +108,8 @@ ensure_keys(Keys, Section) ->
           [config_part()].
 parse_section(Path, M, #section{items = Items, defaults = Defaults}) ->
     FilteredDefaults = maps:filter(fun(K, _V) -> not maps:is_key(K, M) end, Defaults),
-    ProcessedConfig = maps:map(fun(K, V) -> handle([K|Path], V, get_spec_for_key(K, Items)) end, M),
+    M1 = maps:merge(get_always_included(Items), M),
+    ProcessedConfig = maps:map(fun(K, V) -> handle([K|Path], V, get_spec_for_key(K, Items)) end, M1),
     ProcessedDefaults = maps:map(fun(K, V) -> handle_default([K|Path], V, maps:get(K, Items)) end,
                                  FilteredDefaults),
     lists:flatmap(fun({_K, ConfigParts}) -> ConfigParts end,
@@ -126,6 +127,9 @@ get_spec_for_key(Key, Items) ->
             end
     end.
 
+get_always_included(Items) ->
+    maps:from_list([{K, #{}} || {K, #section{include = always}} <- maps:to_list(Items)]).
+
 -spec parse_list(path(), [toml_value()], mongoose_config_spec:config_list()) -> [config_part()].
 parse_list(Path, L, #list{items = ItemSpec}) ->
     lists:flatmap(fun(Elem) ->

src/config/mongoose_config_spec.erl

@@ -102,12 +102,13 @@
 
 root() ->
     General = general(),
+    Auth = auth(),
     #section{
        items = #{<<"general">> => General#section{required = [<<"default_server_domain">>],
                                                   process = fun ?MODULE:process_general/1,
                                                   defaults = general_defaults()},
                  <<"listen">> => listen(),
-                 <<"auth">> => auth(),
+                 <<"auth">> => Auth#section{include = always},
                  <<"outgoing_pools">> => outgoing_pools(),
                  <<"services">> => services(),
                  <<"modules">> => modules(),
@@ -483,8 +484,11 @@ auth() ->
                                                 validate = {module, cyrsasl},
                                                 process = fun ?MODULE:process_sasl_mechanism/1}}
                      },
-       wrap = host_config,
-       format_items = map
+       format_items = map,
+       defaults = #{<<"methods">> => [],
+                    <<"sasl_external">> => [standard],
+                    <<"sasl_mechanisms">> => cyrsasl:default_modules()},
+       wrap = host_config
       }.
 
 all_auth_methods() ->

src/sasl/cyrsasl.erl

@@ -29,12 +29,11 @@
 -export([listmech/1,
          server_new/6,
          server_start/4,
-         server_step/2]).
+         server_step/2,
+         default_modules/0]).
 
 -ignore_xref([behaviour_info/1]).
 
--include("mongoose.hrl").
-
 -type sasl_module() :: module().
 -type mechanism() :: binary().
 

src/sasl/cyrsasl_external.erl

@@ -105,13 +105,7 @@ authorize(Creds) ->
 
 get_verification_list(Creds) ->
     HostType = mongoose_credentials:host_type(Creds),
-    case mongoose_config:get_opt([{auth, HostType}, sasl_external], [standard]) of
-        [] -> [standard];
-        List when is_list(List) -> List;
-        standard -> [standard];
-        use_common_name -> [standard, common_name];
-        allow_just_user_identity -> [standard, auth_id]
-    end.
+    mongoose_config:get_opt([{auth, HostType}, sasl_external]).
 
 verification_loop([VerificationFn | T], Creds) ->
     case verify_creds(VerificationFn, Creds) of

test/config_parser_SUITE.erl

@@ -818,17 +818,19 @@ listen_http_handlers_domain(_Config) ->
 %% tests: auth
 
 auth_methods(_Config) ->
-    ?cfgh(auth, #{methods => [internal, rdbms]},
+    ?cfg([{auth, global}, methods], [], #{}), % global default
+    ?cfgh([auth, methods], [], #{<<"auth">> => #{}}), % default
+    ?cfgh([auth, methods], [internal, rdbms],
           #{<<"auth">> => #{<<"methods">> => [<<"internal">>, <<"rdbms">>]}}),
     ?errh(#{<<"auth">> => #{<<"methods">> => [<<"supernatural">>]}}).
 
 auth_password_format(_Config) ->
-    ?cfgh(auth, #{password_format => {scram, [sha, sha256]}},
+    ?cfgh([auth, password_format], {scram, [sha, sha256]},
           #{<<"auth">> => #{<<"password">> => #{<<"format">> => <<"scram">>,
                                                 <<"hash">> => [<<"sha">>, <<"sha256">>]}}}),
-    ?cfgh(auth, #{password_format => scram},
+    ?cfgh([auth, password_format], scram,
           #{<<"auth">> => #{<<"password">> => #{<<"format">> => <<"scram">>}}}),
-    ?cfgh(auth, #{password_format => plain},
+    ?cfgh([auth, password_format], plain,
           #{<<"auth">> => #{<<"password">> => #{<<"format">> => <<"plain">>}}}),
     ?errh(#{<<"auth">> => #{<<"password">> => #{<<"format">> => <<"no password">>}}}),
     ?errh(#{<<"auth">> => #{<<"password">> => #{<<"format">> => <<"scram">>,
@@ -837,68 +839,73 @@ auth_password_format(_Config) ->
                                                 <<"hash">> => [<<"sha1234">>]}}}).
 
 auth_scram_iterations(_Config) ->
-    ?cfgh(auth, #{scram_iterations => 1000},
+    ?cfgh([auth, scram_iterations], 1000,
           #{<<"auth">> => #{<<"scram_iterations">> => 1000}}),
     ?errh(#{<<"auth">> => #{<<"scram_iterations">> => false}}).
 
 auth_sasl_external(_Config) ->
-    ?cfgh(auth, #{sasl_external => [standard,
-                                    common_name,
-                                    {mod, cyrsasl_external_verification}]},
+    ?cfg([{auth, global}, sasl_external], [standard], #{}), % global default
+    ?cfgh([auth, sasl_external], [standard], #{<<"auth">> => #{}}), % default
+    ?cfgh([auth, sasl_external], [standard,
+                                  common_name,
+                                  {mod, cyrsasl_external_verification}],
           #{<<"auth">> => #{<<"sasl_external">> =>
                                 [<<"standard">>,
                                  <<"common_name">>,
                                  <<"cyrsasl_external_verification">>]}}),
     ?errh(#{<<"auth">> => #{<<"sasl_external">> => [<<"unknown">>]}}).
 
 auth_sasl_mechanisms(_Config) ->
-    ?cfgh(auth, #{sasl_mechanisms => [cyrsasl_external, cyrsasl_scram]},
+    Default = cyrsasl:default_modules(),
+    ?cfg([{auth, global}, sasl_mechanisms], Default, #{}), % global default
+    ?cfg([{auth, global}, sasl_mechanisms], Default, #{<<"auth">> => #{}}), % default
+    ?cfgh([auth, sasl_mechanisms], [cyrsasl_external, cyrsasl_scram],
           #{<<"auth">> => #{<<"sasl_mechanisms">> => [<<"external">>, <<"scram">>]}}),
     ?errh(#{<<"auth">> => #{<<"sasl_mechanisms">> => [<<"none">>]}}).
 
 auth_allow_multiple_connections(_Config) ->
-    ?cfgh(auth, #{anonymous => #{allow_multiple_connections => true}},
+    ?cfgh([auth, anonymous, allow_multiple_connections], true,
           auth_raw(<<"anonymous">>, #{<<"allow_multiple_connections">> => true})),
     ?errh(auth_raw(<<"anonymous">>, #{<<"allow_multiple_connections">> => <<"yes">>})).
 
 auth_anonymous_protocol(_Config) ->
-    ?cfgh(auth, #{anonymous => #{protocol => login_anon}},
+    ?cfgh([auth, anonymous, protocol], login_anon,
           auth_raw(<<"anonymous">>, #{<<"protocol">> => <<"login_anon">>})),
     ?errh(auth_raw(<<"anonymous">>, #{<<"protocol">> => <<"none">>})).
 
 auth_ldap_pool(_Config) ->
-    ?cfgh(auth, #{ldap => #{pool_tag => ldap_pool}},
+    ?cfgh([auth, ldap], #{pool_tag => ldap_pool},
           auth_ldap_raw(#{<<"pool_tag">> => <<"ldap_pool">>})),
     ?errh(auth_ldap_raw(#{<<"pool_tag">> => <<>>})).
 
 auth_ldap_bind_pool(_Config) ->
-    ?cfgh(auth, #{ldap => #{bind_pool_tag => ldap_bind_pool}},
+    ?cfgh([auth, ldap], #{bind_pool_tag => ldap_bind_pool},
           auth_ldap_raw(#{<<"bind_pool_tag">> => <<"ldap_bind_pool">>})),
     ?errh(auth_ldap_raw(#{<<"bind_pool_tag">> => true})).
 
 auth_ldap_base(_Config) ->
-    ?cfgh(auth, #{ldap => #{base => <<"ou=Users,dc=example,dc=com">>}},
+    ?cfgh([auth, ldap, base], <<"ou=Users,dc=example,dc=com">>,
           auth_ldap_raw(#{<<"base">> => <<"ou=Users,dc=example,dc=com">>})),
     ?errh(auth_ldap_raw(#{<<"base">> => 10})).
 
 auth_ldap_uids(_Config) ->
-    ?cfgh(auth, #{ldap => #{uids => [{<<"uid1">>, <<"user=%u">>}]}},
+    ?cfgh([auth, ldap, uids], [{<<"uid1">>, <<"user=%u">>}],
           auth_ldap_raw(#{<<"uids">> => [#{<<"attr">> => <<"uid1">>,
                                            <<"format">> => <<"user=%u">>}]})),
-    ?cfgh(auth, #{ldap => #{uids => [<<"uid1">>]}},
+    ?cfgh([auth, ldap, uids], [<<"uid1">>],
           auth_ldap_raw(#{<<"uids">> => [#{<<"attr">> => <<"uid1">>}]})),
     ?errh(auth_ldap_raw(#{<<"uids">> => [#{<<"format">> => <<"user=%u">>}]})).
 
 auth_ldap_filter(_Config) ->
-    ?cfgh(auth, #{ldap => #{filter => <<"(objectClass=inetOrgPerson)">>}},
+    ?cfgh([auth, ldap, filter], <<"(objectClass=inetOrgPerson)">>,
           auth_ldap_raw(#{<<"filter">> => <<"(objectClass=inetOrgPerson)">>})),
     ?errh(auth_ldap_raw(#{<<"filter">> => 10})).
 
 auth_ldap_dn_filter(_Config) ->
-    ?cfgh(auth, #{ldap => #{dn_filter => {<<"(user=%u@%d)">>, []}}},
+    ?cfgh([auth, ldap, dn_filter], {<<"(user=%u@%d)">>, []},
           auth_ldap_raw(#{<<"dn_filter">> => #{<<"filter">> => <<"(user=%u@%d)">>}})),
     Pattern = <<"(&(name=%s)(owner=%D)(user=%u@%d))">>,
-    ?cfgh(auth, #{ldap => #{dn_filter => {Pattern, [<<"sn">>]}}},
+    ?cfgh([auth, ldap, dn_filter], {Pattern, [<<"sn">>]},
           auth_ldap_raw(#{<<"dn_filter">> => #{<<"filter">> => Pattern,
                                                <<"attributes">> => [<<"sn">>]}})),
     ?errh(auth_ldap_raw(#{<<"dn_filter">> => #{<<"attributes">> => [<<"sn">>]}})),
@@ -910,7 +917,7 @@ auth_ldap_local_filter(_Config) ->
     Filter = #{<<"operation">> => <<"equal">>,
                <<"attribute">> => <<"accountStatus">>,
                <<"values">> => [<<"enabled">>]},
-    ?cfgh(auth, #{ldap => #{local_filter => {equal, {"accountStatus", ["enabled"]}}}},
+    ?cfgh([auth, ldap, local_filter], {equal, {"accountStatus", ["enabled"]}},
           auth_ldap_raw(#{<<"local_filter">> => Filter})),
     [?errh(auth_ldap_raw(#{<<"local_filter">> => maps:remove(K, Filter)})) ||
         K <- maps:keys(Filter)],
@@ -919,22 +926,22 @@ auth_ldap_local_filter(_Config) ->
     ?errh(auth_ldap_raw(#{<<"local_filter">> => Filter#{<<"values">> := []}})).
 
 auth_ldap_deref(_Config) ->
-    ?cfgh(auth, #{ldap => #{deref => always}}, auth_ldap_raw(#{<<"deref">> => <<"always">>})),
+    ?cfgh([auth, ldap, deref], always, auth_ldap_raw(#{<<"deref">> => <<"always">>})),
     ?errh(auth_ldap_raw(#{<<"deref">> => <<"sometimes">>})).
 
 auth_external(_Config) ->
     RequiredOpts = #{<<"program">> => <<"/usr/bin/auth">>},
     Config = #{program => "/usr/bin/auth"},
-    ?cfgh(auth, #{external => Config},
+    ?cfgh([auth, external], Config,
           auth_raw(<<"external">>, RequiredOpts)),
-    ?cfgh(auth, #{external => Config#{instances => 2}},
+    ?cfgh([auth, external, instances], 2,
           auth_raw(<<"external">>, RequiredOpts#{<<"instances">> => 2})),
     ?errh(auth_raw(<<"external">>, #{<<"program">> => <<>>})),
     ?errh(auth_raw(<<"external">>, #{<<"instances">> => 2})),
     ?errh(auth_raw(<<"external">>, RequiredOpts#{<<"instances">> => 0})).
 
 auth_http_basic_auth(_Config) ->
-    ?cfgh(auth, #{http => #{basic_auth => "admin:admin123"}},
+    ?cfgh([auth, http, basic_auth], "admin:admin123",
           auth_raw(<<"http">>, #{<<"basic_auth">> => <<"admin:admin123">>})),
     ?errh(auth_raw(<<"http">>, #{<<"basic_auth">> => true})).
 
@@ -945,11 +952,11 @@ auth_jwt(_Config) ->
     Config = #{algorithm => <<"HS512">>,
                secret => {value, "secret123"},
                username_key => user},
-    ?cfgh(auth, #{jwt => Config},
+    ?cfgh([auth, jwt], Config,
           auth_raw(<<"jwt">>, Opts)),
-    ?cfgh(auth, #{jwt => Config#{secret => {file, "/home/user/jwt_secret"}}},
+    ?cfgh([auth, jwt, secret], {file, "/home/user/jwt_secret"},
           auth_raw(<<"jwt">>, Opts#{<<"secret">> := #{<<"file">> => <<"/home/user/jwt_secret">>}})),
-    ?cfgh(auth, #{jwt => Config#{secret => {env, "SECRET"}}},
+    ?cfgh([auth, jwt, secret], {env, "SECRET"},
           auth_raw(<<"jwt">>, Opts#{<<"secret">> := #{<<"env">> => <<"SECRET">>}})),
     ?errh(auth_raw(<<"jwt">>, Opts#{<<"secret">> := #{<<"value">> => 123}})),
     ?errh(auth_raw(<<"jwt">>, Opts#{<<"secret">> := #{<<"file">> => <<>>}})),
@@ -961,19 +968,19 @@ auth_jwt(_Config) ->
     [?errh(auth_raw(<<"jwt">>, maps:without([K], Opts))) || K <- maps:keys(Opts)].
 
 auth_riak_bucket_type(_Config) ->
-    ?cfgh(auth, #{riak => #{bucket_type => <<"buckethead">>}},
+    ?cfgh([auth, riak, bucket_type], <<"buckethead">>,
           auth_raw(<<"riak">>, #{<<"bucket_type">> => <<"buckethead">>})),
     ?errh(auth_raw(<<"riak">>, #{<<"bucket_type">> => <<>>})).
 
 auth_rdbms_users_number_estimate(_Config) ->
-    ?cfgh(auth, #{rdbms => #{users_number_estimate => true}},
+    ?cfgh([auth, rdbms, users_number_estimate], true,
           auth_raw(<<"rdbms">>, #{<<"users_number_estimate">> => true})),
     ?errh(auth_raw(<<"rdbms">>, #{<<"users_number_estimate">> => 1200})).
 
 auth_dummy(_Config) ->
-    ?cfgh(auth, #{dummy => #{base_time => 0}},
+    ?cfgh([auth, dummy, base_time], 0,
           auth_raw(<<"dummy">>, #{<<"base_time">> => 0})),
-    ?cfgh(auth, #{dummy => #{variance => 10}},
+    ?cfgh([auth, dummy, variance], 10,
           auth_raw(<<"dummy">>, #{<<"variance">> => 10})),
     ?errh(auth_raw(<<"dummy">>, #{<<"base_time">> => -5})),
     ?errh(auth_raw(<<"dummy">>, #{<<"variance">> => 0})).
@@ -3014,15 +3021,20 @@ assert_options_host(ExpectedOptions, RawConfig) ->
     HostOptions = [{host_key(Key, ?HOST_TYPE), Value} || {Key, Value} <- ExpectedOptions],
     assert_options(HostOptions, HostConfig).
 
--type key_prefix() :: {shaper | acl | access, atom()} | atom() | {atom(), jid:lserver()}.
+-type key_prefix() :: top_level_key_prefix() | key_path_prefix().
+-type top_level_key_prefix() :: {shaper | acl | access, atom()} | atom().
+-type key_path_prefix() :: [atom() | binary()].
 
-%% @doc Create full per-host config key for host-or-global options
--spec host_key(key_prefix(), mongooseim:host_type_or_global()) -> mongoose_config:key().
+%% @doc Build full per-host config key for host-or-global options
+-spec host_key(top_level_key_prefix(), mongooseim:host_type_or_global()) -> mongoose_config:key();
+              (key_path_prefix(), mongooseim:host_type_or_global()) -> mongoose_config:key_path().
 host_key({Key, Tag}, HostType) when Key =:= shaper;
                                     Key =:= acl;
                                     Key =:= access ->
     {Key, Tag, HostType};
-host_key(Key, HostType) ->
+host_key([TopKey | Rest], HostType) when is_atom(TopKey) ->
+    [{TopKey, HostType} | Rest];
+host_key(Key, HostType) when is_atom(Key) ->
     {Key, HostType}.
 
 -spec assert_error_host_or_global(mongoose_config_parser_toml:toml_section()) -> any().
@@ -3056,18 +3068,26 @@ maybe_insert_dummy_domain(M, DomainName) ->
 
 %% helpers for testing individual options
 
--spec assert_options([{mongoose_config:key(), mongoose_config:value()}],
+-spec assert_options([{mongoose_config:key() | mongoose_config:key_path(), mongoose_config:value()}],
                      [mongoose_config_parser_toml:config()]) -> any().
 assert_options(ExpectedOptions, Config) ->
     lists:foreach(fun({Key, Value}) -> assert_option(Key, Value, Config) end, ExpectedOptions).
 
--spec assert_option(mongoose_config:key(), mongoose_config:value(),
+-spec assert_option(mongoose_config:key() | mongoose_config:key_path(), mongoose_config:value(),
                     [mongoose_config_parser_toml:config()]) -> any().
 assert_option(Key, Value, Config) ->
-    case lists:keyfind(Key, 1, Config) of
-        false -> ct:fail({"option not found", Key, Value, Config});
-        ActualOpt -> handle_config_option({Key, Value}, ActualOpt)
-    end.
+    compare_values(Key, Value, get_config_value(Key, Config)).
+
+-spec get_config_value(mongoose_config:key() | mongoose_config:key_path(),
+                       [mongoose_config_parser_toml:config()]) ->
+          mongoose_config:value().
+get_config_value([TopKey | Rest], Config) ->
+    case lists:keyfind(TopKey, 1, Config) of
+        false -> ct:fail({"option not found", TopKey, Config});
+        {_, TopValue} -> lists:foldl(fun maps:get/2, TopValue, Rest)
+    end;
+get_config_value(Key, Config) ->
+    get_config_value([Key], Config).
 
 -spec assert_error([mongoose_config_parser_toml:config()]) -> any().
 assert_error(Config) ->
@@ -3078,7 +3098,7 @@ assert_error(Config) ->
 
 test_config_file(Config, File) ->
     OptionsPath = ejabberd_helper:data(Config, File ++ ".options"),
-    {ok, ExpectedOpts} = file:consult(OptionsPath),
+    ExpectedOpts = config_parser_helper:options(File),
 
     TOMLPath = ejabberd_helper:data(Config, File ++ ".toml"),
     State = mongoose_config_parser:parse_file(TOMLPath),