From 0b32bf72f7e84f7fdab8e92f6228d02322b88e1c Mon Sep 17 00:00:00 2001 From: DDDHuang <44492639+DDDHuang@users.noreply.github.com> Date: Tue, 21 Jun 2022 18:34:24 +0800 Subject: [PATCH 1/2] fix: dashboard users api, cannot delete self --- .../src/emqx_dashboard.appup.src | 10 +++- .../emqx_dashboard/src/emqx_dashboard_api.erl | 51 ++++++++++++++++--- .../src/emqx_dashboard_token.erl | 9 ++++ 3 files changed, 61 insertions(+), 9 deletions(-) diff --git a/apps/emqx_dashboard/src/emqx_dashboard.appup.src b/apps/emqx_dashboard/src/emqx_dashboard.appup.src index 82e0980a5..c499e1d9f 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard.appup.src +++ b/apps/emqx_dashboard/src/emqx_dashboard.appup.src @@ -1,7 +1,13 @@ %% -*- mode: erlang -*- %% Unless you know what you are doing, DO NOT edit manually!! {VSN, - [{"5.0.0",[{load_module,emqx_dashboard_api,brutal_purge,soft_purge,[]}]}, + [{"5.0.0", [ + {load_module,emqx_dashboard_api,brutal_purge,soft_purge,[]}, + {load_module,emqx_dashboard_token,brutal_purge,soft_purge,[]} + ]}, {<<".*">>,[]}], - [{"5.0.0",[{load_module,emqx_dashboard_api,brutal_purge,soft_purge,[]}]}, + [{"5.0.0", [ + {load_module,emqx_dashboard_api,brutal_purge,soft_purge,[]}, + {load_module,emqx_dashboard_token,brutal_purge,soft_purge,[]} + ]}, {<<".*">>,[]}]}. diff --git a/apps/emqx_dashboard/src/emqx_dashboard_api.erl b/apps/emqx_dashboard/src/emqx_dashboard_api.erl index ee288052e..b6231e7c6 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_api.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_api.erl @@ -272,22 +272,59 @@ user(put, #{bindings := #{username := Username}, body := Params}) -> {error, Reason} -> {404, ?USER_NOT_FOUND, Reason} end; -user(delete, #{bindings := #{username := Username}}) -> +user(delete, #{bindings := #{username := Username}, headers := Headers}) -> case Username == emqx_dashboard_admin:default_username() of true -> ?SLOG(info, #{msg => "Dashboard delete admin user failed", username => Username}), Message = list_to_binary(io_lib:format("Cannot delete user ~p", [Username])), {400, ?NOT_ALLOWED, Message}; false -> - case emqx_dashboard_admin:remove_user(Username) of - {error, Reason} -> - {404, ?USER_NOT_FOUND, Reason}; - {ok, _} -> - ?SLOG(info, #{msg => "Dashboard delete admin user", username => Username}), - {204} + case is_self_auth(Username, Headers) of + true -> + {400, ?NOT_ALLOWED, <<"Cannot delete self">>}; + false -> + case emqx_dashboard_admin:remove_user(Username) of + {error, Reason} -> + {404, ?USER_NOT_FOUND, Reason}; + {ok, _} -> + ?SLOG(info, #{ + msg => "Dashboard delete admin user", username => Username + }), + {204} + end end end. +is_self_auth(Username, #{<<"authorization">> := Token}) -> + is_self_auth(Username, Token); +is_self_auth(Username, #{<<"Authorization">> := Token}) -> + is_self_auth(Username, Token); +is_self_auth(Username, <<"basic ", Token/binary>>) -> + is_self_auth_basic(Username, Token); +is_self_auth(Username, <<"Basic ", Token/binary>>) -> + is_self_auth_basic(Username, Token); +is_self_auth(Username, <<"bearer ", Token/binary>>) -> + is_self_auth_token(Username, Token); +is_self_auth(Username, <<"Bearer ", Token/binary>>) -> + is_self_auth_token(Username, Token). + +is_self_auth_basic(Username, Token) -> + UP = base64:decode(Token), + case binary:match(UP, Username) of + {0, N} -> + binary:part(UP, {N, 1}) == <<":">>; + _ -> + false + end. + +is_self_auth_token(Username, Token) -> + case emqx_dashboard_token:owner(Token) of + {ok, Owner} -> + Owner == Username; + {error, _NotFound} -> + false + end. + change_pwd(put, #{bindings := #{username := Username}, body := Params}) -> LogMeta = #{msg => "Dashboard change password", username => Username}, OldPwd = maps:get(<<"old_pwd">>, Params), diff --git a/apps/emqx_dashboard/src/emqx_dashboard_token.erl b/apps/emqx_dashboard/src/emqx_dashboard_token.erl index fc693daf1..4fbde7538 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_token.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_token.erl @@ -22,6 +22,7 @@ sign/2, verify/1, lookup/1, + owner/1, destroy/1, destroy_by_username/1 ]). @@ -161,6 +162,14 @@ lookup_by_username(Username) -> {atomic, List} = mria:ro_transaction(?DASHBOARD_SHARD, Fun), List. +-spec owner(Token :: binary()) -> {ok, Username :: binary} | {error, not_found}. +owner(Token) -> + Fun = fun() -> mnesia:read(?TAB, Token) end, + case mria:ro_transaction(?DASHBOARD_SHARD, Fun) of + {atomic, [#?ADMIN_JWT{username = Username}]} -> {ok, Username}; + {atomic, []} -> {error, not_found} + end. + jwk(Username, Password, Salt) -> Key = crypto:hash(md5, <>), #{ From f502e09f5f749843f17b5ab99365514f804b7054 Mon Sep 17 00:00:00 2001 From: DDDHuang <44492639+DDDHuang@users.noreply.github.com> Date: Tue, 21 Jun 2022 19:09:51 +0800 Subject: [PATCH 2/2] fix(dashboard): add more test suite --- .../src/emqx_dashboard_token.erl | 2 +- .../test/emqx_dashboard_SUITE.erl | 55 +++++++++---------- 2 files changed, 27 insertions(+), 30 deletions(-) diff --git a/apps/emqx_dashboard/src/emqx_dashboard_token.erl b/apps/emqx_dashboard/src/emqx_dashboard_token.erl index 4fbde7538..a8806bb0f 100644 --- a/apps/emqx_dashboard/src/emqx_dashboard_token.erl +++ b/apps/emqx_dashboard/src/emqx_dashboard_token.erl @@ -162,7 +162,7 @@ lookup_by_username(Username) -> {atomic, List} = mria:ro_transaction(?DASHBOARD_SHARD, Fun), List. --spec owner(Token :: binary()) -> {ok, Username :: binary} | {error, not_found}. +-spec owner(Token :: binary()) -> {ok, Username :: binary()} | {error, not_found}. owner(Token) -> Fun = fun() -> mnesia:read(?TAB, Token) end, case mria:ro_transaction(?DASHBOARD_SHARD, Fun) of diff --git a/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl b/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl index c2e26160e..b3735109f 100644 --- a/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl +++ b/apps/emqx_dashboard/test/emqx_dashboard_SUITE.erl @@ -44,23 +44,17 @@ -define(APP_MANAGEMENT, emqx_management). -define(OVERVIEWS, [ - 'alarms/activated', - 'alarms/deactivated', - banned, - brokers, - stats, - metrics, - listeners, - clients, - subscriptions, - routes, - plugins + "alarms", + "banned", + "stats", + "metrics", + "listeners", + "clients", + "subscriptions" ]). all() -> - %% TODO: V5 API - %% emqx_common_test_helpers:all(?MODULE). - [t_cli, t_lookup_by_username_jwt, t_clean_expired_jwt, t_rest_api]. + emqx_common_test_helpers:all(?MODULE). end_suite() -> end_suite([]). @@ -98,37 +92,40 @@ t_overview(_) -> mnesia:clear_table(?ADMIN), emqx_dashboard_admin:add_user(<<"admin">>, <<"public">>, <<"simple_description">>), [ - ?assert( - request_dashboard( - get, - api_path(erlang:atom_to_list(Overview)), - auth_header_() - ) - ) + {ok, _} = request_dashboard(get, api_path([Overview]), auth_header_()) || Overview <- ?OVERVIEWS ]. t_admins_add_delete(_) -> mnesia:clear_table(?ADMIN), Desc = <<"simple description">>, - ok = emqx_dashboard_admin:add_user(<<"username">>, <<"password">>, Desc), - ok = emqx_dashboard_admin:add_user(<<"username1">>, <<"password1">>, Desc), + {ok, _} = emqx_dashboard_admin:add_user(<<"username">>, <<"password">>, Desc), + {ok, _} = emqx_dashboard_admin:add_user(<<"username1">>, <<"password1">>, Desc), Admins = emqx_dashboard_admin:all_users(), ?assertEqual(2, length(Admins)), - ok = emqx_dashboard_admin:remove_user(<<"username1">>), + {ok, _} = emqx_dashboard_admin:remove_user(<<"username1">>), Users = emqx_dashboard_admin:all_users(), ?assertEqual(1, length(Users)), - ok = emqx_dashboard_admin:change_password( + {ok, _} = emqx_dashboard_admin:change_password( <<"username">>, <<"password">>, <<"pwd">> ), timer:sleep(10), - Header = auth_header_(<<"username">>, <<"pwd">>), - ?assert(request_dashboard(get, api_path("brokers"), Header)), + {ok, _} = emqx_dashboard_admin:remove_user(<<"username">>). - ok = emqx_dashboard_admin:remove_user(<<"username">>), - ?assertNotEqual(true, request_dashboard(get, api_path("brokers"), Header)). +t_admin_delete_self_failed(_) -> + mnesia:clear_table(?ADMIN), + Desc = <<"simple description">>, + _ = emqx_dashboard_admin:add_user(<<"username1">>, <<"password">>, Desc), + Admins = emqx_dashboard_admin:all_users(), + ?assertEqual(1, length(Admins)), + Header = auth_header_(<<"username1">>, <<"password">>), + {error, {_, 400, _}} = request_dashboard(delete, api_path(["users", "username1"]), Header), + Token = erlang:iolist_to_binary(["Basic ", base64:encode("username1:password")]), + Header2 = {"Authorization", Token}, + {error, {_, 400, _}} = request_dashboard(delete, api_path(["users", "username1"]), Header2), + mnesia:clear_table(?ADMIN). t_rest_api(_Config) -> mnesia:clear_table(?ADMIN),