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 generic oidc provider #25

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

Conversation

maximilianmikus
Copy link
Contributor

WIP

This PR is based upon #12

This PR adds a generic provider for OIDC. It currently supports the 'code' response type / grant type 'authorization_code' and also optionally 'pkce'. Other response types and grant type combinations are not yet tested, but might in some cases already work.

Feedback welcome.

Copy link

@vinayakkulkarni vinayakkulkarni left a comment

Choose a reason for hiding this comment

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

LGTM

@atinux
Copy link
Owner

atinux commented Dec 7, 2023

Happy to fix the conflicts and update the readme? No idea what oidc is 😅

@maximilianmikus
Copy link
Contributor Author

@atinux "oidc" is OpenID Connect

Will update the readme and fix the conflicts.

@itpropro
Copy link

itpropro commented Dec 14, 2023

This looks like a great addition! I have some suggestions/feedback regarding the implementation:

  • If this should be a generic OIDC provider, I would remove the "auth0" annotations from the type descriptions, as many other vendors also provide generic OIDC endpoints. I am just talking about things like Auth0 OAuth Audience, which should be OAuth Audience, not the @see annotation, they are really helpful
  • As we are in a SSR scenario we have a confidential client scenario, where we can use the clientSecret, I would point out the criticality of using a client secret as well as the secure storage of it in the docs. It's very important to remember this, if this library changes it's scope to also include client side scenarios at some point, as the usage of a clientSecret would be considered a critical security flaw. But as long as we are strictly working server side, there should be no security issue. Any plans to change to also support client side @atinux ?
  • If the option to use a client secret is provided, we should also provide the possibility of using client certificate credentials as a more secure option
  • userinfoUrl should be a optional parameter, as some providers don't have or provide a user info endpoint.
  • I would suggest adding a config option called userNameClaim to be able to identify and map the designated user name, as these can often be customized while setting up the OAuth identity provider configuration
  • I would suggest adding a certification uri (also known as jwks_uri) for token validation. For token validation in general, we could also resolve the validation URIs from the iss field. As defined by the OAuth standard, the jwks_uri should be listed in the well-known properties, which can be found under {ISSUER_FIELD}/.well-known/openid-configuration.
  • For proper token validation, there should also an array config option to list the valid issuers to avoid malicious issuers being verified.
  • I would suggest narrowing down the types for response_type to the possibilities defined by the OAuth spec ("code", "code token", "code id_token", "id_token token", "code id_token token")
  • I would suggest adding an optional response_mode field with the options "query" and "fragment"

EDIT:

  • As some other libraries do, it could also be a good idea to introduce a openidConfiguration option, where the user can directly provide the /.well-known/openid-configuration metadata document url to directly get all the required provider settings
  • grant_type should be limited to authorization_code
  • I would suggest leaving out the claims config for now, as it is normally only needed, if an API returns a claims challenge to the client, which would required handling the complete claims challenge flow and which is not defined as part of RFC 6749

@maximilianmikus
Copy link
Contributor Author

@itpropro thank you for the feedback! I will try to incorporate it.

* main:
  chore(release): v0.0.15
  chore: up deps
  feat: added aws cognito provider (atinux#36)
  feat: add auth0 connection parameter to config (atinux#39)
  fix: replace encoded space characters with regular spaces (atinux#40)
  chore(release): v0.0.14
  chore: update deps
  feat: added keycloak as oauth provider (atinux#23)
  chore: test bundler module resolution (atinux#32)
  chore(release): v0.0.13
  chore: rename session from verify to fetch
  chore(release): v0.0.12
  fix: correct arguments for hooks
  chore(release): v0.0.11
  feat: add sessionHooks to extend user sessions
@espensgr
Copy link

Any progress on this? Have a project that i would love to test the oidc implementation on

@atinux
Copy link
Owner

atinux commented Mar 20, 2024

They are still some conflicts and I would like to have the readme updated in order for users to understand how to use it.

Happy to take a stab at it?

@espensgr
Copy link

Don't know enough auth and openid spec to be able to do that 😢
I guess @maximilianmikus is the more competent of us here 😄

@Jorgagu
Copy link

Jorgagu commented Jun 14, 2024

Hi @atinux and @maximilianmikus, it seems no one is taking up the topic again, do you need help?

Copy link

@septatrix septatrix left a comment

Choose a reason for hiding this comment

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

I would really love to seem this move forward as this is currently our blocker for using this library. We currently vendor a similar change (basically adjusting the Keycloak handler) on a feature branch for a while now and are really happy with it

connection: config.connection || ''
>>>>>>> main

Choose a reason for hiding this comment

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

There is a remainder of a merge conflict here

@atinux
Copy link
Owner

atinux commented Jul 3, 2024

I am up for that, but why do we need to update the Auth0 provider in this pull request?

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.

8 participants