mirror of
https://github.com/OpenHands/OpenHands.git
synced 2026-03-22 05:37:20 +08:00
fix(backend): resolve missing email and display name for user identity tracking (#12719)
This commit is contained in:
@@ -179,6 +179,9 @@ async def keycloak_callback(
|
||||
user = await UserStore.get_user_by_id_async(user_id)
|
||||
if not user:
|
||||
user = await UserStore.create_user(user_id, user_info)
|
||||
else:
|
||||
# Existing user — gradually backfill contact_name if it still has a username-style value
|
||||
await UserStore.backfill_contact_name(user_id, user_info)
|
||||
|
||||
if not user:
|
||||
logger.error(f'Failed to authenticate user {user_info["preferred_username"]}')
|
||||
|
||||
@@ -4,6 +4,7 @@ from fastapi import APIRouter, Depends, Query, status
|
||||
from fastapi.responses import JSONResponse
|
||||
from pydantic import SecretStr
|
||||
from server.auth.token_manager import TokenManager
|
||||
from utils.identity import resolve_display_name
|
||||
|
||||
from openhands.integrations.provider import (
|
||||
PROVIDER_TOKEN_TYPE,
|
||||
@@ -121,6 +122,8 @@ async def saas_get_user(
|
||||
login=(user_info.get('preferred_username') if user_info else '') or '',
|
||||
avatar_url='',
|
||||
email=user_info.get('email') if user_info else None,
|
||||
name=resolve_display_name(user_info) if user_info else None,
|
||||
company=user_info.get('company') if user_info else None,
|
||||
),
|
||||
user_info=user_info,
|
||||
)
|
||||
|
||||
@@ -27,6 +27,7 @@ from storage.org_member import OrgMember
|
||||
from storage.role_store import RoleStore
|
||||
from storage.user import User
|
||||
from storage.user_settings import UserSettings
|
||||
from utils.identity import resolve_display_name
|
||||
|
||||
from openhands.utils.async_utils import GENERAL_TIMEOUT, call_async_from_sync
|
||||
|
||||
@@ -53,7 +54,8 @@ class UserStore:
|
||||
org = Org(
|
||||
id=uuid.UUID(user_id),
|
||||
name=f'user_{user_id}_org',
|
||||
contact_name=user_info['preferred_username'],
|
||||
contact_name=resolve_display_name(user_info)
|
||||
or user_info.get('preferred_username', ''),
|
||||
contact_email=user_info['email'],
|
||||
v1_enabled=True,
|
||||
)
|
||||
@@ -175,7 +177,8 @@ class UserStore:
|
||||
id=uuid.UUID(user_id),
|
||||
name=f'user_{user_id}_org',
|
||||
org_version=user_settings.user_version,
|
||||
contact_name=user_info['username'],
|
||||
contact_name=resolve_display_name(user_info)
|
||||
or user_info.get('username', ''),
|
||||
contact_email=user_info['email'],
|
||||
)
|
||||
session.add(org)
|
||||
@@ -756,6 +759,49 @@ class UserStore:
|
||||
with session_maker() as session:
|
||||
return session.query(User).all()
|
||||
|
||||
@staticmethod
|
||||
async def backfill_contact_name(user_id: str, user_info: dict) -> None:
|
||||
"""Update contact_name on the personal org if it still has a username-style value.
|
||||
|
||||
Called during login to gradually fix existing users whose contact_name
|
||||
was stored as their username (before the resolve_display_name fix).
|
||||
Preserves custom values that were set via the PATCH endpoint.
|
||||
"""
|
||||
real_name = resolve_display_name(user_info)
|
||||
if not real_name:
|
||||
logger.debug(
|
||||
'backfill_contact_name:no_real_name',
|
||||
extra={'user_id': user_id},
|
||||
)
|
||||
return
|
||||
|
||||
preferred_username = user_info.get('preferred_username', '')
|
||||
username = user_info.get('username', '')
|
||||
|
||||
async with a_session_maker() as session:
|
||||
result = await session.execute(
|
||||
select(Org).filter(Org.id == uuid.UUID(user_id))
|
||||
)
|
||||
org = result.scalars().first()
|
||||
if not org:
|
||||
logger.debug(
|
||||
'backfill_contact_name:org_not_found',
|
||||
extra={'user_id': user_id},
|
||||
)
|
||||
return
|
||||
|
||||
if org.contact_name in (preferred_username, username):
|
||||
logger.info(
|
||||
'backfill_contact_name:updated',
|
||||
extra={
|
||||
'user_id': user_id,
|
||||
'old': org.contact_name,
|
||||
'new': real_name,
|
||||
},
|
||||
)
|
||||
org.contact_name = real_name
|
||||
await session.commit()
|
||||
|
||||
# Prevent circular imports
|
||||
from typing import TYPE_CHECKING
|
||||
|
||||
|
||||
@@ -153,6 +153,7 @@ async def test_keycloak_callback_user_not_allowed(mock_request):
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.migrate_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = False
|
||||
@@ -188,6 +189,7 @@ async def test_keycloak_callback_success_with_valid_offline_token(mock_request):
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.migrate_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_token_manager.get_keycloak_tokens = AsyncMock(
|
||||
return_value=('test_access_token', 'test_refresh_token')
|
||||
@@ -259,6 +261,7 @@ async def test_keycloak_callback_email_not_verified(mock_request):
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
# Act
|
||||
result = await keycloak_callback(
|
||||
@@ -306,6 +309,7 @@ async def test_keycloak_callback_email_not_verified_missing_field(mock_request):
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
# Act
|
||||
result = await keycloak_callback(
|
||||
@@ -347,6 +351,7 @@ async def test_keycloak_callback_success_without_offline_token(mock_request):
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.migrate_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_token_manager.get_keycloak_tokens = AsyncMock(
|
||||
return_value=('test_access_token', 'test_refresh_token')
|
||||
@@ -581,6 +586,7 @@ async def test_keycloak_callback_blocked_email_domain(mock_request):
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_domain_blocker.is_active.return_value = True
|
||||
mock_domain_blocker.is_domain_blocked.return_value = True
|
||||
@@ -644,6 +650,7 @@ async def test_keycloak_callback_allowed_email_domain(mock_request):
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_domain_blocker.is_active.return_value = True
|
||||
mock_domain_blocker.is_domain_blocked.return_value = False
|
||||
@@ -707,6 +714,7 @@ async def test_keycloak_callback_domain_blocking_inactive(mock_request):
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_domain_blocker.is_active.return_value = False
|
||||
mock_domain_blocker.is_domain_blocked.return_value = False
|
||||
@@ -768,6 +776,7 @@ async def test_keycloak_callback_missing_email(mock_request):
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_domain_blocker.is_active.return_value = True
|
||||
|
||||
@@ -813,6 +822,7 @@ async def test_keycloak_callback_duplicate_email_detected(mock_request):
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
# Act
|
||||
result = await keycloak_callback(
|
||||
@@ -857,6 +867,7 @@ async def test_keycloak_callback_duplicate_email_deletion_fails(mock_request):
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
# Act
|
||||
result = await keycloak_callback(
|
||||
@@ -914,6 +925,7 @@ async def test_keycloak_callback_duplicate_check_exception(mock_request):
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -971,6 +983,7 @@ async def test_keycloak_callback_no_duplicate_email(mock_request):
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1031,6 +1044,7 @@ async def test_keycloak_callback_no_email_in_user_info(mock_request):
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1187,6 +1201,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1251,6 +1266,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_domain_blocker.is_domain_blocked.return_value = False
|
||||
|
||||
@@ -1333,6 +1349,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1420,6 +1437,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1504,6 +1522,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1587,6 +1606,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1667,6 +1687,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1733,6 +1754,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1805,6 +1827,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.accepted_tos = '2025-01-01'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_verifier.is_active.return_value = True
|
||||
mock_verifier.is_user_allowed.return_value = True
|
||||
@@ -1875,6 +1898,7 @@ class TestKeycloakCallbackRecaptcha:
|
||||
mock_user.current_org_id = 'test_org_id'
|
||||
mock_user_store.get_user_by_id_async = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.create_user = AsyncMock(return_value=mock_user)
|
||||
mock_user_store.backfill_contact_name = AsyncMock()
|
||||
|
||||
mock_domain_blocker.is_domain_blocked.return_value = False
|
||||
|
||||
|
||||
116
enterprise/tests/unit/test_identity_utils.py
Normal file
116
enterprise/tests/unit/test_identity_utils.py
Normal file
@@ -0,0 +1,116 @@
|
||||
"""Tests for the resolve_display_name helper.
|
||||
|
||||
The resolve_display_name helper extracts the best available display name from
|
||||
Keycloak user_info claims. It is used by both the /api/user/info fallback path
|
||||
and the user_store create/migrate paths to avoid duplicating name-resolution logic.
|
||||
|
||||
The fallback chain is: name → given_name + family_name → None.
|
||||
It intentionally does NOT fall back to preferred_username/username — callers
|
||||
that need a guaranteed non-None value handle that separately, because the
|
||||
/api/user/info route should return name=None when no real name is available.
|
||||
"""
|
||||
|
||||
from utils.identity import resolve_display_name
|
||||
|
||||
|
||||
class TestResolveDisplayName:
|
||||
"""Test resolve_display_name with various Keycloak claim combinations."""
|
||||
|
||||
def test_returns_name_when_present(self):
|
||||
"""When user_info has a 'name' claim, use it directly."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'name': 'Jane Doe',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) == 'Jane Doe'
|
||||
|
||||
def test_combines_given_and_family_name_when_name_absent(self):
|
||||
"""When 'name' is missing, combine given_name + family_name."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) == 'Jane Doe'
|
||||
|
||||
def test_uses_given_name_only_when_family_name_absent(self):
|
||||
"""When only given_name is available, use it alone."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'given_name': 'Jane',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) == 'Jane'
|
||||
|
||||
def test_uses_family_name_only_when_given_name_absent(self):
|
||||
"""When only family_name is available, use it alone."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'family_name': 'Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) == 'Doe'
|
||||
|
||||
def test_returns_none_when_no_name_claims(self):
|
||||
"""When no name claims exist at all, return None."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'preferred_username': 'j.doe',
|
||||
'email': 'jane@example.com',
|
||||
}
|
||||
assert resolve_display_name(user_info) is None
|
||||
|
||||
def test_returns_none_for_empty_name(self):
|
||||
"""When 'name' is an empty string, treat it as absent."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'name': '',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) is None
|
||||
|
||||
def test_returns_none_for_whitespace_only_name(self):
|
||||
"""When 'name' is whitespace only, treat it as absent."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'name': ' ',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) is None
|
||||
|
||||
def test_returns_none_for_empty_given_and_family_names(self):
|
||||
"""When given_name and family_name are both empty strings, return None."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'given_name': '',
|
||||
'family_name': '',
|
||||
'preferred_username': 'j.doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) is None
|
||||
|
||||
def test_returns_none_for_empty_dict(self):
|
||||
"""An empty user_info dict returns None."""
|
||||
assert resolve_display_name({}) is None
|
||||
|
||||
def test_strips_whitespace_from_combined_name(self):
|
||||
"""Whitespace around given_name/family_name is stripped."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'given_name': ' Jane ',
|
||||
'family_name': ' Doe ',
|
||||
}
|
||||
assert resolve_display_name(user_info) == 'Jane Doe'
|
||||
|
||||
def test_name_claim_takes_priority_over_given_family(self):
|
||||
"""When both 'name' and given/family are present, 'name' wins."""
|
||||
user_info = {
|
||||
'sub': '123',
|
||||
'name': 'Dr. Jane Doe',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Doe',
|
||||
}
|
||||
assert resolve_display_name(user_info) == 'Dr. Jane Doe'
|
||||
163
enterprise/tests/unit/test_user_route_fallback.py
Normal file
163
enterprise/tests/unit/test_user_route_fallback.py
Normal file
@@ -0,0 +1,163 @@
|
||||
"""Tests for the fallback User path in the /api/user/info endpoint.
|
||||
|
||||
When a user authenticates via Keycloak without provider tokens (e.g., SAML, enterprise SSO),
|
||||
the endpoint constructs a User from OIDC claims. These tests verify that name and company
|
||||
fields are correctly populated from Keycloak claims in this fallback path.
|
||||
"""
|
||||
|
||||
from unittest.mock import AsyncMock, patch
|
||||
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
|
||||
from openhands.integrations.service_types import User
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_token_manager():
|
||||
"""Mock the token_manager used by user.py routes."""
|
||||
with patch('server.routes.user.token_manager') as mock_tm:
|
||||
yield mock_tm
|
||||
|
||||
|
||||
@pytest.fixture
|
||||
def mock_check_idp():
|
||||
"""Mock _check_idp to pass through the default_value (the fallback User).
|
||||
|
||||
This isolates the test to just the User construction logic in saas_get_user,
|
||||
without needing to set up Keycloak IDP token checks.
|
||||
"""
|
||||
with patch('server.routes.user._check_idp') as mock_fn:
|
||||
# Return the default_value argument that was passed to _check_idp
|
||||
mock_fn.side_effect = lambda **kwargs: kwargs.get('default_value')
|
||||
yield mock_fn
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fallback_user_includes_name_from_name_claim(
|
||||
mock_token_manager, mock_check_idp
|
||||
):
|
||||
"""When Keycloak provides a 'name' claim, the fallback User should include it."""
|
||||
from server.routes.user import saas_get_user
|
||||
|
||||
mock_token_manager.get_user_info = AsyncMock(
|
||||
return_value={
|
||||
'sub': '248289761001',
|
||||
'name': 'Jane Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
'email': 'jane@example.com',
|
||||
}
|
||||
)
|
||||
|
||||
result = await saas_get_user(
|
||||
provider_tokens=None,
|
||||
access_token=SecretStr('test-token'),
|
||||
user_id='248289761001',
|
||||
)
|
||||
|
||||
assert isinstance(result, User)
|
||||
assert result.name == 'Jane Doe'
|
||||
assert result.email == 'jane@example.com'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fallback_user_combines_given_and_family_name(
|
||||
mock_token_manager, mock_check_idp
|
||||
):
|
||||
"""When 'name' is absent, combine given_name + family_name."""
|
||||
from server.routes.user import saas_get_user
|
||||
|
||||
mock_token_manager.get_user_info = AsyncMock(
|
||||
return_value={
|
||||
'sub': '248289761001',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
'email': 'jane@example.com',
|
||||
}
|
||||
)
|
||||
|
||||
result = await saas_get_user(
|
||||
provider_tokens=None,
|
||||
access_token=SecretStr('test-token'),
|
||||
user_id='248289761001',
|
||||
)
|
||||
|
||||
assert isinstance(result, User)
|
||||
assert result.name == 'Jane Doe'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fallback_user_name_is_none_when_no_name_claims(
|
||||
mock_token_manager, mock_check_idp
|
||||
):
|
||||
"""When no name claims exist, name should be None."""
|
||||
from server.routes.user import saas_get_user
|
||||
|
||||
mock_token_manager.get_user_info = AsyncMock(
|
||||
return_value={
|
||||
'sub': '248289761001',
|
||||
'preferred_username': 'j.doe',
|
||||
'email': 'jane@example.com',
|
||||
}
|
||||
)
|
||||
|
||||
result = await saas_get_user(
|
||||
provider_tokens=None,
|
||||
access_token=SecretStr('test-token'),
|
||||
user_id='248289761001',
|
||||
)
|
||||
|
||||
assert isinstance(result, User)
|
||||
assert result.name is None
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fallback_user_includes_company_claim(mock_token_manager, mock_check_idp):
|
||||
"""When Keycloak provides a 'company' claim, include it in the User."""
|
||||
from server.routes.user import saas_get_user
|
||||
|
||||
mock_token_manager.get_user_info = AsyncMock(
|
||||
return_value={
|
||||
'sub': '248289761001',
|
||||
'name': 'Jane Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
'email': 'jane@example.com',
|
||||
'company': 'Acme Corp',
|
||||
}
|
||||
)
|
||||
|
||||
result = await saas_get_user(
|
||||
provider_tokens=None,
|
||||
access_token=SecretStr('test-token'),
|
||||
user_id='248289761001',
|
||||
)
|
||||
|
||||
assert isinstance(result, User)
|
||||
assert result.company == 'Acme Corp'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_fallback_user_company_is_none_when_absent(
|
||||
mock_token_manager, mock_check_idp
|
||||
):
|
||||
"""When 'company' is not in Keycloak claims, company should be None."""
|
||||
from server.routes.user import saas_get_user
|
||||
|
||||
mock_token_manager.get_user_info = AsyncMock(
|
||||
return_value={
|
||||
'sub': '248289761001',
|
||||
'name': 'Jane Doe',
|
||||
'preferred_username': 'j.doe',
|
||||
'email': 'jane@example.com',
|
||||
}
|
||||
)
|
||||
|
||||
result = await saas_get_user(
|
||||
provider_tokens=None,
|
||||
access_token=SecretStr('test-token'),
|
||||
user_id='248289761001',
|
||||
)
|
||||
|
||||
assert isinstance(result, User)
|
||||
assert result.company is None
|
||||
@@ -1,16 +1,16 @@
|
||||
import uuid
|
||||
from contextlib import asynccontextmanager
|
||||
from unittest.mock import AsyncMock, MagicMock, patch
|
||||
|
||||
import pytest
|
||||
from pydantic import SecretStr
|
||||
|
||||
# Mock the database module before importing UserStore
|
||||
with patch('storage.database.engine'), patch('storage.database.a_engine'):
|
||||
from storage.user import User
|
||||
from storage.user_store import UserStore
|
||||
|
||||
from sqlalchemy.orm import configure_mappers
|
||||
|
||||
# Database connection is lazy (no module-level engines), so no patching needed
|
||||
from storage.org import Org
|
||||
from storage.user import User
|
||||
from storage.user_store import UserStore
|
||||
|
||||
from openhands.storage.data_models.settings import Settings
|
||||
|
||||
|
||||
@@ -162,3 +162,363 @@ def test_get_kwargs_from_settings():
|
||||
assert 'enable_sound_notifications' in kwargs
|
||||
# Should not include fields that don't exist in User model
|
||||
assert 'llm_api_key' not in kwargs
|
||||
|
||||
|
||||
# --- Tests for contact_name resolution in migrate_user() ---
|
||||
# migrate_user() should use resolve_display_name() to populate contact_name
|
||||
# from Keycloak name claims, falling back to username only when no real name
|
||||
# is available. This mirrors the create_user() fix and ensures migrated Org
|
||||
# records also store the user's actual display name.
|
||||
|
||||
|
||||
class _StopAfterOrgCreation(Exception):
|
||||
"""Halt migrate_user() after Org creation for contact_name inspection."""
|
||||
|
||||
pass
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_user_contact_name_uses_name_claim():
|
||||
"""When user_info has a 'name' claim, migrate_user() should use it for contact_name."""
|
||||
user_id = str(uuid.uuid4())
|
||||
user_info = {
|
||||
'username': 'jdoe',
|
||||
'email': 'jdoe@example.com',
|
||||
'name': 'John Doe',
|
||||
}
|
||||
|
||||
mock_session = MagicMock()
|
||||
mock_sm = MagicMock()
|
||||
mock_sm.return_value.__enter__ = MagicMock(return_value=mock_session)
|
||||
mock_sm.return_value.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
mock_user_settings = MagicMock()
|
||||
mock_user_settings.user_version = 1
|
||||
|
||||
with (
|
||||
patch('storage.user_store.session_maker', mock_sm),
|
||||
patch(
|
||||
'storage.user_store.decrypt_legacy_model',
|
||||
return_value={'keycloak_user_id': user_id},
|
||||
),
|
||||
patch('storage.user_store.UserSettings'),
|
||||
patch(
|
||||
'storage.lite_llm_manager.LiteLlmManager.migrate_entries',
|
||||
new_callable=AsyncMock,
|
||||
side_effect=_StopAfterOrgCreation,
|
||||
),
|
||||
):
|
||||
with pytest.raises(_StopAfterOrgCreation):
|
||||
await UserStore.migrate_user(user_id, mock_user_settings, user_info)
|
||||
|
||||
org = mock_session.add.call_args_list[0][0][0]
|
||||
assert isinstance(org, Org)
|
||||
assert org.contact_name == 'John Doe'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_user_contact_name_uses_given_family_names():
|
||||
"""When only given_name and family_name are present, migrate_user() should combine them."""
|
||||
user_id = str(uuid.uuid4())
|
||||
user_info = {
|
||||
'username': 'jsmith',
|
||||
'email': 'jsmith@example.com',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Smith',
|
||||
}
|
||||
|
||||
mock_session = MagicMock()
|
||||
mock_sm = MagicMock()
|
||||
mock_sm.return_value.__enter__ = MagicMock(return_value=mock_session)
|
||||
mock_sm.return_value.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
mock_user_settings = MagicMock()
|
||||
mock_user_settings.user_version = 1
|
||||
|
||||
with (
|
||||
patch('storage.user_store.session_maker', mock_sm),
|
||||
patch(
|
||||
'storage.user_store.decrypt_legacy_model',
|
||||
return_value={'keycloak_user_id': user_id},
|
||||
),
|
||||
patch('storage.user_store.UserSettings'),
|
||||
patch(
|
||||
'storage.lite_llm_manager.LiteLlmManager.migrate_entries',
|
||||
new_callable=AsyncMock,
|
||||
side_effect=_StopAfterOrgCreation,
|
||||
),
|
||||
):
|
||||
with pytest.raises(_StopAfterOrgCreation):
|
||||
await UserStore.migrate_user(user_id, mock_user_settings, user_info)
|
||||
|
||||
org = mock_session.add.call_args_list[0][0][0]
|
||||
assert isinstance(org, Org)
|
||||
assert org.contact_name == 'Jane Smith'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_migrate_user_contact_name_falls_back_to_username():
|
||||
"""When no name claims exist, migrate_user() should fall back to username."""
|
||||
user_id = str(uuid.uuid4())
|
||||
user_info = {
|
||||
'username': 'jdoe',
|
||||
'email': 'jdoe@example.com',
|
||||
}
|
||||
|
||||
mock_session = MagicMock()
|
||||
mock_sm = MagicMock()
|
||||
mock_sm.return_value.__enter__ = MagicMock(return_value=mock_session)
|
||||
mock_sm.return_value.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
mock_user_settings = MagicMock()
|
||||
mock_user_settings.user_version = 1
|
||||
|
||||
with (
|
||||
patch('storage.user_store.session_maker', mock_sm),
|
||||
patch(
|
||||
'storage.user_store.decrypt_legacy_model',
|
||||
return_value={'keycloak_user_id': user_id},
|
||||
),
|
||||
patch('storage.user_store.UserSettings'),
|
||||
patch(
|
||||
'storage.lite_llm_manager.LiteLlmManager.migrate_entries',
|
||||
new_callable=AsyncMock,
|
||||
side_effect=_StopAfterOrgCreation,
|
||||
),
|
||||
):
|
||||
with pytest.raises(_StopAfterOrgCreation):
|
||||
await UserStore.migrate_user(user_id, mock_user_settings, user_info)
|
||||
|
||||
org = mock_session.add.call_args_list[0][0][0]
|
||||
assert isinstance(org, Org)
|
||||
assert org.contact_name == 'jdoe'
|
||||
|
||||
|
||||
# --- Tests for contact_name resolution in create_user() ---
|
||||
# create_user() should use resolve_display_name() to populate contact_name
|
||||
# from Keycloak name claims, falling back to preferred_username only when
|
||||
# no real name is available. This ensures Org records store the user's
|
||||
# actual display name for use in UI and analytics.
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_user_contact_name_uses_name_claim():
|
||||
"""When user_info has a 'name' claim, create_user() should use it for contact_name."""
|
||||
user_id = str(uuid.uuid4())
|
||||
user_info = {
|
||||
'preferred_username': 'jdoe',
|
||||
'email': 'jdoe@example.com',
|
||||
'name': 'John Doe',
|
||||
}
|
||||
|
||||
mock_session = MagicMock()
|
||||
mock_sm = MagicMock()
|
||||
mock_sm.return_value.__enter__ = MagicMock(return_value=mock_session)
|
||||
mock_sm.return_value.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
with (
|
||||
patch('storage.user_store.session_maker', mock_sm),
|
||||
patch.object(
|
||||
UserStore,
|
||||
'create_default_settings',
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
result = await UserStore.create_user(user_id, user_info)
|
||||
|
||||
assert result is None # create_default_settings returned None
|
||||
# The Org should have been added to the session with the real display name
|
||||
org = mock_session.add.call_args_list[0][0][0]
|
||||
assert isinstance(org, Org)
|
||||
assert org.contact_name == 'John Doe'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_user_contact_name_uses_given_family_names():
|
||||
"""When only given_name and family_name are present, create_user() should combine them."""
|
||||
user_id = str(uuid.uuid4())
|
||||
user_info = {
|
||||
'preferred_username': 'jsmith',
|
||||
'email': 'jsmith@example.com',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Smith',
|
||||
}
|
||||
|
||||
mock_session = MagicMock()
|
||||
mock_sm = MagicMock()
|
||||
mock_sm.return_value.__enter__ = MagicMock(return_value=mock_session)
|
||||
mock_sm.return_value.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
with (
|
||||
patch('storage.user_store.session_maker', mock_sm),
|
||||
patch.object(
|
||||
UserStore,
|
||||
'create_default_settings',
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
result = await UserStore.create_user(user_id, user_info)
|
||||
|
||||
assert result is None
|
||||
org = mock_session.add.call_args_list[0][0][0]
|
||||
assert isinstance(org, Org)
|
||||
assert org.contact_name == 'Jane Smith'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_create_user_contact_name_falls_back_to_username():
|
||||
"""When no name claims exist, create_user() should fall back to preferred_username."""
|
||||
user_id = str(uuid.uuid4())
|
||||
user_info = {
|
||||
'preferred_username': 'jdoe',
|
||||
'email': 'jdoe@example.com',
|
||||
}
|
||||
|
||||
mock_session = MagicMock()
|
||||
mock_sm = MagicMock()
|
||||
mock_sm.return_value.__enter__ = MagicMock(return_value=mock_session)
|
||||
mock_sm.return_value.__exit__ = MagicMock(return_value=False)
|
||||
|
||||
with (
|
||||
patch('storage.user_store.session_maker', mock_sm),
|
||||
patch.object(
|
||||
UserStore,
|
||||
'create_default_settings',
|
||||
new_callable=AsyncMock,
|
||||
return_value=None,
|
||||
),
|
||||
):
|
||||
result = await UserStore.create_user(user_id, user_info)
|
||||
|
||||
assert result is None
|
||||
org = mock_session.add.call_args_list[0][0][0]
|
||||
assert isinstance(org, Org)
|
||||
assert org.contact_name == 'jdoe'
|
||||
|
||||
|
||||
# --- Tests for backfill_contact_name on login ---
|
||||
# Existing users created before the resolve_display_name fix may have
|
||||
# username-style values in contact_name. The backfill updates these to
|
||||
# the user's real display name when they next log in, but preserves
|
||||
# custom values set via the PATCH endpoint.
|
||||
|
||||
|
||||
def _wrap_sync_as_async_session_maker(sync_sm):
|
||||
"""Wrap a sync session_maker so it can be used in place of a_session_maker."""
|
||||
|
||||
@asynccontextmanager
|
||||
async def _async_sm():
|
||||
session = sync_sm()
|
||||
try:
|
||||
|
||||
class _AsyncWrapper:
|
||||
async def execute(self, *args, **kwargs):
|
||||
return session.execute(*args, **kwargs)
|
||||
|
||||
async def commit(self):
|
||||
session.commit()
|
||||
|
||||
yield _AsyncWrapper()
|
||||
finally:
|
||||
session.close()
|
||||
|
||||
return _async_sm
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_backfill_contact_name_updates_when_matches_preferred_username(
|
||||
session_maker,
|
||||
):
|
||||
"""When contact_name matches preferred_username and a real name is available, update it."""
|
||||
user_id = str(uuid.uuid4())
|
||||
# Create org with username-style contact_name (as create_user used to store)
|
||||
with session_maker() as session:
|
||||
org = Org(
|
||||
id=uuid.UUID(user_id),
|
||||
name=f'user_{user_id}_org',
|
||||
contact_name='jdoe',
|
||||
contact_email='jdoe@example.com',
|
||||
)
|
||||
session.add(org)
|
||||
session.commit()
|
||||
|
||||
user_info = {
|
||||
'preferred_username': 'jdoe',
|
||||
'name': 'John Doe',
|
||||
}
|
||||
|
||||
with patch(
|
||||
'storage.user_store.a_session_maker',
|
||||
_wrap_sync_as_async_session_maker(session_maker),
|
||||
):
|
||||
await UserStore.backfill_contact_name(user_id, user_info)
|
||||
|
||||
with session_maker() as session:
|
||||
org = session.query(Org).filter(Org.id == uuid.UUID(user_id)).first()
|
||||
assert org.contact_name == 'John Doe'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_backfill_contact_name_updates_when_matches_username(session_maker):
|
||||
"""When contact_name matches username (migrate_user legacy) and a real name is available, update it."""
|
||||
user_id = str(uuid.uuid4())
|
||||
# Create org with username-style contact_name (as migrate_user used to store)
|
||||
with session_maker() as session:
|
||||
org = Org(
|
||||
id=uuid.UUID(user_id),
|
||||
name=f'user_{user_id}_org',
|
||||
contact_name='jdoe',
|
||||
contact_email='jdoe@example.com',
|
||||
)
|
||||
session.add(org)
|
||||
session.commit()
|
||||
|
||||
user_info = {
|
||||
'username': 'jdoe',
|
||||
'given_name': 'Jane',
|
||||
'family_name': 'Doe',
|
||||
}
|
||||
|
||||
with patch(
|
||||
'storage.user_store.a_session_maker',
|
||||
_wrap_sync_as_async_session_maker(session_maker),
|
||||
):
|
||||
await UserStore.backfill_contact_name(user_id, user_info)
|
||||
|
||||
with session_maker() as session:
|
||||
org = session.query(Org).filter(Org.id == uuid.UUID(user_id)).first()
|
||||
assert org.contact_name == 'Jane Doe'
|
||||
|
||||
|
||||
@pytest.mark.asyncio
|
||||
async def test_backfill_contact_name_preserves_custom_value(session_maker):
|
||||
"""When contact_name differs from both username fields, do not overwrite it."""
|
||||
user_id = str(uuid.uuid4())
|
||||
# Org has a custom contact_name set via PATCH endpoint
|
||||
with session_maker() as session:
|
||||
org = Org(
|
||||
id=uuid.UUID(user_id),
|
||||
name=f'user_{user_id}_org',
|
||||
contact_name='Custom Corp Name',
|
||||
contact_email='jdoe@example.com',
|
||||
)
|
||||
session.add(org)
|
||||
session.commit()
|
||||
|
||||
user_info = {
|
||||
'preferred_username': 'jdoe',
|
||||
'username': 'jdoe',
|
||||
'name': 'John Doe',
|
||||
}
|
||||
|
||||
with patch(
|
||||
'storage.user_store.a_session_maker',
|
||||
_wrap_sync_as_async_session_maker(session_maker),
|
||||
):
|
||||
await UserStore.backfill_contact_name(user_id, user_info)
|
||||
|
||||
with session_maker() as session:
|
||||
org = session.query(Org).filter(Org.id == uuid.UUID(user_id)).first()
|
||||
assert org.contact_name == 'Custom Corp Name'
|
||||
|
||||
0
enterprise/utils/__init__.py
Normal file
0
enterprise/utils/__init__.py
Normal file
25
enterprise/utils/identity.py
Normal file
25
enterprise/utils/identity.py
Normal file
@@ -0,0 +1,25 @@
|
||||
"""Shared identity helpers for resolving user profile fields from Keycloak claims."""
|
||||
|
||||
|
||||
def resolve_display_name(user_info: dict) -> str | None:
|
||||
"""Resolve the best available display name from Keycloak user_info claims.
|
||||
|
||||
Fallback chain: name → given_name + family_name → None
|
||||
|
||||
Does NOT fall back to preferred_username/username — callers that need
|
||||
a guaranteed non-None value should handle that separately. This keeps
|
||||
the helper focused on real-name claims so that the /api/user/info route
|
||||
can return name=None when no real name is available, while user_store
|
||||
callers can append their own username fallback.
|
||||
"""
|
||||
name = user_info.get('name', '')
|
||||
if name and name.strip():
|
||||
return name.strip()
|
||||
|
||||
given = user_info.get('given_name', '').strip()
|
||||
family = user_info.get('family_name', '').strip()
|
||||
combined = f'{given} {family}'.strip()
|
||||
if combined:
|
||||
return combined
|
||||
|
||||
return None
|
||||
Reference in New Issue
Block a user