OF-2244 Store presence stanza for offline subscription requests #1855

dwd · 2021-06-25T10:27:16Z

This is best reviewed by commit:

First, ExternaliableUtil is extended to allow direct serialization of XML, allowing easy extension of Externalizable objects to serializing XML.
Next, the RosterItem and DefaultRosterItemProvider are extended to support storage of the Presence stanza causing the request.
Finally, PresenceSubscribeHandler and PresenceUpdateHandler are extended to store and use this stored stanza instead of constructing their own.

I've done this by imbuing ExternalizableUtil with the ability to serialize and parse XML. This then allows lower-level strategies to be entirely unaware of the nature of the string. Higher-level code can then serialize XML, and trivially manage serializing Stanzas.

This extends the RosterItem to store a Presence stanza for the case of new subscription requests. This is serialized and stored in the database as usual. There are getters and setters for the stored stanza, but also a utility getter which can create a dummy stanza as needed.

This change means that subscription requests are stored and resent verbatim on login.

guusdk · 2021-06-25T15:24:32Z

The code uses a new column in the database table ofRoster named stanza, but does not provide a database upgrade script to add this new column.

xmppserver/src/main/java/org/jivesoftware/util/cache/ExternalizableUtil.java

xmppserver/src/main/java/org/jivesoftware/openfire/roster/DefaultRosterItemProvider.java

The solution for OF-2244 is based on storing the stanza in a database table (ofRoster). This commit introduces the database structure in the installation and upgrade scripts to hold this data.

SAXReader instances, used to parse XML, are created all over the place. They need a bit of specific configuration to circumvent vulnerabilities. Often, their lifecycle needs to be maintained, as their creation is resource intensive. All of this should be put in one central utility, instead of live as re-invented wheel fragments all over the place.

This commit replaces individual snippets that create/use a SAXReader instance with one that's provided by the new utility implementation. I'm aware that this undoes a bit of the change that was implemented as part of OF-2259. As part of those changes, the same SAXReader instance was used to parse all data for a collection of items (instead of recreating a new parser for each item). In the changes I'm making here, this is replaced by scheduling a task in an ExecutorService for each item, blocking for it to be processed by a reusable parser, before moving to the next item. Although more involved, this should still perform a lot better than recreating a parser for each item.

guusdk · 2021-06-28T19:49:23Z

After discussing the PR with @dwd offline, I've made some changes. Notably, I've raised OF-2269, and provided a fix for that issue in this PR, then modified the other changes in this PR to make use of that fix (ideally, I'd rebase the entire thing for OF-2269 to go 'before' OF-2244, but it is late, and really, what's the point?)

In @dwd's provided PR, a somewhat odd usage of a SAXReader pool was included. In answering my remarks on that, Dave mentioned that this was used all over the code, and that we'd ideally centralize the SAXReader management. That's what I attempted to do with OF-2269.

Note that this somewhat affects the changes in OF-2259 (which replaced creating a new SAXReader for every message read from a database table with re-usage of the same entity, removing the overhead of SAXReader construction, and thus speeding up the process considerably). With the changes in this PR, that's replaced scheduling a new task per database row, which would then be processed by a select group of reusable SAXReaders. There's a bit more overhead in that, as compared to the solution that hardcodedly used one instance, but as compared to creating a new instance per database row, benefits should still be considerable.

guusdk · 2021-06-28T19:51:34Z

Trying to rope in @GregDThomas as he's good at spotting the mistakes in changes like these :)

GregDThomas

A few comments, mostly opinions rather than identified flaws.

distribution/src/database/openfire_oracle.sql

xmppserver/src/main/java/org/jivesoftware/openfire/crowd/CrowdVCardProvider.java

xmppserver/src/main/java/org/jivesoftware/util/SAXReaderUtil.java

The rewrite of SAXReader utilization introduces a new source of potential InterruptExceptions. This occurs when 'someone' wants to the thread to stop execution. By catching this as a regular exception, the code at hand will handle that scenario as appropriate, but will also hide the fact that 'someone' wants to stop the thread from its own invocator. To let the caller know that someone wants to stop the thread from execution, the interrupt flag should be set. See https://programming.guide/java/handling-interrupted-exceptions.html for more rationale.

This commit ensures that the stanza of a presence subscribe request is actually stored in the database (as opposed to only be added in memory, without that change being pushed to the database. Additionally, the stanza should: - only be present in the roster item for the receiving entity - be removed from the database after the request has been rejected or accepted (to not fill up the database with stale data)

guusdk · 2021-06-30T19:06:12Z

I've provided an automated test to the Smack Integration Test framework for the behavior around having extension elements in subscription stanzas in igniterealtime/Smack#481

Dave Cridland added 4 commits June 25, 2021 11:13

Store and retrieve subscription request stanzas

6685d4c

This change means that subscription requests are stored and resent verbatim on login.

Actually remember the presence stanza need setting!

aa0354d