Fix saml conversion from ext_perm -> basic (#4343)

* fix saml conversion from ext_perm -> basic

* quick nit

* minor fix

* finalize

* update

* quick fix

backend/ee/onyx/server/saml.py

@@ -36,8 +36,12 @@ from onyx.utils.logger import setup_logger
 logger = setup_logger()
 router = APIRouter(prefix="/auth/saml")
 
+# Define non-authenticated user roles that should be re-created during SAML login
+NON_AUTHENTICATED_ROLES = {UserRole.SLACK_USER, UserRole.EXT_PERM_USER}
+
 
 async def upsert_saml_user(email: str) -> User:
+    logger.debug(f"Attempting to upsert SAML user with email: {email}")
     get_async_session_context = contextlib.asynccontextmanager(
         get_async_session
     )  # type:ignore
@@ -48,9 +52,13 @@ async def upsert_saml_user(email: str) -> User:
         async with get_user_db_context(session) as user_db:
             async with get_user_manager_context(user_db) as user_manager:
                 try:
-                    return await user_manager.get_by_email(email)
+                    user = await user_manager.get_by_email(email)
+                    # If user has a non-authenticated role, treat as non-existent
+                    if user.role in NON_AUTHENTICATED_ROLES:
+                        raise exceptions.UserNotExists()
+                    return user
                 except exceptions.UserNotExists:
-                    logger.notice("Creating user from SAML login")
+                    logger.info("Creating user from SAML login")
 
                 user_count = await get_user_count()
                 role = UserRole.ADMIN if user_count == 0 else UserRole.BASIC
@@ -59,11 +67,10 @@ async def upsert_saml_user(email: str) -> User:
                 password = fastapi_users_pw_helper.generate()
                 hashed_pass = fastapi_users_pw_helper.hash(password)
 
-                user: User = await user_manager.create(
+                user = await user_manager.create(
                     UserCreate(
                         email=email,
                         password=hashed_pass,
-                        is_verified=True,
                         role=role,
                     )
                 )

backend/onyx/db/users.py

@@ -24,7 +24,9 @@ from onyx.db.models import User__UserGroup
 from onyx.utils.variable_functionality import fetch_ee_implementation_or_noop
 
 
-def validate_user_role_update(requested_role: UserRole, current_role: UserRole) -> None:
+def validate_user_role_update(
+    requested_role: UserRole, current_role: UserRole, explicit_override: bool = False
+) -> None:
     """
     Validate that a user role update is valid.
     Assumed only admins can hit this endpoint.
@@ -57,6 +59,9 @@ def validate_user_role_update(requested_role: UserRole, current_role: UserRole)
             detail="To change a Limited User's role, they must first login to Onyx via the web app.",
         )
 
+    if explicit_override:
+        return
+
     if requested_role == UserRole.CURATOR:
         # This shouldn't happen, but just in case
         raise HTTPException(

backend/onyx/server/manage/models.py

@@ -132,6 +132,7 @@ class UserByEmail(BaseModel):
 class UserRoleUpdateRequest(BaseModel):
     user_email: str
     new_role: UserRole
+    explicit_override: bool = False
 
 
 class UserRoleResponse(BaseModel):

backend/onyx/server/manage/users.py

@@ -102,6 +102,7 @@ def set_user_role(
     validate_user_role_update(
         requested_role=requested_role,
         current_role=current_role,
+        explicit_override=user_role_update_request.explicit_override,
     )
 
     if user_to_update.id == current_user.id:
@@ -122,6 +123,22 @@ def set_user_role(
     db_session.commit()
 
 
+class TestUpsertRequest(BaseModel):
+    email: str
+
+
+@router.post("/manage/users/test-upsert-user")
+async def test_upsert_user(
+    request: TestUpsertRequest,
+    _: User = Depends(current_admin_user),
+) -> None | FullUserSnapshot:
+    """Test endpoint for upsert_saml_user. Only used for integration testing."""
+    user = await fetch_ee_implementation_or_noop(
+        "onyx.server.saml", "upsert_saml_user", None
+    )(email=request.email)
+    return FullUserSnapshot.from_user_model(user) if user else None
+
+
 @router.get("/manage/users/accepted")
 def list_accepted_users(
     q: str | None = Query(default=None),

backend/tests/integration/common_utils/managers/user.py

@@ -123,10 +123,15 @@ class UserManager:
         user_to_set: DATestUser,
         target_role: UserRole,
         user_performing_action: DATestUser,
+        explicit_override: bool = False,
     ) -> DATestUser:
         response = requests.patch(
             url=f"{API_SERVER_URL}/manage/set-user-role",
-            json={"user_email": user_to_set.email, "new_role": target_role.value},
+            json={
+                "user_email": user_to_set.email,
+                "new_role": target_role.value,
+                "explicit_override": explicit_override,
+            },
             headers=user_performing_action.headers,
         )
         response.raise_for_status()

backend/tests/integration/tests/auth/test_saml_user_conversion.py

@@ -0,0 +1,90 @@
+import requests
+
+from onyx.auth.schemas import UserRole
+from tests.integration.common_utils.constants import API_SERVER_URL
+from tests.integration.common_utils.managers.user import UserManager
+from tests.integration.common_utils.test_models import DATestUser
+
+
+def test_saml_user_conversion(reset: None) -> None:
+    """
+    Test that SAML login correctly converts users with non-authenticated roles
+    (SLACK_USER or EXT_PERM_USER) to authenticated roles (BASIC).
+
+    This test:
+    1. Creates an admin and a regular user
+    2. Changes the regular user's role to EXT_PERM_USER
+    3. Simulates a SAML login by calling the test endpoint
+    4. Verifies the user's role is converted to BASIC
+
+    This tests the fix that ensures users with non-authenticated roles (SLACK_USER or EXT_PERM_USER)
+    are properly converted to authenticated roles during SAML login.
+    """
+    # Create an admin user (first user created is automatically an admin)
+    admin_user: DATestUser = UserManager.create(email="admin@onyx-test.com")
+
+    # Create a regular user that we'll convert to EXT_PERM_USER
+    test_user_email = "ext_perm_user@example.com"
+    test_user = UserManager.create(email=test_user_email)
+
+    # Verify the user was created with BASIC role initially
+    assert UserManager.is_role(test_user, UserRole.BASIC)
+
+    # Change the user's role to EXT_PERM_USER using the UserManager
+    UserManager.set_role(
+        user_to_set=test_user,
+        target_role=UserRole.EXT_PERM_USER,
+        user_performing_action=admin_user,
+        explicit_override=True,
+    )
+
+    # Verify the user has EXT_PERM_USER role now
+    assert UserManager.is_role(test_user, UserRole.EXT_PERM_USER)
+
+    # Simulate SAML login by calling the test endpoint
+    response = requests.post(
+        f"{API_SERVER_URL}/manage/users/test-upsert-user",
+        json={"email": test_user_email},
+        headers=admin_user.headers,  # Use admin headers for authorization
+    )
+    response.raise_for_status()
+
+    # Verify the response indicates the role changed to BASIC
+    user_data = response.json()
+    assert user_data["role"] == UserRole.BASIC.value
+
+    # Verify user role was changed in the database
+    assert UserManager.is_role(test_user, UserRole.BASIC)
+
+    # Do the same test with SLACK_USER
+    slack_user_email = "slack_user@example.com"
+    slack_user = UserManager.create(email=slack_user_email)
+
+    # Verify the user was created with BASIC role initially
+    assert UserManager.is_role(slack_user, UserRole.BASIC)
+
+    # Change the user's role to SLACK_USER
+    UserManager.set_role(
+        user_to_set=slack_user,
+        target_role=UserRole.SLACK_USER,
+        user_performing_action=admin_user,
+        explicit_override=True,
+    )
+
+    # Verify the user has SLACK_USER role
+    assert UserManager.is_role(slack_user, UserRole.SLACK_USER)
+
+    # Simulate SAML login again
+    response = requests.post(
+        f"{API_SERVER_URL}/manage/users/test-upsert-user",
+        json={"email": slack_user_email},
+        headers=admin_user.headers,
+    )
+    response.raise_for_status()
+
+    # Verify the response indicates the role changed to BASIC
+    user_data = response.json()
+    assert user_data["role"] == UserRole.BASIC.value
+
+    # Verify the user's role was changed in the database
+    assert UserManager.is_role(slack_user, UserRole.BASIC)