-
Notifications
You must be signed in to change notification settings - Fork 33
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
Adding Redacted Keys #217
base: next
Are you sure you want to change the base?
Adding Redacted Keys #217
Conversation
Co-authored-by: Tom Longridge <[email protected]>
* @param redactedKeys a list of String keys to be redacted from metaData | ||
*/ | ||
public void setRedactedKeys(String... redactedKeys) { | ||
config.redactedKeys = redactedKeys; |
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.
Unfortunately we're stuck with Configuration.filters being public, so we probably want to keep filters
and redactedKeys
synchronised otherwise things could get confusing if both are used for any reason. Suggest also setting config.filters
here and removing the line from setFilters
above?
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.
Looking at the changes, I wonder if in fact this change is more risky as a non-breaking change than I thought as we'll need to compile the filter strings as regexes (L249). At very least we need a try-catch for a PatternSyntaxException
, but it makes me a bit nervous even if 99.9% of users will have pretty straightforward key names with an occasional .
which will still match a literal .
.
@lemnik - thoughts here? The alternative is that we go back to running the two config options independently alongside each other (which I think was @clr182's first take).
public String[] redactedKeys = new String[]{"password", "secret", "Authorization", "Cookie"}; | ||
public String[] filters = new String[]{"password", "secret", "Authorization", "Cookie"}; |
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 think it would be appropriate to set this to the same instance rather than a different array with the same content. Just in case anyone is amending elements in the array.
public String[] redactedKeys = new String[]{"password", "secret", "Authorization", "Cookie"}; | |
public String[] filters = new String[]{"password", "secret", "Authorization", "Cookie"}; | |
public String[] redactedKeys = new String[]{"password", "secret", "Authorization", "Cookie"}; | |
public String[] filters = redactedKeys; |
assertEquals(2, config.redactedKeys.length); | ||
ArrayList<String> redactedKeys = new ArrayList<String>(Arrays.asList(config.redactedKeys)); | ||
assertTrue(redactedKeys.contains("password")); | ||
assertTrue(redactedKeys.contains("credit_card_number")); |
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.
It would be worth adding a few tests to verify the expected interplay between filters
and redactedKeys
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've added functionality so that when both are called, setRedactedKeys creates a temp array to append the filters to the redactedKeys config option to avoid overwriting.
* @param redactedKeys a list of String keys to be redacted from metaData | ||
*/ | ||
public void setRedactedKeys(String... redactedKeys) { | ||
config.redactedKeys = redactedKeys; |
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.
Looking at the changes, I wonder if in fact this change is more risky as a non-breaking change than I thought as we'll need to compile the filter strings as regexes (L249). At very least we need a try-catch for a PatternSyntaxException
, but it makes me a bit nervous even if 99.9% of users will have pretty straightforward key names with an occasional .
which will still match a literal .
.
@lemnik - thoughts here? The alternative is that we go back to running the two config options independently alongside each other (which I think was @clr182's first take).
Co-authored-by: Tom Longridge <[email protected]>
Goal
Filtering config option is case sensitive and is no longer in line with our notifier spec; as such it should be deprecated in favour of
redactedKeys
.Design
Added
redactedKeys
configuration option, which is case insensitive, as a replacement for thefilters
configuration option. Additionally due to conflictions the defaultfilters
parameters were changed.Changeset
Added
redactedKeys
config option.Made
redactedKeys
case insensitive.Changed
filters
config default parameters from{"password", "secret", "Authorization", "Cookie"};
to
{"ipAddress", "logLevel"};
Set
refactedKeys
config default parameters to{"password", "secret", "Authorization", "Cookie"};
Testing
Added and updated tests for filtering mechanism and redactedKeys mechanism.