reducktion / socrates

PHP package to Validate and Extract information from National Identification Numbers.
MIT License
47 stars 4 forks source link

Correct Portugal validation algorithm #57

Closed SLourenco closed 4 years ago

SLourenco commented 4 years ago

The Portugal validation algorithm has a bug in an if code block that is never run. In

public function validate(string $id): bool
    {
        $id = $this->sanitize($id);

        $sum = 0;
        $toggleDigit = false;

        for ($i = 11; $i >= 0; $i--) {
            $value = $this->getCharacterValue($id[$i]);

            if ($toggleDigit) {
                $value *= 2;

                if ($value > 9) {
                    $value -= 9;
                }

                $sum += $value;
                $toggleDigit = !$toggleDigit;
            }
        }

        return ($sum % 10) === 0;
    }

the $toggleDigit is initialized to false and is not modified outside of the if statement, which depends on it being true to run.

AlexOlival commented 4 years ago

Thank you very very much!

You ended up exposing a problem in our Validation algorithm tests. Our assertions were:

public function test_validation_behaviour(): void
{
    foreach ($this->validIds as $id) {
        $this->assertTrue(
            Socrates::validateId($id, 'PT')
        );
    }

    // Here be dragons...
    $this->expectException(InvalidLengthException::class);

    Socrates::validateId('11084129 8 ZX', 'PT');

    // Everything here passes...
    $this->assertFalse(
        Socrates::validateId('14897475 4 ZY5', 'PT')
    );
}

Not only some countries test for a single invalid ID - which isn't ideal - but even those that test with multiple of those have that same problem.

I went ahead and changed the order of assertions and also made it test against an array of Invalid IDs:

public function test_validation_behaviour(): void
{
    foreach ($this->validIds as $id) {
        $this->assertTrue(
            Socrates::validateId($id, 'PT')
        );
    }

    foreach ($this->invalidIds as $invalidId) {
        $this->assertFalse(
            Socrates::validateId($invalidId, 'PT')
        );
    }

    $this->expectException(InvalidLengthException::class);

    Socrates::validateId('11084129 8 ZX', 'PT');
}

Also, I opened an issue #59 for us to go ahead and fix our test suite. Sounds like a new release is in order @JoaoFSCruz