Introduce defaults in the auth section #3439
Conversation
Some subsections (e.g. auth.password) group similar config options and their presence or absence does not change system behaviour. For example, user can skip 'auth.password' or define it as empty and the system still stores passwords, just with the default settings. When such subsections include defaults, they need to be always present, hence specifying 'include = always' makes the defaults always available. Such behaviour cannot be applied universally as for most sections their presence or absence is meaningful.
The defaults should be always available even if auth section is missing from the config (which is valid), so the top-level auth section needs to be always included.
Also remove processing of old (already removed) 'sasl_external' values.
As there are more and more defaults, it becomes necessary to check only one expected option value even if it is nested in maps, e.g. for auth options. It is now possible to do it with ?cfg and ?cfgh macros, e.g. ?cfg(Path, ExpectedValue, RawConfig)
Check only the required options - otherwise all expected default values would need to be provided in each assertion.
Listing the sasl mechanisms is verbose, but there is no better way to do it right now. If the number of such default values grows a lot, it might be necessary to - either have a dynamic way of listing them - or reduce the number of host (types) in the files. So far it does not seem to be a major issue.
chrzaszcz
force-pushed
the
auth-password-config
branch
from
9c68bd8
to
0f8e8ef
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3439 +/- ##
==========================================
+ Coverage 80.77% 80.78% +0.01%
==========================================
Files 415 415
Lines 32318 32316 -2
==========================================
+ Hits 26105 26107 +2
+ Misses 6213 6209 -4
Continue to review full report at Codecov.
|
This comment has been minimized.
This comment has been minimized.
chrzaszcz
force-pushed
the
auth-password-config
branch
from
4d10fc7
to
2a98f47
Compare
This comment has been minimized.
This comment has been minimized.
|
small_tests_24 / small_tests / 2a98f47 small_tests_23 / small_tests / 2a98f47 dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 2a98f47 dynamic_domains_mysql_redis_24 / mysql_redis / 2a98f47 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 2a98f47 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 2a98f47 ldap_mnesia_24 / ldap_mnesia / 2a98f47 rest_client_SUITE:messages:msg_is_sent_and_delivered_over_sse{error,{{badmap,{error,timeout}},
[{erlang,map_get,
[data,{error,timeout}],
[{error_info,#{module => erl_erts_errors}}]},
{rest_client_SUITE,msg_is_sent_and_delivered_over_sse,1,
[{file,"/home/circleci/project/big_tests/tests/rest_client_SUITE.erl"},
{line,217}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}}ldap_mnesia_23 / ldap_mnesia / 2a98f47 internal_mnesia_24 / internal_mnesia / 2a98f47 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 2a98f47 pgsql_mnesia_24 / pgsql_mnesia / 2a98f47 pgsql_mnesia_23 / pgsql_mnesia / 2a98f47 mssql_mnesia_24 / odbc_mssql_mnesia / 2a98f47 riak_mnesia_24 / riak_mnesia / 2a98f47 |
Move away from explicit Erlang terms to avoid increasing verbosity when introducing defaults and having to copy-paste code fragments when such defaults change.
chrzaszcz
force-pushed
the
auth-password-config
branch
from
2a98f47
to
953b388
Compare
There was a problem hiding this comment.
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, like, in
, it could have a new element like-record(option, {type :: mongoose_config_spec:option_type(),
default :: some_type(),
validate = any :: mongoose_config_validator:validator(),
process :: undefined | mongoose_config_parser_toml:processor(),
wrap = default :: mongoose_config_spec:wrapper()}).Probably there's some reasons not to do this one, like, Erlang would just create all the records with undefined there, but that's probably possible to work around. Maybe the work-around would be much more tedious and we should discard that idea?
Asking for some thoughts 🙂
|
small_tests_24 / small_tests / 953b388 small_tests_23 / small_tests / 953b388 dynamic_domains_mysql_redis_24 / mysql_redis / 953b388 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 953b388 sm_SUITE:parallel:messages_are_properly_flushed_during_resumption_p1_fsm_old{error,
{{badmatch,
{error,
{connection_step_failed,
{#Fun<sm_SUITE.11.68776247>,
{client,
<<"alicE_messages_are_properly_flushed_during_resumption_p1_fsm_old_1925@domain.example.com">>,
escalus_tcp,<0.31491.1>,undefined,
[{username,
<<"alicE_messages_are_properly_flushed_during_resumption_p1_fsm_old_1925">>},
{server,<<"domain.example.com">>},
{host,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_management,true},
{stream_id,<<"964a7edbe8588105">>}]},
[{compression,[<<"zlib">>]},
{starttls,true},
{stream_management,true},
{advanced_message_processing,true},
{client_state_indication,false},
{sasl_mechanisms,[<<"SCRAM-SHA-256">>,<<"PLAIN">>]},
{caps,undefined}]},
{timeout,get_resumed}}}},
[{sm_SUITE,
'-messages_are_properly_flushed_during_resumption_p1_fsm_old/1-fun-1-',
3,
[{file,"/home/circleci/project/big_tests/tests/sm_SUITE.erl"},
{line,1270}]},
{escalus_story,story,4,
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_se...dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 953b388 ldap_mnesia_23 / ldap_mnesia / 953b388 internal_mnesia_24 / internal_mnesia / 953b388 dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 953b388 ldap_mnesia_24 / ldap_mnesia / 953b388 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 953b388 pgsql_mnesia_24 / pgsql_mnesia / 953b388 pgsql_mnesia_23 / pgsql_mnesia / 953b388 mysql_redis_24 / mysql_redis / 953b388 mssql_mnesia_24 / odbc_mssql_mnesia / 953b388 pep_SUITE:pep_tests:authorize_access_model{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,<<"alicE_authorize_access_model_1852@localhost/res1">>,
escalus_tcp,<0.29059.1>,
[{event_manager,<0.29029.1>},
{server,<<"localhost">>},
{username,<<"alicE_authorize_access_model_1852">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.29029.1>},
{server,<<"localhost">>},
{username,<<"alicE_authorize_access_model_1852">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"alicE_authorize_access_model_1852">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"alicE_authorize_access_model_1852">>},
{server,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"08e248a050f4e372">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{pubsub_tools,receive_response,3,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,479}]},
{pubsub_tools,receive_and_check_response,4,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,470}]},
{pep_SUITE,'-authorize_access_model/1...pep_SUITE:pep_tests:delayed_receive_with_sm{error,{{badmatch,[]},
[{pep_SUITE,'-delayed_receive_with_sm/1-fun-0-',3,
[{file,"/home/circleci/project/big_tests/tests/pep_SUITE.erl"},
{line,295}]},
{escalus_story,story,4,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,72}]},
{test_server,ts_tc,3,[{file,"test_server.erl"},{line,1783}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1292}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}}pep_SUITE:pep_tests:send_caps_after_login_test{error,{{assertion_failed,assert_many,false,[is_roster_set,is_presence],[],[]},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{escalus_story,'-make_all_clients_friends/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,111}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,distinct_pairs,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,60}]},
{escalus_story,make_all_clients_friends,1,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,106}]}]}}pep_SUITE:pep_tests:unsubscribe_after_presence_unsubscription{error,{{assertion_failed,assert_many,false,
[is_roster_set,is_presence,is_presence],
[],[]},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{escalus_story,'-make_all_clients_friends/1-fun-0-',2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,114}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,distinct_pairs,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,60}]},
{escalus_story,make_all_clients_friends,1,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,106}]}]}}pep_SUITE:pep_tests:h_ok_after_notify_test{error,
{timeout_when_waiting_for_stanza,
[{escalus_client,wait_for_stanza,
[{client,<<"alicE_h_ok_after_notify_test_1851@localhost/res1">>,
escalus_tcp,<0.29046.1>,
[{event_manager,<0.29022.1>},
{server,<<"localhost">>},
{username,<<"alicE_h_ok_after_notify_test_1851">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.29022.1>},
{server,<<"localhost">>},
{username,<<"alicE_h_ok_after_notify_test_1851">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,<<"alicE_h_ok_after_notify_test_1851">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,<<"alicE_h_ok_after_notify_test_1851">>},
{server,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"9e2dce0ce59c6b68">>}]},
5000],
[{file,
"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_client.erl"},
{line,136}]},
{pubsub_tools,receive_response,3,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,479}]},
{pubsub_tools,receive_and_check_response,4,
[{file,"/home/circleci/project/big_tests/tests/pubsub_tools.erl"},
{line,470}]},
{pep_SUITE,'-h_ok_after_notify_test/1...pep_SUITE:pep_tests:publish_and_notify_test{error,{{assertion_failed,assert_many,false,
[is_roster_set,is_presence,is_presence],
[{xmlel,<<"presence">>,
[{<<"from">>,
<<"alicE_publish_and_notify_test_1846@localhost/res1">>},
{<<"to">>,
<<"bob_publish_and_notify_test_1846@localhost/res1">>},
{<<"xml:lang">>,<<"en">>}],
[]}],
" <presence from='alicE_publish_and_notify_test_1846@localhost/res1' to='bob_publish_and_notify_test_1846@localhost/res1' xml:lang='en'/>"},
[{escalus_new_assert,assert_true,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,'-each_with_index/3-fun-0-',3,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,87}]},
{lists,foldl,3,[{file,"lists.erl"},{line,1267}]},
{escalus_utils,distinct_pairs,2,
[{file,"/home/circleci/project/big_tests/_build/default/lib/escalus/src/escalus_utils.erl"},
{line,60}]},
{escalus_story,make_all_clients_friends,1,
[{file,"/home/circleci/project/big_te...riak_mnesia_24 / riak_mnesia / 953b388 |
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
|
| use_common_name -> [standard, common_name]; | ||
| allow_just_user_identity -> [standard, auth_id] | ||
| end. | ||
| mongoose_config:get_opt([{auth, HostType}, sasl_external]). |
There was a problem hiding this comment.
Yeah these kinds of things improve so much 😄
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_mechanismsandsasl_externalin theauthsection.Motivation - advantages of defaults:
Challenges:
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.*.optionsfiles 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.