Support dynamic domains in auth backends #3106
Conversation
Codecov Report
@@ Coverage Diff @@
## master #3106 +/- ##
==========================================
+ Coverage 79.22% 79.38% +0.15%
==========================================
Files 389 390 +1
Lines 31959 31936 -23
==========================================
+ Hits 25321 25352 +31
+ Misses 6638 6584 -54
Continue to review full report at Codecov.
|
leszke
force-pushed
the
multi-tenancy-dynamic-domains-in-auth
branch
from
ee60b1b to
aebbbe1
Compare
chrzaszcz
force-pushed
the
multi-tenancy-dynamic-domains-in-auth
branch
4 times, most recently
from
da50419 to
4ff3765
Compare
chrzaszcz
force-pushed
the
multi-tenancy-dynamic-domains-in-auth
branch
9 times, most recently
from
a8f0f50 to
dab4db4
Compare
Key differences:
- Callbacks take host type as the first argument
- More optional callbacks for less cluttered backend code
and more unified default values.
- One 'is_exported' helper to be consistent for all functions.
'ensure_loaded' is called always as it is a very cheap call
for already loaded modules.
- Reworked 'get_registered_users*'
- Less calls for simplicity
- Removed the "dirty" call for all users as it is not needed
(it called ejabberd_auth domain-by-domain in a convoluted way).
- Removed 'vh' as this infix was confusing.
There are no other versions of these functions,
so a 'domain' suffix does not seem necessary.
chrzaszcz
force-pushed
the
multi-tenancy-dynamic-domains-in-auth
branch
from
dab4db4 to
546401b
Compare
This change prepares the removal of 'dirty_get_registered_users',
which has the following logic:
- For each auth backend:
- For each domain using that backend [1]:
- Get users for that domain
The new logic does the same, but in a simpler way.
[1] Does not apply to 'internal', which is not recommended anyway.
We could optimize 'get_registered_users' for Mnesia
if we ever need to do so (not likely).
This commit prepares the removal of dirty_get_registered_users - Do the user (count) checks for the two configured domains - Do the carboncopy check (was unused)
Note: this commit starts a sequence of changes that break tests,
because the backends are not reworked yet.
The last commit it the sequence will make them pass again.
- Extract looping through auth modules to a separate function
- makes the logic more consistent
- allows easier introduction of host types
- Remove dirty_get_registered_users
- Replace 'does_user_exist_in_other_modules' with a more specific
'does_stored_user_exist' as it was used only for that purpose.
The API functions are not changed if not necessary
e.g. the 'vh' infix is not removed (yet)
Also: - Simplify the is_protocol_enabled logic
Also: - Add specs
Convert extauth to use host types as well
Group user-related arguments in 'params'. This approach could be used for backend callbacks as well.
- Keep dirty_get_registered_users for its efficiency.
It can be used in the Erlang shell if needed.
- Simplify names of 'get_*users*' functions
Main motivation: code readability
Remove unimplemented optional callbacks.
- Set/get the state for host types - Use lserver in dn_filter to support dynamic domains - Remove unimplemented callbacks and unused functions - User registration still works only for a single domain (as before)
Store the domain in a new column.
- Make it the first column in the primary key for queries
by domain name.
- The name 'server' is used for consistency.
Bucket type needs to be configured differently for each host type to avoid conflicts when dynamic domains are used with multiple host types. This might need to be documented later.
Also:
The errors for 'authorize' were simplified,
only 'not_authorized' needs to be handled now.
This was inconsistent across the sasl modules anyway.
Also: remove tests for deleted functions. This commit fixes the tests. They should pass from now on.
chrzaszcz
force-pushed
the
multi-tenancy-dynamic-domains-in-auth
branch
from
546401b to
f4fc9b4
Compare
| ok. | ||
|
|
||
| -spec remove_domain(mongooseim:host_type(), jid:lserver()) -> ok. |
There was a problem hiding this comment.
This handler is present only in the dummy module out of the auth modules, right? Is it an oversight?
[edited - sorry, I made a silly error reading the spec]
There was a problem hiding this comment.
this is not a hook handler. this is ejabberd_auth callback. the hook handler is in ejabberd_auth
There was a problem hiding this comment.
Oh, sorry, for some reason I misread the spec... Obviously this is fine - I'll edit the comment. But still it's a callback that is present only in the dummy module - is it deliberate?
There was a problem hiding this comment.
Handling domain removal has to be added to the TODO list. Doing it now.
There was a problem hiding this comment.
Generally looks good, the only thing that make me worrying that we don't have tests for ejabberd_auth module itself. We should definitely create some unit tests for it with multiple mocked auth methods that return different values to ensure that checks are done until the first successful and in the same sequence as methods configured. Also, that would be nice to ensure that mongoose_gen_auth handles properly missing optional callbacks (can be done in the same ejabberd_auth unit tests). Being honest, I would trust such unit tests more than any code review, because it's a bit hard to follow all the changes.
Approving this PR, but please let's create a ticket for ejabberd_auth unit tests.
I think we could add more tests when the unification of error handling in |
| fun check_private/0, | ||
| fun check_vcard/0, | ||
| fun check_roster/0, | ||
| fun check_carboncopy/0], |
There was a problem hiding this comment.
@chrzaszcz I've just realised, this is checking an ets table called carboncopy that doesn't really exists in mim 🤔
Rework the auth backends to support host types and dynamic domains.
mongoose_gen_authbehaviour is introduced with updated and simplified callbacks and a new API.mongoose_authis unified and updated.The following is intentionally not done yet, but it can be done in upcoming PR's:
remove_domainin backends.ejabberd_authAPI functions e.g.get_vh_*to make them more consistent with the backend API.ejabberd_auth, which differs between API functions and can be unified. After unification, add more tests forejabberd_auth.ejabberd_auth_anonymous,ejabberd_auth_externalLUser, LServer, HostTypetriplet in a map, maybe extended with optional password data.supported_features/0- why have it when all backends support dynamic domains?For details, see individual commits.