From 3717f25d4b6047a4bcca2421b5c5833b560f889d Mon Sep 17 00:00:00 2001 From: Kamil Waz Date: Thu, 17 Nov 2022 11:55:48 +0100 Subject: [PATCH 1/2] Improve error handling in token --- big_tests/tests/graphql_token_SUITE.erl | 32 +++++++++++++++--- big_tests/tests/oauth_SUITE.erl | 4 +-- .../mongoose_graphql_token_admin_mutation.erl | 4 +-- .../mongoose_graphql_last_user_mutation.erl | 2 +- .../user/mongoose_graphql_last_user_query.erl | 2 +- .../mongoose_graphql_token_user_mutation.erl | 6 ++-- src/mod_auth_token.erl | 7 ++-- src/mod_auth_token_api.erl | 33 ++++++++----------- 8 files changed, 54 insertions(+), 36 deletions(-) diff --git a/big_tests/tests/graphql_token_SUITE.erl b/big_tests/tests/graphql_token_SUITE.erl index aa49c37bb0..906141f9c0 100644 --- a/big_tests/tests/graphql_token_SUITE.erl +++ b/big_tests/tests/graphql_token_SUITE.erl @@ -2,6 +2,7 @@ -compile([export_all, nowarn_export_all]). +-import(common_helper, [unprep/1]). -import(distributed_helper, [require_rpc_nodes/1, mim/0]). -import(graphql_helper, [execute_command/4, execute_user_command/5, user_to_bin/1, get_ok_value/2, get_err_code/1, get_unauthorized/1, get_not_loaded/1]). @@ -46,17 +47,21 @@ user_token_not_configured_tests() -> domain_admin_tests() -> [admin_request_token_test, + admin_request_token_test_unprep, domain_admin_request_token_no_permission_test, domain_admin_revoke_token_no_permission_test, admin_revoke_token_no_token_test, - admin_revoke_token_test]. + admin_revoke_token_test, + admin_revoke_token_test_unprep]. admin_tests() -> [admin_request_token_test, + admin_request_token_test_unprep, admin_request_token_no_user_test, admin_revoke_token_no_user_test, admin_revoke_token_no_token_test, - admin_revoke_token_test]. + admin_revoke_token_test, + admin_revoke_token_test_unprep]. admin_token_not_configured_tests() -> [admin_request_token_test_not_configured, @@ -155,7 +160,7 @@ user_revoke_token_test(Config, Alice) -> user_request_token(Alice, Config), Res2 = user_revoke_token(Alice, Config), ParsedRes = get_ok_value([data, token, revokeToken], Res2), - ?assertEqual(<<"Revoked.">>, ParsedRes). + ?assertEqual(<<"Revoked">>, ParsedRes). % User test cases mod_token not configured @@ -213,6 +218,16 @@ admin_request_token_test(Config, Alice) -> ?assert(is_binary(Refresh)), ?assert(is_binary(Access)). +admin_request_token_test_unprep(Config) -> + escalus:fresh_story_with_config(Config, [{alice, 1}], fun admin_request_token_test_unprep/2). + +admin_request_token_test_unprep(Config, Alice) -> + Res = admin_request_token(unprep(user_to_bin(Alice)), Config), + #{<<"refresh">> := Refresh, <<"access">> := Access} = + get_ok_value([data, token, requestToken], Res), + ?assert(is_binary(Refresh)), + ?assert(is_binary(Access)). + admin_request_token_no_user_test(Config) -> Res = admin_request_token(<<"AAAAA">>, Config), ?assertEqual(<<"not_found">>, get_err_code(Res)). @@ -235,7 +250,16 @@ admin_revoke_token_test(Config, Alice) -> admin_request_token(user_to_bin(Alice), Config), Res2 = admin_revoke_token(user_to_bin(Alice), Config), ParsedRes = get_ok_value([data, token, revokeToken], Res2), - ?assertEqual(<<"Revoked.">>, ParsedRes). + ?assertEqual(<<"Revoked">>, ParsedRes). + +admin_revoke_token_test_unprep(Config) -> + escalus:fresh_story_with_config(Config, [{alice, 1}], fun admin_revoke_token_test_unprep/2). + +admin_revoke_token_test_unprep(Config, Alice) -> + admin_request_token(unprep(user_to_bin(Alice)), Config), + Res2 = admin_revoke_token(unprep(user_to_bin(Alice)), Config), + ParsedRes = get_ok_value([data, token, revokeToken], Res2), + ?assertEqual(<<"Revoked">>, ParsedRes). % Admin test cases token not configured diff --git a/big_tests/tests/oauth_SUITE.erl b/big_tests/tests/oauth_SUITE.erl index 24cf4515f8..527e107e57 100644 --- a/big_tests/tests/oauth_SUITE.erl +++ b/big_tests/tests/oauth_SUITE.erl @@ -291,7 +291,7 @@ revoke_token_cmd_when_no_token(Config) -> %% when revoking token R = mimctl(Config, ["revoke_token", escalus_users:get_jid(Config, bob)]), %% then no token was found - "Error: \"User or token not found.\"\n" = R. + "Error: \"User or token not found\"\n" = R. revoke_token_cmd(Config) -> %% given existing user and token present in the database @@ -299,7 +299,7 @@ revoke_token_cmd(Config) -> %% when R = mimctl(Config, ["revoke_token", escalus_users:get_jid(Config, bob)]), %% then - "Revoked.\n" = R. + "Revoked\n" = R. token_removed_on_user_removal(Config) -> %% given existing user with token and XMPP (de)registration available diff --git a/src/graphql/admin/mongoose_graphql_token_admin_mutation.erl b/src/graphql/admin/mongoose_graphql_token_admin_mutation.erl index cb6929a44d..9c1f4ecfe5 100644 --- a/src/graphql/admin/mongoose_graphql_token_admin_mutation.erl +++ b/src/graphql/admin/mongoose_graphql_token_admin_mutation.erl @@ -20,12 +20,12 @@ request_token(JID) -> case mod_auth_token_api:create_token(JID) of {ok, _} = Result -> Result; - {error, Error} -> make_error(Error, #{user => JID}) + Error -> make_error(Error, #{user => JID}) end. -spec revoke_token(jid:jid()) -> {ok, string()} | {error, resolver_error()}. revoke_token(JID) -> case mod_auth_token_api:revoke_token_command(JID) of {ok, _} = Result -> Result; - {error, Error} -> make_error(Error, #{user => JID}) + Error -> make_error(Error, #{user => JID}) end. diff --git a/src/graphql/user/mongoose_graphql_last_user_mutation.erl b/src/graphql/user/mongoose_graphql_last_user_mutation.erl index 21e082943d..8bc70ad1c0 100644 --- a/src/graphql/user/mongoose_graphql_last_user_mutation.erl +++ b/src/graphql/user/mongoose_graphql_last_user_mutation.erl @@ -11,7 +11,7 @@ -type last_info() :: map(). -type args() :: mongoose_graphql:args(). --type ctx() :: mongoose_graphql:ctx(). +-type ctx() :: mongoose_graphql:context(). execute(Ctx, last, <<"setLast">>, Args) -> set_last(Ctx, Args). diff --git a/src/graphql/user/mongoose_graphql_last_user_query.erl b/src/graphql/user/mongoose_graphql_last_user_query.erl index d3655e075d..11cc710d2c 100644 --- a/src/graphql/user/mongoose_graphql_last_user_query.erl +++ b/src/graphql/user/mongoose_graphql_last_user_query.erl @@ -11,7 +11,7 @@ -type last_info() :: mongoose_graphql_last_helper:last_info(). -type args() :: mongoose_graphql:args(). --type ctx() :: mongoose_graphql:ctx(). +-type ctx() :: mongoose_graphql:context(). execute(Ctx, last, <<"getLast">>, Args) -> get_last(Ctx, Args). diff --git a/src/graphql/user/mongoose_graphql_token_user_mutation.erl b/src/graphql/user/mongoose_graphql_token_user_mutation.erl index e2ee3c0702..1be13f2d92 100644 --- a/src/graphql/user/mongoose_graphql_token_user_mutation.erl +++ b/src/graphql/user/mongoose_graphql_token_user_mutation.erl @@ -11,7 +11,7 @@ -type token_info() :: map(). -type args() :: mongoose_graphql:args(). - -type ctx() :: mongoose_graphql:ctx(). + -type ctx() :: mongoose_graphql:context(). execute(Ctx, token, <<"requestToken">>, Args) -> request_token(Ctx, Args); @@ -22,12 +22,12 @@ request_token(#{user := JID}, #{}) -> case mod_auth_token_api:create_token(JID) of {ok, _} = Result -> Result; - {error, Error} -> make_error(Error, #{user => JID}) + Error -> make_error(Error, #{user => JID}) end. -spec revoke_token(ctx(), args()) -> {ok, string()} | {error, resolver_error()}. revoke_token(#{user := JID}, #{}) -> case mod_auth_token_api:revoke_token_command(JID) of {ok, _} = Result -> Result; - {error, Error} -> make_error(Error, #{user => JID}) + Error -> make_error(Error, #{user => JID}) end. diff --git a/src/mod_auth_token.erl b/src/mod_auth_token.erl index 11161c83ab..b90851939e 100644 --- a/src/mod_auth_token.erl +++ b/src/mod_auth_token.erl @@ -416,13 +416,14 @@ key_name(refresh) -> token_secret; key_name(provision) -> provision_pre_shared. -spec revoke_token_command(Owner) -> ResTuple when - Owner :: jid:jid(), + Owner :: binary(), ResCode :: ok | not_found | error, ResTuple :: {ResCode, string()}. revoke_token_command(Owner) -> - case mod_auth_token_api:revoke_token_command(Owner) of + JID = jid:from_binary(Owner), + case mod_auth_token_api:revoke_token_command(JID) of {ok, _} = Result -> Result; - {error, Error} -> Error + Error -> Error end. -spec clean_tokens(Acc, Params, Extra) -> {ok, Acc} when diff --git a/src/mod_auth_token_api.erl b/src/mod_auth_token_api.erl index be4305de10..7734116c14 100644 --- a/src/mod_auth_token_api.erl +++ b/src/mod_auth_token_api.erl @@ -10,47 +10,40 @@ -spec revoke_token_command(User) -> Result when User :: jid:jid(), - Reason :: {not_found | internal_server_error, string()}, - Result :: {ok, string()} | {error, Reason}. + Result :: {ok, string()} | {not_found | internal_server_error, string()}. revoke_token_command(User) -> - #jid{lserver = LServer} = Jid = convert_user(User), + #jid{lserver = LServer} = User, case mongoose_domain_api:get_domain_host_type(LServer) of {ok, HostType} -> - try mod_auth_token:revoke(HostType, Jid) of + try mod_auth_token:revoke(HostType, User) of not_found -> - {error, {not_found, "User or token not found."}}; + {not_found, "User or token not found"}; ok -> - {ok, "Revoked."}; + {ok, "Revoked"}; error -> - {error, {internal_server_error, "Internal server error."}} + {internal_server_error, "Internal server error"} catch Class:Reason:Stacktrace -> ?LOG_ERROR(#{what => auth_token_revoke_failed, class => Class, reason => Reason, stacktrace => Stacktrace}), - {error, {internal_server_error, "Internal server error."}} + {internal_server_error, "Internal server error"} end; _ -> - {error, {not_found, "Unknown domain"}} + {not_found, "Unknown domain"} end. -spec create_token(User) -> Result when User :: jid:jid(), - Reason :: {not_found | internal_server_error, string()}, - Result :: {ok, #{binary() => string()}} | {error, Reason}. + Result :: {ok, #{binary() => string()}} | {not_found | internal_server_error, string()}. create_token(User) -> - #jid{lserver = LServer} = Jid = convert_user(User), + #jid{lserver = LServer} = User, case mongoose_domain_api:get_domain_host_type(LServer) of {ok, HostType} -> - case {token(HostType, Jid, access), token(HostType, Jid, refresh)} of + case {token(HostType, User, access), token(HostType, User, refresh)} of {#token{} = AccessToken, #token{} = RefreshToken} -> {ok, #{<<"access">> => serialize(AccessToken), <<"refresh">> => serialize(RefreshToken)}}; - _ -> {error, {internal_server_error, "Internal server errror."}} + _ -> {internal_server_error, "Internal server error"} end; _ -> - {error, {not_found, "Unknown domain"}} + {not_found, "Unknown domain"} end. - -convert_user(User) when is_binary(User) -> - jid:from_binary(User); -convert_user(User) -> - User. From e90c45779eabdd16f89038f965e17e45c4e00495 Mon Sep 17 00:00:00 2001 From: Kamil Waz Date: Mon, 21 Nov 2022 09:02:39 +0100 Subject: [PATCH 2/2] Apply review comments --- big_tests/tests/graphql_muc_SUITE.erl | 2 +- big_tests/tests/graphql_private_SUITE.erl | 8 +++--- big_tests/tests/graphql_token_SUITE.erl | 26 +++++++++--------- big_tests/tests/graphql_vcard_SUITE.erl | 32 +++++++++++------------ src/graphql/mongoose_graphql_scalar.erl | 2 ++ src/vcard/mod_vcard_api.erl | 4 +-- 6 files changed, 38 insertions(+), 36 deletions(-) diff --git a/big_tests/tests/graphql_muc_SUITE.erl b/big_tests/tests/graphql_muc_SUITE.erl index 7aaaf7efe6..d1d17502c0 100644 --- a/big_tests/tests/graphql_muc_SUITE.erl +++ b/big_tests/tests/graphql_muc_SUITE.erl @@ -1615,7 +1615,7 @@ user_owner_set_user_affiliation_muc_not_configured(Config) -> fun user_owner_set_user_affiliation_muc_not_configured_story/2). user_owner_set_user_affiliation_muc_not_configured_story(Config, Alice) -> - Res = user_set_user_affiliation(Alice, get_room_name(), <<"ali">>, member, Config), + Res = user_set_user_affiliation(Alice, get_room_name(), <<"eddie@localhost">>, member, Config), get_not_loaded(Res). user_moderator_set_user_role_muc_not_configured(Config) -> diff --git a/big_tests/tests/graphql_private_SUITE.erl b/big_tests/tests/graphql_private_SUITE.erl index 530d9a78d1..787ed16240 100644 --- a/big_tests/tests/graphql_private_SUITE.erl +++ b/big_tests/tests/graphql_private_SUITE.erl @@ -201,11 +201,11 @@ admin_get_private(Config, Alice) -> no_user_error_set(Config) -> ElemStr = exml:to_binary(private_input()), - Result = admin_set_private(<<"AAAAA">>, ElemStr, Config), + Result = admin_set_private(<<"eddie@otherhost">>, ElemStr, Config), ?assertEqual(<<"not_found">>, get_err_code(Result)). no_user_error_get(Config) -> - Result = admin_get_private(<<"AAAAA">>, <<"my_element">>, <<"alice:private:ns">>, Config), + Result = admin_get_private(<<"eddie@otherhost">>, <<"my_element">>, <<"alice:private:ns">>, Config), ?assertEqual(<<"not_found">>, get_err_code(Result)). private_input() -> @@ -241,7 +241,7 @@ domain_admin_user_set_private_no_permission(Config, AliceBis) -> ElemStr = exml:to_binary(private_input()), Result = admin_set_private(user_to_bin(AliceBis), ElemStr, Config), get_unauthorized(Result), - Result2 = admin_set_private(<<"AAAAA">>, ElemStr, Config), + Result2 = admin_set_private(<<"eddie@otherhost">>, ElemStr, Config), get_unauthorized(Result2). domain_admin_user_get_private_no_permission(Config) -> @@ -252,7 +252,7 @@ domain_admin_user_get_private_no_permission(Config, AliceBis) -> AliceBisBin = user_to_bin(AliceBis), Result = admin_get_private(AliceBisBin, <<"my_element">>, <<"alice:private:ns">>, Config), get_unauthorized(Result), - Result2 = admin_get_private(<<"AAAAA">>, <<"my_element">>, <<"alice:private:ns">>, Config), + Result2 = admin_get_private(<<"eddie@otherhost">>, <<"my_element">>, <<"alice:private:ns">>, Config), get_unauthorized(Result2). %% Commands diff --git a/big_tests/tests/graphql_token_SUITE.erl b/big_tests/tests/graphql_token_SUITE.erl index 906141f9c0..c417ce957f 100644 --- a/big_tests/tests/graphql_token_SUITE.erl +++ b/big_tests/tests/graphql_token_SUITE.erl @@ -5,7 +5,7 @@ -import(common_helper, [unprep/1]). -import(distributed_helper, [require_rpc_nodes/1, mim/0]). -import(graphql_helper, [execute_command/4, execute_user_command/5, user_to_bin/1, - get_ok_value/2, get_err_code/1, get_unauthorized/1, get_not_loaded/1]). + get_ok_value/2, get_err_code/1, get_err_msg/1, get_unauthorized/1, get_not_loaded/1]). -include_lib("eunit/include/eunit.hrl"). @@ -57,8 +57,8 @@ domain_admin_tests() -> admin_tests() -> [admin_request_token_test, admin_request_token_test_unprep, - admin_request_token_no_user_test, - admin_revoke_token_no_user_test, + admin_request_token_no_host_test, + admin_revoke_token_no_host_test, admin_revoke_token_no_token_test, admin_revoke_token_test, admin_revoke_token_test_unprep]. @@ -190,8 +190,8 @@ domain_admin_request_token_no_permission_test(Config, AliceBis) -> % External domain user Res = admin_request_token(user_to_bin(AliceBis), Config), get_unauthorized(Res), - % Non-existing user - Res2 = admin_request_token(<<"AAAAA">>, Config), + % Non-existing domain + Res2 = admin_request_token(<<"eddie@otherhost">>, Config), get_unauthorized(Res2). domain_admin_revoke_token_no_permission_test(Config) -> @@ -202,8 +202,8 @@ domain_admin_revoke_token_no_permission_test(Config, AliceBis) -> % External domain user Res = admin_revoke_token(user_to_bin(AliceBis), Config), get_unauthorized(Res), - % Non-existing user - Res2 = admin_revoke_token(<<"AAAAA">>, Config), + % Non-existing domain + Res2 = admin_revoke_token(<<"eddie@otherhost">>, Config), get_unauthorized(Res2). % Admin tests @@ -228,13 +228,13 @@ admin_request_token_test_unprep(Config, Alice) -> ?assert(is_binary(Refresh)), ?assert(is_binary(Access)). -admin_request_token_no_user_test(Config) -> - Res = admin_request_token(<<"AAAAA">>, Config), - ?assertEqual(<<"not_found">>, get_err_code(Res)). +admin_request_token_no_host_test(Config) -> + Res = admin_request_token(<<"eddie@otherhost">>, Config), + ?assertEqual(<<"Unknown domain">>, get_err_msg(Res)). -admin_revoke_token_no_user_test(Config) -> - Res = admin_revoke_token(<<"AAAAA">>, Config), - ?assertEqual(<<"not_found">>, get_err_code(Res)). +admin_revoke_token_no_host_test(Config) -> + Res = admin_revoke_token(<<"eddie@otherhost">>, Config), + ?assertEqual(<<"Unknown domain">>, get_err_msg(Res)). admin_revoke_token_no_token_test(Config) -> escalus:fresh_story_with_config(Config, [{alice, 1}], fun admin_revoke_token_no_token_test/2). diff --git a/big_tests/tests/graphql_vcard_SUITE.erl b/big_tests/tests/graphql_vcard_SUITE.erl index 1781c02607..a1afbdf33d 100644 --- a/big_tests/tests/graphql_vcard_SUITE.erl +++ b/big_tests/tests/graphql_vcard_SUITE.erl @@ -53,20 +53,20 @@ user_vcard_not_configured_tests() -> domain_admin_vcard_tests() -> [admin_set_vcard, admin_set_vcard_incomplete_fields, + domain_admin_set_vcard_no_host, domain_admin_set_vcard_no_permission, - domain_admin_set_vcard_no_user, admin_get_vcard, admin_get_vcard_no_vcard, - domain_admin_get_vcard_no_user, + domain_admin_get_vcard_no_host, domain_admin_get_vcard_no_permission]. admin_vcard_tests() -> [admin_set_vcard, admin_set_vcard_incomplete_fields, - admin_set_vcard_no_user, + admin_set_vcard_no_host, admin_get_vcard, admin_get_vcard_no_vcard, - admin_get_vcard_no_user]. + admin_get_vcard_no_host]. admin_vcard_not_configured_tests() -> [admin_set_vcard_not_configured, @@ -199,8 +199,8 @@ user_get_others_vcard_no_user(Config) -> fun user_get_others_vcard_no_user/2). user_get_others_vcard_no_user(Config, Alice) -> - Result = user_get_vcard(Alice, <<"AAAAA">>, Config), - ?assertEqual(<<"User does not exist">>, get_err_msg(Result)). + Result = user_get_vcard(Alice, <<"eddie@otherhost">>, Config), + ?assertEqual(<<"Host does not exist">>, get_err_msg(Result)). % User VCard not configured test cases @@ -231,12 +231,12 @@ user_get_their_vcard_not_configured(Config, Alice) -> %% Domain admin test cases -domain_admin_set_vcard_no_user(Config) -> +domain_admin_set_vcard_no_host(Config) -> Vcard = complete_vcard_input(), - get_unauthorized(set_vcard(Vcard, <<"AAAAA">>, Config)). + get_unauthorized(set_vcard(Vcard, <<"eddie@otherhost">>, Config)). -domain_admin_get_vcard_no_user(Config) -> - get_unauthorized(get_vcard(<<"AAAAA">>, Config)). +domain_admin_get_vcard_no_host(Config) -> + get_unauthorized(get_vcard(<<"eddie@otherhost">>, Config)). domain_admin_get_vcard_no_permission(Config) -> escalus:fresh_story_with_config(Config, [{alice_bis, 1}], @@ -276,10 +276,10 @@ admin_set_vcard(Config, Alice, _Bob) -> ParsedResultGet = get_ok_value([data, vcard, getVcard], ResultGet), ?assertEqual(Vcard, skip_null_fields(ParsedResultGet)). -admin_set_vcard_no_user(Config) -> +admin_set_vcard_no_host(Config) -> Vcard = complete_vcard_input(), - Result = set_vcard(Vcard, <<"AAAAA">>, Config), - ?assertEqual(<<"User does not exist">>, get_err_msg(Result)). + Result = set_vcard(Vcard, <<"eddie@otherhost">>, Config), + ?assertEqual(<<"Host does not exist">>, get_err_msg(Result)). admin_get_vcard(Config) -> escalus:fresh_story_with_config(Config, [{alice, 1}, {bob, 1}], @@ -308,9 +308,9 @@ admin_get_vcard_no_vcard(Config, Alice) -> Result = get_vcard(user_to_bin(Alice), Config), ?assertEqual(<<"Vcard for user not found">>, get_err_msg(Result)). -admin_get_vcard_no_user(Config) -> - Result = get_vcard(<<"AAAAA">>, Config), - ?assertEqual(<<"User does not exist">>, get_err_msg(Result)). +admin_get_vcard_no_host(Config) -> + Result = get_vcard(<<"eddie@otherhost">>, Config), + ?assertEqual(<<"Host does not exist">>, get_err_msg(Result)). %% Admin VCard not configured test cases diff --git a/src/graphql/mongoose_graphql_scalar.erl b/src/graphql/mongoose_graphql_scalar.erl index 305e07ddc0..4731fe74df 100644 --- a/src/graphql/mongoose_graphql_scalar.erl +++ b/src/graphql/mongoose_graphql_scalar.erl @@ -41,6 +41,8 @@ jid_from_binary(Value) -> case jid:from_binary(Value) of error -> {error, failed_to_parse_jid}; + #jid{luser = <<>>} -> + {error, jid_without_user}; Jid -> {ok, Jid} end. diff --git a/src/vcard/mod_vcard_api.erl b/src/vcard/mod_vcard_api.erl index 8f0401e813..17a8d73941 100644 --- a/src/vcard/mod_vcard_api.erl +++ b/src/vcard/mod_vcard_api.erl @@ -24,7 +24,7 @@ set_vcard(#jid{luser = LUser, lserver = LServer} = UserJID, Vcard) -> {internal, "Internal server error"} end; _ -> - {not_found, "User does not exist"} + {not_found, "Host does not exist"} end. -spec get_vcard(jid:jid()) -> @@ -40,7 +40,7 @@ get_vcard(#jid{luser = LUser, lserver = LServer}) -> {vcard_not_configured_error, "Mod_vcard is not loaded for this host"} end; _ -> - {not_found, "User does not exist"} + {not_found, "Host does not exist"} end. set_vcard(HostType, UserJID, Vcard) ->