resovled qoto comments

Signed-off-by: Yi Cai <yicai@redhat.com>

Author: ciiay

workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-validator.test.ts

@@ -0,0 +1,99 @@
+/*
+ * Copyright Red Hat, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *     http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+import { McpServerValidator } from './mcp-server-validator';
+
+describe('McpServerValidator auth header behavior', () => {
+  const url = 'https://mcp.example.com';
+  const childMock = jest.fn();
+  const logger: ConstructorParameters<typeof McpServerValidator>[0] = {
+    debug: jest.fn(),
+    info: jest.fn(),
+    warn: jest.fn(),
+    error: jest.fn(),
+    child: childMock,
+  };
+  childMock.mockImplementation(() => logger);
+
+  const originalFetch = global.fetch;
+
+  afterEach(() => {
+    global.fetch = originalFetch;
+    jest.clearAllMocks();
+  });
+
+  it('tries raw token first, then falls back to Bearer on 401/403', async () => {
+    const fetchMock = jest
+      .fn()
+      .mockResolvedValueOnce(new Response(null, { status: 401 }))
+      .mockResolvedValueOnce(
+        new Response(
+          JSON.stringify({
+            jsonrpc: '2.0',
+            result: { capabilities: { tools: {} } },
+            id: 1,
+          }),
+          {
+            status: 200,
+            headers: { 'content-type': 'application/json' },
+          },
+        ),
+      )
+      .mockResolvedValueOnce(new Response(null, { status: 204 }))
+      .mockResolvedValueOnce(
+        new Response(
+          JSON.stringify({
+            jsonrpc: '2.0',
+            result: { tools: [{ name: 'tool-1' }] },
+            id: 2,
+          }),
+          {
+            status: 200,
+            headers: { 'content-type': 'application/json' },
+          },
+        ),
+      );
+    global.fetch = fetchMock;
+
+    const validator = new McpServerValidator(logger);
+    const result = await validator.validate(url, 'raw-token');
+
+    expect(result.valid).toBe(true);
+    expect(fetchMock).toHaveBeenCalledTimes(4);
+    expect(fetchMock.mock.calls[0][1]?.headers).toMatchObject({
+      Authorization: 'raw-token',
+    });
+    expect(fetchMock.mock.calls[1][1]?.headers).toMatchObject({
+      Authorization: 'Bearer raw-token',
+    });
+  });
+
+  it('does not rewrite tokens that already include an auth scheme', async () => {
+    const fetchMock = jest
+      .fn()
+      .mockResolvedValue(new Response(null, { status: 401 }));
+    global.fetch = fetchMock;
+
+    const validator = new McpServerValidator(logger);
+    const result = await validator.validate(url, 'Basic abc123');
+
+    expect(result.valid).toBe(false);
+    expect(fetchMock).toHaveBeenCalledTimes(1);
+    expect(fetchMock.mock.calls[0][1]?.headers).toMatchObject({
+      Authorization: 'Basic abc123',
+    });
+  });
+});

workspaces/lightspeed/plugins/lightspeed-backend/src/service/mcp-server-validator.ts

@@ -19,6 +19,8 @@ import type { LoggerService } from '@backstage/backend-plugin-api';
 import { McpValidationResult } from './mcp-server-types';
 
 const REQUEST_TIMEOUT_MS = 10_000;
+const INVALID_CREDENTIALS_ERROR =
+  'Invalid credentials — server returned 401/403';
 
 const getEndpointLabel = (targetUrl: string): string => {
   try {
@@ -107,9 +109,47 @@ export class McpServerValidator {
 
   async validate(url: string, token: string): Promise<McpValidationResult> {
     const trimmedToken = token.trim();
-    const authorizationHeader = /^Bearer\s+/i.test(trimmedToken)
-      ? trimmedToken
-      : `Bearer ${trimmedToken}`;
+    const hasAuthScheme = /^[A-Za-z][A-Za-z0-9_-]*\s+/.test(trimmedToken);
+    const authorizationHeaders = hasAuthScheme
+      ? [trimmedToken]
+      : [trimmedToken, `Bearer ${trimmedToken}`];
+
+    let lastResult: McpValidationResult = {
+      valid: false,
+      toolCount: 0,
+      tools: [],
+      error: INVALID_CREDENTIALS_ERROR,
+    };
+
+    for (const [index, authorizationHeader] of authorizationHeaders.entries()) {
+      const result = await this.validateWithAuthorizationHeader(
+        url,
+        authorizationHeader,
+      );
+      lastResult = result;
+
+      const isLastAttempt = index === authorizationHeaders.length - 1;
+      const shouldRetryWithAlternativeAuth =
+        !isLastAttempt &&
+        !result.valid &&
+        result.error === INVALID_CREDENTIALS_ERROR;
+
+      if (!shouldRetryWithAlternativeAuth) {
+        return result;
+      }
+
+      this.logger.debug(
+        `MCP validation got 401/403 for ${url}; retrying with an alternate Authorization header format`,
+      );
+    }
+
+    return lastResult;
+  }
+
+  private async validateWithAuthorizationHeader(
+    url: string,
+    authorizationHeader: string,
+  ): Promise<McpValidationResult> {
     const headers: Record<string, string> = {
       'Content-Type': 'application/json',
       Authorization: authorizationHeader,
@@ -139,7 +179,7 @@ export class McpServerValidator {
           valid: false,
           toolCount: 0,
           tools: [],
-          error: 'Invalid credentials — server returned 401/403',
+          error: INVALID_CREDENTIALS_ERROR,
         };
       }
 

workspaces/lightspeed/plugins/lightspeed/src/components/McpServersSettings.tsx

@@ -17,6 +17,7 @@
 import { MouseEvent, useCallback, useEffect, useMemo, useState } from 'react';
 
 import { configApiRef, fetchApiRef, useApi } from '@backstage/core-plugin-api';
+import { usePermission } from '@backstage/plugin-permission-react';
 
 import { makeStyles } from '@material-ui/core';
 import ModeEditOutlineOutlinedIcon from '@mui/icons-material/ModeEditOutlineOutlined';
@@ -33,6 +34,8 @@ import {
 } from '@patternfly/react-icons';
 import { Table, Tbody, Td, Th, Thead, Tr } from '@patternfly/react-table';
 
+import { lightspeedMcpManagePermission } from '@red-hat-developer-hub/backstage-plugin-lightspeed-common';
+
 type ServerStatus = 'tokenRequired' | 'disabled' | 'ok' | 'failed' | 'unknown';
 
 type McpServer = {
@@ -209,8 +212,8 @@ const getStatusIcon = (status: ServerStatus, className: string) => {
 };
 
 const getDisplayStatus = (server: McpServer): ServerStatus => {
-  if (!server.enabled) return 'disabled';
   if (!server.hasToken) return 'tokenRequired';
+  if (!server.enabled) return 'disabled';
   if (server.status === 'error') return 'failed';
   if (server.status === 'connected') return 'ok';
   return 'unknown';
@@ -248,6 +251,10 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
   const classes = useStyles();
   const configApi = useApi(configApiRef);
   const fetchApi = useApi(fetchApiRef);
+  const mcpManagePermission = usePermission({
+    permission: lightspeedMcpManagePermission,
+  });
+  const canManageMcp = mcpManagePermission.allowed;
   const [servers, setServers] = useState<McpServer[]>([]);
   const [sortAsc, setSortAsc] = useState(true);
   const [isLoading, setIsLoading] = useState(true);
@@ -328,30 +335,32 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
       const uiServers = (data.servers ?? []).map(server => toUiServer(server));
       setServers(uiServers);
 
-      const serversToValidate = uiServers.filter(server => server.hasToken);
-      void Promise.allSettled(
-        serversToValidate.map(async server => {
-          try {
-            await validateServer(server.name);
-          } catch (validationError) {
-            setError(
-              prev =>
-                prev ??
-                (validationError instanceof Error
-                  ? validationError.message
-                  : `Failed to validate ${server.name}`),
-            );
-          }
-        }),
-      );
+      if (canManageMcp) {
+        const serversToValidate = uiServers.filter(server => server.hasToken);
+        void Promise.allSettled(
+          serversToValidate.map(async server => {
+            try {
+              await validateServer(server.name);
+            } catch (validationError) {
+              setError(
+                prev =>
+                  prev ??
+                  (validationError instanceof Error
+                    ? validationError.message
+                    : `Failed to validate ${server.name}`),
+              );
+            }
+          }),
+        );
+      }
     } catch (e) {
       setError(
         e instanceof Error ? e.message : 'Failed to load MCP server settings',
       );
     } finally {
       setIsLoading(false);
     }
-  }, [fetchJson, getBaseUrl, validateServer]);
+  }, [canManageMcp, fetchJson, getBaseUrl, validateServer]);
 
   useEffect(() => {
     loadServers();
@@ -362,6 +371,9 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
       serverName: string,
       body: { enabled?: boolean; token?: string | null },
     ) => {
+      if (!canManageMcp) {
+        return;
+      }
       setError(null);
       setIsSaving(prev => ({ ...prev, [serverName]: true }));
       try {
@@ -395,7 +407,7 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
         setIsSaving(prev => ({ ...prev, [serverName]: false }));
       }
     },
-    [fetchJson, getBaseUrl, loadServers],
+    [canManageMcp, fetchJson, getBaseUrl, loadServers],
   );
 
   const selectedCount = useMemo(
@@ -449,6 +461,14 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
           className={classes.alert}
         />
       )}
+      {!mcpManagePermission.loading && !canManageMcp && (
+        <Alert
+          variant="info"
+          isInline
+          title="You have read-only access to MCP servers."
+          className={classes.alert}
+        />
+      )}
 
       <Table
         variant="compact"
@@ -481,6 +501,11 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
               <Td colSpan={4}>Loading MCP servers...</Td>
             </Tr>
           )}
+          {!isLoading && sortedServers.length === 0 && (
+            <Tr>
+              <Td colSpan={4}>No MCP servers available.</Td>
+            </Tr>
+          )}
           {sortedServers.map(server => {
             const displayStatus = getDisplayStatus(server);
             const displayDetail = getDisplayDetail(server, displayStatus);
@@ -508,7 +533,9 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
                         id={`mcp-switch-${server.id}`}
                         aria-label={`Toggle ${server.name}`}
                         isChecked={isChecked}
-                        isDisabled={isUnavailable || isRowSaving}
+                        isDisabled={
+                          isUnavailable || isRowSaving || !canManageMcp
+                        }
                         onChange={(_event, checked) => {
                           patchServer(server.name, { enabled: checked });
                         }}
@@ -557,6 +584,7 @@ export const McpServersSettings = ({ onClose }: McpServersSettingsProps) => {
                     icon={<ModeEditOutlineOutlinedIcon fontSize="small" />}
                     variant="plain"
                     className={classes.actionButton}
+                    isDisabled={!canManageMcp}
                     onClick={onEditClick}
                   />
                 </Td>