Merge pull request zendesk#1225 from zendesk/grosser/visible

PAAS-339 allow users to see some secrets

app/controllers/admin/secrets_controller.rb

@@ -28,10 +28,8 @@ def create
   end
 
   def update
-    attributes = {
-      user_id: current_user.id,
-      value:  secret_params.fetch(:value)
-    }
+    attributes = secret_params.slice(:visible, :value)
+    attributes[:user_id] = current_user.id
     if SecretStorage.write(key, attributes)
       successful_response 'Secret created.'
     else
@@ -47,7 +45,7 @@ def destroy
   private
 
   def secret_params
-    @secret_params ||= params.require(:secret).permit(*SecretStorage::SECRET_KEYS_PARTS, :value)
+    @secret_params ||= params.require(:secret).permit(*SecretStorage::SECRET_KEYS_PARTS, :value, :visible)
   end
 
   def key
@@ -77,7 +75,7 @@ def failure_response(message)
   end
 
   def find_secret
-    @secret = SecretStorage.read(key)
+    @secret = SecretStorage.read(key, include_secret: true)
   end
 
   def find_project_permalinks

app/views/admin/secrets/edit.html.erb

@@ -10,14 +10,14 @@
 <section>
   <% url = (id ? admin_secret_path(id) : admin_secrets_path) %>
   <% method = (id ? :put : :post) %>
-  <% data = (id ? SecretStorage.parse_secret_key(id) : (params[:secret] || {})) %>
+  <% secret = (id ? @secret.merge(SecretStorage.parse_secret_key(id)) : (params[:secret] || {})) %>
   <%= form_tag url, class: "form-horizontal", method: method do %>
     <fieldset>
       <div class="form-group">
         <%= label_tag 'secret[environment_permalink]', "Environment", class: "col-lg-2 control-label" %>
         <div class="col-lg-4">
           <%= select_tag 'secret[environment_permalink]',
-            options_for_select(environments, data[:environment_permalink]),
+            options_for_select(environments, secret[:environment_permalink]),
             class: "form-control", include_blank: true, disabled: !!id %>
         </div>
       </div>
@@ -26,7 +26,7 @@
         <%= label_tag 'secret[project_permalink]', "Project", class: "col-lg-2 control-label" %>
         <div class="col-lg-4">
           <%= select_tag 'secret[project_permalink]',
-            options_for_select(@project_permalinks, data[:project_permalink]),
+            options_for_select(@project_permalinks, secret[:project_permalink]),
             class: "form-control", include_blank: true, disabled: !!id %>
         </div>
       </div>
@@ -35,28 +35,35 @@
         <%= label_tag 'secret[deploy_group_permalink]', "Deploy Group", class: "col-lg-2 control-label" %>
         <div class="col-lg-4">
           <%= select_tag 'secret[deploy_group_permalink]',
-            options_for_select([['Loading ...', data[:deploy_group_permalink]]], data[:deploy_group_permalink]),
+            options_for_select([['Loading ...', secret[:deploy_group_permalink]]], secret[:deploy_group_permalink]),
             class: "form-control", include_blank: true, disabled: !!id %>
         </div>
       </div>
 
       <div class="form-group">
         <%= label_tag 'secret[key]', 'Key', class: "col-lg-2 control-label" %>
         <div class="col-lg-6">
-          <%= text_field_tag 'secret[key]', data[:key], class: "form-control", disabled: !!id %>
+          <%= text_field_tag 'secret[key]', secret[:key], class: "form-control", disabled: !!id %>
+        </div>
+      </div>
+
+      <div class="form-group">
+        <%= label_tag 'secret[visible]', 'Visible', class: "col-lg-2 control-label", title: 'Value visible in samson edit UI' %>
+        <div class="col-lg-6">
+          <%= check_box_tag 'secret[visible]', true, secret[:visible], class: "form-control" %>
         </div>
       </div>
 
       <div class="form-group">
         <%= label_tag 'secret[value]', 'Value', class: "col-lg-2 control-label" %>
         <div class="col-lg-6">
-          <%= text_area_tag 'secret[value]', '', class: "form-control", rows: 10 %>
+          <%= text_area_tag 'secret[value]', (secret[:value] if secret[:visible]), class: "form-control", rows: 10 %>
           <div id="value_json_warning" class="alert-danger"></div>
         </div>
       </div>
 
       <% if @secret %>
-        <% @secret.except(:key).each do |attribute, value| %>
+        <% @secret.except(:key, :value, :visible).each do |attribute, value| %>
           <div class="form-group">
             <%= label_tag attribute, attribute.to_s.titlecase, class: "col-lg-2 control-label" %>
             <div class="col-lg-6">

db/migrate/20160818194622_add_visibility_to_secrets.rb

@@ -0,0 +1,6 @@
+# frozen_string_literal: true
+class AddVisibilityToSecrets < ActiveRecord::Migration
+  def change
+    add_column :secrets, :visible, :boolean, default: false, null: false
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160726210144) do
+ActiveRecord::Schema.define(version: 20160818194622) do
 
   create_table "builds", force: :cascade do |t|
     t.integer  "project_id",                                       null: false
@@ -365,15 +365,17 @@
 
   create_table "secrets", id: false, force: :cascade do |t|
     t.string   "id",                 limit: 255
-    t.string   "encrypted_value",    limit: 255, null: false
-    t.string   "encrypted_value_iv", limit: 255, null: false
-    t.string   "encryption_key_sha", limit: 255, null: false
-    t.integer  "updater_id",         limit: 4,   null: false
-    t.integer  "creator_id",         limit: 4,   null: false
-    t.datetime "created_at",                     null: false
-    t.datetime "updated_at",                     null: false
+    t.string   "encrypted_value",    limit: 255,                 null: false
+    t.string   "encrypted_value_iv", limit: 255,                 null: false
+    t.string   "encryption_key_sha", limit: 255,                 null: false
+    t.integer  "updater_id",         limit: 4,                   null: false
+    t.integer  "creator_id",         limit: 4,                   null: false
+    t.datetime "created_at",                                     null: false
+    t.datetime "updated_at",                                     null: false
+    t.boolean  "visible",                        default: false, null: false
   end
 
+  add_index "secrets", ["id"], name: "index_secrets_on_id", unique: true, length: {"id"=>191}, using: :btree
 
   create_table "slack_channels", force: :cascade do |t|
     t.string   "name",       limit: 255, null: false
@@ -384,7 +386,6 @@
   end
 
   add_index "slack_channels", ["stage_id"], name: "index_slack_channels_on_stage_id", using: :btree
-  add_index "secrets", ["id"], name: "index_secrets_on_id", unique: true, length: {"id"=>191}, using: :btree
 
   create_table "slack_identifiers", force: :cascade do |t|
     t.integer  "user_id",    limit: 4

lib/samson/secrets/db_backend.rb

@@ -36,9 +36,10 @@ def read_multi(keys)
 
         def write(key, data)
           secret = Secret.where(id: key).first_or_initialize
+          secret.value = data.fetch(:value)
+          secret.visible = data.fetch(:visible)
           secret.updater_id = data.fetch(:user_id)
           secret.creator_id ||= data.fetch(:user_id)
-          secret.value = data.fetch(:value)
           secret.save
         end
 
@@ -55,11 +56,12 @@ def keys
         def secret_to_hash(secret)
           {
             key: secret.id,
+            value: secret.value,
+            visible: secret.visible,
             updater_id: secret.updater_id,
             creator_id: secret.creator_id,
             updated_at: secret.updated_at,
-            created_at: secret.created_at,
-            value: secret.value
+            created_at: secret.created_at
           }
         end
       end

lib/samson/secrets/hashicorp_vault_backend.rb

@@ -30,7 +30,12 @@ def read_multi(keys)
         end
 
         def write(key, data)
-          vault_client.write(vault_path(key), vault: data[:value])
+          vault_client.write(
+            vault_path(key),
+            vault: data.fetch(:value),
+            visible: data.fetch(:visible),
+            creator_id: data.fetch(:user_id)
+          )
         end
 
         def delete(key)

plugins/kubernetes/test/models/kubernetes/resource_template_test.rb

@@ -113,7 +113,7 @@
         new_metadata = "role: some-role\n      annotations:\n        secret/FOO: bar\n    "
         assert doc.raw_template.sub!(old_metadata, new_metadata)
 
-        SecretStorage.write(secret_key, value: 'something', user_id: 123)
+        create_secret(secret_key)
       end
 
       it "creates a sidecar" do

test/controllers/admin/secrets_controller_test.rb

@@ -82,7 +82,8 @@ def create_global
           project_permalink: 'foo',
           deploy_group_permalink: 'pod2',
           key: 'v',
-          value: 'echo hi'
+          value: 'echo hi',
+          visible: true
         }
       end
 
@@ -93,6 +94,7 @@ def create_global
         secret = SecretStorage::DbBackend::Secret.find('production/foo/pod2/v')
         secret.updater_id.must_equal user.id
         secret.creator_id.must_equal user.id
+        secret.visible.must_equal true
       end
 
       it "redirects to new form when user wants to create another secret" do
@@ -122,6 +124,13 @@ def create_global
         response.body.wont_include secret.value
       end
 
+      it 'shows visible secerts' do
+        secret.update_column(:visible, true)
+        get :edit, id: secret
+        assert_template :edit
+        response.body.must_include secret.value
+      end
+
       it 'renders with unfound users' do
         secret.update_column(:updater_id, 32232323)
         get :edit, id: secret
@@ -136,7 +145,7 @@ def create_global
     end
 
     describe '#update' do
-      let(:attributes) { { value: 'hi' } }
+      let(:attributes) { { value: 'hi', visible: false } }
 
       before do
         patch :update, id: secret.id, secret: attributes
@@ -151,7 +160,9 @@ def create_global
       end
 
       describe 'invalid' do
-        let(:attributes) { { value: '' } }
+        def attributes
+          super.merge(value: '')
+        end
 
         it 'fails to update' do
           assert_template :edit
@@ -160,8 +171,8 @@ def create_global
       end
 
       describe 'updating key' do
-        let(:attributes) do
-          {value: 'hi', project_permalink: other_project.permalink, key: 'bar'}
+        def attributes
+          super.merge(key: 'bar')
         end
 
         it "is not supported" do
@@ -202,13 +213,15 @@ def create_global
 
   as_a_admin do
     let(:secret) { create_global }
+
     describe '#create' do
       let(:attributes) do
         {
           environment_permalink: 'production',
           project_permalink: 'foo',
           deploy_group_permalink: 'pod2',
           key: 'v',
+          visible: true,
           value: 'echo hi'
         }
       end
@@ -243,7 +256,8 @@ def create_global
           project_permalink: 'foo',
           deploy_group_permalink: 'pod2',
           key: 'hi',
-          value: 'secret'
+          value: 'secret',
+          visible: false
         }
         assert_redirected_to admin_secrets_path
       end

test/lib/samson/secrets/db_backend_test.rb

@@ -43,8 +43,8 @@
 
   describe ".write" do
     it "stores the secret" do
-      Samson::Secrets::DbBackend.write(secret.id + 'x', value: 'SECRET', user_id: 1)
-      Samson::Secrets::DbBackend.read(secret.id + 'x')[:value].must_equal 'SECRET'
+      create_secret secret.id + 'x'
+      Samson::Secrets::DbBackend.read(secret.id + 'x')[:value].must_equal 'MY-SECRET'
     end
   end
 

test/lib/samson/secrets/hashicorp_vault_backend_test.rb

@@ -69,8 +69,12 @@
 
   describe ".write" do
     it "writes a key with /secret" do
-      assert Samson::Secrets::HashicorpVaultBackend.write('production/foo/group/isbar/foo', value: 'whatever')
-      client.set.must_equal(secret_namespace + "production/foo/group/isbar%2Ffoo" => {vault: 'whatever'})
+      assert Samson::Secrets::HashicorpVaultBackend.write(
+        'production/foo/group/isbar/foo', value: 'whatever', visible: false, user_id: 1
+      )
+      client.set.must_equal(
+        secret_namespace + "production/foo/group/isbar%2Ffoo" => {vault: 'whatever', visible: false, creator_id: 1}
+      )
     end
   end
 

test/lib/samson/secrets/key_resolver_test.rb

@@ -10,7 +10,7 @@
   let(:errors) { resolver.instance_variable_get(:@errors) }
 
   describe "#expand" do
-    before { SecretStorage.write("global/global/global/bar", value: 'something', user_id: 123) }
+    before { create_secret("global/global/global/bar") }
 
     it "expands" do
       resolver.expand('ABC', 'bar').must_equal [["ABC", "global/global/global/bar"]]
@@ -21,7 +21,7 @@
     end
 
     it "looks up by specificity" do
-      SecretStorage.write("global/#{project.permalink}/global/bar", value: 'something', user_id: 123)
+      create_secret("global/#{project.permalink}/global/bar")
       resolver.expand('ABC', 'bar').must_equal [["ABC", "global/#{project.permalink}/global/bar"]]
     end
 
@@ -58,19 +58,19 @@
       end
 
       it "expands duplicate wildcard key at the right place" do
-        SecretStorage.write("global/global/global/global_global_global", value: 'something', user_id: 123)
+        create_secret("global/global/global/global_global_global")
         resolver.expand('ABC_*', 'global_glob*').must_equal(
           [["ABC_AL_GLOBAL", "global/global/global/global_global_global"]]
         )
       end
 
       it "prioritizes ids with the same key" do
-        SecretStorage.write("global/#{project.permalink}/global/bar", value: 'something', user_id: 123)
+        create_secret("global/#{project.permalink}/global/bar")
         resolver.expand('ABC_*', 'ba*').must_equal [["ABC_R", "global/#{project.permalink}/global/bar"]]
       end
 
       it "finds all matching ids" do
-        SecretStorage.write("global/global/global/baz", value: 'something', user_id: 123)
+        create_secret("global/global/global/baz")
         resolver.expand('ABC_*', 'ba*').sort.must_equal(
           [
             ["ABC_R", "global/global/global/bar"],

test/models/secret_storage_test.rb

@@ -19,7 +19,7 @@
   describe ".write" do
     it "writes" do
       secret_key = 'production/foo/pod2/hello'
-      SecretStorage.write(secret_key, value: '111', user_id: users(:admin).id).must_equal true
+      SecretStorage.write(secret_key, value: '111', user_id: users(:admin).id, visible: false).must_equal true
       secret = SecretStorage::DbBackend::Secret.find(secret_key)
       secret.value.must_equal '111'
       secret.creator_id.must_equal users(:admin).id
@@ -29,24 +29,24 @@
     it "expires keys" do
       SecretStorage.keys
       secret_key = 'production/foo/pod2/hello'
-      SecretStorage.write(secret_key, value: '111', user_id: users(:admin).id).must_equal true
+      SecretStorage.write(secret_key, value: '111', user_id: users(:admin).id, visible: false).must_equal true
       SecretStorage.keys.must_equal [secret_key]
     end
 
     it "refuses to write empty keys" do
-      SecretStorage.write('', value: '111', user_id: 11).must_equal false
+      SecretStorage.write('', value: '111', user_id: 11, visible: false).must_equal false
     end
 
     it "refuses to write keys with spaces" do
-      SecretStorage.write('  production/foo/pod2/hello', value: '111', user_id: 11).must_equal false
+      SecretStorage.write('  production/foo/pod2/hello', value: '111', user_id: 11, visible: false).must_equal false
     end
 
     it "refuses to write empty values" do
-      SecretStorage.write('production/foo/pod2/hello', value: '   ', user_id: 11).must_equal false
+      SecretStorage.write('production/foo/pod2/hello', value: '   ', user_id: 11, visible: false).must_equal false
     end
 
     it "refuses to write keys we will not be able to replace in commands" do
-      SecretStorage.write('a"b', value: '111', user_id: 11).must_equal false
+      SecretStorage.write('a"b', value: '111', user_id: 11, visible: false).must_equal false
     end
   end
 

test/test_helper.rb

@@ -150,6 +150,7 @@ def create_secret(key)
     SecretStorage::DbBackend::Secret.create!(
       id: key,
       value: 'MY-SECRET',
+      visible: false,
       updater_id: users(:admin).id,
       creator_id: users(:admin).id
     )