[GitHub] commons-lang issue #335: LANG-1400: Add StringUtils.mask() function
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/335 Thank you for your examples. Just to clarify: `maskedStart` from this PR corresponds to `minMasked` from #332 but the discussion is about adding a `maxUnmasked`, right? > some messages are short and contain one time passwords it doesn't need for a masking at all. Short term generated values, OTPs and tokens like OAuth `access_token` (but not `refresh_token`) are safe to write to logs. If hacker stole logs we will have nothing to do with the data. > have a `minMasked` value of 30 Why exactly 30? What if sensitive data will be still shown in unmasked part? For this kind of data it's better to mask whole string. > when we print part of the address we need at least the first 15 characters to be masked If I understood correctly, it's enough `maskedStart` of 15 and `maxUnmasked` not needed in the case. > a generic API the method should be flexible, in order to cover a wide variety of usecases. What I'm afraid is that making the API too general leads to spending a time to read the docs, or incorrect understanding and misuse. As a developer I can clearly know how much chars to mask because I'm expect some kind of string but how much chars will be in real string is unknown variable for me, so `maxUnmasked` will be always a speculation. For example java's `StringBuilder` has a constructor with initial capacity and from my experience programmers are never even try to use it while this is critical for performance. So adding a new parameter `maxUnmasked` may be not so useful in real life but confusing. ---
[GitHub] commons-lang issue #335: LANG-1400: Add StringUtils.mask() function
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/335 Sound reasonable in the same time it will not always obviously which minMasked to use. For example already mentioned credit card numbers in fact can have varying length as I remember up to 24 digits. As I understood I created this function for specific case which you had. Can you provide some examples from real life when minMasked was needed? ---
[GitHub] commons-lang issue #335: LANG-1400: Add StringUtils.mask() function
Github user stokito commented on the issue:
https://github.com/apache/commons-lang/pull/335
Hi @greenman18523
> would you consider an extra parameter, to clearly specify the minimum
number of masked characters?
For those use cases which I mentioned (masking credit cards and passwords)
this looks not needed for me. Maybe you know some cases when this may be needed?
As I understood you are telling about more safety and do not unmask any
symbol if incoming string is too short while implementation which I proposed
will try to show at least some symbols from start.
For example `mask("123456", 4, 4) = "12"` which makes hidden symbols
more guessable.
But, to be honest, if someone uses so short password then it doesn't matter
if it will be shown.
Another solution in this case we can mask everything when str len is 6 <
unmaskendStart 4 + unmaskedEnd 4. I.e. `mask("123456", 4, 4) = "**"`. This
is easier to understood but in the same time it still may be useful to unmask
at least something but I don't think it's so critical.
What do you think about this proposition? E.g.
```
mask("12345678", 4, 4) = ""
mask("123456789", 4, 4) = "5"
mask("1234567890", 4, 4) = "56"
```
I hope that `unmaskedStart` and `unmaskedEnd` in real life will be always
reasonable (1-6) and the incoming string will be always bigger. We can actually
restrict passing strings less that some length and throw an exception.
But from possible use cases it looks that `mask()` function should be
failsafe because it may be used just for logging of external input which can be
anything and we shouldn't break it's processing. I even think about returning
an empty string if null was passed.
Also we have to think about performance because I expect that the function
will be widely used for in logging filters for any incoming request.
---
[GitHub] commons-lang issue #332: Add methods allowing masking of Strings
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/332 Hi @greenman18523 thank for your contribution - from my experience this is "must have" functionality for Commons Lang library because in almost all bit projects what I saw was their home grown masking function. But what I would like to propose is to simplify the api and use one function `mask()` instead of two `maskStart()` and `maskEnd()`. This will simplify code of the functions especially both of them are quite similar. I created a pull request with my alternative implementation #335 Also I created a JIRA ticket https://issues.apache.org/jira/browse/LANG-1400 Please give your feedback, Thank you ---
[GitHub] commons-lang pull request #335: LANG-1400: Add StringUtils.mask() function
GitHub user stokito opened a pull request: https://github.com/apache/commons-lang/pull/335 LANG-1400: Add StringUtils.mask() function My alternative implementation for #332 See https://issues.apache.org/jira/browse/LANG-1400 for details You can merge this pull request into a Git repository by running: $ git pull https://github.com/stokito/commons-lang LANG-1400 Alternatively you can review and apply these changes as the patch at: https://github.com/apache/commons-lang/pull/335.patch To close this pull request, make a commit to your master/trunk branch with (at least) the following in the commit message: This closes #335 commit 001b769fab526f2034c155dab422dfe10bf48cff Author: Sergey Ponomarev Date: 2018-06-23T22:47:33Z LANG-1400: Add StringUtils.mask() function ---
[GitHub] commons-lang issue #318: clean code
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/318 Changes are ok ---
[GitHub] commons-lang issue #278: Lang-1345 Enhance non-empty strings
Github user stokito commented on the issue: https://github.com/apache/commons-lang/pull/278 For me is not clear importance of the new methods. I can't remember situations when I needed for something like this. And even if I need something like this I'll write the function myself and I'll not even think about to search something in Commons Lang library. Time spent to looking for the method will be ten time more then write myself. Maybe commons lang itself uses the function internally then it can make sense. It would be great if you can give us an examples of usage and how they are important. ---
[GitHub] commons-lang issue #334: Code refactoring and cleaning
Github user stokito commented on the issue:
https://github.com/apache/commons-lang/pull/334
I checked and everything is ok in terms of backward compatibility. But the
commit 7f8571a506c7081bae0cf27bd93295e0344160bf "flips the order of conditional
expressions and 'if' statements whose conditions were negated." may be not so
good in terms of code readability.
First of all, there is always some natural flow when we expect that
something is more likely happens.
Let's take a look on the method `addProcessor()`:
```java
private static void addProcessor(final String key, final Processor
processor) {
if (!ARCH_TO_PROCESSOR.containsKey(key)) {
ARCH_TO_PROCESSOR.put(key, processor);
} else {
final String msg = "Key " + key + " already exists in processor
map";
throw new IllegalStateException(msg);
}
}
```
Here developer sees a "happy path" first and only then he or she sees
exception situation handling.
This is more natural to understand. As I remember this style was mentioned
in Complete Code book.
Also this style has slightly better performance because in most situations
processor will go forward to prefetched instructions or even it can be already
executed by [branch prediction](https://en.wikipedia.org/wiki/Branch_predictor).
Meanwhile removing of negation usually doesn't have any performance impact
because processors have already negated instructions.
I thinks it's ok to merge this commit but I just wan't you to know that
sometimes it is better to make unnecessary negation.
---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Github user stokito commented on a diff in the pull request: https://github.com/apache/commons-lang/pull/299#discussion_r144134867 --- Diff: .travis.yml --- @@ -17,12 +17,10 @@ language: java sudo: false jdk: - - openjdk7 --- End diff -- Sounds sad :( Especially because, I guess, jdk8 compiler willn't just ignore the `module-info.java`. Maybe some other projects will even try to generate the `module-info.java` ---
[GitHub] commons-lang pull request #299: Add module-info for Java 9
Github user stokito commented on a diff in the pull request: https://github.com/apache/commons-lang/pull/299#discussion_r143869192 --- Diff: .travis.yml --- @@ -17,12 +17,10 @@ language: java sudo: false jdk: - - openjdk7 --- End diff -- Why did you removed the build for jdk7 and 8? If lang3 still support them then it should be runned tests on 7 and 8 ---
