Facl fixes

[MorganRodgers]

Addresses:

• Warning message about fixing "sensitive directory" permissions to 775 is erroneous #19
• Warn The User If Key Directories Do Not Have FACL C Attribute Set #21
• Make Mapping Success / Error Messages More Verbose #22 - Including making the log more verbose
• Replace X With Verbose Error Reason in Mapping#show #23
• In Mapping#should_set_facls? Replace owned? with a FACL Check #24
• In users list, add user "gecos" i.e. full names as well #25

Note that after some consideration #20 has been closed as not-a-bug.

[ericfranz]

1. You can accomplish the same thing doing this:

   Pathname.new(super.to_s)

   since nil.to_s == ''

2. Pathname.new('') is not a great "null object". This is a difficult thing with Ruby that I don't have a good answer to right now. Notice:

irb(main):012:0> Pathname.new('').to_s
=> ""
irb(main):013:0> Pathname.new('').exist?
=> false
irb(main):014:0> Pathname.new('').file?
=> false
irb(main):015:0> Pathname.new('').directory?
=> false
irb(main):016:0> Pathname.new('').expand_path
=> #<Pathname:/Users/efranz>
irb(main):016:0> Pathname.new('').expand_path.exist?
=> true
irb(main):025:0> Pathname.new('').expand_path.directory?
=> true

And I might get clever and suggest Pathname.new('/dev/null') but then this has its own problems:

irb(main):022:0> Pathname.new('/dev/null').expand_path
=> #<Pathname:/dev/null> # good!
irb(main):018:0> Pathname.new('/dev/null').directory?
=> false  # good!
irb(main):019:0> Pathname.new('/dev/null').file?
=> false  # good!
irb(main):020:0> Pathname.new('/dev/null').exist?
=> true  # bad!

Of course, nil is normally not desired, because then you have to add guard clauses everywhere and nil is generally a bad idea. But in this case I wonder if we should use it for the "null object", unless there is a sensible null object for Pathname that can be produced (maybe we could make our own? - like Pathname.new('') and then override expand_path and other methods that return values we don't like).

[MorganRodgers]

I was looking for a default constructor or invalid test for Pathname. Using /dev/null is a nice idea, except for its perfectly correct exist? behavior.

[ericfranz]

Instead of having this, why can't we just do in the controller:

mapping = Mapping.find(params[:id])

and in the controller handle the ActiveRecord::RecordNotFound. Is it necessary to handle the ActiveRecord::RecordNotFound, which results in a 404?

[MorganRodgers]

One situation where they would see it is if there is a delay between submitting the first delete and immediately sending another. I know quite a few people who click buttons more than once.

I agree that the OpenStruct usage isn't ideal, it's just something that I was playing with on Friday. I liked that it let me avoid having to add an error handler for RecordNotFound.

[ericfranz]

As far as the multiple button clicks, the presence of the alert should prevent this from occurring:

shiny_admin/app/views/mappings/index.html.erb

Line 27 in 46261c7

<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>

A single click presents the confirmation alert, and only if that is "okay" does the delete request get sent.

[ericfranz]

And of course as we discussed offline, I think its appropriate to not add rescue for RecordNotFound since Rails should handle this to be 404.

[ericfranz]

Some thoughts for refactoring but of course this could wait till its functional.

Seems like there is room for adding a new class PathnameWithFacl - since there are many class methods now whose argument is a Pathname. A possibility.

Also, the way error handling is done could be modified to more closely follow the Rails pattern. You would use https://guides.rubyonrails.org/v4.2/active_record_validations.html#working-with-validation-errors to set the errors on the instance itself, the method that is doing the action would return false, and then add @model.errors messages to the flash or in the view itself. For example, if you look at scaffolding on a default controller create action, you'll see that in the failure situations instead of doing a redirect, in most Rails controllers we render :new instead. That way the instance has access to the error messages.

By using the flash or render the view immediately and use @model.errors to set error messages like these, you can obviate the need for custom views or view blocks for specific error messages.

[ericfranz]

For my previous comment, this is an example: https://github.com/OSC/ood-dashboard/blob/cf9b45dd972d61a832f404e8a6d24bd3bf1d7e6b/app/controllers/batch_connect/session_contexts_controller.rb#L22-L46

if save succeeds, we redirect_to, else, we render :new

[MorganRodgers]

There does seem to be room for an extension to Pathname. Given your comment about the possiblity of us doing more app-sharing, it could be done as an extension of OodSupport?

[MorganRodgers]

Ready for review.

[ericfranz]

When doing render below, you should use flash.now instead of flash:

https://guides.rubyonrails.org/v4.2/action_controller_overview.html#the-flash (see 5.2.1 flash.now)

By default, adding values to the flash will make them available to the next request, but sometimes you may want to access those values in the same request. For example, if the create action fails to save a resource and you render the new template directly, that's not going to result in a new request, but you may still want to display a message using the flash. To do this, you can use flash.now in the same way you use the normal flash:

Note the flash.now is special in the controller. This ensures when rendering the view flash is set so this works whether flash or flash.now was used in the controller:

shiny_admin/app/views/layouts/application.html.erb

Lines 55 to 63 in 46261c7

	<% flash.each do |key, value| %>
	<div class="alert alert-<%= key %> alert-dismissible" role="alert">
	<button type="button" class="close" data-dismiss="alert">
	<span aria-hidden="true">&times;</span>
	<span class="sr-only">Close</span>
	</button>
	<%= value %>
	</div>
	<% end %>

[ericfranz]

Like in other methods, you could put it here with begin, or you could put the error handling at the end:

def destroy
  @mapping = Mapping.find(params[:id])
  if @mapping.destroy_and_remove_facls()
    ...
    redirect_to action: :index
  end
rescue ActiveRecord::RecordNotFound
  flash[:warning] = "Unable to find mapping #{params[:id]} to remove it."
  redirect_to action: :index
end

The nice thing about this is that it looks closer to the default Rails scaffolding, so when you glance at it there is less to think about, the flow of the "happy trail" is easier to read since right after you find you destroy, and you can remove the extra begin/end and return lines, and this is more ruby-esque to place the rescue at the end of the method. But the rescue at the end does distance itself slightly from the location you expect to throw it. Though from this code its obvious what would throw it since it is a small method.

[MorganRodgers]

I didn't know you could skip the begin and final end like that.

[ericfranz]

Might be worth logging when this occurs, even if we are not raising an error. If we are debugging and looking through logs this could be useful to know. Do you know what situations OodSupport::InvalidPath, OodSupport::BadExitCode would occur?

[ericfranz]

Don't need this anymore if you don't use it anywhere.

[ericfranz]

I think the dropdown display value should contain both the gecos and the username. For example, the gecos for an0047 and efranz, both bmishiny users, will be "Eric Franz". Of course, I'm the exception to the rule, and most people will have only 1 account after, but even then it could be useful to verify what username I'm actually sharing with.

irb(main):006:0> puts Etc.getgrnam('bmishiny').mem.map { |user| "#{Etc.getpwnam(user).gecos.strip} - #{user}" }
Morgan Rodgers - mrodgers
Hatice Gulcin Ozer - osu1039
Maciej Pietrzak - osu8150
Sundar Gadepalli - osu8903
Eric Franz - efranz
Eric Franz - an0047
BISR Developers - bmiadmin
Eric Franz - osu10424

[ericfranz]

Like in the dropdown, also show the actual username here too.