From 4dbe3ccf71f1e7b54d2423960fc6a5e7011d2e5c Mon Sep 17 00:00:00 2001 From: "Zaiming (Stone) Shi" Date: Tue, 26 Oct 2021 14:41:33 +0200 Subject: [PATCH] refactor(dashboard): rename 'password' field to 'pwdhash' (#5990) * refactor(dashboard): rename 'password' field to 'pwdhash' rename as it is not plaintext password stored in db * refactor(emqx_dashboard): rename records * test(emqx_dashboard_token): add test case to cover match specs --- .../emqx_dashboard/include/emqx_dashboard.hrl | 24 ++++---- .../src/emqx_dashboard_admin.erl | 59 +++++++++++-------- .../emqx_dashboard/src/emqx_dashboard_api.erl | 5 +- .../src/emqx_dashboard_schema.erl | 4 ++ .../src/emqx_dashboard_token.erl | 53 +++++++++-------- .../test/emqx_dashboard_SUITE.erl | 48 ++++++++++++--- scripts/find-apps.sh | 2 +- 7 files changed, 124 insertions(+), 71 deletions(-) diff --git a/apps/emqx_dashboard/include/emqx_dashboard.hrl b/apps/emqx_dashboard/include/emqx_dashboard.hrl index 265552bf7..e7fb4557b 100644 --- a/apps/emqx_dashboard/include/emqx_dashboard.hrl +++ b/apps/emqx_dashboard/include/emqx_dashboard.hrl @@ -14,20 +14,24 @@ %% limitations under the License. %%-------------------------------------------------------------------- --record(mqtt_admin, { - username :: binary(), - password :: binary(), - tags :: list() | binary(), - role = undefined :: atom() +-record(emqx_admin, { + username :: binary(), + pwdhash :: binary(), + tags :: list() | binary(), + role = undefined :: atom(), + extra = [] :: term() %% not used so far, for future extension }). --record(mqtt_admin_jwt, { - token :: binary(), - username :: binary(), - exptime :: integer() +-define(ADMIN, emqx_admin). + +-record(emqx_admin_jwt, { + token :: binary(), + username :: binary(), + exptime :: integer(), + extra = [] :: term() %% not used so far, fur future extension }). --type(mqtt_admin() :: #mqtt_admin{}). +-define(ADMIN_JWT, emqx_admin_jwt). -define(EMPTY_KEY(Key), ((Key == undefined) orelse (Key == <<>>))). diff --git a/apps/emqx_dashboard/src/emqx_dashboard_admin.erl b/apps/emqx_dashboard/src/emqx_dashboard_admin.erl index 1cde1035e..9622e6ed8 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_admin.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_admin.erl @@ -25,7 +25,6 @@ %% Mnesia bootstrap -export([mnesia/1]). -%% mqtt_admin api -export([ add_user/3 , force_add_user/3 , remove_user/1 @@ -44,17 +43,19 @@ -export([add_default_user/0]). +-type emqx_admin() :: #?ADMIN{}. + %%-------------------------------------------------------------------- %% Mnesia bootstrap %%-------------------------------------------------------------------- mnesia(boot) -> - ok = mria:create_table(mqtt_admin, [ + ok = mria:create_table(?ADMIN, [ {type, set}, {rlog_shard, ?DASHBOARD_SHARD}, {storage, disc_copies}, - {record_name, mqtt_admin}, - {attributes, record_info(fields, mqtt_admin)}, + {record_name, ?ADMIN}, + {attributes, record_info(fields, ?ADMIN)}, {storage_properties, [{ets, [{read_concurrency, true}, {write_concurrency, true}]}]}]). @@ -64,14 +65,15 @@ mnesia(boot) -> -spec(add_user(binary(), binary(), binary()) -> ok | {error, any()}). add_user(Username, Password, Tags) when is_binary(Username), is_binary(Password) -> - Admin = #mqtt_admin{username = Username, password = hash(Password), tags = Tags}, + Admin = #?ADMIN{username = Username, pwdhash = hash(Password), tags = Tags}, return(mria:transaction(?DASHBOARD_SHARD, fun add_user_/1, [Admin])). +%% black-magic: force overwrite a user force_add_user(Username, Password, Tags) -> AddFun = fun() -> - mnesia:write(#mqtt_admin{username = Username, - password = Password, - tags = Tags}) + mnesia:write(#?ADMIN{username = Username, + pwdhash = hash(Password), + tags = Tags}) end, case mria:transaction(?DASHBOARD_SHARD, AddFun) of {atomic, ok} -> ok; @@ -79,8 +81,8 @@ force_add_user(Username, Password, Tags) -> end. %% @private -add_user_(Admin = #mqtt_admin{username = Username}) -> - case mnesia:wread({mqtt_admin, Username}) of +add_user_(Admin = #?ADMIN{username = Username}) -> + case mnesia:wread({?ADMIN, Username}) of [] -> mnesia:write(Admin); [_] -> mnesia:abort(<<"Username Already Exist">>) end. @@ -93,7 +95,7 @@ remove_user(Username) when is_binary(Username) -> mnesia:abort(<<"Username Not Found">>); _ -> ok end, - mnesia:delete({mqtt_admin, Username}) + mnesia:delete({?ADMIN, Username}) end, return(mria:transaction(?DASHBOARD_SHARD, Trans)). @@ -103,9 +105,9 @@ update_user(Username, Tags) when is_binary(Username) -> %% @private update_user_(Username, Tags) -> - case mnesia:wread({mqtt_admin, Username}) of + case mnesia:wread({?ADMIN, Username}) of [] -> mnesia:abort(<<"Username Not Found">>); - [Admin] -> mnesia:write(Admin#mqtt_admin{tags = Tags}) + [Admin] -> mnesia:write(Admin#?ADMIN{tags = Tags}) end. change_password(Username, OldPasswd, NewPasswd) when is_binary(Username) -> @@ -119,7 +121,7 @@ change_password(Username, Password) when is_binary(Username), is_binary(Password change_password_hash(Username, PasswordHash) -> update_pwd(Username, fun(User) -> - User#mqtt_admin{password = PasswordHash} + User#?ADMIN{pwdhash = PasswordHash} end). update_pwd(Username, Fun) -> @@ -135,14 +137,23 @@ update_pwd(Username, Fun) -> return(mria:transaction(?DASHBOARD_SHARD, Trans)). --spec(lookup_user(binary()) -> [mqtt_admin()]). +-spec(lookup_user(binary()) -> [emqx_admin()]). lookup_user(Username) when is_binary(Username) -> - Fun = fun() -> mnesia:read(mqtt_admin, Username) end, + Fun = fun() -> mnesia:read(?ADMIN, Username) end, {atomic, User} = mria:ro_transaction(?DASHBOARD_SHARD, Fun), User. --spec(all_users() -> [#mqtt_admin{}]). -all_users() -> ets:tab2list(mqtt_admin). +-spec(all_users() -> [map()]). +all_users() -> + lists:map(fun(#?ADMIN{username = Username, + tags = Tags + }) -> + #{username => Username, + %% named tag but not tags, for unknown reason + %% TODO: fix this comment + tag => Tags + } + end, ets:tab2list(?ADMIN)). return({atomic, _}) -> ok; @@ -150,18 +161,18 @@ return({aborted, Reason}) -> {error, Reason}. check(undefined, _) -> - {error, <<"Username undefined">>}; + {error, <<"username_not_provided">>}; check(_, undefined) -> - {error, <<"Password undefined">>}; + {error, <<"password_not_provided">>}; check(Username, Password) -> case lookup_user(Username) of - [#mqtt_admin{password = <>}] -> + [#?ADMIN{pwdhash = <>}] -> case Hash =:= md5_hash(Salt, Password) of true -> ok; - false -> {error, <<"PASSWORD_ERROR">>} + false -> {error, <<"BAD_USERNAME_OR_PASSWORD">>} end; [] -> - {error, <<"USERNAME_ERROR">>} + {error, <<"BAD_USERNAME_OR_PASSWORD">>} end. %%-------------------------------------------------------------------- @@ -179,7 +190,7 @@ verify_token(Token) -> destroy_token_by_username(Username, Token) -> case emqx_dashboard_token:lookup(Token) of - {ok, #mqtt_admin_jwt{username = Username}} -> + {ok, #?ADMIN_JWT{username = Username}} -> emqx_dashboard_token:destroy(Token); _ -> {error, not_found} diff --git a/apps/emqx_dashboard/src/emqx_dashboard_api.erl b/apps/emqx_dashboard/src/emqx_dashboard_api.erl index 5b22facf6..5cc4d2a16 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_api.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_api.erl @@ -188,7 +188,7 @@ logout(_, #{body := #{<<"username">> := Username}, end. users(get, _Request) -> - {200, [row(User) || User <- emqx_dashboard_admin:all_users()]}; + {200, emqx_dashboard_admin:all_users()}; users(post, #{body := Params}) -> Tag = maps:get(<<"tag">>, Params), @@ -231,6 +231,3 @@ change_pwd(put, #{bindings := #{username := Username}, body := Params}) -> {error, Reason} -> {400, #{code => <<"CHANGE_PWD_FAIL">>, message => Reason}} end. - -row(#mqtt_admin{username = Username, tags = Tag}) -> - #{username => Username, tag => Tag}. diff --git a/apps/emqx_dashboard/src/emqx_dashboard_schema.erl b/apps/emqx_dashboard/src/emqx_dashboard_schema.erl index ff3be9320..58e154da8 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_schema.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_schema.erl @@ -57,6 +57,10 @@ default_username(_) -> undefined. default_password(type) -> string(); default_password(default) -> "public"; default_password(nullable) -> false; +default_password(sensitive) -> true; +default_password(desc) -> """ +The initial default password for dashboard 'admin' user. +For safty, it should be changed as soon as possible."""; default_password(_) -> undefined. sc(Type, Meta) -> hoconsc:mk(Type, Meta). diff --git a/apps/emqx_dashboard/src/emqx_dashboard_token.erl b/apps/emqx_dashboard/src/emqx_dashboard_token.erl index e0fa9c415..f8c023b46 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_token.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_token.erl @@ -18,8 +18,6 @@ -include("emqx_dashboard.hrl"). --define(TAB, mqtt_admin_jwt). - -export([ sign/2 , verify/1 , lookup/1 @@ -31,9 +29,12 @@ -export([mnesia/1]). --define(EXPTIME, 60 * 60 * 1000). +-ifdef(TEST). +-export([lookup_by_username/1, clean_expired_jwt/1]). +-endif. --define(CLEAN_JWT_INTERVAL, 60 * 60 * 1000). +-define(TAB, ?ADMIN_JWT). +-define(EXPTIME, 60 * 60 * 1000). %%-------------------------------------------------------------------- %% gen server part @@ -60,13 +61,12 @@ sign(Username, Password) -> verify(Token) -> do_verify(Token). --spec(destroy(KeyOrKeys :: list() | binary() | #mqtt_admin_jwt{}) -> ok). +-spec(destroy(KeyOrKeys :: list() | binary() | #?ADMIN_JWT{}) -> ok). destroy([]) -> ok; destroy(JWTorTokenList) when is_list(JWTorTokenList)-> - [destroy(JWTorToken) || JWTorToken <- JWTorTokenList], - ok; -destroy(#mqtt_admin_jwt{token = Token}) -> + lists:foreach(fun destroy/1, JWTorTokenList); +destroy(#?ADMIN_JWT{token = Token}) -> destroy(Token); destroy(Token) when is_binary(Token)-> do_destroy(Token). @@ -80,8 +80,8 @@ mnesia(boot) -> {type, set}, {rlog_shard, ?DASHBOARD_SHARD}, {storage, disc_copies}, - {record_name, mqtt_admin_jwt}, - {attributes, record_info(fields, mqtt_admin_jwt)}, + {record_name, ?ADMIN_JWT}, + {attributes, record_info(fields, ?ADMIN_JWT)}, {storage_properties, [{ets, [{read_concurrency, true}, {write_concurrency, true}]}]}]). @@ -106,10 +106,10 @@ do_sign(Username, Password) -> do_verify(Token)-> case lookup(Token) of - {ok, JWT = #mqtt_admin_jwt{exptime = ExpTime}} -> + {ok, JWT = #?ADMIN_JWT{exptime = ExpTime}} -> case ExpTime > erlang:system_time(millisecond) of true -> - NewJWT = JWT#mqtt_admin_jwt{exptime = jwt_expiration_time()}, + NewJWT = JWT#?ADMIN_JWT{exptime = jwt_expiration_time()}, {atomic, Res} = mria:transaction(?DASHBOARD_SHARD, fun mnesia:write/1, [NewJWT]), Res; _ -> @@ -129,7 +129,7 @@ do_destroy_by_username(Username) -> %%-------------------------------------------------------------------- %% jwt internal util function --spec(lookup(Token :: binary()) -> {ok, #mqtt_admin_jwt{}} | {error, not_found}). +-spec(lookup(Token :: binary()) -> {ok, #?ADMIN_JWT{}} | {error, not_found}). lookup(Token) -> Fun = fun() -> mnesia:read(?TAB, Token) end, case mria:ro_transaction(?DASHBOARD_SHARD, Fun) of @@ -137,13 +137,13 @@ lookup(Token) -> {atomic, []} -> {error, not_found} end. +-dialyzer({nowarn_function, lookup_by_username/1}). lookup_by_username(Username) -> - Spec = [{{mqtt_admin_jwt, '_', Username, '_'}, [], ['$_']}], + Spec = [{#?ADMIN_JWT{username = Username, _ = '_'}, [], ['$_']}], Fun = fun() -> mnesia:select(?TAB, Spec) end, {atomic, List} = mria:ro_transaction(?DASHBOARD_SHARD, Fun), List. - jwk(Username, Password, Salt) -> Key = erlang:md5(<>), #{ @@ -152,8 +152,10 @@ jwk(Username, Password, Salt) -> }. jwt_expiration_time() -> - ExpTime = emqx_conf:get([emqx_dashboard, token_expired_time], ?EXPTIME), - erlang:system_time(millisecond) + ExpTime. + erlang:system_time(millisecond) + token_ttl(). + +token_ttl() -> + emqx_conf:get([emqx_dashboard, token_expired_time], ?EXPTIME). salt() -> _ = emqx_misc:rand_seed(), @@ -161,7 +163,7 @@ salt() -> <>. format(Token, Username, ExpTime) -> - #mqtt_admin_jwt{ + #?ADMIN_JWT{ token = Token, username = Username, exptime = ExpTime @@ -189,10 +191,7 @@ handle_cast(_Request, State) -> handle_info(clean_jwt, State) -> timer_clean(self()), Now = erlang:system_time(millisecond), - Spec = [{{mqtt_admin_jwt, '_', '_', '$1'}, [{'<', '$1', Now}], ['$_']}], - {atomic, JWTList} = mria:ro_transaction(?DASHBOARD_SHARD, - fun() -> mnesia:select(?TAB, Spec) end), - destroy(JWTList), + ok = clean_expired_jwt(Now), {noreply, State}; handle_info(_Info, State) -> {noreply, State}. @@ -204,4 +203,12 @@ code_change(_OldVsn, State, _Extra) -> {ok, State}. timer_clean(Pid) -> - erlang:send_after(?CLEAN_JWT_INTERVAL, Pid, clean_jwt). + erlang:send_after(token_ttl(), Pid, clean_jwt). + +-dialyzer({nowarn_function, clean_expired_jwt/1}). +clean_expired_jwt(Now) -> + Spec = [{#?ADMIN_JWT{exptime = '$1', token = '$2', _ = '_'}, + [{'<', '$1', Now}], ['$2']}], + {atomic, JWTList} = mria:ro_transaction(?DASHBOARD_SHARD, + fun() -> mnesia:select(?TAB, Spec) end), + ok = destroy(JWTList). diff --git a/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl b/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl index 9f5fb3526..42ffd7d45 100644 --- a/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl +++ b/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl @@ -26,8 +26,8 @@ ]). -include_lib("eunit/include/eunit.hrl"). - -include_lib("emqx/include/emqx.hrl"). +-include("emqx_dashboard.hrl"). -define(CONTENT_TYPE, "application/x-www-form-urlencoded"). @@ -41,8 +41,8 @@ all() -> %% TODO: V5 API -%% emqx_common_test_helpers:all(?MODULE). - []. +% emqx_common_test_helpers:all(?MODULE). + [t_cli, t_lookup_by_username_jwt, t_clean_expired_jwt]. init_per_suite(Config) -> emqx_common_test_helpers:start_apps([emqx_management, emqx_dashboard],fun set_special_configs/1), @@ -60,12 +60,12 @@ set_special_configs(_) -> ok. t_overview(_) -> - mnesia:clear_table(mqtt_admin), + mnesia:clear_table(?ADMIN), emqx_dashboard_admin:add_user(<<"admin">>, <<"public">>, <<"tag">>), [?assert(request_dashboard(get, api_path(erlang:atom_to_list(Overview)), auth_header_()))|| Overview <- ?OVERVIEWS]. t_admins_add_delete(_) -> - mnesia:clear_table(mqtt_admin), + mnesia:clear_table(?ADMIN), ok = emqx_dashboard_admin:add_user(<<"username">>, <<"password">>, <<"tag">>), ok = emqx_dashboard_admin:add_user(<<"username1">>, <<"password1">>, <<"tag1">>), Admins = emqx_dashboard_admin:all_users(), @@ -81,7 +81,7 @@ t_admins_add_delete(_) -> ?assertNotEqual(true, request_dashboard(get, api_path("brokers"), auth_header_("username", "pwd"))). t_rest_api(_Config) -> - mnesia:clear_table(mqtt_admin), + mnesia:clear_table(?ADMIN), emqx_dashboard_admin:add_user(<<"admin">>, <<"public">>, <<"administrator">>), {ok, Res0} = http_get("users"), @@ -102,13 +102,13 @@ t_rest_api(_Config) -> ok. t_cli(_Config) -> - [mria:dirty_delete(mqtt_admin, Admin) || Admin <- mnesia:dirty_all_keys(mqtt_admin)], + [mria:dirty_delete(?ADMIN, Admin) || Admin <- mnesia:dirty_all_keys(?ADMIN)], emqx_dashboard_cli:admins(["add", "username", "password"]), - [{mqtt_admin, <<"username">>, <>, _}] = + [#?ADMIN{ username = <<"username">>, pwdhash = <>}] = emqx_dashboard_admin:lookup_user(<<"username">>), ?assertEqual(Hash, erlang:md5(<>/binary>>)), emqx_dashboard_cli:admins(["passwd", "username", "newpassword"]), - [{mqtt_admin, <<"username">>, <>, _}] = + [#?ADMIN{username = <<"username">>, pwdhash = <>}] = emqx_dashboard_admin:lookup_user(<<"username">>), ?assertEqual(Hash1, erlang:md5(<>/binary>>)), emqx_dashboard_cli:admins(["del", "username"]), @@ -118,10 +118,40 @@ t_cli(_Config) -> AdminList = emqx_dashboard_admin:all_users(), ?assertEqual(2, length(AdminList)). +t_lookup_by_username_jwt(_Config) -> + User = bin(["user-", integer_to_list(random_num())]), + Pwd = bin(integer_to_list(random_num())), + emqx_dashboard_token:sign(User, Pwd), + ?assertMatch([#?ADMIN_JWT{username = User}], + emqx_dashboard_token:lookup_by_username(User)), + ok = emqx_dashboard_token:destroy_by_username(User), + %% issue a gen_server call to sync the async destroy gen_server cast + ok = gen_server:call(emqx_dashboard_token, dummy, infinity), + ?assertMatch([], emqx_dashboard_token:lookup_by_username(User)), + ok. + +t_clean_expired_jwt(_Config) -> + User = bin(["user-", integer_to_list(random_num())]), + Pwd = bin(integer_to_list(random_num())), + emqx_dashboard_token:sign(User, Pwd), + [#?ADMIN_JWT{username = User, exptime = ExpTime}] = + emqx_dashboard_token:lookup_by_username(User), + ok = emqx_dashboard_token:clean_expired_jwt(_Now1 = ExpTime), + ?assertMatch([#?ADMIN_JWT{username = User}], + emqx_dashboard_token:lookup_by_username(User)), + ok = emqx_dashboard_token:clean_expired_jwt(_Now2 = ExpTime + 1), + ?assertMatch([], emqx_dashboard_token:lookup_by_username(User)), + ok. + %%------------------------------------------------------------------------------ %% Internal functions %%------------------------------------------------------------------------------ +bin(X) -> iolist_to_binary(X). + +random_num() -> + erlang:system_time(nanosecond). + http_get(Path) -> request_api(get, api_path(Path), auth_header_()). diff --git a/scripts/find-apps.sh b/scripts/find-apps.sh index 177a4f57c..47b71c7ca 100755 --- a/scripts/find-apps.sh +++ b/scripts/find-apps.sh @@ -7,7 +7,7 @@ cd -P -- "$(dirname -- "$0")/.." find_app() { local appdir="$1" - find "${appdir}" -mindepth 1 -maxdepth 1 -type d | grep -vE "emqx_dashboard" + find "${appdir}" -mindepth 1 -maxdepth 1 -type d } find_app 'apps'