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-0402: Fix issues found during Stockholm XMPP Sprint #835

Merged
merged 15 commits into from Oct 15, 2019

Conversation

@linkmauve
Copy link
Member

commented Sep 28, 2019

This PR might be more easily read commit per commit.

Rendered version: https://linkmauve.fr/extensions/xep-0402.html

I was extremely surprised at the difference of treatment for notifications between publishing an item (XEP-0060 §7.1.2.1) and purging all items (§8.5.2), both sending a notification if the node is configured for that, and retracting an item (§7.2.2.1) where the publisher additionally has to include a notify='1' attribute on the <retract/> element.

Another issue we’ve identified during the Sprint is password migration, as much as I dislike this feature there are existing MUCs being protected by passwords, and we most likely keep those until we decide on either removing this feature from MUC altogether, or find a more secure way to store passwords on the server.

Otherwise, this specification is pretty nice and should be ready for advancing to draft soon I think.

@linkmauve linkmauve requested review from horazont and jcbrand Sep 28, 2019
@iNPUTmice

This comment has been minimized.

Copy link
Contributor

commented Sep 28, 2019

In order not to replace old bookmarks when pushing a new one, we need to request a good max_items value. This one has been picked as some random large enough number, bikeshed welcome!

Also all clients will have to agree on one value because otherwise your preconditons will constantly fail when other clients have used a different value before. Which in turn means we really need support for max_items=0 to mean server side max.

@Ppjet6 Ppjet6 added the Needs Author label Sep 28, 2019
@Ppjet6

This comment has been minimized.

Copy link
Member

commented Sep 28, 2019

This is for @dwd and/or @jcbrand to review.

@Ppjet6 Ppjet6 removed the request for review from horazont Sep 28, 2019
linkmauve and others added 14 commits Sep 27, 2019
In order not to replace old bookmarks when pushing a new one, we need to
request a good max_items value.  This one has been picked as some random
large enough number, bikeshed welcome!

TODO: add support for unlimited values?
TODO: Add the justification for that.
PubSub §7.2.2.1 says “If no error occurs and the <retract/> element
included a 'notify' attribute with a value of "true" or "1", then the
service MUST delete the item and MUST notify all subscribers as shown
below.”

This means the publishing entity, not the server or the subscription
state, is responsible for properly enabling notifications to other
resources, which makes no sense.

See https://xmpp.org/extensions/xep-0060.xml#publisher-delete-success-notify
There is no rationale for this, and as it overrides the room name
“forever” it shouldn’t necessarily be encouraged.
Make it a SHOULD that it joins MUCs on retrieval of autojoin='1'
bookmarks, leaves them on autojoin='0' or no autojoin, and does the same
when it’s the one modifying the bookmarks.
@linkmauve linkmauve force-pushed the linkmauve:xep-0402 branch from 6f2c82a to c29f2b6 Sep 28, 2019
Copy link
Contributor

left a comment

Looks good. I have some suggestions related to language and grammer.

xep-0402.xml Outdated Show resolved Hide resolved
xep-0402.xml Outdated Show resolved Hide resolved
xep-0402.xml Outdated Show resolved Hide resolved
xep-0402.xml Show resolved Hide resolved
xep-0402.xml Show resolved Hide resolved
Thanks for the suggestions!
@linkmauve

This comment has been minimized.

Copy link
Member Author

commented Oct 14, 2019

I fixed the English issues, thanks for giving me a nice explanation!

@jcbrand

This comment has been minimized.

Copy link
Contributor

commented Oct 14, 2019

@linkmauve LGTM

@Ppjet6 Ppjet6 added Ready To Merge and removed Needs Author labels Oct 14, 2019
@Ppjet6 Ppjet6 self-assigned this Oct 14, 2019
@Ppjet6 Ppjet6 referenced this pull request Oct 15, 2019
@Ppjet6 Ppjet6 merged commit 03bd9a1 into xsf:master Oct 15, 2019
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@linkmauve linkmauve deleted the linkmauve:xep-0402 branch Oct 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.