Relegate message retraction to xep-0424 instead of using custom one. #1047

woj-tek · 2021-03-31T17:13:59Z

As per, rather small, thread [XEP-0407] - Message retraction question on Standards I changed MIX specification (XEP-0407) and replaced custom retraction mechanism with reference to XEP-0424

horazont · 2021-04-02T11:24:24Z

Thread: https://mail.jabber.org/pipermail/standards/2021-March/038256.html
Contains author approval.

Kev · 2021-04-02T11:33:26Z

Can I have a chance to review as a whole when I’m back at work next week before it’s merged/published please? This is slightly more involved than it appears at first glance and I’d rather think it through first rather than potentially reverting it later. /K

…

On 2 Apr 2021, at 12:24, Jonas Schäfer ***@***.***> wrote: Thread: https://mail.jabber.org/pipermail/standards/2021-March/038256.html <https://mail.jabber.org/pipermail/standards/2021-March/038256.html> Contains author approval. — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#1047 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAC5CXKI5FSJG3IIVKVBETTGWSPLANCNFSM42FDR2HA>.

horazont · 2021-04-02T13:40:24Z

@Kev Yes, no problem. I will tag it as Needs Author. Please report back when you’re done.

Kev · 2021-05-10T14:32:46Z

Sorry for the delay, thanks for your patience. I've not been able to think of a reason moving to 424 in principle wouldn't work, but I think there might be bits missing from this PR:

Under the previous text it was hopefully obvious how to construct the ID (from the MAM service of the MIX - which it needs to be), while 424 usually works on the origin-id, which wouldn't be appropriate here.
The old protocol stored a note of who had retracted the item in the retraction, which I think is missing in 424.
I think with the removal of the examples (which arguably shouldn't be the only place anyway, I realise), addressing of the retraction was obvious, which might be less true with the examples gone.
It's probably worth keeping an example in place even while refering to 424.
If delegating to 424, it probably matters which version of 424 needs to be supported.

horazont · 2021-05-11T16:53:38Z

@Kev Thanks for finding the time to provide feedback!

@woj-tek Can you please take a look at that feedback and let me know how you want to proceed?

woj-tek · 2021-05-12T08:38:09Z

@horazont I'll try to address @Kev 's comments and refine this PR.

…mbstones, add examples

woj-tek · 2021-05-14T17:09:33Z

I just pushed commit that should address @Kev 's concerns:

Under the previous text it was hopefully obvious how to construct the ID (from the MAM service of the MIX - which it needs to be), while 424 usually works on the origin-id, which wouldn't be appropriate here.

Somewhat of a OT and rant, but origin-id is just awful. Having server generated stanza-id would fix a lot of issues…

I added explicit comment about using stanza-id here, though I'm not sure it doesn't violates/contradicts 424: "by referring to its Unique and Stable Stanza IDs (XEP-0359) [5] origin ID."

The old protocol stored a note of who had retracted the item in the retraction, which I think is missing in 424.

Shouldn't all messages have from? So even if message is retracted it's meta (to/from) remains intact? (though, it's missing from 424's examples - candidate for another PR?)

I think with the removal of the examples (which arguably shouldn't be the only place anyway, I realise), addressing of the retraction was obvious, which might be less true with the examples gone.
It's probably worth keeping an example in place even while refering to 424.

Restored and adjusted examples.

If delegating to 424, it probably matters which version of 424 needs to be supported.

Included particular features - is that ok?

horazont · 2021-05-25T15:01:55Z

@woj-tek Thanks for adapting your work! I pinged @Kev, hopefully he’ll find the time for another round soon.

Kev

Sorry for the ludicrous delay in replying. I have no excuse.

Under the previous text it was hopefully obvious how to construct the ID (from the MAM service of the MIX - which it needs to be), while 424 usually works on the origin-id, which wouldn't be appropriate here.
Somewhat of a OT and rant, but origin-id is just awful. Having server generated stanza-id would fix a lot of issues…

Yes, we have to use the MIX channel generated stanza-id here, not the origin-id (or any other stanza-id)

I added explicit comment about using stanza-id here, though I'm not sure it doesn't violates/contradicts 424: "by referring to its Unique and Stable Stanza IDs (XEP-0359) [5] origin ID."

This switches to using stanza-id, but it's still using a stanza-id stamped by the original sender, not by the MIX channel, unless I'm badly misreading it.

The old protocol stored a note of who had retracted the item in the retraction, which I think is missing in 424.
Shouldn't all messages have from? So even if message is retracted it's meta (to/from) remains intact? (though, it's missing from 424's examples - candidate for another PR?)

Whether the message has a from is different from storing who it was who issued the retraction, no?

I think with the removal of the examples (which arguably shouldn't be the only place anyway, I realise), addressing of the retraction was obvious, which might be less true with the examples gone.

It's probably worth keeping an example in place even while refering to 424.

Restored and adjusted examples.

Thanks.

If delegating to 424, it probably matters which version of 424 needs to be supported.
Included particular features - is that ok?

I don't think that's enough, is it? If someone implements MIX they need to know which version of 424 to read to implement it.

Kev · 2021-08-05T13:56:46Z

xep-0407.xml

 </message>

 <message from='hag66@shakespeare.example/UUID-a1j/7533'
         to='coven@mix.shakespeare.example'
         id='92vax143g'>
-  <retract id='77E07BB0-55CF-4BD4-890E-3F7C0E686BBD' xmlns='urn:xmpp:mix:misc:0'/>
+  <apply-to id="a568706e-867b-49f1-a31b-e428ba9cece1" xmlns="urn:xmpp:fasten:0">


This is using the id as sent by the original client, and it needs to be an id as assigned by the mix channel, I believe (I said this in the first round of comments)

I think this could be solved if apply-to would not only refer to the id, but also to the 'by' value of the stanza-id element. For example

<apply-to xmlns="urn:xmpp:fasten:0" id="a568706e-867b-49f1-a31b-e428ba9cece1" by='hag66@shakespeare.example'>

See also my 2019 mail to @standards: https://mail.jabber.org/pipermail/standards/2019-September/036435.html

I don't think stamping the origin of the id actually matters very much in this case (it very much does in others), because in this case only one id is going to be usable - the one from the MIX itself, so the stamper can be implicit.

Maybe, but I wonder how many implementations will get the implicit value wrong and open a security hole due that. I am usually a fan of avoiding unnecessary bytes on the wire and implicit values. But this smells a little bit like the carbon 'from' security hole that many implementations had.

I think at least, it should be clearly spelled out that the message is fastened to a message containing a stanza-id element where the 'id' matches the 'id' value from apply-to, and the 'by' value is the bare JID of the MIX channel (right?).

I'm with @Kev here - the example was just bad and used wrong ID. Adding "by" wouldn't change that here.

Kev · 2021-08-05T14:00:59Z

xep-0407.xml

-        <retracted xmlns='urn:xmpp:mix:misc:0' by='hag66@shakespeare.example'
-                 time='2010-07-10T23:08:25Z'/>
-      </message>
+        <retracted xmlns='urn:xmpp:message-retract:0' stamp='2010-07-10T23:08:25Z'>


The id of the MAM result needs to be the id the mix channel originally assigned to the retracted stanza.

Should be fixed by correcting first example.

Kev · 2021-08-05T14:01:51Z

xep-0407.xml

-                 time='2010-07-10T23:08:25Z'/>
-      </message>
+        <retracted xmlns='urn:xmpp:message-retract:0' stamp='2010-07-10T23:08:25Z'>
+          <origin-id xmlns='urn:xmpp:sid:0' id='a568706e-867b-49f1-a31b-e428ba9cece1'/>


This is missing the note of who retracted the message (also a comment from my original round ;))

Kev · 2021-08-05T14:03:14Z

xep-0407.xml

@@ -198,19 +204,19 @@
        Two approaches to message retraction can be used.  In the first approach, the retracted message is simply removed.   This is appropriate where retraction is provided as a user service and the user has rights to remove messages sent from the record.
      </p>
      <p>
-        The second approach is to leave a tombstone, which if taken MUST be done in the following manner.  It is recommended to use a tombstone, as simply deleting the message may cause confusion with MAM queries.   Use of a tombstone is appropriate where it is desired to leave a record of the message that was redacted.
-        With this approach, the original message &lt;body&gt; is removed and replaced with a tombstone using the &lt;retracted&gt; element qualified by the 'urn:xmpp:mix:misc:0' namespace that shows the JID of user performing the retraction and the time of the retraction.
+        The second approach is to leave a tombstone.  It is recommended to use a tombstone, as simply deleting the message may cause confusion with MAM queries.   Use of a tombstone is appropriate where it is desired to leave a record of the message that was redacted. The tombstones are also defined in &xep0424;. A service which supports tombstones MUST advertise the 'urn:xmpp:message-retract:0#tombstone' feature in its Service Discovery responses.


I still think we need to be explicit which version of 424 is required.

Should be fixed by correcting first example.

woj-tek · 2021-12-03T12:18:47Z

Hi, apologies for delay on my part - this got buried in the notifications.

I improved the example so it should be clearer now which ID should be used. On a sidenode - I think that xep-0369 should add dependency on xep-0359 as well.

Whether the message has a from is different from storing who it was who issued the retraction, no?

Correct. I haven't add any information regarding who made the retraction as... it seems that xep-0424 doesn't have that bit? And it should be included in 0424 first?

I don't think that's enough, is it? If someone implements MIX they need to know which version of 424 to read to implement it.

My previous change already included:

MUST advertise it's support by advertising 'urn:xmpp:message-retract:0' feature

so it did include version? Or am I missing something?

Relegate message retraction to xep-0424 instead of using custom one.

f9ba694

horazont added Needs Version Block The change requires a version block, and this is to be done by Editors at merge time. Ready To Merge No acknowledgements of other parties are needed anymore. There may be changes to do at merge time. labels Apr 2, 2021

horazont added Needs Author The XEP is experimental and the PR was not made by the author. The author needs to acknowledge it. and removed Ready To Merge No acknowledgements of other parties are needed anymore. There may be changes to do at merge time. labels Apr 2, 2021

woj-tek added 2 commits May 14, 2021 18:42

Clarify retraction and 424 usage: stanza-id in retraction payload, to…

63c963b

…mbstones, add examples

Remove old text and fix the build.

420cdbe

Kev reviewed Aug 5, 2021

View reviewed changes

Improve message retraction in MIX example

3f679af

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Relegate message retraction to xep-0424 instead of using custom one. #1047

Relegate message retraction to xep-0424 instead of using custom one. #1047

woj-tek commented Mar 31, 2021

horazont commented Apr 2, 2021

Kev commented Apr 2, 2021 via email

horazont commented Apr 2, 2021

Kev commented May 10, 2021

horazont commented May 11, 2021

woj-tek commented May 12, 2021

woj-tek commented May 14, 2021

horazont commented May 25, 2021

Kev left a comment

Kev Aug 5, 2021

Flowdalic Aug 5, 2021

Kev Aug 5, 2021

Flowdalic Aug 5, 2021

woj-tek Dec 3, 2021

Kev Aug 5, 2021

woj-tek Dec 3, 2021

Kev Aug 5, 2021

Kev Aug 5, 2021

woj-tek Dec 3, 2021

woj-tek commented Dec 3, 2021

Relegate message retraction to xep-0424 instead of using custom one. #1047

Are you sure you want to change the base?

Relegate message retraction to xep-0424 instead of using custom one. #1047

Conversation

woj-tek commented Mar 31, 2021

horazont commented Apr 2, 2021

Kev commented Apr 2, 2021 via email

horazont commented Apr 2, 2021

Kev commented May 10, 2021

horazont commented May 11, 2021

woj-tek commented May 12, 2021

woj-tek commented May 14, 2021

horazont commented May 25, 2021

Kev left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

woj-tek commented Dec 3, 2021