Section-based auth config #3446

chrzaszcz · 2021-12-10T16:36:28Z

Change the way auth methods are specified:

Sections for each auth method are now required.
A list of auth methods is no longer required. It can be used to enable selected methods in a specific order. When not specified, it is created automatically from the auth method sections in alphabetical order.

Motivation:

Check for required options was broken - when a method was on the list but there was no section for it (no options configured), the check did not work.
Defaults could not be introduced without having all sections always enabled, which would clutter the configuration.

Other changes:

Moved the check for unsupported auth methods and modules to the TOML processing. This way it is easier to test and more consistent. The tests are changed to have less mocking and to be consistent with the rest of the suite and the check for error patterns can now be used in other test cases as well.

Planned in the next PR:

Defaults for the auth methods.
Moving scram_iterations to auth.password.

Example 1 - before:

[auth]
  methods = ["ldap"]

  [auth.ldap]
    base = "ou=Users,dc=esl,dc=com"
    filter = "(objectClass=inetOrgPerson)"

After - the auth section itself can be skipped as it has no options:

[auth.ldap]
  base = "ou=Users,dc=esl,dc=com
  filter = "(objectClass=inetOrgPerson)"

Example 2 - before:

[auth]
  methods = ["rdbms"]
  scram_iterations = 64

After - the subsection is now necessary:

[auth]
  scram_iterations = 64
  [auth.rdbms]

Alternatively:

[auth]
  scram_iterations = 64
  rdbms = {}

Example 3 - most complicated, unlikely:

[auth]
  methods = ["rdbms", "ldap"]

  [auth.ldap]
    base = "ou=Users,dc=esl,dc=com"
    filter = "(objectClass=inetOrgPerson)"

After - rdbms section needs to be added. methods is still needed to sort them.

[auth]
  methods = ["rdbms", "ldap"]

  [auth.ldap]
    base = "ou=Users,dc=esl,dc=com"
    filter = "(objectClass=inetOrgPerson)"

  [auth.rdbms]

codecov · 2021-12-10T16:44:27Z

Codecov Report

Merging #3446 (33f2837) into master (461a10a) will decrease coverage by 0.00%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master    #3446      +/-   ##
==========================================
- Coverage   80.81%   80.81%   -0.01%     
==========================================
  Files         415      415              
  Lines       32317    32306      -11     
==========================================
- Hits        26117    26107      -10     
+ Misses       6200     6199       -1

Impacted Files	Coverage Δ
src/config/mongoose_config_parser.erl	`64.70% <50.00%> (-23.87%)`	⬇️
src/auth/mongoose_gen_auth.erl	`82.92% <100.00%> (ø)`
src/config/mongoose_config_spec.erl	`98.88% <100.00%> (+0.11%)`	⬆️
src/event_pusher/mod_event_pusher.erl	`65.00% <0.00%> (-22.50%)`	⬇️
src/config/mongoose_config_parser_toml.erl	`97.81% <0.00%> (-1.46%)`	⬇️
src/logger/mongoose_log_filter.erl	`78.08% <0.00%> (-1.37%)`	⬇️
src/domain/mongoose_domain_loader.erl	`89.28% <0.00%> (-0.90%)`	⬇️
src/mod_roster_rdbms.erl	`95.38% <0.00%> (-0.77%)`	⬇️
src/gen_mod.erl	`65.18% <0.00%> (-0.75%)`	⬇️
... and 10 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 461a10a...33f2837. Read the comment docs.

Now the auth mthods are declared by including the section(s) instead of listing the methods. The list of methods is optional and used to specify method order. Motivation: - Required method options were enforced only if the relevant section was present, but the section could be omitted even though the method was enabled. - The possibility to specify default values for method options without always including all subsections for unused methods. - Simplicity: in most cases only one method is enabled and the list of methods can be omitted.

Also: - Test auth methods with real names as the mocked modules were invalid. Using real names is easier than mocking helper functions inside mongoose_config_spec. - Test missing modules in a separate simple test case.

Use the callback for the root element for the check. Main motivation is to keep error handling consistent and simplify testing.

Errors are generated the same way for all types of issues, so the same code can be reused. Also: - Module names changed to real ones in the file test - assert_error logic is updated to handle more detailed tests with checking the reason and other attributes

Also: add missing information about the changed host_config.

mongoose-im · 2021-12-16T14:36:17Z

small_tests_24 / small_tests / 33f2837
Reports root / small

small_tests_23 / small_tests / 33f2837
Reports root / small

dynamic_domains_pgsql_mnesia_23 / pgsql_mnesia / 33f2837
Reports root/ big
OK: 2727 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0

dynamic_domains_mysql_redis_24 / mysql_redis / 33f2837
Reports root/ big
OK: 2710 / Failed: 0 / User-skipped: 203 / Auto-skipped: 0

dynamic_domains_mssql_mnesia_24 / odbc_mssql_mnesia / 33f2837
Reports root/ big
OK: 2727 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0

ldap_mnesia_24 / ldap_mnesia / 33f2837
Reports root/ big
OK: 1502 / Failed: 0 / User-skipped: 386 / Auto-skipped: 0

ldap_mnesia_23 / ldap_mnesia / 33f2837
Reports root/ big
OK: 1502 / Failed: 0 / User-skipped: 386 / Auto-skipped: 0

elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 33f2837
Reports root/ big
OK: 1880 / Failed: 0 / User-skipped: 313 / Auto-skipped: 0

pgsql_mnesia_24 / pgsql_mnesia / 33f2837
Reports root/ big
OK: 3114 / Failed: 0 / User-skipped: 195 / Auto-skipped: 0

pgsql_mnesia_23 / pgsql_mnesia / 33f2837
Reports root/ big
OK: 3114 / Failed: 0 / User-skipped: 195 / Auto-skipped: 0

mssql_mnesia_24 / odbc_mssql_mnesia / 33f2837
Reports root/ big
OK: 3114 / Failed: 0 / User-skipped: 195 / Auto-skipped: 0

mysql_redis_24 / mysql_redis / 33f2837
Reports root/ big
OK: 3109 / Failed: 0 / User-skipped: 200 / Auto-skipped: 0

riak_mnesia_24 / riak_mnesia / 33f2837
Reports root/ big
OK: 1726 / Failed: 0 / User-skipped: 314 / Auto-skipped: 0

dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 33f2837
Reports root/ big
OK: 2727 / Failed: 0 / User-skipped: 186 / Auto-skipped: 0

internal_mnesia_24 / internal_mnesia / 33f2837
Reports root/ big
OK: 1587 / Failed: 0 / User-skipped: 301 / Auto-skipped: 0

Premwoik

Looks good to me :) I like how unsupported auth methods and modules are checked now.

This comment has been minimized.

Sign in to view

mongoose-im mentioned this pull request Dec 10, 2021

CI build failures summary #3360

Closed

This comment has been minimized.

Sign in to view

chrzaszcz added 4 commits December 16, 2021 08:39

Format default (empty) auth config sections as maps for consistency

c7bd0d8

Update tests with the new required auth config sections

c8f900b

Also: - Test auth methods with real names as the mocked modules were invalid. Using real names is easier than mocking helper functions inside mongoose_config_spec. - Test missing modules in a separate simple test case.

Update tested config files with the new required auth sections

f6b679f

chrzaszcz force-pushed the auth-config-sections branch from 43b897d to 4ae6972 Compare December 16, 2021 07:42

This comment has been minimized.

Sign in to view

chrzaszcz added 3 commits December 16, 2021 09:01

Update auth config format in the templates

ed6b8d0

Check for unsupported auth methods and modules with 'process'

c613e09

Use the callback for the root element for the check. Main motivation is to keep error handling consistent and simplify testing.

chrzaszcz force-pushed the auth-config-sections branch from 4ae6972 to 35f510d Compare December 16, 2021 08:01

This comment has been minimized.

Sign in to view

chrzaszcz added 2 commits December 16, 2021 15:20

Remove cyrsasl_external - it has the default value

d618735

Document the section-based auth config

96c0701

chrzaszcz force-pushed the auth-config-sections branch from ad87bce to e95c1e0 Compare December 16, 2021 14:26

Add migration information for the section-based auth config

33f2837

Also: add missing information about the changed host_config.

chrzaszcz force-pushed the auth-config-sections branch from e95c1e0 to 33f2837 Compare December 16, 2021 14:27

This comment has been minimized.

Sign in to view

chrzaszcz marked this pull request as ready for review December 16, 2021 14:45

chrzaszcz added the waiting-for-review label Dec 17, 2021

Premwoik approved these changes Dec 17, 2021

View reviewed changes

Premwoik merged commit 38fcb5e into master Dec 17, 2021

Premwoik deleted the auth-config-sections branch December 17, 2021 12:48

Premwoik added this to the 5.1.0 milestone May 25, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Section-based auth config #3446

Section-based auth config #3446

chrzaszcz commented Dec 10, 2021 •

edited

codecov bot commented Dec 10, 2021 •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mongoose-im commented Dec 16, 2021 •

edited

Premwoik left a comment

Section-based auth config #3446

Section-based auth config #3446

Conversation

chrzaszcz commented Dec 10, 2021 • edited

codecov bot commented Dec 10, 2021 • edited

Codecov Report

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

mongoose-im commented Dec 16, 2021 • edited

Premwoik left a comment

Choose a reason for hiding this comment

chrzaszcz commented Dec 10, 2021 •

edited

codecov bot commented Dec 10, 2021 •

edited

mongoose-im commented Dec 16, 2021 •

edited