Generated XEP-0388 encoders serialize child elements in schema-invalid order

[rallep71]

The generated XEP-0388 codec serializes several child elements in an order that does not match the
XEP-0388 schema. The underlying causes are a reference-ordering problem in the fast_xml generator
and an incorrect sasl2_continue declaration order in xmpp/specs/xmpp_codec.spec.

thx

[badlop]

Nice finding! Can you mention what's the ordering problem in fast_xml and the incorrect declaration order in the xmpp library?

[rallep71]

Hi, if I manage to get round to it today, I’ll put together a more detailed explanation. It’s also linked to the bug I’ve reported – though it isn’t public yet; prefix is looking into it. You could always enquire about it. It should get the go-ahead from you lot first.

[rallep71]

Here are the details.

The first problem is in fxml_gen:group_refs/1. It groups references in a dict
and returns them through dict:to_list/1, so the declaration order from
#elem.refs is lost. make_elem_enc_fun/5 then folds over that result and reverses
the accumulator. The generated sibling order therefore depends on the dict
order instead of the order declared in the codec specification.

With the current generated xep0388.erl this results in, for example:

authentication: inline before mechanism
success: authorization-identifier before additional-data
continue: text, tasks, additional-data

XEP-0388 requires:

mechanism(s), then inline
additional-data, then authorization-identifier
additional-data, tasks, text

The second issue is independent of that generator problem:
sasl2_continue in specs/xmpp_codec.spec currently declares additional-data,
text, tasks. It should declare additional-data, tasks, text.

The source fix should be made in fast_xml and xmpp_codec.spec, followed by
regeneration. Editing generated xep0388.erl alone would only hide the problem.

processone/fast_xml#53 processone/xmpp#112 thx

[prefiks]

Looking at those linked pull requests, those changes make sense. This will likely change order inside other stanzas as well, which i don't think this will change handling in ejabberd, but it may possibly be issue in test suite if some tests aren't using xmpp decoder, and are comparing to raw data that could be changed by this.

[rallep71]

Hello and thank you for your reply,

I only realised this whilst integrating and debugging SASL2 and Bind2 into the XMPP client.
I hadn’t considered that this might cause problems with other dependencies in the test suite.
What do you suggest? Right, I’ll look into it a bit more deeply to see how these proposed changes might affect other parts of the system.

[prefiks]

Don't worry about this, we will just fix tests when this happen, i just left that as a note to myself or badlop when this will be merged

[badlop]

As the fixes are already available in rallep library forks, I'll use them in my ejabberd fork to run the tests, and check if ejabberd tests need some update before merging :)

[badlop]

Right, the fixes in fast_xml produce several changes in xmpp when running "make spec xdata".

The good news: ejabberd passes all tests after applying fast_xml and xmpp patches

Just a small note:

The source fix should be made in fast_xml and xmpp_codec.spec, followed by regeneration.

It would be great if you can split your fast_xml commit into two:

1. First commit including your manual changes, the ones that you made yourself
2. Second commit with the automatic result of regeneration, something like "Result of running make spec" or whatever

[rallep71]

Good to hear that ejabberd passes the full test suite with both patches applied.

Yes, that split makes sense. I’ll update processone/fast_xml#53 so the first commit contains the hand-written generator and regression-test changes, while the second contains only the regenerated src/fxmlrpc_codec.erl. I’ll note REBAR=rebar3 make spec as the regeneration command.

[rallep71]

It’s sorted, but my status is still showing as ‘pending’,

[badlop]

All is merged and tests are passed... I mean the tests that cover 35% of ejabberd source code. The next ejabberd release notes should mention this fix, in case there is somewhere some code that depends on the previous order.

Something like this, does it look correct?

---

Fixed the ordering of some XML child elements

There was a reference-ordering problem in the fast_xml generator, it generated XML encoders that could reorder child elements in a different order from the one declared in the codec specification. From now, the order is the one declared in the codec specification. See details in processone/fast_xml#53

Additionally there was an incorrect sasl2_continue declaration order in the xmpp erlang library: it declared "additional-data, text, tasks". Now it follows the ordering defined in XEP-0388 schema: "additional-data, tasks, text". See details in processone/xmpp#112

Acknowledgments

• rallep71 for the fixes in fast_xml and xmpp XML child ordering

[rallep71]

Yes, that looks good to me. It describes both parts of the fix correctly, and mentioning it in the release notes makes sense
in case anything relied on the old generated order. The acknowledgement is fine too. Thanks for merging both changes and
running the ejabberd tests.

THX