From 646ca7372a66c8d027118ee944893a960589dec8 Mon Sep 17 00:00:00 2001 From: Ralf Lang Date: Tue, 31 Mar 2026 21:32:52 +0200 Subject: [PATCH] feat: Transition to injection and handle user fallback horde/activesync#22 originally had an architecture violation, calling into Horde Registry and using globals from library level This change instead moves all user detection logic to an overloaded driver->getUser method in horde/core level. This keeps the activesync library clean of horde glue. As a side goal we inject the registry and the modern PSR-7 request from the factory. Horde_ActiveSync normally uses the homegrown Horde_Controller_Request object but we can wrap the PSR-7 object into a Horde_Controller_Request (Psr7Wrapper). So going forward we will step by step migrate to PSR-7 standard. The original PR's fallback logic is kept (base getUser implementation, get "user" parameter from request, fall back to authenticated horde user) --- lib/Horde/Core/ActiveSync/Driver.php | 63 +++++ lib/Horde/Core/Factory/ActiveSyncBackend.php | 23 +- test/ActiveSyncTests.php | 92 ++++++- test/Unit/ActiveSync/DriverGetUserTest.php | 252 ++++++++++++++++++ .../Factory/ActiveSyncBackendFactoryTest.php | 183 +++++++++++++ 5 files changed, 610 insertions(+), 3 deletions(-) create mode 100644 test/Unit/ActiveSync/DriverGetUserTest.php create mode 100644 test/Unit/Factory/ActiveSyncBackendFactoryTest.php diff --git a/lib/Horde/Core/ActiveSync/Driver.php b/lib/Horde/Core/ActiveSync/Driver.php index 297ef75b..7b53e731 100644 --- a/lib/Horde/Core/ActiveSync/Driver.php +++ b/lib/Horde/Core/ActiveSync/Driver.php @@ -1,5 +1,7 @@ Id map * @@ -157,6 +173,20 @@ public function __construct(array $params = []) throw new InvalidArgumentException('Missing required Auth object'); } + $serverRequest = self::extractArrayValue($params, 'serverrequest'); + if ($serverRequest instanceof ServerRequestInterface) { + $this->_serverRequest = $serverRequest; + } else { + throw new InvalidArgumentException('Missing required PSR-7 ServerRequest object.'); + } + + $registry = self::extractArrayValue($params, 'registry'); + if ($registry instanceof Horde_Registry) { + $this->_registry = $registry; + } else { + throw new InvalidArgumentException('Missing required Horde_Registry object.'); + } + $this->_imap = self::extractArrayValue($params, 'imap'); $this->_cache = self::extractArrayValue($params, 'cache'); @@ -310,6 +340,39 @@ public function clearAuthentication() return true; } + /** + * Get the username for this request with Horde-specific resolution logic. + * + * Priority order: + * 1. Authenticated user (from authenticate() flow) + * 2. GET parameter ?User=username (Horde ActiveSync extension for device provisioning) + * 3. Registry authenticated user (fallback for edge cases) + * + * @return string The resolved username + */ + public function getUser() + { + // Priority 1: Authenticated user (from parent::authenticate()) + $user = parent::getUser(); + + // Priority 2: GET parameter (Horde-specific ActiveSync extension) + // Used in device provisioning scenarios where auth hasn't completed yet + if (empty($user)) { + $queryParams = $this->_serverRequest->getQueryParams(); + if (!empty($queryParams['User'])) { + $user = $queryParams['User']; + } + } + + // Priority 3: Registry auth fallback + // For scenarios where request comes from authenticated session + if (empty($user)) { + $user = $this->_registry->getAuth(); + } + + return $user; + } + /** * Setup sync parameters. The user provided here is the user the backend * will sync with. This allows you to authenticate as one user, and sync as diff --git a/lib/Horde/Core/Factory/ActiveSyncBackend.php b/lib/Horde/Core/Factory/ActiveSyncBackend.php index 243a567c..5175179a 100644 --- a/lib/Horde/Core/Factory/ActiveSyncBackend.php +++ b/lib/Horde/Core/Factory/ActiveSyncBackend.php @@ -1,5 +1,7 @@ get(ServerRequest::class); + } catch (Horde_Exception_NotFound $e) { + // Fallback: Create from PHP superglobals + $serverRequest = new ServerRequest( + $_SERVER['REQUEST_METHOD'] ?? 'GET', + $_SERVER['REQUEST_URI'] ?? '/', + getallheaders() ?: [], + fopen('php://input', 'r'), + $_SERVER['SERVER_PROTOCOL'] ?? '1.1', + $_SERVER + ); + } + // Backend driver and dependencies $params = ['registry' => $registry]; $adapter_params = ['factory' => new Horde_Core_ActiveSync_Imap_Factory()]; @@ -21,13 +38,15 @@ public function create(Horde_Injector $injector) $driver_params = [ 'connector' => new Horde_Core_ActiveSync_Connector($params), + 'serverrequest' => $serverRequest, + 'registry' => $registry, 'imap' => $emailsyncEnabled ? new Horde_ActiveSync_Imap_Adapter($adapter_params) : null, 'ping' => $conf['activesync']['ping'], - 'state' => $injector->getInstance('Horde_ActiveSyncState'), + 'state' => $injector->get('Horde_ActiveSyncState'), 'auth' => $this->_getAuth(), - 'cache' => $injector->getInstance('Horde_Cache')]; + 'cache' => $injector->get('Horde_Cache')]; return new Horde_Core_ActiveSync_Driver($driver_params); } diff --git a/test/ActiveSyncTests.php b/test/ActiveSyncTests.php index 983e495d..1e8246a9 100644 --- a/test/ActiveSyncTests.php +++ b/test/ActiveSyncTests.php @@ -31,7 +31,7 @@ class ActiveSyncTests extends TestCase protected $_mailboxes; protected $_special; - public function setUp() + public function setUp(): void { $this->_auth = $this->getMockSkipConstructor('Horde_Auth_Auto'); $this->_state = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); @@ -465,4 +465,94 @@ public function testFbGeneration() $expected = '440000000000000000000000000000220000000000000000'; $this->assertEquals($expected, $fb); } + + public function testGetUserReturnsAuthenticatedUser() + { + $serverRequest = new \Horde\Http\ServerRequest('POST', '/'); + $connector = new MockConnector(); + $mockRegistry = $this->getMockSkipConstructor('Horde_Registry'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'state' => $this->_state, + 'connector' => $connector, + 'auth' => $this->_auth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'imap' => null, + ]); + + // Simulate authentication + $driver->authenticate('authenticated_user', 'password'); + + $this->assertEquals('authenticated_user', $driver->getUser()); + } + + public function testGetUserFallsBackToGetParameter() + { + $uri = new \Horde\Http\Uri('http://example.com/path?User=get_param_user'); + $serverRequest = new \Horde\Http\ServerRequest('POST', $uri); + $connector = new MockConnector(); + $mockRegistry = $this->getMockSkipConstructor('Horde_Registry'); + + $mockRegistry->expects($this->never()) + ->method('getAuth'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'state' => $this->_state, + 'connector' => $connector, + 'auth' => $this->_auth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'imap' => null, + ]); + + // No authentication, should use GET parameter + $this->assertEquals('get_param_user', $driver->getUser()); + } + + public function testGetUserFallsBackToRegistry() + { + $serverRequest = new \Horde\Http\ServerRequest('POST', '/'); + $connector = new MockConnector(); + $mockRegistry = $this->getMockSkipConstructor('Horde_Registry'); + + $mockRegistry->expects($this->once()) + ->method('getAuth') + ->willReturn('registry_user'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'state' => $this->_state, + 'connector' => $connector, + 'auth' => $this->_auth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'imap' => null, + ]); + + // No authentication, no GET parameter, should use registry + $this->assertEquals('registry_user', $driver->getUser()); + } + + public function testGetParameterOverridesRegistry() + { + $uri = new \Horde\Http\Uri('http://example.com/path?User=get_param_user'); + $serverRequest = new \Horde\Http\ServerRequest('POST', $uri); + $connector = new MockConnector(); + $mockRegistry = $this->getMockSkipConstructor('Horde_Registry'); + + $mockRegistry->expects($this->never()) + ->method('getAuth'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'state' => $this->_state, + 'connector' => $connector, + 'auth' => $this->_auth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'imap' => null, + ]); + + // No authentication, GET parameter present, should not call registry + $this->assertEquals('get_param_user', $driver->getUser()); + } } diff --git a/test/Unit/ActiveSync/DriverGetUserTest.php b/test/Unit/ActiveSync/DriverGetUserTest.php new file mode 100644 index 00000000..d7ec9b2d --- /dev/null +++ b/test/Unit/ActiveSync/DriverGetUserTest.php @@ -0,0 +1,252 @@ + + * @category Horde + * @license http://www.horde.org/licenses/lgpl21 LGPL + * @package Core + * @subpackage UnitTests + */ + +namespace Horde\Core\Test\Unit\ActiveSync; + +use Horde\Test\TestCase; +use Horde\Http\ServerRequest; +use Horde\Http\Uri; +use PHPUnit\Framework\Attributes\CoversClass; +use Horde_Core_ActiveSync_Driver; + +/** + * Unit tests for Horde_Core_ActiveSync_Driver::getUser() priority logic + * + * Note: These tests require horde/activesync to be available. + * Run with: phpunit --bootstrap test/bootstrap.php test/Unit/ActiveSync/DriverGetUserTest.php + * + * @author Ralf Lang + * @category Horde + * @license http://www.horde.org/licenses/lgpl21 LGPL + * @package Core + * @subpackage UnitTests + */ +#[CoversClass(Horde_Core_ActiveSync_Driver::class)] +class DriverGetUserTest extends TestCase +{ + /** + * Test Priority 1: Authenticated user takes precedence + */ + public function testGetUserReturnsAuthenticatedUser(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $serverRequest = new ServerRequest('POST', '/'); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockAuth->method('authenticate')->willReturn(true); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + $mockRegistry = $this->getMockSkipConstructor(\Horde_Registry::class); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'state' => $mockState, + ]); + + // Simulate authentication - this sets _authUser in parent + $result = $driver->authenticate('authenticated_user', 'password'); + + $this->assertEquals('authenticated_user', $driver->getUser()); + } + + /** + * Test Priority 2: GET parameter when no authenticated user + */ + public function testGetUserFallsBackToGetParameter(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $uri = new Uri('http://example.com/path?User=get_param_user&DeviceId=123'); + $serverRequest = new ServerRequest('POST', $uri); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + $mockRegistry = $this->getMockSkipConstructor(\Horde_Registry::class); + + $mockRegistry->expects($this->never()) + ->method('getAuth'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'state' => $mockState, + ]); + + // No authentication, should use GET parameter + $this->assertEquals('get_param_user', $driver->getUser()); + } + + /** + * Test Priority 3: Registry fallback when no auth and no GET parameter + */ + public function testGetUserFallsBackToRegistry(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $serverRequest = new ServerRequest('POST', '/'); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + $mockRegistry = $this->getMockSkipConstructor(\Horde_Registry::class); + + $mockRegistry->expects($this->once()) + ->method('getAuth') + ->willReturn('registry_user'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'state' => $mockState, + ]); + + // No authentication, no GET parameter, should use registry + $this->assertEquals('registry_user', $driver->getUser()); + } + + /** + * Test: Authenticated user overrides GET parameter + */ + public function testAuthenticatedUserOverridesGetParameter(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $uri = new Uri('http://example.com/path?User=get_param_user'); + $serverRequest = new ServerRequest('POST', $uri); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockAuth->method('authenticate')->willReturn(true); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + $mockRegistry = $this->getMockSkipConstructor(\Horde_Registry::class); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'state' => $mockState, + ]); + + // Authenticate, should ignore GET parameter + $driver->authenticate('authenticated_user', 'password'); + + $this->assertEquals('authenticated_user', $driver->getUser()); + } + + /** + * Test: GET parameter overrides registry + */ + public function testGetParameterOverridesRegistry(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $uri = new Uri('http://example.com/path?User=get_param_user'); + $serverRequest = new ServerRequest('POST', $uri); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + $mockRegistry = $this->getMockSkipConstructor(\Horde_Registry::class); + + $mockRegistry->expects($this->never()) + ->method('getAuth'); + + $driver = new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'serverrequest' => $serverRequest, + 'registry' => $mockRegistry, + 'state' => $mockState, + ]); + + // No authentication, GET parameter present, should not call registry + $this->assertEquals('get_param_user', $driver->getUser()); + } + + /** + * Test: Constructor requires serverrequest + */ + public function testConstructorRequiresServerRequest(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Missing required PSR-7 ServerRequest object.'); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + $mockRegistry = $this->getMockSkipConstructor(\Horde_Registry::class); + + new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'registry' => $mockRegistry, + 'state' => $mockState, + // Missing serverrequest + ]); + } + + /** + * Test: Constructor requires registry + */ + public function testConstructorRequiresRegistry(): void + { + if (!class_exists('Horde_ActiveSync_State_Sql')) { + $this->markTestSkipped('horde/activesync not available'); + } + + $this->expectException(\InvalidArgumentException::class); + $this->expectExceptionMessage('Missing required Horde_Registry object.'); + + $serverRequest = new ServerRequest('POST', '/'); + + $mockConnector = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Connector::class); + $mockAuth = $this->getMockSkipConstructor(\Horde_Core_ActiveSync_Auth::class); + $mockState = $this->getMockSkipConstructor('Horde_ActiveSync_State_Sql'); + + new Horde_Core_ActiveSync_Driver([ + 'connector' => $mockConnector, + 'auth' => $mockAuth, + 'serverrequest' => $serverRequest, + 'state' => $mockState, + // Missing registry + ]); + } +} diff --git a/test/Unit/Factory/ActiveSyncBackendFactoryTest.php b/test/Unit/Factory/ActiveSyncBackendFactoryTest.php new file mode 100644 index 00000000..c9018d3d --- /dev/null +++ b/test/Unit/Factory/ActiveSyncBackendFactoryTest.php @@ -0,0 +1,183 @@ + + * @category Horde + * @license http://www.horde.org/licenses/lgpl21 LGPL + * @package Core + * @subpackage UnitTests + */ + +namespace Horde\Core\Test\Unit\Factory; + +use Horde\Test\TestCase; +use Horde\Http\ServerRequest; +use PHPUnit\Framework\Attributes\CoversClass; +use Horde_Core_Factory_ActiveSyncBackend; +use Horde_Injector; +use Horde_Registry; +use Horde_ActiveSync_State_Sql; +use Horde_Cache; +use Horde_Exception_NotFound; + +/** + * Unit tests for Horde_Core_Factory_ActiveSyncBackend + * + * @author Ralf Lang + * @category Horde + * @license http://www.horde.org/licenses/lgpl21 LGPL + * @package Core + * @subpackage UnitTests + */ +#[CoversClass(Horde_Core_Factory_ActiveSyncBackend::class)] +class ActiveSyncBackendFactoryTest extends TestCase +{ + /** + * Test that factory passes ServerRequest to driver + */ + public function testFactoryPassesServerRequestToDriver(): void + { + global $conf, $registry; + + // Setup minimal global config + $conf = [ + 'activesync' => [ + 'emailsync' => false, + 'ping' => [], + 'auth' => ['type' => 'basic'], + ] + ]; + + // Mock registry + $registry = $this->getMockSkipConstructor(Horde_Registry::class); + $registry->method('hasInterface')->willReturn(false); + + // Mock injector + $mockInjector = $this->getMockSkipConstructor(Horde_Injector::class); + + // Setup ServerRequest in injector + $serverRequest = new ServerRequest('POST', '/Microsoft-Server-ActiveSync'); + $mockInjector->method('get') + ->willReturnCallback(function ($class) use ($serverRequest) { + if ($class === ServerRequest::class) { + return $serverRequest; + } + if ($class === 'Horde_ActiveSyncState') { + return $this->getMockSkipConstructor(Horde_ActiveSync_State_Sql::class); + } + if ($class === 'Horde_Cache') { + return $this->getMockSkipConstructor(Horde_Cache::class); + } + throw new Horde_Exception_NotFound(); + }); + + $factory = new Horde_Core_Factory_ActiveSyncBackend($mockInjector); + $driver = $factory->create($mockInjector); + + $this->assertInstanceOf(\Horde_Core_ActiveSync_Driver::class, $driver); + + // Verify driver can access dependencies (getUser should work) + // Without authentication or GET params, should fall back to registry + $registry->expects($this->once()) + ->method('getAuth') + ->willReturn(''); + + $driver->getUser(); + } + + /** + * Test that factory creates ServerRequest from globals if not in injector + */ + public function testFactoryCreatesServerRequestFromGlobalsAsFallback(): void + { + global $conf, $registry; + + // Setup minimal global config + $conf = [ + 'activesync' => [ + 'emailsync' => false, + 'ping' => [], + 'auth' => ['type' => 'basic'], + ] + ]; + + // Mock registry + $registry = $this->getMockSkipConstructor(Horde_Registry::class); + $registry->method('hasInterface')->willReturn(false); + $registry->method('getAuth')->willReturn('test_user'); + + // Mock injector that doesn't have ServerRequest + $mockInjector = $this->getMockSkipConstructor(Horde_Injector::class); + + $mockInjector->method('get') + ->willReturnCallback(function ($class) { + if ($class === ServerRequest::class) { + throw new Horde_Exception_NotFound('Not found'); + } + if ($class === 'Horde_ActiveSyncState') { + return $this->getMockSkipConstructor(Horde_ActiveSync_State_Sql::class); + } + if ($class === 'Horde_Cache') { + return $this->getMockSkipConstructor(Horde_Cache::class); + } + throw new Horde_Exception_NotFound(); + }); + + $factory = new Horde_Core_Factory_ActiveSyncBackend($mockInjector); + $driver = $factory->create($mockInjector); + + $this->assertInstanceOf(\Horde_Core_ActiveSync_Driver::class, $driver); + } + + /** + * Test that factory passes registry to driver + */ + public function testFactoryPassesRegistryToDriver(): void + { + global $conf, $registry; + + $conf = [ + 'activesync' => [ + 'emailsync' => false, + 'ping' => [], + 'auth' => ['type' => 'basic'], + ] + ]; + + $registry = $this->getMockSkipConstructor(Horde_Registry::class); + $registry->method('hasInterface')->willReturn(false); + $registry->expects($this->once()) + ->method('getAuth') + ->willReturn('registry_user'); + + $mockInjector = $this->getMockSkipConstructor(Horde_Injector::class); + + $serverRequest = new ServerRequest('POST', '/'); + $mockInjector->method('get') + ->willReturnCallback(function ($class) use ($serverRequest) { + if ($class === ServerRequest::class) { + return $serverRequest; + } + if ($class === 'Horde_ActiveSyncState') { + return $this->getMockSkipConstructor(Horde_ActiveSync_State_Sql::class); + } + if ($class === 'Horde_Cache') { + return $this->getMockSkipConstructor(Horde_Cache::class); + } + throw new Horde_Exception_NotFound(); + }); + + $factory = new Horde_Core_Factory_ActiveSyncBackend($mockInjector); + $driver = $factory->create($mockInjector); + + // Verify registry is used + $this->assertEquals('registry_user', $driver->getUser()); + } +}