Merge pull request #1223 from processone/expand_api

More API fixes and improvements

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">>}

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",
+	    Msg = io_lib:format("User ~s@~s already registered", [User, Host]),
-				   [User, Host, node()]),
+	    {error, conflict, 10090, Msg};
-	    {exists, String};
 	{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) ->

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) ->

src/ejabberd_ctl.erl

@@ -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) ->

src/ejabberd_oauth.erl

@@ -302,12 +302,17 @@ check_token(User, Server, ScopeList, Token) ->
                      expire = Expire} ->
             {MegaSecs, Secs, _} = os:timestamp(),
             TS = 1000000 * MegaSecs + Secs,
+            if
+                Expire > TS ->
                     TokenScopeSet = oauth2_priv_set:new(TokenScope),
                     lists:any(fun(Scope) ->
                                       oauth2_priv_set:is_member(Scope, TokenScopeSet) end,
-                ScopeList) andalso Expire > TS;
+                              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,
+            if
+                Expire > TS ->
                     TokenScopeSet = oauth2_priv_set:new(TokenScope),
                     case lists:any(fun(Scope) ->
                                            oauth2_priv_set:is_member(Scope, TokenScopeSet) end,
-                ScopeList) andalso Expire > TS of
+                                   ScopeList) of
                         true -> {ok, user, US};
-                false -> false
+                        false -> {false, no_matching_scope}
+                    end;
+                true ->
+                    {false, expired}
             end;
         _ ->
-            false
+            {false, not_found}
     end.
 
 

src/ejabberd_s2s.erl

@@ -473,28 +473,34 @@ send_element(Pid, El) ->
 %%% ejabberd commands
 
 get_commands_spec() ->
-    [#ejabberd_commands{name = incoming_s2s_number,
+    [#ejabberd_commands{
+        name = incoming_s2s_number,
         tags = [stats, s2s],
-			desc =
+        desc = "Number of incoming s2s connections on the node",
-			    "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,
+     #ejabberd_commands{
+        name = outgoing_s2s_number,
         tags = [stats, s2s],
-			desc =
+        desc = "Number of outgoing s2s connections on the node",
-			    "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

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,

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, Reason} ->
-                        false
+                        {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,25 +213,21 @@ 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
+        Args = extract_args(Data),
-            List when is_list(List) -> List;
-            {List} when is_list(List) -> List;
-            Other -> [Other]
-        end,
         log(Call, Args, IPPort),
         case check_permissions(Req, Call) of
             {allowed, Cmd, Auth} ->
-                case handle(Cmd, Auth, Args, Version, IP) of
+                Result = handle(Cmd, Auth, Args, Version, IP),
-                    {Code, Result} ->
+                json_format(Result);
-                        json_response(Code, jiffy:encode(Result));
-                    {HTMLCode, JSONErrorCode, Message} ->
-                        json_error(HTMLCode, JSONErrorCode, Message)
-                    end;
             %% Warning: check_permission direcly formats 401 reply if not authorized
             ErrorResponse ->
                 ErrorResponse
         end
-    catch _:{error,{_,invalid_json}} = _Err ->
+    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 ->
@@ -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),
+                Result = handle(Cmd, Auth, Args, Version, IP),
-                json_response(Code, jiffy:encode(Result));
+                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}) ->
@@ -448,10 +459,12 @@ format_command_result(Cmd, Auth, Result, Version) ->
             {200, 0};
         {{_, rescode}, _} ->
             {200, 1};
-	{{_, restuple}, {V1, Text1}} when V1 == true; V1 == ok ->
+        {_, {error, ErrorAtom, Code, Msg}} ->
-	    {200, iolist_to_binary(Text1)};
+            format_error_result(ErrorAtom, Code, Msg);
-	{{_, restuple}, {_, Text2}} ->
+        {{_, restuple}, {V, Text}} when V == true; V == ok ->
-	    {500, iolist_to_binary(Text2)};
+            {200, iolist_to_binary(Text)};
+        {{_, restuple}, {ErrorAtom, Msg}} ->
+            format_error_result(ErrorAtom, 0, Msg);
         {{_, {list, _}}, _V} ->
             {_, L} = format_result(Result, ResultFormat),
             {200, L};
@@ -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}.
 

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)