[jitsi-dev] [jitsi-videobridge] make prosody happy, too (#27)


#1

prosody returns forbidden errors for already existing nodes with a different owner (per xep-0060)
You can merge this Pull Request by running:

  git pull https://github.com/fippo/jitsi-videobridge ignorenodecreationerrors

Or you can view, comment on it, or merge it online at:

  https://github.com/jitsi/jitsi-videobridge/pull/27

-- Commit Summary --

  * make prosody happy, too

-- File Changes --

    M src/org/jitsi/videobridge/pubsub/PubSubPublisher.java (16)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/27.patch
https://github.com/jitsi/jitsi-videobridge/pull/27.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27


#2

Merged #27.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#event-177944957


#3

Merged. Thanks!

Is there a reason for the second if to live in a separate clause given that it repeats the exact same content?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#issuecomment-58971313


#4

Wouldn't an XMPP server return a "forbidden" error when the videobridge is not allowed to create a node AND the node doesn't exist? In this case we shouldn't assume success.

I think that the approach from https://github.com/jitsi/jitsi-videobridge/pull/26 is a (more) reasonable compromise between implementing the extra logic for checking for node existence and not allowing the bridge to work without permissions to create a node.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#issuecomment-59011301


#5

@fippo could you please fix the code duplication I referred to in my previous comment?

@bgrozev yes, that's what's supposed to happen, and if so we will notify the user when publishing fails. Note that this would have been a deployment mistake. Not a generic situation that we can face on a regular basis. It would have been great to be able to rely on consistent feedback from all XMPP servers here but apparently that's not possible.

The patch from 26 requires explicit configuration that will be a) obscure to most users and b) necessary in most cases. I'd rather our code be smarter than that.

Also, not sure I understand your concern about not allowing the bridge to proceed without permissions ... the very purpose of permissions is to prevent you from doing something you are not supposed to. Why would you need a configuration option to have them respected?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#issuecomment-59013151


#6

My concern is that in the use case where the videobridge is supposed to have create rights, but it doesn't (e.g. the user forgot to give rights to the bridge, as I did the first time), notifying the user that *publishing* failed might not be very helpful. As you suggested offlist, adding a log message when node creation fails with "forbidden" would address this.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#issuecomment-59015348


#7

Indeed, a log message would be fine. As per our offlist chat: Could not create PubSub node. This is not necessarily a problem as with some servers it simply indicates the room already exists. However, if Jitsi Videobridge fails to publish statistics later, this can be the very reason.

My concern is that I don't want us to expect users to learn the meaning of an extra configuration option that they would very likely need to turn on.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#issuecomment-59015835


#8

See https://github.com/jitsi/jitsi-videobridge/pull/28 -- logging a warning now. Requires a trunk version of prosody to work, the error type returned by prosody was incorrect.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/27#issuecomment-59018290