-
Notifications
You must be signed in to change notification settings - Fork 13k
fix: Orphan team when last owner user deleted #36807
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
Merged
Merged
Changes from all commits
Commits
Show all changes
23 commits
Select commit
Hold shift + click to select a range
e5b1acf
fix
tiagoevanp 2cc505d
Change @rocket.chat/meteor version to minor
tiagoevanp 0d84a58
requested changes
abhinavkrin 8ec5629
added fix and tests
abhinavkrin 9184343
requested changes
abhinavkrin 04b3ad8
requested changes
abhinavkrin e35572c
minor improvement
abhinavkrin 0735cd8
requested changes
abhinavkrin 7e96897
added changeset
abhinavkrin c5ab064
requested changes
abhinavkrin fcd5087
fix: lint
dougfabris ed8a645
minor changes
abhinavkrin 5332c09
requested changes
abhinavkrin 33c43af
requested changes
abhinavkrin 8bd4be6
requested changes
abhinavkrin c1246da
added unit tests
abhinavkrin f2ebf99
requested changes
abhinavkrin 7df48ca
requested changes
abhinavkrin 9e44567
lint fix
abhinavkrin 75af2b7
test fix
abhinavkrin 6732e19
Merge branch 'develop' into fix/users.delete-last-owner-team
kodiakhq[bot] 2467b58
Merge branch 'develop' into fix/users.delete-last-owner-team
scuciatto 937a0df
Merge branch 'develop' into fix/users.delete-last-owner-team
kodiakhq[bot] File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Some comments aren't visible on the classic Files Changed page.
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,6 @@ | ||
| --- | ||
| '@rocket.chat/core-services': minor | ||
| '@rocket.chat/meteor': minor | ||
| --- | ||
|
|
||
| Adds a `deletedRooms` field to the `users.delete` endpoint response, indicating which rooms were deleted as part of the user deletion process. | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| --- | ||
| "@rocket.chat/meteor": minor | ||
| --- | ||
|
|
||
| Fix issue where a team would become orphaned when its last owner was deleted. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,272 @@ | ||
| import { expect } from 'chai'; | ||
| import proxyquireRaw from 'proxyquire'; | ||
| import * as sinon from 'sinon'; | ||
|
|
||
| const proxyquire = proxyquireRaw.noCallThru(); | ||
|
|
||
| type Stubbed = { [k: string]: any }; | ||
|
|
||
| describe('eraseTeam (TypeScript) module', () => { | ||
| let sandbox: sinon.SinonSandbox; | ||
| let stubs: Stubbed; | ||
| let subject: any; | ||
|
|
||
| beforeEach(() => { | ||
| sandbox = sinon.createSandbox(); | ||
|
|
||
| stubs = { | ||
| 'Team': { | ||
| getMatchingTeamRooms: sandbox.stub().resolves([]), | ||
| unsetTeamIdOfRooms: sandbox.stub().resolves(), | ||
| removeAllMembersFromTeam: sandbox.stub().resolves(), | ||
| deleteById: sandbox.stub().resolves(), | ||
| }, | ||
| 'Users': { | ||
| findOneById: sandbox.stub().resolves(null), | ||
| }, | ||
| 'Rooms': { | ||
| findOneById: sandbox.stub().resolves(null), | ||
| }, | ||
| 'eraseRoomStub': sandbox.stub().resolves(true), | ||
| 'deleteRoomStub': sandbox.stub().resolves(), | ||
| '../../../../server/lib/logger/system': { | ||
| SystemLogger: { | ||
| error: sandbox.stub(), | ||
| }, | ||
| }, | ||
| '@rocket.chat/apps': { | ||
| AppEvents: { | ||
| IPreRoomDeletePrevent: 'IPreRoomDeletePrevent', | ||
| IPostRoomDeleted: 'IPostRoomDeleted', | ||
| }, | ||
| Apps: { | ||
| self: { isLoaded: () => false }, | ||
| getBridges: () => ({ | ||
| getListenerBridge: () => ({ | ||
| roomEvent: sandbox.stub().resolves(false), | ||
| }), | ||
| }), | ||
| }, | ||
| }, | ||
| '@rocket.chat/models': { | ||
| Rooms: { | ||
| findOneById: (...args: any[]) => stubs.Rooms.findOneById(...args), | ||
| }, | ||
| Users: { | ||
| findOneById: (...args: any[]) => stubs.Users.findOneById(...args), | ||
| }, | ||
| }, | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: (function () { | ||
| class MeteorError extends Error { | ||
| public error: string | undefined; | ||
|
|
||
| public details: any; | ||
|
|
||
| constructor(message?: string, error?: string, details?: any) { | ||
| super(message); | ||
| this.error = error; | ||
| this.details = details; | ||
| } | ||
| } | ||
| return MeteorError; | ||
| })(), | ||
| }, | ||
| }; | ||
|
|
||
| subject = proxyquire('./eraseTeam', { | ||
| '@rocket.chat/apps': stubs['@rocket.chat/apps'], | ||
| '@rocket.chat/models': stubs['@rocket.chat/models'], | ||
| '../../../../server/lib/eraseRoom': { __esModule: true, eraseRoom: stubs.eraseRoomStub }, | ||
| '../../../lib/server/functions/deleteRoom': { __esModule: true, deleteRoom: stubs.deleteRoomStub }, | ||
| '../../../../server/lib/logger/system': stubs['../../../../server/lib/logger/system'], | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: stubs['@rocket.chat/core-services'].MeteorError, | ||
| Team: stubs.Team, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| afterEach(() => { | ||
| sandbox.restore(); | ||
| }); | ||
|
|
||
| describe('eraseTeamShared', () => { | ||
| it('throws when user is undefined', async () => { | ||
| // eslint-disable-next-line @typescript-eslint/no-empty-function | ||
| await expect(subject.eraseTeamShared(undefined, { _id: 'team1', roomId: 'teamRoom' }, [], () => {})).to.be.rejected; | ||
| }); | ||
|
|
||
| it('erases provided rooms (excluding team.roomId) and cleans up team', async () => { | ||
| const team = { _id: 'team-id', roomId: 'team-room' }; | ||
| const user = { _id: 'user-1', username: 'u' }; | ||
| stubs.Team.getMatchingTeamRooms.resolves(['room-1', 'room-2', team.roomId]); | ||
|
|
||
| const erased: Array<{ rid: string; user: any }> = []; | ||
| const eraseRoomFn = async (rid: string, user: any) => { | ||
| erased.push({ rid, user }); | ||
| }; | ||
|
|
||
| await subject.eraseTeamShared(user, team, ['room-1', 'room-2', team.roomId], eraseRoomFn); | ||
|
|
||
| expect(erased.some((r) => r.rid === 'room-1')).to.be.true; | ||
| expect(erased.some((r) => r.rid === 'room-2')).to.be.true; | ||
| sinon.assert.calledOnce(stubs.Team.unsetTeamIdOfRooms); | ||
| expect(erased.some((r) => r.rid === team.roomId)).to.be.true; | ||
| sinon.assert.calledOnce(stubs.Team.removeAllMembersFromTeam); | ||
| sinon.assert.calledOnce(stubs.Team.deleteById); | ||
| }); | ||
abhinavkrin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| }); | ||
|
|
||
| describe('eraseTeam', () => { | ||
| it('calls eraseRoom for the team main room (via eraseTeamShared)', async () => { | ||
| const team = { _id: 't1', roomId: 't-room' }; | ||
| const user = { _id: 'u1', username: 'u', name: 'User' }; | ||
| stubs.Team.getMatchingTeamRooms.resolves([]); | ||
| const { eraseRoomStub } = stubs; | ||
| eraseRoomStub.resolves(true); | ||
|
|
||
| await subject.eraseTeam(user, team, []); | ||
|
|
||
| sinon.assert.calledWith(eraseRoomStub, team.roomId, 'u1'); | ||
| }); | ||
| }); | ||
|
|
||
| describe('eraseTeamOnRelinquishRoomOwnerships', () => { | ||
| it('returns successfully deleted room ids only', async () => { | ||
| const team = { _id: 't1', roomId: 't-room' }; | ||
| stubs.Team.getMatchingTeamRooms.resolves(['r1', 'r2']); | ||
|
|
||
| stubs.Rooms.findOneById.withArgs('r1').resolves({ _id: 'r1', federated: false }); | ||
| stubs.Rooms.findOneById.withArgs('r2').resolves(null); | ||
|
|
||
| stubs.deleteRoomStub.withArgs('r1').resolves(); | ||
| stubs.deleteRoomStub.withArgs('r2').rejects(new Error('boom')); | ||
|
|
||
| const base = proxyquire('./eraseTeam', { | ||
| '@rocket.chat/apps': stubs['@rocket.chat/apps'], | ||
| '@rocket.chat/models': stubs['@rocket.chat/models'], | ||
| '../../../../server/lib/eraseRoom': { __esModule: true, eraseRoom: stubs.eraseRoomStub }, | ||
| '../../../lib/server/functions/deleteRoom': { __esModule: true, deleteRoom: stubs.deleteRoomStub }, | ||
| '../../../../server/lib/logger/system': stubs['../../../../server/lib/logger/system'], | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: stubs['@rocket.chat/core-services'].MeteorError, | ||
| Team: stubs.Team, | ||
| }, | ||
| }); | ||
|
|
||
| const result: string[] = await base.eraseTeamOnRelinquishRoomOwnerships(team, ['r1', 'r2']); | ||
| expect(result).to.be.an('array').that.includes('r1').and.not.includes('r2'); | ||
| }); | ||
| }); | ||
abhinavkrin marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| describe('eraseRoomLooseValidation', () => { | ||
| let baseModule: any; | ||
|
|
||
| beforeEach(() => { | ||
| baseModule = proxyquire('./eraseTeam', { | ||
| '@rocket.chat/apps': stubs['@rocket.chat/apps'], | ||
| '@rocket.chat/models': stubs['@rocket.chat/models'], | ||
| '../../../../server/lib/eraseRoom': { __esModule: true, eraseRoom: stubs.eraseRoomStub }, | ||
| '../../../lib/server/functions/deleteRoom': { __esModule: true, deleteRoom: stubs.deleteRoomStub }, | ||
| '../../../../server/lib/logger/system': stubs['../../../../server/lib/logger/system'], | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: stubs['@rocket.chat/core-services'].MeteorError, | ||
| Team: stubs.Team, | ||
| }, | ||
| }); | ||
| }); | ||
|
|
||
| it('returns false when room not found', async () => { | ||
| stubs.Rooms.findOneById.resolves(null); | ||
| const res = await baseModule.eraseRoomLooseValidation('does-not-exist'); | ||
| expect(res).to.be.false; | ||
| }); | ||
|
|
||
| it('returns false when room.federated is true', async () => { | ||
| stubs.Rooms.findOneById.resolves({ _id: 'r', federated: true }); | ||
| const res = await baseModule.eraseRoomLooseValidation('r'); | ||
| expect(res).to.be.false; | ||
| }); | ||
|
|
||
| it('returns false when app pre-delete prevents deletion', async () => { | ||
| const listenerStub = sandbox.stub().resolves(true); | ||
| const AppsStub = { | ||
| AppEvents: stubs['@rocket.chat/apps'].AppEvents, | ||
| Apps: { | ||
| self: { isLoaded: () => true }, | ||
| getBridges: () => ({ getListenerBridge: () => ({ roomEvent: listenerStub }) }), | ||
| }, | ||
| }; | ||
|
|
||
| const m = proxyquire('./eraseTeam', { | ||
| '@rocket.chat/apps': AppsStub, | ||
| '@rocket.chat/models': stubs['@rocket.chat/models'], | ||
| '../../../../server/lib/eraseRoom': { __esModule: true, eraseRoom: stubs.eraseRoomStub }, | ||
| '../../../lib/server/functions/deleteRoom': { __esModule: true, deleteRoom: stubs.deleteRoomStub }, | ||
| '../../../../server/lib/logger/system': stubs['../../../../server/lib/logger/system'], | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: stubs['@rocket.chat/core-services'].MeteorError, | ||
| Team: stubs.Team, | ||
| }, | ||
| }); | ||
|
|
||
| stubs.Rooms.findOneById.resolves({ _id: 'r', federated: false }); | ||
|
|
||
| const res = await m.eraseRoomLooseValidation('r'); | ||
| expect(listenerStub.calledOnce).to.be.true; | ||
| expect(res).to.be.false; | ||
| }); | ||
|
|
||
| it('logs and returns false when deleteRoom throws', async () => { | ||
| stubs.Rooms.findOneById.resolves({ _id: 'r', federated: false }); | ||
| stubs.deleteRoomStub.rejects(new Error('boom')); | ||
|
|
||
| const m = proxyquire('./eraseTeam', { | ||
| '@rocket.chat/apps': stubs['@rocket.chat/apps'], | ||
| '@rocket.chat/models': stubs['@rocket.chat/models'], | ||
| '../../../../server/lib/eraseRoom': { __esModule: true, eraseRoom: stubs.eraseRoomStub }, | ||
| '../../../lib/server/functions/deleteRoom': { __esModule: true, deleteRoom: stubs.deleteRoomStub }, | ||
| '../../../../server/lib/logger/system': stubs['../../../../server/lib/logger/system'], | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: stubs['@rocket.chat/core-services'].MeteorError, | ||
| Team: stubs.Team, | ||
| }, | ||
| }); | ||
|
|
||
| const res = await m.eraseRoomLooseValidation('r'); | ||
| expect(res).to.be.false; | ||
| sinon.assert.calledOnce(stubs['../../../../server/lib/logger/system'].SystemLogger.error); | ||
| }); | ||
|
|
||
| it('calls post-deleted event and returns true on success', async () => { | ||
| const roomEventStub = sandbox.stub().onFirstCall().resolves(false).onSecondCall().resolves(); | ||
| const AppsStub = { | ||
| AppEvents: stubs['@rocket.chat/apps'].AppEvents, | ||
| Apps: { | ||
| self: { isLoaded: () => true }, | ||
| getBridges: () => ({ getListenerBridge: () => ({ roomEvent: roomEventStub }) }), | ||
| }, | ||
| }; | ||
|
|
||
| stubs.deleteRoomStub.resolves(); | ||
| const m = proxyquire('./eraseTeam', { | ||
| '@rocket.chat/apps': AppsStub, | ||
| '@rocket.chat/models': stubs['@rocket.chat/models'], | ||
| '../../../../server/lib/eraseRoom': { __esModule: true, eraseRoom: stubs.eraseRoomStub }, | ||
| '../../../lib/server/functions/deleteRoom': { __esModule: true, deleteRoom: stubs.deleteRoomStub }, | ||
| '../../../../server/lib/logger/system': stubs['../../../../server/lib/logger/system'], | ||
| '@rocket.chat/core-services': { | ||
| MeteorError: stubs['@rocket.chat/core-services'].MeteorError, | ||
| Team: stubs.Team, | ||
| }, | ||
| }); | ||
|
|
||
| stubs.Rooms.findOneById.resolves({ _id: 'r', federated: false }); | ||
|
|
||
| const res = await m.eraseRoomLooseValidation('r'); | ||
| expect(res).to.be.true; | ||
| sinon.assert.calledTwice(roomEventStub); | ||
| }); | ||
| }); | ||
| }); | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| import { AppEvents, Apps } from '@rocket.chat/apps'; | ||
| import { MeteorError, Team } from '@rocket.chat/core-services'; | ||
| import type { AtLeast, IRoom, ITeam, IUser } from '@rocket.chat/core-typings'; | ||
| import { Rooms } from '@rocket.chat/models'; | ||
|
|
||
| import { eraseRoom } from '../../../../server/lib/eraseRoom'; | ||
| import { SystemLogger } from '../../../../server/lib/logger/system'; | ||
| import { deleteRoom } from '../../../lib/server/functions/deleteRoom'; | ||
|
|
||
| type eraseRoomFnType = (rid: string, user: AtLeast<IUser, '_id' | 'username' | 'name'>) => Promise<boolean | void>; | ||
|
|
||
| export const eraseTeamShared = async ( | ||
| user: AtLeast<IUser, '_id' | 'username' | 'name'>, | ||
| team: ITeam, | ||
| roomsToRemove: IRoom['_id'][] = [], | ||
| eraseRoomFn: eraseRoomFnType, | ||
| ) => { | ||
| const rooms: string[] = roomsToRemove.length | ||
| ? (await Team.getMatchingTeamRooms(team._id, roomsToRemove)).filter((roomId) => roomId !== team.roomId) | ||
| : []; | ||
|
|
||
| if (!user) { | ||
| throw new MeteorError('Invalid user provided for erasing team', 'error-invalid-user', { | ||
| method: 'eraseTeamShared', | ||
| }); | ||
| } | ||
|
|
||
| // If we got a list of rooms to delete along with the team, remove them first | ||
| await Promise.all(rooms.map((room) => eraseRoomFn(room, user))); | ||
|
|
||
| // Move every other room back to the workspace | ||
| await Team.unsetTeamIdOfRooms(user, team); | ||
|
|
||
| // Remove the team's main room | ||
| await eraseRoomFn(team.roomId, user); | ||
|
|
||
| // Delete all team memberships | ||
| await Team.removeAllMembersFromTeam(team._id); | ||
|
|
||
| // And finally delete the team itself | ||
| await Team.deleteById(team._id); | ||
| }; | ||
|
|
||
| export const eraseTeam = async (user: AtLeast<IUser, '_id' | 'username' | 'name'>, team: ITeam, roomsToRemove: IRoom['_id'][]) => { | ||
| await eraseTeamShared(user, team, roomsToRemove, async (rid, user) => { | ||
| return eraseRoom(rid, user._id); | ||
| }); | ||
| }; | ||
|
|
||
| /** | ||
| * @param team | ||
| * @param roomsToRemove | ||
| * @returns deleted room ids | ||
| */ | ||
| export const eraseTeamOnRelinquishRoomOwnerships = async (team: ITeam, roomsToRemove: IRoom['_id'][] = []): Promise<string[]> => { | ||
| const deletedRooms = new Set<string>(); | ||
| await eraseTeamShared({ _id: 'rocket.cat', username: 'rocket.cat', name: 'Rocket.Cat' }, team, roomsToRemove, async (rid) => { | ||
| const isDeleted = await eraseRoomLooseValidation(rid); | ||
| if (isDeleted) { | ||
| deletedRooms.add(rid); | ||
| } | ||
| }); | ||
| return Array.from(deletedRooms); | ||
| }; | ||
|
|
||
| export async function eraseRoomLooseValidation(rid: string): Promise<boolean> { | ||
| const room = await Rooms.findOneById(rid); | ||
|
|
||
| if (!room) { | ||
| return false; | ||
| } | ||
|
|
||
| if (room.federated) { | ||
| return false; | ||
| } | ||
|
|
||
| if (Apps.self?.isLoaded()) { | ||
| const prevent = await Apps.getBridges()?.getListenerBridge().roomEvent(AppEvents.IPreRoomDeletePrevent, room); | ||
| if (prevent) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| try { | ||
| await deleteRoom(rid); | ||
| } catch (e) { | ||
| SystemLogger.error(e); | ||
| return false; | ||
| } | ||
|
|
||
| if (Apps.self?.isLoaded()) { | ||
| void Apps.getBridges()?.getListenerBridge().roomEvent(AppEvents.IPostRoomDeleted, room); | ||
| } | ||
|
|
||
| return true; | ||
| } |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.