New operator verifySVNR that finds Austrian social security numbers #2063
Conversation
Adds info about the changes
adds new operator verifySVNR
…into v3/master
…utiful formatting.
victorhora
added
3.x
| ~VerifySVNR() { | ||
| delete m_re; | ||
| } | ||
| bool evaluate(Transaction *transaction, Rule *rule, |
There was a problem hiding this comment.
You do not need to specify those here. VerifySVNR inherit from Operator which already contains code to do exactly the same -
It is safe to remove this evaluate declaration. It is better to remove, so we will have a cleaner code in case of a refactoring or major change.
| const std::string &input) override { | ||
| return evaluate(transaction, NULL, input, NULL); | ||
| } | ||
| bool evaluate(Transaction *transaction, |
There was a problem hiding this comment.
Same for the evaluate above. No need to co-exist here.
| const std::string& input, | ||
| std::shared_ptr<RuleMessage> ruleMessage) override; | ||
|
|
||
| int convert_to_int(const char c); |
There was a problem hiding this comment.
Those could be private. Their usage is restricted to the very own class.
| @@ -0,0 +1,46 @@ | |||
| [ | |||
There was a problem hiding this comment.
Thumbs up for the test case :) that really speed up my review. Thank you.
src/operators/verify_svnr.cc
Outdated
| } | ||
|
|
||
| for (i = 0; i < input.size() - 1; i++) { | ||
| printf ("Decimal i: %d\n", i); |
There was a problem hiding this comment.
Although that is a very useful debug information, it may not a good idea for production I would consider removing it.
| namespace modsecurity { | ||
| namespace operators { | ||
|
|
||
| int VerifySVNR::convert_to_int(const char c) |
There was a problem hiding this comment.
I see that this function is also used in verify cpf. It seems to me that in a further step, we can move it to utils and make both to share the same base code.
Not need to be done now, that could be an optimization.
| { | ||
| "enabled":1, | ||
| "version_min":300000, | ||
| "title":"Testing Operator :: @verifysvnr (1/2)", |
|
Hi @Rufus125, I've made all the changes that I have suggested on the review and merged the patch. Do you mind to double check to see if I am not missing something? Thank you! |
Relates to #2062
I also created some tests and a PR, therefore the travis build will fail: owasp-modsecurity/secrules-language-tests#5