Merge pull request #18680 from BerriAI/litellm_sso_modal_fix

[Fix] UI - SSO Edit Modal Clear Role Mapping Values on Provider Change

ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/BaseSSOSettingsForm.tsx

@@ -189,11 +189,16 @@ const BaseSSOSettingsForm: React.FC<BaseSSOSettingsFormProps> = ({ form, onFormS
 
         <Form.Item
           noStyle
-          shouldUpdate={(prevValues, currentValues) => prevValues.use_role_mappings !== currentValues.use_role_mappings}
+          shouldUpdate={(prevValues, currentValues) =>
+            prevValues.use_role_mappings !== currentValues.use_role_mappings ||
+            prevValues.sso_provider !== currentValues.sso_provider
+          }
         >
           {({ getFieldValue }) => {
             const useRoleMappings = getFieldValue("use_role_mappings");
-            return useRoleMappings ? (
+            const provider = getFieldValue("sso_provider");
+            const supportsRoleMappings = provider === "okta" || provider === "generic";
+            return useRoleMappings && supportsRoleMappings ? (
               <Form.Item
                 label="Group Claim"
                 name="group_claim"
@@ -207,11 +212,16 @@ const BaseSSOSettingsForm: React.FC<BaseSSOSettingsFormProps> = ({ form, onFormS
 
         <Form.Item
           noStyle
-          shouldUpdate={(prevValues, currentValues) => prevValues.use_role_mappings !== currentValues.use_role_mappings}
+          shouldUpdate={(prevValues, currentValues) =>
+            prevValues.use_role_mappings !== currentValues.use_role_mappings ||
+            prevValues.sso_provider !== currentValues.sso_provider
+          }
         >
           {({ getFieldValue }) => {
             const useRoleMappings = getFieldValue("use_role_mappings");
-            return useRoleMappings ? (
+            const provider = getFieldValue("sso_provider");
+            const supportsRoleMappings = provider === "okta" || provider === "generic";
+            return useRoleMappings && supportsRoleMappings ? (
               <>
                 <Form.Item label="Default Role" name="default_role" initialValue="Internal User">
                   <Select>

ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/DeleteSSOSettingsModal.test.tsx

@@ -1,20 +1,60 @@
+import { QueryClient, QueryClientProvider } from "@tanstack/react-query";
 import { render, screen } from "@testing-library/react";
 import { describe, expect, it, vi } from "vitest";
 import DeleteSSOSettingsModal from "./DeleteSSOSettingsModal";
 
+vi.mock("@/app/(dashboard)/hooks/sso/useSSOSettings", () => ({
+  useSSOSettings: vi.fn(() => ({
+    data: {
+      values: {
+        google_client_id: "test-client-id",
+      },
+    },
+  })),
+}));
+
+vi.mock("@/app/(dashboard)/hooks/sso/useEditSSOSettings", () => ({
+  useEditSSOSettings: vi.fn(() => ({
+    mutateAsync: vi.fn(),
+    isPending: false,
+  })),
+}));
+
+vi.mock("@/app/(dashboard)/hooks/useAuthorized", () => ({
+  default: vi.fn(() => ({
+    accessToken: "test-token",
+    userId: "test-user-id",
+    userRole: "proxy_admin",
+  })),
+}));
+
+const createQueryClient = () =>
+  new QueryClient({
+    defaultOptions: {
+      queries: {
+        retry: false,
+        gcTime: 0,
+      },
+    },
+  });
+
 describe("DeleteSSOSettingsModal", () => {
   it("should render", () => {
     const onCancel = vi.fn();
     const onSuccess = vi.fn();
+    const queryClient = createQueryClient();
 
     render(
-      <DeleteSSOSettingsModal isVisible={true} onCancel={onCancel} onSuccess={onSuccess} accessToken="test-token" />,
+      <QueryClientProvider client={queryClient}>
+        <DeleteSSOSettingsModal isVisible={true} onCancel={onCancel} onSuccess={onSuccess} />
+      </QueryClientProvider>,
     );
 
     expect(screen.getByText("Confirm Clear SSO Settings")).toBeInTheDocument();
     expect(
-      screen.getByText("Are you sure you want to clear all SSO settings? This action cannot be undone."),
+      screen.getByText(
+        "Are you sure you want to clear all SSO settings? Users will no longer be able to login using SSO after this change.",
+      ),
     ).toBeInTheDocument();
-    expect(screen.getByText("Users will no longer be able to login using SSO after this change.")).toBeInTheDocument();
   });
 });

ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/Modals/DeleteSSOSettingsModal.tsx

@@ -1,79 +1,66 @@
-import { Modal } from "antd";
+import { useEditSSOSettings } from "@/app/(dashboard)/hooks/sso/useEditSSOSettings";
+import { useSSOSettings } from "@/app/(dashboard)/hooks/sso/useSSOSettings";
 import React from "react";
+import DeleteResourceModal from "../../../../common_components/DeleteResourceModal";
 import NotificationsManager from "../../../../molecules/notifications_manager";
-import { updateSSOSettings } from "../../../../networking";
 import { parseErrorMessage } from "../../../../shared/errorUtils";
+import { detectSSOProvider } from "../utils";
 
 interface DeleteSSOSettingsModalProps {
   isVisible: boolean;
   onCancel: () => void;
   onSuccess: () => void;
-  accessToken: string | null;
 }
 
-const DeleteSSOSettingsModal: React.FC<DeleteSSOSettingsModalProps> = ({
-  isVisible,
-  onCancel,
-  onSuccess,
-  accessToken,
-}) => {
+const DeleteSSOSettingsModal: React.FC<DeleteSSOSettingsModalProps> = ({ isVisible, onCancel, onSuccess }) => {
+  const { data: ssoSettings } = useSSOSettings();
+  const { mutateAsync: editSSOSettings, isPending: isEditingSSOSettings } = useEditSSOSettings();
+
   // Handle clearing SSO settings
   const handleClearSSO = async () => {
-    if (!accessToken) {
-      NotificationsManager.fromBackend("No access token available");
-      return;
-    }
+    const clearSettings = {
+      google_client_id: null,
+      google_client_secret: null,
+      microsoft_client_id: null,
+      microsoft_client_secret: null,
+      microsoft_tenant: null,
+      generic_client_id: null,
+      generic_client_secret: null,
+      generic_authorization_endpoint: null,
+      generic_token_endpoint: null,
+      generic_userinfo_endpoint: null,
+      proxy_base_url: null,
+      user_email: null,
+      sso_provider: null,
+      role_mappings: null,
+    };
 
-    try {
-      // Clear all SSO settings
-      const clearSettings = {
-        google_client_id: null,
-        google_client_secret: null,
-        microsoft_client_id: null,
-        microsoft_client_secret: null,
-        microsoft_tenant: null,
-        generic_client_id: null,
-        generic_client_secret: null,
-        generic_authorization_endpoint: null,
-        generic_token_endpoint: null,
-        generic_userinfo_endpoint: null,
-        proxy_base_url: null,
-        user_email: null,
-        sso_provider: null,
-      };
-
-      await updateSSOSettings(accessToken, clearSettings);
-
-      NotificationsManager.success("SSO settings cleared successfully");
-
-      // Close modal and trigger success callback
-      onCancel();
-      onSuccess();
-    } catch (error) {
-      console.error("Failed to clear SSO settings:", error);
-      NotificationsManager.fromBackend("Failed to clear SSO settings: " + parseErrorMessage(error));
-    }
+    await editSSOSettings(clearSettings, {
+      onSuccess: () => {
+        NotificationsManager.success("SSO settings cleared successfully");
+        onCancel();
+        onSuccess();
+      },
+      onError: (error) => {
+        NotificationsManager.fromBackend("Failed to clear SSO settings: " + parseErrorMessage(error));
+      },
+    });
   };
 
   return (
-    <Modal
+    <DeleteResourceModal
+      isOpen={isVisible}
       title="Confirm Clear SSO Settings"
-      visible={isVisible}
-      onOk={handleClearSSO}
+      alertMessage="This action cannot be undone."
+      message="Are you sure you want to clear all SSO settings? Users will no longer be able to login using SSO after this change."
+      resourceInformationTitle="SSO Settings"
+      resourceInformation={[
+        { label: "Provider", value: (ssoSettings?.values && detectSSOProvider(ssoSettings?.values)) || "Generic" },
+      ]}
       onCancel={onCancel}
-      okText="Yes, Clear"
-      cancelText="Cancel"
-      okButtonProps={{
-        danger: true,
-        style: {
-          backgroundColor: "#dc2626",
-          borderColor: "#dc2626",
-        },
-      }}
-    >
-      <p>Are you sure you want to clear all SSO settings? This action cannot be undone.</p>
-      <p>Users will no longer be able to login using SSO after this change.</p>
-    </Modal>
+      onOk={handleClearSSO}
+      confirmLoading={isEditingSSOSettings}
+    />
   );
 };
 

ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/SSOSettings.tsx

@@ -1,7 +1,6 @@
 "use client";
 
 import { useSSOSettings, type SSOSettingsValues } from "@/app/(dashboard)/hooks/sso/useSSOSettings";
-import useAuthorized from "@/app/(dashboard)/hooks/useAuthorized";
 import { Button, Card, Descriptions, Space, Typography } from "antd";
 import { Edit, Shield, Trash2 } from "lucide-react";
 import { useState } from "react";
@@ -13,12 +12,12 @@ import RedactableField from "./RedactableField";
 import RoleMappings from "./RoleMappings";
 import SSOSettingsEmptyPlaceholder from "./SSOSettingsEmptyPlaceholder";
 import SSOSettingsLoadingSkeleton from "./SSOSettingsLoadingSkeleton";
+import { detectSSOProvider } from "./utils";
 
 const { Title, Text } = Typography;
 
 export default function SSOSettings() {
   const { data: ssoSettings, refetch, isLoading } = useSSOSettings();
-  const { accessToken } = useAuthorized();
   const [isDeleteModalVisible, setIsDeleteModalVisible] = useState(false);
   const [isAddModalVisible, setIsAddModalVisible] = useState(false);
   const [isEditModalVisible, setIsEditModalVisible] = useState(false);
@@ -27,23 +26,6 @@ export default function SSOSettings() {
     Boolean(ssoSettings?.values.microsoft_client_id) ||
     Boolean(ssoSettings?.values.generic_client_id);
 
-  // Determine the SSO provider based on the configuration
-  const detectSSOProvider = (values: SSOSettingsValues): string | null => {
-    if (values.google_client_id) return "google";
-    if (values.microsoft_client_id) return "microsoft";
-    if (values.generic_client_id) {
-      // Check if it looks like Okta/Auth0 based on endpoints
-      if (
-        values.generic_authorization_endpoint?.includes("okta") ||
-        values.generic_authorization_endpoint?.includes("auth0")
-      ) {
-        return "okta";
-      }
-      return "generic";
-    }
-    return null;
-  };
-
   const selectedProvider = ssoSettings?.values ? detectSSOProvider(ssoSettings.values) : null;
   const isRoleMappingsEnabled = Boolean(ssoSettings?.values.role_mappings);
 
@@ -233,7 +215,6 @@ export default function SSOSettings() {
         isVisible={isDeleteModalVisible}
         onCancel={() => setIsDeleteModalVisible(false)}
         onSuccess={() => refetch()}
-        accessToken={accessToken}
       />
 
       <AddSSOSettingsModal

ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.test.ts

@@ -55,6 +55,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "proxy_admin",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
         other_field: "value",
       };
 
@@ -83,6 +84,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "internal_user",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -100,6 +102,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "internal_user_viewer",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -121,6 +124,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "proxy_admin_viewer",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -142,6 +146,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "internal_user",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -160,6 +165,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "internal_user",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -174,6 +180,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "internal_user_viewer",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -186,6 +193,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "internal_user",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -198,6 +206,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "proxy_admin_viewer",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -210,6 +219,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "proxy_admin",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -222,6 +232,7 @@ describe("processSSOSettingsPayload", () => {
         default_role: "unknown_role",
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);
@@ -233,6 +244,7 @@ describe("processSSOSettingsPayload", () => {
       const formValues = {
         group_claim: "groups",
         use_role_mappings: true,
+        sso_provider: "generic",
       };
 
       const result = processSSOSettingsPayload(formValues);

ui/litellm-dashboard/src/components/Settings/AdminSettings/SSOSettings/utils.ts

@@ -1,3 +1,5 @@
+import { SSOSettingsValues } from "@/app/(dashboard)/hooks/sso/useSSOSettings";
+
 /**
  * Processes SSO settings form values and transforms them into the payload format expected by the API
  * Handles role mappings transformation and field extraction
@@ -18,7 +20,7 @@ export const processSSOSettingsPayload = (formValues: Record<string, any>): Reco
     ...rest,
   };
 
-  // Add role mappings if use_role_mappings is checked
+  // Add role mappings only if use_role_mappings is checked AND provider supports role mappings
   if (use_role_mappings) {
     // Helper function to split comma-separated string into array
     const splitTeams = (teams: string | undefined): string[] => {
@@ -52,3 +54,20 @@ export const processSSOSettingsPayload = (formValues: Record<string, any>): Reco
 
   return payload;
 };
+
+// Determine the SSO provider based on the configuration
+export const detectSSOProvider = (values: SSOSettingsValues): string | null => {
+  if (values.google_client_id) return "google";
+  if (values.microsoft_client_id) return "microsoft";
+  if (values.generic_client_id) {
+    // Check if it looks like Okta/Auth0 based on endpoints
+    if (
+      values.generic_authorization_endpoint?.includes("okta") ||
+      values.generic_authorization_endpoint?.includes("auth0")
+    ) {
+      return "okta";
+    }
+    return "generic";
+  }
+  return null;
+};