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

OF-2760: Adds option to retire a MUC room name on deletion #2630

Merged
merged 14 commits into from
Dec 16, 2024

Conversation

viv
Copy link
Collaborator

@viv viv commented Dec 5, 2024

Retiring a room means that this room's name cannot be used again after the room is deleted.

Where a MUC may have particular meaning to plugins or to other systems, assume that deletion is explicit and intentional. Do not allow the MUC name (and so JID) to be repurposed, preventing impersonation / corruption / access of MUC history, or other unforeseen side effects.

viv added 3 commits December 5, 2024 14:40
Tomb-stoning a room means that this room's name is retired on room deletion and cannot be used again. Where a MUC may have particular meaning to plugins or to other systems, assume that deletion is explicit and intentional. Do not allow the MUC name (and so JID) to be repurposed, preventing impersonation / corruption / access of MUC history, or other unforeseen side effects.
- Renames "tombstone" to "retire" everywhere
- Adds admin page to list & delete retired rooms
@viv viv changed the title OF-2760: Adds option to tombstone a MUC room on deletion OF-2760: Adds option to retire a MUC room name on deletion Dec 10, 2024
@viv viv marked this pull request as ready for review December 10, 2024 10:46
@viv viv requested a review from guusdk December 10, 2024 10:46
viv added 6 commits December 11, 2024 11:20
Makes it more clear that deleting a retiree is bringing it out of retirement.
Adding the service ID to the log could help chase down issues.
Only setting the fetch size is not sufficient to prevent the entire result set from being loaded in memory (at least not for MSSQL). That requires a bit more configuration of the query/result set, added in this commit.
This data is not yet exposed on the admin console retirees page.
@viv
Copy link
Collaborator Author

viv commented Dec 12, 2024

This data is not yet exposed on the admin console retirees page.

I'll pick this up on Monday morning.

viv added 2 commits December 16, 2024 10:02
This change helps admins better understand and manage retired rooms.

- Created MUCRoomRetiree.java for structured data access
- Updated MultiUserChatManager to expose new fields
- Enhanced muc-room-retirees.jsp to display additional information
@viv
Copy link
Collaborator Author

viv commented Dec 16, 2024

I think this is ready for another review.

@guusdk
Copy link
Member

guusdk commented Dec 16, 2024

Rooms that are non-persistent but do have 'retire-on-delete' set, do not retire. I think in principle, the setting should be honored.

It might be desirable to have a different default setting for 'retire-on-delete' for persistent vs. non-persistent rooms (but I'm happy for that improvement to go in a new ticket).

@guusdk
Copy link
Member

guusdk commented Dec 16, 2024

The 'retire-on-delete' option should be changeable through the room configuration form.

In the screenshot below a new config setting that we've recently added does pop up, but not 'retire-on-delete'.

image

@guusdk
Copy link
Member

guusdk commented Dec 16, 2024

I think this is ready for another review.

I've re-reviewed it, and left some suggestions to be improved in this PR, but also raised a couple of tickets for us to tackle outside of the scope of this PR (OF-2933, OF-2934).

Are you aware of the 're-request review' functionality in GitHub? That has the benefit of sending out all the required emails etc.

viv added 2 commits December 16, 2024 15:28
With this change rooms that are non-persistent, but do have 'retire-on-delete' set, will be retired when they are deleted.
@viv viv requested a review from guusdk December 16, 2024 16:00
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.

I've tested both new changes, which seem to work as intended. Thanks!

@guusdk guusdk merged commit 8ea5934 into igniterealtime:main Dec 16, 2024
16 of 17 checks passed
@viv viv deleted the OF-2760/tombstone-rooms branch December 17, 2024 09:15
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.

2 participants