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

Specifying and validating nonce in auth code flow #173

Merged
merged 3 commits into from Mar 23, 2020
Merged

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented Mar 18, 2020

The nonce is a behavior defined in Open ID Connect.

This PR adds a nonce parameter into the get_authorization_request_url(..., nonce=...) so that it would be sent to Azure AD.

This PR also adds a nonce parameter into the acquire_token_by_authorization_code(..., nonce=...) so that this method will perform the nonce check on the returned id token, automatically. We test it here. By the way, validating nonce has been embedded in msal .net, although it happens implicitly when using public client application.

@henrik-me
Copy link

Suggestion, add functionality which combines the two steps into one?

msal/application.py Outdated Show resolved Hide resolved
@henrik-me
Copy link

    return self.client.obtain_token_by_authorization_code(

does this already do nonce validation?


Refers to: msal/application.py:332 in ea137ae. [](commit_id = ea137ae, deletion_comment = False)

Copy link
Collaborator Author

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Thanks for the review!

Answering two high level questions here:

does this already do nonce validation?

   return self.client.obtain_token_by_authorization_code(

The answer is yes. In fact we changed unit test case this time and ran test to confirm that. This PR is mainly about the public API surface change.

Suggestion, add functionality which combines the two steps into one?

We will, but not in this PR. Combining the two steps is essentially the other task in backlog of "providing acquire_token_interactive()".

And the 3rd question is answered below, inline.

msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
tests/test_e2e.py Outdated Show resolved Hide resolved
Copy link
Contributor

@abhidnya13 abhidnya13 left a comment

Choose a reason for hiding this comment

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

Thanks @rayluo for the efforts 👍

@rayluo rayluo merged commit 7856e8a into dev Mar 23, 2020
@rayluo rayluo deleted the nonce-in-msal branch March 23, 2020 22:52
@rayluo rayluo mentioned this pull request Mar 31, 2020
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

3 participants