Added new audit event sub-types #2170

apurvabhaleMS · 2021-08-17T18:56:06Z

Description

Added new audit event sub-types for -

Conditional-Update as its own sub-type
Conditional-Create as its own sub-type
Conditional-Delete as its own sub-type
Operations - $member-match, $purge-history

Related issues

Addresses [77729].
Addresses #1453

Testing

Manual Testing

FHIR Team Checklist

Update the title of the PR to be succinct and less than 50 characters
Add a milestone to the PR for the sprint that it is merged (i.e. add S47)
Tag the PR with the type of update: Bug, Dependencies, Enhancement, or New-Feature
Tag the PR with Azure API for FHIR if this will release to the managed service
Review squash-merge requirements

Semver Change (docs)

Patch|Skip|Feature|Breaking (reason)

Ivanidzo4ka · 2021-08-17T19:07:29Z

    [AuditEventType(AuditEventSubType.Search)]

Since we doing this, is it worth to add CompartmentSearch here?

Refers to: src/Microsoft.Health.Fhir.Shared.Api/Controllers/FhirController.cs:466 in 2423c46. [](commit_id = 2423c46, deletion_comment = False)

Ivanidzo4ka

Ivanidzo4ka · 2021-08-17T23:20:44Z

src/Microsoft.Health.Fhir.ValueSets/SpecialValues.cs

@@ -8,5 +8,7 @@ namespace Microsoft.Health.Fhir.ValueSets
    public static class SpecialValues
    {
        public const string System = "http://hl7.org/fhir/special-values";
+
+        public const string CustomAuditHeaderPrefix = "http://hl7.org/fhir/special-values";


http://hl7.org/fhir/special-values

Are you sure? #Closed

No :) I just updated the correct value

revoking review

Ivanidzo4ka · 2021-08-17T23:27:45Z

src/Microsoft.Health.Fhir.Api/Configs/FhirServerConfiguration.cs

@@ -22,7 +23,7 @@ public class FhirServerConfiguration : IApiConfiguration

        public OperationsConfiguration Operations { get; } = new OperationsConfiguration();

-        public AuditConfiguration Audit { get; } = new AuditConfiguration("X-MS-AZUREFHIR-AUDIT-");
+        public AuditConfiguration Audit { get; } = new AuditConfiguration(SpecialValues.CustomAuditHeaderPrefix);


new AuditConfiguration(SpecialValues.CustomAuditHeaderPrefix)

That's not right.
And it's not your fault, but if you touch this code we have chance to make it right.

It's either should be config or it should be const string, it can't be both.
I would suggest to move to away from config to const, for simpicity reasons. #Closed

Ivanidzo4ka · 2021-08-17T23:47:31Z

src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosResponseProcessor.cs

@@ -145,6 +146,7 @@ private async Task AddRequestChargeToFhirRequestContext(double responseRequestCh
                }

                requestContext.ResponseHeaders[CosmosDbHeaders.RequestCharge] = responseRequestCharge.ToString(CultureInfo.InvariantCulture);
+                requestContext.RequestHeaders[SpecialValues.CustomAuditHeaderPrefix + CosmosDbHeaders.RequestCharge] = responseRequestCharge.ToString(CultureInfo.InvariantCulture);


RequestHeaders

That doesn't sound right.

What request header has to do with request charge? #Closed

I mean, that collection setup on receiving request, and it's ok (actually not) to modify it while you preparing to process request, but after that, we need to work with response collection.

brendankowitz · 2021-08-18T16:13:13Z

src/Microsoft.Health.Fhir.ValueSets/SpecialValues.cs

@@ -8,5 +8,7 @@ namespace Microsoft.Health.Fhir.ValueSets
    public static class SpecialValues
    {
        public const string System = "http://hl7.org/fhir/special-values";
+
+        public const string CustomAuditHeaderPrefix = "X-MS-AZUREFHIR-AUDIT-";


This isn't part of a FHIR ValueSet. I'd say this should be in https://github.com/microsoft/fhir-server/blob/main/src/Microsoft.Health.Fhir.Core/Features/KnownHeaders.cs

Read custom audit headers from the response context

…split-telemetry-add-new-operations

lgtm-com · 2021-08-18T22:29:01Z

This pull request introduces 2 alerts when merging b8048ef into 3d53871 - view on LGTM.com

new alerts:

2 for Dereferenced variable may be null

Ivanidzo4ka · 2021-08-18T22:42:29Z

src/Microsoft.Health.Fhir.Api/Features/Audit/AuditHelper.cs

+
+                object cachedCustomHeaders;
+                var customHeaders = new Dictionary<string, string>();
+                if (httpContext.Items.TryGetValue(AuditConstants.CustomAuditHeaderKeyValue, out cachedCustomHeaders))


CustomAuditHeaderKeyValue

This would interfere with auditHeaderReader.Read, right?

Also in general is there case where you would call this method twice during request processing? If not, I would omit whole caching process, because it's not needed. #Closed

Ivanidzo4ka · 2021-08-18T22:43:50Z

src/Microsoft.Health.Fhir.Shared.Api.UnitTests/Features/Audit/AuditHelperTests.cs

@@ -25,10 +28,12 @@ public class AuditHelperTests
        private static readonly IReadOnlyCollection<KeyValuePair<string, string>> Claims = new List<KeyValuePair<string, string>>();
        private static readonly IPAddress CallerIpAddress = new IPAddress(new byte[] { 0xA, 0x0, 0x0, 0x0 }); // 10.0.0.0
        private const string CallerIpAddressInString = "10.0.0.0";
+        private static readonly string _customAuditHeaderPrefix = KnownHeaders.CustomAuditHeaderPrefix;


private static readonly string _customAuditHeaderPrefix = KnownHeaders.CustomAuditHeaderPrefix;

Feels excessive to create const to assign it another const. #Closed

Ivanidzo4ka · 2021-08-18T22:45:56Z

    [AuditEventType(AuditEventSubType.Search)]

https://thumbs.gfycat.com/CourageousSoreCurlew-max-1mb.gif

In reply to: 900559395

Refers to: src/Microsoft.Health.Fhir.Shared.Api/Controllers/FhirController.cs:466 in 2423c46. [](commit_id = 2423c46, deletion_comment = False)

Ivanidzo4ka · 2021-08-19T00:02:38Z

src/Microsoft.Health.Fhir.Api/Features/Audit/AuditHelper.cs

+            if (auditAction == AuditAction.Executed)
+            {
+                CheckForCustomAuditHeadersInResponse(httpContext, customHeaders);
+            }


Put it under if statement below.
No need to do computation if they not gonna be used. #Closed

Ivanidzo4ka · 2021-08-19T00:06:09Z

src/Microsoft.Health.Fhir.Api/Features/Audit/AuditHelper.cs

@@ -68,6 +77,13 @@ private void Log(AuditAction auditAction, HttpStatusCode? statusCode, HttpContex
            IFhirRequestContext fhirRequestContext = _fhirRequestContextAccessor.RequestContext;

            string auditEventType = fhirRequestContext.AuditEventType;
+            var customHeaders = _auditHeaderReader.Read(httpContext) as Dictionary<string, string>;


as Dictionary<string, string>

This is dangerous.
Not every IReadOnlyDictionary is a Dictionary, so this can become null if implementation of Read would change.

You can do
var newDictionary = Dictionary<string,string>(_auditHeaderReader.Read(httpContext))

to make it editable.

You change it to casting it to type, it's a same semantic.
Don't cast it, create a new one.

Ivanidzo4ka · 2021-08-19T00:19:08Z

src/Microsoft.Health.Fhir.CosmosDb/Features/Storage/CosmosResponseProcessor.cs

@@ -145,6 +150,7 @@ private async Task AddRequestChargeToFhirRequestContext(double responseRequestCh
                }

                requestContext.ResponseHeaders[CosmosDbHeaders.RequestCharge] = responseRequestCharge.ToString(CultureInfo.InvariantCulture);
+                requestContext.ResponseHeaders[_auditConfiguration.Value.CustomAuditHeaderPrefix + CosmosDbHeaders.RequestCharge] = responseRequestCharge.ToString(CultureInfo.InvariantCulture);


_auditConfiguration.Value.CustomAuditHeaderPrefix + CosmosDbHeaders.RequestCharge

Probably should ask that question few iterations before, but why we use custom header mechanism to report database consumption?

First should it even go through custom header section of the audit i.e. AuditLogger.AuditMessageFormat
and why not add another section instead of custom header.

If that's ok we can extend IFhirRequestContext to have float/integer to represent consumption charge (with default to 0) and set it up respectfully for sql/cosmos.

Also how it's gonna be used?

Ivanidzo4ka · 2021-08-20T02:13:47Z

src/Microsoft.Health.Fhir.Api/Features/Audit/AuditHelper.cs

+                    var headerValue = header.Value.ToString();
+                    if (headerValue.Length > AuditConstants.MaximumLengthOfCustomHeader)
+                    {
+                        throw new AuditHeaderTooLargeException(header.Key, headerValue.Length);


AuditHeaderTooLargeException

Why we check it twice?

Ivanidzo4ka

Ivanidzo4ka · 2021-08-20T18:40:45Z

LGTM, but I would add compartment search as separate audit event.

…split-telemetry-add-new-operations

Added new audit event sub-types

2423c46

Ivanidzo4ka previously approved these changes Aug 17, 2021

View reviewed changes

apurvabhaleMS added this to the S69 milestone Aug 17, 2021

Log RU's to Audit CustomHeaders

4018dc5

Ivanidzo4ka reviewed Aug 17, 2021

View reviewed changes

Correct the value for CustomAuditHeaderPrefix

30b2f12

Ivanidzo4ka reviewed Aug 17, 2021

View reviewed changes

brendankowitz reviewed Aug 18, 2021

View reviewed changes

apurvabhaleMS added 4 commits August 18, 2021 13:57

Moved CustomAuditHeaderPrefix to KnownHeaders

5372a01

Read custom audit headers from the response context

Merge remote-tracking branch 'origin/main' into personal/apurvabhale/…

1e80e62

…split-telemetry-add-new-operations

Updated to unit tests

02b878b

Updates to Unit test

b8048ef

Ivanidzo4ka reviewed Aug 18, 2021

View reviewed changes

apurvabhaleMS added 2 commits August 18, 2021 16:32

Updates to Unit tests

ceb9283

Updates to AuditHelper

0b1bbaa

Ivanidzo4ka reviewed Aug 19, 2021

View reviewed changes

Updates to AuditHelper

ecd093e

Ivanidzo4ka reviewed Aug 19, 2021

View reviewed changes

Fix failing unit tests

88c93ce

Ivanidzo4ka reviewed Aug 20, 2021

View reviewed changes

apurvabhaleMS added 2 commits August 20, 2021 08:10

Reverted Audit custom headers related changes for RU's

dd200ae

Reverted test files

25f372a

Ivanidzo4ka approved these changes Aug 20, 2021

View reviewed changes

apurvabhaleMS added 2 commits August 20, 2021 14:23

Merge remote-tracking branch 'origin/main' into personal/apurvabhale/…

9ee34a0

…split-telemetry-add-new-operations

Merge remote-tracking branch 'origin/main' into personal/apurvabhale/…

30b2ade

…split-telemetry-add-new-operations

CaitlinV39 modified the milestones: S69, S70 Aug 23, 2021

apurvabhaleMS marked this pull request as ready for review August 24, 2021 05:44

apurvabhaleMS requested a review from a team as a code owner August 24, 2021 05:44

apurvabhaleMS merged commit 34dca88 into main Aug 24, 2021

apurvabhaleMS deleted the personal/apurvabhale/split-telemetry-add-new-operations branch August 24, 2021 05:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added new audit event sub-types #2170

Added new audit event sub-types #2170

apurvabhaleMS commented Aug 17, 2021 •

edited by CaitlinV39

Loading

Ivanidzo4ka commented Aug 17, 2021

Ivanidzo4ka left a comment

Ivanidzo4ka Aug 17, 2021 •

edited

Loading

apurvabhaleMS Aug 17, 2021

Ivanidzo4ka Aug 17, 2021 •

edited

Loading

Ivanidzo4ka Aug 17, 2021 •

edited

Loading

Ivanidzo4ka Aug 17, 2021

brendankowitz Aug 18, 2021

lgtm-com bot commented Aug 18, 2021

Ivanidzo4ka Aug 18, 2021 •

edited

Loading

Ivanidzo4ka Aug 18, 2021 •

edited

Loading

Ivanidzo4ka commented Aug 18, 2021

Ivanidzo4ka Aug 19, 2021 •

edited

Loading

Ivanidzo4ka Aug 19, 2021 •

edited

Loading

Ivanidzo4ka Aug 20, 2021

Ivanidzo4ka Aug 19, 2021 •

edited

Loading

Ivanidzo4ka Aug 20, 2021

Ivanidzo4ka left a comment

Ivanidzo4ka commented Aug 20, 2021

Added new audit event sub-types #2170

Added new audit event sub-types #2170

Conversation

apurvabhaleMS commented Aug 17, 2021 • edited by CaitlinV39 Loading

Description

Related issues

Testing

FHIR Team Checklist

Semver Change (docs)

Ivanidzo4ka commented Aug 17, 2021

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

apurvabhaleMS Aug 17, 2021

Choose a reason for hiding this comment

Ivanidzo4ka Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 17, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 17, 2021

Choose a reason for hiding this comment

brendankowitz Aug 18, 2021

Choose a reason for hiding this comment

lgtm-com bot commented Aug 18, 2021

Ivanidzo4ka Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 18, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka commented Aug 18, 2021

Ivanidzo4ka Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 20, 2021

Choose a reason for hiding this comment

Ivanidzo4ka Aug 19, 2021 • edited Loading

Choose a reason for hiding this comment

Ivanidzo4ka Aug 20, 2021

Choose a reason for hiding this comment

Ivanidzo4ka left a comment

Choose a reason for hiding this comment

Ivanidzo4ka commented Aug 20, 2021

apurvabhaleMS commented Aug 17, 2021 •

edited by CaitlinV39

Loading

Ivanidzo4ka Aug 17, 2021 •

edited

Loading

Ivanidzo4ka Aug 17, 2021 •

edited

Loading

Ivanidzo4ka Aug 17, 2021 •

edited

Loading

Ivanidzo4ka Aug 18, 2021 •

edited

Loading

Ivanidzo4ka Aug 18, 2021 •

edited

Loading

Ivanidzo4ka Aug 19, 2021 •

edited

Loading

Ivanidzo4ka Aug 19, 2021 •

edited

Loading

Ivanidzo4ka Aug 19, 2021 •

edited

Loading