microsoft-authentication-library-for-dotnet icon indicating copy to clipboard operation
microsoft-authentication-library-for-dotnet copied to clipboard

Published 20 hours ago • verified publisher icon AzureAD

Adding IsProofOfPosessionSupportedByClient api

Open trwalke opened this issue 3 years ago • 1 comments
trafficstars

Fixes # fix for https://github.com/AzureAD/microsoft-authentication-library-for-dotnet/issues/3496

Changes proposed in this request Adding api for IsProofOfPosessionSupportedByClient to be used to determine if the current broker is able to support POP

Testing Unit test

Performance impact N/A

Documentation

  • [ ] All relevant documentation is updated.

trwalke avatar Aug 09 '22 16:08 trwalke

Spec also mentions to throw an exception with the recommendation to use IsPopSupported method if incorrect usage of WithPop is detected.

pmaytak avatar Aug 10 '22 07:08 pmaytak

Spec also mentions to throw an exception with the recommendation to use IsPopSupported method if incorrect usage of WithPop is detected.

@pmaytak I dont think I see that In the spec. There is something related to that in the appendix for the higher level api but it is not required for the low level api proposed in proposal 2

trwalke avatar Aug 16 '22 06:08 trwalke

Spec also mentions to throw an exception with the recommendation to use IsPopSupported method if incorrect usage of WithPop is detected.

@pmaytak I dont think I see that In the spec. There is something related to that in the appendix for the higher level api but it is not required for the low level api proposed in proposal 2

@trwalke Don't exactly remember what I was referring to, but maybe it was the second acceptance test. Currently in WithPop methods we just throw general exception that Pop is not supported. We should update those error messages methods asking developer to use this new API.

pmaytak avatar Aug 17 '22 06:08 pmaytak

Spec also mentions to throw an exception with the recommendation to use IsPopSupported method if incorrect usage of WithPop is detected.

@pmaytak I dont think I see that In the spec. There is something related to that in the appendix for the higher level api but it is not required for the low level api proposed in proposal 2

@trwalke Don't exactly remember what I was referring to, but maybe it was the second acceptance test. Currently in WithPop methods we just throw general exception that Pop is not supported. We should update those error messages methods asking developer to use this new API.

@pmaytak OK, makes sense now. Its not necessarily throwing an exception but informing developers to use the new api via documentation and exception error messages.

trwalke avatar Aug 17 '22 22:08 trwalke

Did some additional thinking on this and I think there are a few more exceptions that need to be thrown to satisfy the exception tests.

Acceptance tests

  • Try to acquire a POP token on Win 11 when broker is enabled. Expect: pop token
  • Try to acquire a pop token on Win 11 when broker is not enabled. Expect: error asking developer to use IsProofOfPosessionSupportedByClient, which would return false.
  • Try to acquire a pop token on Win 11 when using an {ADFS, B2C} authority. Expect: error as above
  • Try to acquire POP token on {Mac, Linux}. Expect: error as above

Specifically, the last 2. I made some updates to reflect that.

@bgavrilMS @pmaytak

trwalke avatar Aug 31 '22 15:08 trwalke