From bf643b479764a495a210f703b6386926b2e4bc1d Mon Sep 17 00:00:00 2001 From: WolverinDEV Date: Mon, 12 Apr 2021 19:12:02 +0200 Subject: [PATCH] Fixed the default channel group assignment --- server/src/TS3ServerClientManager.cpp | 28 +++++++++---------- server/src/client/command_handler/channel.cpp | 12 ++++---- server/src/groups/GroupAssignmentManager.cpp | 8 ++++-- 3 files changed, 27 insertions(+), 21 deletions(-) diff --git a/server/src/TS3ServerClientManager.cpp b/server/src/TS3ServerClientManager.cpp index f5ec51a..2fa8f03 100644 --- a/server/src/TS3ServerClientManager.cpp +++ b/server/src/TS3ServerClientManager.cpp @@ -409,6 +409,11 @@ void VirtualServer::delete_channel(shared_ptr channel, const } } +/* + * This method had previously owned the clients command lock but that's not really needed. + * Everything which is related to the server channel tree or the client channel tree should be locked with + * the appropiate mutexes. + */ void VirtualServer::client_move( const shared_ptr &target, shared_ptr target_channel, @@ -419,32 +424,27 @@ void VirtualServer::client_move( std::unique_lock &server_channel_write_lock) { TIMING_START(timings); - if(server_channel_write_lock.owns_lock()) { + if(!server_channel_write_lock.owns_lock()) { server_channel_write_lock.unlock(); } - - lock_guard client_command_lock(target->command_lock); - server_channel_write_lock.lock(); TIMING_STEP(timings, "chan tree l"); if(target->currentChannel == target_channel) { return; } - /* first step: resolve the target channel / or fix missing */ + /* first step: verify thew source and target channel */ auto s_target_channel = dynamic_pointer_cast(target_channel); auto s_source_channel = dynamic_pointer_cast(target->currentChannel); assert(!target->currentChannel || s_source_channel != nullptr); - deque client_updates; - std::deque changed_groups{}; + std::deque updated_client_properties{}; if(target_channel) { assert(s_target_channel); if(s_target_channel->deleted) { - target_channel = this->channelTree->getDefaultChannel(); - s_target_channel = dynamic_pointer_cast(target_channel); - assert(s_target_channel); + return; } } + auto l_target_channel = s_target_channel ? this->channelTree->findLinkedChannel(s_target_channel->channelId()) : nullptr; auto l_source_channel = s_source_channel ? this->channelTree->findLinkedChannel(s_source_channel->channelId()) : nullptr; TIMING_STEP(timings, "channel res"); @@ -559,9 +559,9 @@ void VirtualServer::client_move( target->properties()[property::CLIENT_IS_TALKER] = 0; target->properties()[property::CLIENT_TALK_REQUEST] = 0; target->properties()[property::CLIENT_TALK_REQUEST_MSG] = ""; - client_updates.push_back(property::CLIENT_IS_TALKER); - client_updates.push_back(property::CLIENT_TALK_REQUEST); - client_updates.push_back(property::CLIENT_TALK_REQUEST_MSG); + updated_client_properties.push_back(property::CLIENT_IS_TALKER); + updated_client_properties.push_back(property::CLIENT_TALK_REQUEST); + updated_client_properties.push_back(property::CLIENT_TALK_REQUEST_MSG); } TIMING_STEP(timings, "src chan up"); } @@ -600,7 +600,7 @@ void VirtualServer::client_move( } client_channel_lock.unlock(); /* both methods lock if they require stuff */ - this->notifyClientPropertyUpdates(target, client_updates, s_source_channel ? true : false); + this->notifyClientPropertyUpdates(target, updated_client_properties, s_source_channel ? true : false); TIMING_STEP(timings, "notify cpro"); if(s_target_channel) { target->updateChannelClientProperties(false, s_source_channel ? true : false); diff --git a/server/src/client/command_handler/channel.cpp b/server/src/client/command_handler/channel.cpp index 97d5d88..cec026c 100644 --- a/server/src/client/command_handler/channel.cpp +++ b/server/src/client/command_handler/channel.cpp @@ -593,13 +593,12 @@ command_result ConnectedClient::handleCommandChannelCreate(Command &cmd) { auto admin_group_id = this->server->properties()[property::VIRTUALSERVER_DEFAULT_CHANNEL_ADMIN_GROUP].as_or(0); auto admin_group = this->server->group_manager()->channel_groups()->find_group(groups::GroupCalculateMode::GLOBAL, admin_group_id); if(!admin_group) { - logError(this->getServerId(), "Missing servers default channel admin group {}. Using the default channel gropup.", admin_group_id); - admin_group = this->server->default_channel_group(); + logError(this->getServerId(), "Missing servers default channel admin group {}.", admin_group_id); } /* admin_group might still be null since default_channel_group() could return nullptr */ if(admin_group) { - this->server->group_manager()->assignments().set_channel_group(this->getClientDatabaseId(), created_channel->channelId(), admin_group->group_id(), false); + this->server->group_manager()->assignments().set_channel_group(this->getClientDatabaseId(), admin_group->group_id(), created_channel->channelId(), false); serverInstance->action_logger()->group_assignment_logger.log_group_assignment_remove( this->getServerId(), this->server->getServerRoot(), log::GroupTarget::CHANNEL, @@ -1194,8 +1193,11 @@ ts::command_result ConnectedClient::execute_channel_edit(ChannelId& channel_id, } } + /* We need to calculate the type for other checks as well do don't only calculate it when updating the type */ ChannelType::ChannelType target_channel_type; - if(updating_type) { + + (void) updating_type; + { auto flag_permanent = converter::from_string_view(target_channel_property_value(property::CHANNEL_FLAG_PERMANENT)); auto flag_semi_permanent = converter::from_string_view(target_channel_property_value(property::CHANNEL_FLAG_SEMI_PERMANENT)); @@ -1228,7 +1230,7 @@ ts::command_result ConnectedClient::execute_channel_edit(ChannelId& channel_id, * We may should test if the user really set them to -1 but nvm. * * Since we must come from a limited channel, else we would not have a change, we can ensure that the previous value isn't -1. - * This means that we've definatifly a change. + * This means that we've definitively a change. */ changed_values[property::CHANNEL_MAXCLIENTS] = "-1"; } else if(!changed_values.contains(property::CHANNEL_MAXCLIENTS)) { diff --git a/server/src/groups/GroupAssignmentManager.cpp b/server/src/groups/GroupAssignmentManager.cpp index e04620d..c7a3bda 100644 --- a/server/src/groups/GroupAssignmentManager.cpp +++ b/server/src/groups/GroupAssignmentManager.cpp @@ -578,15 +578,19 @@ GroupAssignmentResult GroupAssignmentManager::set_channel_group(ClientDbId clien { std::lock_guard cache_lock{*this->client_cache_lock}; for(auto& entry : this->client_cache) { - if(entry->client_database_id != client) continue; + if(entry->client_database_id != client) { + continue; + } auto it = std::find_if(entry->channel_group_assignments.begin(), entry->channel_group_assignments.end(), [&](const std::unique_ptr& assignment) { return assignment->channel_id == channel_id; }); if(it != entry->channel_group_assignments.end()) { if(group) { - if((*it)->group_id == group) + if((*it)->group_id == group) { return GroupAssignmentResult::SET_ALREADY_MEMBER_OF_GROUP; + } + (*it)->group_id = group; } else { entry->channel_group_assignments.erase(it);