Skip to content

avoid self role revoke in conferences.#1751

Closed
lakadbagha wants to merge 2 commits intoigniterealtime:4.6from
lakadbagha:4.6
Closed

avoid self role revoke in conferences.#1751
lakadbagha wants to merge 2 commits intoigniterealtime:4.6from
lakadbagha:4.6

Conversation

@lakadbagha
Copy link

Please review now and let me know.

Copy link
Member

@guusdk guusdk left a comment

Choose a reason for hiding this comment

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

Hello! Thanks for your contribution!

This PR has a lot of changes that affect formatting, but not the code. This makes it hard to review the PR.

The only functional change that I see is that there's now a check for null, for the 'from' attribute of a stanza. It is not immediately clear to me (or to anyone that will be maintaining this code) why this check solves an issue. Please provide additional comments around that check, to explain why this check is needed.

Furthermore, my suspicion is that the check for null almost accidentally has the desired effect (the 'from' attribute happens to be null in the desired scenario, but I wonder if that's relevant to the functionality that's being modified). If I'm right, then this change needs to be improved, to be more a lot more explicit.

@gjaekel
Copy link
Contributor

gjaekel commented Nov 18, 2020

If this CR will prevent to self-revoke the room admin role, I would disagree to accept it: It's an thinkable usecase for me (and others in our company) first to act as a "chairman" to establish a room in a good time before, then spread the admin role to the arriving speaker and it's team. At this point, a former admin may want to release this role.

@lakadbagha
Copy link
Author

lakadbagha commented Nov 18, 2020

If this CR will prevent to self-revoke the room admin role, I would disagree to accept it: It's an thinkable usecase for me (and others in our company) first to act as a "chairman" to establish a room in a good time before, then spread the admin role to the arriving speaker and it's team. At this point, a former admin may want to release this role.

@gjaekel its upto you sir. I don't find this implementation is unsafe little. suppose if a chairman organised a chat. where he granted some peoples as owner. Then they should not allowed release this role because if you think practical if somebody don't want then they immediately talk about this with the chairman. If admin disagree or don't like to go such meetings then they leave that conference.

See openfire is really awesome open source project to me. Am learning something everyday about openfire.
Openfire is used in many social lives chat apps. I heard about openfire from nimbuzz in 2012 around and I started coding from nimbuzz. My point is we don't have strong security check like verify user by OTP on email/sms or there is no 2 factor auth implementation. and there is already many hackers who makes brutus cracker to hack ID's and hack conferences by granting other ID owner and release hack ID role.
So security wise this could be a biggest flaw.
When from is null thats basically what I found that we are changing role from openfire admin dashboard. so thats why there is one condition of null case.
@guusdk sir I hope you also understand my explanation.

@akrherz
Copy link
Member

akrherz commented Mar 21, 2025

Can a verdict be reached on this PR?

@guusdk
Copy link
Member

guusdk commented Mar 21, 2025

Giving that this is lingering for 4 years without to much activity, I'm going to be a bit hand-wavy:

  • What affiliation and role changes are allowed to be applied in group chats is governed by the specification in XEP-0045
  • The XMPP Interop Testing project has tests that verify if the implementation allows 'the correct thing'.
  • Openfire passes those tests.

To me, that's a good indication that the existing behavior of Openfire is doing the right thing, and that this change is not needed to conform to the specification.

As there are various other concerns with this PR, I'm going to close this without a merge.

@guusdk guusdk closed this Mar 21, 2025
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.

4 participants