More launcher id checks (#3774)

Only set launcher @id once and check against regex and add some more checks when saving.

apps/dashboard/app/models/launcher.rb

@@ -60,14 +60,13 @@ def scripts?(project_dir)
 
   ID_REX = /\A\w{8}\Z/.freeze
 
-  validates(:id, format: { with: ID_REX, allow_blank: true, message: :format }, on: [:save])
-  validates(:id, format: { with: ID_REX, message: :format }, on: [:update])
+  validates(:id, format: { with: ID_REX, message: "ID does not match #{Launcher::ID_REX.inspect}" }, on: [:save])
 
   def initialize(opts = {})
     opts = opts.to_h.with_indifferent_access
 
     @project_dir = opts[:project_dir] || raise(StandardError, 'You must set the project directory')
-    @id = opts[:id] if opts[:id].to_s.empty? || opts[:id].to_s.match?(ID_REX)
+    @id = opts[:id].to_s.match?(ID_REX) ? opts[:id].to_s : Launcher.next_id
     @title = opts[:title].to_s
     @created_at = opts[:created_at]
     sm_opts = {
@@ -149,9 +148,11 @@ def []=(_id, value)
   end
 
   def save
-    @id = Launcher.next_id if @id.nil? || !@id.to_s.match?(ID_REX)
+    return false unless valid?(:save)
+
     @created_at = Time.now.to_i if @created_at.nil?
     script_path = Launcher.script_path(project_dir, id)
+
     script_path.mkpath unless script_path.exist?
     File.write(Launcher.script_form_file(script_path), to_yaml)
 
@@ -223,6 +224,10 @@ def create_default_script
   private
 
   def self.script_path(root_dir, script_id)
+    unless script_id.to_s.match?(ID_REX)
+      raise(StandardError, "#{script_id} is invalid. Does not match #{ID_REX.inspect}")
+    end
+
     Pathname.new(File.join(Launcher.scripts_dir(root_dir), script_id.to_s))
   end
 

apps/dashboard/test/models/launcher_test.rb

@@ -95,12 +95,16 @@ def setup
     end
   end
 
-  test 'launchers will not assign wrong id' do
+  test 'launchers will re-assign wrong id' do
     Dir.mktmpdir do |tmp|
       projects_path = Pathname.new(tmp)
       OodAppkit.stubs(:dataroot).returns(projects_path)
-      launcher = Launcher.new({ project_dir: projects_path.to_s, id: '1234', title: 'Test Script' })
-      assert_nil(launcher.id)
+      bad_id = '1234'
+      launcher = Launcher.new({ project_dir: projects_path.to_s, id: bad_id, title: 'Test Script' })
+
+      assert(launcher.id.to_s.match?(Launcher::ID_REX))
+      refute(bad_id.match?(Launcher::ID_REX))
+      refute(bad_id.to_s == launcher.id)
     end
   end
 
@@ -134,4 +138,23 @@ def setup
       assert_equal false, Pathname(File.join(projects_path, 'hello_world.sh')).exist?
     end
   end
+
+  test 'will not save even if id is resest' do
+    Dir.mktmpdir do |tmp|
+      bad_id = '1234'
+      launcher = Launcher.new({ project_dir: tmp.to_s, id: bad_id, title: 'Default Script' })
+
+      # initializer reset the id, but we can reset it
+      refute(launcher.id.to_s == bad_id.to_s)
+      launcher.instance_variable_set('@id', bad_id)
+      assert_equal(launcher.id, bad_id)
+      assert(launcher.errors.size, 0)
+
+      # now try to save it, and it fails
+      refute(launcher.save)
+      assert(launcher.errors.size, 1)
+      assert_equal(launcher.errors.full_messages[0], "Id ID does not match #{Launcher::ID_REX.inspect}")
+      assert(Dir.empty?(Launcher.scripts_dir(tmp).to_s))
+    end
+  end
 end

apps/dashboard/test/system/project_manager_test.rb

@@ -728,18 +728,18 @@ def add_auto_environment_variable(project_id, script_id, save: true)
   test 'cant show invalid script' do
     Dir.mktmpdir do |dir|
       project_id = setup_project(dir)
-      visit project_launcher_path(project_id, '1')
+      visit project_launcher_path(project_id, '12345678')
       assert_current_path("/projects/#{project_id}")
-      assert_selector('.alert-danger', text: "Close\nCannot find script 1")
+      assert_selector('.alert-danger', text: "Close\nCannot find script 12345678")
     end
   end
 
   test 'cant edit invalid script' do
     Dir.mktmpdir do |dir|
       project_id = setup_project(dir)
-      visit edit_project_launcher_path(project_id, '1')
+      visit edit_project_launcher_path(project_id, '12345678')
       assert_current_path("/projects/#{project_id}")
-      assert_selector('.alert-danger', text: "Close\nCannot find script 1")
+      assert_selector('.alert-danger', text: "Close\nCannot find script 12345678")
     end
   end