Reimplement the Audit Feature and Git Pack Parsing #769
Conversation
| $"{nameof(GitHandlerInvocationService)} has found {inspectStream.PackObjectCount} objects in the receive-pack stream."); | ||
|
|
||
| DirectoryInfo repoDirectory = _repoLocator.GetRepositoryDirectoryPath(repositoryName); | ||
| Debug.Assert(repoDirectory.Exists); |
There was a problem hiding this comment.
Should this throw an exception rather than being an assert? the assert is not part of a release build afaik?
There was a problem hiding this comment.
Yes, Debug.Assert is usually removed by the preprocessor for a release build. Because the Git process has successfully executed right before the repo directory is resvoled, I do just assume that it should exist in production.
The reason why I'm doing the assert there is to simplify debugging in case the IRepositoryLocator implementation doesn't work as expected - something which will most likely be noticed during debug / test sessions and not during production.
|
I have quickly looked through it and tested it with a few small commits plus a ~1 GB commit and it seems to work fine :) Generally it looks very well written. So +1 for some unit test. In addition I used git 2.15.0.windows.1 as my local client. |
|
Is it possible to add more handlers or is only one supporrted as is? for me it looks as only one is supported, but at places you write as if multiple handlers could be added? |
Thank you sir.
Yep, if you wanted to add more handlers you'd just decorate one another - pretty much the same way as the |
The tests use request data as previously recorded from a Git for Windows client v. 2.15.1, stored as raw binary data in file assets.
|
Alright, added some unit tests for the important parts of |
|
This could be a good starting point thinking about the usefulness of implementing webhooks. |
|
if it had so good reviews i only con wonder why it was denied still after 2 years... |
The new implementation is slimmer and easier to grab and shouldn't cause any noticeable performance overhead at all (which the old implementation did very much).
The way I designed it should make implementing features like #13 and #299 a breeze, at least regarding the Git stuff. It can also be extended to introduce things like per-branch permissions in the future, if desired.
Compared to the current state of the codebase, the only critical thing this implementation adds is the ReceivePackInspectStream class, because an instance of it is installed in the input stream for every push operation. It could cause pushes to fail. There's no way but to test it throughly against different Git clients and push scenarios before we can call this production ready.
That said, nothing else added by this PR can be considered critical - the worst thing which could happen is that some commits may not have notes added to them when audit is enabled...
Note that I've removed the durability / recovery feature completely as it didn't really make sense to me. If the Git process fails, the new handler which adds the commit notes will never be invoked. If the Git process succeeds but adding notes to the commits fails for some reason, then we should try to fix the root of that problem instead of temporarily saving request data to disk and trying to add the notes over and over again until it works.
I don't see why it was implemented like that.
I've removed these configuration settings:
Please let me know what you think about the new implementation. If it looks alright, I'll go ahead and write some tests, especially for the ReceivePackInspectStream class.