Merge pull request #26 from OSC/facl_fixes

Facl fixes

app/controllers/mappings_controller.rb

@@ -3,6 +3,7 @@
 
 class MappingsController < ApplicationController
   def index
+    @directory_permissions_command = Mapping.directory_permissions_command
   end
 
 
@@ -15,23 +16,27 @@ def create
     @mapping = Mapping.new(mapping_params)
 
     if @mapping.save_and_set_facls
-      flash[:success] = @mapping.save_message
+      flash[:success] = 'Mapping successfully created.'
       redirect_to action: :index
     else
-      flash[:warning] = 'Unable to create new mapping. ' + @mapping.save_message
-      redirect_to new_mapping_path, locals: params
+      flash.now[:warning] = 'Unable to create new mapping. ' + @mapping.errors.full_messages.join(' ')
+      render :new
     end
   end
 
   # POST /mappings
-  def destroy 
-    if Mapping.destroy_and_remove_facls(params[:id])
-      flash[:success] = 'Successfully deleted mapping.'
+  def destroy
+    @mapping = Mapping.find(params[:id])
+    if @mapping.destroy_and_remove_facls()
+      flash[:success] = 'Mapping successfully removed.'
       redirect_to action: :index
     else
-      flash[:danger] = 'Unable to delete mapping ' + params[:id]
+      flash[:danger] = 'Unable to remove mapping. ' + @mapping.errors.full_messages.join(' ')
       redirect_to action: :index
     end
+  rescue ActiveRecord::RecordNotFound
+    flash[:warning] = "Unable to find mapping #{params[:id]} to remove it."
+    redirect_to action: :index
   end
 
 

app/helpers/mappings_helper.rb

@@ -1,23 +1,23 @@
-require 'uri'
+require 'etc'
+require 'pathname'
 
 module MappingsHelper
   def user_list
-    users_from_group = ENV['USERS_FROM_GROUP']
-    result = `getent group #{users_from_group}`
+    Etc.getgrnam(Configuration.users_from_group).mem.sort
+  end
 
-    # example output
-    #    "wiagstf:*:5362:mrodgers,efranz\n"
-    result.strip.split(':')[3].split(',').sort
+  def user_select_list
+    user_list.map { |user| [display_username(user), user] }
   end
 
   def user_list_help
-    "Users list is built from users in group: #{ENV['USERS_FROM_GROUP']}"
+    "Users list is built from users in group: #{Configuration.users_from_group}"
   end
 
 
   # Get a list of the various Shiny apps
   def app_list
-    Dir.glob( Configuration.shared_apps_root.join('bc_shiny_*')).sort
+    Dir.glob( Configuration.shared_apps_root.join('bc_shiny_*')).sort.map{|path| Pathname.new(path)}
   end
 
   def app_list_help
@@ -26,7 +26,7 @@ def app_list_help
 
 
   def get_app_name(app_path)
-    URI(app_path).path.split('/').last
+    app_path.basename
   end
 
 
@@ -44,4 +44,13 @@ def known_datasets
   def known_datasets_help
    "Known datasets include files or directories under #{Configuration.app_dataset_root.to_s} and arbitrary paths already added to this database" 
   end
+
+  # Attempt to get a full name for the user
+  # @return [String]
+  def display_username(user)
+    full_name = Etc.getpwnam(user).gecos.strip
+    full_name = full_name.empty? ? user : full_name
+
+    "#{full_name} - #{user}"
+  end
 end

app/models/mapping.rb

@@ -6,19 +6,20 @@
 
 
 class Mapping < ActiveRecord::Base
-  attr_accessor :save_message
-  validates :user, :app, :dataset, presence: true
+  attr_accessor :dataset_non_std_location_value
+  validates :user, :app, :dataset, presence: { message: "A %{attribute} must be selected." }
   validate :dataset_path_must_exist
-  validates_uniqueness_of :user, scope: [:user, :app], message: "Unable to create a second mapping between user and app."
+  validate :app_path_may_not_be_blank
+  validates_uniqueness_of :user, scope: [:user, :app], message: "may only be mapped once to a given app."
 
   # Type dataset as a Pathname
   def dataset
-    Pathname.new(super)
+    Pathname.new(super.to_s)
   end
 
   # Type app as a Pathname
   def app
-    Pathname.new(super)
+    Pathname.new(super.to_s)
   end
 
   # @return [Array<String>]
@@ -46,7 +47,17 @@ def self.dump_to_yaml
   # Ensure that a user can use a given mapping
   # @return [Boolean]
   def is_still_valid?
-    return app.exist? && dataset.exist? && user_has_permissions_on_both?
+    app.exist? && dataset.exist? && user_has_permissions_on_both?
+  end
+
+  # Explain why a given mapping is not valid
+  # @return [String]
+  def reason_invalid
+    [
+      app.exist? ? nil : "#{app} is no longer visible on the file system.",
+      dataset.exist? ? nil : "#{dataset} is no longer visible on the file system.",
+      user_has_permissions_on_both? ? nil : "User #{user} does not have read/write permissions on both `#{app}` and `#{dataset}`."
+    ].select{ |value| ! value.nil? }.join(' ')
   end
 
   # @return [Hash]
@@ -55,52 +66,45 @@ def to_hash
   end
 
   # Custom destructor
-  def self.destroy_and_remove_facls(id)
+  def destroy_and_remove_facls
     begin
-      mapping = find(id)
-      mapping.remove_rx_facl(mapping.dataset)
-      mapping.remove_rx_facl(mapping.app)
-      mapping.destroy
-      dump_to_yaml
-      @save_message = 'Mapping successfully destroyed.'
+      remove_rx_facl(dataset)
+      remove_rx_facl(app)
+      destroy
+      Mapping.dump_to_yaml
 
       return true
     rescue OodSupport::InvalidPath, OodSupport::BadExitCode => e
-      @save_message = 'Unable to destroy mapping because ' + e.to_s
+      @errors[:base] << 'Unable to destroy mapping because ' + e.to_s
 
-      return false
-    rescue ActiveRecord::RecordNotFound  # User is probably mashing the delete
       return false
     end
   end
 
-  # Create a user readable string from the error.messages hash
-  def format_error_messages
-    @save_message = errors.messages.map {|_, message| message}.join(' ')
+  # Builds an Array of apps that need their permissions changed to include the C attribute for GROUP
+  #
+  # An example of acceptable FACL entry for GROUP:
+  #   A:g:GROUP@:rwaDxtTnNcCy
+  #
+  # @return [Pathname]
+  def self.installed_apps_with_busted_permissions?
+    ApplicationController.helpers.app_list.select { |app| ! can_modify_facl?(app) }
   end
 
   # Custom save method
   def save_and_set_facls
-    unless valid?
-      format_error_messages
-      return false
-    end
+    return false unless valid?
 
     begin
       success = save(:validate => false)
 
       add_rx_facl(app)
       add_rx_facl(dataset)
       Mapping.dump_to_yaml
-      @save_message = 'Mapping successfully created.'
 
       return true
-    rescue ActiveRecord::RecordNotUnique => e
-      @save_message = "Unable to create duplicate mapping between #{user}, #{app}, and #{dataset}"
-
-      return false
     rescue OodSupport::InvalidPath, OodSupport::BadExitCode => e
-      @save_message = "Unable to set FACLS because " + e.to_s
+      @errors[:base] << "Unable to set FACLS because " + e.to_s
 
       return false
     end
@@ -126,13 +130,13 @@ def self.permission_sensitive_dirs
 
   # @return [String]
   def self.directory_permissions_command
-    permission_sensitive_dirs.map{|directory| "chmod 0775 #{directory.to_s}"}.join(' && ')
+    permission_sensitive_dirs.map{|directory| "chmod 2775 #{directory.to_s}"}.join(' && ')
   end
 
   # @return [Boolean]
   def should_add_facl?(pathname)
     # Calling owned first protects rx_facl_exists? from throwing InvalidPath
-    pathname.owned? && ! rx_facl_exists?(pathname)
+    Mapping.can_modify_facl?(pathname) && ! rx_facl_exists?(pathname)
   end
 
   # Idempotently add a RX entry to the ACL for a file
@@ -141,6 +145,8 @@ def add_rx_facl(pathname)
 
     entry = build_facl_entry_for_user(user, Configuration.facl_user_domain)
     OodSupport::ACLs::Nfs4ACL.add_facl(path: pathname, entry: entry)
+
+    logger.debug { "add_rx_facl(#{pathname}) succeeded for #{user} and #{pathname.to_s}" }
   end
 
   # Check if pathname / user combination occurs once or less in the database
@@ -154,7 +160,7 @@ def pathname_uniq_for_user?(pathname)
 
   # @return [Boolean]
   def should_remove_facl?(pathname)
-    pathname.owned? && rx_facl_exists?(pathname) && pathname_uniq_for_user?(pathname)
+    Mapping.can_modify_facl?(pathname) && rx_facl_exists?(pathname) && pathname_uniq_for_user?(pathname)
   end
 
   # Conditionally remove RX FACLs
@@ -166,6 +172,8 @@ def remove_rx_facl(pathname)
 
     entry = build_facl_entry_for_user(user, Configuration.facl_user_domain)
     OodSupport::ACLs::Nfs4ACL.rem_facl(path: pathname, entry: entry)
+
+    logger.debug { "remove_rx_facl(#{pathname}) succeeded for #{user} and #{pathname.to_s}" }
   end
 
   # Build FACL for user and domain combination
@@ -188,7 +196,7 @@ def rx_facl_exists?(pathname)
       expected = build_facl_entry_for_user(user, Configuration.facl_user_domain)
 
       return acl.entries.include?(expected)
-    rescue
+    rescue OodSupport::InvalidPath, OodSupport::BadExitCode
       return false
     end
   end
@@ -199,9 +207,10 @@ def user_has_permissions_on_both?
     ood_user = OodSupport::User.new(user)
     required_permissions = [:r, :x]
 
-    app_facl = OodSupport::ACLs::Nfs4ACL.get_facl(path: app)
+    app_facl = nil
     dataset_facl = nil
     begin
+      app_facl = OodSupport::ACLs::Nfs4ACL.get_facl(path: app)
       dataset_facl = OodSupport::ACLs::Nfs4ACL.get_facl(path: dataset)
     rescue OodSupport::InvalidPath, OodSupport::BadExitCode
       return false
@@ -221,9 +230,56 @@ def dataset_path_must_exist
     errors.add(:base, "Dataset must exist.") unless dataset.exist?
   end
 
+  # Validator for app
+  def app_path_may_not_be_blank
+    errors.add(:base, "You must select an app.") unless app.exist?
+  end
+
   # Directories between $HOME and the configured database directory must be set to 775
   # @return [Boolean]
   def self.directory_perms_are_775?(directory)
     directory.stat.mode.to_s(8).end_with?('775')
   end
+
+  # Checks to see if FACLs are modifiable by the user
+  #
+  # Note that this check assumes that no negative permissions have been set.
+  # Also we are not using an ownership check since that could mask problems
+  # with permissions that would impact other users.
+  #
+  # @return [Boolean]
+  def self.can_modify_facl?(pathname)
+    pathname.exist? && group_facl_entry_has_C_set?(pathname)
+  end
+
+  # Get the group owner's name
+  # @return [String]
+  def self.get_groupname_for_pathname(pathname)
+    Etc.getgrgid(pathname.stat.gid).name
+  end
+
+  # Check if pathname is owned by the admin_group
+  # @return [Boolean]
+  def self.app_has_correct_group_ownership?(pathname)
+    get_groupname_for_pathname(pathname) == Configuration.admin_group
+  end
+
+  # Does the app have permission modification enabled for the GROUP principle
+  # @return [Boolean]
+  def self.group_facl_entry_has_C_set?(pathname)
+    result = false
+    begin
+      result = OodSupport::ACLs::Nfs4ACL.get_facl(
+        path: pathname
+      ).entries.select{
+        |entry| entry.principle == 'GROUP'
+      }.first.permissions.include?(:C)
+    rescue OodSupport::InvalidPath, OodSupport::BadExitCode
+      logger.fatal { "Unable to check FACL for #{pathname} - is it on an NFS4 file system?" }
+    end
+
+    logger.debug { "group_facl_entry_has_C_set?(#{pathname}) == #{result}" }
+
+    result
+  end
 end

app/views/mappings/index.html.erb

@@ -1,7 +1,7 @@
 <% if Mapping.has_directory_permission_errors? %>
   <div class="alert alert-danger" role="alert">
-    <div class="panel-body">Warning: certain directories owned by <code>bmiadmin</code> have incorrect permissions which will prevent app and dataset sharing.. As <code>bmiadmin</code> run the following to set the correct permissions:</div>
-    <div class="panel-body"><pre><code><%= Mapping.directory_permissions_command %></code></pre></div>
+    <div class="panel-body">Warning: certain directories owned by <code>bmiadmin</code> have incorrect permissions which will prevent app and dataset sharing. As <code>bmiadmin</code> run the following to set the correct permissions:</div>
+    <div class="panel-body"><pre><code><%= @directory_permissions_command %></code></pre></div>
   </div>
 <% end %>
 <div class="panel">
@@ -19,10 +19,10 @@
   <tbody>
   <%  Mapping.find_each do |mapping| %>
     <tr>
-      <td><%= mapping.user %></td>
+      <td><%= display_username(mapping.user) %></td>
       <td><%= mapping.app.split.last %></td>
       <td><%= mapping.dataset %></td>
-      <td><%= mapping.is_still_valid? ? fa_icon('check') : fa_icon('exclamation-triangle') %></td>
+      <td><%= mapping.is_still_valid? ? fa_icon('check') : fa_icon('exclamation-triangle') + ' ' + mapping.reason_invalid %></td>
       <td><%= mapping.updated_at %></td>
       <td><%= link_to 'Destroy',  mapping_path(mapping), method: :delete, class: 'btn btn-danger', data: { confirm: 'Are you sure you want to delete this mapping?' } %></td>
     </tr>

app/views/mappings/new.html.erb

@@ -4,8 +4,8 @@
 <%= bootstrap_form_for(:mapping, url: mappings_path) do |f| %>
   <%= f.alert_message 'Please fix the errors below.' %>
 
-  <%= f.select :user, user_list, :include_blank => true, help: user_list_help  %>
-  <%= f.select :app, app_list.map{|uri| [get_app_name(uri), uri]}, :include_blank => true, help: app_list_help %>
+  <%= f.select :user, user_select_list, :include_blank => true, help: user_list_help  %>
+  <%= f.select :app, app_list.map{ |uri| [get_app_name(uri), uri]}, :include_blank => true, help: app_list_help %>
 
   <%= f.form_group :dataset, label: { text: 'Dataset (absolute path)' }, help: known_datasets_help do %>
     <% known_datasets.each do |dataset| %>

app/views/mappings/no_apps_installed.html.erb

@@ -1,8 +1,8 @@
 <div class="container">
   <div class="jumbotron">
     <h1 class="display-4">No Apps Installed</h1>
-    <p>Please install one or more apps to <%= ENV['SHARED_APPS_ROOT'] =%>.</p>
-    <p>Datasets should be installed to <%= ENV['APP_DATASET_ROOT'] =%>.</p>
+    <p>Please install one or more apps to <%= Configuration.shared_apps_root =%>.</p>
+    <p>Datasets should be installed to <%= Configuration.app_dataset_root =%>.</p>
   </div>
   <%= link_to 'Back', mappings_path, class: 'btn btn-default' %>
 </div>

config/configuration_singleton.rb

@@ -96,6 +96,10 @@ def facl_user_domain
     ENV['FACL_USER_DOMAIN']
   end
 
+  def users_from_group
+    ENV['USERS_FROM_GROUP']
+  end
+
   def yaml_file_path
     Pathname.new(ENV['SHARED_APPS_ROOT']).expand_path().join('mappings.yaml')
   end

config/environments/development.rb

@@ -10,6 +10,8 @@
   # config.eager_load = false
   config.eager_load = true
 
+  config.colorize_logging = false
+
   # Show full error reports and disable caching.
   config.consider_all_requests_local       = true
   config.action_controller.perform_caching = false