@@ -1,3 +1,5 @@ +* Change Support: Suggestion grouping, sort by affected users [sb] + * Changed context derived from objects to #class.name.tableize to fix STI [sb] * Simplified controller authorization with filter_resource_access [sb] CHANGELOG @@ -35,7 +35,10 @@ class AuthorizationRulesController < ApplicationController @users.sort! {|a, b| a.login <=> b.login } @privileges = authorization_engine.auth_rules.collect {|rule| rule.privileges.to_a}.flatten.uniq - @privileges = @privileges.collect {|priv| Authorization::DevelopmentSupport::AnalyzerEngine::Privilege.for_sym(priv, authorization_engine).descendants.map(&:to_sym) }.flatten.uniq + @privileges = @privileges.collect do |priv| + priv = Authorization::DevelopmentSupport::AnalyzerEngine::Privilege.for_sym(priv, authorization_engine) + (priv.descendants + priv.ancestors).map(&:to_sym) + end.flatten.uniq @privileges.sort_by {|priv| priv.to_s} @privilege = params[:privilege].to_sym rescue @privileges.first @contexts = authorization_engine.auth_rules.collect {|rule| rule.contexts.to_a}.flatten.uniq @@ -64,19 +67,40 @@ class AuthorizationRulesController < ApplicationController deserialize_changes(spec).flatten end - users_keys = users_permission.keys analyzer = Authorization::DevelopmentSupport::ChangeSupporter.new(authorization_engine) privilege = params[:privilege].to_sym context = params[:context].to_sym + all_users = User.all @context = context - @approaches = analyzer.find_approaches_for(:users => users_keys, :prohibited_actions => prohibited_actions) do + @approaches = analyzer.find_approaches_for(:users => all_users, :prohibited_actions => prohibited_actions) do users.each_with_index do |user, idx| - args = [privilege, {:context => context, :user => user}] - assert(users_permission[users_keys[idx]] ? permit?(*args) : !permit?(*args)) + unless users_permission[all_users[idx]].nil? + args = [privilege, {:context => context, :user => user}] + assert(users_permission[all_users[idx]] ? permit?(*args) : !permit?(*args)) + end end end + @affected_users = @approaches.each_with_object({}) do |approach, memo| + memo[approach] = approach.affected_users(authorization_engine, all_users, privilege, context).length + end + max_affected_users = @affected_users.values.max + if params[:affected_users] + @approaches = @approaches.sort_by do |approach| + affected_users_count = @affected_users[approach] + if params[:affected_users] == "many" + #approach.weight.to_f / [affected_users_count, 0.1].min + approach.weight + (max_affected_users - affected_users_count) * 10 + else + #approach.weight * affected_users_count + approach.weight + affected_users_count * 10 + end + end + end + + @grouped_approaches = analyzer.group_approaches(@approaches) + respond_to do |format| format.js do render :partial => 'suggestions' app/controllers/authorization_rules_controller.rb @@ -46,7 +46,7 @@ module AuthorizationRulesHelper def navigation link_to("Rules", authorization_rules_path) << ' | ' << - link_to("Change Supporter", change_authorization_rules_path) << ' | ' << + link_to("Change Support", change_authorization_rules_path) << ' | ' << link_to("Graphical view", graph_authorization_rules_path) << ' | ' << link_to("Usages", authorization_usages_path) #<< ' | ' << # 'Edit | ' << @@ -118,8 +118,8 @@ module AuthorizationRulesHelper def prohibit_link (step, text, title, options) options[:with_removal] ? - ' ' + link_to_function("[x]", "prohibit_action('#{serialize_action(step)}', '#{text}')", - :class => 'unimportant', :title => title) : + link_to_function("[x]", "prohibit_action('#{serialize_action(step)}', '#{text}')", + :class => 'prohibit', :title => title) : '' end @@ -149,6 +149,10 @@ module AuthorizationRulesHelper @changes && @changes[args[0]] && @changes[args[0]].include?(args[1..-1]) end + def affected_users_count (approach) + @affected_users[approach] + end + def auth_usage_info_classes (auth_info) classes = [] if auth_info[:controller_permissions] app/helpers/authorization_rules_helper.rb @@ -1,5 +1,5 @@ <form> -<h2>1. Choose permission to change</h2> +<h2>Which permission to change?</h2> <p class="action-options"> <label>Privilege</label> <%= select_tag :privilege, options_for_select(@privileges.map(&:to_s).sort, @privilege.to_s) %> @@ -7,10 +7,19 @@ <label>On</label> <%= select_tag :context, options_for_select(@contexts.map(&:to_s).sort, @context.to_s) %> <br/> - <%= link_to_function "Current permissions", "show_current_permissions()", :class => 'unimportant' %> + <label></label> + <%= link_to_function "Show current permissions", "show_current_permissions()", :class => 'unimportant' %> + <br/><br/> + How many users should be <strong>affected</strong>? + <br/> + <label></label> + <%= radio_button_tag :affected_users, :few, params[:affected_users] == 'few' %> + <label class="inline">A <strong>few</strong> users</label> + <%= radio_button_tag :affected_users, :many, params[:affected_users] == 'many' %> + <label class="inline"><strong>Many</strong> users</label> </p> -<h2>2. Whose permission should be changed?</h2> +<h2>Whose permission should be changed?</h2> <table class="change-options"> <thead> <tr> app/views/authorization_rules/_change.erb @@ -32,6 +32,6 @@ } <% end %> <div id="graph-container" style="display:none"> -<%= link_to_function "Hide", "$('graph-container').hide()" %><br/> -<object id="graph" data="" type="image/svg+xml" style="max-width:100%"/> +<%= link_to_function "Hide", "$('graph-container').hide()", :class => 'important' %><br/> +<object id="graph" data="" type="image/svg+xml" style="max-width:100%;margin-top: 0.5em"/> </div> \ No newline at end of file app/views/authorization_rules/_show_graph.erb @@ -4,21 +4,45 @@ <% if @approaches.first.changes.empty? %> <p>No changes necessary.</p> <% else %> - <p>Suggested changes (<%= link_to_function "show", "show_suggest_graph('#{serialize_changes(@approaches.first)}', '#{serialize_relevant_roles(@approaches.first)}', '#{@context}', relevant_user_ids())" %>):</p> - <%= render "suggestion", :object => @approaches.first %> + <p class="unimportant"> + <%= pluralize(@approaches.length, 'approach') %> in + <%= pluralize(@grouped_approaches.length, 'group') %> + <%= params[:affected_users] ? "– #{params[:affected_users] == 'few' ? "fewer" : "most"} affected users first" : "" %> + </p> + <ul> + <% @grouped_approaches.each_with_index do |grouped_approach, group_index| %> + <% ([grouped_approach.approach] + grouped_approach.similar_approaches).each_with_index do |approach, index| %> + <li <%= (group_index < 3 || params[:show_all]) && index == 0 ? '' : 'style="display: none"' %> class="<%= index == 0 ? 'primary' : "secondary" %> group-<%= group_index %>"> + <!--<span class="ord"><%= (index + 1) %></span>--> + <span class="unimportant">Affected users</span> <strong><%= affected_users_count(approach) %></strong> &nbsp; + <span class="unimportant">Complexity</span> <strong><%= approach.weight %></strong> &nbsp; + <%= link_to_function "Diagram", "show_suggest_graph('#{serialize_changes(approach)}', '#{serialize_relevant_roles(approach)}', '#{@context}', relevant_user_ids())", :class => "show-approach" %> + <ul> + <% approach.changes.each do |action| %> + <% (action.to_a[0].is_a?(Enumerable) ? action.to_a : [action.to_a]).each do |step| %> + <li> + <%= describe_step(step.to_a, :with_removal => true) %> + </li> + <% end %> + <% end %> + </ul> + <% if index == 0 %> + <% if grouped_approach.similar_approaches.empty? %> + <span class="unimportant show-others-in-group">No further suggestions in this group</span> + <% else %> + <%= link_to_function "Show further #{pluralize grouped_approach.similar_approaches.length, "suggestion"} in this group", "show_group_approaches(#{group_index})", :class => "show-others-in-group" %> + <% end %> + <% end %> + <%= link_to_function "Hide further suggestions", "hide_group_approaches(#{group_index})" if index > 0 and index == grouped_approach.similar_approaches.length %> + </li> + <% if index == 0 and group_index == 2 and @grouped_approaches.length > 3 %> + <li <%= !params[:show_all] ? '' : 'style="display: none"' %>><%= link_to_function "Show further #{pluralize(@grouped_approaches.length - 3, 'group')}", "show_all_groups(); $(this).up().hide()" %></li> + <% end %> + <% end %> + <% end %> + </ul> + <% end %> <% else %> <p><strong>No approach found.</strong></p> <% end %> - -<% if @approaches.length > 1 %> - <p <%= !params[:show_all] ? '' : 'style="display: none"' %>><a href="#" onclick="$(this).up().hide();$('more-suggestions').show();return false">Show other <%= pluralize(@approaches.length - 1, 'approach') %></a></p> - <div id="more-suggestions" <%= params[:show_all] ? '' : 'style="display: none"' %>> - <% @approaches[1..-1].each_with_index do |approach, index| %> - <p> - <%= (index + 2).ordinalize %> best approach (<%= pluralize(approach.changes.length, 'step') %>, <%= link_to_function "show", "show_suggest_graph('#{serialize_changes(approach)}', '#{serialize_relevant_roles(approach)}', '#{@context}', relevant_user_ids())" %>) - <%= render "suggestion", :object => approach %> - </p> - <% end %> - </div> -<% end %> app/views/authorization_rules/_suggestions.erb @@ -4,12 +4,13 @@ <%= link_to_function "Hide", "$(this).up().hide()", :class => 'important' %> <%= link_to_function "Toggle stacked roles", "toggle_graph_params('suggest-graph', 'stacked_roles');" %> <%= link_to_function "Toggle only users' roles", "toggle_graph_params('suggest-graph', 'only_relevant_roles');" %><br/> -<object id="suggest-graph" data="" type="image/svg+xml" style="max-width: 98%;max-height: 95%"/> +<object id="suggest-graph" data="" type="image/svg+xml" style="max-width: 98%;max-height: 95%;margin-top: 0.5em"/> </div> <%= render 'show_graph' %> <style type="text/css"> .action-options label { display: block; float: left; width: 7em; padding-bottom: 0.5em } + .action-options label.inline { display: inline; float: none } .action-options select { float: left; } .action-options br { clear: both; } .action-options { margin-bottom: 2em } @@ -23,21 +24,33 @@ .submit { margin-top: 0 } .submit input { font-weight: bold; font-size: 120% } #suggest-result { - position: absolute; left: 60%; right: 10px; - border-left: 2px solid grey; - padding-left: 1em; + position: absolute; left: 55%; right: 10px; + border-left: 1px solid grey; + padding-left: 2em; z-index: 10; } + #suggest-result>ul { padding-left: 0 } + #suggest-result>ul>li { list-style: none; margin-bottom: 1em } + #suggest-result>ul>li.secondary { padding-left: 2em } + #suggest-result li { padding-left: 0 } + #suggest-result ul ul { padding-left: 2em } + #suggest-result .ord { float: left; display: block; width: 2em; font-weight: bold; color: grey } + .show-approach, .show-others-in-group { visibility: hidden } + .prohibit { text-decoration: none; visibility: hidden } + li:hover .show-approach, li:hover .prohibit, + li:hover .show-others-in-group { visibility: visible } #suggest-graph-container { - background: white; border:1px solid #ccc; + background: white; border:1px solid grey; position:fixed; z-index: 20; - left:10%; bottom: 10%; right: 10%; top: 10% + left:10%; bottom: 10%; right: 10%; top: 10%; + padding: 0.5em } #graph-container { - background: white; margin: 1em; border:1px solid #ccc; + background: white; margin: 1em; border:1px solid grey; padding: 0.5em; max-width:50%; position:fixed; z-index: 20; right:0; } - .unimportant, .remove { color: grey } + .unimportant, .remove, .prohibit { color: grey } + .important { font-weight: bold } .remove { cursor: pointer } </style> <% javascript_tag do %> @@ -72,8 +85,9 @@ function install_change_observers () { $$('#change select').each(function (el) { el.observe('change', function (event) { + var form = $('change').down('form'); new Ajax.Updater({success: 'change'}, '<%= url_for %>', { - parameters: { context: $F('context'), privilege: $F('privilege') }, + parameters: Form.serializeElements(form.select('select').concat(form.getInputs('radio', 'affected_users')), true), method: 'get', onComplete: function () { install_change_observers(); @@ -98,6 +112,20 @@ show_graph($F('privilege'), $F('context')/*, relevant_user_ids()*/); } + function show_all_groups () { + $$('#suggest-result>ul>li.primary').invoke("show"); + } + + function show_group_approaches (group_no) { + $$('#suggest-result>ul>li.secondary.group-' + group_no).invoke("show"); + $$('#suggest-result>ul>li.primary.group-' + group_no + ' a.show-others-in-group').invoke("hide"); + } + + function hide_group_approaches (group_no) { + $$('#suggest-result>ul>li.secondary.group-' + group_no).invoke("hide"); + $$('#suggest-result>ul>li.primary.group-' + group_no + ' a.show-others-in-group').invoke("show"); + } + function relevant_user_ids () { return $$('#change .user_id').collect(function (el) { return el.innerHTML; app/views/authorization_rules/change.html.erb @@ -8,7 +8,6 @@ module Authorization # * Objective function: # * affected user count, # * as specific as possible (roles, privileges) - # -> counter-productive? # * as little changes as necessary # * Modify role, privilege hierarchy # * Merge, split roles @@ -18,7 +17,7 @@ module Authorization # * group similar candidates: only show abstract methods? # * restructure GUI layout: more room for analyzing suggestions # * changelog, previous tests, etc. - # * different permissions in tests + # * multiple permissions in tests # * Evaluation of approaches with Analyzer algorithms # * Authorization constraints # @@ -41,6 +40,11 @@ module Authorization # class ChangeSupporter < AbstractAnalyzer + # Returns a list of possible approaches for changes to the current + # authorization rules that achieve a given goal. The goal is given as + # permission tests in the block. The instance method +users+ is available + # when the block is executed to refer to the then-current users, whose + # roles might have changed as one suggestion. def find_approaches_for (options, &tests) @prohibited_actions = (options[:prohibited_actions] || []).to_set @@ -63,10 +67,29 @@ module Authorization end # remove subsets - suggestions.sort! end + # Returns an array of GroupedApproaches for the given array of approaches. + # Only groups directly adjacent approaches + def group_approaches (approaches) + approaches.each_with_object([]) do |approach, grouped| + if grouped.last and grouped.last.approach.similar_to(approach) + grouped.last.similar_approaches << approach + else + grouped << GroupedApproach.new(approach) + end + end + end + + class GroupedApproach + attr_accessor :approach, :similar_approaches + def initialize (approach) + @approach = approach + @similar_approaches = [] + end + end + class ApproachChecker attr_reader :users, :failed_tests @@ -119,6 +142,15 @@ module Authorization res end + def affected_users (original_engine, original_users, privilege, context) + (0...@users.length).select do |i| + original_engine.permit?(privilege, :context => context, + :skip_attribute_test => true, :user => original_users[i]) != + @engine.permit?(privilege, :context => context, + :skip_attribute_test => true, :user => @users[i]) + end.collect {|i| original_users[i]} + end + def initialize_copy (other) @engine = @engine.clone @users = @users.clone @@ -171,7 +203,17 @@ module Authorization end def sort_value - changes.sum(&:weight) + @failed_tests.length + weight + @failed_tests.length + end + + def weight + changes.sum(&:weight) + end + + def similar_to (other) + other.weight == weight and + other.changes.map {|change| change.class.name}.sort == + changes.map {|change| change.class.name}.sort end def inspect lib/declarative_authorization/development_support/change_supporter.rb @@ -594,4 +594,92 @@ class ChangeSupporterTest < Test::Unit::TestCase assert_not_equal 0, approaches.length assert !approaches.any? {|approach| approach.steps.any? {|step| step.class == Authorization::DevelopmentSupport::ChangeSupporter::RemoveRoleFromUserAction}} end + + def test_affected_users + reader = Authorization::Reader::DSLReader.new + reader.parse %{ + authorization do + role :test_role do + end + role :test_role_2 do + includes :test_role + end + end + } + engine = Authorization::Engine.new(reader) + analyzer = Authorization::DevelopmentSupport::ChangeSupporter.new(engine) + + user_to_extend_permissions = MockUser.new(:test_role_2) + another_user = MockUser.new(:test_role) + all_users = [user_to_extend_permissions, another_user] + + approaches = analyzer.find_approaches_for(:users => all_users) do + assert permit?(:read, :context => :permissions, :user => users[0]) + assert !permit?(:read, :context => :permissions, :user => users[1]) + end + + assert approaches.any? {|approach| + approach.steps.any? {|step| step.class == Authorization::DevelopmentSupport::ChangeSupporter::AssignPrivilegeToRoleAction} && + approach.affected_users(engine, all_users, :read, :permissions).length == 1 } + end + + def test_affected_users_with_user_change + reader = Authorization::Reader::DSLReader.new + reader.parse %{ + authorization do + role :test_role do + end + role :test_role_2 do + has_permission_on :permissions, :to => :read + end + end + } + engine = Authorization::Engine.new(reader) + analyzer = Authorization::DevelopmentSupport::ChangeSupporter.new(engine) + + user_to_extend_permissions = MockUser.new(:test_role) + another_user = MockUser.new(:test_role) + all_users = [user_to_extend_permissions, another_user] + + approaches = analyzer.find_approaches_for(:users => all_users) do + assert permit?(:read, :context => :permissions, :user => users.first) + assert !permit?(:read, :context => :permissions, :user => users[1]) + end + + assert approaches.any? {|approach| + approach.changes.first.class == Authorization::DevelopmentSupport::ChangeSupporter::AssignRoleToUserAction && + approach.affected_users(engine, all_users, :read, :permissions).length == 1 } + end + + def test_group_approaches + reader = Authorization::Reader::DSLReader.new + reader.parse %{ + authorization do + role :test_role do + includes :test_role_2 + end + role :test_role_2 do + has_permission_on :permissions, :to => :read + end + end + } + engine = Authorization::Engine.new(reader) + analyzer = Authorization::DevelopmentSupport::ChangeSupporter.new(engine) + + user_to_extend_permissions = MockUser.new() + another_user = MockUser.new() + all_users = [user_to_extend_permissions, another_user] + + approaches = analyzer.find_approaches_for(:users => all_users) do + assert permit?(:read, :context => :permissions, :user => users.first) + end + + assert approaches.first.similar_to(approaches[1]), + "First two approaches should be similar" + + grouped_approaches = analyzer.group_approaches(approaches) + assert_equal 2, grouped_approaches.length + assert grouped_approaches.first.approach + assert_equal 1, grouped_approaches.first.similar_approaches.length + end end test/development_support/change_supporter_test.rb app/views/authorization_rules/_suggestion.erb 95dbc6813c8b8f3f77c906bc2a100822400ba83b Steffen Bartsch sbartsch@tzi.org http://github.com/stffn/declarative_authorization/commit/51b8bd1979612fa66547ed5cb5c71d843234521a 51b8bd1979612fa66547ed5cb5c71d843234521a 2009-09-10T01:47:36-07:00 2009-08-27T05:24:10-07:00 Change support: suggestions: grouping, sorting by affected users d693b048bdcbba1b8fdef99e9ffbc71a7af020af Steffen Bartsch sbartsch@tzi.org