Fix the implementation so that it uses unregister command

big_tests/tests/gdpr_SUITE.erl

@@ -14,7 +14,6 @@
 -export([
          retrieve_vcard/1,
          remove_vcard/1,
-         try_remove_data_of_non_existing_user/1,
          retrieve_roster/1,
          retrieve_mam/1,
          retrieve_offline/1,
@@ -51,7 +50,8 @@ all() ->
      {group, retrieve_personal_data},
      {group, retrieve_personal_data_with_mods_disabled},
      {group, retrieve_negative},
-     {group, remove_personal_data}
+     {group, remove_personal_data},
+     {group, remove_personal_data_with_mods_disabled}
     ].
 
 groups() ->
@@ -93,9 +93,12 @@ groups() ->
      {retrieve_negative, [], [
                               data_is_not_retrieved_for_missing_user
                              ]},
-        {remove_personal_data, [parallel], [
+        {remove_personal_data, [], [
+            % per type
+            remove_vcard
+        ]},
+        {remove_personal_data_with_mods_disabled, [], [
             % per type
-            try_remove_data_of_non_existing_user,
             remove_vcard
         ]}
     ].
@@ -113,6 +116,8 @@ end_per_suite(Config) ->
 init_per_group(retrieve_personal_data_with_mods_disabled, Config) ->
     dynamic_modules:ensure_modules(domain(), pubsub_required_modules()),
     [{disable_module, true} | Config];
+init_per_group(remove_personal_data_with_mods_disabled, Config) ->
+    [{disable_module, true} | Config];
 init_per_group(retrieve_personal_data_pubsub, Config) ->
     dynamic_modules:ensure_modules(domain(), pubsub_required_modules()),
     Config;
@@ -138,6 +143,7 @@ init_per_testcase(remove_vcard = CN, Config) ->
         true ->
             {skip, skipped_for_simplicity_for_now}; % TODO: Fix the case for LDAP as well
         _ ->
+            vcard_started(),
             escalus:init_per_testcase(CN, Config)
     end;
 init_per_testcase(retrieve_mam = CN, Config) ->
@@ -190,6 +196,9 @@ pick_backend_for_mam() ->
 mam_required_modules(Backend) ->
     [{mod_mam_meta, [{backend, Backend}, {pm, []}]}].
 
+vcard_required_modules() ->
+    [{mod_vcard, [{backend, mnesia}]}].
+
 pubsub_required_modules() ->
     [{mod_caps, []}, {mod_pubsub, [
                                    {backend, mongoose_helper:mnesia_or_rdbms_backend()},
@@ -199,13 +208,14 @@ pubsub_required_modules() ->
                                   ]
                      }].
 
+vcard_started() ->
+    dynamic_modules:ensure_modules(domain(), vcard_required_modules()).
+
 %% -------------------------------------------------------------
 %% Test cases
 %% -------------------------------------------------------------
 
 %% ------------------------- Data retrieval - per type verification -------------------------
-try_remove_data_of_non_existing_user(Config) ->
-    {"Error: \"User does not exist\"\n", 1} = ejabberdctl("remove_personal_data", [<<"user_not_exist">>, <<"localhost">>], Config).
 
 retrieve_vcard(Config) ->
     escalus:fresh_story(Config, [{alice, 1}], fun(Alice) ->
@@ -233,9 +243,18 @@ remove_vcard(Config) ->
         AliceSetResultStanza
             = escalus:send_and_wait(Alice, escalus_stanza:vcard_update(AliceFields)),
         escalus:assert(is_iq_result, AliceSetResultStanza),
+        AliceU = escalus_utils:jid_to_lower(escalus_client:username(Alice)),
+        AliceS = escalus_utils:jid_to_lower(escalus_client:server(Alice)),
+
+        maybe_stop_and_unload_module(mod_vcard, mod_vcard_backend, Config),
+        {0, _} = unregister(Alice, Config),
+
+        mongoose_helper:wait_until(
+            fun() ->
+                mongoose_helper:successful_rpc(mod_vcard, get_personal_data,
+                    [AliceU, AliceS])
+            end, [{vcard,["jid","vcard"],[]}])
 
-        {0, _} = remove_personal_data(Alice, Config),
-        retrieve_and_validate_personal_data(Alice, Config, "vcard", ["jid", "vcard"], [])
         end).
 
 retrieve_roster(Config) ->
@@ -643,10 +662,10 @@ retrieve_personal_data(User, Domain, Config) ->
     {CommandOutput, Code} = ejabberdctl("retrieve_personal_data", [User, Domain, Filename], Config),
     {Filename, Code, CommandOutput}.
 
-remove_personal_data(Client, Config) ->
+unregister(Client, Config) ->
     User = escalus_client:username(Client),
     Domain = escalus_client:server(Client),
-    {CommandOutput, Code} = ejabberdctl("remove_personal_data", [User, Domain], Config),
+    {CommandOutput, Code} = ejabberdctl("unregister", [User, Domain], Config),
     {Code, CommandOutput}.
 
 random_filename(Config) ->

src/admin_extra/service_admin_extra_gdpr.erl

@@ -3,7 +3,7 @@
 -include("ejabberd_commands.hrl").
 -include("mongoose_logger.hrl").
 
--export([commands/0, retrieve_all/3, remove_all/2]).
+-export([commands/0, retrieve_all/3]).
 
 % Exported for RPC call
 -export([retrieve_logs/2]).
@@ -19,14 +19,6 @@ commands() -> [
             module = ?MODULE,
             function = retrieve_all,
             args = [{username, binary}, {domain, binary}, {path, binary}],
-            result = {res, rescode}},
-        #ejabberd_commands{name = remove_personal_data, tags = [gdpr],
-            desc = "Remove all user's presonal data.",
-            longdesc = "Removes all personal data from MongooseIM for a given user. Example:\n"
-                      " mongooseimctl remove_personal_data alice localhost ",
-            module = ?MODULE,
-            function = remove_all,
-            args = [{username, binary}, {domain, binary}],
             result = {res, rescode}}
     ].
 
@@ -56,15 +48,6 @@ retrieve_all(Username, Domain, ResultFilePath) ->
             {error, "User does not exist"}
     end.
 
--spec remove_all(jid:user(), jid:server()) -> ok | {error, Reason :: any()}.
-remove_all(Username, Domain) ->
-    case user_exists(Username, Domain) of
-        true ->
-            remove_data_from_modules(Username, Domain);
-        false ->
-            {error, "User does not exist"}
-    end.
-
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
 %%%                       Private funs
 %%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%%
@@ -90,13 +73,6 @@ try_get_data_from_module(Module, Username, Domain) ->
             []
     end.
 
--spec remove_data_from_modules(jid:user(), jid:server()) -> ok.
-remove_data_from_modules(Username, Domain) ->
-%%TODO: when all modules ready use the one from below
-%%    Modules = modules_with_personal_data(),
-    Modules = [mod_vcard],
-    lists:foreach(fun(M) -> M:remove_personal_data(Username, Domain) end, Modules).
-
 -spec to_csv_file(CsvFilename :: binary(), gdpr:schema(), gdpr:entities(), file:name()) -> ok.
 to_csv_file(Filename, DataSchema, DataRows, TmpDir) ->
     FilePath = <<(list_to_binary(TmpDir))/binary, "/", Filename/binary>>,

src/auth/ejabberd_auth.erl

@@ -544,8 +544,22 @@ do_remove_user(LUser, LServer) ->
                               lserver => LServer,
                               element => undefined }),
     ejabberd_hooks:run_fold(remove_user, LServer, Acc, [LUser, LServer]),
+    %% We need to take care about cleaning the data in modules that are currently disabled.
+    %% Mich duplicate Module:remove_user/2 calls for enabled modules
+    gdpr_remove_user_from_all_modules(LUser, LServer),
     ok.
 
+gdpr_remove_user_from_all_modules(LUser, LServer) ->
+    Modules = mongoose_lib:find_behaviour_implementations(gdpr),
+    lists:foreach(fun(M) -> try_remove_user_from_module(M, LUser, LServer) end, Modules).
+
+try_remove_user_from_module(Module, LUser, LServer) ->
+    try
+        Module:remove_user(LUser, LServer)
+    catch
+        _:_ -> []
+    end.
+
 %% @doc Try to remove user if the provided password is correct.
 %% The removal is attempted in each auth method provided:
 %% when one returns 'ok' the loop stops;

src/mod_vcard.erl

@@ -75,7 +75,7 @@
 -export([config_change/4]).
 
 %% GDPR related
--export([get_personal_data/2, remove_personal_data/2]).
+-export([get_personal_data/2]).
 
 -define(PROCNAME, ejabberd_mod_vcard).
 
@@ -148,21 +148,6 @@ get_personal_data(Username, Server) ->
                             end, mongoose_lib:find_behaviour_implementations(mod_vcard)),
     [{vcard, Schema, Entries}].
 
--spec remove_personal_data(jid:user(), jid:server()) -> ok.
-remove_personal_data(Username, Server) ->
-    LUser = jid:nodeprep(Username),
-    LServer = jid:nameprep(Server),
-    lists:foreach(fun(B) -> try_remove_personal_data(LUser, LServer, B) end, mongoose_lib:find_behaviour_implementations(mod_vcard)).
-
-try_remove_personal_data(LUser, LServer, Module) ->
-    try
-        Module:remove_user(LUser, LServer)
-    catch
-        _:_ ->
-            ok
-    end.
-
-
 -spec default_search_fields() -> list().
 default_search_fields() ->
     [{<<"User">>, <<"user">>},
@@ -430,7 +415,13 @@ remove_user(Acc, User, Server) ->
 remove_user(User, Server) ->
     LUser = jid:nodeprep(User),
     LServer = jid:nodeprep(Server),
-    mod_vcard_backend:remove_user(LUser, LServer).
+    lists:foreach(fun(B) ->
+        try
+            B:remove_user(LUser, LServer)
+        catch
+            _:_ ->
+                ok
+        end end, mongoose_lib:find_behaviour_implementations(mod_vcard)).
 
 %% react to "global" config change
 config_change(Acc, Host, ldap, _NewConfig) ->