Previously, blocking all incoming iqs also cut off communication with the server, also in legitimate cases - like receiving an iq result from my own request. This is not required by XEP-0199, and it broke some features, like mod_ping. There was also a race condition - when a user sets a privacy lists, his c2s process routes two messages - iq result and a broadcast, which also sets the list in the same process. Whether the result came back or not, depended on which of these two messages was handled first.

This PR skips privacy checks for iq stanzas sent to a user by his own server account or by the server itself.

Apr 29 '20 12:04 bartekgorny

8205.1 / Erlang 22.0 / small_tests / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root / small

8205.2 / Erlang 22.0 / internal_mnesia / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big OK: 1427 / Failed: 0 / User-skipped: 157 / Auto-skipped: 0

8205.4 / Erlang 22.0 / mysql_redis / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big OK: 2651 / Failed: 0 / User-skipped: 213 / Auto-skipped: 0

8205.3 / Erlang 22.0 / odbc_mssql_mnesia / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big OK: 2656 / Failed: 0 / User-skipped: 208 / Auto-skipped: 0

8205.5 / Erlang 22.0 / riak_mnesia / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big OK: 1552 / Failed: 0 / User-skipped: 172 / Auto-skipped: 0

8205.7 / Erlang 22.0 / elasticsearch_and_cassandra_mnesia / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big OK: 328 / Failed: 0 / User-skipped: 28 / Auto-skipped: 0

8205.6 / Erlang 22.0 / ldap_mnesia / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big OK: 1366 / Failed: 0 / User-skipped: 218 / Auto-skipped: 0

8205.9 / Erlang 21.3 / pgsql_mnesia / 544818e2e1a4073193648ea77ed2f6906ee9656e Reports root/ big / small OK: 2669 / Failed: 0 / User-skipped: 195 / Auto-skipped: 0

Apr 29 '20 12:04 mongoose-im

Codecov Report

Merging #2724 into master will increase coverage by 2.94%. The diff coverage is 83.33%.

@@            Coverage Diff             @@
##           master    #2724      +/-   ##
==========================================
+ Coverage   76.32%   79.27%   +2.94%     
==========================================
  Files         368      368              
  Lines       30510    30516       +6     
==========================================
+ Hits        23287    24191     +904     
+ Misses       7223     6325     -898

Impacted Files	Coverage Δ
src/mongoose_privacy.erl	`90.47% <83.33%> (-2.86%)`	:arrow_down:
...c/global_distrib/mod_global_distrib_server_mgr.erl	`79.14% <0.00%> (-2.46%)`	:arrow_down:
src/rdbms/mongoose_rdbms.erl	`68.87% <0.00%> (-1.54%)`	:arrow_down:
src/ejabberd_local.erl	`72.18% <0.00%> (-1.51%)`	:arrow_down:
src/mod_roster.erl	`77.75% <0.00%> (-0.48%)`	:arrow_down:
src/ejabberd_sm.erl	`75.31% <0.00%> (-0.32%)`	:arrow_down:
src/mod_muc_log.erl	`77.69% <0.00%> (ø)`
src/mod_muc_room.erl	`77.41% <0.00%> (+0.05%)`	:arrow_up:
src/pubsub/mod_pubsub.erl	`72.34% <0.00%> (+0.05%)`	:arrow_up:
src/mod_muc.erl	`77.77% <0.00%> (+0.52%)`	:arrow_up:
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update fe5f5cb...5c2974c. Read the comment docs.

Apr 29 '20 12:04 codecov[bot]

Previously, blocking all incoming iqs also cut off communication with the server, also in legitimate cases - like receiving an iq result from my own request. This is not required by XEP-0199, and it broke some features, like mod_ping. There was also a race condition - when a user sets a privacy lists, his c2s process routes two messages - iq result and a broadcast, which also sets the list in the same process. Whether the result came back or not, depended on which of these two messages was handled first.

XEP-0199 is about Ping, the one for privacy lists is XEP-0016, which describes the IQ blocking feature in the following way:

Server-side privacy lists enable a user to block incoming IQ stanzas from other entities based on the entity's JID, roster group, or subscription status (or globally).

As an entity is a user OR any service, I don't see a reason to exclude any IQ stanzas from the blocking feature. I think that if we want to change this, we would need to get an opinion on the XSF mailing list first.

I am aware of the fact that Example 44 explicitly mentions "users", but the previous examples just mention "entities".

Regarding the race condition, could we add a test for it first and make sure this test fails randomly to prove that it really exists?

Apr 30 '20 10:04 chrzaszcz

XEP-0199 is about Ping, the one for privacy lists is XEP-0016,

Of course, my mistake.

As an entity is a user OR any service, I don't see a reason to exclude any IQ stanzas from the blocking feature. I think that if we want to change this, we would need to get an opinion on the XSF mailing list first.

Yup, this is exactly why I expected a discussion. The XEP doesn't specifically state which IQs should be blocked and which shouldn't, so there is a need for interpretation. The purpose of privacy lists is to avoid unwanted communication, like malicious users or servers. Blocking IQs from my own server in effect breaks some other xeps (like mod_ping), but also RFC 6120 (which says that the server should reply to an IQ with a result or an error) and RFC 6121 (roster push). Even more, it breaks the same XEP, which specifically states that if you edit a privacy list you should receive a iq result.

Having said that, I'm not opposed to consulting XSF if there is a need for it.

Regarding the race condition, could we add a test for it first and make sure this test fails randomly to prove that it really exists?

Not really, because the messages always arrived in the same order, so technically speaking it was not a race condition since one horse was winning it all the time. However, a minor change in code layout was sufficient to reverse the order, so the race must have been pretty tight.

Apr 30 '20 10:04 bartekgorny

Yup, this is exactly why I expected a discussion.

Let's have a discussion then, I think the best place for it would be a tech meeting, but the opinion on the XSF mailing list would be also very valuable.

Apr 30 '20 10:04 chrzaszcz

@bartekgorny, @NelsonVides, @chrzaszcz: Have you progressed on this PR?

Sep 21 '21 01:09 Neustradamus

@chrzaszcz @NelsonVides Can we get back to this one? To me it seems still valid, though if you guys decide otherwise we can close this PR.

Apr 08 '24 13:04 bartekgorny

privacy blocking iq should not stop stanzas sent by my server to me

Codecov Report