Skip to content
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

Adding IsProofOfPosessionSupportedByClient api #3581

Merged
merged 21 commits into from
Sep 2, 2022
Merged

Conversation

trwalke
Copy link
Member

@trwalke trwalke commented Aug 9, 2022

Fixes #
fix for #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.

Copy link
Contributor

@gladjohn gladjohn left a comment

Choose a reason for hiding this comment

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

currently for the desktop broker we show "The broker does not support Proof-of-Possession on the current platform.", we can extend this message asking developers to adopt to the new broker if they need PoP support.

Copy link
Contributor

@pmaytak pmaytak left a comment

Choose a reason for hiding this comment

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

Breaking change updating the public IPCA interface. Was that discussed and okay'ed?

@pmaytak
Copy link
Contributor

pmaytak commented Aug 10, 2022

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

Copy link
Member

@bgavrilMS bgavrilMS left a comment

Choose a reason for hiding this comment

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

Implementation of helper needs changes.

trwalke and others added 6 commits August 15, 2022 17:08
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
Co-authored-by: Gladwin Johnson <90415114+gladjohn@users.noreply.github.com>
# Conflicts:
#	src/client/Microsoft.Identity.Client/IPublicClientApplication.cs
#	src/client/Microsoft.Identity.Client/PublicClientApplication.cs
@trwalke
Copy link
Member Author

trwalke commented Aug 16, 2022

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

@pmaytak
Copy link
Contributor

pmaytak commented Aug 17, 2022

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.

@trwalke
Copy link
Member Author

trwalke commented Aug 17, 2022

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 trwalke enabled auto-merge (squash) August 31, 2022 06:38
@trwalke
Copy link
Member Author

trwalke commented Aug 31, 2022

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 trwalke merged commit 5d550f4 into main Sep 2, 2022
@trwalke trwalke deleted the trwalke/IsPopSupported branch September 2, 2022 00:17
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.

None yet

5 participants