EPMRPP-108464 || OAuth password grant type support - #233
Conversation
WalkthroughAdds OAuth 2.0 Password Grant authentication: new OAuthInterceptor with token acquisition/refresh logic, config parsing and types, tests, README updates, and wiring to RestClient/ReportPortal client to optionally inject Bearer tokens instead of using an API key. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant RestClient
participant OAuthInterceptor as OAuthInterceptor
participant TokenServer as TokenServer
participant RP as ReportPortalAPI
Client->>RestClient: makeRequest()
RestClient->>OAuthInterceptor: request interceptor invoked
alt No valid token / token expiring
OAuthInterceptor->>OAuthInterceptor: check token / in-flight promise
alt renewal in-flight
OAuthInterceptor->>OAuthInterceptor: await existing renewal
else start renewal
OAuthInterceptor->>TokenServer: POST /token (password or refresh)
TokenServer-->>OAuthInterceptor: { access_token, refresh_token?, expires_in? }
OAuthInterceptor->>OAuthInterceptor: cache token, set expiry, clear in-flight
end
end
OAuthInterceptor->>RestClient: add Authorization: Bearer <token>
RestClient->>RP: proxied authenticated request
RP-->>RestClient: response
RestClient-->>Client: return response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.d.ts (1)
247-247: Type/impl mismatch: updateLaunch signature is wrongJS implementation is updateLaunch(launchId, options), but d.ts declares only (options). Fix the public type to prevent TS consumers from breaking.
Apply this diff:
- updateLaunch(options: LaunchOptions): { tempId: string; promise: Promise<any> }; + updateLaunch(launchId: string, options: LaunchOptions): { tempId: string; promise: Promise<any> };
🧹 Nitpick comments (6)
index.d.ts (1)
71-75: Mark token as deprecated in the public typesDocs say token is deprecated. Add a JSDoc deprecation notice so IDEs warn.
- token?: string; + /** + * Deprecated: use apiKey or oauth instead. + * @deprecated + */ + token?: string;README.md (1)
80-81: Add a security note about handling credentialsRecommend not hardcoding username/password; prefer environment variables or a secrets manager. Add a short note and example.
**Note:** The OAuth interceptor automatically handles token refresh when the token is about to expire (1 minute before expiration). + +Security tip: Avoid hardcoding OAuth credentials in source control. Prefer environment variables or a secrets manager. + +Example: +```javascript +const rpClient = new RPClient({ + endpoint: process.env.RP_ENDPOINT, + launch: process.env.RP_LAUNCH, + project: process.env.RP_PROJECT, + oauth: { + tokenEndpoint: process.env.RP_OAUTH_TOKEN_ENDPOINT, + username: process.env.RP_OAUTH_USERNAME, + password: process.env.RP_OAUTH_PASSWORD, + clientId: process.env.RP_OAUTH_CLIENT_ID, + clientSecret: process.env.RP_OAUTH_CLIENT_SECRET, + scope: process.env.RP_OAUTH_SCOPE, + } +}); +```lib/commons/config.js (1)
81-88: Don’t emit token deprecation warning when OAuth is configuredCurrently you still call getApiKey() with OAuth present, which can log a deprecation warning if token is set even though it’s unused. Read apiKey directly to avoid noise.
- } else { - // If OAuth is configured, apiKey is optional (use it if provided) - try { - apiKey = getApiKey(options); - } catch (error) { - // Ignore error if OAuth is configured - apiKey = null; - } - } + } else { + // If OAuth is configured, apiKey is optional; do not read deprecated `token` + apiKey = options.apiKey ?? null; + }lib/oauth.js (2)
1-2: Ensure URLSearchParams availability across Node versionsBe explicit to avoid relying on a global in older runtimes.
-const axios = require('axios'); +const axios = require('axios'); +const { URLSearchParams } = require('url');
153-169: Optional: refresh-on-401 response interceptor (single retry)Improves resilience if tokens are invalidated server-side before local expiry. Retry once to avoid loops; keep idempotency in mind.
attach(axiosInstance) { axiosInstance.interceptors.request.use( async (config) => { @@ (error) => Promise.reject(error), ); + + // Optional: single retry on 401 with fresh token + axiosInstance.interceptors.response.use( + (resp) => resp, + async (error) => { + const cfg = error?.config || {}; + if (error?.response?.status === 401 && !cfg._rpAuthRetried) { + cfg._rpAuthRetried = true; + // drop possibly bad token and fetch a new one + this.accessToken = null; + this.tokenExpiresAt = null; + try { + const token = await this.getAccessToken(); + cfg.headers = { ...(cfg.headers || {}), Authorization: `Bearer ${token}` }; + return axiosInstance.request(cfg); + } catch { + // fall through + } + } + return Promise.reject(error); + }, + ); }Based on learnings
lib/rest.js (1)
45-45: Use logger module for consistency.Line 45 uses
console.errordirectly, while line 52 uses theloggermodule. For consistency with the rest of the codebase, consider using the logger module here as well.Apply this diff to use the logger module:
- console.error('[RestClient] Failed to initialize OAuth interceptor:', error.message); + logger.error('[RestClient] Failed to initialize OAuth interceptor:', error.message);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
README.md(2 hunks)__tests__/oauth.spec.js(1 hunks)index.d.ts(2 hunks)lib/commons/config.js(3 hunks)lib/oauth.js(1 hunks)lib/report-portal-client.js(1 hunks)lib/rest.js(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
lib/oauth.js (1)
lib/rest.js (2)
axios(1-1)OAuthInterceptor(6-6)
lib/rest.js (1)
lib/oauth.js (1)
axios(1-1)
🔇 Additional comments (12)
lib/report-portal-client.js (1)
43-56: Conditional auth header and OAuth interceptor attachment verifiedRestClient correctly instantiates OAuthInterceptor with oauthConfig and debug (lib/rest.js:39), attaches it to axios instance (lib/rest.js:43), and handles errors gracefully. Authorization header is conditionally set only when OAuth is absent. All concerns confirmed.
__tests__/oauth.spec.js (9)
1-22: LGTM! Clean test setup.The test configuration is well-structured with clear constants and appropriate mocking scope.
24-57: LGTM! Comprehensive password grant test.This test thoroughly validates the initial token acquisition flow, including request parameters, headers, and token expiry calculation.
59-72: LGTM! Good coverage of token caching.This test properly validates that cached tokens are reused when not expiring soon, avoiding unnecessary network calls.
74-99: LGTM! Good coverage of refresh flow and default expiration.This test validates both the refresh token flow and the default expiration fallback when
expires_inis missing from the response (line 96). This defensive behavior is important for resilience.
101-131: LGTM! Excellent concurrency test.This test validates crucial in-flight request deduplication, preventing token refresh stampedes when multiple concurrent requests need tokens. The cleanup validation (line 128) ensures no memory leaks.
133-151: LGTM! Solid error handling coverage.This test validates both error logging and thrown error message formatting, ensuring failures are properly communicated.
153-161: LGTM! Adequate debug logging test.Simple and effective validation of debug mode behavior.
163-187: LGTM! Good integration test for header injection.This test validates the complete flow of attaching the OAuth interceptor and injecting the Authorization header with a Bearer token.
189-217: LGTM! Critical resilience test.This test validates graceful degradation when token acquisition fails, ensuring OAuth issues don't block all requests. The request continues without Authorization (line 208), which allows fallback to API key authentication.
lib/rest.js (2)
6-6: LGTM! Clean import.
26-27: LGTM! Appropriate field storage.These fields are properly stored and forwarded to OAuthInterceptor.
…he token lifetime
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
lib/oauth.js (1)
186-202: Past discussion: Consider defensive coding for headers object.As noted in the previous review,
config.headerscould be undefined. You mentioned this is guarded inreport-portal-client.jsline 43. While that's a valid approach, defensive coding here would makeattach()more robust if used in other contexts:const token = await this.getAccessToken(); // eslint-disable-next-line no-param-reassign - config.headers.Authorization = `Bearer ${token}`; + config.headers = { ...(config.headers || {}), Authorization: `Bearer ${token}` }; this.logDebug(`Request to ${config.url} with OAuth token`);This ensures the interceptor is self-contained and doesn't rely on external initialization.
🧹 Nitpick comments (2)
lib/oauth.js (2)
21-34: Add input validation for required parameters.The constructor accepts configuration but doesn't validate required fields. Consider adding validation to fail fast with clear error messages:
constructor(config) { + if (!config.tokenEndpoint || typeof config.tokenEndpoint !== 'string') { + throw new Error('OAuth config: tokenEndpoint is required and must be a non-empty string'); + } + if (!config.username || typeof config.username !== 'string') { + throw new Error('OAuth config: username is required and must be a non-empty string'); + } + if (!config.password || typeof config.password !== 'string') { + throw new Error('OAuth config: password is required and must be a non-empty string'); + } + if (!config.clientId || typeof config.clientId !== 'string') { + throw new Error('OAuth config: clientId is required and must be a non-empty string'); + } + this.tokenEndpoint = config.tokenEndpoint; this.username = config.username;
128-128: Consider importing URLSearchParams for consistency.While
URLSearchParamsis available as a global in Node.js 10+, other files in the project (e.g.,report-portal-client.js) explicitly import it from theurlmodule. For codebase consistency:+const { URLSearchParams } = require('url'); + const TOKEN_REFRESH_THRESHOLD_MS = 60000;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
__tests__/oauth.spec.js(1 hunks)lib/oauth.js(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/oauth.spec.js
🧰 Additional context used
🧬 Code graph analysis (1)
lib/oauth.js (3)
__tests__/oauth.spec.js (4)
axios(1-1)TOKEN_REFRESH_THRESHOLD_MS(17-17)DEFAULT_TOKEN_EXPIRATION_MS(18-18)OAuthInterceptor(2-2)lib/rest.js (2)
axios(1-1)OAuthInterceptor(6-6)lib/report-portal-client.js (4)
require(3-3)require(6-6)require(8-8)require(9-9)
🔇 Additional comments (3)
lib/oauth.js (3)
1-8: Well-structured constants for OAuth timing and grant types.The token refresh threshold (1 minute before expiry) and default expiration (1 hour) are sensible defaults. Using named constants for grant types improves maintainability.
46-75: Excellent concurrency handling for token acquisition.The use of
tokenRenewPromiseto cache in-flight renewal requests prevents duplicate concurrent token fetches. The cleanup in thefinallyblock ensures the promise is always cleared. Well done.
81-119: Robust fallback mechanism from refresh token to password grant.The graceful degradation when refresh tokens fail—clearing the invalid token and falling back to password grant—prevents auth loops and provides good resilience. Error messages are detailed and helpful for debugging.
1 participant
Summary by CodeRabbit
New Features
Documentation
Tests