Add OpenID Connect auth support #2194
Conversation
thezzisu
changed the title
📝 WalkthroughWalkthroughThe changes introduce a new documentation file, Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant IdentityProvider
participant Autolab
User->>IdentityProvider: Register and obtain credentials
IdentityProvider-->>User: Provide client_id and client_secret
User->>Autolab: Configure OIDC with credentials
Autolab->>IdentityProvider: Validate user identity
IdentityProvider-->>Autolab: Return user identity
Autolab-->>User: Confirm authentication success
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
Also a working oauth_config.yml using OIDC: ---
openid_connect:
issuer: <Your issuer here, https://example.com>
discovery: true
uid_field: <Claim name which should be used as uid, default 'sub'>
client_auth_method: other
send_nonce: false
client_options:
identifier: <AppID>
secret: <AppSecret>
redirect_uri: <Replace using your own site url example http://localhost:3303/auth/users/auth/openid_connect/callback>See https://github.com/omniauth/omniauth_openid_connect/tree/master for details |
|
For the most part this looks okay, although I would prefer a method to edit the configuration via UI. I am unable to get OIDC set up on my end since I don't have an internal identity provider, and I also can't get OIDC working with testing tools, so I am unable to review any further at this time. |
Thank you for your reply and sorry for this delay. An example using Auth0 by Okta can be following: ---
openid_connect:
issuer: https://dev-s5lhkwr76zowpqbs.us.auth0.com/
discovery: true
uid_field: 'sub'
client_auth_method: other
scope: ['openid', 'email', 'profile']
send_nonce: false
client_options:
identifier: 4RfbkCTRdfxQYs7vVABzHbWDwkpq58u6
secret: O6vwmDkp31jE63r_VLR8SKvcIBFdrUqvnm1wv958DRJTFEiCOQsLU7haPobqmVwi
redirect_uri: http://localhost:3303/auth/users/auth/openid_connect/callback
(Assume http://localhost:3303 is your deployment url) You can login with following username/password to test: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📥 Commits
Files that changed from the base of the PR and between 700ec5daa98f5e8f939ef7614411d06ab48c029d and db575594b5d7b7956c01ca0e326dacfea4f6bd80.
⛔ Files ignored due to path filters (1)
Gemfile.lockis excluded by!**/*.lock,!**/*.lock
📒 Files selected for processing (5)
- Gemfile (1 hunks)
- app/controllers/users/omniauth_callbacks_controller.rb (1 hunks)
- app/models/user.rb (4 hunks)
- app/views/admins/_oauth_integration.html.erb (1 hunks)
- config/initializers/devise.rb (1 hunks)
🔇 Additional comments (4)
Gemfile (1)
65-65: Verify compatibility and security of the new OpenID Connect gemThe addition of the
omniauth_openid_connectgem aligns with the PR objective of introducing OpenID Connect (OIDC) authentication support. However, please consider the following points:
- Ensure compatibility with existing authentication gems (e.g.,
devise,omniauth,omniauth-google-oauth2) and the Rails version (6.1.7.6).- Verify the security and maintenance status of the
omniauth_openid_connectgem.- Consider pinning the gem to a specific version (e.g., '0.8.0') instead of using the pessimistic operator (~>) for better control over updates in a production environment.
To verify the gem's compatibility and status, please run the following commands:
After running these checks, please review the output and ensure that:
- The gem is compatible with the current Rails version.
- The gem is actively maintained and has recent updates.
- There are no known vulnerabilities.
- Bundle install succeeds without conflicts.
config/initializers/devise.rb (1)
230-233: LGTM with suggestions for improvementThe implementation of OpenID Connect (OIDC) support looks good overall. It follows the existing pattern used for Google OAuth2, which maintains consistency in the codebase. The use of
OauthConfigController.get_oauth_credentialsfor dynamic configuration is a good practice for managing sensitive information.However, I have a few suggestions to enhance robustness and maintainability:
Consider adding error handling within the lambda function. If
OauthConfigController.get_oauth_credentialsfails, it could lead to unclear errors during the authentication process.It would be beneficial to add a comment documenting the expected structure of the OIDC configuration returned by
get_oauth_credentials. This would make it easier for other developers to understand and maintain the code.Here's a suggested improvement with error handling and documentation:
# Set up OpenID Connect interceptor # Expected OIDC configuration structure: # { # client_options: { # identifier: 'your_client_id', # secret: 'your_client_secret', # redirect_uri: 'your_redirect_uri' # }, # issuer: 'https://your-oidc-provider.com', # discovery: true, # ... (other OIDC-specific options) # } config.omniauth :openid_connect, setup: lambda { |env| begin oidc_config = OauthConfigController.get_oauth_credentials(:openid_connect) env['omniauth.strategy'].options.merge!(oidc_config) rescue => e Rails.logger.error "Failed to configure OIDC: #{e.message}" raise "OIDC configuration error. Please check your settings." end }To ensure the
OauthConfigController.get_oauth_credentialsmethod is implemented correctly, please run the following verification script:This script will help verify that the
OauthConfigControllerand itsget_oauth_credentialsmethod are properly implemented to handle OIDC configuration.app/models/user.rb (2)
12-12: Adding OpenID Connect provider to Devise configurationThe addition of
:openid_connectto theomniauth_providerslist correctly enables OpenID Connect authentication support.
102-106: Implementation offind_for_openid_connect_oauthmethodThe new method
find_for_openid_connect_oauthis implemented consistently with existing OAuth methods, ensuring proper retrieval of users authenticated via OpenID Connect.
|
Thanks again for your work here, I was able to verify that OIDC does work given the testing credentials, seems good to me! One other thing though, do you mind updating the documentation with the information necessary to setup OIDC on Autolab (for example, providing the example yml file)? |
Thank you for your reply! I will soon update corresponding docs. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
docs/installation/oidc_integration.md (2)
1-6: Improve grammar in the introduction.Replace "sign into" with "sign in to" for standard English usage.
-It allows applications to verify user identities through an external identity provider (IdP). By implementing OIDC, you can enable students to sign into Autolab using a third-party identity provider (e.g., your institution's CAS). +It allows applications to verify user identities through an external identity provider (IdP). By implementing OIDC, you can enable students to sign in to Autolab using a third-party identity provider (e.g., your institution's CAS).🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The verb ‘sign into’ is not standard English, except in the context of the law (“The bill was signed into law”). Write “sign in to”. For websites and computers, other options are “log in to” or “log on to”.
Context: ...enting OIDC, you can enable students to sign into Autolab using a third-party identity pr...(SIGN_INTO)
42-50: Enhance claims documentation with examples.Add example values to make the claims requirements more clear.
### Required Claims Ensure your IdP's ID token includes the following claims (configure via `scope` or your IdP's management console): -- `email` -- `name` OR `first_name` and `last_name` +- `email` (e.g., "user@example.com") +- One of the following name configurations: + 1. `name` (e.g., "John Smith") + 2. Both `first_name` and `last_name` (e.g., "John" and "Smith") - If both `first_name` and `last_name` are provided, they will be used for the user's name - - If only `name` is provided, it will be used as `first_name`, and `last_name` will be empty + - If only `name` is provided, it will be used as `first_name`, and `last_name` will be set to an empty string
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
docs/images/openid_setup.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (1)
- docs/installation/oidc_integration.md (1 hunks)
🧰 Additional context used
🪛 LanguageTool
docs/installation/oidc_integration.md
[grammar] ~3-~3: The verb ‘sign into’ is not standard English, except in the context of the law (“The bill was signed into law”). Write “sign in to”. For websites and computers, other options are “log in to” or “log on to”.
Context: ...enting OIDC, you can enable students to sign into Autolab using a third-party identity pr...(SIGN_INTO)
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...rameters to configure: -redirect_url: Must be set to `{scheme}://{your_autola...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~13-~13: Loose punctuation mark.
Context: ...figuration and access URL. -client_id: - Some IdPs allow you to specify a cu...(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~15-~15: Loose punctuation mark.
Context: ... ID for the next step. -client_secret: - Upon registration completion, the I...(UNLIKELY_OPENING_PUNCTUATION)
🪛 Gitleaks
docs/installation/oidc_integration.md
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
The documentation is updated. |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (9)
app/views/admins/_oauth_integration.html.erb (1)
26-34: Enhance user guidance for OpenID configuration.
While keeping the disk-based configuration approach is appropriate given the complexity of OIDC settings, the current guidance could be more helpful.
Consider this improvement:
- Edit configuration on disk
+ <div class="collapsible-body">
+ <p>OpenID Connect configuration requires editing the configuration file directly due to its complex nature.</p>
+ <p>Please refer to the <a href="/docs/installation/oidc_integration.md">OIDC Integration Guide</a> for detailed setup instructions.</p>
+ <p class="grey-text">Configuration file location: <code>config/oauth_config.yml</code></p>
+ </div>This change:
- Maintains the disk-based configuration approach
- Provides clear context about why UI configuration isn't available
- Links to the documentation
- Shows the configuration file location
docs/installation/oidc_integration.md (3)
3-3: Fix grammar in the introduction.
Change "sign into" to "sign in to" for correct English usage.
-OpenID Connect (OIDC) is a modern authentication protocol built on top of OAuth 2.0. It allows applications to verify user identities through an external identity provider (IdP). By implementing OIDC, you can enable students to sign into Autolab using a third-party identity provider (e.g., your institution's CAS).
+OpenID Connect (OIDC) is a modern authentication protocol built on top of OAuth 2.0. It allows applications to verify user identities through an external identity provider (IdP). By implementing OIDC, you can enable students to sign in to Autolab using a third-party identity provider (e.g., your institution's CAS).🧰 Tools
🪛 LanguageTool
[grammar] ~3-~3: The verb ‘sign into’ is not standard English, except in the context of the law (“The bill was signed into law”). Write “sign in to”. For websites and computers, other options are “log in to” or “log on to”.
Context: ...enting OIDC, you can enable students to sign into Autolab using a third-party identity pr...
(SIGN_INTO)
28-39: Add warning about example credentials.
While the example configuration is helpful, add a clear warning to prevent confusion.
openid_connect:
- # Example configuration for Auth0 with sample credentials
+ # Example configuration for Auth0
+ # WARNING: The following credentials are examples only.
+ # Replace these with your own credentials from your IdP.
issuer: https://dev-s5lhkwr76zowpqbs.us.auth0.com/🧰 Tools
🪛 Gitleaks
37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
54-56: Enhance verification steps.
The verification section would benefit from more detailed steps and troubleshooting guidance.
-After updating `oauth_config.yml` and restarting Autolab, you can verify the configuration in Autolab's **OAuth Integration** settings page:
+After updating `oauth_config.yml` and restarting Autolab, follow these steps to verify the configuration:
+
+1. Navigate to Autolab's admin panel
+2. Access the **OAuth Integration** settings page
+3. Verify that OpenID Connect appears as an available authentication method
+4. Test the integration by attempting to sign in with your IdP credentials
+5. Verify that user information (email and name) is correctly populated
+
+If you encounter any issues:
+- Check Autolab's application logs for error messages
+- Verify that all required claims are present in the ID token
+- Ensure your IdP's configuration matches Autolab's settings
Gemfile (1)
64-64: Consider using a more permissive version constraint.
The current version constraint ~> 0.8.0 only allows patch updates. Consider using >= 0.8.0, < 0.9.0 to ensure you receive all security patches while maintaining compatibility.
Apply this diff:
-gem 'omniauth_openid_connect', '~> 0.8.0'
+gem 'omniauth_openid_connect', '>= 0.8.0', '< 0.9.0'app/controllers/users/omniauth_callbacks_controller.rb (1)
107-108: Fix indentation for better readability.
The indentation of the create parameters is inconsistent with the rest of the file.
- current_user.authentications.create(provider: data["provider"],
- uid: data["uid"])
+ current_user.authentications.create(
+ provider: data["provider"],
+ uid: data["uid"]
+ )app/models/user.rb (1)
102-106: LGTM with refactoring opportunity
The implementation correctly handles OpenID Connect authentication lookup. However, there's an opportunity to reduce code duplication across all OAuth finder methods.
Consider consolidating all OAuth finder methods into a single method:
def self.find_for_oauth(auth, _signed_in_resource = nil)
provider = auth.provider == "CMU-Shibboleth" ? "CMU-Shibboleth" : auth.provider
authentication = Authentication.find_by(
provider: provider,
uid: auth.uid
)
authentication.user if authentication&.user
endconfig/initializers/devise.rb (2)
230-233: LGTM with suggestions for improved error handling.
The OIDC configuration follows the same pattern as the existing Google OAuth2 setup, which is good for consistency. However, consider adding error handling for missing or invalid OIDC configurations.
Consider wrapping the credentials retrieval in a begin-rescue block:
config.omniauth :openid_connect, setup: lambda{ |env|
- env['omniauth.strategy'].options.merge!(OauthConfigController.get_oauth_credentials :openid_connect)
+ begin
+ credentials = OauthConfigController.get_oauth_credentials(:openid_connect)
+ raise 'Invalid OIDC configuration' if credentials.nil? || !credentials.key?(:issuer)
+ env['omniauth.strategy'].options.merge!(credentials)
+ rescue => e
+ Rails.logger.error "Failed to load OIDC configuration: #{e.message}"
+ raise "OIDC authentication is not properly configured"
+ end
}230-233: Consider documenting OIDC configuration requirements.
Since OIDC configuration can be complex, consider adding inline documentation about required parameters and their purpose.
Add a comment block explaining the configuration requirements:
+ # OpenID Connect configuration requires:
+ # - issuer: The OIDC provider URL
+ # - client_options:
+ # - identifier: The client ID from your OIDC provider
+ # - secret: The client secret
+ # - redirect_uri: The callback URL for your application
+ # For more details, see docs/installation/oidc_integration.md
config.omniauth :openid_connect, setup: lambda{ |env|📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (2)
Gemfile.lockis excluded by!**/*.lock,!**/*.lockdocs/images/openid_setup.pngis excluded by!**/*.png,!**/*.png
📒 Files selected for processing (6)
- Gemfile (1 hunks)
- app/controllers/users/omniauth_callbacks_controller.rb (1 hunks)
- app/models/user.rb (4 hunks)
- app/views/admins/_oauth_integration.html.erb (1 hunks)
- config/initializers/devise.rb (1 hunks)
- docs/installation/oidc_integration.md (1 hunks)
🧰 Additional context used
📓 Learnings (3)
app/controllers/users/omniauth_callbacks_controller.rb (1)
Learnt from: thezzisu
PR: autolab/Autolab#2194
File: app/controllers/users/omniauth_callbacks_controller.rb:103-123
Timestamp: 2024-10-10T10:46:27.394Z
Learning: In `Users::OmniauthCallbacksController`, flash messages use provider names with underscores (e.g., `Google_OAuth2`, `OpenID_Connect`) for consistency.
app/views/admins/_oauth_integration.html.erb (1)
Learnt from: thezzisu
PR: autolab/Autolab#2194
File: app/views/admins/_oauth_integration.html.erb:26-34
Timestamp: 2024-10-10T10:44:47.213Z
Learning: The OpenID Connect configuration scheme is more complex than Google's, so adding a detailed configuration form in the UI may not be practical. It's acceptable to require users to edit the configuration on disk for OpenID Connect settings.
docs/installation/oidc_integration.md (2)
Learnt from: thezzisu
PR: autolab/Autolab#2194
File: docs/installation/oidc_integration.md:25-39
Timestamp: 2024-10-29T08:25:35.118Z
Learning: In the Autolab documentation, it's acceptable to include sample OIDC configurations with example credentials, even if they resemble real credentials.
Learnt from: thezzisu
PR: autolab/Autolab#2194
File: docs/installation/oidc_integration.md:54-56
Timestamp: 2024-10-29T08:26:43.071Z
Learning: In the Autolab documentation, images are referenced using absolute paths starting with `/images/...`, so image paths in markdown files should use absolute paths.
🪛 LanguageTool
docs/installation/oidc_integration.md
[grammar] ~3-~3: The verb ‘sign into’ is not standard English, except in the context of the law (“The bill was signed into law”). Write “sign in to”. For websites and computers, other options are “log in to” or “log on to”.
Context: ...enting OIDC, you can enable students to sign into Autolab using a third-party identity pr...
(SIGN_INTO)
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...rameters to configure: - redirect_url: Must be set to `{scheme}://{your_autola...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ...figuration and access URL. - client_id: - Some IdPs allow you to specify a cu...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ... ID for the next step. - client_secret: - Upon registration completion, the I...
(UNLIKELY_OPENING_PUNCTUATION)
🪛 Gitleaks
docs/installation/oidc_integration.md
37-37: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🔇 Additional comments (7)
docs/installation/oidc_integration.md (2)
7-20: LGTM! Well-documented security practices.
The registration process section effectively covers the key parameters and includes important security best practices, particularly around handling credentials.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~11-~11: Loose punctuation mark.
Context: ...rameters to configure: - redirect_url: Must be set to `{scheme}://{your_autola...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~14-~14: Loose punctuation mark.
Context: ...figuration and access URL. - client_id: - Some IdPs allow you to specify a cu...
(UNLIKELY_OPENING_PUNCTUATION)
[uncategorized] ~16-~16: Loose punctuation mark.
Context: ... ID for the next step. - client_secret: - Upon registration completion, the I...
(UNLIKELY_OPENING_PUNCTUATION)
45-53: LGTM! Clear explanation of claims handling.
The required claims section effectively documents the necessary claims and how they are processed.
Gemfile (1)
64-64: LGTM! Ensure SSL enforcement for OIDC endpoints.
The omniauth_openid_connect gem is appropriate for implementing OIDC authentication. Since OIDC handles sensitive authentication data, ensure that SSL is enforced for all OIDC-related endpoints using the existing rack-ssl-enforcer configuration.
Let's verify the SSL configuration:
✅ Verification successful
SSL enforcement is properly configured for OIDC endpoints
The verification shows that SSL is properly enforced in production through multiple layers:
- Global SSL enforcement is enabled with
config.force_ssl = truein production - Rack::SslEnforcer middleware is configured with specific exceptions (only for log/local submissions)
- OIDC routes (
lti_launch/oidc_login) fall under the global SSL enforcement
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check SSL enforcement configuration
# Expected: Find SSL enforcement for OIDC routes
# Test 1: Look for SSL enforcement in config files
echo "Checking SSL configuration..."
rg -g 'config/**/*' -A 5 'force_ssl|ssl_enforcer|:ssl|https'
# Test 2: Look for OIDC route definitions
echo "Checking OIDC routes..."
rg -g 'config/routes.rb' -A 5 'openid|oidc|omniauth'
Length of output: 11543
app/controllers/users/omniauth_callbacks_controller.rb (1)
103-123: LGTM! The OpenID Connect implementation follows established patterns.
The implementation mirrors the existing Google OAuth2 authentication flow, maintaining consistency in the codebase. The flash message format using OpenID_Connect aligns with the existing convention (e.g., Google_OAuth2).
app/models/user.rb (3)
12-12: LGTM: OpenID Connect provider correctly added to Devise configuration
The addition of :openid_connect to the OmniAuth providers list is consistent with the existing authentication setup.
136-146: Verify name handling with your OIDC provider
The code handles both split (first_name/last_name) and combined name formats. Ensure your OIDC provider consistently supplies these fields in the expected format.
#!/bin/bash
# Description: Check OIDC name field handling across the codebase
# Look for any other places where OIDC user info is processed
# Search for OIDC user info processing
rg -l "openid_connect.*info" --type ruby
# Search for name field processing
rg -l "first_name.*last_name" --type ruby165-168: LGTM: Consistent with existing OAuth handling
The OpenID Connect authentication data is correctly processed, maintaining consistency with other providers.
| @user = User.find_for_openid_connect_oauth(request.env["omniauth.auth"], current_user) | ||
| unless @user | ||
| # automatic cleanup of devise.* after sign in | ||
| session["devise.openid_connect_data"] = request.env["omniauth.auth"].except("extra") | ||
| @user = User.add_oauth_if_user_exists session | ||
| @user ||= User.new_with_session(nil, session) | ||
| end |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider adding error handling for authentication failures.
While the code handles the case when a user is not found, it doesn't handle potential failures during user creation or authentication.
else
@user = User.find_for_openid_connect_oauth(request.env["omniauth.auth"], current_user)
- unless @user
+ if @user.nil?
# automatic cleanup of devise.* after sign in
session["devise.openid_connect_data"] = request.env["omniauth.auth"].except("extra")
@user = User.add_oauth_if_user_exists session
@user ||= User.new_with_session(nil, session)
+
+ if @user&.new_record?
+ flash[:alert] = "Failed to create user from OpenID Connect data"
+ redirect_to new_user_registration_url and return
+ end
endCommittable suggestion was skipped due to low confidence.
| end | ||
| redirect_to root_path | ||
| else | ||
| @user = User.find_for_openid_connect_oauth(request.env["omniauth.auth"], current_user) |
There was a problem hiding this comment.
💡 Codebase verification
OpenID Connect implementation found but needs enhancement
The find_for_openid_connect_oauth method is implemented in the User model but appears to have minimal functionality:
- It only looks up the user through Authentication model and returns the associated user
- Missing error handling and user creation logic for new OpenID Connect users
- The method should handle cases where the user doesn't exist yet
Suggested improvements:
- Add user creation logic when authentication is not found
- Implement proper error handling
- Consider adding validation for required OpenID Connect claims
🔗 Analysis chain
Verify User model implementation for OpenID Connect.
Ensure that the User model implements the required methods for OIDC authentication.
Let me try a different approach to search for these methods using rg which might be more effective for Ruby files.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify User model implementation for OpenID Connect
# Check for the find_for_openid_connect_oauth method
echo "Checking for find_for_openid_connect_oauth method..."
ast-grep --pattern 'def self.find_for_openid_connect_oauth'
# Check for add_oauth_if_user_exists method
echo "Checking for add_oauth_if_user_exists method..."
ast-grep --pattern 'def self.add_oauth_if_user_exists'
# Check for new_with_session method
echo "Checking for new_with_session method..."
ast-grep --pattern 'def self.new_with_session'
Length of output: 476
Script:
#!/bin/bash
# Search for OIDC-related methods in User model and related files
echo "Searching for OpenID Connect related methods in User model..."
rg -A 5 "find_for_openid_connect_oauth" app/models/
echo -e "\nSearching for OAuth-related methods in User model..."
rg -A 5 "class User" app/models/user.rb
echo -e "\nChecking for any OpenID Connect related files..."
fd -t f openid
Length of output: 1163
| # Set up OpenID Connect interceptor | ||
| config.omniauth :openid_connect, setup: lambda{ |env| | ||
| env['omniauth.strategy'].options.merge!(OauthConfigController.get_oauth_credentials :openid_connect) | ||
| } |
There was a problem hiding this comment.
💡 Codebase verification
Based on the implementation details found, I can now provide a final response:
OIDC configuration has security concerns that need addressing
The current implementation has several security gaps:
- Credentials are stored in plaintext YAML files without encryption
- No validation of OIDC configuration values before use
- File permissions are not explicitly set for the config file
- Cache is used but without encryption for sensitive data
Required changes:
- Implement encryption for
client_secretstorage using Rails credentials or a secure key management solution - Add validation for OIDC configuration values in
get_oauth_credentials - Set explicit restrictive file permissions (0600) when writing
oauth_config.yml - Add input sanitization for provider names before using them as hash keys
- Consider moving sensitive data to environment variables or Rails credentials instead of YAML files
🔗 Analysis chain
Verify OIDC configuration management security.
The credentials are loaded at runtime which is good for flexibility, but ensure:
- The configuration file has appropriate access restrictions
- Sensitive OIDC credentials are properly encrypted/secured
- The configuration is validated before use
Let me gather more information about the configuration validation and security measures.
Let me try a different approach to find the implementation details.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for potential security issues in configuration management
# Look for OIDC configuration files
echo "Searching for OIDC configuration files..."
fd -t f -e yml -e yaml | rg -i "oauth|oidc"
# Check for potential credential exposure in version control
echo "Checking for potential credential exposure..."
rg -i "client_secret|client_id|private_key"
Length of output: 10075
Script:
#!/bin/bash
# Check for OIDC configuration validation and security measures
# Look for OauthConfigController implementation
echo "Checking OauthConfigController implementation..."
ast-grep --pattern 'def get_oauth_credentials'
# Check for encryption/security related code
echo "Checking for encryption/security measures..."
rg -i "encrypt|secure|validate" app/controllers/oauth_config_controller.rb
# Check for configuration storage location and access controls
echo "Checking configuration storage..."
rg -i "config.*store|save.*config" app/controllers/oauth_config_controller.rb
Length of output: 488
Script:
#!/bin/bash
# Search for the entire OauthConfigController class and related files
echo "Searching for OauthConfigController implementation..."
rg -l "class OauthConfigController"
echo "Looking for the file content..."
fd -t f oauth_config_controller.rb -x cat {}
echo "Checking for configuration files handling..."
rg -g "*.rb" -i "yaml|yml.*load|credentials.*load" app/controllers/
Length of output: 6506
This PR adds OIDC login ability to AutoLab.
Description
Changes:
Motivation and Context
By supporting OIDC, institutions like universities can easily integrate AutoLab with their account systems.
How Has This Been Tested?
Types of changes
Checklist:
overcommit --install && overcommit --signto use pre-commit hook for lintingOther issues / help required
None