fix(automotive): qr code for launching webex app #4012
Conversation
WalkthroughThe changes introduce a new private method Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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: 1
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
264-264: Consider making the base URL configurableThe base URL
'https://web.webex.com/deviceAuth'is hardcoded. Consider moving it to configuration to support different environments.- const baseUrl = 'https://web.webex.com/deviceAuth'; + const baseUrl = this.config.qrCodeAuthUrl || 'https://web.webex.com/deviceAuth';packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (2)
446-490: Enhance test coverage for_generateQRCodeVerificationUrl()While the current test cases cover the basic scenarios, consider adding the following test cases to improve robustness:
- Error handling when oauth-helper service is unavailable
- Handling of malformed verification URLs
- URL encoding edge cases with special characters in userCode
it('should handle errors when oauth-helper service is unavailable', () => { const verificationUrl = 'https://example.com/verify?userCode=123456'; const webex = makeWebex('http://example.com'); const oauthHelperSpy = sinon.stub(webex.internal.services, 'get').throws(new Error('Service unavailable')); const result = webex.authorization._generateQRCodeVerificationUrl(verificationUrl); assert.equal(result, verificationUrl); oauthHelperSpy.restore(); }); it('should handle malformed verification URLs', () => { const verificationUrl = 'invalid-url'; const webex = makeWebex('http://example.com'); const result = webex.authorization._generateQRCodeVerificationUrl(verificationUrl); assert.equal(result, verificationUrl); });
526-526: Strengthen assertions ininitQRCodeLogin()testThe test verifies that
_generateQRCodeVerificationUrlis called, but it should also verify:
- The correct parameters are passed to the method
- The returned URL is used appropriately in the subsequent flow
Add these assertions after line 535:
assert.calledWithExactly(webex.authorization._generateQRCodeVerificationUrl, sampleData.verification_uri); const generatedUrl = webex.authorization._generateQRCodeVerificationUrl.firstCall.returnValue; assert.equal(emitSpy.getCall(0).args[1].verificationUrl, generatedUrl);Also applies to: 535-535
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js(2 hunks)packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js(3 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 272-272: Illegal return statement outside of a function
(parse)
[error] 274-274: Illegal return statement outside of a function
(parse)
🔇 Additional comments (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
310-316: 🛠️ Refactor suggestion
Verify error handling for malformed verification URLs
The code needs to handle cases where verification_uri_complete is malformed or undefined. Consider adding error handling:
Also, verify that consumers of the qRCodeLogin event can handle potentially undefined verificationUriComplete.
- const verificationUriComplete = this._generateQRCodeVerificationUrl(verification_uri_complete);
+ let verificationUriComplete;
+ try {
+ verificationUriComplete = this._generateQRCodeVerificationUrl(verification_uri_complete);
+ } catch (error) {
+ this.logger.warn('Failed to generate QR code verification URL', error);
+ verificationUriComplete = verification_uri_complete;
+ }packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (1)
Line range hint 1-1017: Overall test implementation looks good!
The test suite is well-structured with good coverage of the QR code functionality. The suggested improvements would enhance the robustness of the tests, but the current implementation is solid.
| /** | ||
| * Generate a QR code URL to launch the Webex app when scanning with the camera | ||
| * @instance | ||
| * @memberof AuthorizationBrowserFirstParty | ||
| * @param {String} verificationUrl | ||
| * @returns {String} | ||
| */ | ||
| _generateQRCodeVerificationUrl(verificationUrl = '') { | ||
| const baseUrl = 'https://web.webex.com/deviceAuth'; | ||
| const match = verificationUrl.match(/[?&]userCode=([^&]+)/); | ||
| const userCode = match ? match[1] : null; | ||
|
|
||
| if (userCode) { | ||
| const {services} = this.webex.internal; | ||
| const oauthHelperUrl = services.get('oauth-helper'); | ||
| const params = `usercode=${userCode}&oauthhelper=${oauthHelperUrl}`; | ||
| return `${baseUrl}?${encodeURIComponent(params)}`; | ||
| } else { | ||
| return verificationUrl; | ||
| } | ||
| }, |
There was a problem hiding this comment.
Security: Improve URL handling and parameter encoding
The URL construction needs several security improvements:
- Input validation for verificationUrl
- Proper URL encoding for all parameters
- Use URL constructor instead of string concatenation
Consider this safer implementation:
_generateQRCodeVerificationUrl(verificationUrl = '') {
const baseUrl = 'https://web.webex.com/deviceAuth';
const match = verificationUrl.match(/[?&]userCode=([^&]+)/);
const userCode = match ? match[1] : null;
if (userCode) {
const {services} = this.webex.internal;
const oauthHelperUrl = services.get('oauth-helper');
- const params = `usercode=${userCode}&oauthhelper=${oauthHelperUrl}`;
- return `${baseUrl}?${encodeURIComponent(params)}`;
+ const url = new URL(baseUrl);
+ url.searchParams.set('usercode', userCode);
+ url.searchParams.set('oauthhelper', oauthHelperUrl);
+ return url.toString();
} else {
return verificationUrl;
}
},📝 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.
| /** | |
| * Generate a QR code URL to launch the Webex app when scanning with the camera | |
| * @instance | |
| * @memberof AuthorizationBrowserFirstParty | |
| * @param {String} verificationUrl | |
| * @returns {String} | |
| */ | |
| _generateQRCodeVerificationUrl(verificationUrl = '') { | |
| const baseUrl = 'https://web.webex.com/deviceAuth'; | |
| const match = verificationUrl.match(/[?&]userCode=([^&]+)/); | |
| const userCode = match ? match[1] : null; | |
| if (userCode) { | |
| const {services} = this.webex.internal; | |
| const oauthHelperUrl = services.get('oauth-helper'); | |
| const params = `usercode=${userCode}&oauthhelper=${oauthHelperUrl}`; | |
| return `${baseUrl}?${encodeURIComponent(params)}`; | |
| } else { | |
| return verificationUrl; | |
| } | |
| }, | |
| /** | |
| * Generate a QR code URL to launch the Webex app when scanning with the camera | |
| * @instance | |
| * @memberof AuthorizationBrowserFirstParty | |
| * @param {String} verificationUrl | |
| * @returns {String} | |
| */ | |
| _generateQRCodeVerificationUrl(verificationUrl = '') { | |
| const baseUrl = 'https://web.webex.com/deviceAuth'; | |
| const match = verificationUrl.match(/[?&]userCode=([^&]+)/); | |
| const userCode = match ? match[1] : null; | |
| if (userCode) { | |
| const {services} = this.webex.internal; | |
| const oauthHelperUrl = services.get('oauth-helper'); | |
| const url = new URL(baseUrl); | |
| url.searchParams.set('usercode', userCode); | |
| url.searchParams.set('oauthhelper', oauthHelperUrl); | |
| return url.toString(); | |
| } else { | |
| return verificationUrl; | |
| } | |
| }, |
🧰 Tools
🪛 Biome (1.9.4)
[error] 272-272: Illegal return statement outside of a function
(parse)
[error] 274-274: Illegal return statement outside of a function
(parse)
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
COMPLETES #SPARK-587035
This pull request addresses
It need to generate a QR Code Url as the format like native app, so that when the QRCode is scanning with camera, the QRCode can open webex app automatically.
by making the following changes
< DESCRIBE YOUR CHANGES >
Change 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
Bug Fixes
Tests