-
Notifications
You must be signed in to change notification settings - Fork 8
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
Fix #11, Implement setting X-Audit-Log-Reason header #12
base: master
Are you sure you want to change the base?
Fix #11, Implement setting X-Audit-Log-Reason header #12
Conversation
…methods adds a header which by default is not included for all client methods with a corresponding audit log entry type
@@ -43,9 +43,9 @@ internal interface IDiscordRestApi : IDisposable | |||
/// If modifying a category, individual Channel Update events will fire for each child channel that also changes. | |||
/// </summary> | |||
[Patch("channels/{channelId}")] | |||
Task<Channel> ModifyChannelAsync([Path] Snowflake channelId, [Body] ModifyChannelParams args); | |||
Task<Channel> ModifyChannelAsync([Path] Snowflake channelId, [Body] ModifyChannelParams args, [Header(WumpusRestClient.ReasonHeader)] string reason = null); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think moving the header declaration to be a property akin to the authorization header would be more appropriate than passing it in as a method parameter. This would make it accessible immediately to any new endpoints that arise that can utilize it for audit logging.
The client would need to null out this property after every request though if changed to this, as not to reuse the same header value between requests, and...I'm not quite sure where that logic should go now that I look at what's possible with RestEase.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Given that this property may change very frequently, I think this approach would also require considering thread saftey
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Forgot to consider that. Do you know if the endpoints you've added the header param for are the only ones that actually do anything if you specify the audit log header?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I went off of the list of events in the Audit Log documentation and tried to match each one. I have not tested each one individually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These should probably be Utf8String? Also testing is needed before it can be approved.
Currently I don't know of a good way to test this behavior without making the mock REST server return something that it shouldn't. The server and these tests don't have any state, so we can't just test them with the I've been able to at least check that this header is being sent as part of the request by mangling one of the tests and it's controller (in addition to using debugging tools): [HttpDelete("channels/{channelId}")]
public async Task<IActionResult> DeleteChannelAsync(Snowflake channelId)
{
Utf8String reason = null;
if (Request.Headers.ContainsKey("X-Audit-Log-Reason"))
{
reason = new Utf8String(Request.Headers["X-Audit-Log-Reason"]);
}
return Ok(new Channel
{
Id = channelId,
Name = reason
});
} [Fact]
public void DeleteChannelAsync()
{
RunTest(c => c.DeleteChannelAsync(123, new Utf8String("This is my reason")), x =>
{
Assert.Equal(123UL, x.Id.RawValue);
Assert.Equal("This is my reason", x.Name.ToString());
});
} This way I can see that the header is sent, but of course it modifies the output from what we should be expecting. If anyone has any suggestions I'd like to know how this could be tested better. |
Sorry, I specifically meant about your comment |
Fix #11
This adds an optional
reason
parameter to all relevant methods (those with corresponding event Audit Log event types) to the rest API tasks. When this parameter is not null, it's value will be included as part of this header.I had questions about a few implementation details: