Sensitive commands [WIP] #1705
Sensitive commands [WIP] #1705
Conversation
Test doesn't compile, needs feedback. #1703 related
|
@ljacqu what do you think about that? I think that using the ConsoleFilter(LogFilterService) constructor is a bit dirty, any suggestion? |
| * @param sensitive The sensitive data flag | ||
| * @return The builder | ||
| */ | ||
| public CommandBuilder sensitive(boolean sensitive) { |
There was a problem hiding this comment.
Minor: I would suggest making sensitive to a small boolean on the builder and to change this method to just be isSensitive(), setting the sensitive flag to true.
| String rawCommand = message.substring(message.indexOf("/")); | ||
| List<String> components = Arrays.asList(rawCommand.split(" ")); | ||
| FoundCommandResult command = commandMapper.mapPartsToCommand(null, components); | ||
| switch (command.getResultStatus()) { |
There was a problem hiding this comment.
This is really cool! Do you think we should somehow cache invocations of the commands? I'm a little worried about performance since this will be performed every time someone issues a command, and I'm not quite sure how efficient the commandMapper is.
Failing that, I just saw that the LogFilterHelper has a test (LogFilterHelperTest) that ensures that all of those previously hard-coded labels are present. The alternative would be to keep the list and to update the test to use the sensitive flag.
There was a problem hiding this comment.
- yeah, performance can be a problem hmm... we should test it.
- I'm not sure about the test :/ What would you do?
There was a problem hiding this comment.
I'll drop the test class for the moment, as it needs to be redone.
There was a problem hiding this comment.
Actually you know what, it makes sense to drop the test and to move some of that logic into the filter service. If the filter service gets all commands it can filter out the sensitive ones and create the set of "forbidden messages" by itself. This skips the command mapper and makes sure it's always up-to-date, solving both problems :)
There was a problem hiding this comment.
hmm, yeah, which portion of logic do you want to move?
There was a problem hiding this comment.
Previously it knew all forbidden command labels instead of calling the command mapper, right? Well, the test had the logic (more or less) to construct them - if you inject the CommandInitializer into here you have access to all the sensitive commands and can do exactly that
| default: | ||
| break; | ||
| } | ||
| return command.getCommandDescription().isSensitive(); |
There was a problem hiding this comment.
Minor: if we're playing code beauty contest I'd suggest putting the return statement in the default clause instead of breaking from it there; that way we oppose both cases directly to each other.
| private LogFilterService filterService; | ||
|
|
||
| public Log4JFilter(LogFilterService filterService) { | ||
| this.filterService = filterService; |
There was a problem hiding this comment.
I think that using the ConsoleFilter(LogFilterService) constructor is a bid dirty, any suggestion?
I don't really have a better suggestion. In theory we could put @Inject on the constructor and have a Log4JFilter instance injected into OnstartupTasks, but given the nature of how it is set up (in OnStartupTasks we have compatibility checks in case a certain log class doesn't exist) I wouldn't recommend doing that. I think your current solution works out nicely :)
|
@ljacqu Could you please have again a look at this? :) |
|
Comment #1705 (comment) still stands, ideally we can construct all combinations of sensitive commands and keep them in a Set, rather than calling the slow command mapper on every command. I'm also missing tests for the new LogFilterService ;) |
Test doesn't compile, needs feedback.
#1703 related