Merge branch 'feature/api_hide_service_password' of gitlab.com:cernvcs/gitlab-cecernvcs_hide_passwrd

5 files changed, 53 insertions, 3 deletions

diff --git a/CHANGELOG b/CHANGELOG
index c3fd5baa67a..2b6fb5beb36 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -51,6 +51,7 @@ v 8.1.0 (unreleased)
   - Project names are not fully shown if group name is too big, even on group page view
   - Apply new design for Files page
   - Add "New Page" button to Wiki Pages tab (Stan Hu)
+  - Hide passwords from services API (Alex Lossent)
 
 v 8.0.4
   - Fix Message-ID header to be RFC 2111-compliant to prevent e-mails being dropped (Stan Hu)
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 519072d0157..883a5e14b17 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -262,6 +262,18 @@ module API
       expose :notification_level
     end
 
+    class ProjectService < Grape::Entity
+      expose :id, :title, :created_at, :updated_at, :active
+      expose :push_events, :issues_events, :merge_requests_events, :tag_push_events, :note_events
+      # Expose serialized properties
+      expose :properties do |service, options|
+        field_names = service.fields.
+          select { |field| options[:include_passwords] || field[:type] != 'password' }.
+          map { |field| field[:name] }
+        service.properties.slice(*field_names)
+      end
+    end
+
     class ProjectWithAccess < Project
       expose :permissions do
         expose :project_access, using: Entities::ProjectAccess do |project, options|
diff --git a/lib/api/services.rb b/lib/api/services.rb
index 6727e80ac1e..203f04a6259 100644
--- a/lib/api/services.rb
+++ b/lib/api/services.rb
@@ -57,7 +57,7 @@ module API
       #   GET /project/:id/services/gitlab-ci
       #
       get ':id/services/:service_slug' do
-        present project_service
+        present project_service, with: Entities::ProjectService, include_passwords: current_user.is_admin?
       end
     end
   end
diff --git a/spec/requests/api/services_spec.rb b/spec/requests/api/services_spec.rb
index 9aa60826f21..c0226605a23 100644
--- a/spec/requests/api/services_spec.rb
+++ b/spec/requests/api/services_spec.rb
@@ -3,6 +3,8 @@ require "spec_helper"
 describe API::API, api: true  do
   include ApiHelpers
   let(:user) { create(:user) }
+  let(:admin) { create(:admin) }
+  let(:user2) { create(:user) }
   let(:project) {create(:project, creator_id: user.id, namespace: user.namespace) }
 
   Service.available_services_names.each do |service|
@@ -51,11 +53,40 @@ describe API::API, api: true  do
     describe "GET /projects/:id/services/#{service.dasherize}" do
       include_context service
 
-      it "should get #{service} settings" do
+      # inject some properties into the service
+      before do
+        project.build_missing_services
+        service_object = project.send(service_method)
+        service_object.properties = service_attrs
+        service_object.save
+      end
+
+      it 'should return authentication error when unauthenticated' do
+        get api("/projects/#{project.id}/services/#{dashed_service}")
+        expect(response.status).to eq(401)
+      end
+      
+      it "should return all properties of service #{service} when authenticated as admin" do
+        get api("/projects/#{project.id}/services/#{dashed_service}", admin)
+        
+        expect(response.status).to eq(200)
+        expect(json_response['properties'].keys.map(&:to_sym)).to match_array(service_attrs_list.map)
+      end
+
+      it "should return properties of service #{service} other than passwords when authenticated as project owner" do
         get api("/projects/#{project.id}/services/#{dashed_service}", user)
 
         expect(response.status).to eq(200)
+        expect(json_response['properties'].keys.map(&:to_sym)).to match_array(service_attrs_list_without_passwords)
       end
+
+      it "should return error when authenticated but not a project owner" do
+        project.team << [user2, :developer]
+        get api("/projects/#{project.id}/services/#{dashed_service}", user2)
+        
+        expect(response.status).to eq(403)
+      end
+
     end
   end
 end
diff --git a/spec/support/services_shared_context.rb b/spec/support/services_shared_context.rb
index 4d007ae55ee..d1c999cad4d 100644
--- a/spec/support/services_shared_context.rb
+++ b/spec/support/services_shared_context.rb
@@ -3,7 +3,13 @@ Service.available_services_names.each do |service|
     let(:dashed_service) { service.dasherize }
     let(:service_method) { "#{service}_service".to_sym }
     let(:service_klass) { "#{service}_service".classify.constantize }
-    let(:service_attrs_list) { service_klass.new.fields.inject([]) {|arr, hash| arr << hash[:name].to_sym } }
+    let(:service_fields) { service_klass.new.fields }
+    let(:service_attrs_list) { service_fields.inject([]) {|arr, hash| arr << hash[:name].to_sym } }
+    let(:service_attrs_list_without_passwords) do
+      service_fields.
+        select { |field| field[:type] != 'password' }.
+        map { |field| field[:name].to_sym}
+    end
     let(:service_attrs) do
       service_attrs_list.inject({}) do |hash, k|
         if k =~ /^(token*|.*_token|.*_key)/