Skip to content

AT Proof-Of-Possession #2: Handle proof-of-possession tokens correctly in responses #2153

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

Merged
merged 19 commits into from
Aug 28, 2020

Conversation

pkanher617
Copy link
Contributor

This PR is a follow up to PR #2151, and implements the response handling part of the proof-of-possession flow in msal-common.

This PR specifically adds the following:

  • Renaming IdToken and IdTokenClaims to more generic class names
  • Update request objects to include the resource request method and URI, and pass these to the sign token method
  • Adds token signing interface method
  • Updates cache method to include token type
  • Updates response handling flow to handle proof-of-possession flow

Copy link
Collaborator

@tnorling tnorling left a comment

Choose a reason for hiding this comment

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

Overall looks good. Just a couple more places to update references to id tokens. Also please take a look at the comments and update where necessary as there are lots that would be left outdated with these changes.

@github-actions github-actions bot added the msal-browser Related to msal-browser package label Aug 18, 2020
Copy link
Contributor

@sangonzal sangonzal left a comment

Choose a reason for hiding this comment

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

Looks good. Would be good to have some tests.

@@ -34,12 +34,10 @@ export class RefreshTokenClient extends BaseClient {
);

responseHandler.validateTokenResponse(response.body);
const tokenResponse = responseHandler.handleServerTokenResponse(
return await responseHandler.handleServerTokenResponse(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this also pass in request.resourceRequestMethod and request.resourceRequestUri given that they were added to RefreshTokenRequest?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've handled this in a later PR, this PR only covers the authorization code 2 legged flow, not the refresh flow.

@pkanher617 pkanher617 changed the base branch from pop-req-thumbprint to pop August 20, 2020 22:13
@pkanher617 pkanher617 merged commit 9b1144f into pop Aug 28, 2020
@pkanher617 pkanher617 deleted the pop-handle-response branch October 2, 2020 18:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature-pop msal-browser Related to msal-browser package msal-common Related to msal-common package samples Related to the samples apps for the library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants