Conversation
There was a problem hiding this comment.
Pull request overview
Adds OpenID Connect (OIDC) support as an additional OmniAuth/Devise authentication provider, alongside existing Google and Keycloak options.
Changes:
- Adds
omniauth_openid_connectdependency and associated transitive gems. - Introduces OIDC configuration via environment variables and Devise OmniAuth initializer wiring.
- Adds an OIDC login button and implements the
openid_connectOmniAuth callback flow.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| Gemfile.lock | Locks omniauth_openid_connect and required OIDC-related dependencies. |
| Gemfile | Adds omniauth_openid_connect gem to dependencies. |
| docs/operating_documentation.md | Updates operating docs TOC and OAuth section to mention OpenID Connect. |
| config/initializers/devise.rb | Registers the :openid_connect OmniAuth strategy when configured. |
| config/initializers/customize_quepid.rb | Adds env-driven configuration for OIDC base URL, client credentials, and issuer. |
| app/views/sessions/new.html.erb | Adds “Sign in with OpenID Connect” button when provider is enabled. |
| app/models/user.rb | Adds :openid_connect to Devise omniauth_providers. |
| app/controllers/users/omniauth_callbacks_controller.rb | Adds openid_connect callback action and enables it for unauthenticated access. |
| .env.example | Documents the new OIDC-related environment variables. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 10 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| @user = create_user_from_omniauth(request.env['omniauth.auth']) | ||
|
|
||
| @user.errors.add(:base, "Can't log in a locked user." ) if @user.locked | ||
| Rails.logger.info "User errors after checking for locked status: #{@user.errors.full_messages.join(', ')}" | ||
|
|
||
| if @user.errors.empty? | ||
| session[:current_user_id] = @user.id # this populates our session variable. | ||
|
|
There was a problem hiding this comment.
Locked-user prevention happens after create_user_from_omniauth, which currently saves the user (and increments num_logins) before the locked check runs. This means locked login attempts can still mutate the user record; consider checking for an existing locked user by email before calling create_user_from_omniauth, or moving the locked check inside create_user_from_omniauth so it can short-circuit before persisting changes.
There was a problem hiding this comment.
I'm choosing to not change this to keep it in line with the other auth providers.
JPrevost
force-pushed
the
openid-connect-support
branch
from
328f9b5 to
d94aede
Compare
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
None of my comments are blocking - I'm comfortable with this moving forward as-is. I see the tests I expected to see (although I raised one question about an option in one of the tests), and I've already seen this working as deployed.
Looking forward to seeing this proposed upstream!
| Rails.application.config.keycloak_site = ENV.fetch('KEYCLOAK_SITE', '') | ||
|
|
||
| # == OpenID Connect Settings = | ||
| # All 4 of these settings are required for OpenID Connect authentication to work properly. |
There was a problem hiding this comment.
I'm wondering if the phrasing "All 4 of these settings" is confusing when there are 5 settings in the section? I see that the fifth is also labelled optional, and has a default value - so I think it is probably fine for folks who read these comments carefully.
Maybe this should be something like "The first 4 of these settings" rather than "All"?
There was a problem hiding this comment.
Ha! The 5th setting was a bonus I added at the last minute when I realized it would be really useful. I'll reword with your suggestions in mind. Thanks!
| Rails.application.config.keycloak_realm = ENV.fetch('KEYCLOAK_REALM', '') | ||
| Rails.application.config.keycloak_site = ENV.fetch('KEYCLOAK_SITE', '') | ||
|
|
||
| # == OpenID Connect Settings = |
There was a problem hiding this comment.
Is OpenID Connect considered an OAuth provider? I don't think so, but am not super familiar with the definitions. I ask because I see a line just above (L82) that says "We currently only support these two OAuth providers" and I'm wondering if that should be updated with this change.
There was a problem hiding this comment.
Good catch. I'll update it.
| OmniAuth.config.add_mock(:openid_connect, { info: { 'email' => 'fake@fake.com' } }) | ||
| @request.env['omniauth.auth'] = OmniAuth.config.mock_auth[:openid_connect] | ||
|
|
||
| post :openid_connect |
There was a problem hiding this comment.
Looking at the test above, is it worth doing an assert_difference...0 around this block to confirm that nothing gets created?
There was a problem hiding this comment.
Probably. This was modeled after the existing test for Google auth, but I do like the extra check. I'll see if I can add it without it causing more trouble than it's worth.
matt-bernhardt
left a comment
matt-bernhardt
left a comment
There was a problem hiding this comment.
Yep - tweak to ToC is fine
JPrevost
force-pushed
the
openid-connect-support
branch
2 times, most recently
from
a76e9f1 to
3f43794
Compare
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.
JPrevost
force-pushed
the
openid-connect-support
branch
from
3f43794 to
90c590f
Compare
No description provided.