Skip to content

Downcase consortium ID when determining access to individual DOI records for cases where doi.provider.consortium_id is downcased #1498

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
codycooperross wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Downcase consortium ID when determining access to individual DOI records for cases where doi.provider.consortium_id is downcased #1498

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion app/models/concerns/authenticable.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -222,7 +222,7 @@ def not_allowed_by_doi_and_user(doi: nil, user: nil)
return false if %w[staff_admin staff_user].include?(user.role_id)
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
Comment on lines 223 to 227
Copy link

@coderabbitai coderabbitai bot Mar 18, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🟠 Major

🧩 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 ruby

Repository: 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 30

Repository: 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 2

Repository: 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.rb

Repository: 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")
EOF

Repository: 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 -30

Repository: 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 ruby

Repository: 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.rb

Repository: 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 -80

Repository: 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.rb

Repository: 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 -40

Repository: 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 2

Repository: 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 2

Repository: 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 -20

Repository: 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 -40

Repository: 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 -10

Repository: 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 -50

Repository: 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.rb

Repository: 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.rb

Repository: 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 -60

Repository: datacite/lupo

Length of output: 3831

🏁 Script executed:

# Check the Doi.consortium_id method implementation
sed -n '1895,1905p' app/models/doi.rb

Repository: 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.rb

Repository: 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.rb

Repository: 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.rb

Repository: 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.

if %w[provider_admin provider_user].include?(user.role_id) &&
Expand Down
Loading