Skip to content
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

Jump to bottom

Add input validation for Activation Code and Document Number #415

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Add input validation for Activation Code and Document Number #415

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
e84e47f
Perform input validation on activation codes and document numbers
tvdijen Jan 4, 2024
aff9b0f
Test document number in a more meaningful way
tvdijen Jan 4, 2024
afe4604
Replace non-standard character for unknown document with null-case
tvdijen Jan 4, 2024
d470e6f
Fix secondary tests
tvdijen Jan 4, 2024
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
10 changes: 6 additions & 4 deletions src/Surfnet/Stepup/Identity/Entity/VerifiedSecondFactor.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,6 +35,8 @@
use Surfnet\StepupBundle\Value\SecondFactorType;
use Surfnet\StepupBundle\Value\VettingType as StepupVettingType;

use function preg_match;

/**
* A second factor whose possession has been proven by the registrant and the registrant's e-mail address has been
* verified. The registrant must visit a registration authority next.
Expand Down Expand Up @@ -89,10 +91,10 @@ public static function create(
SecondFactorType $type,
SecondFactorIdentifier $secondFactorIdentifier,
DateTime $registrationRequestedAt,
$registrationCode
string $registrationCode
) {
if (!is_string($registrationCode)) {
throw InvalidArgumentException::invalidType('string', 'registrationCode', $registrationCode);
if (!preg_match('/^[A-Z0-9]{8}$/i', $registrationCode)) {
Copy link
Member

Choose a reason for hiding this comment

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

I love to have a more specific input validation here. It might be a good idea to introduce a registration code value object that is tasked with validating the code? That's more in line with the way the domain is set up. For now we kept it primitive because we did not realy bother with elaborate validation.

Copy link
Contributor Author

@tvdijen tvdijen Jan 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

If you want to go the extra mile, then please be my guest. I'm not going to solve the world's problems here, just the ones reported by our auditor ;)
Also not sure what you mean with 'more specific'

Copy link
Member

Choose a reason for hiding this comment

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

In this case I think it's just a first guard on "sane" input as a defense-in-depth: does this even look like a registration code? The actual validity of the given code will be verified later obviously. So I can imagine that it's just fine to do the basic check that Tim implemented.

throw InvalidArgumentException::invalidType('valid characters', 'registrationCode', $registrationCode);
}

$secondFactor = new self;
Expand Down Expand Up @@ -123,7 +125,7 @@ public function getId()
* @param SecondFactorIdentifier $secondFactorIdentifier
* @return bool
*/
public function hasRegistrationCodeAndIdentifier($registrationCode, SecondFactorIdentifier $secondFactorIdentifier)
public function hasRegistrationCodeAndIdentifier(string $registrationCode, SecondFactorIdentifier $secondFactorIdentifier)
{
return strcasecmp($registrationCode, $this->registrationCode) === 0
&& $secondFactorIdentifier->equals($this->secondFactorIdentifier);
Expand Down
27 changes: 17 additions & 10 deletions src/Surfnet/Stepup/Identity/Value/DocumentNumber.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,47 +21,54 @@
use JsonSerializable;
use Surfnet\Stepup\Exception\InvalidArgumentException;

use function preg_match;
use function strval;

final class DocumentNumber implements JsonSerializable
{
/**
* @var string
* @var string|null
*/
private $documentNumber;

/**
* @return self
*/
public static function unknown()
public static function unknown(): self
{
return new self('—');
return new self(null);
Copy link
Member

Choose a reason for hiding this comment

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

Does this not cause problems when an identitie is forgotten (right to be forgotten) and the entry pops up in the audit log? The value that is set for an unknown document number is displayed then and there in the audit log.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is really beyond my understanding... If you can get the regexp to work with this 'funny' character, we can leave this 'as-is'...

}

/**
* @param string $documentNumber
* @param string|null $documentNumber
*/
public function __construct($documentNumber)
public function __construct(?string $documentNumber)
{
if (!is_string($documentNumber) || empty($documentNumber)) {
if ($documentNumber === null) {
// Created using the static ::unknown method
Copy link
Member

Choose a reason for hiding this comment

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

Some work remains to be done here, also see my previous comment about a forgotten Identity.

Copy link
Member

Choose a reason for hiding this comment

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

This is a defense in depth measure. Since we do not know what the format is of the document numbers – these are whatever the issuer decided to put on the document – retroactively setting a limit in the middleware in this way can cause issues when there are existing document numbers in the event stream that do not validate anymore. Limiting what is allowed in is fine, but this should be done on entry. We could do this in the middleware, but as a defense in depth it is best placed in the RA when the form post is received, not in the middleware.

SInce we do generate the activation codes, we know exactly what they look like so we could validate them in the middleware without additional risk.

Copy link
Contributor Author

@tvdijen tvdijen Feb 14, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I did that at first, but Peter didn't want it there :-/

} elseif (empty($documentNumber)) {
throw InvalidArgumentException::invalidType('non-empty string', 'documentNumber', $documentNumber);
} elseif (!preg_match('/^([-]|[A-Z0-9-]{6})$/i', $documentNumber)) {
throw InvalidArgumentException::invalidType('valid characters', 'documentNumber', $documentNumber);
}

$this->documentNumber = $documentNumber;
}

/**
* @return string
* @return string|null
*/
public function getDocumentNumber()
public function getDocumentNumber(): ?string
{
return $this->documentNumber;
}

public function __toString()
{
return $this->documentNumber;
return strval($this->documentNumber);
}

public function equals(DocumentNumber $other)
public function equals(DocumentNumber $other): bool
{
return $this->documentNumber === $other->documentNumber;
}
Expand Down
16 changes: 8 additions & 8 deletions src/Surfnet/Stepup/Tests/Identity/Event/EventSerializationAndDeserializationTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -551,7 +551,7 @@ public function serializedDataProvider(){
// Tests for changes in BC support for adding the VettingType in the SecondFactorVettedEvents in favour of the 'DocumentNumber'
'SecondFactorVettedEvent:support-new-event-with-vetting-type' => [
'{"identity_id":"b260f10b-ce7c-4d09-b6a4-50a3923d637f","name_id":"urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1","identity_institution":"institution-d.example.com","second_factor_id":"512de1ff-0ae0-41b7-bb21-b71d77e570b8","second_factor_type":"yubikey","preferred_locale":"nl_NL"}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","vetting_type":{"type":"on-premise","document_number":"012345678"}}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","vetting_type":{"type":"on-premise","document_number":"AB-123"}}',
new SecondFactorVettedEvent(
new IdentityId('b260f10b-ce7c-4d09-b6a4-50a3923d637f'),
new NameId('urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1'),
Expand All @@ -562,12 +562,12 @@ public function serializedDataProvider(){
new CommonName('jane-d1 Institution-D.EXAMPLE.COM'),
new Email('[email protected]'),
new Locale('nl_NL'),
new OnPremiseVettingType(new DocumentNumber('012345678'))
new OnPremiseVettingType(new DocumentNumber('AB-123'))
),
],
'SecondFactorVettedEvent:support-old-event-with-document-number' => [
'{"identity_id":"b260f10b-ce7c-4d09-b6a4-50a3923d637f","name_id":"urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1","identity_institution":"institution-d.example.com","second_factor_id":"512de1ff-0ae0-41b7-bb21-b71d77e570b8","second_factor_type":"yubikey","preferred_locale":"nl_NL"}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","document_number":"012345678"}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","document_number":"AB-123"}',
new SecondFactorVettedEvent(
new IdentityId('b260f10b-ce7c-4d09-b6a4-50a3923d637f'),
new NameId('urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1'),
Expand All @@ -578,12 +578,12 @@ public function serializedDataProvider(){
new CommonName('jane-d1 Institution-D.EXAMPLE.COM'),
new Email('[email protected]'),
new Locale('nl_NL'),
new OnPremiseVettingType(new DocumentNumber('012345678'))
new OnPremiseVettingType(new DocumentNumber('AB-123'))
),
],
'SecondFactorVettedWithoutTokenProofOfPossession:support-new-event-with-vetting-type' => [
'{"identity_id":"b260f10b-ce7c-4d09-b6a4-50a3923d637f","name_id":"urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1","identity_institution":"institution-d.example.com","second_factor_id":"512de1ff-0ae0-41b7-bb21-b71d77e570b8","second_factor_type":"yubikey","preferred_locale":"nl_NL"}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","vetting_type":{"type":"on-premise","document_number":"012345678"}}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","vetting_type":{"type":"on-premise","document_number":"AB-123"}}',
new SecondFactorVettedWithoutTokenProofOfPossession(
new IdentityId('b260f10b-ce7c-4d09-b6a4-50a3923d637f'),
new NameId('urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1'),
Expand All @@ -594,12 +594,12 @@ public function serializedDataProvider(){
new CommonName('jane-d1 Institution-D.EXAMPLE.COM'),
new Email('[email protected]'),
new Locale('nl_NL'),
new OnPremiseVettingType(new DocumentNumber('012345678'))
new OnPremiseVettingType(new DocumentNumber('AB-123'))
),
],
'SecondFactorVettedWithoutTokenProofOfPossession:support-old-event-with-document-number' => [
'{"identity_id":"b260f10b-ce7c-4d09-b6a4-50a3923d637f","name_id":"urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1","identity_institution":"institution-d.example.com","second_factor_id":"512de1ff-0ae0-41b7-bb21-b71d77e570b8","second_factor_type":"yubikey","preferred_locale":"nl_NL"}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","document_number":"012345678"}',
'{"common_name":"jane-d1 Institution-D.EXAMPLE.COM","email":"[email protected]","second_factor_type":"yubikey","second_factor_identifier":"123465293846985","document_number":"AB-123"}',
new SecondFactorVettedWithoutTokenProofOfPossession(
new IdentityId('b260f10b-ce7c-4d09-b6a4-50a3923d637f'),
new NameId('urn:collab:person:Institution-D.EXAMPLE.COM:jane-d1'),
Expand All @@ -610,7 +610,7 @@ public function serializedDataProvider(){
new CommonName('jane-d1 Institution-D.EXAMPLE.COM'),
new Email('[email protected]'),
new Locale('nl_NL'),
new OnPremiseVettingType(new DocumentNumber('012345678'))
new OnPremiseVettingType(new DocumentNumber('AB-123'))
),
],
];
Expand Down
73 changes: 58 additions & 15 deletions src/Surfnet/Stepup/Tests/Identity/Value/DocumentNumberTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,50 +19,93 @@
namespace Surfnet\Stepup\Tests\Identity\Value;

use PHPUnit\Framework\TestCase as UnitTest;
use Surfnet\Stepup\Exception\InvalidArgumentException;
use Surfnet\Stepup\Identity\Value\DocumentNumber;

class DocumentNumberTest extends UnitTest
{
/**
* @test
* @group domain
* @dataProvider invalidArgumentProvider
* @dataProvider validDocumentNumberProvider
*
* @param mixed $invalidValue
* @param string|null $documentNumber
*/
public function the_document_number_must_be_a_non_empty_string($invalidValue)
public function the_document_number_must_be_valid(?string $documentNumber): void
{
$this->expectException(\Surfnet\Stepup\Exception\InvalidArgumentException::class);
$document = new DocumentNumber($documentNumber);
$this->assertInstanceOf(DocumentNumber::class, $document);
}


/**
* @test
* @group domain
* @dataProvider invalidDocumentNumberProvider
*
* @param string $invalidValue
*/
public function the_document_number_must_not_contain_illegal_characters(string $invalidValue): void
{
$this->expectException(InvalidArgumentException::class);
new DocumentNumber($invalidValue);
}


/**
* @test
* @group domain
*/
public function the_document_number_must_be_a_non_empty_string(): void
{
$this->expectException(InvalidArgumentException::class);
new DocumentNumber('');
}


/**
* @test
* @group domain
*/
public function two_document_numbers_with_the_same_value_are_equal()
public function two_document_numbers_with_the_same_value_are_equal(): void
{
$commonName = new DocumentNumber('John Doe');
$theSame = new DocumentNumber('John Doe');
$different = new DocumentNumber('Jane Doe');
$commonName = new DocumentNumber('JHA1B4');
$theSame = new DocumentNumber('JHA1B4');
$different = new DocumentNumber('IGZ0A3');
$unknown = DocumentNumber::unknown();

$this->assertTrue($commonName->equals($theSame));
$this->assertFalse($commonName->equals($different));
$this->assertFalse($commonName->equals($unknown));
}


/**
* provider for {@see the_document_number_address_must_not_contain_illegal_characters()}
*/
public function invalidDocumentNumberProvider(): array
{
return [
'Illegal character' => ['#12345'],
'Too long' => ['TooLong'],
'Too short' => ['Shor'], // Short
'Contains space' => ['AB 123'],
];
}


/**
* provider for {@see the_document_number_address_must_be_a_non_empty_string()}
* provider for {@see the_document_number_address_must_be_valid()}
*/
public function invalidArgumentProvider()
public function validDocumentNumberProvider(): array
{
return [
'empty string' => [''],
'array' => [[]],
'integer' => [1],
'float' => [1.2],
'object' => [new \StdClass()],
'Single hyphen' => ['-'],
'Contains hyphen' => ['123-45'],
'Unknown document' => [null],
'Uppercase' => ['A1B2C3'],
'Lowercase' => ['a2b2c3'],
'Mixed case' => ['a2B2c3'],
];
}
}
6 changes: 3 additions & 3 deletions src/Surfnet/StepupMiddleware/ApiBundle/Tests/Doctrine/Type/DocumentNumberTypeTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,11 +65,11 @@ public function a_normal_document_number_is_converted_to_a_database_value()
{
$type = Type::getType(DocumentNumberType::NAME);

$input = new DocumentNumber('a');
$input = new DocumentNumber('A1B2C3');
$output = $type->convertToDatabaseValue($input, $this->platform);

$this->assertTrue(is_string($output));
$this->assertEquals('a', $output);
$this->assertEquals('A1B2C3', $output);
}

/**
Expand Down Expand Up @@ -108,7 +108,7 @@ public function a_non_null_value_is_converted_to_the_stepup_document_number_obje
{
$type = Type::getType(DocumentNumberType::NAME);

$input = '12345';
$input = 'A12345';
$output = $type->convertToPHPValue($input, $this->platform);

$this->assertInstanceOf('Surfnet\Stepup\Identity\Value\DocumentNumber', $output);
Expand Down
4 changes: 2 additions & 2 deletions ...mmandHandlingBundle/Tests/Identity/CommandHandler/IdentityCommandHandlerMoveTokenTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -461,7 +461,7 @@ public function test_can_not_be_moved_if_already_present_as_vetted_token()
new SecondFactorType('yubikey'),
$targetYubikeySecFacId,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$targetRegistrantCommonName,
$targetRegistrantEmail,
new Locale('en_GB')
Expand Down Expand Up @@ -569,7 +569,7 @@ public function test_can_not_be_moved_if_already_present_as_verified_token()
new SecondFactorType('yubikey'),
$targetYubikeySecFacId,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$targetRegistrantCommonName,
$targetRegistrantEmail,
new Locale('en_GB')
Expand Down
18 changes: 9 additions & 9 deletions ...lingBundle/Tests/Identity/CommandHandler/IdentityCommandHandlerSelfAssertedTokensTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -561,7 +561,7 @@ public function test_a_token_can_be_registered_self_asserted()
new SecondFactorType('yubikey'),
$yubikeyPublicId,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down Expand Up @@ -644,7 +644,7 @@ public function test_self_asserted_token_registration_requires_possession_of_rec
new SecondFactorType('yubikey'),
$yubikeyPublicId,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down Expand Up @@ -721,7 +721,7 @@ public function test_a_sat_token_can_be_used_to_self_vet_a_token()

$command = new SelfVetSecondFactorCommand();
$command->secondFactorId = '+31 (0) 612345678';
$command->registrationCode = 'REGCODE';
$command->registrationCode = 'A1B2C3D4';
$command->identityId = $this->id->getIdentityId();
$command->authoringSecondFactorLoa = "loa-self-asserted";
$command->secondFactorType = 'sms';
Expand Down Expand Up @@ -754,7 +754,7 @@ public function test_a_sat_token_can_be_used_to_self_vet_a_token()
new SecondFactorType('yubikey'),
$yubikeyPublicId,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down Expand Up @@ -803,7 +803,7 @@ public function test_a_sat_token_can_be_used_to_self_vet_a_token()
new SecondFactorType('sms'),
$phoneIdentifier,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down Expand Up @@ -855,7 +855,7 @@ public function test_sat_not_allowed_when_one_vetted_token_is_identity_vetted()

$command = new SelfVetSecondFactorCommand();
$command->secondFactorId = 'identifier-for-a-gssp';
$command->registrationCode = 'REGCODE';
$command->registrationCode = 'A1B2C3D4';
$command->identityId = $this->id->getIdentityId();
$command->authoringSecondFactorLoa = "loa-self-asserted";
$command->secondFactorType = 'tiqr';
Expand Down Expand Up @@ -905,7 +905,7 @@ public function test_sat_not_allowed_when_one_vetted_token_is_identity_vetted()
new SecondFactorType('yubikey'),
$yubikeyPublicId,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down Expand Up @@ -954,7 +954,7 @@ public function test_sat_not_allowed_when_one_vetted_token_is_identity_vetted()
new SecondFactorType('sms'),
$phoneIdentifier,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down Expand Up @@ -995,7 +995,7 @@ public function test_sat_not_allowed_when_one_vetted_token_is_identity_vetted()
new SecondFactorType('tiqr'),
$gsspIdentifier,
DateTime::now(),
'REGCODE',
'A1B2C3D4',
$this->commonName,
$this->email,
new Locale('en_GB')
Expand Down
Loading
Loading