-
Notifications
You must be signed in to change notification settings - Fork 16
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
Respond with 400 instead of 500 when CSTG request validation fails #633
Conversation
@@ -453,6 +453,10 @@ else if(emailHash != null) { | |||
input = InputUtil.normalizePhoneHash(phoneHash); | |||
} | |||
|
|||
if (this.phoneSupport ? !checkTokenInputV1(input, rc) : !checkTokenInput(input, rc)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This expression this.phoneSupport ? !checkTokenInputV1(input, rc) : !checkTokenInput(input, rc)
appears 6 times in this file and ideally should be wrapped in a method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only difference between checkTokenInputV1
and checkTokenInput
is the error message returned.
Can we remove checkTokenInputV1
and do the this.phoneSupport
check in checkTokenInput
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I removed the checkTokenInputV1
and did the this.phoneSuppor
t check in checkTokenInput
. Also invert the method.
} | ||
|
||
@Test | ||
void cstgInvalidPhoneHashInput(Vertx vertx, VertxTestContext testContext) throws NoSuchAlgorithmException, InvalidKeyException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
possible to eliminate some of the duplication? the tests are identical other than the input. Can the input be parameterized?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is a good idea, I used the @ParameterizedTest
@@ -453,6 +453,10 @@ else if(emailHash != null) { | |||
input = InputUtil.normalizePhoneHash(phoneHash); | |||
} | |||
|
|||
if (checkTokenInput(input, rc)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if (checkTokenInput)
makes it seem like it succeeded. How about we rename this method to checkForInvalidTokenInput
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I rename checkTokenInput to checkForInvalidTokenInput. true for invalid input, false for valid input.
No description provided.