From 168e51be528839077f204dabbc3ebf3ce607b1a6 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 4 Nov 2024 09:58:00 -0600 Subject: [PATCH 01/22] initial saml implementation --- Gemfile | 2 +- Gemfile.lock | 17 +++--- .../users/omniauth_callbacks_controller.rb | 17 ++++++ app/models/user.rb | 52 +++++-------------- app/views/_user_util_links.html.erb | 2 +- config/environments/production.rb | 20 +++++++ config/initializers/devise.rb | 26 +--------- config/initializers/omniauth.rb | 17 ++++++ config/routes.rb | 4 +- 9 files changed, 81 insertions(+), 76 deletions(-) create mode 100644 app/controllers/users/omniauth_callbacks_controller.rb create mode 100644 config/initializers/omniauth.rb diff --git a/Gemfile b/Gemfile index 3d0084a3..b6f57b84 100644 --- a/Gemfile +++ b/Gemfile @@ -11,7 +11,7 @@ gem 'coffee-rails', '~> 4.2' gem 'dalli' gem 'devise' gem 'devise-guests', '~> 0.8' -gem "devise_saml_authenticatable", git: "https://github.com/apokalipto/devise_saml_authenticatable" +gem 'omniauth-saml' gem 'dotenv-rails' gem 'hydra-role-management' gem 'hyrax', git: 'https://github.com/samvera/hyrax', ref: '9c58751' diff --git a/Gemfile.lock b/Gemfile.lock index f0e5de97..bb045da1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -1,11 +1,3 @@ -GIT - remote: https://github.com/apokalipto/devise_saml_authenticatable - revision: f8a85c3f3ed069fd40701424e2eef075219ef17f - specs: - devise_saml_authenticatable (1.9.1) - devise (> 2.0.0) - ruby-saml (~> 1.7) - GIT remote: https://github.com/brentd/xray-rails revision: f121814718c9907b20058dc9357b80a53afab821 @@ -792,6 +784,13 @@ GEM multi_json (~> 1.3) multi_xml (~> 0.5) rack (>= 1.2, < 4) + omniauth (2.1.2) + hashie (>= 3.4.6) + rack (>= 2.2.3) + rack-protection + omniauth-saml (2.2.1) + omniauth (~> 2.1) + ruby-saml (~> 1.17) openseadragon (0.7.0) rails (> 6.1.0) orm_adapter (0.5.0) @@ -1264,7 +1263,6 @@ DEPENDENCIES debug (>= 1.0.0) devise devise-guests (~> 0.8) - devise_saml_authenticatable! dotenv-rails ed25519 (>= 1.2, < 2.0) factory_bot_rails @@ -1274,6 +1272,7 @@ DEPENDENCIES jquery-rails listen (>= 3.0.5, < 3.2) mutex_m (~> 0.2.0) + omniauth-saml pg (~> 1.3) pry-doc pry-rails diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb new file mode 100644 index 00000000..99136748 --- /dev/null +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -0,0 +1,17 @@ +class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController + def saml + @user = User.from_omniauth(request.env["omniauth.auth"]) + + if @user.persisted? + sign_in_and_redirect @user, event: :authentication + set_flash_message(:notice, :success, kind: "SAML") if is_navigational_format? + else + session["devise.saml_data"] = request.env["omniauth.auth"].except(:extra) + redirect_to new_user_registration_url + end + end + + def failure + redirect_to root_path + end +end \ No newline at end of file diff --git a/app/models/user.rb b/app/models/user.rb index 731b77d5..f7afd577 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -12,8 +12,8 @@ class User < ApplicationRecord include Blacklight::User # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable - devise :database_authenticatable, :saml_authenticatable, :registerable, - :recoverable, :rememberable, :validatable + devise :database_authenticatable, :registerable, + :recoverable, :rememberable, :validatable, :omniauthable, omniauth_providers: [:saml] # temporary fix for SAML login while we have a hybrid login validates :password, presence: true, if: :password_required? @@ -23,49 +23,23 @@ def password_required? end def saml_authenticatable? - uid.present? + provider == 'saml' && uid.present? end - # Method added by Blacklight; Blacklight uses #to_s on your - # user class to get a user-displayable login/identifier for - # the account. def to_s email end - def self.from_saml(auth) - return unless Rails.env.production? - user = find_or_initialize_by(email: auth.info.mail) - set_user_attributes(user, auth) - - if user.save - user - else - log_saml_error(auth) - User.new - end - end - - def self.log_saml_error(auth) - if auth.info.mail.blank? - Rails.logger.error "Nil user detected: SAML didn't pass an email for #{auth.inspect}" - else - Rails.logger.error "Failed to create/update user: #{auth.inspect}" + def self.from_omniauth(auth) + email = auth.info.net_id + '@emory.edu' unless auth.info.net_id == 'tezproxy' + where(email: email, uid: auth.info.net_id).first_or_create do |user| + user.email = email + user.display_name = auth.info.displayName + user.department = auth.info.ou + user.title = auth.info.title + user.password = SecureRandom.hex(16) + user.password_confirmation = user.password end + user end end - -private - -def set_user_attributes(user, auth) - password = SecureRandom.hex(16) - user.assign_attributes( - display_name: auth.info.displayName, - department: auth.info.ou, - title: auth.info.title, - uid: auth.uid, - # set a random password temporarily to allow a hybrid login - password:, - password_confirmation: password - ) -end diff --git a/app/views/_user_util_links.html.erb b/app/views/_user_util_links.html.erb index 1f6075b5..9750b361 100644 --- a/app/views/_user_util_links.html.erb +++ b/app/views/_user_util_links.html.erb @@ -24,7 +24,7 @@ <% end %> <% end %> \ No newline at end of file diff --git a/config/environments/production.rb b/config/environments/production.rb index 6265ee4e..3b64cfe1 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -95,4 +95,24 @@ # Do not dump schema after migrations. config.active_record.dump_schema_after_migration = false + + # OmniAuth configuration settings + config.idp_slo_target_url = ENV['IDP_SLO_TARGET_URL'] + config.assertion_consumer_service_url = ENV['ASSERTION_CS_URL'] + config.assertion_consumer_logout_service_url = ENV['ASSERTION_LOGOUT_URL'] + config.issuer = ENV['ISSUER'] + config.idp_sso_target_url = ENV['IDP_SSO_TARGET_URL'] + config.idp_cert = ENV['IDP_CERT'] + config.certificate = ENV['SP_CERT'] + config.private_key = ENV['SP_KEY'] + config.attribute_statements = { + :net_id => ["urn:oid:0.9.2342.19200300.100.1.1"], + :first_name => ["urn:oid:1.3.6.1.4.1.5923.1.1.1.2"], + :last_name => ["urn:oid:2.5.4.4"] + } + config.uid_attribute = "urn:oid:2.5.4.5" + config.security = { + :want_assertions_encrypted => true, #makes a 2nd KeyDescriptor, this one says use="encryption" + :want_assertions_signed => true # goes on md SPSSODescriptor tag + } end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index a6b66e1d..8297dd11 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -272,7 +272,7 @@ # Add a new OmniAuth provider. Check the wiki for more information on setting # up on your models and hooks. # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' - + config.omniauth :saml # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or # change the failure app, you can configure them inside the config.warden block. @@ -308,28 +308,4 @@ # When set to false, does not sign a user in automatically after their password is # changed. Defaults to true, so a user is signed in automatically after changing a password. # config.sign_in_after_change_password = true - - config.saml_route_helper_prefix = 'saml' - - # Configure with your SAML settings (see ruby-saml's README for more information: https://github.com/onelogin/ruby-saml). - config.saml_configure do |settings| - base_url = ApplicationUrl.base_url - settings.assertion_consumer_service_url = "https://#{base_url}/users/saml/auth/" - settings.sp_entity_id = "https://#{base_url}/users/saml/metadata" - settings.assertion_consumer_service_binding = "urn:oasis:names:tc:SAML:2.0:bindings:HTTP-POST" - settings.name_identifier_format = "urn:oasis:names:tc:SAML:2.0:nameid-format:transient" - settings.authn_context = "" - settings.idp_slo_service_url = "https://shib.open.library.emory.edu/Shibboleth.sso/SLO/POST" - settings.idp_sso_service_url = "https://shib.open.library.emory.edu/Shibboleth.sso/Login" - # used the use="signing" key below - settings.idp_cert_fingerprint = ENV['IDP_CERT_FINGERPRINT'] - settings.idp_cert_fingerprint_algorithm = "http://www.w3.org/2000/09/xmldsig#sha1" - end - - config.saml_create_user = true - config.saml_update_user = true - config.saml_default_user_key = :email - config.saml_session_index_key = :session_index - config.saml_use_subject = true - config.saml_resource_locator = ->(model, saml_response) { model.from_saml(saml_response) } end diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb new file mode 100644 index 00000000..bb71103b --- /dev/null +++ b/config/initializers/omniauth.rb @@ -0,0 +1,17 @@ +Rails.application.config.middleware.use OmniAuth::Builder do + provider :saml, + :assertion_consumer_service_url => Rails.application.config.assertion_consumer_service_url, + :assertion_consumer_logout_service_url => Rails.application.config.assertion_consumer_logout_service_url, + :issuer => Rails.application.config.issuer, + :idp_sso_target_url => Rails.application.config.idp_sso_target_url, + :idp_slo_target_url => Rails.application.config.idp_slo_target_url, + :idp_cert => Rails.application.config.idp_cert, + :certificate => Rails.application.config.certificate, + :private_key => Rails.application.config.private_key, + :attribute_statements => Rails.application.config.attribute_statements, + :uid_attribute => Rails.application.config.uid_attribute, + :security => Rails.application.config.security +end + +# Protect against forgery attacks +OmniAuth.config.allowed_request_methods = [:post] \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index ebc0956a..41ab2c62 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -14,7 +14,9 @@ concerns :searchable end - devise_for :users, controllers: { saml_sessions: 'saml_sessions' } + devise_for :users, controllers: { + omniauth_callbacks: 'users/omniauth_callbacks' + } mount Hydra::RoleManagement::Engine => '/' From ff39ec5cd72ac8cd0804eb17cacd57398a076c0b Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Thu, 7 Nov 2024 09:49:52 -0600 Subject: [PATCH 02/22] updates functionality for saml --- app/controllers/omniauth_controller.rb | 10 ++ app/controllers/saml_sessions_controller.rb | 79 ------------ .../users/omniauth_callbacks_controller.rb | 9 +- app/models/auth_config.rb | 9 ++ app/models/user.rb | 65 ++++++++-- config/environments/production.rb | 2 +- config/initializers/devise.rb | 2 +- config/initializers/omniauth.rb | 26 ++-- config/routes.rb | 20 ++- ...07154801_add_provider_and_ppid_to_users.rb | 6 + db/schema.rb | 4 +- .../saml_sessions_controller_spec.rb | 117 ------------------ 12 files changed, 123 insertions(+), 226 deletions(-) create mode 100644 app/controllers/omniauth_controller.rb delete mode 100644 app/controllers/saml_sessions_controller.rb create mode 100644 app/models/auth_config.rb create mode 100644 db/migrate/20241107154801_add_provider_and_ppid_to_users.rb delete mode 100644 spec/controllers/saml_sessions_controller_spec.rb diff --git a/app/controllers/omniauth_controller.rb b/app/controllers/omniauth_controller.rb new file mode 100644 index 00000000..2ed1cd30 --- /dev/null +++ b/app/controllers/omniauth_controller.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true +class OmniauthController < Devise::SessionsController + def new + if Rails.env.production? + redirect_to user_shibboleth_omniauth_authorize_path + else + super + end + end +end diff --git a/app/controllers/saml_sessions_controller.rb b/app/controllers/saml_sessions_controller.rb deleted file mode 100644 index b67626a3..00000000 --- a/app/controllers/saml_sessions_controller.rb +++ /dev/null @@ -1,79 +0,0 @@ -# frozen_string_literal: true -class SamlSessionsController < Devise::SamlSessionsController - before_action :store_redirect_path, only: [:new] - before_action :check_environment - - def new - super - end - - def create - authenticate_resource - handle_authenticated_resource - rescue StandardError => e - handle_authentication_error(e) - end - - def after_sign_in_path_for(resource) - stored_location_for(resource) || root_path - end - - def after_sign_out_path_for(_resource_or_scope) - root_path - end - - private - - def authenticate_resource - self.resource = warden.authenticate!(auth_options) - end - - def handle_authenticated_resource - if resource&.persisted? - sign_in_resource - else - handle_invalid_resource - end - end - - def sign_in_resource - set_flash_message!(:notice, :signed_in) - sign_in(resource_name, resource) - yield resource if block_given? - respond_with resource, location: after_sign_in_path_for(resource) - end - - def handle_invalid_resource - error_flash_and_redirect("Unable to create or update user account.") - end - - def handle_authentication_error(error) - Rails.logger.error("SAML authentication error: #{error.message}") - error_flash_and_redirect("Unable to create or update user account.") - end - - def error_flash_and_redirect(reason) - flash[:alert] = I18n.t("devise.failure.saml_invalid", reason:) - redirect_to root_path - end - - protected - - def store_redirect_path - if params[:redirect_to].present? - store_location_for(:user, params[:redirect_to]) - elsif request.referer.present? && request.referer != new_saml_user_session_url - store_location_for(:user, request.referer) - end - end - - def stored_redirect_path_or_default - stored_location_for(:user) || root_path - end - - def check_environment - return if Rails.env.production? - flash[:alert] = "SAML authentication is only available in production." - redirect_to new_user_session_path - end -end diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 99136748..2752242b 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -3,11 +3,12 @@ def saml @user = User.from_omniauth(request.env["omniauth.auth"]) if @user.persisted? - sign_in_and_redirect @user, event: :authentication - set_flash_message(:notice, :success, kind: "SAML") if is_navigational_format? + sign_in @user + redirect_to request.env["omniauth.origin"] || hyrax.dashboard_path + set_flash_message(:notice, :success, kind: "SAML") else - session["devise.saml_data"] = request.env["omniauth.auth"].except(:extra) - redirect_to new_user_registration_url + redirect_to root_path + set_flash_message(:notice, :failure, kind: "SAML", reason: "you aren't authorized to use this application.") end end diff --git a/app/models/auth_config.rb b/app/models/auth_config.rb new file mode 100644 index 00000000..1f6b1f7f --- /dev/null +++ b/app/models/auth_config.rb @@ -0,0 +1,9 @@ +# frozen_string_literal: true +class AuthConfig + # In production, we use Shibboleth for user authentication, + # but in development mode, you may want to use local database + # authentication instead. + def self.use_database_auth? + ENV['DATABASE_AUTH'] == 'true' + end +end diff --git a/app/models/user.rb b/app/models/user.rb index f7afd577..0d5818cc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,13 +7,24 @@ class User < ApplicationRecord # Connects this user object to Hyrax behaviors. include Hyrax::User include Hyrax::UserUsageStats + + class NilSamlUserError < RuntimeError + attr_accessor :auth + + def initialize(message = nil, auth = nil) + super(message) + self.auth = auth + end + end # Connects this user object to Blacklights Bookmarks. include Blacklight::User # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable - devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :validatable, :omniauthable, omniauth_providers: [:saml] + devise_modules = [:registerable, :recoverable, :rememberable, :validatable, :omniauthable, omniauth_providers: [:saml]] + devise_modules.prepend(:database_authenticatable) if AuthConfig.use_database_auth? + devise(*devise_modules) + # temporary fix for SAML login while we have a hybrid login validates :password, presence: true, if: :password_required? @@ -31,15 +42,49 @@ def to_s end def self.from_omniauth(auth) - email = auth.info.net_id + '@emory.edu' unless auth.info.net_id == 'tezproxy' - where(email: email, uid: auth.info.net_id).first_or_create do |user| - user.email = email - user.display_name = auth.info.displayName - user.department = auth.info.ou - user.title = auth.info.title - user.password = SecureRandom.hex(16) - user.password_confirmation = user.password + begin + user = find_by!(provider: auth.provider, uid: auth.info.net_id) + rescue ActiveRecord::RecordNotFound + log_omniauth_error(auth) + return User.new end + + user.assign_attributes( + display_name: auth.info.display_name, + ppid: auth.uid + ) + + # Set email only if net_id is not 'tezprox' + if auth.info.net_id != 'tezprox' + user.email = auth.info.net_id + '@emory.edu' + end + + user.save user end + + def self.log_omniauth_error(auth) + if auth.info.net_id.blank? + Rails.logger.error "Nil user detected: SAML didn't pass a net_id for #{auth.inspect}" + else + # Log unauthorized logins to error + Rails.logger.error "Unauthorized user attempted login: #{auth.inspect}" + end + end +end + +# Override a Hyrax class that expects to create system users with passwords. +# Since in production we're using shibboleth, and this removes the password +# methods from the User model, we need to override it. +module Hyrax::User + module ClassMethods + def find_or_create_system_user(user_key) + u = ::User.find_or_create_by(uid: user_key) + u.display_name = user_key + u.email = "#{user_key}@example.com" + u.password = ('a'..'z').to_a.shuffle(random: Random.new).join if AuthConfig.use_database_auth? + u.save + u + end + end end diff --git a/config/environments/production.rb b/config/environments/production.rb index 3b64cfe1..7b8be4de 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -107,7 +107,7 @@ config.private_key = ENV['SP_KEY'] config.attribute_statements = { :net_id => ["urn:oid:0.9.2342.19200300.100.1.1"], - :first_name => ["urn:oid:1.3.6.1.4.1.5923.1.1.1.2"], + :display_name => ["urn:oid:1.3.6.1.4.1.5923.1.1.1.2"], :last_name => ["urn:oid:2.5.4.4"] } config.uid_attribute = "urn:oid:2.5.4.5" diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 8297dd11..1f399a20 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -46,7 +46,7 @@ # session. If you need permissions, you should implement that in a before filter. # You can also supply a hash where the value is a boolean determining whether # or not authentication should be aborted when the value is not present. - # config.authentication_keys = [:email] + config.authentication_keys = [:uid] # Configure parameters from the request object used for authentication. Each entry # given should be a request method and it will automatically be passed to the diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index bb71103b..585d3921 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,16 +1,18 @@ Rails.application.config.middleware.use OmniAuth::Builder do - provider :saml, - :assertion_consumer_service_url => Rails.application.config.assertion_consumer_service_url, - :assertion_consumer_logout_service_url => Rails.application.config.assertion_consumer_logout_service_url, - :issuer => Rails.application.config.issuer, - :idp_sso_target_url => Rails.application.config.idp_sso_target_url, - :idp_slo_target_url => Rails.application.config.idp_slo_target_url, - :idp_cert => Rails.application.config.idp_cert, - :certificate => Rails.application.config.certificate, - :private_key => Rails.application.config.private_key, - :attribute_statements => Rails.application.config.attribute_statements, - :uid_attribute => Rails.application.config.uid_attribute, - :security => Rails.application.config.security + if !AuthConfig.use_database_auth? + provider :saml, + assertion_consumer_service_url: Rails.application.config.assertion_consumer_service_url, + assertion_consumer_logout_service_url: Rails.application.config.assertion_consumer_logout_service_url, + issuer: Rails.application.config.issuer, + idp_sso_target_url: Rails.application.config.idp_sso_target_url, + idp_slo_target_url: Rails.application.config.idp_slo_target_url, + idp_cert: Rails.application.config.idp_cert, + certificate: Rails.application.config.certificate, + private_key: Rails.application.config.private_key, + attribute_statements: Rails.application.config.attribute_statements, + uid_attribute: Rails.application.config.uid_attribute, + security: Rails.application.config.security + end end # Protect against forgery attacks diff --git a/config/routes.rb b/config/routes.rb index 41ab2c62..5c582e43 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -14,10 +14,18 @@ concerns :searchable end - devise_for :users, controllers: { + devise_for :users, skip: [:registrations], controllers: { omniauth_callbacks: 'users/omniauth_callbacks' } + unless AuthConfig.use_database_auth? + devise_scope :user do + get 'sign_in', to: 'omniauth#new', as: :new_user_session + post 'sign_in', to: 'users/omniauth_callbacks#saml', as: :new_session + get 'sign_out', to: 'devise/sessions#destroy', as: :destroy_user_session + end + end + mount Hydra::RoleManagement::Engine => '/' mount Sidekiq::Web => '/sidekiq' @@ -41,4 +49,14 @@ end end # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html + + # Disable these routes if you are using Devise's + # database_authenticatable in your development environment. + unless AuthConfig.use_database_auth? + devise_scope :user do + get 'sign_in', to: 'omniauth#new', as: :new_user_session + post 'sign_in', to: 'users/omniauth_callbacks#saml', as: :new_session + get 'sign_out', to: 'devise/sessions#destroy', as: :destroy_user_session + end + end end diff --git a/db/migrate/20241107154801_add_provider_and_ppid_to_users.rb b/db/migrate/20241107154801_add_provider_and_ppid_to_users.rb new file mode 100644 index 00000000..939975b2 --- /dev/null +++ b/db/migrate/20241107154801_add_provider_and_ppid_to_users.rb @@ -0,0 +1,6 @@ +class AddProviderAndPpidToUsers < ActiveRecord::Migration[6.1] + def change + add_column :users, :provider, :string + add_column :users, :ppid, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 2658acfc..58d435b2 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 2024_09_23_144555) do +ActiveRecord::Schema.define(version: 2024_11_07_154801) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -702,6 +702,8 @@ t.string "zotero_userid" t.string "preferred_locale" t.string "uid" + t.string "provider" + t.string "ppid" t.index ["email"], name: "index_users_on_email", unique: true t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end diff --git a/spec/controllers/saml_sessions_controller_spec.rb b/spec/controllers/saml_sessions_controller_spec.rb deleted file mode 100644 index 85c08745..00000000 --- a/spec/controllers/saml_sessions_controller_spec.rb +++ /dev/null @@ -1,117 +0,0 @@ -# frozen_string_literal: true -require 'rails_helper' - -RSpec.describe SamlSessionsController, type: :controller do - include Devise::Test::ControllerHelpers - - let(:warden) { double(Warden::Proxy) } - let(:user) { instance_double(User, persisted?: true) } - - before do - @request.env["devise.mapping"] = Devise.mappings[:user] - @request.env['warden'] = warden - allow(warden).to receive(:authenticate?).and_return(false) - allow(warden).to receive(:set_user) - allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) - end - - describe "GET #new" do - before do - allow_any_instance_of(OneLogin::RubySaml::Authrequest).to receive(:create).and_return("http://idp.example.com/saml/auth") - end - - it "stores the redirect_to parameter" do - get :new, params: { redirect_to: '/dashboard' } - expect(session['user_return_to']).to eq('/dashboard') - end - - it "stores the referer if no redirect_to parameter" do - request.env['HTTP_REFERER'] = '/previous_page' - get :new - expect(session['user_return_to']).to eq('/previous_page') - end - - it "doesn't store referer if it matches new_user_session_url" do - request.env['HTTP_REFERER'] = new_saml_user_session_url - get :new - expect(session['user_return_to']).to be_nil - end - - it "initiates SAML authentication" do - get :new - expect(response).to redirect_to("http://idp.example.com/saml/auth") - end - end - - describe "POST #create" do - context "in production environment" do - before do - allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) - end - - it "sets a success notice and redirects to after_sign_in_path when authentication is successful" do - allow(warden).to receive(:authenticate!).and_return(user) - allow(warden).to receive(:user).with(:user).and_return(user) - allow(controller).to receive(:after_sign_in_path_for).with(user).and_return('/dashboard') - post :create - expect(flash[:notice]).to eq(I18n.t("devise.sessions.signed_in")) - expect(response).to redirect_to('/dashboard') - end - - it "sets an alert and redirects to root_path when authentication fails" do - allow(warden).to receive(:authenticate!).and_return(nil) - allow(warden).to receive(:user).with(:user).and_return(nil) - post :create - expect(flash[:alert]).to eq(I18n.t("devise.failure.saml_invalid", reason: "Unable to create or update user account.")) - expect(response).to redirect_to(root_path) - end - end - - context "in non-production environment" do - before do - allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("development")) - end - - it "redirects to sign in path with an alert" do - post :create - expect(response).to redirect_to(new_user_session_path) - expect(flash[:alert]).to eq("SAML authentication is only available in production.") - end - end - end - - describe "#after_sign_in_path_for" do - let(:user) { instance_double(User) } - - it "returns stored location if available" do - allow(controller).to receive(:stored_location_for).with(user).and_return('/stored_path') - expect(controller.after_sign_in_path_for(user)).to eq('/stored_path') - end - - it "returns root path if no stored location" do - allow(controller).to receive(:stored_location_for).with(user).and_return(nil) - expect(controller.after_sign_in_path_for(user)).to eq(root_path) - end - end - - describe "#check_environment" do - context "in production" do - before { allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("production")) } - - it "allows access" do - get :new - expect(response).not_to redirect_to(new_user_session_path) - end - end - - context "in non-production" do - before { allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new("development")) } - - it "redirects to sign in path with an alert" do - get :new - expect(response).to redirect_to(new_user_session_path) - expect(flash[:alert]).to eq("SAML authentication is only available in production.") - end - end - end -end From eb0b063b02eedb97b9cc9139dc2a78de9a1c3094 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Thu, 7 Nov 2024 10:27:07 -0600 Subject: [PATCH 03/22] fixes rubocop errors --- Gemfile | 3 +- Gemfile.lock | 28 +++++++++++++++++++ .../users/omniauth_callbacks_controller.rb | 6 ++-- app/models/user.rb | 20 +++++++------ config/environments/production.rb | 12 ++++---- config/initializers/omniauth.rb | 6 ++-- 6 files changed, 55 insertions(+), 20 deletions(-) diff --git a/Gemfile b/Gemfile index b6f57b84..f2274532 100644 --- a/Gemfile +++ b/Gemfile @@ -11,13 +11,13 @@ gem 'coffee-rails', '~> 4.2' gem 'dalli' gem 'devise' gem 'devise-guests', '~> 0.8' -gem 'omniauth-saml' gem 'dotenv-rails' gem 'hydra-role-management' gem 'hyrax', git: 'https://github.com/samvera/hyrax', ref: '9c58751' gem 'jbuilder', '~> 2.5' gem 'jquery-rails' gem 'mutex_m', '~> 0.2.0' +gem 'omniauth-saml' gem 'pg', '~> 1.3' gem 'puma' gem 'rails', '~> 6.1' @@ -33,6 +33,7 @@ gem 'uglifier', '>= 1.3.0' group :development do # Add command line in browser when errors gem 'better_errors' + gem 'solargraph' # Add deeper stack trace used by better errors gem 'binding_of_caller' diff --git a/Gemfile.lock b/Gemfile.lock index bb045da1..e530f944 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -186,6 +186,7 @@ GEM babel-transpiler (0.7.0) babel-source (>= 4.0, < 6) execjs (~> 2.0) + backport (1.2.0) bagit (0.4.6) docopt (~> 0.5.0) validatable (~> 1.6) @@ -195,6 +196,7 @@ GEM bcrypt_pbkdf (1.1.1) bcrypt_pbkdf (1.1.1-arm64-darwin) bcrypt_pbkdf (1.1.1-x86_64-darwin) + benchmark (0.3.0) better_errors (2.10.1) erubi (>= 1.0.0) rack (>= 0.9.0) @@ -399,6 +401,7 @@ GEM dry-initializer (~> 3.0) dry-schema (>= 1.12, < 2) zeitwerk (~> 2.6) + e2mmap (0.1.0) ebnf (2.4.0) htmlentities (~> 4.3) rdf (~> 3.3) @@ -603,6 +606,7 @@ GEM rdoc (>= 4.0.0) reline (>= 0.4.2) iso8601 (0.9.1) + jaro_winkler (1.6.0) jbuilder (2.12.0) actionview (>= 5.0.0) activesupport (>= 5.0.0) @@ -640,6 +644,10 @@ GEM activerecord kaminari-core (= 1.2.2) kaminari-core (1.2.2) + kramdown (2.4.0) + rexml + kramdown-parser-gfm (1.1.0) + kramdown (~> 2.0) language_list (1.2.1) language_server-protocol (3.17.0.3) launchy (3.0.1) @@ -882,6 +890,7 @@ GEM rb-fsevent (0.11.2) rb-inotify (0.11.1) ffi (~> 1.0) + rbs (2.8.4) rdf (3.3.2) bcp47_spec (~> 0.2) bigdecimal (~> 3.1, >= 3.1.5) @@ -985,6 +994,8 @@ GEM actionpack (>= 5.2) railties (>= 5.2) retriable (3.1.2) + reverse_markdown (2.1.1) + nokogiri rexml (3.3.9) riiif (2.5.0) deprecation (>= 1.0.0) @@ -1118,6 +1129,22 @@ GEM snaky_hash (2.0.1) hashie version_gem (~> 1.1, >= 1.1.1) + solargraph (0.50.0) + backport (~> 1.2) + benchmark + bundler (~> 2.0) + diff-lcs (~> 1.4) + e2mmap + jaro_winkler (~> 1.5) + kramdown (~> 2.3) + kramdown-parser-gfm (~> 1.1) + parser (~> 3.0) + rbs (~> 2.0) + reverse_markdown (~> 2.0) + rubocop (~> 1.38) + thor (~> 1.0) + tilt (~> 2.0) + yard (~> 0.9, >= 0.9.24) sparql (3.3.0) builder (~> 3.2, >= 3.2.4) ebnf (~> 2.4) @@ -1292,6 +1319,7 @@ DEPENDENCIES sass-rails (~> 6.0) selenium-webdriver (~> 4.4) sidekiq (~> 6.4) + solargraph spring spring-watcher-listen (~> 2.0.0) turbolinks (~> 5) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index 2752242b..5389e043 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -1,10 +1,12 @@ +# frozen_string_literal: true + class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController def saml @user = User.from_omniauth(request.env["omniauth.auth"]) if @user.persisted? sign_in @user - redirect_to request.env["omniauth.origin"] || hyrax.dashboard_path + redirect_to request.env["omniauth.origin"] || hyrax.dashboard_path set_flash_message(:notice, :success, kind: "SAML") else redirect_to root_path @@ -15,4 +17,4 @@ def saml def failure redirect_to root_path end -end \ No newline at end of file +end diff --git a/app/models/user.rb b/app/models/user.rb index 0d5818cc..f67dbdd0 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -4,10 +4,10 @@ class User < ApplicationRecord include Hydra::User # Connects this user object to Role-management behaviors. include Hydra::RoleManagement::UserRoles - # Connects this user object to Hyrax behaviors. + include Hyrax::User include Hyrax::UserUsageStats - + class NilSamlUserError < RuntimeError attr_accessor :auth @@ -49,20 +49,22 @@ def self.from_omniauth(auth) return User.new end + assign_user_attributes(user, auth) + user.save + user + end + + def self.assign_user_attributes(user, auth) user.assign_attributes( display_name: auth.info.display_name, ppid: auth.uid ) - # Set email only if net_id is not 'tezprox' - if auth.info.net_id != 'tezprox' - user.email = auth.info.net_id + '@emory.edu' - end - - user.save - user + user.email = "#{auth.info.net_id}@emory.edu" unless auth.info.net_id == 'tezprox' end + private_class_method :assign_user_attributes + def self.log_omniauth_error(auth) if auth.info.net_id.blank? Rails.logger.error "Nil user detected: SAML didn't pass a net_id for #{auth.inspect}" diff --git a/config/environments/production.rb b/config/environments/production.rb index 7b8be4de..a392f4e9 100644 --- a/config/environments/production.rb +++ b/config/environments/production.rb @@ -106,13 +106,13 @@ config.certificate = ENV['SP_CERT'] config.private_key = ENV['SP_KEY'] config.attribute_statements = { - :net_id => ["urn:oid:0.9.2342.19200300.100.1.1"], - :display_name => ["urn:oid:1.3.6.1.4.1.5923.1.1.1.2"], - :last_name => ["urn:oid:2.5.4.4"] - } + net_id: ["urn:oid:0.9.2342.19200300.100.1.1"], + display_name: ["urn:oid:1.3.6.1.4.1.5923.1.1.1.2"], + last_name: ["urn:oid:2.5.4.4"] + } config.uid_attribute = "urn:oid:2.5.4.5" config.security = { - :want_assertions_encrypted => true, #makes a 2nd KeyDescriptor, this one says use="encryption" - :want_assertions_signed => true # goes on md SPSSODescriptor tag + want_assertions_encrypted: true, # Makes a 2nd KeyDescriptor, this one says use="encryption" + want_assertions_signed: true # Goes on md SPSSODescriptor tag } end diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index 585d3921..3a4413b0 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,5 +1,7 @@ +# frozen_string_literal: true + Rails.application.config.middleware.use OmniAuth::Builder do - if !AuthConfig.use_database_auth? + unless AuthConfig.use_database_auth? provider :saml, assertion_consumer_service_url: Rails.application.config.assertion_consumer_service_url, assertion_consumer_logout_service_url: Rails.application.config.assertion_consumer_logout_service_url, @@ -16,4 +18,4 @@ end # Protect against forgery attacks -OmniAuth.config.allowed_request_methods = [:post] \ No newline at end of file +OmniAuth.config.allowed_request_methods = [:post] From d3c9fcd8bf4e76fb0f942faa53761131af6130a9 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Thu, 7 Nov 2024 10:45:00 -0600 Subject: [PATCH 04/22] sets database_auth to true --- dotenv.sample | 1 + 1 file changed, 1 insertion(+) diff --git a/dotenv.sample b/dotenv.sample index 380540d0..7927ddb4 100644 --- a/dotenv.sample +++ b/dotenv.sample @@ -51,3 +51,4 @@ FITS_SERVLET_VERSION= FEDORA_VERSION= OPENEMORY_WORKFLOW_ADMIN_SET_ID= OPENEMORY_COLLECTION_ID= +DATABASE_AUTH=true From 421200a9684ac3fbb67c8559fe417e5d2d8d8bb3 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Sun, 10 Nov 2024 16:30:31 -0600 Subject: [PATCH 05/22] sets checks for environment --- config/initializers/omniauth.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index 3a4413b0..d1642e5c 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true Rails.application.config.middleware.use OmniAuth::Builder do - unless AuthConfig.use_database_auth? + if !Rails.env.development? && !Rails.env.test? provider :saml, assertion_consumer_service_url: Rails.application.config.assertion_consumer_service_url, assertion_consumer_logout_service_url: Rails.application.config.assertion_consumer_logout_service_url, From a175d138fa711fe80b7a48c0ae4a9d2295bc323f Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Sun, 10 Nov 2024 16:44:48 -0600 Subject: [PATCH 06/22] sets empty variables --- config/initializers/1_saml_config.rb | 26 ++++++++++++++++++++++++++ 1 file changed, 26 insertions(+) create mode 100644 config/initializers/1_saml_config.rb diff --git a/config/initializers/1_saml_config.rb b/config/initializers/1_saml_config.rb new file mode 100644 index 00000000..756c6eec --- /dev/null +++ b/config/initializers/1_saml_config.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true +Rails.application.config.class.class_eval do + attr_accessor :assertion_consumer_service_url, + :assertion_consumer_logout_service_url, + :issuer, + :idp_sso_target_url, + :idp_slo_target_url, + :idp_cert, + :certificate, + :private_key, + :attribute_statements, + :uid_attribute, + :security +end + +Rails.application.config.assertion_consumer_service_url = nil +Rails.application.config.assertion_consumer_logout_service_url = nil +Rails.application.config.issuer = nil +Rails.application.config.idp_sso_target_url = nil +Rails.application.config.idp_slo_target_url = nil +Rails.application.config.idp_cert = nil +Rails.application.config.certificate = nil +Rails.application.config.private_key = nil +Rails.application.config.attribute_statements = {} +Rails.application.config.uid_attribute = nil +Rails.application.config.security = {} From d09e8bb44bb09d5426dea66a459bc90a3eb55b0e Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Sun, 10 Nov 2024 17:02:06 -0600 Subject: [PATCH 07/22] fixing routes --- config/routes.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 5c582e43..cda2e8be 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -20,9 +20,9 @@ unless AuthConfig.use_database_auth? devise_scope :user do - get 'sign_in', to: 'omniauth#new', as: :new_user_session - post 'sign_in', to: 'users/omniauth_callbacks#saml', as: :new_session - get 'sign_out', to: 'devise/sessions#destroy', as: :destroy_user_session + get 'sign_in', to: 'omniauth#new' + post 'sign_in', to: 'users/omniauth_callbacks#saml' + get 'sign_out', to: 'devise/sessions#destroy' end end From 161aa16733617e690ab08a44c5b8b96a48d5d88c Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Sun, 10 Nov 2024 17:10:16 -0600 Subject: [PATCH 08/22] removes devise_saml_authenticatable --- spec/rails_helper.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 0a49136e..bfac53d7 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -10,7 +10,6 @@ # Add additional requires below this line. Rails is not loaded until this point! require 'devise' -require 'devise_saml_authenticatable' require 'hyrax/specs/shared_specs/factories/strategies/json_strategy' require 'hyrax/specs/shared_specs/factories/strategies/valkyrie_resource' require 'hyrax/specs/shared_specs/factories/users' From bc2881b00e0bee56f83c591cd3d7ab15379673d6 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Sun, 10 Nov 2024 18:02:51 -0600 Subject: [PATCH 09/22] fixing circle ci --- .circleci/config.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.circleci/config.yml b/.circleci/config.yml index ab754aee..71477bba 100644 --- a/.circleci/config.yml +++ b/.circleci/config.yml @@ -52,6 +52,7 @@ jobs: HYRAX_VALKYRIE: true RACK_ENV: test RAILS_ENV: test + DATABASE_AUTH: true SOLR_TEST_URL: http://127.0.0.1:8983/solr/dlp-selfdeposit-test SPEC_OPTS: >- --profile 10 --format RspecJunitFormatter --out /tmp/test-results/rspec.xml From af789046b57f7518bc191816566cb995c343ed2d Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Sun, 10 Nov 2024 18:35:14 -0600 Subject: [PATCH 10/22] fixes user registrations --- app/models/user.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index f67dbdd0..419c6257 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -22,7 +22,7 @@ def initialize(message = nil, auth = nil) # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable devise_modules = [:registerable, :recoverable, :rememberable, :validatable, :omniauthable, omniauth_providers: [:saml]] - devise_modules.prepend(:database_authenticatable) if AuthConfig.use_database_auth? + devise_modules.prepend(:database_authenticatable, :registerable) if AuthConfig.use_database_auth? devise(*devise_modules) # temporary fix for SAML login while we have a hybrid login From c619d054d52f41421098d119d2c222f293b65a06 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Wed, 13 Nov 2024 11:52:03 -0600 Subject: [PATCH 11/22] fixing user factory --- app/models/user.rb | 32 +-- config/routes.rb | 25 +-- .../indexers/file_set_indexer_spec.rb | 1 - spec/models/user_spec.rb | 190 +++++++++++------- spec/rails_helper.rb | 3 +- spec/support/factories/emory_saml_user.rb | 26 +++ spec/support/factories/users.rb | 67 ++++++ 7 files changed, 216 insertions(+), 128 deletions(-) create mode 100644 spec/support/factories/emory_saml_user.rb create mode 100644 spec/support/factories/users.rb diff --git a/app/models/user.rb b/app/models/user.rb index 419c6257..28daea0e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -22,21 +22,9 @@ def initialize(message = nil, auth = nil) # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable devise_modules = [:registerable, :recoverable, :rememberable, :validatable, :omniauthable, omniauth_providers: [:saml]] - devise_modules.prepend(:database_authenticatable, :registerable) if AuthConfig.use_database_auth? + devise_modules.prepend(:database_authenticatable, :registerable) # if AuthConfig.use_database_auth? devise(*devise_modules) - # temporary fix for SAML login while we have a hybrid login - validates :password, presence: true, if: :password_required? - - def password_required? - return false if saml_authenticatable? - super - end - - def saml_authenticatable? - provider == 'saml' && uid.present? - end - def to_s email end @@ -63,8 +51,6 @@ def self.assign_user_attributes(user, auth) user.email = "#{auth.info.net_id}@emory.edu" unless auth.info.net_id == 'tezprox' end - private_class_method :assign_user_attributes - def self.log_omniauth_error(auth) if auth.info.net_id.blank? Rails.logger.error "Nil user detected: SAML didn't pass a net_id for #{auth.inspect}" @@ -74,19 +60,3 @@ def self.log_omniauth_error(auth) end end end - -# Override a Hyrax class that expects to create system users with passwords. -# Since in production we're using shibboleth, and this removes the password -# methods from the User model, we need to override it. -module Hyrax::User - module ClassMethods - def find_or_create_system_user(user_key) - u = ::User.find_or_create_by(uid: user_key) - u.display_name = user_key - u.email = "#{user_key}@example.com" - u.password = ('a'..'z').to_a.shuffle(random: Random.new).join if AuthConfig.use_database_auth? - u.save - u - end - end -end diff --git a/config/routes.rb b/config/routes.rb index cda2e8be..fae830fa 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -14,17 +14,17 @@ concerns :searchable end - devise_for :users, skip: [:registrations], controllers: { + devise_for :users, controllers: { omniauth_callbacks: 'users/omniauth_callbacks' } - unless AuthConfig.use_database_auth? - devise_scope :user do - get 'sign_in', to: 'omniauth#new' - post 'sign_in', to: 'users/omniauth_callbacks#saml' - get 'sign_out', to: 'devise/sessions#destroy' - end + # unless AuthConfig.use_database_auth? + devise_scope :user do + get 'sign_in', to: 'omniauth#new' + post 'sign_in', to: 'users/omniauth_callbacks#saml' + get 'sign_out', to: 'devise/sessions#destroy' end + # end mount Hydra::RoleManagement::Engine => '/' @@ -48,15 +48,4 @@ delete 'clear' end end - # For details on the DSL available within this file, see http://guides.rubyonrails.org/routing.html - - # Disable these routes if you are using Devise's - # database_authenticatable in your development environment. - unless AuthConfig.use_database_auth? - devise_scope :user do - get 'sign_in', to: 'omniauth#new', as: :new_user_session - post 'sign_in', to: 'users/omniauth_callbacks#saml', as: :new_session - get 'sign_out', to: 'devise/sessions#destroy', as: :destroy_user_session - end - end end diff --git a/spec/indexers/self_deposit/indexers/file_set_indexer_spec.rb b/spec/indexers/self_deposit/indexers/file_set_indexer_spec.rb index f92674b3..798e045f 100644 --- a/spec/indexers/self_deposit/indexers/file_set_indexer_spec.rb +++ b/spec/indexers/self_deposit/indexers/file_set_indexer_spec.rb @@ -4,7 +4,6 @@ # Shared testing and factories imported from Hyrax v5.0.1 require 'hyrax/specs/shared_specs/indexers' require 'hyrax/specs/shared_specs/factories/uploaded_files' -require 'hyrax/specs/shared_specs/factories/users' RSpec.describe SelfDeposit::Indexers::FileSetIndexer do shared_context 'with typical preservation_event' do diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 0c675341..7da27fd4 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -2,115 +2,151 @@ require 'rails_helper' RSpec.describe User, type: :model do - describe '.from_saml' do + describe '.from_omniauth' do let(:auth_hash) do OpenStruct.new( + provider: 'saml', uid: '12345', info: OpenStruct.new( - mail: 'user@example.com', - displayName: 'Test User', - ou: 'Test Department', - title: 'Test Title' + net_id: 'testuser', + display_name: 'Test User' ) ) end - context 'when in production environment' do - before do - allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('production')) - end + context 'when user exists' do + let!(:existing_user) { create(:emory_saml_user, uid: 'testuser') } - context 'when user does not exist' do - it 'creates a new user' do - expect { User.from_saml(auth_hash) }.to change(User, :count).by(1) - end - - it 'sets the correct attributes' do - user = User.from_saml(auth_hash) - puts "User: #{user.inspect}" - expect(user.email).to eq('user@example.com') - expect(user.display_name).to eq('Test User') - expect(user.department).to eq('Test Department') - expect(user.title).to eq('Test Title') - expect(user.uid).to eq('12345') - end + it 'updates the user attributes' do + user = User.from_omniauth(auth_hash) + expect(user.display_name).to eq('Test User') + expect(user.ppid).to eq('12345') + expect(user.email).to eq('testuser@emory.edu') end - context 'when user already exists' do - let!(:existing_user) { User.create(email: 'user@example.com', password: 'password') } - - it 'does not create a new user' do - expect { User.from_saml(auth_hash) }.not_to change(User, :count) - end - - it 'updates the existing user' do - user = User.from_saml(auth_hash) - expect(user.id).to eq(existing_user.id) - expect(user.display_name).to eq('Test User') - expect(user.department).to eq('Test Department') - expect(user.title).to eq('Test Title') - expect(user.uid).to eq('12345') - end + it 'does not create a new user' do + expect { User.from_omniauth(auth_hash) }.not_to change(User, :count) end + end - context 'when SAML response is invalid' do - let(:invalid_auth_hash) do - OpenStruct.new( - uid: '12345', - info: OpenStruct.new( - mail: nil, - displayName: 'Test User', - ou: 'Test Department', - title: 'Test Title' - ) + context 'when net_id is tezprox' do + let(:tezprox_auth) do + OpenStruct.new( + provider: 'saml', + uid: '12345', + info: OpenStruct.new( + net_id: 'tezprox', + display_name: 'Test User' ) - end - - it 'logs an error' do - expect(Rails.logger).to receive(:error).with(/Nil user detected/) - User.from_saml(invalid_auth_hash) - end - - it 'returns a new, unsaved user' do - user = User.from_saml(invalid_auth_hash) - expect(user).to be_a(User) - expect(user).not_to be_persisted - end + ) + end + + let!(:existing_user) { create(:emory_saml_user, :tezprox) } + + it 'updates other attributes' do + user = User.from_omniauth(tezprox_auth) + expect(user.display_name).to eq('Test User') + expect(user.ppid).to eq('12345') + end + + it 'does not set the email' do + original_email = existing_user.email + user = User.from_omniauth(tezprox_auth) + expect(user.email).to eq(original_email) end end - context 'when not in production environment' do - before do - allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('development')) + context 'when SAML response is invalid' do + let(:invalid_auth_hash) do + OpenStruct.new( + provider: 'saml', + uid: '12345', + info: OpenStruct.new( + net_id: nil, + display_name: 'Test User' + ) + ) + end + + it 'logs an error' do + expect(Rails.logger).to receive(:error).with(/Nil user detected/) + User.from_omniauth(invalid_auth_hash) end - it 'does not create or update a user' do - expect(User.from_saml(auth_hash)).to be_nil + it 'returns a new unsaved user' do + user = User.from_omniauth(invalid_auth_hash) + expect(user).to be_a(User) + expect(user).not_to be_persisted end end end - describe '.log_saml_error' do + describe '.log_omniauth_error' do + let(:auth_hash) do + OpenStruct.new( + provider: 'saml', + info: OpenStruct.new( + net_id: nil, + display_name: 'Test User' + ) + ) + end + + it 'logs an error when net_id is missing' do + expect(Rails.logger).to receive(:error).with(/Nil user detected: SAML didn't pass a net_id/) + User.log_omniauth_error(auth_hash) + end + + it 'logs an unauthorized login attempt when net_id is present' do + auth_hash.info.net_id = 'testuser' + expect(Rails.logger).to receive(:error).with(/Unauthorized user attempted login/) + User.log_omniauth_error(auth_hash) + end + end + + describe '.assign_user_attributes' do + let(:user) { create(:emory_saml_user) } let(:auth_hash) do OpenStruct.new( + provider: 'saml', + uid: '12345', info: OpenStruct.new( - mail: nil, - displayName: 'Test User', - ou: 'Test Department', - title: 'Test Title' + net_id: 'testuser', + display_name: 'Test User' ) ) end - it 'logs an error when email is missing' do - expect(Rails.logger).to receive(:error).with(/Nil user detected: SAML didn't pass an email/) - User.log_saml_error(auth_hash) + it 'assigns the correct attributes' do + User.assign_user_attributes(user, auth_hash) + expect(user.display_name).to eq('Test User') + expect(user.ppid).to eq('12345') + expect(user.email).to eq('testuser@emory.edu') end - it 'logs an error when user creation or update fails' do - auth_hash.info.mail = 'user@example.com' - expect(Rails.logger).to receive(:error).with(/Failed to create\/update user/) - User.log_saml_error(auth_hash) + context 'when net_id is tezprox' do + let(:tezprox_auth) do + OpenStruct.new( + provider: 'saml', + uid: '12345', + info: OpenStruct.new( + net_id: 'tezprox', + display_name: 'Test User' + ) + ) + end + + it 'does not update email' do + original_email = user.email + User.assign_user_attributes(user, tezprox_auth) + expect(user.email).to eq(original_email) + end + + it 'updates other attributes' do + User.assign_user_attributes(user, tezprox_auth) + expect(user.display_name).to eq('Test User') + expect(user.ppid).to eq('12345') + end end end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index bfac53d7..ff0cb1d5 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -12,7 +12,6 @@ require 'devise' require 'hyrax/specs/shared_specs/factories/strategies/json_strategy' require 'hyrax/specs/shared_specs/factories/strategies/valkyrie_resource' -require 'hyrax/specs/shared_specs/factories/users' require 'hyrax/specs/capybara' require 'rspec/active_model/mocks' @@ -71,6 +70,8 @@ def database_exists? # config.use_active_record = false config.infer_spec_type_from_file_location! + config.include FactoryBot::Syntax::Methods + # Filter lines from Rails gems in backtraces. config.filter_rails_from_backtrace! # arbitrary gems may also be filtered via: diff --git a/spec/support/factories/emory_saml_user.rb b/spec/support/factories/emory_saml_user.rb new file mode 100644 index 00000000..8bbbbb2a --- /dev/null +++ b/spec/support/factories/emory_saml_user.rb @@ -0,0 +1,26 @@ +FactoryBot.define do + factory :emory_saml_user, class: 'User' do + sequence(:email) { |n| "samluser#{n}@emory.edu" } + sequence(:uid) { |n| "samluser#{n}" } + provider { 'saml' } + + after(:build) do |user| + user.password = 'testing123' + user.password_confirmation = 'testing123' + end + + trait :with_display_name do + sequence(:display_name) { |n| "Test User #{n}" } + end + + trait :with_ppid do + sequence(:ppid) { |n| "PPID#{n}" } + end + + trait :tezprox do + uid { 'tezprox' } + provider { 'saml' } + display_name { 'Tezprox User' } + end + end +end \ No newline at end of file diff --git a/spec/support/factories/users.rb b/spec/support/factories/users.rb new file mode 100644 index 00000000..74ba12d8 --- /dev/null +++ b/spec/support/factories/users.rb @@ -0,0 +1,67 @@ +# frozen_string_literal: true +FactoryBot.define do + factory :user do + sequence(:email) { |n| "user#{n}@example.com" } + password { 'password' } + + transient do + # Allow for custom groups when a user is instantiated. + # @example create(:user, groups: 'avacado') + groups { [] } + end + + after(:build) do |user, evaluator| + User.group_service.add(user: user, groups: evaluator.groups) + end + + after(:create) do |user, _evaluator| + # Set uid to email by default for SAML compatibility + user.update(uid: user.email) unless user.uid.present? + end + + trait :with_uid do + sequence(:uid) { |n| "user#{n}" } + end + + trait :saml do + provider { 'saml' } + sequence(:uid) { |n| "samluser#{n}" } + sequence(:ppid) { |n| "PPID#{n}" } + end + + factory :admin do + groups { ['admin'] } + end + + factory :user_with_mail do + after(:create) do |user| + # Create examples of single file successes and failures + (1..10).each do |number| + file = MockFile.new(number.to_s, "Single File #{number}") + User.batch_user.send_message(user, 'File 1 could not be updated. You do not have sufficient privileges to edit it.', file.to_s, false) + User.batch_user.send_message(user, 'File 1 has been saved', file.to_s, false) + end + + # Create examples of mulitple file successes and failures + files = [] + (1..50).each do |number| + files << MockFile.new(number.to_s, "File #{number}") + end + User.batch_user.send_message(user, 'These files could not be updated. You do not have sufficient privileges to edit them.', 'Batch upload permission denied', false) + User.batch_user.send_message(user, 'These files have been saved', 'Batch upload complete', false) + end + end + end + + trait :guest do + guest { true } + end +end + +class MockFile + attr_accessor :to_s, :id + def initialize(id, string) + self.id = id + self.to_s = string + end +end From bf50cbcb69b3e8d505d9064b9457f7bf6f09212e Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Wed, 13 Nov 2024 12:01:31 -0600 Subject: [PATCH 12/22] fixing rubocop --- spec/support/factories/emory_saml_user.rb | 7 ++++--- spec/support/factories/users.rb | 9 ++------- 2 files changed, 6 insertions(+), 10 deletions(-) diff --git a/spec/support/factories/emory_saml_user.rb b/spec/support/factories/emory_saml_user.rb index 8bbbbb2a..4e17da3a 100644 --- a/spec/support/factories/emory_saml_user.rb +++ b/spec/support/factories/emory_saml_user.rb @@ -1,14 +1,15 @@ +# frozen_string_literal: true FactoryBot.define do factory :emory_saml_user, class: 'User' do sequence(:email) { |n| "samluser#{n}@emory.edu" } sequence(:uid) { |n| "samluser#{n}" } provider { 'saml' } - + after(:build) do |user| user.password = 'testing123' user.password_confirmation = 'testing123' end - + trait :with_display_name do sequence(:display_name) { |n| "Test User #{n}" } end @@ -23,4 +24,4 @@ display_name { 'Tezprox User' } end end -end \ No newline at end of file +end diff --git a/spec/support/factories/users.rb b/spec/support/factories/users.rb index 74ba12d8..7993f2e2 100644 --- a/spec/support/factories/users.rb +++ b/spec/support/factories/users.rb @@ -5,18 +5,15 @@ password { 'password' } transient do - # Allow for custom groups when a user is instantiated. - # @example create(:user, groups: 'avacado') groups { [] } end after(:build) do |user, evaluator| - User.group_service.add(user: user, groups: evaluator.groups) + User.group_service.add(user, groups: evaluator.groups) end after(:create) do |user, _evaluator| - # Set uid to email by default for SAML compatibility - user.update(uid: user.email) unless user.uid.present? + user.update(uid: user.email) if user.uid.blank? end trait :with_uid do @@ -35,14 +32,12 @@ factory :user_with_mail do after(:create) do |user| - # Create examples of single file successes and failures (1..10).each do |number| file = MockFile.new(number.to_s, "Single File #{number}") User.batch_user.send_message(user, 'File 1 could not be updated. You do not have sufficient privileges to edit it.', file.to_s, false) User.batch_user.send_message(user, 'File 1 has been saved', file.to_s, false) end - # Create examples of mulitple file successes and failures files = [] (1..50).each do |number| files << MockFile.new(number.to_s, "File #{number}") From a53d15246d1f072226ffc9c78026bf9cf521cb8a Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Wed, 13 Nov 2024 12:15:59 -0600 Subject: [PATCH 13/22] fixing rubocop --- spec/support/factories/users.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/spec/support/factories/users.rb b/spec/support/factories/users.rb index 7993f2e2..746a39d3 100644 --- a/spec/support/factories/users.rb +++ b/spec/support/factories/users.rb @@ -9,7 +9,8 @@ end after(:build) do |user, evaluator| - User.group_service.add(user, groups: evaluator.groups) + current_user = user + User.group_service.add(user: current_user, groups: evaluator.groups) end after(:create) do |user, _evaluator| From a8a61bbeb089df2cbb6563e8f7fef424a2b311e6 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Wed, 13 Nov 2024 13:22:50 -0600 Subject: [PATCH 14/22] adds more tests --- app/controllers/omniauth_controller.rb | 2 +- config/routes.rb | 1 + 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/omniauth_controller.rb b/app/controllers/omniauth_controller.rb index 2ed1cd30..939a21a4 100644 --- a/app/controllers/omniauth_controller.rb +++ b/app/controllers/omniauth_controller.rb @@ -2,7 +2,7 @@ class OmniauthController < Devise::SessionsController def new if Rails.env.production? - redirect_to user_shibboleth_omniauth_authorize_path + redirect_to user_saml_omniauth_authorize_path else super end diff --git a/config/routes.rb b/config/routes.rb index fae830fa..3ebed2f0 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -23,6 +23,7 @@ get 'sign_in', to: 'omniauth#new' post 'sign_in', to: 'users/omniauth_callbacks#saml' get 'sign_out', to: 'devise/sessions#destroy' + get 'auth/failure', to: 'users/omniauth_callbacks#failure' end # end From 2a6bfdc5608a1282af93a3ca8179e5feea1abc44 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Wed, 13 Nov 2024 13:23:00 -0600 Subject: [PATCH 15/22] adds more tests --- spec/controllers/omniauth_controller_spec.rb | 59 +++++++++++++++++ .../omniauth_callbacks_controller_spec.rb | 64 +++++++++++++++++++ spec/models/auth_config_spec.rb | 37 +++++++++++ 3 files changed, 160 insertions(+) create mode 100644 spec/controllers/omniauth_controller_spec.rb create mode 100644 spec/controllers/users/omniauth_callbacks_controller_spec.rb create mode 100644 spec/models/auth_config_spec.rb diff --git a/spec/controllers/omniauth_controller_spec.rb b/spec/controllers/omniauth_controller_spec.rb new file mode 100644 index 00000000..4b23b6c3 --- /dev/null +++ b/spec/controllers/omniauth_controller_spec.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe OmniauthController do + describe '#new' do + before do + @request.env['devise.mapping'] = Devise.mappings[:user] + @request.env['warden'] = double(Warden::Proxy) + allow(@request.env['warden']).to receive(:authenticate?).and_return(false) + allow(@request.env['warden']).to receive(:authenticate!).and_return(false) + end + + context 'when in production environment' do + before do + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('production')) + end + + it 'redirects to shibboleth login' do + get :new + expect(response).to redirect_to(user_saml_omniauth_authorize_path) + end + end + + context 'when not in production environment' do + before do + allow(Rails).to receive(:env).and_return(ActiveSupport::StringInquirer.new('development')) + end + + it 'renders the default devise login page' do + get :new + expect(response).to be_successful + expect(response).to render_template('devise/sessions/new') + end + end + end + + describe 'routing' do + it 'routes GET /sign_in to omniauth#new' do + expect(get: '/sign_in').to route_to( + controller: 'omniauth', + action: 'new' + ) + end + + it 'routes POST /sign_in to users/omniauth_callbacks#saml' do + expect(post: '/sign_in').to route_to( + controller: 'users/omniauth_callbacks', + action: 'saml' + ) + end + + it 'routes GET /sign_out to devise/sessions#destroy' do + expect(get: '/sign_out').to route_to( + controller: 'devise/sessions', + action: 'destroy' + ) + end + end +end diff --git a/spec/controllers/users/omniauth_callbacks_controller_spec.rb b/spec/controllers/users/omniauth_callbacks_controller_spec.rb new file mode 100644 index 00000000..a1667daf --- /dev/null +++ b/spec/controllers/users/omniauth_callbacks_controller_spec.rb @@ -0,0 +1,64 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe Users::OmniauthCallbacksController do + include Rails.application.routes.url_helpers + + def dashboard_path + '/dashboard?locale=en' + end + + before do + @request.env["devise.mapping"] = Devise.mappings[:user] + @request.env['warden'] = double(Warden::Proxy) + allow(@request.env['warden']).to receive(:authenticate!).and_return(true) + allow(@request.env['warden']).to receive(:authenticate?).and_return(false) + allow(@request.env['warden']).to receive(:user).and_return(user) + allow(@request.env['warden']).to receive(:set_user) + allow(controller).to receive(:current_user).and_return(user) + end + + describe '#saml' do + let(:auth_hash) do + OmniAuth::AuthHash.new( + provider: 'saml', + uid: '12345', + info: { + net_id: 'test123', + display_name: 'Test User', + email: 'test123@emory.edu' + } + ) + end + + before do + @request.env["omniauth.auth"] = auth_hash + end + + context 'when user exists and is authorized' do + let(:user) { create(:user) } + + before do + allow(User).to receive(:from_omniauth).and_return(user) + @request.env["omniauth.origin"] = '/custom/path' + end + + it 'signs in and redirects the user' do + get :saml + expect(response).to redirect_to('/custom/path') + expect(flash[:notice]).to match(/Successfully authenticated from SAML account./) + end + + context 'when omniauth.origin is not present' do + before do + @request.env["omniauth.origin"] = nil + end + + it 'redirects to the dashboard' do + get :saml + expect(response).to redirect_to(dashboard_path) + end + end + end + end +end diff --git a/spec/models/auth_config_spec.rb b/spec/models/auth_config_spec.rb new file mode 100644 index 00000000..3fff2d45 --- /dev/null +++ b/spec/models/auth_config_spec.rb @@ -0,0 +1,37 @@ +# frozen_string_literal: true +require 'rails_helper' + +RSpec.describe AuthConfig do + describe '.use_database_auth?' do + context 'when DATABASE_AUTH environment variable is set to "true"' do + before do + allow(ENV).to receive(:[]).with('DATABASE_AUTH').and_return('true') + end + + it 'returns true' do + expect(described_class.use_database_auth?).to be true + end + end + + context 'when DATABASE_AUTH environment variable is not "true"' do + before do + allow(ENV).to receive(:[]).with('DATABASE_AUTH').and_return(nil) + end + + it 'returns false' do + expect(described_class.use_database_auth?).to be false + end + + it 'returns false for any other value' do + allow(ENV).to receive(:[]).with('DATABASE_AUTH').and_return('false') + expect(described_class.use_database_auth?).to be false + + allow(ENV).to receive(:[]).with('DATABASE_AUTH').and_return('1') + expect(described_class.use_database_auth?).to be false + + allow(ENV).to receive(:[]).with('DATABASE_AUTH').and_return('') + expect(described_class.use_database_auth?).to be false + end + end + end +end From cef908401812a0ddbfa57f444d15c4b4487167f9 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 18 Nov 2024 08:23:15 -0600 Subject: [PATCH 16/22] fixes coverage --- Gemfile | 1 - Gemfile.lock | 28 ---------------- config/initializers/1_saml_config.rb | 26 --------------- config/initializers/omniauth.rb | 26 +++++++++++++++ .../omniauth_callbacks_controller_spec.rb | 32 ++++++++++++++++++- 5 files changed, 57 insertions(+), 56 deletions(-) delete mode 100644 config/initializers/1_saml_config.rb diff --git a/Gemfile b/Gemfile index f2274532..2ac83672 100644 --- a/Gemfile +++ b/Gemfile @@ -33,7 +33,6 @@ gem 'uglifier', '>= 1.3.0' group :development do # Add command line in browser when errors gem 'better_errors' - gem 'solargraph' # Add deeper stack trace used by better errors gem 'binding_of_caller' diff --git a/Gemfile.lock b/Gemfile.lock index e530f944..bb045da1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -186,7 +186,6 @@ GEM babel-transpiler (0.7.0) babel-source (>= 4.0, < 6) execjs (~> 2.0) - backport (1.2.0) bagit (0.4.6) docopt (~> 0.5.0) validatable (~> 1.6) @@ -196,7 +195,6 @@ GEM bcrypt_pbkdf (1.1.1) bcrypt_pbkdf (1.1.1-arm64-darwin) bcrypt_pbkdf (1.1.1-x86_64-darwin) - benchmark (0.3.0) better_errors (2.10.1) erubi (>= 1.0.0) rack (>= 0.9.0) @@ -401,7 +399,6 @@ GEM dry-initializer (~> 3.0) dry-schema (>= 1.12, < 2) zeitwerk (~> 2.6) - e2mmap (0.1.0) ebnf (2.4.0) htmlentities (~> 4.3) rdf (~> 3.3) @@ -606,7 +603,6 @@ GEM rdoc (>= 4.0.0) reline (>= 0.4.2) iso8601 (0.9.1) - jaro_winkler (1.6.0) jbuilder (2.12.0) actionview (>= 5.0.0) activesupport (>= 5.0.0) @@ -644,10 +640,6 @@ GEM activerecord kaminari-core (= 1.2.2) kaminari-core (1.2.2) - kramdown (2.4.0) - rexml - kramdown-parser-gfm (1.1.0) - kramdown (~> 2.0) language_list (1.2.1) language_server-protocol (3.17.0.3) launchy (3.0.1) @@ -890,7 +882,6 @@ GEM rb-fsevent (0.11.2) rb-inotify (0.11.1) ffi (~> 1.0) - rbs (2.8.4) rdf (3.3.2) bcp47_spec (~> 0.2) bigdecimal (~> 3.1, >= 3.1.5) @@ -994,8 +985,6 @@ GEM actionpack (>= 5.2) railties (>= 5.2) retriable (3.1.2) - reverse_markdown (2.1.1) - nokogiri rexml (3.3.9) riiif (2.5.0) deprecation (>= 1.0.0) @@ -1129,22 +1118,6 @@ GEM snaky_hash (2.0.1) hashie version_gem (~> 1.1, >= 1.1.1) - solargraph (0.50.0) - backport (~> 1.2) - benchmark - bundler (~> 2.0) - diff-lcs (~> 1.4) - e2mmap - jaro_winkler (~> 1.5) - kramdown (~> 2.3) - kramdown-parser-gfm (~> 1.1) - parser (~> 3.0) - rbs (~> 2.0) - reverse_markdown (~> 2.0) - rubocop (~> 1.38) - thor (~> 1.0) - tilt (~> 2.0) - yard (~> 0.9, >= 0.9.24) sparql (3.3.0) builder (~> 3.2, >= 3.2.4) ebnf (~> 2.4) @@ -1319,7 +1292,6 @@ DEPENDENCIES sass-rails (~> 6.0) selenium-webdriver (~> 4.4) sidekiq (~> 6.4) - solargraph spring spring-watcher-listen (~> 2.0.0) turbolinks (~> 5) diff --git a/config/initializers/1_saml_config.rb b/config/initializers/1_saml_config.rb deleted file mode 100644 index 756c6eec..00000000 --- a/config/initializers/1_saml_config.rb +++ /dev/null @@ -1,26 +0,0 @@ -# frozen_string_literal: true -Rails.application.config.class.class_eval do - attr_accessor :assertion_consumer_service_url, - :assertion_consumer_logout_service_url, - :issuer, - :idp_sso_target_url, - :idp_slo_target_url, - :idp_cert, - :certificate, - :private_key, - :attribute_statements, - :uid_attribute, - :security -end - -Rails.application.config.assertion_consumer_service_url = nil -Rails.application.config.assertion_consumer_logout_service_url = nil -Rails.application.config.issuer = nil -Rails.application.config.idp_sso_target_url = nil -Rails.application.config.idp_slo_target_url = nil -Rails.application.config.idp_cert = nil -Rails.application.config.certificate = nil -Rails.application.config.private_key = nil -Rails.application.config.attribute_statements = {} -Rails.application.config.uid_attribute = nil -Rails.application.config.security = {} diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index d1642e5c..00a50cd2 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,5 +1,31 @@ # frozen_string_literal: true +Rails.application.config.class.class_eval do + attr_accessor :assertion_consumer_service_url, + :assertion_consumer_logout_service_url, + :issuer, + :idp_sso_target_url, + :idp_slo_target_url, + :idp_cert, + :certificate, + :private_key, + :attribute_statements, + :uid_attribute, + :security +end + +Rails.application.config.assertion_consumer_service_url = nil +Rails.application.config.assertion_consumer_logout_service_url = nil +Rails.application.config.issuer = nil +Rails.application.config.idp_sso_target_url = nil +Rails.application.config.idp_slo_target_url = nil +Rails.application.config.idp_cert = nil +Rails.application.config.certificate = nil +Rails.application.config.private_key = nil +Rails.application.config.attribute_statements = {} +Rails.application.config.uid_attribute = nil +Rails.application.config.security = {} + Rails.application.config.middleware.use OmniAuth::Builder do if !Rails.env.development? && !Rails.env.test? provider :saml, diff --git a/spec/controllers/users/omniauth_callbacks_controller_spec.rb b/spec/controllers/users/omniauth_callbacks_controller_spec.rb index a1667daf..ec6cf798 100644 --- a/spec/controllers/users/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/users/omniauth_callbacks_controller_spec.rb @@ -8,6 +8,10 @@ def dashboard_path '/dashboard?locale=en' end + def root_path + '/?locale=en' + end + before do @request.env["devise.mapping"] = Devise.mappings[:user] @request.env['warden'] = double(Warden::Proxy) @@ -60,5 +64,31 @@ def dashboard_path end end end + + context 'when user is not authorized' do + let(:unauthorized_user) { User.new } + let(:user) { unauthorized_user } + + before do + allow(User).to receive(:from_omniauth).and_return(unauthorized_user) + end + + it 'redirects to root with error message' do + get :saml + expect(response).to redirect_to(root_path) + expect(flash[:notice]).to eq( + 'Could not authenticate you from SAML because "you aren\'t authorized to use this application.".' + ) + end + end + end + + describe '#failure' do + let(:user) { nil } + + it 'redirects to root path' do + get :failure + expect(response).to redirect_to(root_path) + end end -end +end \ No newline at end of file From 92dfba787c2572bb704be43a21b84cdb3e7f4c36 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 18 Nov 2024 08:44:35 -0600 Subject: [PATCH 17/22] fixes rubocop --- spec/controllers/users/omniauth_callbacks_controller_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/controllers/users/omniauth_callbacks_controller_spec.rb b/spec/controllers/users/omniauth_callbacks_controller_spec.rb index ec6cf798..e496a54e 100644 --- a/spec/controllers/users/omniauth_callbacks_controller_spec.rb +++ b/spec/controllers/users/omniauth_callbacks_controller_spec.rb @@ -91,4 +91,4 @@ def root_path expect(response).to redirect_to(root_path) end end -end \ No newline at end of file +end From 70ec3047062e9abb7e81a225554cf890df2eec17 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 18 Nov 2024 09:46:11 -0600 Subject: [PATCH 18/22] adds all params to new user --- app/models/user.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/app/models/user.rb b/app/models/user.rb index 28daea0e..bb08a4bc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -45,7 +45,9 @@ def self.from_omniauth(auth) def self.assign_user_attributes(user, auth) user.assign_attributes( display_name: auth.info.display_name, - ppid: auth.uid + ppid: auth.uid, + provider: auth.provider, + uid: auth.info.net_id ) user.email = "#{auth.info.net_id}@emory.edu" unless auth.info.net_id == 'tezprox' From 3794bf505dcac66c64a716a9605b87643ea26acc Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 18 Nov 2024 09:48:35 -0600 Subject: [PATCH 19/22] removes config --- config/initializers/omniauth.rb | 26 -------------------------- 1 file changed, 26 deletions(-) diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index 00a50cd2..d1642e5c 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,31 +1,5 @@ # frozen_string_literal: true -Rails.application.config.class.class_eval do - attr_accessor :assertion_consumer_service_url, - :assertion_consumer_logout_service_url, - :issuer, - :idp_sso_target_url, - :idp_slo_target_url, - :idp_cert, - :certificate, - :private_key, - :attribute_statements, - :uid_attribute, - :security -end - -Rails.application.config.assertion_consumer_service_url = nil -Rails.application.config.assertion_consumer_logout_service_url = nil -Rails.application.config.issuer = nil -Rails.application.config.idp_sso_target_url = nil -Rails.application.config.idp_slo_target_url = nil -Rails.application.config.idp_cert = nil -Rails.application.config.certificate = nil -Rails.application.config.private_key = nil -Rails.application.config.attribute_statements = {} -Rails.application.config.uid_attribute = nil -Rails.application.config.security = {} - Rails.application.config.middleware.use OmniAuth::Builder do if !Rails.env.development? && !Rails.env.test? provider :saml, From bd341074188789bcc7aea7aa71fee117e8bbab92 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 2 Dec 2024 09:57:45 -0600 Subject: [PATCH 20/22] adds requierd gem for devise --- Gemfile | 1 + Gemfile.lock | 4 ++++ 2 files changed, 5 insertions(+) diff --git a/Gemfile b/Gemfile index 2ac83672..9a968c4b 100644 --- a/Gemfile +++ b/Gemfile @@ -17,6 +17,7 @@ gem 'hyrax', git: 'https://github.com/samvera/hyrax', ref: '9c58751' gem 'jbuilder', '~> 2.5' gem 'jquery-rails' gem 'mutex_m', '~> 0.2.0' +gem 'omniauth-rails_csrf_protection' gem 'omniauth-saml' gem 'pg', '~> 1.3' gem 'puma' diff --git a/Gemfile.lock b/Gemfile.lock index bb045da1..72c55e22 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -788,6 +788,9 @@ GEM hashie (>= 3.4.6) rack (>= 2.2.3) rack-protection + omniauth-rails_csrf_protection (1.0.2) + actionpack (>= 4.2) + omniauth (~> 2.0) omniauth-saml (2.2.1) omniauth (~> 2.1) ruby-saml (~> 1.17) @@ -1272,6 +1275,7 @@ DEPENDENCIES jquery-rails listen (>= 3.0.5, < 3.2) mutex_m (~> 0.2.0) + omniauth-rails_csrf_protection omniauth-saml pg (~> 1.3) pry-doc From 89e301f3b3134b603df7dde1c2c35eef90013ba8 Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Mon, 13 Jan 2025 10:17:59 -0600 Subject: [PATCH 21/22] updates devise settings --- config/initializers/devise.rb | 10 ++++++++- config/initializers/omniauth.rb | 36 ++++++++++++++++----------------- 2 files changed, 27 insertions(+), 19 deletions(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 1f399a20..3d416e0f 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -272,7 +272,15 @@ # Add a new OmniAuth provider. Check the wiki for more information on setting # up on your models and hooks. # config.omniauth :github, 'APP_ID', 'APP_SECRET', scope: 'user,public_repo' - config.omniauth :saml + config.omniauth :saml, + idp_cert: ENV['IDP_CERT'], + certificate: ENV['SP_CERT'], + private_key: ENV['SP_KEY'], + idp_sso_target_url: 'https://login.emory.edu/idp/profile/SAML2/Redirect/SSO', + idp_sso_service_url: ENV['IDP_SSO_TARGET_URL'] + + + # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or # change the failure app, you can configure them inside the config.warden block. diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb index d1642e5c..aef5e327 100644 --- a/config/initializers/omniauth.rb +++ b/config/initializers/omniauth.rb @@ -1,21 +1,21 @@ # frozen_string_literal: true -Rails.application.config.middleware.use OmniAuth::Builder do - if !Rails.env.development? && !Rails.env.test? - provider :saml, - assertion_consumer_service_url: Rails.application.config.assertion_consumer_service_url, - assertion_consumer_logout_service_url: Rails.application.config.assertion_consumer_logout_service_url, - issuer: Rails.application.config.issuer, - idp_sso_target_url: Rails.application.config.idp_sso_target_url, - idp_slo_target_url: Rails.application.config.idp_slo_target_url, - idp_cert: Rails.application.config.idp_cert, - certificate: Rails.application.config.certificate, - private_key: Rails.application.config.private_key, - attribute_statements: Rails.application.config.attribute_statements, - uid_attribute: Rails.application.config.uid_attribute, - security: Rails.application.config.security - end -end +# Rails.application.config.middleware.use OmniAuth::Builder do +# if !Rails.env.development? && !Rails.env.test? +# provider :saml, +# assertion_consumer_service_url: Rails.application.config.assertion_consumer_service_url, +# assertion_consumer_logout_service_url: Rails.application.config.assertion_consumer_logout_service_url, +# issuer: Rails.application.config.issuer, +# idp_sso_target_url: Rails.application.config.idp_sso_target_url, +# idp_slo_target_url: Rails.application.config.idp_slo_target_url, +# idp_cert: Rails.application.config.idp_cert, +# certificate: Rails.application.config.certificate, +# private_key: Rails.application.config.private_key, +# attribute_statements: Rails.application.config.attribute_statements, +# uid_attribute: Rails.application.config.uid_attribute, +# security: Rails.application.config.security +# end +# end -# Protect against forgery attacks -OmniAuth.config.allowed_request_methods = [:post] +# # Protect against forgery attacks +# OmniAuth.config.allowed_request_methods = [:post] From ac7eac9f2e36afc74a978ac5432ce719e7af5e3c Mon Sep 17 00:00:00 2001 From: Alex Zotov Date: Tue, 14 Jan 2025 11:57:29 -0600 Subject: [PATCH 22/22] fixes rubocop --- config/initializers/devise.rb | 2 -- config/routes.rb | 1 - 2 files changed, 3 deletions(-) diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 3d416e0f..08df0ee6 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -279,8 +279,6 @@ idp_sso_target_url: 'https://login.emory.edu/idp/profile/SAML2/Redirect/SSO', idp_sso_service_url: ENV['IDP_SSO_TARGET_URL'] - - # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or # change the failure app, you can configure them inside the config.warden block. diff --git a/config/routes.rb b/config/routes.rb index 911217d1..6dde9bb6 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -54,7 +54,6 @@ end end - # rubocop:disable Rails/FindEach def latency_text ret_hsh = { queues: [] }