Limit the number of users per domain #4059
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## master #4059 +/- ##
==========================================
+ Coverage 81.26% 83.86% +2.60%
==========================================
Files 526 526
Lines 33139 33168 +29
==========================================
+ Hits 26929 27817 +888
+ Misses 6210 5351 -859
☔ View full report in Codecov by Sentry. |
jacekwegr
force-pushed
the
limit-number-of-users-per-domain
branch
from
2c1acaa
to
a4dda2a
Compare
This comment was marked as outdated.
This comment was marked as outdated.
jacekwegr
force-pushed
the
limit-number-of-users-per-domain
branch
from
a4dda2a
to
4c5892e
Compare
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
src/auth/ejabberd_auth.erl
Outdated
| @@ -578,3 +583,13 @@ fold_auth_modules([AuthModule | AuthModules], F, CurAcc) -> | |||
| {stop, Value} -> | |||
| Value | |||
| end. | |||
|
|
|||
| is_user_limit_per_domain_exceeded(Domain) -> | |||
There was a problem hiding this comment.
Function name is misleading. If there are exactly as many users as the limit, it is not exceeded, yet it returns true.
doc/configuration/general.md
Outdated
| @@ -157,6 +157,13 @@ See the section about [redis connection setup](./outgoing-connections.md#redis-s | |||
|
|
|||
| When a user's session is replaced (due to a full JID conflict) by a new one, this parameter specifies the time MongooseIM waits for the old sessions to close. The default value is sufficient in most cases. If you observe `replaced_wait_timeout` warning in logs, then most probably the old sessions are frozen for some reason and it should be investigated. | |||
|
|
|||
| ### `general.max_users_per_domain` | |||
There was a problem hiding this comment.
I think this is in the wrong section - it is user account management, not session management.
Just a question: why not place it in the auth section which has all options related to user accounts?
There was a problem hiding this comment.
I’ve chosen to include it in the general section as I believed it was more connected with the domains themselves rather than user authentication. However, if it would be more appropriate, I can move it to the auth section.
There was a problem hiding this comment.
Actually it only limits certain authentication methods, because some of them (external, http, jwt, pki, ...) don't create users the usual way, so I think it's best placed in the auth section - not to give the impression that it works for all methods.
| Resp2 = register_user(Domain, proplists:get_value(user2, Config), Password, Config), | ||
| ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Resp2), <<"successfully registered">>)), | ||
| %% One user was registered in the init_per_group, and two more were registered in this test case | ||
| %% The next registration should exceed the limit of three |
There was a problem hiding this comment.
Three does not exceed the limit of three, to be precise.
| %% One user was registered in the init_per_group, and two more were registered in this test case | ||
| %% The next registration should exceed the limit of three | ||
| Resp3 = register_user(Domain, proplists:get_value(user3, Config), Password, Config), | ||
| ?assertMatch({_, _}, binary:match(get_err_msg(Resp3), <<"limit has been exceeded">>)). |
There was a problem hiding this comment.
Just a note: maybe make sure the third account is not created?
| end_per_testcase(admin_register_user_limit_error = C, Config) -> | ||
| Domain = domain_helper:domain(), | ||
| rpc(mim(), mongoose_account_api, unregister_user, [proplists:get_value(user1, Config), Domain]), | ||
| rpc(mim(), mongoose_account_api, unregister_user, [proplists:get_value(user2, Config), Domain]), |
There was a problem hiding this comment.
It might be good to delete all users here to make sure that if the user creation succeeds, all accounts are deleted and the state is clean.
Moved max_users_per_domain to the auth section in the configuration file. Adjusted tests. Made the name of the function and comments more clear.
|
small_tests_24 / small_tests / 3516724 elasticsearch_and_cassandra_25 / elasticsearch_and_cassandra_mnesia / 3516724 small_tests_25 / small_tests / 3516724 small_tests_25_arm64 / small_tests / 3516724 ldap_mnesia_24 / ldap_mnesia / 3516724 dynamic_domains_pgsql_mnesia_24 / pgsql_mnesia / 3516724 mam_SUITE:rdbms_async_cache_muc_all:muc_configurable_archiveid:muc_no_elements{failed,
{mam_SUITE,end_per_testcase,
{'EXIT',
{{room_archive_size,0,[{times,200,1}],ok},
[{mongoose_helper,do_wait_until,2,
[{file,
"/home/circleci/project/big_tests/tests/mongoose_helper.erl"},
{line,357}]},
{mam_helper,wait_for_room_archive_size,3,
[{file,
"/home/circleci/project/big_tests/tests/mam_helper.erl"},
{line,789}]},
{mam_helper,clean_room_archive,1,
[{file,
"/home/circleci/project/big_tests/tests/mam_helper.erl"},
{line,733}]},
{mam_helper,destroy_room,1,
[{file,
"/home/circleci/project/big_tests/tests/mam_helper.erl"},
{line,726}]},
{mam_SUITE,end_per_testcase,2,
[{file,
"/home/circleci/project/big_tests/tests/mam_SUITE.erl"},
{line,962}]},
{test_server,do_end_per_testcase,4,
[{file,"test_server.erl"},{line,1627}]},
{test_server,run_test_case_eval1,6,
[{file,"test_server.erl"},{line,1335}]},
{test_server,run_test_case_eval,9,
[{file,"test_server.erl"},{line,1224}]}]}}}}dynamic_domains_pgsql_mnesia_25 / pgsql_mnesia / 3516724 dynamic_domains_mysql_redis_25 / mysql_redis / 3516724 ldap_mnesia_25 / ldap_mnesia / 3516724 pgsql_mnesia_24 / pgsql_mnesia / 3516724 dynamic_domains_mssql_mnesia_25 / odbc_mssql_mnesia / 3516724 internal_mnesia_25 / internal_mnesia / 3516724 pgsql_mnesia_25 / pgsql_mnesia / 3516724 mysql_redis_25 / mysql_redis / 3516724 mssql_mnesia_25 / odbc_mssql_mnesia / 3516724 |
This PR adds an option to limit the number of users per domain. This feature allows the configuration of user limits based on host type, which can be adjusted within the
generalsection.