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

Add a generic OIDC OAuth2 provider #420

Merged
merged 3 commits into from
Aug 1, 2023
Merged

Conversation

stnguyen90
Copy link
Contributor

@stnguyen90 stnguyen90 commented Apr 28, 2023

What does this PR do?

Add an OIDC OAuth2 Provider to the Console

Test Plan

image image image

Related PRs and Issues

Have you read the Contributing Guidelines on issues?

Yes

@vercel
Copy link

vercel bot commented Apr 28, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
console ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 31, 2023 5:09pm
console-cloud ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 31, 2023 5:09pm
console-next ✅ Ready (Inspect) Visit Preview 💬 Add feedback Jul 31, 2023 5:09pm

Copy link
Contributor

@TorstenDittmann TorstenDittmann left a comment

Choose a reason for hiding this comment

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

lets wait for backend PR 👍🏻

@binaryfire
Copy link

Nice work @stnguyen90. Hoping this gets merged soon. Will be great to have Keycloak support.

@eldadfux
Copy link
Member

It should be OIDC in the UI, and not Oidc

@stnguyen90
Copy link
Contributor Author

It should be OIDC in the UI, and not Oidc

@eldadfux, the API returns Oidc for the name and we use that in the Console:

<p class="u-margin-block-start-8">{provider.name}</p>

We would have to add and maintain a list of exceptions in the console repo somewhere. That okay?

@eldadfux
Copy link
Member

It should be OIDC in the UI, and not Oidc

@eldadfux, the API returns Oidc for the name and we use that in the Console:

<p class="u-margin-block-start-8">{provider.name}</p>

We would have to add and maintain a list of exceptions in the console repo somewhere. That okay?

I think we should avoid that. I think we need to find a better way to expose the name, which endpoint do we use for getting it right now?

@stnguyen90
Copy link
Contributor Author

I think we should avoid that. I think we need to find a better way to expose the name, which endpoint do we use for getting it right now?

@eldadfux, it comes from the List/Get Projects endpoints. The providers attribute is a list of providers where name is the provider key.

https://github.com/appwrite/appwrite/blob/e12a5738aa56bcbb6dc41c8b5b0d201bb2eae5e7/src/Appwrite/Utopia/Response/Model/Project.php#L276

Btw, our setup is really limiting since it makes it so that we can only have 1 of each OAuth2 provider.

@eldadfux
Copy link
Member

I don't see a reason why we can't use the key for getting the name. If needed we can also add new key attribute. This API is also private for the console so we should be OK with the change.

https://github.com/appwrite/appwrite/blob/e12a5738aa56bcbb6dc41c8b5b0d201bb2eae5e7/src/Appwrite/Utopia/Response/Model/Project.php#L276

@stnguyen90
Copy link
Contributor Author

I don't see a reason why we can't use the key for getting the name. If needed we can also add new key attribute. This API is also private for the console so we should be OK with the change.

@eldadfux, here's a PR to add the provider key to the response: appwrite/appwrite#5857

@stnguyen90
Copy link
Contributor Author

@TorstenDittmann, anything else before this can be merged?

Show provider name in the Console and use the provider key for
everything else.
@stnguyen90
Copy link
Contributor Author

@TorstenDittmann is this good to merge now?

@eldadfux eldadfux merged commit 2188120 into main Aug 1, 2023
@stnguyen90 stnguyen90 deleted the feat-4299-oidc-oauth2-provider branch August 8, 2023 22:52
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.

4 participants