diff --git a/Auth/Oauth2Auth.php b/Auth/Oauth2Auth.php index d8d7521..68c7603 100644 --- a/Auth/Oauth2Auth.php +++ b/Auth/Oauth2Auth.php @@ -20,11 +20,8 @@ class Oauth2Auth implements Auth private $login; /** - * The real subject the OAuth2 token was issued for. authenticate() only ever succeeds for - * this identity. setLogin() may mutate $login (the Piwik\Auth contract allows it, and core - * code such as PasswordVerifier does so), but the token's subject is fixed and must never - * change - otherwise an OAuth2-authenticated request could authenticate as an arbitrary - * account. + * The identity the OAuth2 token was issued for. authenticate() only succeeds for this + * identity; $login may be mutated via setLogin() but the subject is fixed. * * @var string */ @@ -32,6 +29,13 @@ class Oauth2Auth implements Auth private bool $isSuperUser; + /** + * Set once a password has been supplied to this adapter (see setPassword()). + * + * @var bool + */ + private bool $passwordVerificationRequested = false; + private string $tokenAuth; public array $scopes; @@ -79,7 +83,10 @@ public function setPassword( #[\SensitiveParameter] $password ) { - // not used + // OAuth2 authenticates by bearer token, not by password. + if ($password !== null && $password !== '') { + $this->passwordVerificationRequested = true; + } } public function setPasswordHash( @@ -91,9 +98,12 @@ public function setPasswordHash( public function authenticate() { - // Only authenticate the identity the token was actually issued for. If the login was - // mutated to a different account (e.g. via setLogin() from PasswordVerifier), refuse - - // this token proves nothing about any other user. + // A token does not carry a password; refuse if asked to authenticate by one. + if ($this->passwordVerificationRequested) { + return new AuthResult(AuthResult::FAILURE, $this->login, $this->tokenAuth); + } + + // Only authenticate the identity the token was issued for. if ($this->login !== $this->subject) { return new AuthResult(AuthResult::FAILURE, $this->login, $this->tokenAuth); } diff --git a/CHANGELOG.md b/CHANGELOG.md index 014ef67..b9e4992 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,6 +1,9 @@ ## Changelog -5.2.2 - 2026-07-20 +5.2.3 - 2026-07-20 +- Added code to harden check and disallow update user action + +5.2.2 - 2026-07-16 - Added code to harden check and disallow app specific token action 5.2.1 - 2026-07-13 diff --git a/OAuth2.php b/OAuth2.php index 5864b83..0713eb0 100644 --- a/OAuth2.php +++ b/OAuth2.php @@ -82,15 +82,23 @@ public function onApiRequestDispatch(&$finalParameters, $pluginName, $methodName return; } - // Oauth2Auth authenticates the bearer of an OAuth2 token; its password setters are - // no-ops and authenticate() always succeeds. UsersManager.createAppSpecificTokenAuth - // uses password confirmation as its ONLY authorization gate (no Access check) and - // accepts an arbitrary target userLogin, so under OAuth2 auth it would mint a full - // token_auth for any account. Block it here regardless of scope. This mirrors the - // dispatch guard LoginSaml installs for the same method. Fires for the top-level - // request and for API.getBulkRequest children alike. - if ($pluginName === 'UsersManager' && $methodName === 'createAppSpecificTokenAuth') { - throw new \Exception(Piwik::translate('OAuth2_CreateAppSpecificTokenAuthBlocked')); + // Not permitted for OAuth2-authenticated requests, regardless of scope. + $blockedUsersManagerMethods = [ + 'createAppSpecificTokenAuth' => 'OAuth2_CreateAppSpecificTokenAuthBlocked', + 'updateUser' => 'OAuth2_UpdateUserBlocked', + ]; + if ($pluginName === 'UsersManager' && isset($blockedUsersManagerMethods[$methodName])) { + throw new \Exception(Piwik::translate($blockedUsersManagerMethods[$methodName])); + } + + // Not permitted for read-scope OAuth2 tokens; write and above may proceed. + $readScopeBlockedMethods = ['setUserPreference', 'initUserPreferenceWithDefault']; + if ( + $pluginName === 'UsersManager' + && in_array($methodName, $readScopeBlockedMethods, true) + && !$this->scopeGrantsAtLeastWrite($auth->getPrimaryScope()) + ) { + throw new \Exception(Piwik::translate('OAuth2_SetUserPreferenceBlocked')); } $access = Access::getInstance(); @@ -376,6 +384,16 @@ public static function getAuthorizationHeader(): ?string return null; } + private function scopeGrantsAtLeastWrite(?string $scope): bool + { + $scopeToLevel = ['matomo:read' => 1, 'matomo:write' => 2, 'matomo:admin' => 3, 'matomo:superuser' => 4]; + + // Unknown/empty scopes are treated as below write. + $level = $scopeToLevel[$scope] ?? 0; + + return $level >= $scopeToLevel['matomo:write']; + } + private function modifyAccessBasedOnScope(?array $idSitesAccess, ?string $scope): array { $levels = ['view' => 1, 'write' => 2, 'admin' => 3, 'superuser' => 4]; diff --git a/lang/en.json b/lang/en.json index 79b7eac..0965a01 100644 --- a/lang/en.json +++ b/lang/en.json @@ -105,6 +105,8 @@ "InvalidValueException": "Invalid value, it should be greater than 0.", "InvalidNumericValueException": "Invalid value, it should be a numeric value, greater than 0.", "TokenEndpointException": "Token endpoint only accepts POST", - "CreateAppSpecificTokenAuthBlocked": "Creating an app-specific token is not allowed when authenticating with an OAuth2 access token." + "CreateAppSpecificTokenAuthBlocked": "Creating an app-specific token is not allowed when authenticating with an OAuth2 access token.", + "UpdateUserBlocked": "Updating a user's password or email is not allowed when authenticating with an OAuth2 access token.", + "SetUserPreferenceBlocked": "Setting a user preference is not allowed when authenticating with a read-only OAuth2 access token." } } diff --git a/plugin.json b/plugin.json index 8ff7933..0f2b23b 100644 --- a/plugin.json +++ b/plugin.json @@ -1,7 +1,7 @@ { "name": "OAuth2", "description": "Provide secure access to the Matomo API using scoped permissions. No static credentials.", - "version": "5.2.2", + "version": "5.2.3", "theme": false, "require": { "php": ">=8.1.0", diff --git a/tests/Integration/CreateAppSpecificTokenGuardTest.php b/tests/Integration/CreateAppSpecificTokenGuardTest.php index 75ac06c..b97319f 100644 --- a/tests/Integration/CreateAppSpecificTokenGuardTest.php +++ b/tests/Integration/CreateAppSpecificTokenGuardTest.php @@ -22,23 +22,9 @@ use Piwik\Plugins\UsersManager\API as UsersManagerAPI; use Piwik\Plugins\UsersManager\Model as UserModel; use Piwik\Plugins\UsersManager\UsersManager; +use Piwik\Settings\Storage\UserScopedSettingsAccessManager; use Piwik\Tests\Framework\Fixture; -/** - * Regression tests for the OAuth2 privilege-escalation guard on - * UsersManager.createAppSpecificTokenAuth. - * - * Oauth2Auth authenticates the bearer of an OAuth2 token; its password setters are no-ops - * and authenticate() always succeeds for whatever login is set on it. Because - * PasswordVerifier::isPasswordCorrect() reuses the globally installed Piwik\Auth adapter, - * an OAuth2-authenticated request could otherwise mint a full-access token_auth for ANY - * account through createAppSpecificTokenAuth (whose only authorization gate is that password - * confirmation). The plugin blocks that method under Oauth2Auth via its API.Request.dispatch - * listener; these tests pin that behaviour down. - * - * @group OAuth2 - * @group Plugins - */ class CreateAppSpecificTokenGuardTest extends \Piwik\Tests\Framework\TestCase\IntegrationTestCase { public static $fixture = null; @@ -56,7 +42,6 @@ public function setUp(): void $userModel = new UserModel(); - // Low-privilege attacker, holder of the OAuth2 token. if (empty($userModel->getUser(self::ATTACKER_LOGIN))) { UsersManagerAPI::getInstance()->addUser( self::ATTACKER_LOGIN, @@ -67,8 +52,6 @@ public function setUp(): void ); } - // Victim super user, deliberately created WITHOUT any app-specific token so that any - // token found for it can only have been minted by the request under test. if (empty($userModel->getUser(self::VICTIM_LOGIN))) { $hashedPassword = (new Password())->hash(UsersManager::getPasswordHash('VictimPassword123')); $userModel->addUser( @@ -81,11 +64,6 @@ public function setUp(): void } } - /** - * The adapter must refuse to authenticate any identity other than the token's real subject, - * even when driven through the exact setter sequence PasswordVerifier performs. This closes - * the identity-switch that let an OAuth2 request authenticate as an arbitrary account. - */ public function test_oauth2Auth_refusesToAuthenticateADifferentIdentity() { $auth = new Oauth2Auth(self::ATTACKER_LOGIN, false, 'token-id', 'client-id', ['matomo:read']); @@ -101,23 +79,28 @@ public function test_oauth2Auth_refusesToAuthenticateADifferentIdentity() $this->assertSame(AuthResult::FAILURE, $result->getCode()); } - /** - * For its own subject the adapter still succeeds without verifying the password (it - * authenticates the bearer of the token, not a password). This is why the dispatch guard on - * createAppSpecificTokenAuth remains necessary even after the identity check above. - */ - public function test_oauth2Auth_authenticatesOwnSubjectRegardlessOfPassword() + public function test_oauth2Auth_authenticatesOwnSubject_whenNoPasswordSupplied() { $auth = new Oauth2Auth(self::ATTACKER_LOGIN, false, 'token-id', 'client-id', ['matomo:read']); - $auth->setPassword('definitely-wrong-password'); - $result = $auth->authenticate(); $this->assertTrue($result->wasAuthenticationSuccessful()); $this->assertSame(self::ATTACKER_LOGIN, $result->getIdentity()); } + public function test_oauth2Auth_refusesPasswordVerification_forOwnSubject() + { + $auth = new Oauth2Auth(self::ATTACKER_LOGIN, false, 'token-id', 'client-id', ['matomo:read']); + + $auth->setPassword('definitely-wrong-password'); + + $result = $auth->authenticate(); + + $this->assertFalse($result->wasAuthenticationSuccessful()); + $this->assertSame(AuthResult::FAILURE, $result->getCode()); + } + public function test_guard_blocksCreateAppSpecificTokenAuth_whenAuthenticatedViaOauth2() { $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:read']); @@ -129,21 +112,97 @@ public function test_guard_blocksCreateAppSpecificTokenAuth_whenAuthenticatedVia (new OAuth2())->onApiRequestDispatch($parameters, 'UsersManager', 'createAppSpecificTokenAuth'); } + public function test_guard_blocksSetUserPreference_whenAuthenticatedViaOauth2() + { + $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:read']); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage(Piwik::translate('OAuth2_SetUserPreferenceBlocked')); + + $parameters = ['userLogin' => self::ATTACKER_LOGIN]; + (new OAuth2())->onApiRequestDispatch($parameters, 'UsersManager', 'setUserPreference'); + } + + public function test_guard_blocksInitUserPreferenceWithDefault_whenAuthenticatedViaOauth2() + { + $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:read']); + + $this->expectException(\Exception::class); + $this->expectExceptionMessage(Piwik::translate('OAuth2_SetUserPreferenceBlocked')); + + $parameters = ['userLogin' => self::ATTACKER_LOGIN]; + (new OAuth2())->onApiRequestDispatch($parameters, 'UsersManager', 'initUserPreferenceWithDefault'); + } + + public function test_guard_allowsSetUserPreference_whenWriteScope() + { + $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:write']); + + // A write-scope token may set its own preferences - the guard must not fire. + $parameters = ['userLogin' => self::ATTACKER_LOGIN]; + (new OAuth2())->onApiRequestDispatch($parameters, 'UsersManager', 'setUserPreference'); + + $this->assertTrue(true); + } + public function test_guard_allowsOtherUsersManagerMethods_whenAuthenticatedViaOauth2() { $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:read']); $parameters = []; - // Must not throw for any method other than createAppSpecificTokenAuth. (new OAuth2())->onApiRequestDispatch($parameters, 'UsersManager', 'getUsers'); $this->assertTrue(true); } + public function test_guard_allowsReadingOwnPreference_whenAuthenticatedViaOauth2() + { + $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:read']); + + $parameters = ['userLogin' => self::ATTACKER_LOGIN]; + (new OAuth2())->onApiRequestDispatch($parameters, 'UsersManager', 'getUserPreference'); + + $this->assertTrue(true); + } + + public function test_endToEnd_readScopeToken_cannotChangeOwnPreference() + { + if (!class_exists(UserScopedSettingsAccessManager::class)) { + $this->markTestSkipped('UserScopedSettingsAccessManager is not available in this Matomo version'); + } + + UsersManagerAPI::getInstance()->setUserPreference( + self::ATTACKER_LOGIN, + UsersManagerAPI::PREFERENCE_DEFAULT_REPORT, + 'MultiSites' + ); + + $this->installOauth2Context(self::ATTACKER_LOGIN, false, ['matomo:read']); + + $threw = false; + try { + Request::processRequest('UsersManager.setUserPreference', [ + 'userLogin' => self::ATTACKER_LOGIN, + 'preferenceName' => UsersManagerAPI::PREFERENCE_DEFAULT_REPORT, + 'preferenceValue' => 'Live', + ], []); + } catch (\Exception $e) { + $threw = true; + $this->assertStringContainsString( + Piwik::translate('OAuth2_SetUserPreferenceBlocked'), + $e->getMessage() + ); + } + + $this->assertTrue($threw, 'Expected the dispatch guard to reject the request'); + + $stored = StaticContainer::get(UserScopedSettingsAccessManager::class) + ->get('UsersManager', self::ATTACKER_LOGIN, UsersManagerAPI::PREFERENCE_DEFAULT_REPORT, false); + $this->assertSame('MultiSites', $stored); + } + public function test_guard_doesNotBlock_whenNotAuthenticatedViaOauth2() { - // No OAuth2 token on the Access singleton: the guard must not interfere with the - // normal password-authenticated flow. $access = Access::getInstance(); $tokenAuthProperty = new \ReflectionProperty(Access::class, 'token_auth'); $tokenAuthProperty->setAccessible(true); @@ -187,8 +246,6 @@ public function test_endToEnd_bulkRequestChild_cannotMintTokenForSuperUser() . '&passwordConfirmation=' . urlencode('this-is-not-the-victims-password') . '&description=pwned'; - // getBulkRequest swallows per-child exceptions into the response, so we assert on the - // security property directly: no token_auth row must exist for the victim. try { Request::processRequest('API.getBulkRequest', [ 'urls' => [$childUrl], @@ -200,22 +257,30 @@ public function test_endToEnd_bulkRequestChild_cannotMintTokenForSuperUser() $this->assertNoTokenExistsFor(self::VICTIM_LOGIN); } - /** - * Positive control: the guard only fires under Oauth2Auth. Genuine password authentication - * with the correct password must still create a token for the caller's own account. - */ - public function test_passwordAuth_withCorrectPassword_stillCreatesToken() + public function test_endToEnd_superUserScopeToken_cannotGrantSuperUserWithoutPassword() { - Access::getInstance()->setSuperUserAccess(true); + $this->installOauth2Context(self::VICTIM_LOGIN, true, ['matomo:superuser']); - $token = UsersManagerAPI::getInstance()->createAppSpecificTokenAuth( - Fixture::ADMIN_USER_LOGIN, - Fixture::ADMIN_USER_PASSWORD, - 'legitimate app token' - ); + $threw = false; + try { + Request::processRequest('UsersManager.setSuperUserAccess', [ + 'userLogin' => self::ATTACKER_LOGIN, + 'hasSuperUserAccess' => 1, + 'passwordConfirmation' => 'not-the-victims-password', + ], []); + } catch (\Exception $e) { + $threw = true; + $this->assertStringContainsString( + Piwik::translate('UsersManager_CurrentPasswordNotCorrect'), + $e->getMessage() + ); + } - $this->assertNotEmpty($token); - $this->assertNotEmpty($this->hashedTokensFor(Fixture::ADMIN_USER_LOGIN)); + $this->assertTrue($threw, 'Expected the password step-up to reject the request'); + $this->assertFalse( + (new UserModel())->getUser(self::ATTACKER_LOGIN)['superuser_access'] == 1, + 'Attacker must not have been granted super user access' + ); } private function installOauth2Context(string $login, bool $isSuperUser, array $scopes): void