Skip to content

staging into main #186

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

Merged
rsmoke merged 3 commits into from
Feb 23, 2026
Merged

staging into main #186

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
4257d69
Refactor entry policy to streamline access for container administrators
rsmoke Feb 23, 2026
ff42f03
Merge pull request #185 from lsa-mis/fix_access_to_details
rsmoke Feb 23, 2026
d869a30
Add test for unauthorized access by Collection Administrators
rsmoke Feb 23, 2026
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
11 changes: 4 additions & 7 deletions app/policies/entry_policy.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,19 +36,16 @@ def show?
class Scope < Scope
def resolve
base_scope = scope.where(deleted: false) # Only show non-deleted entries by default
admin_role_ids = Role.where(kind: ['Collection Administrator', 'Collection Manager']).pluck(:id)
admin_container_ids = user.present? ? user.assignments.where(role_id: admin_role_ids).pluck(:container_id).uniq : []
Comment on lines +39 to +40
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

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

The queries to fetch role IDs and container IDs are now executed unconditionally for all users at the beginning of the resolve method, even when the user is nil or axis_mundi. This could cause unnecessary database queries.

Consider moving these queries inside the elsif admin_container_ids.any? block or restructuring to only execute when needed. For example:

if user.nil?
  scope.none
elsif user.axis_mundi?
  base_scope
elsif user.present?
  admin_role_ids = Role.where(kind: ['Collection Administrator', 'Collection Manager']).pluck(:id)
  admin_container_ids = user.assignments.where(role_id: admin_role_ids).pluck(:container_id).uniq
  
  if admin_container_ids.any?
    # Collection administrators and managers logic
  elsif user.judge?
    # Judge logic
  # ... rest of conditions
end

This would avoid running the queries for nil users and axis_mundi users.

Copilot uses AI. Check for mistakes.

if user.nil?
scope.none
elsif user.axis_mundi?
# Axis mundi can see all entries
base_scope
elsif user.administrator? || user.manager?
# Collection administrators and managers can see entries from their containers
admin_role_ids = Role.where(kind: ['Collection Administrator', 'Collection Manager']).pluck(:id)
admin_container_ids = user.assignments
.where(role_id: admin_role_ids)
.pluck(:container_id)

elsif admin_container_ids.any?
# Collection administrators and managers (by container assignment) can see entries from their containers
base_scope.joins(contest_instance: { contest_description: :container })
.where(containers: { id: admin_container_ids })
Comment on lines +47 to 50
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

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

While the controller test verifies that Collection Administrators with container assignments can access entries, there is no corresponding test coverage for the EntryPolicy::Scope#resolve method itself. The policy spec file (spec/policies/entry_policy_spec.rb) only tests the show? method but does not test the Scope class.

Given that this PR introduces significant changes to the Scope#resolve logic, consider adding comprehensive tests for the Scope class similar to what exists for ContainerPolicy::Scope (spec/policies/container_policy_spec.rb lines 55-86). This would verify that:

  • Users with container-specific admin/manager assignments see only entries from their containers
  • Users without such assignments don't see entries they shouldn't
  • Edge cases like users with assignments to multiple containers are handled correctly

Copilot uses AI. Check for mistakes.
Comment on lines +39 to 50
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

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

The condition change from user.administrator? || user.manager? to admin_container_ids.any? creates inconsistency with other methods in the same policy class.

For example, the show? method (line 27) uses user&.has_container_role?(container) which correctly checks for container-specific role assignments. However, other methods like view_applicant_profile? (line 17) also use has_container_role?.

Consider using the existing has_container_role? helper method for consistency:

containers_with_admin_access = user.present? ? user.assignments.joins(:role).where(roles: { kind: ['Collection Administrator', 'Collection Manager'] }).pluck(:container_id).uniq : []

if containers_with_admin_access.any?
  base_scope.joins(contest_instance: { contest_description: :container })
           .where(containers: { id: containers_with_admin_access })

This maintains consistency with the existing authorization patterns in the policy.

Copilot uses AI. Check for mistakes.
elsif user.judge?
Expand Down
37 changes: 37 additions & 0 deletions spec/controllers/entries_controller_spec.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,43 @@
end
end

context "when user is a Container Administrator for the entry's container" do
Copy link

Copilot AI Feb 23, 2026

Choose a reason for hiding this comment

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

The test context description uses "Container Administrator" but the role kind is 'Collection Administrator'. This inconsistency exists in the codebase (see line 66 which has the same pattern), but it could cause confusion.

Throughout the codebase, the role kind in the database is 'Collection Administrator', but test descriptions sometimes refer to them as "Container Administrator". Consider standardizing on one term - preferably "Collection Administrator" to match the database value and Role model. This would make tests clearer and reduce potential confusion about whether these are different role types.

Suggested change
context "when user is a Container Administrator for the entry's container" do
context "when user is a Collection Administrator for the entry's container" do

Copilot uses AI. Check for mistakes.
let(:container) { contest_instance.contest_description.container }
let(:admin_user) { create(:user) }
let(:admin_role) { create(:role, kind: 'Collection Administrator') }

before do
create(:assignment, user: admin_user, container: container, role: admin_role)
sign_in admin_user
get :modal_details, params: { id: entry.id }
end

it "returns a successful response" do
expect(response).to be_successful
end

it "renders the details partial" do
expect(response).to render_template('entries/modal_details')
end
end

context "when user is a Collection Administrator for a different container" do
let(:other_container) { create(:container) }
let(:other_admin_user) { create(:user) }
let(:admin_role) { create(:role, kind: 'Collection Administrator') }

before do
create(:assignment, user: other_admin_user, container: other_container, role: admin_role)
sign_in other_admin_user
get :modal_details, params: { id: entry.id }
end

it "redirects with unauthorized message" do
expect(response).to redirect_to(root_path)
expect(flash[:alert]).to eq("!!! Not authorized !!!")
end
end

context "when user is not authorized" do
let(:other_user) { create(:user) }

Expand Down