Downcase consortium ID when determining access to individual DOI records for cases where doi.provider.consortium_id is downcased#1498
Conversation
📝 WalkthroughWalkthroughA case-sensitivity fix in the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use Trivy to scan for security misconfigurations and secrets in Infrastructure as Code files.Add a .trivyignore file to your project to customize which findings Trivy reports. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
app/models/concerns/authenticable.rb (1)
228-231: Consider applying the same case normalization for provider_admin comparison.For consistency, the
provider_admin/provider_usercomparison at line 230 also comparesuser.provider_iddirectly withdoi.provider_idwithout case normalization. If similar casing mismatches exist for non-consortium providers, this could cause the same access issues.If the underlying data is expected to be consistent (unlike the consortium case), this can be deferred. Otherwise, consider normalizing:
user.provider_id.downcase == doi.provider_id.downcase🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/models/concerns/authenticable.rb` around lines 228 - 231, The comparison that returns false for users with roles provider_admin/provider_user currently checks user.provider_id == doi.provider_id without normalizing case, so update the conditional in authenticable.rb (the if that references user.role_id, provider_admin/provider_user, user.provider_id and doi.provider_id) to compare normalized values instead (e.g., compare user.provider_id.to_s.downcase with doi.provider_id.to_s.downcase) to avoid nil errors and casing mismatches; keep the rest of the condition logic unchanged.
🤖 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/concerns/authenticable.rb`:
- Around line 223-227: The conditional that checks consortium access calls
doi.provider.consortium_id.downcase without nil guards, which can raise
NoMethodError; update the condition in authenticable.rb to first ensure
doi.provider and doi.provider.consortium_id are present (or use
Doi.consortium_id helper) before calling downcase, e.g. add presence checks
around doi.provider and consortium_id in the existing if that returns false, and
mirror the nil-safe pattern used in Doi.consortium_id and in
app/models/ability.rb (line ~58) to prevent calling .downcase on nil.
---
Nitpick comments:
In `@app/models/concerns/authenticable.rb`:
- Around line 228-231: The comparison that returns false for users with roles
provider_admin/provider_user currently checks user.provider_id ==
doi.provider_id without normalizing case, so update the conditional in
authenticable.rb (the if that references user.role_id,
provider_admin/provider_user, user.provider_id and doi.provider_id) to compare
normalized values instead (e.g., compare user.provider_id.to_s.downcase with
doi.provider_id.to_s.downcase) to avoid nil errors and casing mismatches; keep
the rest of the condition logic unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 83327960-eef2-48e2-a609-e527735db7d2
📒 Files selected for processing (1)
app/models/concerns/authenticable.rb
| if %w[consortium_admin].include?(user.role_id) && | ||
| user.provider_id.present? && | ||
| user.provider_id.upcase == doi.provider.consortium_id | ||
| user.provider_id.downcase == doi.provider.consortium_id.downcase | ||
| return false | ||
| end |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Search for existing nil safety patterns around consortium_id access
# Check how consortium_id is typically accessed
rg -n -C2 'consortium_id' --type ruby -g '!spec/**'
# Check for safe navigation patterns with provider
rg -n '\.provider&\.' --type rubyRepository: datacite/lupo
Length of output: 29447
🏁 Script executed:
# Find the not_allowed_by_doi_and_user method
rg -n 'def not_allowed_by_doi_and_user' app/models/concerns/authenticable.rb -A 30Repository: datacite/lupo
Length of output: 1218
🏁 Script executed:
# Check the Doi model to see if provider association is always present
rg -n 'belongs_to.*provider|has_one.*provider' app/models/doi.rb -A 2 -B 2Repository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Check if there are guards/early returns before line 225
sed -n '200,227p' app/models/concerns/authenticable.rbRepository: datacite/lupo
Length of output: 877
🏁 Script executed:
# Test .casecmp behavior with nil in Ruby
python3 << 'EOF'
# casecmp is Ruby's case-insensitive comparison method
# Let's verify the behavior
print("Testing .casecmp() with nil:")
print("In Ruby, calling nil.casecmp(x) raises NoMethodError")
print("In Ruby, calling 'string'.casecmp(nil) raises TypeError")
EOFRepository: datacite/lupo
Length of output: 194
🏁 Script executed:
# Check Doi model associations and validations
rg -n 'belongs_to.*provider|validates.*provider' app/models/doi.rb -A 2 -B 1 | head -30Repository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Check if doi.provider is ever nil in the codebase by searching for safe navigation usage
rg -n 'doi\.provider&\.' --type rubyRepository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Check the Provider model to confirm consortium_id can be nil
rg -n 'consortium_id.present\?' app/models/provider.rbRepository: datacite/lupo
Length of output: 144
🏁 Script executed:
# Find and examine the Doi model associations
rg -n 'class Doi' app/models/doi.rb -A 50 | head -80Repository: datacite/lupo
Length of output: 1762
🏁 Script executed:
# Check for foreign key constraints and associations
rg -n 'belongs_to.*provider|has_many.*doi|foreign_key.*provider' app/models/doi.rb app/models/provider.rbRepository: datacite/lupo
Length of output: 2863
🏁 Script executed:
# Search for test cases showing the behavior when provider is nil
rg -n 'provider.*nil\|nil.*provider' spec/ -A 2 -B 2 | head -40Repository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Find the belongs_to :client association in Doi model
rg -n 'belongs_to.*client' app/models/doi.rb -A 2Repository: datacite/lupo
Length of output: 394
🏁 Script executed:
# Check Client model for belongs_to :provider
rg -n 'belongs_to.*provider' app/models/client.rb -A 2Repository: datacite/lupo
Length of output: 218
🏁 Script executed:
# Check if provider is always present in the DOI context
rg -n 'doi\.provider' spec/ --type ruby | head -20Repository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Verify the behavior by looking at actual code accessing doi.provider
rg -n 'doi\.provider\.' app/models/ --type ruby -A 1 | head -40Repository: datacite/lupo
Length of output: 495
🏁 Script executed:
# Check if Doi has a direct provider_id column
rg -n 'provider_id' app/models/doi.rb | grep -E 'belongs_to|has_one|t\.' | head -10Repository: datacite/lupo
Length of output: 125
🏁 Script executed:
# Look at the Doi schema to see if it has provider_id column
rg -n 'provider_id' db/schema.rb | grep -A 2 -B 2 'dois'Repository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Check the actual usage context - is line 225 guaranteed to have provider?
rg -n 'not_allowed_by_doi_and_user' app/ --type ruby -B 5 | head -50Repository: datacite/lupo
Length of output: 1908
🏁 Script executed:
# Check ability.rb context to see if there are guards
sed -n '28,68p' app/models/ability.rbRepository: datacite/lupo
Length of output: 1777
🏁 Script executed:
# Check if the query that fetches DOIs at the call site guarantees a provider
sed -n '435,450p' app/controllers/datacite_dois_controller.rbRepository: datacite/lupo
Length of output: 587
🏁 Script executed:
# Check if there are any tests that test this method with nil provider scenarios
rg -n 'not_allowed_by_doi_and_user' spec/ --type ruby -B 3 -A 5 | head -60Repository: datacite/lupo
Length of output: 3831
🏁 Script executed:
# Check the Doi.consortium_id method implementation
sed -n '1895,1905p' app/models/doi.rbRepository: datacite/lupo
Length of output: 387
🏁 Script executed:
# Check if DOI has provider_id column directly
cat -n db/schema.rb | grep -A 200 'create_table "dois"' | grep -E 'provider_id|^[0-9]+-\s*end'Repository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Check the Doi model's direct columns vs associations
sed -n '80,140p' app/models/doi.rbRepository: datacite/lupo
Length of output: 4484
🏁 Script executed:
# Check if there's a guard before calling not_allowed_by_doi_and_user with a client
rg -n 'client.*nil\|client.present' app/models/concerns/authenticable.rb app/controllers/datacite_dois_controller.rbRepository: datacite/lupo
Length of output: 39
🏁 Script executed:
# Verify the fix approach by checking how Doi.consortium_id method handles this
sed -n '1895,1905p' app/models/doi.rbRepository: datacite/lupo
Length of output: 387
Add nil safety check for doi.provider and consortium_id to prevent NoMethodError.
If the DOI's client is nil (optional association), doi.provider will be nil. Additionally, consortium_id can be nil even when a provider exists. Calling .downcase on nil will raise NoMethodError, causing an exception instead of gracefully denying access.
This same pattern appears in app/models/ability.rb:58 without nil guards, which is inconsistent with the guards properly used in lines 46–47 and 65–66 of the same file. The Doi.consortium_id method (lines 1899–1901) demonstrates the correct approach with explicit nil checks.
🛡️ Proposed fix
if %w[consortium_admin].include?(user.role_id) &&
user.provider_id.present? &&
+ doi.provider&.consortium_id&.present? &&
user.provider_id.downcase == doi.provider.consortium_id.downcase
return false
end🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/models/concerns/authenticable.rb` around lines 223 - 227, The conditional
that checks consortium access calls doi.provider.consortium_id.downcase without
nil guards, which can raise NoMethodError; update the condition in
authenticable.rb to first ensure doi.provider and doi.provider.consortium_id are
present (or use Doi.consortium_id helper) before calling downcase, e.g. add
presence checks around doi.provider and consortium_id in the existing if that
returns false, and mirror the nil-safe pattern used in Doi.consortium_id and in
app/models/ability.rb (line ~58) to prevent calling .downcase on nil.
1 participant
Purpose
See issue below. This PR attempts to resolve a bug wherein consortium accounts cannot access draft/registered DOI pages in Fabrica due to a casing mismatch in consortium symbols.
Possibly related to the following issue, which stemmed from a Rails upgrade some time ago:
closes: datacite/datacite#2506
Approach
Open Questions and Pre-Merge TODOs
Learning
Types of changes
Bug fix (non-breaking change which fixes an issue)
New feature (non-breaking change which adds functionality)
Breaking change (fix or feature that would cause existing functionality to change)
Reviewer, please remember our guidelines:
Summary by CodeRabbit
Bug Fixes