Skip to content

test: cover EmailToken and TokenService signature method behaviors #8092

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
vitormattos merged 1 commit into from
Aug 28, 2026
+151 −5
Merged

test: cover EmailToken and TokenService signature method behaviors #8092

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
126 changes: 121 additions & 5 deletions tests/php/Unit/Service/IdentifyMethod/SignatureMethod/EmailTokenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,14 @@
namespace OCA\Libresign\Tests\Unit\Service\IdentifyMethod\SignatureMethod;

use OCA\Libresign\Db\IdentifyMethod;
use OCA\Libresign\Db\SignRequest;
use OCA\Libresign\Db\SignRequestMapper;
use OCA\Libresign\Exception\LibresignException;
use OCA\Libresign\Service\IdentifyMethod\IdentifyService;
use OCA\Libresign\Service\IdentifyMethod\SignatureMethod\EmailToken;
use OCA\Libresign\Service\IdentifyMethod\SignatureMethod\TokenService;
use OCP\L10N\IFactory as IL10NFactory;
use OCP\Security\IHasher;
use PHPUnit\Framework\Attributes\DataProvider;
use PHPUnit\Framework\MockObject\MockObject;

Expand All @@ -22,10 +26,9 @@ final class EmailTokenTest extends \OCA\Libresign\Tests\Unit\TestCase {

#[\Override]
public function setUp(): void {
$identifyService = $this->createMock(IdentifyService::class);
$identifyService = $this->getMockBuilder(IdentifyService::class)
->disableOriginalConstructor()
->onlyMethods(['getL10n'])
->onlyMethods(['getL10n', 'getSignRequestMapper', 'getHasher', 'save'])
->getMock();
$identifyService->method('getL10n')->willReturn(
\OCP\Server::get(IL10NFactory::class)->get(\OCA\Libresign\AppInfo\Application::APP_ID)
Expand All @@ -41,8 +44,8 @@ private function getClass(): EmailToken {
);
}

#[DataProvider('providerVaidateEmail')]
public function testVaidateEmail(string $email, string $blurred, string $hash): void {
#[DataProvider('providerValidateEmail')]
public function testValidateEmail(string $email, string $blurred, string $hash): void {
$instance = $this->getClass();
$identifyMethod = new IdentifyMethod();
$entity['identifierKey'] = 'email';
Expand All @@ -60,7 +63,7 @@ public function testVaidateEmail(string $email, string $blurred, string $hash):
$this->assertEquals($hash, $actual['hashOfEmail']);
}

public static function providerVaidateEmail(): array {
public static function providerValidateEmail(): array {
return [
['valid@domain.coop', 'val***@***.coop', md5('valid@domain.coop')],
['valiD@Domain.coop', 'val***@***.coop', md5('valid@domain.coop')],
Expand Down Expand Up @@ -154,4 +157,117 @@ public static function providerToArrayWithValidData(): array {
'case_48' => [['code' => '123456', 'identifiedAtDate' => '2025-08-11'], 'abc', ['needCode' => false, 'hasConfirmCode' => true]],
];
}

public function testRequestCodeSendsCodeAndPersistsHash(): void {
$signRequest = new SignRequest();
$signRequest->setDisplayName('John Doe');
$signRequestMapper = $this->createMock(SignRequestMapper::class);
$signRequestMapper->method('getById')
->with(171)
->willReturn($signRequest);
$this->identifyService->method('getSignRequestMapper')->willReturn($signRequestMapper);
$this->tokenService->expects($this->once())
->method('sendCodeByEmail')
->with('valid@domain.coop', 'John Doe')
->willReturn('hashed-code');

$instance = $this->getClass();
$identifyMethod = (new IdentifyMethod())->fromParams([
'identifierKey' => 'email',
'identifierValue' => 'valid@domain.coop',
'signRequestId' => 171,
]);
$instance->setEntity($identifyMethod);
$this->identifyService->expects($this->once())
->method('save')
->with($identifyMethod);

$instance->requestCode('valid@domain.coop', 'email');

$this->assertSame('hashed-code', $identifyMethod->getCode());
}

public function testRequestCodeOmitsDisplayNameWhenEqualToIdentifier(): void {
$signRequest = new SignRequest();
$signRequest->setDisplayName('valid@domain.coop');
$signRequestMapper = $this->createMock(SignRequestMapper::class);
$signRequestMapper->method('getById')
->with(171)
->willReturn($signRequest);
$this->identifyService->method('getSignRequestMapper')->willReturn($signRequestMapper);
$this->tokenService->expects($this->once())
->method('sendCodeByEmail')
->with('valid@domain.coop', '')
->willReturn('hashed-code');

$instance = $this->getClass();
$identifyMethod = (new IdentifyMethod())->fromParams([
'identifierKey' => 'email',
'identifierValue' => 'valid@domain.coop',
'signRequestId' => 171,
]);
$instance->setEntity($identifyMethod);

$instance->requestCode('valid@domain.coop', 'email');

$this->assertSame('hashed-code', $identifyMethod->getCode());
}

public function testValidateToSignWithValidCodeDoesNotThrow(): void {
$hasher = $this->createMock(IHasher::class);
$hasher->method('verify')
->with('123456', 'hashed-code')
->willReturn(true);
$this->identifyService->method('getHasher')->willReturn($hasher);

$instance = $this->getClass();
$identifyMethod = (new IdentifyMethod())->fromParams([
'identifierKey' => 'email',
'identifierValue' => 'valid@domain.coop',
'code' => 'hashed-code',
]);
$instance->setEntity($identifyMethod);
$instance->setCodeSentByUser('123456');

$instance->validateToSign();

$this->addToAssertionCount(1);
}

public function testValidateToSignWithWrongCodeThrows(): void {
$hasher = $this->createMock(IHasher::class);
$hasher->method('verify')
->with('654321', 'hashed-code')
->willReturn(false);
$this->identifyService->method('getHasher')->willReturn($hasher);

$instance = $this->getClass();
$identifyMethod = (new IdentifyMethod())->fromParams([
'identifierKey' => 'email',
'identifierValue' => 'valid@domain.coop',
'code' => 'hashed-code',
]);
$instance->setEntity($identifyMethod);
$instance->setCodeSentByUser('654321');

$this->expectException(LibresignException::class);
$this->expectExceptionMessage('Invalid code.');

@vitormattos vitormattos Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a good idea to avoid adding translatable or easily mutable text to test scenarios. If a non-developer needs to update the text, it becomes complicated because they would need to know to change it in two different places.

The LibresignException class could use an exception code instead, allowing the use of the expectExceptionCode method, but is necessary to check the best approach.

@maia-andre maia-andre Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same point applies here — answered in the TokenServiceTest thread above; the follow-up PR will cover both files.


$instance->validateToSign();
}

public function testValidateToSignThrowsWhenCodeWasSentWithoutBeingRequested(): void {
$instance = $this->getClass();
$identifyMethod = (new IdentifyMethod())->fromParams([
'identifierKey' => 'email',
'identifierValue' => 'valid@domain.coop',
]);
$instance->setEntity($identifyMethod);
$instance->setCodeSentByUser('123456');

$this->expectException(LibresignException::class);
$this->expectExceptionMessage('Invalid code.');

$instance->validateToSign();
}
}
30 changes: 30 additions & 0 deletions tests/php/Unit/Service/IdentifyMethod/SignatureMethod/TokenServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

namespace OCA\Libresign\Tests\Unit\Service\IdentifyMethod\SignatureMethod;

use OCA\Libresign\Exception\LibresignException;
use OCA\Libresign\Service\IdentifyMethod\SignatureMethod\TokenService;
use OCA\Libresign\Service\MailService;
use OCA\Libresign\Service\TwofactorGatewayService;
Expand Down Expand Up @@ -43,6 +44,19 @@ public function setUp(): void {
$this->logger = $this->createMock(LoggerInterface::class);
}

public function testSendCodeByGatewayThrowsWhenGatewayAppIsNotEnabled(): void {
$this->appManager->method('isEnabledForAnyone')->with('twofactor_gateway')->willReturn(false);
$this->container->expects($this->never())
->method('get');
$this->secureRandom->expects($this->never())
->method('generate');

$this->expectException(LibresignException::class);
$this->expectExceptionMessage('App Two-Factor Gateway is not enabled.');

@vitormattos vitormattos Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be a good idea to avoid adding translatable or easily mutable text to test scenarios. If a non-developer needs to update the text, it becomes complicated because they would need to know to change it in two different places.

The LibresignException class could use an exception code instead, allowing the use of the expectExceptionCode method, but is necessary to check the best approach.

@maia-andre maia-andre Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed — asserting on the message couples the test to text that can change for non-technical reasons, and in EmailTokenTest it is even more fragile because the string goes through the real L10N factory.

Two things I checked:

  • The mutants this PR kills do not depend on the message. In TokenServiceTest, if ensureAvailable() is removed the flow reaches isGatewayComplete() and throws OCSForbiddenException, which is not a LibresignException, so expectException(LibresignException::class) alone already distinguishes the mutant. Same for EmailTokenTest (hasher->verify() returning false + the exception class).
  • So the immediate fix is tests-only: drop the three expectExceptionMessage() calls and keep the exception class assertion. I'll open a follow-up PR against main with that and re-run Infection to confirm MSI stays at 100% (and backport it to stable35 after [stable35] test: cover EmailToken and TokenService signature method behaviors #8102).

For the long-term approach I see two directions, and I'd rather not pick one on my own since both touch production code:

  • exception codes on LibresignException + expectExceptionCode(). Today 47 of the 287 throw new LibresignException carry a code, mixing HTTP-like values (404/422/400/401) with 1, and the code is exposed through ErrorPayloadBuilder as code/error_code, so a convention would also touch the API contract;
  • typed subclasses (e.g. InvalidCodeException extends LibresignException) so tests assert the class instead of the text.

The suite currently has 171 expectExceptionMessage() calls across 51 files, so either direction is a broader effort. Would you like me to open an issue with this survey so it can be discussed and split into small PRs, like #8053?

@vitormattos vitormattos Aug 28, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Only a curiosity:

mixing HTTP-like values

At some places we already do this but to prevent mistakes, we added a more digit.

I found some developers considering that 4xx is a HTTP error, not another kind of error.

@vitormattos vitormattos Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As example:

libresign/lib/Helper/JSActions.php

Lines 12 to 22 in b33a20a

public const int ACTION_REDIRECT = 1000;
public const int ACTION_CREATE_ACCOUNT = 1500;
public const int ACTION_DO_NOTHING = 2000;
public const int ACTION_SIGN = 2500;
public const int ACTION_SIGN_INTERNAL = 2625;
public const int ACTION_SIGN_ID_DOC = 2750;
public const int ACTION_SHOW_ERROR = 3000;
public const int ACTION_SIGNED = 3500;
public const int ACTION_CREATE_SIGNATURE_PASSWORD = 4000;
public const int ACTION_RENEW_EMAIL = 4500;
public const int ACTION_INCOMPLETE_SETUP = 5000;

libresign/src/helpers/ActionMapping.ts

Lines 30 to 42 in b33a20a

export const ACTION_CODES: Readonly<ActionCodes> = Object.freeze({
REDIRECT: 1000,
CREATE_ACCOUNT: 1500,
DO_NOTHING: 2000,
SIGN: 2500,
SIGN_INTERNAL: 2625,
SIGN_ID_DOC: 2750,
SHOW_ERROR: 3000,
SIGNED: 3500,
CREATE_SIGNATURE_PASSWORD: 4000,
RENEW_EMAIL: 4500,
INCOMPLETE_SETUP: 5000,
})

@vitormattos vitormattos Aug 28, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you like me to open an issue with this survey so it can be discussed and split into small PRs, like #8053?

Nice idea, could you do this?

@maia-andre maia-andre Aug 28, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done: #8117 has the survey and a proposal to settle the convention (codes with constants and/or the existing typed exceptions) before splitting the work into small PRs.


$this->createService()->sendCodeByGateway('+5511999999999', 'sms');
}

public function testSendCodeByGatewayThrowsWhenGatewayIsIncomplete(): void {
$this->appManager->method('isEnabledForAnyone')->with('twofactor_gateway')->willReturn(true);
$this->container->method('get')
Expand Down Expand Up @@ -86,6 +100,22 @@ public function testSendCodeByGatewayUsesGatewayServiceAndReturnsHashedCode(): v
], $integrationService->sentMessages);
}

public function testSendCodeByEmailSendsCodeAndReturnsHashedCode(): void {
$this->secureRandom->expects($this->once())
->method('generate')
->with(TokenService::TOKEN_LENGTH, ISecureRandom::CHAR_DIGITS)
->willReturn('123456');
$this->mailService->expects($this->once())
->method('sendCodeToSign')
->with('signer@domain.coop', 'John Doe', '123456');
$this->hasher->expects($this->once())
->method('hash')
->with('123456')
->willReturn('hashed-code');

self::assertSame('hashed-code', $this->createService()->sendCodeByEmail('signer@domain.coop', 'John Doe'));
}

private function createService(): TokenService {
return new TokenService(
$this->secureRandom,
Expand Down
Loading