fix(meetings): handle getMe error due to insufficient privileges #4011
Conversation
WalkthroughThe changes in this pull request involve refactoring the Changes
Possibly related PRs
Suggested reviewers
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) 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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-meetings/src/meetings/index.ts (2)
1047-1093: Improve error handling for guest user check.The error handling for the
_getMe()call could be more specific to help with debugging. Consider catching specific error types and providing more detailed error messages.Apply this diff to improve error handling:
return this.webex.people ._getMe() .then((me) => { const isGuestUser = me.type === 'appuser'; if (!isGuestUser) { return this.request.getMeetingPreferences() .then((res) => { // ... existing code ... }); } return Promise.resolve(); }) .catch((error) => { - LoggerProxy.logger.error('Failed to retrieve user information. No site will be set'); + LoggerProxy.logger.error( + 'Failed to retrieve user information. No site will be set.', + { + errorName: error.name, + errorMessage: error.message, + errorStack: error.stack + } + ); + // Re-throw specific errors that should be handled by the caller + if (error.name === 'NotAuthorizedError') { + throw error; + } });
1060-1083: Enhance the fallback mechanism for preferred site retrieval.The current implementation has a good fallback strategy but could benefit from more detailed logging to track which source provided the preferred site.
Apply this diff to improve logging:
if (!this.preferredWebexSite) { + LoggerProxy.logger.info('Falling back to user preferences for preferred site'); return this.webex.internal.user .get() .then((user) => { const preferredWebexSite = user?.userPreferences?.userPreferencesItems?.preferredWebExSite; if (preferredWebexSite) { + LoggerProxy.logger.info( + `Retrieved preferred site from user preferences: ${preferredWebexSite}` + ); this.preferredWebexSite = preferredWebexSite; this.webex.internal.services ._getCatalog() .addAllowedDomains([preferredWebexSite]); } else { throw new Error('site not found'); } }) .catch(() => { LoggerProxy.logger.error( 'Failed to fetch preferred site from user - no site will be set' ); }); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-meetings/src/meetings/index.ts(1 hunks)packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js(2 hunks)
🔇 Additional comments (3)
packages/@webex/plugin-meetings/src/meetings/index.ts (1)
1052-1058: 🛠️ Refactor suggestion
Verify the domain before adding it to allowed domains.
The code adds the preferred site to allowed domains without validating the domain format.
Run the following script to check for domain validation patterns in the codebase:
Apply this diff to add domain validation:
if (res) {
const preferredWebexSite = MeetingsUtil.parseDefaultSiteFromMeetingPreferences(res);
+ if (!MeetingsUtil.isValidDomain(preferredWebexSite)) {
+ LoggerProxy.logger.warn(
+ `Invalid domain format for preferred site: ${preferredWebexSite}`
+ );
+ return;
+ }
this.preferredWebexSite = preferredWebexSite;
this.webex.internal.services._getCatalog().addAllowedDomains([preferredWebexSite]);
}packages/@webex/plugin-meetings/test/unit/spec/meetings/index.js (2)
2080-2094: LGTM! Well-structured test case for error handling.
The test case properly verifies the error handling behavior when user information retrieval fails due to insufficient scopes. It checks both the state update (empty preferred site) and error logging.
2111-2111: LGTM! Good refactoring of test setup.
Moving the _getMe stub setup into the setup helper function improves code organization and maintainability.
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
| return Promise.resolve(); | ||
| }) | ||
| .catch(() => { | ||
| LoggerProxy.logger.error('Failed to retrieve user information. No site will be set'); |
There was a problem hiding this comment.
| LoggerProxy.logger.error('Failed to retrieve user information. No site will be set'); | |
| LoggerProxy.logger.error('Failed to retrieve user information. No preferredWebexSite will be set'); |
COMPLETES # ADHOC
This pull request addresses
a blocker to join meetings when the
spark:people_readscope is missing from an access token. The issue was found on the Control Hub while they were joining meetings.This issue is being caused by #3908 which doesn't handle a failure while calling the
getMe()function. Due to this, the entirewebex.meetings.register()function stops executing and a developer cannot proceed with registration and joining a meeting.Due to this, any developers who upgrade to v3.6.0 and don't have the
spark:people_readscope will get blocked from joining meetings.Error:
by making the following changes
Added a catch to ensure the registration doesn't get blocked due to the failure to get user information.
Change Type
The following scenarios were tested
Logs.zip
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.