Merge branch 'authorized-keys-permission-check' into 'master'

Improve authorized_keys check

The old check only looked if authorized_keys exists. With this change, we look
whether we can actually open the file for reading and writing. When this fails
we try to print useful diagnostic information.

See merge request !79

3 files changed, 36 insertions, 6 deletions

diff --git a/bin/check b/bin/check
index 363cb6a..d34a2d0 100755
--- a/bin/check
+++ b/bin/check
@@ -19,14 +19,12 @@ rescue GitlabNet::ApiUnreachableError
   abort "FAILED: Failed to connect to internal API"
 end
 
-
-puts "\nCheck directories and files: "
-
 config = GitlabConfig.new
 
 abort("ERROR: missing option in config.yml") unless config.auth_file
-print "\t#{config.auth_file}: "
-if File.exists?(config.auth_file)
+
+print "\nAccess to #{config.auth_file}: "
+if system(File.dirname(__FILE__) + '/gitlab-keys', 'check-permissions')
   print 'OK'
 else
   abort "FAILED"
diff --git a/lib/gitlab_keys.rb b/lib/gitlab_keys.rb
index e1b62ad..c719e8d 100644
--- a/lib/gitlab_keys.rb
+++ b/lib/gitlab_keys.rb
@@ -21,6 +21,7 @@ class GitlabKeys
     when 'rm-key';  rm_key
     when 'list-keys'; puts list_keys
     when 'clear';  clear
+    when 'check-permissions'; check_permissions
     else
       $logger.warn "Attempt to execute invalid gitlab-keys command #{@command.inspect}."
       puts 'not allowed'
@@ -92,6 +93,18 @@ class GitlabKeys
     true
   end
 
+  def check_permissions
+    open_auth_file('r+') { true }
+  rescue => ex
+    puts "error: could not open #{auth_file}: #{ex}"
+    if File.exist?(auth_file)
+      system('ls', '-l', auth_file)
+    else
+      # Maybe the parent directory is not writable?
+      system('ls', '-ld', File.dirname(auth_file))
+    end
+    false
+  end
 
   def lock(timeout = 10)
     File.open(lock_file, "w+") do |f|
diff --git a/spec/gitlab_keys_spec.rb b/spec/gitlab_keys_spec.rb
index 5afa467..d761cc4 100644
--- a/spec/gitlab_keys_spec.rb
+++ b/spec/gitlab_keys_spec.rb
@@ -15,7 +15,6 @@ describe GitlabKeys do
     it { gitlab_keys.instance_variable_get(:@key_id).should == 'key-741' }
   end
 
-
   describe :add_key do
     let(:gitlab_keys) { build_gitlab_keys('add-key', 'key-741', 'ssh-rsa AAAAB3NzaDAxx2E') }
 
@@ -145,6 +144,20 @@ describe GitlabKeys do
     end
   end
 
+  describe :check_permissions do
+    let(:gitlab_keys) { build_gitlab_keys('check-permissions') }
+
+    it 'returns true when the file can be opened' do
+      create_authorized_keys_fixture
+      expect(gitlab_keys.exec).to eq(true)
+    end
+
+    it 'returns false if opening raises an exception' do
+      gitlab_keys.should_receive(:open_auth_file).and_raise("imaginary error")
+      expect(gitlab_keys.exec).to eq(false)
+    end
+  end
+
   describe :exec do
     it 'add-key arg should execute add_key method' do
       gitlab_keys = build_gitlab_keys('add-key')
@@ -170,6 +183,12 @@ describe GitlabKeys do
       gitlab_keys.exec
     end
 
+    it 'check-permissions arg should execute check_permissions method' do
+      gitlab_keys = build_gitlab_keys('check-permissions')
+      gitlab_keys.should_receive(:check_permissions)
+      gitlab_keys.exec
+    end
+
     it 'should puts message if unknown command arg' do
       gitlab_keys = build_gitlab_keys('change-key')
       gitlab_keys.should_receive(:puts).with('not allowed')