Skip to content
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

Remove personal data from mod_roster #2301

Merged
merged 5 commits into from
May 21, 2019
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 35 additions & 5 deletions big_tests/tests/gdpr_SUITE.erl
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
retrieve_vcard/1,
remove_vcard/1,
retrieve_roster/1,
remove_roster/1,
retrieve_mam/1,
retrieve_offline/1,
retrieve_pubsub_payloads/1,
Expand Down Expand Up @@ -95,11 +96,13 @@ groups() ->
]},
{remove_personal_data, [], [
% per type
remove_vcard
remove_vcard,
remove_roster
]},
{remove_personal_data_with_mods_disabled, [], [
% per type
remove_vcard
remove_vcard,
remove_roster
]}
].

Expand Down Expand Up @@ -154,6 +157,10 @@ init_per_testcase(retrieve_mam = CN, Config) ->
dynamic_modules:ensure_modules(domain(), mam_required_modules(Backend)),
escalus:init_per_testcase(CN, Config)
end;
init_per_testcase(remove_roster = CN, Config) ->
dynamic_modules:ensure_modules(domain(), [{mod_roster, []}]),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need it at all? If it's really required, then it's broken for non-Mnesia jobs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix: Added mechanism of choosing backed.

escalus_fresh:clean(),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is usually invoked after test case or after suite.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to end per test case.

escalus:init_per_testcase(CN, Config);
init_per_testcase(CN, Config) ->
escalus:init_per_testcase(CN, Config).

Expand Down Expand Up @@ -270,14 +277,35 @@ retrieve_roster(Config) ->
escalus_story:make_all_clients_friends([Alice, Bob]),
BobU = escalus_utils:jid_to_lower(escalus_client:username(Bob)),
BobS = escalus_utils:jid_to_lower(escalus_client:server(Bob)),
ExpectedHeader = ["jid", "name", "subscription",
"ask", "groups", "askmessage", "xs"],
ExpectedItems = [
#{ "jid" => [{contains, BobU}, {contains, BobS}] }
],
maybe_stop_and_unload_module(mod_roster, mod_roster_backend, Config),
retrieve_and_validate_personal_data(
Alice, Config, "roster", ExpectedHeader, ExpectedItems)
Alice, Config, "roster", expected_header(mod_roster), ExpectedItems)
end).

remove_roster(Config) ->
escalus:fresh_story(Config, [{alice, 1}, {bob, 1}], fun(Alice, Bob) ->
escalus_story:make_all_clients_friends([Alice, Bob]),
AliceU = escalus_utils:jid_to_lower(escalus_client:username(Alice)),
AliceS = escalus_utils:jid_to_lower(escalus_client:server(Alice)),
ExpectedItems = [
#{ "jid" => [{contains, AliceU}, {contains, AliceS}] }
],

maybe_stop_and_unload_module(mod_roster, mod_roster_backend, Config),
{0, _} = unregister(Alice, Config),

mongoose_helper:wait_until(
fun() ->
mongoose_helper:successful_rpc(mod_roster, get_personal_data,
[AliceU, AliceS])
end,
[{roster, expected_header(mod_roster), []}]),
retrieve_and_validate_personal_data(
Bob, Config, "roster", expected_header(mod_roster), ExpectedItems)

end).

retrieve_mam(_Config) ->
Expand Down Expand Up @@ -762,3 +790,5 @@ validate_time1(Time) ->
check_list(List) ->
lists:all(fun({V, L}) -> I = list_to_integer(V), I >= 0 andalso I < L end, List).

expected_header(mod_roster) -> ["jid", "name", "subscription",
"ask", "groups", "askmessage", "xs"].
27 changes: 24 additions & 3 deletions src/mod_roster.erl
Original file line number Diff line number Diff line change
Expand Up @@ -883,11 +883,32 @@ remove_user(User, Server) ->
remove_user(Acc, User, Server) ->
LUser = jid:nodeprep(User),
LServer = jid:nameprep(Server),
Acc1 = send_unsubscription_to_rosteritems(Acc, LUser, LServer),
R = mod_roster_backend:remove_user(LUser, LServer),
mongoose_lib:log_if_backend_error(R, ?MODULE, ?LINE, {User, Server}),
Acc1 = maybe_send_unsubscription_to_rosteritems(Acc, LUser, LServer),
Backends = mongoose_lib:find_behaviour_implementations(mod_roster),
lists:foreach(fun(Backend) ->
try
Backend:remove_user(LUser, LServer)
catch
Class:Reason ->
StackTrace = erlang:get_stacktrace(),
?WARNING_MSG("event=cannot_delete_personal_data,backends = ~p,"
"backend=~p,class=~p,reason=~p,stacktrace=~p",
[Backends, Backend, Class, Reason, StackTrace])
end
end, Backends),
Acc1.

maybe_send_unsubscription_to_rosteritems(Acc, LUser, LServer) ->
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It should rather be named try_send... or send...rosteritems_safe because currently the name suggests that unsub. pushes are sent or not, depending on some condition, which is not the case. :)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

try_send... ++

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

renamed to try_send...

try
send_unsubscription_to_rosteritems(Acc, LUser, LServer)
catch
E:R ->
S = erlang:get_stacktrace(),
?WARNING_MSG("event=cannot_send_unsubscription_to_rosteritems,"
"class=~p,reason=~p,stacktrace=~p", [E, R, S]),
Acc
end.

%% For each contact with Subscription:
%% Both or From, send a "unsubscribed" presence stanza;
%% Both or To, send a "unsubscribe" presence stanza.
Expand Down