-
Notifications
You must be signed in to change notification settings - Fork 426
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Multitenancy/privacy #3189
Multitenancy/privacy #3189
Conversation
This comment has been minimized.
This comment has been minimized.
Codecov Report
@@ Coverage Diff @@
## master #3189 +/- ##
=======================================
Coverage 80.28% 80.29%
=======================================
Files 397 397
Lines 32430 32447 +17
=======================================
+ Hits 26037 26052 +15
- Misses 6393 6395 +2
Continue to review full report at Codecov.
|
e8daf28
to
656530f
Compare
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.
9dbf4f2
to
a7d71c0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
661aaf5
to
74f90aa
Compare
This comment has been minimized.
This comment has been minimized.
74f90aa
to
1168279
Compare
This comment has been minimized.
This comment has been minimized.
add5d5f
to
5d5f5e7
Compare
This comment has been minimized.
This comment has been minimized.
5d5f5e7
to
5a21b22
Compare
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I debugged the big test failures and found an issue. I am not sure if it is the only one, please make sure there are no more things like this one. Maybe we could just omit the dependency change in this PR and do it separately?
src/mod_blocking.erl
Outdated
deps(_HostType, Opts) -> | ||
[{mod_privacy, Opts, hard}]. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some tests stop mod_privacy
without stopping mod_blocking
, e.g. mod_global_distrib_SUITE
in line 196:
ModulesToStop = [mod_offline, mod_privacy, mod_roster, mod_last],
After that, modules are started with gen_mod_deps
, which does not start mod_privacy
as it is a dependency of mod_blocking
. This makes big tests fail.
5a21b22
to
a04752a
Compare
This comment has been minimized.
This comment has been minimized.
small_tests_24 / small_tests / 7db8730 internal_mnesia_24 / internal_mnesia / 7db8730 dynamic_domains_24 / pgsql_mnesia / 7db8730 dynamic_domains_23 / pgsql_mnesia / 7db8730 small_tests_22 / small_tests / 7db8730 small_tests_23 / small_tests / 7db8730 ldap_mnesia_24 / ldap_mnesia / 7db8730 ldap_mnesia_22 / ldap_mnesia / 7db8730 ldap_mnesia_23 / ldap_mnesia / 7db8730 pgsql_mnesia_24 / pgsql_mnesia / 7db8730 jingle_SUITE:all:resp_4xx_from_sip_proxy_results_in_session_terminate{error,
{{assertion_failed,assert,is_iq_result,
{xmlel,<<"iq">>,
[{<<"from">>,<<"error.480@localhost">>},
{<<"to">>,
<<"alice_resp_4xx_from_sip_proxy_results_in_session_terminate_94.137345@localhost/res1">>},
{<<"id">>,<<"ca3d6b5e-a84c-4127-9fb4-4017280a2bc1">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"jingle">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:1">>},
{<<"action">>,<<"session-terminate">>},
{<<"sid">>,<<"112694e8-2e01-4ddd-ab75-70fd040daf8e">>}],
[{xmlel,<<"reason">>,[],
[{xmlel,<<"general-error">>,[],[]},
{xmlel,<<"sip-error">>,
[{<<"code">>,<<"480">>}],
[{xmlcdata,<<"Temporarily Unavailable">>}]}]}]}]},
"<iq from='error.480@localhost' to='alice_resp_4xx_from_sip_proxy_results_in_session_terminate_94.137345@localhost/res1' id='ca3d6b5e-a84c-4127-9fb4-4017280a2bc1' type='set'><jingle xmlns='urn:xmpp:jingle:1' action='session-terminate' sid='112694e8-2e01-4ddd-ab75-70fd040daf8e'><reason><general-error/><sip-error code='480'>Temporarily Unavailable</sip-error></reason></jingle></iq>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{jingle_SUITE,send_initiate_and_wait_for_first_iq_set,2,
[{file,"/home/circleci/app/big_tests/tests/jingle_SUITE.erl"},
{line,393}]},
{jingle_SUITE,
'-res... elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 7db8730 pgsql_mnesia_22 / pgsql_mnesia / 7db8730 mysql_redis_24 / mysql_redis / 7db8730 mam_SUITE:rdbms_async_pool_prefs_cases:messages_filtered_when_prefs_default_policy_is_roster{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}} pgsql_mnesia_23 / pgsql_mnesia / 7db8730 mssql_mnesia_24 / odbc_mssql_mnesia / 7db8730 riak_mnesia_24 / riak_mnesia / 7db8730 |
), | ||
mongoose_riak:delete(?BKT_LISTS_NAMES(HostType), key(LUser, LServer)). | ||
|
||
%% TODO |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This TODO stuff should be implemented in other PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, unless anybody know how to implement this for riak, in riak keys are the whole jid as a binary, I don't know how to tell riak to remove keys by a pattern-match 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For this case when those keys will not deleted from riak - what will happens if we will try to put duplicate keys? It's can be provide tests for such a case and make sure that it does not break logic in other places?
Other question is - if we do not delete the keys, does this mean that the keys will be stored forever and gradually clutter up the space?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Keys are the entire jid of the user, so reinserting it would mean I guess reinserting this user's configuration. This hasn't changed, if the old logic was correct, the new one is as well, all that has changed is the key name.
This new hook remove_domain
is related to multitenancy, and no modules had it a month ago. I just don't know how to implement such thing for riak in this case, but I'm pretty confident riak and mod_privacy is not used by anyone, so I wasn't in a hurry to implement this new callback. Still, we should investigate how to delete keys based on a pattern for riak.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added some comments
small_tests_24 / small_tests / 190e39b small_tests_22 / small_tests / 190e39b dynamic_domains_24 / pgsql_mnesia / 190e39b dynamic_domains_23 / pgsql_mnesia / 190e39b internal_mnesia_24 / internal_mnesia / 190e39b small_tests_23 / small_tests / 190e39b ldap_mnesia_22 / ldap_mnesia / 190e39b ldap_mnesia_24 / ldap_mnesia / 190e39b ldap_mnesia_23 / ldap_mnesia / 190e39b elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / 190e39b mssql_mnesia_24 / odbc_mssql_mnesia / 190e39b jingle_SUITE:all:resp_4xx_from_sip_proxy_results_in_session_terminate{error,
{{assertion_failed,assert,is_iq_result,
{xmlel,<<"iq">>,
[{<<"from">>,<<"error.480@localhost">>},
{<<"to">>,
<<"alice_resp_4xx_from_sip_proxy_results_in_session_terminate_35.142848@localhost/res1">>},
{<<"id">>,<<"5e3a396b-396b-4311-a1c0-6d9673034dc8">>},
{<<"type">>,<<"set">>}],
[{xmlel,<<"jingle">>,
[{<<"xmlns">>,<<"urn:xmpp:jingle:1">>},
{<<"action">>,<<"session-terminate">>},
{<<"sid">>,<<"d9a1f71c-903b-4713-b0b2-c538cfcb951a">>}],
[{xmlel,<<"reason">>,[],
[{xmlel,<<"general-error">>,[],[]},
{xmlel,<<"sip-error">>,
[{<<"code">>,<<"480">>}],
[{xmlcdata,<<"Temporarily Unavailable">>}]}]}]}]},
"<iq from='error.480@localhost' to='alice_resp_4xx_from_sip_proxy_results_in_session_terminate_35.142848@localhost/res1' id='5e3a396b-396b-4311-a1c0-6d9673034dc8' type='set'><jingle xmlns='urn:xmpp:jingle:1' action='session-terminate' sid='d9a1f71c-903b-4713-b0b2-c538cfcb951a'><reason><general-error/><sip-error code='480'>Temporarily Unavailable</sip-error></reason></jingle></iq>"},
[{escalus_new_assert,assert_true,2,
[{file,
"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_new_assert.erl"},
{line,84}]},
{jingle_SUITE,send_initiate_and_wait_for_first_iq_set,2,
[{file,"/home/circleci/app/big_tests/tests/jingle_SUITE.erl"},
{line,393}]},
{jingle_SUITE,
'-res... pgsql_mnesia_24 / pgsql_mnesia / 190e39b pgsql_mnesia_22 / pgsql_mnesia / 190e39b mysql_redis_24 / mysql_redis / 190e39b mam_SUITE:rdbms_simple_prefs_cases:messages_filtered_when_prefs_default_policy_is_never{error,{test_case_failed,"ASSERT EQUAL\n\tExpected []\n\tValue [ok]\n"}} pgsql_mnesia_23 / pgsql_mnesia / 190e39b amp_big_SUITE:basic:error_deliver_to_stranger_test{error,
{test_case_failed,
{has_stanzas_but_shouldnt,
{client,
<<"alicE_error_deliver_to_stranger_test_69.415217@localhost/res1">>,
escalus_tcp,<0.2870.0>,
[{event_manager,<0.2745.0>},
{server,<<"localhost">>},
{username,
<<"alicE_error_deliver_to_stranger_test_69.415217">>},
{resource,<<"res1">>}],
[{event_client,
[{event_manager,<0.2745.0>},
{server,<<"localhost">>},
{username,
<<"alicE_error_deliver_to_stranger_test_69.415217">>},
{resource,<<"res1">>}]},
{resource,<<"res1">>},
{username,
<<"alicE_error_deliver_to_stranger_test_69.415217">>},
{server,<<"localhost">>},
{host,<<"localhost">>},
{port,5222},
{auth,{escalus_auth,auth_plain}},
{wspath,undefined},
{username,
<<"alicE_error_deliver_to_stranger_test_69.415217">>},
{server,<<"localhost">>},
{password,<<"matygrysa">>},
{stream_id,<<"aea6a540e8a8bc81">>}]},
[{xmlel,<<"stream:error">>,[],
[{xmlel,<<"conflict">>,
[{<<"xmlns">>,
<<"urn:ietf:params:xml:ns:xmpp-streams">>}],
[]},
{xmlel,<<"text">>,
[{<<"xml:lang">>,<<"en">>},
{<<"xmlns">>,
<<"urn:ietf:params:xml:ns:xmpp-streams">>}],
[{xmlcdata,<<"Replaced by new connection">>}]}]},
{xmlstreamend,<<... riak_mnesia_24 / riak_mnesia / 190e39b |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good changes, I like that migrations are already included.
I have a few minor comments.
[ | ||
block_jid_message, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: no reason to change this, especially that other lists do not contain such extra newlines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That was for me to be able to comment out test cases easily, without removing the list opening bracket. I have it like that in many other test suites.
[ | ||
{group, management}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: no reason to change this, especially that other lists do not contain such extra newlines.
@@ -181,11 +181,12 @@ GO | |||
SET ANSI_PADDING ON | |||
GO | |||
CREATE TABLE [dbo].[privacy_default_list]( | |||
[server] [nvarchar](250) NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the PRIMARY KEY
constraint below implies NOT NULL
for both columns.
server varchar(250) NOT NULL, | ||
username varchar(250) NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: the PRIMARY KEY
constraint below implies NOT NULL
for both columns.
@@ -200,6 +201,7 @@ GO | |||
SET ANSI_PADDING ON | |||
GO | |||
CREATE TABLE [dbo].[privacy_list]( | |||
[server] [nvarchar](250) NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note: for other DBs we have a primary key here. Maybe it would be good to add it for MS SQL as well for consistency.
); | ||
|
||
CREATE TABLE privacy_list ( | ||
server varchar(250) NOT NULL, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor, note: PRIMARY KEY
implies NOT NULL
.
updated_list(_, #userlist{name = SameName}, #userlist{name = SameName} = New) -> New; | ||
updated_list(_, Old, _) -> Old. | ||
|
||
|
||
%% ------------------------------------------------------------------ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor: I like the single lines more, less clutter in the code.
190e39b
to
b02ac0c
Compare
small_tests_24 / small_tests / b02ac0c internal_mnesia_24 / internal_mnesia / b02ac0c small_tests_22 / small_tests / b02ac0c dynamic_domains_24 / pgsql_mnesia / b02ac0c dynamic_domains_23 / pgsql_mnesia / b02ac0c small_tests_23 / small_tests / b02ac0c ldap_mnesia_22 / ldap_mnesia / b02ac0c ldap_mnesia_24 / ldap_mnesia / b02ac0c ldap_mnesia_23 / ldap_mnesia / b02ac0c pgsql_mnesia_22 / pgsql_mnesia / b02ac0c elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / b02ac0c pgsql_mnesia_23 / pgsql_mnesia / b02ac0c pgsql_mnesia_24 / pgsql_mnesia / b02ac0c mysql_redis_24 / mysql_redis / b02ac0c mssql_mnesia_24 / odbc_mssql_mnesia / b02ac0c riak_mnesia_24 / riak_mnesia / b02ac0c dynamic_domains_SUITE:with_mod_dynamic_domains_test:packet_handling_for_subdomain{error,
{{badrpc,
{'EXIT',
{timeout,
[{meck_proc,wait,6,
[{file,
"/home/circleci/app/_build/default/lib/meck/src/meck_proc.erl"},
{line,171}]},
{meck,wait,5,[]}]}}},
[{distributed_helper,rpc,
[#{node => mongooseim@localhost},
meck,wait,
[3,mod_dynamic_domains_test,process_packet,5,500]],
[{file,"/home/circleci/app/big_tests/tests/distributed_helper.erl"},
{line,117}]},
{dynamic_domains_SUITE,'-packet_handling_for_subdomain/1-fun-3-',1,
[{file,
"/home/circleci/app/big_tests/tests/dynamic_domains_SUITE.erl"},
{line,113}]},
{escalus_story,story,4,
[{file,
"/home/circleci/app/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}]}]}} |
b02ac0c
to
a9c7461
Compare
small_tests_24 / small_tests / a9c7461 internal_mnesia_24 / internal_mnesia / a9c7461 small_tests_22 / small_tests / a9c7461 dynamic_domains_24 / pgsql_mnesia / a9c7461 dynamic_domains_23 / pgsql_mnesia / a9c7461 small_tests_23 / small_tests / a9c7461 ldap_mnesia_22 / ldap_mnesia / a9c7461 ldap_mnesia_24 / ldap_mnesia / a9c7461 ldap_mnesia_23 / ldap_mnesia / a9c7461 pgsql_mnesia_24 / pgsql_mnesia / a9c7461 elasticsearch_and_cassandra_24 / elasticsearch_and_cassandra_mnesia / a9c7461 mssql_mnesia_24 / odbc_mssql_mnesia / a9c7461 pgsql_mnesia_22 / pgsql_mnesia / a9c7461 mysql_redis_24 / mysql_redis / a9c7461 mod_event_pusher_rabbit_SUITE:group_chat_message_publish:group_chat_message_received_event_properly_formatted{error,
{{assertMatch,
[{module,mod_event_pusher_rabbit_SUITE},
{line,435},
{expression,
"get_decoded_message_from_rabbit ( AliceGroupChatMsgRecvRK )"},
{pattern,
"# { << \"from_user_id\" >> := BobRoomJID , << \"to_user_id\" >> := AliceFullJID , << \"message\" >> := Message }"},
{value,
#{<<"from_user_id">> => <<"[email protected]">>,
<<"message">> => <<>>,
<<"to_user_id">> =>
<<"alice_unnamed_66.369271@localhost/res1">>}}]},
[{mod_event_pusher_rabbit_SUITE,
'-group_chat_message_received_event_properly_formatted/1-fun-1-',3,
[{file,
"/home/circleci/app/big_tests/tests/mod_event_pusher_rabbit_SUITE.erl"},
{line,435}]},
{escalus_story,story,4,
[{file,
"/home/circleci/app/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}]}]}} pgsql_mnesia_23 / pgsql_mnesia / a9c7461 riak_mnesia_24 / riak_mnesia / a9c7461 mod_ping_SUITE:server_ping:server_ping_pong{error,{{badmatch,[{[<<"localhost">>,mod_ping,ping_response],
{expected_diff,5},
{before_story,0},
{after_story,4}}]},
[{escalus_mongooseim,post_story_check_metrics,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,74}]},
{escalus_mongooseim,maybe_check_metrics_post_story,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,51}]},
{escalus_story,story,4,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,75}]},
{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}]}]}} mod_ping_SUITE:server_ping_kill:server_ping_pong{error,{{badmatch,[{[<<"localhost">>,mod_ping,ping_response],
{expected_diff,5},
{before_story,10},
{after_story,14}}]},
[{escalus_mongooseim,post_story_check_metrics,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,74}]},
{escalus_mongooseim,maybe_check_metrics_post_story,1,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_mongooseim.erl"},
{line,51}]},
{escalus_story,story,4,
[{file,"/home/circleci/app/big_tests/_build/default/lib/escalus/src/escalus_story.erl"},
{line,75}]},
{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}]}]}} |
This also takes the chance that we're modifying the default config to enable
mod_cache_users
, which wasn't enabled before.