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

FEAT(client): Introduced new mumble API #6425

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mryamac
Copy link
Contributor

@mryamac mryamac commented May 22, 2024

This patch introduces a new API to link, unlink, start to listen, stop the listen the channels and send the messages to the plugins

Checks

@mryamac mryamac marked this pull request as ready for review May 22, 2024 08:43
@Krzmbrzl Krzmbrzl added client feature-request This issue or PR deals with a new feature labels May 22, 2024
Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

Sorry for haven taken so long to review this. The proposed changes look mostly fine to me. The review comments are mostly about some minor coding style detail, some rewording of documentation or some small generalization of the proposed API.

plugins/MumblePlugin.h Outdated Show resolved Hide resolved
plugins/MumblePlugin.h Outdated Show resolved Hide resolved
plugins/MumblePlugin.h Outdated Show resolved Hide resolved
plugins/MumblePlugin.h Outdated Show resolved Hide resolved
src/mumble/API_v_1_x_x.cpp Outdated Show resolved Hide resolved
src/mumble/API_v_1_x_x.cpp Outdated Show resolved Hide resolved
src/mumble/API_v_1_x_x.cpp Outdated Show resolved Hide resolved
src/mumble/API_v_1_x_x.cpp Outdated Show resolved Hide resolved
src/mumble/API_v_1_x_x.cpp Outdated Show resolved Hide resolved
src/mumble/API_v_1_x_x.cpp Outdated Show resolved Hide resolved
@Krzmbrzl
Copy link
Member

Krzmbrzl commented Oct 4, 2024

@mryamac could you please rebase your branch against current master? It seems like FreeBSD has bricked the Qt package again. However, we have recently switched to Qt 6 anyway so maybe that is enough to resolve the issue.

This patch introduces a new API to link, unlink, start to listen,
stop the listen the channels and send the messages to the plugins
Copy link
Member

@Krzmbrzl Krzmbrzl left a comment

Choose a reason for hiding this comment

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

I just noticed two more things

  1. Linking channels and creating listeners in channels is subject to ACL restrictions. If the local user is lacking the respective permissions, the actions will (silently) fail. That's just what it is but one scenario that is not covered by this is the case in which the server admin explicitly grants certain permissions to the plugin that the user doesn't have. This is already possible for e.g. the requestUserMove function which accepts a password which can offer special ACLs for this specific call from the plugin. I think that the link- and listen-related API function should also allow for a password to be provided for such a purpose.
  2. This PR introduces a function to send a text message to a group of users. I would propose to extend the function signature to also allow sending text messages to a given set of channels. Then the function can be used to either send to users or to channels or both at the same time allowing for maximum flexibility. Sending text messages to channels introduces ACL restrictions though so the same password parameter as proposed for the other functions would apply.

Comment on lines +1729 to +1735
* @param message The message to send (UTF-8 encoded)
* @param messageSize The size (in bytes) of the message
* @returns The error code. If everything went well, STATUS_OK will be returned.
*/
mumble_error_t(MUMBLE_PLUGIN_CALLING_CONVENTION *requestSendUserTextMessage)(
mumble_plugin_id_t callerID, mumble_connection_t connection, mumble_userid_t *users, std::size_t userAmount,
const char *message, std::size_t messageSize);
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* @param message The message to send (UTF-8 encoded)
* @param messageSize The size (in bytes) of the message
* @returns The error code. If everything went well, STATUS_OK will be returned.
*/
mumble_error_t(MUMBLE_PLUGIN_CALLING_CONVENTION *requestSendUserTextMessage)(
mumble_plugin_id_t callerID, mumble_connection_t connection, mumble_userid_t *users, std::size_t userAmount,
const char *message, std::size_t messageSize);
* @param message The message to send (UTF-8 encoded as a C-string)
* @returns The error code. If everything went well, STATUS_OK will be returned.
*/
mumble_error_t(MUMBLE_PLUGIN_CALLING_CONVENTION *requestSendUserTextMessage)(
mumble_plugin_id_t callerID, mumble_connection_t connection, mumble_userid_t *users, std::size_t userAmount,
const char *message);

Sorry for reverting my own suggestion here. I noticed that we are using C-strings in other parts of the API already which inhibits null bytes from being part of it but since we use that for passwords, we can certainly use this for text messages which indeed can't contain null bytes.

@@ -47,12 +47,22 @@
EXIT_WITH(MUMBLE_EC_INVALID_PLUGIN_ID); \
}

#define VERIFY_CHANNEL_ID(id) \
Copy link
Member

Choose a reason for hiding this comment

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

please #undef all macros at the end of the file


VERIFY_PLUGIN_ID(callerID);

VERIFY_CONNECTION(connection);
Copy link
Member

Choose a reason for hiding this comment

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

All new implementations have to also check that the connection is synchronized.

Comment on lines +61 to +65
#define VERIFY_USER_ID(id) \
if (!ClientUser::get(id)) { \
EXIT_WITH(MUMBLE_EC_USER_NOT_FOUND); \
}

Copy link
Member

Choose a reason for hiding this comment

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

semantically it would make sense to keep the macros related to the connection together and then define this new macro either before both of them or after (but not in between)

@mryamac
Copy link
Contributor Author

mryamac commented Oct 5, 2024

I just noticed two more things

  1. Linking channels and creating listeners in channels is subject to ACL restrictions. If the local user is lacking the respective permissions, the actions will (silently) fail. That's just what it is but one scenario that is not covered by this is the case in which the server admin explicitly grants certain permissions to the plugin that the user doesn't have. This is already possible for e.g. the requestUserMove function which accepts a password which can offer special ACLs for this specific call from the plugin. I think that the link- and listen-related API function should also allow for a password to be provided for such a purpose.
  2. This PR introduces a function to send a text message to a group of users. I would propose to extend the function signature to also allow sending text messages to a given set of channels. Then the function can be used to either send to users or to channels or both at the same time allowing for maximum flexibility. Sending text messages to channels introduces ACL restrictions though so the same password parameter as proposed for the other functions would apply.

For the first item, requestUserMove function uses
joinChannel(unsigned int uiSession, unsigned int channel, const QStringList &temporaryAccessTokens)

This function takes tempAccessToken, but in my cases, link/unlink vs. functions don't have any parameters like that. So, to be able to give permission, do I need to modify the ServerHandler class?

Item two seems clear to me. I will implement this API, but first, I need to understand the ACL case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client feature-request This issue or PR deals with a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants