From b5e83679aff13c5b7866700ac1f5bdf620df3629 Mon Sep 17 00:00:00 2001 From: hieptl Date: Thu, 18 Dec 2025 15:46:13 +0700 Subject: [PATCH] refactor: add description field support for secrets --- .../live_status_app_conversation_service.py | 6 +- .../app_server/user/auth_user_context.py | 7 +- ...st_live_status_app_conversation_service.py | 180 +++++++++++++++--- 3 files changed, 162 insertions(+), 31 deletions(-) diff --git a/openhands/app_server/app_conversation/live_status_app_conversation_service.py b/openhands/app_server/app_conversation/live_status_app_conversation_service.py index a8d490489c..e51b2ba0a8 100644 --- a/openhands/app_server/app_conversation/live_status_app_conversation_service.py +++ b/openhands/app_server/app_conversation/live_status_app_conversation_service.py @@ -557,6 +557,7 @@ class LiveStatusAppConversationService(AppConversationServiceBase): continue secret_name = f'{provider_type.name}_TOKEN' + description = f'{provider_type.name} authentication token' if self.web_url: # Create an access token for web-based authentication @@ -576,12 +577,15 @@ class LiveStatusAppConversationService(AppConversationServiceBase): secrets[secret_name] = LookupSecret( url=self.web_url + '/api/v1/webhooks/secrets', headers=headers, + description=description, ) else: # Use static token for environments without web URL access static_token = await self.user_context.get_latest_token(provider_type) if static_token: - secrets[secret_name] = StaticSecret(value=static_token) + secrets[secret_name] = StaticSecret( + value=static_token, description=description + ) return secrets diff --git a/openhands/app_server/user/auth_user_context.py b/openhands/app_server/user/auth_user_context.py index 4d64888427..7adf7f902a 100644 --- a/openhands/app_server/user/auth_user_context.py +++ b/openhands/app_server/user/auth_user_context.py @@ -81,7 +81,12 @@ class AuthUserContext(UserContext): secrets = await self.user_auth.get_secrets() if secrets: for name, custom_secret in secrets.custom_secrets.items(): - results[name] = StaticSecret(value=custom_secret.secret) + results[name] = StaticSecret( + value=custom_secret.secret, + description=custom_secret.description + if custom_secret.description + else None, + ) return results diff --git a/tests/unit/app_server/test_live_status_app_conversation_service.py b/tests/unit/app_server/test_live_status_app_conversation_service.py index f662f33146..92dd7773ca 100644 --- a/tests/unit/app_server/test_live_status_app_conversation_service.py +++ b/tests/unit/app_server/test_live_status_app_conversation_service.py @@ -4,6 +4,7 @@ from unittest.mock import AsyncMock, Mock, patch from uuid import UUID, uuid4 import pytest +from pydantic import SecretStr from openhands.agent_server.models import SendMessageRequest, StartConversationRequest from openhands.app_server.app_conversation.app_conversation_models import ( @@ -21,7 +22,7 @@ from openhands.app_server.sandbox.sandbox_models import ( ) from openhands.app_server.sandbox.sandbox_spec_models import SandboxSpecInfo from openhands.app_server.user.user_context import UserContext -from openhands.integrations.provider import ProviderType +from openhands.integrations.provider import ProviderToken, ProviderType from openhands.sdk import Agent from openhands.sdk.llm import LLM from openhands.sdk.secret import LookupSecret, StaticSecret @@ -104,10 +105,6 @@ class TestLiveStatusAppConversationService: async def test_setup_secrets_for_git_providers_with_web_url(self): """Test _setup_secrets_for_git_providers with web URL (creates access token).""" # Arrange - from pydantic import SecretStr - - from openhands.integrations.provider import ProviderToken - base_secrets = {} self.mock_user_context.get_secrets.return_value = base_secrets self.mock_jwt_service.create_jws_token.return_value = 'test_access_token' @@ -134,6 +131,9 @@ class TestLiveStatusAppConversationService: == 'https://test.example.com/api/v1/webhooks/secrets' ) assert result['GITHUB_TOKEN'].headers['X-Access-Token'] == 'test_access_token' + # Verify descriptions are included + assert result['GITHUB_TOKEN'].description == 'GITHUB authentication token' + assert result['GITLAB_TOKEN'].description == 'GITLAB authentication token' # Should be called twice, once for each provider assert self.mock_jwt_service.create_jws_token.call_count == 2 @@ -142,10 +142,6 @@ class TestLiveStatusAppConversationService: async def test_setup_secrets_for_git_providers_with_saas_mode(self): """Test _setup_secrets_for_git_providers with SaaS mode (includes keycloak cookie).""" # Arrange - from pydantic import SecretStr - - from openhands.integrations.provider import ProviderToken - self.service.app_mode = 'saas' self.service.keycloak_auth_cookie = 'test_cookie' base_secrets = {} @@ -169,15 +165,13 @@ class TestLiveStatusAppConversationService: assert isinstance(lookup_secret, LookupSecret) assert 'Cookie' in lookup_secret.headers assert lookup_secret.headers['Cookie'] == 'keycloak_auth=test_cookie' + # Verify description is included + assert lookup_secret.description == 'GITLAB authentication token' @pytest.mark.asyncio async def test_setup_secrets_for_git_providers_without_web_url(self): """Test _setup_secrets_for_git_providers without web URL (uses static token).""" # Arrange - from pydantic import SecretStr - - from openhands.integrations.provider import ProviderToken - self.service.web_url = None base_secrets = {} self.mock_user_context.get_secrets.return_value = base_secrets @@ -198,6 +192,8 @@ class TestLiveStatusAppConversationService: assert 'GITHUB_TOKEN' in result assert isinstance(result['GITHUB_TOKEN'], StaticSecret) assert result['GITHUB_TOKEN'].value.get_secret_value() == 'static_token_value' + # Verify description is included + assert result['GITHUB_TOKEN'].description == 'GITHUB authentication token' self.mock_user_context.get_latest_token.assert_called_once_with( ProviderType.GITHUB ) @@ -206,10 +202,6 @@ class TestLiveStatusAppConversationService: async def test_setup_secrets_for_git_providers_no_static_token(self): """Test _setup_secrets_for_git_providers when no static token is available.""" # Arrange - from pydantic import SecretStr - - from openhands.integrations.provider import ProviderToken - self.service.web_url = None base_secrets = {} self.mock_user_context.get_secrets.return_value = base_secrets @@ -230,6 +222,148 @@ class TestLiveStatusAppConversationService: assert 'GITHUB_TOKEN' not in result assert result == base_secrets + @pytest.mark.asyncio + async def test_setup_secrets_for_git_providers_descriptions_included(self): + """Test _setup_secrets_for_git_providers includes descriptions for all provider types.""" + # Arrange + base_secrets = {} + self.mock_user_context.get_secrets.return_value = base_secrets + self.mock_jwt_service.create_jws_token.return_value = 'test_access_token' + + # Mock provider tokens for multiple providers + provider_tokens = { + ProviderType.GITHUB: ProviderToken(token=SecretStr('github_token')), + ProviderType.GITLAB: ProviderToken(token=SecretStr('gitlab_token')), + ProviderType.BITBUCKET: ProviderToken(token=SecretStr('bitbucket_token')), + } + self.mock_user_context.get_provider_tokens = AsyncMock( + return_value=provider_tokens + ) + + # Act + result = await self.service._setup_secrets_for_git_providers(self.mock_user) + + # Assert - verify all secrets have correct descriptions + assert 'GITHUB_TOKEN' in result + assert isinstance(result['GITHUB_TOKEN'], LookupSecret) + assert result['GITHUB_TOKEN'].description == 'GITHUB authentication token' + + assert 'GITLAB_TOKEN' in result + assert isinstance(result['GITLAB_TOKEN'], LookupSecret) + assert result['GITLAB_TOKEN'].description == 'GITLAB authentication token' + + assert 'BITBUCKET_TOKEN' in result + assert isinstance(result['BITBUCKET_TOKEN'], LookupSecret) + assert result['BITBUCKET_TOKEN'].description == 'BITBUCKET authentication token' + + @pytest.mark.asyncio + async def test_setup_secrets_for_git_providers_static_secret_description(self): + """Test _setup_secrets_for_git_providers includes description for StaticSecret.""" + # Arrange + self.service.web_url = None + base_secrets = {} + self.mock_user_context.get_secrets.return_value = base_secrets + self.mock_user_context.get_latest_token.return_value = 'static_token_value' + + # Mock provider tokens for multiple providers + provider_tokens = { + ProviderType.GITHUB: ProviderToken(token=SecretStr('github_token')), + ProviderType.GITLAB: ProviderToken(token=SecretStr('gitlab_token')), + } + self.mock_user_context.get_provider_tokens = AsyncMock( + return_value=provider_tokens + ) + + # Act + result = await self.service._setup_secrets_for_git_providers(self.mock_user) + + # Assert - verify StaticSecret objects have descriptions + assert 'GITHUB_TOKEN' in result + assert isinstance(result['GITHUB_TOKEN'], StaticSecret) + assert result['GITHUB_TOKEN'].description == 'GITHUB authentication token' + + assert 'GITLAB_TOKEN' in result + assert isinstance(result['GITLAB_TOKEN'], StaticSecret) + assert result['GITLAB_TOKEN'].description == 'GITLAB authentication token' + + @pytest.mark.asyncio + async def test_setup_secrets_for_git_providers_preserves_custom_secret_descriptions( + self, + ): + """Test _setup_secrets_for_git_providers preserves descriptions from custom secrets.""" + # Arrange + # Mock custom secrets with descriptions + custom_secret_with_desc = StaticSecret( + value=SecretStr('custom_secret_value'), + description='Custom API key for external service', + ) + custom_secret_no_desc = StaticSecret( + value=SecretStr('another_secret_value'), + description=None, + ) + base_secrets = { + 'CUSTOM_API_KEY': custom_secret_with_desc, + 'ANOTHER_SECRET': custom_secret_no_desc, + } + self.mock_user_context.get_secrets.return_value = base_secrets + self.mock_jwt_service.create_jws_token.return_value = 'test_access_token' + + # Mock provider tokens + provider_tokens = { + ProviderType.GITHUB: ProviderToken(token=SecretStr('github_token')), + } + self.mock_user_context.get_provider_tokens = AsyncMock( + return_value=provider_tokens + ) + + # Act + result = await self.service._setup_secrets_for_git_providers(self.mock_user) + + # Assert - verify custom secrets are preserved with their descriptions + assert 'CUSTOM_API_KEY' in result + assert isinstance(result['CUSTOM_API_KEY'], StaticSecret) + assert ( + result['CUSTOM_API_KEY'].description + == 'Custom API key for external service' + ) + assert ( + result['CUSTOM_API_KEY'].value.get_secret_value() == 'custom_secret_value' + ) + + assert 'ANOTHER_SECRET' in result + assert isinstance(result['ANOTHER_SECRET'], StaticSecret) + assert result['ANOTHER_SECRET'].description is None + assert ( + result['ANOTHER_SECRET'].value.get_secret_value() == 'another_secret_value' + ) + + # Verify git provider token is also included + assert 'GITHUB_TOKEN' in result + assert result['GITHUB_TOKEN'].description == 'GITHUB authentication token' + + @pytest.mark.asyncio + async def test_setup_secrets_for_git_providers_custom_secret_empty_description( + self, + ): + """Test _setup_secrets_for_git_providers handles custom secrets with empty descriptions.""" + # Arrange + custom_secret_empty_desc = StaticSecret( + value=SecretStr('secret_value'), + description='', # Empty string description + ) + base_secrets = {'MY_SECRET': custom_secret_empty_desc} + self.mock_user_context.get_secrets.return_value = base_secrets + self.mock_user_context.get_provider_tokens = AsyncMock(return_value=None) + + # Act + result = await self.service._setup_secrets_for_git_providers(self.mock_user) + + # Assert - empty description should be preserved as-is + assert 'MY_SECRET' in result + assert isinstance(result['MY_SECRET'], StaticSecret) + # Empty string description is preserved + assert result['MY_SECRET'].description == '' + @pytest.mark.asyncio async def test_configure_llm_and_mcp_with_custom_model(self): """Test _configure_llm_and_mcp with custom LLM model.""" @@ -360,8 +494,6 @@ class TestLiveStatusAppConversationService: async def test_configure_llm_and_mcp_tavily_with_user_search_api_key(self): """Test _configure_llm_and_mcp adds tavily when user has search_api_key.""" # Arrange - from pydantic import SecretStr - self.mock_user.search_api_key = SecretStr('user_search_key') self.mock_user_context.get_mcp_api_key.return_value = 'mcp_api_key' @@ -406,8 +538,6 @@ class TestLiveStatusAppConversationService: async def test_configure_llm_and_mcp_tavily_user_key_takes_precedence(self): """Test _configure_llm_and_mcp user search_api_key takes precedence over env key.""" # Arrange - from pydantic import SecretStr - self.mock_user.search_api_key = SecretStr('user_search_key') self.service.tavily_api_key = 'env_tavily_key' self.mock_user_context.get_mcp_api_key.return_value = None @@ -476,8 +606,6 @@ class TestLiveStatusAppConversationService: Even in SAAS mode, if the user has their own search_api_key, tavily should be added. """ # Arrange - simulate SAAS mode with user having their own search key - from pydantic import SecretStr - self.service.app_mode = AppMode.SAAS.value self.service.tavily_api_key = None # In SAAS mode, this should be None self.mock_user.search_api_key = SecretStr('user_search_key') @@ -502,8 +630,6 @@ class TestLiveStatusAppConversationService: async def test_configure_llm_and_mcp_tavily_with_empty_user_search_key(self): """Test _configure_llm_and_mcp handles empty user search_api_key correctly.""" # Arrange - from pydantic import SecretStr - self.mock_user.search_api_key = SecretStr('') # Empty string self.service.tavily_api_key = 'env_tavily_key' self.mock_user_context.get_mcp_api_key.return_value = None @@ -527,8 +653,6 @@ class TestLiveStatusAppConversationService: async def test_configure_llm_and_mcp_tavily_with_whitespace_user_search_key(self): """Test _configure_llm_and_mcp handles whitespace-only user search_api_key correctly.""" # Arrange - from pydantic import SecretStr - self.mock_user.search_api_key = SecretStr(' ') # Whitespace only self.service.tavily_api_key = 'env_tavily_key' self.mock_user_context.get_mcp_api_key.return_value = None @@ -1072,8 +1196,6 @@ class TestLiveStatusAppConversationService: async def test_configure_llm_and_mcp_merges_system_and_custom_servers(self): """Test _configure_llm_and_mcp merges both system and custom MCP servers.""" # Arrange - from pydantic import SecretStr - from openhands.core.config.mcp_config import ( MCPConfig, MCPSSEServerConfig,