feat(messaging): only create reaction hmac if doing encryption #4032
Conversation
|
Warning There were issues while running some tools. Please review the errors and either fix the tool’s configuration or disable the tool if it’s a critical failure. 🔧 eslint
warning [email protected]: This version is no longer supported. Please see https://eslint.org/version-support for other options. (For a CapTP with native promises, see @endo/eventual-send and @endo/captp) WalkthroughThe pull request introduces modifications to the Changes
Suggested Reviewers
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
robstax
force-pushed
the
skip-reaction-hmac
branch
from
0fc2a81 to
cc9dbaf
Compare
robstax
force-pushed
the
skip-reaction-hmac
branch
from
cc9dbaf to
37a1d50
Compare
| assert.deepEqual(conversation.sendReaction.args[0][1].recipients, expected); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| describe('deleteReaction()', () => { |
There was a problem hiding this comment.
there was duplicated code here and the exact same test blocks were here twice. i deleted the duplicated blocks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/@webex/internal-plugin-conversation/src/conversation.js(1 hunks)packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js(1 hunks)
🔇 Additional comments (3)
packages/@webex/internal-plugin-conversation/test/unit/spec/conversation.js (2)
44-52: LGTM! Test verifies HMAC creation with encryption transforms enabled.
The test correctly verifies that createReactionHmac is called when includeEncryptionTransforms is true, while also validating recipient handling.
54-66: LGTM! Test verifies HMAC is skipped when encryption transforms are disabled.
The test effectively validates that createReactionHmac is not called when includeEncryptionTransforms is false, while ensuring recipient handling remains intact.
packages/@webex/internal-plugin-conversation/src/conversation.js (1)
368-372: LGTM! Recipient handling is correctly implemented.
The conditional addition of recipients to the payload is properly implemented and maintains the existing functionality.
| async addReaction(conversation, displayName, activity, recipientId) { | ||
| let hmac; | ||
| if (this.config.includeEncryptionTransforms) { | ||
| hmac = await this.createReactionHmac(displayName, activity); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add error handling for HMAC creation.
While the async/await conversion improves readability, consider adding try/catch for HMAC creation to handle potential errors gracefully.
async addReaction(conversation, displayName, activity, recipientId) {
let hmac;
if (this.config.includeEncryptionTransforms) {
- hmac = await this.createReactionHmac(displayName, activity);
+ try {
+ hmac = await this.createReactionHmac(displayName, activity);
+ } catch (error) {
+ this.logger.error('Failed to create reaction HMAC:', error);
+ throw new Error('Failed to create reaction HMAC: ' + error.message);
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async addReaction(conversation, displayName, activity, recipientId) { | |
| let hmac; | |
| if (this.config.includeEncryptionTransforms) { | |
| hmac = await this.createReactionHmac(displayName, activity); | |
| } | |
| async addReaction(conversation, displayName, activity, recipientId) { | |
| let hmac; | |
| if (this.config.includeEncryptionTransforms) { | |
| try { | |
| hmac = await this.createReactionHmac(displayName, activity); | |
| } catch (error) { | |
| this.logger.error('Failed to create reaction HMAC:', error); | |
| throw new Error('Failed to create reaction HMAC: ' + error.message); | |
| } | |
| } |
| const addReactionPayload = { | ||
| actor: {objectType: 'person', id: this.webex.internal.device.userId}, | ||
| target: { | ||
| id: conversation.id, | ||
| objectType: 'conversation', | ||
| }, | ||
| verb: 'add', | ||
| objectType: 'activity', | ||
| parent: { | ||
| type: 'reaction', | ||
| id: activity.id, | ||
| }, | ||
| object: { | ||
| objectType: 'reaction2', | ||
| displayName, | ||
| hmac, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for displayName parameter.
The displayName parameter should be validated to ensure it matches the allowed reaction types.
+ const VALID_REACTIONS = [
+ 'celebrate',
+ 'heart',
+ 'thumbsup',
+ 'smiley',
+ 'haha',
+ 'confused',
+ 'sad'
+ ];
+
const addReactionPayload = {
actor: {objectType: 'person', id: this.webex.internal.device.userId},
target: {
id: conversation.id,
objectType: 'conversation',
},
verb: 'add',
objectType: 'activity',
parent: {
type: 'reaction',
id: activity.id,
},
object: {
+ objectType: 'reaction2',
+ displayName: VALID_REACTIONS.includes(displayName) ? displayName :
+ (() => { throw new Error(`Invalid reaction type. Must be one of: ${VALID_REACTIONS.join(', ')}`); })(),
- objectType: 'reaction2',
- displayName,
hmac,
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const addReactionPayload = { | |
| actor: {objectType: 'person', id: this.webex.internal.device.userId}, | |
| target: { | |
| id: conversation.id, | |
| objectType: 'conversation', | |
| }, | |
| verb: 'add', | |
| objectType: 'activity', | |
| parent: { | |
| type: 'reaction', | |
| id: activity.id, | |
| }, | |
| object: { | |
| objectType: 'reaction2', | |
| displayName, | |
| hmac, | |
| }, | |
| }; | |
| const VALID_REACTIONS = [ | |
| 'celebrate', | |
| 'heart', | |
| 'thumbsup', | |
| 'smiley', | |
| 'haha', | |
| 'confused', | |
| 'sad' | |
| ]; | |
| const addReactionPayload = { | |
| actor: {objectType: 'person', id: this.webex.internal.device.userId}, | |
| target: { | |
| id: conversation.id, | |
| objectType: 'conversation', | |
| }, | |
| verb: 'add', | |
| objectType: 'activity', | |
| parent: { | |
| type: 'reaction', | |
| id: activity.id, | |
| }, | |
| object: { | |
| objectType: 'reaction2', | |
| displayName: VALID_REACTIONS.includes(displayName) ? displayName : | |
| (() => { throw new Error(`Invalid reaction type. Must be one of: ${VALID_REACTIONS.join(', ')}`); })(), | |
| hmac, | |
| }, | |
| }; |
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
This pull request addresses
there are use cases for the SDK without an encryption key, such as the webcast/recording player widgets. in those cases, the SDK is iframed and the parent page handles encryption/decryption and the JS SDK has no access to encryption keys. generating a reaction hmac requires a key, so in those cases, we should skip hmac generation and let the parent page do it. i refactored addReaction to an async function for maintainability/readability and updated tests. it looks like there was a bad merge at one point in the unit tests and there were duplicate identical tests, so i also removed the duplicated tests
by making the following changes
there is already a config in the conversation plugin
includeEncryptionTransforms. if that is true, create the hmac, if not, skip and leave it undefinedChange Type
The following scenarios were tested
< ENUMERATE TESTS PERFORMED, WHETHER MANUAL OR AUTOMATED >
I certified that
I have read and followed contributing guidelines
I discussed changes with code owners prior to submitting this pull request
I have not skipped any automated checks
All existing and new tests passed
I have updated the documentation accordingly
Make sure to have followed the contributing guidelines before submitting.
Summary by CodeRabbit
New Features
addReactionmethod to support asynchronous operations, improving performance and readability.includeEncryptionTransformsthat modifies HMAC generation behavior.Bug Fixes
addReactionmethod to provide clearer feedback on incorrect usage.Tests
addReactionmethod to validate behavior based on theincludeEncryptionTransformsconfiguration.