PiRSquared17 / activescaffold

Automatically exported from code.google.com/p/activescaffold
MIT License
0 stars 0 forks source link

Security: list action doesn't call authorized_for_read? for existing records. #715

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
What steps will reproduce the problem?
1. Set ActiveScaffold default permission to deny.
  ActiveScaffold.set_defaults do |config|
    config.security.default_permission = false
  end
2. Add authorized_for_read? function to model:
  def authorized_for_read?
    if existing_record_check?
      return true if (current_user == self.owner)
    else
      return true
    end
    return false
  end
3. Check output from list action of appropriate controller.

What is the expected output? What do you see instead?
- Expected: only records authorized_for_read? are visible.
- Actual: all records are visible.

What version (or revision) of the product are you using?
- rails-2.2 and 1.2RC1 

If this bug causes an exception, please paste at least the first 20 lines
below.
- N/A

I was able to modify lib/active_scaffold/finder.rb find_page() function to
simply hide the records which we don't have permission to read as follows:
--- a/vendor/plugins/active_scaffold/lib/active_scaffold/finder.rb
+++ b/vendor/plugins/active_scaffold/lib/active_scaffold/finder.rb
@@ -169,10 +169,14 @@ module ActiveScaffold
         pager = ::Paginator.new(count, options[:per_page]) do |offset,
per_page|
           sorted_collection = sort_collection_by_column(klass.find(:all,
finder_options), *options[:sorting].first)
           sorted_collection.slice(offset, per_page)
+          # Filter based on record level security.
+          sorted_collection.select { |record|
record.authorized_for?(:action => :read) }
         end
       else
         pager = ::Paginator.new(count, options[:per_page]) do |offset,
per_page|
-          klass.find(:all, finder_options.merge(:offset => offset, :limit
=> per_page))
+          sorted_collection = klass.find(:all,
finder_options.merge(:offset => offset, :limit => per_page))
+          # Filter based on record level security.
+          sorted_collection.select { |record|
record.authorized_for?(:action => :read) }
         end
       end

This has the follow limitations:
- total count does not take into consideration records which we don't have
permission to read.
- each page varies in the number of records shown depending on how many are
hidden, thoughts on fixing this:
-- method-based-sorting: we could switch the select() and slice()
operations, but it would mean running authorized_for? on every record even
those that aren't on this page - might be acceptable since you have the
overhead of method based sorting anyway. Would need to update @count with
the correct value as well otherwise the number of pages won't be accurate.
-- sql-based-sorting: can't think of an easy way to do this without running
authorized_for? on every record and that won't scale.

In general I have been using conditions and constraints to only show the
appropriate records and simply have this a second layer of defence in case
of a bug in my conditions/constraints or where the permissions can't be
defined via SQL.

Original issue reported on code.google.com by khals...@gmail.com on 14 Nov 2009 at 2:59

GoogleCodeExporter commented 9 years ago
Due to limitations on pagination, authorized_for_read? should be used only for 
show
and actions like that. You can't get the right number of records per page, so 
the
result would be strange, because you can show 10 rows in a page and 13 rows in 
other
page. column_authorized_for_read? is used in list view, but if you want to limit
what records to show you must use conditions_for_collection in the controller.

Original comment by sergio.c...@gmail.com on 16 Nov 2009 at 8:36

GoogleCodeExporter commented 9 years ago
> column_authorized_for_read? is used in list view

This is a much cleaner solution. I have upgraded to current master to take 
advantage
of this as the rails-2.2 branch does not have this ability. I have logged a 
suggested
enhancement as issue 717: authorized_for? used in list view use empty_field_text
instead of blank if authorisation fails

Original comment by khals...@gmail.com on 21 Nov 2009 at 6:28