Propose PR solving issues #97 and #98 #99
Conversation
| @@ -33,8 +34,24 @@ class HTTPHeaderDict(UserDict): | |||
| d['hElLo'] == 'world' # >>> True | |||
| """ | |||
|
|
|||
| def __init__(self, dict=None, /, **kwargs): | |||
There was a problem hiding this comment.
Positional-only parameters are supported starting from Python 3.8. I'm not sure whether we still need to support Python 3.7, but for now, I would stick to maintaining that compatibility.
There was a problem hiding this comment.
That was mainly copied from original code from UserDict. That's why that signature
| @@ -9,6 +9,7 @@ | |||
| """ | |||
|
|
|||
| from collections import UserDict | |||
| from typing import Any | |||
|
|
|||
|
|
|||
| class HTTPHeaderDict(UserDict): | |||
There was a problem hiding this comment.
To "solidify" these changes, I would create a test file for collections.py (e.g., test_collections.py) with tests like these:
def test_http_header_dict_case_insensitive_get():
headers = HTTPHeaderDict(Authorization="Bearer token")
assert headers.get("authorization") == "Bearer token"
assert headers.get("Authorization") == "Bearer token"
assert headers.get("AUTHORIZATION") == "Bearer token"
assert headers.get("nonexistent", "default") == "default"
def test_http_header_dict_case_insensitive_delitem():
headers = HTTPHeaderDict(Authorization="Bearer token")
del headers["authorization"]
assert headers.get("Authorization") is None
def test_http_header_dict_case_insensitive_init():
headers = HTTPHeaderDict(**{"Authorization": "Bearer token", "Content-Type": "application/json"})
assert headers.get("authorization") == "Bearer token"
assert headers.get("content-type") == "application/json"
| access_token=token.access_token, | ||
| refresh_token=token.refresh_token, |
There was a problem hiding this comment.
I think I got a bit confused about removing the refresh_token from the response. How will the client get a refresh token if it needs to update the access_token? Auth0 uses the offline_access scope for this: https://auth0.com/docs/secure/tokens/refresh-tokens/get-refresh-tokens
This document says that the response MAY contain a refresh_token: https://www.oauth.com/oauth2-servers/making-authenticated-requests/refreshing-an-access-token/
There was a problem hiding this comment.
Another solution could be a feature flag. If it's enabled, the refresh token won't be returned in the response. This way, backward compatibility won't be broken, while still adhering to the behavior outlined in the RFC6749 standard.
There was a problem hiding this comment.
Well, I see your point @aliev. However, this is something very specific of the Implicit Grant. Indeed, the RFC 6749 clearly mentions that this flow was not conceived for refresh tokens:
The implicit grant type is used to obtain access tokens (it does not
support the issuance of refresh tokens) and is optimized for public
clients known to operate a particular redirection URI. These clients
are typically implemented in a browser using a scripting language
such as JavaScript.
(source: RFC 6749, section "4.2 Implicit Grant")
And also here:
The authorization server MUST NOT issue a refresh token.
(source: 4.2.2. Access Token Response)
Regarding this Auth0 link you paste before, the blog is very generic explanation about refresh token, so I guess they didn't really mean it in a concrete context like Implicit Grant. I think this may be why.
In any case, your proposal here:
Another solution could be a feature flag. If it's enabled, the refresh token won't be returned in the response. This way, backward compatibility won't be broken, while still adhering to the behavior outlined in the RFC6749 standard.
looks quite fine to me so that it leaves the decision to the implementer to be fully compliant or not!
Thanks again @aliev !!!
Cheers
Jose M. Prieto
PS: by the way, sorry for the long time I took in replying you, but your know, I was off in my summer break :P
There was a problem hiding this comment.
Hi @PrieJos !
Shall we try to deliver this MR as well?
As I mentioned, the safest way would be to implement a feature flag disabled by default.
You could add it, for example, in the Settings: https://github.com/aliev/aioauth/blob/master/aioauth/config.py#L15
We should also remove the changes related to HTTPHeaderDict from this PR, as they are already in the master branch.
There was a problem hiding this comment.
Hi @aliev
Sorry for the long time to reply. I totally oversee the notification. Sorry again.
Yes, that would be great. I will take a look and come out with a suggestion so that you can review it.
Cheers,
Jose M. Prieto
There was a problem hiding this comment.
Hi @PrieJos
No worries, thank you for your contribution again!
Proposed solutions to the following issues opened by myself:
getmethod ofaioauth.collections.HTTPHeaderDictclass is not case-insensitive #97Linting and test checks are all passed after modifications.
Please take a look and hope you can merge them upstream.
Thanks
Jose M. Prieto