Merge pull request #14949 from rabbitmq/mergify/bp/v4.2.x/pr-14931

HTTP API: when virtual host is known, limit permission filtering to just that specific virtual host (backport #14931)

Author: michaelklishin

deps/rabbitmq_management/src/rabbit_mgmt_util.erl

@@ -313,7 +313,6 @@ get_sort_reverse(ReqData) ->
         V -> list_to_atom(V)
     end.
 
-
 -spec is_pagination_requested(#pagination{} | undefined) -> boolean().
 is_pagination_requested(undefined) ->
     false;
@@ -981,9 +980,28 @@ all_or_one_vhost(ReqData, Fun) ->
         VHost     -> Fun(VHost)
     end.
 
-filter_vhost(List, ReqData, Context) ->
-    User = #user{tags = Tags} = Context#context.user,
-    Fn   = case rabbit_web_dispatch_access_control:is_admin(Tags) of
+filter_vhost(List, ReqData, Context = #context{user = #user{tags = Tags}}) ->
+    IsAdmin = rabbit_web_dispatch_access_control:is_admin(Tags),
+    filter_vhost({vhost, vhost(ReqData)}, IsAdmin, List, ReqData, Context).
+
+filter_vhost({vhost, not_found}, IsAdmin, List, ReqData, Context) ->
+    filter_none_or_not_found_vhost(IsAdmin, List, ReqData, Context);
+filter_vhost({vhost, none}, IsAdmin, List, ReqData, Context) ->
+    filter_none_or_not_found_vhost(IsAdmin, List, ReqData, Context);
+filter_vhost({vhost, _VHost}, _IsAdmin=true, List, _ReqData, _Context) ->
+    [I || I <- List, lists:member(pget(vhost, I), rabbit_vhost:list())];
+filter_vhost({vhost, VHost}, _IsAdmin=false, List, ReqData, #context{user = User}) ->
+    AuthzData = get_authz_data(ReqData),
+    case catch rabbit_access_control:check_vhost_access(User, VHost, AuthzData, #{}) of
+        ok ->
+            [I || I <- List, pget(vhost, I) =:= VHost];
+        NotOK ->
+            log_access_control_result(NotOK),
+            []
+    end.
+
+filter_none_or_not_found_vhost(IsAdmin, List, ReqData, #context{user = User}) ->
+    Fn   = case IsAdmin of
                true  -> fun list_visible_vhosts_names/2;
                false -> fun list_login_vhosts_names/2
            end,
@@ -1090,6 +1108,8 @@ list_login_vhosts(User, AuthzData) ->
           end].
 
 % rabbitmq/rabbitmq-auth-backend-http#100
+log_access_control_result({'EXIT', #amqp_error{name = Name, explanation = Msg}}) ->
+    ?LOG_DEBUG("rabbit_access_control:check_vhost_access result: '~tp', ~ts", [Name, Msg]);
 log_access_control_result(NotOK) ->
     ?LOG_DEBUG("rabbit_access_control:check_vhost_access result: ~tp", [NotOK]).
 

deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl

@@ -1709,33 +1709,47 @@ permissions_vhost_test(Config) ->
                                         {tags, <<"administrator">>}], {group, '2xx'}),
     http_put(Config, "/users/myuser", [{password, <<"myuser">>},
                                        {tags, <<"management">>}], {group, '2xx'}),
+    http_put(Config, "/users/mymonitor", [{password, <<"mymonitor">>},
+                                          {tags, [<<"monitor">>, <<"management">>]}], {group, '2xx'}),
     http_put(Config, "/vhosts/myvhost1", none, {group, '2xx'}),
     http_put(Config, "/vhosts/myvhost2", none, {group, '2xx'}),
     http_put(Config, "/permissions/myvhost1/myuser", PermArgs, {group, '2xx'}),
+    http_put(Config, "/permissions/myvhost1/mymonitor", PermArgs, {group, '2xx'}),
     http_put(Config, "/permissions/myvhost1/guest", PermArgs, {group, '2xx'}),
     http_put(Config, "/permissions/myvhost2/guest", PermArgs, {group, '2xx'}),
     assert_list([#{name => <<"/">>},
                  #{name => <<"myvhost1">>},
                  #{name => <<"myvhost2">>}], http_get(Config, "/vhosts", ?OK)),
     assert_list([#{name => <<"myvhost1">>}],
                 http_get(Config, "/vhosts", "myuser", "myuser", ?OK)),
+    assert_list([#{name => <<"myvhost1">>}],
+                http_get(Config, "/vhosts", "mymonitor", "mymonitor", ?OK)),
     http_put(Config, "/queues/myvhost1/myqueue", QArgs, {group, '2xx'}),
     http_put(Config, "/queues/myvhost2/myqueue", QArgs, {group, '2xx'}),
+    CheckResult = fun (Path, Result) ->
+                          case maps:get(vhost, Result) of
+                              <<"myvhost2">> ->
+                                  throw({got_result_from_vhost2_in, Path, Result});
+                              _ ->
+                                  ok
+                          end
+                  end,
     Test1 =
         fun(Path) ->
-                Results = http_get(Config, Path, "myuser", "myuser", ?OK),
-                [case maps:get(vhost, Result) of
-                     <<"myvhost2">> ->
-                         throw({got_result_from_vhost2_in, Path, Result});
-                     _ ->
-                         ok
-                 end || Result <- Results]
+                Results0 = http_get(Config, Path, "myuser", "myuser", ?OK),
+                [CheckResult(Path, Result0) || Result0 <- Results0],
+                Results1 = http_get(Config, Path, "mymonitor", "mymonitor", ?OK),
+                [CheckResult(Path, Result1) || Result1 <- Results1]
         end,
     Test2 =
         fun(Path1, Path2) ->
                 http_get(Config, Path1 ++ "/myvhost1/" ++ Path2, "myuser", "myuser",
                          ?OK),
                 http_get(Config, Path1 ++ "/myvhost2/" ++ Path2, "myuser", "myuser",
+                         ?NOT_AUTHORISED),
+                http_get(Config, Path1 ++ "/myvhost1/" ++ Path2, "mymonitor", "mymonitor",
+                         ?OK),
+                http_get(Config, Path1 ++ "/myvhost2/" ++ Path2, "mymonitor", "mymonitor",
                          ?NOT_AUTHORISED)
         end,
     Test3 =

deps/rabbitmq_web_dispatch/src/rabbit_web_dispatch_access_control.erl

@@ -20,7 +20,6 @@
          is_authorized_vhost_visible_for_monitoring/3,
          is_authorized_global_parameters/3]).
 
--export([list_visible_vhosts/1, list_visible_vhosts_names/1, list_login_vhosts/2]).
 -export([id/2]).
 -export([not_authorised/3, halt_response/5]).
 
@@ -255,23 +254,34 @@ user_matches_vhost(ReqData, User) ->
         not_found -> true;
         none      -> true;
         V         ->
-            AuthzData = get_authz_data(ReqData),
-            lists:member(V, list_login_vhosts_names(User, AuthzData))
+            check_vhost_access(ReqData, User, V)
     end.
 
-user_matches_vhost_visible(ReqData, User) ->
+user_matches_vhost_visible(ReqData, User = #user{tags = Tags}) ->
     case vhost(ReqData) of
         not_found -> true;
         none      -> true;
         V         ->
-            AuthzData = get_authz_data(ReqData),
-            lists:member(V, list_visible_vhosts_names(User, AuthzData))
+            case is_monitor(Tags) of
+                true  ->
+                    rabbit_vhost:exists(V);
+                false ->
+                    check_vhost_access(ReqData, User, V)
+            end
     end.
 
 get_authz_data(ReqData) ->
     {PeerAddress, _PeerPort} = cowboy_req:peer(ReqData),
     {ip, PeerAddress}.
 
+check_vhost_access(ReqData, User, VHost) ->
+    AuthzData = get_authz_data(ReqData),
+    case catch rabbit_access_control:check_vhost_access(User, VHost, AuthzData, #{}) of
+        ok -> true;
+        NotOK ->
+            log_access_control_result(NotOK),
+            false
+    end.
 
 not_authorised(Reason, ReqData, Context) ->
     %% TODO: consider changing to 403 in 4.0
@@ -328,40 +338,6 @@ id0(Key, ReqData) ->
         Id        -> Id
     end.
 
-
-list_visible_vhosts_names(User) ->
-    list_visible_vhosts(User, undefined).
-
-list_visible_vhosts_names(User, AuthzData) ->
-    list_visible_vhosts(User, AuthzData).
-
-list_visible_vhosts(User) ->
-    list_visible_vhosts(User, undefined).
-
-list_visible_vhosts(User = #user{tags = Tags}, AuthzData) ->
-    case is_monitor(Tags) of
-        true  -> rabbit_vhost:list_names();
-        false -> list_login_vhosts_names(User, AuthzData)
-    end.
-
-list_login_vhosts_names(User, AuthzData) ->
-    [V || V <- rabbit_vhost:list_names(),
-          case catch rabbit_access_control:check_vhost_access(User, V, AuthzData, #{}) of
-              ok -> true;
-              NotOK ->
-                  log_access_control_result(NotOK),
-                  false
-          end].
-
-list_login_vhosts(User, AuthzData) ->
-    [V || V <- rabbit_vhost:all(),
-          case catch rabbit_access_control:check_vhost_access(User, vhost:get_name(V), AuthzData, #{}) of
-              ok -> true;
-              NotOK ->
-                  log_access_control_result(NotOK),
-                  false
-          end].
-
 % rabbitmq/rabbitmq-auth-backend-http#100
 log_access_control_result(NotOK) ->
     ?LOG_DEBUG("rabbit_access_control:check_vhost_access result: ~tp", [NotOK]).