Merge pull request #14170 from nextcloud/feature/update-app-password-activity

Show more information on app password update

settings/Activity/Provider.php

@@ -42,8 +42,10 @@ class Provider implements IProvider {
 	public const EMAIL_CHANGED_SELF = 'email_changed_self';
 	public const EMAIL_CHANGED = 'email_changed';
 	public const APP_TOKEN_CREATED = 'app_token_created';
-	public const APP_TOKEN_UPDATED = 'app_token_updated';
 	public const APP_TOKEN_DELETED = 'app_token_deleted';
+	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';
 
 	/** @var IFactory */
 	protected $languageFactory;
@@ -110,10 +112,14 @@ class Provider implements IProvider {
 
 		} else if ($event->getSubject() === self::APP_TOKEN_CREATED) {
 			$subject = $this->l->t('You created app password "{token}"');
-		} else if ($event->getSubject() === self::APP_TOKEN_UPDATED) {
-			$subject = $this->l->t('You updated app password "{token}"');
 		} else if ($event->getSubject() === self::APP_TOKEN_DELETED) {
 			$subject = $this->l->t('You deleted app password "{token}"');
+		} else if ($event->getSubject() === self::APP_TOKEN_RENAMED) {
+			$subject = $this->l->t('You renamed app password "{token}" to "{newToken}"');
+		} else if ($event->getSubject() === self::APP_TOKEN_FILESYSTEM_GRANTED) {
+			$subject = $this->l->t('You granted filesystem access to app password "{token}"');
+		} else if ($event->getSubject() === self::APP_TOKEN_FILESYSTEM_REVOKED) {
+			$subject = $this->l->t('You revoked filesystem access from app password "{token}"');
 
 		} else {
 			throw new \InvalidArgumentException('Unknown subject');
@@ -146,13 +152,27 @@ class Provider implements IProvider {
 					'actor' => $this->generateUserParameter($parameters[0]),
 				];
 			case self::APP_TOKEN_CREATED:
-			case self::APP_TOKEN_UPDATED:
 			case self::APP_TOKEN_DELETED:
+			case self::APP_TOKEN_FILESYSTEM_GRANTED:
+			case self::APP_TOKEN_FILESYSTEM_REVOKED:
 				return [
 					'token' => [
 						'type' => 'highlight',
 						'id' => $event->getObjectId(),
-						'name' => $parameters[0],
+						'name' => $parameters['name'],
+					]
+				];
+			case self::APP_TOKEN_RENAMED:
+				return [
+					'token' => [
+						'type' => 'highlight',
+						'id' => $event->getObjectId(),
+						'name' => $parameters['name'],
+					],
+					'newToken' => [
+						'type' => 'highlight',
+						'id' => $event->getObjectId(),
+						'name' => $parameters['newName'],
 					]
 				];
 		}

settings/Controller/AuthSettingsController.php

@@ -158,7 +158,7 @@ class AuthSettingsController extends Controller {
 		$tokenData['canDelete'] = true;
 		$tokenData['canRename'] = true;
 
-		$this->publishActivity(Provider::APP_TOKEN_CREATED, $deviceToken->getId(), $deviceToken->getName());
+		$this->publishActivity(Provider::APP_TOKEN_CREATED, $deviceToken->getId(), ['name' => $deviceToken->getName()]);
 
 		return new JSONResponse([
 			'token' => $token,
@@ -206,7 +206,7 @@ class AuthSettingsController extends Controller {
 		}
 
 		$this->tokenProvider->invalidateTokenById($this->uid, $token->getId());
-		$this->publishActivity(Provider::APP_TOKEN_DELETED, $token->getId(), $token->getName());
+		$this->publishActivity(Provider::APP_TOKEN_DELETED, $token->getId(), ['name' => $token->getName()]);
 		return [];
 	}
 
@@ -226,32 +226,34 @@ class AuthSettingsController extends Controller {
 			return new JSONResponse([], Http::STATUS_NOT_FOUND);
 		}
 
-		$token->setScope([
-			'filesystem' => $scope['filesystem']
-		]);
+		$currentName = $token->getName();
 
+		if ($scope !== $token->getScopeAsArray()) {
+			$token->setScope(['filesystem' => $scope['filesystem']]);
+			$this->publishActivity($scope['filesystem'] ? Provider::APP_TOKEN_FILESYSTEM_GRANTED : Provider::APP_TOKEN_FILESYSTEM_REVOKED, $token->getId(), ['name' => $currentName]);
+		}
 
-		if ($token instanceof INamedToken) {
+		if ($token instanceof INamedToken && $name !== $currentName) {
 			$token->setName($name);
+			$this->publishActivity(Provider::APP_TOKEN_RENAMED, $token->getId(), ['name' => $currentName, 'newName' => $name]);
 		}
 
 		$this->tokenProvider->updateToken($token);
-		$this->publishActivity(Provider::APP_TOKEN_UPDATED, $token->getId(), $token->getName());
 		return [];
 	}
 
 	/**
 	 * @param string $subject
 	 * @param int $id
-	 * @param string|null $tokenName
+	 * @param array $parameters
 	 */
-	private function publishActivity(string $subject, int $id, ?string $tokenName = null): void {
+	private function publishActivity(string $subject, int $id, array $parameters = []): void {
 		$event = $this->activityManager->generateEvent();
 		$event->setApp('settings')
 			->setType('security')
 			->setAffectedUser($this->uid)
 			->setAuthor($this->uid)
-			->setSubject($subject, [$tokenName])
+			->setSubject($subject, $parameters)
 			->setObject('app_token', $id, 'App Password');
 
 		try {

tests/Settings/Controller/AuthSettingsControllerTest.php

@@ -239,8 +239,19 @@ class AuthSettingsControllerTest extends TestCase {
 		$this->assertSame(\OCP\AppFramework\Http::STATUS_NOT_FOUND, $response->getStatus());
 	}
 
+	public function dataRenameToken(): array {
+		return [
+			'App password => Other token name' => ['App password', 'Other token name'],
+			'Other token name => App password' => ['Other token name', 'App password'],
+		];
+	}
 
-	public function testUpdateToken() {
+	/**
+	 * @dataProvider dataRenameToken
+	 * @param string $name
+	 * @param string $newName
+	 */
+	public function testUpdateRename(string $name, string $newName): void {
 		$tokenId = 42;
 		$token = $this->createMock(DefaultToken::class);
 
@@ -251,11 +262,90 @@ class AuthSettingsControllerTest extends TestCase {
 			->method('getUID')
 			->willReturn('jane');
 
+		$token->expects($this->once())
+			->method('getName')
+			->willReturn($name);
+
+		$token->expects($this->once())
+			->method('getScopeAsArray')
+			->willReturn(['filesystem' => true]);
+
+		$token->expects($this->once())
+			->method('setName')
+			->with($this->equalTo($newName));
+
+		$this->tokenProvider->expects($this->once())
+			->method('updateToken')
+			->with($this->equalTo($token));
+
+		$this->assertSame([], $this->controller->update($tokenId, ['filesystem' => true], $newName));
+	}
+
+	public function dataUpdateFilesystemScope(): array {
+		return [
+			'Grant filesystem access' => [false, true],
+			'Revoke filesystem access' => [true, false],
+		];
+	}
+
+	/**
+	 * @dataProvider dataUpdateFilesystemScope
+	 * @param bool $filesystem
+	 * @param bool $newFilesystem
+	 */
+	public function testUpdateFilesystemScope(bool $filesystem, bool $newFilesystem): void {
+		$tokenId = 42;
+		$token = $this->createMock(DefaultToken::class);
+
+		$this->mockGetTokenById($tokenId, $token);
+		$this->mockActivityManager();
+
+		$token->expects($this->once())
+			->method('getUID')
+			->willReturn('jane');
+
+		$token->expects($this->once())
+			->method('getName')
+			->willReturn('App password');
+
+		$token->expects($this->once())
+			->method('getScopeAsArray')
+			->willReturn(['filesystem' => $filesystem]);
+
 		$token->expects($this->once())
 			->method('setScope')
-			->with($this->equalTo([
-				'filesystem' => true
-			]));
+			->with($this->equalTo(['filesystem' => $newFilesystem]));
+
+		$this->tokenProvider->expects($this->once())
+			->method('updateToken')
+			->with($this->equalTo($token));
+
+		$this->assertSame([], $this->controller->update($tokenId, ['filesystem' => $newFilesystem], 'App password'));
+	}
+
+	public function testUpdateNoChange(): void {
+		$tokenId = 42;
+		$token = $this->createMock(DefaultToken::class);
+
+		$this->mockGetTokenById($tokenId, $token);
+
+		$token->expects($this->once())
+			->method('getUID')
+			->willReturn('jane');
+
+		$token->expects($this->once())
+			->method('getName')
+			->willReturn('App password');
+
+		$token->expects($this->once())
+			->method('getScopeAsArray')
+			->willReturn(['filesystem' => true]);
+
+		$token->expects($this->never())
+			->method('setName');
+
+		$token->expects($this->never())
+			->method('setScope');
 
 		$this->tokenProvider->expects($this->once())
 			->method('updateToken')