Skip to content

Commit

Permalink
Use Arel.sql to wrap a known safe SQL string in PermissionsService
Browse files Browse the repository at this point in the history
To protect against injection attacks, `#pluck` has deprecated raw SQL strings in
Rails 5.2.

We could use `pluck(:source_id).uniq` here. This would be less efficient in
some cases. I didn't have the time to look into whether those cases are likely
to arise, so we simply mark the SQL string as safe by wrapping it with
`Arel.sql()`.
  • Loading branch information
Tom Johnson committed Jul 16, 2019
1 parent 7a83daa commit 9ab079c
Show file tree
Hide file tree
Showing 3 changed files with 7 additions and 7 deletions.
2 changes: 1 addition & 1 deletion app/models/concerns/hyrax/ability.rb
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def admin_set_with_deposit?
ids = PermissionTemplateAccess.for_user(ability: self,
access: ['deposit', 'manage'])
.joins(:permission_template)
.pluck('DISTINCT source_id')
.pluck(Arel.sql('DISTINCT source_id'))
query = "_query_:\"{!raw f=has_model_ssim}AdminSet\" AND {!terms f=id}#{ids.join(',')}"
Hyrax::SolrService.count(query).positive?
end
Expand Down
10 changes: 5 additions & 5 deletions app/services/hyrax/collection_types/permissions_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,15 +13,15 @@ class PermissionsService
# If calling from Abilities, pass the ability. If you try to get the ability from the user, you end up in an infinit loop.
def self.collection_type_ids_for_user(roles:, user: nil, ability: nil)
return false unless user.present? || ability.present?
return Hyrax::CollectionType.all.pluck('DISTINCT id') if user_admin?(user, ability)
return Hyrax::CollectionType.all.pluck(Arel.sql('DISTINCT id')) if user_admin?(user, ability)
Hyrax::CollectionTypeParticipant.where(agent_type: Hyrax::CollectionTypeParticipant::USER_TYPE,
agent_id: user_id(user, ability),
access: roles)
.or(
Hyrax::CollectionTypeParticipant.where(agent_type: Hyrax::CollectionTypeParticipant::GROUP_TYPE,
agent_id: user_groups(user, ability),
access: roles)
).pluck('DISTINCT hyrax_collection_type_id')
).pluck(Arel.sql('DISTINCT hyrax_collection_type_id'))
end

# @api public
Expand Down Expand Up @@ -174,7 +174,7 @@ def self.access_to_collection_type?(collection_type:, access:, user: nil, abilit
def self.agent_ids_for(collection_type:, agent_type:, access:)
Hyrax::CollectionTypeParticipant.where(hyrax_collection_type_id: collection_type.id,
agent_type: agent_type,
access: access).pluck('DISTINCT agent_id')
access: access).pluck(Arel.sql('DISTINCT agent_id'))
end
private_class_method :agent_ids_for

Expand All @@ -190,7 +190,7 @@ def self.user_edit_grants_for_collection_of_type(collection_type: nil)
return [] unless collection_type
Hyrax::CollectionTypeParticipant.joins(:hyrax_collection_type).where(hyrax_collection_type_id: collection_type.id,
agent_type: Hyrax::CollectionTypeParticipant::USER_TYPE,
access: Hyrax::CollectionTypeParticipant::MANAGE_ACCESS).pluck('DISTINCT agent_id')
access: Hyrax::CollectionTypeParticipant::MANAGE_ACCESS).pluck(Arel.sql('DISTINCT agent_id'))
end

# @api public
Expand All @@ -205,7 +205,7 @@ def self.group_edit_grants_for_collection_of_type(collection_type: nil)
return [] unless collection_type
groups = Hyrax::CollectionTypeParticipant.joins(:hyrax_collection_type).where(hyrax_collection_type_id: collection_type.id,
agent_type: Hyrax::CollectionTypeParticipant::GROUP_TYPE,
access: Hyrax::CollectionTypeParticipant::MANAGE_ACCESS).pluck('DISTINCT agent_id')
access: Hyrax::CollectionTypeParticipant::MANAGE_ACCESS).pluck(Arel.sql('DISTINCT agent_id'))
groups | ['admin']
end

Expand Down
2 changes: 1 addition & 1 deletion app/services/hyrax/collections/permissions_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ class PermissionsService
def self.source_ids_for_user(access:, ability:, source_type: nil, exclude_groups: [])
scope = PermissionTemplateAccess.for_user(ability: ability, access: access, exclude_groups: exclude_groups)
.joins(:permission_template)
ids = scope.pluck('DISTINCT source_id')
ids = scope.pluck(Arel.sql('DISTINCT source_id'))
return ids unless source_type
filter_source(source_type: source_type, ids: ids)
end
Expand Down

0 comments on commit 9ab079c

Please sign in to comment.