From 5e24df5ef0fae9e527d836205eeff9d478cd870b Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Tue, 3 Jan 2023 15:16:15 +0100 Subject: [PATCH 1/9] Add first version of OIDC login Co-authored-by: Alexander Sohn --- Gemfile | 2 + Gemfile.lock | 61 +++++++++++++++++++ .../users/omniauth_callbacks_controller.rb | 17 ++++++ app/models/user.rb | 9 ++- app/views/devise/shared/_links.html.erb | 16 +++-- config/initializers/devise.rb | 19 +++++- config/routes.rb | 2 +- db/schema.rb | 10 +-- 8 files changed, 124 insertions(+), 12 deletions(-) create mode 100644 app/controllers/users/omniauth_callbacks_controller.rb diff --git a/Gemfile b/Gemfile index 551ff0ac..2b44f745 100644 --- a/Gemfile +++ b/Gemfile @@ -58,6 +58,8 @@ gem "devise", github: "heartcombo/devise", ref: "f8d1ea90bc3" # https://steve-co gem "devise-i18n" # https://github.com/tigrish/devise-i18n gem "devise-bootstrap-views" # https://github.com/hisea/devise-bootstrap-views gem "devise-i18n-bootstrap" # https://github.com/maximalink/devise-i18n-bootstrap +gem 'omniauth_openid_connect' +gem "omniauth-rails_csrf_protection" # Use Active Storage variants [https://guides.rubyonrails.org/active_storage_overview.html#transforming-images] # gem "image_processing", "~> 1.2" diff --git a/Gemfile.lock b/Gemfile.lock index 80687ff3..5be1a1d6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -84,10 +84,13 @@ GEM tzinfo (~> 2.0) addressable (2.8.1) public_suffix (>= 2.0.2, < 6.0) + aes_key_wrap (1.1.0) ast (2.4.2) + attr_required (1.0.1) autoprefixer-rails (10.4.7.0) execjs (~> 2) bcrypt (3.1.18) + bindata (2.4.14) bindex (0.8.1) bootsnap (1.13.0) msgpack (~> 1.2) @@ -124,9 +127,17 @@ GEM factory_bot_rails (6.2.0) factory_bot (~> 6.2.0) railties (>= 5.0.0) + faraday (2.7.2) + faraday-net_http (>= 2.0, < 3.1) + ruby2_keywords (>= 0.0.4) + faraday-follow_redirects (0.3.0) + faraday (>= 1, < 3) + faraday-net_http (3.0.2) ffi (1.15.5) globalid (1.0.0) activesupport (>= 5.0) + hashie (5.0.0) + httpclient (2.8.3) i18n (1.12.0) concurrent-ruby (~> 1.0) importmap-rails (1.1.5) @@ -139,6 +150,12 @@ GEM actionview (>= 5.0.0) activesupport (>= 5.0.0) json (2.6.2) + json-jwt (1.16.1) + activesupport (>= 4.2) + aes_key_wrap + bindata + faraday (~> 2.0) + faraday-follow_redirects loofah (2.19.0) crass (~> 1.0.2) nokogiri (>= 1.5.9) @@ -165,6 +182,27 @@ GEM nokogiri (1.13.9) mini_portile2 (~> 2.8.0) racc (~> 1.4) + omniauth (2.1.0) + 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.5.0) + omniauth (>= 1.9, < 3) + openid_connect (~> 1.1) + openid_connect (1.4.2) + activemodel + attr_required (>= 1.0.0) + json-jwt (>= 1.15.0) + net-smtp + rack-oauth2 (~> 1.21) + swd (~> 1.3) + tzinfo + validate_email + validate_url + webfinger (~> 1.2) orm_adapter (0.5.0) parallel (1.22.1) parser (3.1.2.1) @@ -176,6 +214,14 @@ GEM nio4r (~> 2.0) racc (1.6.0) rack (2.2.4) + rack-oauth2 (1.21.3) + activesupport + attr_required + httpclient + json-jwt (>= 1.11.0) + rack (>= 2.1.0) + rack-protection (3.0.5) + rack rack-test (2.0.2) rack (>= 1.3) rails (7.0.4) @@ -288,6 +334,10 @@ GEM mini_portile2 (~> 2.8.0) stimulus-rails (1.1.1) railties (>= 6.0.0) + swd (1.3.0) + activesupport (>= 3) + attr_required (>= 0.0.5) + httpclient (>= 2.4) thor (1.2.1) tilt (2.0.11) timeout (0.3.0) @@ -299,6 +349,12 @@ GEM concurrent-ruby (~> 1.0) unicode-display_width (2.3.0) uri (0.10.0) + validate_email (0.1.6) + activemodel (>= 3.0) + mail (>= 2.2.5) + validate_url (1.0.15) + activemodel (>= 3.0.0) + public_suffix warden (1.2.9) rack (>= 2.0.9) web-console (4.2.0) @@ -310,6 +366,9 @@ GEM nokogiri (~> 1.6) rubyzip (>= 1.3.0) selenium-webdriver (~> 4.0) + webfinger (1.2.0) + activesupport + httpclient (>= 2.4) websocket (1.2.9) websocket-driver (0.7.5) websocket-extensions (>= 0.1.0) @@ -335,6 +394,8 @@ DEPENDENCIES importmap-rails jbuilder net-http + omniauth-rails_csrf_protection + omniauth_openid_connect pg puma (~> 5.0) rails (~> 7.0.4) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb new file mode 100644 index 00000000..ded6215f --- /dev/null +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -0,0 +1,17 @@ +class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController + def openid_connect + @user = User.from_omniauth(request.env["omniauth.auth"]) + if @user.persisted? + sign_in_and_redirect @user + set_flash_message(:notice, :success, kind: "OpenID Connect") if is_navigational_format? + else + set_flash_message(:alert, :failure, kind: OmniAuth::Utils.camelize(failed_strategy.name), reason: failure_message) + redirect_to root_path + end + end + + def failure + set_flash_message(:alert, :failure, kind: OmniAuth::Utils.camelize(failed_strategy.name), reason: failure_message) + 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 0edf6621..9658fc01 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -3,7 +3,7 @@ class User < ApplicationRecord # Include default devise modules. Others available are: # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable devise :database_authenticatable, :registerable, - :recoverable, :rememberable, :validatable + :recoverable, :rememberable, :validatable, :omniauthable, omniauth_providers: [:openid_connect] has_many :notifications, dependent: :destroy has_and_belongs_to_many :items, join_table: "wishlist" @@ -57,4 +57,11 @@ def to_member_of!(group) group.reload reload end + def self.from_omniauth(auth) + # Create user in database if it does not exist yet when logging in via OIDC + where(email:auth.info.email).first_or_create! do |user| + user.email = auth.info.email + user.password = Devise.friendly_token[0, 20] + end + end end diff --git a/app/views/devise/shared/_links.html.erb b/app/views/devise/shared/_links.html.erb index d11c0611..10faeee7 100644 --- a/app/views/devise/shared/_links.html.erb +++ b/app/views/devise/shared/_links.html.erb @@ -1,3 +1,14 @@ +<%- if devise_mapping.omniauthable? %> + <%- resource_class.omniauth_providers.each do |provider| %> + <%= button_to(omniauth_authorize_path(resource_name, provider), + class: 'btn btn-primary', id: "#{provider}-signin" , + method: :post, + data: {disable_with: " Logging in...", turbo: "false"}) do %> + <%= t('.sign_in_with_provider', provider: OmniAuth::Utils.camelize(provider)) %> + <% end %> + <% end %> +<% end %> + <%- if controller_name != 'sessions' %> <%= link_to t(".sign_in"), new_session_path(resource_name) %>
<% end %> @@ -18,8 +29,3 @@ <%= link_to t('.didn_t_receive_unlock_instructions'), new_unlock_path(resource_name) %>
<% end %> -<%- if devise_mapping.omniauthable? %> - <%- resource_class.omniauth_providers.each do |provider| %> - <%= link_to t('.sign_in_with_provider', provider: OmniAuth::Utils.camelize(provider)), omniauth_authorize_path(resource_name, provider), method: :post %>
- <% end %> -<% end %> diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index dd8929a1..694a4787 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -272,7 +272,24 @@ # ==> OmniAuth # 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 :openid_connect, + name: :openid_connect, + scope: %i[openid email profile], + response_type: :code, + client_options: { + port: 443, + scheme: "https", + host: "oidc.hpi.de", + # Instead of env vars, could also use Rails credentials store + # env vars are set on deployed Heroku instance, default to HPI OpenID client setup for local dev + # Requires server to be running on port 3000, as that is also set on the remote OIDC config (and is checked) + identifier: ENV["OIDC_CLIENT_ID"], + secret: ENV["OIDC_CLIENT_SECRET"], + redirect_uri: "#{ENV["APP_BASE_URL"] || "http://localhost:3000" }/users/auth/openid_connect/callback", + authorization_endpoint: "/auth" + }, + client_auth_method: :other, + discovery: true # ==> Warden configuration # If you want to use other strategies, that are not supported by Devise, or diff --git a/config/routes.rb b/config/routes.rb index e4c505e7..642a72d1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -6,7 +6,7 @@ # Define your application routes per the DSL in https://guides.rubyonrails.org/routing.html # https://github.com/heartcombo/devise/blob/main/README.md - devise_for :users, controllers: { registrations: 'users' } + devise_for :users, controllers: { registrations: 'users', omniauth_callbacks: "users/omniauth_callbacks" } devise_scope :user do get 'profile', to: 'users#profile' end diff --git a/db/schema.rb b/db/schema.rb index 77e5afb0..ad0371e9 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,12 +10,12 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2022_12_12_120242) do +ActiveRecord::Schema[7.0].define(version: 2023_01_03_132616) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false - t.bigint "record_id", null: false - t.bigint "blob_id", null: false + t.integer "record_id", null: false + t.integer "blob_id", null: false t.datetime "created_at", null: false t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true @@ -34,7 +34,7 @@ end create_table "active_storage_variant_records", force: :cascade do |t| - t.bigint "blob_id", null: false + t.integer "blob_id", null: false t.string "variation_digest", null: false t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end @@ -125,6 +125,8 @@ t.datetime "remember_created_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "provider", limit: 50, default: "", null: false + t.string "uid", limit: 50, default: "", null: false 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 From b1e9186561197725ebb76209cb1726daf90046d1 Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Tue, 3 Jan 2023 15:18:26 +0100 Subject: [PATCH 2/9] Fix rubocop issues Co-authored-by: Alexander Sohn --- app/controllers/users/omniauth_callbacks_controller.rb | 2 +- app/models/user.rb | 3 ++- config/initializers/devise.rb | 6 +++--- 3 files changed, 6 insertions(+), 5 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index ded6215f..d3e8e98f 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -14,4 +14,4 @@ def failure set_flash_message(:alert, :failure, kind: OmniAuth::Utils.camelize(failed_strategy.name), reason: failure_message) 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 9658fc01..9d4bd3fc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -57,9 +57,10 @@ def to_member_of!(group) group.reload reload end + def self.from_omniauth(auth) # Create user in database if it does not exist yet when logging in via OIDC - where(email:auth.info.email).first_or_create! do |user| + where(email: auth.info.email).first_or_create! do |user| user.email = auth.info.email user.password = Devise.friendly_token[0, 20] end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 694a4787..23e80aad 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -283,9 +283,9 @@ # Instead of env vars, could also use Rails credentials store # env vars are set on deployed Heroku instance, default to HPI OpenID client setup for local dev # Requires server to be running on port 3000, as that is also set on the remote OIDC config (and is checked) - identifier: ENV["OIDC_CLIENT_ID"], - secret: ENV["OIDC_CLIENT_SECRET"], - redirect_uri: "#{ENV["APP_BASE_URL"] || "http://localhost:3000" }/users/auth/openid_connect/callback", + identifier: ENV.fetch("OIDC_CLIENT_ID", nil), + secret: ENV.fetch("OIDC_CLIENT_SECRET", nil), + redirect_uri: "#{ENV['APP_BASE_URL'] || 'http://localhost:3000'}/users/auth/openid_connect/callback", authorization_endpoint: "/auth" }, client_auth_method: :other, From e6aec7f336c8000328a419040fd9640c9bc4d4a2 Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Thu, 5 Jan 2023 15:50:38 +0100 Subject: [PATCH 3/9] Revert DB Schema changes Co-authored-by: Alexander Sohn --- db/schema.rb | 90 +++------------------------------------------------- 1 file changed, 5 insertions(+), 85 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index ad0371e9..e3b9f4b0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,12 +10,12 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_01_03_132616) do +ActiveRecord::Schema[7.0].define(version: 2022_11_22_114920) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false - t.integer "record_id", null: false - t.integer "blob_id", null: false + t.bigint "record_id", null: false + t.bigint "blob_id", null: false t.datetime "created_at", null: false t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true @@ -34,24 +34,11 @@ end create_table "active_storage_variant_records", force: :cascade do |t| - t.integer "blob_id", null: false + t.bigint "blob_id", null: false t.string "variation_digest", null: false t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end - create_table "added_to_waitlist_notifications", force: :cascade do |t| - t.integer "item_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["item_id"], name: "index_added_to_waitlist_notifications_on_item_id" - end - - create_table "groups", force: :cascade do |t| - t.string "name" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - end - create_table "items", force: :cascade do |t| t.string "name" t.string "category" @@ -65,58 +52,19 @@ t.datetime "updated_at", null: false t.integer "owner" t.integer "holder" - t.integer "lend_status", default: 0 t.index ["holder"], name: "index_items_on_holder" t.index ["owner"], name: "index_items_on_owner" end - create_table "lend_request_notifications", force: :cascade do |t| - t.integer "borrower_id", null: false - t.integer "item_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["borrower_id"], name: "index_lend_request_notifications_on_borrower_id" - t.index ["item_id"], name: "index_lend_request_notifications_on_item_id" - end - - create_table "memberships", force: :cascade do |t| - t.string "type" - t.integer "group_id", null: false - t.integer "user_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["group_id", "user_id"], name: "index_memberships_on_group_id_and_user_id", unique: true - t.index ["group_id"], name: "index_memberships_on_group_id" - t.index ["user_id"], name: "index_memberships_on_user_id" - end - - create_table "move_up_on_waitlist_notifications", force: :cascade do |t| - t.integer "item_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["item_id"], name: "index_move_up_on_waitlist_notifications_on_item_id" - end - create_table "notifications", force: :cascade do |t| + t.string "notification_snippet" t.datetime "date" t.integer "user_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "actable_type" - t.integer "actable_id" - t.index ["actable_type", "actable_id"], name: "index_notifications_on_actable" t.index ["user_id"], name: "index_notifications_on_user_id" end - create_table "return_request_notifications", force: :cascade do |t| - t.integer "borrower_id", null: false - t.integer "item_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["borrower_id"], name: "index_return_request_notifications_on_borrower_id" - t.index ["item_id"], name: "index_return_request_notifications_on_item_id" - end - create_table "users", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false @@ -125,41 +73,13 @@ t.datetime "remember_created_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "provider", limit: 50, default: "", null: false - t.string "uid", limit: 50, default: "", null: false 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 - create_table "users_waitlists", id: false, force: :cascade do |t| - t.integer "user_id", null: false - t.integer "waitlist_id", null: false - end - - create_table "waitlists", force: :cascade do |t| - t.integer "item_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["item_id"], name: "index_waitlists_on_item_id" - end - - create_table "wishlist", id: false, force: :cascade do |t| - t.integer "user_id", null: false - t.integer "item_id", null: false - end - add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" - add_foreign_key "added_to_waitlist_notifications", "items" add_foreign_key "items", "users", column: "holder" add_foreign_key "items", "users", column: "owner" - add_foreign_key "lend_request_notifications", "items" - add_foreign_key "lend_request_notifications", "users", column: "borrower_id" - add_foreign_key "memberships", "groups" - add_foreign_key "memberships", "users" - add_foreign_key "move_up_on_waitlist_notifications", "items" add_foreign_key "notifications", "users" - add_foreign_key "return_request_notifications", "items" - add_foreign_key "return_request_notifications", "users", column: "borrower_id" - add_foreign_key "waitlists", "items" end From 4ed31c33ad156ea66146a019624c73ef1a4a9d93 Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Thu, 5 Jan 2023 15:51:47 +0100 Subject: [PATCH 4/9] Revert DB Schema changes - like, eally Co-authored-by: Alexander Sohn --- db/schema.rb | 82 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 80 insertions(+), 2 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index e3b9f4b0..77e5afb0 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: 2022_11_22_114920) do +ActiveRecord::Schema[7.0].define(version: 2022_12_12_120242) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -39,6 +39,19 @@ t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end + create_table "added_to_waitlist_notifications", force: :cascade do |t| + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["item_id"], name: "index_added_to_waitlist_notifications_on_item_id" + end + + create_table "groups", force: :cascade do |t| + t.string "name" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + end + create_table "items", force: :cascade do |t| t.string "name" t.string "category" @@ -52,19 +65,58 @@ t.datetime "updated_at", null: false t.integer "owner" t.integer "holder" + t.integer "lend_status", default: 0 t.index ["holder"], name: "index_items_on_holder" t.index ["owner"], name: "index_items_on_owner" end + create_table "lend_request_notifications", force: :cascade do |t| + t.integer "borrower_id", null: false + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["borrower_id"], name: "index_lend_request_notifications_on_borrower_id" + t.index ["item_id"], name: "index_lend_request_notifications_on_item_id" + end + + create_table "memberships", force: :cascade do |t| + t.string "type" + t.integer "group_id", null: false + t.integer "user_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["group_id", "user_id"], name: "index_memberships_on_group_id_and_user_id", unique: true + t.index ["group_id"], name: "index_memberships_on_group_id" + t.index ["user_id"], name: "index_memberships_on_user_id" + end + + create_table "move_up_on_waitlist_notifications", force: :cascade do |t| + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["item_id"], name: "index_move_up_on_waitlist_notifications_on_item_id" + end + create_table "notifications", force: :cascade do |t| - t.string "notification_snippet" t.datetime "date" t.integer "user_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "actable_type" + t.integer "actable_id" + t.index ["actable_type", "actable_id"], name: "index_notifications_on_actable" t.index ["user_id"], name: "index_notifications_on_user_id" end + create_table "return_request_notifications", force: :cascade do |t| + t.integer "borrower_id", null: false + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["borrower_id"], name: "index_return_request_notifications_on_borrower_id" + t.index ["item_id"], name: "index_return_request_notifications_on_item_id" + end + create_table "users", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false @@ -77,9 +129,35 @@ t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end + create_table "users_waitlists", id: false, force: :cascade do |t| + t.integer "user_id", null: false + t.integer "waitlist_id", null: false + end + + create_table "waitlists", force: :cascade do |t| + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["item_id"], name: "index_waitlists_on_item_id" + end + + create_table "wishlist", id: false, force: :cascade do |t| + t.integer "user_id", null: false + t.integer "item_id", null: false + end + add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" + add_foreign_key "added_to_waitlist_notifications", "items" add_foreign_key "items", "users", column: "holder" add_foreign_key "items", "users", column: "owner" + add_foreign_key "lend_request_notifications", "items" + add_foreign_key "lend_request_notifications", "users", column: "borrower_id" + add_foreign_key "memberships", "groups" + add_foreign_key "memberships", "users" + add_foreign_key "move_up_on_waitlist_notifications", "items" add_foreign_key "notifications", "users" + add_foreign_key "return_request_notifications", "items" + add_foreign_key "return_request_notifications", "users", column: "borrower_id" + add_foreign_key "waitlists", "items" end From 1a9d825969f9df5e887629ecb21f9ccd91a56658 Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Thu, 5 Jan 2023 16:16:55 +0100 Subject: [PATCH 5/9] Add tests Co-authored-by: Alexander Sohn --- config/environments/test.rb | 3 ++ config/initializers/devise.rb | 4 +-- db/schema.rb | 10 +++--- spec/features/users/oidc_spec.rb | 54 ++++++++++++++++++++++++++++++++ 4 files changed, 65 insertions(+), 6 deletions(-) create mode 100644 spec/features/users/oidc_spec.rb diff --git a/config/environments/test.rb b/config/environments/test.rb index 6ea4d1e7..1d05c9fc 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -57,4 +57,7 @@ # Annotate rendered view with file names. # config.action_view.annotate_rendered_view_with_filenames = true + + # Enable testing for OIDC + OmniAuth.config.test_mode = true end diff --git a/config/initializers/devise.rb b/config/initializers/devise.rb index 23e80aad..043c07ae 100644 --- a/config/initializers/devise.rb +++ b/config/initializers/devise.rb @@ -283,8 +283,8 @@ # Instead of env vars, could also use Rails credentials store # env vars are set on deployed Heroku instance, default to HPI OpenID client setup for local dev # Requires server to be running on port 3000, as that is also set on the remote OIDC config (and is checked) - identifier: ENV.fetch("OIDC_CLIENT_ID", nil), - secret: ENV.fetch("OIDC_CLIENT_SECRET", nil), + identifier: ENV.fetch("OIDC_CLIENT_ID", "dev"), + secret: ENV.fetch("OIDC_CLIENT_SECRET", "secret"), redirect_uri: "#{ENV['APP_BASE_URL'] || 'http://localhost:3000'}/users/auth/openid_connect/callback", authorization_endpoint: "/auth" }, diff --git a/db/schema.rb b/db/schema.rb index f39cf66a..3487c150 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,12 +10,12 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_01_02_165603) do +ActiveRecord::Schema[7.0].define(version: 2023_01_03_132616) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false - t.bigint "record_id", null: false - t.bigint "blob_id", null: false + t.integer "record_id", null: false + t.integer "blob_id", null: false t.datetime "created_at", null: false t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true @@ -34,7 +34,7 @@ end create_table "active_storage_variant_records", force: :cascade do |t| - t.bigint "blob_id", null: false + t.integer "blob_id", null: false t.string "variation_digest", null: false t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end @@ -145,6 +145,8 @@ t.datetime "remember_created_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "provider", limit: 50, default: "", null: false + t.string "uid", limit: 50, default: "", null: false 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/features/users/oidc_spec.rb b/spec/features/users/oidc_spec.rb new file mode 100644 index 00000000..54b49eac --- /dev/null +++ b/spec/features/users/oidc_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require "rails_helper" + +describe "OpenId Connect Login", type: :feature do + context "with a valid OIDC session returned" do + before do + OmniAuth.config.mock_auth[:openid_connect] = OmniAuth::AuthHash.new( + provider: "openid_connect", + uid: "test.user", + info: { + email: "test.user@hpi.de" + } + ) + + visit new_user_session_path + find_by_id('openid_connect-signin').click + end + + it "redirects to dashboard path" do + expect(page).to have_current_path(dashboard_path) + end + + it "displays a success message" do + expect(page).to have_css(".alert-success") + end + end + + context "with invalid oidc session returned" do + before do + @omniauth_logger = OmniAuth.config.logger + # Change OmniAuth logger (default output to STDOUT) + OmniAuth.config.logger = Rails.logger + + OmniAuth.config.mock_auth[:openid_connect] = :invalid_credentials + visit new_user_session_path + find_by_id('openid_connect-signin').click + end + + after(:all) do + # Restore the original logger + OmniAuth.config.logger = @omniauth_logger + end + + it "redirects to login path" do + expect(page).to have_current_path(new_user_session_path) + end + + it "shows an error message" do + expect(page).to have_css(".alert-danger") + end + + end +end \ No newline at end of file From b4c4c46224cc321f433754f3e2c2427b06054eae Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Thu, 5 Jan 2023 16:18:11 +0100 Subject: [PATCH 6/9] Fix linter issues Co-authored-by: Alexander Sohn --- spec/features/users/oidc_spec.rb | 7 +------ 1 file changed, 1 insertion(+), 6 deletions(-) diff --git a/spec/features/users/oidc_spec.rb b/spec/features/users/oidc_spec.rb index 54b49eac..83c9ed26 100644 --- a/spec/features/users/oidc_spec.rb +++ b/spec/features/users/oidc_spec.rb @@ -37,11 +37,6 @@ find_by_id('openid_connect-signin').click end - after(:all) do - # Restore the original logger - OmniAuth.config.logger = @omniauth_logger - end - it "redirects to login path" do expect(page).to have_current_path(new_user_session_path) end @@ -51,4 +46,4 @@ end end -end \ No newline at end of file +end From 53a02934ca076ea4b51b01ce963317ddcd009fcb Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Fri, 6 Jan 2023 11:54:45 +0100 Subject: [PATCH 7/9] Remove schema changes and remove CSRF on certain route --- .../users/omniauth_callbacks_controller.rb | 2 + db/schema.rb | 39 ++++--------------- 2 files changed, 9 insertions(+), 32 deletions(-) diff --git a/app/controllers/users/omniauth_callbacks_controller.rb b/app/controllers/users/omniauth_callbacks_controller.rb index d3e8e98f..1043c483 100644 --- a/app/controllers/users/omniauth_callbacks_controller.rb +++ b/app/controllers/users/omniauth_callbacks_controller.rb @@ -1,4 +1,6 @@ class Users::OmniauthCallbacksController < Devise::OmniauthCallbacksController + # See https://github.com/omniauth/omniauth/wiki/FAQ#rails-session-is-clobbered-after-callback-on-developer-strategy + skip_before_action :verify_authenticity_token, only: :openid_connect def openid_connect @user = User.from_omniauth(request.env["omniauth.auth"]) if @user.persisted? diff --git a/db/schema.rb b/db/schema.rb index 3487c150..77e5afb0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,12 +10,12 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[7.0].define(version: 2023_01_03_132616) do +ActiveRecord::Schema[7.0].define(version: 2022_12_12_120242) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false - t.integer "record_id", null: false - t.integer "blob_id", null: false + t.bigint "record_id", null: false + t.bigint "blob_id", null: false t.datetime "created_at", null: false t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true @@ -34,7 +34,7 @@ end create_table "active_storage_variant_records", force: :cascade do |t| - t.integer "blob_id", null: false + t.bigint "blob_id", null: false t.string "variation_digest", null: false t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end @@ -75,7 +75,6 @@ t.integer "item_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.boolean "accepted" t.index ["borrower_id"], name: "index_lend_request_notifications_on_borrower_id" t.index ["item_id"], name: "index_lend_request_notifications_on_item_id" end @@ -100,32 +99,13 @@ create_table "notifications", force: :cascade do |t| t.datetime "date" - t.integer "receiver_id", null: false + t.integer "user_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "actable_type" t.integer "actable_id" - t.boolean "active", default: false, null: false - t.boolean "unread" t.index ["actable_type", "actable_id"], name: "index_notifications_on_actable" - t.index ["receiver_id"], name: "index_notifications_on_receiver_id" - end - - create_table "return_accepted_notifications", force: :cascade do |t| - t.integer "owner_id", null: false - t.integer "item_id", null: false - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["item_id"], name: "index_return_accepted_notifications_on_item_id" - t.index ["owner_id"], name: "index_return_accepted_notifications_on_owner_id" - end - - create_table "return_declined_notifications", force: :cascade do |t| - t.integer "owner_id", null: false - t.string "item_name" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["owner_id"], name: "index_return_declined_notifications_on_owner_id" + t.index ["user_id"], name: "index_notifications_on_user_id" end create_table "return_request_notifications", force: :cascade do |t| @@ -145,8 +125,6 @@ t.datetime "remember_created_at" t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "provider", limit: 50, default: "", null: false - t.string "uid", limit: 50, default: "", null: false 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 @@ -178,10 +156,7 @@ add_foreign_key "memberships", "groups" add_foreign_key "memberships", "users" add_foreign_key "move_up_on_waitlist_notifications", "items" - add_foreign_key "notifications", "users", column: "receiver_id" - add_foreign_key "return_accepted_notifications", "items" - add_foreign_key "return_accepted_notifications", "users", column: "owner_id" - add_foreign_key "return_declined_notifications", "users", column: "owner_id" + add_foreign_key "notifications", "users" add_foreign_key "return_request_notifications", "items" add_foreign_key "return_request_notifications", "users", column: "borrower_id" add_foreign_key "waitlists", "items" From 9aec1c985e5ce22c7bc108454c3876882f0b8f81 Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Fri, 6 Jan 2023 11:56:19 +0100 Subject: [PATCH 8/9] Update schema to current version on main --- db/schema.rb | 39 +++++++++++++++++++++++++++++++++++---- 1 file changed, 35 insertions(+), 4 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index 77e5afb0..69804fbd 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: 2022_12_12_120242) do +ActiveRecord::Schema[7.0].define(version: 2023_01_02_165603) do create_table "active_storage_attachments", force: :cascade do |t| t.string "name", null: false t.string "record_type", null: false @@ -75,10 +75,18 @@ t.integer "item_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.boolean "accepted" t.index ["borrower_id"], name: "index_lend_request_notifications_on_borrower_id" t.index ["item_id"], name: "index_lend_request_notifications_on_item_id" end + create_table "lending_accepted_notifications", force: :cascade do |t| + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["item_id"], name: "index_lending_accepted_notifications_on_item_id" + end + create_table "memberships", force: :cascade do |t| t.string "type" t.integer "group_id", null: false @@ -99,13 +107,32 @@ create_table "notifications", force: :cascade do |t| t.datetime "date" - t.integer "user_id", null: false + t.integer "receiver_id", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false t.string "actable_type" t.integer "actable_id" + t.boolean "active", default: false, null: false + t.boolean "unread" t.index ["actable_type", "actable_id"], name: "index_notifications_on_actable" - t.index ["user_id"], name: "index_notifications_on_user_id" + t.index ["receiver_id"], name: "index_notifications_on_receiver_id" + end + + create_table "return_accepted_notifications", force: :cascade do |t| + t.integer "owner_id", null: false + t.integer "item_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["item_id"], name: "index_return_accepted_notifications_on_item_id" + t.index ["owner_id"], name: "index_return_accepted_notifications_on_owner_id" + end + + create_table "return_declined_notifications", force: :cascade do |t| + t.integer "owner_id", null: false + t.string "item_name" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["owner_id"], name: "index_return_declined_notifications_on_owner_id" end create_table "return_request_notifications", force: :cascade do |t| @@ -153,10 +180,14 @@ add_foreign_key "items", "users", column: "owner" add_foreign_key "lend_request_notifications", "items" add_foreign_key "lend_request_notifications", "users", column: "borrower_id" + add_foreign_key "lending_accepted_notifications", "items" add_foreign_key "memberships", "groups" add_foreign_key "memberships", "users" add_foreign_key "move_up_on_waitlist_notifications", "items" - add_foreign_key "notifications", "users" + add_foreign_key "notifications", "users", column: "receiver_id" + add_foreign_key "return_accepted_notifications", "items" + add_foreign_key "return_accepted_notifications", "users", column: "owner_id" + add_foreign_key "return_declined_notifications", "users", column: "owner_id" add_foreign_key "return_request_notifications", "items" add_foreign_key "return_request_notifications", "users", column: "borrower_id" add_foreign_key "waitlists", "items" From 021d7d92207de13ae78f3ae4ca655b5a230d9071 Mon Sep 17 00:00:00 2001 From: Aaron Schlitt Date: Fri, 6 Jan 2023 13:27:12 +0100 Subject: [PATCH 9/9] Remove unlocalized text --- app/views/devise/shared/_links.html.erb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/devise/shared/_links.html.erb b/app/views/devise/shared/_links.html.erb index 10faeee7..adcafecc 100644 --- a/app/views/devise/shared/_links.html.erb +++ b/app/views/devise/shared/_links.html.erb @@ -3,7 +3,7 @@ <%= button_to(omniauth_authorize_path(resource_name, provider), class: 'btn btn-primary', id: "#{provider}-signin" , method: :post, - data: {disable_with: " Logging in...", turbo: "false"}) do %> + data: {disable_with: "", turbo: "false"}) do %> <%= t('.sign_in_with_provider', provider: OmniAuth::Utils.camelize(provider)) %> <% end %> <% end %>