Skip to content

Add Pop support to wwwAuthenticateParameters#3436

Merged
trwalke merged 35 commits into
mainfrom
trwalke/wwwAuthenticatePop
Dec 14, 2022
Merged

Add Pop support to wwwAuthenticateParameters#3436
trwalke merged 35 commits into
mainfrom
trwalke/wwwAuthenticatePop

Conversation

@trwalke

@trwalke trwalke commented Jun 30, 2022

Copy link
Copy Markdown
Member

Fixes #3026

Changes proposed in this request
Adding POP support to wwwAuthenticateParameters
Revising api to support multiple WWW-Authenticate headers.
Adding support for Authentication-Info headers

Obsolete apis can be replaced with apis in the column on the immediate right except for CreateFromWwwAuthenticateHeaderValue(string wwwAuthenticateValue) which is completely deprecated. Developers should use CreateFromAuthenticationHeaders(HttpResponseHeaders httpResponseHeaders) instead.

Obsolete APIs New APIs for Multiple Schemes (returns list of parsed schemes) New APIs for Single Schemes
CreateFromResourceResponseAsync(string resourceUri) CreateFromAuthenticationResponseAsync(string resourceUri) CreateFromAuthenticationResponseAsync(string resourceUri, string scheme)
CreateFromResourceResponseAsync(string resourceUri, CancellationToken cancellationToken = default) CreateFromAuthenticationResponseAsync(string resourceUri, CancellationToken cancellationToken) CreateFromAuthenticationResponseAsync(string resourceUri, string scheme, CancellationToken cancellationToken)
CreateFromResourceResponseAsync(HttpClient httpClient, string resourceUri, CancellationToken cancellationToken = default) CreateFromAuthenticationResponseAsync(string resourceUri, HttpClient httpClient, CancellationToken cancellationToken) CreateFromAuthenticationResponseAsync(string resourceUri, string scheme, HttpClient httpClient, CancellationToken cancellationToken)
WwwAuthenticateParameters CreateFromResponseHeaders(HttpResponseHeaders httpResponseHeaders, string scheme = "Bearer") CreateFromAuthenticationHeaders(HttpResponseHeaders httpResponseHeaders) CreateFromAuthenticationHeaders(HttpResponseHeaders httpResponseHeaders, string scheme)
CreateFromWwwAuthenticateHeaderValue(string wwwAuthenticateValue)

Testing
Unit testing
integration testing

API review

Performance impact

Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated

@jmprieur jmprieur left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I left a few comments

Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
@trwalke trwalke closed this Jul 7, 2022
trwalke added 2 commits July 14, 2022 21:10
refactoring www auth api
@trwalke trwalke reopened this Jul 20, 2022
@trwalke trwalke linked an issue Jul 20, 2022 that may be closed by this pull request
7 tasks
@trwalke trwalke marked this pull request as ready for review July 20, 2022 21:21
@trwalke trwalke closed this Jul 22, 2022
@trwalke trwalke reopened this Jul 22, 2022
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread tests/Microsoft.Identity.Test.Unit/WwwAuthenticateParametersTests.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs Outdated
Comment thread tests/Microsoft.Identity.Test.Unit/WwwAuthenticateParametersTests.cs Outdated
Comment thread tests/Microsoft.Identity.Test.Unit/WwwAuthenticateParametersTests.cs Outdated
trwalke added 2 commits August 2, 2022 15:57
@trwalke

trwalke commented Aug 4, 2022

Copy link
Copy Markdown
Member Author

next iteration will change IEnumerable<WwwAuthenticateParameters>> to Dictionary<string, WwwAuthenticateParameters>> where string is the scheme

Comment thread src/client/Microsoft.Identity.Client/MsalErrorMessage.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AuthenticationInfoParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AuthenticationInfoParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AuthenticationInfoParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AuthenticationInfoParameters.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AuthenticationInfoParameters.cs Outdated

@bgavrilMS bgavrilMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

  1. "token68" completely throws your parser off
  2. WWWAuth and AuthInfo parsers behave differently when the header is not there
  3. POPNonce is a bad name for a generic param
  4. Exceptions are not specific enough.

Have you considered abandoning the custom parsing logic you have and adopting the MSAL C++ parser instead?

Comment thread src/client/Microsoft.Identity.Client/AuthenticationHeaderParser.cs
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs
Comment thread src/client/Microsoft.Identity.Client/WwwAuthenticateParameters.cs
Comment thread tests/Microsoft.Identity.Test.Unit/WwwAuthenticateParametersTests.cs Outdated
Refactoring.
@bgavrilMS

Copy link
Copy Markdown
Member

On the public API, let's follow the Cancellation Token design guidelines from https://devblogs.microsoft.com/premier-developer/recommended-patterns-for-cancellationtoken/

`It’s a good idea to only make your CancellationToken parameters optional in your public API (if you have one) and leave them as required parameters everywhere else. This really helps to ensure that you intentionally propagate your CancellationTokens through all the methods you call (#2 above). But of course remember to switch to passing CancellationToken.None once you pass the point of no cancellation.

It’s also a good API pattern to keep your CancellationToken as the last parameter your method accepts. This fits nicely with optional parameters anyway since they have to show up after any required parameters.`

So please remove oevrloads without CancellationToken and add default value to CancellationToken , e.g.

ParseAsync(string scheme, CanceallationToken = default)

Comment thread tests/Microsoft.Identity.Test.Unit/WwwAuthenticateParametersTests.cs Outdated
Comment thread src/client/Microsoft.Identity.Client/AuthenticationInfoParameters.cs Outdated

@bgavrilMS bgavrilMS left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Aproving, but please simplify public API by allowing only methods with default cancellation token (see comment)

@bgavrilMS bgavrilMS requested a review from jmprieur December 12, 2022 16:13
trwalke added 3 commits December 12, 2022 23:57
Updating error message
…wwwAuthenticatePop

# Conflicts:
#	src/client/Microsoft.Identity.Client/MsalError.cs
@trwalke trwalke merged commit 4e96195 into main Dec 14, 2022
@trwalke trwalke deleted the trwalke/wwwAuthenticatePop branch December 14, 2022 06:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

WwwAuthenticateParameters fails to create response from headers

6 participants