Facl fixes

app/controllers/mappings_controller.rb

@@ -3,6 +3,7 @@
 
 class MappingsController < ApplicationController
   def index
+    @directory_permissions_command = Mapping.directory_permissions_command
   end
 
 
@@ -18,18 +19,20 @@ def create
       flash[:success] = @mapping.save_message
       redirect_to action: :index
     else
-      flash[:warning] = 'Unable to create new mapping. ' + @mapping.save_message
+      flash[:warning] = 'Unable to create new mapping. ' + @mapping.format_error_messages
       redirect_to new_mapping_path, locals: params
     end
   end
 
   # POST /mappings
-  def destroy 
-    if Mapping.destroy_and_remove_facls(params[:id])
-      flash[:success] = 'Successfully deleted mapping.'
+  def destroy
+    mapping = Mapping.get_mapping_for_id(params[:id])
+
+    if mapping.destroy_and_remove_facls()
+      flash[:success] = mapping.save_message
       redirect_to action: :index
     else
-      flash[:danger] = 'Unable to delete mapping ' + params[:id]
+      flash[:danger] = mapping.save_message
       redirect_to action: :index
     end
   end

app/models/mapping.rb

@@ -1,24 +1,32 @@
 require 'etc'
 require 'fileutils'
 require 'ood_support'
+require 'ostruct'
 require 'pathname'
 require 'yaml/store'
 
 
 class Mapping < ActiveRecord::Base
   attr_accessor :save_message
-  validates :user, :app, :dataset, presence: true
+  validates :user, :app, :dataset, presence: { message: "A %{attribute} must be selected." }
   validate :dataset_path_must_exist
+  validate :app_path_may_not_be_blank
   validates_uniqueness_of :user, scope: [:user, :app], message: "Unable to create a second mapping between user and app."
 
+  def initialize(params)
+    super(params)
+
+    @save_message = ''
+  end
+
   # Type dataset as a Pathname
   def dataset
-    Pathname.new(super)
+    super ? Pathname.new(super): Pathname.new('')
   end
 
   # Type app as a Pathname
   def app
-    Pathname.new(super)
+    super ? Pathname.new(super): Pathname.new('')
   end
 
   # @return [Array<String>]
@@ -55,28 +63,29 @@ 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
+      remove_rx_facl(dataset)
+      remove_rx_facl(app)
+      destroy
+      Mapping.dump_to_yaml
       @save_message = 'Mapping successfully destroyed.'
 
       return true
     rescue OodSupport::InvalidPath, OodSupport::BadExitCode => e
       @save_message = '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(' ')
+    @save_message = errors.messages.map do |key, message|
+      return message.join(' ') unless key == :base
+
+      message
+    end.join(' ')
   end
 
   # Custom save method
@@ -95,10 +104,6 @@ def save_and_set_facls
       @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
 
@@ -126,13 +131,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 +146,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 +161,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 +173,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
@@ -221,9 +230,60 @@ 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.
+  #
+  # @return [Boolean]
+  def self.can_modify_facl?(pathname)
+    pathname.exist? && ( pathname.owned? || 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 = OodSupport::ACLs::Nfs4ACL.get_facl(
+      path: pathname
+    ).entries.select{
+      |entry| entry.principle == 'GROUP'
+    }.first.permissions.include?(:C)
+
+    logger.debug "group_facl_entry_has_C_set?(#{pathname}) == #{result}"
+
+    result
+  end
+
+  def self.get_mapping_for_id(id)
+    begin
+      return Mapping.find(id)
+    rescue ActiveRecord::RecordNotFound
+      OpenStruct.new(
+        :destroy_and_remove_facls => false,
+        :save_message => "No mapping exists with id #{id}"
+      )
+    end
+  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">

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>