xsf / xeps

Hosts the markup for all XMPP Protocol Extensions.
https://xmpp.org/extensions/
Other
126 stars 118 forks source link

XEP-0425: Updates based on list feedback #1271

Closed jcbrand closed 4 months ago

jcbrand commented 1 year ago

Mailing list thread is here: https://mail.jabber.org/pipermail/standards/2021-December/038660.html

I mention this PR here: https://mail.jabber.org/pipermail/standards/2023-February/039167.html

iNPUTmice commented 1 year ago

I think something is wrong with the examples. I assume oldhag is the sender of the original message? And the room 'fakes' a retract on behalf of oldhag? Then it should look something like this:

(Previously the moderation messages was validated by coming from the bare room jid; but if you want to be compatible with retraction it will need to be validated by the sender (+ occupant id maybe))

<message type="groupchat" id='retraction-id-1' from='room@muc.example.com/oldhag' to="account@example.com/resource">
  <retract id="stanza-id-1" xmlns='urn:xmpp:message-retract:1'>
    <moderated by='room@muc.example.com/macbeth' xmlns='urn:xmpp:message-moderate:1'>
      <reason>This message contains inappropriate content for this forum</reason>
    </moderated>
  </retract>
</message>
jcbrand commented 1 year ago

I assume oldhag is the sender of the original message? And the room 'fakes' a retract on behalf of oldhag? Then it should look something like this:

As mentioned in the XSF MUC, I don't think it's appropriate to pretend that a retraction comes from someone it doesn't actually come from and I doubt that maintaining compatibility with clients that only implement XEP-0424 is worth it.

singpolyma commented 1 year ago

Remove the dependency on XEP-0422 Message Fastening

Doesn't that just make it more complex to implement for no benefit?

jcbrand commented 1 year ago

@singpolyma: No, I think it's the contrary. I added message fastening in the anticipation that it would become a widely implemented standard. That didn't happen, in fact nobody besides myself made any attempt (that I'm aware of) to actually use fastenings. So it's a dead end and unnecessary cruft.

jcbrand commented 1 year ago

@Kev, @iNPUTmice Is there anything still preventing this from being merged?

Kev commented 1 year ago

@Kev, @iNPUTmice Is there anything still preventing this from being merged?

Waiting for Council to get back to me on this.

iNPUTmice commented 1 year ago

Minor nit. The groupchat message example has a weird to attribute. It should probably be sent to: romeo@montague.example/home

<message type="groupchat" id='retraction-id-1' from='room@muc.example.com' to="romeo@montague.example/home">
iNPUTmice commented 1 year ago

Council has voted to move XEP-0425 back to experimental (which gives the author control over those changes)

lovetox commented 1 year ago

What logic does the <moderate/d> and <retract/ed> follow? It does not seem to make much sense anymore now.

Sending IQ

<moderate id="stanza-id-1" xmlns='urn:xmpp:message-moderate:1'>
    <retract xmlns='urn:xmpp:message-retract:1'/>
    <reason>This message contains inappropriate content for this forum</reason>
</moderate>

I think the id attribute should be moved to the <retract> element, later in the tombstone use case we depend on the already defined stamp attribute on the retract element, so why not consistently extend the retract XEP and also define the id there?

I dont care much, but it should be at least consistent, the live message case has the id on the retract element.

My Proposal, move id to retract element.

<moderate xmlns='urn:xmpp:message-moderate:1'>
    <retract id="stanza-id-1" xmlns='urn:xmpp:message-retract:1' />
    <reason>This message contains inappropriate content for this forum</reason>
</moderate>

Receiving the message live

<retract id="stanza-id-1" xmlns='urn:xmpp:message-retract:1'>
    <moderated by='room@muc.example.com/macbeth' xmlns='urn:xmpp:message-moderate:1'>
        <reason>This message contains inappropriate content for this forum</reason>
    </moderated>
</retract>

The <apply-to> was replaced with the <retract> but now its inconsistent with the tombstone and the IQ where moderated is always the parent.

Please lets revert this back to (move the id attribute either to the moderated element or keep it in the retract but it should be consistent with the IQ).

 <moderated by='room@muc.example.com/macbeth' xmlns='urn:xmpp:message-moderate:1'>
     <occupant-id xmlns="urn:xmpp:occupant-id:0" id="dd72603deec90a38ba552f7c68cbcc61bca202cd" />
     <retract id="stanza-id-1" xmlns='urn:xmpp:message-retract:1' />
     <reason>This message contains inappropriate content for this forum</reason>
 </moderated>

This makes it consistent with the tombstone. Further please add <occupant-id> into the example and add a sentence maybe.

Receiving a Tombstone

<moderated by="witch@shakespeare.example">
    <occupant-id xmlns="urn:xmpp:occupant-id:0" id="dd72603deec90a38ba552f7c68cbcc61bca202cd" />
    <retracted stamp='2019-09-20T23:19:12Z' xmlns='urn:xmpp:message-retract:0'/>
    <reason>This message contains inappropriate content for this forum</reason>
</moderated>

Example looks good and consistent, but on the <moderated> element the namespace is missing !

Please add a sentence how to discover a tombstone, its very subtle, a tombstone has a <retracted> element, a live message has a <retract> element. Implementations will want to do different things on encountering tombstones, i think its worth to call out how to discover them.

lovetox commented 1 year ago

I just discovered there seems to be no protection against forging tombstones.

Kev commented 8 months ago

@jcbrand So I think this is ok for you to update to change back to Experimental in the same change, and then I can merge.

jcbrand commented 6 months ago

@lovetox Thank you for your comments. Here are my responses.

What logic does the <moderate/d> and <retract/ed> follow? It does not seem to make much sense anymore now.

It's the difference between a request (or command) and a response. Seems to me it's mainly a matter of taste whether one should distinguish like this or not. I prefer it, since it removes ambiguity once you understand the point. If you get retracted, you know this is a message that has already been retracted. If you get retract it means you (as the client) are being asked to retract a message in the chat history.

My Proposal, move id to retract element.

Thanks, but I don't really see the need to change this. The message is being moderated, and the moderation action happens to be a retraction. That was the idea behind putting the id on the moderate element.

later in the tombstone use case we depend on the already defined stamp attribute on the retract element

That is on the retracted element, which is something else. I think I prefer the id attribute on the moderate element.

The was replaced with the but now its inconsistent with the tombstone and the IQ where moderated is always the parent.

Ok, I updated the tombstone so that retracted is the parent of moderated.

Further please add into the example and add a sentence maybe.

Done.

Example looks good and consistent, but on the element the namespace is missing !

Thanks, fixed.

Please add a sentence how to discover a tombstone

There already is a sentence above the tombstone example which explains this.

To try and reduce confusion, I've added the following sentence:

Note the past tense in the element names, e.g. "retracted" instead of just "retract", to indicate that the retraction has already been executed and that this is therefore a tombstone and not an action that should be applied.

jcbrand commented 6 months ago

@Kev I've moved the status back to experimental, can you please go ahead and merge?