Add enable_pkce config, True by default
#765
Conversation
oauthenticator/tests/test_oauth2.py
Outdated
| ) | ||
|
|
||
|
|
||
| async def test_callback_handler_pkce(): |
There was a problem hiding this comment.
Can you also add a test to verify an error is returned if PKCE is requested but the server doesn't support it?
There was a problem hiding this comment.
Done. I guess the way an Oauth server tells it doesn't support PKCE would be by returning a 403 when the client tries to exchange the code for a token.
There was a problem hiding this comment.
Won't it work but just not be checked for validity if the provider doesn't support it (ignored extra parameters)? If that's true, should it be on by default?
There was a problem hiding this comment.
If the parameters are definitely ignored then we could always send them, and change the property name to require_pkce to enforce it on the client side.
There was a problem hiding this comment.
@renan-r-santos Sorry, I realise now I was unclear in my request. I was thinking of testing that raise web.HTTPError(400, "Missing code_verifier") is raised when the server silently ignores the PKCE request.
Regarding whether or not to always send the PKCE request, how about if we rename the parameter require_pkce instead of pkce, but keep the current implementation (only send the PKCE field when require_pkce = True? That lets us switch to always sending PKCE in future if we want, without having to change or add any parameters.
There was a problem hiding this comment.
Regarding whether or not to always send the PKCE request, how about if we rename the parameter require_pkce instead of pkce, but keep the current implementation (only send the PKCE field when require_pkce = True?
That sounds like a good plan to me. I've updated the PR to reflect that.
I was thinking of testing that raise web.HTTPError(400, "Missing code_verifier") is raised when the server silently ignores the PKCE request.
raise web.HTTPError(400, "Missing code_verifier") won't get raised if the server silently ignores the PKCE request. code_verifier is data that we store in a cookie together with state_id and next_url in the login handler, but code_verifier isn't sent to or returned from the OAuth provider during login. It is only during code exchange that the client grabs the code_verifier previously stored in a cookie and sends it to the server so it can hash it and compare it with the code_challenge.
So, the error you mentioned can only happen if the cookie got somehow deleted or corrupted between login and callback handlers. If you still think it is worth adding a test for that, let me know and I'll update the PR. I could be wrong, but I don't think there's a way for a client to know if a server ignores PKCE parameters. On the other hand, a server can enforce that clients use PKCE.
There was a problem hiding this comment.
Yeah, I think PKCE is only for the provider to check; clients only provide information. So the only reason to disable it that I can see is if some provider raises on unrecognized arguments, which is officially wrong:
The authorization server MUST ignore unrecognized request parameters.
So if we're only talking about valid OAuth providers, we don't even need to make it optional.
From the PKCE spec:
As the OAuth 2.0 [RFC6749] server responses are unchanged by this
specification, client implementations of this specification do not
need to know if the server has implemented this specification or not
and SHOULD send the additional parameters as defined in Section 4 to
all servers.
i.e. it's always right to send PKCE, and it's entirely up to the provider to decide whether to validate or not.
There was a problem hiding this comment.
Thank you very much for the references, especially
The authorization server MUST ignore unrecognized request parameters.
which I didn't know and makes a lot of difference.
|
By the way, I think this PR also helps a bit in getting OAuthenticator compatible with OAuth 2.1 (still in draft) as:
|
Remove pkce not supported test
Change enable_pkce default to True Factor out code_verifier and code_challenge generation Improve docs Update tests
|
Guys, I think this is ready for another round of reviews. I made a few improvements and incorporated important points that we discussed here in the |
There was a problem hiding this comment.
I read the docstring and considered the config naming and think its great, i'll leave the implementation review to another person as i think its already been considered and i've not onboarded myself enough to review it myself yet.
Thank you @renan-r-santos for working this so thoroughly!!
consideRatio
changed the title
enable_pkce config, defaults to Trueenable_pkce config, True by default
|
Thank you! |
Fixes #468