We have to competiting patch proposals in order to fix OF-810.
https://github.com/surevine/Openfire/commit/95ae05dbd74d4993a74a0c7ca9a4cb23bb8a 2c67
https://github.com/igniterealtime/Openfire/commit/237cf9c4a98a78b63fda498810023b 38c29b1e44
Without going further into reasons why we have two different solutions. I’d like to ask the authors of the commits to shortly explain the advantages of their commit and the disadvantages of the other commit.
Ok.
Advantage of commit 2:
It takes into account in more detail what is specified in Extensible Messaging and Presence Protocol (XMPP): Instant Messaging and Presence and in Extensible Messaging and Presence Protocol (XMPP): Instant Messaging and Presence
Whether a message should be bounced, is handled in OfflineMessageStrategy.java, which has the advantage, that the user settings of offline-messages.jsp are respected.
It has better documentation.
Disadvantage of commit 1:
No documentation.
A message is always bounced (when in the ‘else’ branch), even if an OF admin chooses “Drop” in offline-messages.jsp. It does not respect both options as specified:
the server MUST either (a) silently ignore the stanza or (b) return an error stanza to the sender
Answering criticism first:
Seriously, if you’re going to argue that there’s no documentation, you’d really best argue better than that. It’s 12 lines, most of which are simply “bounce the message”. The commit message is sufficient explanation, I thought.
A message is always bounced if the messages are not directed to offline storage; if they are, then they are handled according to the existing setting for offline messages. There didn’t seem much point in handling the offline message strategy as a general option in handling bounces – but in any case, this suggestion wasn’t raised with me until after my patch had been discarded by a committer.
However, my patch does fail to bounce correctly in the case where the message is directed to offline storage directly - this isn’t much of a problem, as it happens, and doesn’t contradict RFC 6121, but I dislike it. It’s also been noted to me that it’s mixing tabs and spaces; I’ll correct that as well if there’s interest.
The advantage of the patch is that it’s a 12 line change, and is consistent with what the RFC requires.
As to what’s wrong with the alternative…
It’s nearly four times the length.
It doesn’t actually document the key feature of the patch, which is that it aims to centralizes bounce handling into one codepath.
I don’t think it works.
The common case that I actually needed to solve was where a client drops offline and a remote MUC doesn’t (for some reason) receive the unavailable presence. A message from the MUC is of type=‘groupchat’ and sent to the (now offline) full jid.
In my code, it will generate a bounce due to the 12 lines I added. This is vital, because it signals to the remote MUC that it should remove the stale occupant.
In Christian’s, however, I don’t think it ever bounces, no matter what the setting is, and never passes into offline message handling. This is because the conditional on (new) line 254 evaluates to false, and the flow of control exits the method, dropping the stanza. There’s simply no case I can see in which the full-jid handling that this patch presumably claims to address can ever result in a bounce.
Assuming I’m right, this suggests rather strongly that the code committed directly to the trunk of the primary repository had not been tested at all.
What I think would be needed to make this work is an else clause calling messageStrategy.storeOffline() at line 256; but what this is actually doing is calling storeOffline() only on stanzas which are never stored offline, which to my mind calls into question the entire strategy.
Sorry, re-reading that, it might suggest that lack of documentation had been raised as an issue before my patch had been discarded - that’s not the case, the only criticism raised was that I hadn’t understood the logic involved in RFC 6121; I believe I understand it reasonably well.
- There didn’t seem much point in handling the offline message strategy as a general option in handling bounces
But this is where it’s done in Openfire. The specification says “ignore or return error”. Both alternatives can be set by an administrator and is decided in OfflineMessageStrategy.java.
Your patch always returns an error without having the option to ignore.
In Christian’s, however, I don’t think it ever bounces, no matter what the setting is, and never passes into offline message handling. This is because the conditional on (new) line 254 evaluates to false, and the flow of control exits the method, dropping the stanza.
You are right. I missed an “else messageStrategy.storeOffline((Message) packet);” (as you said)
I am sorry for this little mistake, but that’s also why I asked you to review and discuss it. E.g you could have given me this hint in yesterday’s chat.
but what this is actually doing is calling storeOffline() only on stanzas which are never stored offline, which to my mind calls into question the entire strategy.
True - it’s not stored offline - but at least it decides about the bounce-or-ignore strategy. It only happens to be named OfflineMessageStrategy which might be confusing.
the only criticism raised was that I hadn’t understood the logic involved in RFC 6121
Actually I only wanted to understand the sections in RFC 6121 myself and therefore asked you questions to which sections your patch is referring.
My suggestions is to just add the missing line.
OK, so an option called “Offline Message Policy” should apparently control how bounces work. Let’s go along with that for a moment, even if I think that’s a stupid idea. Let’s further assume there is a sensible use-case for dropping bounces throughout Openfire as a general option. Also, let’s assume you told me this was essential for the change to be acceptable.
Let’s ignore the fact I told you “you’re dropping groupchat (for example) rather than sending an error” - obviously that wasn’t a clear enough “hint” for you.
Let’s ignore that your way of understanding RFC 6121 seems to be by saying things like “OF should not try to deliver to bareJID”, and “we then should store it or bounce it with error, but NOT try bare JID” (emphasis yours). I was really quite patient in explaining, across 25 minutes, not only what RFC 6121 specifies, but also why I was, in fact, right.
In fact, let’s ignore that you threw away a contribution made in good faith without any reason given until after the fact.
You know what, forget all of it. Pretend I never suggested the patch “might also be useful”.
What you seem to be missing is that my job is basically to keep those customers running on Openfire happy, and my options here are really simple:
Maintaining a private branch without any effort to push fixes upstream.
Making the additional effort to ensure the patches are in a form acceptable to upstream, and adjusting them according to feedback.
I’d much rather do (2). It’s not what we’re actually paid to do, but if all goes right we gain a lot from it, and so do our customers. There’s a trade-off though. I can’t tell a customer, “Oh, look, I fixed this bug which caused a problem in this case, that cost you an hour of time – oh, and we pushed the patch upstream, and spent an hour and a half arguing about it. We’ll invoice you for it all later.” I don’t mind spending some of my own time on these things, either.
Nor do I expect every patch to be accepted, but in general, if the patches are going to get rejected, it’d be nice to know why, and if you’re asking me to review a patch of yours that you’ve done in place of mine – which is frankly pretty rude – then when I say “you’re dropping groupchat”, the ideal respons eis not going to be “I don’t drop groupchat.”
In general, your attitude toward me has consistently been that I have no idea what I’m talking about, and haven’t got a clue. I do not claim – would never claim – to have all the answers and always be right, but I believe I have generally demonstrated within the XMPP community over the past few years that my comments shouldn’t be ignored out of hand – by default at least.
I really do not need, or want, to spend any more of my time, or that of my employer, on this.
OK, so an option called “Offline Message Policy” should apparently control how bounces work. Let’s go along with that for a moment, even if I think that’s a stupid idea. Let’s further assume there is a sensible use-case for dropping bounces throughout Openfire as a general option.
Calling it Offline Message Policy was not my idea. It’s there for many years. But I think the name is ok.
Dropping bounces (aka the ignore option) is a valid option as per the specification, so it should be respected.
your way of understanding RFC 6121
I’ve read the relevant sections for the first time. Pretty normal that there’s some need for questions/discussion/clarification and that things can get misunderstood after the first read.
I’d much rather do (2). It’s not what we’re actually paid to do,
Yes, of course. I appreciate, that you revealed a flaw in the routing logic and made it public.
Nobody gets paid for this kind of stuff. I am also volunteering my time on Openfire in the evenings.
You know, I only wanted to improve upon your patch, not to reject it. I’ve understood it as suggestion, and not to blindly accept it as is without reviewing and/or discussing it. Especially if it comes from somebody who has never contributed anything else to OF (at least to my knowledge).
So, if nobody objects within a few days, I will add the missing line.
OK, so an option called “Offline Message Policy” should apparently control how bounces work.
It’s there for the cases where the RFC states that “the server MUST either (a) silently ignore the stanza or (b) return an error stanza to the sender”, ie. where it’s up to the server to decide if it should bounce or drop.
Let’s further assume there is a sensible use-case for dropping bounces throughout Openfire as a general option.
Droping packets that must be bounced is not the intention of the “Offline Message Policy”.
We don’t want to loose any potential contributors. I think your frustration is mostly a result of a misunderstandings. First, CSH should have never pushed his commit into the primary repo’s master branch for review. Developers familar with git, like you, will take that as “this is the commit that won, all others are not so good and thereby rejected”. But when I spoke to CSH he told me that it was not his intention, he’s just inexperienced with git.
The second misunderstanding happended when you two discussed the interpretation of RFC 6121 server rules for processing xml stanzas. From the backlog it appears that csh was under the wrong assumption thatchat messages addressed to a full JID should be dropped or bounced and not delivered to a different but available resource. XMPP routing logic can be quite complex and hard to grasp for newcommers. Please have a bit patience.
We only recenlty migrated from SVN to git. And it appears that most openfire developers are not familar with it and we didn’t establish a policy regarding code review and contribution. I hope that this will change in the near future to avoid such situations.
Dropping bounces (aka the ignore option) is a valid option as per the specification, so it should be respected.
Careful with the terms, bouncing means that the message is replied with an error. This is not the same as ignoring it (aka. “drop”).
I’d like to also stress the fact that RFC 6121 mentions cases where a server must not drop a message, but instead bounce it. This cases must not be handled by the Offline Message Policy, messages to which the RFC requires a bounce must result in a bounce, no matter what the Offline Message Policy setting is.
I’ve understood it as suggestion, and not to blindly accept it as is without reviewing and/or discussing it. Especially if it comes from somebody who has never contributed anything else to OF (at least to my knowledge).
Every change to Openfire’s core should get reviewed, no matter from who the change is.
So, if nobody objects within a few days, I will add the missing line.
Please update the ‘of810’ branch in openfire’s primary repo (or in your personal repo) with an updated commit and post the link to the commit here for review.
Careful with the terms
Yes, I know… just re-using Dave’s terms.
Please update the ‘of810’ branch in openfire’s primary repo (or in your personal repo) with an updated commit and post the link to the commit here for review.
This isn’t sufficient; you need to remove the option checking and always bounce. Otherwise, if the offline strategy “type” isn’t set to bounce, it won’t bounce when it should.
And yes, I understand what the RFC says on this issue, but I also understand why it says that. Even though I’ve never contributed to OF before, it’s true.
The reason servers might not return an error in this case is either:
a) Because it’s really hard to do.
b) Because they don’t return an error in other cases, and want to match.
This (b) is the key here. If the message is bounced when users don’t exist, you want to bounce here, too. Openfire will bounce if the user doesn’t exist, therefore you should bounce throughout. To put it another way, you’ve introduced a security problem.
This is also probably similar why the offline message strategy options exist in Openfire - you might not want to give away the fact you’ve filled it, because that starts to indicate how many messages were in it before you flooded it.
Finally, I still think the majority of this patch is in the wrong place - I think the bulk of the handling ought to be placed directly in routingFailed(), not in storeOffline().
you need to remove the option checking and always bounce. Otherwise, if the offline strategy “type” isn’t set to bounce, it won’t bounce when it should.
Maybe I am still misunderstanding the RFC… I assume you mean that part (line 120)!?:
if (type == Type.bounce) {
bounce(message);
// Here should be an additional return statement, I just realized.
} else {
return;
}
It’s there for the 8.5.3.2.1. Message logic (full JID):
For a message stanza of type “normal”, “groupchat”, or “headline”, the server MUST either (a) silently ignore the stanza or (b) return an error stanza to the sender, which SHOULD be .
Right. I understand you think you should give the administrator a choice. I’m suggesting that this is wrong - you’re the implementor, you get to pick. If an administrator has some reason for not wanting to bounce, they have the code, but having Openfire not bounce at this point would mean the behaviour changed depending on whether the user exists or not. This would be bad.
I understand you think you should give the administrator a choice.
Yes, that’s how I understand it, especially if Openfire already offers an administrator multiple options like “drop”, “bounce”, “bounce if maximum size exceeded”, etc.
An administrator would assume, that if he chooses “drop”, that the message is not bounced.
Concerning the “account-does-not-exist” case: Your concern is, that it would bounce then and in the “resource-is-not-available”-case it would be ignored?
I don’t consider this to be a problem, especially if an administrator chose “drop” for the offline case, but maybe should be discussed with others.
Maybe OF should also have an option for the “account-does-not-exist” case (if it doesn’t already), because RFC 6121 allows for both options (ignore or bounce).