fix(indexing, powerpoint files): Patch markitdown _convert_chart_to_markdown to no-op (#9970) to release v3.1 (#9979)

Co-authored-by: acaprau <48705707+acaprau@users.noreply.github.com>

backend/onyx/configs/constants.py

@@ -12,6 +12,11 @@ SLACK_USER_TOKEN_PREFIX = "xoxp-"
 SLACK_BOT_TOKEN_PREFIX = "xoxb-"
 ONYX_EMAILABLE_LOGO_MAX_DIM = 512
 
+# The mask_string() function in encryption.py uses "•" (U+2022 BULLET) to mask secrets.
+MASK_CREDENTIAL_CHAR = "\u2022"
+# Pattern produced by mask_string for strings >= 14 chars: "abcd...wxyz" (exactly 11 chars)
+MASK_CREDENTIAL_LONG_RE = re.compile(r"^.{4}\.{3}.{4}$")
+
 SOURCE_TYPE = "source_type"
 # stored in the `metadata` of a chunk. Used to signify that this chunk should
 # not be used for QA. For example, Google Drive file types which can't be parsed

backend/onyx/db/api_key.py

@@ -4,7 +4,6 @@ from fastapi_users.password import PasswordHelper
 from sqlalchemy import select
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.orm import joinedload
-from sqlalchemy.orm import selectinload
 from sqlalchemy.orm import Session
 
 from onyx.auth.api_key import ApiKeyDescriptor
@@ -55,7 +54,6 @@ async def fetch_user_for_api_key(
         select(User)
         .join(ApiKey, ApiKey.user_id == User.id)
         .where(ApiKey.hashed_api_key == hashed_api_key)
-        .options(selectinload(User.memories))
     )
 
 

backend/onyx/db/auth.py

@@ -13,7 +13,6 @@ from sqlalchemy import func
 from sqlalchemy import Select
 from sqlalchemy.ext.asyncio import AsyncSession
 from sqlalchemy.future import select
-from sqlalchemy.orm import selectinload
 from sqlalchemy.orm import Session
 
 from onyx.auth.schemas import UserRole
@@ -98,11 +97,6 @@ async def get_user_count(only_admin_users: bool = False) -> int:
 
 # Need to override this because FastAPI Users doesn't give flexibility for backend field creation logic in OAuth flow
 class SQLAlchemyUserAdminDB(SQLAlchemyUserDatabase[UP, ID]):
-    async def _get_user(self, statement: Select) -> UP | None:
-        statement = statement.options(selectinload(User.memories))
-        results = await self.session.execute(statement)
-        return results.unique().scalar_one_or_none()
-
     async def create(
         self,
         create_dict: Dict[str, Any],

backend/onyx/db/federated.py

@@ -8,6 +8,8 @@ from sqlalchemy.orm import selectinload
 from sqlalchemy.orm import Session
 
 from onyx.configs.constants import FederatedConnectorSource
+from onyx.configs.constants import MASK_CREDENTIAL_CHAR
+from onyx.configs.constants import MASK_CREDENTIAL_LONG_RE
 from onyx.db.engine.sql_engine import get_session_with_current_tenant
 from onyx.db.models import DocumentSet
 from onyx.db.models import FederatedConnector
@@ -45,6 +47,23 @@ def fetch_all_federated_connectors_parallel() -> list[FederatedConnector]:
         return fetch_all_federated_connectors(db_session)
 
 
+def _reject_masked_credentials(credentials: dict[str, Any]) -> None:
+    """Raise if any credential string value contains mask placeholder characters.
+
+    mask_string() has two output formats:
+    - Short strings (< 14 chars): "••••••••••••" (U+2022 BULLET)
+    - Long strings (>= 14 chars): "abcd...wxyz" (first4 + "..." + last4)
+    Both must be rejected.
+    """
+    for key, val in credentials.items():
+        if isinstance(val, str) and (
+            MASK_CREDENTIAL_CHAR in val or MASK_CREDENTIAL_LONG_RE.match(val)
+        ):
+            raise ValueError(
+                f"Credential field '{key}' contains masked placeholder characters. Please provide the actual credential value."
+            )
+
+
 def validate_federated_connector_credentials(
     source: FederatedConnectorSource,
     credentials: dict[str, Any],
@@ -66,6 +85,8 @@ def create_federated_connector(
     config: dict[str, Any] | None = None,
 ) -> FederatedConnector:
     """Create a new federated connector with credential and config validation."""
+    _reject_masked_credentials(credentials)
+
     # Validate credentials before creating
     if not validate_federated_connector_credentials(source, credentials):
         raise ValueError(
@@ -277,6 +298,8 @@ def update_federated_connector(
     )
 
     if credentials is not None:
+        _reject_masked_credentials(credentials)
+
         # Validate credentials before updating
         if not validate_federated_connector_credentials(
             federated_connector.source, credentials

backend/onyx/db/pat.py

@@ -8,7 +8,6 @@ from uuid import UUID
 from sqlalchemy import select
 from sqlalchemy import update
 from sqlalchemy.ext.asyncio import AsyncSession
-from sqlalchemy.orm import selectinload
 from sqlalchemy.orm import Session
 
 from onyx.auth.pat import build_displayable_pat
@@ -47,7 +46,6 @@ async def fetch_user_for_pat(
             (PersonalAccessToken.expires_at.is_(None))
             | (PersonalAccessToken.expires_at > now)
         )
-        .options(selectinload(User.memories))
     )
     if not user:
         return None

backend/onyx/db/user_preferences.py

@@ -229,7 +229,9 @@ def get_memories_for_user(
     user_id: UUID,
     db_session: Session,
 ) -> Sequence[Memory]:
-    return db_session.scalars(select(Memory).where(Memory.user_id == user_id)).all()
+    return db_session.scalars(
+        select(Memory).where(Memory.user_id == user_id).order_by(Memory.id.desc())
+    ).all()
 
 
 def update_user_pinned_assistants(

backend/onyx/file_processing/extract_file_text.py

@@ -49,9 +49,21 @@ KNOWN_OPENPYXL_BUGS = [
 
 def get_markitdown_converter() -> "MarkItDown":
     global _MARKITDOWN_CONVERTER
-    from markitdown import MarkItDown
 
     if _MARKITDOWN_CONVERTER is None:
+        from markitdown import MarkItDown
+
+        # Patch this function to effectively no-op because we were seeing this
+        # module take an inordinate amount of time to convert charts to markdown,
+        # making some powerpoint files with many or complicated charts nearly
+        # unindexable.
+        from markitdown.converters._pptx_converter import PptxConverter
+
+        setattr(
+            PptxConverter,
+            "_convert_chart_to_markdown",
+            lambda self, chart: "\n\n[chart omitted]\n\n",  # noqa: ARG005
+        )
         _MARKITDOWN_CONVERTER = MarkItDown(enable_plugins=False)
     return _MARKITDOWN_CONVERTER
 
@@ -202,18 +214,26 @@ def read_pdf_file(
     try:
         pdf_reader = PdfReader(file)
 
-        if pdf_reader.is_encrypted and pdf_pass is not None:
+        if pdf_reader.is_encrypted:
+            # Try the explicit password first, then fall back to an empty
+            # string.  Owner-password-only PDFs (permission restrictions but
+            # no open password) decrypt successfully with "".
+            # See https://github.com/onyx-dot-app/onyx/issues/9754
+            passwords = [p for p in [pdf_pass, ""] if p is not None]
             decrypt_success = False
-            try:
-                decrypt_success = pdf_reader.decrypt(pdf_pass) != 0
-            except Exception:
-                logger.error("Unable to decrypt pdf")
+            for pw in passwords:
+                try:
+                    if pdf_reader.decrypt(pw) != 0:
+                        decrypt_success = True
+                        break
+                except Exception:
+                    pass
 
             if not decrypt_success:
+                logger.error(
+                    "Encrypted PDF could not be decrypted, returning empty text."
+                )
                 return "", metadata, []
-        elif pdf_reader.is_encrypted:
-            logger.warning("No Password for an encrypted PDF, returning empty text.")
-            return "", metadata, []
 
         # Basic PDF metadata
         if pdf_reader.metadata is not None:

backend/onyx/file_processing/password_validation.py

@@ -33,8 +33,20 @@ def is_pdf_protected(file: IO[Any]) -> bool:
 
     with preserve_position(file):
         reader = PdfReader(file)
+        if not reader.is_encrypted:
+            return False
 
-    return bool(reader.is_encrypted)
+        # PDFs with only an owner password (permission restrictions like
+        # print/copy disabled) use an empty user password — any viewer can open
+        # them without prompting.  decrypt("") returns 0 only when a real user
+        # password is required.  See https://github.com/onyx-dot-app/onyx/issues/9754
+        try:
+            return reader.decrypt("") == 0
+        except Exception:
+            logger.exception(
+                "Failed to evaluate PDF encryption; treating as password protected"
+            )
+            return True
 
 
 def is_docx_protected(file: IO[Any]) -> bool:

backend/onyx/mcp_server_main.py

@@ -6,6 +6,7 @@ from onyx.configs.app_configs import MCP_SERVER_ENABLED
 from onyx.configs.app_configs import MCP_SERVER_HOST
 from onyx.configs.app_configs import MCP_SERVER_PORT
 from onyx.utils.logger import setup_logger
+from onyx.utils.variable_functionality import set_is_ee_based_on_env_variable
 
 logger = setup_logger()
 
@@ -16,6 +17,7 @@ def main() -> None:
         logger.info("MCP server is disabled (MCP_SERVER_ENABLED=false)")
         return
 
+    set_is_ee_based_on_env_variable()
     logger.info(f"Starting MCP server on {MCP_SERVER_HOST}:{MCP_SERVER_PORT}")
 
     from onyx.mcp_server.api import mcp_app

backend/onyx/server/manage/models.py

@@ -147,6 +147,7 @@ class UserInfo(BaseModel):
         is_anonymous_user: bool | None = None,
         tenant_info: TenantInfo | None = None,
         assistant_specific_configs: UserSpecificAssistantPreferences | None = None,
+        memories: list[MemoryItem] | None = None,
     ) -> "UserInfo":
         return cls(
             id=str(user.id),
@@ -191,10 +192,7 @@ class UserInfo(BaseModel):
                 role=user.personal_role or "",
                 use_memories=user.use_memories,
                 enable_memory_tool=user.enable_memory_tool,
-                memories=[
-                    MemoryItem(id=memory.id, content=memory.memory_text)
-                    for memory in (user.memories or [])
-                ],
+                memories=memories or [],
                 user_preferences=user.user_preferences or "",
             ),
         )

backend/onyx/server/manage/users.py

@@ -57,6 +57,7 @@ from onyx.db.user_preferences import activate_user
 from onyx.db.user_preferences import deactivate_user
 from onyx.db.user_preferences import get_all_user_assistant_specific_configs
 from onyx.db.user_preferences import get_latest_access_token_for_user
+from onyx.db.user_preferences import get_memories_for_user
 from onyx.db.user_preferences import update_assistant_preferences
 from onyx.db.user_preferences import update_user_assistant_visibility
 from onyx.db.user_preferences import update_user_auto_scroll
@@ -823,6 +824,11 @@ def verify_user_logged_in(
             [],
         ),
     )
+    memories = [
+        MemoryItem(id=memory.id, content=memory.memory_text)
+        for memory in get_memories_for_user(user.id, db_session)
+    ]
+
     user_info = UserInfo.from_model(
         user,
         current_token_created_at=token_created_at,
@@ -833,6 +839,7 @@ def verify_user_logged_in(
             new_tenant=new_tenant,
             invitation=tenant_invitation,
         ),
+        memories=memories,
     )
 
     return user_info
@@ -930,7 +937,8 @@ def update_user_personalization_api(
         else user.enable_memory_tool
     )
     existing_memories = [
-        MemoryItem(id=memory.id, content=memory.memory_text) for memory in user.memories
+        MemoryItem(id=memory.id, content=memory.memory_text)
+        for memory in get_memories_for_user(user.id, db_session)
     ]
     new_memories = (
         request.memories if request.memories is not None else existing_memories

backend/tests/unit/federated_connector/test_reject_masked_credentials.py

@@ -0,0 +1,58 @@
+import pytest
+
+from onyx.configs.constants import MASK_CREDENTIAL_CHAR
+from onyx.db.federated import _reject_masked_credentials
+
+
+class TestRejectMaskedCredentials:
+    """Verify that masked credential values are never accepted for DB writes.
+
+    mask_string() has two output formats:
+    - Short strings (< 14 chars): "••••••••••••" (U+2022 BULLET)
+    - Long strings (>= 14 chars): "abcd...wxyz" (first4 + "..." + last4)
+    _reject_masked_credentials must catch both.
+    """
+
+    def test_rejects_fully_masked_value(self) -> None:
+        masked = MASK_CREDENTIAL_CHAR * 12  # "••••••••••••"
+        with pytest.raises(ValueError, match="masked placeholder"):
+            _reject_masked_credentials({"client_id": masked})
+
+    def test_rejects_long_string_masked_value(self) -> None:
+        """mask_string returns 'first4...last4' for long strings — the real
+        format used for OAuth credentials like client_id and client_secret."""
+        with pytest.raises(ValueError, match="masked placeholder"):
+            _reject_masked_credentials({"client_id": "1234...7890"})
+
+    def test_rejects_when_any_field_is_masked(self) -> None:
+        """Even if client_id is real, a masked client_secret must be caught."""
+        with pytest.raises(ValueError, match="client_secret"):
+            _reject_masked_credentials(
+                {
+                    "client_id": "1234567890.1234567890",
+                    "client_secret": MASK_CREDENTIAL_CHAR * 12,
+                }
+            )
+
+    def test_accepts_real_credentials(self) -> None:
+        # Should not raise
+        _reject_masked_credentials(
+            {
+                "client_id": "1234567890.1234567890",
+                "client_secret": "test_client_secret_value",
+            }
+        )
+
+    def test_accepts_empty_dict(self) -> None:
+        # Should not raise — empty credentials are handled elsewhere
+        _reject_masked_credentials({})
+
+    def test_ignores_non_string_values(self) -> None:
+        # Non-string values (None, bool, int) should pass through
+        _reject_masked_credentials(
+            {
+                "client_id": "real_value",
+                "redirect_uri": None,
+                "some_flag": True,
+            }
+        )

backend/tests/unit/onyx/file_processing/fixtures/owner_protected.pdf

@@ -0,0 +1,76 @@
+%PDF-1.3
+%<25><><EFBFBD><EFBFBD>
+1 0 obj
+<<
+/Producer <1083d595b1>
+>>
+endobj
+2 0 obj
+<<
+/Type /Pages
+/Count 1
+/Kids [ 4 0 R ]
+>>
+endobj
+3 0 obj
+<<
+/Type /Catalog
+/Pages 2 0 R
+>>
+endobj
+4 0 obj
+<<
+/Type /Page
+/Resources <<
+/Font <<
+/F1 <<
+/Type /Font
+/Subtype /Type1
+/BaseFont /Helvetica
+>>
+>>
+>>
+/MediaBox [ 0.0 0.0 200 200 ]
+/Contents 5 0 R
+/Parent 2 0 R
+>>
+endobj
+5 0 obj
+<<
+/Length 42
+>>
+stream
+,N<><6~<7E>)<29><><EFBFBD><EFBFBD><EFBFBD>u<EFBFBD><0C><><EFBFBD>Zc'<27><>>8g<38><67><EFBFBD>n<EFBFBD><6E><EFBFBD><EFBFBD><EFBFBD>9"
+endstream
+endobj
+6 0 obj
+<<
+/V 2
+/R 3
+/Length 128
+/P 4294967292
+/Filter /Standard
+/O <6a340a292629053da84a6d8b19a5d505953b8b3fdac3d2d389fde0e354528d44>
+/U <d6f0dc91c7b9de264a8d708515468e6528bf4e5e4e758a4164004e56fffa0108>
+>>
+endobj
+xref
+0 7
+0000000000 65535 f 
+0000000015 00000 n 
+0000000059 00000 n 
+0000000118 00000 n 
+0000000167 00000 n 
+0000000348 00000 n 
+0000000440 00000 n 
+trailer
+<<
+/Size 7
+/Root 3 0 R
+/Info 1 0 R
+/ID [ <6364336635356135633239323638353039306635656133623165313637366430> <6364336635356135633239323638353039306635656133623165313637366430> ]
+/Encrypt 6 0 R
+>>
+startxref
+655
+%%EOF

backend/tests/unit/onyx/file_processing/test_pdf.py

@@ -54,6 +54,12 @@ class TestReadPdfFile:
         text, _, _ = read_pdf_file(_load("encrypted.pdf"), pdf_pass="wrong")
         assert text == ""
 
+    def test_owner_password_only_pdf_extracts_text(self) -> None:
+        """A PDF encrypted with only an owner password (no user password)
+        should still yield its text content. Regression for #9754."""
+        text, _, _ = read_pdf_file(_load("owner_protected.pdf"))
+        assert "Hello World" in text
+
     def test_empty_pdf(self) -> None:
         text, _, _ = read_pdf_file(_load("empty.pdf"))
         assert text.strip() == ""
@@ -117,6 +123,12 @@ class TestIsPdfProtected:
     def test_protected_pdf(self) -> None:
         assert is_pdf_protected(_load("encrypted.pdf")) is True
 
+    def test_owner_password_only_is_not_protected(self) -> None:
+        """A PDF with only an owner password (permission restrictions) but no
+        user password should NOT be considered protected — any viewer can open
+        it without prompting for a password."""
+        assert is_pdf_protected(_load("owner_protected.pdf")) is False
+
     def test_preserves_file_position(self) -> None:
         pdf = _load("simple.pdf")
         pdf.seek(42)

backend/tests/unit/onyx/file_processing/test_pptx_to_text.py

@@ -0,0 +1,79 @@
+import io
+
+from pptx import Presentation  # type: ignore[import-untyped]
+from pptx.chart.data import CategoryChartData  # type: ignore[import-untyped]
+from pptx.enum.chart import XL_CHART_TYPE  # type: ignore[import-untyped]
+from pptx.util import Inches  # type: ignore[import-untyped]
+
+from onyx.file_processing.extract_file_text import pptx_to_text
+
+
+def _make_pptx_with_chart() -> io.BytesIO:
+    """Create an in-memory pptx with one text slide and one chart slide."""
+    prs = Presentation()
+
+    # Slide 1: text only
+    slide1 = prs.slides.add_slide(prs.slide_layouts[1])
+    slide1.shapes.title.text = "Introduction"
+    slide1.placeholders[1].text = "This is the first slide."
+
+    # Slide 2: chart
+    slide2 = prs.slides.add_slide(prs.slide_layouts[5])  # Blank layout
+    chart_data = CategoryChartData()
+    chart_data.categories = ["Q1", "Q2", "Q3"]
+    chart_data.add_series("Revenue", (100, 200, 300))
+    slide2.shapes.add_chart(
+        XL_CHART_TYPE.COLUMN_CLUSTERED,
+        Inches(1),
+        Inches(1),
+        Inches(6),
+        Inches(4),
+        chart_data,
+    )
+
+    buf = io.BytesIO()
+    prs.save(buf)
+    buf.seek(0)
+    return buf
+
+
+def _make_pptx_without_chart() -> io.BytesIO:
+    """Create an in-memory pptx with a single text-only slide."""
+    prs = Presentation()
+    slide = prs.slides.add_slide(prs.slide_layouts[1])
+    slide.shapes.title.text = "Hello World"
+    slide.placeholders[1].text = "Some content here."
+
+    buf = io.BytesIO()
+    prs.save(buf)
+    buf.seek(0)
+    return buf
+
+
+class TestPptxToText:
+    def test_chart_is_omitted(self) -> None:
+        # Precondition
+        pptx_file = _make_pptx_with_chart()
+
+        # Under test
+        result = pptx_to_text(pptx_file)
+
+        # Postcondition
+        assert "Introduction" in result
+        assert "first slide" in result
+        assert "[chart omitted]" in result
+        # The actual chart data should NOT appear in the output.
+        assert "Revenue" not in result
+        assert "Q1" not in result
+
+    def test_text_only_pptx(self) -> None:
+        # Precondition
+        pptx_file = _make_pptx_without_chart()
+
+        # Under test
+        result = pptx_to_text(pptx_file)
+
+        # Postcondition
+        assert "Hello World" in result
+        assert "Some content" in result
+        assert "[chart omitted]" not in result

pyproject.toml

@@ -70,6 +70,10 @@ backend = [
     "lazy_imports==1.0.1",
     "lxml==5.3.0",
     "Mako==1.2.4",
+    # NOTE: Do not update without understanding the patching behavior in
+    # get_markitdown_converter in
+    # backend/onyx/file_processing/extract_file_text.py and what impacts
+    # updating might have on this behavior.
     "markitdown[pdf, docx, pptx, xlsx, xls]==0.1.2",
     "mcp[cli]==1.26.0",
     "msal==1.34.0",

web/src/components/admin/federated/FederatedConnectorForm.tsx

@@ -133,7 +133,7 @@ async function createFederatedConnector(
 
 async function updateFederatedConnector(
   id: number,
-  credentials: CredentialForm,
+  credentials: CredentialForm | null,
   config?: ConfigForm
 ): Promise<{ success: boolean; message: string }> {
   try {
@@ -143,7 +143,7 @@ async function updateFederatedConnector(
         "Content-Type": "application/json",
       },
       body: JSON.stringify({
-        credentials,
+        credentials: credentials ?? undefined,
         config: config || {},
       }),
     });
@@ -201,7 +201,9 @@ export function FederatedConnectorForm({
   const isEditMode = connectorId !== undefined;
 
   const [formState, setFormState] = useState<FormState>({
-    credentials: preloadedConnectorData?.credentials || {},
+    // In edit mode, don't populate credentials with masked values from the API.
+    // Masked values (e.g. "••••••••••••") would be saved back and corrupt the real credentials.
+    credentials: isEditMode ? {} : preloadedConnectorData?.credentials || {},
     config: preloadedConnectorData?.config || {},
     schema: preloadedCredentialSchema?.credentials || null,
     configurationSchema: null,
@@ -209,6 +211,7 @@ export function FederatedConnectorForm({
     configurationSchemaError: null,
     connectorError: null,
   });
+  const [credentialsModified, setCredentialsModified] = useState(false);
   const [isSubmitting, setIsSubmitting] = useState(false);
   const [submitMessage, setSubmitMessage] = useState<string | null>(null);
   const [submitSuccess, setSubmitSuccess] = useState<boolean | null>(null);
@@ -333,6 +336,7 @@ export function FederatedConnectorForm({
   }
 
   const handleCredentialChange = (key: string, value: string) => {
+    setCredentialsModified(true);
     setFormState((prev) => ({
       ...prev,
       credentials: {
@@ -354,6 +358,11 @@ export function FederatedConnectorForm({
 
   const handleValidateCredentials = async () => {
     if (!formState.schema) return;
+    if (isEditMode && !credentialsModified) {
+      setSubmitMessage("Enter new credential values before validating.");
+      setSubmitSuccess(false);
+      return;
+    }
 
     setIsValidating(true);
     setSubmitMessage(null);
@@ -411,8 +420,10 @@ export function FederatedConnectorForm({
     setSubmitSuccess(null);
 
     try {
-      // Validate required fields
-      if (formState.schema) {
+      const shouldValidateCredentials = !isEditMode || credentialsModified;
+
+      // Validate required fields (skip for credentials in edit mode when unchanged)
+      if (formState.schema && shouldValidateCredentials) {
         const missingRequired = Object.entries(formState.schema)
           .filter(
             ([key, field]) => field.required && !formState.credentials[key]
@@ -442,16 +453,20 @@ export function FederatedConnectorForm({
       }
       setConfigValidationErrors({});
 
-      // Validate credentials before creating/updating
-      const validation = await validateCredentials(
-        connector,
-        formState.credentials
-      );
-      if (!validation.success) {
-        setSubmitMessage(`Credential validation failed: ${validation.message}`);
-        setSubmitSuccess(false);
-        setIsSubmitting(false);
-        return;
+      // Validate credentials before creating/updating (skip in edit mode when unchanged)
+      if (shouldValidateCredentials) {
+        const validation = await validateCredentials(
+          connector,
+          formState.credentials
+        );
+        if (!validation.success) {
+          setSubmitMessage(
+            `Credential validation failed: ${validation.message}`
+          );
+          setSubmitSuccess(false);
+          setIsSubmitting(false);
+          return;
+        }
       }
 
       // Create or update the connector
@@ -459,7 +474,7 @@ export function FederatedConnectorForm({
         isEditMode && connectorId
           ? await updateFederatedConnector(
               connectorId,
-              formState.credentials,
+              credentialsModified ? formState.credentials : null,
               formState.config
             )
           : await createFederatedConnector(
@@ -538,14 +553,16 @@ export function FederatedConnectorForm({
               id={fieldKey}
               type={fieldSpec.secret ? "password" : "text"}
               placeholder={
-                fieldSpec.example
-                  ? String(fieldSpec.example)
-                  : fieldSpec.description
+                isEditMode && !credentialsModified
+                  ? "••••••••  (leave blank to keep current value)"
+                  : fieldSpec.example
+                    ? String(fieldSpec.example)
+                    : fieldSpec.description
               }
               value={formState.credentials[fieldKey] || ""}
               onChange={(e) => handleCredentialChange(fieldKey, e.target.value)}
               className="w-96"
-              required={fieldSpec.required}
+              required={!isEditMode && fieldSpec.required}
             />
           </div>
         ))}

web/src/refresh-pages/admin/GroupsPage/CreateGroupPage.tsx

@@ -1,8 +1,7 @@
 "use client";
 
-import { useMemo, useState } from "react";
+import { useState } from "react";
 import { useRouter } from "next/navigation";
-import useSWR from "swr";
 import { Table, Button } from "@opal/components";
 import { IllustrationContent } from "@opal/layouts";
 import { SvgUsers } from "@opal/icons";
@@ -14,16 +13,14 @@ import Text from "@/refresh-components/texts/Text";
 import SimpleLoader from "@/refresh-components/loaders/SimpleLoader";
 import Separator from "@/refresh-components/Separator";
 import { toast } from "@/hooks/useToast";
-import { errorHandlingFetcher } from "@/lib/fetcher";
-import useAdminUsers from "@/hooks/useAdminUsers";
-import type { ApiKeyDescriptor, MemberRow } from "./interfaces";
+import useGroupMemberCandidates from "./useGroupMemberCandidates";
 import {
   createGroup,
   updateAgentGroupSharing,
   updateDocSetGroupSharing,
   saveTokenLimits,
 } from "./svc";
-import { apiKeyToMemberRow, memberTableColumns, PAGE_SIZE } from "./shared";
+import { memberTableColumns, PAGE_SIZE } from "./shared";
 import SharedGroupResources from "@/refresh-pages/admin/GroupsPage/SharedGroupResources";
 import TokenLimitSection from "./TokenLimitSection";
 import type { TokenLimit } from "./TokenLimitSection";
@@ -41,22 +38,7 @@ function CreateGroupPage() {
     { tokenBudget: null, periodHours: null },
   ]);
 
-  const { users, isLoading: usersLoading, error: usersError } = useAdminUsers();
-
-  const {
-    data: apiKeys,
-    isLoading: apiKeysLoading,
-    error: apiKeysError,
-  } = useSWR<ApiKeyDescriptor[]>("/api/admin/api-key", errorHandlingFetcher);
-
-  const isLoading = usersLoading || apiKeysLoading;
-  const error = usersError ?? apiKeysError;
-
-  const allRows: MemberRow[] = useMemo(() => {
-    const activeUsers = users.filter((u) => u.is_active);
-    const serviceAccountRows = (apiKeys ?? []).map(apiKeyToMemberRow);
-    return [...activeUsers, ...serviceAccountRows];
-  }, [users, apiKeys]);
+  const { rows: allRows, isLoading, error } = useGroupMemberCandidates();
 
   async function handleCreate() {
     const trimmed = groupName.trim();
@@ -133,11 +115,11 @@ function CreateGroupPage() {
         {/* Members table */}
         {isLoading && <SimpleLoader />}
 
-        {error && (
+        {error ? (
           <Text as="p" secondaryBody text03>
             Failed to load users.
           </Text>
-        )}
+        ) : null}
 
         {!isLoading && !error && (
           <Section

web/src/refresh-pages/admin/GroupsPage/EditGroupPage.tsx

@@ -3,6 +3,7 @@
 import { useCallback, useEffect, useMemo, useRef, useState } from "react";
 import { useRouter } from "next/navigation";
 import useSWR, { useSWRConfig } from "swr";
+import useGroupMemberCandidates from "./useGroupMemberCandidates";
 import { Table, Button } from "@opal/components";
 import { IllustrationContent } from "@opal/layouts";
 import { SvgUsers, SvgTrash, SvgMinusCircle, SvgPlusCircle } from "@opal/icons";
@@ -19,20 +20,9 @@ import ConfirmationModalLayout from "@/refresh-components/layouts/ConfirmationMo
 import Separator from "@/refresh-components/Separator";
 import { toast } from "@/hooks/useToast";
 import { errorHandlingFetcher } from "@/lib/fetcher";
-import useAdminUsers from "@/hooks/useAdminUsers";
 import type { UserGroup } from "@/lib/types";
-import type {
-  ApiKeyDescriptor,
-  MemberRow,
-  TokenRateLimitDisplay,
-} from "./interfaces";
-import {
-  apiKeyToMemberRow,
-  baseColumns,
-  memberTableColumns,
-  tc,
-  PAGE_SIZE,
-} from "./shared";
+import type { MemberRow, TokenRateLimitDisplay } from "./interfaces";
+import { baseColumns, memberTableColumns, tc, PAGE_SIZE } from "./shared";
 import {
   USER_GROUP_URL,
   renameGroup,
@@ -104,18 +94,15 @@ function EditGroupPage({ groupId }: EditGroupPageProps) {
   const initialAgentIdsRef = useRef<number[]>([]);
   const initialDocSetIdsRef = useRef<number[]>([]);
 
-  // Users and API keys
-  const { users, isLoading: usersLoading, error: usersError } = useAdminUsers();
-
+  // Users + service accounts (curator-accessible — see hook docs).
   const {
-    data: apiKeys,
-    isLoading: apiKeysLoading,
-    error: apiKeysError,
-  } = useSWR<ApiKeyDescriptor[]>("/api/admin/api-key", errorHandlingFetcher);
+    rows: allRows,
+    isLoading: candidatesLoading,
+    error: candidatesError,
+  } = useGroupMemberCandidates();
 
-  const isLoading =
-    groupLoading || usersLoading || apiKeysLoading || tokenLimitsLoading;
-  const error = groupError ?? usersError ?? apiKeysError;
+  const isLoading = groupLoading || candidatesLoading || tokenLimitsLoading;
+  const error = groupError ?? candidatesError;
 
   // Pre-populate form when group data loads
   useEffect(() => {
@@ -145,12 +132,6 @@ function EditGroupPage({ groupId }: EditGroupPageProps) {
     }
   }, [tokenRateLimits]);
 
-  const allRows = useMemo(() => {
-    const activeUsers = users.filter((u) => u.is_active);
-    const serviceAccountRows = (apiKeys ?? []).map(apiKeyToMemberRow);
-    return [...activeUsers, ...serviceAccountRows];
-  }, [users, apiKeys]);
-
   const memberRows = useMemo(() => {
     const selected = new Set(selectedUserIds);
     return allRows.filter((r) => selected.has(r.id ?? r.email));

web/src/refresh-pages/admin/GroupsPage/useGroupMemberCandidates.ts

@@ -0,0 +1,161 @@
+"use client";
+
+import { useMemo } from "react";
+import useSWR from "swr";
+import { errorHandlingFetcher } from "@/lib/fetcher";
+import { useUser } from "@/providers/UserProvider";
+
+// Curator-accessible listing of all users (and service-account entries via
+// `?include_api_keys=true`). The admin-only `/manage/users/accepted/all` and
+// `/manage/users/invited` endpoints 403 for global curators, which used to
+// break the Edit Group page entirely — see useGroupMemberCandidates docs.
+const GROUP_MEMBER_CANDIDATES_URL = "/api/manage/users?include_api_keys=true";
+const ADMIN_API_KEYS_URL = "/api/admin/api-key";
+import { UserStatus, type UserRole } from "@/lib/types";
+
+// Mirrors `DANSWER_API_KEY_DUMMY_EMAIL_DOMAIN` on the backend; service-account
+// users are identified by this email suffix because release/v3.1 does not yet
+// expose `account_type` on the FullUserSnapshot returned from `/manage/users`.
+const API_KEY_EMAIL_SUFFIX = "@onyxapikey.ai";
+
+function isApiKeyEmail(email: string): boolean {
+  return email.endsWith(API_KEY_EMAIL_SUFFIX);
+}
+import type {
+  UserGroupInfo,
+  UserRow,
+} from "@/refresh-pages/admin/UsersPage/interfaces";
+import type { ApiKeyDescriptor, MemberRow } from "./interfaces";
+
+// Backend response shape for `/api/manage/users?include_api_keys=true`. The
+// existing `AllUsersResponse` in `lib/types.ts` types `accepted` as `User[]`,
+// which is missing fields the table needs (`personal_name`, `account_type`,
+// `groups`, etc.), so we declare an accurate local type here.
+interface FullUserSnapshot {
+  id: string;
+  email: string;
+  role: UserRole;
+  is_active: boolean;
+  password_configured: boolean;
+  personal_name: string | null;
+  created_at: string;
+  updated_at: string;
+  groups: UserGroupInfo[];
+  is_scim_synced: boolean;
+}
+
+interface ManageUsersResponse {
+  accepted: FullUserSnapshot[];
+  invited: { email: string }[];
+  slack_users: FullUserSnapshot[];
+  accepted_pages: number;
+  invited_pages: number;
+  slack_users_pages: number;
+}
+
+function snapshotToMemberRow(snapshot: FullUserSnapshot): MemberRow {
+  return {
+    id: snapshot.id,
+    email: snapshot.email,
+    role: snapshot.role,
+    status: snapshot.is_active ? UserStatus.ACTIVE : UserStatus.INACTIVE,
+    is_active: snapshot.is_active,
+    is_scim_synced: snapshot.is_scim_synced,
+    personal_name: snapshot.personal_name,
+    created_at: snapshot.created_at,
+    updated_at: snapshot.updated_at,
+    groups: snapshot.groups,
+  };
+}
+
+function serviceAccountToMemberRow(
+  snapshot: FullUserSnapshot,
+  apiKey: ApiKeyDescriptor | undefined
+): MemberRow {
+  return {
+    id: snapshot.id,
+    email: "Service Account",
+    role: apiKey?.api_key_role ?? snapshot.role,
+    status: UserStatus.ACTIVE,
+    is_active: true,
+    is_scim_synced: false,
+    personal_name:
+      apiKey?.api_key_name ?? snapshot.personal_name ?? "Unnamed Key",
+    created_at: null,
+    updated_at: null,
+    groups: [],
+    api_key_display: apiKey?.api_key_display,
+  };
+}
+
+interface UseGroupMemberCandidatesResult {
+  /** Active users + service-account rows, in the order the table expects. */
+  rows: MemberRow[];
+  /** Subset of `rows` representing real (non-service-account) users. */
+  userRows: MemberRow[];
+  isLoading: boolean;
+  error: unknown;
+}
+
+/**
+ * Returns the candidate list for the group create/edit member pickers.
+ *
+ * Hits `/api/manage/users?include_api_keys=true`, which is gated by
+ * `current_curator_or_admin_user` on the backend, so this works for both
+ * admins and global curators (the admin-only `/accepted/all` and `/invited`
+ * endpoints used to be called here, which 403'd for global curators and broke
+ * the Edit Group page entirely).
+ *
+ * For admins, we additionally fetch `/admin/api-key` to enrich service-account
+ * rows with the masked api-key display string. That call is admin-only and is
+ * skipped for curators; its failure is non-fatal.
+ */
+export default function useGroupMemberCandidates(): UseGroupMemberCandidatesResult {
+  const { isAdmin } = useUser();
+
+  const {
+    data: usersData,
+    isLoading: usersLoading,
+    error: usersError,
+  } = useSWR<ManageUsersResponse>(
+    GROUP_MEMBER_CANDIDATES_URL,
+    errorHandlingFetcher
+  );
+
+  const { data: apiKeys, isLoading: apiKeysLoading } = useSWR<
+    ApiKeyDescriptor[]
+  >(isAdmin ? ADMIN_API_KEYS_URL : null, errorHandlingFetcher);
+
+  const apiKeysByUserId = useMemo(() => {
+    const map = new Map<string, ApiKeyDescriptor>();
+    for (const key of apiKeys ?? []) map.set(key.user_id, key);
+    return map;
+  }, [apiKeys]);
+
+  const { rows, userRows } = useMemo(() => {
+    const accepted = usersData?.accepted ?? [];
+    const userRowsLocal: MemberRow[] = [];
+    const serviceAccountRows: MemberRow[] = [];
+    for (const snapshot of accepted) {
+      if (!snapshot.is_active) continue;
+      if (isApiKeyEmail(snapshot.email)) {
+        serviceAccountRows.push(
+          serviceAccountToMemberRow(snapshot, apiKeysByUserId.get(snapshot.id))
+        );
+      } else {
+        userRowsLocal.push(snapshotToMemberRow(snapshot));
+      }
+    }
+    return {
+      rows: [...userRowsLocal, ...serviceAccountRows],
+      userRows: userRowsLocal,
+    };
+  }, [usersData, apiKeysByUserId]);
+
+  return {
+    rows,
+    userRows,
+    isLoading: usersLoading || (isAdmin && apiKeysLoading),
+    error: usersError,
+  };
+}