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.

Sign up for GitHub

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

Propose PR solving issues #97 and #98 #99

Open
wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Propose PR solving issues #97 and #98 #99

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
126aae7
Fix case sensitiveness in HTTPHeaderDict (#97)
PrieJos Aug 22, 2024
810870c
Fix compliant issue with implicit grant type (#98)
PrieJos Aug 22, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions aioauth/collections.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
"""

from collections import UserDict
from typing import Any


class HTTPHeaderDict(UserDict):
Copy link
Owner

Choose a reason for hiding this comment

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

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"

Expand All @@ -33,8 +34,24 @@ class HTTPHeaderDict(UserDict):
d['hElLo'] == 'world' # >>> True
"""

def __init__(self, dict=None, /, **kwargs):
Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That was mainly copied from original code from UserDict. That's why that signature

"""Object initialization."""
super().__init__(dict, **kwargs)
self.data = {k.lower(): v for k, v in self.data.items()}

def __setitem__(self, key: str, value: str):
super().__setitem__(key.lower(), value)

def __getitem__(self, key: str):
return super().__getitem__(key.lower())

def __delitem__(self, key: str):
"""Item deletion."""
return super().__delitem__(key.lower())

def get(self, key: str, default: Any = None):
"""Case-insentive get."""
try:
return self[key]
except KeyError:
return default
8 changes: 3 additions & 5 deletions aioauth/response_type.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
else:
from typing_extensions import get_args

from .utils import generate_token
from .errors import (
InvalidClientError,
InvalidRedirectURIError,
Expand All @@ -33,6 +32,7 @@
)
from .storage import TStorage
from .types import CodeChallengeMethod
from .utils import generate_token


class ResponseTypeBase(Generic[TRequest, TStorage]):
Expand Down Expand Up @@ -115,12 +115,10 @@ async def create_authorization_response(
generate_token(48),
)
return TokenResponse(
expires_in=token.expires_in,
refresh_token_expires_in=token.refresh_token_expires_in,
access_token=token.access_token,
refresh_token=token.refresh_token,
Copy link
Owner

Choose a reason for hiding this comment

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

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/

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@PrieJos PrieJos Sep 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @PrieJos! Could you create another PR to fix the issue you mentioned in #97 while I research the refresh token? I would really appreciate your contribution and would be happy to see you on the list of contributors! :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HI @aliev

Here it goes the PR for issue #97: #103

Thanks
Jose M. Prieto

Copy link
Owner

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Owner

Choose a reason for hiding this comment

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

Hi @PrieJos

No worries, thank you for your contribution again!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @aliev,

I'll propose to continue the discussion on the original issue #98 instead of this PR since it became now obsolete after I pushed the new PR #108 for the final proposed solution.

Cheers
Jose M. Prieto

scope=token.scope,
token_type=token.token_type,
expires_in=token.expires_in,
scope=token.scope,
)


Expand Down
8 changes: 4 additions & 4 deletions aioauth/responses.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@
"""
from dataclasses import dataclass, field
from http import HTTPStatus
from typing import Dict
from typing import Dict, Optional

from .collections import HTTPHeaderDict
from .constances import default_headers
Expand Down Expand Up @@ -51,11 +51,11 @@ class TokenResponse:
Used by :py:class:`aioauth.response_type.ResponseTypeToken`.
"""

expires_in: int
refresh_token_expires_in: int
access_token: str
refresh_token: str
expires_in: int
scope: str
refresh_token_expires_in: Optional[int] = None
refresh_token: Optional[str] = None
token_type: str = "Bearer"


Expand Down
13 changes: 5 additions & 8 deletions aioauth/server.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
from http import HTTPStatus
from typing import Any, Dict, Generic, List, Optional, Tuple, Type, Union


if sys.version_info >= (3, 8):
from typing import get_args
else:
Expand Down Expand Up @@ -60,12 +59,7 @@
TokenInactiveIntrospectionResponse,
)
from .storage import TStorage
from .types import (
GrantType,
RequestMethod,
ResponseType,
TokenType,
)
from .types import GrantType, RequestMethod, ResponseType, TokenType
from .utils import (
build_uri,
catch_errors_and_unavailability,
Expand Down Expand Up @@ -417,7 +411,10 @@ async def authorize(request: fastapi.Request) -> fastapi.Response:
response = await response_type.create_authorization_response(
request, client
)
responses.update(asdict(response))
response_asdict = {
k: v for k, v in asdict(response).items() if v is not None
}
responses.update(response_asdict)

# See: https://openid.net/specs/oauth-v2-multiple-response-types-1_0.html#Combinations
if "code" in response_type_list:
Expand Down
29 changes: 14 additions & 15 deletions tests/test_flow.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,6 @@
enforce_list,
generate_token,
)

from tests import factories
from tests.classes import AuthorizationContext
from tests.utils import check_request_validators
Expand Down Expand Up @@ -532,9 +531,9 @@ async def test_multiple_response_types(context_factory):

assert "state" in fragment
assert "expires_in" in fragment
assert "refresh_token_expires_in" in fragment
assert "refresh_token_expires_in" not in fragment
assert "access_token" in fragment
assert "refresh_token" in fragment
assert "refresh_token" not in fragment
assert "scope" in fragment
assert "token_type" in fragment
assert "code" in fragment
Expand Down Expand Up @@ -620,40 +619,40 @@ async def test_response_type_id_token(context_factory, response_mode):

if response_mode == "fragment":
assert "state" in fragment
assert "expires_in" in fragment
assert "refresh_token_expires_in" in fragment
assert "access_token" in fragment
assert "refresh_token" in fragment
assert "expires_in" in fragment
assert "refresh_token" not in fragment
assert "refresh_token_expires_in" not in fragment
assert "scope" in fragment
assert "token_type" in fragment
assert "code" in fragment
assert "id_token" in fragment
elif response_mode == "form_post":
assert "state" in response.content
assert "expires_in" in response.content
assert "refresh_token_expires_in" in response.content
assert "access_token" in response.content
assert "refresh_token" in response.content
assert "expires_in" in response.content
assert "refresh_token" not in response.content
assert "refresh_token_expires_in" not in response.content
assert "scope" in response.content
assert "token_type" in response.content
assert "code" in response.content
assert "id_token" in response.content
elif response_mode == "query":
assert "state" in query
assert "expires_in" in query
assert "refresh_token_expires_in" in query
assert "access_token" in query
assert "refresh_token" in query
assert "expires_in" in query
assert "refresh_token" not in query
assert "refresh_token_expires_in" not in query
assert "scope" in query
assert "token_type" in query
assert "code" in query
assert "id_token" in query
else:
assert "state" in fragment
assert "expires_in" in fragment
assert "refresh_token_expires_in" in fragment
assert "access_token" in fragment
assert "refresh_token" in fragment
assert "expires_in" in fragment
assert "refresh_token" not in fragment
assert "refresh_token_expires_in" not in fragment
assert "scope" in fragment
assert "token_type" in fragment
assert "code" in fragment
Expand Down
Loading