diff --git a/client/components/settings/peopleBody.js b/client/components/settings/peopleBody.js index 63167deeb..ee0e0b927 100644 --- a/client/components/settings/peopleBody.js +++ b/client/components/settings/peopleBody.js @@ -1269,23 +1269,30 @@ Template.settingsUserPopup.events({ }, 'click #deleteButton'(event) { event.preventDefault(); - Users.remove(this.userId); - /* - // Delete user is enabled, but you should remove user from all boards - // before deleting user, because there is possibility of leaving empty user avatars - // to boards. You can remove non-existing user ids manually from database, - // if that happens. - //. See: - // - wekan/client/components/settings/peopleBody.jade deleteButton - // - wekan/client/components/settings/peopleBody.js deleteButton - // - wekan/client/components/sidebar/sidebar.js Popup.afterConfirm('removeMember' - // that does now remove member from board, card members and assignees correctly, - // but that should be used to remove user from all boards similarly - // - wekan/models/users.js Delete is not enabled - // - // - */ - Popup.back(); + + // Use secure server method instead of direct client-side removal + Meteor.call('removeUser', this.userId, (error, result) => { + if (error) { + if (process.env.DEBUG === 'true') { + console.error('Error removing user:', error); + } + // Show error message to user + if (error.error === 'not-authorized') { + alert('You are not authorized to delete this user.'); + } else if (error.error === 'user-not-found') { + alert('User not found.'); + } else if (error.error === 'not-authorized' && error.reason === 'Cannot delete the last administrator') { + alert('Cannot delete the last administrator.'); + } else { + alert('Error deleting user: ' + error.reason); + } + } else { + if (process.env.DEBUG === 'true') { + console.log('User deleted successfully:', result); + } + Popup.back(); + } + }); }, }); diff --git a/client/components/users/userHeader.js b/client/components/users/userHeader.js index 117921277..dd7d56b1d 100644 --- a/client/components/users/userHeader.js +++ b/client/components/users/userHeader.js @@ -241,8 +241,21 @@ Template.editProfilePopup.events({ }, 'click #deleteButton': Popup.afterConfirm('userDelete', function() { Popup.back(); - Users.remove(Meteor.userId()); - AccountsTemplates.logout(); + + // Use secure server method for self-deletion + Meteor.call('removeUser', Meteor.userId(), (error, result) => { + if (error) { + if (process.env.DEBUG === 'true') { + console.error('Error removing user:', error); + } + alert('Error deleting account: ' + error.reason); + } else { + if (process.env.DEBUG === 'true') { + console.log('User deleted successfully:', result); + } + AccountsTemplates.logout(); + } + }); }), }); diff --git a/models/users.js b/models/users.js index 64de5267c..65922edb8 100644 --- a/models/users.js +++ b/models/users.js @@ -571,27 +571,10 @@ Users.allow({ return doc._id === userId; }, remove(userId, doc) { - const adminsNumber = ReactiveCache.getUsers({ - isAdmin: true, - }).length; - const isAdmin = ReactiveCache.getUser( - { - _id: userId, - }, - { - fields: { - isAdmin: 1, - }, - }, - ); - - // Prevents remove of the only one administrator - if (adminsNumber === 1 && isAdmin && userId === doc._id) { - return false; - } - - // If it's the user or an admin - return userId === doc._id || isAdmin; + // Disable direct client-side user removal for security + // All user removal should go through the secure server method 'removeUser' + // This prevents IDOR vulnerabilities and ensures proper authorization checks + return false; }, fetch: [], }); @@ -1314,6 +1297,50 @@ Users.mutations({ }); Meteor.methods({ + // Secure user removal method with proper authorization checks + removeUser(targetUserId) { + check(targetUserId, String); + + const currentUserId = Meteor.userId(); + if (!currentUserId) { + throw new Meteor.Error('not-authorized', 'User must be logged in'); + } + + const currentUser = ReactiveCache.getUser(currentUserId); + if (!currentUser) { + throw new Meteor.Error('not-authorized', 'Current user not found'); + } + + const targetUser = ReactiveCache.getUser(targetUserId); + if (!targetUser) { + throw new Meteor.Error('user-not-found', 'Target user not found'); + } + + // Check if user is trying to delete themselves + if (currentUserId === targetUserId) { + // User can delete themselves + Users.remove(targetUserId); + return { success: true, message: 'User deleted successfully' }; + } + + // Check if current user is admin + if (!currentUser.isAdmin) { + throw new Meteor.Error('not-authorized', 'Only administrators can delete other users'); + } + + // Check if target user is the last admin + const adminsNumber = ReactiveCache.getUsers({ + isAdmin: true, + }).length; + + if (adminsNumber === 1 && targetUser.isAdmin) { + throw new Meteor.Error('not-authorized', 'Cannot delete the last administrator'); + } + + // Admin can delete non-admin users + Users.remove(targetUserId); + return { success: true, message: 'User deleted successfully' }; + }, setListSortBy(value) { check(value, String); ReactiveCache.getCurrentUser().setListSortBy(value);