From d7ad99f14763ed07f51872a2d6e2c9711bf442da Mon Sep 17 00:00:00 2001 From: Mickael Remond Date: Mon, 25 Jul 2016 11:43:49 +0200 Subject: [PATCH] Initial attempt on access on commands May change and will require more work / test / refactor --- include/ejabberd_commands.hrl | 22 +++- src/ejabberd_admin.erl | 2 +- src/ejabberd_commands.erl | 183 ++++++++++++++------------- src/mod_http_api.erl | 13 +- test/ejabberd_commands_mock_test.exs | 42 +++++- 5 files changed, 166 insertions(+), 96 deletions(-) diff --git a/include/ejabberd_commands.hrl b/include/ejabberd_commands.hrl index bafd93a4d..c5c34b743 100644 --- 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 68aaf9be6..f20aeebf0 100644 --- 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 0f8cd2e0a..7bfabf661 100644 --- 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 bc30ee090..ba3a14cf8 100644 --- 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 487cf6a4b..7c15b58b3 100644 --- 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