HV-2117 NIP validation incorrectly accepts values ending in 0

Author: marko-bekhta

engine/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/pl/NIPValidator.java

@@ -32,4 +32,13 @@ public void initialize(NIP constraintAnnotation) {
 	protected int[] getWeights(List<Integer> digits) {
 		return WEIGHTS_NIP;
 	}
+
+	@Override
+	protected boolean checkTwoDigitModuloResult(char checkDigit) {
+		// From https://pl.wikipedia.org/wiki/Numer_identyfikacji_podatkowej:
+		//   > NIP jest tak generowany, aby nigdy w wyniku tego dzielenia, jako reszta, nie uzyskać liczby 10
+		//
+		// which means that the way NIP is generated the checksum can never be 10, so if we got here, we've got an invalid NIP:
+		return false;
+	}
 }

engine/src/main/java/org/hibernate/validator/internal/constraintvalidators/hv/pl/PolishNumberValidator.java

@@ -14,7 +14,7 @@
 import org.hibernate.validator.internal.util.ModUtil;
 
 /**
- * A base class validator for different Polish identification numbers. They differs in the lengths and weights used to calculate the mod sum.
+ * A base class validator for different Polish identification numbers. They differ in the lengths and weights used to calculate the mod sum.
  * In order to implement one you need to provide a method that gives an array of weights
  * and {@link ConstraintValidator#initialize(Annotation)} methods.
  *
@@ -38,11 +38,15 @@ public boolean isCheckDigitValid(List<Integer> digits, char checkDigit) {
 		switch ( modResult ) {
 			case 10:
 			case 11:
-				return checkDigit == '0';
+				return checkTwoDigitModuloResult( checkDigit );
 			default:
 				return Character.isDigit( checkDigit ) && modResult == extractDigit( checkDigit );
 		}
 	}
 
+	protected boolean checkTwoDigitModuloResult(char checkDigit) {
+		return checkDigit == '0';
+	}
+
 	protected abstract int[] getWeights(List<Integer> digits);
 }

engine/src/test/java/org/hibernate/validator/test/constraints/annotations/hv/pl/NIPValidatorTest.java

@@ -11,6 +11,7 @@
 import org.hibernate.validator.constraints.pl.NIP;
 import org.hibernate.validator.test.constraints.annotations.AbstractConstrainedTest;
 
+import org.testng.annotations.DataProvider;
 import org.testng.annotations.Test;
 
 /**
@@ -40,69 +41,59 @@ public void testIncorrectLength() {
 				);
 	}
 
-	@Test
-	public void testCorrectNipNumber() {
-		assertNoViolations( validator.validate( new Person( "5931423811" ) ) );
-		assertNoViolations( validator.validate( new Person( "2596048500" ) ) );
-		assertNoViolations( validator.validate( new Person( "4163450312" ) ) );
-		assertNoViolations( validator.validate( new Person( "1786052059" ) ) );
-		assertNoViolations( validator.validate( new Person( "6660057854" ) ) );
-		assertNoViolations( validator.validate( new Person( "4219220786" ) ) );
-		assertNoViolations( validator.validate( new Person( "3497264632" ) ) );
-
+	@Test(dataProvider = "validNips")
+	public void testCorrectNipNumber(String nip) {
+		assertNoViolations( validator.validate( new Person( nip ) ) );
 	}
 
-	@Test
-	public void testIncorrectNipNumber() {
-		assertThat( validator.validate( new Person( "123-456-78-14" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "123-45-67-812" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "123-456-32-12" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "5931423812" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "2596048505" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "4163450311" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "1786052053" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "6660057852" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "4219220785" ) ) )
-				.containsOnlyViolations(
-						violationOf( NIP.class ).withProperty( "nip" )
-				);
-		assertThat( validator.validate( new Person( "3497264639" ) ) )
+	@Test(dataProvider = "invalidNips")
+	public void testIncorrectNipNumber(String nip) {
+		assertThat( validator.validate( new Person( nip ) ) )
 				.containsOnlyViolations(
 						violationOf( NIP.class ).withProperty( "nip" )
 				);
 	}
 
+	@DataProvider(name = "validNips")
+	private static Object[][] validNips() {
+		return new Object[][] {
+				{ "5931423811" },
+				{ "2596048500" },
+				{ "4163450312" },
+				{ "1786052059" },
+				{ "6660057854" },
+				{ "4219220786" },
+				{ "3497264632" }
+		};
+	}
+
+	@DataProvider(name = "invalidNips")
+	private static Object[][] invalidNips() {
+		return new Object[][] {
+				{ "123-456-78-14" },
+				{ "123-45-67-812" },
+				{ "123-456-32-12" },
+				{ "5931423812" },
+				{ "2596048505" },
+				{ "4163450311" },
+				{ "1786052053" },
+				{ "6660057852" },
+				{ "4219220785" },
+				{ "3497264639" },
+				{ "4062321040" },
+				{ "7985097620" },
+				{ "8808817210" }
+		};
+	}
+
 	public static class Person {
 
 		@NIP
 		private String nip;
 
 		public Person(String nip) {
 			this.nip = nip;
+			System.err.println( nip );
 		}
 	}