Merge branch 'security-id-potential-denial-languages' into 'master'

Return cached languages if they've been detected before

See merge request gitlab/gitlabhq!2998

app/controllers/projects/graphs_controller.rb

@@ -46,13 +46,9 @@ class Projects::GraphsController < Projects::ApplicationController
 
   def get_languages
     @languages =
-      if @project.repository_languages.present?
-        @project.repository_languages.map do |lang|
+      ::Projects::RepositoryLanguagesService.new(@project, current_user).execute.map do |lang|
         { value: lang.share, label: lang.name, color: lang.color, highlight: lang.color }
       end
-      else
-        @project.repository.languages
-      end
   end
 
   def fetch_graph

app/services/projects/detect_repository_languages_service.rb

@@ -2,7 +2,7 @@
 
 module Projects
   class DetectRepositoryLanguagesService < BaseService
-    attr_reader :detected_repository_languages, :programming_languages
+    attr_reader :programming_languages
 
     # rubocop: disable CodeReuse/ActiveRecord
     def execute
@@ -25,6 +25,8 @@ module Projects
           RepositoryLanguage.table_name,
           detection.insertions(matching_programming_languages)
         )
+
+        set_detected_repository_languages
       end
 
       project.repository_languages.reload
@@ -56,5 +58,11 @@ module Projects
       retry
     end
     # rubocop: enable CodeReuse/ActiveRecord
+
+    def set_detected_repository_languages
+      return if project.detected_repository_languages?
+
+      project.update_column(:detected_repository_languages, true)
+    end
   end
 end

app/services/projects/repository_languages_service.rb

+# frozen_string_literal: true
+
+module Projects
+  class RepositoryLanguagesService < BaseService
+    def execute
+      perform_language_detection unless project.detected_repository_languages?
+      persisted_repository_languages
+    end
+
+    private
+
+    def perform_language_detection
+      if persisted_repository_languages.blank?
+        ::DetectRepositoryLanguagesWorker.perform_async(project.id, current_user.id)
+      else
+        project.update_column(:detected_repository_languages, true)
+      end
+    end
+
+    def persisted_repository_languages
+      project.repository_languages
+    end
+  end
+end

changelogs/unreleased/security-id-potential-denial-languages.yml

+---
+title: Return cached languages if they've been detected before
+merge_request:
+author:
+type: security

db/migrate/20190312071108_add_detected_repository_languages_to_projects.rb

+# frozen_string_literal: true
+
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddDetectedRepositoryLanguagesToProjects < ActiveRecord::Migration[5.0]
+  DOWNTIME = false
+
+  def change
+    add_column :projects, :detected_repository_languages, :boolean
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20190301182457) do
+ActiveRecord::Schema.define(version: 20190312071108) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -1739,6 +1739,7 @@ ActiveRecord::Schema.define(version: 20190301182457) do
     t.bigint "pool_repository_id"
     t.string "runners_token_encrypted"
     t.string "bfg_object_map"
+    t.boolean "detected_repository_languages"
     t.index ["ci_id"], name: "index_projects_on_ci_id", using: :btree
     t.index ["created_at"], name: "index_projects_on_created_at", using: :btree
     t.index ["creator_id"], name: "index_projects_on_creator_id", using: :btree

lib/api/projects.rb

@@ -392,11 +392,9 @@ module API
 
       desc 'Get languages in project repository'
       get ':id/languages' do
-        if user_project.repository_languages.present?
-          user_project.repository_languages.map { |l| [l.name, l.share] }.to_h
-        else
-          user_project.repository.languages.map { |language| language.values_at(:label, :value) }.to_h
-        end
+        ::Projects::RepositoryLanguagesService
+          .new(user_project, current_user)
+          .execute.map { |lang| [lang.name, lang.share] }.to_h
       end
 
       desc 'Remove a project'

lib/gitlab/import_export/import_export.yml

@@ -117,6 +117,7 @@ excluded_attributes:
     - :description_html
     - :repository_languages
     - :bfg_object_map
+    - :detected_repository_languages
     - :tag_list
   namespaces:
     - :runners_token

spec/controllers/projects/graphs_controller_spec.rb

@@ -27,6 +27,7 @@ describe Projects::GraphsController do
 
   describe 'charts' do
     context 'when languages were previously detected' do
+      let(:project) { create(:project, :repository, detected_repository_languages: true) }
       let!(:repository_language) { create(:repository_language, project: project) }
 
       it 'sets the languages properly' do

spec/features/projects/graph_spec.rb

@@ -6,6 +6,8 @@ describe 'Project Graph', :js do
   let(:branch_name) { 'master' }
 
   before do
+    ::Projects::DetectRepositoryLanguagesService.new(project, user).execute
+
     project.add_maintainer(user)
 
     sign_in(user)

spec/requests/api/projects_spec.rb

@@ -13,12 +13,18 @@ shared_examples 'languages and percentages JSON response' do
     )
   end
 
+  context "when the languages haven't been detected yet" do
     it 'returns expected language values' do
       get api("/projects/#{project.id}/languages", user)
 
       expect(response).to have_gitlab_http_status(:ok)
-    expect(json_response).to eq(expected_languages)
-    expect(json_response.count).to be > 1
+      expect(json_response).to eq({})
+
+      get api("/projects/#{project.id}/languages", user)
+
+      expect(response).to have_gitlab_http_status(:ok)
+      expect(JSON.parse(response.body)).to eq(expected_languages)
+    end
   end
 
   context 'when the languages were detected before' do

spec/services/projects/detect_repository_languages_service_spec.rb

@@ -19,6 +19,10 @@ describe Projects::DetectRepositoryLanguagesService, :clean_gitlab_redis_shared_
 
         expect(names).to eq(%w[Ruby JavaScript HTML CoffeeScript])
       end
+
+      it 'updates detected_repository_languages flag' do
+        expect { subject.execute }.to change(project, :detected_repository_languages).to(true)
+      end
     end
 
     context 'with a previous detection' do
@@ -36,6 +40,12 @@ describe Projects::DetectRepositoryLanguagesService, :clean_gitlab_redis_shared_
 
         expect(repository_languages).to eq(%w[Ruby D])
       end
+
+      it "doesn't touch detected_repository_languages flag" do
+        expect(project).not_to receive(:update_column).with(:detected_repository_languages, true)
+
+        subject.execute
+      end
     end
 
     context 'when no repository exists' do

spec/services/projects/repository_languages_service_spec.rb

+require 'spec_helper'
+
+describe Projects::RepositoryLanguagesService do
+  let(:service) { described_class.new(project, project.owner) }
+
+  context 'when detected_repository_languages flag is set' do
+    let(:project) { create(:project) }
+
+    context 'when a project is without detected programming languages' do
+      it 'schedules a worker and returns the empty result' do
+        expect(::DetectRepositoryLanguagesWorker).to receive(:perform_async).with(project.id, project.owner.id)
+        expect(service.execute).to eq([])
+      end
+    end
+
+    context 'when a project is with detected programming languages' do
+      let!(:repository_language) { create(:repository_language, project: project) }
+
+      it 'does not schedule a worker and returns the detected languages' do
+        expect(::DetectRepositoryLanguagesWorker).not_to receive(:perform_async).with(project.id, project.owner.id)
+
+        languages = service.execute
+
+        expect(languages.size).to eq(1)
+        expect(languages.last.attributes.values).to eq(
+          [project.id, repository_language.programming_language_id, repository_language.share]
+        )
+      end
+
+      it 'sets detected_repository_languages flag' do
+        expect { service.execute }.to change(project, :detected_repository_languages).from(nil).to(true)
+      end
+    end
+  end
+
+  context 'when detected_repository_languages flag is not set' do
+    let!(:repository_language) { create(:repository_language, project: project) }
+    let(:project) { create(:project, detected_repository_languages: true) }
+    let(:languages) { service.execute }
+
+    it 'returns repository languages' do
+      expect(languages.size).to eq(1)
+      expect(languages.last.attributes.values).to eq(
+        [project.id, repository_language.programming_language_id, repository_language.share]
+      )
+    end
+  end
+end