From 7761b75f8f948f8c7cee09b110672b896edd7669 Mon Sep 17 00:00:00 2001 From: Adam Cooke Date: Mon, 11 Mar 2024 21:54:54 +0000 Subject: [PATCH] wip: oidc --- Gemfile | 2 + Gemfile.lock | 69 ++++++++++++++++ OIDC.md | 8 ++ app/controllers/sessions_controller.rb | 29 +++++-- app/models/concerns/has_authentication.rb | 19 ++++- app/models/user.rb | 79 ++++++++++++++++--- app/views/sessions/new.html.haml | 2 + app/views/users/_form.html.haml | 10 ++- config/initializers/inflections.rb | 1 + config/initializers/omniauth.rb | 18 +++++ config/routes.rb | 2 + .../20240311205229_add_oidc_fields_to_user.rb | 10 +++ db/schema.rb | 4 +- doc/config/environment-variables.md | 9 +++ doc/config/yaml.yml | 21 +++++ lib/postal/config_schema.rb | 45 +++++++++++ lib/postal/yaml_config_exporter.rb | 2 +- spec/factories/user_factory.rb | 16 ++-- spec/models/user_spec.rb | 2 + 19 files changed, 321 insertions(+), 27 deletions(-) create mode 100644 OIDC.md create mode 100644 config/initializers/omniauth.rb create mode 100644 db/migrate/20240311205229_add_oidc_fields_to_user.rb diff --git a/Gemfile b/Gemfile index c3efa3799..b56acc594 100644 --- a/Gemfile +++ b/Gemfile @@ -23,6 +23,8 @@ gem "mysql2" gem "nifty-utils" gem "nilify_blanks" gem "nio4r" +gem "omniauth_openid_connect" +gem "omniauth-rails_csrf_protection" gem "prometheus-client" gem "puma" gem "rails", "= 7.0.8.1" diff --git a/Gemfile.lock b/Gemfile.lock index c6b997b38..0f4fe96bc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -68,16 +68,20 @@ GEM tzinfo (~> 2.0) addressable (2.8.6) public_suffix (>= 2.0.2, < 6.0) + aes_key_wrap (1.1.0) annotate (3.2.0) activerecord (>= 3.2, < 8.0) rake (>= 10.4, < 14.0) ast (2.4.2) + attr_required (1.0.2) authie (4.1.3) activerecord (>= 6.1, < 8.0) autoprefixer-rails (10.4.13.0) execjs (~> 2) + base64 (0.2.0) bcrypt (3.1.20) bigdecimal (3.1.6) + bindata (2.5.0) builder (3.2.4) chronic (0.10.2) coffee-rails (5.0.0) @@ -106,6 +110,8 @@ GEM dynamic_form (1.3.1) actionview (> 5.2.0) activemodel (> 5.2.0) + email_validator (2.2.4) + activemodel encrypto_signo (1.0.0) erubi (1.12.0) execjs (2.7.0) @@ -114,6 +120,12 @@ GEM factory_bot_rails (6.4.3) factory_bot (~> 6.4) railties (>= 5.0.0) + faraday (2.9.0) + faraday-net_http (>= 2.0, < 3.2) + faraday-follow_redirects (0.3.0) + faraday (>= 1, < 3) + faraday-net_http (3.1.0) + net-http ffi (1.15.5) gelf (3.1.0) json @@ -133,6 +145,13 @@ GEM railties (>= 4.2.0) thor (>= 0.14, < 2.0) json (2.7.1) + json-jwt (1.16.6) + activesupport (>= 4.2) + aes_key_wrap + base64 + bindata + faraday (~> 2.0) + faraday-follow_redirects kaminari (1.2.2) activesupport (>= 4.1.0) kaminari-actionview (= 1.2.2) @@ -169,6 +188,8 @@ GEM json rack (>= 1.4) mysql2 (0.5.6) + net-http (0.4.1) + uri net-imap (0.4.10) date net-protocol @@ -194,6 +215,29 @@ GEM racc (~> 1.4) nokogiri (1.16.2-x86_64-linux) racc (~> 1.4) + omniauth (2.1.2) + hashie (>= 3.4.6) + rack (>= 2.2.3) + rack-protection + omniauth-rails_csrf_protection (1.0.1) + actionpack (>= 4.2) + omniauth (~> 2.0) + omniauth_openid_connect (0.7.1) + omniauth (>= 1.9, < 3) + openid_connect (~> 2.2) + openid_connect (2.3.0) + activemodel + attr_required (>= 1.0.0) + email_validator + faraday (~> 2.0) + faraday-follow_redirects + json-jwt (>= 1.16) + mail + rack-oauth2 (~> 2.2) + swd (~> 2.0) + tzinfo + validate_url + webfinger (~> 2.0) parallel (1.22.1) parser (3.2.1.1) ast (~> 2.4.1) @@ -203,6 +247,16 @@ GEM nio4r (~> 2.0) racc (1.7.3) rack (2.2.8.1) + rack-oauth2 (2.2.1) + activesupport + attr_required + faraday (~> 2.0) + faraday-follow_redirects + json-jwt (>= 1.11.0) + rack (>= 2.1.0) + rack-protection (3.2.0) + base64 (>= 0.1.0) + rack (~> 2.2, >= 2.2.4) rack-test (2.1.0) rack (>= 1.3) rails (7.0.8.1) @@ -302,6 +356,11 @@ GEM actionpack (>= 5.2) activesupport (>= 5.2) sprockets (>= 3.0.0) + swd (2.0.3) + activesupport (>= 3) + attr_required (>= 0.0.5) + faraday (~> 2.0) + faraday-follow_redirects temple (0.10.3) thor (1.3.0) tilt (2.3.0) @@ -315,6 +374,14 @@ GEM uglifier (4.2.0) execjs (>= 0.3.0, < 3) unicode-display_width (2.4.2) + uri (0.13.0) + validate_url (1.0.15) + activemodel (>= 3.0.0) + public_suffix + webfinger (2.1.3) + activesupport + faraday (~> 2.0) + faraday-follow_redirects webmock (3.20.0) addressable (>= 2.8.0) crack (>= 0.3.2) @@ -360,6 +427,8 @@ DEPENDENCIES nifty-utils nilify_blanks nio4r + omniauth-rails_csrf_protection + omniauth_openid_connect prometheus-client puma rails (= 7.0.8.1) diff --git a/OIDC.md b/OIDC.md new file mode 100644 index 000000000..4ef3e4a7b --- /dev/null +++ b/OIDC.md @@ -0,0 +1,8 @@ +- Catch errors from the OIDC callback to display properly +- Show OIDC enabled users in the user list +- Disable callback when OIDC is not enabled +- Support for non-discovery mode +- Don't require a password to change user details when the user has no password +- Don't allow the user to set a password locally if they don't have one already +- Tests for the user model +- Tests for the sessions controller diff --git a/app/controllers/sessions_controller.rb b/app/controllers/sessions_controller.rb index 99a80f218..ae30338a3 100644 --- a/app/controllers/sessions_controller.rb +++ b/app/controllers/sessions_controller.rb @@ -4,7 +4,7 @@ class SessionsController < ApplicationController layout "sub" - skip_before_action :login_required, only: [:new, :create, :begin_password_reset, :finish_password_reset, :ip, :raise_error] + skip_before_action :login_required, only: [:new, :create, :begin_password_reset, :finish_password_reset, :ip, :raise_error, :create_from_oidc] def create login(User.authenticate(params[:email_address], params[:password])) @@ -29,12 +29,16 @@ def persist def begin_password_reset return unless request.post? - if user = User.where(email_address: params[:email_address]).first - user.begin_password_reset(params[:return_to]) - redirect_to login_path(return_to: params[:return_to]), notice: "Please check your e-mail and click the link in the e-mail we've sent you." - else - redirect_to login_reset_path(return_to: params[:return_to]), alert: "No user exists with that e-mail address. Please check and try again." + user_scope = Postal::Config.oidc.enabled? ? User.with_password : User + user = user_scope.find_by(email_address: params[:email_address]) + + if user.nil? + redirect_to login_reset_path(return_to: params[:return_to]), alert: "No local user exists with that e-mail address. Please check and try again." + return end + + user.begin_password_reset(params[:return_to]) + redirect_to login_path(return_to: params[:return_to]), notice: "Please check your e-mail and click the link in the e-mail we've sent you." end def finish_password_reset @@ -49,6 +53,7 @@ def finish_password_reset flash.now[:alert] = "You must enter a new password" return end + @user.password = params[:password] @user.password_confirmation = params[:password_confirmation] return unless @user.save @@ -61,4 +66,16 @@ def ip render plain: "ip: #{request.ip} remote ip: #{request.remote_ip}" end + def create_from_oidc + auth = request.env["omniauth.auth"] + user = User.find_from_oidc(auth.extra.raw_info, logger: Postal.logger) + if user.nil? + redirect_to login_path, alert: "No user was found matching your identity. Please contact your administrator." + return + end + + login(user) + redirect_to_with_return_to root_path + end + end diff --git a/app/models/concerns/has_authentication.rb b/app/models/concerns/has_authentication.rb index 17c06d2cf..c19bc7b11 100644 --- a/app/models/concerns/has_authentication.rb +++ b/app/models/concerns/has_authentication.rb @@ -5,15 +5,20 @@ module HasAuthentication extend ActiveSupport::Concern included do - has_secure_password + has_secure_password validations: false validates :password, length: { minimum: 8, allow_blank: true } + validates :password, confirmation: { allow_blank: true } + validate :validate_password_presence + before_save :clear_password_reset_token_on_password_change + + scope :with_password, -> { where.not(password_digest: nil) } end class_methods do def authenticate(email_address, password) - user = where(email_address: email_address).first + user = find_by(email_address: email_address) raise Postal::Errors::AuthenticationError, "InvalidEmailAddress" if user.nil? raise Postal::Errors::AuthenticationError, "InvalidPassword" unless user.authenticate(password) @@ -30,6 +35,10 @@ def authenticate_with_previous_password_first(unencrypted_password) end def begin_password_reset(return_to = nil) + if Postal::Config.oidc.enabled? && (oidc_uid.present? || password_digest.blank?) + raise Postal::Error, "User has OIDC enabled, password resets are not supported" + end + self.password_reset_token = SecureRandom.alphanumeric(24) self.password_reset_token_valid_until = 1.day.from_now save! @@ -45,6 +54,12 @@ def clear_password_reset_token_on_password_change self.password_reset_token_valid_until = nil end + def validate_password_presence + return if password_digest.present? || Postal::Config.oidc.enabled? + + errors.add :password, :blank + end + end # -*- SkipSchemaAnnotations diff --git a/app/models/user.rb b/app/models/user.rb index b2831f3c3..4263ebaca 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -5,19 +5,21 @@ # Table name: users # # id :integer not null, primary key -# uuid :string(255) +# admin :boolean default(FALSE) +# email_address :string(255) +# email_verification_token :string(255) +# email_verified_at :datetime # first_name :string(255) # last_name :string(255) -# email_address :string(255) +# oidc_issuer :string(255) +# oidc_uid :string(255) # password_digest :string(255) +# password_reset_token :string(255) +# password_reset_token_valid_until :datetime # time_zone :string(255) -# email_verification_token :string(255) -# email_verified_at :datetime +# uuid :string(255) # created_at :datetime # updated_at :datetime -# password_reset_token :string(255) -# password_reset_token_valid_until :datetime -# admin :boolean default(FALSE) # # Indexes # @@ -69,8 +71,67 @@ def email_tag "#{name} <#{email_address}>" end - def self.[](email) - where(email_address: email).first + class << self + + # Lookup a user by email address + # + # @param email [String] the email address + # + # @return [User, nil] the user + def [](email) + find_by(email_address: email) + end + + # Find a user based on an OIDC authentication hash + # + # @param auth [Hash] the authentication hash + # @param logger [Logger] a logger to log debug information to + # + # @return [User, nil] the user + def find_from_oidc(auth, logger: nil) + config = Postal::Config.oidc + + uid = auth[config.uid_field] + oidc_name = auth[config.name_field] + oidc_email_address = auth[config.email_address_field] + + # look for an existing user with the same UID and OIDC issuer. If we find one, + # this is the user we'll want to use. + user = where(oidc_uid: uid, oidc_issuer: config.issuer).first + + if user + logger&.debug "found user with UID #{uid} for issuer #{config.issuer} (user ID: #{user.id})" + else + logger&.debug "no user with UID #{uid} for issuer #{config.issuer}" + end + + # if we don't have an existing user, we will look for users which have no OIDC + # credentials but with a matching e-mail address. + if user.nil? && oidc_email_address.present? + user = where(oidc_uid: nil, email_address: oidc_email_address).first + if user + logger&.debug "found user with e-mail address #{oidc_email_address} (user ID: #{user.id})" + else + logger&.debug "no user with e-mail address #{oidc_email_address}" + end + end + + # now, if we still don't have a user, we're not going to create one so we'll just + # return nil (we might auto create users in the future but not right now) + return if user.nil? + + # otherwise, let's update our user as appropriate + user.oidc_uid = uid + user.oidc_issuer = config.issuer + user.email_address = oidc_email_address if oidc_email_address.present? + user.first_name, user.last_name = oidc_name.split(/\s+/, 2) if oidc_name.present? + user.password = nil + user.save! + + # return the user + user + end + end end diff --git a/app/views/sessions/new.html.haml b/app/views/sessions/new.html.haml index bab54527f..7c255e1e6 100644 --- a/app/views/sessions/new.html.haml +++ b/app/views/sessions/new.html.haml @@ -16,3 +16,5 @@ %li= link_to "Forgotten your password?", login_reset_path(:return_to => params[:return_to]) %p= submit_tag "Login", :class => 'button button--positive', :tabindex => 3 + - if Postal::Config.oidc.enabled? + = link_to "Login with #{Postal::Config.oidc.name}", "/auth/oidc", method: :post diff --git a/app/views/users/_form.html.haml b/app/views/users/_form.html.haml index 0011fb143..f7e89ef1f 100644 --- a/app/views/users/_form.html.haml +++ b/app/views/users/_form.html.haml @@ -15,7 +15,15 @@ - unless @user.persisted? .fieldSet__field = f.label :password, :class => 'fieldSet__label' - .fieldSet__input= f.password_field :password, :class => 'input input--text', :placeholder => '•••••••••••', autocomplete: 'one-time-code' + .fieldSet__input + = f.password_field :password, :class => 'input input--text', :placeholder => '•••••••••••', autocomplete: 'one-time-code' + - if Postal::Config.oidc.enabled? + %p.fieldSet__text + You have enabled OIDC which means a password is not required. If you do not provide + a password this user will be matched to an OIDC identity based on the e-mail address + provided above. You may, however, enter a password and this user will be permitted to + use that password until they have successfully logged in with OIDC. + .fieldSet__field = f.label :password_confirmation, "Confirm".html_safe, :class => 'fieldSet__label' .fieldSet__input= f.password_field :password_confirmation, :class => 'input input--text', :placeholder => '•••••••••••', autocomplete: 'one-time-code' diff --git a/config/initializers/inflections.rb b/config/initializers/inflections.rb index 3a68b2733..c4efa4231 100644 --- a/config/initializers/inflections.rb +++ b/config/initializers/inflections.rb @@ -16,6 +16,7 @@ ActiveSupport::Inflector.inflections(:en) do |inflect| inflect.acronym "DKIM" inflect.acronym "HTTP" + inflect.acronym "OIDC" inflect.acronym "SMTP" inflect.acronym "UUID" diff --git a/config/initializers/omniauth.rb b/config/initializers/omniauth.rb new file mode 100644 index 000000000..1cd260b39 --- /dev/null +++ b/config/initializers/omniauth.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +config = Postal::Config.oidc + +if config.enabled? + Postal.logger.info "enabling openid connect for #{config.issuer}" + Rails.application.config.middleware.use OmniAuth::Builder do + provider :openid_connect, name: :oidc, + scope: config.scopes.map(&:to_sym), + uid_field: config.uid_field, + issuer: config.issuer, + discovery: true, + client_options: { + identifier: config.identifier, + secret: config.secret + } + end +end diff --git a/config/routes.rb b/config/routes.rb index 51026e12d..aedb1b81a 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -85,6 +85,8 @@ match "login/reset" => "sessions#begin_password_reset", :via => [:get, :post] match "login/reset/:token" => "sessions#finish_password_reset", :via => [:get, :post] + get "auth/oidc/callback", to: "sessions#create_from_oidc" + get "ip" => "sessions#ip" root "organizations#index" diff --git a/db/migrate/20240311205229_add_oidc_fields_to_user.rb b/db/migrate/20240311205229_add_oidc_fields_to_user.rb new file mode 100644 index 000000000..a617222df --- /dev/null +++ b/db/migrate/20240311205229_add_oidc_fields_to_user.rb @@ -0,0 +1,10 @@ +# frozen_string_literal: true + +class AddOIDCFieldsToUser < ActiveRecord::Migration[7.0] + + def change + add_column :users, :oidc_uid, :string + add_column :users, :oidc_issuer, :string + end + +end diff --git a/db/schema.rb b/db/schema.rb index 01d7ef656..9ab08fdb8 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[7.0].define(version: 2024_02_23_141501) do +ActiveRecord::Schema[7.0].define(version: 2024_03_11_205229) do create_table "additional_route_endpoints", id: :integer, charset: "utf8mb4", collation: "utf8mb4_general_ci", force: :cascade do |t| t.integer "route_id" t.string "endpoint_type" @@ -330,6 +330,8 @@ t.string "password_reset_token" t.datetime "password_reset_token_valid_until", precision: nil t.boolean "admin", default: false + t.string "oidc_uid" + t.string "oidc_issuer" t.index ["email_address"], name: "index_users_on_email_address", length: 8 t.index ["uuid"], name: "index_users_on_uuid", length: 8 end diff --git a/doc/config/environment-variables.md b/doc/config/environment-variables.md index 37b9a4e01..dcd394eb7 100644 --- a/doc/config/environment-variables.md +++ b/doc/config/environment-variables.md @@ -97,3 +97,12 @@ This document contains all the environment variables which are available for thi | `MIGRATION_WAITER_ENABLED` | Boolean | Wait for all migrations to run before starting a process | false | | `MIGRATION_WAITER_ATTEMPTS` | Integer | The number of attempts to try waiting for migrations to complete before start | 120 | | `MIGRATION_WAITER_SLEEP_TIME` | Integer | The number of seconds to wait between each migration check | 2 | +| `OIDC_ENABLED` | Boolean | Enable OIDC authentication | false | +| `OIDC_NAME` | String | The name of the OIDC provider as shown in the UI | OIDC Provider | +| `OIDC_ISSUER` | String | The OIDC issuer URL | | +| `OIDC_CLIENT_ID` | String | The client ID for OIDC | | +| `OIDC_CLIENT_SECRET` | String | The client secret for OIDC | | +| `OIDC_SCOPES` | Array of strings | Scopes to request from the OIDC server. | openid | +| `OIDC_UID_FIELD` | String | The field to use to determine the user's UID | sub | +| `OIDC_EMAIL_ADDRESS_FIELD` | String | The field to use to determine the user's email address | sub | +| `OIDC_NAME_FIELD` | String | The field to use to determine the user's name | name | diff --git a/doc/config/yaml.yml b/doc/config/yaml.yml index b9d91d783..04faddf89 100644 --- a/doc/config/yaml.yml +++ b/doc/config/yaml.yml @@ -219,3 +219,24 @@ migration_waiter: attempts: 120 # The number of seconds to wait between each migration check sleep_time: 2 + +oidc: + # Enable OIDC authentication + enabled: false + # The name of the OIDC provider as shown in the UI + name: OIDC Provider + # The OIDC issuer URL + issuer: + # The client ID for OIDC + client_id: + # The client secret for OIDC + client_secret: + # Scopes to request from the OIDC server. + scopes: + - openid + # The field to use to determine the user's UID + uid_field: sub + # The field to use to determine the user's email address + email_address_field: sub + # The field to use to determine the user's name + name_field: name diff --git a/lib/postal/config_schema.rb b/lib/postal/config_schema.rb index 8e9976273..1859b44b7 100644 --- a/lib/postal/config_schema.rb +++ b/lib/postal/config_schema.rb @@ -508,6 +508,51 @@ module Postal default 2 end end + + group :oidc do + boolean :enabled do + description "Enable OIDC authentication" + default false + end + + string :name do + description "The name of the OIDC provider as shown in the UI" + default "OIDC Provider" + end + + string :issuer do + description "The OIDC issuer URL" + end + + string :identifier do + description "The client ID for OIDC" + end + + string :secret do + description "The client secret for OIDC" + end + + string :scopes do + description "Scopes to request from the OIDC server." + array + default "openid" + end + + string :uid_field do + description "The field to use to determine the user's UID" + default "sub" + end + + string :email_address_field do + description "The field to use to determine the user's email address" + default "sub" + end + + string :name_field do + description "The field to use to determine the user's name" + default "name" + end + end end class << self diff --git a/lib/postal/yaml_config_exporter.rb b/lib/postal/yaml_config_exporter.rb index f7c2ab94e..d85cfb719 100644 --- a/lib/postal/yaml_config_exporter.rb +++ b/lib/postal/yaml_config_exporter.rb @@ -19,7 +19,7 @@ def export contents << " #{name}: []" else contents << " #{name}:" - attr.default.each do |d| + attr.transform(attr.default).each do |d| contents << " - #{d}" end end diff --git a/spec/factories/user_factory.rb b/spec/factories/user_factory.rb index 03373491d..5ece8cf32 100644 --- a/spec/factories/user_factory.rb +++ b/spec/factories/user_factory.rb @@ -5,19 +5,21 @@ # Table name: users # # id :integer not null, primary key -# uuid :string(255) +# admin :boolean default(FALSE) +# email_address :string(255) +# email_verification_token :string(255) +# email_verified_at :datetime # first_name :string(255) # last_name :string(255) -# email_address :string(255) +# oidc_issuer :string(255) +# oidc_uid :string(255) # password_digest :string(255) +# password_reset_token :string(255) +# password_reset_token_valid_until :datetime # time_zone :string(255) -# email_verification_token :string(255) -# email_verified_at :datetime +# uuid :string(255) # created_at :datetime # updated_at :datetime -# password_reset_token :string(255) -# password_reset_token_valid_until :datetime -# admin :boolean default(FALSE) # # Indexes # diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 023ef71de..37bf0d6d9 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -11,6 +11,8 @@ # email_verified_at :datetime # first_name :string(255) # last_name :string(255) +# oidc_issuer :string(255) +# oidc_uid :string(255) # password_digest :string(255) # password_reset_token :string(255) # password_reset_token_valid_until :datetime