Skip to content

Conversation

@bhabalan
Copy link
Contributor

@bhabalan bhabalan commented May 8, 2025

COMPLETES #ADHOC

This pull request addresses

Downloading a file uploaded from web client is failing

by making the following changes

Revert the changes made to the encryptScr() method in #4099

Change Type

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update
  • Tooling change
  • Internal code refactor

The following scenarios were tested

Linked web client locally and tested upload a file and Download the same file on web client in space and in meeting. We have actually reverted the encryptScr method change introduced in #4099 .

Vidcast: https://app.vidcast.io/share/e5600424-e5fc-4c03-9779-5b46d038b7e3

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.

@bhabalan bhabalan self-assigned this May 8, 2025
@bhabalan bhabalan requested a review from a team as a code owner May 8, 2025 06:16
@bhabalan bhabalan added the validated If the pull request is validated for automation. label May 8, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented May 8, 2025

📝 Walkthrough

Walkthrough

The changes update the encryption plugin to use optional chaining for safer property access in the handling of options within the codebase. Specifically, checks for useFileService and the presence of the allow property in options.params now use optional chaining and a more explicit key existence check. The encryptScr method is simplified by removing intermediate validation and error handling, directly converting the SCR object to JWE using the key's JWK. The corresponding test suite is updated to pass individualized options for each file URL in tests, ensuring correct behavior per test case. The JSDoc for the scr parameter is updated to indicate it expects an SCRObject.

Possibly related PRs

Tip

⚡️ Faster reviews with caching
  • CodeRabbit now supports caching for code and dependencies, helping speed up reviews. This means quicker feedback, reduced wait times, and a smoother review experience overall. Cached data is encrypted and stored securely. This feature will be automatically enabled for all accounts on May 16th. To opt out, configure Review - Disable Cache at either the organization or repository level. If you prefer to disable all data retention across your organization, simply turn off the Data Retention setting under your Organization Settings.

Enjoy the performance boost—your workflow just got faster.


📜 Recent review details

Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between f304997 and d75573b.

📒 Files selected for processing (2)
  • packages/@webex/internal-plugin-encryption/src/encryption.js (4 hunks)
  • packages/@webex/internal-plugin-encryption/test/unit/spec/encryption.js (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
  • GitHub Check: Initialize Project
  • GitHub Check: AWS Amplify Console Web Preview
🔇 Additional comments (6)
packages/@webex/internal-plugin-encryption/src/encryption.js (4)

121-121: Improved null safety with optional chaining

The addition of optional chaining (?.) here makes the code more robust against undefined or null options, preventing potential exceptions when accessing properties.


144-144: Enhanced options validation logic

The change from a simple null check to an explicit property existence check using Object.keys(options.params).indexOf('allow') > -1 ensures the 'allow' property actually exists in the params object, rather than just checking if options.params exists.


191-191: Improved documentation accuracy

The JSDoc for the scr parameter now correctly specifies it expects an SCRObject rather than plaintext, which better aligns with the actual expected parameter type.


202-202: Simplified encryptScr implementation

The implementation has been correctly simplified to directly call scr.toJWE(k.jwk), removing unnecessary intermediate steps that were likely causing the file upload/download issue. This aligns with the PR objective of fixing file download failures.

packages/@webex/internal-plugin-encryption/test/unit/spec/encryption.js (2)

28-28: Improved test coverage with individual options

Each test case now has its own specific options configuration, allowing for proper testing of different parameter combinations when calling _fetchDownloadUrl. This covers various scenarios including undefined options, options with allow flag, useFileService flag, and combinations of both.

Also applies to: 32-32, 36-36, 40-40


50-50: Enhanced test implementation

The test now correctly passes individual options for each URL instead of using a shared options object. This change ensures each URL is tested with its specific configuration, providing more accurate test coverage for the updated handling of options in the encryption module.

✨ Finishing Touches
  • 📝 Generate Docstrings

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

Support

Need help? Create a ticket on our support page for assistance with any issues or questions.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR.
  • @coderabbitai generate sequence diagram to generate a sequence diagram of the changes in this PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

@bhabalan bhabalan requested review from adhmenon and sreenara May 8, 2025 06:17
@aws-amplify-us-east-2
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-4246.d3m3l2kee0btzx.amplifyapp.com

@bhabalan bhabalan requested a review from mkesavan13 May 8, 2025 06:34
Copy link
Contributor

@mkesavan13 mkesavan13 left a comment

Choose a reason for hiding this comment

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

One comment & Can we add a Vidcast by locally linking to the WWC?

@bhabalan
Copy link
Contributor Author

bhabalan commented May 8, 2025

Copy link
Contributor

@sreenara sreenara left a comment

Choose a reason for hiding this comment

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

@bhabalan can you please add the list of tests done on the web client?
What about within the meeting window?

@bhabalan
Copy link
Contributor Author

bhabalan commented May 8, 2025

@bhabalan can you please add the list of tests done on the web client? What about within the meeting window?

@sreenara Tested upload a file and Download the same file on web client in space and in meeting. Vidcast attached above. We have actually reverted the encryptScr method change introduced in #4099 . So the changes should work as before.

@bhabalan bhabalan merged commit 9676c19 into webex:next May 8, 2025
13 of 23 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

validated If the pull request is validated for automation.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants