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.

Sign up for GitHub

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

chore: Add unsuccessfull response content logging #1324

Closed
wants to merge 1 commit into from
Closed

chore: Add unsuccessfull response content logging #1324

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
3 changes: 2 additions & 1 deletion src/Digdir.Domain.Dialogporten.Infrastructure/HttpClientExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,8 @@ public static async Task<HttpResponseMessage> PostAsJsonEnsuredAsync(
configureHeaders?.Invoke(httpRequestMessage.Headers);
configureContentHeaders?.Invoke(httpRequestMessage.Content.Headers);
var response = await client.SendAsync(httpRequestMessage, cancellationToken);
response.EnsureSuccessStatusCode();
await response.EnsureSuccessStatusCodeWithContent();
Copy link
Collaborator

Choose a reason for hiding this comment

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

Det er litt skummelt å logge response body for alle kontekster. Meg bekjent brukes denne extension for mer enn bare altinn auth, og det å logge respons (som kan være sensitiv) som en del av exception er litt så-som-så. Vi kan heller sette "Serilog__MinimumLevel__Override__Digdir.Domain.Dialogporten.Infrastructure.Altinn.Authorization.AltinnAuthorizationClient": "Debug" i stage. Det har jeg gjort gjennom app configuration allerede.

Copy link
Member Author

Choose a reason for hiding this comment

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

Enig i at dette i utgangspunktet er litt sketch. Vil det å overridde loglevel her gjøre at vi får logga response body?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Nei, vi skal (ut i fra koden) få request body som vil si at vi kan reprodusere selv via postman.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Men det viktige er at vi får request body for kun denne konteksten så lenge vi har på debug logging

Copy link
Member Author

Choose a reason for hiding this comment

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

Ok, hvis vi skal lene oss på ILogger (som jeg er enig i gir mening), må vi implementere dette et annet sted enn i disse extension metodene. Evt må vi hekte på content i exceptionen på noe vis og hente det ut og logge det et annet sted i pipelinen.


return response;
}
catch (Exception e)
Expand Down
35 changes: 35 additions & 0 deletions src/Digdir.Domain.Dialogporten.Infrastructure/HttpResponseMessageExtensions.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
namespace Digdir.Domain.Dialogporten.Infrastructure;

public static class HttpResponseMessageExtensions
{
private const int MaxContentLength = 1000;

public static async Task<HttpResponseMessage> EnsureSuccessStatusCodeWithContent(this HttpResponseMessage response)
{
if (response.IsSuccessStatusCode)
{
return response;
}

var content = await response.Content.ReadAsStringAsync();
if (string.IsNullOrWhiteSpace(content))
{
// This will throw HttpRequestException with the response status code and reason phrase
return response.EnsureSuccessStatusCode();
}
Comment on lines +14 to +19
Copy link
Contributor

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

Inconsistent exception handling between empty and non-empty content cases

When the response content is empty, the method calls response.EnsureSuccessStatusCode(), which throws an HttpRequestException with standard message formatting. However, when content is present, a new HttpRequestException is thrown with a custom message. This inconsistency may lead to confusion when handling exceptions. Consider standardizing the exception handling mechanism.

Apply this diff to standardize exception handling:

-            return response.EnsureSuccessStatusCode();
+            throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var content = await response.Content.ReadAsStringAsync();
if (string.IsNullOrWhiteSpace(content))
{
// This will throw HttpRequestException with the response status code and reason phrase
return response.EnsureSuccessStatusCode();
}
var content = await response.Content.ReadAsStringAsync();
if (string.IsNullOrWhiteSpace(content))
{
// This will throw HttpRequestException with the response status code and reason phrase
throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode);
}


if (content.Length > MaxContentLength)
{
content = content[..MaxContentLength] + "[truncated after " + MaxContentLength + " characters]";
}

throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() +
" with error content: " + content, null, response.StatusCode);

}
Comment on lines +14 to +29
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Avoid including response content in exception messages to prevent sensitive data leakage

Including the response content in exception messages could inadvertently expose sensitive or confidential information if exceptions are logged or displayed. Consider removing the response content from the exception message and logging it securely instead.

Apply this diff to modify the exception handling:

-        throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() +
-                                       " with error content: " + content, null, response.StatusCode);
+        // Optionally log 'content' securely here
+        throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode);
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var content = await response.Content.ReadAsStringAsync();
if (string.IsNullOrWhiteSpace(content))
{
// This will throw HttpRequestException with the response status code and reason phrase
return response.EnsureSuccessStatusCode();
}
if (content.Length > MaxContentLength)
{
content = content[..MaxContentLength] + "[truncated after " + MaxContentLength + " characters]";
}
throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() +
" with error content: " + content, null, response.StatusCode);
}
var content = await response.Content.ReadAsStringAsync();
if (string.IsNullOrWhiteSpace(content))
{
// This will throw HttpRequestException with the response status code and reason phrase
return response.EnsureSuccessStatusCode();
}
if (content.Length > MaxContentLength)
{
content = content[..MaxContentLength] + "[truncated after " + MaxContentLength + " characters]";
}
// Optionally log 'content' securely here
throw new HttpRequestException("Response unsuccessful (" + response.GetResponseCodeWithReasonPhrase() + ")", null, response.StatusCode);
}


private static string GetResponseCodeWithReasonPhrase(this HttpResponseMessage response) =>
string.IsNullOrWhiteSpace(response.ReasonPhrase)
? response.StatusCode.ToString()
: response.StatusCode + " " + response.ReasonPhrase;
}
Loading