AntiForgery Interfaces. #180
Conversation
| private static readonly AntiForgeryWorker _worker = CreateSingletonAntiForgeryWorker(); | ||
| private static readonly string _purpose = "Microsoft.AspNet.Mvc.AntiXsrf.AntiForgeryToken.v1" ; | ||
|
|
||
| private static AntiForgeryWorker CreateSingletonAntiForgeryWorker() |
There was a problem hiding this comment.
I imagine these would come from the DI and we'd register this as a singleton in the DI rather than have statics here?
There was a problem hiding this comment.
Yup there is a todo for it
There was a problem hiding this comment.
This should go in this PR
There was a problem hiding this comment.
@harshgMSFT could you summarize what will be placed in DI and the scenarios where users would override our default implementations?
|
current code doesn't contain any |
|
@dougbu .. We discussed and we thought it is better for the class to remain a sealed class, and not be introduced in the DI System. The only scenarios where people want extensibility, is |
Also contains some code ported over. This Commit is only for review purpose.
|
|
||
| public AntiForgeryToken GetFormToken(HttpContext httpContext) | ||
| { | ||
| string value = httpContext.Request.GetFormAsync().Result[_config.FormFieldName]; |
There was a problem hiding this comment.
Don't block while reading the form.
Remaining: 1. Generate the AntiForgeryCookieName. 2. Update the ClaimsUidExtractor.
| { | ||
| public sealed class AntiForgeryConfigWrapper : IAntiForgeryConfig | ||
| { | ||
| public IAntiForgeryAdditionalDataProvider AdditionalDataProvider |
There was a problem hiding this comment.
Why isn't this just a service?
|
@GrabYourPitchforks BranchUpdated. |
| /// <param name="context">The http context associated with the current call.</param> | ||
| /// <param name="cookieToken">The token that was supplied in the request cookie.</param> | ||
| /// <param name="formToken">The token that was supplied in the request form body.</param> | ||
| [EditorBrowsable(EditorBrowsableState.Advanced)] |
There was a problem hiding this comment.
Why are we adding these attributes, they should generally be avoided and there has to be a really good reason do add them (which I've yet to ever see except for Obsoleted methods)?
There was a problem hiding this comment.
I'm fine removing these. We can always go back during API reviews and add these back (if at all).
| /// <param name="oldCookieToken">The anti-forgery token - if any - that already existed | ||
| /// for this request. May be null. The anti-forgery system will try to reuse this cookie | ||
| /// value when generating a matching form token.</param> | ||
| /// </remarks> |
There was a problem hiding this comment.
where is the opening of the tag
There was a problem hiding this comment.
Thanks for catching that!
| /// <returns>An HTML string corresponding to an <input type="hidden"> | ||
| /// element. This element should be put inside a <form>.</returns> | ||
| /// <remarks> | ||
| /// This method has a side effect: A response cookie is set if there is no valid cookie associated with the request. |
There was a problem hiding this comment.
line length.
Generally if there is a scroll bar when you set up the PR, I'm going to go and comment on it.
No description provided.