diff --git a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl index 5591d4f1e..7153056ae 100644 --- a/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl +++ b/apps/emqx_dashboard_sso/src/emqx_dashboard_sso_ldap.erl @@ -124,7 +124,7 @@ login( of {ok, []} -> {error, user_not_found}; - {ok, [Entry | _]} -> + {ok, [Entry]} -> case emqx_resource:simple_sync_query( ResourceId, diff --git a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl index 048a4923a..8966ffca9 100644 --- a/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl +++ b/apps/emqx_dashboard_sso/test/emqx_dashboard_sso_ldap_SUITE.erl @@ -13,8 +13,12 @@ -define(LDAP_HOST, "ldap"). -define(LDAP_DEFAULT_PORT, 389). --define(LDAP_USER, <<"mqttuser0001">>). --define(LDAP_USER_PASSWORD, <<"mqttuser0001">>). +-define(LDAP_USER, <<"viewer1">>). +-define(LDAP_USER_PASSWORD, <<"viewer1">>). +-define(LDAP_BASE_DN, <<"ou=dashboard,dc=emqx,dc=io">>). +-define(LDAP_FILTER_WITH_UID, <<"(uid=${username})">>). +%% there are more than one users in this group +-define(LDAP_FILTER_WITH_GROUP, <<"(ugroup=group1)">>). -define(MOD_TAB, emqx_dashboard_sso). -define(MOD_KEY_PATH, [dashboard, sso, ldap]). @@ -22,6 +26,7 @@ -import(emqx_mgmt_api_test_util, [request/2, request/3, uri/1, request_api/3]). +%% order matters all() -> [ t_bad_create, @@ -31,6 +36,7 @@ all() -> t_login_with_bad, t_first_login, t_next_login, + t_more_than_one_user_matched, t_bad_update, t_delete ]. @@ -46,12 +52,13 @@ end_per_suite(_Config) -> [emqx_dashboard_admin:remove_user(Name) || #{username := Name} <- All], emqx_mgmt_api_test_util:end_suite([emqx_conf, emqx_dashboard_sso]). -init_per_testcase(_, Config) -> +init_per_testcase(Case, Config) -> {ok, _} = emqx_cluster_rpc:start_link(), + ?MODULE:Case({init, Config}), Config. -end_per_testcase(Case, _) -> - Case =:= t_delete_backend andalso emqx_dashboard_sso_manager:delete(ldap), +end_per_testcase(Case, Config) -> + ?MODULE:Case({'end', Config}), case erlang:whereis(node()) of undefined -> ok; @@ -61,6 +68,10 @@ end_per_testcase(Case, _) -> end, ok. +t_bad_create({init, Config}) -> + Config; +t_bad_create({'end', _}) -> + ok; t_bad_create(_) -> Path = uri(["sso", "ldap"]), ?assertMatch( @@ -90,6 +101,10 @@ t_bad_create(_) -> ), ok. +t_create({init, Config}) -> + Config; +t_create({'end', _Config}) -> + ok; t_create(_) -> check_running([]), Path = uri(["sso", "ldap"]), @@ -109,6 +124,10 @@ t_create(_) -> ?assertNotEqual(undefined, emqx_dashboard_sso_manager:lookup_state(ldap)), ok. +t_update({init, Config}) -> + Config; +t_update({'end', _Config}) -> + ok; t_update(_) -> Path = uri(["sso", "ldap"]), {ok, 200, Result} = request(put, Path, ldap_config(#{<<"enable">> => <<"true">>})), @@ -118,6 +137,10 @@ t_update(_) -> ?assertNotEqual(undefined, emqx_dashboard_sso_manager:lookup_state(ldap)), ok. +t_get({init, Config}) -> + Config; +t_get({'end', _Config}) -> + ok; t_get(_) -> Path = uri(["sso", "ldap"]), {ok, 200, Result} = request(get, Path), @@ -127,6 +150,10 @@ t_get(_) -> {ok, 400, _} = request(get, NotExists), ok. +t_login_with_bad({init, Config}) -> + Config; +t_login_with_bad({'end', _Config}) -> + ok; t_login_with_bad(_) -> Path = uri(["sso", "login", "ldap"]), Req = #{ @@ -138,6 +165,10 @@ t_login_with_bad(_) -> ?assertMatch(#{code := <<"BAD_USERNAME_OR_PWD">>}, decode_json(Result)), ok. +t_first_login({init, Config}) -> + Config; +t_first_login({'end', _Config}) -> + ok; t_first_login(_) -> Path = uri(["sso", "login", "ldap"]), Req = #{ @@ -154,6 +185,10 @@ t_first_login(_) -> ), ok. +t_next_login({init, Config}) -> + Config; +t_next_login({'end', _Config}) -> + ok; t_next_login(_) -> Path = uri(["sso", "login", "ldap"]), Req = #{ @@ -165,6 +200,38 @@ t_next_login(_) -> ?assertMatch(#{license := _, token := _}, decode_json(Result)), ok. +t_more_than_one_user_matched({init, Config}) -> + emqx_logger:set_primary_log_level(error), + Config; +t_more_than_one_user_matched({'end', _Config}) -> + %% restore default config + Path = uri(["sso", "ldap"]), + {ok, 200, _} = request(put, Path, ldap_config(#{<<"enable">> => true})), + ok; +t_more_than_one_user_matched(_) -> + Path = uri(["sso", "ldap"]), + %% change to query with ugroup=group1 + NewConfig = ldap_config(#{ + <<"enable">> => true, + <<"base_dn">> => ?LDAP_BASE_DN, + <<"filter">> => ?LDAP_FILTER_WITH_GROUP + }), + ?assertMatch({ok, 200, _}, request(put, Path, NewConfig)), + check_running([<<"ldap">>]), + Path1 = uri(["sso", "login", "ldap"]), + Req = #{ + <<"backend">> => <<"ldap">>, + <<"username">> => ?LDAP_USER, + <<"password">> => ?LDAP_USER_PASSWORD + }, + {ok, 401, Result} = request(post, Path1, Req), + ?assertMatch(#{code := <<"BAD_USERNAME_OR_PWD">>}, decode_json(Result)), + ok. + +t_bad_update({init, Config}) -> + Config; +t_bad_update({'end', _Config}) -> + ok; t_bad_update(_) -> Path = uri(["sso", "ldap"]), ?assertMatch( @@ -184,9 +251,12 @@ t_bad_update(_) -> ?assertMatch( [#{backend := <<"ldap">>, enable := true, running := false, last_error := _}], get_sso() ), - ok. +t_delete({init, Config}) -> + Config; +t_delete({'end', _Config}) -> + ok; t_delete(_) -> Path = uri(["sso", "ldap"]), ?assertMatch({ok, 204, _}, request(delete, Path)), @@ -214,8 +284,8 @@ ldap_config(Override) -> <<"backend">> => <<"ldap">>, <<"enable">> => <<"false">>, <<"server">> => ldap_server(), - <<"base_dn">> => <<"uid=${username},ou=testdevice,dc=emqx,dc=io">>, - <<"filter">> => <<"(objectClass=mqttUser)">>, + <<"base_dn">> => ?LDAP_BASE_DN, + <<"filter">> => ?LDAP_FILTER_WITH_UID, <<"username">> => <<"cn=root,dc=emqx,dc=io">>, <<"password">> => <<"public">>, <<"pool_size">> => 8 diff --git a/apps/emqx_ldap/src/emqx_ldap.erl b/apps/emqx_ldap/src/emqx_ldap.erl index 394687bc5..e923ee7c3 100644 --- a/apps/emqx_ldap/src/emqx_ldap.erl +++ b/apps/emqx_ldap/src/emqx_ldap.erl @@ -262,7 +262,25 @@ do_ldap_query( ldap_connector_query_return, #{result => Result} ), - {ok, Result#eldap_search_result.entries}; + case Result#eldap_search_result.entries of + [_] = Entry -> + {ok, Entry}; + [_ | _] = L -> + %% Accept only a single exact match. + %% Multiple matches likely indicate: + %% 1. A misconfiguration in EMQX, allowing overly broad query conditions. + %% 2. Indistinguishable entries in the LDAP database. + %% Neither scenario should be allowed to proceed. + Msg = "ldap_query_found_more_than_one_match", + ?SLOG( + error, + LogMeta#{ + msg => "ldap_query_found_more_than_one_match", + count => length(L) + } + ), + {error, {recoverable_error, Msg}} + end; {error, 'noSuchObject'} -> {ok, []}; {error, Reason} -> diff --git a/apps/emqx_ldap/src/emqx_ldap_authn.erl b/apps/emqx_ldap/src/emqx_ldap_authn.erl index cbe62dfb2..16540bec8 100644 --- a/apps/emqx_ldap/src/emqx_ldap_authn.erl +++ b/apps/emqx_ldap/src/emqx_ldap_authn.erl @@ -131,7 +131,7 @@ authenticate( of {ok, []} -> ignore; - {ok, [Entry | _]} -> + {ok, [Entry]} -> is_enabled(Password, Entry, State); {error, Reason} -> ?TRACE_AUTHN_PROVIDER(error, "ldap_query_failed", #{ diff --git a/apps/emqx_ldap/src/emqx_ldap_authn_bind.erl b/apps/emqx_ldap/src/emqx_ldap_authn_bind.erl index fe192bf88..37216ef3e 100644 --- a/apps/emqx_ldap/src/emqx_ldap_authn_bind.erl +++ b/apps/emqx_ldap/src/emqx_ldap_authn_bind.erl @@ -95,7 +95,7 @@ authenticate( of {ok, []} -> ignore; - {ok, [Entry | _]} -> + {ok, [Entry]} -> case emqx_resource:simple_sync_query( ResourceId, diff --git a/apps/emqx_ldap/src/emqx_ldap_authz.erl b/apps/emqx_ldap/src/emqx_ldap_authz.erl index b48b3a48b..bfff9149f 100644 --- a/apps/emqx_ldap/src/emqx_ldap_authz.erl +++ b/apps/emqx_ldap/src/emqx_ldap_authz.erl @@ -111,11 +111,11 @@ authorize( case emqx_resource:simple_sync_query(ResourceID, {query, Client, Attrs, QueryTimeout}) of {ok, []} -> nomatch; - {ok, [Entry | _]} -> + {ok, [Entry]} -> do_authorize(Action, Topic, Attrs, Entry); {error, Reason} -> ?SLOG(error, #{ - msg => "query_ldap_error", + msg => "ldap_query_failed", reason => emqx_utils:redact(Reason), resource_id => ResourceID }), diff --git a/apps/emqx_management/test/emqx_mgmt_api_test_util.erl b/apps/emqx_management/test/emqx_mgmt_api_test_util.erl index e6c3bb3d6..cd768a529 100644 --- a/apps/emqx_management/test/emqx_mgmt_api_test_util.erl +++ b/apps/emqx_management/test/emqx_mgmt_api_test_util.erl @@ -119,7 +119,7 @@ do_request_api(Method, Request, Opts) -> ReturnAll = maps:get(return_all, Opts, false), CompatibleMode = maps:get(compatible_mode, Opts, false), HttpcReqOpts = maps:get(httpc_req_opts, Opts, []), - ct:pal("Method: ~p, Request: ~p, Opts: ~p", [Method, Request, Opts]), + ct:pal("~p: ~p~nOpts: ~p", [Method, Request, Opts]), case httpc:request(Method, Request, [], HttpcReqOpts) of {error, socket_closed_remotely} -> {error, socket_closed_remotely};