Merge pull request #11811 from lafirest/fix/api_rbac

fix(rbac): for compatibility with old data schema, extend the existing field as an extra

apps/emqx/test/emqx_common_test_http.erl

@@ -34,7 +34,7 @@
 -define(DEFAULT_APP_SECRET, <<"default_app_secret">>).
 
 %% from emqx_dashboard/include/emqx_dashboard_rbac.hrl
--define(ROLE_API_SUPERUSER, <<"api_administrator">>).
+-define(ROLE_API_SUPERUSER, <<"administrator">>).
 
 request_api(Method, Url, Auth) ->
     request_api(Method, Url, [], Auth, []).

apps/emqx_dashboard/include/emqx_dashboard_rbac.hrl

@@ -25,9 +25,9 @@
 -define(ROLE_SUPERUSER, <<"administrator">>).
 -define(ROLE_DEFAULT, ?ROLE_SUPERUSER).
 
--define(ROLE_API_VIEWER, <<"api_viewer">>).
--define(ROLE_API_SUPERUSER, <<"api_administrator">>).
--define(ROLE_API_PUBLISHER, <<"api_publisher">>).
+-define(ROLE_API_VIEWER, <<"viewer">>).
+-define(ROLE_API_SUPERUSER, <<"administrator">>).
+-define(ROLE_API_PUBLISHER, <<"publisher">>).
 -define(ROLE_API_DEFAULT, ?ROLE_API_SUPERUSER).
 
 -endif.

apps/emqx_dashboard_rbac/src/emqx_dashboard_rbac.erl

@@ -59,12 +59,8 @@ valid_role(Type, Role) ->
 %% ===================================================================
 check_rbac(?ROLE_SUPERUSER, _, _, _) ->
     true;
-check_rbac(?ROLE_API_SUPERUSER, _, _, _) ->
-    true;
 check_rbac(?ROLE_VIEWER, <<"GET">>, _, _) ->
     true;
-check_rbac(?ROLE_API_VIEWER, <<"GET">>, _, _) ->
-    true;
 check_rbac(?ROLE_API_PUBLISHER, <<"POST">>, <<"/publish">>, _) ->
     true;
 check_rbac(?ROLE_API_PUBLISHER, <<"POST">>, <<"/publish/bulk">>, _) ->

apps/emqx_management/src/emqx_mgmt_auth.erl

@@ -38,7 +38,7 @@
 -export([authorize/4]).
 -export([post_config_update/5]).
 
--export([backup_tables/0, validate_mnesia_backup/1, migrate_mnesia_backup/1]).
+-export([backup_tables/0, validate_mnesia_backup/1]).
 
 %% Internal exports (RPC)
 -export([
@@ -53,18 +53,17 @@
 -endif.
 
 -define(APP, emqx_app).
--type api_user_role() :: binary().
 
 -record(?APP, {
     name = <<>> :: binary() | '_',
     api_key = <<>> :: binary() | '_',
     api_secret_hash = <<>> :: binary() | '_',
     enable = true :: boolean() | '_',
-    desc = <<>> :: binary() | '_',
+    %% Since v5.4.0 the `desc` has changed to `extra`
+    %% desc = <<>> :: binary() | '_',
+    extra = #{} :: binary() | map() | '_',
     expired_at = 0 :: integer() | undefined | infinity | '_',
-    created_at = 0 :: integer() | '_',
-    role = ?ROLE_DEFAULT :: api_user_role() | '_',
-    extra = #{} :: map() | '_'
+    created_at = 0 :: integer() | '_'
 }).
 
 mnesia(boot) ->
@@ -75,8 +74,7 @@ mnesia(boot) ->
         {storage, disc_copies},
         {record_name, ?APP},
         {attributes, Fields}
-    ]),
-    maybe_migrate_table(Fields).
+    ]).
 
 %%--------------------------------------------------------------------
 %% Data backup
@@ -87,32 +85,19 @@ backup_tables() -> [?APP].
 validate_mnesia_backup({schema, _Tab, CreateList} = Schema) ->
     case emqx_mgmt_data_backup:default_validate_mnesia_backup(Schema) of
         ok ->
-            {ok, over};
+            ok;
         _ ->
             case proplists:get_value(attributes, CreateList) of
+                %% Since v5.4.0 the `desc` has changed to `extra`
                 [name, api_key, api_secret_hash, enable, desc, expired_at, created_at] ->
-                    {ok, migrate};
+                    ok;
                 Fields ->
                     {error, {unknow_fields, Fields}}
             end
     end;
 validate_mnesia_backup(_Other) ->
     ok.
 
-migrate_mnesia_backup({schema, Tab, CreateList}) ->
-    case proplists:get_value(attributes, CreateList) of
-        [name, api_key, api_secret_hash, enable, desc, expired_at, created_at] = Fields ->
-            NewFields = Fields ++ [role, extra],
-            CreateList2 = lists:keyreplace(
-                attributes, 1, CreateList, {attributes, NewFields}
-            ),
-            {ok, {schema, Tab, CreateList2}};
-        Fields ->
-            {error, {unknow_fields, Fields}}
-    end;
-migrate_mnesia_backup(Data) ->
-    {ok, do_table_migrate(Data)}.
-
 post_config_update([api_key], _Req, NewConf, _OldConf, _AppEnvs) ->
     #{bootstrap_file := File} = NewConf,
     case init_bootstrap_file(File) of
@@ -158,13 +143,13 @@ do_update(Name, Enable, ExpiredAt, Desc, Role) ->
     case mnesia:read(?APP, Name, write) of
         [] ->
             mnesia:abort(not_found);
-        [App0 = #?APP{enable = Enable0, desc = Desc0}] ->
+        [App0 = #?APP{enable = Enable0, extra = Extra0}] ->
+            #{desc := Desc0} = Extra = normalize_extra(Extra0),
             App =
                 App0#?APP{
                     expired_at = ExpiredAt,
                     enable = ensure_not_undefined(Enable, Enable0),
-                    desc = ensure_not_undefined(Desc, Desc0),
-                    role = Role
+                    extra = Extra#{desc := ensure_not_undefined(Desc, Desc0), role := Role}
                 },
             ok = mnesia:write(App),
             to_map(App)
@@ -220,10 +205,10 @@ find_by_api_key(ApiKey) ->
     case mria:ro_transaction(?COMMON_SHARD, Fun) of
         {atomic, [
             #?APP{
-                api_secret_hash = SecretHash, enable = Enable, expired_at = ExpiredAt, role = Role
+                api_secret_hash = SecretHash, enable = Enable, expired_at = ExpiredAt, extra = Extra
             }
         ]} ->
-            {ok, Enable, ExpiredAt, SecretHash, Role};
+            {ok, Enable, ExpiredAt, SecretHash, get_role(Extra)};
         _ ->
             {error, "not_found"}
     end.
@@ -234,15 +219,16 @@ ensure_not_undefined(New, _Old) -> New.
 to_map(Apps) when is_list(Apps) ->
     [to_map(App) || App <- Apps];
 to_map(#?APP{
-    name = N, api_key = K, enable = E, expired_at = ET, created_at = CT, desc = D, role = Role
+    name = N, api_key = K, enable = E, expired_at = ET, created_at = CT, extra = Extra0
 }) ->
+    #{role := Role, desc := Desc} = normalize_extra(Extra0),
     #{
         name => N,
         api_key => K,
         enable => E,
         expired_at => ET,
         created_at => CT,
-        desc => D,
+        desc => Desc,
         expired => is_expired(ET),
         role => Role
     }.
@@ -256,11 +242,10 @@ create_app(Name, ApiSecret, Enable, ExpiredAt, Desc, Role) ->
             name = Name,
             enable = Enable,
             expired_at = ExpiredAt,
-            desc = Desc,
+            extra = #{desc => Desc, role => Role},
             created_at = erlang:system_time(second),
             api_secret_hash = emqx_dashboard_admin:hash(ApiSecret),
-            api_key = list_to_binary(emqx_utils:gen_id(16)),
-            role = Role
+            api_key = list_to_binary(emqx_utils:gen_id(16))
         },
     case create_app(App) of
         {ok, Res} ->
@@ -269,7 +254,7 @@ create_app(Name, ApiSecret, Enable, ExpiredAt, Desc, Role) ->
             Error
     end.
 
-create_app(App = #?APP{api_key = ApiKey, name = Name, role = Role}) ->
+create_app(App = #?APP{api_key = ApiKey, name = Name, extra = #{role := Role}}) ->
     case valid_role(Role) of
         ok ->
             trans(fun ?MODULE:do_create_app/3, [App, ApiKey, Name]);
@@ -328,7 +313,7 @@ init_bootstrap_file(<<>>) ->
 init_bootstrap_file(File) ->
     case file:open(File, [read, binary]) of
         {ok, Dev} ->
-            {ok, MP} = re:compile(<<"(\.+):(\.+$)">>, [ungreedy]),
+            {ok, MP} = re:compile(<<"(\.+):(\.+)(?::(\.+))?$">>, [ungreedy]),
             init_bootstrap_file(File, Dev, MP);
         {error, Reason0} ->
             Reason = emqx_utils:explain_posix(Reason0),
@@ -358,13 +343,13 @@ init_bootstrap_file(File, Dev, MP) ->
 add_bootstrap_file(File, Dev, MP, Line) ->
     case file:read_line(Dev) of
         {ok, Bin} ->
-            case re:run(Bin, MP, [global, {capture, all_but_first, binary}]) of
-                {match, [[AppKey, ApiSecret]]} ->
+            case parse_bootstrap_line(Bin, MP) of
+                {ok, [AppKey, ApiSecret, Role]} ->
                     App =
                         #?APP{
                             enable = true,
                             expired_at = infinity,
-                            desc = ?BOOTSTRAP_TAG,
+                            extra = #{desc => ?BOOTSTRAP_TAG, role => Role},
                             created_at = erlang:system_time(second),
                             api_secret_hash = emqx_dashboard_admin:hash(ApiSecret),
                             api_key = AppKey
@@ -375,8 +360,7 @@ add_bootstrap_file(File, Dev, MP, Line) ->
                         {error, Reason} ->
                             throw(#{file => File, line => Line, content => Bin, reason => Reason})
                     end;
-                _ ->
-                    Reason = "invalid_format",
+                {error, Reason} ->
                     ?SLOG(
                         error,
                         #{
@@ -395,6 +379,33 @@ add_bootstrap_file(File, Dev, MP, Line) ->
             throw(#{file => File, line => Line, reason => Reason})
     end.
 
+parse_bootstrap_line(Bin, MP) ->
+    case re:run(Bin, MP, [global, {capture, all_but_first, binary}]) of
+        {match, [[_AppKey, _ApiSecret] = Args]} ->
+            {ok, Args ++ [?ROLE_API_DEFAULT]};
+        {match, [[_AppKey, _ApiSecret, Role] = Args]} ->
+            case valid_role(Role) of
+                ok ->
+                    {ok, Args};
+                _Error ->
+                    {error, {"invalid_role", Role}}
+            end;
+        _ ->
+            {error, "invalid_format"}
+    end.
+
+get_role(#{role := Role}) ->
+    Role;
+%% Before v5.4.0,
+%% the field in the position of the `extra` is `desc` which is a binary for description
+get_role(_Desc) ->
+    ?ROLE_API_DEFAULT.
+
+normalize_extra(Map) when is_map(Map) ->
+    Map;
+normalize_extra(Desc) ->
+    #{desc => Desc, role => ?ROLE_API_DEFAULT}.
+
 -if(?EMQX_RELEASE_EDITION == ee).
 check_rbac(Req, ApiKey, Role) ->
     case emqx_dashboard_rbac:check_rbac(Req, ApiKey, Role) of
@@ -424,28 +435,3 @@ valid_role(_) ->
     {error, <<"Role does not exist">>}.
 
 -endif.
-
-maybe_migrate_table(Fields) ->
-    case mnesia:table_info(?APP, attributes) =:= Fields of
-        true ->
-            ok;
-        false ->
-            TransFun = fun do_table_migrate/1,
-            {atomic, ok} = mnesia:transform_table(?APP, TransFun, Fields, ?APP),
-            ok
-    end.
-
-do_table_migrate({?APP, Name, Key, Hash, Enable, Desc, ExpiredAt, CreatedAt}) ->
-    #?APP{
-        name = Name,
-        api_key = Key,
-        api_secret_hash = Hash,
-        enable = Enable,
-        desc = Desc,
-        expired_at = ExpiredAt,
-        created_at = CreatedAt,
-        role = ?ROLE_API_DEFAULT,
-        extra = #{}
-    };
-do_table_migrate(#?APP{} = App) ->
-    App.

apps/emqx_management/test/emqx_mgmt_api_api_keys_SUITE.erl

@@ -39,7 +39,7 @@ groups() ->
     [
         {parallel, [parallel], [t_create, t_update, t_delete, t_authorize, t_create_unexpired_app]},
         {parallel, [parallel], ?EE_CASES},
-        {sequence, [], [t_bootstrap_file, t_create_failed]}
+        {sequence, [], [t_bootstrap_file, t_bootstrap_file_with_role, t_create_failed]}
     ].
 
 init_per_suite(Config) ->
@@ -86,6 +86,92 @@ t_bootstrap_file(_) ->
     update_file(<<>>),
     ok.
 
+-if(?EMQX_RELEASE_EDITION == ee).
+t_bootstrap_file_with_role(_) ->
+    Search = fun(Name) ->
+        lists:search(
+            fun(#{api_key := AppName}) ->
+                AppName =:= Name
+            end,
+            emqx_mgmt_auth:list()
+        )
+    end,
+
+    Bin = <<"role-1:role-1:viewer\nrole-2:role-2:administrator\nrole-3:role-3">>,
+    File = "./bootstrap_api_keys.txt",
+    ok = file:write_file(File, Bin),
+    update_file(File),
+
+    ?assertMatch(
+        {value, #{api_key := <<"role-1">>, role := <<"viewer">>}},
+        Search(<<"role-1">>)
+    ),
+
+    ?assertMatch(
+        {value, #{api_key := <<"role-2">>, role := <<"administrator">>}},
+        Search(<<"role-2">>)
+    ),
+
+    ?assertMatch(
+        {value, #{api_key := <<"role-3">>, role := <<"administrator">>}},
+        Search(<<"role-3">>)
+    ),
+
+    %% bad role
+    BadBin = <<"role-4:secret-11:bad\n">>,
+    ok = file:write_file(File, BadBin),
+    update_file(File),
+    ?assertEqual(
+        false,
+        Search(<<"role-4">>)
+    ),
+    ok.
+-else.
+t_bootstrap_file_with_role(_) ->
+    Search = fun(Name) ->
+        lists:search(
+            fun(#{api_key := AppName}) ->
+                AppName =:= Name
+            end,
+            emqx_mgmt_auth:list()
+        )
+    end,
+
+    Bin = <<"role-1:role-1:administrator\nrole-2:role-2">>,
+    File = "./bootstrap_api_keys.txt",
+    ok = file:write_file(File, Bin),
+    update_file(File),
+
+    ?assertMatch(
+        {value, #{api_key := <<"role-1">>, role := <<"administrator">>}},
+        Search(<<"role-1">>)
+    ),
+
+    ?assertMatch(
+        {value, #{api_key := <<"role-2">>, role := <<"administrator">>}},
+        Search(<<"role-2">>)
+    ),
+
+    %% only administrator
+    OtherRoleBin = <<"role-3:role-3:viewer\n">>,
+    ok = file:write_file(File, OtherRoleBin),
+    update_file(File),
+    ?assertEqual(
+        false,
+        Search(<<"role-3">>)
+    ),
+
+    %% bad role
+    BadBin = <<"role-4:secret-11:bad\n">>,
+    ok = file:write_file(File, BadBin),
+    update_file(File),
+    ?assertEqual(
+        false,
+        Search(<<"role-4">>)
+    ),
+    ok.
+-endif.
+
 auth_authorize(Path, Key, Secret) ->
     FakePath = erlang:list_to_binary(emqx_dashboard_swagger:relative_uri("/fake")),
     FakeReq = #{method => <<"GET">>, path => FakePath},

apps/emqx_management/test/emqx_mgmt_data_backup_SUITE.erl

@@ -23,7 +23,7 @@
 -include_lib("snabbkaffe/include/snabbkaffe.hrl").
 
 -define(ROLE_SUPERUSER, <<"administrator">>).
--define(ROLE_API_SUPERUSER, <<"api_administrator">>).
+-define(ROLE_API_SUPERUSER, <<"administrator">>).
 -define(BOOTSTRAP_BACKUP, "emqx-export-test-bootstrap-ce.tar.gz").
 
 all() ->