Repository navigation
feat: surface OAuth validation on Catalog cards - #222
Conversation
5d539c2 to
99641b3
Compare
0c59e8e to
c71e85c
Compare
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
c71e85c to
3c0a3ea
Compare
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
3c0a3ea to
ba9a487
Compare
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
None of the issues are blocking, just major or suggestions
| } | ||
| return ( | ||
| (error instanceof OAuthAuthorizationError && error.description) || | ||
| (error instanceof OAuthAuthorizationError && oauthErrorMessage(intl, error.errorCode)) || |
There was a problem hiding this comment.
catalogOAuthErrorMessage returns only oauthErrorMessage(intl, error.errorCode). The Servers page appends Provider message: …, so an unlisted code shows only generic guidance here. This is the same issue raised on #221. Please reuse the Servers logic as a shared helper. Also, oauthErrorMessage always returns a string, so the mcpServer.catalog.oauth.authorizationError fallback on line 376 is unreachable for OAuthAuthorizationError.
| gateway.authType !== "oauth" || | ||
| gateway.oauthConfig?.grant_type !== "authorization_code" |
There was a problem hiding this comment.
This resolver checks only enabled, authType !== "oauth" and grant_type. It skips the token status, and the authType comparison is case-sensitive, so "OAuth" is wrongly rejected as request_invalid. Can we use canValidateOAuthConnection(gateway, oauthStatuses[id]), as Servers.tsx does?
| {canTest && | ||
| (!isCatalogOAuthServer(server) || | ||
| oauthStatuses?.[server.gateway_id ?? ""]?.state === "not_applicable" || | ||
| canTestOAuth) && ( | ||
| <DropdownMenuItem | ||
| disabled={ | ||
| !oauthUsable || | ||
| isAdding || | ||
| isTesting || | ||
| isDisconnecting || | ||
| (isCatalogOAuthServer(server) && | ||
| oauthStatuses?.[server.gateway_id ?? ""]?.state !== | ||
| "not_applicable" && | ||
| !!canTestOAuthServer && | ||
| !canTestOAuthServer(server)) |
There was a problem hiding this comment.
Duplicated and overlapping conditions:
The "OAuth and not not_applicable" check appears in this render guard and in disabled, and a third time in handleTest (ServerCatalog.tsx:920-921). canTestOAuth (line 141) and canTestOAuthServer (line 140) overlap, and with !!canTestOAuthServer && (line 301), omitting the prop silently skips the check. Please extract one helper, or collapse the two props into one.
| ) { | ||
| return; | ||
| } | ||
| const handleTest = async (server: CatalogServer) => { |
There was a problem hiding this comment.
handleTest lost its useCallback**
It is recreated every render. That's harmless today because it's only used through inline lambdas, but it breaks memoization if a child starts depending on it.
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
marekdano
left a comment
marekdano
left a comment
There was a problem hiding this comment.
The changes look good now!
LGTM 🚀
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
| ids.forEach((id) => { | ||
| void api | ||
| .get<MCPServer>(`/v1/mcp-servers/${encodeURIComponent(id)}`, undefined, controller.signal) | ||
| .then((gateway) => { | ||
| if (!controller.signal.aborted) | ||
| setSavedOAuthGateways((previous) => ({ ...previous, [id]: gateway })); | ||
| }) | ||
| .catch(() => undefined); |
There was a problem hiding this comment.
useEffect(() => {
...
setSavedOAuthGateways((previous) => Object.fromEntries(Object.entries(previous).filter(...)));
ids.forEach((id) => { // ❌ one GET per registered OAuth gateway, every time ids changes
void api.get<MCPServer>(`/v1/mcp-servers/${encodeURIComponent(id)}`, undefined, controller.signal)...- On first load a catalog page with many registered OAuth servers issues one request per gateway.
- Whenever
oauthGatewayIdschanges (pagination, filtering...), the previous controller is aborted and all gateways are re-requested, even those already cached insavedOAuthGateways.
Suggestion: cache responses, fetch only ids missing from the cache and abort only on unmount.
| isAdding(server.id) || | ||
| isDisconnecting(server.id) | ||
| ) | ||
| return null; |
There was a problem hiding this comment.
NIT: Add curly brackets for readability.
Signed-off-by: Vishu Bhatnagar <vishu.bhatnagar@ibm.com>
Add stored-token connection validation to registered OAuth Catalog cards. Fetch the saved gateway before testing so the exact saved ID and URL are used, show inline results, and support cancellation, retesting, and reauthorization. Keep validation independent of component import and connection status.
Part 4 of the frontend split for IBM/mcp-context-forge#5722. Based on #221 in native GitHub stack #223. Add the catalog cases to the shared browser suite and audit the secret scanner's false positive on the permission expression.
Validation: production build, ESLint, full frontend coverage suite, and OAuth browser tests across both pages. The complete original implementation passed 3,852 frontend tests, 300 BFF tests, 266 browser tests, and 6 live backend smoke tests; coverage thresholds passed. Browser screenshots are not committed.
Native GitHub stack: #223, rooted at
main.main.#218 contains the independent E2E typing fix and can merge separately. These PRs replace #216.