diff --git a/enterprise/server/routes/auth.py b/enterprise/server/routes/auth.py index 4118969274..e1b6d15e15 100644 --- a/enterprise/server/routes/auth.py +++ b/enterprise/server/routes/auth.py @@ -172,6 +172,13 @@ async def keycloak_callback( authorization = await user_authorizer.authorize_user(user_info) if not authorization.success: + # For duplicate_email errors, clean up the newly created Keycloak user + # (only if they're not already in our UserStore, i.e., they're a new user) + if authorization.error_detail == 'duplicate_email': + existing_user = await UserStore.get_user_by_id(user_info.sub) + if not existing_user: + # New user created during OAuth should be deleted from Keycloak + await token_manager.delete_keycloak_user(user_info.sub) # Return unauthorized raise HTTPException( status_code=status.HTTP_401_UNAUTHORIZED, diff --git a/enterprise/tests/unit/test_auth_routes.py b/enterprise/tests/unit/test_auth_routes.py index e672449fa9..25f5aadd8e 100644 --- a/enterprise/tests/unit/test_auth_routes.py +++ b/enterprise/tests/unit/test_auth_routes.py @@ -846,10 +846,108 @@ async def test_keycloak_callback_duplicate_email_detected( assert exc_info.value.detail == 'duplicate_email' -# Note: test_keycloak_callback_duplicate_email_deletion_fails was removed as part of -# the user authorization refactor. The Keycloak user deletion logic for duplicate emails -# has been removed from keycloak_callback. If this behavior needs to be restored, -# it should be implemented in the DefaultUserAuthorizer or handled separately. +@pytest.mark.asyncio +async def test_keycloak_callback_duplicate_email_deletes_new_keycloak_user( + mock_request, create_keycloak_user_info +): + """Test that new Keycloak user is deleted when duplicate email is detected. + + When a user attempts to sign up with a +modifier email (e.g., joe+1@example.com) + and an account with the base email already exists, the newly created Keycloak + user should be deleted to prevent orphaned accounts from blocking future sign-ins. + """ + with ( + patch('server.routes.auth.token_manager') as mock_token_manager, + patch('server.routes.auth.UserStore') as mock_user_store, + ): + # Arrange + mock_token_manager.get_keycloak_tokens = AsyncMock( + return_value=('test_access_token', 'test_refresh_token') + ) + mock_token_manager.get_user_info = AsyncMock( + return_value=create_keycloak_user_info( + sub='new_user_id', + preferred_username='test_user', + email='joe+1@example.com', + identity_provider='github', + ) + ) + mock_token_manager.delete_keycloak_user = AsyncMock(return_value=True) + + # User does NOT exist in UserStore (new signup attempt) + mock_user_store.get_user_by_id = AsyncMock(return_value=None) + + # Create mock authorizer that returns duplicate_email error + mock_authorizer = create_mock_user_authorizer( + success=False, error_detail='duplicate_email' + ) + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await keycloak_callback( + code='test_code', + state='test_state', + request=mock_request, + user_authorizer=mock_authorizer, + ) + + assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED + assert exc_info.value.detail == 'duplicate_email' + # Keycloak user should be deleted since user doesn't exist in UserStore + mock_token_manager.delete_keycloak_user.assert_called_once_with('new_user_id') + + +@pytest.mark.asyncio +async def test_keycloak_callback_duplicate_email_preserves_existing_user( + mock_request, create_keycloak_user_info +): + """Test that existing users are not deleted when duplicate email is detected. + + When an existing user signs in and duplicate email is detected (e.g., because + another account with the same base email was created while duplicate checking + was disabled), the existing user's Keycloak account should NOT be deleted. + """ + with ( + patch('server.routes.auth.token_manager') as mock_token_manager, + patch('server.routes.auth.UserStore') as mock_user_store, + ): + # Arrange + mock_token_manager.get_keycloak_tokens = AsyncMock( + return_value=('test_access_token', 'test_refresh_token') + ) + mock_token_manager.get_user_info = AsyncMock( + return_value=create_keycloak_user_info( + sub='existing_user_id', + preferred_username='test_user', + email='joe@example.com', + identity_provider='github', + ) + ) + mock_token_manager.delete_keycloak_user = AsyncMock(return_value=True) + + # User EXISTS in UserStore (legitimate existing user) + mock_existing_user = MagicMock() + mock_existing_user.id = 'existing_user_id' + mock_user_store.get_user_by_id = AsyncMock(return_value=mock_existing_user) + + # Create mock authorizer that returns duplicate_email error + mock_authorizer = create_mock_user_authorizer( + success=False, error_detail='duplicate_email' + ) + + # Act & Assert + with pytest.raises(HTTPException) as exc_info: + await keycloak_callback( + code='test_code', + state='test_state', + request=mock_request, + user_authorizer=mock_authorizer, + ) + + assert exc_info.value.status_code == status.HTTP_401_UNAUTHORIZED + assert exc_info.value.detail == 'duplicate_email' + # Keycloak user should NOT be deleted since user exists in UserStore + mock_token_manager.delete_keycloak_user.assert_not_called() @pytest.mark.asyncio