cmd-johnson / deno-oauth2-client

Minimalistic OAuth 2.0 client for Deno.
MIT License
47 stars 9 forks source link

suggestion: `getUserInfo()` #29

Open iuioiua opened 1 year ago

iuioiua commented 1 year ago

Or, more specifically, something like OAuth2Client.getUserInfo(accessToken: string): Promise<unknown>. This would require adding a userInfoUri?: string property to OAuth2ClientConfig. I'd be happy to submit a PR.

P.S. We use this module for the official Deno KV OAuth module. Thank you for this great module. If you have any ideas on improving this module (deno-oauth2-client) or ours (deno_kv_oauth), I'd be happy to help!

cmd-johnson commented 1 year ago

I'm not quite sure how to move forward on this one. The userinfo endpoint isn't really part of the OAuth 2.0 spec and lives in the OpenID Connect spec instead.

While I originally intended this module to be a pure OAuth 2.0 client, I see the potential in extending its functionality to cover OpenID Connect as well. Although I'd rather separate OpenID Connect related code into its own class (e.g. OIDCClient instead of OAuth2Client) to make it easier to follow the specs in the code.

Basing the OIDCClient on OAuth2Client requires a few additional changes to how OAuth 2.0 is handled right now though. For example, there's currently no way to access the Access Token Response in the Authorization Code Grant which for OIDC would additionally include the id_token field.

This in turn means that we'd have to find a nice(ish) way to extend/augment parts of the grant logic in quite a few places. This is especially true when considering that not all Authorization Servers follow the OAuth 2.0 spec to the letter (e.g. returning an array of scopes instead of a space-concatenated string #26).

Maybe the easiest way forward is this:

cmd-johnson commented 1 year ago

Although at that point I'm not really sure if we even need the OAuth2Client class anymore because you can just directly create an instance of the grants you're actually using and be done with it :shrug:

cmd-johnson commented 1 year ago

I started working on an OIDC implementation within this module that includes a few of the changes proposed above, as well as a getUserInfo(…) method: https://github.com/cmd-johnson/deno-oauth2-client/tree/feature/oidc/src/oidc

The implementation is lacking tests and proper documentation, but it'd be great if you could give it a try and report any shortcomings or bugs that you encounter!

The new OIDCClient class works mostly like the OAuth2Client class does, but it only supports the OIDC Authorization Code Flow for now. It does require a bit of additional configuration though:

iuioiua commented 1 year ago

I'm sorry I didn't reply to you about the last comments! It's very exciting to see you're giving OIDC a crack! It'd be a big, once done, for this module and other projects that use this module.

I'll give this a try as soon as I'm able. I've only skimmed the code so far. Perhaps, what might help, both you and me is an example like this one. Perhaps, writing the code in action might help write the implementation 🙂

I'll let you know how I go.

adoublef commented 1 year ago

I'm looking forward to this myself too. I like the idea of using a different class for OIDC as likely gives us, the end user for flexibility. OIDC will definitely solve a query I've been having for a while and so all for it. Thanks

iuioiua commented 1 year ago

Sorry for the delay, @cmd-johnson. I've started tinkering with your OIDC implementation (https://github.com/denoland/deno_kv_oauth/tree/oidc). I'm stuck on defining the correct verifyJwt parameter. Again, perhaps, having an example app like your examples/http.ts would help.

iuioiua commented 1 year ago

Gentle ping, @cmd-johnson

cmd-johnson commented 1 year ago

Oh right, sorry! I designed the type of verifyJwt to be compatible with jose's jwtVerify function, so your code in demo_oidc.ts doesn't look too wrong.

I've added two OIDC examples to the OIDC branch. One for http and one for oak, just like with the OAuth2 examples. I also found and fixed two small errors in my userInfo and OIDC access token validation logic.

iuioiua commented 1 year ago

Thanks! Now, with my current demo, I'm getting the following error:

Error: Invalid token response: id_token is already expired
    at AuthorizationCodeFlow.assertIsValidIDToken (https://raw.githubusercontent.com/cmd-johnson/deno-oauth2-client/feature/oidc/src/oidc/authorization_code_flow.ts:275:13)
    at AuthorizationCodeFlow.getToken (https://raw.githubusercontent.com/cmd-johnson/deno-oauth2-client/feature/oidc/src/oidc/authorization_code_flow.ts:203:10)
    at eventLoopTick (ext:core/01_core.js:183:11)
    at async handleCallback (file:///Users/asher/GitHub/deno_kv_oauth/src/handle_callback.ts:70:18)
    at async handler (file:///Users/asher/GitHub/deno_kv_oauth/demo_oidc.ts:80:30)
    at async ext:deno_http/00_serve.js:436:22

I'll do some further troubleshooting and get back to you.

cmd-johnson commented 1 year ago

Sounds like a bug that I pushed a fix for together with the examples You might just have to run your code with --reload to fix this

iuioiua commented 1 year ago

A little hacky, but it works! Thank you. I've updated the code. WDYT?

Screenshot 2023-08-28 at 3 16 29 pm
iuioiua commented 1 year ago

@cmd-johnson, would you be happy to review the PR if we implement something that uses getUserInfo(), once ready, in Deno KV OAuth? All good if you're unable 🙂

iuioiua commented 1 year ago

Gently pinging @cmd-johnson

cmd-johnson commented 1 year ago

Sure, just point me to the right direction and I can take a look :slightly_smiling_face: Did you encounter anything missing in the implementation? Otherwise I'll go forward and open up a PR to get this into main as soon as I've written tests for the OIDC code

iuioiua commented 1 year ago

I'll create a PR in the coming days and ping you 🙂

I didn't notice anything in the implementation missing.