Replace plain text application secret with copy button

To improve security replace application secret that was displayed in
plain text on the screen with a copy button.

Changelog: changed

app/helpers/button_helper.rb

@@ -22,7 +22,7 @@ module ButtonHelper
   def clipboard_button(data = {})
     css_class = data[:class] || 'btn-clipboard btn-transparent'
     title = data[:title] || _('Copy')
-    button_text = data[:button_text] || ''
+    button_text = data[:button_text] || nil
     hide_tooltip = data[:hide_tooltip] || false
     hide_button_icon = data[:hide_button_icon] || false
     item_prop = data[:itemprop] || nil
@@ -55,8 +55,8 @@ module ButtonHelper
     }
 
     content_tag :button, button_attributes do
-      concat(sprite_icon('copy-to-clipboard')) unless hide_button_icon
-      concat(button_text)
+      concat(sprite_icon('copy-to-clipboard', css_class: ['gl-icon', *('gl-button-icon' unless button_text.nil?)].join(' '))) unless hide_button_icon
+      concat(content_tag(:span, button_text, class: 'gl-button-text')) unless button_text.nil?
     end
   end
 

app/views/admin/applications/_form.html.haml

@@ -40,5 +40,5 @@
       = render 'shared/tokens/scopes_form', prefix: 'doorkeeper_application', token: application, scopes: @scopes
 
   .form-actions
-    = f.submit 'Submit', class: "gl-button btn btn-confirm wide"
-    = link_to "Cancel", admin_applications_path, class: "gl-button btn btn-default btn-cancel"
+    = f.submit _('Save application'), class: "gl-button btn btn-confirm wide"
+    = link_to _('Cancel'), admin_applications_path, class: "gl-button btn btn-default btn-cancel"

app/views/admin/applications/new.html.haml

-- breadcrumb_title _("Applications")
-- page_title _("New Application")
+- breadcrumb_title _("Add new application")
+- page_title _("Add new application")
 
-%h3.page-title New application
+%h3.page-title
+  = _("Add new application")
 - @url = admin_applications_path
 = render 'form', application: @application

app/views/shared/doorkeeper/applications/_show.html.haml

@@ -15,11 +15,7 @@
       %td
         = _('Secret')
       %td
-        .clipboard-group
-          .input-group
-            %input.label.label-monospace.monospace{ id: "secret", type: "text", autocomplete: 'off', value: @application.secret, readonly: true }
-            .input-group-append
-              = clipboard_button(target: '#secret', title: _("Copy secret"), class: "gl-button btn btn-default")
+        = clipboard_button(clipboard_text: @application.secret, button_text: _('Copy'), title: _("Copy secret"), class: "btn btn-default btn-md gl-button")
     %tr
       %td
         = _('Callback URL')

spec/features/admin/admin_manage_applications_spec.rb

@@ -3,62 +3,14 @@
 require 'spec_helper'
 
 RSpec.describe 'admin manage applications' do
+  let_it_be(:new_application_path) { new_admin_application_path }
+  let_it_be(:applications_path) { admin_applications_path }
+
   before do
     admin = create(:admin)
     sign_in(admin)
     gitlab_enable_admin_mode_sign_in(admin)
   end
 
-  it 'creates new oauth application' do
-    visit admin_applications_path
-
-    click_on 'New application'
-    expect(page).to have_content('New application')
-
-    fill_in :doorkeeper_application_name, with: 'test'
-    fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-    check :doorkeeper_application_trusted
-    check :doorkeeper_application_scopes_read_user
-    click_on 'Submit'
-    expect(page).to have_content('Application: test')
-    expect(page).to have_content('Application ID')
-    expect(page).to have_content('Secret')
-    expect(page).to have_content('Trusted Y')
-    expect(page).to have_content('Confidential Y')
-
-    click_on 'Edit'
-    expect(page).to have_content('Edit application')
-
-    fill_in :doorkeeper_application_name, with: 'test_changed'
-    uncheck :doorkeeper_application_trusted
-    uncheck :doorkeeper_application_confidential
-
-    click_on 'Submit'
-    expect(page).to have_content('test_changed')
-    expect(page).to have_content('Application ID')
-    expect(page).to have_content('Secret')
-    expect(page).to have_content('Trusted N')
-    expect(page).to have_content('Confidential N')
-
-    visit admin_applications_path
-    page.within '.oauth-applications' do
-      click_on 'Destroy'
-    end
-    expect(page.find('.oauth-applications')).not_to have_content('test_changed')
-  end
-
-  context 'when scopes are blank' do
-    it 'returns an error' do
-      visit admin_applications_path
-
-      click_on 'New application'
-      expect(page).to have_content('New application')
-
-      fill_in :doorkeeper_application_name, with: 'test'
-      fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-      click_on 'Submit'
-
-      expect(page).to have_content("Scopes can't be blank")
-    end
-  end
+  include_examples 'manage applications'
 end

spec/features/groups/settings/manage_applications_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe 'User manages applications' do
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:new_application_path) { group_settings_applications_path(group) }
+
+  before do
+    group.add_owner(user)
+    sign_in(user)
+  end
+
+  include_examples 'manage applications'
+end

spec/features/profiles/user_manages_applications_spec.rb

@@ -3,55 +3,12 @@
 require 'spec_helper'
 
 RSpec.describe 'User manages applications' do
-  let(:user) { create(:user) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:new_application_path) { applications_profile_path }
 
   before do
     sign_in(user)
-    visit applications_profile_path
   end
 
-  it 'manages applications' do
-    expect(page).to have_content 'Add new application'
-
-    fill_in :doorkeeper_application_name,         with: 'test'
-    fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-    check :doorkeeper_application_scopes_read_user
-    click_on 'Save application'
-
-    expect(page).to have_content 'Application: test'
-    expect(page).to have_content 'Application ID'
-    expect(page).to have_content 'Secret'
-    expect(page).to have_content 'Confidential Yes'
-
-    click_on 'Edit'
-
-    expect(page).to have_content 'Edit application'
-    fill_in :doorkeeper_application_name, with: 'test_changed'
-    uncheck :doorkeeper_application_confidential
-    click_on 'Save application'
-
-    expect(page).to have_content 'test_changed'
-    expect(page).to have_content 'Application ID'
-    expect(page).to have_content 'Secret'
-    expect(page).to have_content 'Confidential No'
-
-    visit applications_profile_path
-
-    page.within '.oauth-applications' do
-      click_on 'Destroy'
-    end
-    expect(page.find('.oauth-applications')).not_to have_content 'test_changed'
-  end
-
-  context 'when scopes are blank' do
-    it 'returns an error' do
-      expect(page).to have_content 'Add new application'
-
-      fill_in :doorkeeper_application_name,         with: 'test'
-      fill_in :doorkeeper_application_redirect_uri, with: 'https://test.com'
-      click_on 'Save application'
-
-      expect(page).to have_content("Scopes can't be blank")
-    end
-  end
+  include_examples 'manage applications'
 end

spec/helpers/button_helper_spec.rb

@@ -174,7 +174,7 @@ RSpec.describe ButtonHelper do
           expect(element.attr('itemprop')).to eq(nil)
           expect(element.inner_text).to eq("")
 
-          expect(element.to_html).to include sprite_icon('copy-to-clipboard')
+          expect(element.to_html).to include sprite_icon('copy-to-clipboard', css_class: 'gl-icon')
         end
       end
 
@@ -195,6 +195,10 @@ RSpec.describe ButtonHelper do
       it 'shows copy to clipboard button with provided `button_text` as button label' do
         expect(element(button_text: 'Copy text').inner_text).to eq('Copy text')
       end
+
+      it 'adds `gl-button-icon` class to icon' do
+        expect(element(button_text: 'Copy text')).to have_css('svg.gl-button-icon')
+      end
     end
 
     context 'with `hide_tooltip` attribute provided' do

spec/support/shared_examples/features/manage_applications_shared_examples.rb

+# frozen_string_literal: true
+
+RSpec.shared_examples 'manage applications' do
+  let_it_be(:application_name) { 'application foo bar' }
+  let_it_be(:application_name_changed) { "#{application_name} changed" }
+  let_it_be(:application_redirect_uri) { 'https://foo.bar' }
+
+  it 'allows user to manage applications' do
+    visit new_application_path
+
+    expect(page).to have_content 'Add new application'
+
+    fill_in :doorkeeper_application_name, with: application_name
+    fill_in :doorkeeper_application_redirect_uri, with: application_redirect_uri
+    check :doorkeeper_application_scopes_read_user
+    click_on 'Save application'
+
+    expect(page).to have_content application_name
+    expect(page).to have_content 'Application ID'
+    expect(page).to have_content 'Secret'
+    expect(page).to have_content 'Confidential Yes'
+
+    application = Doorkeeper::Application.find_by(name: application_name)
+    expect(page).to have_css("button[title=\"Copy secret\"][data-clipboard-text=\"#{application.secret}\"]", text: 'Copy')
+
+    click_on 'Edit'
+
+    application_name_changed = "#{application_name} changed"
+
+    fill_in :doorkeeper_application_name, with: application_name_changed
+    uncheck :doorkeeper_application_confidential
+    click_on 'Save application'
+
+    expect(page).to have_content application_name_changed
+    expect(page).to have_content 'Application ID'
+    expect(page).to have_content 'Secret'
+    expect(page).to have_content 'Confidential No'
+
+    visit_applications_path
+
+    page.within '.oauth-applications' do
+      click_on 'Destroy'
+    end
+    expect(page.find('.oauth-applications')).not_to have_content 'test_changed'
+  end
+
+  context 'when scopes are blank' do
+    it 'returns an error' do
+      visit new_application_path
+
+      expect(page).to have_content 'Add new application'
+
+      fill_in :doorkeeper_application_name, with: application_name
+      fill_in :doorkeeper_application_redirect_uri, with: application_redirect_uri
+      click_on 'Save application'
+
+      expect(page).to have_content("Scopes can't be blank")
+    end
+  end
+
+  def visit_applications_path
+    visit defined?(applications_path) ? applications_path : new_application_path
+  end
+end