* src/ejabberd_auth.erl: If anonymous auth is enabled, when

checking if the account already exists in other auth methods, take
into account if the auth method failed (EJAB-882)
* src/ejabberd_auth_anonymous.erl: Likewise
* src/ejabberd_auth_external.erl: Likewise
* src/ejabberd_auth_internal.erl: Likewise
* src/ejabberd_auth_ldap.erl: Likewise
* src/ejabberd_auth_odbc.erl: Likewise
* src/ejabberd_auth_pam.erl: Likewise

SVN Revision: 1966

8 files changed, 102 insertions, 45 deletions

diff --git a/ChangeLog b/ChangeLog
index d00f57b51..fcdbf34ea 100644
--- a/ChangeLog
+++ b/ChangeLog
@@ -1,3 +1,15 @@
+2009-03-04  Badlop  <badlop@process-one.net>
+
+	* src/ejabberd_auth.erl: If anonymous auth is enabled, when
+	checking if the account already exists in other auth methods, take
+	into account if the auth method failed (EJAB-882)
+	* src/ejabberd_auth_anonymous.erl: Likewise
+	* src/ejabberd_auth_external.erl: Likewise
+	* src/ejabberd_auth_internal.erl: Likewise
+	* src/ejabberd_auth_ldap.erl: Likewise
+	* src/ejabberd_auth_odbc.erl: Likewise
+	* src/ejabberd_auth_pam.erl: Likewise
+
 2009-03-04  Christophe Romain <christophe.romain@process-one.net>
 
 	* src/mod_pubsub/mod_pubsub.erl: Allow node creation without configure
diff --git a/src/ejabberd_auth.erl b/src/ejabberd_auth.erl
index 4be455b06..35b434f63 100644
--- a/src/ejabberd_auth.erl
+++ b/src/ejabberd_auth.erl
@@ -78,20 +78,21 @@ plain_password_required(Server) ->
 %% @spec (User::string(), Server::string(), Password::string()) ->
 %%     true | false
 check_password(User, Server, Password) ->
-    lists:any(
-      fun(M) ->
-	      M:check_password(User, Server, Password)
-      end, auth_modules(Server)).
+    case check_password_with_authmodule(User, Server, Password) of
+	{true, _AuthModule} -> true;
+	false -> false
+    end.
 
 %% @doc Check if the user and password can login in server.
 %% @spec (User::string(), Server::string(), Password::string(),
 %%        StreamID::string(), Digest::string()) ->
 %%     true | false
 check_password(User, Server, Password, StreamID, Digest) ->
-    lists:any(
-      fun(M) ->
-	      M:check_password(User, Server, Password, StreamID, Digest)
-      end, auth_modules(Server)).
+    case check_password_with_authmodule(User, Server, Password,
+					StreamID, Digest) of
+	{true, _AuthModule} -> true;
+	false -> false
+    end.
 
 %% @doc Check if the user and password can login in server.
 %% The user can login if at least an authentication method accepts the user
@@ -104,27 +105,23 @@ check_password(User, Server, Password, StreamID, Digest) ->
 %%                 | ejabberd_auth_internal | ejabberd_auth_ldap
 %%                 | ejabberd_auth_odbc | ejabberd_auth_pam
 check_password_with_authmodule(User, Server, Password) ->
-    Res = lists:dropwhile(
-	    fun(M) ->
-		    not apply(M, check_password,
-			      [User, Server, Password])
-	    end, auth_modules(Server)),
-    case Res of
-	[] -> false;
-	[AuthMod | _] -> {true, AuthMod}
-    end.
+    check_password_loop(auth_modules(Server), [User, Server, Password]).
 
 check_password_with_authmodule(User, Server, Password, StreamID, Digest) ->
-    Res = lists:dropwhile(
-	    fun(M) ->
-		    not apply(M, check_password,
-			      [User, Server, Password, StreamID, Digest])
-	    end, auth_modules(Server)),
-    case Res of
-	[] -> false;
-	[AuthMod | _] -> {true, AuthMod}
+    check_password_loop(auth_modules(Server), [User, Server, Password,
+					       StreamID, Digest]).
+
+check_password_loop([], _Args) ->
+    false;
+check_password_loop([AuthModule | AuthModules], Args) ->
+    case apply(AuthModule, check_password, Args) of
+	true ->
+	    {true, AuthModule};
+	false ->
+	    check_password_loop(AuthModules, Args)
     end.
 
+
 %% @spec (User::string(), Server::string(), Password::string()) ->
 %%       ok | {error, ErrorType}
 %% where ErrorType = empty_password | not_allowed | invalid_jid
@@ -259,11 +256,26 @@ is_user_exists(User, Server) ->
 
 %% Check if the user exists in all authentications module except the module
 %% passed as parameter
+%% @spec (Module::atom(), User, Server) -> true | false | maybe
 is_user_exists_in_other_modules(Module, User, Server) ->
-    lists:any(
-      fun(M) ->
-	      M:is_user_exists(User, Server)
-      end, auth_modules(Server)--[Module]).
+    is_user_exists_in_other_modules_loop(
+      auth_modules(Server)--[Module],
+      User, Server).
+is_user_exists_in_other_modules_loop([], _User, _Server) ->
+    false;
+is_user_exists_in_other_modules_loop([AuthModule|AuthModules], User, Server) ->
+    case AuthModule:is_user_exists(User, Server) of
+	true ->
+	    true;
+	false ->
+	    is_user_exists_in_other_modules_loop(AuthModules, User, Server);
+	{error, Error} ->
+	    ?DEBUG("The authentication module ~p returned an error~nwhen "
+		   "checking user ~p in server ~p~nError message: ~p",
+		   [AuthModule, User, Server, Error]),
+	    maybe
+    end.
+
 
 %% @spec (User, Server) -> ok | error | {error, not_allowed}
 %% @doc Remove user.
diff --git a/src/ejabberd_auth_anonymous.erl b/src/ejabberd_auth_anonymous.erl
index 47a75ac29..9540e7c8c 100644
--- a/src/ejabberd_auth_anonymous.erl
+++ b/src/ejabberd_auth_anonymous.erl
@@ -178,7 +178,10 @@ check_password(User, Server, _Password, _StreamID, _Digest) ->
     %% they however are "reserved")
     case ejabberd_auth:is_user_exists_in_other_modules(?MODULE, 
 						       User, Server) of
+	%% If user exists in other module, reject anonnymous authentication
 	true  -> false;
+	%% If we are not sure whether the user exists in other module, reject anon auth
+	maybe  -> false;
 	false -> login(User, Server)
     end.
 
diff --git a/src/ejabberd_auth_external.erl b/src/ejabberd_auth_external.erl
index af7c5b048..d35d37f73 100644
--- a/src/ejabberd_auth_external.erl
+++ b/src/ejabberd_auth_external.erl
@@ -83,8 +83,13 @@ get_password(_User, _Server) ->
 get_password_s(_User, _Server) ->
     "".
 
+%% @spec (User, Server) -> true | false | {error, Error}
 is_user_exists(User, Server) ->
-    extauth:is_user_exists(User, Server).
+    try extauth:is_user_exists(User, Server) of
+	Res -> Res
+    catch
+	_:Error -> {error, Error}
+    end.
 
 remove_user(_User, _Server) ->
     {error, not_allowed}.
diff --git a/src/ejabberd_auth_internal.erl b/src/ejabberd_auth_internal.erl
index 14459521c..2b8c62fd6 100644
--- a/src/ejabberd_auth_internal.erl
+++ b/src/ejabberd_auth_internal.erl
@@ -225,6 +225,7 @@ get_password_s(User, Server) ->
 	    []
     end.
 
+%% @spec (User, Server) -> true | false | {error, Error}
 is_user_exists(User, Server) ->
     LUser = jlib:nodeprep(User),
     LServer = jlib:nameprep(Server),
@@ -234,8 +235,8 @@ is_user_exists(User, Server) ->
 	    false;
 	[_] ->
 	    true;
-	_ ->
-	    false
+	Other ->
+	    {error, Other}
     end.
 
 %% @spec (User, Server) -> ok
diff --git a/src/ejabberd_auth_ldap.erl b/src/ejabberd_auth_ldap.erl
index 2063ee5fc..1e57e0216 100644
--- a/src/ejabberd_auth_ldap.erl
+++ b/src/ejabberd_auth_ldap.erl
@@ -179,10 +179,11 @@ get_password(_User, _Server) ->
 get_password_s(_User, _Server) ->
     "".
 
+%% @spec (User, Server) -> true | false | {error, Error}
 is_user_exists(User, Server) ->
     case catch is_user_exists_ldap(User, Server) of
-	{'EXIT', _} ->
-	    false;
+	{'EXIT', Error} ->
+	    {error, Error};
 	Result ->
 	    Result
     end.
diff --git a/src/ejabberd_auth_odbc.erl b/src/ejabberd_auth_odbc.erl
index da2e6cb59..edd493581 100644
--- a/src/ejabberd_auth_odbc.erl
+++ b/src/ejabberd_auth_odbc.erl
@@ -57,6 +57,7 @@ start(_Host) ->
 plain_password_required() ->
     false.
 
+%% @spec (User, Server, Password) -> true | false | {error, Error}
 check_password(User, Server, Password) ->
     case jlib:nodeprep(User) of
 	error ->
@@ -64,14 +65,22 @@ check_password(User, Server, Password) ->
 	LUser ->
 	    Username = ejabberd_odbc:escape(LUser),
 	    LServer = jlib:nameprep(Server),
-	    case catch odbc_queries:get_password(LServer, Username) of
+	    try odbc_queries:get_password(LServer, Username) of
 		{selected, ["password"], [{Password}]} ->
-		    Password /= "";
-		_ ->
-		    false
+		    Password /= ""; %% Password is correct, and not empty
+		{selected, ["password"], [{_Password2}]} ->
+		    false; %% Password is not correct
+		{selected, ["password"], []} ->
+		    false; %% Account does not exist
+		{error, _Error} ->
+		    false %% Typical error is that table doesn't exist
+	    catch
+		_:_ ->
+		    false %% Typical error is database not accessible
 	    end
     end.
 
+%% @spec (User, Server, Password, StreamID, Digest) -> true | false | {error, Error}
 check_password(User, Server, Password, StreamID, Digest) ->
     case jlib:nodeprep(User) of
 	error ->
@@ -79,7 +88,8 @@ check_password(User, Server, Password, StreamID, Digest) ->
 	LUser ->
 	    Username = ejabberd_odbc:escape(LUser),
 	    LServer = jlib:nameprep(Server),
-	    case catch odbc_queries:get_password(LServer, Username) of
+	    try odbc_queries:get_password(LServer, Username) of
+		%% Account exists, check if password is valid
 		{selected, ["password"], [{Passwd}]} ->
 		    DigRes = if
 				 Digest /= "" ->
@@ -92,8 +102,13 @@ check_password(User, Server, Password, StreamID, Digest) ->
 		       true ->
 			    (Passwd == Password) and (Password /= "")
 		    end;
-		_ ->
-		    false
+		{selected, ["password"], []} ->
+		    false; %% Account does not exist
+		{error, _Error} ->
+		    false %% Typical error is that table doesn't exist
+	    catch
+		_:_ ->
+		    false %% Typical error is database not accessible
 	    end
     end.
 
@@ -204,6 +219,7 @@ get_password_s(User, Server) ->
 	    end
     end.
 
+%% @spec (User, Server) -> true | false | {error, Error}
 is_user_exists(User, Server) ->
     case jlib:nodeprep(User) of
 	error ->
@@ -211,11 +227,16 @@ is_user_exists(User, Server) ->
 	LUser ->
 	    Username = ejabberd_odbc:escape(LUser),
 	    LServer = jlib:nameprep(Server),
-	    case catch odbc_queries:get_password(LServer, Username) of
+	    try odbc_queries:get_password(LServer, Username) of
 		{selected, ["password"], [{_Password}]} ->
-		    true;
-		_ ->
-		    false
+		    true; %% Account exists
+		{selected, ["password"], []} ->
+		    false; %% Account does not exist
+		{error, Error} ->
+		    {error, Error} %% Typical error is that table doesn't exist
+	    catch
+		_:B ->
+		    {error, B} %% Typical error is database not accessible
 	    end
     end.
 
diff --git a/src/ejabberd_auth_pam.erl b/src/ejabberd_auth_pam.erl
index c8a8031c2..72237282e 100644
--- a/src/ejabberd_auth_pam.erl
+++ b/src/ejabberd_auth_pam.erl
@@ -80,6 +80,8 @@ get_password(_User, _Server) ->
 get_password_s(_User, _Server) ->
     "".
 
+%% @spec (User, Server) -> true | false | {error, Error}
+%% TODO: Improve this function to return an error instead of 'false' when connection to PAM failed
 is_user_exists(User, Host) ->
     Service = get_pam_service(Host),
     case catch epam:acct_mgmt(Service, User) of