Skip to content

Conversation

@kevin-atnos
Copy link
Collaborator

@kevin-atnos kevin-atnos commented Oct 17, 2023

This pull request introduces significant updates to the application, focusing on adding SAML-based single sign-on (SSO) support, improving database query performance, and enhancing configuration and session management. Below is a summary of the most important changes grouped by theme.

SAML-Based Single Sign-On (SSO) Integration:

  • Added SamlController to handle SAML metadata, SSO, logout, and SLO responses, enabling SAML-based authentication workflows (app/controllers/saml_controller.rb).
  • Configured SAML-related routes for metadata, SSO, logout, and assertion consumer services in config/routes.rb.
  • Updated devise initializer with commented-out SAML configuration options to support SAML-based user authentication (config/initializers/devise.rb).

Performance Improvements:

  • Modified Pia and PiaPolicy queries to use eager_load for preloading associated user_pias, reducing N+1 query issues (app/controllers/pias_controller.rb, app/policies/pia_policy.rb) [1] [2].

Session and Configuration Enhancements:

  • Configured Redis as the session store and enabled session management middleware for better scalability and performance (config/initializers/session_store.rb).
  • Added config.secret_key_base to config/application.rb for secure application configuration (config/application.rb).

Dependency Updates:

  • Added new gems: ruby-saml and devise_saml_authenticatable for SAML authentication, and redis with redis-actionpack for Redis-based session storage (Gemfile).

Minor Code and Syntax Improvements:

  • Updated syntax for hash shorthand and string quoting in various files for consistency (app/models/user.rb, config/application.rb) [1] [2].

user.unlock_access!
else
password = [*'0'..'9', *'a'..'z', *'A'..'Z', *'!'..'?'].sample(16).join
user = User.create!(email:, password:, password_confirmation: password)

Check failure

Code scanning / CodeQL

Clear-text storage of sensitive information

This stores sensitive data returned by [an assignment to password](1) as clear text.

Copilot Autofix

AI 8 months ago

To fix the issue, we need to ensure that the password is hashed before being stored in the database. Rails provides built-in support for securely hashing passwords using the has_secure_password method in the User model, which relies on bcrypt. This method automatically hashes the password when it is assigned to the password attribute.

The fix involves:

  1. Ensuring the User model uses has_secure_password (this is assumed to be already implemented since password and password_confirmation are used).
  2. Modifying the consume method to assign the plain-text password to the password attribute, allowing Rails to handle the hashing automatically.

Suggested changeset 1
app/controllers/saml_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/controllers/saml_controller.rb b/app/controllers/saml_controller.rb
--- a/app/controllers/saml_controller.rb
+++ b/app/controllers/saml_controller.rb
@@ -24,3 +24,3 @@
         password = SecureRandom.hex(16)
-        user = User.create!(email:, password:, password_confirmation: password)
+        user = User.create!(email:, password:)
         user.is_user = true
EOF
@@ -24,3 +24,3 @@
password = SecureRandom.hex(16)
user = User.create!(email:, password:, password_confirmation: password)
user = User.create!(email:, password:)
user.is_user = true
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
user.unlock_access!
else
password = [*'0'..'9', *'a'..'z', *'A'..'Z', *'!'..'?'].sample(16).join
user = User.create!(email:, password:, password_confirmation: password)

Check failure

Code scanning / CodeQL

Clear-text storage of sensitive information

This stores sensitive data returned by [an assignment to password](1) as clear text.

Copilot Autofix

AI 8 months ago

To fix the issue, the password should be hashed before being stored in the database. In Rails, this is typically handled by the has_secure_password method provided by the bcrypt gem. This method automatically hashes passwords when they are assigned to the password attribute of a model. The User model should already be configured to use has_secure_password for this fix to work. If it is not, additional changes to the User model will be required.

The fix involves replacing the direct assignment of the password and password_confirmation attributes with a single assignment to the password attribute. This ensures that the password is hashed before being stored.


Suggested changeset 1
app/controllers/saml_controller.rb

Autofix patch

Autofix patch
Run the following command in your local git repository to apply this patch
cat << 'EOF' | git apply
diff --git a/app/controllers/saml_controller.rb b/app/controllers/saml_controller.rb
--- a/app/controllers/saml_controller.rb
+++ b/app/controllers/saml_controller.rb
@@ -24,3 +24,3 @@
         password = SecureRandom.hex(16)
-        user = User.create!(email:, password:, password_confirmation: password)
+        user = User.create!(email:, password:)
         user.is_user = true
EOF
@@ -24,3 +24,3 @@
password = SecureRandom.hex(16)
user = User.create!(email:, password:, password_confirmation: password)
user = User.create!(email:, password:)
user.is_user = true
Copilot is powered by AI and may make mistakes. Always verify output.
Unable to commit as this autofix suggestion is now outdated
@kevin-atnos
Copy link
Collaborator Author

kevin-atnos commented Jan 24, 2024

@brunto brunto changed the base branch from master to rails_8.0 May 12, 2025 14:53
@brunto brunto changed the base branch from rails_8.0 to master May 19, 2025 20:38
@brunto brunto requested review from brunto and Copilot May 19, 2025 20:44
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR implements Single Sign-On (SSO) functionality by adding new SAML routes and controllers, updating session management with Redis, and configuring SAML settings in Devise.

  • Added SAML routes in routes.rb
  • Introduced a new SamlController that handles SSO, ACS, and SLO flows
  • Updated session store and extended Devise initializer with commented SAML config for future adjustments

Reviewed Changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
config/routes.rb Added SAML endpoints for metadata, SSO, ACS, logout, and SLO handling
config/initializers/session_store.rb Configured Redis as the session store with updated parameters
config/initializers/devise.rb Enabled email authentication and provided commented sample SAML settings
config/application.rb Minor changes with stylistic quote updates and added secret_key_base
app/policies/pia_policy.rb Switched to eager loading of associated records for performance improvements
app/models/user.rb Updated login uniqueness validation using Ruby shorthand, but with issues
app/controllers/saml_controller.rb Introduced new controller handling SSO requests and SAML attribute population
app/controllers/pias_controller.rb Adjusted association loading for performance
app/controllers/application_controller.rb Extended info API to include SSO enabled flag
Gemfile Added new dependencies for ruby-saml, devise_saml_authenticatable, and Redis

brunto
brunto previously approved these changes Jul 18, 2025
attributes_allowed = ENV['SANITIZED_ALLOWED_ATTRIBUTES'] ? ENV['SANITIZED_ALLOWED_ATTRIBUTES'].split(' ') : []
config.action_view.sanitized_allowed_attributes = attributes_allowed

config.secret_key_base = Rails.application.credentials.secret_key_base
Copy link

@mrdev023 mrdev023 Nov 25, 2025

Choose a reason for hiding this comment

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

@kevin-atnos The changes in application.rb is useless because Ruby on Rails already try to load from ENV and then from credentials the secret_key_base.

Source

Copy link

@mrdev023 mrdev023 Jan 13, 2026

Choose a reason for hiding this comment

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

@brunto I think it's better to let the choice to the sys administrator how to deploy it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants