Skip to content

E2618 - OIDC login#335

Open
johnmweisz wants to merge 97 commits into
expertiza:mainfrom
johnmweisz:2618-oidc-login
Open

E2618 - OIDC login#335
johnmweisz wants to merge 97 commits into
expertiza:mainfrom
johnmweisz:2618-oidc-login

Conversation

@johnmweisz
Copy link
Copy Markdown

@johnmweisz johnmweisz commented Apr 20, 2026

Expertiza currently authenticates users with its own login page, implemented by the Expertiza application. Expertiza has been used at many campuses, however, and each has their own SSO (single signon) protocol that students and staff use to log into other applications. Supporting these standard protocols at sites where they are in use is more secure for the application, provides a familiar and streamlined login experience, and frees Expertiza from managing credentials for users whose institution already does so. This design introduces OIDC login as an additional authentication option alongside the existing username and password login. Both methods will continue to be supported, allowing users to choose their preferred approach.

Wiki: https://wiki.expertiza.ncsu.edu/index.php?title=CSC/ECE_517_Spring_2026_-_E2618._Support_OIDC_Logins

johnmweisz and others added 30 commits April 7, 2026 16:36
Added seed data for three users with specific attributes.
Removed the callback_redirect method for handling IdP redirects directly.
remove discovery option, assume always true to match OIDC spec
- 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>
… 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>
…ion-provider-endpoints

Add Swagger/OpenAPI documentation for OIDC provider endpoints
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 7

🧹 Nitpick comments (3)
Gemfile (1)

61-61: Pin openid_connect to a tested version range.

Line 61 leaves the dependency unconstrained. While the latest stable version (2.3.1) is fully compatible with Ruby 3.4.x, pinning to a tested version range following your verification or bundle lock prevents unexpected behavior changes on future upgrades. Use a pessimistic constraint (e.g., ~> 2.3.0) aligned to your tested lockfile version.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@Gemfile` at line 61, The Gemfile currently lists an unconstrained dependency
gem 'openid_connect'; update this to a pinned, pessimistic version constraint
(for example use gem 'openid_connect', '~> 2.3.0' or the exact range matching
your tested Gemfile.lock) to prevent accidental upgrades—modify the line
containing gem 'openid_connect' to include the chosen version constraint and run
bundle install to verify dependency resolution.
swagger/v1/swagger.yaml (1)

1343-1350: Consider adding maxItems constraint to topic_ids array parameter.

Unbounded arrays in API inputs can be exploited for denial-of-service. Adding a reasonable maximum helps protect against abuse.

♻️ Add maxItems constraint
       - name: topic_ids
         in: query
         items:
           type: string
         description: Topic Identifiers to delete
         required: false
         schema:
           type: array
+          maxItems: 100
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@swagger/v1/swagger.yaml` around lines 1343 - 1350, The query parameter
"topic_ids" defines an unbounded array which is a DoS risk; modify the OpenAPI
parameter definition for topic_ids (the parameter named topic_ids) to include a
sensible maxItems constraint (e.g., add "maxItems: 100" under its schema) so the
array is capped, keeping its existing items.type: string and other metadata
unchanged.
app/models/oidc_config.rb (1)

52-61: Consider whether validate! should be non-mutating.

reject! mutates the providers hash in place. This works since the hash originates from YAML.safe_load within the same memoization block, but returning a filtered copy would be more defensive if the loading logic ever changes.

♻️ Optional: Non-mutating alternative
 def self.validate!(providers)
-  providers.reject! do |key, cfg|
+  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(', ')}")
       true
     end
   end
-  providers
 end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/oidc_config.rb` around lines 52 - 61, The method validate!
currently mutates its argument via providers.reject!, make it non-mutating by
producing and returning a filtered copy instead: iterate over the input
providers (using providers.reject or providers.each_with_object) to build a new
hash that skips entries with missing REQUIRED_KEYS while still logging with
Rails.logger.warn for each skipped provider; ensure the method returns the new
filtered hash and does not change the original providers object, referencing the
validate! method and REQUIRED_KEYS constant to locate the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/models/user.rb`:
- Line 2: Add a single blank line after the file's magic comment (e.g., "#
frozen_string_literal: true") so the require 'json_web_token' statement is
separated from the magic comment, fixing the Layout/EmptyLineAfterMagicComment
violation; locate the top of the file and insert the blank line immediately
after the magic comment above the require 'json_web_token' line.
- Line 160: The JWT payload construction in the User model uses institution.id
directly (institution_id: institution.id) which can raise NoMethodError when
institution is nil; update the User model's JWT payload method (where
institution_id is set) to safely access the association (e.g., institution&.id
or institution.try(:id)) so institution_id becomes nil when absent rather than
raising an error, and ensure any callers that expect an integer handle nil
appropriately.

In `@config/initializers/oidc.rb`:
- Around line 1-2: The call to OidcConfig.providers in
Rails.application.config.after_initialize can raise Errno::ENOENT if
config/oidc_providers.yml is missing; wrap the OidcConfig.providers invocation
in a begin/rescue (or use inline rescue) that catches Errno::ENOENT and calls
Rails.logger.info("OIDC config file not found; OIDC disabled") so the app boots
without OIDC instead of crashing.

In `@db/migrate/20260421223609_enforce_oidc_requests_constraints.rb`:
- Around line 3-10: Migration enforces NOT NULL on
:state/:nonce/:code_verifier/:provider/:username and replaces the :state index
with a unique one, but dropping the index before cleaning data can break
non-transactional DDL; instead, first backfill/fix existing rows for
oidc_requests (set non-null defaults and deduplicate state values), then create
the new unique index (using a new name or "CONCURRENTLY"/the DB-specific safe
method) and only after that remove the old index if necessary, finally apply
change_column_null; update the migration around change_column_null,
remove_index, and add_index to perform data cleanup and safe unique-index
creation for the state column before making NOT NULL changes.

In `@spec/factories.rb`:
- Around line 4-9: The oidc_request factory currently hardcodes state (causing
unique index collisions) and omits the now-required username column; update the
factory :oidc_request to generate a unique state (use FactoryBot.sequence or a
faker/random generator for the state attribute) and add a username attribute
that supplies a valid, non-nil string (e.g., sequence or association to a user
email/username). Ensure other attributes (nonce, code_verifier, provider) remain
unchanged unless tests require variability.

In `@spec/requests/authentication_spec.rb`:
- Line 75: The file ends with a final "end" token but is missing a trailing
newline; add a single newline character at the end of
spec/requests/authentication_spec.rb so the file terminates with a final blank
line (i.e., ensure the file ends with "\n" after the final end).

In `@swagger/v1/swagger.yaml`:
- Around line 721-722: The OpenAPI security references use the wrong case:
replace all occurrences of the security requirement key "bearer_auth" with the
defined scheme name "bearerAuth" so the security entries (e.g., the security
block currently showing - bearer_auth: []) match the declared security scheme
"bearerAuth"; update each security list where "bearer_auth" appears (the
referenced occurrences in the diff) to "bearerAuth" so the references are
case-correct and valid.

---

Nitpick comments:
In `@app/models/oidc_config.rb`:
- Around line 52-61: The method validate! currently mutates its argument via
providers.reject!, make it non-mutating by producing and returning a filtered
copy instead: iterate over the input providers (using providers.reject or
providers.each_with_object) to build a new hash that skips entries with missing
REQUIRED_KEYS while still logging with Rails.logger.warn for each skipped
provider; ensure the method returns the new filtered hash and does not change
the original providers object, referencing the validate! method and
REQUIRED_KEYS constant to locate the change.

In `@Gemfile`:
- Line 61: The Gemfile currently lists an unconstrained dependency gem
'openid_connect'; update this to a pinned, pessimistic version constraint (for
example use gem 'openid_connect', '~> 2.3.0' or the exact range matching your
tested Gemfile.lock) to prevent accidental upgrades—modify the line containing
gem 'openid_connect' to include the chosen version constraint and run bundle
install to verify dependency resolution.

In `@swagger/v1/swagger.yaml`:
- Around line 1343-1350: The query parameter "topic_ids" defines an unbounded
array which is a DoS risk; modify the OpenAPI parameter definition for topic_ids
(the parameter named topic_ids) to include a sensible maxItems constraint (e.g.,
add "maxItems: 100" under its schema) so the array is capped, keeping its
existing items.type: string and other metadata unchanged.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e9ebe9ff-3f58-4b20-8ae2-f3c4211e0a84

📥 Commits

Reviewing files that changed from the base of the PR and between cc03ecd and 5163c3f.

⛔ Files ignored due to path filters (1)
  • Gemfile.lock is excluded by !**/*.lock
📒 Files selected for processing (22)
  • Gemfile
  • app/controllers/authentication_controller.rb
  • app/controllers/oidc_login_controller.rb
  • app/jobs/cleanup_stale_oidc_requests_job.rb
  • app/models/oidc_config.rb
  • app/models/oidc_request.rb
  • app/models/user.rb
  • config/initializers/oidc.rb
  • config/oidc_providers.yml
  • config/routes.rb
  • db/migrate/20260407003623_create_auth_requests.rb
  • db/migrate/20260411120000_rename_auth_requests_to_oidc_requests.rb
  • db/migrate/20260416184458_add_username_to_oidc_requests.rb
  • db/migrate/20260421223609_enforce_oidc_requests_constraints.rb
  • db/schema.rb
  • spec/factories.rb
  • spec/models/oidc_config_spec.rb
  • spec/models/oidc_request_spec.rb
  • spec/models/user_spec.rb
  • spec/requests/authentication_spec.rb
  • spec/requests/oidc_login_spec.rb
  • swagger/v1/swagger.yaml

Comment thread app/models/user.rb
Comment thread app/models/user.rb Outdated
Comment thread config/initializers/oidc.rb
Comment thread db/migrate/20260421223609_enforce_oidc_requests_constraints.rb
Comment thread spec/factories.rb
Comment thread spec/requests/authentication_spec.rb
Comment thread swagger/v1/swagger.yaml
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 1

🧹 Nitpick comments (1)
app/models/oidc_request.rb (1)

96-99: This normalization query will bypass ordinary indexes on users.name and users.email.

LOWER(TRIM(...)) on both columns is fine for correctness, but it turns the OIDC callback lookup into a scan on larger user tables. If this path will see real traffic, normalize these values on write or add generated/functional indexed columns for the normalized form.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/oidc_request.rb` around lines 96 - 99, The current lookup using
User.where("LOWER(TRIM(name)) = ? AND LOWER(TRIM(email)) = ?",
normalized_username, normalized_email).first forces a full table scan; change
the data model and query to use precomputed normalized columns or
generated/functional indexed columns instead: add persisted normalized fields
(e.g., name_normalized, email_normalized) populated on write (or add
DB-generated columns with indexes) and update the OIDC lookup to compare against
those fields (the code around User.where and the
normalized_username/normalized_email variables), keeping the same
AuthenticationError raise semantics.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@app/models/oidc_request.rb`:
- Around line 15-20: The current consume_recent_by_state! destroys the OIDC
request row immediately, preventing retries if OidcLoginController#callback
fails before authenticate_user! completes; instead, change
consume_recent_by_state! to acquire a row lock and mark the record as
in-progress (e.g., set an in_progress flag or status column and/or consumed_at
timestamp) inside the transaction and return the record without deleting it, and
then update the controller flow so the actual deletion (or final consumed
status) happens only after authenticate_user! and the provider exchange succeed;
update model methods (consume_recent_by_state!, any scopes referencing
VALIDITY_WINDOW) and the OidcLoginController#callback flow to perform the final
destroy!/mark-consumed step on success.

---

Nitpick comments:
In `@app/models/oidc_request.rb`:
- Around line 96-99: The current lookup using User.where("LOWER(TRIM(name)) = ?
AND LOWER(TRIM(email)) = ?", normalized_username, normalized_email).first forces
a full table scan; change the data model and query to use precomputed normalized
columns or generated/functional indexed columns instead: add persisted
normalized fields (e.g., name_normalized, email_normalized) populated on write
(or add DB-generated columns with indexes) and update the OIDC lookup to compare
against those fields (the code around User.where and the
normalized_username/normalized_email variables), keeping the same
AuthenticationError raise semantics.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: d0bbebfb-0559-467e-9588-af42cdc725b9

📥 Commits

Reviewing files that changed from the base of the PR and between 5cb7147 and 0690799.

📒 Files selected for processing (3)
  • app/models/oidc_request.rb
  • spec/models/oidc_request_spec.rb
  • spec/requests/oidc_login_spec.rb

Comment thread app/models/oidc_request.rb
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

♻️ Duplicate comments (3)
app/models/oidc_config.rb (2)

10-31: ⚠️ Potential issue | 🟠 Major

Handle a missing config file in OidcConfig.providers, not just at boot.

Rescuing Errno::ENOENT only in config/initializers/oidc.rb lets the app start, but any later call to OidcConfig.public_list, OidcConfig.find, or OidcConfig.providers still raises and returns a 500. Returning {} here keeps “OIDC disabled” consistent for every caller.

Suggested fix
   def self.providers
     `@providers` ||= begin
-                     yaml = ERB.new(File.read(CONFIG_FILE)).result
-                     parsed = YAML.safe_load(yaml, permitted_classes: [], aliases: true)
+                     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}"
                        )
                        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)
+                   rescue Errno::ENOENT
+                     Rails.logger.info("OIDC config file not found; OIDC disabled")
+                     {}
                    end
   end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/oidc_config.rb` around lines 10 - 31, OidcConfig.providers
currently reads CONFIG_FILE without handling a missing file, causing
Errno::ENOENT on later calls; modify the providers method (where
File.read(CONFIG_FILE) and YAML.safe_load are invoked) to rescue Errno::ENOENT
and treat it as an empty config by logging a warning and returning an empty Hash
for providers (so `@providers` memoizes to {} and subsequent calls to
public_list/find/providers behave as “OIDC disabled”); ensure you still call
validate!(providers) only when providers is a Hash and avoid re-raising the
file-not-found error.

65-72: ⚠️ Potential issue | 🟠 Major

Skip non-hash provider entries instead of crashing config load.

validate! assumes every providers[key] is a hash. A YAML entry like provider_a: null or provider_b: true will raise here, which takes down config loading even though the class comment says invalid providers are skipped with a warning.

Suggested fix
   def self.validate!(providers)
     providers.reject! do |key, cfg|
+      unless cfg.is_a?(Hash)
+        Rails.logger.warn("OIDC provider '#{key}' skipped: expected a mapping, got #{cfg.class}")
+        next true
+      end
+
       missing = REQUIRED_KEYS.select { |k| cfg[k].blank? }
       if missing.any?
         Rails.logger.warn("OIDC provider '#{key}' skipped: missing #{missing.join(', ')}")
         true
       end
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/oidc_config.rb` around lines 65 - 72, The validate! method
currently assumes each providers value is a Hash and will crash on entries like
null or true; update validate! to first check that cfg is a Hash (e.g.,
cfg.is_a?(Hash)) and if not, log a warning that the provider "#{key}" was
skipped because its value is not a hash, then return true from the reject! block
to remove it; otherwise compute missing = REQUIRED_KEYS.select { |k|
cfg[k].blank? } as before, warn and reject when missing.any?, ensuring both
non-hash and missing-key cases are handled without raising.
app/models/oidc_request.rb (1)

24-29: ⚠️ Potential issue | 🟠 Major

Don’t destroy the request before the callback actually succeeds.

This still consumes the row before authenticate_user! does discovery and token exchange. If anything after Line 27 fails, the browser is left with a state value that cannot be retried even though no login completed. Lock/mark the row here, then finalize deletion only after successful authentication.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/models/oidc_request.rb` around lines 24 - 29, The method
consume_recent_by_state! currently locks and immediately destroys the OIDC
request, which prevents retries if downstream authenticate_user! fails; instead,
change consume_recent_by_state! to only lock and mark the row as consumed (e.g.
update a new consumed_at or consumed boolean column) and return the locked
record, and then have the caller (authenticate_user! or its caller flow) perform
the final deletion (or set a final_state) only after the token exchange and
callbacks succeed; update any tests and add a migration to add
consumed_at/consumed flag if one does not exist and ensure the lookup
(where("created_at > ?", VALIDITY_WINDOW.ago)) excludes already-marked records.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Duplicate comments:
In `@app/models/oidc_config.rb`:
- Around line 10-31: OidcConfig.providers currently reads CONFIG_FILE without
handling a missing file, causing Errno::ENOENT on later calls; modify the
providers method (where File.read(CONFIG_FILE) and YAML.safe_load are invoked)
to rescue Errno::ENOENT and treat it as an empty config by logging a warning and
returning an empty Hash for providers (so `@providers` memoizes to {} and
subsequent calls to public_list/find/providers behave as “OIDC disabled”);
ensure you still call validate!(providers) only when providers is a Hash and
avoid re-raising the file-not-found error.
- Around line 65-72: The validate! method currently assumes each providers value
is a Hash and will crash on entries like null or true; update validate! to first
check that cfg is a Hash (e.g., cfg.is_a?(Hash)) and if not, log a warning that
the provider "#{key}" was skipped because its value is not a hash, then return
true from the reject! block to remove it; otherwise compute missing =
REQUIRED_KEYS.select { |k| cfg[k].blank? } as before, warn and reject when
missing.any?, ensuring both non-hash and missing-key cases are handled without
raising.

In `@app/models/oidc_request.rb`:
- Around line 24-29: The method consume_recent_by_state! currently locks and
immediately destroys the OIDC request, which prevents retries if downstream
authenticate_user! fails; instead, change consume_recent_by_state! to only lock
and mark the row as consumed (e.g. update a new consumed_at or consumed boolean
column) and return the locked record, and then have the caller
(authenticate_user! or its caller flow) perform the final deletion (or set a
final_state) only after the token exchange and callbacks succeed; update any
tests and add a migration to add consumed_at/consumed flag if one does not exist
and ensure the lookup (where("created_at > ?", VALIDITY_WINDOW.ago)) excludes
already-marked records.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: b53619c9-16fa-4e4a-929e-ad39a748b912

📥 Commits

Reviewing files that changed from the base of the PR and between 0690799 and 4b0c1e6.

📒 Files selected for processing (5)
  • app/controllers/oidc_login_controller.rb
  • app/models/oidc_config.rb
  • app/models/oidc_request.rb
  • app/models/user.rb
  • config/initializers/oidc.rb
🚧 Files skipped from review as they are similar to previous changes (1)
  • app/models/user.rb

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

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

Actionable comments posted: 2

🧹 Nitpick comments (3)
spec/models/oidc_request_spec.rb (1)

189-194: Remove duplicated successful-email examples.

The examples at Line 189 and Line 213 assert the same behavior (email_verified: true returns the email). Keeping one is enough.

Also applies to: 213-218

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@spec/models/oidc_request_spec.rb` around lines 189 - 194, Two identical
successful-email examples are present for the verified-email behavior; remove
the duplicate. Keep one of the examples that stubs token exchange with
email_verified: true (the block that calls stub_token_exchange(email:
'oidcuser@ncsu.edu', email_verified: true) and expects
oidc_request.verified_email_from_code!(provider_key: 'google-ncsu', code:
'authorization-code') to equal the email) and delete the other redundant example
(the other identical it block). Ensure the remaining spec still uses
stub_token_exchange and verifies the same call to verified_email_from_code!.
spec/requests/oidc_login_spec.rb (1)

226-345: Add an endpoint-level replay test for callback state consumption.

You already test invalid state, but there’s no request-spec assertion that the same valid state cannot be used twice. Adding that scenario would verify controller wiring to OidcRequest.consume_recent_by_state! end-to-end.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@spec/requests/oidc_login_spec.rb` around lines 226 - 345, Add a new
request-spec example that exercises state replay: use create_oidc_request to
make a valid oidc_request,
stub_provider_config/stub_discovery/stub_token_exchange as in the successful
'200' example, POST the callback once and assert it returns a token, then POST
again with the same oidc_request.state and assert it returns a 401 and the
"Authentication failed" error; this verifies the controller invokes
OidcRequest.consume_recent_by_state! (or equivalent) to prevent reuse of the
same state.
spec/models/oidc_config_spec.rb (1)

113-155: Consider DRYing repeated config-file stubbing blocks.

Lines 114–115, 121–122, 128–129, 135–136, and 146–147 repeat the same File.read setup pattern. A small helper (e.g., stub_config(yaml)) would reduce maintenance churn.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@spec/models/oidc_config_spec.rb` around lines 113 - 155, The repeated
File.read stubbing in these examples should be extracted into a small helper to
reduce duplication: add a spec helper method (e.g., stub_config(yaml)) that
calls allow(File).to receive(:read).and_call_original and then allow(File).to
receive(:read).with(described_class::CONFIG_FILE).and_return(yaml), and update
each example to call stub_config with the desired YAML string before asserting
described_class.providers; reference the existing CONFIG_FILE constant and
described_class.providers when replacing the repeated blocks.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@spec/models/oidc_request_spec.rb`:
- Around line 171-177: The spec currently asserts uniqueness by calling
create_request directly which only hits the DB constraint; update the example to
exercise the authorization_uri_for! flow instead by calling
OidcRequest.authorization_uri_for! (or the appropriate class method under test)
twice with the same state to trigger the uniqueness handling inside that method;
keep the first call to create a request (or let the first authorization_uri_for!
create it) and then call authorization_uri_for! again using the duplicate state
(and a different nonce if needed) and expect ActiveRecord::RecordNotUnique, so
the test actually exercises the method authorization_uri_for! rather than only
create_request.

In `@spec/requests/oidc_login_spec.rb`:
- Around line 7-10: The test setup uses Institution.first which makes
`@institution` order-dependent; change the setup in the before(:each) block that
assigns `@institution` (where create_roles_hierarchy is called and `@institution` is
used) to perform a deterministic lookup/creation such as using
Institution.find_or_create_by!(...) or find_by!(unique_attribute) and create!
when missing, ensuring the test always gets the intended Institution rather than
depending on test execution order.

---

Nitpick comments:
In `@spec/models/oidc_config_spec.rb`:
- Around line 113-155: The repeated File.read stubbing in these examples should
be extracted into a small helper to reduce duplication: add a spec helper method
(e.g., stub_config(yaml)) that calls allow(File).to
receive(:read).and_call_original and then allow(File).to
receive(:read).with(described_class::CONFIG_FILE).and_return(yaml), and update
each example to call stub_config with the desired YAML string before asserting
described_class.providers; reference the existing CONFIG_FILE constant and
described_class.providers when replacing the repeated blocks.

In `@spec/models/oidc_request_spec.rb`:
- Around line 189-194: Two identical successful-email examples are present for
the verified-email behavior; remove the duplicate. Keep one of the examples that
stubs token exchange with email_verified: true (the block that calls
stub_token_exchange(email: 'oidcuser@ncsu.edu', email_verified: true) and
expects oidc_request.verified_email_from_code!(provider_key: 'google-ncsu',
code: 'authorization-code') to equal the email) and delete the other redundant
example (the other identical it block). Ensure the remaining spec still uses
stub_token_exchange and verifies the same call to verified_email_from_code!.

In `@spec/requests/oidc_login_spec.rb`:
- Around line 226-345: Add a new request-spec example that exercises state
replay: use create_oidc_request to make a valid oidc_request,
stub_provider_config/stub_discovery/stub_token_exchange as in the successful
'200' example, POST the callback once and assert it returns a token, then POST
again with the same oidc_request.state and assert it returns a 401 and the
"Authentication failed" error; this verifies the controller invokes
OidcRequest.consume_recent_by_state! (or equivalent) to prevent reuse of the
same state.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 870ac724-3ea3-4f20-95e6-f180ccdd5d0b

📥 Commits

Reviewing files that changed from the base of the PR and between 4b0c1e6 and a2bc177.

📒 Files selected for processing (3)
  • spec/models/oidc_config_spec.rb
  • spec/models/oidc_request_spec.rb
  • spec/requests/oidc_login_spec.rb

Comment thread spec/models/oidc_request_spec.rb
Comment thread spec/requests/oidc_login_spec.rb
JaredM2028 and others added 11 commits April 22, 2026 21:46
Co-authored-by: Copilot <copilot@github.com>
Co-authored-by: Copilot <copilot@github.com>
…nments

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 <copilot@github.com>
OIDC Rate Limiting via Rack::Attack
…ation-in-prod

provider fail on startup when prod
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants