Fix AuthenticationParamters #1602
Conversation
| @@ -154,7 +159,8 @@ private async Task<AuthenticationParameters> CreateFromResourceUrlCommonAsync(Ur | |||
|
|
|||
| try | |||
| { | |||
| await _httpManager.SendGetAsync( | |||
| HttpManager httpManager = new HttpManager(); | |||
There was a problem hiding this comment.
will this mean that we're getting a different HttpClient than the default one? I need to dig into the httpmanager constructor to validate. I"m mainly concerned if someone has decorated their httpclientfactory with proxy information then it might not get through here.
There was a problem hiding this comment.
This is fine, because this is a completely separate helper class, has nothing to do with ADAL. You're not even creating an AuthenticationContext, so a custom HttpClient can't even be used.
If we weren't already in on a stable version, I would propose to remove this API completely.
There was a problem hiding this comment.
To be honest, an HttpManager isn't even needed here (see how many nulls are used in initialization) - I could create a simple HttpClient to make a GET call. The only minor advantage of HttpManager is a bit of extra logging.
|
Can we introduce a factory method instead of restructuring? |
| /// </summary> | ||
| /// <param name="resourceUrl">Address of the resource</param> | ||
| /// <returns>AuthenticationParameters object containing authentication parameters</returns> | ||
| public async Task<AuthenticationParameters> CreateFromResourceUrlAsync(Uri resourceUrl) | ||
| /// <remarks>Most protected APIs, including those owned by Microsoft, no longer advertise a resource. Authentication should be done using MSAL, which uses scopes. See https://aka.ms/msal-net-migration-adal-msal </remarks> |
There was a problem hiding this comment.
@henrik-me @jmprieur - how about I add these "remarks" to a nice Obsolete message instead? :D
There was a problem hiding this comment.
Makes sense to me... except some might be using it who will not be able to if it's marked as obsolete
In reply to: 284784777 [](ancestors = 284784777)
| throw new ArgumentNullException("resourceUrl"); | ||
| } | ||
|
|
||
| HttpClient httpClient = new HttpClient(); |
There was a problem hiding this comment.
httpClient [](start = 23, length = 10)
not sure why we would not want to allow passing in a httpclient, allowing re-use and proxy, etc. ...
Fix for #1599