Merge pull request #1223 from processone/expand_api

More API fixes and improvements

9 files changed, 132 insertions, 81 deletions

diff --git a/src/cyrsasl_oauth.erl b/src/cyrsasl_oauth.erl
index 16f1e3dfb..80ba315ed 100644
--- a/src/cyrsasl_oauth.erl
+++ b/src/cyrsasl_oauth.erl
@@ -51,7 +51,7 @@ mech_step(State, ClientIn) ->
                     {ok,
                      [{username, User}, {authzid, AuthzId},
                       {auth_module, ejabberd_oauth}]};
-                false ->
+                _ ->
                     {error, <<"not-authorized">>, User}
             end;
         _ -> {error, <<"bad-protocol">>}
diff --git a/src/ejabberd_admin.erl b/src/ejabberd_admin.erl
index f20aeebf0..615459f77 100644
--- a/src/ejabberd_admin.erl
+++ b/src/ejabberd_admin.erl
@@ -87,6 +87,7 @@ get_commands_spec() ->
 			args = [], result = {res, rescode}},
      #ejabberd_commands{name = reopen_log, tags = [logs, server],
 			desc = "Reopen the log files",
+			policy = admin,
 			module = ?MODULE, function = reopen_log,
 			args = [], result = {res, rescode}},
      #ejabberd_commands{name = rotate_log, tags = [logs, server],
@@ -380,13 +381,12 @@ register(User, Host, Password) ->
 	{atomic, ok} ->
 	    {ok, io_lib:format("User ~s@~s successfully registered", [User, Host])};
 	{atomic, exists} ->
-	    String = io_lib:format("User ~s@~s already registered at node ~p",
-				   [User, Host, node()]),
-	    {exists, String};
+	    Msg = io_lib:format("User ~s@~s already registered", [User, Host]),
+	    {error, conflict, 10090, Msg};
 	{error, Reason} ->
 	    String = io_lib:format("Can't register user ~s@~s at node ~p: ~p",
 				   [User, Host, node(), Reason]),
-	    {cannot_register, String}
+	    {error, cannot_register, 10001, String}
     end.
 
 unregister(User, Host) ->
diff --git a/src/ejabberd_commands.erl b/src/ejabberd_commands.erl
index 2c095440f..a8b3e25ab 100644
--- a/src/ejabberd_commands.erl
+++ b/src/ejabberd_commands.erl
@@ -425,7 +425,7 @@ get_command_definition(Name, Version) ->
                          {V, C}
                  end)))) of
         [{_, Command} | _ ] -> Command;
-        _E -> throw(unknown_command)
+        _E -> throw({error, unknown_command})
     end.
 
 -spec get_commands_definition(integer()) -> [ejabberd_commands()].
@@ -682,7 +682,7 @@ check_auth(Command, {User, Server, {oauth, Token}, _}) ->
     case ejabberd_oauth:check_token(User, Server, ScopeList, Token) of
         true ->
             {ok, User, Server};
-        false ->
+        _ ->
             throw({error, invalid_account_data})
     end;
 check_auth(_Command, {User, Server, Password, _}) when is_binary(Password) ->
diff --git a/src/ejabberd_ctl.erl b/src/ejabberd_ctl.erl
index d52d1c0a9..d52b55cf9 100644
--- a/src/ejabberd_ctl.erl
+++ b/src/ejabberd_ctl.erl
@@ -212,7 +212,7 @@ process(["help" | Mode], Version) ->
     end;
 
 process(["--version", Arg | Args], _) ->
-    Version = 
+    Version =
 	try
 	    list_to_integer(Arg)
 	catch _:_ ->
@@ -321,7 +321,7 @@ call_command([CmdString | Args], Auth, AccessCommands, Version) ->
 	{ArgsFormat, ResultFormat} ->
 	    case (catch format_args(Args, ArgsFormat)) of
 		ArgsFormatted when is_list(ArgsFormatted) ->
-		    Result = ejabberd_commands:execute_command(AccessCommands, 
+		    Result = ejabberd_commands:execute_command(AccessCommands,
 							       Auth, Command,
 							       ArgsFormatted,
 							       Version),
@@ -374,6 +374,12 @@ format_arg2(Arg, Parse)->
 format_result({error, ErrorAtom}, _) ->
     {io_lib:format("Error: ~p", [ErrorAtom]), make_status(error)};
 
+%% An error should always be allowed to return extended error to help with API.
+%% Extended error is of the form:
+%%  {error, type :: atom(), code :: int(), Desc :: string()}
+format_result({error, ErrorAtom, Code, _Msg}, _) ->
+    {io_lib:format("Error: ~p", [ErrorAtom]), make_status(Code)};
+
 format_result(Atom, {_Name, atom}) ->
     io_lib:format("~p", [Atom]);
 
@@ -433,6 +439,8 @@ format_result(404, {_Name, _}) ->
 
 make_status(ok) -> ?STATUS_SUCCESS;
 make_status(true) -> ?STATUS_SUCCESS;
+make_status(Code) when is_integer(Code), Code > 255 -> ?STATUS_ERROR;
+make_status(Code) when is_integer(Code), Code > 0 -> Code;
 make_status(_Error) -> ?STATUS_ERROR.
 
 get_list_commands(Version) ->
diff --git a/src/ejabberd_oauth.erl b/src/ejabberd_oauth.erl
index c45a69d17..e4396260e 100644
--- a/src/ejabberd_oauth.erl
+++ b/src/ejabberd_oauth.erl
@@ -302,12 +302,17 @@ check_token(User, Server, ScopeList, Token) ->
                      expire = Expire} ->
             {MegaSecs, Secs, _} = os:timestamp(),
             TS = 1000000 * MegaSecs + Secs,
-            TokenScopeSet = oauth2_priv_set:new(TokenScope),
-            lists:any(fun(Scope) ->
-                oauth2_priv_set:is_member(Scope, TokenScopeSet) end,
-                ScopeList) andalso Expire > TS;
+            if
+                Expire > TS ->
+                    TokenScopeSet = oauth2_priv_set:new(TokenScope),
+                    lists:any(fun(Scope) ->
+                                      oauth2_priv_set:is_member(Scope, TokenScopeSet) end,
+                              ScopeList);
+                true ->
+                    {false, expired}
+            end;
         _ ->
-            false
+            {false, not_found}
     end.
 
 check_token(ScopeList, Token) ->
@@ -318,15 +323,20 @@ check_token(ScopeList, Token) ->
                      expire = Expire} ->
             {MegaSecs, Secs, _} = os:timestamp(),
             TS = 1000000 * MegaSecs + Secs,
-            TokenScopeSet = oauth2_priv_set:new(TokenScope),
-            case lists:any(fun(Scope) ->
-                oauth2_priv_set:is_member(Scope, TokenScopeSet) end,
-                ScopeList) andalso Expire > TS of
-                true -> {ok, user, US};
-                false -> false
+            if
+                Expire > TS ->
+                    TokenScopeSet = oauth2_priv_set:new(TokenScope),
+                    case lists:any(fun(Scope) ->
+                                           oauth2_priv_set:is_member(Scope, TokenScopeSet) end,
+                                   ScopeList) of
+                        true -> {ok, user, US};
+                        false -> {false, no_matching_scope}
+                    end;
+                true ->
+                    {false, expired}
             end;
         _ ->
-            false
+            {false, not_found}
     end.
 
 
diff --git a/src/ejabberd_s2s.erl b/src/ejabberd_s2s.erl
index 19de64adb..2a17c75cb 100644
--- a/src/ejabberd_s2s.erl
+++ b/src/ejabberd_s2s.erl
@@ -473,28 +473,34 @@ send_element(Pid, El) ->
 %%% ejabberd commands
 
 get_commands_spec() ->
-    [#ejabberd_commands{name = incoming_s2s_number,
-			tags = [stats, s2s],
-			desc =
-			    "Number of incoming s2s connections on "
-			    "the node",
-                        policy = admin,
-			module = ?MODULE, function = incoming_s2s_number,
-			args = [], result = {s2s_incoming, integer}},
-     #ejabberd_commands{name = outgoing_s2s_number,
-			tags = [stats, s2s],
-			desc =
-			    "Number of outgoing s2s connections on "
-			    "the node",
-                        policy = admin,
-			module = ?MODULE, function = outgoing_s2s_number,
-			args = [], result = {s2s_outgoing, integer}}].
-
+    [#ejabberd_commands{
+        name = incoming_s2s_number,
+        tags = [stats, s2s],
+        desc = "Number of incoming s2s connections on the node",
+        policy = admin,
+        module = ?MODULE, function = incoming_s2s_number,
+        args = [], result = {s2s_incoming, integer}},
+     #ejabberd_commands{
+        name = outgoing_s2s_number,
+        tags = [stats, s2s],
+        desc = "Number of outgoing s2s connections on the node",
+        policy = admin,
+        module = ?MODULE, function = outgoing_s2s_number,
+        args = [], result = {s2s_outgoing, integer}}].
+
+%% TODO Move those stats commands to ejabberd stats command ?
 incoming_s2s_number() ->
-    length(supervisor:which_children(ejabberd_s2s_in_sup)).
+    supervisor_count(ejabberd_s2s_in_sup).
 
 outgoing_s2s_number() ->
-    length(supervisor:which_children(ejabberd_s2s_out_sup)).
+    supervisor_count(ejabberd_s2s_out_sup).
+
+supervisor_count(Supervisor) ->
+    case catch supervisor:which_children(Supervisor) of
+        {'EXIT', _} -> 0;
+        Result ->
+            length(Result)
+    end.
 
 %%%----------------------------------------------------------------------
 %%% Update Mnesia tables
diff --git a/src/mod_admin_extra.erl b/src/mod_admin_extra.erl
index 2ad1cc28e..c23fd7818 100644
--- a/src/mod_admin_extra.erl
+++ b/src/mod_admin_extra.erl
@@ -535,7 +535,7 @@ get_commands_spec() ->
                         policy = user,
 			module = mod_offline, function = count_offline_messages,
 			args = [],
-			result = {res, integer}},
+			result = {value, integer}},
      #ejabberd_commands{name = send_message, tags = [stanza],
 			desc = "Send a message to a local or remote bare of full JID",
 			module = ?MODULE, function = send_message,
diff --git a/src/mod_http_api.erl b/src/mod_http_api.erl
index ba3a14cf8..73e6f7e4e 100644
--- a/src/mod_http_api.erl
+++ b/src/mod_http_api.erl
@@ -162,14 +162,15 @@ check_permissions2(#request{auth = HTTPAuth, headers = Headers}, Call, _, ScopeL
                 case oauth_check_token(ScopeList, Token) of
                     {ok, user, {User, Server}} ->
                         {ok, {User, Server, {oauth, Token}, Admin}};
-                    false ->
-                        false
+                    {false, Reason} ->
+                        {false, Reason}
                 end;
             _ ->
                 false
         end,
     case Auth of
         {ok, A} -> {allowed, Call, A};
+        {false, no_matching_scope} -> outofscope_response();
         _ -> unauthorized_response()
     end;
 check_permissions2(_Request, Call, open, _Scope) ->
@@ -189,7 +190,7 @@ check_permissions2(#request{ip={IP, _Port}}, Call, _Policy, _Scope) ->
         Commands when is_list(Commands) ->
             case lists:member(Call, Commands) of
                 true -> {allowed, Call, admin};
-                _ -> unauthorized_response()
+                _ -> outofscope_response()
             end;
         _E ->
             {allowed, Call, noauth}
@@ -212,28 +213,24 @@ process(_, #request{method = 'POST', data = <<>>}) ->
 process([Call], #request{method = 'POST', data = Data, ip = {IP, _} = IPPort} = Req) ->
     Version = get_api_version(Req),
     try
-        Args = case jiffy:decode(Data) of
-            List when is_list(List) -> List;
-            {List} when is_list(List) -> List;
-            Other -> [Other]
-        end,
+        Args = extract_args(Data),
         log(Call, Args, IPPort),
         case check_permissions(Req, Call) of
             {allowed, Cmd, Auth} ->
-                case handle(Cmd, Auth, Args, Version, IP) of
-                    {Code, Result} ->
-                        json_response(Code, jiffy:encode(Result));
-                    {HTMLCode, JSONErrorCode, Message} ->
-                        json_error(HTMLCode, JSONErrorCode, Message)
-                    end;
+                Result = handle(Cmd, Auth, Args, Version, IP),
+                json_format(Result);
             %% Warning: check_permission direcly formats 401 reply if not authorized
             ErrorResponse ->
                 ErrorResponse
         end
-    catch _:{error,{_,invalid_json}} = _Err ->
-	    ?DEBUG("Bad Request: ~p", [_Err]),
-	    badrequest_response(<<"Invalid JSON input">>);
-	  _:_Error ->
+    catch
+        %% TODO We need to refactor to remove redundant error return formatting
+        throw:{error, unknown_command} ->
+            {404, 40, <<"Command not found.">>};
+        _:{error,{_,invalid_json}} = _Err ->
+            ?DEBUG("Bad Request: ~p", [_Err]),
+            badrequest_response(<<"Invalid JSON input">>);
+          _:_Error ->
             ?DEBUG("Bad Request: ~p ~p", [_Error, erlang:get_stacktrace()]),
             badrequest_response()
     end;
@@ -247,13 +244,18 @@ process([Call], #request{method = 'GET', q = Data, ip = IP} = Req) ->
         log(Call, Args, IP),
         case check_permissions(Req, Call) of
             {allowed, Cmd, Auth} ->
-                {Code, Result} = handle(Cmd, Auth, Args, Version, IP),
-                json_response(Code, jiffy:encode(Result));
+                Result = handle(Cmd, Auth, Args, Version, IP),
+                json_format(Result);
             %% Warning: check_permission direcly formats 401 reply if not authorized
             ErrorResponse ->
                 ErrorResponse
         end
-    catch _:_Error ->
+    catch
+        %% TODO We need to refactor to remove redundant error return formatting
+        throw:{error, unknown_command} ->
+            json_format({404, 44, <<"Command not found.">>});
+        _:_Error ->
+
         ?DEBUG("Bad Request: ~p ~p", [_Error, erlang:get_stacktrace()]),
         badrequest_response()
     end;
@@ -261,7 +263,16 @@ process([], #request{method = 'OPTIONS', data = <<>>}) ->
     {200, ?OPTIONS_HEADER, []};
 process(_Path, Request) ->
     ?DEBUG("Bad Request: no handler ~p", [Request]),
-    badrequest_response().
+    json_error(400, 40, <<"Missing command name.">>).
+
+%% Be tolerant to make API more easily usable from command-line pipe.
+extract_args(<<"\n">>) -> [];
+extract_args(Data) ->
+    case jiffy:decode(Data) of
+        List when is_list(List) -> List;
+        {List} when is_list(List) -> List;
+        Other -> [Other]
+    end.
 
 % get API version N from last "vN" element in URL path
 get_api_version(#request{path = Path}) ->
@@ -302,7 +313,7 @@ handle(Call, Auth, Args, Version, IP) when is_atom(Call), is_list(Args) ->
                             [{Key, undefined}|Acc]
                     end, [], ArgsSpec),
 	    try
-		handle2(Call, Auth, match(Args2, Spec), Version, IP)
+          handle2(Call, Auth, match(Args2, Spec), Version, IP)
 	    catch throw:not_found ->
 		    {404, <<"not_found">>};
 		  throw:{not_found, Why} when is_atom(Why) ->
@@ -444,22 +455,24 @@ ejabberd_command(Auth, Cmd, Args, Version, IP) ->
 format_command_result(Cmd, Auth, Result, Version) ->
     {_, ResultFormat} = ejabberd_commands:get_command_format(Cmd, Auth, Version),
     case {ResultFormat, Result} of
-	{{_, rescode}, V} when V == true; V == ok ->
-	    {200, 0};
-	{{_, rescode}, _} ->
-	    {200, 1};
-	{{_, restuple}, {V1, Text1}} when V1 == true; V1 == ok ->
-	    {200, iolist_to_binary(Text1)};
-	{{_, restuple}, {_, Text2}} ->
-	    {500, iolist_to_binary(Text2)};
-	{{_, {list, _}}, _V} ->
-	    {_, L} = format_result(Result, ResultFormat),
-	    {200, L};
-	{{_, {tuple, _}}, _V} ->
-	    {_, T} = format_result(Result, ResultFormat),
-	    {200, T};
-	_ ->
-	    {200, {[format_result(Result, ResultFormat)]}}
+        {{_, rescode}, V} when V == true; V == ok ->
+            {200, 0};
+        {{_, rescode}, _} ->
+            {200, 1};
+        {_, {error, ErrorAtom, Code, Msg}} ->
+            format_error_result(ErrorAtom, Code, Msg);
+        {{_, restuple}, {V, Text}} when V == true; V == ok ->
+            {200, iolist_to_binary(Text)};
+        {{_, restuple}, {ErrorAtom, Msg}} ->
+            format_error_result(ErrorAtom, 0, Msg);
+        {{_, {list, _}}, _V} ->
+            {_, L} = format_result(Result, ResultFormat),
+            {200, L};
+        {{_, {tuple, _}}, _V} ->
+            {_, T} = format_result(Result, ResultFormat),
+            {200, T};
+        _ ->
+            {200, {[format_result(Result, ResultFormat)]}}
     end.
 
 format_result(Atom, {Name, atom}) ->
@@ -497,14 +510,28 @@ format_result(Tuple, {Name, {tuple, Def}}) ->
 format_result(404, {_Name, _}) ->
     "not_found".
 
+
+format_error_result(conflict, Code, Msg) ->
+    {409, Code, iolist_to_binary(Msg)};
+format_error_result(_ErrorAtom, Code, Msg) ->
+    {500, Code, iolist_to_binary(Msg)}.
+
 unauthorized_response() ->
     json_error(401, 10, <<"Oauth Token is invalid or expired.">>).
 
+outofscope_response() ->
+    json_error(401, 11, <<"Token does not grant usage to command required scope.">>).
+
 badrequest_response() ->
     badrequest_response(<<"400 Bad Request">>).
 badrequest_response(Body) ->
     json_response(400, jiffy:encode(Body)).
 
+json_format({Code, Result}) ->
+    json_response(Code, jiffy:encode(Result));
+json_format({HTMLCode, JSONErrorCode, Message}) ->
+    json_error(HTMLCode, JSONErrorCode, Message).
+
 json_response(Code, Body) when is_integer(Code) ->
     {Code, ?HEADER(?CT_JSON), Body}.
 
diff --git a/test/ejabberd_commands_mock_test.exs b/test/ejabberd_commands_mock_test.exs
index 439a3c1d3..9d33d7573 100644
--- a/test/ejabberd_commands_mock_test.exs
+++ b/test/ejabberd_commands_mock_test.exs
@@ -174,7 +174,7 @@ defmodule EjabberdCommandsMockTest do
 		# default version is latest one
 		assert :result3 == :ejabberd_commands.execute_command(command_name, [])
 		# no such command in APIv0
-		assert :unknown_command ==
+		assert {:error, :unknown_command} ==
 			catch_throw :ejabberd_commands.execute_command(command_name, [], 0)
 		assert :result1 == :ejabberd_commands.execute_command(command_name, [], 1)
 		assert :result1 == :ejabberd_commands.execute_command(command_name, [], 2)