Skip to content
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

Added new audit event sub-types #2170

Merged
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
using Microsoft.Health.Api.Features.Cors;
using Microsoft.Health.Core.Configs;
using Microsoft.Health.Fhir.Core.Configs;
using Microsoft.Health.Fhir.Core.Features;

namespace Microsoft.Health.Fhir.Api.Configs
{
Expand All @@ -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(KnownHeaders.CustomAuditHeaderPrefix);

public CoreFeatureConfiguration CoreFeatures { get; } = new CoreFeatureConfiguration();

Expand Down
52 changes: 50 additions & 2 deletions src/Microsoft.Health.Fhir.Api/Features/Audit/AuditHelper.cs
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,15 @@
// Licensed under the MIT License (MIT). See LICENSE in the repo root for license information.
// -------------------------------------------------------------------------------------------------

using System;
using System.Collections.Generic;
using System.Linq;
using System.Net;
using EnsureThat;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Options;
using Microsoft.Health.Api.Features.Audit;
using Microsoft.Health.Core.Configs;
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Core.Features.Security;
using Microsoft.Health.Fhir.Core.Features.Context;
Expand All @@ -21,19 +26,23 @@ public class AuditHelper : IAuditHelper
private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor;
private readonly IAuditLogger _auditLogger;
private readonly IAuditHeaderReader _auditHeaderReader;
private readonly IOptions<AuditConfiguration> _auditConfiguration;

public AuditHelper(
RequestContextAccessor<IFhirRequestContext> fhirRequestContextAccessor,
IAuditLogger auditLogger,
IAuditHeaderReader auditHeaderReader)
IAuditHeaderReader auditHeaderReader,
IOptions<AuditConfiguration> auditConfiguration)
{
EnsureArg.IsNotNull(fhirRequestContextAccessor, nameof(fhirRequestContextAccessor));
EnsureArg.IsNotNull(auditLogger, nameof(auditLogger));
EnsureArg.IsNotNull(auditHeaderReader, nameof(auditHeaderReader));
EnsureArg.IsNotNull(auditConfiguration, nameof(auditConfiguration));

_fhirRequestContextAccessor = fhirRequestContextAccessor;
_auditLogger = auditLogger;
_auditHeaderReader = auditHeaderReader;
_auditConfiguration = auditConfiguration;
}

/// <inheritdoc />
Expand Down Expand Up @@ -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>;
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// check for custom audit headers in context-response
if (auditAction == AuditAction.Executed)
{
CheckForCustomAuditHeadersInResponse(httpContext, customHeaders);
}
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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


// Audit the call if an audit event type is associated with the action.
if (!string.IsNullOrEmpty(auditEventType))
Expand All @@ -81,7 +97,39 @@ private void Log(AuditAction auditAction, HttpStatusCode? statusCode, HttpContex
correlationId: fhirRequestContext.CorrelationId,
callerIpAddress: httpContext.Connection?.RemoteIpAddress?.ToString(),
callerClaims: claimsExtractor.Extract(),
customHeaders: _auditHeaderReader.Read(httpContext));
customHeaders: customHeaders);
}
}

private void CheckForCustomAuditHeadersInResponse(HttpContext httpContext, Dictionary<string, string> customHeaders)
{
var responseCustomHeaders = httpContext.Response.Headers.Where(x => x.Key.StartsWith(_auditConfiguration.Value.CustomAuditHeaderPrefix, StringComparison.OrdinalIgnoreCase)).ToDictionary(a => a.Key, a => a.Value.ToString());
if (responseCustomHeaders.Any())
{
var largeHeaders = responseCustomHeaders.Where(x => x.Value.Length > AuditConstants.MaximumLengthOfCustomHeader).ToDictionary(a => a.Key, a => a.Value.ToString());
if (largeHeaders.Any())
{
throw new AuditHeaderTooLargeException(largeHeaders.First().Key, largeHeaders.First().Value.Length);
}

foreach (var header in responseCustomHeaders)
{
var headerValue = header.Value.ToString();
if (headerValue.Length > AuditConstants.MaximumLengthOfCustomHeader)
{
throw new AuditHeaderTooLargeException(header.Key, headerValue.Length);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

AuditHeaderTooLargeException

Why we check it twice?

}

if (!customHeaders.ContainsKey(header.Key))
{
customHeaders.Add(header.Key, headerValue);
}
}

if (customHeaders.Count > AuditConstants.MaximumNumberOfCustomHeaders)
{
throw new AuditHeaderCountExceededException(customHeaders.Count);
}
}
}
}
Expand Down
1 change: 1 addition & 0 deletions src/Microsoft.Health.Fhir.Core/Features/KnownHeaders.cs
Original file line number Diff line number Diff line change
Expand Up @@ -26,5 +26,6 @@ internal static class KnownHeaders
public const string PartiallyIndexedParamsHeaderName = "x-ms-use-partial-indices";
public const string EnableChainSearch = "x-ms-enable-chained-search";
public const string ProfileValidation = "x-ms-profile-validation";
public const string CustomAuditHeaderPrefix = "X-MS-AZUREFHIR-AUDIT-";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -11,10 +11,13 @@
using MediatR;
using Microsoft.Azure.Cosmos;
using Microsoft.Extensions.Logging.Abstractions;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;
using Microsoft.Health.Abstractions.Exceptions;
using Microsoft.Health.Core.Configs;
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Fhir.Core.Exceptions;
using Microsoft.Health.Fhir.Core.Features;
using Microsoft.Health.Fhir.Core.Features.Context;
using Microsoft.Health.Fhir.CosmosDb.Features.Metrics;
using Microsoft.Health.Fhir.CosmosDb.Features.Queries;
Expand All @@ -31,6 +34,7 @@ public class CosmosResponseProcessorTests
private readonly CosmosResponseProcessor _cosmosResponseProcessor;
private readonly IMediator _mediator;
private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor;
private readonly IOptions<AuditConfiguration> _optionsAuditConfiguration;

public CosmosResponseProcessorTests()
{
Expand All @@ -42,8 +46,14 @@ public CosmosResponseProcessorTests()

_mediator = Substitute.For<IMediator>();
var nullLogger = NullLogger<CosmosResponseProcessor>.Instance;
var auditConfiguration = new AuditConfiguration()
{
CustomAuditHeaderPrefix = KnownHeaders.CustomAuditHeaderPrefix,
};

_cosmosResponseProcessor = new CosmosResponseProcessor(_fhirRequestContextAccessor, _mediator, Substitute.For<ICosmosQueryLogger>(), nullLogger);
_optionsAuditConfiguration = Substitute.For<IOptions<AuditConfiguration>>();
_optionsAuditConfiguration.Value.Returns(auditConfiguration);
_cosmosResponseProcessor = new CosmosResponseProcessor(_fhirRequestContextAccessor, _mediator, Substitute.For<ICosmosQueryLogger>(), _optionsAuditConfiguration, nullLogger);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,10 @@
using MediatR;
using Microsoft.Azure.Cosmos;
using Microsoft.Extensions.Logging;
using Microsoft.Extensions.Options;
using Microsoft.Extensions.Primitives;
using Microsoft.Health.Abstractions.Exceptions;
using Microsoft.Health.Core.Configs;
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Fhir.Core.Features.Context;
using Microsoft.Health.Fhir.CosmosDb.Features.Metrics;
Expand All @@ -27,18 +29,21 @@ public class CosmosResponseProcessor : ICosmosResponseProcessor
private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor;
private readonly IMediator _mediator;
private readonly ICosmosQueryLogger _queryLogger;
private readonly IOptions<AuditConfiguration> _auditConfiguration;
private readonly ILogger<CosmosResponseProcessor> _logger;

public CosmosResponseProcessor(RequestContextAccessor<IFhirRequestContext> fhirRequestContextAccessor, IMediator mediator, ICosmosQueryLogger queryLogger, ILogger<CosmosResponseProcessor> logger)
public CosmosResponseProcessor(RequestContextAccessor<IFhirRequestContext> fhirRequestContextAccessor, IMediator mediator, ICosmosQueryLogger queryLogger, IOptions<AuditConfiguration> auditConfiguration, ILogger<CosmosResponseProcessor> logger)
{
EnsureArg.IsNotNull(fhirRequestContextAccessor, nameof(fhirRequestContextAccessor));
EnsureArg.IsNotNull(mediator, nameof(mediator));
EnsureArg.IsNotNull(queryLogger, nameof(queryLogger));
EnsureArg.IsNotNull(queryLogger, nameof(queryLogger));
EnsureArg.IsNotNull(logger, nameof(logger));

_fhirRequestContextAccessor = fhirRequestContextAccessor;
_mediator = mediator;
_queryLogger = queryLogger;
_auditConfiguration = auditConfiguration;
_logger = logger;
}

Expand Down Expand Up @@ -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);
Copy link
Contributor

@Ivanidzo4ka Ivanidzo4ka Aug 19, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_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?

}

var cosmosMetrics = new CosmosStorageRequestMetricsNotification(requestContext.AuditEventType, requestContext.ResourceType)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -7,10 +7,13 @@
using System.Collections.Generic;
using System.Net;
using Microsoft.AspNetCore.Http;
using Microsoft.Extensions.Options;
using Microsoft.Health.Api.Features.Audit;
using Microsoft.Health.Core.Configs;
using Microsoft.Health.Core.Features.Context;
using Microsoft.Health.Core.Features.Security;
using Microsoft.Health.Fhir.Api.Features.Audit;
using Microsoft.Health.Fhir.Core.Features;
using Microsoft.Health.Fhir.Core.Features.Context;
using NSubstitute;
using Xunit;
Expand All @@ -29,6 +32,7 @@ public class AuditHelperTests
private readonly RequestContextAccessor<IFhirRequestContext> _fhirRequestContextAccessor = Substitute.For<RequestContextAccessor<IFhirRequestContext>>();
private readonly IAuditLogger _auditLogger = Substitute.For<IAuditLogger>();
private readonly IAuditHeaderReader _auditHeaderReader = Substitute.For<IAuditHeaderReader>();
private readonly IOptions<AuditConfiguration> _optionsAuditConfiguration;

private readonly IFhirRequestContext _fhirRequestContext = Substitute.For<IFhirRequestContext>();

Expand All @@ -49,7 +53,14 @@ public AuditHelperTests()

_claimsExtractor.Extract().Returns(Claims);

_auditHelper = new AuditHelper(_fhirRequestContextAccessor, _auditLogger, _auditHeaderReader);
var auditConfiguration = new AuditConfiguration()
{
CustomAuditHeaderPrefix = KnownHeaders.CustomAuditHeaderPrefix,
};

_optionsAuditConfiguration = Substitute.For<IOptions<AuditConfiguration>>();
_optionsAuditConfiguration.Value.Returns(auditConfiguration);
_auditHelper = new AuditHelper(_fhirRequestContextAccessor, _auditLogger, _auditHeaderReader, _optionsAuditConfiguration);
}

[Fact]
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ public async Task<IActionResult> Create([FromBody] Resource resource)
[HttpPost]
[ConditionalConstraint]
[Route(KnownRoutes.ResourceType)]
[AuditEventType(AuditEventSubType.Create)]
[AuditEventType(AuditEventSubType.ConditionalCreate)]
public async Task<IActionResult> ConditionalCreate([FromBody] Resource resource)
{
StringValues conditionalCreateHeader = HttpContext.Request.Headers[KnownHeaders.IfNoneExist];
Expand Down Expand Up @@ -207,7 +207,7 @@ public async Task<IActionResult> Update([FromBody] Resource resource, [ModelBind
/// <param name="resource">The resource.</param>
[HttpPut]
[Route(KnownRoutes.ResourceType)]
[AuditEventType(AuditEventSubType.Update)]
[AuditEventType(AuditEventSubType.ConditionalUpdate)]
public async Task<IActionResult> ConditionalUpdate([FromBody] Resource resource)
{
IReadOnlyList<Tuple<string, string>> conditionalParameters = GetQueriesForSearch();
Expand Down Expand Up @@ -372,7 +372,7 @@ public async Task<IActionResult> Delete(string typeParameter, string idParameter
/// <param name="idParameter">The identifier.</param>
[HttpDelete]
[Route(KnownRoutes.PurgeHistoryResourceTypeById)]
[AuditEventType(AuditEventSubType.Delete)]
[AuditEventType(AuditEventSubType.PurgeHistory)]
public async Task<IActionResult> PurgeHistory(string typeParameter, string idParameter)
{
DeleteResourceResponse response = await _mediator.DeleteResourceAsync(new ResourceKey(typeParameter, idParameter), DeleteOperation.PurgeHistory, HttpContext.RequestAborted);
Expand All @@ -388,7 +388,7 @@ public async Task<IActionResult> PurgeHistory(string typeParameter, string idPar
/// <param name="maxDeleteCount">Specifies the maximum number of resources that can be deleted.</param>
[HttpDelete]
[Route(KnownRoutes.ResourceType)]
[AuditEventType(AuditEventSubType.Delete)]
[AuditEventType(AuditEventSubType.ConditionalDelete)]
public async Task<IActionResult> ConditionalDelete(string typeParameter, [FromQuery] bool hardDelete, [FromQuery(Name = KnownQueryParameterNames.Count)] int? maxDeleteCount)
{
IReadOnlyList<Tuple<string, string>> conditionalParameters = GetQueriesForSearch();
Expand Down
8 changes: 8 additions & 0 deletions src/Microsoft.Health.Fhir.ValueSets/AuditEventSubType.cs
Original file line number Diff line number Diff line change
Expand Up @@ -14,14 +14,20 @@ public static class AuditEventSubType

public const string Create = "create";

public const string ConditionalCreate = "conditional-create";

public const string Read = "read";

public const string VRead = "vread";

public const string Update = "update";

public const string ConditionalUpdate = "conditional-update";

public const string Delete = "delete";

public const string ConditionalDelete = "conditional-delete";

public const string History = "history";

public const string HistoryType = "history-type";
Expand Down Expand Up @@ -66,5 +72,7 @@ public static class AuditEventSubType
public const string MemberMatch = "member-match";

public const string Everything = "everything";

public const string PurgeHistory = "purge-history";
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
using Microsoft.Health.Api.Features.Audit;
using Microsoft.Health.Fhir.Client;
using Microsoft.Health.Fhir.Core.Extensions;
using Microsoft.Health.Fhir.Core.Features;
using Microsoft.Health.Fhir.Tests.Common;
using Microsoft.Health.Fhir.Tests.Common.FixtureParameters;
using Microsoft.Health.Fhir.Tests.E2E.Common;
Expand All @@ -30,7 +31,6 @@ namespace Microsoft.Health.Fhir.Tests.E2E.Rest.Audit
public class AuditTests : IClassFixture<AuditTestFixture>
{
private const string RequestIdHeaderName = "X-Request-Id";
private const string CustomAuditHeaderPrefix = "X-MS-AZUREFHIR-AUDIT-";
private const string ExpectedClaimKey = "client_id";

private readonly AuditTestFixture _fixture;
Expand Down Expand Up @@ -364,15 +364,15 @@ public async Task GivenASmartOnFhirRequest_WhenTokenIsCalled_TheAuditLogEntriesS
};

var content = new FormUrlEncodedContent(formFields);
content.Headers.Add(CustomAuditHeaderPrefix + "test", "test");
content.Headers.Add(KnownHeaders.CustomAuditHeaderPrefix + "test", "test");
await ExecuteAndValidate(
async () => await _client.HttpClient.PostAsync(pathSegment, content),
"smart-on-fhir-token",
pathSegment,
HttpStatusCode.BadRequest,
"1234",
"client_id",
new Dictionary<string, string>() { [CustomAuditHeaderPrefix + "test"] = "test" });
new Dictionary<string, string>() { [KnownHeaders.CustomAuditHeaderPrefix + "test"] = "test" });
}

[Fact]
Expand Down
Loading