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

Generic OAuth provider #1372

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

Conversation

carlobeltrame
Copy link

@carlobeltrame carlobeltrame commented Jan 10, 2024

What kind of change does this PR introduce?

Fixes most of #451. This PR introduces a new generic OAuth provider, which can be configured to connect to a wide range of OAuth 2.0 identity providers. Thus, most requests in #451 should be solvable using this new provider.

What is the current behavior?

Only explicitly supported providers can be connected to Supabase auth. For applications which need domain-specific or niche providers, it is not practical or even possible to implement a separate provider class and contribute it to supabase auth / gotrue. At the same time, the keycloak provider (which would allow delegation to arbitrary identity providers) might soon be deprecated according to #1108, further closing off Supabase for very domain-specific applications.

What is the new behavior?

Up to three instances of the same new generic OAuth 2.0 provider can now be configured in gotrue. This provider works largely the same as other, hardcoded providers. For mapping the identity provider API payloads to gotrue JWT claims, a mapping can be specified in the config, including resolution of nested fields in the API payload. See test.env for an example mapping.
Currently not supported are custom claims as well as reading claim values from the authorization response (as would be common for OIDC if I understand correctly). The implementation could be extended in the future to support these tasks if needed.

Additional context

I chose not to use reflection for setting the claim values, for security reasons. Also for security reasons, I chose not to use Sprintf for converting claim values to string. Rather, I took a quite manual, explicit approach for both of these tasks. However, I am no Go developer, so please let me know in case I should change something.

I based the provider on the last merged OAuth provider Kakao, in an attempt to have as up-to-date a codestyle as possible.

I have successfully tested the provider with a local instance of https://github.com/hitobito/hitobito/, but that setup is quite involved (requiring to clone at least 3 git repositories and modifying the docker compose setup of gotrue and hitobito). I have called for other people to test with their own identity providers in #451 (comment).

Tested with a local instance of github.com/hitobito/hitobito
@hf
Copy link
Contributor

hf commented Jan 19, 2024

Wow! Thank you for attempting to solve this issue, this is quite an effort!

I will take a deeper look soon, but here's some high-level observations so far:

  • GENERIC1, 2, 3... config options are not very useful or scalable. We need to figure out a different way to do this. Ideally this goes via the database as we're already struggling maintaining giant configs.
  • It's not possible to just "parse" the userinfo endpoint statically. We definitely need a mechanism, probably a hook, that will supply the userinfo data and this would return back the parsed claims.

The team generally wants to keep such large features internal, as there's quite a bit of feedback that's not publicly available and that may not be properly addressed. Please don't get discouraged from contributing further if we decide to put this implementation on hold.

@carlobeltrame
Copy link
Author

Thanks for your feedback! I'm glad to hear this is at least getting some attention. It is insignificant to me whether my code or someone else's code will do the job in the end. I just wanted to get the discussion rolling and lower the mental hurdle of "we don't have time to implement this" a little.

GENERIC1, 2, 3... config options are not very useful or scalable. We need to figure out a different way to do this. Ideally this goes via the database as we're already struggling maintaining giant configs.

Totally agree that this does not scale arbitrarily. I had to do this because all the current providers are configurable via envconfig, and I did not want to introduce a paradigm shift there since this PR is already big enough. I also imagine for most small to medium sized applications, up to 3 (or up to 5 or 10 if you prefer) custom OAuth providers should be enough. But I understand that you want to keep the product flexible to support even the largest of applications.

Keep in mind that so far, gotrue only supported one single Keycloak OAuth provider, which in theory there could be multiple. So in this regard having 3 of the custom providers would already represent a step forward in my opinion. But if this were merged, you would of course also have to think about migration later, when you get around to implementing something more scalable.

It's not possible to just "parse" the userinfo endpoint statically. We definitely need a mechanism, probably a hook, that will supply the userinfo data and this would return back the parsed claims.

Well, evidently it's possible, but not with unlimited customizability. Here as well, I did not want to create a paradigm shift by introducing user-defined code which will run inside gotrue (or at least communicates directly with gotrue), especially for security reasons. So as a first step, I reused envconfig's map parsing abilities to implement a simple mapping, as suggested by several community members in the issue. If you find a better way I'm all for it!

Please just consider keeping Keycloak support around until some kind of generic OAuth provider is integrated into gotrue. It's currently the only way for someone like me (and all the interested people which I tagged in #451 (comment)) to connect Supabase to smaller, domain-specific identity providers.

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.

None yet

2 participants