Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XEP-0425: Updates based on list feedback #1271

Merged
merged 1 commit into from
Mar 12, 2024

Conversation

jcbrand
Copy link
Contributor

@jcbrand jcbrand commented Feb 19, 2023

  • Remove the dependency on XEP-0422 Message Fastening
  • Rename to 'Moderated Message Retraction' and focus only on the retraction use-case
  • Ensure compatibility with clients that only implement XEP-0424
  • Clarify the purpose of the reason element

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
Copy link
Contributor

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='[email protected]/oldhag' to="[email protected]/resource">
  <retract id="stanza-id-1" xmlns='urn:xmpp:message-retract:1'>
    <moderated by='[email protected]/macbeth' xmlns='urn:xmpp:message-moderate:1'>
      <reason>This message contains inappropriate content for this forum</reason>
    </moderated>
  </retract>
</message>

xep-0425.xml Outdated Show resolved Hide resolved
xep-0425.xml Outdated Show resolved Hide resolved
@jcbrand jcbrand force-pushed the jcbrand/xep-0425-remove-fastening branch from 00454d4 to 980141f Compare February 20, 2023 10:12
@jcbrand
Copy link
Contributor Author

jcbrand commented Feb 20, 2023

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
Copy link
Contributor

Remove the dependency on XEP-0422 Message Fastening

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

@jcbrand
Copy link
Contributor Author

jcbrand commented Feb 23, 2023

@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.

@Kev Kev added the Needs Council The affected XEP has the Council as Approving Body and it needs to decide on the change. label Feb 28, 2023
xep-0425.xml Outdated Show resolved Hide resolved
@jcbrand jcbrand marked this pull request as draft March 1, 2023 09:07
@jcbrand jcbrand force-pushed the jcbrand/xep-0425-remove-fastening branch from 980141f to 7731329 Compare March 2, 2023 08:57
@jcbrand jcbrand marked this pull request as ready for review March 2, 2023 09:00
xep-0425.xml Outdated Show resolved Hide resolved
@jcbrand jcbrand force-pushed the jcbrand/xep-0425-remove-fastening branch from 7731329 to 1ff4761 Compare March 2, 2023 10:42
@jcbrand
Copy link
Contributor Author

jcbrand commented Apr 25, 2023

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

@Kev
Copy link
Member

Kev commented Apr 25, 2023

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

Waiting for Council to get back to me on this.

@iNPUTmice
Copy link
Contributor

Minor nit. The groupchat message example has a weird to attribute. It should probably be sent to: [email protected]/home

<message type="groupchat" id='retraction-id-1' from='[email protected]' to="[email protected]/home">

xep-0425.xml Outdated
<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>
</iq>]]></example>

<section2 topic='Success case' anchor='usecase-success'>

<p>If the moderator is allowed to moderate the message, the groupchat service will send a message stanza to all participants (including the moderator), indicating that the message has been retracted and by whom.</p>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this the only protection against a non-moderator issuing a retraction? If so it's not enough because the MUC may not understand this protocol and therefore won't be able to perform this filtering.

Regardless, how, when, and where retractions are supposed to be validated should be in the security considerations section.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To prevent message spoofing, it's very important to check that the moderation message comes from the MUC service

its already in the security considerations, this is enough, don’t you think?

@iNPUTmice
Copy link
Contributor

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

xep-0425.xml Outdated
</iq>]]></example>

<section2 topic='Success case' anchor='usecase-success'>

<p>If the moderator is allowed to moderate the message, the groupchat service will send a message stanza to all participants (including the moderator), indicating that the message has been retracted and by whom.</p>
<p>This message will use &xep0422; to indicate that it applies to the moderated message, by referring to its XEP-0359 Stanza ID.</p>
<p>This message will use &xep0424; to indicate that it is a retraction, and will refer to the retracted message by its XEP-0359 Stanza ID. Care must be taken to use the stanza ID that was added by the MUC, i.e. which has a "by" attribute set to the MUC JID, and not some other stanza ID. The client MUST check that the correct stanza ID was used and ignore the stanza if not.</p>
<p>Inside the &lt;retract/&gt; element is a &lt;moderated/&gt; element indicating that the retraction was a moderator action and containing the optional reason.</p>
Copy link

@lovetox lovetox Jun 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please describe the elements that should or must be in the message here when the message is first introduced

  • Is the "by" attributed a MUST, or optional?
  • Is the "by" attribute set to a real JID in non-anon rooms? Or always the occupant JID?
  • Should a server that understands the occupant-id XEP add a <occupant-id> tag inside the moderated tag?

Some of these things are defined later down in the Tombstone section, which someone may not even read because they have no interest in implementing tombstones, and may interpret only relevant for the tombstone use case.

@lovetox
Copy link

lovetox commented Jun 17, 2023

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='[email protected]/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='[email protected]/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="[email protected]">
    <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
Copy link

lovetox commented Jun 18, 2023

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

@Kev
Copy link
Member

Kev commented Oct 30, 2023

@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.

@Kev Kev added Needs Changes and removed Needs Council The affected XEP has the Council as Approving Body and it needs to decide on the change. labels Oct 30, 2023
@jcbrand jcbrand force-pushed the jcbrand/xep-0425-remove-fastening branch 2 times, most recently from 6e7fd67 to d3ae5e2 Compare December 20, 2023 21:53
- Remove the dependency on XEP-0422 Message Fastening
- Rename to 'Moderated Message Retraction' and focus only on the retraction use-case
- Clarify the purpose of the `reason` element
- Specify that clients must check that the correct stanza ID was used (based on the `by` attribute)
- Relax the requirement that a `by` attribute be added to the `<moderated>` element to SHOULD.

Mailing list thread is here:
https://mail.jabber.org/pipermail/standards/2021-December/038660.html
@jcbrand jcbrand force-pushed the jcbrand/xep-0425-remove-fastening branch from d3ae5e2 to baecbad Compare December 20, 2023 22:28
@jcbrand
Copy link
Contributor Author

jcbrand commented Dec 20, 2023

@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
Copy link
Contributor Author

jcbrand commented Dec 20, 2023

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

@Kev Kev removed the Needs Changes label Jan 30, 2024
<moderated by="[email protected]">
<occupant-id xmlns="urn:xmpp:occupant-id:0" id="dd72603deec90a38ba552f7c68cbcc61bca202cd" />
<retracted stamp='2019-09-20T23:19:12Z' xmlns='urn:xmpp:message-retract:0'/>
<retracted stamp='2019-09-20T23:19:12Z' xmlns='urn:xmpp:message-retract:0'>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Update to urn:xmpp:message-retract:1?

@iNPUTmice iNPUTmice merged commit ef79d2c into master Mar 12, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants