Merge pull request #2676 from rabbitmq/rabbitmq-server-2667

acogoluegnes · web-flow · commit e3bbdfe6dfcd · 2020-12-15T14:14:23.000+01:00
Definition export: change user tags to a JSON array
diff --git a/deps/rabbit/src/rabbit_auth_backend_internal.erl b/deps/rabbit/src/rabbit_auth_backend_internal.erl
@@ -39,6 +39,8 @@
 %% for testing
 -export([hashing_module_for_user/1, expand_topic_permission/2]).
 
+-import(rabbit_data_coercion, [to_atom/1, to_list/1, to_binary/1]).
+
 %%----------------------------------------------------------------------------
 
 -type regexp() :: binary().
@@ -662,9 +664,8 @@ put_user(User, Version, ActingUser) ->
                                   true  -> [administrator];
                                   false -> []
                               end;
-                          {TagsS, _} ->
-                              [list_to_atom(string:strip(T)) ||
-                                  T <- string:tokens(binary_to_list(TagsS), ",")]
+                          {TagsVal, _} ->
+                              tag_list_from(TagsVal)
                       end,
 
     %% pre-configured, only applies to newly created users
@@ -813,6 +814,11 @@ clear_user_limits(Username, LimitType, ActingUser) ->
                           end),
     notify_limit_clear(Username, ActingUser).
 
+tag_list_from(Tags) when is_list(Tags) ->
+    [to_atom(string:strip(to_list(T))) || T <- Tags];
+tag_list_from(Tags) when is_binary(Tags) ->
+    [to_atom(string:strip(T)) || T <- string:tokens(to_list(Tags), ",")].
+
 flatten_errors(L) ->
     case [{F, A} || I <- lists:flatten([L]), {error, F, A} <- [I]] of
         [] -> ok;
diff --git a/deps/rabbit/src/rabbit_definitions.erl b/deps/rabbit/src/rabbit_definitions.erl
@@ -26,6 +26,7 @@
 -export([decode/1, decode/2, args/1]).
 
 -import(rabbit_misc, [pget/2]).
+-import(rabbit_data_coercion, [to_binary/1]).
 
 %%
 %% API
@@ -765,4 +766,4 @@ topic_permission_definition(P0) ->
     maps:from_list(P).
 
 tags_as_binaries(Tags) ->
-    list_to_binary(string:join([atom_to_list(T) || T <- Tags], ",")).
+    [to_binary(T) || T <- Tags].
diff --git a/deps/rabbit/test/definition_import_SUITE.erl b/deps/rabbit/test/definition_import_SUITE.erl
@@ -40,7 +40,8 @@ groups() ->
                                import_case11,
                                import_case12,
                                import_case13,
-                               import_case14
+                               import_case14,
+                               import_case15
                               ]},
         {boot_time_import, [], [
             import_on_a_booting_node
@@ -143,6 +144,8 @@ import_case13(Config) ->
     end.
 
 import_case14(Config) -> import_file_case(Config, "case14").
+%% contains a user with tags as a list
+import_case15(Config) -> import_file_case(Config, "case15").
 
 export_import_round_trip_case1(Config) ->
     %% case 6 has runtime parameters that do not depend on any plugins
diff --git a/deps/rabbit/test/definition_import_SUITE_data/case15.json b/deps/rabbit/test/definition_import_SUITE_data/case15.json
@@ -0,0 +1,53 @@
+{
+  "bindings": [],
+  "exchanges": [],
+  "global_parameters": [
+    {
+      "name": "cluster_name",
+      "value": "rabbit@rabbitmq"
+    }
+  ],
+  "parameters": [],
+  "permissions": [
+    {
+      "configure": ".*",
+      "read": ".*",
+      "user": "guest",
+      "vhost": "/",
+      "write": ".*"
+    }
+  ],
+  "policies": [],
+  "queues": [],
+  "rabbit_version": "3.9.0",
+  "rabbitmq_version": "3.9.0",
+  "topic_permissions": [],
+  "users": [
+    {
+      "hashing_algorithm": "rabbit_password_hashing_sha256",
+      "name": "guest",
+      "password_hash": "BYipq3D94qlyiZVOAAYLVdN1v8H0BOrOpM9SH6ma5aB354FA",
+      "tags": "administrator"
+    },
+    {
+      "hashing_algorithm": "rabbit_password_hashing_sha256",
+      "name": "tagged-user",
+      "password_hash": "t/Ah03PwU/ol8vkarb+oEYpylRSBWXFAau3eXz3lrjGxtGEK",
+      "tags": [
+        "monitoring",
+        "policymaker",
+        "impersonator"
+      ]
+    }
+  ],
+  "vhosts": [
+    {
+      "limits": [],
+      "metadata": {
+        "description": "Default virtual host",
+        "tags": []
+      },
+      "name": "/"
+    }
+  ]
+}
diff --git a/deps/rabbitmq_management/priv/www/js/global.js b/deps/rabbitmq_management/priv/www/js/global.js
@@ -630,7 +630,7 @@ var user;
 function setup_global_vars() {
     var overview = JSON.parse(sync_get('/overview'));
     rates_mode = overview.rates_mode;
-    user_tags = expand_user_tags(user.tags.split(","));
+    user_tags = expand_user_tags(user.tags);
     user_administrator = jQuery.inArray("administrator", user_tags) != -1;
     is_user_policymaker = jQuery.inArray("policymaker", user_tags) != -1;
     user_monitor = jQuery.inArray("monitoring", user_tags) != -1;
diff --git a/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl b/deps/rabbitmq_management/test/rabbit_mgmt_http_SUITE.erl
@@ -491,43 +491,43 @@ vhosts_trace_test(Config) ->
     passed.
 
 users_test(Config) ->
-    assert_item(#{name => <<"guest">>, tags => <<"administrator">>},
+    assert_item(#{name => <<"guest">>, tags => [<<"administrator">>]},
                 http_get(Config, "/whoami")),
     rabbit_ct_broker_helpers:rpc(Config, 0, application, set_env,
                                  [rabbitmq_management, login_session_timeout, 100]),
     assert_item(#{name => <<"guest">>,
-                  tags => <<"administrator">>,
+                  tags => [<<"administrator">>],
                   login_session_timeout => 100},
                 http_get(Config, "/whoami")),
     http_get(Config, "/users/myuser", ?NOT_FOUND),
     http_put_raw(Config, "/users/myuser", "Something not JSON", ?BAD_REQUEST),
     http_put(Config, "/users/myuser", [{flim, <<"flam">>}], ?BAD_REQUEST),
-    http_put(Config, "/users/myuser", [{tags,     <<"management">>},
+    http_put(Config, "/users/myuser", [{tags,     [<<"management">>]},
                                        {password, <<"myuser">>}],
              {group, '2xx'}),
     http_put(Config, "/users/myuser", [{password_hash, <<"not_hash">>}], ?BAD_REQUEST),
     http_put(Config, "/users/myuser", [{password_hash,
                                         <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>},
                                        {tags, <<"management">>}], {group, '2xx'}),
-    assert_item(#{name => <<"myuser">>, tags => <<"management">>,
+    assert_item(#{name => <<"myuser">>, tags => [<<"management">>],
                   password_hash => <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>,
                   hashing_algorithm => <<"rabbit_password_hashing_sha256">>},
                 http_get(Config, "/users/myuser")),
 
     http_put(Config, "/users/myuser", [{password_hash,
                                         <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>},
                                        {hashing_algorithm, <<"rabbit_password_hashing_md5">>},
-                                       {tags, <<"management">>}], {group, '2xx'}),
-    assert_item(#{name => <<"myuser">>, tags => <<"management">>,
+                                       {tags, [<<"management">>]}], {group, '2xx'}),
+    assert_item(#{name => <<"myuser">>, tags => [<<"management">>],
                   password_hash => <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>,
                   hashing_algorithm => <<"rabbit_password_hashing_md5">>},
                 http_get(Config, "/users/myuser")),
     http_put(Config, "/users/myuser", [{password, <<"password">>},
-                                       {tags, <<"administrator, foo">>}], {group, '2xx'}),
-    assert_item(#{name => <<"myuser">>, tags => <<"administrator,foo">>},
+                                       {tags, [<<"administrator">>, <<"foo">>]}], {group, '2xx'}),
+    assert_item(#{name => <<"myuser">>, tags => [<<"administrator">>, <<"foo">>]},
                 http_get(Config, "/users/myuser")),
-    assert_list(lists:sort([#{name => <<"myuser">>, tags => <<"administrator,foo">>},
-                 #{name => <<"guest">>, tags => <<"administrator">>}]),
+    assert_list(lists:sort([#{name => <<"myuser">>, tags => [<<"administrator">>, <<"foo">>]},
+                 #{name => <<"guest">>, tags => [<<"administrator">>]}]),
                 lists:sort(http_get(Config, "/users"))),
     test_auth(Config, ?OK, [auth_header("myuser", "password")]),
     http_delete(Config, "/users/myuser", {group, '2xx'}),
@@ -536,7 +536,7 @@ users_test(Config) ->
     passed.
 
 without_permissions_users_test(Config) ->
-    assert_item(#{name => <<"guest">>, tags => <<"administrator">>},
+    assert_item(#{name => <<"guest">>, tags => [<<"administrator">>]},
                 http_get(Config, "/whoami")),
     http_put(Config, "/users/myuser", [{password_hash,
                                         <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>},
@@ -546,7 +546,7 @@ without_permissions_users_test(Config) ->
     http_put(Config, "/users/myuserwithoutpermissions", [{password_hash,
                                                           <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>},
                                                          {tags, <<"management">>}], {group, '2xx'}),
-    assert_list([#{name => <<"myuserwithoutpermissions">>, tags => <<"management">>,
+    assert_list([#{name => <<"myuserwithoutpermissions">>, tags => [<<"management">>],
                    hashing_algorithm => <<"rabbit_password_hashing_sha256">>,
                    password_hash => <<"IECV6PZI/Invh0DL187KFpkO5Jc=">>}],
                 http_get(Config, "/users/without-permissions")),
@@ -555,7 +555,7 @@ without_permissions_users_test(Config) ->
     passed.
 
 users_bulk_delete_test(Config) ->
-    assert_item(#{name => <<"guest">>, tags => <<"administrator">>},
+    assert_item(#{name => <<"guest">>, tags => [<<"administrator">>]},
                 http_get(Config, "/whoami")),
     http_put(Config, "/users/myuser1", [{tags, <<"management">>}, {password, <<"myuser">>}],
              {group, '2xx'}),
@@ -584,9 +584,9 @@ users_legacy_administrator_test(Config) ->
     http_put(Config, "/users/myuser2", [{administrator, <<"false">>},
                                         {password,      <<"myuser2">>}],
              {group, '2xx'}),
-    assert_item(#{name => <<"myuser1">>, tags => <<"administrator">>},
+    assert_item(#{name => <<"myuser1">>, tags => [<<"administrator">>]},
                 http_get(Config, "/users/myuser1")),
-    assert_item(#{name => <<"myuser2">>, tags => <<"">>},
+    assert_item(#{name => <<"myuser2">>, tags => []},
                 http_get(Config, "/users/myuser2")),
     http_delete(Config, "/users/myuser1", {group, '2xx'}),
     http_delete(Config, "/users/myuser2", {group, '2xx'}),
@@ -685,7 +685,7 @@ updating_a_user_without_password_or_hash_clears_password_test(Config) ->
     %% clear users' credentials
     http_put(Config, "/users/myuser", [{tags,     <<"management">>}], [?CREATED, ?NO_CONTENT]),
     assert_item(#{name => <<"myuser">>,
-                  tags => <<"management">>,
+                  tags => [<<"management">>],
                   password_hash => <<>>,
                   hashing_algorithm => <<"rabbit_password_hashing_sha256">>},
                 http_get(Config, "/users/myuser")),
@@ -723,21 +723,21 @@ updating_tags_of_a_passwordless_user_test(Config) ->
     %% clear user's password
     http_put(Config, "/users/abc", [{tags,     <<"management">>}], [?CREATED, ?NO_CONTENT]),
     assert_item(#{name => ?NON_GUEST_USERNAME,
-                  tags => <<"management">>,
+                  tags => [<<"management">>],
                   password_hash => <<>>,
                   hashing_algorithm => <<"rabbit_password_hashing_sha256">>},
                 http_get(Config, "/users/abc")),
 
     http_put(Config, "/users/abc", [{tags,     <<"impersonator">>}], [?CREATED, ?NO_CONTENT]),
     assert_item(#{name => ?NON_GUEST_USERNAME,
-                  tags => <<"impersonator">>,
+                  tags => [<<"impersonator">>],
                   password_hash => <<>>,
                   hashing_algorithm => <<"rabbit_password_hashing_sha256">>},
                 http_get(Config, "/users/abc")),
 
     http_put(Config, "/users/abc", [{tags,     <<"">>}], [?CREATED, ?NO_CONTENT]),
     assert_item(#{name => ?NON_GUEST_USERNAME,
-                  tags => <<"">>,
+                  tags => [],
                   password_hash => <<>>,
                   hashing_algorithm => <<"rabbit_password_hashing_sha256">>},
                 http_get(Config, "/users/abc")),
@@ -1613,7 +1613,7 @@ definitions_test(Config) ->
          #{name              => <<"myuser">>,
            password_hash     => <<"WAbU0ZIcvjTpxM3Q3SbJhEAM2tQ=">>,
            hashing_algorithm => <<"rabbit_password_hashing_sha256">>,
-           tags              => <<"management">>}),
+           tags              => [<<"management">>]}),
     defs(Config, vhosts, "/vhosts/myvhost", put,
          #{name => <<"myvhost">>}),
     defs(Config, permissions, "/permissions/%2F/guest", put,
@@ -1788,7 +1788,7 @@ definitions_password_test(Config) ->
     Expected35 = #{name              => <<"myuser">>,
                    password_hash     => <<"WAbU0ZIcvjTpxM3Q3SbJhEAM2tQ=">>,
                    hashing_algorithm => <<"rabbit_password_hashing_md5">>,
-                   tags              => <<"management">>},
+                   tags              => [<<"management">>]},
     http_post(Config, "/definitions", Config35, {group, '2xx'}),
     Definitions35 = http_get(Config, "/definitions", ?OK),
     ct:pal("Definitions35: ~p", [Definitions35]),
@@ -1804,7 +1804,7 @@ definitions_password_test(Config) ->
     Expected36 = #{name              => <<"myuser">>,
                    password_hash     => <<"WAbU0ZIcvjTpxM3Q3SbJhEAM2tQ=">>,
                    hashing_algorithm => <<"rabbit_password_hashing_sha256">>,
-                   tags              => <<"management">>},
+                   tags              => [<<"management">>]},
     http_post(Config, "/definitions", Config36, {group, '2xx'}),
 
     Definitions36 = http_get(Config, "/definitions", ?OK),
@@ -1824,7 +1824,7 @@ definitions_password_test(Config) ->
     ExpectedDefault = #{name              => <<"myuser">>,
                         password_hash     => <<"WAbU0ZIcvjTpxM3Q3SbJhEAM2tQ=">>,
                         hashing_algorithm => <<"rabbit_password_hashing_sha512">>,
-                        tags              => <<"management">>},
+                        tags              => [<<"management">>]},
     http_post(Config, "/definitions", ConfigDefault, {group, '2xx'}),
 
     DefinitionsDefault = http_get(Config, "/definitions", ?OK),
diff --git a/deps/rabbitmq_management_agent/src/rabbit_mgmt_format.erl b/deps/rabbitmq_management_agent/src/rabbit_mgmt_format.erl
@@ -30,6 +30,7 @@
 -export([args_hash/1]).
 
 -import(rabbit_misc, [pget/2, pget/3, pset/3]).
+-import(rabbit_data_coercion, [to_binary/1]).
 
 -include_lib("rabbit_common/include/rabbit.hrl").
 -include_lib("rabbit_common/include/rabbit_framing.hrl").
@@ -215,15 +216,16 @@ internal_user(User) ->
      {password_hash,     base64:encode(internal_user:get_password_hash(User))},
      {hashing_algorithm, rabbit_auth_backend_internal:hashing_module_for_user(
                              User)},
-     {tags,              tags(internal_user:get_tags(User))},
+     {tags,              tags_as_binaries(internal_user:get_tags(User))},
      {limits,            internal_user:get_limits(User)}].
 
 user(User) ->
     [{name, User#user.username},
-     {tags, tags(User#user.tags)}].
+     {tags, tags_as_binaries(User#user.tags)}].
+
+tags_as_binaries(Tags) ->
+    [to_binary(T) || T <- Tags].
 
-tags(Tags) ->
-    list_to_binary(string:join([atom_to_list(T) || T <- Tags], ",")).
 
 listener(#listener{node = Node, protocol = Protocol,
                    ip_address = IPAddress, port = Port, opts=Opts}) ->
