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

Added WorldId provider #819

Closed
wants to merge 6 commits into from
Closed

Conversation

sandrohanea
Copy link

Hello,
Added a provider for World ID.

I hope I didn't miss anything as it's my first time adding a provider here.

Copy link
Member

@martincostello martincostello left a comment

Choose a reason for hiding this comment

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

Some initial comments, but if this is a very new service provider and still in beta then I don't think we'd actually want to have to integrate and support this at this stage.

README.md Outdated Show resolved Hide resolved
@sandrohanea
Copy link
Author

Some initial comments, but if this is a very new service provider and still in beta then I don't think we'd actually want to have to integrate and support this at this stage.

The provider is not in beta, but already have production applications and support and more than 2.5 million unique humans are already in the platform.

Indeed, those 2 properties (likely_human and credential_type) are still under beta object so I understand why you might not want them in the package as they might change soon, so I removed them + all references for those.

@kevinchalet
Copy link
Member

Thanks for your PR, @sandrohanea!

To match the naming rules used by the .NET team (e.g Microsoft.AspNetCore.Authentication.OpenIdConnect), shouldn't we name this provider AspNet.Security.OAuth.WorldId (i.e without the capital D)?

@sandrohanea
Copy link
Author

Thanks for your PR, @sandrohanea!

To match the naming rules used by the .NET team (e.g Microsoft.AspNetCore.Authentication.OpenIdConnect), shouldn't we name this provider AspNet.Security.OAuth.WorldId (i.e without the capital D)?

They are using the name World ID, but I understand that in this package, it would be better alligned if WorldId is used (without the capital D), so I renamed it.

@sandrohanea sandrohanea changed the title Added WorldID provider Added WorldId provider Dec 14, 2023
@kevinchalet
Copy link
Member

@sandrohanea did you try the provider? I tried adding an OpenIddict web integration for WorldID and all I get is a 500 response at the token request stage.

Apparently, WorldID seems to crash when you specify a redirect_uri parameter in the token request (yet, it's a mandatory parameter when you define a redirect_uri in the authorization request... and for OIDC, it's always required).

@kevinchalet
Copy link
Member

2 other remarks:

  • WorldID supports PKCE, we'll want to enable it by default in the aspnet-contrib provider.
  • WorldID doesn't flow the nonce parameter in the identity token, which is a serious protocol violation. We don't have id_token validation in place in any of the aspnet-contrib providers so it's not blocking here, but it sucks WorldID is one of those non-standard implementations...

@sandrohanea
Copy link
Author

2 other remarks:

  • WorldID supports PKCE, we'll want to enable it by default in the aspnet-contrib provider.
  • WorldID doesn't flow the nonce parameter in the identity token, which is a serious protocol violation. We don't have id_token validation in place in any of the aspnet-contrib providers so it's not blocking here, but it sucks WorldID is one of those non-standard implementations...

I don't think it supports PKCE currently: https://docs.worldcoin.org/reference/sign-in#exchange-code
That endpoint is just accepting basic auth and not accepting code_verifier or code_challenge, or am I missing something?

Regarding:

@sandrohanea did you try the provider? I tried adding an OpenIddict web integration for WorldID and all I get is a 500 response at the token request stage.

Apparently, WorldID seems to crash when you specify a redirect_uri parameter in the token request (yet, it's a mandatory parameter when you define a redirect_uri in the authorization request... and for OIDC, it's always required).

Yes, I tried the provider with both staging app and production app, worked correctly for this OAuth2 approach.

@kevinchalet
Copy link
Member

I don't think it supports PKCE currently: https://docs.worldcoin.org/reference/sign-in#exchange-code
That endpoint is just accepting basic auth and not accepting code_verifier or code_challenge, or am I missing something?

It's not documented but the OIDC discovery document (https://id.worldcoin.org/.well-known/openid-configuration) lists S256 as a supported code_challenge_method, which means PKCE is supported:

"code_challenge_methods_supported":["S256"]

Yes, I tried the provider with both staging app and production app, worked correctly for this OAuth2 approach.

Weird. I'll give it another try: it's not normal to get a 500 error when the redirect_uri is sent.

BTW, how do you authenticate when using a staging app? Do you need a special iOS/Android app or is there a setting somewhere?

@kevinchalet
Copy link
Member

I was curious so I tried your provider and I get exactly the same error without changing anything to the code:

HTTP/1.1 500 Internal Server Error
Date: Thu, 21 Dec 2023 16:18:12 GMT
Content-Length: 0
Connection: keep-alive
Cache-Control: public, max-age=0, must-revalidate
Content-Security-Policy: default-src 'self'; script-src 'self' 'nonce-7f4aa660-c53d-4374-80d7-c69df8393d88' 'strict-dynamic'; font-src 'self' https://world-id-public.s3.amazonaws.com; style-src 'self' 'unsafe-inline' fonts.googleapis.com; connect-src 'self' https://app.posthog.com https://docs.worldcoin.org https://status.worldcoin.org https://developer.worldcoin.org https://rum.browser-intake-datadoghq.com https://bridge.worldcoin.org; img-src 'self' https://worldcoin.org https://world-id-public.s3.amazonaws.com
Strict-Transport-Security: max-age=63072000
Vary: RSC, Next-Router-State-Tree, Next-Router-Prefetch, Next-Url
X-Matched-Path: /oidc-route
X-Vercel-Cache: MISS
X-Vercel-Execution-Region: iad1
X-Vercel-Id: cdg1::iad1::9v64t-1703175491891-796b4fd8448b
CF-Cache-Status: DYNAMIC
Server: cloudflare
CF-RAY: 839170882c1f7028-CDG

Which manifests as a JSON exception with your provider since the body is empty:

Microsoft.AspNetCore.Authentication.AuthenticationFailureException: An error was encountered while handling the remote login.
 ---> System.Text.Json.JsonReaderException: The input does not contain any JSON tokens. Expected the input to start with a valid JSON token, when isFinalBlock is true. LineNumber: 0 | BytePositionInLine: 0.
   at System.Text.Json.ThrowHelper.ThrowJsonReaderException(Utf8JsonReader& json, ExceptionResource resource, Byte nextByte, ReadOnlySpan`1 bytes)
   at System.Text.Json.Utf8JsonReader.Read()
   at System.Text.Json.JsonDocument.Parse(ReadOnlySpan`1 utf8JsonSpan, JsonReaderOptions readerOptions, MetadataDb& database, StackRowStack& stack)
   at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 utf8Json, JsonReaderOptions readerOptions, Byte[] extraRentedArrayPoolBytes, PooledByteBufferWriter extraPooledByteBufferWriter)
   at System.Text.Json.JsonDocument.Parse(ReadOnlyMemory`1 json, JsonDocumentOptions options)
   at System.Text.Json.JsonDocument.Parse(String json, JsonDocumentOptions options)
   at Microsoft.AspNetCore.Authentication.OAuth.OAuthHandler`1.PrepareFailedOAuthTokenReponse(HttpResponseMessage response, String body)
   at Microsoft.AspNetCore.Authentication.OAuth.OAuthHandler`1.ExchangeCodeAsync(OAuthCodeExchangeContext context)
   at Microsoft.AspNetCore.Authentication.OAuth.OAuthHandler`1.HandleRemoteAuthenticateAsync()
   at Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler`1.HandleRequestAsync()
   --- End of inner exception stack trace ---
   at Microsoft.AspNetCore.Authentication.RemoteAuthenticationHandler`1.HandleRequestAsync()
   at Microsoft.AspNetCore.Authentication.AuthenticationMiddleware.Invoke(HttpContext context)
   at Microsoft.WebTools.BrowserLink.Net.BrowserLinkMiddleware.InvokeAsync(HttpContext context)
   at Microsoft.AspNetCore.Watch.BrowserRefresh.BrowserRefreshMiddleware.InvokeAsync(HttpContext context)
   at Microsoft.AspNetCore.Server.IIS.Core.IISHttpContextOfT`1.ProcessRequestAsync()

If I replay the same request without the redirect_uri parameter attached, I get a normal response so there's definitely something very funky with this service.

@martincostello I suggest we wait to merge this provider, at least until Worldcoin fixes this issue on their end.

@kevinchalet
Copy link
Member

After re-reading the documentation, it seems they just don't support the standard redirect_uri parameter in grant_type=authorization_code token requests. That said, it doesn't explain why it didn't work for me - I just tried again and I'm getting the same result - but worked for you 🤣

If we want to merge this provider, we'll need to override the token request phase to avoid sending the redirect_uri parameter that causes 500 errors.

@kevinchalet
Copy link
Member

Doing some housecleaning but feel free to reopen if you're still interested in working on this provider.

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

Successfully merging this pull request may close these issues.

3 participants