Improve error handling in account API

esl · Oct 28, 2022 · 9667a19 · 9667a19
1 parent 0704606
commit 9667a19
Show file tree

Hide file tree

Showing 10 changed files with 137 additions and 64 deletions.
diff --git a/big_tests/tests/graphql_account_SUITE.erl b/big_tests/tests/graphql_account_SUITE.erl
@@ -10,6 +10,7 @@
                          execute_domain_admin_command/4, get_unauthorized/1]).
 
 -define(NOT_EXISTING_JID, <<"unknown987@unknown">>).
+-define(NOT_EXISTING_NAME, <<"unknown987@", (domain_helper:domain())/binary>>).
 
 suite() ->
     require_rpc_nodes([mim]) ++ escalus:suite().
@@ -36,7 +37,7 @@ admin_account_tests() ->
      admin_count_users,
      admin_count_users_unknown_domain,
      admin_check_password,
-     admin_check_password_non_exisiting_user,
+     admin_check_password_non_existing_user,
      admin_check_password_hash,
      admin_check_password_hash_non_existing_user,
      admin_check_plain_password_hash,
@@ -47,7 +48,9 @@ admin_account_tests() ->
      admin_remove_non_existing_user,
      admin_remove_existing_user,
      admin_ban_user,
-     admin_change_user_password].
+     admin_ban_non_existing_user,
+     admin_change_user_password,
+     admin_change_non_existing_user_password].
 
 domain_admin_tests() ->
     [admin_list_users,
@@ -174,8 +177,9 @@ user_unregister_story(Config, Alice) ->
     ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Resp), <<"successfully unregistered">>)),
     % Ensure the user is removed
     AllUsers = rpc(mim(), mongoose_account_api, list_users, [domain_helper:domain()]),
+    {_, AllUsersList} = AllUsers,
     LAliceJID = jid:to_binary(jid:from_binary(escalus_client:short_jid(Alice))),
-    ?assertNot(lists:member(LAliceJID, AllUsers)).
+    ?assertNot(lists:member(LAliceJID, AllUsersList)).
 
 user_change_password(Config) ->
     escalus:fresh_story_with_config(Config, [{alice, 1}], fun user_change_password_story/2).
@@ -199,7 +203,7 @@ admin_list_users(Config) ->
 
 admin_list_users_unknown_domain(Config) ->
     Resp = list_users(<<"unknown-domain">>, Config),
-    ?assertEqual([], get_ok_value([data, account, listUsers], Resp)).
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"Domain does not exist">>)).
 
 admin_count_users(Config) ->
     % A domain with at least one user
@@ -209,7 +213,7 @@ admin_count_users(Config) ->
 
 admin_count_users_unknown_domain(Config) ->
     Resp = count_users(<<"unknown-domain">>, Config),
-    ?assertEqual(0, get_ok_value([data, account, countUsers], Resp)).
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"Domain does not exist">>)).
 
 admin_check_password(Config) ->
     Password = lists:last(escalus_users:get_usp(Config, alice)),
@@ -222,7 +226,7 @@ admin_check_password(Config) ->
     Resp2 = check_password(BinJID, <<"incorrect_pw">>, Config),
     ?assertMatch(#{<<"correct">> := false, <<"message">> := _}, get_ok_value(Path, Resp2)).
 
-admin_check_password_non_exisiting_user(Config) ->
+admin_check_password_non_existing_user(Config) ->
     Password = lists:last(escalus_users:get_usp(Config, alice)),
     Resp = check_password(?NOT_EXISTING_JID, Password, Config),
     ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"not exist">>)).
@@ -278,7 +282,10 @@ admin_register_user(Config) ->
     ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Resp1), <<"successfully registered">>)),
     % Try to register a user with existing name
     Resp2 = register_user(Domain, Username, Password, Config),
-    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp2), <<"already registered">>)).
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp2), <<"already registered">>)),
+    % Try to register a user without any name
+    Resp3 = register_user(Domain, <<"">>, Password, Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp3), <<"Invalid JID">>)).
 
 admin_register_random_user(Config) ->
     Password = <<"my_password">>,
@@ -293,9 +300,12 @@ admin_register_random_user(Config) ->
     {ok, _} = rpc(mim(), mongoose_account_api, unregister_user, [Username, Server]).
 
 admin_remove_non_existing_user(Config) ->
+    % Non-existing user, non-existing domain
     Resp = remove_user(?NOT_EXISTING_JID, Config),
-    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"not exist">>)).
-
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"not exist">>)),
+    % Non-existing user, existing domain
+    Resp2 = remove_user(?NOT_EXISTING_NAME, Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp2), <<"not exist">>)).
 admin_remove_existing_user(Config) ->
     escalus:fresh_story(Config, [{alice, 1}], fun(Alice) ->
         Path = [data, account, removeUser, message],
@@ -317,8 +327,12 @@ admin_ban_user(Config) ->
 
 admin_ban_non_existing_user(Config) ->
     Reason = <<"annoying">>,
+    % Non-existing name, non-existing domain
     Resp = ban_user(?NOT_EXISTING_JID, Reason, Config),
-    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"not allowed">>)).
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"does not exist">>)),
+    % Non-existing name, existing domain
+    Resp2 = ban_user(?NOT_EXISTING_NAME, Reason, Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp2), <<"does not exist">>)).
 
 admin_change_user_password(Config) ->
     Path = [data, account, changeUserPassword, message],
@@ -333,10 +347,14 @@ admin_change_user_password(Config) ->
         ?assertNotEqual(nomatch, binary:match(get_ok_value(Path, Resp2), <<"Password changed">>))
     end).
 
-admin_change_non_exisisting_user_password(Config) ->
+admin_change_non_existing_user_password(Config) ->
     NewPassword = <<"new password">>,
+    % Non-existing name, non-existing domain
     Resp = change_user_password(?NOT_EXISTING_JID, NewPassword, Config),
-    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"not allowed">>)).
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp), <<"does not exist">>)),
+    % Non-existing name, existing domain
+    Resp2 = change_user_password(?NOT_EXISTING_NAME, NewPassword, Config),
+    ?assertNotEqual(nomatch, binary:match(get_err_msg(Resp2), <<"does not exist">>)).
 
 domain_admin_list_users_no_permission(Config) ->
     % An unknown domain

diff --git a/big_tests/tests/rest_SUITE.erl b/big_tests/tests/rest_SUITE.erl
@@ -415,7 +415,7 @@ password_change_errors(Config) ->
     Alice = binary_to_list(escalus_users:get_username(Config, alice)),
     {AnonUser, AnonDomain} = anon_us(),
     Args = #{newpass => <<"secret">>},
-    {?FORBIDDEN, <<"Password change not allowed">>} =
+    {?FORBIDDEN, <<"User does not exist or you are not authorised properly">>} =
         putt(admin, <<"/users/", AnonDomain/binary, "/", AnonUser/binary>>, Args),
     {?BAD_REQUEST, <<"Missing user name">>} =
         putt(admin, path("users", []), Args),
@@ -425,6 +425,8 @@ password_change_errors(Config) ->
         putt(admin, path("users", [Alice]), #{newpass => <<>>}),
     {?BAD_REQUEST, <<"Invalid JID">>} =
         putt(admin, path("users", ["@invalid"]), Args).
+    % {?BAD_REQUEST, <<"You are not authorized properly">>} =
+    %     putt(user, path("users", ["bob"]), Args), %% move it to rest_client_SUITE.erl
 
 anon_us() ->
     AnonConfig = [{escalus_users, escalus_ct:get_config(escalus_anon_users)}],

diff --git a/priv/graphql/schemas/admin/account.gql b/priv/graphql/schemas/admin/account.gql
@@ -3,10 +3,10 @@ Allow admin to get information about accounts.
 """
 type AccountAdminQuery @protected{
   "List users per domain"
-  listUsers(domain: String!): [String!]!
+  listUsers(domain: String!): [String!]
     @protected(type: DOMAIN, args: ["domain"])
   "Get number of users per domain"
-  countUsers(domain: String!): Int!
+  countUsers(domain: String!): Int
     @protected(type: DOMAIN, args: ["domain"])
   "Check if a password is correct"
   checkPassword(user: JID!, password: String!): CheckPasswordPayload

diff --git a/src/auth/ejabberd_auth.erl b/src/auth/ejabberd_auth.erl
@@ -188,25 +188,38 @@ check_digest(Digest, DigestGen, _Password, Passwd) ->
     Digest == DigestGen(Passwd).
 
 -spec set_password(jid:jid() | error, binary()) ->
-    ok | {error, empty_password | not_allowed | invalid_jid}.
+    ok | {error, empty_password | not_allowed | invalid_jid }.
 set_password(_, <<"">>) ->
     {error, empty_password};
 set_password(error, _) ->
     {error, invalid_jid};
 set_password(#jid{luser = LUser, lserver = LServer}, Password) ->
+    JID = jid:make(LUser, LServer, <<>>),
     F = fun(HostType, Mod) ->
                 case mongoose_gen_auth:set_password(Mod, HostType, LUser, LServer, Password) of
                     ok -> {stop, ok};
                     {error, Error} -> {continue, {error, Error}}
                 end
         end,
     Opts = #{default => {error, not_allowed}},
-    call_auth_modules_for_domain(LServer, F, Opts).
+    case ejabberd_auth:does_user_exist(JID) of
+        true ->
+            case call_auth_modules_for_domain(LServer, F, Opts) of
+                {ok, Result} ->
+                    {ok, Result};
+                Error ->
+                    Error
+            end;
+        false ->
+            {error, not_allowed}
+    end.
 
 -spec try_register(jid:jid() | error, binary()) ->
-    ok | {error, exists | not_allowed | invalid_jid | null_password}.
-try_register(_, <<"">>) ->
+    ok | {error, exists | not_allowed | invalid_jid | null_password }.
+try_register(_, <<>>) ->
     {error, null_password};
+try_register(#jid{luser = <<>>}, _) ->
+    {error, invalid_jid};
 try_register(error, _) ->
     {error, invalid_jid};
 try_register(JID, Password) ->
@@ -349,30 +362,36 @@ does_method_support(AuthMethod, Feature) ->
 
 %% @doc Remove user.
 %% Note: it may return ok even if there was some problem removing the user.
--spec remove_user(JID :: jid:jid()) -> ok | {error, not_allowed};
+-spec remove_user(JID :: jid:jid()) -> ok | {error, not_allowed | user_does_not_exist};
                  (error) -> error.
 remove_user(error) -> error;
 remove_user(#jid{luser = LUser, lserver = LServer}) ->
+    JID = jid:make(LUser, LServer, <<>>),
     F = fun(HostType, Mod) ->
                 case mongoose_gen_auth:remove_user(Mod, HostType, LUser, LServer) of
                     ok -> {continue, {ok, HostType}};
                     {error, _Error} -> continue
                 end
         end,
-    case call_auth_modules_for_domain(LServer, F, #{default => {error, not_allowed}}) of
-        {ok, HostType} ->
-            Acc = mongoose_acc:new(#{ location => ?LOCATION,
-                                       host_type => HostType,
-                                       lserver => LServer,
-                                       element => undefined }),
-            mongoose_hooks:remove_user(Acc, LServer, LUser),
-            ok;
-        Error ->
-            ?LOG_ERROR(#{what => backend_disallows_user_removal,
-                         user => LUser, server => LServer,
-                         reason => Error}),
-            Error
-    end.
+    case ejabberd_auth:does_user_exist(JID) of
+        true ->
+            case call_auth_modules_for_domain(LServer, F, #{default => {error, not_allowed}}) of
+                {ok, HostType} ->
+                    Acc = mongoose_acc:new(#{location => ?LOCATION,
+                                                host_type => HostType,
+                                                lserver => LServer,
+                                                element => undefined}),
+                    mongoose_hooks:remove_user(Acc, LServer, LUser),
+                    ok;
+                Error ->
+                    ?LOG_ERROR(#{what => backend_disallows_user_removal,
+                    user => LUser, server => LServer,
+                    reason => Error}),
+                    Error
+                end;
+        false ->
+            {error, user_does_not_exist}
+        end.
 
 %% @doc Calculate informational entropy.
 -spec entropy(iolist()) -> float().

diff --git a/src/ejabberd_admin.erl b/src/ejabberd_admin.erl
@@ -241,7 +241,7 @@ unregister(User, Host) ->
     mongoose_account_api:unregister_user(User, Host).
 
 
--spec registered_users(Host :: jid:server()) -> [binary()].
+-spec registered_users(Host :: jid:server()) -> mongoose_account_api:list_user_result().
 registered_users(Host) ->
     mongoose_account_api:list_users(Host).
 

diff --git a/src/ejabberd_ctl.erl b/src/ejabberd_ctl.erl
@@ -464,8 +464,9 @@ format_result(ElementsTuple, {_Name, {tuple, ElementsDef}}) ->
        fun({Element, ElementDef}) ->
                ["\t" | format_result(Element, ElementDef)]
        end,
-       ElementsAndDef)].
-
+       ElementsAndDef)];
+format_result({ok, List}, ListDef) ->
+    format_result(List, ListDef).
 
 -spec make_status(ok | true | _) -> 0 | 1.
 make_status(ok) -> ?STATUS_SUCCESS;

diff --git a/src/graphql/admin/mongoose_graphql_account_admin_mutation.erl b/src/graphql/admin/mongoose_graphql_account_admin_mutation.erl
@@ -55,7 +55,7 @@ format_user_payload(InResult, JID) ->
         {ok, Msg} ->
             {ok, make_user_payload(Msg, jid:to_binary(JID))};
         Result ->
-            make_error(Result, #{jid => JID})
+            make_error(Result, #{jid => jid:to_binary(JID)})
     end.
 
 -spec make_user_payload(string(), jid:literal_jid()) -> map().

diff --git a/src/graphql/admin/mongoose_graphql_account_admin_query.erl b/src/graphql/admin/mongoose_graphql_account_admin_query.erl
@@ -22,21 +22,30 @@ execute(_Ctx, _Obj, <<"checkUser">>, Args) ->
 
 % Internal
 
--spec list_users(map()) -> {ok, [{ok, binary()}]}.
+-spec list_users(map()) -> {ok, [{ok, binary()}]} | {error, resolver_error()}.
 list_users(#{<<"domain">> := Domain}) ->
-    Users = mongoose_account_api:list_users(Domain),
-    Users2 = lists:map(fun(U) -> {ok, U} end, Users),
-    {ok, Users2}.
+    case mongoose_account_api:list_users(jid:nameprep(Domain)) of
+        {domain_not_found, Msg} ->
+            make_error(domain_not_found, Msg, #{domain => Domain});
+        {ok, Users} ->
+            Users2 = lists:map(fun(U) -> {ok, U} end, Users),
+            {ok, Users2}
+    end.
 
 -spec count_users(map()) -> {ok, non_neg_integer()}.
 count_users(#{<<"domain">> := Domain}) ->
-    {ok, mongoose_account_api:count_users(Domain)}.
+    case mongoose_account_api:count_users(Domain) of
+        {domain_not_found, Msg} ->
+            make_error(domain_not_found, Msg, #{domain => Domain});
+        {ok, UserCount} ->
+            {ok, UserCount}
+    end.
 
 -spec check_password(map()) -> {ok, map()} | {error, resolver_error()}.
 check_password(#{<<"user">> := JID, <<"password">> := Password}) ->
     case mongoose_account_api:check_password(JID, Password) of
         {user_does_not_exist, Msg} ->
-            make_error(user_does_not_exist, Msg, #{jid => JID});
+            make_error(user_does_not_exist, Msg, #{jid => jid:to_binary(JID)});
         {incorrect, Msg} ->
             {ok, #{<<"correct">> => false, <<"message">> => Msg}};
         {ok, Msg} ->
@@ -54,7 +63,7 @@ check_password_hash(#{<<"user">> := JID, <<"passwordHash">> := Hash,
         {ok, Msg} ->
             {ok, #{<<"correct">> => true, <<"message">> => Msg}};
         {ErrCode, Msg} ->
-            make_error(ErrCode, Msg, #{jid => JID})
+            make_error(ErrCode, Msg, #{jid => jid:to_binary(JID)})
     end.
 
 -spec check_user(map()) -> {ok, map()}.