Log FhirOperation linked to anonymous calls to Request metrics #2295
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…sOperationType to AuditLogs and Fixed failing audit tests
…calls-to-requestMetric
…calls-to-requestMetric # Conflicts: # Directory.Build.props
Fix failing unit tests
LTA-Thinking
reviewed
Nov 3, 2021
src/Microsoft.Health.Fhir.ValueSets/FhirAnonymousOperationType.cs
Outdated
Show resolved
Hide resolved
LTA-Thinking
reviewed
Nov 3, 2021
test/Microsoft.Health.Fhir.Shared.Tests.E2E/Rest/Audit/AuditTests.cs
Outdated
Show resolved
Hide resolved
LTA-Thinking
reviewed
Nov 3, 2021
LTA-Thinking
reviewed
Nov 3, 2021
src/Microsoft.Health.Fhir.Shared.Api.UnitTests/Features/Audit/AuditHelperTests.cs
Show resolved
Hide resolved
| /// <summary> | ||
| /// Value set for Fhir operations which do not require authorization | ||
| /// </summary> | ||
| public static class FhirAnonymousOperationType |
There was a problem hiding this comment.
This doesn't look like a FHIR ValueSet
There was a problem hiding this comment.
hm right, moved to Fhir.Api.Features.AnonymousOperations. Let me know if there is a better place
| /// Return all the values of constants of the specified type | ||
| /// </summary> | ||
| /// <returns>List of constant values</returns> | ||
| public static IList<string> GetConstants() |
There was a problem hiding this comment.
nit: ambiguous name, perhaps GetAnonymousOperations.
The data type also looks like it could be Lazy<List<string>>...to be clearer, private type Lazy<List<string>> exposed type is still IList<string>, this could then be converted to a property
There was a problem hiding this comment.
Updated the PR as per suggestions
LTA-Thinking
reviewed
Nov 4, 2021
Added Lazy value initialization for _fhirAnonymousOperationTypeList
LTA-Thinking
approved these changes
Nov 5, 2021
|
/azp run |
|
Pull request contains merge conflicts. |
…log-anonymous-endpoint-calls-to-requestMetric # Conflicts: # src/Microsoft.Health.Fhir.Api/Features/Audit/AuditHelper.cs
…log-anonymous-endpoint-calls-to-requestMetric
apurvabhaleMS
deleted the
personal/apurvabhale/log-anonymous-endpoint-calls-to-requestMetric
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
We use audit infrastructure to log into RequestMetric due to which we currently don't log operations for endpoint that does not require authentication. In RequestMetrics, AuditEventType is logged as FhirOperation. For anonymous endpoint calls like Metadata or Versions we log {"Authentication":"","Operation":"","ResourceType":""} in RequestMetric table.
With this PR, we added new FhirAnonymousOperationAttribute which extends AllowAnonymousAttribute (making sure these calls are still marked as anonymous calls) and gives us the flexibility to provide the Fhir operation which will be used as AuditEventType/FhirOperation type in RequestMetrics log. After this change we should see an entry as {"Authentication":"","Operation":"metadata","ResourceType":""} for metadata call, in request metric. This PR also ensures that extra audit log entries are not created for anonymous calls like metadata/$versions
Related issues
Addresses [74172].
Testing
FHIR Team Checklist
Semver Change (docs)
Patch|Skip|Feature|Breaking (reason)