Repository navigation
Fix/mod privilege - #4651
Open
annelinlol wants to merge 3 commits into
Open
Fix/mod privilege#4651annelinlol wants to merge 3 commits into
annelinlol wants to merge 3 commits into
Conversation
…e#4472) component_send_packet/1 dropped an IQ that a component sent to a bare JID whenever mod_privilege had no privileges recorded for that component on the server of the destination, before it checked whether the IQ was a XEP-0356 <privileged_iq/> at all: the "Component seems disconnected" clause matched ahead of the unprivileged_iq one. a4062f6 meant to route such plain IQs as they are, but because of the clause order that only happened when the destination's server had the component's privileges recorded. So a gateway's IQs to a service that is not a host (upload.<domain> for an HTTP File Upload slot, pubsub.<domain>), or any IQ from a component that was granted nothing, were dropped without an answer. This was broken since the IQ permission support in 81906b7 (24.10). Match unprivileged_iq first, so an IQ that wraps no <privileged_iq/> goes as it is whatever its destination. Wrapped IQs are handled exactly as before. The test configuration now loads mod_privilege with no privileges, so the component group runs with it (test_component_send times out without this fix). The new test_component_privilege checks plain IQs to the host and to upload.localhost before and after the component is granted a namespace, that a privileged IQ in that namespace is forwarded, and that one in another namespace is still dropped.
component_connected/1 and component_disconnected/2 walk all vhosts and look up the mod_privilege process of each with gen_mod:get_module_proc_check/2. Since 602fe1c (26.09) that function uses binary_to_existing_atom/2, so on the first vhost that does not load mod_privilege the call fails with badarg, the hook logs a crash and the loop stops: no vhost listed after it records the component's privileges on connect, nor forgets them on disconnect. Walk only the vhosts where the module is loaded, as mod_antispam does. The new test_component_privilege_vhosts loads mod_privilege on mnesia.localhost only, which comes after localhost and configtest.localhost in the test hosts, runs component_connected and expects the <privilege/> message from mnesia.localhost. Without this fix it times out.
component_send_packet/1 inspects every IQ a component sends to a bare JID
whose type is not error, and get_iq_encapsulated_details/1 matches only an
IQ with exactly one child. A result with no payload, such as a push
service's answer to mod_push's publish or to a ping, has none, so the hook
crashes with function_clause on every such answer:
Hook component_send_packet crashed when running mod_privilege:component_send_packet/1:
** exception error: no function clause matching
mod_privilege:get_iq_encapsulated_details({iq,<<"rr-...">>,result,<<>>,
{jid,<<>>,<<"push.example.org">>,...
The IQ is still routed (the hook's accumulator survives the crash), so the
only damage is an error logged per answer, but a push service answers
every push.
Only a get or a set can carry a privileged IQ, so the clause now matches
those two types; a result or an error goes through as it is, as the
unprivileged ones did already.
The new test_component_privilege_result passes a payload-less result and
an error through the hook and expects them back unchanged; without the
fix it fails with function_clause.
CT_BACKENDS=agnostic make test-component: all 16 tests pass.
Collaborator
|
Hi @annelinlol, many thanks for your contribution! In order for us to evaluate and accept your PR, we ask that you sign a contribution license agreement. It's all electronic and will take just minutes. |
p1bot
added
the
cla-missing
Collaborator
|
You did it @annelinlol! Thank you for signing the ProcessOne Contribution License Agreement. We will have a look at your contribution! |
p1bot
removed
the
cla-missing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
mod_privilege: Record privileges on every vhost that loads it
When an external component connects,
mod_privilegeis supposed to record the privileges granted to it on each vhost that loads the module and send it a<privilege/>message from each of them. Since 26.09 that stops at the first vhost that does not loadmod_privilege: thecomponent_connectedhook crashes there withbadarg, and every vhost listed after it inhostsnever records the component's privileges. The component's privileged IQs, roster access and so on are then refused on those vhosts, and the log shows:component_disconnectedfails the same way, so privileges recorded on such vhosts are never forgotten either.Why
component_connected/1andcomponent_disconnected/2iterate overejabberd_option:hosts()and look up each vhost's process withgen_mod:get_module_proc_check/2. Since 602fe1c ("Add gen_mod:get_module_proc_check()", 26.09) that function usesbinary_to_existing_atom/2, so it raisesbadargfor a vhost where the module was never started. The hook is global and runs the whole loop in one call, so one such vhost aborts the rest. Before 602fe1c the process name was created withbinary_to_atom/2, and the cast to a nonexistent process was harmlessly ignored.How
Both functions now walk only the vhosts where
gen_mod:is_loaded(Host, mod_privilege)holds, asmod_antispamdoes for its own per-host loops.Tests
New
test_component_privilege_vhostsin thecomponentgroup: it loadsmod_privilege(grantingurn:xmpp:ping) onmnesia.localhostonly, which comes afterlocalhostandconfigtest.localhostin the test configuration'shosts, runs thecomponent_connectedhook and expects the<privilege/>message frommnesia.localhost. It then stops the module and restores the configuration, so the rest of the suite runs as before.CT_BACKENDS=agnostic make test-component: all 16 tests pass with the fix. Without it,test_component_privilege_vhostsfails on a timetrap timeout (the hook crash above is logged) and the 2 tests after it in the sequence are skipped.