OIDC token response to authorization_code grant type should always include an id_token #76

tdg5 · 2023-03-23T13:02:56Z

This creates an OIDC specific version of the AuthorizationCodeGrantType that always includes an id_token. The scope is narrower than I thought previously, and it seems like id_token is really only required when getting a token via authorization_code. There are some indications that the password grant type could also include an id token, but so far it seems to vary by vendor.

codecov-commenter · 2023-03-23T13:37:50Z

Codecov Report

Merging #76 (7d1180d) into master (266fc9e) will decrease coverage by 3.47%.
The diff coverage is 7.14%.

📣 This organization is not using Codecov’s GitHub App Integration. We recommend you install it so Codecov can continue to function properly for your repositories. Learn more

@@            Coverage Diff             @@
##           master      #76      +/-   ##
==========================================
- Coverage   97.87%   94.41%   -3.47%     
==========================================
  Files          15       17       +2     
  Lines         707      734      +27     
  Branches      110      114       +4     
==========================================
+ Hits          692      693       +1     
- Misses          6       32      +26     
  Partials        9        9

Impacted Files	Coverage Δ
aioauth/oidc/core/grant_type.py	`0.00% <0.00%> (ø)`
aioauth/oidc/core/responses.py	`0.00% <0.00%> (ø)`
aioauth/response_type.py	`100.00% <ø> (ø)`
aioauth/storage.py	`100.00% <ø> (ø)`
aioauth/oidc/core/requests.py	`100.00% <100.00%> (ø)`

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

aioauth/responses.py

aioauth/response_type.py

tdg5 · 2023-03-27T14:39:00Z

I've converted this PR to a draft because I want to update it based on what I've proposed in #75.

tdg5 · 2023-03-27T17:17:31Z

@aliev , though you should take a look at #75 first since this PR depends on it, I think this PR is ready for review again.

aioauth/oidc/core/grant_type.py

aioauth/storage.py

tdg5 · 2023-03-27T17:23:11Z

tests/oidc/core/test_flow.py

+        "authorization_code": AuthorizationCodeGrantType[Request, TStorage],
+    },
+)
+async def test_authorization_code_flow_token_response_includes_id_token(


Maybe I should include more of the other authorization code flow tests also?

tdg5 · 2023-03-28T12:55:49Z

I lean toward handling it in another PR, but I ran into a scenario today where it seems like the password grant should also have an option of returning an id_token.

…d_token

aioauth/oidc/core/grant_type.py

aliev · 2023-04-02T21:45:01Z

aioauth/oidc/core/grant_type.py

+        )
+
+        # validate_request will have already ensured the request includes a code.
+        assert request.post.code is not None


Are these asserts for debugging purposes?

The asserts are to make the type checker happy. I've added appropriate if TYPE_CHECKING: conditions around them to make that more clear.

aliev · 2023-04-02T21:49:05Z

aioauth/response_type.py

-            request.query.code_challenge_method,
-            request.query.code_challenge,
-            generate_token(42),
+            client_id=client.client_id,


I was thinking of allowing only positional arguments, in which case there will be an explicit indication of what arguments are required:

async def create_authorization_code( self, request: TRequest, *, client_id: str, scope: str, response_type: ResponseType, redirect_uri: str, code_challenge_method: Optional[CodeChallengeMethod], code_challenge: Optional[str], code: str, ) -> TAuthorizationCode:

it's should not be done in scope of this PR, just as an idea. What do you think?

I'm flexible on the matter. I tend to feel like positional arguments can get unwieldy when there is a lot of them and I also like that keyword arguments help someone reading the code outside of an IDE (e.g. in Github for example) to better understand what the purpose of a given argument is without having to go look at the function that is being called to understand what it's positional arguments are.

To that end, I've updated the signature for create_authorization_code and get_id_token to include a tail **kwargs to allow for keyword args to be passed that may be used by a subclass of Storage. I think this achieves kind of the same thing in that the required arguments are still explicit, but other implementations can take additional arguments as needed.

tdg5 · 2023-04-06T16:58:15Z

@aliev, sorry for the delay in getting back to your comments. I've made some updates based on your comments and think this PR should be ready for you to take another look whenever it is convenient for you.

aliev

Overall looks great! There are a couple of minor changes that can be made within the scope of this PR:

It would be great:

to include the information about OIDC support to the README.md file: https://github.com/aliev/aioauth/blob/master/README.md?plain=1#L15
to include instructions on how to enable OIDC support in the documentation: https://github.com/aliev/aioauth/blob/master/docs/source/sections/examples/fastapi.rst

aliev · 2023-04-07T09:17:42Z

aioauth/oidc/core/requests.py

+    # specifies whether the Authorization Server prompts the End-User for
+    # reauthentication and consent. The defined values are: none, login,
+    # consent, select_account.
+    # https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest
    prompt: Optional[str] = None


How about turning this comment into documentation for the prompt attribute?

Suggested change

prompt: Optional[str] = None

prompt: Optional[str] = None

"""

Space delimited, case sensitive list of ASCII string values that

specifies whether the Authorization Server prompts the End-User for

reauthentication and consent. The defined values are: none, login,

consent, select_account.

https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest

"""

tdg5 · 2023-04-24T13:47:57Z

@aliev thanks for merging this. Sorry I didn't address your comments; I've had a busy few weeks. I will try to find some time this week to address any comments you haven't already addressed here.

aliev · 2023-04-24T19:23:51Z

@tdg5 no worries at all. thank you!

tdg5 requested a review from aliev as a code owner March 23, 2023 13:02

aliev reviewed Mar 23, 2023

View reviewed changes

aioauth/responses.py Outdated Show resolved Hide resolved

tdg5 commented Mar 24, 2023

View reviewed changes

aioauth/response_type.py Outdated Show resolved Hide resolved

tdg5 mentioned this pull request Mar 25, 2023

Allow OIDC prompt query param #75

Merged

tdg5 marked this pull request as draft March 27, 2023 14:38

tdg5 force-pushed the oidc-token-always-includes-id-token branch from 9e5d4ae to 1314a9b Compare March 27, 2023 17:10

tdg5 changed the title ~~An OIDC token response always includes an id_token~~ OIDC token response to authorization_code grant type should always includes an id_token Mar 27, 2023

tdg5 force-pushed the oidc-token-always-includes-id-token branch from 1314a9b to d897f85 Compare March 27, 2023 17:15

tdg5 changed the title ~~OIDC token response to authorization_code grant type should always includes an id_token~~ OIDC token response to authorization_code grant type should always include an id_token Mar 27, 2023

tdg5 marked this pull request as ready for review March 27, 2023 17:16

tdg5 commented Mar 27, 2023

View reviewed changes

aioauth/oidc/core/grant_type.py Show resolved Hide resolved

tdg5 commented Mar 27, 2023

View reviewed changes

aioauth/storage.py Outdated Show resolved Hide resolved

tdg5 commented Mar 27, 2023

View reviewed changes

tdg5 force-pushed the oidc-token-always-includes-id-token branch from c9187c9 to 00cda97 Compare March 27, 2023 17:55

tdg5 force-pushed the oidc-token-always-includes-id-token branch 2 times, most recently from e98f440 to 872df5b Compare March 29, 2023 12:57

tdg5 assigned aliev Mar 29, 2023

Danny Guinther added 3 commits March 30, 2023 06:14

Add OIDC version of AuthorizationCodeGrantType that always includes i…

bcfce04

…d_token

Make it possible to include nonce in authorization code data

bbd898a

Add some docs for OIDC Request and prompt param

24ec274

tdg5 force-pushed the oidc-token-always-includes-id-token branch from 872df5b to 24ec274 Compare March 30, 2023 10:20

Add doc reference to OIDC AuthorizationCode grant type

cc8d518

aliev reviewed Apr 2, 2023

View reviewed changes

Danny Guinther added 2 commits April 6, 2023 12:26

Sync with upstream tests

365fb13

Merge branch 'upstream-master' into oidc-token-always-includes-id-token

da3ef5a

tdg5 force-pushed the oidc-token-always-includes-id-token branch from b01aa7f to da3ef5a Compare April 6, 2023 16:41

Danny Guinther added 3 commits April 6, 2023 12:43

Better identify asserts that are for type checking

06fe90b

Add **kwargs to get_id_token function signature

6f0b895

Switch nonce to kwarg

3952620

aliev approved these changes Apr 7, 2023

View reviewed changes

Merge branch 'master' into oidc-token-always-includes-id-token

7d1180d

aliev merged commit d3b670f into aliev:master Apr 22, 2023

tdg5 deleted the oidc-token-always-includes-id-token branch April 24, 2023 13:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OIDC token response to authorization_code grant type should always include an id_token #76

OIDC token response to authorization_code grant type should always include an id_token #76

tdg5 commented Mar 23, 2023 •

edited

Loading

codecov-commenter commented Mar 23, 2023 •

edited

Loading

tdg5 commented Mar 27, 2023

tdg5 commented Mar 27, 2023

tdg5 Mar 27, 2023

tdg5 commented Mar 28, 2023

aliev Apr 2, 2023

tdg5 Apr 6, 2023

aliev Apr 2, 2023

tdg5 Apr 6, 2023

tdg5 commented Apr 6, 2023

aliev left a comment

aliev Apr 7, 2023

tdg5 commented Apr 24, 2023

aliev commented Apr 24, 2023

OIDC token response to authorization_code grant type should always include an id_token #76

OIDC token response to authorization_code grant type should always include an id_token #76

Conversation

tdg5 commented Mar 23, 2023 • edited Loading

codecov-commenter commented Mar 23, 2023 • edited Loading

Codecov Report

tdg5 commented Mar 27, 2023

tdg5 commented Mar 27, 2023

tdg5 Mar 27, 2023

Choose a reason for hiding this comment

tdg5 commented Mar 28, 2023

aliev Apr 2, 2023

Choose a reason for hiding this comment

tdg5 Apr 6, 2023

Choose a reason for hiding this comment

aliev Apr 2, 2023

Choose a reason for hiding this comment

tdg5 Apr 6, 2023

Choose a reason for hiding this comment

tdg5 commented Apr 6, 2023

aliev left a comment

Choose a reason for hiding this comment

aliev Apr 7, 2023

Choose a reason for hiding this comment

tdg5 commented Apr 24, 2023

aliev commented Apr 24, 2023

tdg5 commented Mar 23, 2023 •

edited

Loading

codecov-commenter commented Mar 23, 2023 •

edited

Loading