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.

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

Fix validation of personal numbers (years 2000-2009) #46

Closed
wants to merge 1 commit into from

Conversation

pn
Copy link

@pn pn commented Aug 17, 2022

Type of change

  • New feature
  • Bug fix
  • Security patch
  • Documentation update

Description

Pad least significant digits in a year variable with zero. Fixes case when
year < 10 (e.g. year == 0 would have produced new_y 190 and 200
instead of 1900 and 2000) leading to failed validation of some of
personal numbers for people born in years 2000-2009.

Related issue

None.

Motivation

Fixes significant bug in validating of personal numbers (e.g. personal numbers of people born in years 2000-2009 may be affected).

Checklist

  • I have read the CONTRIBUTING document.
  • I have read and accepted the Code of conduct
  • Tests passes.
  • Style lints passes.
  • Documentation of new public methods exists.

Pad least significant digits in a year with zero. Fixes case when
year < 10 (e.g. year == 0 would have produced new_y 190 and 200
instead of 1900 and 2000)
@frozzare
Copy link
Member

Awesome, thanks for your pull request. Can you add a test case that verifies this?

@dannyhajj
Copy link
Member

dannyhajj commented Nov 8, 2023

I would say the whole test_date method is strange. It would make more sense to just pass int(parts['century'] + parts['year']) to the method instead of having it run through a loop of two separate centuries since it's already calculated.

Btw, not everyone born in those 2000-2009 (also 1900-1909) will be impacted. It's only people born in leap years on Feb. 29 (for example, someone born in 2000-02-29 will fail validation). Because a date of 190-01-01 will not fail and is a valid date to use.

This is an example SSN that will fail validation incorrectly: 200002282382. While 200002282382 will pass validation.
These SSN numbers is taken from the list of available numbers for testing here: https://swedish.identityinfo.net/personalidentitynumber/testdata

>>> personnummer.Personnummer('200002292399').valid()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/usr/local/lib/python3.10/site-packages/personnummer/personnummer.py", line 29, in __init__
    raise PersonnummerException(
personnummer.personnummer.PersonnummerException: 200002292399 Not a valid Swedish personal identity number!
>>> personnummer.Personnummer('200002282382').valid()
True

On a similar note, it is possible for this method to create false positives as well, not only false negatives. For example, 2020 was not a leap year, but 1920 was. So if someone uses an SSN with a valid Luhn checksum but uses the '2020-02-29' date, it will flag it as positive. For example, the following SSN should not be valid [redacted].

>>> personnummer.Personnummer('[redacted]').valid()
True

@frozzare
Copy link
Member

frozzare commented Nov 9, 2023

The python version of personnummer isn't maintained unfortunately, so not much will happen until we have someone that can help with that. Me and the others has enough of packages to maintain, but I can try to answer at least. We will try to get some info about which packages are maintained or not on the site and in the repos.

I've looked up 200002292399 and 200002282382 on the site you linked to, and both should be valid and both are valid in other packages, so it's a problem with the python version and should be fixed.

The second part about leap year I don't understand since you're saying that 2020 was not a leap year, but according to the calendars it's not true.

Screenshot 2023-11-08 at 17 58 03

@dannyhajj
Copy link
Member

dannyhajj commented Nov 9, 2023

@frozzare Yes, sorry about the 2020 one. I don't know what I was thinking, lol. (I edited and redacted the ssn from my previous comment as it may be real).

Regarding the first part, yes, it's related to the Python version due to how the package tests the dates. I'll send in a separate PR shortly since one looks to be stale.

@frozzare
Copy link
Member

frozzare commented Nov 9, 2023

Thanks for your contribution! Much appreciated :)

@dannyhajj dannyhajj mentioned this pull request Nov 10, 2023
10 tasks
@Johannestegner
Copy link
Member

Replaced with #57

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants