feat(automotive): qr code login flow for automotive app #3974
Conversation
WalkthroughThe changes introduce new functionalities to the Changes
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: 5
📜 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(1 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 252-278: Illegal return statement outside of a function
(parse)
[error] 279-288: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof Credentials
- @param {Object} options
- @returns {Object}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 288-288: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 290-290: Illegal return statement outside of a function
(parse)
[error] 294-294: Illegal return statement outside of a function
(parse)
[error] 302-351: Illegal return statement outside of a function
(parse)
[error] 352-360: Expected a statement but instead found ',
/**
Expected a statement here.
(parse)
[error] 251-279: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 288-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 360-365: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (3)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (3)
69-70: Appropriate addition of the pollingRequest property
The pollingRequest property is appropriately added and initialized to null, preparing for its use in QR code polling methods.
360-365:
Fix syntax errors in cancelQRCodePolling method due to incorrect class method syntax
The cancelQRCodePolling method has syntax errors resulting from improper indentation and the inclusion of a comma after the method definition. Ensure the method is correctly defined within the class without extra indentation or commas between methods.
Apply the following diff:
- cancelQRCodePolling() {
+ cancelQRCodePolling() {
// method body
// ...
- },
+ }- Realign the method's indentation with other class methods.
- Remove the comma after the closing brace of the method.
Likely invalid or redundant comment.
🧰 Tools
🪛 Biome
[error] 360-365: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
245-365: Verify matching braces and overall class structure
After correcting the indentation and removing unnecessary commas, ensure that all opening and closing braces {} are properly matched to maintain the correct class structure.
To verify the syntax and structure of the class, consider running the following script:
✅ Verification successful
All braces are properly matched and class structure is correct
After analyzing the file content and brace matching:
- The code is part of a well-structured class extension using
WebexPlugin.extend() - All methods (
getQRCodeLoginDetails,startQRCodePolling,cancelQRCodePolling) are properly defined within the class - Opening and closing braces are correctly matched for all methods, nested blocks, and the class definition itself
- The indentation and comma usage follows a consistent pattern
- The class definition is properly closed with
});at the end of the file
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for unmatched braces and syntax errors in the JavaScript file.
# Test: Use a JavaScript parser to check for syntax errors.
# Expected: No syntax errors reported.
npx eslint packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Length of output: 161
Script:
#!/bin/bash
# Let's examine the file content and structure using basic shell commands
cat packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Length of output: 13982
Script:
#!/bin/bash
# Search for class or object definition containing these methods
ast-grep --pattern $'({
$$$
getQRCodeLoginDetails() {
$$$
},
startQRCodePolling($$$) {
$$$
},
cancelQRCodePolling() {
$$$
}
$$$
})'
Length of output: 160
Script:
#!/bin/bash
# Look for any syntax errors by checking if the file is properly formatted
# and has matching braces
cat packages/@webex/plugin-authorization-browser-first-party/src/authorization.js | grep -n "[{}]"
Length of output: 5268
🧰 Tools
🪛 Biome
[error] 252-278: Illegal return statement outside of a function
(parse)
[error] 279-288: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof Credentials
- @param {Object} options
- @returns {Object}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 288-288: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 290-290: Illegal return statement outside of a function
(parse)
[error] 294-294: Illegal return statement outside of a function
(parse)
[error] 302-351: Illegal return statement outside of a function
(parse)
[error] 352-360: Expected a statement but instead found ',
/**
Expected a statement here.
(parse)
[error] 251-279: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 288-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 360-365: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
|
This pull request is automatically being deployed by Amplify Hosting (learn more). |
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Show resolved
Hide resolved
| auth: { | ||
| user: this.config.client_id, | ||
| pass: this.config.client_secret, | ||
| sendImmediately: true, | ||
| }, |
There was a problem hiding this comment.
I do not think you need the auth param for this request. Could you please check and link the docs if it is needed.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (6)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (4)
69-69: Add JSDoc documentation for thepollingRequestpropertyPlease add JSDoc documentation to describe the purpose and type of the
pollingRequestproperty.+ /** + * Stores the interval ID for QR code polling + * @instance + * @memberof AuthorizationBrowserFirstParty + * @type {?number} + * @private + */ pollingRequest: null,
245-250: Improve method documentationThe JSDoc needs to be more descriptive about the return value and error scenarios.
/** * Get an OAuth Login URL for QRCode. Generate QR code based on the returned URL. * @instance * @memberof Credentials + * @throws {Error} When the request fails + * @returns {Promise<{verification_uri: string, user_code: string, device_code: string, interval: number, expires_in: number}>} */
297-300: Extract magic numbers into named constantsConsider extracting the default values into named constants at the class level for better maintainability.
+ /** + * Default polling interval in seconds + * @type {number} + * @private + */ + _DEFAULT_POLLING_INTERVAL: 2, + + /** + * Default expiration time in seconds + * @type {number} + * @private + */ + _DEFAULT_EXPIRES_IN: 300, - const {device_code: deviceCode, interval = 2, expires_in: expiresIn = 300} = options; + const {device_code: deviceCode, interval = this._DEFAULT_POLLING_INTERVAL, expires_in: expiresIn = this._DEFAULT_EXPIRES_IN} = options;
354-365: Add error handling tocancelQRCodePollingThe cancel method should handle edge cases and provide feedback.
/** * cancel polling request * @instance * @memberof Credentials + * @returns {boolean} true if polling was cancelled, false if no polling was in progress */ cancelQRCodePolling() { if (this.pollingRequest) { clearInterval(this.pollingRequest); this.pollingRequest = null; + return true; } + return false; }🧰 Tools
🪛 Biome
[error] 360-365: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (2)
446-482: Enhance test coverage forgetQRCodeLoginDetails().The test suite should be expanded to include:
- Validation tests for required parameters
- Different response scenarios
- Request headers verification
Add these test cases:
describe('#getQRCodeLoginDetails()', () => { // ... existing tests ... + it('should require client_id', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + client_id: undefined + } + }); + assert.isRejected(webex.authorization.getQRCodeLoginDetails(), /client_id is required/); + }); + + it('should handle successful response with device code', () => { + const webex = makeWebex('http://example.com'); + const expectedResponse = { + device_code: 'test_device_code', + user_code: 'test_user_code', + verification_uri: 'https://example.com/verify', + interval: 5, + expires_in: 300 + }; + webex.request.resolves({ body: expectedResponse }); + + return webex.authorization.getQRCodeLoginDetails() + .then((response) => { + assert.deepEqual(response, expectedResponse); + }); + }); + + it('should send correct headers', () => { + const webex = makeWebex('http://example.com'); + webex.authorization.getQRCodeLoginDetails(); + const request = webex.request.getCall(0); + + assert.equal(request.args[0].headers['Content-Type'], 'application/x-www-form-urlencoded'); + }); });
484-590: Add missing test cases for error scenarios.The test suite should include tests for handling unexpected errors and access denied scenarios.
Add these test cases:
describe('#startQRCodePolling()', () => { // ... existing tests ... + it('should handle access_denied error', async () => { + const clock = sinon.useFakeTimers(); + const webex = makeWebex('http://example.com'); + const options = { + device_code: 'test_code', + interval: 2, + expires_in: 300 + }; + + webex.request.rejects({ + statusCode: 400, + body: { error: 'access_denied', error_description: 'User denied access' } + }); + + const promise = webex.authorization.startQRCodePolling(options); + clock.tick(2000); + + await assert.isRejected(promise, /User denied access/); + clock.restore(); + }); + + it('should handle unexpected errors', async () => { + const clock = sinon.useFakeTimers(); + const webex = makeWebex('http://example.com'); + const options = { + device_code: 'test_code', + interval: 2, + expires_in: 300 + }; + + webex.request.rejects(new Error('Unexpected error')); + + const promise = webex.authorization.startQRCodePolling(options); + clock.tick(2000); + + await assert.isRejected(promise, /Unexpected error/); + clock.restore(); + }); });
📜 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(1 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 252-278: Illegal return statement outside of a function
(parse)
[error] 279-288: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof Credentials
- @param {Object} options
- @returns {Object}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 288-288: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 290-290: Illegal return statement outside of a function
(parse)
[error] 294-294: Illegal return statement outside of a function
(parse)
[error] 302-351: Illegal return statement outside of a function
(parse)
[error] 352-360: Expected a statement but instead found ',
/**
Expected a statement here.
(parse)
[error] 251-279: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 288-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 360-365: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
261-265: Consider removing the auth parameter
As mentioned in the previous review, the auth parameter might be unnecessary for this request. Please verify if it's required according to the API documentation.
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (1)
592-620: LGTM!
The test suite for cancelQRCodePolling() has good coverage of the essential scenarios:
- Clearing interval and resetting polling request
- Handling cancellation when no polling is in progress
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (2)
446-482: Enhance test coverage forgetQRCodeLoginDetails().While the basic test cases are good, consider adding these scenarios:
- Test for missing required parameters (client_id)
- Test successful response handling and parsing
- Test with different scope combinations (single scope, multiple scopes)
describe('#getQRCodeLoginDetails()', () => { + it('should reject when client_id is missing', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + client_id: undefined + } + }); + assert.isRejected(webex.authorization.getQRCodeLoginDetails(), /client_id is required/); + }); + + it('should handle successful response', async () => { + const webex = makeWebex('http://example.com'); + const expectedResponse = { + device_code: 'device_123', + user_code: 'USER123', + verification_uri: 'https://example.com/verify', + expires_in: 300, + interval: 5 + }; + webex.request.resolves({ body: expectedResponse }); + + const result = await webex.authorization.getQRCodeLoginDetails(); + assert.deepEqual(result, expectedResponse); + }); + + it('should handle multiple scopes', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + scope: 'scope1 scope2 scope3' + } + }); + webex.authorization.getQRCodeLoginDetails(); + const request = webex.request.getCall(0); + assert.equal(request.args[0].form.scope, 'scope1 scope2 scope3'); + });
574-592: Enhance timeout test assertions.The test should be more explicit about the number of polling attempts before timeout.
it('should timeout after expires_in seconds', async () => { const clock = sinon.useFakeTimers(); const webex = makeWebex('http://example.com'); const options = { device_code: 'test_code', interval: 5, expires_in: 10 }; webex.request.rejects({statusCode: 428, body: {error: 'authorization_pending'}}); sinon.spy(webex.authorization, 'cancelQRCodePolling'); const promise = webex.authorization.startQRCodePolling(options); + // Should make 2 attempts (at 0s and 5s) before timing out at 10s clock.tick(10000); await assert.isRejected(promise, /Authorization timed out/); - assert.calledTwice(webex.authorization.cancelQRCodePolling); + assert.calledTwice(webex.request); + assert.calledOnce(webex.authorization.cancelQRCodePolling); clock.restore(); });
📜 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(1 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 261-287: Illegal return statement outside of a function
(parse)
[error] 288-298: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof AuthorizationBrowserFirstParty
- @param {Object} options
- @throws {Error} When the request fails
- @returns {Promise}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 298-298: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 300-300: Illegal return statement outside of a function
(parse)
[error] 304-304: Illegal return statement outside of a function
(parse)
[error] 312-358: Illegal return statement outside of a function
(parse)
[error] 359-367: Expected a statement but instead found ',
/**
- cancel polling request
- @instance
- @memberof AuthorizationBrowserFirstParty
- @returns {void}
*/
cancelQRCodePolling()'.
Expected a statement here.
(parse)
[error] 260-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 298-298: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 367-372: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (3)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (3)
69-77: LGTM! Property declaration is well-documented.
The pollingRequest property is properly declared with appropriate JSDoc documentation and type annotation.
361-372: LGTM! Clean implementation of polling cleanup.
The method correctly handles the cleanup of polling resources by clearing the interval and resetting the property.
🧰 Tools
🪛 Biome
[error] 367-372: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
253-288: Verify if auth parameter is necessary for the QR code login request
As mentioned in the past review comments, the auth parameter in the request config might be unnecessary for this endpoint. Please verify if this is required according to the API documentation.
✅ Verification successful
Let me search for more context about the auth usage pattern in the OAuth helper endpoints.
Let me try a different approach to find the token endpoint implementation.
Let me try one final search to find the complete implementation.
Auth parameter is required for OAuth device flow endpoints
Based on the code analysis, the auth parameter is necessary as both OAuth device flow endpoints (/actions/device/authorize and /actions/device/token) consistently use client credentials authentication. The token endpoint implementation shows the same auth configuration, confirming this is the intended authentication pattern for these endpoints.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for similar OAuth helper endpoints to verify auth usage pattern
rg -A 5 "service: 'oauth-helper'" --glob '!{test,dist,build}/**'
Length of output: 1463
Script:
#!/bin/bash
# Search for the token endpoint implementation to compare auth usage
ast-grep --pattern 'resource: "/actions/device/token"' -A 10
Length of output: 62
Script:
#!/bin/bash
# Search for the complete method implementation containing the token endpoint
rg -U "device/token.*?\{.*?\}" -A 20 --multiline
Length of output: 50
Script:
#!/bin/bash
# Search for the token endpoint method with more context
rg -A 20 "/actions/device/token" --glob '!{test,dist,build}/**'
Length of output: 2417
🧰 Tools
🪛 Biome
[error] 261-287: Illegal return statement outside of a function
(parse)
[error] 260-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
368-373: Consider returningthisfor method chainingThe implementation is correct, but consider returning
thisto allow method chaining, which is a common pattern in this codebase.cancelQRCodePolling() { if (this.pollingRequest) { clearInterval(this.pollingRequest); this.pollingRequest = null; } + return this; }🧰 Tools
🪛 Biome
[error] 368-373: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (2)
446-482: Enhance test coverage forgetQRCodeLoginDetails.The test suite should be expanded to include:
- Test case for missing client_id
- Test case for missing scope
- Test case for successful response handling
Add these test cases:
describe('#getQRCodeLoginDetails()', () => { // ... existing tests ... + it('should handle missing client_id', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + client_id: undefined, + } + }); + assert.isRejected(webex.authorization.getQRCodeLoginDetails(), /client_id is required/); + }); + + it('should handle missing scope', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + scope: undefined, + } + }); + assert.isRejected(webex.authorization.getQRCodeLoginDetails(), /scope is required/); + }); + + it('should handle successful response', async () => { + const webex = makeWebex('http://example.com'); + const expectedResponse = { + device_code: 'device_code', + user_code: 'user_code', + verification_uri: 'https://example.com/verify', + expires_in: 300, + interval: 5 + }; + webex.request.resolves({body: expectedResponse}); + + const response = await webex.authorization.getQRCodeLoginDetails(); + assert.deepEqual(response, expectedResponse); + }); });
574-592: Enhance timeout test coverage.The timeout test should verify that no more polling requests are made after the timeout occurs.
Apply this enhancement:
it('should timeout after expires_in seconds', async () => { const clock = sinon.useFakeTimers(); const webex = makeWebex('http://example.com'); const options = { device_code: 'test_code', interval: 5, expires_in: 10 }; webex.request.rejects({statusCode: 428, body: {error: 'authorization_pending'}}); sinon.spy(webex.authorization, 'cancelQRCodePolling'); const promise = webex.authorization.startQRCodePolling(options); + // First poll at 5 seconds + clock.tick(5000); + assert.calledOnce(webex.request); + // Second poll at 10 seconds (timeout) clock.tick(10000); + assert.calledTwice(webex.request); + // No more polls after timeout + clock.tick(5000); + assert.calledTwice(webex.request); await assert.isRejected(promise, /Authorization timed out/); assert.calledOnce(webex.authorization.cancelQRCodePolling); clock.restore(); });
📜 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(1 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 261-287: Illegal return statement outside of a function
(parse)
[error] 288-298: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof AuthorizationBrowserFirstParty
- @param {Object} options
- @throws {Error} When the request fails
- @returns {Promise}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 298-298: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 300-300: Illegal return statement outside of a function
(parse)
[error] 304-304: Illegal return statement outside of a function
(parse)
[error] 312-359: Illegal return statement outside of a function
(parse)
[error] 360-368: Expected a statement but instead found ',
/**
- cancel polling request
- @instance
- @memberof AuthorizationBrowserFirstParty
- @returns {void}
*/
cancelQRCodePolling()'.
Expected a statement here.
(parse)
[error] 260-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 298-298: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 368-373: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (3)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (2)
69-77: LGTM: Property declaration is well-documented
The pollingRequest property is properly declared with clear JSDoc documentation indicating its purpose and type.
270-274: Remove unnecessary auth parameter
As noted in previous reviews, the auth parameter appears unnecessary for this request. Please verify if it's required according to the API documentation.
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (1)
595-647: LGTM! Comprehensive test coverage for cancelQRCodePolling.
The test suite effectively covers:
- Stopping polling after cancellation
- Clearing intervals and resetting state
- Edge cases like cancelling when no polling is in progress
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
368-379: LGTM with a minor suggestionThe cancellation method is well-implemented. Consider adding a return value to indicate whether a polling request was actually cancelled.
cancelQRCodePolling() { + const wasPolling = Boolean(this.pollingRequest); if (this.pollingRequest) { clearInterval(this.pollingRequest); this.pollingRequest = null; } + return wasPolling; }🧰 Tools
🪛 Biome
[error] 374-379: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (3)
446-482: Enhance test coverage forgetQRCodeLoginDetails().The test suite should be expanded to include:
- Validation for missing/invalid client_id
- Validation for missing/invalid scope
- Response format validation
- Rate limiting scenarios
Add these test cases:
describe('#getQRCodeLoginDetails()', () => { // ... existing tests ... + it('should reject when client_id is missing', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + client_id: undefined + } + }); + assert.isRejected(webex.authorization.getQRCodeLoginDetails(), /client_id is required/); + }); + + it('should reject when scope is missing', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + scope: undefined + } + }); + assert.isRejected(webex.authorization.getQRCodeLoginDetails(), /scope is required/); + }); + + it('should validate response format', async () => { + const webex = makeWebex('http://example.com'); + webex.request.resolves({ + body: { + device_code: 'test_device_code', + user_code: 'test_user_code', + verification_uri: 'http://example.com/verify', + interval: 5, + expires_in: 300 + } + }); + + const response = await webex.authorization.getQRCodeLoginDetails(); + assert.hasAllKeys(response.body, [ + 'device_code', + 'user_code', + 'verification_uri', + 'interval', + 'expires_in' + ]); + }); + + it('should handle rate limiting', async () => { + const webex = makeWebex('http://example.com'); + webex.request.rejects({ + statusCode: 429, + body: { error: 'too_many_requests' } + }); + + await assert.isRejected( + webex.authorization.getQRCodeLoginDetails(), + /too_many_requests/ + ); + }); });
584-594: Enhance timeout error verification.The test should verify the complete error message format and ensure proper cleanup.
Apply this enhancement:
webex.request.rejects({statusCode: 428, body: {message: 'authorization_pending'}}); sinon.spy(webex.authorization, 'cancelQRCodePolling'); const promise = webex.authorization.startQRCodePolling(options); clock.tick(10000); - await assert.isRejected(promise, /Authorization timed out/); + await assert.isRejected(promise, { + name: 'AuthorizationError', + message: 'The current page has timed out, please refresh the page and try again.', + timeout: true + }); assert.calledTwice(webex.request); assert.calledOnce(webex.authorization.cancelQRCodePolling); + assert.isNull(webex.authorization.pollingRequest); clock.restore();
597-649: Add error handling tests for cancelQRCodePolling.The test suite should verify error handling during cancellation and multiple cancellation calls.
Add these test cases:
describe('#cancelQRCodePolling()', () => { // ... existing tests ... + it('should handle errors during cancellation gracefully', async () => { + const clock = sinon.useFakeTimers(); + const webex = makeWebex('http://example.com'); + const options = { + device_code: 'test_code', + interval: 2, + expires_in: 300 + }; + + // Simulate an error during clearInterval + const error = new Error('Failed to clear interval'); + const clearIntervalStub = sinon.stub(global, 'clearInterval').throws(error); + + webex.authorization.startQRCodePolling(options); + webex.authorization.cancelQRCodePolling(); + + assert.isNull(webex.authorization.pollingRequest); + assert.calledOnce(clearIntervalStub); + + clearIntervalStub.restore(); + clock.restore(); + }); + + it('should be idempotent for multiple cancellation calls', () => { + const clock = sinon.useFakeTimers(); + const webex = makeWebex('http://example.com'); + const clearIntervalSpy = sinon.spy(global, 'clearInterval'); + + webex.authorization.cancelQRCodePolling(); + webex.authorization.cancelQRCodePolling(); + webex.authorization.cancelQRCodePolling(); + + assert.isNull(webex.authorization.pollingRequest); + assert.notCalled(clearIntervalSpy); + + clearIntervalSpy.restore(); + clock.restore(); + }); });
📜 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(1 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 261-287: Illegal return statement outside of a function
(parse)
[error] 288-298: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof AuthorizationBrowserFirstParty
- @param {Object} options
- @throws {Error} When the request fails
- @returns {Promise}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 298-298: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 300-300: Illegal return statement outside of a function
(parse)
[error] 304-304: Illegal return statement outside of a function
(parse)
[error] 312-365: Illegal return statement outside of a function
(parse)
[error] 366-374: Expected a statement but instead found ',
/**
- cancel polling request
- @instance
- @memberof AuthorizationBrowserFirstParty
- @returns {void}
*/
cancelQRCodePolling()'.
Expected a statement here.
(parse)
[error] 260-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 298-298: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 374-379: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (2)
69-77: LGTM: Property addition is well-documented
The pollingRequest property is properly documented and serves a clear purpose for managing the QR code polling interval.
253-288: Verify the necessity of auth parameter
Based on previous discussions, let's verify if the auth parameter is truly required for the QR code login details request.
🧰 Tools
🪛 Biome
[error] 261-287: Illegal return statement outside of a function
(parse)
[error] 260-288: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (3)
446-482: Enhance test coverage forgetQRCodeLoginDetails().The test suite should be expanded to include:
- Validation of required parameters (client_id, scope)
- Successful response handling
- Different error response formats (400, 401, 500, etc.)
Add these test cases:
describe('#getQRCodeLoginDetails()', () => { // ... existing tests ... + it('should reject when client_id is missing', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + client_id: undefined + } + }); + return assert.isRejected( + webex.authorization.getQRCodeLoginDetails(), + /client_id is required/ + ); + }); + + it('should reject when scope is missing', () => { + const webex = makeWebex('http://example.com', undefined, undefined, { + credentials: { + scope: undefined + } + }); + return assert.isRejected( + webex.authorization.getQRCodeLoginDetails(), + /scope is required/ + ); + }); + + it('should handle successful response', async () => { + const webex = makeWebex('http://example.com'); + const expectedResponse = { + device_code: 'device_code', + user_code: 'user_code', + verification_uri: 'https://example.com/verify', + expires_in: 300, + interval: 5 + }; + webex.request.resolves({ body: expectedResponse }); + + const response = await webex.authorization.getQRCodeLoginDetails(); + assert.deepEqual(response, expectedResponse); + }); + + it('should handle different error responses', () => { + const webex = makeWebex('http://example.com'); + webex.request.rejects({ + statusCode: 400, + body: { error: 'invalid_request' } + }); + + return assert.isRejected( + webex.authorization.getQRCodeLoginDetails(), + /invalid_request/ + ); + }); });
575-594: Enhance timeout test with access token validation.The test should verify that the polling stops and the access token is properly handled after successful authorization.
Add token validation:
const promise = webex.authorization.startQRCodePolling(options); clock.tick(10000); await assert.isRejected(promise, /Authorization timed out/); assert.calledTwice(webex.request); assert.calledOnce(webex.authorization.cancelQRCodePolling); + // Verify no token was set + assert.isFalse(webex.credentials.canAuthorize); clock.restore();
617-669: Add tests for resource cleanup and error handling.The test suite should verify proper cleanup of resources and error handling during cancellation.
Add these test cases:
describe('#cancelQRCodePolling()', () => { // ... existing tests ... + it('should cleanup resources after cancellation', async () => { + const clock = sinon.useFakeTimers(); + const webex = makeWebex('http://example.com'); + const options = { + device_code: 'test_code', + interval: 2, + expires_in: 300 + }; + + webex.request.rejects({statusCode: 428, body: {error: 'authorization_pending'}}); + const promise = webex.authorization.startQRCodePolling(options); + + clock.tick(2000); + webex.authorization.cancelQRCodePolling(); + + await assert.isRejected(promise, /Polling cancelled/); + assert.isNull(webex.authorization.pollingRequest); + assert.isFalse(webex.credentials.canAuthorize); + clock.restore(); + }); + + it('should handle errors during cancellation gracefully', () => { + const clock = sinon.useFakeTimers(); + const webex = makeWebex('http://example.com'); + const error = new Error('Cancellation error'); + + // Force an error during cancellation + webex.authorization.pollingRequest = { + reject: () => { throw error; } + }; + + // Should not throw + assert.doesNotThrow(() => { + webex.authorization.cancelQRCodePolling(); + }); + + assert.isNull(webex.authorization.pollingRequest); + clock.restore(); + }); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js(1 hunks)
🔇 Additional comments (1)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (1)
552-573:
Fix incorrect error response format in slow_down test.
According to OAuth 2.0 Device Authorization Grant (RFC 8628), the error should be in the error field, not message.
Apply this fix:
- webex.request.onFirstCall().rejects({statusCode: 428, body: {message: 'authorization_pending'}});
- webex.request.onSecondCall().rejects({statusCode: 400, body: {message: 'slow_down'}});
+ webex.request.onFirstCall().rejects({statusCode: 428, body: {error: 'authorization_pending'}});
+ webex.request.onSecondCall().rejects({statusCode: 400, body: {error: 'slow_down'}});
webex.request.onThirdCall().resolves({statusCode: 200, body: {access_token: 'token'}});Likely invalid or redundant comment.
xinhyao
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (1)
256-296: Enhance error handling in initQRCodeLoginThe error handling could be more specific and informative:
.catch((res) => { - return Promise.reject(res); + const errorMessage = res.body?.error || 'Failed to initialize QR code login'; + this.eventEmitter.emit('qrcode-login', { + eventType: 'initialization_failed', + error: errorMessage + }); + return Promise.reject(new Error(errorMessage)); });🧰 Tools
🪛 Biome
[error] 264-295: Illegal return statement outside of a function
(parse)
[error] 263-296: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (3)
497-497: Fix redundant resolves() call.The line contains a double resolves() call which is redundant.
- webex.request.resolves().resolves({statusCode: 200, body: sampleData}); + webex.request.resolves({statusCode: 200, body: sampleData});
452-459: Extract repeated test data into a shared fixture.The sample data structure is duplicated across tests. Consider extracting it into a shared fixture or beforeEach hook to improve maintainability.
+ let sampleData; + beforeEach(() => { + sampleData = { + device_code: "test123", + expires_in: 300, + user_code: "421175", + verification_uri: "http://example.com", + verification_uri_complete: "http://example.com", + interval: 2 + }; + });Also applies to: 489-496
592-616: Enhance timeout test coverage.The timeout test should verify the error message and ensure no more requests are made after timeout.
it('should timeout after expires_in seconds', async () => { const clock = sinon.useFakeTimers(); const webex = makeWebex('http://example.com'); const options = { device_code: 'test_code', interval: 5, expires_in: 10 }; - webex.request.rejects({statusCode: 428, body: {message: 'authorization_pending'}}); + webex.request.rejects({statusCode: 428, body: {error: 'authorization_pending'}}); sinon.spy(webex.authorization, 'cancelQRCodePolling'); const emitSpy = sinon.spy(webex.authorization.eventEmitter, 'emit'); const promise = webex.authorization.startQRCodePolling(options); clock.tick(10000); + // Wait for additional interval to ensure no more requests + clock.tick(5000); await assert.isRejected(promise, /Authorization timed out/); assert.calledTwice(webex.request); + // Verify no additional requests after timeout + clock.tick(5000); + assert.calledTwice(webex.request); assert.calledOnce(webex.authorization.cancelQRCodePolling); assert.calledThrice(emitSpy); assert.equal(emitSpy.getCall(0).args[1].eventType, 'authorization_pending'); assert.equal(emitSpy.getCall(1).args[1].eventType, 'authorization_failed'); assert.equal(emitSpy.getCall(2).args[1].eventType, 'polling_canceled'); clock.restore(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js(3 hunks)packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js(2 hunks)
🧰 Additional context used
🪛 Biome
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
[error] 264-295: Illegal return statement outside of a function
(parse)
[error] 296-306: Expected a statement but instead found ',
/**
- Polling the server to check whether the user has completed authorization
- @instance
- @memberof AuthorizationBrowserFirstParty
- @param {Object} options
- @throws {Error} When the request fails
- @returns {Promise}
*/
startQRCodePolling(options ='.
Expected a statement here.
(parse)
[error] 306-306: Expected a statement but instead found ')'.
Expected a statement here.
(parse)
[error] 308-308: Illegal return statement outside of a function
(parse)
[error] 312-312: Illegal return statement outside of a function
(parse)
[error] 320-378: Illegal return statement outside of a function
(parse)
[error] 379-387: Expected a statement but instead found ',
/**
- cancel polling request
- @instance
- @memberof AuthorizationBrowserFirstParty
- @returns {void}
*/
cancelQRCodePolling()'.
Expected a statement here.
(parse)
[error] 263-296: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 306-306: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
[error] 387-395: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
🔇 Additional comments (4)
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js (2)
70-81: LGTM: Property and event emitter additions are well-documented
The new property pollingRequest is properly documented with JSDoc comments, and the EventEmitter is correctly initialized.
381-395: LGTM: Polling cancellation is properly implemented
The cancelQRCodePolling method correctly handles cleanup and provides appropriate event feedback.
🧰 Tools
🪛 Biome
[error] 387-395: This block statement doesn't serve any purpose and can be safely removed.
Standalone block statements without any block-level declarations are redundant in JavaScript and can be removed to simplify the code.
(lint/complexity/noUselessLoneBlockStatements)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (2)
17-17: LGTM!
The EventEmitter import is correctly added and necessary for handling events in the QR code login flow.
639-696: LGTM!
The cancellation test suite is comprehensive and well-structured, covering all essential scenarios including stopping polling, clearing intervals, and edge cases.
packages/@webex/plugin-authorization-browser-first-party/src/authorization.js
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js (1)
446-695: Consider implementing shared test utilities and constants.To improve maintainability and consistency across tests:
- Create shared test data fixtures for common response formats
- Extract clock management into a test utility
- Create constants for event types and error codes
- Implement shared validation helpers for common assertions
Example implementation:
// test/unit/helpers/qr-code-fixtures.js export const QR_CODE_EVENTS = { USER_CODE: 'user-code', AUTH_SUCCESS: 'authorization_success', AUTH_PENDING: 'authorization_pending', AUTH_FAILED: 'authorization_failed', POLLING_CANCELED: 'polling_canceled' }; export const QR_CODE_ERRORS = { AUTH_PENDING: { statusCode: 428, body: {error: 'authorization_pending'} }, SLOW_DOWN: { statusCode: 400, body: {error: 'slow_down'} } }; // test/unit/helpers/clock-helper.js export const withClock = (fn) => { const clock = sinon.useFakeTimers(); try { return fn(clock); } finally { clock.restore(); } };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js(1 hunks)
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Show resolved
Hide resolved
packages/@webex/plugin-authorization-browser-first-party/test/unit/spec/authorization.js
Show resolved
Hide resolved
COMPLETES #SPARK-569607
This pull request addresses
< DESCRIBE THE CONTEXT OF THE ISSUE >
Fetch QR code details and start polling the server to check whether the user has completed authorization.
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
Summary by CodeRabbit
New Features
Bug Fixes
Tests