OpenID Connect Support#1690
Conversation
Why are these changes being introduced: * OpenID Connect is a popular authentication protocol that allows users to log in using their existing accounts from other providers. * Adding support for OpenID Connect will enhance the user experience by providing more login options. How does this address that need: * Implemented OpenID Connect support using the omniauth-openid-connect gem. This models the similar implementation in place in this codebase for Keycloak and Google. Document any side effects to this change: * Some typos were fixed in the documentation and code comments not directly related to OpenID Connect support.
epugh
left a comment
epugh
left a comment
There was a problem hiding this comment.
This is looking really good... I wonder, can we drop the Keycloak set up and use openid to connect wiht Keycloak? Any suggestions on how to test this?
| # comments on the other flows imply that this is only for new users, which is not the case so it is not entirely | ||
| # clear what the intent is. If the intent is to only create a case for new users, then we should probably check | ||
| # if the user is new before creating a case. | ||
| @user.cases.build case_name: "Case #{@user.cases.size}" |
There was a problem hiding this comment.
You know what, I think this is leftover from the dark ages when we didn't have a "homepage" for Quepid. You literally just got dropped right into the Case creation screen.
I think we don't need this, and honestly, probably should remove them from the others as well.
Also, when we formalize the "Judge (Rater) user persona" they won't even ever see cases!
There was a problem hiding this comment.
Gotcha, that makes sense. Would you prefer I remove it from this OpenID Connect configuration or leave it so you can clean them all up at the same time?
|
|
||
| <% if Rails.application.config.devise.omniauth_providers.include?(:openid_connect) %> | ||
| <% email_login_only = false %> | ||
| <!-- <a href="https://www.flaticon.com/free-icons/digital" title="digital icons">Digital icons created by Freepik - Flaticon</a> --> |
There was a problem hiding this comment.
??? Do we need this for crediting the image origin?
There was a problem hiding this comment.
I copied this from the other implementations. I figured someone made a decision to do that intentionally in the past so I just went with it. I'm happy to adjust or leave as-is based on your preference.
|
|
||
| # OAuth Settings | ||
| # OAuth Settings: remove these to disable Google OAuth or set them to your own values | ||
| GOOGLE_CLIENT_ID=your_google_client_id |
There was a problem hiding this comment.
i wonder if we need to comment out these settings so when users first fire it up, they get reasonable defaults? I believe the setup script just copies .env.example to .env, so mayb we need to comment these? The keycload one is there I guess becasue when you fire up quepid in dev mode we already have it.
There was a problem hiding this comment.
maybe we need GOOGLE_CLIENT_ID= #your_google_client_id ???
There was a problem hiding this comment.
Defaults are hard for sure and I don't have a sense how frequently people benefit from having the non-functional Google option showing. It might be good so they know it is an option and just needs proper configuration, or it might be really frustrating. Personally I didn't really mind it the way it is and it didn't take long to figure out I needed to comment it out to make it go away.
|
@epugh While a Keycload server can provide OpenID Connect services, I'm not confident whether the integration in this app is using OpenID Connect or another supported Keycloak authentication flow. I suspect, but have not confirmed, removing the direct Keycloak option from Quepid would be a breaking change that requires re-configuration at best so I'd suggest if you want to remove the option it would be best to deprecate it first with a timeline to removal just in case it is in active use in production someplace. |
|
Hi @JPrevost I wanted to give you an update that I have been working on how to use Keycloak with OpenID directly.. I ran into one hiccup which is our dev Keycloak runs on http, not https, and that is an issue. I found a ugly workaround, and then have opened up omniauth/omniauth_openid_connect#211. If we do get this PR in, then we can actually REMOVE the If this is holding you up, we could merge this PR, and then when the openid PR lands, then do the refactoring thiere. |
|
This is the local version of my refactor... eric_refactor.patch |
|
@epugh We are not blocked at all with this. We're using a fork with our OpenID Connect support in place and will eventually retire the fork if it lands here. No rush and thanks for taking the time to consider how best this fits in the app longterm. |
|
are you on relevance slack #quepid channel? |
|
Okay, I've beat my head on this for a long time, but I haven't been able to get the omniauth-openid gem to get updated. I'm going to push a fix or two here, and call it good. |
|
I ran into merge conflicts, so opened up #1701... |
2 participants
Description
How does this address that need:
Motivation and Context
#1287
How Has This Been Tested?
We are using this configuration against an Okta OpenId Connect provider in our local environment.
Screenshots or GIFs (if appropriate):
Types of changes
Checklist: