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

Pass through nonce in code flow #481

Merged
merged 3 commits into from
Sep 18, 2017
Merged

Conversation

skion
Copy link
Member

@skion skion commented Jul 28, 2017

Nonces are optional in authorization_code flow, but still should be passed through if the RP provided one.

If present in the Authentication Request, Authorization Servers MUST include a nonce Claim in the ID Token

Also the claims parameter appeared to be processed twice.

@thedrow
Copy link
Collaborator

thedrow commented Jul 30, 2017

How come the tests did pass before that? This implies we're missing coverage for a code branch where nonces are not present.
Can you please add a unit test for this code branch?

@skion
Copy link
Member Author

skion commented Jul 30, 2017

Reopening accidental close...

@skion skion reopened this Jul 30, 2017
@wiliamsouza
Copy link
Member

nonce is no longer required as stated by http://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

nonce
OPTIONAL. String value used to associate a Client session with an ID Token, and to mitigate replay attacks. The value is passed through unmodified from the Authentication Request to the ID Token. Sufficient entropy MUST be present in the nonce values used to prevent attackers from guessing values. For implementation notes, see Section 15.5.2.

@skion
Copy link
Member Author

skion commented Aug 19, 2017

@wiliamsouza It's always been optional in code flow, but should still be included if present.

@wiliamsouza
Copy link
Member

With missing tests LGTM

@@ -307,6 +307,7 @@ def openid_authorization_validator(self, request):

Copy link
Member

Choose a reason for hiding this comment

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

Suggestion: Add the following text to method doc string:

+        nonce
+                OPTIONAL. String value used to associate a Client session with an
+                ID Token, and to mitigate replay attacks. The value is passed
+                through unmodified from the Authentication Request to the ID Token.
+                Sufficient entropy MUST be present in the nonce values used to
+                prevent attackers from guessing values

@skion
Copy link
Member Author

skion commented Sep 13, 2017

Can you please add a unit test for this code branch?

@thedrow Something like this 13c6cf5?

Also, should the other OIDC params end up as credentials as well? If so, then can add them in similar way.

@thedrow
Copy link
Collaborator

thedrow commented Sep 17, 2017

@skion Can you please rebase this branch?
Also, what other OIDC parameters are we talking about?

@skion
Copy link
Member Author

skion commented Sep 17, 2017

I think I did not spot max_age and acr_values anywhere, but we may also want things like claims_locales, request or request_uri eventually?

Rebased!

@thedrow
Copy link
Collaborator

thedrow commented Sep 18, 2017

Frankly I'm not familiar with OpenID as much as OAuth2 to tell you yes or no. Up to you.

@thedrow thedrow merged commit 52d5067 into oauthlib:master Sep 18, 2017
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.

3 participants