Initial attempt on access on commands

May change and will require more work / test / refactor

diff --git a/include/ejabberd_commands.hrl b/include/ejabberd_commands.hrl
index bafd93a4d8b450080117d441553fe069eea90f5c..c5c34b743d20627646892d0261e8c58c78ae1091 100644 (file)
--- a/include/ejabberd_commands.hrl
+++ b/include/ejabberd_commands.hrl
@@ -28,6 +28,23 @@
 
 -type oauth_scope() :: atom().
 
+%% ejabberd_commands OAuth ReST ACL definition:
+%% Two fields exist that are used to control access on a command from ReST API:
+%% 1. Policy
+%% If policy is:
+%%  - restricted: command is not exposed as OAuth Rest API.
+%%  - admin: Command is allowed for user that have Admin Rest command enabled by access rule: commands_admin_access
+%%  - user: Command might be called by any server user.
+%%  - open: Command can be called by anyone.
+%%
+%% Policy is just used to control who can call the command. A specific additional access rules can be performed, as
+%% defined by access option.
+%% Access option can be a list of:
+%% - {Module, accessName, DefaultValue}: Reference and existing module access to limit who can use the command.
+%% - AccessRule name: direct name of the access rule to check in config file.
+%% TODO: Access option could be atom command (not a list). In the case, User performing the command, will be added as first parameter
+%% to command, so that the command can perform additional check.
+
 -record(ejabberd_commands,
        {name                    :: atom(),
          tags = []               :: [atom()] | '_' | '$2',
@@ -38,7 +55,8 @@
          function                :: atom() | '_',
          args = []               :: [aterm()] | '_' | '$1' | '$2',
          policy = restricted     :: open | restricted | admin | user,
-         access_rules = []       :: [atom()],
+        %% access is: [accessRuleName] or [{Module, AccessOption, DefaultAccessRuleName}]
+         access = []             :: [{atom(),atom(),atom()}|atom()],
          result = {res, rescode} :: rterm() | '_' | '$2',
          args_desc = none        :: none | [string()] | '_',
          result_desc = none      :: none | string() | '_',
@@ -55,7 +73,7 @@
                                                 function :: atom(),
                                                 args :: [aterm()],
                                                 policy :: open | restricted | admin | user,
-                                                access_rules :: [atom()],
+                                                access :: [{atom(),atom(),atom()}|atom()],
                                                 result :: rterm()}.
 
 %% @type ejabberd_commands() = #ejabberd_commands{

diff --git a/src/ejabberd_admin.erl b/src/ejabberd_admin.erl
index 68aaf9be6e1e895df1eacda8bf29d7545ff38bda..f20aeebf00bf54d8d1297c008ea07d8e383ed0f6 100644 (file)
--- a/src/ejabberd_admin.erl
+++ b/src/ejabberd_admin.erl
@@ -130,7 +130,7 @@ get_commands_spec() ->
      #ejabberd_commands{name = register, tags = [accounts],
                        desc = "Register a user",
                        policy = admin,
-                       access_rules = [configure],
+      access = [{mod_register, access, configure}],
                        module = ?MODULE, function = register,
                        args = [{user, binary}, {host, binary}, {password, binary}],
                        result = {res, restuple}},

diff --git a/src/ejabberd_commands.erl b/src/ejabberd_commands.erl
index 0f8cd2e0a81932056b239a2fc368afadda55d488..7bfabf661c2de4ff0b33db3c76df239fae375257 100644 (file)
--- a/src/ejabberd_commands.erl
+++ b/src/ejabberd_commands.erl
@@ -319,8 +319,8 @@ list_commands() ->
 list_commands(Version) ->
     Commands = get_commands_definition(Version),
     [{Name, Args, Desc} || #ejabberd_commands{name = Name,
-                                             args = Args,
-                                             desc = Desc} <- Commands].
+                                              args = Args,
+                                              desc = Desc} <- Commands].
 
 
 -spec list_commands_policy(integer()) ->
@@ -331,10 +331,10 @@ list_commands(Version) ->
 list_commands_policy(Version) ->
     Commands = get_commands_definition(Version),
     [{Name, Args, Desc, Policy} ||
-       #ejabberd_commands{name = Name,
-                          args = Args,
-                          desc = Desc,
-                          policy = Policy} <- Commands].
+        #ejabberd_commands{name = Name,
+                           args = Args,
+                           desc = Desc,
+                           policy = Policy} <- Commands].
 
 -spec get_command_format(atom()) -> {[aterm()], rterm()}.
 
@@ -356,14 +356,14 @@ get_command_format(Name, Auth, Version) ->
     Admin = is_admin(Name, Auth, #{}),
     #ejabberd_commands{args = Args,
                       result = Result,
-                      policy = Policy} =
-       get_command_definition(Name, Version),
+                       policy = Policy} =
+        get_command_definition(Name, Version),
     case Policy of
-       user when Admin;
-                 Auth == noauth ->
-           {[{user, binary}, {server, binary} | Args], Result};
-       _ ->
-           {Args, Result}
+        user when Admin;
+                  Auth == noauth ->
+            {[{user, binary}, {server, binary} | Args], Result};
+        _ ->
+            {Args, Result}
     end.
 
 -spec get_command_policy_and_scope(atom()) -> {ok, open|user|admin|restricted, [oauth_scope()]} | {error, command_not_found}.
@@ -394,16 +394,16 @@ get_command_definition(Name) ->
 %% @doc Get the definition record of a command in a given API version.
 get_command_definition(Name, Version) ->
     case lists:reverse(
-          lists:sort(
-            mnesia:dirty_select(
-              ejabberd_commands,
-              ets:fun2ms(
-                fun(#ejabberd_commands{name = N, version = V} = C)
-                      when N == Name, V =< Version ->
-                        {V, C}
-                end)))) of
-       [{_, Command} | _ ] -> Command;
-       _E -> throw(unknown_command)
+           lists:sort(
+             mnesia:dirty_select(
+               ejabberd_commands,
+               ets:fun2ms(
+                 fun(#ejabberd_commands{name = N, version = V} = C)
+                       when N == Name, V =< Version ->
+                         {V, C}
+                 end)))) of
+        [{_, Command} | _ ] -> Command;
+        _E -> throw(unknown_command)
     end.
 
 -spec get_commands_definition(integer()) -> [ejabberd_commands()].
@@ -411,20 +411,20 @@ get_command_definition(Name, Version) ->
 % @doc Returns all commands for a given API version
 get_commands_definition(Version) ->
     L = lists:reverse(
-         lists:sort(
-           mnesia:dirty_select(
-             ejabberd_commands,
-             ets:fun2ms(
-               fun(#ejabberd_commands{name = Name, version = V} = C)
-                     when V =< Version ->
-                       {Name, V, C}
-               end)))),
+          lists:sort(
+            mnesia:dirty_select(
+              ejabberd_commands,
+              ets:fun2ms(
+                fun(#ejabberd_commands{name = Name, version = V} = C)
+                      when V =< Version ->
+                        {Name, V, C}
+                end)))),
     F = fun({_Name, _V, Command}, []) ->
-               [Command];
-          ({Name, _V, _Command}, [#ejabberd_commands{name=Name}|_T] = Acc) ->
-               Acc;
-          ({_Name, _V, Command}, Acc) -> [Command | Acc]
-       end,
+                [Command];
+           ({Name, _V, _Command}, [#ejabberd_commands{name=Name}|_T] = Acc) ->
+                Acc;
+           ({_Name, _V, Command}, Acc) -> [Command | Acc]
+        end,
     lists:foldl(F, [], L).
 
 %% @spec (Name::atom(), Arguments) -> ResultTerm
@@ -433,7 +433,7 @@ get_commands_definition(Version) ->
 %% @doc Execute a command.
 %% Can return the following exceptions:
 %% command_unknown | account_unprivileged | invalid_account_data |
-%% no_auth_provided
+%% no_auth_provided | access_rules_unauthorized
 execute_command(Name, Arguments) ->
     execute_command(Name, Arguments, ?DEFAULT_VERSION).
 
@@ -505,7 +505,7 @@ execute_command(AccessCommands1, Auth1, Name, Arguments, Version, CallerInfo) ->
            end,
     TokenJID = oauth_token_user(Auth1),
     Command = get_command_definition(Name, Version),
-    AccessCommands = get_access_commands(AccessCommands1, Version),
+    AccessCommands = get_all_access_commands(AccessCommands1),
     case check_access_commands(AccessCommands, Auth, Name, Command, Arguments, CallerInfo) of
         ok -> execute_check_policy(Auth, TokenJID, Command, Arguments)
     end.
@@ -530,15 +530,22 @@ execute_check_policy(
   {User, Server, _, _}, JID, #ejabberd_commands{policy = user} = Command, Arguments) ->
     execute_check_access(JID, Command, [User, Server | Arguments]).
 
-execute_check_access(_FromJID, #ejabberd_commands{access_rules = []} = Command, Arguments) ->
+execute_check_access(_FromJID, #ejabberd_commands{access = []} = Command, Arguments) ->
     do_execute_command(Command, Arguments);
-execute_check_access(FromJID, #ejabberd_commands{access_rules = Rules} = Command, Arguments) ->
+execute_check_access(FromJID, #ejabberd_commands{access = AccessRefs} = Command, Arguments) ->
     %% TODO Review: Do we have smarter / better way to check rule on other Host than global ?
-    case acl:any_rules_allowed(global, Rules, FromJID) of
+    Host = global,
+    Rules = lists:map(fun({Mod, AccessName, Default}) ->
+                              gen_mod:get_module_opt(Host, Mod,
+                                                     AccessName, fun(A) -> A end, Default);
+                         (Default) ->
+                              Default
+                      end, AccessRefs),
+    case acl:any_rules_allowed(Host, Rules, FromJID) of
         true ->
             do_execute_command(Command, Arguments);
         false ->
-            {error, access_rules_unauthorized}
+            throw({error, access_rules_unauthorized})
     end.
 
 do_execute_command(Command, Arguments) ->
@@ -611,31 +618,31 @@ check_access_commands(AccessCommands, Auth, Method, Command1, Arguments, CallerI
                 Command1
         end,
     AccessCommandsAllowed =
-       lists:filter(
-         fun({Access, Commands, ArgumentRestrictions}) ->
-                 case check_access(Command, Access, Auth, CallerInfo) of
-                     true ->
-                         check_access_command(Commands, Command,
-                                              ArgumentRestrictions,
-                                              Method, Arguments);
-                     false ->
-                         false
-                 end;
-             ({Access, Commands}) ->
-                 ArgumentRestrictions = [],
-                 case check_access(Command, Access, Auth, CallerInfo) of
-                     true ->
-                         check_access_command(Commands, Command,
-                                              ArgumentRestrictions,
-                                              Method, Arguments);
-                     false ->
-                         false
-                 end
-         end,
-         AccessCommands),
+        lists:filter(
+          fun({Access, Commands, ArgumentRestrictions}) ->
+                  case check_access(Command, Access, Auth, CallerInfo) of
+                      true ->
+                          check_access_command(Commands, Command,
+                                               ArgumentRestrictions,
+                                               Method, Arguments);
+                      false ->
+                          false
+                  end;
+             ({Access, Commands}) ->
+                  ArgumentRestrictions = [],
+                  case check_access(Command, Access, Auth, CallerInfo) of
+                      true ->
+                          check_access_command(Commands, Command,
+                                               ArgumentRestrictions,
+                                               Method, Arguments);
+                      false ->
+                          false
+                  end
+          end,
+          AccessCommands),
     case AccessCommandsAllowed of
-       [] -> throw({error, account_unprivileged});
-       L when is_list(L) -> ok
+        [] -> throw({error, account_unprivileged});
+        L when is_list(L) -> ok
     end.
 
 -spec check_auth(ejabberd_commands(), noauth) -> noauth_provided;
@@ -699,9 +706,9 @@ check_access2(Access, AccessInfo, Server) ->
 check_access_command(Commands, Command, ArgumentRestrictions,
                     Method, Arguments) ->
     case Commands==all orelse lists:member(Method, Commands) of
-       true -> check_access_arguments(Command, ArgumentRestrictions,
-                                      Arguments);
-       false -> false
+        true -> check_access_arguments(Command, ArgumentRestrictions,
+                                       Arguments);
+        false -> false
     end.
 
 check_access_arguments(Command, ArgumentRestrictions, Arguments) ->
@@ -724,6 +731,10 @@ tag_arguments(ArgsDefs, Args) ->
       Args).
 
 
+%% Get commands for all version
+get_all_access_commands(AccessCommands) ->
+    get_access_commands(AccessCommands, ?DEFAULT_VERSION).
+
 get_access_commands(undefined, Version) ->
     Cmds = get_commands(Version),
     [{?POLICY_ACCESS, Cmds, []}];
@@ -736,7 +747,7 @@ get_commands(Version) ->
     Opts0 = ejabberd_config:get_option(
              commands,
              fun(V) when is_list(V) -> V end,
-             []),
+              []),
     Opts = lists:map(fun(V) when is_tuple(V) -> [V]; (V) -> V end, Opts0),
     CommandsList = list_commands_policy(Version),
     OpenCmds = [N || {N, _, _, open} <- CommandsList],
@@ -746,27 +757,29 @@ get_commands(Version) ->
     Cmds =
         lists:foldl(
           fun([{add_commands, L}], Acc) ->
-                  Cmds = case L of
-                             open -> OpenCmds;
-                             restricted -> RestrictedCmds;
-                             admin -> AdminCmds;
-                             user -> UserCmds;
-                             _ when is_list(L) -> L
-                         end,
+                  Cmds = expand_commands(L, OpenCmds, UserCmds, AdminCmds, RestrictedCmds),
                   lists:usort(Cmds ++ Acc);
              ([{remove_commands, L}], Acc) ->
-                  Cmds = case L of
-                             open -> OpenCmds;
-                             restricted -> RestrictedCmds;
-                             admin -> AdminCmds;
-                             user -> UserCmds;
-                             _ when is_list(L) -> L
-                         end,
+                  Cmds = expand_commands(L, OpenCmds, UserCmds, AdminCmds, RestrictedCmds),
                   Acc -- Cmds;
              (_, Acc) -> Acc
-          end, AdminCmds ++ UserCmds, Opts),
+          end, [], Opts),
     Cmds.
 
+expand_commands(L, OpenCmds, UserCmds, AdminCmds, RestrictedCmds) when is_list(L) ->
+    lists:foldl(fun(El, Acc) ->
+                        expand_commands(El, OpenCmds, UserCmds, AdminCmds, RestrictedCmds) ++ Acc
+                end, [], L);
+expand_commands(El, OpenCmds, UserCmds, AdminCmds, RestrictedCmds) ->
+    case El of
+        open -> OpenCmds;
+        restricted -> RestrictedCmds;
+        admin -> AdminCmds;
+        user -> UserCmds;
+        _ -> [El]
+    end.
+
+
 oauth_token_user(noauth) ->
     undefined;
 oauth_token_user(admin) ->

diff --git a/src/mod_http_api.erl b/src/mod_http_api.erl
index bc30ee0908466e80188f8fc529fb34d9cc769d08..ba3a14cf8c09714f91b418b25c7d03a47898d95f 100644 (file)
--- a/src/mod_http_api.erl
+++ b/src/mod_http_api.erl
@@ -136,8 +136,7 @@ check_permissions(Request, Command) ->
             {ok, CommandPolicy, Scope} = ejabberd_commands:get_command_policy_and_scope(Call),
             check_permissions2(Request, Call, CommandPolicy, Scope);
         _ ->
-            %% TODO Should this be a 404 or 400 instead of 401 ?
-            unauthorized_response()
+            json_error(404, 40, <<"Endpoint not found.">>)
     end.
 
 check_permissions2(#request{auth = HTTPAuth, headers = Headers}, Call, _, ScopeList)
@@ -269,10 +268,10 @@ get_api_version(#request{path = Path}) ->
     get_api_version(lists:reverse(Path));
 get_api_version([<<"v", String/binary>> | Tail]) ->
     case catch jlib:binary_to_integer(String) of
-       N when is_integer(N) ->
-           N;
-       _ ->
-           get_api_version(Tail)
+        N when is_integer(N) ->
+            N;
+        _ ->
+            get_api_version(Tail)
     end;
 get_api_version([_Head | Tail]) ->
     get_api_version(Tail);
@@ -318,6 +317,8 @@ handle(Call, Auth, Args, Version, IP) when is_atom(Call), is_list(Args) ->
                    {401, iolist_to_binary(Msg)};
       throw:{error, account_unprivileged} ->
         {403, 31, <<"Command need to be run with admin priviledge.">>};
+      throw:{error, access_rules_unauthorized} ->
+        {403, 32, <<"AccessRules: Account associated to token does not have the right to perform the operation.">>};
                  throw:{invalid_parameter, Msg} ->
                    {400, iolist_to_binary(Msg)};
                  throw:{error, Why} when is_atom(Why) ->

diff --git a/test/ejabberd_commands_mock_test.exs b/test/ejabberd_commands_mock_test.exs
index 487cf6a4b9b05d62bbcc618922d0bc2653d0c039..7c15b58b3462c492eac36905ef2c9da550441da4 100644 (file)
--- a/test/ejabberd_commands_mock_test.exs
+++ b/test/ejabberd_commands_mock_test.exs
@@ -18,6 +18,8 @@
 #
 # ----------------------------------------------------------------------
 
+## TODO Fix next test error: add admin user ACL
+
 defmodule EjabberdCommandsMockTest do
        use ExUnit.Case, async: false
 
@@ -44,6 +46,9 @@ defmodule EjabberdCommandsMockTest do
                        _ -> :ok
                end
                :mnesia.start
+    :ok = :jid.start
+    :ok = :ejabberd_config.start(["domain1", "domain2"], [])
+    :ok = :acl.start
                EjabberdOauthMock.init
                :ok
        end
@@ -313,7 +318,6 @@ defmodule EjabberdCommandsMockTest do
        end
 
 
-
        test "API command with admin policy" do
                mock_commands_config
 
@@ -393,6 +397,40 @@ defmodule EjabberdCommandsMockTest do
                assert :meck.validate @module
        end
 
+  test "Commands can perform extra check on access" do
+    mock_commands_config
+
+               command_name = :test
+               function = :test_command
+               command = ejabberd_commands(name: command_name,
+                       args: [{:user, :binary}, {:host, :binary}],
+      access: [:basic_rule_1],
+                       module: @module,
+                       function: function,
+                       policy: :open)
+               :meck.expect(@module, function,
+                       fn(user, domain) when is_binary(user) and is_binary(domain) ->
+                               {user, domain}
+                       end)
+               assert :ok == :ejabberd_commands.register_commands [command]
+
+    :acl.add(:global, :basic_acl_1, {:user, @user})
+    :acl.add_access(:global, :basic_rule_1, [{:allow, [{:acl, :basic_acl_1}]}])
+
+    assert {@user, @domain} ==
+                       :ejabberd_commands.execute_command(:undefined,
+        {@user, @domain,
+                                @userpass, false},
+                               command_name,
+                               [@user, @domain])
+    assert {@user, @domain} ==
+                       :ejabberd_commands.execute_command(:undefined,
+        {@admin, @domain,
+                                @adminpass, false},
+                               command_name,
+                               [@user, @domain])
+
+  end
 
        ##########################################################
        # Utils
@@ -412,7 +450,7 @@ defmodule EjabberdCommandsMockTest do
                        end)
                :meck.expect(:ejabberd_config, :get_myhosts,
                        fn() -> [@domain]       end)
-               :meck.new :acl
+               :meck.new :acl #, [:passthrough]
                :meck.expect(:acl, :access_matches,
                        fn(:commands_admin_access, info, _scope) ->
         case info do