Merge pull request #60745 from nextcloud/backport/60644/stable34

[stable34] fix(settings): confirm app-token revoke and preserve wipe state

Author: artonge

apps/oauth2/lib/Controller/SettingsController.php

@@ -8,20 +8,23 @@
  */
 namespace OCA\OAuth2\Controller;
 
+use OC\Authentication\Token\IProvider as IAuthTokenProvider;
 use OCA\OAuth2\Db\AccessTokenMapper;
 use OCA\OAuth2\Db\Client;
 use OCA\OAuth2\Db\ClientMapper;
 use OCP\AppFramework\Controller;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\Attribute\PasswordConfirmationRequired;
 use OCP\AppFramework\Http\JSONResponse;
-use OCP\Authentication\Token\IProvider as IAuthTokenProvider;
+use OCP\Authentication\Exceptions\InvalidTokenException;
+use OCP\Authentication\Exceptions\WipeTokenException;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserManager;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
+use Psr\Log\LoggerInterface;
 
 class SettingsController extends Controller {
 
@@ -37,6 +40,7 @@ public function __construct(
 		private IAuthTokenProvider $tokenProvider,
 		private IUserManager $userManager,
 		private ICrypto $crypto,
+		private LoggerInterface $logger,
 	) {
 		parent::__construct($appName, $request);
 	}
@@ -73,7 +77,26 @@ public function deleteClient(int $id): JSONResponse {
 		$client = $this->clientMapper->getByUid($id);
 
 		$this->userManager->callForSeenUsers(function (IUser $user) use ($client): void {
-			$this->tokenProvider->invalidateTokensOfUser($user->getUID(), $client->getName());
+			// Skip tokens that are marked for remote wipe so revoking the
+			// OAuth2 client does not silently cancel a pending wipe.
+			$tokens = $this->tokenProvider->getTokenByUser($user->getUID());
+			foreach ($tokens as $token) {
+				if ($token->getName() !== $client->getName()) {
+					continue;
+				}
+				try {
+					$this->tokenProvider->getTokenById($token->getId());
+				} catch (WipeTokenException $e) {
+					$this->logger->info('Preserving token {tokenId} of user {uid}: marked for remote wipe, OAuth2 client revoke would cancel the wipe.', [
+						'tokenId' => $token->getId(),
+						'uid' => $user->getUID(),
+					]);
+					continue;
+				} catch (InvalidTokenException $e) {
+					// Token already invalid; let invalidateTokenById handle it.
+				}
+				$this->tokenProvider->invalidateTokenById($user->getUID(), $token->getId());
+			}
 		});
 
 		$this->accessTokenMapper->deleteByClientId($id);

apps/oauth2/tests/Controller/SettingsControllerTest.php

@@ -6,20 +6,24 @@
  */
 namespace OCA\OAuth2\Tests\Controller;
 
+use OC\Authentication\Token\IProvider as IAuthTokenProvider;
 use OCA\OAuth2\Controller\SettingsController;
 use OCA\OAuth2\Db\AccessTokenMapper;
 use OCA\OAuth2\Db\Client;
 use OCA\OAuth2\Db\ClientMapper;
 use OCP\AppFramework\Http;
 use OCP\AppFramework\Http\JSONResponse;
-use OCP\Authentication\Token\IProvider as IAuthTokenProvider;
+use OCP\Authentication\Exceptions\WipeTokenException;
+use OCP\Authentication\Token\IToken;
 use OCP\IL10N;
 use OCP\IRequest;
 use OCP\IUser;
 use OCP\IUserManager;
 use OCP\Security\ICrypto;
 use OCP\Security\ISecureRandom;
 use OCP\Server;
+use PHPUnit\Framework\MockObject\MockObject;
+use Psr\Log\LoggerInterface;
 use Test\TestCase;
 
 #[\PHPUnit\Framework\Attributes\Group(name: 'DB')]
@@ -42,6 +46,7 @@ class SettingsControllerTest extends TestCase {
 	private $l;
 	/** @var ICrypto|\PHPUnit\Framework\MockObject\MockObject */
 	private $crypto;
+	private LoggerInterface&MockObject $logger;
 
 	protected function setUp(): void {
 		parent::setUp();
@@ -53,6 +58,7 @@ protected function setUp(): void {
 		$this->authTokenProvider = $this->createMock(IAuthTokenProvider::class);
 		$this->userManager = $this->createMock(IUserManager::class);
 		$this->crypto = $this->createMock(ICrypto::class);
+		$this->logger = $this->createMock(LoggerInterface::class);
 		$this->l = $this->createMock(IL10N::class);
 		$this->l->method('t')
 			->willReturnArgument(0);
@@ -65,7 +71,8 @@ protected function setUp(): void {
 			$this->l,
 			$this->authTokenProvider,
 			$this->userManager,
-			$this->crypto
+			$this->crypto,
+			$this->logger,
 		);
 
 	}
@@ -132,11 +139,15 @@ public function testDeleteClient(): void {
 		$user1->updateLastLoginTimestamp();
 		$tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock();
 
-		// expect one call per user and ensure the correct client name
+		// One getTokenByUser call per user; we return no matching tokens here
+		// so invalidateTokenById is never invoked.
 		$tokenProviderMock
 			->expects($this->exactly($count + 1))
-			->method('invalidateTokensOfUser')
-			->with($this->isType('string'), 'My Client Name');
+			->method('getTokenByUser')
+			->willReturn([]);
+		$tokenProviderMock
+			->expects($this->never())
+			->method('invalidateTokenById');
 
 		$client = new Client();
 		$client->setId(123);
@@ -167,7 +178,8 @@ public function testDeleteClient(): void {
 			$this->l,
 			$tokenProviderMock,
 			$userManager,
-			$this->crypto
+			$this->crypto,
+			$this->logger,
 		);
 
 		$result = $settingsController->deleteClient(123);
@@ -177,6 +189,96 @@ public function testDeleteClient(): void {
 		$user1->delete();
 	}
 
+	public function testDeleteClientPreservesWipePendingToken(): void {
+		$userManager = Server::get(IUserManager::class);
+		$user = $userManager->createUser('test_wipe_preserve', 'test_wipe_preserve');
+		$user->updateLastLoginTimestamp();
+
+		$client = new Client();
+		$client->setId(456);
+		$client->setName('My Client Name');
+		$client->setRedirectUri('https://example.com/');
+		$client->setSecret(bin2hex('MyHashedSecret'));
+		$client->setClientIdentifier('MyClientIdentifier');
+
+		// Token marked for wipe with a matching client name: must NOT be invalidated.
+		$wipeToken = $this->createMock(IToken::class);
+		$wipeToken->method('getId')->willReturn(11);
+		$wipeToken->method('getName')->willReturn('My Client Name');
+
+		// Regular token with matching name: must be invalidated.
+		$regularToken = $this->createMock(IToken::class);
+		$regularToken->method('getId')->willReturn(12);
+		$regularToken->method('getName')->willReturn('My Client Name');
+
+		// Non-matching name: must be left alone.
+		$otherToken = $this->createMock(IToken::class);
+		$otherToken->method('getId')->willReturn(13);
+		$otherToken->method('getName')->willReturn('Some Other Client');
+
+		$tokenProviderMock = $this->getMockBuilder(IAuthTokenProvider::class)->getMock();
+		$tokenProviderMock
+			->method('getTokenByUser')
+			->willReturnCallback(function (string $uid) use ($wipeToken, $regularToken, $otherToken) {
+				return $uid === 'test_wipe_preserve'
+					? [$wipeToken, $regularToken, $otherToken]
+					: [];
+			});
+		// Wipe state is signalled via WipeTokenException from getTokenById.
+		$tokenProviderMock
+			->method('getTokenById')
+			->willReturnCallback(function (int $id) use ($wipeToken, $regularToken) {
+				if ($id === 11) {
+					throw new WipeTokenException($wipeToken);
+				}
+				return $regularToken;
+			});
+		$tokenProviderMock
+			->expects($this->once())
+			->method('invalidateTokenById')
+			->with('test_wipe_preserve', 12);
+
+		$this->clientMapper
+			->method('getByUid')
+			->with(456)
+			->willReturn($client);
+		$this->accessTokenMapper
+			->expects($this->once())
+			->method('deleteByClientId')
+			->with(456);
+		$this->clientMapper
+			->expects($this->once())
+			->method('delete')
+			->with($client);
+
+		$logger = $this->createMock(LoggerInterface::class);
+		$logger->expects($this->atLeastOnce())
+			->method('info')
+			->with($this->stringContains('Preserving token'), $this->callback(function (array $context) {
+				return ($context['tokenId'] ?? null) === 11
+					&& ($context['uid'] ?? null) === 'test_wipe_preserve';
+			}));
+
+		$settingsController = new SettingsController(
+			'oauth2',
+			$this->request,
+			$this->clientMapper,
+			$this->secureRandom,
+			$this->accessTokenMapper,
+			$this->l,
+			$tokenProviderMock,
+			$userManager,
+			$this->crypto,
+			$logger,
+		);
+
+		$result = $settingsController->deleteClient(456);
+		$this->assertInstanceOf(JSONResponse::class, $result);
+		$this->assertEquals([], $result->getData());
+
+		$user->delete();
+	}
+
 	public function testInvalidRedirectUri(): void {
 		$result = $this->settingsController->addClient('test', 'invalidurl');
 

apps/settings/lib/Activity/Provider.php

@@ -27,6 +27,7 @@ class Provider implements IProvider {
 	public const EMAIL_CHANGED = 'email_changed';
 	public const APP_TOKEN_CREATED = 'app_token_created';
 	public const APP_TOKEN_DELETED = 'app_token_deleted';
+	public const APP_TOKEN_DELETED_WIPE_CANCELLED = 'app_token_deleted_wipe_cancelled';
 	public const APP_TOKEN_RENAMED = 'app_token_renamed';
 	public const APP_TOKEN_FILESYSTEM_GRANTED = 'app_token_filesystem_granted';
 	public const APP_TOKEN_FILESYSTEM_REVOKED = 'app_token_filesystem_revoked';
@@ -86,6 +87,8 @@ public function parse($language, IEvent $event, ?IEvent $previousEvent = null):
 			}
 		} elseif ($event->getSubject() === self::APP_TOKEN_DELETED) {
 			$subject = $this->l->t('You deleted app password "{token}"');
+		} elseif ($event->getSubject() === self::APP_TOKEN_DELETED_WIPE_CANCELLED) {
+			$subject = $this->l->t('You deleted app password "{token}" and cancelled its pending remote wipe');
 		} elseif ($event->getSubject() === self::APP_TOKEN_RENAMED) {
 			$subject = $this->l->t('You renamed app password "{token}" to "{newToken}"');
 		} elseif ($event->getSubject() === self::APP_TOKEN_FILESYSTEM_GRANTED) {
@@ -125,6 +128,7 @@ protected function getParameters(IEvent $event): array {
 				];
 			case self::APP_TOKEN_CREATED:
 			case self::APP_TOKEN_DELETED:
+			case self::APP_TOKEN_DELETED_WIPE_CANCELLED:
 			case self::APP_TOKEN_FILESYSTEM_GRANTED:
 			case self::APP_TOKEN_FILESYSTEM_REVOKED:
 				return [

apps/settings/lib/Controller/AuthSettingsController.php

@@ -179,17 +179,21 @@ public function destroy($id) {
 			return new JSONResponse([], Http::STATUS_BAD_REQUEST);
 		}
 
+		$subject = Provider::APP_TOKEN_DELETED;
 		try {
 			$token = $this->findTokenByIdAndUser($id);
 		} catch (WipeTokenException $e) {
-			//continue as we can destroy tokens in wipe
+			// Deleting a wipe-pending token cancels the pending wipe; the device
+			// may already be uninstalled so we allow it, but record it under a
+			// distinct subject so the audit trail captures the consequence.
 			$token = $e->getToken();
+			$subject = Provider::APP_TOKEN_DELETED_WIPE_CANCELLED;
 		} catch (InvalidTokenException $e) {
 			return new JSONResponse([], Http::STATUS_NOT_FOUND);
 		}
 
 		$this->tokenProvider->invalidateTokenById($this->userId, $token->getId());
-		$this->publishActivity(Provider::APP_TOKEN_DELETED, $token->getId(), ['name' => $token->getName()]);
+		$this->publishActivity($subject, $token->getId(), ['name' => $token->getName()]);
 		return [];
 	}