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.

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

feat: add id token support #169

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Conversation

drev74
Copy link

@drev74 drev74 commented Feb 2, 2025

No description provided.

@evert
Copy link
Collaborator

evert commented Feb 9, 2025

Hey, I've rejected a very similar PR in the past, #118

But I've actually changed my mind since then. I still don't intend to turn this in a full-blown OIDC library, but this seems like a fairly low-lift way for people to just get the idToken and potentially do the JWT parsing themselves, so well... lets do it I guess.

However, I have notes:

  • This should have a test.
  • Existing tests and Typescript should pass
  • I think the type of idToken in OAuth2Token should be idToken?: string instead of idToken: string | null, and the token should ideally on the object (not even as an undefined value) so it doesn't appear as null and in serializations for users that never care about OIDC.

@drev74
Copy link
Author

drev74 commented Feb 9, 2025

Hi @evert

Thanks for your feedback. I think adding ID Token won't hurt this lib, since most if not all modern Oauth2 providers DO support OIDC ☝️ . In fact, I also own an OIDC/Oauth2 provider which returns ID token by default 🤗

Providing an ID token saves Frontend from making at least 1 extra call to the provider endpoint to get a user ID hence yields in a lower latency and a lower traffic load.

UPD:

I fixed my code as per your feedback and added a test. For some reason, current eslint config forces double quotes in my VSCode so apart of tiny changes we have a larger formatting diff 🙄

@evert
Copy link
Collaborator

evert commented Feb 9, 2025

since most if not all modern Oauth2 providers DO support OIDC

There's lots of OAuth2 use-cases. You don't really see this as much for example when OAuth2 is used to integrate with APIs of commercial software, and frankly OIDC is overkill for many cases.

If you can, try to undo those formatting changes as they are causing a conflict, and will also down the road cause a conflict with #172 which is also actively being worked on!

@drev74
Copy link
Author

drev74 commented Feb 9, 2025

Restored original formatting. Added a test for id_token

@evert
Copy link
Collaborator

evert commented Feb 9, 2025

There's still a conflict and some changes unrelated to this PR! Once those are fix, this is good to merge, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants