Multi tenancy auth #3063

DenysGonchar · 2021-03-30T23:45:49Z

This PR introduces the new callback supported_features/0 for auth backends:

for now there is the only one optional feature - dynamic_domains.
currently only ejabberd_auth_dummy supports dynamic_domains feature.
all other auth modules must be rechecked and adopted in order to
support this feature.
the following things must be done per auth module:
- recheck that ejabberd_auth set_opts and get_opt interfaces are
  used with host type, not domain name.
- recheck that mongoose_scram interfaces are used with host type,
  not domain name.
- recheck that ejabberd_config get_local_option interfaces are
  used with host type, not domain name.
- create proper tests to ensure that auth module works correctly
  with dynamic domains.

In order to reduce the number of calls to mongoose_domain_api:get_host_type/1 HostType parameter is added to some callbacks of ejabberd_gen_auth behaviour.

to run test execute: test-runner.sh --skip-cover --skip-small-tests --preset pgsql_mnesia --db pgsql -- dynamic_domains_pm:can_authenticate

codecov · 2021-03-31T00:16:13Z

Codecov Report

Merging #3063 (b6df430) into multi-tenancy-phase-1 (f991ee2) will increase coverage by 0.61%.
The diff coverage is 89.01%.

@@                    Coverage Diff                    @@
##           multi-tenancy-phase-1    #3063      +/-   ##
=========================================================
+ Coverage                  78.09%   78.71%   +0.61%     
=========================================================
  Files                        374      386      +12     
  Lines                      31219    31643     +424     
=========================================================
+ Hits                       24382    24909     +527     
+ Misses                      6837     6734     -103

Impacted Files	Coverage Δ
src/admin_extra/service_admin_extra.erl	`100.00% <ø> (ø)`
src/ejabberd_gen_sm.erl	`90.90% <ø> (ø)`
src/ejabberd_local.erl	`62.68% <0.00%> (-1.50%)`	⬇️
src/ejabberd_users.erl	`81.63% <ø> (ø)`
src/muc_light/mod_muc_light_db_mnesia.erl	`89.47% <ø> (+4.47%)`	⬆️
src/offline/mod_offline_mnesia.erl	`51.47% <0.00%> (ø)`
src/pubsub/pubsub_form_utils.erl	`56.60% <0.00%> (+1.04%)`	⬆️
src/rdbms/rdbms_queries_mssql.erl	`100.00% <ø> (ø)`
src/auth/ejabberd_auth_pki.erl	`17.64% <20.00%> (ø)`
src/auth/ejabberd_auth_external.erl	`29.12% <38.46%> (ø)`
... and 108 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 b5ad312...b6df430. Read the comment docs.

…se_domain_core_SUITE:init_per_testcase/2

esuring dialyzer is not complaning: ./rebar3 dialyzer

introducing the new callback supported_features/0: * for now there is the only one optional feature - dynamic_domains. * currently only ejabberd_auth_dummy supports dynamic_domains feature. * all other auth modules must be rechecked and adopted in order to support dynamic_domains feature. * the following things must be done per auth module: - recheck that ejabberd_auth set_opts and get_opt interfaces are used with host type, not domain name. - recheck that mongoose_scram interfaces are used with host type, not domain name. - recheck that ejabberd_config get_local_option interfaces are used with host type, not domain name. - create proper tests to ensure that auth module works correctly with dynamic domains. commit verified with the following commands: test-runner.sh --skip-cover --skip-small-tests --preset pgsql_mnesia --db pgsql -- dynamic_domains_pm:can_authenticate ./rebar3 dialyzer

verified with commands: tools/setup-redis.sh ./rebar3 ct

tested with this command: ./rebar3 ct --suite auth_dummy_SUITE --case supports_dynamic_domains

tested with command: ./rebar3 ct --suite config_parser_SUITE --group host_types_group

it causes execution of meck:unload in parallel which results in spontaneous failuers. tested with: ./rebar3 ct --suite config_parser_SUITE

chrzaszcz

Lots of good work! Just a few comments from me.

Also: please take care of the failing tests and update the description.

big_tests/tests/dynamic_domains_pm_SUITE.erl

src/auth/ejabberd_auth_anonymous.erl

test/auth_http_SUITE.erl

test/commands_SUITE.erl

test/config_parser_SUITE.erl

src/config/mongoose_config_parser.erl

chrzaszcz · 2021-04-06T12:02:34Z

include/ejabberd_c2s.hrl

@@ -29,6 +29,7 @@
                tls_options = [],
                tls_verify            :: verify_none | verify_peer,
                authenticated = false :: authenticated_state(),
+                host_type = <<>>      :: binary(),


Is there any reason to set an empty host type?

otherwise, dialyzer complains :)

Hm... maybe it has a reason to complain? What is the error? If it's in tests, then ok, but are you sure that we won't get an empty binary for an online user?

it complains that undefined is not binary() type. and nope - we won't have it empty for an online user.
we set a valid host_type at the very early stage of a session, actually authentication is impossible without it.

could be binary() | undefined type

Then it will complain about usage of undefined type everywhere where only binary() is expected.
There is no problem with the current type definition, there is no problem with the default value either.

I like what @arcusfelis suggested. Dialyzer does not complain, I checked it.

Dialyzer doesn't complain about it right now, because we don't really use that field yet. But we will start in the following PRs, I'm absolutely against this change. Dialyzer is not smart enough to understand FSM lifecycle and understand that in some state it cannot be undefined any more.
empty binary is the same illegal value for the host type as undefined, we cannot configure it in toml.

I still think Dialyzer would not complain because of how success typing works. However, it is a minor thing for me and I can merge the changes as they are now.

did some extra tests, looks you're right. will change it in the following PR.

src/auth/ejabberd_auth.erl

…iour

chrzaszcz

Looks good to me.

arcusfelis

looks reasonable

implementing can_authenticate test case for dynamic_domains_pm_SUITE

d37d00e

to run test execute: test-runner.sh --skip-cover --skip-small-tests --preset pgsql_mnesia --db pgsql -- dynamic_domains_pm:can_authenticate

DenysGonchar added the WIP 🚧 label Mar 30, 2021

DenysGonchar changed the base branch from master to multi-tenancy-phase-1 March 30, 2021 23:46

arcusfelis force-pushed the multi-tenancy-phase-1 branch from d7f064a to 135fec2 Compare April 2, 2021 12:13

DenysGonchar added 3 commits April 2, 2021 19:03

extending can_authenticate test case in dynamic_domains_pm_SUITE

739f086

adding validation that mongoose_domain_core starts properly at mongoo…

7caff14

…se_domain_core_SUITE:init_per_testcase/2

make auth. passing for dynamic domains with dummy auth module

05b6c6d

esuring dialyzer is not complaning: ./rebar3 dialyzer

DenysGonchar force-pushed the multi-tenancy-auth branch from dd90346 to a39052b Compare April 3, 2021 17:53

fixing unit tests

df0c1a3

DenysGonchar force-pushed the multi-tenancy-auth branch 3 times, most recently from 46c4ef3 to 7d46418 Compare April 6, 2021 01:32

DenysGonchar added 7 commits April 6, 2021 09:58

fixing auth. unit tests

01270c6

verified with commands: tools/setup-redis.sh ./rebar3 ct

adding test for ejabberd_auth:does_method_support/2 method

5a52a21

tested with this command: ./rebar3 ct --suite auth_dummy_SUITE --case supports_dynamic_domains

adding auth. methods verification for host types

2003504

tested with command: ./rebar3 ct --suite config_parser_SUITE --group host_types_group

mergin host_types_missing_* test cases into one

30f75e1

reworking host_types_missing_auth_methods_and_modules test case

3de1bf4

avoid parallel execution of the tests with host_types_group

a2843c1

it causes execution of meck:unload in parallel which results in spontaneous failuers. tested with: ./rebar3 ct --suite config_parser_SUITE

DenysGonchar force-pushed the multi-tenancy-auth branch from 7e97adb to a2843c1 Compare April 6, 2021 08:25

chrzaszcz reviewed Apr 6, 2021

View reviewed changes

making check_password callbacks mandatory for ejabberd_gen_auth behav…

ffbe9cf

…iour

DenysGonchar added waiting-for-review and removed WIP 🚧 labels Apr 6, 2021

minor changes after code review

b6df430

chrzaszcz approved these changes Apr 7, 2021

View reviewed changes

chrzaszcz merged commit 4171b6f into multi-tenancy-phase-1 Apr 7, 2021

chrzaszcz deleted the multi-tenancy-auth branch April 7, 2021 08:27

arcusfelis approved these changes Apr 7, 2021

View reviewed changes

DenysGonchar mentioned this pull request Apr 22, 2021

Multi tenancy phase 1 #3095

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Multi tenancy auth #3063

Multi tenancy auth #3063

DenysGonchar commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

chrzaszcz left a comment •

edited

Loading

chrzaszcz Apr 6, 2021

DenysGonchar Apr 6, 2021

chrzaszcz Apr 6, 2021

DenysGonchar Apr 6, 2021 •

edited

Loading

arcusfelis Apr 6, 2021

DenysGonchar Apr 6, 2021

chrzaszcz Apr 7, 2021

DenysGonchar Apr 7, 2021 •

edited

Loading

chrzaszcz Apr 7, 2021

DenysGonchar Apr 8, 2021 •

edited

Loading

chrzaszcz left a comment

arcusfelis left a comment

Multi tenancy auth #3063

Multi tenancy auth #3063

Conversation

DenysGonchar commented Mar 30, 2021 • edited Loading

codecov bot commented Mar 31, 2021 • edited Loading

Codecov Report

chrzaszcz left a comment • edited Loading

Choose a reason for hiding this comment

chrzaszcz Apr 6, 2021

Choose a reason for hiding this comment

DenysGonchar Apr 6, 2021

Choose a reason for hiding this comment

chrzaszcz Apr 6, 2021

Choose a reason for hiding this comment

DenysGonchar Apr 6, 2021 • edited Loading

Choose a reason for hiding this comment

arcusfelis Apr 6, 2021

Choose a reason for hiding this comment

DenysGonchar Apr 6, 2021

Choose a reason for hiding this comment

chrzaszcz Apr 7, 2021

Choose a reason for hiding this comment

DenysGonchar Apr 7, 2021 • edited Loading

Choose a reason for hiding this comment

chrzaszcz Apr 7, 2021

Choose a reason for hiding this comment

DenysGonchar Apr 8, 2021 • edited Loading

Choose a reason for hiding this comment

chrzaszcz left a comment

Choose a reason for hiding this comment

arcusfelis left a comment

Choose a reason for hiding this comment

DenysGonchar commented Mar 30, 2021 •

edited

Loading

codecov bot commented Mar 31, 2021 •

edited

Loading

chrzaszcz left a comment •

edited

Loading

DenysGonchar Apr 6, 2021 •

edited

Loading

DenysGonchar Apr 7, 2021 •

edited

Loading

DenysGonchar Apr 8, 2021 •

edited

Loading