Feature pathmask #27

sunnamed434 · 2023-02-20T09:36:37Z

This new feature replaces paths and involves support for Windows, Linux, macOS

sandermvanvliet · 2023-02-20T19:29:05Z

Thank you for your PR 👍

Just to manage expectations: I’ll review it soon but can’t promise it’ll be tomorrow 😉 Feel free to remind me if you haven’t heard from me in a few days

sunnamed434 · 2023-02-21T04:15:19Z

Hi @sunnamed434!

Thank you for your PR 👍

Just to manage expectations: I’ll review it soon but can’t promise it’ll be tomorrow 😉 Feel free to remind me if you haven’t heard from me in a few days

Hi,

Yes, sure, I'll remind

sunnamed434 · 2023-02-23T08:59:20Z

Hi @sandermvanvliet!

It's a reminder =)

sandermvanvliet

I like this extra operator and I suspect there will be many folks that find it useful.

I've added a few comments, overall it looks good but there are some tweaks to make it better 👍

sandermvanvliet · 2023-03-04T14:17:34Z

README.md

@@ -102,6 +102,7 @@ By default the enricher uses the following masking operators:
 - EmailAddressMaskingOperator
 - IbanMaskingOperator
 - CreditCardMaskingOperator
+- PathMaskingOperator


Adding the default operators when I built this library was a bit of a mistake.
Could you remove it from the docs here? (I see you didn't actually add it to the list of default operators)

Added to the default list of operators

sandermvanvliet · 2023-03-04T14:19:45Z

test/Serilog.Enrichers.Sensitive.Tests.Benchmark/PathMaskingOperatorBenchmarks.cs

+
+namespace Serilog.Enrichers.Sensitive.Tests.Benchmark;
+
+[SimpleJob(RunStrategy.Throughput, warmupCount: 1)]


The existing benchmark for regexes already proves a compiled regex is faster so adding another benchmark to prove it again isn't necessary.
You can remove this class altogether.

Class removed

sandermvanvliet · 2023-03-04T14:20:25Z

test/Serilog.Enrichers.Sensitive.Tests.Benchmark/Program.cs

@@ -9,6 +9,7 @@ private static void Main(string[] args)
            BenchmarkRunner.Run<BenchmarkCompiledEmailRegex>();
            BenchmarkRunner.Run<BenchmarkCompiledIbanRegex>();
            BenchmarkRunner.Run<CreditCardMarkingBenchmarks>();
+            BenchmarkRunner.Run<PathMaskingOperatorBenchmarks>();


This can be removed too

Removed also

sandermvanvliet · 2023-03-04T14:23:38Z

test/Serilog.Enrichers.Sensitive.Tests.Unit/WhenMaskingPaths.cs

+    [InlineData(@"C:\Users\Admin\Secret\Hidden", @"***\Hidden", true)]
+    [InlineData(@"C:\Users\Admin\Secret", @"***\Secret", true)]
+    [InlineData(@"C:\Users\", @"***\Users", true)]
+    [InlineData(@"/home/i_use_arch_linux_btw", @"***\i_use_arch_linux_btw", true)]


This made me laugh 😄

sandermvanvliet · 2023-03-04T14:24:55Z

src/Serilog.Enrichers.Sensitive/PathWrapper.cs

+
+namespace Serilog.Enrichers.Sensitive;
+
+public class PathWrapper : IPathWrapper


I would strongly recommend to remove this abstraction, it's not adding any value right now as there is no other implementation of IPathWrapper and the methods you are using do not actually depend on the filesystem being available.

So I'd prefer that where you're calling _pathWrapper.GetDirectoryName(), just call Path.GetDIrectoryName instead.

The only reason I made such abstraction, cuz I thought if there's a specific when some of the OS unable to get the path properly, then developer might make it own implementation. In case if this feature with abstraction would be removed and If I were such developer, I'd just delete the package and go cry. Let me know if this is still should be removed

On the other hand, Path.GetDIrectoryName works weird, sometimes It's unable to get the directory name correct

I understand where you're coming from, however you may expect from the runtime to handle this for you.

Now path wrapper removed

sandermvanvliet · 2023-03-04T14:32:55Z

src/Serilog.Enrichers.Sensitive/PathMaskingOperator.cs

+    /// <summary>
+    /// Initializes a new instance of the <see cref="PathMaskingOperator"/> class.
+    /// </summary>
+    public PathMaskingOperator() : this(combineMaskWithPath: true)


This constructor isn't necessary as the one you are calling has a default value for the parameter.

Empty ctor removed

sandermvanvliet · 2023-03-04T14:33:37Z

src/Serilog.Enrichers.Sensitive/PathMaskingOperator.cs

+    /// </summary>
+    /// <param name="pathWrapper">The path wrapper.</param>
+    /// <param name="combineMaskWithPath">This means if set to <see langword="true"/> then the mask will combine with the file name or its directory name.</param>
+    public PathMaskingOperator(IPathWrapper pathWrapper, bool combineMaskWithPath = true) : base(PathPattern)


As the IPathWrapper abstraction isn't necessary (see my other comment below) you can simplify this constructor after inlining the Path.* methods.

Now path wrapper removed

sandermvanvliet · 2023-03-04T14:35:07Z

test/Serilog.Enrichers.Sensitive.Tests.Unit/WhenMaskingPaths.cs

+    private const string Mask = @"***\";
+
+    [Theory]
+    [InlineData(@"C:\Users\Admin\Secret\File.dll", @"***\", false)]


I would expect this to be only the mask in the expected output. Right now it looks like you're keeping the directory separator which seems odd.

Don't really understand what do you mean

sandermvanvliet · 2023-03-04T14:36:37Z

src/Serilog.Enrichers.Sensitive/PathMaskingOperator.cs

+public class PathMaskingOperator : RegexMaskingOperator
+{
+    private readonly IPathWrapper _pathWrapper;
+    private readonly bool _combineMaskWithPath;


It took me a while to figure out exactly what this meant. Perhaps changing it to _keepLastPartOfPath or _doNotMaskLastPartOfPath would be more descriptive and easier to understand for consumers of this.

Renamed to the _keepLastPartOfPath

sandermvanvliet · 2023-03-04T14:41:01Z

@sunnamed434 thanks for the reminder 😉 Life got in the way...

sunnamed434 · 2023-03-05T10:40:57Z

Merge conflicts are resolved now (from master branch)

sunnamed434 · 2023-04-28T06:32:43Z

@sandermvanvliet Hi! Can you accept PR?

sunnamed434 added 7 commits February 20, 2023 11:05

Add path wrapper

b798c30

Add tests

9fc309f

Add demo

7ec4223

Add benchmark

50c80db

Update version to 0.2.0.0

144b637

Update README and add new PathMaskingOperator

9c9b506

Update version correctly

d4c2ac8

sandermvanvliet requested changes Mar 4, 2023

View reviewed changes

sunnamed434 added 5 commits March 4, 2023 18:57

Remove benchmark

2296354

Add PathMaskingOperator as new default masking operator

968e9b9

Code refactoring

163a215

Remove path wrapper

245da54

Merge branch 'master' into feature/pathmask

9326948

Merge branch 'master' into feature/pathmask

1f9bf0b

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature pathmask #27

Feature pathmask #27

sunnamed434 commented Feb 20, 2023

sandermvanvliet commented Feb 20, 2023

sunnamed434 commented Feb 21, 2023

sunnamed434 commented Feb 23, 2023

sandermvanvliet left a comment

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 5, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 5, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 5, 2023

sandermvanvliet Mar 4, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 4, 2023 •

edited

Loading

sunnamed434 Mar 4, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 5, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 4, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 5, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 5, 2023

sandermvanvliet Mar 4, 2023

sunnamed434 Mar 4, 2023

sandermvanvliet commented Mar 4, 2023

sunnamed434 commented Mar 5, 2023 •

edited

Loading

sunnamed434 commented Apr 28, 2023


		namespace Serilog.Enrichers.Sensitive.Tests.Benchmark;

		[SimpleJob(RunStrategy.Throughput, warmupCount: 1)]


		namespace Serilog.Enrichers.Sensitive;

		public class PathWrapper : IPathWrapper

Feature pathmask #27

Are you sure you want to change the base?

Feature pathmask #27

Conversation

sunnamed434 commented Feb 20, 2023

sandermvanvliet commented Feb 20, 2023

sunnamed434 commented Feb 21, 2023

sunnamed434 commented Feb 23, 2023

sandermvanvliet left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sunnamed434 Mar 4, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sandermvanvliet commented Mar 4, 2023

sunnamed434 commented Mar 5, 2023 • edited Loading

sunnamed434 commented Apr 28, 2023

sunnamed434 Mar 4, 2023 •

edited

Loading

sunnamed434 commented Mar 5, 2023 •

edited

Loading