-
-
Notifications
You must be signed in to change notification settings - Fork 170
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
api implementation for OpenID Connect users #207
Conversation
This is the api implementation for OpenID Connect users. async signInWithIDToken( id_token: string, nonce: string, provider: string ) cf. - #169 - supabase/auth#189
Hi @koba-ninkigumi and thanks much for this PR. Might you also be able to write up unit tests for a few scenarios?
Given this flow : and the GoTrue rules in https://github.com/supabase/gotrue/blob/8ba49df7c7bbcc87c9d5b211a32cb5aa17db231d/api/token.go#L316
This way we can have some tests in place to cover cases developers may encounter when using. Thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, I left some notes and a request to add several tests.
Thanks again for the PR.
Enabling OpenID Connect means that you are accepting a huge group of users from OpenID Connect providers (Google, Facebook, etc.) as already signed up. That's exactly what "OpenID" means. The idea of disabling signup on the supabase side is incompatible with OpenID Connect. |
If you or I know how to pretend to be an OpenID Connect provider and return id_token, it means that the OpenID Connect specification is broken. And since we can't get the id_token, we can't write a test. Please see the following test for reference. https://github.com/supabase/gotrue-js/blob/master/test/GoTrueClient.test.ts#L182 This is probably the same reason why there are no test cases for oauth providers here. If you think it is possible to write a test for OpenID Connect, could you please clarify how to do so in more detail? |
Renamed the Method. Added explanation of nonce.
Anything I can help with to get this merged? Would love to use this PR asap @koba-ninkigumi @dthyresson |
Hi @He1nr1chK thanks for offering to help. I think that these new functions could use:
|
Sign In With Google Handle credential responses with JavaScript functions ↑ You can find instructions on how to get the token here. (It is included in the response) Sign In With Google HTML API reference Sign In With Google JavaScript API reference ↑ Here is the description of the nonce that @dthyresson was requesting. This is sample code.
|
As I mentioned above, due to the OpenID Connect specification, it should be impossible to get an idtoken that works properly. If someone thinks it is possible, it would be great if that someone could write the unittest of this method. |
@dthyresson |
Could you mock the request and return expected results? This would ensure that the method signature or other logic doesn't accidentally get changed without some test check. For example, in a PR the cookie is being mocked to return headers as: export const mockCookieRequest = ({
event,
session,
method = 'POST',
}: {
event?: string
session?: Session | null
method?: string
}) => {
return {
method,
headers: { host: 'localhost:9999' },
body: { event, session },
}
}
export const mockCookieResponse = () => {
return {
getHeader: jest.fn(() => undefined),
setHeader: jest.fn(),
status: jest.fn(() => {
return { json: jest.fn(), end: jest.fn() }
}),
}
} See: https://github.com/supabase/gotrue-js/pull/198/files Maybe something similar? |
Instead of using a cookie as an example, use oauth2 from gotrue.js as an example. Currently, none of the tests for signIn() in gotrue.js specify a provider. |
I don't know of any other way to get a working access token other than getting the official Google private key. |
I can't prepare the test that @dthyresson requires. I would like to wait for someone's help. |
@dthyresson I have done some research into the possible tests one can write for OIDC and I have to agree with @koba-ninkigumi. It's going to be difficult if not impossible to write a proper test for the function. Is the test absolutely necessary? |
hey @koba-ninkigumi @He1nr1chK this is awesome! Thanks for your hard work.
It would be very useful to have some automated tests to make sure we don't accidentally break the logic later on.
Could you expand on what is the difficult bit? Could we mock the request as @dthyresson suggested? Again, thanks for all your help on this! 🙏 |
OpenID Connect is an authentication. It's a specification for proving "who you are." Who is the test runner? It is the test runner. A normal OpenID Connect provider will try to verify that a human actually accessed the site with a browser. OpenID Connect providers are tasked with rejecting non-human access, such as test runners. If a test runner is authenticated by OpenID Connect, even if it means a successful test of supabase goture. How do you exploit that hole to illegally obtain an idtoken? |
In order to connect to supabase with OpenID Connect, you need to get a legitimate idtoken beforehand. If it were possible to write a test to authenticate the idtoken, it could be done in two ways.
|
My suggestion is to release this feature first. |
I would also add that if you are setting up a fake OpenID Connect Provider in your testing, you will need to provide the necessary information on an externally accessible https server, per the OpenID Connect specification. |
Yes, again many thanks for your work and patience. Let's get this in, but maybe test out for a few weeks and if any changes need to be made, then can address -- and then release the docs with those updates. |
🎉 This PR is included in version 1.22.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@koba-ninkigumi @kangmingtay @dthyresson can this function be used yet? I updated my @supabase/supabase-js package to v1.30.3 which exposes this function but when I try to call it, I get |
@He1nr1chK I'm already using it with Sign In With Google, and google-one-tap is also working fine. |
@koba-ninkigumi that's fantastic news. How are you invoking the function? Like this:
|
Please see my first post on this page.
|
If you want to specify Google, you should not set issuer and client_id, but write 'google' in provider and set client_id by logging in to the supabase site. |
Perfect. Thank you @koba-ninkigumi (on the JS implementation) and @kangmingtay (on the Golang implementation) for the amazing work you both did to make this feature a reality. Much appreciated! |
Hi @koba-ninkigumi, when using the method with Apple as the provider I get the following error: |
Hey @He1nr1chK, could you include your request format? Can you try the following request format below and see if it works for you please?
|
Hi @kangmingtay thanks for getting back to me. This seems to have solved the issue but now I get |
@He1nr1chK can you please email us at [email protected] with your project reference so we can help to debug further? thanks! |
I think this PR doesn't implement linking |
@He1nr1chK did you ever get Apple authentication working? I've been trying but can't figure it out. Keep getting 500 errors. |
Hey @WJimmyCook, yes I did with the following function:
I use |
@He1nr1chK You're a legend, bro! Worked like a charm. |
@He1nr1chK: I hope you could help me also. I always get the error "invalid nonce" - {"message": "invalid nonce", "status": 400}. My provider is google and i tried both OAuth 2.0 Client IDs (the auto created from Google Service and the self created like the documentation mentioned). Both did not work. I also tried the curl-statement from @kangmingtay but it doesn't work also (same error). I don't know where the error come from (i don't think it's from gotrue-js) My code (simplified) looks like:
|
|
Ok, thank you. I missed the part with the hashed nonce, but i use a bare react-native project with @react-native-google-signin/google-signin and i have no options to pass the hashed nonce to the GoogleSignin.signIn() method. Do you have any idea? |
@megacherry not sure what non-expo options there are but I'm running a bare react-native project and I ended up adding expo unimodules (or whatever they call it now) so I can use any expo lib in the bare project. |
I'm hitting a wall with oidc (I'm attempting to use this server side with nodejs) - this seems obsolete? const { user, session, error } = await supabase.auth.signIn({
oidc:{
id_token: identityToken,
nonce: nonce,
provider: 'apple'
}
})
ERRORS ->
supabase.auth.signIn is not a function doesn't seem like this got ported to v2 ? am I missing something? there's no mention of this here this is my sample repo - https://github.com/johndpope/Sign-in-with-Apple-for-supabase |
@wweevv-johndpope I think oidc was removed from v2 but there are plans to add it back based on this comment: supabase/auth#434 (comment) I'm sticking with v1 until that get fixed. |
Hoping the team add it on v2 too, I've been configuring my project on supabase v2 and didn't know that this feature is not added. If the feature is already added on v1, I would appreciate supporting it back on v2 even just an option as experimental as there is still a lot of issues related to OIDC implementation. |
What kind of change does this PR introduce?
This is the api implementation for OpenID Connect users.
This pull request solves the following issue.
This pull request is made possible by the following pull request.
What is the current behavior?
none.
What is the new behavior?
Numerous issues will be resolved.
Additional context
supabase/supabase#5181
You should check the OpenID Connect specification for details. https://openid.net/developers/specs/
Sign In With Google
https://developers.google.com/identity/gsi/web/guides/overview
Handle credential responses with JavaScript functions
https://developers.google.com/identity/gsi/web/guides/handle-credential-responses-js-functions
↑ You can find instructions on how to get the token here. (It is included in the response)
Sign In With Google HTML API reference
https://developers.google.com/identity/gsi/web/reference/html-reference
Sign In With Google JavaScript API reference
https://developers.google.com/identity/gsi/web/reference/js-reference
↑ Here is the description of the nonce that @dthyresson was requesting.
This is sample code.