From f4621e773b1cf01a596f413ab86bb10b4efbf1a1 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 7 Apr 2026 16:36:32 -0400 Subject: [PATCH 01/74] add oidc config, controller, and models --- Gemfile | 1 + Gemfile.lock | 47 ++++++ app/controllers/oidc_login_controller.rb | 158 ++++++++++++++++++ app/models/auth_request.rb | 2 + app/models/oidc_config.rb | 48 ++++++ config/initializers/oidc.rb | 3 + config/oidc_providers.yml | 20 +++ config/routes.rb | 5 + .../20260407003623_create_auth_requests.rb | 13 ++ db/schema.rb | 14 +- db/seeds_oidc.rb | 7 + spec/factories.rb | 7 + spec/models/auth_request_spec.rb | 5 + spec/requests/oidc_login_spec.rb | 25 +++ 14 files changed, 353 insertions(+), 2 deletions(-) create mode 100644 app/controllers/oidc_login_controller.rb create mode 100644 app/models/auth_request.rb create mode 100644 app/models/oidc_config.rb create mode 100644 config/initializers/oidc.rb create mode 100644 config/oidc_providers.yml create mode 100644 db/migrate/20260407003623_create_auth_requests.rb create mode 100644 db/seeds_oidc.rb create mode 100644 spec/models/auth_request_spec.rb create mode 100644 spec/requests/oidc_login_spec.rb diff --git a/Gemfile b/Gemfile index 540f19cb5..7cd218362 100644 --- a/Gemfile +++ b/Gemfile @@ -58,6 +58,7 @@ gem 'find_with_order' # For handling zip file uploads and extraction gem 'rubyzip' +gem 'openid_connect' group :development, :test do gem 'debug', platforms: %i[mri mingw x64_mingw] diff --git a/Gemfile.lock b/Gemfile.lock index aac9f8c1a..070284ddc 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -79,11 +79,14 @@ GEM uri (>= 0.13.1) addressable (2.8.7) public_suffix (>= 2.0.2, < 7.0) + aes_key_wrap (1.1.0) ast (2.4.3) + attr_required (1.0.2) base64 (0.3.0) bcrypt (3.1.20) benchmark (0.4.1) bigdecimal (3.2.3) + bindata (2.5.1) bootsnap (1.18.6) msgpack (~> 1.2) builder (3.3.0) @@ -134,6 +137,8 @@ GEM docile (1.4.1) domain_name (0.6.20240107) drb (2.2.3) + email_validator (2.2.4) + activemodel erb (5.0.2) erubi (1.13.1) factory_bot (6.5.5) @@ -147,6 +152,8 @@ GEM faraday-net_http (>= 2.0, < 3.5) json logger + faraday-follow_redirects (0.5.0) + faraday (>= 1, < 3) faraday-http-cache (2.5.1) faraday (>= 0.8) faraday-net_http (3.4.1) @@ -174,6 +181,13 @@ GEM rdoc (>= 4.0.0) reline (>= 0.4.2) json (2.15.0) + json-jwt (1.17.0) + activesupport (>= 4.2) + aes_key_wrap + base64 + bindata + faraday (~> 2.0) + faraday-follow_redirects json-schema (5.2.2) addressable (~> 2.8) bigdecimal (~> 3.1) @@ -238,6 +252,19 @@ GEM faraday (>= 1, < 3) sawyer (~> 0.9) open4 (1.3.4) + openid_connect (2.3.1) + 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) ostruct (0.6.3) parallel (1.27.0) parser (3.3.9.0) @@ -260,6 +287,13 @@ GEM rack-cors (3.0.0) logger rack (>= 3.0.14) + rack-oauth2 (2.3.0) + activesupport + attr_required + faraday (~> 2.0) + faraday-follow_redirects + json-jwt (>= 1.11.0) + rack (>= 2.1.0) rack-session (2.1.1) base64 (>= 0.1.0) rack (>= 3.0.0) @@ -374,6 +408,11 @@ GEM sqlite3 (1.7.3) mini_portile2 (~> 2.8.0) stringio (3.1.7) + swd (2.0.3) + activesupport (>= 3) + attr_required (>= 0.0.5) + faraday (~> 2.0) + faraday-follow_redirects sync (0.5.0) term-ansicolor (1.11.3) tins (~> 1) @@ -395,6 +434,13 @@ GEM unicode-emoji (4.1.0) uri (1.0.3) useragent (0.16.11) + validate_url (1.0.15) + activemodel (>= 3.0.0) + public_suffix + webfinger (2.1.3) + activesupport + faraday (~> 2.0) + faraday-follow_redirects websocket-driver (0.8.0) base64 websocket-extensions (>= 0.1.0) @@ -435,6 +481,7 @@ DEPENDENCIES mutex_m mysql2 (~> 0.5.7) observer + openid_connect ostruct psych (~> 5.2) puma (~> 6.4) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb new file mode 100644 index 000000000..0c2b6558c --- /dev/null +++ b/app/controllers/oidc_login_controller.rb @@ -0,0 +1,158 @@ +require 'json_web_token' + +class OidcLoginController < ApplicationController + skip_before_action :authenticate_request! + + # GET /auth/providers + def providers + render json: OidcConfig.public_list + end + + # POST /auth/client-select + def client_select + provider = OidcConfig.find(params[:provider]) + client = build_client(provider) + + # Generate state, nonce, and PKCE + state = SecureRandom.hex(32) + nonce = SecureRandom.hex(32) + code_verifier = SecureRandom.urlsafe_base64(64) + code_challenge = Base64.urlsafe_encode64( + Digest::SHA256.digest(code_verifier), padding: false + ) + + # Store in DB for callback verification + AuthRequest.create!( + state: state, + nonce: nonce, + code_verifier: code_verifier, + provider: params[:provider] + ) + + # Build the authorization URL + authorization_uri = client.authorization_uri( + scope: [:openid, :email, :profile], + state: state, + nonce: nonce, + code_challenge: code_challenge, + code_challenge_method: "S256" + ) + + render json: { redirect_uri: authorization_uri } + end + + # POST /auth/callback + def callback + # Look up and consume the auth request + auth_request = AuthRequest + .where("created_at > ?", 5.minutes.ago) + .find_by!(state: params[:state]) + auth_request.destroy! + + provider = OidcConfig.find(auth_request.provider) + client = build_client(provider) + + # Exchange authorization code for tokens + client.authorization_code = params[:code] + access_token = client.access_token!( + code_verifier: auth_request.code_verifier + ) + + # Decode and verify the ID token + discovery = discover(provider) + id_token = OpenIDConnect::ResponseObject::IdToken.decode( + access_token.id_token, + discovery.jwks + ) + id_token.verify!( + issuer: discovery.issuer, + client_id: provider["client_id"], + nonce: auth_request.nonce + ) + + # Match to existing user by email + email = id_token.raw_attributes["email"] + user = User.find_by(email: email) + + if user + token = issue_jwt(user) + render json: { user: user.as_json, session_token: token } + else + render json: { error: "No account found for #{email}" }, status: :not_found + end + + rescue ActiveRecord::RecordNotFound + render json: { error: "Invalid or expired login request" }, status: :unprocessable_entity + rescue OpenIDConnect::ResponseObject::IdToken::InvalidToken => e + render json: { error: "Token verification failed: #{e.message}" }, status: :unauthorized + end + + # Temporary: handle IdP redirect directly instead of via frontend + def callback_redirect + auth_request = AuthRequest + .where("created_at > ?", 5.minutes.ago) + .find_by!(state: params[:state]) + auth_request.destroy! + + provider = OidcConfig.find(auth_request.provider) + client = build_client(provider) + + client.authorization_code = params[:code] + access_token = client.access_token!( + code_verifier: auth_request.code_verifier + ) + + discovery = discover(provider) + id_token = OpenIDConnect::ResponseObject::IdToken.decode( + access_token.id_token, + discovery.jwks + ) + id_token.verify!( + issuer: discovery.issuer, + client_id: provider["client_id"], + nonce: auth_request.nonce + ) + + email = id_token.raw_attributes["email"] + user = User.find_by(email: email) + + if user + render json: { user: user.as_json, session_token: issue_jwt(user) } + else + render json: { error: "No account found for #{email}", email: email }, status: :not_found + end + end + + private + + def build_client(provider) + discovery = discover(provider) + OpenIDConnect::Client.new( + identifier: provider["client_id"], + secret: provider["client_secret"], + redirect_uri: provider["redirect_uri"], + authorization_endpoint: discovery.authorization_endpoint, + token_endpoint: discovery.token_endpoint, + userinfo_endpoint: discovery.userinfo_endpoint + ) + end + + def discover(provider) + # Cache discovery per provider at app level + @discoveries ||= {} + @discoveries[provider["issuer"]] ||= + OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) + end + + # Note this is the same as authentication_controller, could combine into the user model + def issue_jwt(user) + payload = { + id: user.id, + name: user.name, + full_name: user.full_name, + role: user.role.name, + institution_id: user.institution.id + } + JsonWebToken.encode(payload, 24.hours.from_now) + end +end diff --git a/app/models/auth_request.rb b/app/models/auth_request.rb new file mode 100644 index 000000000..e2029e943 --- /dev/null +++ b/app/models/auth_request.rb @@ -0,0 +1,2 @@ +class AuthRequest < ApplicationRecord +end diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb new file mode 100644 index 000000000..31e9a6eaf --- /dev/null +++ b/app/models/oidc_config.rb @@ -0,0 +1,48 @@ +class OidcConfig + class ProviderNotFound < StandardError; end + + CONFIG_FILE = Rails.root.join("config", "oidc_providers.yml").freeze + + def self.providers + @providers ||= begin + yaml = ERB.new(File.read(CONFIG_FILE)).result + parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) + providers_config = parsed&.fetch("providers", {}) + validate!(providers_config || {}) + end + end + + def self.find(provider_key) + providers.fetch(provider_key) do + raise ProviderNotFound, "Unknown OIDC provider: #{provider_key}" + end + end + + def self.public_list + providers.map { |key, cfg| provider_summary(key, cfg) } + end + + def self.reload! + @providers = nil + end + + private + + REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri scopes].freeze + + def self.validate!(providers) + return {} if providers.blank? + + providers.each do |key, cfg| + missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } + if missing.any? + raise "OIDC provider '#{key}' is missing required config: #{missing.join(', ')}" + end + end + providers + end + + def self.provider_summary(key, cfg) + { id: key, name: cfg["display_name"] } + end +end \ No newline at end of file diff --git a/config/initializers/oidc.rb b/config/initializers/oidc.rb new file mode 100644 index 000000000..419b95f1f --- /dev/null +++ b/config/initializers/oidc.rb @@ -0,0 +1,3 @@ +Rails.application.config.after_initialize do + OidcConfig.providers +end \ No newline at end of file diff --git a/config/oidc_providers.yml b/config/oidc_providers.yml new file mode 100644 index 000000000..7cc8d7af9 --- /dev/null +++ b/config/oidc_providers.yml @@ -0,0 +1,20 @@ +providers: + google-ncsu: + display_name: Google NCSU + issuer: https://accounts.google.com + client_id: <%= ENV['GOOG_CLIENT_ID'] %> + client_secret: <%= ENV['GOOG_CLIENT_SECRET'] %> + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + discovery: true + +# Add more providers here: + # okta: + # display_name: Okta + # icon: okta + # issuer: https://.okta.com + # client_id: <%= ENV['OKTA_OIDC_CLIENT_ID'] %> + # client_secret: <%= ENV['OKTA_OIDC_CLIENT_SECRET'] %> + # redirect_uri: <%= ENV['OKTA_OIDC_REDIRECT_URI'] %> + # scopes: openid email profile + # discovery: true \ No newline at end of file diff --git a/config/routes.rb b/config/routes.rb index 57559d007..4b846e40d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -214,4 +214,9 @@ resources :assignments do resources :duties, controller: 'assignments_duties', only: [:index, :create, :destroy] end + + get "auth/providers", to: "oidc_login#providers" + post "auth/client-select", to: "oidc_login#client_select" + post "auth/callback", to: "oidc_login#callback" + get "auth/callback", to: "oidc_login#callback" # temporary: for direct IdP redirect during testing end diff --git a/db/migrate/20260407003623_create_auth_requests.rb b/db/migrate/20260407003623_create_auth_requests.rb new file mode 100644 index 000000000..e334af1b5 --- /dev/null +++ b/db/migrate/20260407003623_create_auth_requests.rb @@ -0,0 +1,13 @@ +class CreateAuthRequests < ActiveRecord::Migration[8.0] + def change + create_table :auth_requests do |t| + t.string :state + t.string :nonce + t.string :code_verifier + t.string :provider + + t.timestamps + end + add_index :auth_requests, :state + end +end diff --git a/db/schema.rb b/db/schema.rb index cddbe12c6..58019229a 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[8.0].define(version: 2026_03_13_064334) do +ActiveRecord::Schema[8.0].define(version: 2026_04_07_003623) do create_table "account_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "username" t.string "full_name" @@ -117,6 +117,16 @@ t.index ["duty_id"], name: "index_assignments_duties_on_duty_id" end + create_table "auth_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| + t.string "state" + t.string "nonce" + t.string "code_verifier" + t.string "provider" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["state"], name: "index_auth_requests_on_state" + end + create_table "bookmark_ratings", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.integer "bookmark_id" t.integer "user_id" @@ -456,9 +466,9 @@ add_foreign_key "assignments_duties", "duties" add_foreign_key "courses", "institutions" add_foreign_key "courses", "users", column: "instructor_id" + add_foreign_key "duties", "users", column: "instructor_id" add_foreign_key "invitations", "participants", column: "from_id" add_foreign_key "invitations", "participants", column: "to_id" - add_foreign_key "duties", "users", column: "instructor_id" add_foreign_key "items", "questionnaires" add_foreign_key "participants", "duties" add_foreign_key "participants", "join_team_requests" diff --git a/db/seeds_oidc.rb b/db/seeds_oidc.rb new file mode 100644 index 000000000..651659077 --- /dev/null +++ b/db/seeds_oidc.rb @@ -0,0 +1,7 @@ +User.find_or_create_by!(email: "jweisz@ncsu.edu") do |user| + user.name = "jweisz" + user.password = "password123" + user.full_name = "John Weisz" + user.institution_id = 1 + user.role_id = 1 +end \ No newline at end of file diff --git a/spec/factories.rb b/spec/factories.rb index 1556b02b2..778ca2a06 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -1,6 +1,13 @@ # frozen_string_literal: true FactoryBot.define do + factory :auth_request do + state { "MyString" } + nonce { "MyString" } + code_verifier { "MyString" } + provider { "MyString" } + end + factory :student_task do assignment { nil } current_stage { "MyString" } diff --git a/spec/models/auth_request_spec.rb b/spec/models/auth_request_spec.rb new file mode 100644 index 000000000..071de2242 --- /dev/null +++ b/spec/models/auth_request_spec.rb @@ -0,0 +1,5 @@ +require 'rails_helper' + +RSpec.describe AuthRequest, type: :model do + pending "add some examples to (or delete) #{__FILE__}" +end diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb new file mode 100644 index 000000000..321de528c --- /dev/null +++ b/spec/requests/oidc_login_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' + +RSpec.describe "OidcLogins", type: :request do + describe "GET /providers" do + it "returns http success" do + get "/oidc_login/providers" + expect(response).to have_http_status(:success) + end + end + + describe "GET /client_select" do + it "returns http success" do + get "/oidc_login/client_select" + expect(response).to have_http_status(:success) + end + end + + describe "GET /callback" do + it "returns http success" do + get "/oidc_login/callback" + expect(response).to have_http_status(:success) + end + end + +end From bf658f6f01972f848eca74f9545f611d6f72e8c2 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Tue, 7 Apr 2026 16:44:42 -0400 Subject: [PATCH 02/74] seed team users that match google login for dev convenience Added seed data for three users with specific attributes. --- db/seeds_oidc.rb | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/db/seeds_oidc.rb b/db/seeds_oidc.rb index 651659077..6e4a2b81d 100644 --- a/db/seeds_oidc.rb +++ b/db/seeds_oidc.rb @@ -1,7 +1,26 @@ +# For conenience, will drop on final PR User.find_or_create_by!(email: "jweisz@ncsu.edu") do |user| user.name = "jweisz" user.password = "password123" user.full_name = "John Weisz" user.institution_id = 1 user.role_id = 1 -end \ No newline at end of file +end + +# For conenience, will drop on final PR +User.find_or_create_by!(email: "jvargas6@ncsu.edu") do |user| + user.name = "jvargas6" + user.password = "password123" + user.full_name = "Jose Vargas" + user.institution_id = 1 + user.role_id = 1 +end + +# For conenience, will drop on final PR +User.find_or_create_by!(email: "jcmonseu@ncsu.edu") do |user| + user.name = "jcmonseu" + user.password = "password123" + user.full_name = "Jared Monseur" + user.institution_id = 1 + user.role_id = 1 +end From 0c3f19e5cc2c274aac6c6fa7f4358ba2f86b2644 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Tue, 7 Apr 2026 17:37:21 -0400 Subject: [PATCH 03/74] Remove callback_redirect method from oidc_login_controller Removed the callback_redirect method for handling IdP redirects directly. --- app/controllers/oidc_login_controller.rb | 36 ------------------------ 1 file changed, 36 deletions(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 0c2b6558c..cbf5b0947 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -87,42 +87,6 @@ def callback render json: { error: "Token verification failed: #{e.message}" }, status: :unauthorized end - # Temporary: handle IdP redirect directly instead of via frontend - def callback_redirect - auth_request = AuthRequest - .where("created_at > ?", 5.minutes.ago) - .find_by!(state: params[:state]) - auth_request.destroy! - - provider = OidcConfig.find(auth_request.provider) - client = build_client(provider) - - client.authorization_code = params[:code] - access_token = client.access_token!( - code_verifier: auth_request.code_verifier - ) - - discovery = discover(provider) - id_token = OpenIDConnect::ResponseObject::IdToken.decode( - access_token.id_token, - discovery.jwks - ) - id_token.verify!( - issuer: discovery.issuer, - client_id: provider["client_id"], - nonce: auth_request.nonce - ) - - email = id_token.raw_attributes["email"] - user = User.find_by(email: email) - - if user - render json: { user: user.as_json, session_token: issue_jwt(user) } - else - render json: { error: "No account found for #{email}", email: email }, status: :not_found - end - end - private def build_client(provider) From e386c6dd9ab4740db4e1b270c5dff84d1ec802f0 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Tue, 7 Apr 2026 17:37:54 -0400 Subject: [PATCH 04/74] remove testing route --- config/routes.rb | 1 - 1 file changed, 1 deletion(-) diff --git a/config/routes.rb b/config/routes.rb index 4b846e40d..4ec5199c3 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -218,5 +218,4 @@ get "auth/providers", to: "oidc_login#providers" post "auth/client-select", to: "oidc_login#client_select" post "auth/callback", to: "oidc_login#callback" - get "auth/callback", to: "oidc_login#callback" # temporary: for direct IdP redirect during testing end From 52a89d964a4430907d37b3f418d44e08d7c10efb Mon Sep 17 00:00:00 2001 From: John Weisz Date: Fri, 10 Apr 2026 16:58:10 -0400 Subject: [PATCH 05/74] remove discovery option, assume always true to match OIDC spec --- app/controllers/oidc_login_controller.rb | 2 +- config/oidc_providers.yml | 4 +--- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index cbf5b0947..86f28f435 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -102,7 +102,7 @@ def build_client(provider) end def discover(provider) - # Cache discovery per provider at app level + # Avoid duplicate discovery calls within the same request @discoveries ||= {} @discoveries[provider["issuer"]] ||= OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) diff --git a/config/oidc_providers.yml b/config/oidc_providers.yml index 7cc8d7af9..0901c9403 100644 --- a/config/oidc_providers.yml +++ b/config/oidc_providers.yml @@ -6,7 +6,6 @@ providers: client_secret: <%= ENV['GOOG_CLIENT_SECRET'] %> redirect_uri: http://localhost:3000/auth/callback scopes: openid email profile - discovery: true # Add more providers here: # okta: @@ -16,5 +15,4 @@ providers: # client_id: <%= ENV['OKTA_OIDC_CLIENT_ID'] %> # client_secret: <%= ENV['OKTA_OIDC_CLIENT_SECRET'] %> # redirect_uri: <%= ENV['OKTA_OIDC_REDIRECT_URI'] %> - # scopes: openid email profile - # discovery: true \ No newline at end of file + # scopes: openid email profile \ No newline at end of file From 5a42d96f8d1aca9e0371207669a3a36cb7c644d2 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Apr 2026 21:31:06 +0000 Subject: [PATCH 06/74] Initial plan From 01b969af6cfb8cfd81b7d953f94e4595bfc40566 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Fri, 10 Apr 2026 21:44:52 +0000 Subject: [PATCH 07/74] Add Swagger/OpenAPI documentation for OIDC provider endpoints - Replace auto-generated oidc_login_spec.rb with rswag Swagger annotations - Document GET /auth/providers with response schema and examples - Document POST /auth/client-select with request/response schemas - Document POST /auth/callback with success (200), not found (404), and invalid state (422) response schemas - Regenerate swagger/v1/swagger.yaml with OIDC endpoints Agent-Logs-Url: https://github.com/johnmweisz/reimplementation-back-end/sessions/ef5f760e-93dd-4f11-a15f-a0f4fe906cc5 Co-authored-by: johnmweisz <47699943+johnmweisz@users.noreply.github.com> --- spec/requests/oidc_login_spec.rb | 296 ++++++++++- swagger/v1/swagger.yaml | 843 +++++++++++++++++++++++++------ 2 files changed, 971 insertions(+), 168 deletions(-) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 321de528c..a36df97d6 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -1,25 +1,291 @@ -require 'rails_helper' +# frozen_string_literal: true -RSpec.describe "OidcLogins", type: :request do - describe "GET /providers" do - it "returns http success" do - get "/oidc_login/providers" - expect(response).to have_http_status(:success) - end +require 'swagger_helper' +require 'json_web_token' + +RSpec.describe OidcLoginController, type: :request do + before(:all) do + @roles = create_roles_hierarchy + @institution = Institution.first || Institution.create!(name: "Test Institution") end - describe "GET /client_select" do - it "returns http success" do - get "/oidc_login/client_select" - expect(response).to have_http_status(:success) + # ─── GET /auth/providers ───────────────────────────────────────────── + + path '/auth/providers' do + get 'List available OIDC providers' do + tags 'OIDC Authentication' + produces 'application/json' + security [] + description 'Returns the list of configured OIDC identity providers that the front end can offer to users.' + + response '200', 'list of providers' do + schema type: :array, + items: { + type: :object, + properties: { + id: { type: :string, example: 'google-ncsu' }, + name: { type: :string, example: 'Google NCSU' } + }, + required: %w[id name] + } + + before do + allow(OidcConfig).to receive(:public_list).and_return([ + { id: "google-ncsu", name: "Google NCSU" } + ]) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json).to be_an(Array) + expect(json.first).to include("id", "name") + end + end end end - describe "GET /callback" do - it "returns http success" do - get "/oidc_login/callback" - expect(response).to have_http_status(:success) + # ─── POST /auth/client-select ─────────────────────────────────────── + + path '/auth/client-select' do + post 'Initiate OIDC login for a chosen provider' do + tags 'OIDC Authentication' + consumes 'application/json' + produces 'application/json' + security [] + description <<~DESC + Accepts a provider key, performs OIDC discovery, generates PKCE and state parameters, + persists an AuthRequest for later verification, and returns the provider's authorization URL + that the front end should redirect the user to. + DESC + + parameter name: :body, in: :body, schema: { + type: :object, + properties: { + provider: { type: :string, example: 'google-ncsu', description: 'Key identifying the OIDC provider' } + }, + required: %w[provider] + } + + response '200', 'authorization redirect URI' do + schema type: :object, + properties: { + redirect_uri: { type: :string, example: 'https://accounts.google.com/o/oauth2/v2/auth?client_id=...&scope=openid+email+profile&state=...&nonce=...' } + }, + required: %w[redirect_uri] + + let(:body) { { provider: "google-ncsu" } } + + before do + provider_cfg = { + "display_name" => "Google NCSU", + "issuer" => "https://accounts.google.com", + "client_id" => "test-client-id", + "client_secret" => "test-client-secret", + "redirect_uri" => "http://localhost:3000/auth/callback", + "scopes" => "openid email profile" + } + allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) + + discovery = instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo" + ) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with("https://accounts.google.com").and_return(discovery) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["redirect_uri"]).to be_present + end + end end end + # ─── POST /auth/callback ──────────────────────────────────────────── + + path '/auth/callback' do + post 'Exchange an OIDC authorization code for a session token' do + tags 'OIDC Authentication' + consumes 'application/json' + produces 'application/json' + security [] + description <<~DESC + Called by the front end after the user is redirected back from the identity provider. + Exchanges the authorization code for tokens, verifies the ID token, and returns + a local JWT session token if the user's email matches an existing account. + DESC + + parameter name: :body, in: :body, schema: { + type: :object, + properties: { + state: { type: :string, description: 'The state parameter returned by the identity provider' }, + code: { type: :string, description: 'The authorization code returned by the identity provider' } + }, + required: %w[state code] + } + + # ── 200 — successful authentication ── + + response '200', 'authenticated user with session token' do + schema type: :object, + properties: { + user: { + type: :object, + description: 'Serialized user record' + }, + session_token: { type: :string, description: 'JWT session token' } + }, + required: %w[user session_token] + + let(:user) do + User.create!( + name: "oidcuser", + password: "password", + role_id: @roles[:student].id, + full_name: "OIDC User", + email: "oidcuser@ncsu.edu", + institution: @institution + ) + end + + let(:auth_request) do + AuthRequest.create!( + state: "valid-state-hex", + nonce: "valid-nonce-hex", + code_verifier: "valid-code-verifier", + provider: "google-ncsu" + ) + end + + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + # Ensure user exists before callback + user + + provider_cfg = { + "display_name" => "Google NCSU", + "issuer" => "https://accounts.google.com", + "client_id" => "test-client-id", + "client_secret" => "test-client-secret", + "redirect_uri" => "http://localhost:3000/auth/callback", + "scopes" => "openid email profile" + } + allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) + + discovery = instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", + issuer: "https://accounts.google.com", + jwks: instance_double(JSON::JWK::Set) + ) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with("https://accounts.google.com").and_return(discovery) + + fake_access_token = instance_double( + OpenIDConnect::AccessToken, + id_token: "fake.id.token" + ) + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) + .and_return(fake_access_token) + + id_token_obj = instance_double(OpenIDConnect::ResponseObject::IdToken, + raw_attributes: { "email" => "oidcuser@ncsu.edu" }) + allow(id_token_obj).to receive(:verify!).and_return(true) + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .and_return(id_token_obj) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["session_token"]).to be_present + expect(json["user"]).to be_present + end + end + + # ── 404 — no account for the email ── + + response '404', 'no account found for the email' do + schema type: :object, + properties: { + error: { type: :string, example: 'No account found for unknown@example.com' } + }, + required: %w[error] + + let(:auth_request) do + AuthRequest.create!( + state: "state-no-user", + nonce: "nonce-no-user", + code_verifier: "verifier-no-user", + provider: "google-ncsu" + ) + end + + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + provider_cfg = { + "display_name" => "Google NCSU", + "issuer" => "https://accounts.google.com", + "client_id" => "test-client-id", + "client_secret" => "test-client-secret", + "redirect_uri" => "http://localhost:3000/auth/callback", + "scopes" => "openid email profile" + } + allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) + + discovery = instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", + issuer: "https://accounts.google.com", + jwks: instance_double(JSON::JWK::Set) + ) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with("https://accounts.google.com").and_return(discovery) + + fake_access_token = instance_double( + OpenIDConnect::AccessToken, + id_token: "fake.id.token" + ) + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) + .and_return(fake_access_token) + + id_token_obj = instance_double(OpenIDConnect::ResponseObject::IdToken, + raw_attributes: { "email" => "unknown@example.com" }) + allow(id_token_obj).to receive(:verify!).and_return(true) + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .and_return(id_token_obj) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/No account found/) + end + end + + # ── 422 — invalid or expired state ── + + response '422', 'invalid or expired login request' do + schema type: :object, + properties: { + error: { type: :string, example: 'Invalid or expired login request' } + }, + required: %w[error] + + let(:body) { { state: "nonexistent-state", code: "authorization-code" } } + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to eq("Invalid or expired login request") + end + end + end + end end diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 10e04b446..c819424ec 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -713,6 +713,200 @@ paths: responses: '204': description: successful + "/grades/{assignment_id}/view_all_scores": + get: + summary: Retrieve all review scores for an assignment + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: assignment_id + in: path + description: ID of the assignment + required: true + schema: + type: integer + - name: participant_id + in: query + required: false + description: ID of the participant + schema: + type: integer + - name: team_id + in: query + required: false + description: ID of the team + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns team scores when team_id provided + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized + "/grades/{assignment_id}/view_our_scores": + get: + summary: Retrieve team scores for the requesting student + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: assignment_id + in: path + description: ID of the assignment + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns team scores + '403': + description: Assignment Participant not found + '401': + description: Unauthorized + "/grades/{assignment_id}/view_my_scores": + get: + summary: Retrieve individual participant scores + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: assignment_id + in: path + description: ID of the assignment + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns participant scores + '403': + description: Participant not found + '401': + description: Unauthorized + "/grades/{participant_id}/edit": + get: + summary: Get grade assignment interface data + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: participant_id + in: path + description: ID of the participant + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns participant, assignment, items, and scores + '404': + description: Participant not found + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized + "/grades/{participant_id}/assign_grade": + patch: + summary: Assign grades and comment to team + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: participant_id + in: path + description: ID of the participant + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Team grade and comment assigned successfully + '422': + description: Failed to assign team grade or comment + '404': + description: Participant not found + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized + requestBody: + content: + application/json: + schema: + type: object + properties: + grade_for_submission: + type: number + description: Grade for the submission + comment_for_submission: + type: string + description: Comment for the submission + "/grades/{participant_id}/instructor_review": + get: + summary: Begin or continue grading a submission + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: participant_id + in: path + description: ID of the participant + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns mapping and redirect information for existing review + '404': + description: Participant not found + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized "/institutions": get: summary: list institutions @@ -808,6 +1002,12 @@ paths: summary: list invitations tags: - Invitations + parameters: + - name: Authorization + in: header + required: true + schema: + type: string responses: '200': description: Success @@ -815,12 +1015,18 @@ paths: summary: create invitation tags: - Invitations - parameters: [] + parameters: + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string responses: '201': description: Create successful - '422': - description: Invalid request + '404': + description: Participant not found requestBody: content: application/json: @@ -829,24 +1035,24 @@ paths: properties: assignment_id: type: integer - from_id: - type: integer - to_id: - type: integer - reply_status: + username: type: string required: - assignment_id - - from_id - - to_id + - username "/invitations/{id}": parameters: - name: id in: path - description: id of the invitation required: true schema: type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string get: summary: show invitation tags: @@ -854,8 +1060,8 @@ paths: responses: '200': description: Show invitation - '404': - description: Not found + '403': + description: Cannot see other's invitations patch: summary: update invitation tags: @@ -863,11 +1069,11 @@ paths: parameters: [] responses: '200': - description: Update successful + description: Retraction successful + '403': + description: Cannot retract other's invitations '422': description: Invalid request - '404': - description: Not found requestBody: content: application/json: @@ -876,39 +1082,87 @@ paths: properties: reply_status: type: string - required: [] delete: summary: Delete invitation tags: - Invitations + parameters: + - name: Authorization + in: header + required: true + schema: + type: string responses: - '204': + '403': + description: Student cannot delete invitations + '200': description: Delete successful - '404': - description: Not found - "/invitations/user/{user_id}/assignment/{assignment_id}": + "/invitations/sent_by/team/{team_id}": parameters: - - name: user_id + - name: team_id in: path - description: id of user required: true schema: type: integer - - name: assignment_id + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + get: + summary: Show all invitations sent by team + tags: + - Invitations + responses: + '200': + description: OK + '403': + description: Not allowed + "/invitations/sent_by/participant/{participant_id}": + parameters: + - name: participant_id in: path - description: id of assignment required: true schema: type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string get: - summary: Show all invitation for the given user and assignment + summary: Show all invitations sent by participant tags: - Invitations responses: '200': - description: Show all invitations for the user for an assignment - '404': - description: Not found + description: OK + '403': + description: Not allowed + "/invitations/sent_to/{participant_id}": + parameters: + - name: participant_id + in: path + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + get: + summary: Show all invitations sent to participant + tags: + - Invitations + responses: + '200': + description: OK + '403': + description: Not allowed "/participants/user/{user_id}": get: summary: Retrieve participants for a specific user @@ -1036,28 +1290,149 @@ paths: schema: type: string responses: - '201': - description: Participant successfully added - '500': - description: Participant already exist - '404': - description: Assignment not found + '201': + description: Participant successfully added + '500': + description: Participant already exist + '404': + description: Assignment not found + '422': + description: Invalid authorization + requestBody: + content: + application/json: + schema: + type: object + properties: + user_id: + type: integer + description: ID of the user + assignment_id: + type: integer + description: ID of the assignment + required: + - user_id + - assignment_id + "/project_topics": + get: + summary: Get project topics + parameters: + - name: assignment_id + in: query + required: true + schema: + type: integer + - name: topic_identifier + in: query + required: false + schema: + type: string + tags: + - ProjectTopic + responses: + '200': + description: successful + delete: + summary: Delete project topics + parameters: + - name: assignment_id + in: query + description: Assignment ID + schema: + type: integer + - name: topic_ids + in: query + items: + type: string + description: Topic Identifiers to delete + required: false + schema: + type: array + - name: Authorization + in: header + tags: + - ProjectTopic + responses: + '422': + description: when assignment_id parameter is missing + '204': + description: when assignment_id parameter is present but topic_ids parameter + is missing + post: + summary: create a new topic in the sheet + tags: + - ProjectTopic + parameters: [] + responses: + '201': + description: when the assignment is not a microtask + '422': + description: when the assignment does not exist + requestBody: + content: + application/json: + schema: + type: object + properties: + topic_identifier: + type: string + topic_name: + type: string + max_choosers: + type: integer + category: + type: string + assignment_id: + type: integer + micropayment: + type: integer + required: + - topic_identifier + - topic_name + - max_choosers + - category + - assignment_id + - micropayment + "/project_topics/{id}": + parameters: + - name: id + in: path + description: ID of the project topic + required: true + schema: + type: integer + put: + summary: update a topic in the sheet + tags: + - ProjectTopic + parameters: [] + responses: + '200': + description: when the assignment is not a microtask '422': - description: Invalid authorization + description: when the assignment does not exist requestBody: content: application/json: schema: type: object properties: - user_id: + topic_identifier: + type: string + topic_name: + type: string + max_choosers: type: integer - description: ID of the user + category: + type: string assignment_id: type: integer - description: ID of the assignment + micropayment: + type: integer required: - - user_id + - topic_identifier + - topic_name + - category - assignment_id "/questionnaires": get: @@ -1201,6 +1576,27 @@ paths: required: true schema: type: integer + - name: questionnaire1 + in: body + schema: + type: object + properties: + name: + type: string + questionnaire_type: + type: string + private: + type: boolean + min_question_score: + type: integer + max_question_score: + type: integer + instructor_id: + type: integer + required: + - name + - questionnaire_type + - instructor_id post: summary: copy questionnaire tags: @@ -1508,16 +1904,134 @@ paths: description: participant not found '401': description: unauthorized request has error response + "/student_teams/view": + get: + summary: View student team + tags: + - Student Teams + parameters: + - name: student_id + in: query + schema: + type: integer + - name: Authorization + in: header + required: true + schema: + type: string + responses: + '200': + description: Student not on any team + '403': + description: TA cannot access + "/student_teams": + post: + summary: Create team + tags: + - Student Teams + parameters: + - name: Authorization + in: header + required: true + schema: + type: string + responses: + '200': + description: Create successful + '422': + description: Duplicate name + '403': + description: Unauthorized + requestBody: + content: + application/json: + schema: + type: object + properties: + team: + type: object + properties: + name: + type: string + required: + - name + assignment_id: + type: integer + student_id: + type: integer + required: + - team + - assignment_id + - student_id + "/student_teams/update": + put: + summary: Update team name + tags: + - Student Teams + parameters: + - name: Authorization + in: header + required: true + schema: + type: string + - name: student_id + in: query + schema: + type: integer + responses: + '200': + description: Update successful + '422': + description: Duplicate name + '403': + description: Unauthorized + requestBody: + content: + application/json: + schema: + type: object + properties: + team: + type: object + properties: + name: + type: string + required: + - team + "/student_teams/leave": + put: + summary: Leave team + tags: + - Student Teams + parameters: + - name: Authorization + in: header + required: true + schema: + type: string + - name: student_id + in: query + required: true + schema: + type: integer + responses: + '200': + description: Leave successful + '403': + description: TA cannot leave "/submitted_content": get: summary: list all submission records tags: - SubmittedContent + responses: + '200': + description: successful post: summary: create a submission record tags: - SubmittedContent - parameters: [ ] + parameters: [] responses: '201': description: created @@ -1566,106 +2080,8 @@ paths: description: successful '404': description: not found - "/submitted_content/submit_hyperlink": - post: - summary: submit hyperlink (swagger) - tags: - - SubmittedContent - parameters: - - name: Authorization - in: header - schema: - type: string - - name: id - in: query - schema: - type: string - required: true - - name: submit_link - in: query - schema: - type: string - required: true - topic_identifier: - type: integer - topic_name: - type: string - max_choosers: - type: integer - category: - type: string - assignment_id: - type: integer - micropayment: - type: integer - required: - - topic_identifier - - topic_name - - max_choosers - - category - - assignment_id - - micropayment - "/project_topics/{id}": - parameters: - - name: id - in: path - description: id of the sign up topic - required: true - schema: - type: integer - put: - summary: update a new topic in the sheet - tags: - - ProjectTopic - parameters: [ ] - responses: - '200': - description: successful - "/project_topics": - get: - summary: Get project topics - parameters: - - name: assignment_id - in: query - description: Assignment ID - required: true - schema: - type: integer - - name: topic_ids - in: query - description: Topic Identifier - required: false - schema: - type: string - tags: - - ProjectTopic - responses: - '200': - description: successful - delete: - summary: Delete project topics - parameters: - - name: assignment_id - in: query - description: Assignment ID - required: true - schema: - type: integer - - name: topic_ids - in: query - items: - type: string - description: Topic Identifiers to delete - required: false - schema: - type: array - tags: - - ProjectTopic - responses: - '200': - description: successful "/submitted_content/remove_hyperlink": - post: + delete: summary: remove hyperlink (swagger) tags: - SubmittedContent @@ -1716,11 +2132,8 @@ paths: content: multipart/form-data: schema: - type: object - properties: - uploaded_file: - type: string - format: binary + type: string + format: binary required: true "/submitted_content/folder_action": post: @@ -1760,7 +2173,6 @@ paths: required: true schema: type: string - description: Folder path (use "/" for root) - name: download in: query required: true @@ -1776,8 +2188,6 @@ paths: description: cannot send whole folder '404': description: file does not exist - '200': - description: file downloaded "/submitted_content/list_files": get: summary: list files and hyperlinks @@ -1789,13 +2199,10 @@ paths: required: true schema: type: string - - name: folder + - name: folder[name] in: query schema: - type: object - properties: - name: - type: string + type: string responses: '200': description: directory listed @@ -1929,6 +2336,136 @@ paths: required: - user_name - password + "/auth/providers": + get: + summary: List available OIDC providers + tags: + - OIDC Authentication + security: [] + description: Returns the list of configured OIDC identity providers that the + front end can offer to users. + responses: + '200': + description: list of providers + content: + application/json: + schema: + type: array + items: + type: object + properties: + id: + type: string + example: google-ncsu + name: + type: string + example: Google NCSU + required: + - id + - name + "/auth/client-select": + post: + summary: Initiate OIDC login for a chosen provider + tags: + - OIDC Authentication + security: [] + description: | + Accepts a provider key, performs OIDC discovery, generates PKCE and state parameters, + persists an AuthRequest for later verification, and returns the provider's authorization URL + that the front end should redirect the user to. + parameters: [] + responses: + '200': + description: authorization redirect URI + content: + application/json: + schema: + type: object + properties: + redirect_uri: + type: string + example: https://accounts.google.com/o/oauth2/v2/auth?client_id=...&scope=openid+email+profile&state=...&nonce=... + required: + - redirect_uri + requestBody: + content: + application/json: + schema: + type: object + properties: + provider: + type: string + example: google-ncsu + description: Key identifying the OIDC provider + required: + - provider + "/auth/callback": + post: + summary: Exchange an OIDC authorization code for a session token + tags: + - OIDC Authentication + security: [] + description: | + Called by the front end after the user is redirected back from the identity provider. + Exchanges the authorization code for tokens, verifies the ID token, and returns + a local JWT session token if the user's email matches an existing account. + parameters: [] + responses: + '200': + description: authenticated user with session token + content: + application/json: + schema: + type: object + properties: + user: + type: object + description: Serialized user record + session_token: + type: string + description: JWT session token + required: + - user + - session_token + '404': + description: no account found for the email + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: No account found for unknown@example.com + required: + - error + '422': + description: invalid or expired login request + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: Invalid or expired login request + required: + - error + requestBody: + content: + application/json: + schema: + type: object + properties: + state: + type: string + description: The state parameter returned by the identity provider + code: + type: string + description: The authorization code returned by the identity provider + required: + - state + - code servers: - url: http://{defaultHost} variables: From 1f1cd0bbc4a7ac61ab86316dd4272e9d877ae0d0 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Fri, 10 Apr 2026 18:05:46 -0400 Subject: [PATCH 08/74] normalize login response --- app/controllers/oidc_login_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 86f28f435..94029312b 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -76,7 +76,7 @@ def callback if user token = issue_jwt(user) - render json: { user: user.as_json, session_token: token } + render json: { token: }, status: :ok else render json: { error: "No account found for #{email}" }, status: :not_found end From c4d418bd3bb7794113ad178f54ec6756cb38d7e1 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Fri, 10 Apr 2026 18:14:44 -0400 Subject: [PATCH 09/74] move jwt logic into user --- app/controllers/authentication_controller.rb | 7 +------ app/controllers/oidc_login_controller.rb | 16 +--------------- app/models/user.rb | 12 ++++++++++-- 3 files changed, 12 insertions(+), 23 deletions(-) diff --git a/app/controllers/authentication_controller.rb b/app/controllers/authentication_controller.rb index 696c4ab21..8f855afab 100644 --- a/app/controllers/authentication_controller.rb +++ b/app/controllers/authentication_controller.rb @@ -1,6 +1,3 @@ -# app/controllers/authentication_controller.rb -require 'json_web_token' - class AuthenticationController < ApplicationController skip_before_action :authenticate_request! @@ -8,9 +5,7 @@ class AuthenticationController < ApplicationController def login user = User.find_by(name: params[:user_name]) || User.find_by(email: params[:user_name]) if user&.authenticate(params[:password]) - payload = { id: user.id, name: user.name, full_name: user.full_name, role: user.role.name, - institution_id: user.institution.id } - token = JsonWebToken.encode(payload, 24.hours.from_now) + token = user.generate_jwt render json: { token: }, status: :ok else render json: { error: 'Invalid username / password' }, status: :unauthorized diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 94029312b..39df27413 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -1,5 +1,3 @@ -require 'json_web_token' - class OidcLoginController < ApplicationController skip_before_action :authenticate_request! @@ -75,7 +73,7 @@ def callback user = User.find_by(email: email) if user - token = issue_jwt(user) + token = user.generate_jwt render json: { token: }, status: :ok else render json: { error: "No account found for #{email}" }, status: :not_found @@ -107,16 +105,4 @@ def discover(provider) @discoveries[provider["issuer"]] ||= OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) end - - # Note this is the same as authentication_controller, could combine into the user model - def issue_jwt(user) - payload = { - id: user.id, - name: user.name, - full_name: user.full_name, - role: user.role.name, - institution_id: user.institution.id - } - JsonWebToken.encode(payload, 24.hours.from_now) - end end diff --git a/app/models/user.rb b/app/models/user.rb index 0e77e25dc..c8bb5d65a 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true +require 'json_web_token' class User < ApplicationRecord has_secure_password @@ -149,8 +150,15 @@ def set_defaults self.etc_icons_on_homepage ||= true end - def generate_jwt - JWT.encode({ id: id, exp: 60.days.from_now.to_i }, Rails.application.credentials.secret_key_base) + def generate_jwt(expiry = 24.hours.from_now) + payload = { + id: id, + name: name, + full_name: full_name, + role: role.name, + institution_id: institution.id + } + JsonWebToken.encode(payload, expiry) end end From 0efcd03b31f3a599bcf437104f67aa5b79ba523d Mon Sep 17 00:00:00 2001 From: John Weisz Date: Fri, 10 Apr 2026 18:29:16 -0400 Subject: [PATCH 10/74] add comment --- app/models/user.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/user.rb b/app/models/user.rb index c8bb5d65a..a8bc313b9 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -150,6 +150,7 @@ def set_defaults self.etc_icons_on_homepage ||= true end + # Return a signed jwt with a payload for frontend session creation def generate_jwt(expiry = 24.hours.from_now) payload = { id: id, From a4d499b6b83323c2c032b2bb36c3c04fc54cd61a Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 14:14:56 -0400 Subject: [PATCH 11/74] add user jwt tests, update login tests --- app/models/oidc_config.rb | 4 +++- spec/requests/authentication_spec.rb | 5 ++--- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index 31e9a6eaf..87c77fabb 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -36,7 +36,9 @@ def self.validate!(providers) providers.each do |key, cfg| missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? - raise "OIDC provider '#{key}' is missing required config: #{missing.join(', ')}" + Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}") + providers.delete(key) + next end end providers diff --git a/spec/requests/authentication_spec.rb b/spec/requests/authentication_spec.rb index 7441c7799..2543c6a15 100644 --- a/spec/requests/authentication_spec.rb +++ b/spec/requests/authentication_spec.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true require 'swagger_helper' -require 'json_web_token' RSpec.describe AuthenticationController, type: :request do before(:all) do @@ -38,7 +37,7 @@ end let(:credentials) { { user_name: user.name, password: 'password' } } - let(:token) { JsonWebToken.encode({id: user.id}) } + let(:token) { user.generate_jwt } let(:Authorization) { "Bearer #{token}" } let(:headers) { { "Authorization" => "Bearer #{token}" } } run_test! do |response| @@ -71,7 +70,7 @@ ) end let(:credentials) { { user_name: user.name, password: 'wrongpassword' } } - let(:token) { JsonWebToken.encode({id: user.id}) } + let(:token) { user.generate_jwt } let(:Authorization) { "Bearer #{token}" } let(:headers) { { "Authorization" => "Bearer #{token}" } } run_test! From 50cb82054f5a74a693f85c03ce3db2c6261902c8 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 14:16:48 -0400 Subject: [PATCH 12/74] add user jwt tests, update login tests --- spec/models/user_spec.rb | 42 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 spec/models/user_spec.rb diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb new file mode 100644 index 000000000..96269b187 --- /dev/null +++ b/spec/models/user_spec.rb @@ -0,0 +1,42 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe User, type: :model do + describe '#generate_jwt' do + it 'encodes the user attributes into a jwt' do + user = create( + :user, + name: 'jdoe', + email: 'jdoe@example.com', + full_name: 'John Doe' + ) + expiry = 2.hours.from_now + + token = user.generate_jwt(expiry) + payload = JsonWebToken.decode(token) + + # Aligns with frontend expectations + expect(payload[:id]).to eq(user.id) + expect(payload[:name]).to eq(user.name) + expect(payload[:full_name]).to eq(user.full_name) + expect(payload[:role]).to eq(user.role.name) + expect(payload[:institution_id]).to eq(user.institution.id) + expect(payload[:exp]).to eq(expiry.to_i) + end + it 'defaults to 24 hour expiry' do + user = create(:user) + token = user.generate_jwt + payload = JsonWebToken.decode(token) + + expect(payload[:exp]).to be_within(5).of(24.hours.from_now.to_i) + end + it 'raises an error when the token signature is invalid' do + user = create(:user) + token = user.generate_jwt + tampered_token = token.chop + + expect { JsonWebToken.decode(tampered_token) }.to raise_error(JWT::DecodeError) + end + end +end From 3008e3d13aa302eafbefdde3d4ba9c660334e286 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Apr 2026 18:39:17 +0000 Subject: [PATCH 13/74] Fix callback response schema, add 401 response, scope swagger changes to OIDC only - Update /auth/callback 200 schema to { token } matching current controller - Add 401 response for ID token verification failure - Change before(:all) to before(:each) for proper test isolation - Revert swagger.yaml to base and add only OIDC endpoint definitions Agent-Logs-Url: https://github.com/johnmweisz/reimplementation-back-end/sessions/5101208e-ff33-4866-9aa2-f677e45bb2ae Co-authored-by: johnmweisz <47699943+johnmweisz@users.noreply.github.com> --- spec/requests/oidc_login_spec.rb | 72 +++- swagger/v1/swagger.yaml | 711 +++++++------------------------ 2 files changed, 219 insertions(+), 564 deletions(-) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index a36df97d6..7df8b9cda 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -4,7 +4,7 @@ require 'json_web_token' RSpec.describe OidcLoginController, type: :request do - before(:all) do + before(:each) do @roles = create_roles_hierarchy @institution = Institution.first || Institution.create!(name: "Test Institution") end @@ -132,13 +132,9 @@ response '200', 'authenticated user with session token' do schema type: :object, properties: { - user: { - type: :object, - description: 'Serialized user record' - }, - session_token: { type: :string, description: 'JWT session token' } + token: { type: :string, description: 'JWT session token' } }, - required: %w[user session_token] + required: %w[token] let(:user) do User.create!( @@ -203,8 +199,7 @@ run_test! do |response| json = JSON.parse(response.body) - expect(json["session_token"]).to be_present - expect(json["user"]).to be_present + expect(json["token"]).to be_present end end @@ -286,6 +281,65 @@ expect(json["error"]).to eq("Invalid or expired login request") end end + + # ── 401 — token verification failed ── + + response '401', 'token verification failed' do + schema type: :object, + properties: { + error: { type: :string, example: 'Token verification failed: invalid signature' } + }, + required: %w[error] + + let(:auth_request) do + AuthRequest.create!( + state: "state-bad-token", + nonce: "nonce-bad-token", + code_verifier: "verifier-bad-token", + provider: "google-ncsu" + ) + end + + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + provider_cfg = { + "display_name" => "Google NCSU", + "issuer" => "https://accounts.google.com", + "client_id" => "test-client-id", + "client_secret" => "test-client-secret", + "redirect_uri" => "http://localhost:3000/auth/callback", + "scopes" => "openid email profile" + } + allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) + + discovery = instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", + issuer: "https://accounts.google.com", + jwks: instance_double(JSON::JWK::Set) + ) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with("https://accounts.google.com").and_return(discovery) + + fake_access_token = instance_double( + OpenIDConnect::AccessToken, + id_token: "fake.id.token" + ) + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) + .and_return(fake_access_token) + + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new("invalid signature")) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/Token verification failed/) + end + end end end end diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index c819424ec..bb41395cf 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -713,200 +713,6 @@ paths: responses: '204': description: successful - "/grades/{assignment_id}/view_all_scores": - get: - summary: Retrieve all review scores for an assignment - tags: - - Grades - security: - - bearer_auth: [] - parameters: - - name: assignment_id - in: path - description: ID of the assignment - required: true - schema: - type: integer - - name: participant_id - in: query - required: false - description: ID of the participant - schema: - type: integer - - name: team_id - in: query - required: false - description: ID of the team - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - responses: - '200': - description: Returns team scores when team_id provided - '403': - description: Forbidden - Student cannot access - '401': - description: Unauthorized - "/grades/{assignment_id}/view_our_scores": - get: - summary: Retrieve team scores for the requesting student - tags: - - Grades - security: - - bearer_auth: [] - parameters: - - name: assignment_id - in: path - description: ID of the assignment - required: true - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - responses: - '200': - description: Returns team scores - '403': - description: Assignment Participant not found - '401': - description: Unauthorized - "/grades/{assignment_id}/view_my_scores": - get: - summary: Retrieve individual participant scores - tags: - - Grades - security: - - bearer_auth: [] - parameters: - - name: assignment_id - in: path - description: ID of the assignment - required: true - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - responses: - '200': - description: Returns participant scores - '403': - description: Participant not found - '401': - description: Unauthorized - "/grades/{participant_id}/edit": - get: - summary: Get grade assignment interface data - tags: - - Grades - security: - - bearer_auth: [] - parameters: - - name: participant_id - in: path - description: ID of the participant - required: true - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - responses: - '200': - description: Returns participant, assignment, items, and scores - '404': - description: Participant not found - '403': - description: Forbidden - Student cannot access - '401': - description: Unauthorized - "/grades/{participant_id}/assign_grade": - patch: - summary: Assign grades and comment to team - tags: - - Grades - security: - - bearer_auth: [] - parameters: - - name: participant_id - in: path - description: ID of the participant - required: true - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - responses: - '200': - description: Team grade and comment assigned successfully - '422': - description: Failed to assign team grade or comment - '404': - description: Participant not found - '403': - description: Forbidden - Student cannot access - '401': - description: Unauthorized - requestBody: - content: - application/json: - schema: - type: object - properties: - grade_for_submission: - type: number - description: Grade for the submission - comment_for_submission: - type: string - description: Comment for the submission - "/grades/{participant_id}/instructor_review": - get: - summary: Begin or continue grading a submission - tags: - - Grades - security: - - bearer_auth: [] - parameters: - - name: participant_id - in: path - description: ID of the participant - required: true - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - responses: - '200': - description: Returns mapping and redirect information for existing review - '404': - description: Participant not found - '403': - description: Forbidden - Student cannot access - '401': - description: Unauthorized "/institutions": get: summary: list institutions @@ -1002,12 +808,6 @@ paths: summary: list invitations tags: - Invitations - parameters: - - name: Authorization - in: header - required: true - schema: - type: string responses: '200': description: Success @@ -1015,18 +815,12 @@ paths: summary: create invitation tags: - Invitations - parameters: - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string + parameters: [] responses: '201': description: Create successful - '404': - description: Participant not found + '422': + description: Invalid request requestBody: content: application/json: @@ -1035,24 +829,24 @@ paths: properties: assignment_id: type: integer - username: + from_id: + type: integer + to_id: + type: integer + reply_status: type: string required: - assignment_id - - username + - from_id + - to_id "/invitations/{id}": parameters: - name: id in: path + description: id of the invitation required: true schema: type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string get: summary: show invitation tags: @@ -1060,8 +854,8 @@ paths: responses: '200': description: Show invitation - '403': - description: Cannot see other's invitations + '404': + description: Not found patch: summary: update invitation tags: @@ -1069,11 +863,11 @@ paths: parameters: [] responses: '200': - description: Retraction successful - '403': - description: Cannot retract other's invitations + description: Update successful '422': description: Invalid request + '404': + description: Not found requestBody: content: application/json: @@ -1082,87 +876,39 @@ paths: properties: reply_status: type: string + required: [] delete: summary: Delete invitation tags: - Invitations - parameters: - - name: Authorization - in: header - required: true - schema: - type: string responses: - '403': - description: Student cannot delete invitations - '200': + '204': description: Delete successful - "/invitations/sent_by/team/{team_id}": - parameters: - - name: team_id - in: path - required: true - schema: - type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - get: - summary: Show all invitations sent by team - tags: - - Invitations - responses: - '200': - description: OK - '403': - description: Not allowed - "/invitations/sent_by/participant/{participant_id}": + '404': + description: Not found + "/invitations/user/{user_id}/assignment/{assignment_id}": parameters: - - name: participant_id + - name: user_id in: path + description: id of user required: true schema: type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string - get: - summary: Show all invitations sent by participant - tags: - - Invitations - responses: - '200': - description: OK - '403': - description: Not allowed - "/invitations/sent_to/{participant_id}": - parameters: - - name: participant_id + - name: assignment_id in: path + description: id of assignment required: true schema: type: integer - - name: Authorization - in: header - required: true - description: Bearer token - schema: - type: string get: - summary: Show all invitations sent to participant + summary: Show all invitation for the given user and assignment tags: - Invitations responses: '200': - description: OK - '403': - description: Not allowed + description: Show all invitations for the user for an assignment + '404': + description: Not found "/participants/user/{user_id}": get: summary: Retrieve participants for a specific user @@ -1313,127 +1059,6 @@ paths: required: - user_id - assignment_id - "/project_topics": - get: - summary: Get project topics - parameters: - - name: assignment_id - in: query - required: true - schema: - type: integer - - name: topic_identifier - in: query - required: false - schema: - type: string - tags: - - ProjectTopic - responses: - '200': - description: successful - delete: - summary: Delete project topics - parameters: - - name: assignment_id - in: query - description: Assignment ID - schema: - type: integer - - name: topic_ids - in: query - items: - type: string - description: Topic Identifiers to delete - required: false - schema: - type: array - - name: Authorization - in: header - tags: - - ProjectTopic - responses: - '422': - description: when assignment_id parameter is missing - '204': - description: when assignment_id parameter is present but topic_ids parameter - is missing - post: - summary: create a new topic in the sheet - tags: - - ProjectTopic - parameters: [] - responses: - '201': - description: when the assignment is not a microtask - '422': - description: when the assignment does not exist - requestBody: - content: - application/json: - schema: - type: object - properties: - topic_identifier: - type: string - topic_name: - type: string - max_choosers: - type: integer - category: - type: string - assignment_id: - type: integer - micropayment: - type: integer - required: - - topic_identifier - - topic_name - - max_choosers - - category - - assignment_id - - micropayment - "/project_topics/{id}": - parameters: - - name: id - in: path - description: ID of the project topic - required: true - schema: - type: integer - put: - summary: update a topic in the sheet - tags: - - ProjectTopic - parameters: [] - responses: - '200': - description: when the assignment is not a microtask - '422': - description: when the assignment does not exist - requestBody: - content: - application/json: - schema: - type: object - properties: - topic_identifier: - type: string - topic_name: - type: string - max_choosers: - type: integer - category: - type: string - assignment_id: - type: integer - micropayment: - type: integer - required: - - topic_identifier - - topic_name - - category - - assignment_id "/questionnaires": get: summary: list questionnaires @@ -1576,27 +1201,6 @@ paths: required: true schema: type: integer - - name: questionnaire1 - in: body - schema: - type: object - properties: - name: - type: string - questionnaire_type: - type: string - private: - type: boolean - min_question_score: - type: integer - max_question_score: - type: integer - instructor_id: - type: integer - required: - - name - - questionnaire_type - - instructor_id post: summary: copy questionnaire tags: @@ -1904,134 +1508,16 @@ paths: description: participant not found '401': description: unauthorized request has error response - "/student_teams/view": - get: - summary: View student team - tags: - - Student Teams - parameters: - - name: student_id - in: query - schema: - type: integer - - name: Authorization - in: header - required: true - schema: - type: string - responses: - '200': - description: Student not on any team - '403': - description: TA cannot access - "/student_teams": - post: - summary: Create team - tags: - - Student Teams - parameters: - - name: Authorization - in: header - required: true - schema: - type: string - responses: - '200': - description: Create successful - '422': - description: Duplicate name - '403': - description: Unauthorized - requestBody: - content: - application/json: - schema: - type: object - properties: - team: - type: object - properties: - name: - type: string - required: - - name - assignment_id: - type: integer - student_id: - type: integer - required: - - team - - assignment_id - - student_id - "/student_teams/update": - put: - summary: Update team name - tags: - - Student Teams - parameters: - - name: Authorization - in: header - required: true - schema: - type: string - - name: student_id - in: query - schema: - type: integer - responses: - '200': - description: Update successful - '422': - description: Duplicate name - '403': - description: Unauthorized - requestBody: - content: - application/json: - schema: - type: object - properties: - team: - type: object - properties: - name: - type: string - required: - - team - "/student_teams/leave": - put: - summary: Leave team - tags: - - Student Teams - parameters: - - name: Authorization - in: header - required: true - schema: - type: string - - name: student_id - in: query - required: true - schema: - type: integer - responses: - '200': - description: Leave successful - '403': - description: TA cannot leave "/submitted_content": get: summary: list all submission records tags: - SubmittedContent - responses: - '200': - description: successful post: summary: create a submission record tags: - SubmittedContent - parameters: [] + parameters: [ ] responses: '201': description: created @@ -2080,8 +1566,106 @@ paths: description: successful '404': description: not found - "/submitted_content/remove_hyperlink": + "/submitted_content/submit_hyperlink": + post: + summary: submit hyperlink (swagger) + tags: + - SubmittedContent + parameters: + - name: Authorization + in: header + schema: + type: string + - name: id + in: query + schema: + type: string + required: true + - name: submit_link + in: query + schema: + type: string + required: true + topic_identifier: + type: integer + topic_name: + type: string + max_choosers: + type: integer + category: + type: string + assignment_id: + type: integer + micropayment: + type: integer + required: + - topic_identifier + - topic_name + - max_choosers + - category + - assignment_id + - micropayment + "/project_topics/{id}": + parameters: + - name: id + in: path + description: id of the sign up topic + required: true + schema: + type: integer + put: + summary: update a new topic in the sheet + tags: + - ProjectTopic + parameters: [ ] + responses: + '200': + description: successful + "/project_topics": + get: + summary: Get project topics + parameters: + - name: assignment_id + in: query + description: Assignment ID + required: true + schema: + type: integer + - name: topic_ids + in: query + description: Topic Identifier + required: false + schema: + type: string + tags: + - ProjectTopic + responses: + '200': + description: successful delete: + summary: Delete project topics + parameters: + - name: assignment_id + in: query + description: Assignment ID + required: true + schema: + type: integer + - name: topic_ids + in: query + items: + type: string + description: Topic Identifiers to delete + required: false + schema: + type: array + tags: + - ProjectTopic + responses: + '200': + description: successful + "/submitted_content/remove_hyperlink": + post: summary: remove hyperlink (swagger) tags: - SubmittedContent @@ -2132,8 +1716,11 @@ paths: content: multipart/form-data: schema: - type: string - format: binary + type: object + properties: + uploaded_file: + type: string + format: binary required: true "/submitted_content/folder_action": post: @@ -2173,6 +1760,7 @@ paths: required: true schema: type: string + description: Folder path (use "/" for root) - name: download in: query required: true @@ -2188,6 +1776,8 @@ paths: description: cannot send whole folder '404': description: file does not exist + '200': + description: file downloaded "/submitted_content/list_files": get: summary: list files and hyperlinks @@ -2199,10 +1789,13 @@ paths: required: true schema: type: string - - name: folder[name] + - name: folder in: query schema: - type: string + type: object + properties: + name: + type: string responses: '200': description: directory listed @@ -2418,15 +2011,11 @@ paths: schema: type: object properties: - user: - type: object - description: Serialized user record - session_token: + token: type: string description: JWT session token required: - - user - - session_token + - token '404': description: no account found for the email content: @@ -2451,6 +2040,18 @@ paths: example: Invalid or expired login request required: - error + '401': + description: token verification failed + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: 'Token verification failed: invalid signature' + required: + - error requestBody: content: application/json: From c82cc5b605348d76194e231645196e1600d52631 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 14:59:11 -0400 Subject: [PATCH 14/74] regen swagger file --- swagger/v1/swagger.yaml | 691 +++++++++++++++++++++++++++++++--------- 1 file changed, 549 insertions(+), 142 deletions(-) diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index bb41395cf..4edcacbfc 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -713,6 +713,200 @@ paths: responses: '204': description: successful + "/grades/{assignment_id}/view_all_scores": + get: + summary: Retrieve all review scores for an assignment + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: assignment_id + in: path + description: ID of the assignment + required: true + schema: + type: integer + - name: participant_id + in: query + required: false + description: ID of the participant + schema: + type: integer + - name: team_id + in: query + required: false + description: ID of the team + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns team scores when team_id provided + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized + "/grades/{assignment_id}/view_our_scores": + get: + summary: Retrieve team scores for the requesting student + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: assignment_id + in: path + description: ID of the assignment + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns team scores + '403': + description: Assignment Participant not found + '401': + description: Unauthorized + "/grades/{assignment_id}/view_my_scores": + get: + summary: Retrieve individual participant scores + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: assignment_id + in: path + description: ID of the assignment + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns participant scores + '403': + description: Participant not found + '401': + description: Unauthorized + "/grades/{participant_id}/edit": + get: + summary: Get grade assignment interface data + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: participant_id + in: path + description: ID of the participant + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns participant, assignment, items, and scores + '404': + description: Participant not found + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized + "/grades/{participant_id}/assign_grade": + patch: + summary: Assign grades and comment to team + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: participant_id + in: path + description: ID of the participant + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Team grade and comment assigned successfully + '422': + description: Failed to assign team grade or comment + '404': + description: Participant not found + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized + requestBody: + content: + application/json: + schema: + type: object + properties: + grade_for_submission: + type: number + description: Grade for the submission + comment_for_submission: + type: string + description: Comment for the submission + "/grades/{participant_id}/instructor_review": + get: + summary: Begin or continue grading a submission + tags: + - Grades + security: + - bearer_auth: [] + parameters: + - name: participant_id + in: path + description: ID of the participant + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + responses: + '200': + description: Returns mapping and redirect information for existing review + '404': + description: Participant not found + '403': + description: Forbidden - Student cannot access + '401': + description: Unauthorized "/institutions": get: summary: list institutions @@ -808,6 +1002,12 @@ paths: summary: list invitations tags: - Invitations + parameters: + - name: Authorization + in: header + required: true + schema: + type: string responses: '200': description: Success @@ -815,12 +1015,18 @@ paths: summary: create invitation tags: - Invitations - parameters: [] + parameters: + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string responses: '201': description: Create successful - '422': - description: Invalid request + '404': + description: Participant not found requestBody: content: application/json: @@ -829,24 +1035,24 @@ paths: properties: assignment_id: type: integer - from_id: - type: integer - to_id: - type: integer - reply_status: + username: type: string required: - assignment_id - - from_id - - to_id + - username "/invitations/{id}": parameters: - name: id in: path - description: id of the invitation required: true schema: type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string get: summary: show invitation tags: @@ -854,8 +1060,8 @@ paths: responses: '200': description: Show invitation - '404': - description: Not found + '403': + description: Cannot see other's invitations patch: summary: update invitation tags: @@ -863,11 +1069,11 @@ paths: parameters: [] responses: '200': - description: Update successful + description: Retraction successful + '403': + description: Cannot retract other's invitations '422': description: Invalid request - '404': - description: Not found requestBody: content: application/json: @@ -876,39 +1082,87 @@ paths: properties: reply_status: type: string - required: [] delete: summary: Delete invitation tags: - Invitations + parameters: + - name: Authorization + in: header + required: true + schema: + type: string responses: - '204': + '403': + description: Student cannot delete invitations + '200': description: Delete successful - '404': - description: Not found - "/invitations/user/{user_id}/assignment/{assignment_id}": + "/invitations/sent_by/team/{team_id}": parameters: - - name: user_id + - name: team_id in: path - description: id of user required: true schema: type: integer - - name: assignment_id + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + get: + summary: Show all invitations sent by team + tags: + - Invitations + responses: + '200': + description: OK + '403': + description: Not allowed + "/invitations/sent_by/participant/{participant_id}": + parameters: + - name: participant_id in: path - description: id of assignment required: true schema: type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string get: - summary: Show all invitation for the given user and assignment + summary: Show all invitations sent by participant tags: - Invitations responses: '200': - description: Show all invitations for the user for an assignment - '404': - description: Not found + description: OK + '403': + description: Not allowed + "/invitations/sent_to/{participant_id}": + parameters: + - name: participant_id + in: path + required: true + schema: + type: integer + - name: Authorization + in: header + required: true + description: Bearer token + schema: + type: string + get: + summary: Show all invitations sent to participant + tags: + - Invitations + responses: + '200': + description: OK + '403': + description: Not allowed "/participants/user/{user_id}": get: summary: Retrieve participants for a specific user @@ -1059,6 +1313,127 @@ paths: required: - user_id - assignment_id + "/project_topics": + get: + summary: Get project topics + parameters: + - name: assignment_id + in: query + required: true + schema: + type: integer + - name: topic_identifier + in: query + required: false + schema: + type: string + tags: + - ProjectTopic + responses: + '200': + description: successful + delete: + summary: Delete project topics + parameters: + - name: assignment_id + in: query + description: Assignment ID + schema: + type: integer + - name: topic_ids + in: query + items: + type: string + description: Topic Identifiers to delete + required: false + schema: + type: array + - name: Authorization + in: header + tags: + - ProjectTopic + responses: + '422': + description: when assignment_id parameter is missing + '204': + description: when assignment_id parameter is present but topic_ids parameter + is missing + post: + summary: create a new topic in the sheet + tags: + - ProjectTopic + parameters: [] + responses: + '201': + description: when the assignment is not a microtask + '422': + description: when the assignment does not exist + requestBody: + content: + application/json: + schema: + type: object + properties: + topic_identifier: + type: string + topic_name: + type: string + max_choosers: + type: integer + category: + type: string + assignment_id: + type: integer + micropayment: + type: integer + required: + - topic_identifier + - topic_name + - max_choosers + - category + - assignment_id + - micropayment + "/project_topics/{id}": + parameters: + - name: id + in: path + description: ID of the project topic + required: true + schema: + type: integer + put: + summary: update a topic in the sheet + tags: + - ProjectTopic + parameters: [] + responses: + '200': + description: when the assignment is not a microtask + '422': + description: when the assignment does not exist + requestBody: + content: + application/json: + schema: + type: object + properties: + topic_identifier: + type: string + topic_name: + type: string + max_choosers: + type: integer + category: + type: string + assignment_id: + type: integer + micropayment: + type: integer + required: + - topic_identifier + - topic_name + - category + - assignment_id "/questionnaires": get: summary: list questionnaires @@ -1201,6 +1576,27 @@ paths: required: true schema: type: integer + - name: questionnaire1 + in: body + schema: + type: object + properties: + name: + type: string + questionnaire_type: + type: string + private: + type: boolean + min_question_score: + type: integer + max_question_score: + type: integer + instructor_id: + type: integer + required: + - name + - questionnaire_type + - instructor_id post: summary: copy questionnaire tags: @@ -1508,16 +1904,134 @@ paths: description: participant not found '401': description: unauthorized request has error response + "/student_teams/view": + get: + summary: View student team + tags: + - Student Teams + parameters: + - name: student_id + in: query + schema: + type: integer + - name: Authorization + in: header + required: true + schema: + type: string + responses: + '200': + description: Student not on any team + '403': + description: TA cannot access + "/student_teams": + post: + summary: Create team + tags: + - Student Teams + parameters: + - name: Authorization + in: header + required: true + schema: + type: string + responses: + '200': + description: Create successful + '422': + description: Duplicate name + '403': + description: Unauthorized + requestBody: + content: + application/json: + schema: + type: object + properties: + team: + type: object + properties: + name: + type: string + required: + - name + assignment_id: + type: integer + student_id: + type: integer + required: + - team + - assignment_id + - student_id + "/student_teams/update": + put: + summary: Update team name + tags: + - Student Teams + parameters: + - name: Authorization + in: header + required: true + schema: + type: string + - name: student_id + in: query + schema: + type: integer + responses: + '200': + description: Update successful + '422': + description: Duplicate name + '403': + description: Unauthorized + requestBody: + content: + application/json: + schema: + type: object + properties: + team: + type: object + properties: + name: + type: string + required: + - team + "/student_teams/leave": + put: + summary: Leave team + tags: + - Student Teams + parameters: + - name: Authorization + in: header + required: true + schema: + type: string + - name: student_id + in: query + required: true + schema: + type: integer + responses: + '200': + description: Leave successful + '403': + description: TA cannot leave "/submitted_content": get: summary: list all submission records tags: - SubmittedContent + responses: + '200': + description: successful post: summary: create a submission record tags: - SubmittedContent - parameters: [ ] + parameters: [] responses: '201': description: created @@ -1566,106 +2080,8 @@ paths: description: successful '404': description: not found - "/submitted_content/submit_hyperlink": - post: - summary: submit hyperlink (swagger) - tags: - - SubmittedContent - parameters: - - name: Authorization - in: header - schema: - type: string - - name: id - in: query - schema: - type: string - required: true - - name: submit_link - in: query - schema: - type: string - required: true - topic_identifier: - type: integer - topic_name: - type: string - max_choosers: - type: integer - category: - type: string - assignment_id: - type: integer - micropayment: - type: integer - required: - - topic_identifier - - topic_name - - max_choosers - - category - - assignment_id - - micropayment - "/project_topics/{id}": - parameters: - - name: id - in: path - description: id of the sign up topic - required: true - schema: - type: integer - put: - summary: update a new topic in the sheet - tags: - - ProjectTopic - parameters: [ ] - responses: - '200': - description: successful - "/project_topics": - get: - summary: Get project topics - parameters: - - name: assignment_id - in: query - description: Assignment ID - required: true - schema: - type: integer - - name: topic_ids - in: query - description: Topic Identifier - required: false - schema: - type: string - tags: - - ProjectTopic - responses: - '200': - description: successful - delete: - summary: Delete project topics - parameters: - - name: assignment_id - in: query - description: Assignment ID - required: true - schema: - type: integer - - name: topic_ids - in: query - items: - type: string - description: Topic Identifiers to delete - required: false - schema: - type: array - tags: - - ProjectTopic - responses: - '200': - description: successful "/submitted_content/remove_hyperlink": - post: + delete: summary: remove hyperlink (swagger) tags: - SubmittedContent @@ -1716,11 +2132,8 @@ paths: content: multipart/form-data: schema: - type: object - properties: - uploaded_file: - type: string - format: binary + type: string + format: binary required: true "/submitted_content/folder_action": post: @@ -1760,7 +2173,6 @@ paths: required: true schema: type: string - description: Folder path (use "/" for root) - name: download in: query required: true @@ -1776,8 +2188,6 @@ paths: description: cannot send whole folder '404': description: file does not exist - '200': - description: file downloaded "/submitted_content/list_files": get: summary: list files and hyperlinks @@ -1789,13 +2199,10 @@ paths: required: true schema: type: string - - name: folder + - name: folder[name] in: query schema: - type: object - properties: - name: - type: string + type: string responses: '200': description: directory listed From e9864835557a5fad8d3023d3ecd0d3fdd729596a Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 16:58:06 -0400 Subject: [PATCH 15/74] move logic into oidc_request model, add tests --- app/controllers/oidc_login_controller.rb | 81 +----- app/models/auth_request.rb | 2 - app/models/oidc_config.rb | 42 +++- app/models/oidc_request.rb | 69 ++++++ config/oidc_providers.yml | 3 +- ...0_rename_auth_requests_to_oidc_requests.rb | 17 ++ db/schema.rb | 22 +- spec/models/auth_request_spec.rb | 5 - spec/models/oidc_config_spec.rb | 231 ++++++++++++++++++ spec/models/oidc_request_spec.rb | 149 +++++++++++ spec/requests/oidc_login_spec.rb | 32 ++- 11 files changed, 552 insertions(+), 101 deletions(-) delete mode 100644 app/models/auth_request.rb create mode 100644 app/models/oidc_request.rb create mode 100644 db/migrate/20260411120000_rename_auth_requests_to_oidc_requests.rb delete mode 100644 spec/models/auth_request_spec.rb create mode 100644 spec/models/oidc_config_spec.rb create mode 100644 spec/models/oidc_request_spec.rb diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 39df27413..cb2daad17 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -1,39 +1,21 @@ class OidcLoginController < ApplicationController skip_before_action :authenticate_request! + rescue_from OidcConfig::ProviderNotFound do |e| + render json: { error: e.message }, status: :not_found + end + # GET /auth/providers def providers - render json: OidcConfig.public_list + render json: OidcConfig.public_list.to_json end # POST /auth/client-select def client_select provider = OidcConfig.find(params[:provider]) - client = build_client(provider) - - # Generate state, nonce, and PKCE - state = SecureRandom.hex(32) - nonce = SecureRandom.hex(32) - code_verifier = SecureRandom.urlsafe_base64(64) - code_challenge = Base64.urlsafe_encode64( - Digest::SHA256.digest(code_verifier), padding: false - ) - - # Store in DB for callback verification - AuthRequest.create!( - state: state, - nonce: nonce, - code_verifier: code_verifier, - provider: params[:provider] - ) - - # Build the authorization URL - authorization_uri = client.authorization_uri( - scope: [:openid, :email, :profile], - state: state, - nonce: nonce, - code_challenge: code_challenge, - code_challenge_method: "S256" + authorization_uri = OidcRequest.authorization_uri_for!( + provider_key: params[:provider], + provider: provider ) render json: { redirect_uri: authorization_uri } @@ -42,34 +24,12 @@ def client_select # POST /auth/callback def callback # Look up and consume the auth request - auth_request = AuthRequest - .where("created_at > ?", 5.minutes.ago) - .find_by!(state: params[:state]) - auth_request.destroy! + auth_request = OidcRequest.consume_recent_by_state!(params[:state]) provider = OidcConfig.find(auth_request.provider) - client = build_client(provider) - - # Exchange authorization code for tokens - client.authorization_code = params[:code] - access_token = client.access_token!( - code_verifier: auth_request.code_verifier - ) - - # Decode and verify the ID token - discovery = discover(provider) - id_token = OpenIDConnect::ResponseObject::IdToken.decode( - access_token.id_token, - discovery.jwks - ) - id_token.verify!( - issuer: discovery.issuer, - client_id: provider["client_id"], - nonce: auth_request.nonce - ) # Match to existing user by email - email = id_token.raw_attributes["email"] + email = auth_request.verified_email_from_code!(provider: provider, code: params[:code]) user = User.find_by(email: email) if user @@ -84,25 +44,4 @@ def callback rescue OpenIDConnect::ResponseObject::IdToken::InvalidToken => e render json: { error: "Token verification failed: #{e.message}" }, status: :unauthorized end - - private - - def build_client(provider) - discovery = discover(provider) - OpenIDConnect::Client.new( - identifier: provider["client_id"], - secret: provider["client_secret"], - redirect_uri: provider["redirect_uri"], - authorization_endpoint: discovery.authorization_endpoint, - token_endpoint: discovery.token_endpoint, - userinfo_endpoint: discovery.userinfo_endpoint - ) - end - - def discover(provider) - # Avoid duplicate discovery calls within the same request - @discoveries ||= {} - @discoveries[provider["issuer"]] ||= - OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) - end end diff --git a/app/models/auth_request.rb b/app/models/auth_request.rb deleted file mode 100644 index e2029e943..000000000 --- a/app/models/auth_request.rb +++ /dev/null @@ -1,2 +0,0 @@ -class AuthRequest < ApplicationRecord -end diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index 87c77fabb..cfc08fa6e 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -2,13 +2,18 @@ class OidcConfig class ProviderNotFound < StandardError; end CONFIG_FILE = Rails.root.join("config", "oidc_providers.yml").freeze + DEFAULT_SCOPES = %i[openid email profile].freeze def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) - providers_config = parsed&.fetch("providers", {}) - validate!(providers_config || {}) + parsed = {} unless parsed.is_a?(Hash) + providers_config = parsed["providers"] + validate!(providers_config.is_a?(Hash) ? providers_config : {}) + rescue Errno::ENOENT, Psych::SyntaxError => e + Rails.logger.error("OIDC config load failed: #{e.message}") + {}.freeze end end @@ -26,22 +31,47 @@ def self.reload! @providers = nil end + def self.scopes_for(provider) + raw_scopes = provider["scopes"] + + scopes = case raw_scopes + when String + raw_scopes.split(/[\s,]+/).reject(&:blank?) + when Array + raw_scopes.map(&:to_s).reject(&:blank?) + else + [] + end + + scopes = DEFAULT_SCOPES if scopes.blank? + scopes.map(&:to_sym) + end + private - REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri scopes].freeze + REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri].freeze def self.validate!(providers) - return {} if providers.blank? + return {}.freeze if providers.blank? + + valid_providers = {} providers.each do |key, cfg| + unless cfg.is_a?(Hash) + Rails.logger.warn("OIDC provider '#{key}' skipped: invalid config format") + next + end + missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}") - providers.delete(key) next end + + valid_providers[key] = cfg.deep_dup.freeze end - providers + + valid_providers.freeze end def self.provider_summary(key, cfg) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb new file mode 100644 index 000000000..b2ad0b4b3 --- /dev/null +++ b/app/models/oidc_request.rb @@ -0,0 +1,69 @@ +class OidcRequest < ApplicationRecord + scope :recent, ->(window = 5.minutes) { where("created_at > ?", window.ago) } + + def self.consume_recent_by_state!(state, window: 5.minutes) + transaction do + request = recent(window).lock.find_by!(state: state) + request.destroy! + request + end + end + + def self.authorization_uri_for!(provider_key:, provider:) + discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) + client = new_client(provider, discovery: discovery) + + state = SecureRandom.hex(32) + nonce = SecureRandom.hex(32) + code_verifier = SecureRandom.urlsafe_base64(64) + code_challenge = Base64.urlsafe_encode64( + Digest::SHA256.digest(code_verifier), padding: false + ) + + create!( + state: state, + nonce: nonce, + code_verifier: code_verifier, + provider: provider_key + ) + + client.authorization_uri( + scope: OidcConfig.scopes_for(provider), + state: state, + nonce: nonce, + code_challenge: code_challenge, + code_challenge_method: "S256" + ) + end + + def verified_email_from_code!(provider:, code:) + discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) + client = self.class.new_client(provider, discovery: discovery) + + client.authorization_code = code + access_token = client.access_token!(code_verifier: code_verifier) + + id_token = OpenIDConnect::ResponseObject::IdToken.decode( + access_token.id_token, + discovery.jwks + ) + id_token.verify!( + issuer: discovery.issuer, + client_id: provider["client_id"], + nonce: nonce + ) + + id_token.raw_attributes["email"] + end + + def self.new_client(provider, discovery:) + OpenIDConnect::Client.new( + identifier: provider["client_id"], + secret: provider["client_secret"], + redirect_uri: provider["redirect_uri"], + authorization_endpoint: discovery.authorization_endpoint, + token_endpoint: discovery.token_endpoint, + userinfo_endpoint: discovery.userinfo_endpoint + ) + end +end \ No newline at end of file diff --git a/config/oidc_providers.yml b/config/oidc_providers.yml index 0901c9403..12f3d5af0 100644 --- a/config/oidc_providers.yml +++ b/config/oidc_providers.yml @@ -4,8 +4,7 @@ providers: issuer: https://accounts.google.com client_id: <%= ENV['GOOG_CLIENT_ID'] %> client_secret: <%= ENV['GOOG_CLIENT_SECRET'] %> - redirect_uri: http://localhost:3000/auth/callback - scopes: openid email profile + redirect_uri: <%= ENV['GOOG_REDIRECT_URI'] %> # Add more providers here: # okta: diff --git a/db/migrate/20260411120000_rename_auth_requests_to_oidc_requests.rb b/db/migrate/20260411120000_rename_auth_requests_to_oidc_requests.rb new file mode 100644 index 000000000..7db5993a0 --- /dev/null +++ b/db/migrate/20260411120000_rename_auth_requests_to_oidc_requests.rb @@ -0,0 +1,17 @@ +class RenameAuthRequestsToOidcRequests < ActiveRecord::Migration[8.0] + def up + rename_table :auth_requests, :oidc_requests + + if index_name_exists?(:oidc_requests, 'index_auth_requests_on_state') + rename_index :oidc_requests, 'index_auth_requests_on_state', 'index_oidc_requests_on_state' + end + end + + def down + rename_table :oidc_requests, :auth_requests + + if index_name_exists?(:auth_requests, 'index_oidc_requests_on_state') + rename_index :auth_requests, 'index_oidc_requests_on_state', 'index_auth_requests_on_state' + end + end +end diff --git a/db/schema.rb b/db/schema.rb index 58019229a..cc725f68b 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[8.0].define(version: 2026_04_07_003623) do +ActiveRecord::Schema[8.0].define(version: 2026_04_11_120000) do create_table "account_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "username" t.string "full_name" @@ -117,16 +117,6 @@ t.index ["duty_id"], name: "index_assignments_duties_on_duty_id" end - create_table "auth_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| - t.string "state" - t.string "nonce" - t.string "code_verifier" - t.string "provider" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["state"], name: "index_auth_requests_on_state" - end - create_table "bookmark_ratings", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.integer "bookmark_id" t.integer "user_id" @@ -250,6 +240,16 @@ t.datetime "updated_at", null: false end + create_table "oidc_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| + t.string "state" + t.string "nonce" + t.string "code_verifier" + t.string "provider" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["state"], name: "index_oidc_requests_on_state" + end + create_table "participants", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.bigint "user_id" t.datetime "created_at", null: false diff --git a/spec/models/auth_request_spec.rb b/spec/models/auth_request_spec.rb deleted file mode 100644 index 071de2242..000000000 --- a/spec/models/auth_request_spec.rb +++ /dev/null @@ -1,5 +0,0 @@ -require 'rails_helper' - -RSpec.describe AuthRequest, type: :model do - pending "add some examples to (or delete) #{__FILE__}" -end diff --git a/spec/models/oidc_config_spec.rb b/spec/models/oidc_config_spec.rb new file mode 100644 index 000000000..aaed789d9 --- /dev/null +++ b/spec/models/oidc_config_spec.rb @@ -0,0 +1,231 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe OidcConfig, type: :model do + before do + described_class.reload! + allow(Rails.logger).to receive(:warn) + allow(Rails.logger).to receive(:error) + end + + after do + described_class.reload! + end + + describe '.providers' do + it 'loads providers from YAML and evaluates ERB env vars' do + original_client_id = ENV['GOOG_CLIENT_ID'] + original_client_secret = ENV['GOOG_CLIENT_SECRET'] + begin + ENV['GOOG_CLIENT_ID'] = 'client-id-from-env' + ENV['GOOG_CLIENT_SECRET'] = 'client-secret-from-env' + + yaml = <<~YAML + providers: + google-ncsu: + display_name: Google NCSU + issuer: https://accounts.google.com + client_id: <%= ENV['GOOG_CLIENT_ID'] %> + client_secret: <%= ENV['GOOG_CLIENT_SECRET'] %> + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + YAML + + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(yaml) + + providers = described_class.providers + + expect(providers.keys).to eq(['google-ncsu']) + expect(providers['google-ncsu']['client_id']).to eq('client-id-from-env') + expect(providers['google-ncsu']['client_secret']).to eq('client-secret-from-env') + expect(providers).to be_frozen + expect(providers['google-ncsu']).to be_frozen + ensure + ENV['GOOG_CLIENT_ID'] = original_client_id + ENV['GOOG_CLIENT_SECRET'] = original_client_secret + end + end + + it 'memoizes results until reload! is called' do + first_yaml = <<~YAML + providers: + first: + display_name: First + issuer: https://issuer.example.com + client_id: id-1 + client_secret: secret-1 + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + YAML + + second_yaml = <<~YAML + providers: + second: + display_name: Second + issuer: https://issuer-2.example.com + client_id: id-2 + client_secret: secret-2 + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + YAML + + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(first_yaml) + + expect(described_class.providers.keys).to eq(['first']) + + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(second_yaml) + expect(described_class.providers.keys).to eq(['first']) + + described_class.reload! + expect(described_class.providers.keys).to eq(['second']) + end + + it 'skips invalid providers and warns for missing keys or invalid formats' do + yaml = <<~YAML + providers: + valid: + display_name: Valid + issuer: https://issuer.example.com + client_id: id + client_secret: secret + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + no_scopes: + display_name: No Scopes + issuer: https://issuer.example.com + client_id: id + client_secret: secret + redirect_uri: http://localhost:3000/auth/callback + missing_secret: + display_name: Missing Secret + issuer: https://issuer.example.com + client_id: id + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + malformed: hello + YAML + + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(yaml) + + providers = described_class.providers + + expect(providers.keys).to contain_exactly('valid', 'no_scopes') + expect(Rails.logger).to have_received(:warn).with(/missing_secret.*missing client_secret/) + expect(Rails.logger).to have_received(:warn).with(/malformed.*invalid config format/) + end + + it 'returns an empty hash when YAML has no providers map' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return('[]') + + expect(described_class.providers).to eq({}) + expect(described_class.providers).to be_frozen + end + + it 'supports YAML aliases in provider definitions' do + yaml = <<~YAML + providers: + google: + &base + display_name: Google + issuer: https://accounts.google.com + client_id: id + client_secret: secret + redirect_uri: http://localhost:3000/auth/callback + scopes: openid email profile + google-copy: + <<: *base + display_name: Google Copy + YAML + + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(yaml) + + providers = described_class.providers + + expect(providers.keys).to contain_exactly('google', 'google-copy') + expect(providers['google-copy']['issuer']).to eq('https://accounts.google.com') + end + + it 'returns an empty hash and logs an error for malformed YAML' do + malformed_yaml = "providers:\n bad: [unterminated" + + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(malformed_yaml) + + expect(described_class.providers).to eq({}) + expect(Rails.logger).to have_received(:error).with(/OIDC config load failed:/) + end + end + + describe '.find' do + it 'returns a provider config by key' do + allow(described_class).to receive(:providers).and_return( + 'google-ncsu' => { + 'display_name' => 'Google NCSU', + 'issuer' => 'https://accounts.google.com', + 'client_id' => 'id', + 'client_secret' => 'secret', + 'redirect_uri' => 'http://localhost:3000/auth/callback', + 'scopes' => 'openid email profile' + } + ) + + expect(described_class.find('google-ncsu')['display_name']).to eq('Google NCSU') + end + + it 'raises ProviderNotFound for unknown provider keys' do + allow(described_class).to receive(:providers).and_return({}) + + expect { described_class.find('unknown') } + .to raise_error(OidcConfig::ProviderNotFound, 'Unknown OIDC provider: unknown') + end + end + + describe '.public_list' do + it 'returns only id and name for each provider' do + allow(described_class).to receive(:providers).and_return( + 'google-ncsu' => { + 'display_name' => 'Google NCSU', + 'issuer' => 'https://accounts.google.com', + 'client_id' => 'id', + 'client_secret' => 'secret', + 'redirect_uri' => 'http://localhost:3000/auth/callback', + 'scopes' => 'openid email profile' + } + ) + + expect(described_class.public_list).to eq([{ id: 'google-ncsu', name: 'Google NCSU' }]) + end + end + + describe '.scopes_for' do + it 'parses whitespace-delimited scope strings' do + provider = { 'scopes' => 'openid email profile custom' } + + expect(described_class.scopes_for(provider)).to eq(%i[openid email profile custom]) + end + + it 'parses comma-delimited scope strings' do + provider = { 'scopes' => 'openid,email,profile' } + + expect(described_class.scopes_for(provider)).to eq(%i[openid email profile]) + end + + it 'parses array scopes and stringifies symbols' do + provider = { 'scopes' => [:openid, 'email', 'profile'] } + + expect(described_class.scopes_for(provider)).to eq(%i[openid email profile]) + end + + it 'falls back to default scopes when missing' do + provider = { 'scopes' => nil } + + expect(described_class.scopes_for(provider)).to eq(%i[openid email profile]) + end + end +end diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb new file mode 100644 index 000000000..8373f2399 --- /dev/null +++ b/spec/models/oidc_request_spec.rb @@ -0,0 +1,149 @@ +require 'rails_helper' + +RSpec.describe OidcRequest, type: :model do + let(:provider) do + { + 'display_name' => 'Google NCSU', + 'issuer' => 'https://accounts.google.com', + 'client_id' => 'test-client-id', + 'client_secret' => 'test-client-secret', + 'redirect_uri' => 'http://localhost:3000/auth/callback', + 'scopes' => 'openid email profile' + } + end + + let(:discovery) do + instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: 'https://accounts.google.com/o/oauth2/v2/auth', + token_endpoint: 'https://oauth2.googleapis.com/token', + userinfo_endpoint: 'https://openidconnect.googleapis.com/v1/userinfo', + issuer: 'https://accounts.google.com', + jwks: instance_double(JSON::JWK::Set) + ) + end + + let(:client) { instance_double(OpenIDConnect::Client) } + + before do + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with('https://accounts.google.com').and_return(discovery) + allow(OpenIDConnect::Client).to receive(:new).and_return(client) + end + + describe '.consume_recent_by_state!' do + let!(:recent_request) do + OidcRequest.create!( + state: 'recent-state', + nonce: 'nonce', + code_verifier: 'verifier', + provider: 'google-ncsu' + ) + end + + it 'returns and destroys a recent request matching state' do + consumed = described_class.consume_recent_by_state!('recent-state') + + expect(consumed.id).to eq(recent_request.id) + expect(described_class.find_by(id: recent_request.id)).to be_nil + end + + it 'raises RecordNotFound for unknown state' do + expect { described_class.consume_recent_by_state!('missing-state') } + .to raise_error(ActiveRecord::RecordNotFound) + end + + it 'raises RecordNotFound for expired requests' do + recent_request.update_columns(created_at: 10.minutes.ago) + + expect { described_class.consume_recent_by_state!('recent-state') } + .to raise_error(ActiveRecord::RecordNotFound) + expect(described_class.find_by(id: recent_request.id)).to be_present + end + + it 'supports a custom recency window' do + recent_request.update_columns(created_at: 10.minutes.ago) + + consumed = described_class.consume_recent_by_state!('recent-state', window: 15.minutes) + + expect(consumed.id).to eq(recent_request.id) + expect(described_class.find_by(id: recent_request.id)).to be_nil + end + end + + describe '.authorization_uri_for!' do + it 'creates auth request and returns provider authorization URI' do + allow(client).to receive(:authorization_uri) + .with(hash_including(scope: %i[openid email profile], code_challenge_method: 'S256')) + .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') + + expect do + uri = described_class.authorization_uri_for!(provider_key: 'google-ncsu', provider: provider) + expect(uri).to include('scope=openid+email+profile') + end.to change(described_class, :count).by(1) + end + + it 'uses default scopes when provider scopes are missing' do + allow(client).to receive(:authorization_uri) + .with(hash_including(scope: %i[openid email profile], code_challenge_method: 'S256')) + .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') + + described_class.authorization_uri_for!( + provider_key: 'google-ncsu', + provider: provider.merge('scopes' => nil) + ) + end + end + + describe '#verified_email_from_code!' do + let(:oidc_request) do + described_class.create!( + state: 'state', + nonce: 'nonce', + code_verifier: 'verifier', + provider: 'google-ncsu' + ) + end + + let(:id_token_obj) do + instance_double( + OpenIDConnect::ResponseObject::IdToken, + raw_attributes: { 'email' => 'oidcuser@ncsu.edu' } + ) + end + + it 'exchanges code, verifies token and returns email' do + allow(client).to receive(:authorization_code=).with('authorization-code') + allow(client).to receive(:access_token!).with(code_verifier: 'verifier') + .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) + + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .with('fake.id.token', discovery.jwks) + .and_return(id_token_obj) + allow(id_token_obj).to receive(:verify!).with( + issuer: 'https://accounts.google.com', + client_id: 'test-client-id', + nonce: 'nonce' + ) + + email = oidc_request.verified_email_from_code!(provider: provider, code: 'authorization-code') + expect(email).to eq('oidcuser@ncsu.edu') + end + end + + describe '.new_client' do + it 'builds an OpenIDConnect::Client with provider credentials and discovery endpoints' do + expect(OpenIDConnect::Client).to receive(:new).with( + identifier: 'test-client-id', + secret: 'test-client-secret', + redirect_uri: 'http://localhost:3000/auth/callback', + authorization_endpoint: 'https://accounts.google.com/o/oauth2/v2/auth', + token_endpoint: 'https://oauth2.googleapis.com/token', + userinfo_endpoint: 'https://openidconnect.googleapis.com/v1/userinfo' + ).and_return(client) + + result = described_class.new_client(provider, discovery: discovery) + expect(result).to eq(client) + end + end +end \ No newline at end of file diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 7df8b9cda..1b54f3794 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -54,7 +54,7 @@ security [] description <<~DESC Accepts a provider key, performs OIDC discovery, generates PKCE and state parameters, - persists an AuthRequest for later verification, and returns the provider's authorization URL + persists an OidcRequest for later verification, and returns the provider's authorization URL that the front end should redirect the user to. DESC @@ -94,6 +94,10 @@ ) allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) .with("https://accounts.google.com").and_return(discovery) + + allow_any_instance_of(OpenIDConnect::Client).to receive(:authorization_uri) + .with(hash_including(scope: %i[openid email profile])) + .and_return("https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile") end run_test! do |response| @@ -101,6 +105,26 @@ expect(json["redirect_uri"]).to be_present end end + + response '404', 'unknown OIDC provider' do + schema type: :object, + properties: { + error: { type: :string, example: 'Unknown OIDC provider: nonexistent' } + }, + required: %w[error] + + let(:body) { { provider: "nonexistent" } } + + before do + allow(OidcConfig).to receive(:find).with("nonexistent") + .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: nonexistent") + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/Unknown OIDC provider/) + end + end end end @@ -148,7 +172,7 @@ end let(:auth_request) do - AuthRequest.create!( + OidcRequest.create!( state: "valid-state-hex", nonce: "valid-nonce-hex", code_verifier: "valid-code-verifier", @@ -213,7 +237,7 @@ required: %w[error] let(:auth_request) do - AuthRequest.create!( + OidcRequest.create!( state: "state-no-user", nonce: "nonce-no-user", code_verifier: "verifier-no-user", @@ -292,7 +316,7 @@ required: %w[error] let(:auth_request) do - AuthRequest.create!( + OidcRequest.create!( state: "state-bad-token", nonce: "nonce-bad-token", code_verifier: "verifier-bad-token", From e96398284edd4a708a70f5b07c623757751a3d45 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 17:16:29 -0400 Subject: [PATCH 16/74] refactor and simplify --- app/controllers/oidc_login_controller.rb | 6 +-- app/models/oidc_config.rb | 48 ++++-------------------- app/models/oidc_request.rb | 3 +- spec/models/oidc_request_spec.rb | 13 ++++--- spec/requests/oidc_login_spec.rb | 23 +----------- 5 files changed, 21 insertions(+), 72 deletions(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index cb2daad17..a38ad36da 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -12,11 +12,7 @@ def providers # POST /auth/client-select def client_select - provider = OidcConfig.find(params[:provider]) - authorization_uri = OidcRequest.authorization_uri_for!( - provider_key: params[:provider], - provider: provider - ) + authorization_uri = OidcRequest.authorization_uri_for!(provider_key: params[:provider]) render json: { redirect_uri: authorization_uri } end diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index cfc08fa6e..599fa0f7e 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -2,18 +2,13 @@ class OidcConfig class ProviderNotFound < StandardError; end CONFIG_FILE = Rails.root.join("config", "oidc_providers.yml").freeze - DEFAULT_SCOPES = %i[openid email profile].freeze + REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri].freeze def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) - parsed = {} unless parsed.is_a?(Hash) - providers_config = parsed["providers"] - validate!(providers_config.is_a?(Hash) ? providers_config : {}) - rescue Errno::ENOENT, Psych::SyntaxError => e - Rails.logger.error("OIDC config load failed: #{e.message}") - {}.freeze + validate!(parsed.fetch("providers", {})) end end @@ -24,7 +19,7 @@ def self.find(provider_key) end def self.public_list - providers.map { |key, cfg| provider_summary(key, cfg) } + providers.map { |key, cfg| { id: key, name: cfg["display_name"] } } end def self.reload! @@ -32,49 +27,22 @@ def self.reload! end def self.scopes_for(provider) - raw_scopes = provider["scopes"] - - scopes = case raw_scopes - when String - raw_scopes.split(/[\s,]+/).reject(&:blank?) - when Array - raw_scopes.map(&:to_s).reject(&:blank?) - else - [] - end - - scopes = DEFAULT_SCOPES if scopes.blank? - scopes.map(&:to_sym) + raw = provider["scopes"].to_s.split(/[\s,]+/).reject(&:blank?) + raw.presence || %w[openid email profile] end private - REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri].freeze - def self.validate!(providers) - return {}.freeze if providers.blank? - - valid_providers = {} - providers.each do |key, cfg| - unless cfg.is_a?(Hash) - Rails.logger.warn("OIDC provider '#{key}' skipped: invalid config format") - next - end - missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}") - next + providers.delete(key) end - - valid_providers[key] = cfg.deep_dup.freeze end - - valid_providers.freeze + providers end - def self.provider_summary(key, cfg) - { id: key, name: cfg["display_name"] } - end + private_class_method :validate! end \ No newline at end of file diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index b2ad0b4b3..1c3d52ee7 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -9,7 +9,8 @@ def self.consume_recent_by_state!(state, window: 5.minutes) end end - def self.authorization_uri_for!(provider_key:, provider:) + def self.authorization_uri_for!(provider_key:) + provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) client = new_client(provider, discovery: discovery) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 8373f2399..a3c7116b7 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -72,26 +72,29 @@ end describe '.authorization_uri_for!' do + before do + allow(OidcConfig).to receive(:find).with('google-ncsu').and_return(provider) + end + it 'creates auth request and returns provider authorization URI' do allow(client).to receive(:authorization_uri) .with(hash_including(scope: %i[openid email profile], code_challenge_method: 'S256')) .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') expect do - uri = described_class.authorization_uri_for!(provider_key: 'google-ncsu', provider: provider) + uri = described_class.authorization_uri_for!(provider_key: 'google-ncsu') expect(uri).to include('scope=openid+email+profile') end.to change(described_class, :count).by(1) end it 'uses default scopes when provider scopes are missing' do + allow(OidcConfig).to receive(:find).with('google-ncsu') + .and_return(provider.merge('scopes' => nil)) allow(client).to receive(:authorization_uri) .with(hash_including(scope: %i[openid email profile], code_challenge_method: 'S256')) .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') - described_class.authorization_uri_for!( - provider_key: 'google-ncsu', - provider: provider.merge('scopes' => nil) - ) + described_class.authorization_uri_for!(provider_key: 'google-ncsu') end end diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 1b54f3794..cbae322e0 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -76,27 +76,8 @@ let(:body) { { provider: "google-ncsu" } } before do - provider_cfg = { - "display_name" => "Google NCSU", - "issuer" => "https://accounts.google.com", - "client_id" => "test-client-id", - "client_secret" => "test-client-secret", - "redirect_uri" => "http://localhost:3000/auth/callback", - "scopes" => "openid email profile" - } - allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) - - discovery = instance_double( - OpenIDConnect::Discovery::Provider::Config::Response, - authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", - token_endpoint: "https://oauth2.googleapis.com/token", - userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo" - ) - allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .with("https://accounts.google.com").and_return(discovery) - - allow_any_instance_of(OpenIDConnect::Client).to receive(:authorization_uri) - .with(hash_including(scope: %i[openid email profile])) + allow(OidcRequest).to receive(:authorization_uri_for!) + .with(provider_key: "google-ncsu") .and_return("https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile") end From f6bb663d3b5ee0b3ae11765a45e49b1974e83953 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 17:39:54 -0400 Subject: [PATCH 17/74] add clarifying details to config --- config/oidc_providers.yml | 29 ++++++++++++++++++++--------- 1 file changed, 20 insertions(+), 9 deletions(-) diff --git a/config/oidc_providers.yml b/config/oidc_providers.yml index 12f3d5af0..e3d0244a5 100644 --- a/config/oidc_providers.yml +++ b/config/oidc_providers.yml @@ -6,12 +6,23 @@ providers: client_secret: <%= ENV['GOOG_CLIENT_SECRET'] %> redirect_uri: <%= ENV['GOOG_REDIRECT_URI'] %> -# Add more providers here: - # okta: - # display_name: Okta - # icon: okta - # issuer: https://.okta.com - # client_id: <%= ENV['OKTA_OIDC_CLIENT_ID'] %> - # client_secret: <%= ENV['OKTA_OIDC_CLIENT_SECRET'] %> - # redirect_uri: <%= ENV['OKTA_OIDC_REDIRECT_URI'] %> - # scopes: openid email profile \ No newline at end of file +# Add more providers here (values below are examples, not real credentials): + +# Provider key used to fetch config +# okta: +# Name shown to users in the login dropdown +# display_name: Okta + +# OIDC issuer URL, used to fetch .well-known/openid-configuration +# issuer: https://dev-123456.okta.com + +# OAuth client credentials, register with the provider to obtain these +# client_id: 0oa1b2c3d4e5f6g7h8i9 +# client_secret: AbCdEfGhIjKlMnOpQrStUvWxYz0123456789 + +# Where the IdP redirects after authentication, must match provider registration +# This should probably point to the frontend callback endpoint +# redirect_uri: http://localhost:3000/auth/callback + +# Space-separated OIDC scopes, defaults to "openid email profile" if omitted +# scopes: openid email profile \ No newline at end of file From 83d9dda084700b6a448d5847635a132480a1ce6e Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 17:48:55 -0400 Subject: [PATCH 18/74] update tests to reflect simplified config --- spec/models/oidc_config_spec.rb | 48 +++++++++------------------------ 1 file changed, 13 insertions(+), 35 deletions(-) diff --git a/spec/models/oidc_config_spec.rb b/spec/models/oidc_config_spec.rb index aaed789d9..47f3f34ec 100644 --- a/spec/models/oidc_config_spec.rb +++ b/spec/models/oidc_config_spec.rb @@ -6,7 +6,6 @@ before do described_class.reload! allow(Rails.logger).to receive(:warn) - allow(Rails.logger).to receive(:error) end after do @@ -40,8 +39,6 @@ expect(providers.keys).to eq(['google-ncsu']) expect(providers['google-ncsu']['client_id']).to eq('client-id-from-env') expect(providers['google-ncsu']['client_secret']).to eq('client-secret-from-env') - expect(providers).to be_frozen - expect(providers['google-ncsu']).to be_frozen ensure ENV['GOOG_CLIENT_ID'] = original_client_id ENV['GOOG_CLIENT_SECRET'] = original_client_secret @@ -57,7 +54,6 @@ client_id: id-1 client_secret: secret-1 redirect_uri: http://localhost:3000/auth/callback - scopes: openid email profile YAML second_yaml = <<~YAML @@ -68,7 +64,6 @@ client_id: id-2 client_secret: secret-2 redirect_uri: http://localhost:3000/auth/callback - scopes: openid email profile YAML allow(File).to receive(:read).and_call_original @@ -83,7 +78,7 @@ expect(described_class.providers.keys).to eq(['second']) end - it 'skips invalid providers and warns for missing keys or invalid formats' do + it 'skips providers missing required keys and warns' do yaml = <<~YAML providers: valid: @@ -104,8 +99,6 @@ issuer: https://issuer.example.com client_id: id redirect_uri: http://localhost:3000/auth/callback - scopes: openid email profile - malformed: hello YAML allow(File).to receive(:read).and_call_original @@ -115,15 +108,13 @@ expect(providers.keys).to contain_exactly('valid', 'no_scopes') expect(Rails.logger).to have_received(:warn).with(/missing_secret.*missing client_secret/) - expect(Rails.logger).to have_received(:warn).with(/malformed.*invalid config format/) end - it 'returns an empty hash when YAML has no providers map' do + it 'returns an empty hash when no providers key exists' do allow(File).to receive(:read).and_call_original - allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return('[]') + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return('{}') expect(described_class.providers).to eq({}) - expect(described_class.providers).to be_frozen end it 'supports YAML aliases in provider definitions' do @@ -136,7 +127,6 @@ client_id: id client_secret: secret redirect_uri: http://localhost:3000/auth/callback - scopes: openid email profile google-copy: <<: *base display_name: Google Copy @@ -150,16 +140,6 @@ expect(providers.keys).to contain_exactly('google', 'google-copy') expect(providers['google-copy']['issuer']).to eq('https://accounts.google.com') end - - it 'returns an empty hash and logs an error for malformed YAML' do - malformed_yaml = "providers:\n bad: [unterminated" - - allow(File).to receive(:read).and_call_original - allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(malformed_yaml) - - expect(described_class.providers).to eq({}) - expect(Rails.logger).to have_received(:error).with(/OIDC config load failed:/) - end end describe '.find' do @@ -170,8 +150,7 @@ 'issuer' => 'https://accounts.google.com', 'client_id' => 'id', 'client_secret' => 'secret', - 'redirect_uri' => 'http://localhost:3000/auth/callback', - 'scopes' => 'openid email profile' + 'redirect_uri' => 'http://localhost:3000/auth/callback' } ) @@ -194,8 +173,7 @@ 'issuer' => 'https://accounts.google.com', 'client_id' => 'id', 'client_secret' => 'secret', - 'redirect_uri' => 'http://localhost:3000/auth/callback', - 'scopes' => 'openid email profile' + 'redirect_uri' => 'http://localhost:3000/auth/callback' } ) @@ -207,25 +185,25 @@ it 'parses whitespace-delimited scope strings' do provider = { 'scopes' => 'openid email profile custom' } - expect(described_class.scopes_for(provider)).to eq(%i[openid email profile custom]) + expect(described_class.scopes_for(provider)).to eq(%w[openid email profile custom]) end it 'parses comma-delimited scope strings' do provider = { 'scopes' => 'openid,email,profile' } - expect(described_class.scopes_for(provider)).to eq(%i[openid email profile]) + expect(described_class.scopes_for(provider)).to eq(%w[openid email profile]) end - it 'parses array scopes and stringifies symbols' do - provider = { 'scopes' => [:openid, 'email', 'profile'] } + it 'falls back to default scopes when scopes is nil' do + provider = { 'scopes' => nil } - expect(described_class.scopes_for(provider)).to eq(%i[openid email profile]) + expect(described_class.scopes_for(provider)).to eq(%w[openid email profile]) end - it 'falls back to default scopes when missing' do - provider = { 'scopes' => nil } + it 'falls back to default scopes when scopes key is absent' do + provider = {} - expect(described_class.scopes_for(provider)).to eq(%i[openid email profile]) + expect(described_class.scopes_for(provider)).to eq(%w[openid email profile]) end end end From e16eb99d2bf15a0b41b64f09e7e6c8821e86f867 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Sat, 11 Apr 2026 17:57:26 -0400 Subject: [PATCH 19/74] Update app/controllers/oidc_login_controller.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/controllers/oidc_login_controller.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index a38ad36da..fdb83fa97 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -7,7 +7,7 @@ class OidcLoginController < ApplicationController # GET /auth/providers def providers - render json: OidcConfig.public_list.to_json + render json: OidcConfig.public_list end # POST /auth/client-select From 69a24f743747eb327727e9ff55589554c923b2dd Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 17:59:45 -0400 Subject: [PATCH 20/74] scopes are strings --- spec/models/oidc_request_spec.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index a3c7116b7..09dda1d54 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -78,7 +78,7 @@ it 'creates auth request and returns provider authorization URI' do allow(client).to receive(:authorization_uri) - .with(hash_including(scope: %i[openid email profile], code_challenge_method: 'S256')) + .with(hash_including(scope: %w[openid email profile], code_challenge_method: 'S256')) .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') expect do @@ -91,7 +91,7 @@ allow(OidcConfig).to receive(:find).with('google-ncsu') .and_return(provider.merge('scopes' => nil)) allow(client).to receive(:authorization_uri) - .with(hash_including(scope: %i[openid email profile], code_challenge_method: 'S256')) + .with(hash_including(scope: %w[openid email profile], code_challenge_method: 'S256')) .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') described_class.authorization_uri_for!(provider_key: 'google-ncsu') From 3888048ba5c7fe04491d8e7d996c8a968a305f50 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Sat, 11 Apr 2026 22:04:11 +0000 Subject: [PATCH 21/74] fix validate! hash-during-iteration bug and add nil guards for YAML parsing Agent-Logs-Url: https://github.com/johnmweisz/reimplementation-back-end/sessions/2e5805af-dab8-41ac-8af3-b54f67b97439 Co-authored-by: johnmweisz <47699943+johnmweisz@users.noreply.github.com> --- app/models/oidc_config.rb | 10 ++++++---- spec/models/oidc_config_spec.rb | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 4 deletions(-) diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index 599fa0f7e..4e49f5b4f 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -7,8 +7,9 @@ class ProviderNotFound < StandardError; end def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result - parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) - validate!(parsed.fetch("providers", {})) + parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) || {} + raw = parsed.fetch("providers", nil) || {} + validate!(raw) end end @@ -34,13 +35,14 @@ def self.scopes_for(provider) private def self.validate!(providers) - providers.each do |key, cfg| + invalid_keys = providers.each_with_object([]) do |(key, cfg), keys| missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}") - providers.delete(key) + keys << key end end + invalid_keys.each { |key| providers.delete(key) } providers end diff --git a/spec/models/oidc_config_spec.rb b/spec/models/oidc_config_spec.rb index 47f3f34ec..d14f0b7b4 100644 --- a/spec/models/oidc_config_spec.rb +++ b/spec/models/oidc_config_spec.rb @@ -117,6 +117,20 @@ expect(described_class.providers).to eq({}) end + it 'returns an empty hash when YAML is empty (safe_load returns nil)' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return('') + + expect(described_class.providers).to eq({}) + end + + it 'returns an empty hash when providers key is null' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return("providers: null\n") + + expect(described_class.providers).to eq({}) + end + it 'supports YAML aliases in provider definitions' do yaml = <<~YAML providers: From 792773ae34efd899adc1e9cd12b00f9b026da4c7 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 18:13:53 -0400 Subject: [PATCH 22/74] simplify --- app/models/oidc_config.rb | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index 4e49f5b4f..89c054fee 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -7,9 +7,8 @@ class ProviderNotFound < StandardError; end def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result - parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) || {} - raw = parsed.fetch("providers", nil) || {} - validate!(raw) + parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) + validate!(parsed&.dig("providers") || {}) end end @@ -35,14 +34,13 @@ def self.scopes_for(provider) private def self.validate!(providers) - invalid_keys = providers.each_with_object([]) do |(key, cfg), keys| + providers.reject! do |key, cfg| missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}") - keys << key + true end end - invalid_keys.each { |key| providers.delete(key) } providers end From c3bc93ea30a67271d6f8dfe905180f1049e347fc Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 18:23:36 -0400 Subject: [PATCH 23/74] add 404 client select --- spec/requests/oidc_login_spec.rb | 31 ++++++++++++++++++++++++++++++- swagger/v1/swagger.yaml | 18 +++++++++++++++--- 2 files changed, 45 insertions(+), 4 deletions(-) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index cbae322e0..2b0f397f1 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -97,7 +97,7 @@ let(:body) { { provider: "nonexistent" } } before do - allow(OidcConfig).to receive(:find).with("nonexistent") + allow(OidcRequest).to receive(:authorization_uri_for!).with(provider_key: "nonexistent") .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: nonexistent") end @@ -270,6 +270,35 @@ end end + response '404', 'unknown OIDC provider for stored login request' do + schema type: :object, + properties: { + error: { type: :string, example: 'Unknown OIDC provider: deleted-provider' } + }, + required: %w[error] + + let(:auth_request) do + OidcRequest.create!( + state: "state-missing-provider", + nonce: "nonce-missing-provider", + code_verifier: "verifier-missing-provider", + provider: "deleted-provider" + ) + end + + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + allow(OidcConfig).to receive(:find).with("deleted-provider") + .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: deleted-provider") + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to eq("Unknown OIDC provider: deleted-provider") + end + end + # ── 422 — invalid or expired state ── response '422', 'invalid or expired login request' do diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 4edcacbfc..833fd9b16 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -2371,7 +2371,7 @@ paths: security: [] description: | Accepts a provider key, performs OIDC discovery, generates PKCE and state parameters, - persists an AuthRequest for later verification, and returns the provider's authorization URL + persists an OidcRequest for later verification, and returns the provider's authorization URL that the front end should redirect the user to. parameters: [] responses: @@ -2387,6 +2387,18 @@ paths: example: https://accounts.google.com/o/oauth2/v2/auth?client_id=...&scope=openid+email+profile&state=...&nonce=... required: - redirect_uri + '404': + description: unknown OIDC provider + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: 'Unknown OIDC provider: nonexistent' + required: + - error requestBody: content: application/json: @@ -2424,7 +2436,7 @@ paths: required: - token '404': - description: no account found for the email + description: unknown OIDC provider for stored login request content: application/json: schema: @@ -2432,7 +2444,7 @@ paths: properties: error: type: string - example: No account found for unknown@example.com + example: 'Unknown OIDC provider: deleted-provider' required: - error '422': From 24cdb6eabb8ec3dda00149d017b696f68f3d1629 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Sat, 11 Apr 2026 18:41:12 -0400 Subject: [PATCH 24/74] Update app/models/oidc_config.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/models/oidc_config.rb | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-) diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index 89c054fee..b1bd51726 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -8,7 +8,23 @@ def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) - validate!(parsed&.dig("providers") || {}) + + unless parsed.is_a?(Hash) + Rails.logger.warn( + "OIDC config ignored: expected top-level mapping in #{CONFIG_FILE}, got #{parsed.class}" + ) + parsed = {} + end + + providers = parsed["providers"] + unless providers.is_a?(Hash) + Rails.logger.warn( + "OIDC config ignored: expected 'providers' to be a mapping in #{CONFIG_FILE}, got #{providers.class}" + ) + providers = {} + end + + validate!(providers) end end From f2c9b29317242507cdc01d798097aa30f921531b Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sat, 11 Apr 2026 18:42:19 -0400 Subject: [PATCH 25/74] consolidate 404 --- spec/requests/oidc_login_spec.rb | 154 +++++++++++++++---------------- swagger/v1/swagger.yaml | 4 +- 2 files changed, 77 insertions(+), 81 deletions(-) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 2b0f397f1..38beb8169 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -208,94 +208,90 @@ end end - # ── 404 — no account for the email ── + # ── 404 — no matching account or unknown provider ── - response '404', 'no account found for the email' do + response '404', 'no account found or unknown provider' do schema type: :object, properties: { error: { type: :string, example: 'No account found for unknown@example.com' } }, required: %w[error] - let(:auth_request) do - OidcRequest.create!( - state: "state-no-user", - nonce: "nonce-no-user", - code_verifier: "verifier-no-user", - provider: "google-ncsu" - ) + context 'when no user exists for the email' do + let(:auth_request) do + OidcRequest.create!( + state: "state-no-user", + nonce: "nonce-no-user", + code_verifier: "verifier-no-user", + provider: "google-ncsu" + ) + end + + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + provider_cfg = { + "display_name" => "Google NCSU", + "issuer" => "https://accounts.google.com", + "client_id" => "test-client-id", + "client_secret" => "test-client-secret", + "redirect_uri" => "http://localhost:3000/auth/callback", + "scopes" => "openid email profile" + } + allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) + + discovery = instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", + issuer: "https://accounts.google.com", + jwks: instance_double(JSON::JWK::Set) + ) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with("https://accounts.google.com").and_return(discovery) + + fake_access_token = instance_double( + OpenIDConnect::AccessToken, + id_token: "fake.id.token" + ) + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) + .and_return(fake_access_token) + + id_token_obj = instance_double(OpenIDConnect::ResponseObject::IdToken, + raw_attributes: { "email" => "unknown@example.com" }) + allow(id_token_obj).to receive(:verify!).and_return(true) + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .and_return(id_token_obj) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/No account found/) + end end - let(:body) { { state: auth_request.state, code: "authorization-code" } } - - before do - provider_cfg = { - "display_name" => "Google NCSU", - "issuer" => "https://accounts.google.com", - "client_id" => "test-client-id", - "client_secret" => "test-client-secret", - "redirect_uri" => "http://localhost:3000/auth/callback", - "scopes" => "openid email profile" - } - allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) - - discovery = instance_double( - OpenIDConnect::Discovery::Provider::Config::Response, - authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", - token_endpoint: "https://oauth2.googleapis.com/token", - userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", - issuer: "https://accounts.google.com", - jwks: instance_double(JSON::JWK::Set) - ) - allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .with("https://accounts.google.com").and_return(discovery) - - fake_access_token = instance_double( - OpenIDConnect::AccessToken, - id_token: "fake.id.token" - ) - allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) - .and_return(fake_access_token) - - id_token_obj = instance_double(OpenIDConnect::ResponseObject::IdToken, - raw_attributes: { "email" => "unknown@example.com" }) - allow(id_token_obj).to receive(:verify!).and_return(true) - allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .and_return(id_token_obj) - end - - run_test! do |response| - json = JSON.parse(response.body) - expect(json["error"]).to match(/No account found/) - end - end - - response '404', 'unknown OIDC provider for stored login request' do - schema type: :object, - properties: { - error: { type: :string, example: 'Unknown OIDC provider: deleted-provider' } - }, - required: %w[error] - - let(:auth_request) do - OidcRequest.create!( - state: "state-missing-provider", - nonce: "nonce-missing-provider", - code_verifier: "verifier-missing-provider", - provider: "deleted-provider" - ) - end - - let(:body) { { state: auth_request.state, code: "authorization-code" } } - - before do - allow(OidcConfig).to receive(:find).with("deleted-provider") - .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: deleted-provider") - end - - run_test! do |response| - json = JSON.parse(response.body) - expect(json["error"]).to eq("Unknown OIDC provider: deleted-provider") + context 'when the stored provider no longer exists' do + let(:auth_request) do + OidcRequest.create!( + state: "state-missing-provider", + nonce: "nonce-missing-provider", + code_verifier: "verifier-missing-provider", + provider: "deleted-provider" + ) + end + + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + allow(OidcConfig).to receive(:find).with("deleted-provider") + .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: deleted-provider") + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to eq("Unknown OIDC provider: deleted-provider") + end end end diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 833fd9b16..7f43e2f5d 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -2436,7 +2436,7 @@ paths: required: - token '404': - description: unknown OIDC provider for stored login request + description: no account found or unknown provider content: application/json: schema: @@ -2444,7 +2444,7 @@ paths: properties: error: type: string - example: 'Unknown OIDC provider: deleted-provider' + example: No account found for unknown@example.com required: - error '422': From fe2b91752efd0aba9af2aba2cf551c4f80ea5499 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Sat, 11 Apr 2026 18:48:47 -0400 Subject: [PATCH 26/74] Update app/models/oidc_request.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/models/oidc_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 1c3d52ee7..c6f7c3ab8 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -16,7 +16,7 @@ def self.authorization_uri_for!(provider_key:) state = SecureRandom.hex(32) nonce = SecureRandom.hex(32) - code_verifier = SecureRandom.urlsafe_base64(64) + code_verifier = SecureRandom.urlsafe_base64(64, false) code_challenge = Base64.urlsafe_encode64( Digest::SHA256.digest(code_verifier), padding: false ) From 82ae00f1a9d8bcfa7b661b639767b264974f8a18 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Mon, 13 Apr 2026 09:53:19 -0400 Subject: [PATCH 27/74] refactor for consistency --- app/controllers/oidc_login_controller.rb | 9 ++++++--- app/models/oidc_request.rb | 5 ++++- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index fdb83fa97..e3453a698 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -5,12 +5,17 @@ class OidcLoginController < ApplicationController render json: { error: e.message }, status: :not_found end + rescue_from OpenIDConnect::Discovery::DiscoveryFailed, Rack::OAuth2::Client::Error do |e| + render json: { error: "Provider communication failed: #{e.message}" }, status: :bad_gateway + end + # GET /auth/providers def providers render json: OidcConfig.public_list end # POST /auth/client-select + # This is a good candidate for rate limiting def client_select authorization_uri = OidcRequest.authorization_uri_for!(provider_key: params[:provider]) @@ -22,10 +27,8 @@ def callback # Look up and consume the auth request auth_request = OidcRequest.consume_recent_by_state!(params[:state]) - provider = OidcConfig.find(auth_request.provider) - # Match to existing user by email - email = auth_request.verified_email_from_code!(provider: provider, code: params[:code]) + email = auth_request.verified_email_from_code!(provider_key: auth_request.provider, code: params[:code]) user = User.find_by(email: email) if user diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index c6f7c3ab8..adc294b73 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -37,7 +37,8 @@ def self.authorization_uri_for!(provider_key:) ) end - def verified_email_from_code!(provider:, code:) + def verified_email_from_code!(provider_key:, code:) + provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) client = self.class.new_client(provider, discovery: discovery) @@ -67,4 +68,6 @@ def self.new_client(provider, discovery:) userinfo_endpoint: discovery.userinfo_endpoint ) end + + private_class_method :new_client end \ No newline at end of file From 9cc6d737271090a97b50f15614cbcbbf877a373f Mon Sep 17 00:00:00 2001 From: John Weisz Date: Mon, 13 Apr 2026 10:02:03 -0400 Subject: [PATCH 28/74] revert private method --- app/models/oidc_request.rb | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index adc294b73..3bf86c92f 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -68,6 +68,4 @@ def self.new_client(provider, discovery:) userinfo_endpoint: discovery.userinfo_endpoint ) end - - private_class_method :new_client end \ No newline at end of file From e3b11519a1e76db34eed01f8e46373656509512d Mon Sep 17 00:00:00 2001 From: John Weisz Date: Mon, 13 Apr 2026 10:26:40 -0400 Subject: [PATCH 29/74] update tests with discovery rescue --- spec/models/oidc_request_spec.rb | 12 +- spec/requests/oidc_login_spec.rb | 263 ++++++++++++++----------------- swagger/v1/swagger.yaml | 24 +++ 3 files changed, 151 insertions(+), 148 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 09dda1d54..823244c86 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -99,6 +99,10 @@ end describe '#verified_email_from_code!' do + before do + allow(OidcConfig).to receive(:find).with('google-ncsu').and_return(provider) + end + let(:oidc_request) do described_class.create!( state: 'state', @@ -118,18 +122,18 @@ it 'exchanges code, verifies token and returns email' do allow(client).to receive(:authorization_code=).with('authorization-code') allow(client).to receive(:access_token!).with(code_verifier: 'verifier') - .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) + .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .with('fake.id.token', discovery.jwks) - .and_return(id_token_obj) + .with('fake.id.token', discovery.jwks) + .and_return(id_token_obj) allow(id_token_obj).to receive(:verify!).with( issuer: 'https://accounts.google.com', client_id: 'test-client-id', nonce: 'nonce' ) - email = oidc_request.verified_email_from_code!(provider: provider, code: 'authorization-code') + email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') expect(email).to eq('oidcuser@ncsu.edu') end end diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 38beb8169..ede18d46b 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -9,6 +9,56 @@ @institution = Institution.first || Institution.create!(name: "Test Institution") end + let(:provider_cfg) do + { + "display_name" => "Google NCSU", + "issuer" => "https://accounts.google.com", + "client_id" => "test-client-id", + "client_secret" => "test-client-secret", + "redirect_uri" => "http://localhost:3000/auth/callback", + "scopes" => "openid email profile" + } + end + + def stub_provider_config(provider_key = "google-ncsu") + allow(OidcConfig).to receive(:find).with(provider_key).and_return(provider_cfg) + end + + def stub_discovery + discovery = instance_double( + OpenIDConnect::Discovery::Provider::Config::Response, + authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", + token_endpoint: "https://oauth2.googleapis.com/token", + userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", + issuer: "https://accounts.google.com", + jwks: instance_double(JSON::JWK::Set) + ) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .with("https://accounts.google.com").and_return(discovery) + discovery + end + + def stub_token_exchange(email:) + fake_access_token = instance_double(OpenIDConnect::AccessToken, id_token: "fake.id.token") + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!).and_return(fake_access_token) + + id_token_obj = instance_double( + OpenIDConnect::ResponseObject::IdToken, + raw_attributes: { "email" => email } + ) + allow(id_token_obj).to receive(:verify!).and_return(true) + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode).and_return(id_token_obj) + end + + def create_oidc_request(state:, provider: "google-ncsu") + OidcRequest.create!( + state: state, + nonce: "nonce-#{state}", + code_verifier: "verifier-#{state}", + provider: provider + ) + end + # ─── GET /auth/providers ───────────────────────────────────────────── path '/auth/providers' do @@ -31,8 +81,8 @@ before do allow(OidcConfig).to receive(:public_list).and_return([ - { id: "google-ncsu", name: "Google NCSU" } - ]) + { id: "google-ncsu", name: "Google NCSU" } + ]) end run_test! do |response| @@ -77,8 +127,8 @@ before do allow(OidcRequest).to receive(:authorization_uri_for!) - .with(provider_key: "google-ncsu") - .and_return("https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile") + .with(provider_key: "google-ncsu") + .and_return("https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile") end run_test! do |response| @@ -97,8 +147,8 @@ let(:body) { { provider: "nonexistent" } } before do - allow(OidcRequest).to receive(:authorization_uri_for!).with(provider_key: "nonexistent") - .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: nonexistent") + allow(OidcRequest).to receive(:authorization_uri_for!) + .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: nonexistent") end run_test! do |response| @@ -106,6 +156,26 @@ expect(json["error"]).to match(/Unknown OIDC provider/) end end + + response '502', 'provider discovery failed' do + schema type: :object, + properties: { + error: { type: :string, example: 'Provider communication failed: ...' } + }, + required: %w[error] + + let(:body) { { provider: "google-ncsu" } } + + before do + allow(OidcRequest).to receive(:authorization_uri_for!) + .and_raise(OpenIDConnect::Discovery::DiscoveryFailed.new("Connection refused")) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/Provider communication failed/) + end + end end end @@ -143,63 +213,19 @@ let(:user) do User.create!( - name: "oidcuser", - password: "password", - role_id: @roles[:student].id, - full_name: "OIDC User", - email: "oidcuser@ncsu.edu", - institution: @institution - ) - end - - let(:auth_request) do - OidcRequest.create!( - state: "valid-state-hex", - nonce: "valid-nonce-hex", - code_verifier: "valid-code-verifier", - provider: "google-ncsu" + name: "oidcuser", password: "password", role_id: @roles[:student].id, + full_name: "OIDC User", email: "oidcuser@ncsu.edu", institution: @institution ) end + let(:auth_request) { create_oidc_request(state: "valid-state") } let(:body) { { state: auth_request.state, code: "authorization-code" } } before do - # Ensure user exists before callback user - - provider_cfg = { - "display_name" => "Google NCSU", - "issuer" => "https://accounts.google.com", - "client_id" => "test-client-id", - "client_secret" => "test-client-secret", - "redirect_uri" => "http://localhost:3000/auth/callback", - "scopes" => "openid email profile" - } - allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) - - discovery = instance_double( - OpenIDConnect::Discovery::Provider::Config::Response, - authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", - token_endpoint: "https://oauth2.googleapis.com/token", - userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", - issuer: "https://accounts.google.com", - jwks: instance_double(JSON::JWK::Set) - ) - allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .with("https://accounts.google.com").and_return(discovery) - - fake_access_token = instance_double( - OpenIDConnect::AccessToken, - id_token: "fake.id.token" - ) - allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) - .and_return(fake_access_token) - - id_token_obj = instance_double(OpenIDConnect::ResponseObject::IdToken, - raw_attributes: { "email" => "oidcuser@ncsu.edu" }) - allow(id_token_obj).to receive(:verify!).and_return(true) - allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .and_return(id_token_obj) + stub_provider_config + stub_discovery + stub_token_exchange(email: "oidcuser@ncsu.edu") end run_test! do |response| @@ -218,51 +244,13 @@ required: %w[error] context 'when no user exists for the email' do - let(:auth_request) do - OidcRequest.create!( - state: "state-no-user", - nonce: "nonce-no-user", - code_verifier: "verifier-no-user", - provider: "google-ncsu" - ) - end - + let(:auth_request) { create_oidc_request(state: "state-no-user") } let(:body) { { state: auth_request.state, code: "authorization-code" } } before do - provider_cfg = { - "display_name" => "Google NCSU", - "issuer" => "https://accounts.google.com", - "client_id" => "test-client-id", - "client_secret" => "test-client-secret", - "redirect_uri" => "http://localhost:3000/auth/callback", - "scopes" => "openid email profile" - } - allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) - - discovery = instance_double( - OpenIDConnect::Discovery::Provider::Config::Response, - authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", - token_endpoint: "https://oauth2.googleapis.com/token", - userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", - issuer: "https://accounts.google.com", - jwks: instance_double(JSON::JWK::Set) - ) - allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .with("https://accounts.google.com").and_return(discovery) - - fake_access_token = instance_double( - OpenIDConnect::AccessToken, - id_token: "fake.id.token" - ) - allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) - .and_return(fake_access_token) - - id_token_obj = instance_double(OpenIDConnect::ResponseObject::IdToken, - raw_attributes: { "email" => "unknown@example.com" }) - allow(id_token_obj).to receive(:verify!).and_return(true) - allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .and_return(id_token_obj) + stub_provider_config + stub_discovery + stub_token_exchange(email: "unknown@example.com") end run_test! do |response| @@ -272,15 +260,7 @@ end context 'when the stored provider no longer exists' do - let(:auth_request) do - OidcRequest.create!( - state: "state-missing-provider", - nonce: "nonce-missing-provider", - code_verifier: "verifier-missing-provider", - provider: "deleted-provider" - ) - end - + let(:auth_request) { create_oidc_request(state: "state-missing-provider", provider: "deleted-provider") } let(:body) { { state: auth_request.state, code: "authorization-code" } } before do @@ -321,48 +301,18 @@ }, required: %w[error] - let(:auth_request) do - OidcRequest.create!( - state: "state-bad-token", - nonce: "nonce-bad-token", - code_verifier: "verifier-bad-token", - provider: "google-ncsu" - ) - end - + let(:auth_request) { create_oidc_request(state: "state-bad-token") } let(:body) { { state: auth_request.state, code: "authorization-code" } } before do - provider_cfg = { - "display_name" => "Google NCSU", - "issuer" => "https://accounts.google.com", - "client_id" => "test-client-id", - "client_secret" => "test-client-secret", - "redirect_uri" => "http://localhost:3000/auth/callback", - "scopes" => "openid email profile" - } - allow(OidcConfig).to receive(:find).with("google-ncsu").and_return(provider_cfg) - - discovery = instance_double( - OpenIDConnect::Discovery::Provider::Config::Response, - authorization_endpoint: "https://accounts.google.com/o/oauth2/v2/auth", - token_endpoint: "https://oauth2.googleapis.com/token", - userinfo_endpoint: "https://openidconnect.googleapis.com/v1/userinfo", - issuer: "https://accounts.google.com", - jwks: instance_double(JSON::JWK::Set) - ) - allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .with("https://accounts.google.com").and_return(discovery) + stub_provider_config + stub_discovery - fake_access_token = instance_double( - OpenIDConnect::AccessToken, - id_token: "fake.id.token" - ) - allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) - .and_return(fake_access_token) + fake_access_token = instance_double(OpenIDConnect::AccessToken, id_token: "fake.id.token") + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!).and_return(fake_access_token) allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new("invalid signature")) + .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new("invalid signature")) end run_test! do |response| @@ -370,6 +320,31 @@ expect(json["error"]).to match(/Token verification failed/) end end + + # ── 502 — provider communication failed ── + + response '502', 'provider communication failed' do + schema type: :object, + properties: { + error: { type: :string, example: 'Provider communication failed: ...' } + }, + required: %w[error] + + let(:auth_request) { create_oidc_request(state: "state-discovery-fail") } + let(:body) { { state: auth_request.state, code: "authorization-code" } } + + before do + stub_provider_config + + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .and_raise(OpenIDConnect::Discovery::DiscoveryFailed.new("Connection refused")) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/Provider communication failed/) + end + end end end -end +end \ No newline at end of file diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 7f43e2f5d..0a1cfa6c6 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -2399,6 +2399,18 @@ paths: example: 'Unknown OIDC provider: nonexistent' required: - error + '502': + description: provider discovery failed + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: 'Provider communication failed: ...' + required: + - error requestBody: content: application/json: @@ -2471,6 +2483,18 @@ paths: example: 'Token verification failed: invalid signature' required: - error + '502': + description: provider communication failed + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: 'Provider communication failed: ...' + required: + - error requestBody: content: application/json: From 1fa8678e4f4f2a7c29453c7f278cf8f678509c4e Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 14 Apr 2026 09:23:21 -0400 Subject: [PATCH 30/74] remove dev seed --- db/seeds_oidc.rb | 26 -------------------------- 1 file changed, 26 deletions(-) delete mode 100644 db/seeds_oidc.rb diff --git a/db/seeds_oidc.rb b/db/seeds_oidc.rb deleted file mode 100644 index 6e4a2b81d..000000000 --- a/db/seeds_oidc.rb +++ /dev/null @@ -1,26 +0,0 @@ -# For conenience, will drop on final PR -User.find_or_create_by!(email: "jweisz@ncsu.edu") do |user| - user.name = "jweisz" - user.password = "password123" - user.full_name = "John Weisz" - user.institution_id = 1 - user.role_id = 1 -end - -# For conenience, will drop on final PR -User.find_or_create_by!(email: "jvargas6@ncsu.edu") do |user| - user.name = "jvargas6" - user.password = "password123" - user.full_name = "Jose Vargas" - user.institution_id = 1 - user.role_id = 1 -end - -# For conenience, will drop on final PR -User.find_or_create_by!(email: "jcmonseu@ncsu.edu") do |user| - user.name = "jcmonseu" - user.password = "password123" - user.full_name = "Jared Monseur" - user.institution_id = 1 - user.role_id = 1 -end From 747f8b3932240f761691ea5dd8bc67627182a757 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Thu, 16 Apr 2026 14:41:00 -0400 Subject: [PATCH 31/74] propogate oidc_request rename --- app/controllers/oidc_login_controller.rb | 4 ++-- spec/factories.rb | 2 +- spec/requests/oidc_login_spec.rb | 20 ++++++++++---------- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index e3453a698..29bcc019f 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -25,10 +25,10 @@ def client_select # POST /auth/callback def callback # Look up and consume the auth request - auth_request = OidcRequest.consume_recent_by_state!(params[:state]) + oidc_request = OidcRequest.consume_recent_by_state!(params[:state]) # Match to existing user by email - email = auth_request.verified_email_from_code!(provider_key: auth_request.provider, code: params[:code]) + email = oidc_request.verified_email_from_code!(provider_key: oidc_request.provider, code: params[:code]) user = User.find_by(email: email) if user diff --git a/spec/factories.rb b/spec/factories.rb index 778ca2a06..dde396eb4 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true FactoryBot.define do - factory :auth_request do + factory :oidc_request do state { "MyString" } nonce { "MyString" } code_verifier { "MyString" } diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index ede18d46b..06bd1d903 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -218,8 +218,8 @@ def create_oidc_request(state:, provider: "google-ncsu") ) end - let(:auth_request) { create_oidc_request(state: "valid-state") } - let(:body) { { state: auth_request.state, code: "authorization-code" } } + let(:oidc_request) { create_oidc_request(state: "valid-state") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do user @@ -244,8 +244,8 @@ def create_oidc_request(state:, provider: "google-ncsu") required: %w[error] context 'when no user exists for the email' do - let(:auth_request) { create_oidc_request(state: "state-no-user") } - let(:body) { { state: auth_request.state, code: "authorization-code" } } + let(:oidc_request) { create_oidc_request(state: "state-no-user") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do stub_provider_config @@ -260,8 +260,8 @@ def create_oidc_request(state:, provider: "google-ncsu") end context 'when the stored provider no longer exists' do - let(:auth_request) { create_oidc_request(state: "state-missing-provider", provider: "deleted-provider") } - let(:body) { { state: auth_request.state, code: "authorization-code" } } + let(:oidc_request) { create_oidc_request(state: "state-missing-provider", provider: "deleted-provider") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do allow(OidcConfig).to receive(:find).with("deleted-provider") @@ -301,8 +301,8 @@ def create_oidc_request(state:, provider: "google-ncsu") }, required: %w[error] - let(:auth_request) { create_oidc_request(state: "state-bad-token") } - let(:body) { { state: auth_request.state, code: "authorization-code" } } + let(:oidc_request) { create_oidc_request(state: "state-bad-token") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do stub_provider_config @@ -330,8 +330,8 @@ def create_oidc_request(state:, provider: "google-ncsu") }, required: %w[error] - let(:auth_request) { create_oidc_request(state: "state-discovery-fail") } - let(:body) { { state: auth_request.state, code: "authorization-code" } } + let(:oidc_request) { create_oidc_request(state: "state-discovery-fail") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do stub_provider_config From ee0b8cde31b712b8d3005c33f9522bcfecdf1bd2 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Thu, 16 Apr 2026 15:39:57 -0400 Subject: [PATCH 32/74] add username requirement to oidc --- app/controllers/oidc_login_controller.rb | 54 +++++++++++-------- app/models/oidc_request.rb | 30 +++++++++-- ...416184458_add_username_to_oidc_requests.rb | 5 ++ db/schema.rb | 3 +- 4 files changed, 66 insertions(+), 26 deletions(-) create mode 100644 db/migrate/20260416184458_add_username_to_oidc_requests.rb diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 29bcc019f..dafe71e99 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -2,45 +2,55 @@ class OidcLoginController < ApplicationController skip_before_action :authenticate_request! rescue_from OidcConfig::ProviderNotFound do |e| - render json: { error: e.message }, status: :not_found + render_error e.message, status: :not_found end rescue_from OpenIDConnect::Discovery::DiscoveryFailed, Rack::OAuth2::Client::Error do |e| - render json: { error: "Provider communication failed: #{e.message}" }, status: :bad_gateway + render_error "Provider communication failed: #{e.message}", status: :bad_gateway + end + + rescue_from ActionController::ParameterMissing do |e| + render_error e.message, status: :bad_request end # GET /auth/providers + # Returns only public info (id, name) — no secrets or endpoints def providers render json: OidcConfig.public_list end # POST /auth/client-select + # Username is required because emails are not unique # This is a good candidate for rate limiting def client_select - authorization_uri = OidcRequest.authorization_uri_for!(provider_key: params[:provider]) + provider = params.require(:provider) + username = params.require(:username) + authorization_uri = OidcRequest.authorization_uri_for!( + provider_key: provider, + username: username + ) render json: { redirect_uri: authorization_uri } end # POST /auth/callback + # Returns a generic error for all failure modes to avoid leaking + # whether the state, user, or token verification was the cause def callback - # Look up and consume the auth request - oidc_request = OidcRequest.consume_recent_by_state!(params[:state]) - - # Match to existing user by email - email = oidc_request.verified_email_from_code!(provider_key: oidc_request.provider, code: params[:code]) - user = User.find_by(email: email) - - if user - token = user.generate_jwt - render json: { token: }, status: :ok - else - render json: { error: "No account found for #{email}" }, status: :not_found - end - - rescue ActiveRecord::RecordNotFound - render json: { error: "Invalid or expired login request" }, status: :unprocessable_entity - rescue OpenIDConnect::ResponseObject::IdToken::InvalidToken => e - render json: { error: "Token verification failed: #{e.message}" }, status: :unauthorized + state = params.require(:state) + code = params.require(:code) + + oidc_request = OidcRequest.consume_recent_by_state!(state) + user = oidc_request.authenticate_user!(code: code) + render json: { token: user.generate_jwt }, status: :ok + rescue ActiveRecord::RecordNotFound, OidcRequest::AuthenticationError, + OpenIDConnect::ResponseObject::IdToken::InvalidToken + render_error "Authentication failed", status: :unauthorized + end + + private + + def render_error(message, status:) + render json: { error: message }, status: status end -end +end \ No newline at end of file diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 3bf86c92f..b73a3811a 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -1,6 +1,9 @@ class OidcRequest < ApplicationRecord + class AuthenticationError < StandardError; end + scope :recent, ->(window = 5.minutes) { where("created_at > ?", window.ago) } + # Atomically finds and deletes the request to prevent replay attacks def self.consume_recent_by_state!(state, window: 5.minutes) transaction do request = recent(window).lock.find_by!(state: state) @@ -9,7 +12,9 @@ def self.consume_recent_by_state!(state, window: 5.minutes) end end - def self.authorization_uri_for!(provider_key:) + # Generates PKCE, state, and nonce — stores them for callback verification + # PKCE is always sent; providers that don't support it will ignore the extra params + def self.authorization_uri_for!(provider_key:, username:) provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) client = new_client(provider, discovery: discovery) @@ -25,7 +30,8 @@ def self.authorization_uri_for!(provider_key:) state: state, nonce: nonce, code_verifier: code_verifier, - provider: provider_key + provider: provider_key, + username: username ) client.authorization_uri( @@ -37,6 +43,10 @@ def self.authorization_uri_for!(provider_key:) ) end + # Exchanges the authorization code for tokens, then verifies: + # - ID token signature via provider JWKS + # - Issuer, audience (client_id), and nonce claims + # - email_verified claim if the provider includes it def verified_email_from_code!(provider_key:, code:) provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) @@ -55,9 +65,23 @@ def verified_email_from_code!(provider_key:, code:) nonce: nonce ) - id_token.raw_attributes["email"] + claims = id_token.raw_attributes + + if claims.key?("email_verified") && claims["email_verified"] != true + raise AuthenticationError, "Email not verified by provider" + end + + claims["email"] + end + + # Matches on both username from input and email from id_token because emails are not unique + def authenticate_user!(code:) + email = verified_email_from_code!(provider_key: provider, code: code) + User.find_by(name: username, email: email) || + raise(AuthenticationError, "No account found for #{username} with email #{email}") end + # Internal: builds an OpenIDConnect::Client from provider config and discovery def self.new_client(provider, discovery:) OpenIDConnect::Client.new( identifier: provider["client_id"], diff --git a/db/migrate/20260416184458_add_username_to_oidc_requests.rb b/db/migrate/20260416184458_add_username_to_oidc_requests.rb new file mode 100644 index 000000000..83c8907cb --- /dev/null +++ b/db/migrate/20260416184458_add_username_to_oidc_requests.rb @@ -0,0 +1,5 @@ +class AddUsernameToOidcRequests < ActiveRecord::Migration[8.0] + def change + add_column :oidc_requests, :username, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index cc725f68b..ffdcc1f0d 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[8.0].define(version: 2026_04_11_120000) do +ActiveRecord::Schema[8.0].define(version: 2026_04_16_184458) do create_table "account_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "username" t.string "full_name" @@ -247,6 +247,7 @@ t.string "provider" t.datetime "created_at", null: false t.datetime "updated_at", null: false + t.string "username" t.index ["state"], name: "index_oidc_requests_on_state" end From 9f96ecced973ebd79823c6ff7529f0806c24e2fc Mon Sep 17 00:00:00 2001 From: John Weisz Date: Thu, 16 Apr 2026 16:40:07 -0400 Subject: [PATCH 33/74] update tests and swagger --- spec/models/oidc_request_spec.rb | 196 +++++++++++++++++++++------ spec/requests/authentication_spec.rb | 8 +- spec/requests/oidc_login_spec.rb | 146 ++++++++++++-------- swagger/v1/swagger.yaml | 51 ++++--- 4 files changed, 277 insertions(+), 124 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 823244c86..51e7f5892 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -1,6 +1,7 @@ require 'rails_helper' RSpec.describe OidcRequest, type: :model do + include RolesHelper let(:provider) do { 'display_name' => 'Google NCSU', @@ -27,19 +28,43 @@ before do allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .with('https://accounts.google.com').and_return(discovery) + .with('https://accounts.google.com').and_return(discovery) allow(OpenIDConnect::Client).to receive(:new).and_return(client) end + # ─── Helper ───────────────────────────────────────────────────────── + + def create_request(state: 'state', nonce: 'nonce', verifier: 'verifier', + provider: 'google-ncsu', username: 'oidcuser') + described_class.create!( + state: state, + nonce: nonce, + code_verifier: verifier, + provider: provider, + username: username + ) + end + + def stub_token_exchange(email:, email_verified: nil) + allow(client).to receive(:authorization_code=) + allow(client).to receive(:access_token!) + .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) + + claims = { 'email' => email } + claims['email_verified'] = email_verified unless email_verified.nil? + + id_token_obj = instance_double( + OpenIDConnect::ResponseObject::IdToken, + raw_attributes: claims + ) + allow(id_token_obj).to receive(:verify!) + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode).and_return(id_token_obj) + end + + # ─── .consume_recent_by_state! ────────────────────────────────────── + describe '.consume_recent_by_state!' do - let!(:recent_request) do - OidcRequest.create!( - state: 'recent-state', - nonce: 'nonce', - code_verifier: 'verifier', - provider: 'google-ncsu' - ) - end + let!(:recent_request) { create_request(state: 'recent-state') } it 'returns and destroys a recent request matching state' do consumed = described_class.consume_recent_by_state!('recent-state') @@ -58,6 +83,7 @@ expect { described_class.consume_recent_by_state!('recent-state') } .to raise_error(ActiveRecord::RecordNotFound) + # Expired row is not deleted — only consumed rows are destroyed expect(described_class.find_by(id: recent_request.id)).to be_present end @@ -69,75 +95,161 @@ expect(consumed.id).to eq(recent_request.id) expect(described_class.find_by(id: recent_request.id)).to be_nil end + + it 'prevents replay by destroying the row on consumption' do + described_class.consume_recent_by_state!('recent-state') + + expect { described_class.consume_recent_by_state!('recent-state') } + .to raise_error(ActiveRecord::RecordNotFound) + end end + # ─── .authorization_uri_for! ──────────────────────────────────────── + describe '.authorization_uri_for!' do before do allow(OidcConfig).to receive(:find).with('google-ncsu').and_return(provider) end - it 'creates auth request and returns provider authorization URI' do + it 'creates auth request with username and returns provider authorization URI' do allow(client).to receive(:authorization_uri) - .with(hash_including(scope: %w[openid email profile], code_challenge_method: 'S256')) - .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') + .with(hash_including(scope: %w[openid email profile], code_challenge_method: 'S256')) + .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') expect do - uri = described_class.authorization_uri_for!(provider_key: 'google-ncsu') + uri = described_class.authorization_uri_for!(provider_key: 'google-ncsu', username: 'oidcuser') expect(uri).to include('scope=openid+email+profile') end.to change(described_class, :count).by(1) + + created = described_class.last + expect(created.username).to eq('oidcuser') + expect(created.provider).to eq('google-ncsu') end it 'uses default scopes when provider scopes are missing' do allow(OidcConfig).to receive(:find).with('google-ncsu') - .and_return(provider.merge('scopes' => nil)) + .and_return(provider.merge('scopes' => nil)) allow(client).to receive(:authorization_uri) - .with(hash_including(scope: %w[openid email profile], code_challenge_method: 'S256')) - .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') + .with(hash_including(scope: %w[openid email profile], code_challenge_method: 'S256')) + .and_return('https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile') - described_class.authorization_uri_for!(provider_key: 'google-ncsu') + described_class.authorization_uri_for!(provider_key: 'google-ncsu', username: 'oidcuser') end end + # ─── #verified_email_from_code! ───────────────────────────────────── + describe '#verified_email_from_code!' do before do allow(OidcConfig).to receive(:find).with('google-ncsu').and_return(provider) end - let(:oidc_request) do - described_class.create!( - state: 'state', - nonce: 'nonce', - code_verifier: 'verifier', - provider: 'google-ncsu' - ) + let(:oidc_request) { create_request } + + it 'exchanges code, verifies token and returns email' do + stub_token_exchange(email: 'oidcuser@ncsu.edu') + + email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') + expect(email).to eq('oidcuser@ncsu.edu') end - let(:id_token_obj) do - instance_double( - OpenIDConnect::ResponseObject::IdToken, - raw_attributes: { 'email' => 'oidcuser@ncsu.edu' } - ) + it 'returns email when email_verified is true' do + stub_token_exchange(email: 'oidcuser@ncsu.edu', email_verified: true) + + email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') + expect(email).to eq('oidcuser@ncsu.edu') end - it 'exchanges code, verifies token and returns email' do - allow(client).to receive(:authorization_code=).with('authorization-code') - allow(client).to receive(:access_token!).with(code_verifier: 'verifier') - .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) - - allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .with('fake.id.token', discovery.jwks) - .and_return(id_token_obj) - allow(id_token_obj).to receive(:verify!).with( - issuer: 'https://accounts.google.com', - client_id: 'test-client-id', - nonce: 'nonce' - ) + it 'returns email when provider does not include email_verified claim' do + stub_token_exchange(email: 'oidcuser@ncsu.edu') email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') expect(email).to eq('oidcuser@ncsu.edu') end + + it 'raises AuthenticationError when email_verified is false' do + stub_token_exchange(email: 'oidcuser@ncsu.edu', email_verified: false) + + expect { oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') } + .to raise_error(OidcRequest::AuthenticationError, /Email not verified/) + end + end + + # ─── #authenticate_user! ──────────────────────────────────────────── + + describe '#authenticate_user!' do + before(:each) do + @roles = create_roles_hierarchy + @institution = Institution.first || Institution.create!(name: "Test Institution") + allow(OidcConfig).to receive(:find).with('google-ncsu').and_return(provider) + end + + let!(:user) do + User.create!( + name: "OidcUser", password: "password", role_id: @roles[:student].id, + full_name: "OIDC User", email: "OidcUser@ncsu.edu", institution: @institution + ) + end + + it 'matches user by username and email' do + oidc_request = create_request(username: 'OidcUser') + stub_token_exchange(email: 'OidcUser@ncsu.edu') + + result = oidc_request.authenticate_user!(code: 'authorization-code') + expect(result.id).to eq(user.id) + end + + it 'matches user case-insensitively on username' do + oidc_request = create_request(username: 'oidcuser') + stub_token_exchange(email: 'OidcUser@ncsu.edu') + + result = oidc_request.authenticate_user!(code: 'authorization-code') + expect(result.id).to eq(user.id) + end + + it 'matches user case-insensitively on email' do + oidc_request = create_request(username: 'OidcUser') + stub_token_exchange(email: 'oidcuser@ncsu.edu') + + result = oidc_request.authenticate_user!(code: 'authorization-code') + expect(result.id).to eq(user.id) + end + + it 'matches user case-insensitively on both username and email' do + oidc_request = create_request(username: 'OIDCUSER') + stub_token_exchange(email: 'OIDCUSER@NCSU.EDU') + + result = oidc_request.authenticate_user!(code: 'authorization-code') + expect(result.id).to eq(user.id) + end + + it 'raises AuthenticationError when email matches but username does not' do + oidc_request = create_request(username: 'wronguser') + stub_token_exchange(email: 'OidcUser@ncsu.edu') + + expect { oidc_request.authenticate_user!(code: 'authorization-code') } + .to raise_error(OidcRequest::AuthenticationError, /No account found/) + end + + it 'raises AuthenticationError when username matches but email does not' do + oidc_request = create_request(username: 'OidcUser') + stub_token_exchange(email: 'different@example.com') + + expect { oidc_request.authenticate_user!(code: 'authorization-code') } + .to raise_error(OidcRequest::AuthenticationError, /No account found/) + end + + it 'raises AuthenticationError when neither username nor email match' do + oidc_request = create_request(username: 'nobody') + stub_token_exchange(email: 'nobody@example.com') + + expect { oidc_request.authenticate_user!(code: 'authorization-code') } + .to raise_error(OidcRequest::AuthenticationError, /No account found/) + end end + # ─── .new_client ──────────────────────────────────────────────────── + describe '.new_client' do it 'builds an OpenIDConnect::Client with provider credentials and discovery endpoints' do expect(OpenIDConnect::Client).to receive(:new).with( diff --git a/spec/requests/authentication_spec.rb b/spec/requests/authentication_spec.rb index 2543c6a15..2b6d45120 100644 --- a/spec/requests/authentication_spec.rb +++ b/spec/requests/authentication_spec.rb @@ -37,9 +37,6 @@ end let(:credentials) { { user_name: user.name, password: 'password' } } - let(:token) { user.generate_jwt } - let(:Authorization) { "Bearer #{token}" } - let(:headers) { { "Authorization" => "Bearer #{token}" } } run_test! do |response| json_response = JSON.parse(response.body) token = json_response['token'] @@ -70,11 +67,8 @@ ) end let(:credentials) { { user_name: user.name, password: 'wrongpassword' } } - let(:token) { user.generate_jwt } - let(:Authorization) { "Bearer #{token}" } - let(:headers) { { "Authorization" => "Bearer #{token}" } } run_test! end end end -end +end \ No newline at end of file diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 06bd1d903..f3881bb08 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -50,12 +50,13 @@ def stub_token_exchange(email:) allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode).and_return(id_token_obj) end - def create_oidc_request(state:, provider: "google-ncsu") + def create_oidc_request(state:, provider: "google-ncsu", username: "oidcuser") OidcRequest.create!( state: state, nonce: "nonce-#{state}", code_verifier: "verifier-#{state}", - provider: provider + provider: provider, + username: username ) end @@ -103,17 +104,19 @@ def create_oidc_request(state:, provider: "google-ncsu") produces 'application/json' security [] description <<~DESC - Accepts a provider key, performs OIDC discovery, generates PKCE and state parameters, - persists an OidcRequest for later verification, and returns the provider's authorization URL - that the front end should redirect the user to. + Accepts a provider key and username, performs OIDC discovery, generates PKCE and state + parameters, persists an OidcRequest for later verification, and returns the provider's + authorization URL that the front end should redirect the user to. + Username is required because emails are not unique in Expertiza. DESC parameter name: :body, in: :body, schema: { type: :object, properties: { - provider: { type: :string, example: 'google-ncsu', description: 'Key identifying the OIDC provider' } + provider: { type: :string, example: 'google-ncsu', description: 'Key identifying the OIDC provider' }, + username: { type: :string, example: 'jdoe', description: 'Expertiza username for account matching' } }, - required: %w[provider] + required: %w[provider username] } response '200', 'authorization redirect URI' do @@ -123,11 +126,11 @@ def create_oidc_request(state:, provider: "google-ncsu") }, required: %w[redirect_uri] - let(:body) { { provider: "google-ncsu" } } + let(:body) { { provider: "google-ncsu", username: "oidcuser" } } before do allow(OidcRequest).to receive(:authorization_uri_for!) - .with(provider_key: "google-ncsu") + .with(provider_key: "google-ncsu", username: "oidcuser") .and_return("https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile") end @@ -137,6 +140,21 @@ def create_oidc_request(state:, provider: "google-ncsu") end end + response '400', 'missing required parameters' do + schema type: :object, + properties: { + error: { type: :string, example: 'param is missing or the value is empty: username' } + }, + required: %w[error] + + let(:body) { { provider: "google-ncsu" } } + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/username/) + end + end + response '404', 'unknown OIDC provider' do schema type: :object, properties: { @@ -144,7 +162,7 @@ def create_oidc_request(state:, provider: "google-ncsu") }, required: %w[error] - let(:body) { { provider: "nonexistent" } } + let(:body) { { provider: "nonexistent", username: "oidcuser" } } before do allow(OidcRequest).to receive(:authorization_uri_for!) @@ -164,7 +182,7 @@ def create_oidc_request(state:, provider: "google-ncsu") }, required: %w[error] - let(:body) { { provider: "google-ncsu" } } + let(:body) { { provider: "google-ncsu", username: "oidcuser" } } before do allow(OidcRequest).to receive(:authorization_uri_for!) @@ -190,7 +208,8 @@ def create_oidc_request(state:, provider: "google-ncsu") description <<~DESC Called by the front end after the user is redirected back from the identity provider. Exchanges the authorization code for tokens, verifies the ID token, and returns - a local JWT session token if the user's email matches an existing account. + a local JWT session token if the user's username and email match an existing account. + Returns a generic error for all failure modes to avoid information leakage. DESC parameter name: :body, in: :body, schema: { @@ -218,7 +237,7 @@ def create_oidc_request(state:, provider: "google-ncsu") ) end - let(:oidc_request) { create_oidc_request(state: "valid-state") } + let(:oidc_request) { create_oidc_request(state: "valid-state", username: "oidcuser") } let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do @@ -234,17 +253,17 @@ def create_oidc_request(state:, provider: "google-ncsu") end end - # ── 404 — no matching account or unknown provider ── + # ── 401 — authentication failed (generic for all failure modes) ── - response '404', 'no account found or unknown provider' do + response '401', 'authentication failed' do schema type: :object, properties: { - error: { type: :string, example: 'No account found for unknown@example.com' } + error: { type: :string, example: 'Authentication failed' } }, required: %w[error] - context 'when no user exists for the email' do - let(:oidc_request) { create_oidc_request(state: "state-no-user") } + context 'when no user matches the username and email' do + let(:oidc_request) { create_oidc_request(state: "state-no-user", username: "nobody") } let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do @@ -255,69 +274,90 @@ def create_oidc_request(state:, provider: "google-ncsu") run_test! do |response| json = JSON.parse(response.body) - expect(json["error"]).to match(/No account found/) + expect(json["error"]).to eq("Authentication failed") end end - context 'when the stored provider no longer exists' do - let(:oidc_request) { create_oidc_request(state: "state-missing-provider", provider: "deleted-provider") } + context 'when email matches but username does not' do + let(:oidc_request) { create_oidc_request(state: "state-wrong-user", username: "wronguser") } let(:body) { { state: oidc_request.state, code: "authorization-code" } } before do - allow(OidcConfig).to receive(:find).with("deleted-provider") - .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: deleted-provider") + User.create!( + name: "oidcuser", password: "password", role_id: @roles[:student].id, + full_name: "OIDC User", email: "oidcuser@ncsu.edu", institution: @institution + ) + stub_provider_config + stub_discovery + stub_token_exchange(email: "oidcuser@ncsu.edu") end run_test! do |response| json = JSON.parse(response.body) - expect(json["error"]).to eq("Unknown OIDC provider: deleted-provider") + expect(json["error"]).to eq("Authentication failed") end end - end - # ── 422 — invalid or expired state ── + context 'when state is invalid or expired' do + let(:body) { { state: "nonexistent-state", code: "authorization-code" } } - response '422', 'invalid or expired login request' do - schema type: :object, - properties: { - error: { type: :string, example: 'Invalid or expired login request' } - }, - required: %w[error] + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to eq("Authentication failed") + end + end - let(:body) { { state: "nonexistent-state", code: "authorization-code" } } + context 'when token verification fails' do + let(:oidc_request) { create_oidc_request(state: "state-bad-token") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } - run_test! do |response| - json = JSON.parse(response.body) - expect(json["error"]).to eq("Invalid or expired login request") + before do + stub_provider_config + stub_discovery + + fake_access_token = instance_double(OpenIDConnect::AccessToken, id_token: "fake.id.token") + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!).and_return(fake_access_token) + + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new("invalid signature")) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to eq("Authentication failed") + end + end + + context 'when the stored provider no longer exists' do + let(:oidc_request) { create_oidc_request(state: "state-missing-provider", provider: "deleted-provider") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } + + before do + allow(OidcConfig).to receive(:find).with("deleted-provider") + .and_raise(OidcConfig::ProviderNotFound, "Unknown OIDC provider: deleted-provider") + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to eq("Authentication failed") + end end end - # ── 401 — token verification failed ── + # ── 400 — missing required parameters ── - response '401', 'token verification failed' do + response '400', 'missing required parameters' do schema type: :object, properties: { - error: { type: :string, example: 'Token verification failed: invalid signature' } + error: { type: :string, example: 'param is missing or the value is empty: state' } }, required: %w[error] - let(:oidc_request) { create_oidc_request(state: "state-bad-token") } - let(:body) { { state: oidc_request.state, code: "authorization-code" } } - - before do - stub_provider_config - stub_discovery - - fake_access_token = instance_double(OpenIDConnect::AccessToken, id_token: "fake.id.token") - allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!).and_return(fake_access_token) - - allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new("invalid signature")) - end + let(:body) { { code: "authorization-code" } } run_test! do |response| json = JSON.parse(response.body) - expect(json["error"]).to match(/Token verification failed/) + expect(json["error"]).to match(/state/) end end diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 0a1cfa6c6..3989f5231 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -2370,9 +2370,10 @@ paths: - OIDC Authentication security: [] description: | - Accepts a provider key, performs OIDC discovery, generates PKCE and state parameters, - persists an OidcRequest for later verification, and returns the provider's authorization URL - that the front end should redirect the user to. + Accepts a provider key and username, performs OIDC discovery, generates PKCE and state + parameters, persists an OidcRequest for later verification, and returns the provider's + authorization URL that the front end should redirect the user to. + Username is required because emails are not unique in Expertiza. parameters: [] responses: '200': @@ -2387,6 +2388,18 @@ paths: example: https://accounts.google.com/o/oauth2/v2/auth?client_id=...&scope=openid+email+profile&state=...&nonce=... required: - redirect_uri + '400': + description: missing required parameters + content: + application/json: + schema: + type: object + properties: + error: + type: string + example: 'param is missing or the value is empty: username' + required: + - error '404': description: unknown OIDC provider content: @@ -2421,8 +2434,13 @@ paths: type: string example: google-ncsu description: Key identifying the OIDC provider + username: + type: string + example: jdoe + description: Expertiza username for account matching required: - provider + - username "/auth/callback": post: summary: Exchange an OIDC authorization code for a session token @@ -2432,7 +2450,8 @@ paths: description: | Called by the front end after the user is redirected back from the identity provider. Exchanges the authorization code for tokens, verifies the ID token, and returns - a local JWT session token if the user's email matches an existing account. + a local JWT session token if the user's username and email match an existing account. + Returns a generic error for all failure modes to avoid information leakage. parameters: [] responses: '200': @@ -2447,20 +2466,8 @@ paths: description: JWT session token required: - token - '404': - description: no account found or unknown provider - content: - application/json: - schema: - type: object - properties: - error: - type: string - example: No account found for unknown@example.com - required: - - error - '422': - description: invalid or expired login request + '401': + description: authentication failed content: application/json: schema: @@ -2468,11 +2475,11 @@ paths: properties: error: type: string - example: Invalid or expired login request + example: Authentication failed required: - error - '401': - description: token verification failed + '400': + description: missing required parameters content: application/json: schema: @@ -2480,7 +2487,7 @@ paths: properties: error: type: string - example: 'Token verification failed: invalid signature' + example: 'param is missing or the value is empty: state' required: - error '502': From 6da7443d112125cf49d9fa929225a40a905071f6 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Thu, 16 Apr 2026 17:00:00 -0400 Subject: [PATCH 34/74] refine tests, fix existing login swagger --- app/controllers/oidc_login_controller.rb | 3 ++- spec/requests/authentication_spec.rb | 1 + swagger/v1/swagger.yaml | 1 + 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index dafe71e99..3d618fcd9 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -44,7 +44,8 @@ def callback user = oidc_request.authenticate_user!(code: code) render json: { token: user.generate_jwt }, status: :ok rescue ActiveRecord::RecordNotFound, OidcRequest::AuthenticationError, - OpenIDConnect::ResponseObject::IdToken::InvalidToken + OpenIDConnect::ResponseObject::IdToken::InvalidToken, + OidcConfig::ProviderNotFound render_error "Authentication failed", status: :unauthorized end diff --git a/spec/requests/authentication_spec.rb b/spec/requests/authentication_spec.rb index 2b6d45120..8755c4b07 100644 --- a/spec/requests/authentication_spec.rb +++ b/spec/requests/authentication_spec.rb @@ -12,6 +12,7 @@ post 'Logs in a user' do tags 'Authentication' consumes 'application/json' + security [] parameter name: :credentials, in: :body, schema: { type: :object, properties: { diff --git a/swagger/v1/swagger.yaml b/swagger/v1/swagger.yaml index 3989f5231..e793d2f8b 100644 --- a/swagger/v1/swagger.yaml +++ b/swagger/v1/swagger.yaml @@ -2317,6 +2317,7 @@ paths: summary: Logs in a user tags: - Authentication + security: [] parameters: [] responses: '200': From a55c419c306d00a77f830a7327d5d63352742f99 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Thu, 16 Apr 2026 17:13:18 -0400 Subject: [PATCH 35/74] Update client_select method comments Added comment about username requirement and rate limiting. --- app/controllers/oidc_login_controller.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 3d618fcd9..78f01649d 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -21,6 +21,7 @@ def providers # POST /auth/client-select # Username is required because emails are not unique + # Provider only knows email # This is a good candidate for rate limiting def client_select provider = params.require(:provider) @@ -54,4 +55,4 @@ def callback def render_error(message, status:) render json: { error: message }, status: status end -end \ No newline at end of file +end From f1579400b201caa0bb3cbe5ee6a45051397ba538 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Fri, 17 Apr 2026 19:33:45 -0400 Subject: [PATCH 36/74] explicit case insensitive lookup --- app/models/oidc_request.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index b73a3811a..1fb3833a7 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -77,7 +77,7 @@ def verified_email_from_code!(provider_key:, code:) # Matches on both username from input and email from id_token because emails are not unique def authenticate_user!(code:) email = verified_email_from_code!(provider_key: provider, code: code) - User.find_by(name: username, email: email) || + User.where("LOWER(name) = ? AND LOWER(email) = ?", username.downcase, email.downcase).first || raise(AuthenticationError, "No account found for #{username} with email #{email}") end From 5ea7dd820116d73acb87a3e63cccabf98fdf052a Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Wed, 15 Apr 2026 21:42:35 -0400 Subject: [PATCH 37/74] Consolidated validity window. --- app/models/oidc_request.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 3bf86c92f..c18e9794a 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -1,5 +1,8 @@ class OidcRequest < ApplicationRecord - scope :recent, ->(window = 5.minutes) { where("created_at > ?", window.ago) } + VALIDITY_WINDOW = 5.minutes + + scope :recent, ->(window = VALIDITY_WINDOW) { where("created_at > ?", window.ago) } + scope :stale, ->(window = VALIDITY_WINDOW) { where("created_at <= ?", window.ago) } def self.consume_recent_by_state!(state, window: 5.minutes) transaction do From 3a508a5b2721056651a7113e3b07fdf8368a0c9f Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Wed, 15 Apr 2026 21:57:35 -0400 Subject: [PATCH 38/74] Stale logins are deleted if attempted by user. --- app/models/oidc_request.rb | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index c18e9794a..554755e47 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -4,9 +4,17 @@ class OidcRequest < ApplicationRecord scope :recent, ->(window = VALIDITY_WINDOW) { where("created_at > ?", window.ago) } scope :stale, ->(window = VALIDITY_WINDOW) { where("created_at <= ?", window.ago) } - def self.consume_recent_by_state!(state, window: 5.minutes) + def stale?(window = VALIDITY_WINDOW) + created_at <= window.ago + end + + def self.consume_recent_by_state!(state, window: VALIDITY_WINDOW) transaction do - request = recent(window).lock.find_by!(state: state) + request = lock.find_by!(state: state) + if request.stale?(window) + request.destroy! + raise ActiveRecord::RecordNotFound + end request.destroy! request end From 33f33f060643790095e6fc28ce8634972ec06faa Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 15:23:02 -0400 Subject: [PATCH 39/74] Probabilistic cleanup for new OIDC login request. --- app/models/oidc_request.rb | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 554755e47..56832395a 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -1,5 +1,8 @@ class OidcRequest < ApplicationRecord VALIDITY_WINDOW = 5.minutes + CLEANUP_PROBABILITY = 0.10 + + after_create :maybe_cleanup_stale scope :recent, ->(window = VALIDITY_WINDOW) { where("created_at > ?", window.ago) } scope :stale, ->(window = VALIDITY_WINDOW) { where("created_at <= ?", window.ago) } @@ -79,4 +82,10 @@ def self.new_client(provider, discovery:) userinfo_endpoint: discovery.userinfo_endpoint ) end + + private + + def maybe_cleanup_stale + CleanupStaleOidcRequestsJob.perform_later if rand < CLEANUP_PROBABILITY + end end \ No newline at end of file From a0b97912c6a2c56a4843b885e8a00d54e0de235f Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 15:32:02 -0400 Subject: [PATCH 40/74] Fixed existing tests that previously checked if stale OIDC requests remained in the DB. --- spec/models/oidc_request_spec.rb | 102 ++++++++++++++++++++++++++++++- 1 file changed, 100 insertions(+), 2 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 823244c86..31ea909a9 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -53,12 +53,12 @@ .to raise_error(ActiveRecord::RecordNotFound) end - it 'raises RecordNotFound for expired requests' do + it 'raises RecordNotFound for expired requests and destroys the stale row' do recent_request.update_columns(created_at: 10.minutes.ago) expect { described_class.consume_recent_by_state!('recent-state') } .to raise_error(ActiveRecord::RecordNotFound) - expect(described_class.find_by(id: recent_request.id)).to be_present + expect(described_class.find_by(id: recent_request.id)).to be_nil end it 'supports a custom recency window' do @@ -153,4 +153,102 @@ expect(result).to eq(client) end end + + describe 'stale row cleanup' do + def make_stale_requests(count) + count.times.map do |i| + req = OidcRequest.create!( + state: "stale-state-#{i}-#{SecureRandom.hex(4)}", + nonce: "nonce-#{i}", + code_verifier: "verifier-#{i}", + provider: 'google-ncsu' + ) + req.update_columns(created_at: 10.minutes.ago) + req + end + end + + before do + # Suppress actual job execution — we test DB state directly + allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) + end + + context 'detect-and-delete on consume_recent_by_state!' do + it 'immediately destroys a handful of stale rows when each is looked up' do + requests = make_stale_requests(5) + + requests.each do |req| + expect { described_class.consume_recent_by_state!(req.state) } + .to raise_error(ActiveRecord::RecordNotFound) + end + + surviving_ids = described_class.where(id: requests.map(&:id)).pluck(:id) + expect(surviving_ids).to be_empty + end + + it 'leaves fresh rows untouched while destroying stale ones' do + stale = make_stale_requests(3) + fresh = OidcRequest.create!( + state: 'fresh-state', + nonce: 'fresh-nonce', + code_verifier: 'fresh-verifier', + provider: 'google-ncsu' + ) + + stale.each do |req| + expect { described_class.consume_recent_by_state!(req.state) } + .to raise_error(ActiveRecord::RecordNotFound) + end + + expect(described_class.find_by(id: fresh.id)).to be_present + expect(described_class.where(id: stale.map(&:id)).count).to eq(0) + end + end + end + + describe 'probabilistic cleanup via after_create' do + it 'enqueues CleanupStaleOidcRequestsJob approximately 10% of the time over many requests' do + total = 500 + enqueued = 0 + + allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) { enqueued += 1 } + + total.times do |i| + OidcRequest.create!( + state: "prob-state-#{i}-#{SecureRandom.hex(4)}", + nonce: "nonce-#{i}", + code_verifier: "verifier-#{i}", + provider: 'google-ncsu' + ) + end + + rate = enqueued.to_f / total + expect(rate).to be_between(0.04, 0.20), + "Expected ~10% cleanup rate, got #{(rate * 100).round(1)}% (#{enqueued}/#{total})" + end + + it 'does not enqueue the job when rand is above the threshold' do + allow(described_class).to receive(:rand).and_return(0.99) + expect(CleanupStaleOidcRequestsJob).not_to receive(:perform_later) + + OidcRequest.create!( + state: 'no-cleanup-state', + nonce: 'nonce', + code_verifier: 'verifier', + provider: 'google-ncsu' + ) + end + + it 'always enqueues the job when rand is below the threshold' do + allow(described_class).to receive(:rand).and_return(0.01) + expect(CleanupStaleOidcRequestsJob).to receive(:perform_later).once + + OidcRequest.create!( + state: 'yes-cleanup-state', + nonce: 'nonce', + code_verifier: 'verifier', + provider: 'google-ncsu' + ) + end + end end \ No newline at end of file From e697897153348266fef4020850a89db5694b0502 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 15:52:06 -0400 Subject: [PATCH 41/74] raising in the if-stale check rolls back the destroy, so the row in the DB isn't deleted, so I fixed that. --- app/models/oidc_request.rb | 15 +++++++++------ 1 file changed, 9 insertions(+), 6 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 56832395a..02987e0f5 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -12,15 +12,18 @@ def stale?(window = VALIDITY_WINDOW) end def self.consume_recent_by_state!(state, window: VALIDITY_WINDOW) + was_stale = false + request = nil + transaction do - request = lock.find_by!(state: state) - if request.stale?(window) - request.destroy! - raise ActiveRecord::RecordNotFound - end + request = lock.find_by!(state: state) + was_stale = request.stale?(window) request.destroy! - request end + + raise ActiveRecord::RecordNotFound if was_stale + + request end def self.authorization_uri_for!(provider_key:) From 75d109cbfa19a90c96d827d60eb47cc4a67b4e0d Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 15:53:04 -0400 Subject: [PATCH 42/74] Added cleanup job from other PR because having the cleanup as a job means we can use perform_later to preserve login performance. --- app/jobs/cleanup_stale_oidc_requests_job.rb | 7 +++++++ 1 file changed, 7 insertions(+) create mode 100644 app/jobs/cleanup_stale_oidc_requests_job.rb diff --git a/app/jobs/cleanup_stale_oidc_requests_job.rb b/app/jobs/cleanup_stale_oidc_requests_job.rb new file mode 100644 index 000000000..41b6143cf --- /dev/null +++ b/app/jobs/cleanup_stale_oidc_requests_job.rb @@ -0,0 +1,7 @@ +class CleanupStaleOidcRequestsJob < ApplicationJob + queue_as :default + + def perform + OidcRequest.stale.delete_all + end +end From c3efd70735fed01f7d280911933d20012912374e Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 15:59:55 -0400 Subject: [PATCH 43/74] Cleaned up OIDC tests and added new ones for cleanup. --- spec/models/oidc_request_spec.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 31ea909a9..f7802c161 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -169,7 +169,7 @@ def make_stale_requests(count) end before do - # Suppress actual job execution — we test DB state directly + # Suppress actual cleanup execution — we test DB state directly allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) end @@ -208,10 +208,10 @@ def make_stale_requests(count) describe 'probabilistic cleanup via after_create' do it 'enqueues CleanupStaleOidcRequestsJob approximately 10% of the time over many requests' do - total = 500 - enqueued = 0 + total = 500 + cleaned = 0 - allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) { enqueued += 1 } + allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) { cleaned += 1 } total.times do |i| OidcRequest.create!( @@ -222,13 +222,13 @@ def make_stale_requests(count) ) end - rate = enqueued.to_f / total + rate = cleaned.to_f / total expect(rate).to be_between(0.04, 0.20), - "Expected ~10% cleanup rate, got #{(rate * 100).round(1)}% (#{enqueued}/#{total})" + "Expected ~10% job enqueue rate, got #{(rate * 100).round(1)}% (#{cleaned}/#{total})" end - it 'does not enqueue the job when rand is above the threshold' do - allow(described_class).to receive(:rand).and_return(0.99) + it 'does not enqueue CleanupStaleOidcRequestsJob when rand is above the threshold' do + allow_any_instance_of(OidcRequest).to receive(:rand).and_return(0.99) expect(CleanupStaleOidcRequestsJob).not_to receive(:perform_later) OidcRequest.create!( @@ -239,8 +239,8 @@ def make_stale_requests(count) ) end - it 'always enqueues the job when rand is below the threshold' do - allow(described_class).to receive(:rand).and_return(0.01) + it 'enqueues CleanupStaleOidcRequestsJob when rand is below the threshold' do + allow_any_instance_of(OidcRequest).to receive(:rand).and_return(0.01) expect(CleanupStaleOidcRequestsJob).to receive(:perform_later).once OidcRequest.create!( From 21284d5656cd93e55c5a2255a20b47e6f6edfdbd Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 16:09:55 -0400 Subject: [PATCH 44/74] Quick format fix. --- spec/models/oidc_request_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index f7802c161..d38f39a1a 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -169,7 +169,7 @@ def make_stale_requests(count) end before do - # Suppress actual cleanup execution — we test DB state directly + # Suppress actual cleanup execution - we test DB state directly allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) end From b85bf2a2eb3dddab0e6de3a75e7a7218c787b972 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 21:38:44 -0400 Subject: [PATCH 45/74] Refactored design to make :recent and :stale private. --- app/jobs/cleanup_stale_oidc_requests_job.rb | 2 +- app/models/oidc_request.rb | 5 +++++ 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/app/jobs/cleanup_stale_oidc_requests_job.rb b/app/jobs/cleanup_stale_oidc_requests_job.rb index 41b6143cf..147f55b70 100644 --- a/app/jobs/cleanup_stale_oidc_requests_job.rb +++ b/app/jobs/cleanup_stale_oidc_requests_job.rb @@ -2,6 +2,6 @@ class CleanupStaleOidcRequestsJob < ApplicationJob queue_as :default def perform - OidcRequest.stale.delete_all + OidcRequest.delete_stale end end diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 02987e0f5..e8e742eca 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -6,6 +6,11 @@ class OidcRequest < ApplicationRecord scope :recent, ->(window = VALIDITY_WINDOW) { where("created_at > ?", window.ago) } scope :stale, ->(window = VALIDITY_WINDOW) { where("created_at <= ?", window.ago) } + private_class_method :recent, :stale + + def self.delete_stale(window: VALIDITY_WINDOW) + stale(window).delete_all + end def stale?(window = VALIDITY_WINDOW) created_at <= window.ago From f2f424f6010b8702b4a101209925b027b28debaf Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 21:46:45 -0400 Subject: [PATCH 46/74] Internalized window, which means we can't override it anymore in the tests. --- app/models/oidc_request.rb | 4 ++-- spec/models/oidc_request_spec.rb | 9 --------- 2 files changed, 2 insertions(+), 11 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index e8e742eca..cefedeb9f 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -16,13 +16,13 @@ def stale?(window = VALIDITY_WINDOW) created_at <= window.ago end - def self.consume_recent_by_state!(state, window: VALIDITY_WINDOW) + def self.consume_recent_by_state!(state) was_stale = false request = nil transaction do request = lock.find_by!(state: state) - was_stale = request.stale?(window) + was_stale = request.stale? request.destroy! end diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index d38f39a1a..28ab76956 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -60,15 +60,6 @@ .to raise_error(ActiveRecord::RecordNotFound) expect(described_class.find_by(id: recent_request.id)).to be_nil end - - it 'supports a custom recency window' do - recent_request.update_columns(created_at: 10.minutes.ago) - - consumed = described_class.consume_recent_by_state!('recent-state', window: 15.minutes) - - expect(consumed.id).to eq(recent_request.id) - expect(described_class.find_by(id: recent_request.id)).to be_nil - end end describe '.authorization_uri_for!' do From 4ab2e42f6d1fb07994798f3d8858233a95956d0d Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sat, 18 Apr 2026 22:01:52 -0400 Subject: [PATCH 47/74] Improved test for probabilistic cleanup. --- spec/models/oidc_request_spec.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 28ab76956..adf2ab101 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -198,7 +198,7 @@ def make_stale_requests(count) end describe 'probabilistic cleanup via after_create' do - it 'enqueues CleanupStaleOidcRequestsJob approximately 10% of the time over many requests' do + it 'enqueues CleanupStaleOidcRequestsJob neither every time nor never over many requests' do total = 500 cleaned = 0 @@ -213,9 +213,8 @@ def make_stale_requests(count) ) end - rate = cleaned.to_f / total - expect(rate).to be_between(0.04, 0.20), - "Expected ~10% job enqueue rate, got #{(rate * 100).round(1)}% (#{cleaned}/#{total})" + expect(cleaned).to be > 0, "Expected cleanup to fire at least once in #{total} requests" + expect(cleaned).to be < total, "Expected cleanup to be skipped at least once in #{total} requests" end it 'does not enqueue CleanupStaleOidcRequestsJob when rand is above the threshold' do From effeff827c5a7490fb1de366ae1d633aaff8a6d1 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sun, 19 Apr 2026 10:23:08 -0400 Subject: [PATCH 48/74] resolve conflicts, minor refactor --- app/models/oidc_request.rb | 19 +++++++++++-- spec/models/oidc_request_spec.rb | 47 +++++++++++++++++++++++++------- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 1fb3833a7..f960f86b5 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -1,12 +1,19 @@ class OidcRequest < ApplicationRecord class AuthenticationError < StandardError; end - scope :recent, ->(window = 5.minutes) { where("created_at > ?", window.ago) } + VALIDITY_WINDOW = 5.minutes + CLEANUP_PROBABILITY = 0.10 + + after_create :maybe_enqueue_stale_cleanup + + def self.delete_stale + where("created_at <= ?", VALIDITY_WINDOW.ago).delete_all + end # Atomically finds and deletes the request to prevent replay attacks - def self.consume_recent_by_state!(state, window: 5.minutes) + def self.consume_recent_by_state!(state) transaction do - request = recent(window).lock.find_by!(state: state) + request = where("created_at > ?", VALIDITY_WINDOW.ago).lock.find_by!(state: state) request.destroy! request end @@ -92,4 +99,10 @@ def self.new_client(provider, discovery:) userinfo_endpoint: discovery.userinfo_endpoint ) end + + private + + def maybe_enqueue_stale_cleanup + CleanupStaleOidcRequestsJob.perform_later if rand < CLEANUP_PROBABILITY + end end \ No newline at end of file diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 51e7f5892..06965ad74 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -2,6 +2,7 @@ RSpec.describe OidcRequest, type: :model do include RolesHelper + let(:provider) do { 'display_name' => 'Google NCSU', @@ -32,7 +33,7 @@ allow(OpenIDConnect::Client).to receive(:new).and_return(client) end - # ─── Helper ───────────────────────────────────────────────────────── + # ─── Helpers ──────────────────────────────────────────────────────── def create_request(state: 'state', nonce: 'nonce', verifier: 'verifier', provider: 'google-ncsu', username: 'oidcuser') @@ -87,15 +88,6 @@ def stub_token_exchange(email:, email_verified: nil) expect(described_class.find_by(id: recent_request.id)).to be_present end - it 'supports a custom recency window' do - recent_request.update_columns(created_at: 10.minutes.ago) - - consumed = described_class.consume_recent_by_state!('recent-state', window: 15.minutes) - - expect(consumed.id).to eq(recent_request.id) - expect(described_class.find_by(id: recent_request.id)).to be_nil - end - it 'prevents replay by destroying the row on consumption' do described_class.consume_recent_by_state!('recent-state') @@ -104,6 +96,41 @@ def stub_token_exchange(email:, email_verified: nil) end end + # ─── .delete_stale ────────────────────────────────────────────────── + + describe '.delete_stale' do + it 'deletes rows older than the validity window and preserves fresh rows' do + fresh = create_request(state: 'fresh') + stale = create_request(state: 'stale') + stale.update_columns(created_at: 10.minutes.ago) + + described_class.delete_stale + + expect(described_class.find_by(id: fresh.id)).to be_present + expect(described_class.find_by(id: stale.id)).to be_nil + end + end + + # ─── after_create :maybe_enqueue_stale_cleanup ────────────────────── + + describe 'probabilistic cleanup on create' do + it 'enqueues CleanupStaleOidcRequestsJob when rand falls under the threshold' do + allow_any_instance_of(described_class).to receive(:rand).and_return(0.0) + + expect(CleanupStaleOidcRequestsJob).to receive(:perform_later) + + create_request(state: 'any') + end + + it 'does not enqueue when rand falls above the threshold' do + allow_any_instance_of(described_class).to receive(:rand).and_return(0.99) + + expect(CleanupStaleOidcRequestsJob).not_to receive(:perform_later) + + create_request(state: 'any') + end + end + # ─── .authorization_uri_for! ──────────────────────────────────────── describe '.authorization_uri_for!' do From 9ad2ec7960d2350b642ea41828e7c0e09c2602ba Mon Sep 17 00:00:00 2001 From: John Weisz Date: Sun, 19 Apr 2026 10:36:08 -0400 Subject: [PATCH 49/74] add raise note --- app/models/oidc_request.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index f960f86b5..1561e793e 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -11,6 +11,7 @@ def self.delete_stale end # Atomically finds and deletes the request to prevent replay attacks + # Raises RecordNotFound if entry does not exist def self.consume_recent_by_state!(state) transaction do request = where("created_at > ?", VALIDITY_WINDOW.ago).lock.find_by!(state: state) From 0b05104d8593417def845122ed46b0395e13bf51 Mon Sep 17 00:00:00 2001 From: John Weisz <47699943+johnmweisz@users.noreply.github.com> Date: Sun, 19 Apr 2026 14:42:01 -0400 Subject: [PATCH 50/74] Update app/models/oidc_request.rb Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com> --- app/models/oidc_request.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 1561e793e..87feac565 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -79,7 +79,10 @@ def verified_email_from_code!(provider_key:, code:) raise AuthenticationError, "Email not verified by provider" end - claims["email"] + email = claims["email"].to_s.strip + raise AuthenticationError, "Email missing from provider response" if email.blank? + + email end # Matches on both username from input and email from id_token because emails are not unique From 03b24472e30bf46a299945ca564bd55eaea6cd3e Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sun, 19 Apr 2026 16:12:54 -0400 Subject: [PATCH 51/74] Improved existing OIDC test. --- spec/requests/oidc_login_spec.rb | 38 ++++++++++++++++++++++++-------- 1 file changed, 29 insertions(+), 9 deletions(-) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 06bd1d903..d2ab60ae8 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -330,19 +330,39 @@ def create_oidc_request(state:, provider: "google-ncsu") }, required: %w[error] - let(:oidc_request) { create_oidc_request(state: "state-discovery-fail") } - let(:body) { { state: oidc_request.state, code: "authorization-code" } } + context 'when OIDC discovery fails' do + let(:oidc_request) { create_oidc_request(state: "state-discovery-fail") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } - before do - stub_provider_config + before do + stub_provider_config - allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) - .and_raise(OpenIDConnect::Discovery::DiscoveryFailed.new("Connection refused")) + allow(OpenIDConnect::Discovery::Provider::Config).to receive(:discover!) + .and_raise(OpenIDConnect::Discovery::DiscoveryFailed.new("Connection refused")) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/Provider communication failed/) + end end - run_test! do |response| - json = JSON.parse(response.body) - expect(json["error"]).to match(/Provider communication failed/) + context 'when token exchange raises an OAuth2 client error' do + let(:oidc_request) { create_oidc_request(state: "state-token-exchange-fail") } + let(:body) { { state: oidc_request.state, code: "authorization-code" } } + + before do + stub_provider_config + stub_discovery + + allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!) + .and_raise(Rack::OAuth2::Client::Error.new(400, error: "invalid_grant")) + end + + run_test! do |response| + json = JSON.parse(response.body) + expect(json["error"]).to match(/Provider communication failed/) + end end end end From ccae37aebd4b705a9e886bd3a9274485ecd73bd8 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sun, 19 Apr 2026 16:18:02 -0400 Subject: [PATCH 52/74] test(oidc_request): add delete_stale, stale?, and InvalidToken coverage --- spec/models/oidc_request_spec.rb | 50 ++++++++++++++++++++++++++++++++ 1 file changed, 50 insertions(+) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index adf2ab101..e58905461 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -127,6 +127,22 @@ email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') expect(email).to eq('oidcuser@ncsu.edu') end + + it 'raises InvalidToken when id token verification fails' do + allow(client).to receive(:authorization_code=).with('authorization-code') + allow(client).to receive(:access_token!).with(code_verifier: 'verifier') + .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) + + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) + .with('fake.id.token', discovery.jwks) + .and_return(id_token_obj) + allow(id_token_obj).to receive(:verify!) + .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new('nonce mismatch')) + + expect { + oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') + }.to raise_error(OpenIDConnect::ResponseObject::IdToken::InvalidToken, /nonce mismatch/) + end end describe '.new_client' do @@ -145,6 +161,40 @@ end end + describe '.delete_stale' do + it 'removes stale rows and leaves fresh ones untouched' do + fresh = OidcRequest.create!(state: 'fresh-del', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + stale = OidcRequest.create!(state: 'stale-del', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + stale.update_columns(created_at: 10.minutes.ago) + + described_class.delete_stale + + expect(described_class.find_by(id: fresh.id)).to be_present + expect(described_class.find_by(id: stale.id)).to be_nil + end + + it 'returns 0 when no stale rows exist' do + OidcRequest.create!(state: 'only-fresh', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + + expect(described_class.delete_stale).to eq(0) + end + end + + describe '#stale?' do + it 'returns false for a freshly created request' do + req = OidcRequest.create!(state: 'new-req', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + + expect(req.stale?).to be false + end + + it 'returns true for a request older than the validity window' do + req = OidcRequest.create!(state: 'old-req', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + req.update_columns(created_at: 10.minutes.ago) + + expect(req.stale?).to be true + end + end + describe 'stale row cleanup' do def make_stale_requests(count) count.times.map do |i| From 4dac121c239228c51ac5d4598881b4977cbe78d4 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Sun, 19 Apr 2026 16:26:21 -0400 Subject: [PATCH 53/74] cover non-Hash YAML guards and mixed delimiter scopes --- spec/models/oidc_config_spec.rb | 23 +++++++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/spec/models/oidc_config_spec.rb b/spec/models/oidc_config_spec.rb index d14f0b7b4..16c7b45ca 100644 --- a/spec/models/oidc_config_spec.rb +++ b/spec/models/oidc_config_spec.rb @@ -131,6 +131,23 @@ expect(described_class.providers).to eq({}) end + it 'returns an empty hash when the top-level YAML is not a Hash' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return("- item_one\n- item_two\n") + + expect(described_class.providers).to eq({}) + expect(Rails.logger).to have_received(:warn).with(/expected top-level mapping/) + end + + it 'returns an empty hash when the providers value is not a Hash' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE) + .and_return("providers:\n - item_one\n - item_two\n") + + expect(described_class.providers).to eq({}) + expect(Rails.logger).to have_received(:warn).with(/expected 'providers' to be a mapping/) + end + it 'supports YAML aliases in provider definitions' do yaml = <<~YAML providers: @@ -219,5 +236,11 @@ expect(described_class.scopes_for(provider)).to eq(%w[openid email profile]) end + + it 'parses mixed comma and whitespace delimiters' do + provider = { 'scopes' => 'openid, email, profile' } + + expect(described_class.scopes_for(provider)).to eq(%w[openid email profile]) + end end end From 6efc3f83aafc7665f311f2e5127ec9482cd9c5e0 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Mon, 20 Apr 2026 14:58:05 -0400 Subject: [PATCH 54/74] single quote routes --- config/routes.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/config/routes.rb b/config/routes.rb index 4ec5199c3..32724f58d 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -215,7 +215,7 @@ resources :duties, controller: 'assignments_duties', only: [:index, :create, :destroy] end - get "auth/providers", to: "oidc_login#providers" - post "auth/client-select", to: "oidc_login#client_select" - post "auth/callback", to: "oidc_login#callback" + get 'auth/providers', to: 'oidc_login#providers' + post 'auth/client-select', to: 'oidc_login#client_select' + post 'auth/callback', to: 'oidc_login#callback' end From 04895dad0ab5c3b1a2a146d8149b546bc123874c Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Apr 2026 01:00:16 +0000 Subject: [PATCH 55/74] Deduplicate stale cleanup specs after merge Co-authored-by: JaredM2028 <194444205+JaredM2028@users.noreply.github.com> --- spec/models/oidc_request_spec.rb | 25 ++++++------------------- 1 file changed, 6 insertions(+), 19 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index cec2851c8..e565e5989 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -109,6 +109,12 @@ def stub_token_exchange(email:, email_verified: nil) expect(described_class.find_by(id: fresh.id)).to be_present expect(described_class.find_by(id: stale.id)).to be_nil end + + it 'returns 0 when no stale rows exist' do + create_request(state: 'only-fresh') + + expect(described_class.delete_stale).to eq(0) + end end # ─── after_create :maybe_enqueue_stale_cleanup ────────────────────── @@ -308,25 +314,6 @@ def stub_token_exchange(email:, email_verified: nil) expect(result).to eq(client) end end - describe '.delete_stale' do - it 'removes stale rows and leaves fresh ones untouched' do - fresh = OidcRequest.create!(state: 'fresh-del', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') - stale = OidcRequest.create!(state: 'stale-del', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') - stale.update_columns(created_at: 10.minutes.ago) - - described_class.delete_stale - - expect(described_class.find_by(id: fresh.id)).to be_present - expect(described_class.find_by(id: stale.id)).to be_nil - end - - it 'returns 0 when no stale rows exist' do - OidcRequest.create!(state: 'only-fresh', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') - - expect(described_class.delete_stale).to eq(0) - end - end - describe '#stale?' do it 'returns false for a freshly created request' do req = OidcRequest.create!(state: 'new-req', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') From 6af15a6a9fe6e6036065e150e51cd6943d6699a4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 21 Apr 2026 01:01:17 +0000 Subject: [PATCH 56/74] Use create_request helper in merged stale tests Co-authored-by: JaredM2028 <194444205+JaredM2028@users.noreply.github.com> --- spec/models/oidc_request_spec.rb | 29 ++++++++++++----------------- 1 file changed, 12 insertions(+), 17 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index e565e5989..de689747f 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -316,13 +316,13 @@ def stub_token_exchange(email:, email_verified: nil) end describe '#stale?' do it 'returns false for a freshly created request' do - req = OidcRequest.create!(state: 'new-req', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + req = create_request(state: 'new-req') expect(req.stale?).to be false end it 'returns true for a request older than the validity window' do - req = OidcRequest.create!(state: 'old-req', nonce: 'n', code_verifier: 'v', provider: 'google-ncsu') + req = create_request(state: 'old-req') req.update_columns(created_at: 10.minutes.ago) expect(req.stale?).to be true @@ -332,11 +332,10 @@ def stub_token_exchange(email:, email_verified: nil) describe 'stale row cleanup' do def make_stale_requests(count) count.times.map do |i| - req = OidcRequest.create!( + req = create_request( state: "stale-state-#{i}-#{SecureRandom.hex(4)}", nonce: "nonce-#{i}", - code_verifier: "verifier-#{i}", - provider: 'google-ncsu' + code_verifier: "verifier-#{i}" ) req.update_columns(created_at: 10.minutes.ago) req @@ -363,11 +362,10 @@ def make_stale_requests(count) it 'leaves fresh rows untouched while destroying stale ones' do stale = make_stale_requests(3) - fresh = OidcRequest.create!( + fresh = create_request( state: 'fresh-state', nonce: 'fresh-nonce', - code_verifier: 'fresh-verifier', - provider: 'google-ncsu' + code_verifier: 'fresh-verifier' ) stale.each do |req| @@ -389,11 +387,10 @@ def make_stale_requests(count) allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) { cleaned += 1 } total.times do |i| - OidcRequest.create!( + create_request( state: "prob-state-#{i}-#{SecureRandom.hex(4)}", nonce: "nonce-#{i}", - code_verifier: "verifier-#{i}", - provider: 'google-ncsu' + code_verifier: "verifier-#{i}" ) end @@ -405,11 +402,10 @@ def make_stale_requests(count) allow_any_instance_of(OidcRequest).to receive(:rand).and_return(0.99) expect(CleanupStaleOidcRequestsJob).not_to receive(:perform_later) - OidcRequest.create!( + create_request( state: 'no-cleanup-state', nonce: 'nonce', - code_verifier: 'verifier', - provider: 'google-ncsu' + code_verifier: 'verifier' ) end @@ -417,11 +413,10 @@ def make_stale_requests(count) allow_any_instance_of(OidcRequest).to receive(:rand).and_return(0.01) expect(CleanupStaleOidcRequestsJob).to receive(:perform_later).once - OidcRequest.create!( + create_request( state: 'yes-cleanup-state', nonce: 'nonce', - code_verifier: 'verifier', - provider: 'google-ncsu' + code_verifier: 'verifier' ) end end From 00dcb5d143b5eca07e0a2a87c6f6666aa8b27ccc Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Mon, 20 Apr 2026 21:48:19 -0400 Subject: [PATCH 57/74] Test cleanup after username PR. --- spec/models/oidc_request_spec.rb | 99 ++++---------------------------- 1 file changed, 10 insertions(+), 89 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index de689747f..c572ba223 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -187,14 +187,15 @@ def stub_token_exchange(email:, email_verified: nil) end it 'raises InvalidToken when id token verification fails' do - allow(client).to receive(:authorization_code=).with('authorization-code') - allow(client).to receive(:access_token!).with(code_verifier: 'verifier') - .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) - - allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode) - .with('fake.id.token', discovery.jwks) - .and_return(id_token_obj) - allow(id_token_obj).to receive(:verify!) + bad_token = instance_double( + OpenIDConnect::ResponseObject::IdToken, + raw_attributes: { 'email' => 'oidcuser@ncsu.edu' } + ) + allow(client).to receive(:authorization_code=) + allow(client).to receive(:access_token!) + .and_return(instance_double(OpenIDConnect::AccessToken, id_token: 'fake.id.token')) + allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode).and_return(bad_token) + allow(bad_token).to receive(:verify!) .and_raise(OpenIDConnect::ResponseObject::IdToken::InvalidToken.new('nonce mismatch')) expect { @@ -314,69 +315,11 @@ def stub_token_exchange(email:, email_verified: nil) expect(result).to eq(client) end end - describe '#stale?' do - it 'returns false for a freshly created request' do - req = create_request(state: 'new-req') - - expect(req.stale?).to be false - end - - it 'returns true for a request older than the validity window' do - req = create_request(state: 'old-req') - req.update_columns(created_at: 10.minutes.ago) - - expect(req.stale?).to be true - end - end - describe 'stale row cleanup' do - def make_stale_requests(count) - count.times.map do |i| - req = create_request( - state: "stale-state-#{i}-#{SecureRandom.hex(4)}", - nonce: "nonce-#{i}", - code_verifier: "verifier-#{i}" - ) - req.update_columns(created_at: 10.minutes.ago) - req - end - end - before do # Suppress actual cleanup execution - we test DB state directly allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) end - - context 'detect-and-delete on consume_recent_by_state!' do - it 'immediately destroys a handful of stale rows when each is looked up' do - requests = make_stale_requests(5) - - requests.each do |req| - expect { described_class.consume_recent_by_state!(req.state) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - surviving_ids = described_class.where(id: requests.map(&:id)).pluck(:id) - expect(surviving_ids).to be_empty - end - - it 'leaves fresh rows untouched while destroying stale ones' do - stale = make_stale_requests(3) - fresh = create_request( - state: 'fresh-state', - nonce: 'fresh-nonce', - code_verifier: 'fresh-verifier' - ) - - stale.each do |req| - expect { described_class.consume_recent_by_state!(req.state) } - .to raise_error(ActiveRecord::RecordNotFound) - end - - expect(described_class.find_by(id: fresh.id)).to be_present - expect(described_class.where(id: stale.map(&:id)).count).to eq(0) - end - end end describe 'probabilistic cleanup via after_create' do @@ -390,34 +333,12 @@ def make_stale_requests(count) create_request( state: "prob-state-#{i}-#{SecureRandom.hex(4)}", nonce: "nonce-#{i}", - code_verifier: "verifier-#{i}" + verifier: "verifier-#{i}" ) end expect(cleaned).to be > 0, "Expected cleanup to fire at least once in #{total} requests" expect(cleaned).to be < total, "Expected cleanup to be skipped at least once in #{total} requests" end - - it 'does not enqueue CleanupStaleOidcRequestsJob when rand is above the threshold' do - allow_any_instance_of(OidcRequest).to receive(:rand).and_return(0.99) - expect(CleanupStaleOidcRequestsJob).not_to receive(:perform_later) - - create_request( - state: 'no-cleanup-state', - nonce: 'nonce', - code_verifier: 'verifier' - ) - end - - it 'enqueues CleanupStaleOidcRequestsJob when rand is below the threshold' do - allow_any_instance_of(OidcRequest).to receive(:rand).and_return(0.01) - expect(CleanupStaleOidcRequestsJob).to receive(:perform_later).once - - create_request( - state: 'yes-cleanup-state', - nonce: 'nonce', - code_verifier: 'verifier' - ) - end end end From dba74d018752690794658c8651d93058ef1f503f Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Mon, 20 Apr 2026 22:18:52 -0400 Subject: [PATCH 58/74] Addressed copilots feedback. --- spec/models/oidc_request_spec.rb | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index c572ba223..57bd9f909 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -84,7 +84,6 @@ def stub_token_exchange(email:, email_verified: nil) expect { described_class.consume_recent_by_state!('recent-state') } .to raise_error(ActiveRecord::RecordNotFound) - # Expired row is not deleted — only consumed rows are destroyed expect(described_class.find_by(id: recent_request.id)).to be_present end @@ -315,30 +314,4 @@ def stub_token_exchange(email:, email_verified: nil) expect(result).to eq(client) end end - describe 'stale row cleanup' do - before do - # Suppress actual cleanup execution - we test DB state directly - allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) - end - end - - describe 'probabilistic cleanup via after_create' do - it 'enqueues CleanupStaleOidcRequestsJob neither every time nor never over many requests' do - total = 500 - cleaned = 0 - - allow(CleanupStaleOidcRequestsJob).to receive(:perform_later) { cleaned += 1 } - - total.times do |i| - create_request( - state: "prob-state-#{i}-#{SecureRandom.hex(4)}", - nonce: "nonce-#{i}", - verifier: "verifier-#{i}" - ) - end - - expect(cleaned).to be > 0, "Expected cleanup to fire at least once in #{total} requests" - expect(cleaned).to be < total, "Expected cleanup to be skipped at least once in #{total} requests" - end - end end From 5aa5cac02f71660aa4e1d71e749b7031ce94d62d Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 21 Apr 2026 18:49:06 -0400 Subject: [PATCH 59/74] enforce param requirements also in db --- ...0421223609_enforce_oidc_requests_constraints.rb | 12 ++++++++++++ db/schema.rb | 14 +++++++------- spec/models/oidc_request_spec.rb | 8 ++++++++ 3 files changed, 27 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20260421223609_enforce_oidc_requests_constraints.rb diff --git a/db/migrate/20260421223609_enforce_oidc_requests_constraints.rb b/db/migrate/20260421223609_enforce_oidc_requests_constraints.rb new file mode 100644 index 000000000..f0833481e --- /dev/null +++ b/db/migrate/20260421223609_enforce_oidc_requests_constraints.rb @@ -0,0 +1,12 @@ +class EnforceOidcRequestsConstraints < ActiveRecord::Migration[8.0] + def change + change_column_null :oidc_requests, :state, false + change_column_null :oidc_requests, :nonce, false + change_column_null :oidc_requests, :code_verifier, false + change_column_null :oidc_requests, :provider, false + change_column_null :oidc_requests, :username, false + + remove_index :oidc_requests, :state + add_index :oidc_requests, :state, unique: true + end +end diff --git a/db/schema.rb b/db/schema.rb index ffdcc1f0d..54c678a37 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[8.0].define(version: 2026_04_16_184458) do +ActiveRecord::Schema[8.0].define(version: 2026_04_21_223609) do create_table "account_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| t.string "username" t.string "full_name" @@ -241,14 +241,14 @@ end create_table "oidc_requests", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| - t.string "state" - t.string "nonce" - t.string "code_verifier" - t.string "provider" + t.string "state", null: false + t.string "nonce", null: false + t.string "code_verifier", null: false + t.string "provider", null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false - t.string "username" - t.index ["state"], name: "index_oidc_requests_on_state" + t.string "username", null: false + t.index ["state"], name: "index_oidc_requests_on_state", unique: true end create_table "participants", charset: "utf8mb4", collation: "utf8mb4_0900_ai_ci", force: :cascade do |t| diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 06965ad74..e47af0dbc 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -162,6 +162,14 @@ def stub_token_exchange(email:, email_verified: nil) described_class.authorization_uri_for!(provider_key: 'google-ncsu', username: 'oidcuser') end + + it 'raises when creating a request with a duplicate state' do + create_request(state: 'duplicate-state') + + expect { + create_request(state: 'duplicate-state', nonce: 'different-nonce') + }.to raise_error(ActiveRecord::RecordNotUnique) + end end # ─── #verified_email_from_code! ───────────────────────────────────── From 710140ff5b8eb4f02bd8200f5680dc200e8b3252 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 21 Apr 2026 19:37:15 -0400 Subject: [PATCH 60/74] require email verified from idp --- app/models/oidc_request.rb | 4 +--- spec/models/oidc_request_spec.rb | 22 +++++++++++----------- 2 files changed, 12 insertions(+), 14 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 87feac565..353e25ea0 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -75,9 +75,7 @@ def verified_email_from_code!(provider_key:, code:) claims = id_token.raw_attributes - if claims.key?("email_verified") && claims["email_verified"] != true - raise AuthenticationError, "Email not verified by provider" - end + raise AuthenticationError, "Email not verified by provider" unless claims["email_verified"] == true email = claims["email"].to_s.strip raise AuthenticationError, "Email missing from provider response" if email.blank? diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 06965ad74..578f187f6 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -174,7 +174,7 @@ def stub_token_exchange(email:, email_verified: nil) let(:oidc_request) { create_request } it 'exchanges code, verifies token and returns email' do - stub_token_exchange(email: 'oidcuser@ncsu.edu') + stub_token_exchange(email: 'oidcuser@ncsu.edu', email_verified: true) email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') expect(email).to eq('oidcuser@ncsu.edu') @@ -187,11 +187,11 @@ def stub_token_exchange(email:, email_verified: nil) expect(email).to eq('oidcuser@ncsu.edu') end - it 'returns email when provider does not include email_verified claim' do + it 'raises AuthenticationError when email_verified claim is absent' do stub_token_exchange(email: 'oidcuser@ncsu.edu') - email = oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') - expect(email).to eq('oidcuser@ncsu.edu') + expect { oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code: 'authorization-code') } + .to raise_error(OidcRequest::AuthenticationError, /Email not verified/) end it 'raises AuthenticationError when email_verified is false' do @@ -220,7 +220,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'matches user by username and email' do oidc_request = create_request(username: 'OidcUser') - stub_token_exchange(email: 'OidcUser@ncsu.edu') + stub_token_exchange(email: 'OidcUser@ncsu.edu', email_verified: true) result = oidc_request.authenticate_user!(code: 'authorization-code') expect(result.id).to eq(user.id) @@ -228,7 +228,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'matches user case-insensitively on username' do oidc_request = create_request(username: 'oidcuser') - stub_token_exchange(email: 'OidcUser@ncsu.edu') + stub_token_exchange(email: 'OidcUser@ncsu.edu', email_verified: true) result = oidc_request.authenticate_user!(code: 'authorization-code') expect(result.id).to eq(user.id) @@ -236,7 +236,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'matches user case-insensitively on email' do oidc_request = create_request(username: 'OidcUser') - stub_token_exchange(email: 'oidcuser@ncsu.edu') + stub_token_exchange(email: 'oidcuser@ncsu.edu', email_verified: true) result = oidc_request.authenticate_user!(code: 'authorization-code') expect(result.id).to eq(user.id) @@ -244,7 +244,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'matches user case-insensitively on both username and email' do oidc_request = create_request(username: 'OIDCUSER') - stub_token_exchange(email: 'OIDCUSER@NCSU.EDU') + stub_token_exchange(email: 'OIDCUSER@NCSU.EDU', email_verified: true) result = oidc_request.authenticate_user!(code: 'authorization-code') expect(result.id).to eq(user.id) @@ -252,7 +252,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'raises AuthenticationError when email matches but username does not' do oidc_request = create_request(username: 'wronguser') - stub_token_exchange(email: 'OidcUser@ncsu.edu') + stub_token_exchange(email: 'OidcUser@ncsu.edu', email_verified: true) expect { oidc_request.authenticate_user!(code: 'authorization-code') } .to raise_error(OidcRequest::AuthenticationError, /No account found/) @@ -260,7 +260,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'raises AuthenticationError when username matches but email does not' do oidc_request = create_request(username: 'OidcUser') - stub_token_exchange(email: 'different@example.com') + stub_token_exchange(email: 'different@example.com', email_verified: true) expect { oidc_request.authenticate_user!(code: 'authorization-code') } .to raise_error(OidcRequest::AuthenticationError, /No account found/) @@ -268,7 +268,7 @@ def stub_token_exchange(email:, email_verified: nil) it 'raises AuthenticationError when neither username nor email match' do oidc_request = create_request(username: 'nobody') - stub_token_exchange(email: 'nobody@example.com') + stub_token_exchange(email: 'nobody@example.com', email_verified: true) expect { oidc_request.authenticate_user!(code: 'authorization-code') } .to raise_error(OidcRequest::AuthenticationError, /No account found/) From 23c778146868d9bc91285be9aa659a4670da096f Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 21 Apr 2026 19:56:33 -0400 Subject: [PATCH 61/74] normalize user lookup --- app/models/oidc_request.rb | 13 +++++++++++-- spec/models/oidc_request_spec.rb | 17 +++++++++++++++++ 2 files changed, 28 insertions(+), 2 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 87feac565..34c20f733 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -86,10 +86,19 @@ def verified_email_from_code!(provider_key:, code:) end # Matches on both username from input and email from id_token because emails are not unique + # Normalizes whitespace and case to handle legacy data with inconsistent formatting def authenticate_user!(code:) email = verified_email_from_code!(provider_key: provider, code: code) - User.where("LOWER(name) = ? AND LOWER(email) = ?", username.downcase, email.downcase).first || - raise(AuthenticationError, "No account found for #{username} with email #{email}") + raise AuthenticationError, "No email claim in ID token" if email.blank? + raise AuthenticationError, "No username associated with this request" if username.blank? + + normalized_username = username.to_s.strip.downcase + normalized_email = email.to_s.strip.downcase + + User.where( + "LOWER(TRIM(name)) = ? AND LOWER(TRIM(email)) = ?", + normalized_username, normalized_email + ).first || raise(AuthenticationError, "No account found for #{username} with email #{email}") end # Internal: builds an OpenIDConnect::Client from provider config and discovery diff --git a/spec/models/oidc_request_spec.rb b/spec/models/oidc_request_spec.rb index 06965ad74..e19f20eec 100644 --- a/spec/models/oidc_request_spec.rb +++ b/spec/models/oidc_request_spec.rb @@ -273,6 +273,23 @@ def stub_token_exchange(email:, email_verified: nil) expect { oidc_request.authenticate_user!(code: 'authorization-code') } .to raise_error(OidcRequest::AuthenticationError, /No account found/) end + + it 'matches user when DB stores values with leading/trailing whitespace' do + user.update_columns(name: ' OidcUser ', email: ' OidcUser@ncsu.edu ') + oidc_request = create_request(username: 'OidcUser') + stub_token_exchange(email: 'OidcUser@ncsu.edu', email_verified: true) + + result = oidc_request.authenticate_user!(code: 'authorization-code') + expect(result.id).to eq(user.id) + end + + it 'raises AuthenticationError when email is blank' do + oidc_request = create_request(username: 'OidcUser') + stub_token_exchange(email: '', email_verified: true) + + expect { oidc_request.authenticate_user!(code: 'authorization-code') } + .to raise_error(OidcRequest::AuthenticationError, /Email missing/) + end end # ─── .new_client ──────────────────────────────────────────────────── From af73f5cf06bd9f1b03e3ff06e5a241f940219b51 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 21 Apr 2026 20:04:13 -0400 Subject: [PATCH 62/74] update tests --- app/models/oidc_request.rb | 2 +- spec/requests/oidc_login_spec.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 353e25ea0..efa10bdec 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -54,7 +54,7 @@ def self.authorization_uri_for!(provider_key:, username:) # Exchanges the authorization code for tokens, then verifies: # - ID token signature via provider JWKS # - Issuer, audience (client_id), and nonce claims - # - email_verified claim if the provider includes it + # - email_verified claim is always required def verified_email_from_code!(provider_key:, code:) provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index f3881bb08..3f21b71b3 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -38,13 +38,13 @@ def stub_discovery discovery end - def stub_token_exchange(email:) + def stub_token_exchange(email:, email_verified: true) fake_access_token = instance_double(OpenIDConnect::AccessToken, id_token: "fake.id.token") allow_any_instance_of(OpenIDConnect::Client).to receive(:access_token!).and_return(fake_access_token) id_token_obj = instance_double( OpenIDConnect::ResponseObject::IdToken, - raw_attributes: { "email" => email } + raw_attributes: { "email" => email, "email_verified" => email_verified } ) allow(id_token_obj).to receive(:verify!).and_return(true) allow(OpenIDConnect::ResponseObject::IdToken).to receive(:decode).and_return(id_token_obj) From 5b184dc88ff492a60975168974a669595c0102fe Mon Sep 17 00:00:00 2001 From: John Weisz Date: Tue, 21 Apr 2026 20:27:20 -0400 Subject: [PATCH 63/74] update factory --- spec/factories.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/spec/factories.rb b/spec/factories.rb index dde396eb4..7c57f8b65 100644 --- a/spec/factories.rb +++ b/spec/factories.rb @@ -2,10 +2,11 @@ FactoryBot.define do factory :oidc_request do - state { "MyString" } - nonce { "MyString" } - code_verifier { "MyString" } - provider { "MyString" } + sequence(:state) { |n| "state-#{n}-#{SecureRandom.hex(8)}" } + nonce { SecureRandom.hex(32) } + code_verifier { SecureRandom.urlsafe_base64(64) } + provider { "google-ncsu" } + username { "oidcuser" } end factory :student_task do From f48eb15dac219c0e7ee7fd63e07b40988576c3d3 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Tue, 21 Apr 2026 22:52:46 -0400 Subject: [PATCH 64/74] Made yaml test more readable. --- spec/models/oidc_config_spec.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/spec/models/oidc_config_spec.rb b/spec/models/oidc_config_spec.rb index 16c7b45ca..8d517605a 100644 --- a/spec/models/oidc_config_spec.rb +++ b/spec/models/oidc_config_spec.rb @@ -133,7 +133,10 @@ it 'returns an empty hash when the top-level YAML is not a Hash' do allow(File).to receive(:read).and_call_original - allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return("- item_one\n- item_two\n") + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(<<~YAML) + - item_one + - item_two + YAML expect(described_class.providers).to eq({}) expect(Rails.logger).to have_received(:warn).with(/expected top-level mapping/) @@ -141,8 +144,11 @@ it 'returns an empty hash when the providers value is not a Hash' do allow(File).to receive(:read).and_call_original - allow(File).to receive(:read).with(described_class::CONFIG_FILE) - .and_return("providers:\n - item_one\n - item_two\n") + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(<<~YAML) + providers: + - item_one + - item_two + YAML expect(described_class.providers).to eq({}) expect(Rails.logger).to have_received(:warn).with(/expected 'providers' to be a mapping/) From 8d7bd7604d471b0c13862dd1d68a2334bf93dbd5 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Wed, 22 Apr 2026 09:05:28 -0400 Subject: [PATCH 65/74] add comments and resolve nits --- app/controllers/oidc_login_controller.rb | 24 +++++++++---- app/models/oidc_config.rb | 13 +++++++ app/models/oidc_request.rb | 43 ++++++++++++++++++------ app/models/user.rb | 5 +-- config/initializers/oidc.rb | 2 ++ 5 files changed, 68 insertions(+), 19 deletions(-) diff --git a/app/controllers/oidc_login_controller.rb b/app/controllers/oidc_login_controller.rb index 78f01649d..e3d1dbdac 100644 --- a/app/controllers/oidc_login_controller.rb +++ b/app/controllers/oidc_login_controller.rb @@ -1,28 +1,36 @@ class OidcLoginController < ApplicationController + # OIDC login does not require an existing session — this is how users sign in. skip_before_action :authenticate_request! + # Unknown provider keys from the frontend return 404 with the specific message. rescue_from OidcConfig::ProviderNotFound do |e| render_error e.message, status: :not_found end + # IdP discovery or token endpoint unreachable — surface as 502 Bad Gateway + # so the frontend can distinguish provider outages from user-facing errors. rescue_from OpenIDConnect::Discovery::DiscoveryFailed, Rack::OAuth2::Client::Error do |e| render_error "Provider communication failed: #{e.message}", status: :bad_gateway end + # Missing required params (from params.require) return 400 with the param name. rescue_from ActionController::ParameterMissing do |e| render_error e.message, status: :bad_request end # GET /auth/providers - # Returns only public info (id, name) — no secrets or endpoints + # Returns the list of configured OIDC providers for the frontend dropdown. + # Only public info (id, name) — no secrets or endpoint URLs. def providers render json: OidcConfig.public_list end # POST /auth/client-select - # Username is required because emails are not unique - # Provider only knows email - # This is a good candidate for rate limiting + # Initiates an OIDC login. The frontend calls this with the chosen provider + # and the user's Expertiza username, and receives an authorization URL to + # redirect the browser to. Username is required here because the IdP only + # returns an email claim, and Expertiza emails are not unique across accounts. + # Candidate for rate limiting — it creates a DB row and triggers provider discovery. def client_select provider = params.require(:provider) username = params.require(:username) @@ -35,8 +43,11 @@ def client_select end # POST /auth/callback - # Returns a generic error for all failure modes to avoid leaking - # whether the state, user, or token verification was the cause + # Completes an OIDC login after the IdP redirects the user back to the frontend. + # Consumes the stored OidcRequest by state, verifies the ID token, matches a + # local user, and issues a session JWT. + # Returns a generic 401 "Authentication failed" for all verification or matching + # failures to avoid leaking which check failed (state, token, user match, etc.). def callback state = params.require(:state) code = params.require(:code) @@ -52,6 +63,7 @@ def callback private + # Standardizes the JSON error response shape across all endpoints. def render_error(message, status:) render json: { error: message }, status: status end diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index b1bd51726..a3480dee8 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -4,6 +4,9 @@ class ProviderNotFound < StandardError; end CONFIG_FILE = Rails.root.join("config", "oidc_providers.yml").freeze REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri].freeze + # Loads, parses, and validates the OIDC provider YAML file. + # Memoized per process — call reload! to force a re-read. + # Invalid providers are skipped with a warning rather than crashing the app. def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result @@ -28,20 +31,28 @@ def self.providers end end + # Looks up a provider config by its key (e.g. "google-ncsu"). + # Raises ProviderNotFound if the key is not configured. def self.find(provider_key) providers.fetch(provider_key) do raise ProviderNotFound, "Unknown OIDC provider: #{provider_key}" end end + # Returns the list of providers safe to expose to the frontend. + # Only includes display information — never secrets or endpoint URLs. def self.public_list providers.map { |key, cfg| { id: key, name: cfg["display_name"] } } end + # Clears the memoized config so the next call to `providers` re-reads the YAML file. + # Primarily useful for tests and hot-reloading in development. def self.reload! @providers = nil end + # Parses the provider's scopes string (whitespace or comma-separated) into an array. + # Falls back to the default OIDC scopes if none are configured. def self.scopes_for(provider) raw = provider["scopes"].to_s.split(/[\s,]+/).reject(&:blank?) raw.presence || %w[openid email profile] @@ -49,6 +60,8 @@ def self.scopes_for(provider) private + # Removes providers missing any REQUIRED_KEYS and logs a warning for each. + # Mutates the provided hash so that `providers` returns only valid entries. def self.validate!(providers) providers.reject! do |key, cfg| missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } diff --git a/app/models/oidc_request.rb b/app/models/oidc_request.rb index 2c2f03fb8..25d55291a 100644 --- a/app/models/oidc_request.rb +++ b/app/models/oidc_request.rb @@ -1,17 +1,26 @@ class OidcRequest < ApplicationRecord + # Raised for any authentication failure (missing claim, unverified email, no matching user). + # The controller rescues this and returns a generic 401 to avoid leaking which check failed. class AuthenticationError < StandardError; end + # How long a newly-created auth request is considered valid before being treated as stale. VALIDITY_WINDOW = 5.minutes + + # Probability (0.0–1.0) of triggering a stale-cleanup job on each successful create. + # Amortizes cleanup cost without requiring a dedicated scheduler. CLEANUP_PROBABILITY = 0.10 after_create :maybe_enqueue_stale_cleanup + # Deletes all auth requests older than the validity window. + # Called by CleanupStaleOidcRequestsJob; safe to invoke directly for manual cleanup. def self.delete_stale where("created_at <= ?", VALIDITY_WINDOW.ago).delete_all end - # Atomically finds and deletes the request to prevent replay attacks - # Raises RecordNotFound if entry does not exist + # Atomically finds and destroys the request matching the given state to prevent + # replay attacks. Only considers requests within the validity window. + # Raises ActiveRecord::RecordNotFound if no matching recent request exists. def self.consume_recent_by_state!(state) transaction do request = where("created_at > ?", VALIDITY_WINDOW.ago).lock.find_by!(state: state) @@ -20,8 +29,12 @@ def self.consume_recent_by_state!(state) end end - # Generates PKCE, state, and nonce — stores them for callback verification - # PKCE is always sent; providers that don't support it will ignore the extra params + # Starts an OIDC login for the given provider and username: + # - Performs provider discovery + # - Generates fresh state, nonce, and PKCE values + # - Persists an OidcRequest row so the callback can verify and consume it + # Returns the authorization URL the frontend should redirect the user to. + # PKCE is always sent; providers that don't support it will ignore the extra params. def self.authorization_uri_for!(provider_key:, username:) provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) @@ -51,10 +64,12 @@ def self.authorization_uri_for!(provider_key:, username:) ) end - # Exchanges the authorization code for tokens, then verifies: - # - ID token signature via provider JWKS + # Exchanges the authorization code for tokens and verifies the ID token: + # - Signature via provider JWKS # - Issuer, audience (client_id), and nonce claims - # - email_verified claim is always required + # - email_verified claim must be explicitly true + # Returns the user's email from the ID token claims. + # Raises AuthenticationError if the email is unverified or missing. def verified_email_from_code!(provider_key:, code:) provider = OidcConfig.find(provider_key) discovery = OpenIDConnect::Discovery::Provider::Config.discover!(provider["issuer"]) @@ -83,8 +98,11 @@ def verified_email_from_code!(provider_key:, code:) email end - # Matches on both username from input and email from id_token because emails are not unique - # Normalizes whitespace and case to handle legacy data with inconsistent formatting + # Verifies the OIDC callback and resolves it to a local user. + # Matches on both the stored username and the verified email claim because + # emails are not unique in Expertiza. Whitespace and case are normalized on + # both sides to handle legacy data with inconsistent formatting. + # Raises AuthenticationError if no matching user is found. def authenticate_user!(code:) email = verified_email_from_code!(provider_key: provider, code: code) raise AuthenticationError, "No email claim in ID token" if email.blank? @@ -99,7 +117,8 @@ def authenticate_user!(code:) ).first || raise(AuthenticationError, "No account found for #{username} with email #{email}") end - # Internal: builds an OpenIDConnect::Client from provider config and discovery + # Internal: builds an OpenIDConnect::Client from provider config and discovery. + # Used by both authorization_uri_for! and verified_email_from_code!. def self.new_client(provider, discovery:) OpenIDConnect::Client.new( identifier: provider["client_id"], @@ -113,7 +132,9 @@ def self.new_client(provider, discovery:) private + # after_create callback. Probabilistically enqueues a cleanup job for stale rows. + # Runs inline since it's non-blocking (just pushes to the job queue). def maybe_enqueue_stale_cleanup CleanupStaleOidcRequestsJob.perform_later if rand < CLEANUP_PROBABILITY end -end \ No newline at end of file +end diff --git a/app/models/user.rb b/app/models/user.rb index a8bc313b9..566e5cadc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + require 'json_web_token' class User < ApplicationRecord @@ -156,8 +157,8 @@ def generate_jwt(expiry = 24.hours.from_now) id: id, name: name, full_name: full_name, - role: role.name, - institution_id: institution.id + role: role&.name, + institution_id: institution&.id } JsonWebToken.encode(payload, expiry) end diff --git a/config/initializers/oidc.rb b/config/initializers/oidc.rb index 419b95f1f..fe5b381e7 100644 --- a/config/initializers/oidc.rb +++ b/config/initializers/oidc.rb @@ -1,3 +1,5 @@ Rails.application.config.after_initialize do OidcConfig.providers +rescue Errno::ENOENT + Rails.logger.info("OIDC config file not found; OIDC disabled") end \ No newline at end of file From a8354152d4766068b5fad19aecd1e23cb390e25c Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Wed, 22 Apr 2026 21:46:11 -0400 Subject: [PATCH 66/74] Added Ruby Gem --- Gemfile | 1 + 1 file changed, 1 insertion(+) diff --git a/Gemfile b/Gemfile index 7cd218362..8fa1f42f2 100644 --- a/Gemfile +++ b/Gemfile @@ -59,6 +59,7 @@ gem 'find_with_order' gem 'rubyzip' gem 'openid_connect' +gem 'rack-attack' group :development, :test do gem 'debug', platforms: %i[mri mingw x64_mingw] From 69e09dfd42fa8e600de047b9981007089a9307c1 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Wed, 22 Apr 2026 21:54:48 -0400 Subject: [PATCH 67/74] updated gemfile.lock --- Gemfile.lock | 3 +++ 1 file changed, 3 insertions(+) diff --git a/Gemfile.lock b/Gemfile.lock index 070284ddc..caef7c813 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -284,6 +284,8 @@ GEM nio4r (~> 2.0) racc (1.8.1) rack (3.2.1) + rack-attack (6.8.0) + rack (>= 1.0, < 4) rack-cors (3.0.0) logger rack (>= 3.0.14) @@ -485,6 +487,7 @@ DEPENDENCIES ostruct psych (~> 5.2) puma (~> 6.4) + rack-attack rack-cors rails (~> 8.0, >= 8.0.1) rspec-rails From 60b014ebe037fee71f77cc6c7736e407c567a6ef Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Wed, 22 Apr 2026 22:02:21 -0400 Subject: [PATCH 68/74] Rack attack rules --- config/initializers/rack_attack.rb | 54 ++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 config/initializers/rack_attack.rb diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb new file mode 100644 index 000000000..2a11a5875 --- /dev/null +++ b/config/initializers/rack_attack.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +# Rack::Attack middleware configuration for OIDC login rate limiting. +# Uses a dedicated MemoryStore in test so the null_store in test.rb doesn't +# interfere, and relies on Rails.cache (Redis) in all other environments. +if Rails.env.test? + Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new +else + Rack::Attack.cache.store = Rails.cache +end + +# ── Throttles ───────────────────────────────────────────────────────────────── + +# Limit authorization initiation to 5 requests per minute per IP. +# This endpoint creates a DB row and triggers OIDC provider discovery, making +# it expensive and a prime target for abuse. +Rack::Attack.throttle("oidc/client-select/ip", limit: 5, period: 60) do |req| + req.ip if req.post? && req.path == "/auth/client-select" +end + +# Tighten the limit further per IP+username combination to prevent an attacker +# from targeting a specific account across retries. +Rack::Attack.throttle("oidc/client-select/ip+username", limit: 3, period: 60) do |req| + if req.post? && req.path == "/auth/client-select" + body = req.body.read + req.body.rewind + params = JSON.parse(body) rescue {} + username = params["username"].to_s.strip + "#{req.ip}:#{username}" unless username.empty? + end +end + +# Limit callback completions to 10 requests per minute per IP. +# Protects against code-reuse replay attempts and brute-force state guessing. +Rack::Attack.throttle("oidc/callback/ip", limit: 10, period: 60) do |req| + req.ip if req.post? && req.path == "/auth/callback" +end + +# ── Throttled response ───────────────────────────────────────────────────────── + +# Return JSON-formatted 429 responses consistent across OidcLoginController. +Rack::Attack.throttled_responder = lambda do |req| + match_data = req.env["rack.attack.match_data"] + retry_after = match_data ? (match_data[:period] - (Time.now.to_i % match_data[:period])) : 60 + + [ + 429, + { + "Content-Type" => "application/json", + "Retry-After" => retry_after.to_s + }, + [{ error: "Rate limit exceeded. Try again later." }.to_json] + ] +end From d2840f2dae3cabbf9d1ee2f45994b33c75e58993 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Wed, 22 Apr 2026 22:15:23 -0400 Subject: [PATCH 69/74] Test coverage Co-authored-by: Copilot --- spec/requests/oidc_login_spec.rb | 90 ++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 4d3b7b98e..3171e7645 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -407,4 +407,94 @@ def create_oidc_request(state:, provider: "google-ncsu", username: "oidcuser") end end end + + # ─── Rate limiting (Rack::Attack) ─────────────────────────────────────────── + + describe 'rate limiting' do + before do + # Reset the Rack::Attack cache between each test so throttle counters + # don't bleed across examples. + Rack::Attack.cache.store.clear + end + + describe 'POST /auth/client-select' do + let(:valid_params) { { provider: "google-ncsu", username: "oidcuser" }.to_json } + let(:headers) { { "CONTENT_TYPE" => "application/json", "REMOTE_ADDR" => "1.2.3.4" } } + + before do + allow(OidcRequest).to receive(:authorization_uri_for!) + .and_return("https://accounts.google.com/o/oauth2/v2/auth?scope=openid+email+profile") + end + + it 'allows requests within the per-IP limit' do + # Use distinct usernames so the tighter ip+username throttle (limit: 3) + # is not triggered — this test is specifically for the per-IP limit (5). + 5.times do |i| + params = { provider: "google-ncsu", username: "user#{i}" }.to_json + post '/auth/client-select', params: params, headers: headers + expect(response).not_to have_http_status(:too_many_requests) + end + end + + it 'throttles requests that exceed the per-IP limit' do + 5.times { post '/auth/client-select', params: valid_params, headers: headers } + post '/auth/client-select', params: valid_params, headers: headers + expect(response).to have_http_status(:too_many_requests) + json = JSON.parse(response.body) + expect(json["error"]).to match(/Rate limit exceeded/) + end + + it 'throttles requests that exceed the per-IP+username limit' do + same_user_params = { provider: "google-ncsu", username: "targeted_user" }.to_json + 3.times { post '/auth/client-select', params: same_user_params, headers: headers } + post '/auth/client-select', params: same_user_params, headers: headers + expect(response).to have_http_status(:too_many_requests) + json = JSON.parse(response.body) + expect(json["error"]).to match(/Rate limit exceeded/) + end + + it 'returns a Retry-After header when throttled' do + 5.times { post '/auth/client-select', params: valid_params, headers: headers } + post '/auth/client-select', params: valid_params, headers: headers + expect(response.headers["Retry-After"]).to be_present + end + + it 'does not throttle requests from different IPs independently' do + 5.times { post '/auth/client-select', params: valid_params, headers: headers } + + other_headers = headers.merge("REMOTE_ADDR" => "9.8.7.6") + post '/auth/client-select', params: valid_params, headers: other_headers + expect(response).not_to have_http_status(:too_many_requests) + end + end + + describe 'POST /auth/callback' do + let(:headers) { { "CONTENT_TYPE" => "application/json", "REMOTE_ADDR" => "1.2.3.4" } } + + before do + allow(OidcRequest).to receive(:consume_recent_by_state!).and_raise(ActiveRecord::RecordNotFound) + end + + it 'allows requests within the per-IP limit' do + 10.times do + post '/auth/callback', params: { state: "s", code: "c" }.to_json, headers: headers + expect(response).not_to have_http_status(:too_many_requests) + end + end + + it 'throttles requests that exceed the per-IP limit' do + 10.times { post '/auth/callback', params: { state: "s", code: "c" }.to_json, headers: headers } + post '/auth/callback', params: { state: "s", code: "c" }.to_json, headers: headers + expect(response).to have_http_status(:too_many_requests) + json = JSON.parse(response.body) + expect(json["error"]).to match(/Rate limit exceeded/) + end + + it 'returns a Retry-After header when throttled' do + 10.times { post '/auth/callback', params: { state: "s", code: "c" }.to_json, headers: headers } + post '/auth/callback', params: { state: "s", code: "c" }.to_json, headers: headers + expect(response.headers["Retry-After"]).to be_present + end + end + end end \ No newline at end of file From 4157b68231baaec5bb63e9cc0dd45e3c4834ba03 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Thu, 23 Apr 2026 07:08:16 -0400 Subject: [PATCH 70/74] Address PR comments Co-authored-by: Copilot --- config/initializers/rack_attack.rb | 13 ++++++++++--- spec/requests/oidc_login_spec.rb | 2 +- 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 2a11a5875..4cfced6b9 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -2,7 +2,10 @@ # Rack::Attack middleware configuration for OIDC login rate limiting. # Uses a dedicated MemoryStore in test so the null_store in test.rb doesn't -# interfere, and relies on Rails.cache (Redis) in all other environments. +# interfere, and relies on Rails.cache as configured in other environments. +# Note: Rails.cache is configured with raise_errors: false in this app +# (config/application.rb), so a Redis outage will silently drop counters and +# cause throttles to fail open. Monitor Redis availability accordingly. if Rails.env.test? Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new else @@ -24,8 +27,12 @@ if req.post? && req.path == "/auth/client-select" body = req.body.read req.body.rewind - params = JSON.parse(body) rescue {} - username = params["username"].to_s.strip + params = begin + JSON.parse(body) + rescue JSON::ParserError, TypeError + {} + end + username = params["username"].to_s.strip.downcase "#{req.ip}:#{username}" unless username.empty? end end diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 3171e7645..65b991e47 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -459,7 +459,7 @@ def create_oidc_request(state:, provider: "google-ncsu", username: "oidcuser") expect(response.headers["Retry-After"]).to be_present end - it 'does not throttle requests from different IPs independently' do + it 'throttles requests independently per IP (different IPs are not affected by each other)' do 5.times { post '/auth/client-select', params: valid_params, headers: headers } other_headers = headers.merge("REMOTE_ADDR" => "9.8.7.6") From 8254bd2fce921f1f8f041124ed35326d96ccf809 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Thu, 23 Apr 2026 09:53:50 -0400 Subject: [PATCH 71/74] provider fail on startup when prod --- app/models/oidc_config.rb | 29 +++++++++++++----------- spec/models/oidc_config_spec.rb | 39 +++++++++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 13 deletions(-) diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index a3480dee8..97258d35f 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -1,29 +1,27 @@ class OidcConfig class ProviderNotFound < StandardError; end + class InvalidConfiguration < StandardError; end CONFIG_FILE = Rails.root.join("config", "oidc_providers.yml").freeze REQUIRED_KEYS = %w[display_name issuer client_id client_secret redirect_uri].freeze # Loads, parses, and validates the OIDC provider YAML file. # Memoized per process — call reload! to force a re-read. - # Invalid providers are skipped with a warning rather than crashing the app. + # In production, invalid config raises InvalidConfiguration to block startup. + # In other environments, invalid providers are skipped with a warning. def self.providers @providers ||= begin yaml = ERB.new(File.read(CONFIG_FILE)).result parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true) unless parsed.is_a?(Hash) - Rails.logger.warn( - "OIDC config ignored: expected top-level mapping in #{CONFIG_FILE}, got #{parsed.class}" - ) + handle_invalid("OIDC config: expected top-level mapping in #{CONFIG_FILE}, got #{parsed.class}") parsed = {} end providers = parsed["providers"] unless providers.is_a?(Hash) - Rails.logger.warn( - "OIDC config ignored: expected 'providers' to be a mapping in #{CONFIG_FILE}, got #{providers.class}" - ) + handle_invalid("OIDC config: expected 'providers' to be a mapping in #{CONFIG_FILE}, got #{providers.class}") providers = {} end @@ -58,20 +56,25 @@ def self.scopes_for(provider) raw.presence || %w[openid email profile] end - private - - # Removes providers missing any REQUIRED_KEYS and logs a warning for each. - # Mutates the provided hash so that `providers` returns only valid entries. + # Removes providers missing any REQUIRED_KEYS. + # In production, raises InvalidConfiguration to prevent startup with misconfigured providers. + # In other environments, logs a warning and skips the provider. def self.validate!(providers) providers.reject! do |key, cfg| missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? - Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}") + handle_invalid("OIDC provider '#{key}' invalid: missing #{missing.join(', ')}") true end end providers end - private_class_method :validate! + # Raises in production to block startup; logs a warning elsewhere. + def self.handle_invalid(message) + raise InvalidConfiguration, message if Rails.env.production? + Rails.logger.warn(message) + end + + private_class_method :validate!, :handle_invalid end \ No newline at end of file diff --git a/spec/models/oidc_config_spec.rb b/spec/models/oidc_config_spec.rb index 8d517605a..c786995e3 100644 --- a/spec/models/oidc_config_spec.rb +++ b/spec/models/oidc_config_spec.rb @@ -249,4 +249,43 @@ expect(described_class.scopes_for(provider)).to eq(%w[openid email profile]) end end + + describe 'production behavior' do + before do + allow(Rails.env).to receive(:production?).and_return(true) + end + + it 'raises InvalidConfiguration when a provider is missing required keys' do + yaml = <<~YAML + providers: + bad: + display_name: Bad + issuer: https://issuer.example.com + client_id: id + redirect_uri: http://localhost:3000/auth/callback + YAML + + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return(yaml) + + expect { described_class.providers } + .to raise_error(OidcConfig::InvalidConfiguration, /missing client_secret/) + end + + it 'raises InvalidConfiguration when the top-level YAML is not a Hash' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return("- item_one\n") + + expect { described_class.providers } + .to raise_error(OidcConfig::InvalidConfiguration, /expected top-level mapping/) + end + + it 'raises InvalidConfiguration when the providers value is not a Hash' do + allow(File).to receive(:read).and_call_original + allow(File).to receive(:read).with(described_class::CONFIG_FILE).and_return("providers:\n - item_one\n") + + expect { described_class.providers } + .to raise_error(OidcConfig::InvalidConfiguration, /expected 'providers' to be a mapping/) + end + end end From 0f5395cf5356efed7ab55483e65750945f8a4f44 Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Thu, 23 Apr 2026 20:05:17 -0400 Subject: [PATCH 72/74] Trimmed IP+username limiter since its benefit was marginal. --- config/initializers/rack_attack.rb | 16 ---------------- spec/requests/oidc_login_spec.rb | 16 ++-------------- 2 files changed, 2 insertions(+), 30 deletions(-) diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index 4cfced6b9..b6b60071a 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -21,22 +21,6 @@ req.ip if req.post? && req.path == "/auth/client-select" end -# Tighten the limit further per IP+username combination to prevent an attacker -# from targeting a specific account across retries. -Rack::Attack.throttle("oidc/client-select/ip+username", limit: 3, period: 60) do |req| - if req.post? && req.path == "/auth/client-select" - body = req.body.read - req.body.rewind - params = begin - JSON.parse(body) - rescue JSON::ParserError, TypeError - {} - end - username = params["username"].to_s.strip.downcase - "#{req.ip}:#{username}" unless username.empty? - end -end - # Limit callback completions to 10 requests per minute per IP. # Protects against code-reuse replay attempts and brute-force state guessing. Rack::Attack.throttle("oidc/callback/ip", limit: 10, period: 60) do |req| diff --git a/spec/requests/oidc_login_spec.rb b/spec/requests/oidc_login_spec.rb index 65b991e47..d8ab0ba52 100644 --- a/spec/requests/oidc_login_spec.rb +++ b/spec/requests/oidc_login_spec.rb @@ -427,11 +427,8 @@ def create_oidc_request(state:, provider: "google-ncsu", username: "oidcuser") end it 'allows requests within the per-IP limit' do - # Use distinct usernames so the tighter ip+username throttle (limit: 3) - # is not triggered — this test is specifically for the per-IP limit (5). - 5.times do |i| - params = { provider: "google-ncsu", username: "user#{i}" }.to_json - post '/auth/client-select', params: params, headers: headers + 5.times do + post '/auth/client-select', params: valid_params, headers: headers expect(response).not_to have_http_status(:too_many_requests) end end @@ -444,15 +441,6 @@ def create_oidc_request(state:, provider: "google-ncsu", username: "oidcuser") expect(json["error"]).to match(/Rate limit exceeded/) end - it 'throttles requests that exceed the per-IP+username limit' do - same_user_params = { provider: "google-ncsu", username: "targeted_user" }.to_json - 3.times { post '/auth/client-select', params: same_user_params, headers: headers } - post '/auth/client-select', params: same_user_params, headers: headers - expect(response).to have_http_status(:too_many_requests) - json = JSON.parse(response.body) - expect(json["error"]).to match(/Rate limit exceeded/) - end - it 'returns a Retry-After header when throttled' do 5.times { post '/auth/client-select', params: valid_params, headers: headers } post '/auth/client-select', params: valid_params, headers: headers From 8aee801fb59a1b72a50b8901b82e5db4fd70cc8a Mon Sep 17 00:00:00 2001 From: JaredM2028 Date: Thu, 23 Apr 2026 20:18:14 -0400 Subject: [PATCH 73/74] Use MemoryStore for Rack::Attack cache in test and development environments Rails.cache defaults to :null_store in both test and development, which silently discards all throttle counters and makes rate limiting non-functional; switching to MemoryStore ensures counters accumulate correctly without requiring a Redis dependency in local or CI environments. Co-authored-by: Copilot --- config/initializers/rack_attack.rb | 20 ++++++++++++-------- 1 file changed, 12 insertions(+), 8 deletions(-) diff --git a/config/initializers/rack_attack.rb b/config/initializers/rack_attack.rb index b6b60071a..32c0ea989 100644 --- a/config/initializers/rack_attack.rb +++ b/config/initializers/rack_attack.rb @@ -1,15 +1,19 @@ # frozen_string_literal: true # Rack::Attack middleware configuration for OIDC login rate limiting. -# Uses a dedicated MemoryStore in test so the null_store in test.rb doesn't -# interfere, and relies on Rails.cache as configured in other environments. -# Note: Rails.cache is configured with raise_errors: false in this app -# (config/application.rb), so a Redis outage will silently drop counters and -# cause throttles to fail open. Monitor Redis availability accordingly. -if Rails.env.test? - Rack::Attack.cache.store = ActiveSupport::Cache::MemoryStore.new +# +# Cache store strategy: +# test/development — MemoryStore: always available, no Redis required. +# test.rb sets Rails.cache to :null_store (counters would never accumulate), +# and development uses :null_store by default, so we use MemoryStore directly. +# production — Rails.cache (Redis): shared across requests within a process. +# Note: configured with raise_errors: false in this app, so a Redis outage +# will silently drop counters and cause throttles to fail open. Monitor +# Redis availability accordingly. +Rack::Attack.cache.store = if Rails.env.production? + Rails.cache else - Rack::Attack.cache.store = Rails.cache + ActiveSupport::Cache::MemoryStore.new end # ── Throttles ───────────────────────────────────────────────────────────────── From 306f67e3ab36b21f5bd550a010208b6304648450 Mon Sep 17 00:00:00 2001 From: John Weisz Date: Fri, 24 Apr 2026 08:20:52 -0400 Subject: [PATCH 74/74] refine handling --- app/models/oidc_config.rb | 16 +++++++++++++--- 1 file changed, 13 insertions(+), 3 deletions(-) diff --git a/app/models/oidc_config.rb b/app/models/oidc_config.rb index 97258d35f..b9e7bfe22 100644 --- a/app/models/oidc_config.rb +++ b/app/models/oidc_config.rb @@ -19,7 +19,7 @@ def self.providers parsed = {} end - providers = parsed["providers"] + providers = parsed["providers"] || {} unless providers.is_a?(Hash) handle_invalid("OIDC config: expected 'providers' to be a mapping in #{CONFIG_FILE}, got #{providers.class}") providers = {} @@ -52,8 +52,13 @@ def self.reload! # Parses the provider's scopes string (whitespace or comma-separated) into an array. # Falls back to the default OIDC scopes if none are configured. def self.scopes_for(provider) - raw = provider["scopes"].to_s.split(/[\s,]+/).reject(&:blank?) - raw.presence || %w[openid email profile] + raw = provider["scopes"] + list = case raw + when Array then raw.map { |s| s.to_s.strip } + when String then raw.split(/[\s,]+/) + else [] + end + list.reject(&:blank?).presence || %w[openid email profile] end # Removes providers missing any REQUIRED_KEYS. @@ -61,6 +66,11 @@ def self.scopes_for(provider) # In other environments, logs a warning and skips the provider. def self.validate!(providers) providers.reject! do |key, cfg| + unless cfg.is_a?(Hash) + handle_invalid("OIDC provider '#{key}' invalid: expected mapping, got #{cfg.class}") + next true + end + missing = REQUIRED_KEYS.select { |k| cfg[k].blank? } if missing.any? handle_invalid("OIDC provider '#{key}' invalid: missing #{missing.join(', ')}")