From d392f147fc2b08cf3139e2cce2a264eaf0bc4a48 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 7 Sep 2016 14:52:13 +0200
Subject: Group similar builds

---
 app/models/commit_status.rb                                  |  4 ++++
 app/views/projects/commit/_pipeline.html.haml                |  9 +++++++--
 app/views/projects/commit/_pipeline_grouped_status.html.haml | 12 ++++++++++++
 db/fixtures/development/14_pipelines.rb                      | 10 +++++++---
 4 files changed, 30 insertions(+), 5 deletions(-)
 create mode 100644 app/views/projects/commit/_pipeline_grouped_status.html.haml

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 4a628924499..af739342256 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -95,6 +95,10 @@ class CommitStatus < ActiveRecord::Base
     pipeline.before_sha || Gitlab::Git::BLANK_SHA
   end
 
+  def group_name
+    name.gsub(/\d+[\s:]+\d+\s*/, '')
+  end
+
   def self.stages
     # We group by stage name, but order stages by theirs' index
     unscoped.from(all, :sg).group('stage').order('max(stage_idx)', 'stage').pluck('sg.stage')
diff --git a/app/views/projects/commit/_pipeline.html.haml b/app/views/projects/commit/_pipeline.html.haml
index 20a85148ab5..a330c14061f 100644
--- a/app/views/projects/commit/_pipeline.html.haml
+++ b/app/views/projects/commit/_pipeline.html.haml
@@ -39,8 +39,13 @@
               = stage.titleize
           .builds-container
             %ul
-              - statuses.each do |status|
-                = render "projects/#{status.to_partial_path}_pipeline", subject: status
+              - status_groups = statuses.group_by(&:group_name)
+              - status_groups.each do |group_name, grouped_statuses|
+                - if grouped_statuses.one?
+                  - status = grouped_statuses.first
+                  = render "projects/#{status.to_partial_path}_pipeline", subject: status
+                - else
+                  = render "projects/commit/pipeline_grouped_status", name: group_name, subject: grouped_statuses
 
 
 - if pipeline.yaml_errors.present?
diff --git a/app/views/projects/commit/_pipeline_grouped_status.html.haml b/app/views/projects/commit/_pipeline_grouped_status.html.haml
new file mode 100644
index 00000000000..7e02703f0a6
--- /dev/null
+++ b/app/views/projects/commit/_pipeline_grouped_status.html.haml
@@ -0,0 +1,12 @@
+%li.build
+  .curve
+  .build-content
+    - group_status = CommitStatus.where(id: subject).status
+    = render_status_with_link('build', group_status)
+    %span.ci-status-text
+      = name
+      = subject.length
+
+    // Access all other grouped statuses
+    //- subject.each do |status|
+    //  = render "projects/#{status.to_partial_path}_pipeline", subject: status
diff --git a/db/fixtures/development/14_pipelines.rb b/db/fixtures/development/14_pipelines.rb
index 49e6e2361b1..650b410595c 100644
--- a/db/fixtures/development/14_pipelines.rb
+++ b/db/fixtures/development/14_pipelines.rb
@@ -3,9 +3,13 @@ class Gitlab::Seeder::Pipelines
   BUILDS = [
     { name: 'build:linux', stage: 'build', status: :success },
     { name: 'build:osx', stage: 'build', status: :success },
-    { name: 'rspec:linux', stage: 'test', status: :success },
-    { name: 'rspec:windows', stage: 'test', status: :success },
-    { name: 'rspec:windows', stage: 'test', status: :success },
+    { name: 'rspec:linux 0 3', stage: 'test', status: :success },
+    { name: 'rspec:linux 1 3', stage: 'test', status: :success },
+    { name: 'rspec:linux 2 3', stage: 'test', status: :success },
+    { name: 'rspec:windows 0 3', stage: 'test', status: :success },
+    { name: 'rspec:windows 1 3', stage: 'test', status: :success },
+    { name: 'rspec:windows 2 3', stage: 'test', status: :success },
+    { name: 'rspec:windows 2 3', stage: 'test', status: :success },
     { name: 'rspec:osx', stage: 'test', status_event: :success },
     { name: 'spinach:linux', stage: 'test', status: :success },
     { name: 'spinach:osx', stage: 'test', status: :failed, allow_failure: true},
-- 
cgit v1.2.1


From 1ef8be768df68b42d238ca9368aea65982ad0659 Mon Sep 17 00:00:00 2001
From: Annabel Dunstone Gray <annabel.dunstone@gmail.com>
Date: Wed, 7 Sep 2016 10:31:32 -0500
Subject: Style grouped builds dropdown

---
 app/assets/stylesheets/pages/pipelines.scss        | 81 ++++++++++++++++++++--
 .../projects/ci/builds/_build_pipeline.html.haml   |  4 +-
 .../commit/_pipeline_grouped_status.html.haml      | 29 ++++++--
 3 files changed, 99 insertions(+), 15 deletions(-)

diff --git a/app/assets/stylesheets/pages/pipelines.scss b/app/assets/stylesheets/pages/pipelines.scss
index 2d66ab25da6..cc71b8eb045 100644
--- a/app/assets/stylesheets/pages/pipelines.scss
+++ b/app/assets/stylesheets/pages/pipelines.scss
@@ -318,9 +318,17 @@
 
     .build-content {
       width: 130px;
-      white-space: nowrap;
-      overflow: hidden;
-      text-overflow: ellipsis;
+
+      .ci-status-text {
+        width: 110px;
+        white-space: nowrap;
+        overflow: hidden;
+        text-overflow: ellipsis;
+        vertical-align: middle;
+        display: inline-block;
+        position: relative;
+        top: -1px;
+      }
 
       a {
         color: $layout-link-gray;
@@ -331,13 +339,74 @@
             text-decoration: underline;
           }
         }
+      }
+
+      .dropdown-menu-toggle {
+        border: none;
+        width: auto;
+        padding: 0;
+        color: $layout-link-gray;
+
+        .ci-status-text {
+          width: 80px;
+        }
+      }
+
+      .grouped-pipeline-dropdown {
+        padding: 8px 0;
+        width: 200px;
+        left: auto;
+        right: -214px;
+        top: -9px;
+
+        a:hover {
+          .ci-status-text {
+            text-decoration: none;
+          }
+        }
+
+        .ci-status-text {
+          width: 145px;
+        }
+
+        .arrow {
+          &:before,
+          &:after {
+            content: '';
+            display: inline-block;
+            position: absolute;
+            width: 0;
+            height: 0;
+            border-color: transparent;
+            border-style: solid;
+            top: 18px;
+          }
+
+          &:before {
+            left: -5px;
+            margin-top: -6px;
+            border-width: 7px 5px 7px 0;
+            border-right-color: $border-color;
+          }
 
+          &:after {
+            left: -4px;
+            margin-top: -9px;
+            border-width: 10px 7px 10px 0;
+            border-right-color: $white-light;
+          }
+        }
+      }
+
+      .badge {
+        background-color: $gray-dark;
+        color: $layout-link-gray;
+        font-weight: normal;
       }
     }
 
     svg {
-      position: relative;
-      top: 2px;
+      vertical-align: middle;
       margin-right: 5px;
     }
 
@@ -442,7 +511,7 @@
       width: 21px;
       height: 25px;
       position: absolute;
-      top: -28.5px;
+      top: -29px;
       border-top: 2px solid $border-color;
     }
 
diff --git a/app/views/projects/ci/builds/_build_pipeline.html.haml b/app/views/projects/ci/builds/_build_pipeline.html.haml
index 36fb0300aeb..5289cd672f5 100644
--- a/app/views/projects/ci/builds/_build_pipeline.html.haml
+++ b/app/views/projects/ci/builds/_build_pipeline.html.haml
@@ -5,11 +5,11 @@
     - if is_playable
       = link_to play_namespace_project_build_path(subject.project.namespace, subject.project, subject, return_to: request.original_url), method: :post, title: 'Play' do
         = render_status_with_link('build', 'play')
-        %span.ci-status-text= subject.name
+        .ci-status-text= subject.name
     - elsif can?(current_user, :read_build, @project)
       = link_to namespace_project_build_path(subject.project.namespace, subject.project, subject) do
         = render_status_with_link('build', subject.status)
-        %span.ci-status-text= subject.name
+        .ci-status-text= subject.name
     - else
       = render_status_with_link('build', subject.status)
       = ci_icon_for_status(subject.status)
diff --git a/app/views/projects/commit/_pipeline_grouped_status.html.haml b/app/views/projects/commit/_pipeline_grouped_status.html.haml
index 7e02703f0a6..dc8efc83d48 100644
--- a/app/views/projects/commit/_pipeline_grouped_status.html.haml
+++ b/app/views/projects/commit/_pipeline_grouped_status.html.haml
@@ -3,10 +3,25 @@
   .build-content
     - group_status = CommitStatus.where(id: subject).status
     = render_status_with_link('build', group_status)
-    %span.ci-status-text
-      = name
-      = subject.length
-
-    // Access all other grouped statuses
-    //- subject.each do |status|
-    //  = render "projects/#{status.to_partial_path}_pipeline", subject: status
+    .dropdown.inline
+      %button.dropdown-menu-toggle{type: 'button', data: {toggle: 'dropdown'}}
+        %span.ci-status-text
+          = name
+        %span.badge= subject.length
+      %ul.dropdown-menu.grouped-pipeline-dropdown
+        .arrow
+        - subject.each do |status|
+          -# = render "projects/#{status.to_partial_path}_pipeline", subject: status
+          - is_playable = status.playable? && can?(current_user, :update_build, @project)
+          %li
+            - if is_playable
+              = link_to play_namespace_project_build_path(status.project.namespace, status.project, status, return_to: request.original_url), method: :post, title: 'Play' do
+                = render_status_with_link('build', 'play')
+                .ci-status-text= status.name
+            - elsif can?(current_user, :read_build, @project)
+              = link_to namespace_project_build_path(status.project.namespace, status.project, status) do
+                = render_status_with_link('build', status.status)
+                .ci-status-text= status.name
+            - else
+              = render_status_with_link('build', status.status)
+              = ci_icon_for_status(status.status)
-- 
cgit v1.2.1


From 287663b2b11c052906aefe42354f75718f980568 Mon Sep 17 00:00:00 2001
From: Phil Hughes <me@iamphill.com>
Date: Fri, 9 Sep 2016 15:11:37 +0100
Subject: Trigger autosize update after template selection

Closes #21982
---
 CHANGELOG                                         |  1 +
 app/assets/javascripts/blob/template_selector.js  |  7 +++++++
 spec/features/projects/issuable_templates_spec.rb | 13 +++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/CHANGELOG b/CHANGELOG
index f528ca074fa..d45b1694ff9 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -27,6 +27,7 @@ v 8.12.0 (unreleased)
   - Fix bug where pagination is still displayed despite all todos marked as done (ClemMakesApps)
   - Center build stage columns in pipeline overview (ClemMakesApps)
   - Rename behaviour to behavior in bug issue template for consistency (ClemMakesApps)
+  - Fix bug stopping issue description being scrollable after selecting issue template
   - Remove suggested colors hover underline (ClemMakesApps)
   - Shorten task status phrase (ClemMakesApps)
   - Fix project visibility level fields on settings
diff --git a/app/assets/javascripts/blob/template_selector.js b/app/assets/javascripts/blob/template_selector.js
index b0a37ef0e0a..4866add779d 100644
--- a/app/assets/javascripts/blob/template_selector.js
+++ b/app/assets/javascripts/blob/template_selector.js
@@ -13,6 +13,9 @@
       this.buildDropdown();
       this.bindEvents();
       this.onFilenameUpdate();
+
+      this.autosizeUpdateEvent = document.createEvent('Event');
+      this.autosizeUpdateEvent.initEvent('autosize:update', true, false);
     }
 
     TemplateSelector.prototype.buildDropdown = function() {
@@ -69,6 +72,10 @@
     TemplateSelector.prototype.requestFileSuccess = function(file, skipFocus) {
       this.editor.setValue(file.content, 1);
       if (!skipFocus) this.editor.focus();
+
+      if (this.editor instanceof jQuery) {
+        this.editor.get(0).dispatchEvent(this.autosizeUpdateEvent);
+      }
     };
 
     TemplateSelector.prototype.startLoadingSpinner = function() {
diff --git a/spec/features/projects/issuable_templates_spec.rb b/spec/features/projects/issuable_templates_spec.rb
index 4a83740621a..d0f4e5469ed 100644
--- a/spec/features/projects/issuable_templates_spec.rb
+++ b/spec/features/projects/issuable_templates_spec.rb
@@ -13,10 +13,12 @@ feature 'issuable templates', feature: true, js: true do
 
   context 'user creates an issue using templates' do
     let(:template_content) { 'this is a test "bug" template' }
+    let(:longtemplate_content) { %Q(this\n\n\n\n\nis\n\n\n\n\na\n\n\n\n\nbug\n\n\n\n\ntemplate) }
     let(:issue) { create(:issue, author: user, assignee: user, project: project) }
 
     background do
       project.repository.commit_file(user, '.gitlab/issue_templates/bug.md', template_content, 'added issue template', 'master', false)
+      project.repository.commit_file(user, '.gitlab/issue_templates/test.md', longtemplate_content, 'added issue template', 'master', false)
       visit edit_namespace_project_issue_path project.namespace, project, issue
       fill_in :'issue[title]', with: 'test issue title'
     end
@@ -27,6 +29,17 @@ feature 'issuable templates', feature: true, js: true do
       preview_template
       save_changes
     end
+
+    it 'updates height of markdown textarea' do
+      start_height = page.evaluate_script('$(".markdown-area").outerHeight()')
+
+      select_template 'test'
+      wait_for_ajax
+
+      end_height = page.evaluate_script('$(".markdown-area").outerHeight()')
+      
+      expect(end_height).not_to eq(start_height)
+    end
   end
 
   context 'user creates a merge request using templates' do
-- 
cgit v1.2.1


From bc9700a6a848886ae5fde6475d4a68707e46df15 Mon Sep 17 00:00:00 2001
From: Christian Couder <chriscool@tuxfamily.org>
Date: Mon, 12 Sep 2016 07:48:32 +0000
Subject: doc/ci/quick_start/README: improve sentence about link to a Lint tool

The link is now a button under **Pipelines > Pipelines** and **Pipelines > Builds**.
There is no more **CI settings** menu.
---
 doc/ci/quick_start/README.md | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/doc/ci/quick_start/README.md b/doc/ci/quick_start/README.md
index c835ebc2d44..c40cdd55ea5 100644
--- a/doc/ci/quick_start/README.md
+++ b/doc/ci/quick_start/README.md
@@ -105,7 +105,8 @@ What is important is that each job is run independently from each other.
 
 If you want to check whether your `.gitlab-ci.yml` file is valid, there is a
 Lint tool under the page `/ci/lint` of your GitLab instance. You can also find
-the link under **Settings > CI settings** in your project.
+a "CI Lint" button to go to this page under **Pipelines > Pipelines** and
+**Pipelines > Builds** in your project.
 
 For more information and a complete `.gitlab-ci.yml` syntax, please read
 [the documentation on .gitlab-ci.yml](../yaml/README.md).
-- 
cgit v1.2.1


From ff423bbb290f3bcb8b6d2859c67d428ac80f681e Mon Sep 17 00:00:00 2001
From: Christian Couder <chriscool@tuxfamily.org>
Date: Mon, 12 Sep 2016 10:14:11 +0200
Subject: doc/user/permissions: update option and menu names

It looks like the option is now called **Public pipelines**
and the menu item is now **Project Settings > CI/CD Pipelines**.
---
 doc/user/permissions.md | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/doc/user/permissions.md b/doc/user/permissions.md
index 1498cb361c8..f1b75298180 100644
--- a/doc/user/permissions.md
+++ b/doc/user/permissions.md
@@ -63,7 +63,7 @@ The following table depicts the various user permission levels in a project.
 | Force push to protected branches [^2] |         |            |             |          |        |
 | Remove protected branches [^2]        |         |            |             |          |        |
 
-[^1]: If **Allow guest to access builds** is enabled in CI settings
+[^1]: If **Public pipelines** is enabled in **Project Settings > CI/CD Pipelines**
 [^2]: Not allowed for Guest, Reporter, Developer, Master, or Owner
 
 ## Group
-- 
cgit v1.2.1


From 683d8b7f007ad750d088b074e50339c66bf5dd82 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Sep 2016 18:55:49 +0800
Subject: Fix the ordering of transition callbacks:

Because pipeline status could be changed for the builds in the next
stages, if we process next stages first, the current build would be
out of synchronized, and would need a reload for that matter.

Alternatively, like what I did in this commit, we could process the
next stages later (by using `after_transition` rather than
`around_transition`), and complete what're doing for the current
build first. This way we don't have to reload because nothing is
out synchronized.

Note that since giving `false` in `after_transition` would halt the
callbacks chain, according to:

https://github.com/state-machines/state_machines-activemodel/blob/v0.4.0/lib/state_machines/integrations/active_model.rb#L426-L429

We'll need to make sure we're not returning false because we don't
intend to interrupt the chain.

This fixes #22010.

After this fix, both pipeline events and build events would only show
up once.
---
 app/models/commit_status.rb | 12 +++++-------
 1 file changed, 5 insertions(+), 7 deletions(-)

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 4a628924499..438f807b118 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -69,17 +69,15 @@ class CommitStatus < ActiveRecord::Base
       commit_status.update_attributes finished_at: Time.now
     end
 
-    # We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
-    around_transition any => [:success, :failed, :canceled] do |commit_status, block|
-      block.call
-
-      commit_status.pipeline.try(:process!)
-    end
-
     after_transition do |commit_status, transition|
       commit_status.pipeline.try(:build_updated) unless transition.loopback?
     end
 
+    after_transition any => [:success, :failed, :canceled] do |commit_status|
+      commit_status.pipeline.try(:process!)
+      true
+    end
+
     after_transition [:created, :pending, :running] => :success do |commit_status|
       MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status)
     end
-- 
cgit v1.2.1


From 47dccfb32935426a4f1cf386466961f61225208a Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Sep 2016 20:22:59 +0800
Subject: Fix test (credits to Kamil)

---
 app/models/concerns/has_status.rb | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index f7b8352405c..77fc66a7105 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -8,8 +8,9 @@ module HasStatus
 
   class_methods do
     def status_sql
-      scope = all.relevant
+      scope = all
       builds = scope.select('count(*)').to_sql
+      created = scope.created.select('count(*)').to_sql
       success = scope.success.select('count(*)').to_sql
       ignored = scope.ignored.select('count(*)').to_sql if scope.respond_to?(:ignored)
       ignored ||= '0'
@@ -22,9 +23,9 @@ module HasStatus
         WHEN (#{builds})=0 THEN NULL
         WHEN (#{builds})=(#{skipped}) THEN 'skipped'
         WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success'
-        WHEN (#{builds})=(#{pending})+(#{skipped}) THEN 'pending'
+        WHEN (#{builds})=(#{created})+(#{pending})+(#{skipped}) THEN 'pending'
         WHEN (#{builds})=(#{canceled})+(#{success})+(#{ignored})+(#{skipped}) THEN 'canceled'
-        WHEN (#{running})+(#{pending})>0 THEN 'running'
+        WHEN (#{running})+(#{pending})+(#{created})>0 THEN 'running'
         ELSE 'failed'
       END)"
 
-- 
cgit v1.2.1


From 575dc2b0d78b2680d6e5bb43f293c98d378b8733 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Sep 2016 20:37:44 +0800
Subject: reload instead, so that we don't have to change order

---
 app/models/commit_status.rb | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index 438f807b118..ad4e1c25231 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -69,15 +69,22 @@ class CommitStatus < ActiveRecord::Base
       commit_status.update_attributes finished_at: Time.now
     end
 
-    after_transition do |commit_status, transition|
-      commit_status.pipeline.try(:build_updated) unless transition.loopback?
-    end
+    # We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
+    around_transition any => [:success, :failed, :canceled] do |commit_status, block|
+      block.call
 
-    after_transition any => [:success, :failed, :canceled] do |commit_status|
       commit_status.pipeline.try(:process!)
       true
     end
 
+    after_transition do |commit_status, transition|
+      pipeline = commit_status.pipeline
+      if !transition.loopback? && pipeline
+        pipeline.reload
+        pipeline.build_updated
+      end
+    end
+
     after_transition [:created, :pending, :running] => :success do |commit_status|
       MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status)
     end
-- 
cgit v1.2.1


From 6baf9971db231a2c9923bd4d73e1f59fe255aab8 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Sep 2016 21:27:46 +0800
Subject: Revert "reload instead, so that we don't have to change order"

This reverts commit 575dc2b0d78b2680d6e5bb43f293c98d378b8733.
---
 app/models/commit_status.rb | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index ad4e1c25231..438f807b118 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -69,22 +69,15 @@ class CommitStatus < ActiveRecord::Base
       commit_status.update_attributes finished_at: Time.now
     end
 
-    # We use around_transition to process pipeline on next stages as soon as possible, before the `after_*` is executed
-    around_transition any => [:success, :failed, :canceled] do |commit_status, block|
-      block.call
+    after_transition do |commit_status, transition|
+      commit_status.pipeline.try(:build_updated) unless transition.loopback?
+    end
 
+    after_transition any => [:success, :failed, :canceled] do |commit_status|
       commit_status.pipeline.try(:process!)
       true
     end
 
-    after_transition do |commit_status, transition|
-      pipeline = commit_status.pipeline
-      if !transition.loopback? && pipeline
-        pipeline.reload
-        pipeline.build_updated
-      end
-    end
-
     after_transition [:created, :pending, :running] => :success do |commit_status|
       MergeRequests::MergeWhenBuildSucceedsService.new(commit_status.pipeline.project, nil).trigger(commit_status)
     end
-- 
cgit v1.2.1


From af7d383675269523ca4c6b7bddf8a34f6054f6af Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Mon, 12 Sep 2016 21:27:59 +0800
Subject: should show the status of the latest one

---
 app/models/commit.rb              | 2 +-
 spec/requests/api/commits_spec.rb | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/app/models/commit.rb b/app/models/commit.rb
index e64fd1e0c1b..0e2ab76d347 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -231,7 +231,7 @@ class Commit
 
   def status
     return @status if defined?(@status)
-    @status ||= pipelines.status
+    @status ||= pipelines.order(:id).last.try(:status)
   end
 
   def revert_branch_name
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index 5b3dc60aba2..e24e92e063c 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -110,7 +110,7 @@ describe API::API, api: true  do
         get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user)
 
         expect(response).to have_http_status(200)
-        expect(json_response['status']).to be_nil
+        expect(json_response['status']).to eq('created')
       end
     end
 
-- 
cgit v1.2.1


From ddcb8c880cbeb27a10e36fe1acb1e46963c3a377 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 13 Sep 2016 15:53:10 +0800
Subject: Fix styling

---
 spec/models/commit_status_spec.rb | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index fcfa3138ce5..6355f636bb9 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -40,7 +40,7 @@ describe CommitStatus, models: true do
       it { is_expected.to be_falsey }
     end
 
-    %w(running success failed).each do |status|
+    %w[running success failed].each do |status|
       context "if commit status is #{status}" do
         before { commit_status.status = status }
 
@@ -48,7 +48,7 @@ describe CommitStatus, models: true do
       end
     end
 
-    %w(pending canceled).each do |status|
+    %w[pending canceled].each do |status|
       context "if commit status is #{status}" do
         before { commit_status.status = status }
 
@@ -60,7 +60,7 @@ describe CommitStatus, models: true do
   describe '#active?' do
     subject { commit_status.active? }
 
-    %w(pending running).each do |state|
+    %w[pending running].each do |state|
       context "if commit_status.status is #{state}" do
         before { commit_status.status = state }
 
@@ -68,7 +68,7 @@ describe CommitStatus, models: true do
       end
     end
 
-    %w(success failed canceled).each do |state|
+    %w[success failed canceled].each do |state|
       context "if commit_status.status is #{state}" do
         before { commit_status.status = state }
 
@@ -80,7 +80,7 @@ describe CommitStatus, models: true do
   describe '#complete?' do
     subject { commit_status.complete? }
 
-    %w(success failed canceled).each do |state|
+    %w[success failed canceled].each do |state|
       context "if commit_status.status is #{state}" do
         before { commit_status.status = state }
 
@@ -88,7 +88,7 @@ describe CommitStatus, models: true do
       end
     end
 
-    %w(pending running).each do |state|
+    %w[pending running].each do |state|
       context "if commit_status.status is #{state}" do
         before { commit_status.status = state }
 
@@ -187,7 +187,7 @@ describe CommitStatus, models: true do
       subject { CommitStatus.where(pipeline: pipeline).stages }
 
       it 'returns ordered list of stages' do
-        is_expected.to eq(%w(build test deploy))
+        is_expected.to eq(%w[build test deploy])
       end
     end
 
-- 
cgit v1.2.1


From 18d7ae43099040d21dddbb114761c34c833ec766 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 13 Sep 2016 16:14:20 +0800
Subject: Add a test for #22010

The observed faulty state transition is probably hard to test,
because we need to hook into internal states to observe them.
Namely this:

    07:30:16 | Build#ruby-2.2 enqueue: created -> pending
    07:30:16 | Pipeline#32    enqueue: created -> pending
    07:30:16 | Build#ruby-2.3 enqueue: created -> pending
    07:30:16 | Build#ruby-2.2     run: pending -> running
    07:30:16 | Pipeline#32        run: pending -> running
    07:30:29 | Build#ruby-2.2    drop: running -> failed
    07:30:29 | Pipeline#32        run: running -> running
    07:30:29 | Build#ruby-2.3     run: pending -> running
    07:30:30 | Pipeline#32        run: running -> running
    07:30:57 | Build#gem:build   skip: created -> skipped
    07:30:57 | Pipeline#32       drop: running -> failed
    07:30:57 | Build#gem:release skip: created -> skipped
    07:30:57 | Pipeline#32       drop:  failed -> failed
    07:30:57 | Build#ruby-2.3    drop: running -> failed
    07:30:57 | Pipeline#32       drop: running -> failed
                                       ^^^ Should be failed -> failed

However, the consequence of this, executing hooks twice would be
easy enough to observe. So we could at least test against this.
Keep in mind that if we ever changed how we execute the hooks
this won't be testing against faulty state transition.
---
 spec/models/ci/pipeline_spec.rb | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index fbf945c757c..5d9063c0bc5 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -373,8 +373,8 @@ describe Ci::Pipeline, models: true do
   end
 
   describe '#execute_hooks' do
-    let!(:build_a) { create_build('a') }
-    let!(:build_b) { create_build('b') }
+    let!(:build_a) { create_build('a', 0) }
+    let!(:build_b) { create_build('b', 1) }
 
     let!(:hook) do
       create(:project_hook, project: project, pipeline_events: enabled)
@@ -427,6 +427,16 @@ describe Ci::Pipeline, models: true do
           end
         end
 
+        context 'when stage one failed' do
+          before do
+            build_a.drop
+          end
+
+          it 'receive a failed event once' do
+            expect(WebMock).to have_requested_pipeline_hook('failed').once
+          end
+        end
+
         def have_requested_pipeline_hook(status)
           have_requested(:post, hook.url).with do |req|
             json_body = JSON.parse(req.body)
@@ -450,8 +460,12 @@ describe Ci::Pipeline, models: true do
       end
     end
 
-    def create_build(name)
-      create(:ci_build, :created, pipeline: pipeline, name: name)
+    def create_build(name, stage_idx)
+      create(:ci_build,
+             :created,
+             pipeline: pipeline,
+             name: name,
+             stage_idx: stage_idx)
     end
   end
 end
-- 
cgit v1.2.1


From 05faeec708824bf97c3114b23235859663631b27 Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Tue, 13 Sep 2016 16:22:02 +0800
Subject: Fix English

---
 spec/models/ci/pipeline_spec.rb | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/spec/models/ci/pipeline_spec.rb b/spec/models/ci/pipeline_spec.rb
index 5d9063c0bc5..f1857f846dc 100644
--- a/spec/models/ci/pipeline_spec.rb
+++ b/spec/models/ci/pipeline_spec.rb
@@ -398,7 +398,7 @@ describe Ci::Pipeline, models: true do
             build_b.enqueue
           end
 
-          it 'receive a pending event once' do
+          it 'receives a pending event once' do
             expect(WebMock).to have_requested_pipeline_hook('pending').once
           end
         end
@@ -411,7 +411,7 @@ describe Ci::Pipeline, models: true do
             build_b.run
           end
 
-          it 'receive a running event once' do
+          it 'receives a running event once' do
             expect(WebMock).to have_requested_pipeline_hook('running').once
           end
         end
@@ -422,7 +422,7 @@ describe Ci::Pipeline, models: true do
             build_b.success
           end
 
-          it 'receive a success event once' do
+          it 'receives a success event once' do
             expect(WebMock).to have_requested_pipeline_hook('success').once
           end
         end
@@ -432,7 +432,7 @@ describe Ci::Pipeline, models: true do
             build_a.drop
           end
 
-          it 'receive a failed event once' do
+          it 'receives a failed event once' do
             expect(WebMock).to have_requested_pipeline_hook('failed').once
           end
         end
-- 
cgit v1.2.1


From 50e62b3eb8ab030e123e990d1335f68478cc4a4b Mon Sep 17 00:00:00 2001
From: Lin Jen-Shin <godfat@godfat.org>
Date: Wed, 14 Sep 2016 01:25:26 +0800
Subject: Fix Commit#status, feedback:

https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6305#note_15230024
---
 app/models/commit.rb              | 2 +-
 app/models/concerns/has_status.rb | 2 +-
 spec/requests/api/commits_spec.rb | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/app/models/commit.rb b/app/models/commit.rb
index 0e2ab76d347..e64fd1e0c1b 100644
--- a/app/models/commit.rb
+++ b/app/models/commit.rb
@@ -231,7 +231,7 @@ class Commit
 
   def status
     return @status if defined?(@status)
-    @status ||= pipelines.order(:id).last.try(:status)
+    @status ||= pipelines.status
   end
 
   def revert_branch_name
diff --git a/app/models/concerns/has_status.rb b/app/models/concerns/has_status.rb
index 77fc66a7105..d658552f695 100644
--- a/app/models/concerns/has_status.rb
+++ b/app/models/concerns/has_status.rb
@@ -20,7 +20,7 @@ module HasStatus
       skipped = scope.skipped.select('count(*)').to_sql
 
       deduce_status = "(CASE
-        WHEN (#{builds})=0 THEN NULL
+        WHEN (#{builds})=(#{created}) THEN NULL
         WHEN (#{builds})=(#{skipped}) THEN 'skipped'
         WHEN (#{builds})=(#{success})+(#{ignored})+(#{skipped}) THEN 'success'
         WHEN (#{builds})=(#{created})+(#{pending})+(#{skipped}) THEN 'pending'
diff --git a/spec/requests/api/commits_spec.rb b/spec/requests/api/commits_spec.rb
index e24e92e063c..5b3dc60aba2 100644
--- a/spec/requests/api/commits_spec.rb
+++ b/spec/requests/api/commits_spec.rb
@@ -110,7 +110,7 @@ describe API::API, api: true  do
         get api("/projects/#{project.id}/repository/commits/#{project.repository.commit.id}", user)
 
         expect(response).to have_http_status(200)
-        expect(json_response['status']).to eq('created')
+        expect(json_response['status']).to be_nil
       end
     end
 
-- 
cgit v1.2.1


From 3411212e414208dfdc7abd03c3fee4c4ec214476 Mon Sep 17 00:00:00 2001
From: Phil Hughes <me@iamphill.com>
Date: Wed, 14 Sep 2016 10:51:42 +0100
Subject: Fixed add to tree button on mobile

Closes #22128
---
 app/assets/stylesheets/pages/projects.scss | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/app/assets/stylesheets/pages/projects.scss b/app/assets/stylesheets/pages/projects.scss
index 3e6e50375f6..db46d8072ce 100644
--- a/app/assets/stylesheets/pages/projects.scss
+++ b/app/assets/stylesheets/pages/projects.scss
@@ -334,6 +334,10 @@ a.deploy-project-label {
   a {
     color: $gl-dark-link-color;
   }
+
+  .dropdown-menu {
+    width: 240px;
+  }
 }
 
 .last-push-widget {
-- 
cgit v1.2.1


From 8759770c962f7b6fe509a2d650dd420f75864de6 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 14 Sep 2016 15:03:45 +0200
Subject: Re-use as much of views as possible

---
 .../projects/ci/builds/_build_pipeline.html.haml   | 27 +++++++++++-----------
 app/views/projects/commit/_pipeline.html.haml      |  8 +------
 .../commit/_pipeline_grouped_status.html.haml      | 27 ----------------------
 .../projects/commit/_pipeline_stage.html.haml      | 14 +++++++++++
 .../commit/_pipeline_status_group.html.haml        | 11 +++++++++
 .../_generic_commit_status_pipeline.html.haml      | 17 ++++++--------
 6 files changed, 46 insertions(+), 58 deletions(-)
 delete mode 100644 app/views/projects/commit/_pipeline_grouped_status.html.haml
 create mode 100644 app/views/projects/commit/_pipeline_stage.html.haml
 create mode 100644 app/views/projects/commit/_pipeline_status_group.html.haml

diff --git a/app/views/projects/ci/builds/_build_pipeline.html.haml b/app/views/projects/ci/builds/_build_pipeline.html.haml
index 5289cd672f5..d19e193c432 100644
--- a/app/views/projects/ci/builds/_build_pipeline.html.haml
+++ b/app/views/projects/ci/builds/_build_pipeline.html.haml
@@ -1,15 +1,14 @@
 - is_playable = subject.playable? && can?(current_user, :update_build, @project)
-%li.build{class: ("playable" if is_playable)}
-  .curve
-  .build-content
-    - if is_playable
-      = link_to play_namespace_project_build_path(subject.project.namespace, subject.project, subject, return_to: request.original_url), method: :post, title: 'Play' do
-        = render_status_with_link('build', 'play')
-        .ci-status-text= subject.name
-    - elsif can?(current_user, :read_build, @project)
-      = link_to namespace_project_build_path(subject.project.namespace, subject.project, subject) do
-        = render_status_with_link('build', subject.status)
-        .ci-status-text= subject.name
-    - else
-      = render_status_with_link('build', subject.status)
-      = ci_icon_for_status(subject.status)
+- if is_playable
+  = link_to play_namespace_project_build_path(subject.project.namespace, subject.project, subject, return_to: request.original_url), method: :post, title: 'Play' do
+    = render_status_with_link('build', 'play')
+    .ci-status-text= subject.name
+- elsif can?(current_user, :read_build, @project)
+  = link_to namespace_project_build_path(subject.project.namespace, subject.project, subject) do
+    = render_status_with_link('build', subject.status)
+    .ci-status-text= subject.name
+- else
+  = render_status_with_link('build', subject.status)
+  = ci_icon_for_status(subject.status)
+
+
diff --git a/app/views/projects/commit/_pipeline.html.haml b/app/views/projects/commit/_pipeline.html.haml
index a330c14061f..9258f4b3c25 100644
--- a/app/views/projects/commit/_pipeline.html.haml
+++ b/app/views/projects/commit/_pipeline.html.haml
@@ -39,13 +39,7 @@
               = stage.titleize
           .builds-container
             %ul
-              - status_groups = statuses.group_by(&:group_name)
-              - status_groups.each do |group_name, grouped_statuses|
-                - if grouped_statuses.one?
-                  - status = grouped_statuses.first
-                  = render "projects/#{status.to_partial_path}_pipeline", subject: status
-                - else
-                  = render "projects/commit/pipeline_grouped_status", name: group_name, subject: grouped_statuses
+              = render "projects/commit/pipeline_stage", statuses: statuses
 
 
 - if pipeline.yaml_errors.present?
diff --git a/app/views/projects/commit/_pipeline_grouped_status.html.haml b/app/views/projects/commit/_pipeline_grouped_status.html.haml
deleted file mode 100644
index dc8efc83d48..00000000000
--- a/app/views/projects/commit/_pipeline_grouped_status.html.haml
+++ /dev/null
@@ -1,27 +0,0 @@
-%li.build
-  .curve
-  .build-content
-    - group_status = CommitStatus.where(id: subject).status
-    = render_status_with_link('build', group_status)
-    .dropdown.inline
-      %button.dropdown-menu-toggle{type: 'button', data: {toggle: 'dropdown'}}
-        %span.ci-status-text
-          = name
-        %span.badge= subject.length
-      %ul.dropdown-menu.grouped-pipeline-dropdown
-        .arrow
-        - subject.each do |status|
-          -# = render "projects/#{status.to_partial_path}_pipeline", subject: status
-          - is_playable = status.playable? && can?(current_user, :update_build, @project)
-          %li
-            - if is_playable
-              = link_to play_namespace_project_build_path(status.project.namespace, status.project, status, return_to: request.original_url), method: :post, title: 'Play' do
-                = render_status_with_link('build', 'play')
-                .ci-status-text= status.name
-            - elsif can?(current_user, :read_build, @project)
-              = link_to namespace_project_build_path(status.project.namespace, status.project, status) do
-                = render_status_with_link('build', status.status)
-                .ci-status-text= status.name
-            - else
-              = render_status_with_link('build', status.status)
-              = ci_icon_for_status(status.status)
diff --git a/app/views/projects/commit/_pipeline_stage.html.haml b/app/views/projects/commit/_pipeline_stage.html.haml
new file mode 100644
index 00000000000..368564481c4
--- /dev/null
+++ b/app/views/projects/commit/_pipeline_stage.html.haml
@@ -0,0 +1,14 @@
+- status_groups = statuses.group_by(&:group_name)
+- status_groups.each do |group_name, grouped_statuses|
+  - if grouped_statuses.one?
+    - status = grouped_statuses.first
+    - is_playable = status.playable? && can?(current_user, :update_build, @project)
+    %li.build{class: ("playable" if is_playable)}
+      .curve
+      .build-content
+        = render "projects/#{status.to_partial_path}_pipeline", subject: status
+  - else
+    %li.build
+      .curve
+      .build-content
+        = render "projects/commit/pipeline_status_group", name: group_name, subject: grouped_statuses
diff --git a/app/views/projects/commit/_pipeline_status_group.html.haml b/app/views/projects/commit/_pipeline_status_group.html.haml
new file mode 100644
index 00000000000..03b6249963b
--- /dev/null
+++ b/app/views/projects/commit/_pipeline_status_group.html.haml
@@ -0,0 +1,11 @@
+- group_status = CommitStatus.where(id: subject).status
+= render_status_with_link('build', group_status)
+.dropdown.inline
+  %button.dropdown-menu-toggle{type: 'button', data: {toggle: 'dropdown'}}
+    %span.ci-status-text
+      = name
+    %span.badge= subject.length
+  %ul.dropdown-menu.grouped-pipeline-dropdown
+    .arrow
+    - subject.each do |status|
+      = render "projects/#{status.to_partial_path}_pipeline", subject: status
diff --git a/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml b/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml
index 576d0bec51b..9b54c2bc3af 100644
--- a/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml
+++ b/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml
@@ -1,10 +1,7 @@
-%li.build
-  .curve
-  .build-content
-    - if subject.target_url
-      - link_to subject.target_url do
-        = render_status_with_link('commit status', subject.status)
-        %span.ci-status-text= subject.name
-    - else
-      = render_status_with_link('commit status', subject.status)
-      %span.ci-status-text= subject.name
+- if subject.target_url
+  - link_to subject.target_url do
+    = render_status_with_link('commit status', subject.status)
+    %span.ci-status-text= subject.name
+- else
+  = render_status_with_link('commit status', subject.status)
+  %span.ci-status-text= subject.name
-- 
cgit v1.2.1


From b964c6c579f57f1ff83eb27caf8ff7f7be6d7671 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 14 Sep 2016 15:04:12 +0200
Subject: Add grouping tests

---
 app/models/commit_status.rb       |  6 +++++-
 spec/models/commit_status_spec.rb | 27 +++++++++++++++++++++++++++
 2 files changed, 32 insertions(+), 1 deletion(-)

diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb
index af739342256..1ae1a24c168 100644
--- a/app/models/commit_status.rb
+++ b/app/models/commit_status.rb
@@ -96,7 +96,7 @@ class CommitStatus < ActiveRecord::Base
   end
 
   def group_name
-    name.gsub(/\d+[\s:]+\d+\s*/, '')
+    name.gsub(/\d+[\s:\/\\]+\d+\s*/, '').strip
   end
 
   def self.stages
@@ -117,6 +117,10 @@ class CommitStatus < ActiveRecord::Base
     allow_failure? && (failed? || canceled?)
   end
 
+  def playable?
+    false
+  end
+
   def duration
     calculate_duration
   end
diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index fcfa3138ce5..ea3b8295364 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -223,4 +223,31 @@ describe CommitStatus, models: true do
       expect(commit_status.commit).to eq project.commit
     end
   end
+
+  describe '#group_name' do
+    subject { commit_status.group_name }
+
+    tests = {
+      'rspec:windows' => 'rspec:windows',
+      'rspec:windows 0' => 'rspec:windows 0',
+      'rspec:windows 0 test' => 'rspec:windows 0 test',
+      'rspec:windows 0 1' => 'rspec:windows',
+      'rspec:windows 0 1 name' => 'rspec:windows name',
+      'rspec:windows 0/1' => 'rspec:windows',
+      'rspec:windows 0/1 name' => 'rspec:windows name',
+      'rspec:windows 0:1' => 'rspec:windows',
+      'rspec:windows 0:1 name' => 'rspec:windows name',
+      'rspec:windows 10000 20000' => 'rspec:windows',
+      'rspec:windows 0 : / 1' => 'rspec:windows',
+      'rspec:windows 0 : / 1 name' => 'rspec:windows name',
+    }
+
+    tests.each do |name, group_name|
+      it "'#{name}' puts in '#{group_name}'" do
+        commit_status.name = name
+
+        is_expected.to eq(group_name)
+      end
+    end
+  end
 end
-- 
cgit v1.2.1


From d8ba09fca9ac9e0ad55c30686decb1bee7584468 Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 14 Sep 2016 15:23:07 +0200
Subject: Add view specs for pipelines graph

---
 .../projects/pipelines/show.html.haml_spec.rb      | 51 ++++++++++++++++++++++
 1 file changed, 51 insertions(+)
 create mode 100644 spec/views/projects/pipelines/show.html.haml_spec.rb

diff --git a/spec/views/projects/pipelines/show.html.haml_spec.rb b/spec/views/projects/pipelines/show.html.haml_spec.rb
new file mode 100644
index 00000000000..920d7528892
--- /dev/null
+++ b/spec/views/projects/pipelines/show.html.haml_spec.rb
@@ -0,0 +1,51 @@
+require 'spec_helper'
+
+describe 'projects/pipelines/show' do
+  include Devise::TestHelpers
+
+  let(:project) { create(:project) }
+  let(:pipeline) do
+    create(:ci_empty_pipeline, project: project,
+           sha: project.commit.id)
+  end
+
+  before do
+    create_build('build', 0, 'build')
+    create_build('test', 1, 'rspec 0 2')
+    create_build('test', 1, 'rspec 1 2')
+    create_build('test', 1, 'audit')
+    create_build('deploy', 2, 'production')
+
+    create(:generic_commit_status, pipeline: pipeline, stage: 'external', name: 'jenkins', stage_idx: 3)
+
+    assign(:project, project)
+    assign(:pipeline, pipeline)
+
+    allow(view).to receive(:can?).and_return(true)
+  end
+
+  it 'shows a graph with grouped stages' do
+    render
+
+    expect(rendered).to have_css('.pipeline-graph')
+    expect(rendered).to have_css('.grouped-pipeline-dropdown')
+
+    # stages
+    expect(rendered).to have_text('Build')
+    expect(rendered).to have_text('Test')
+    expect(rendered).to have_text('Deploy')
+    expect(rendered).to have_text('External')
+
+    # builds
+    expect(rendered).to have_text('rspec')
+    expect(rendered).to have_text('rspec 0:1')
+    expect(rendered).to have_text('production')
+    expect(rendered).to have_text('jenkins')
+  end
+
+  private
+
+  def create_build(stage, stage_idx, name)
+    create(:ci_build, pipeline: pipeline, stage: stage, stage_idx: stage_idx, name: name)
+  end
+end
-- 
cgit v1.2.1


From 4e60f79e4027961ed3ec33fc16e2260c660d545a Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 14 Sep 2016 15:25:04 +0200
Subject: Add more regexp tests

---
 spec/models/commit_status_spec.rb | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/spec/models/commit_status_spec.rb b/spec/models/commit_status_spec.rb
index ea3b8295364..49984fbc5aa 100644
--- a/spec/models/commit_status_spec.rb
+++ b/spec/models/commit_status_spec.rb
@@ -240,6 +240,8 @@ describe CommitStatus, models: true do
       'rspec:windows 10000 20000' => 'rspec:windows',
       'rspec:windows 0 : / 1' => 'rspec:windows',
       'rspec:windows 0 : / 1 name' => 'rspec:windows name',
+      '0 1 name ruby' => 'name ruby',
+      '0 :/ 1 name ruby' => 'name ruby'
     }
 
     tests.each do |name, group_name|
-- 
cgit v1.2.1


From 2f3dc314f42dbd79813e6251792853bc231e69dd Mon Sep 17 00:00:00 2001
From: Kamil Trzcinski <ayufan@ayufan.eu>
Date: Wed, 14 Sep 2016 16:57:36 +0200
Subject: Fix spec failures

---
 app/views/projects/ci/builds/_build_pipeline.html.haml      |  2 --
 app/views/projects/commit/_pipeline_stage.html.haml         |  2 +-
 app/views/projects/commit/_pipeline_status_group.html.haml  |  4 ++--
 .../_generic_commit_status_pipeline.html.haml               |  2 +-
 spec/views/projects/pipelines/show.html.haml_spec.rb        | 13 ++++++-------
 5 files changed, 10 insertions(+), 13 deletions(-)

diff --git a/app/views/projects/ci/builds/_build_pipeline.html.haml b/app/views/projects/ci/builds/_build_pipeline.html.haml
index d19e193c432..547bc0c9c19 100644
--- a/app/views/projects/ci/builds/_build_pipeline.html.haml
+++ b/app/views/projects/ci/builds/_build_pipeline.html.haml
@@ -10,5 +10,3 @@
 - else
   = render_status_with_link('build', subject.status)
   = ci_icon_for_status(subject.status)
-
-
diff --git a/app/views/projects/commit/_pipeline_stage.html.haml b/app/views/projects/commit/_pipeline_stage.html.haml
index 368564481c4..23c5c51fbc2 100644
--- a/app/views/projects/commit/_pipeline_stage.html.haml
+++ b/app/views/projects/commit/_pipeline_stage.html.haml
@@ -3,7 +3,7 @@
   - if grouped_statuses.one?
     - status = grouped_statuses.first
     - is_playable = status.playable? && can?(current_user, :update_build, @project)
-    %li.build{class: ("playable" if is_playable)}
+    %li.build{ class: ("playable" if is_playable) }
       .curve
       .build-content
         = render "projects/#{status.to_partial_path}_pipeline", subject: status
diff --git a/app/views/projects/commit/_pipeline_status_group.html.haml b/app/views/projects/commit/_pipeline_status_group.html.haml
index 03b6249963b..4e7a6f1af08 100644
--- a/app/views/projects/commit/_pipeline_status_group.html.haml
+++ b/app/views/projects/commit/_pipeline_status_group.html.haml
@@ -1,10 +1,10 @@
 - group_status = CommitStatus.where(id: subject).status
 = render_status_with_link('build', group_status)
 .dropdown.inline
-  %button.dropdown-menu-toggle{type: 'button', data: {toggle: 'dropdown'}}
+  %button.dropdown-menu-toggle{ type: 'button', data: { toggle: 'dropdown' } }
     %span.ci-status-text
       = name
-    %span.badge= subject.length
+    %span.badge= subject.size
   %ul.dropdown-menu.grouped-pipeline-dropdown
     .arrow
     - subject.each do |status|
diff --git a/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml b/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml
index 9b54c2bc3af..409f4701e4b 100644
--- a/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml
+++ b/app/views/projects/generic_commit_statuses/_generic_commit_status_pipeline.html.haml
@@ -1,5 +1,5 @@
 - if subject.target_url
-  - link_to subject.target_url do
+  = link_to subject.target_url do
     = render_status_with_link('commit status', subject.status)
     %span.ci-status-text= subject.name
 - else
diff --git a/spec/views/projects/pipelines/show.html.haml_spec.rb b/spec/views/projects/pipelines/show.html.haml_spec.rb
index 920d7528892..c5b16c1c304 100644
--- a/spec/views/projects/pipelines/show.html.haml_spec.rb
+++ b/spec/views/projects/pipelines/show.html.haml_spec.rb
@@ -4,15 +4,14 @@ describe 'projects/pipelines/show' do
   include Devise::TestHelpers
 
   let(:project) { create(:project) }
-  let(:pipeline) do
-    create(:ci_empty_pipeline, project: project,
-           sha: project.commit.id)
-  end
+  let(:pipeline) { create(:ci_empty_pipeline, project: project, sha: project.commit.id) }
 
   before do
+    controller.prepend_view_path('app/views/projects')
+
     create_build('build', 0, 'build')
-    create_build('test', 1, 'rspec 0 2')
-    create_build('test', 1, 'rspec 1 2')
+    create_build('test', 1, 'rspec 0:2')
+    create_build('test', 1, 'rspec 1:2')
     create_build('test', 1, 'audit')
     create_build('deploy', 2, 'production')
 
@@ -38,7 +37,7 @@ describe 'projects/pipelines/show' do
 
     # builds
     expect(rendered).to have_text('rspec')
-    expect(rendered).to have_text('rspec 0:1')
+    expect(rendered).to have_text('rspec 0:2')
     expect(rendered).to have_text('production')
     expect(rendered).to have_text('jenkins')
   end
-- 
cgit v1.2.1


From e7e628bf959b77d5a58471d224ebd18c4e97032d Mon Sep 17 00:00:00 2001
From: Robert Speicher <rspeicher@gmail.com>
Date: Wed, 14 Sep 2016 16:50:15 -0400
Subject: Improve validity of spec/features/todos/todos_filtering_spec.rb

Previously, we were checking that a CSS selector string didn't have some
page-related content, which, of course it didn't.

Now we check against the actual content of the selector, we use a more
semantic selector, and we add an additional expectation for the text
that _should_ be there, as an additional sanity check.
---
 spec/features/todos/todos_filtering_spec.rb | 20 ++++++++++++++++----
 1 file changed, 16 insertions(+), 4 deletions(-)

diff --git a/spec/features/todos/todos_filtering_spec.rb b/spec/features/todos/todos_filtering_spec.rb
index 83cf306437d..b9e66243d84 100644
--- a/spec/features/todos/todos_filtering_spec.rb
+++ b/spec/features/todos/todos_filtering_spec.rb
@@ -29,8 +29,11 @@ describe 'Dashboard > User filters todos', feature: true, js: true do
       fill_in 'Search projects', with: project_1.name_with_namespace
       click_link project_1.name_with_namespace
     end
+
     wait_for_ajax
-    expect('.prepend-top-default').not_to have_content project_2.name_with_namespace
+
+    expect(page).to     have_content project_1.name_with_namespace
+    expect(page).not_to have_content project_2.name_with_namespace
   end
 
   it 'filters by author' do
@@ -39,8 +42,11 @@ describe 'Dashboard > User filters todos', feature: true, js: true do
       fill_in 'Search authors', with: user_1.name
       click_link user_1.name
     end
+
     wait_for_ajax
-    expect('.prepend-top-default').not_to have_content user_2.name
+
+    expect(find('.todos-list')).to     have_content user_1.name
+    expect(find('.todos-list')).not_to have_content user_2.name
   end
 
   it 'filters by type' do
@@ -48,8 +54,11 @@ describe 'Dashboard > User filters todos', feature: true, js: true do
     within '.dropdown-menu-type' do
       click_link 'Issue'
     end
+
     wait_for_ajax
-    expect('.prepend-top-default').not_to have_content ' merge request !'
+
+    expect(find('.todos-list')).to     have_content issue.to_reference
+    expect(find('.todos-list')).not_to have_content merge_request.to_reference
   end
 
   it 'filters by action' do
@@ -57,7 +66,10 @@ describe 'Dashboard > User filters todos', feature: true, js: true do
     within '.dropdown-menu-action' do
       click_link 'Assigned'
     end
+
     wait_for_ajax
-    expect('.prepend-top-default').not_to have_content ' mentioned '
+
+    expect(find('.todos-list')).to     have_content ' assigned you '
+    expect(find('.todos-list')).not_to have_content ' mentioned '
   end
 end
-- 
cgit v1.2.1


From 02bc717f3423b2eb84328f4d908862db10ec76ce Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Alejandro=20Rodr=C3=ADguez?= <alejorro70@gmail.com>
Date: Wed, 14 Sep 2016 15:15:28 -0300
Subject: Update references to deprecated `repos_path` configuration key to
 avoid errors on updates from older versions

---
 db/migrate/20140502125220_migrate_repo_size.rb | 5 ++++-
 lib/gitlab/import_export/repo_restorer.rb      | 4 ----
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/db/migrate/20140502125220_migrate_repo_size.rb b/db/migrate/20140502125220_migrate_repo_size.rb
index 84463727b3b..e8de7ccf3db 100644
--- a/db/migrate/20140502125220_migrate_repo_size.rb
+++ b/db/migrate/20140502125220_migrate_repo_size.rb
@@ -1,12 +1,15 @@
 # rubocop:disable all
 class MigrateRepoSize < ActiveRecord::Migration
+  DOWNTIME = false
+
   def up
     project_data = execute('SELECT projects.id, namespaces.path AS namespace_path, projects.path AS project_path FROM projects LEFT JOIN namespaces ON projects.namespace_id = namespaces.id')
 
     project_data.each do |project|
       id = project['id']
       namespace_path = project['namespace_path'] || ''
-      path = File.join(Gitlab.config.gitlab_shell.repos_path, namespace_path, project['project_path'] + '.git')
+      repos_path = Gitlab.config.gitlab_shell['repos_path'] || Gitlab.config.repositories.storages.default
+      path = File.join(repos_path, namespace_path, project['project_path'] + '.git')
 
       begin
         repo = Gitlab::Git::Repository.new(path)
diff --git a/lib/gitlab/import_export/repo_restorer.rb b/lib/gitlab/import_export/repo_restorer.rb
index 6d9379acf25..d1e33ea8678 100644
--- a/lib/gitlab/import_export/repo_restorer.rb
+++ b/lib/gitlab/import_export/repo_restorer.rb
@@ -22,10 +22,6 @@ module Gitlab
 
       private
 
-      def repos_path
-        Gitlab.config.gitlab_shell.repos_path
-      end
-
       def path_to_repo
         @project.repository.path_to_repo
       end
-- 
cgit v1.2.1


From e40e3fdc8271d1becf7952c7e30546c5abecb79b Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Thu, 25 Aug 2016 17:26:20 -0500
Subject: Added LFS support to SSH - Required on the GitLab Rails side is
 mostly authentication and API related.

---
 .../projects/git_http_client_controller.rb         | 42 +++++++++++++++-------
 app/helpers/lfs_helper.rb                          |  2 +-
 app/models/deploy_key.rb                           |  5 +++
 app/models/user.rb                                 |  3 +-
 .../20160825173042_add_lfs_token_to_users.rb       | 16 +++++++++
 db/migrate/20160825182924_add_lfs_token_to_keys.rb | 16 +++++++++
 lib/api/entities.rb                                |  2 +-
 lib/api/internal.rb                                | 13 ++++++-
 lib/gitlab/auth.rb                                 | 13 ++++++-
 spec/lib/gitlab/auth_spec.rb                       | 16 +++++++++
 spec/models/concerns/token_authenticatable_spec.rb | 20 +++++++++++
 spec/requests/api/internal_spec.rb                 | 26 +++++++++++---
 spec/requests/lfs_http_spec.rb                     | 16 +++++++++
 13 files changed, 169 insertions(+), 21 deletions(-)
 create mode 100644 db/migrate/20160825173042_add_lfs_token_to_users.rb
 create mode 100644 db/migrate/20160825182924_add_lfs_token_to_keys.rb

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index f5ce63fdfed..1e6709dc8eb 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,6 +4,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
+  class MissingPersonalTokenError < StandardError; end
+
   attr_reader :user
 
   # Git clients will not know what authenticity token to send along
@@ -21,18 +23,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
     if allow_basic_auth? && basic_auth_provided?
       login, password = user_name_and_password(request)
-      auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
-
-      if auth_result.type == :ci && download_request?
-        @ci = true
-      elsif auth_result.type == :oauth && !download_request?
-        # Not allowed
-      elsif auth_result.type == :missing_personal_token
-        render_missing_personal_token
-        return # Render above denied access, nothing left to do
-      else
-        @user = auth_result.user
-      end
+
+      handle_authentication(login, password)
 
       if ci? || user
         return # Allow access
@@ -48,6 +40,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
     send_challenges
     render plain: "HTTP Basic: Access denied\n", status: 401
+
+  rescue MissingPersonalTokenError
+    render_missing_personal_token
+    return
   end
 
   def basic_auth_provided?
@@ -118,6 +114,28 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @ci.present?
   end
 
+  def handle_authentication(login, password)
+    auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
+
+    if auth_result.type == :ci && download_request?
+      @ci = true
+    elsif auth_result.type == :oauth && !download_request?
+      # Not allowed
+    elsif auth_result.type == :missing_personal_token
+      raise MissingPersonalTokenError
+    elsif auth_result.type == :lfs_deploy_token && download_request?
+      @lfs_deploy_key = true
+      @user = auth_result.user
+    else
+      @user = auth_result.user
+    end
+  end
+
+  def lfs_deploy_key?
+    key = user
+    @lfs_deploy_key.present? && (key && key.projects.include?(project))
+  end
+
   def verify_workhorse_api!
     Gitlab::Workhorse.verify_api_request!(request.headers)
   end
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 5d82abfca79..0c867fc8741 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -25,7 +25,7 @@ module LfsHelper
   def lfs_download_access?
     return false unless project.lfs_enabled?
 
-    project.public? || ci? || (user && user.can?(:download_code, project))
+    project.public? || ci? || lfs_deploy_key? || (user && user.can?(:download_code, project))
   end
 
   def lfs_upload_access?
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index 2c525d4cd7a..de51b63c120 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -1,7 +1,12 @@
 class DeployKey < Key
+  include TokenAuthenticatable
+  add_authentication_token_field :lfs_token
+
   has_many :deploy_keys_projects, dependent: :destroy
   has_many :projects, through: :deploy_keys_projects
 
+  before_save :ensure_lfs_token
+
   scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
   scope :are_public,  -> { where(public: true) }
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 6996740eebd..94ae3911859 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -13,6 +13,7 @@ class User < ActiveRecord::Base
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
   add_authentication_token_field :authentication_token
+  add_authentication_token_field :lfs_token
 
   default_value_for :admin, false
   default_value_for(:external) { current_application_settings.user_default_external }
@@ -117,7 +118,7 @@ class User < ActiveRecord::Base
   before_validation :set_public_email, if: ->(user) { user.public_email_changed? }
 
   after_update :update_emails_with_primary_email, if: ->(user) { user.email_changed? }
-  before_save :ensure_authentication_token
+  before_save :ensure_authentication_token, :ensure_lfs_token
   before_save :ensure_external_user_rights
   after_save :ensure_namespace_correct
   after_initialize :set_projects_limit
diff --git a/db/migrate/20160825173042_add_lfs_token_to_users.rb b/db/migrate/20160825173042_add_lfs_token_to_users.rb
new file mode 100644
index 00000000000..f7f210bcd67
--- /dev/null
+++ b/db/migrate/20160825173042_add_lfs_token_to_users.rb
@@ -0,0 +1,16 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLfsTokenToUsers < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_column :users, :lfs_token, :string
+    add_concurrent_index :users, :lfs_token
+  end
+end
diff --git a/db/migrate/20160825182924_add_lfs_token_to_keys.rb b/db/migrate/20160825182924_add_lfs_token_to_keys.rb
new file mode 100644
index 00000000000..3ff010ef328
--- /dev/null
+++ b/db/migrate/20160825182924_add_lfs_token_to_keys.rb
@@ -0,0 +1,16 @@
+# See http://doc.gitlab.com/ce/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddLfsTokenToKeys < ActiveRecord::Migration
+  include Gitlab::Database::MigrationHelpers
+
+  # Set this constant to true if this migration requires downtime.
+  DOWNTIME = false
+
+  disable_ddl_transaction!
+
+  def change
+    add_column :keys, :lfs_token, :string
+    add_concurrent_index :keys, :lfs_token
+  end
+end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index 4f736e4ec2b..b4fcacca896 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1,7 +1,7 @@
 module API
   module Entities
     class UserSafe < Grape::Entity
-      expose :name, :username
+      expose :name, :username, :lfs_token
     end
 
     class UserBasic < UserSafe
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 6e6efece7c4..7c0a6eaa652 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -69,6 +69,10 @@ module API
             else
               project.repository.path_to_repo
             end
+
+          # Return HTTP full path, so that gitlab-shell has this information
+          # ready for git-lfs-authenticate
+          response[:repository_http_path] = project.http_url_to_repo
         end
 
         response
@@ -83,7 +87,14 @@ module API
       #
       get "/discover" do
         key = Key.find(params[:key_id])
-        present key.user, with: Entities::UserSafe
+        user = key.user
+        if user
+          user.ensure_lfs_token!
+          present user, with: Entities::UserSafe
+        else
+          key.ensure_lfs_token!
+          { username: 'lfs-deploy-key', lfs_token: key.lfs_token }
+        end
       end
 
       get "/check" do
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 91f0270818a..5446093de4d 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -79,12 +79,13 @@ module Gitlab
         result =
           user_with_password_for_git(login, password) ||
           oauth_access_token_check(login, password) ||
+          lfs_token_check(login, password) ||
           personal_access_token_check(login, password)
 
         if result
           result.type = nil unless result.user
 
-          if result.user && result.user.two_factor_enabled? && result.type == :gitlab_or_ldap
+          if result.user && result.type == :gitlab_or_ldap && result.user.two_factor_enabled?
             result.type = :missing_personal_token
           end
         end
@@ -114,6 +115,16 @@ module Gitlab
           Result.new(user, :personal_token) if user == validation
         end
       end
+
+      def lfs_token_check(login, password)
+        if login == 'lfs-deploy-key'
+          key = DeployKey.find_by_lfs_token(password)
+          Result.new(key, :lfs_deploy_token) if key
+        else
+          user = User.find_by_lfs_token(password)
+          Result.new(user, :lfs_token) if user && user.username == login
+        end
+      end
     end
   end
 end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 7c23e02d05a..cd00a15be3b 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -23,6 +23,22 @@ describe Gitlab::Auth, lib: true do
       expect(gl_auth.find_for_git_client(user.username, 'password', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :gitlab_or_ldap))
     end
 
+    it 'recognizes user lfs tokens' do
+      user = create(:user)
+      ip = 'ip'
+
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
+      expect(gl_auth.find_for_git_client(user.username, user.lfs_token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
+    end
+
+    it 'recognizes deploy key lfs tokens' do
+      key = create(:deploy_key)
+      ip = 'ip'
+
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'lfs-deploy-key')
+      expect(gl_auth.find_for_git_client('lfs-deploy-key', key.lfs_token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
+    end
+
     it 'recognizes OAuth tokens' do
       user = create(:user)
       application = Doorkeeper::Application.create!(name: "MyApp", redirect_uri: "https://app.com", owner: user)
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index eb64f3d0c83..82076600f3b 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -18,6 +18,26 @@ describe User, 'TokenAuthenticatable' do
     subject { create(:user).send(token_field) }
     it { is_expected.to be_a String }
   end
+
+  describe 'lfs token' do
+    let(:token_field) { :lfs_token }
+    it_behaves_like 'TokenAuthenticatable'
+
+    describe 'ensure it' do
+      subject { create(:user).send(token_field) }
+      it { is_expected.to be_a String }
+    end
+  end
+end
+
+describe DeployKey, 'TokenAuthenticatable' do
+  let(:token_field) { :lfs_token }
+  it_behaves_like 'TokenAuthenticatable'
+
+  describe 'ensures authentication token' do
+    subject { create(:deploy_key).send(token_field) }
+    it { is_expected.to be_a String }
+  end
 end
 
 describe ApplicationSetting, 'TokenAuthenticatable' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 46d1b868782..95fc5f790e8 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -101,12 +101,28 @@ describe API::API, api: true  do
   end
 
   describe "GET /internal/discover" do
-    it do
-      get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+    context 'user key' do
+      it 'returns the correct information about the key' do
+        get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
 
-      expect(response).to have_http_status(200)
+        expect(response).to have_http_status(200)
+
+        expect(json_response['name']).to eq(user.name)
+        expect(json_response['lfs_token']).to eq(user.lfs_token)
+      end
+    end
 
-      expect(json_response['name']).to eq(user.name)
+    context 'deploy key' do
+      let(:key) { create(:deploy_key) }
+
+      it 'returns the correct information about the key' do
+        get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+
+        expect(response).to have_http_status(200)
+
+        expect(json_response['username']).to eq('lfs-deploy-key')
+        expect(json_response['lfs_token']).to eq(key.lfs_token)
+      end
     end
   end
 
@@ -143,6 +159,7 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
+          expect(json_response["repository_http_path"]).to eq(project.http_url_to_repo)
         end
       end
 
@@ -153,6 +170,7 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
+          expect(json_response["repository_http_path"]).to eq(project.http_url_to_repo)
         end
       end
     end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 6e551bb65fa..58f8515c0e2 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -244,6 +244,18 @@ describe 'Git LFS API and storage' do
           end
         end
 
+        context 'when deploy key is authorized' do
+          let(:key) { create(:deploy_key) }
+          let(:authorization) { authorize_deploy_key }
+
+          let(:update_permissions) do
+            project.deploy_keys << key
+            project.lfs_objects << lfs_object
+          end
+
+          it_behaves_like 'responds with a file'
+        end
+
         context 'when CI is authorized' do
           let(:authorization) { authorize_ci_project }
 
@@ -904,6 +916,10 @@ describe 'Git LFS API and storage' do
     ActionController::HttpAuthentication::Basic.encode_credentials(user.username, user.password)
   end
 
+  def authorize_deploy_key
+    ActionController::HttpAuthentication::Basic.encode_credentials('lfs-deploy-key', key.lfs_token)
+  end
+
   def fork_project(project, user, object = nil)
     allow(RepositoryForkWorker).to receive(:perform_async).and_return(true)
     Projects::ForkService.new(project, user, {}).execute
-- 
cgit v1.2.1


From 372be2d2e8fe8d607011aa7e2b2fca99eeea007d Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Thu, 25 Aug 2016 18:43:14 -0500
Subject: Added CHANGELOG item and documentation.

---
 CHANGELOG                                              | 1 +
 doc/workflow/lfs/lfs_administration.md                 | 4 ++--
 doc/workflow/lfs/manage_large_binaries_with_git_lfs.md | 8 ++++++++
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/CHANGELOG b/CHANGELOG
index 0056c6cc649..9458413669d 100644
--- a/CHANGELOG
+++ b/CHANGELOG
@@ -93,6 +93,7 @@ v 8.12.0 (unreleased)
   - Remove green outline from `New branch unavailable` button on issue page !5858 (winniehell)
   - Fix repo title alignment (ClemMakesApps)
   - Change update interval of contacted_at
+  - Add LFS support to SSH !6043
   - Fix branch title trailing space on hover (ClemMakesApps)
   - Don't include 'Created By' tag line when importing from GitHub if there is a linked GitLab account (EspadaV8)
   - Award emoji tooltips containing more than 10 usernames are now truncated !4780 (jlogandavison)
diff --git a/doc/workflow/lfs/lfs_administration.md b/doc/workflow/lfs/lfs_administration.md
index 9dc1e9b47e3..b3c73e947f0 100644
--- a/doc/workflow/lfs/lfs_administration.md
+++ b/doc/workflow/lfs/lfs_administration.md
@@ -45,5 +45,5 @@ In `config/gitlab.yml`:
 * Currently, storing GitLab Git LFS objects on a non-local storage (like S3 buckets)
   is not supported
 * Currently, removing LFS objects from GitLab Git LFS storage is not supported
-* LFS authentications via SSH is not supported for the time being
-* Only compatible with the GitLFS client versions 1.1.0 or 1.0.2.
+* LFS authentications via SSH was added with GitLab 8.12
+* Only compatible with the GitLFS client versions 1.1.0 and up, or 1.0.2.
diff --git a/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md b/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md
index 9fe065fa680..1a4f213a792 100644
--- a/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md
+++ b/doc/workflow/lfs/manage_large_binaries_with_git_lfs.md
@@ -35,6 +35,10 @@ Documentation for GitLab instance administrators is under [LFS administration do
   credentials store is recommended
 * Git LFS always assumes HTTPS so if you have GitLab server on HTTP you will have
   to add the URL to Git config manually (see #troubleshooting)
+  
+>**Note**: With 8.12 GitLab added LFS support to SSH. The Git LFS communication
+ still goes over HTTP, but now the SSH client passes the correct credentials
+ to the Git LFS client, so no action is required by the user.
 
 ## Using Git LFS
 
@@ -132,6 +136,10 @@ git config --add lfs.url "http://gitlab.example.com/group/project.git/info/lfs"
 
 ### Credentials are always required when pushing an object
 
+>**Note**: With 8.12 GitLab added LFS support to SSH. The Git LFS communication
+ still goes over HTTP, but now the SSH client passes the correct credentials
+ to the Git LFS client, so no action is required by the user.
+
 Given that Git LFS uses HTTP Basic Authentication to authenticate the user pushing
 the LFS object on every push for every object, user HTTPS credentials are required.
 
-- 
cgit v1.2.1


From cb85cf1f0a7047c485d7b29b2792b8965e270898 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Mon, 29 Aug 2016 13:05:07 -0500
Subject: Refactor LFS token logic to use a Redis key instead of a DB field,
 making it a 1 use only token.

---
 .../projects/git_http_client_controller.rb         |  3 +-
 app/helpers/lfs_helper.rb                          |  6 +++-
 app/models/deploy_key.rb                           |  5 ----
 app/models/user.rb                                 |  3 +-
 .../20160825173042_add_lfs_token_to_users.rb       | 16 ----------
 db/migrate/20160825182924_add_lfs_token_to_keys.rb | 16 ----------
 lib/api/entities.rb                                |  2 +-
 lib/api/internal.rb                                |  9 +++---
 lib/gitlab/auth.rb                                 | 12 ++++----
 lib/gitlab/lfs_token.rb                            | 29 ++++++++++++++++++
 spec/lib/gitlab/auth_spec.rb                       |  8 +++--
 spec/lib/gitlab/lfs_token_spec.rb                  | 35 ++++++++++++++++++++++
 spec/models/concerns/token_authenticatable_spec.rb | 20 -------------
 spec/requests/api/internal_spec.rb                 |  6 ++--
 spec/requests/lfs_http_spec.rb                     |  2 +-
 15 files changed, 93 insertions(+), 79 deletions(-)
 delete mode 100644 db/migrate/20160825173042_add_lfs_token_to_users.rb
 delete mode 100644 db/migrate/20160825182924_add_lfs_token_to_keys.rb
 create mode 100644 lib/gitlab/lfs_token.rb
 create mode 100644 spec/lib/gitlab/lfs_token_spec.rb

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 1e6709dc8eb..4dff1ce6568 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -132,8 +132,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   end
 
   def lfs_deploy_key?
-    key = user
-    @lfs_deploy_key.present? && (key && key.projects.include?(project))
+    @lfs_deploy_key.present? && (user && user.projects.include?(project))
   end
 
   def verify_workhorse_api!
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 0c867fc8741..2f5709a7395 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -25,7 +25,11 @@ module LfsHelper
   def lfs_download_access?
     return false unless project.lfs_enabled?
 
-    project.public? || ci? || lfs_deploy_key? || (user && user.can?(:download_code, project))
+    return true if project.public?
+    return true if ci?
+    return true if lfs_deploy_key?
+
+    (user && user.can?(:download_code, project))
   end
 
   def lfs_upload_access?
diff --git a/app/models/deploy_key.rb b/app/models/deploy_key.rb
index de51b63c120..2c525d4cd7a 100644
--- a/app/models/deploy_key.rb
+++ b/app/models/deploy_key.rb
@@ -1,12 +1,7 @@
 class DeployKey < Key
-  include TokenAuthenticatable
-  add_authentication_token_field :lfs_token
-
   has_many :deploy_keys_projects, dependent: :destroy
   has_many :projects, through: :deploy_keys_projects
 
-  before_save :ensure_lfs_token
-
   scope :in_projects, ->(projects) { joins(:deploy_keys_projects).where('deploy_keys_projects.project_id in (?)', projects) }
   scope :are_public,  -> { where(public: true) }
 
diff --git a/app/models/user.rb b/app/models/user.rb
index 94ae3911859..6996740eebd 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -13,7 +13,6 @@ class User < ActiveRecord::Base
   DEFAULT_NOTIFICATION_LEVEL = :participating
 
   add_authentication_token_field :authentication_token
-  add_authentication_token_field :lfs_token
 
   default_value_for :admin, false
   default_value_for(:external) { current_application_settings.user_default_external }
@@ -118,7 +117,7 @@ class User < ActiveRecord::Base
   before_validation :set_public_email, if: ->(user) { user.public_email_changed? }
 
   after_update :update_emails_with_primary_email, if: ->(user) { user.email_changed? }
-  before_save :ensure_authentication_token, :ensure_lfs_token
+  before_save :ensure_authentication_token
   before_save :ensure_external_user_rights
   after_save :ensure_namespace_correct
   after_initialize :set_projects_limit
diff --git a/db/migrate/20160825173042_add_lfs_token_to_users.rb b/db/migrate/20160825173042_add_lfs_token_to_users.rb
deleted file mode 100644
index f7f210bcd67..00000000000
--- a/db/migrate/20160825173042_add_lfs_token_to_users.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
-class AddLfsTokenToUsers < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # Set this constant to true if this migration requires downtime.
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def change
-    add_column :users, :lfs_token, :string
-    add_concurrent_index :users, :lfs_token
-  end
-end
diff --git a/db/migrate/20160825182924_add_lfs_token_to_keys.rb b/db/migrate/20160825182924_add_lfs_token_to_keys.rb
deleted file mode 100644
index 3ff010ef328..00000000000
--- a/db/migrate/20160825182924_add_lfs_token_to_keys.rb
+++ /dev/null
@@ -1,16 +0,0 @@
-# See http://doc.gitlab.com/ce/development/migration_style_guide.html
-# for more information on how to write migrations for GitLab.
-
-class AddLfsTokenToKeys < ActiveRecord::Migration
-  include Gitlab::Database::MigrationHelpers
-
-  # Set this constant to true if this migration requires downtime.
-  DOWNTIME = false
-
-  disable_ddl_transaction!
-
-  def change
-    add_column :keys, :lfs_token, :string
-    add_concurrent_index :keys, :lfs_token
-  end
-end
diff --git a/lib/api/entities.rb b/lib/api/entities.rb
index b4fcacca896..4f736e4ec2b 100644
--- a/lib/api/entities.rb
+++ b/lib/api/entities.rb
@@ -1,7 +1,7 @@
 module API
   module Entities
     class UserSafe < Grape::Entity
-      expose :name, :username, :lfs_token
+      expose :name, :username
     end
 
     class UserBasic < UserSafe
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 7c0a6eaa652..760f69663ab 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -88,12 +88,13 @@ module API
       get "/discover" do
         key = Key.find(params[:key_id])
         user = key.user
+
         if user
-          user.ensure_lfs_token!
-          present user, with: Entities::UserSafe
+          token = Gitlab::LfsToken.new(user).set_token
+          { name: user.name, username: user.username, lfs_token: token }
         else
-          key.ensure_lfs_token!
-          { username: 'lfs-deploy-key', lfs_token: key.lfs_token }
+          token = Gitlab::LfsToken.new(key).set_token
+          { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
         end
       end
 
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 5446093de4d..e43f8119658 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -117,12 +117,14 @@ module Gitlab
       end
 
       def lfs_token_check(login, password)
-        if login == 'lfs-deploy-key'
-          key = DeployKey.find_by_lfs_token(password)
-          Result.new(key, :lfs_deploy_token) if key
+        if login.include?('lfs-deploy-key')
+          key = DeployKey.find(login.gsub('lfs-deploy-key-', ''))
+          token = Gitlab::LfsToken.new(key).get_value
+          Result.new(key, :lfs_deploy_token) if key && token == password
         else
-          user = User.find_by_lfs_token(password)
-          Result.new(user, :lfs_token) if user && user.username == login
+          user = User.by_login(login)
+          token = Gitlab::LfsToken.new(user).get_value
+          Result.new(user, :lfs_token) if user && token == password
         end
       end
     end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
new file mode 100644
index 00000000000..0685eb775ef
--- /dev/null
+++ b/lib/gitlab/lfs_token.rb
@@ -0,0 +1,29 @@
+module Gitlab
+  class LfsToken
+    attr_accessor :actor
+
+    def initialize(actor)
+      @actor = actor
+    end
+
+    def set_token
+      token = Devise.friendly_token(50)
+      Gitlab::Redis.with do |redis|
+        redis.set(redis_key, token, ex: 3600)
+      end
+      token
+    end
+
+    def get_value
+      Gitlab::Redis.with do |redis|
+        redis.get(redis_key)
+      end
+    end
+
+    private
+
+    def redis_key
+      "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
+    end
+  end
+end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index cd00a15be3b..6ce680e3c26 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -26,17 +26,19 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes user lfs tokens' do
       user = create(:user)
       ip = 'ip'
+      token = Gitlab::LfsToken.new(user).set_token
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
-      expect(gl_auth.find_for_git_client(user.username, user.lfs_token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
+      expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
     end
 
     it 'recognizes deploy key lfs tokens' do
       key = create(:deploy_key)
       ip = 'ip'
+      token = Gitlab::LfsToken.new(key).set_token
 
-      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'lfs-deploy-key')
-      expect(gl_auth.find_for_git_client('lfs-deploy-key', key.lfs_token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs-deploy-key-#{key.id}")
+      expect(gl_auth.find_for_git_client("lfs-deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
     end
 
     it 'recognizes OAuth tokens' do
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
new file mode 100644
index 00000000000..76b348637c7
--- /dev/null
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -0,0 +1,35 @@
+require 'spec_helper'
+
+describe Gitlab::LfsToken, lib: true do
+  describe '#set_token and #get_value' do
+    shared_examples 'an LFS token generator' do
+      it 'returns a randomly generated token' do
+        token = handler.set_token
+
+        expect(token).not_to be_nil
+        expect(token).to be_a String
+        expect(token.length).to eq 50
+      end
+
+      it 'returns the correct token based on the key' do
+        token = handler.set_token
+
+        expect(handler.get_value).to eq(token)
+      end
+    end
+
+    context 'when the actor is a user' do
+      let(:actor) { create(:user) }
+      let(:handler) { described_class.new(actor) }
+
+      it_behaves_like 'an LFS token generator'
+    end
+
+    context 'when the actor is a deploy key' do
+      let(:actor) { create(:deploy_key) }
+      let(:handler) { described_class.new(actor) }
+
+      it_behaves_like 'an LFS token generator'
+    end
+  end
+end
diff --git a/spec/models/concerns/token_authenticatable_spec.rb b/spec/models/concerns/token_authenticatable_spec.rb
index 82076600f3b..eb64f3d0c83 100644
--- a/spec/models/concerns/token_authenticatable_spec.rb
+++ b/spec/models/concerns/token_authenticatable_spec.rb
@@ -18,26 +18,6 @@ describe User, 'TokenAuthenticatable' do
     subject { create(:user).send(token_field) }
     it { is_expected.to be_a String }
   end
-
-  describe 'lfs token' do
-    let(:token_field) { :lfs_token }
-    it_behaves_like 'TokenAuthenticatable'
-
-    describe 'ensure it' do
-      subject { create(:user).send(token_field) }
-      it { is_expected.to be_a String }
-    end
-  end
-end
-
-describe DeployKey, 'TokenAuthenticatable' do
-  let(:token_field) { :lfs_token }
-  it_behaves_like 'TokenAuthenticatable'
-
-  describe 'ensures authentication token' do
-    subject { create(:deploy_key).send(token_field) }
-    it { is_expected.to be_a String }
-  end
 end
 
 describe ApplicationSetting, 'TokenAuthenticatable' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 95fc5f790e8..59df5af770b 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -108,7 +108,7 @@ describe API::API, api: true  do
         expect(response).to have_http_status(200)
 
         expect(json_response['name']).to eq(user.name)
-        expect(json_response['lfs_token']).to eq(user.lfs_token)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).get_value)
       end
     end
 
@@ -120,8 +120,8 @@ describe API::API, api: true  do
 
         expect(response).to have_http_status(200)
 
-        expect(json_response['username']).to eq('lfs-deploy-key')
-        expect(json_response['lfs_token']).to eq(key.lfs_token)
+        expect(json_response['username']).to eq("lfs-deploy-key-#{key.id}")
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).get_value)
       end
     end
   end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index 58f8515c0e2..d15e72b2570 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -917,7 +917,7 @@ describe 'Git LFS API and storage' do
   end
 
   def authorize_deploy_key
-    ActionController::HttpAuthentication::Basic.encode_credentials('lfs-deploy-key', key.lfs_token)
+    ActionController::HttpAuthentication::Basic.encode_credentials("lfs-deploy-key-#{key.id}", Gitlab::LfsToken.new(key).set_token)
   end
 
   def fork_project(project, user, object = nil)
-- 
cgit v1.2.1


From 48f1a61fd5c6aac395be0ce5d59aee61bbb69fe9 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Tue, 30 Aug 2016 13:38:22 -0500
Subject: Refactored LFS auth logic when using SSH to use its own API endpoint
 `/lfs_authenticate` and added tests.

---
 lib/api/internal.rb                | 30 ++++++++++++++++------------
 lib/gitlab/auth.rb                 |  4 ++--
 lib/gitlab/lfs_token.rb            |  8 +++++---
 spec/lib/gitlab/auth_spec.rb       |  4 ++--
 spec/lib/gitlab/lfs_token_spec.rb  |  6 +++---
 spec/requests/api/internal_spec.rb | 40 +++++++++++++++++++++++++++++---------
 spec/requests/lfs_http_spec.rb     |  2 +-
 7 files changed, 62 insertions(+), 32 deletions(-)

diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 760f69663ab..1b3388347a8 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -69,12 +69,26 @@ module API
             else
               project.repository.path_to_repo
             end
+        end
+
+        response
+      end
+
+      post "/lfs_authenticate" do
+        status 200
+
+        key = Key.find(params[:key_id])
+        user = key.user
 
-          # Return HTTP full path, so that gitlab-shell has this information
-          # ready for git-lfs-authenticate
-          response[:repository_http_path] = project.http_url_to_repo
+        if user
+          token = Gitlab::LfsToken.new(user).generate
+          response = { username: user.username, lfs_token: token }
+        else
+          token = Gitlab::LfsToken.new(key).generate
+          response = { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
         end
 
+        response[:repository_http_path] = project.http_url_to_repo
         response
       end
 
@@ -87,15 +101,7 @@ module API
       #
       get "/discover" do
         key = Key.find(params[:key_id])
-        user = key.user
-
-        if user
-          token = Gitlab::LfsToken.new(user).set_token
-          { name: user.name, username: user.username, lfs_token: token }
-        else
-          token = Gitlab::LfsToken.new(key).set_token
-          { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
-        end
+        present key.user, with: Entities::UserSafe
       end
 
       get "/check" do
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index e43f8119658..1b0398d18ee 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -119,11 +119,11 @@ module Gitlab
       def lfs_token_check(login, password)
         if login.include?('lfs-deploy-key')
           key = DeployKey.find(login.gsub('lfs-deploy-key-', ''))
-          token = Gitlab::LfsToken.new(key).get_value
+          token = Gitlab::LfsToken.new(key).value
           Result.new(key, :lfs_deploy_token) if key && token == password
         else
           user = User.by_login(login)
-          token = Gitlab::LfsToken.new(user).get_value
+          token = Gitlab::LfsToken.new(user).value
           Result.new(user, :lfs_token) if user && token == password
         end
       end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 0685eb775ef..63656f0b4f1 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -6,15 +6,17 @@ module Gitlab
       @actor = actor
     end
 
-    def set_token
+    def generate
       token = Devise.friendly_token(50)
+
       Gitlab::Redis.with do |redis|
-        redis.set(redis_key, token, ex: 3600)
+        redis.set(redis_key, token, ex: 600)
       end
+
       token
     end
 
-    def get_value
+    def value
       Gitlab::Redis.with do |redis|
         redis.get(redis_key)
       end
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 6ce680e3c26..4c8e09cd904 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -26,7 +26,7 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes user lfs tokens' do
       user = create(:user)
       ip = 'ip'
-      token = Gitlab::LfsToken.new(user).set_token
+      token = Gitlab::LfsToken.new(user).generate
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username)
       expect(gl_auth.find_for_git_client(user.username, token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :lfs_token))
@@ -35,7 +35,7 @@ describe Gitlab::Auth, lib: true do
     it 'recognizes deploy key lfs tokens' do
       key = create(:deploy_key)
       ip = 'ip'
-      token = Gitlab::LfsToken.new(key).set_token
+      token = Gitlab::LfsToken.new(key).generate
 
       expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs-deploy-key-#{key.id}")
       expect(gl_auth.find_for_git_client("lfs-deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index 76b348637c7..1d2e4fd9566 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -4,7 +4,7 @@ describe Gitlab::LfsToken, lib: true do
   describe '#set_token and #get_value' do
     shared_examples 'an LFS token generator' do
       it 'returns a randomly generated token' do
-        token = handler.set_token
+        token = handler.generate
 
         expect(token).not_to be_nil
         expect(token).to be_a String
@@ -12,9 +12,9 @@ describe Gitlab::LfsToken, lib: true do
       end
 
       it 'returns the correct token based on the key' do
-        token = handler.set_token
+        token = handler.generate
 
-        expect(handler.get_value).to eq(token)
+        expect(handler.value).to eq(token)
       end
     end
 
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 59df5af770b..ff697286927 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -100,15 +100,20 @@ describe API::API, api: true  do
     end
   end
 
-  describe "GET /internal/discover" do
+  describe "POST /internal/lfs_authenticate" do
+    before do
+      project.team << [user, :developer]
+    end
+
     context 'user key' do
       it 'returns the correct information about the key' do
-        get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+        lfs_auth(key, project)
 
         expect(response).to have_http_status(200)
+        expect(json_response['username']).to eq(user.username)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).value)
 
-        expect(json_response['name']).to eq(user.name)
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).get_value)
+        expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
     end
 
@@ -116,16 +121,26 @@ describe API::API, api: true  do
       let(:key) { create(:deploy_key) }
 
       it 'returns the correct information about the key' do
-        get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+        lfs_auth(key, project)
 
         expect(response).to have_http_status(200)
-
         expect(json_response['username']).to eq("lfs-deploy-key-#{key.id}")
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).get_value)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value)
+        expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
     end
   end
 
+  describe "GET /internal/discover" do
+    it do
+      get(api("/internal/discover"), key_id: key.id, secret_token: secret_token)
+
+      expect(response).to have_http_status(200)
+
+      expect(json_response['name']).to eq(user.name)
+    end
+  end
+
   describe "POST /internal/allowed" do
     context "access granted" do
       before do
@@ -159,7 +174,6 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
-          expect(json_response["repository_http_path"]).to eq(project.http_url_to_repo)
         end
       end
 
@@ -170,7 +184,6 @@ describe API::API, api: true  do
           expect(response).to have_http_status(200)
           expect(json_response["status"]).to be_truthy
           expect(json_response["repository_path"]).to eq(project.repository.path_to_repo)
-          expect(json_response["repository_http_path"]).to eq(project.http_url_to_repo)
         end
       end
     end
@@ -407,4 +420,13 @@ describe API::API, api: true  do
       protocol: 'ssh'
     )
   end
+
+  def lfs_auth(key, project)
+    post(
+      api("/internal/lfs_authenticate"),
+      key_id: key.id,
+      secret_token: secret_token,
+      project: project.path_with_namespace
+    )
+  end
 end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index d15e72b2570..e61502400ff 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -917,7 +917,7 @@ describe 'Git LFS API and storage' do
   end
 
   def authorize_deploy_key
-    ActionController::HttpAuthentication::Basic.encode_credentials("lfs-deploy-key-#{key.id}", Gitlab::LfsToken.new(key).set_token)
+    ActionController::HttpAuthentication::Basic.encode_credentials("lfs-deploy-key-#{key.id}", Gitlab::LfsToken.new(key).generate)
   end
 
   def fork_project(project, user, object = nil)
-- 
cgit v1.2.1


From c25630ee2c2804e351a2c3ae4fd9224434e4698a Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Tue, 30 Aug 2016 18:43:24 -0500
Subject: Refactored handling of the `LfsToken` and added functionality to it
 to simplify external code.

---
 app/helpers/lfs_helper.rb         |  4 +---
 lib/api/internal.rb               | 20 +++++++++++---------
 lib/gitlab/auth.rb                | 19 ++++++++++---------
 lib/gitlab/lfs_token.rb           |  8 ++++++++
 spec/lib/gitlab/lfs_token_spec.rb | 16 ++++++++++++++++
 5 files changed, 46 insertions(+), 21 deletions(-)

diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 2f5709a7395..031e7e72909 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -25,9 +25,7 @@ module LfsHelper
   def lfs_download_access?
     return false unless project.lfs_enabled?
 
-    return true if project.public?
-    return true if ci?
-    return true if lfs_deploy_key?
+    return true if project.public? || ci? || lfs_deploy_key?
 
     (user && user.can?(:download_code, project))
   end
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 1b3388347a8..1f189d81d16 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -80,16 +80,18 @@ module API
         key = Key.find(params[:key_id])
         user = key.user
 
-        if user
-          token = Gitlab::LfsToken.new(user).generate
-          response = { username: user.username, lfs_token: token }
-        else
-          token = Gitlab::LfsToken.new(key).generate
-          response = { username: "lfs-deploy-key-#{key.id}", lfs_token: token }
-        end
+        token_handler =
+          if user
+            Gitlab::LfsToken.new(user)
+          else
+            Gitlab::LfsToken.new(key)
+          end
 
-        response[:repository_http_path] = project.http_url_to_repo
-        response
+        {
+          username: token_handler.actor_name,
+          lfs_token: token_handler.generate,
+          repository_http_path: project.http_url_to_repo
+        }
       end
 
       get "/merge_request_urls" do
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 1b0398d18ee..821c0ef87e9 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -117,15 +117,16 @@ module Gitlab
       end
 
       def lfs_token_check(login, password)
-        if login.include?('lfs-deploy-key')
-          key = DeployKey.find(login.gsub('lfs-deploy-key-', ''))
-          token = Gitlab::LfsToken.new(key).value
-          Result.new(key, :lfs_deploy_token) if key && token == password
-        else
-          user = User.by_login(login)
-          token = Gitlab::LfsToken.new(user).value
-          Result.new(user, :lfs_token) if user && token == password
-        end
+        actor =
+          if login.include?('lfs-deploy-key')
+            DeployKey.find(login.gsub('lfs-deploy-key-', ''))
+          else
+            User.by_login(login)
+          end
+
+        token_handler = Gitlab::LfsToken.new(actor)
+
+        Result.new(actor, token_handler.type) if actor && token_handler.value == password
       end
     end
   end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 63656f0b4f1..8f49deb4d03 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -22,6 +22,14 @@ module Gitlab
       end
     end
 
+    def type
+      actor.is_a?(User) ? :lfs_token : :lfs_deploy_token
+    end
+
+    def actor_name
+      actor.is_a?(User) ? actor.username : "lfs-deploy-key-#{actor.id}"
+    end
+
     private
 
     def redis_key
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index 1d2e4fd9566..f9812664e3b 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -23,6 +23,14 @@ describe Gitlab::LfsToken, lib: true do
       let(:handler) { described_class.new(actor) }
 
       it_behaves_like 'an LFS token generator'
+
+      it 'returns the correct username' do
+        expect(handler.actor_name).to eq(actor.username)
+      end
+
+      it 'returns the correct token type' do
+        expect(handler.type).to eq(:lfs_token)
+      end
     end
 
     context 'when the actor is a deploy key' do
@@ -30,6 +38,14 @@ describe Gitlab::LfsToken, lib: true do
       let(:handler) { described_class.new(actor) }
 
       it_behaves_like 'an LFS token generator'
+
+      it 'returns the correct username' do
+        expect(handler.actor_name).to eq("lfs-deploy-key-#{actor.id}")
+      end
+
+      it 'returns the correct token type' do
+        expect(handler.type).to eq(:lfs_deploy_token)
+      end
     end
   end
 end
-- 
cgit v1.2.1


From 85152f0291b7e6dd4a92a068e7d5c4334df54e80 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 31 Aug 2016 11:03:46 -0500
Subject: Improve string handling.

---
 lib/gitlab/auth.rb | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 821c0ef87e9..02b33c8c683 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -118,8 +118,8 @@ module Gitlab
 
       def lfs_token_check(login, password)
         actor =
-          if login.include?('lfs-deploy-key')
-            DeployKey.find(login.gsub('lfs-deploy-key-', ''))
+          if login.start_with?('lfs-deploy-key')
+            DeployKey.find(login.sub('lfs-deploy-key-', ''))
           else
             User.by_login(login)
           end
-- 
cgit v1.2.1


From c144db2935f0f71c7f282a3015d126526bc16b57 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Tue, 6 Sep 2016 16:32:39 -0500
Subject: Better authentication handling, syntax fixes and better actor
 handling for LFS Tokens

---
 .../projects/git_http_client_controller.rb         | 27 ++++++++---------
 app/helpers/lfs_helper.rb                          |  2 +-
 lib/api/internal.rb                                |  9 +-----
 lib/gitlab/auth.rb                                 | 35 +++++++++++-----------
 lib/gitlab/lfs_token.rb                            | 21 +++++++++++--
 spec/requests/api/internal_spec.rb                 |  2 +-
 6 files changed, 51 insertions(+), 45 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 4dff1ce6568..b4ec5b3fae1 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,8 +4,6 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
-  class MissingPersonalTokenError < StandardError; end
-
   attr_reader :user
 
   # Git clients will not know what authenticity token to send along
@@ -40,10 +38,8 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
     send_challenges
     render plain: "HTTP Basic: Access denied\n", status: 401
-
-  rescue MissingPersonalTokenError
+  rescue Gitlab::Auth::MissingPersonalTokenError
     render_missing_personal_token
-    return
   end
 
   def basic_auth_provided?
@@ -117,17 +113,20 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   def handle_authentication(login, password)
     auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
 
-    if auth_result.type == :ci && download_request?
-      @ci = true
-    elsif auth_result.type == :oauth && !download_request?
-      # Not allowed
-    elsif auth_result.type == :missing_personal_token
-      raise MissingPersonalTokenError
-    elsif auth_result.type == :lfs_deploy_token && download_request?
-      @lfs_deploy_key = true
+    case auth_result.type
+    when :ci
+      @ci = true if download_request?
+    when :oauth
+      @user = auth_result.user if download_request?
+    when :lfs_deploy_token
+      if download_request?
+        @lfs_deploy_key = true
+        @user = auth_result.user
+      end
+    when :lfs_token, :personal_token, :gitlab_or_ldap
       @user = auth_result.user
     else
-      @user = auth_result.user
+      # Not allowed
     end
   end
 
diff --git a/app/helpers/lfs_helper.rb b/app/helpers/lfs_helper.rb
index 031e7e72909..de7c9f253b2 100644
--- a/app/helpers/lfs_helper.rb
+++ b/app/helpers/lfs_helper.rb
@@ -27,7 +27,7 @@ module LfsHelper
 
     return true if project.public? || ci? || lfs_deploy_key?
 
-    (user && user.can?(:download_code, project))
+    user && user.can?(:download_code, project)
   end
 
   def lfs_upload_access?
diff --git a/lib/api/internal.rb b/lib/api/internal.rb
index 1f189d81d16..f8211bdd8af 100644
--- a/lib/api/internal.rb
+++ b/lib/api/internal.rb
@@ -78,14 +78,7 @@ module API
         status 200
 
         key = Key.find(params[:key_id])
-        user = key.user
-
-        token_handler =
-          if user
-            Gitlab::LfsToken.new(user)
-          else
-            Gitlab::LfsToken.new(key)
-          end
+        token_handler = Gitlab::LfsToken.new(key)
 
         {
           username: token_handler.actor_name,
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 02b33c8c683..14e29124aac 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -2,21 +2,13 @@ module Gitlab
   module Auth
     Result = Struct.new(:user, :type)
 
+    class MissingPersonalTokenError < StandardError; end
+
     class << self
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
-        result = Result.new
-
-        if valid_ci_request?(login, password, project)
-          result.type = :ci
-        else
-          result = populate_result(login, password)
-        end
-
-        success = result.user.present? || [:ci, :missing_personal_token].include?(result.type)
-        rate_limit!(ip, success: success, login: login)
-        result
+        populate_result(login, password, project, ip)
       end
 
       def find_with_user_password(login, password)
@@ -75,21 +67,26 @@ module Gitlab
         end
       end
 
-      def populate_result(login, password)
-        result =
+      def populate_result(login, password, project, ip)
+        result = Result.new(nil, :ci) if valid_ci_request?(login, password, project)
+
+        result ||=
           user_with_password_for_git(login, password) ||
           oauth_access_token_check(login, password) ||
           lfs_token_check(login, password) ||
           personal_access_token_check(login, password)
 
-        if result
+        if result && result.type != :ci
           result.type = nil unless result.user
 
           if result.user && result.type == :gitlab_or_ldap && result.user.two_factor_enabled?
-            result.type = :missing_personal_token
+            raise Gitlab::Auth::MissingPersonalTokenError
           end
         end
 
+        success = result ? result.user.present? || [:ci].include?(result.type) : false
+        rate_limit!(ip, success: success, login: login)
+
         result || Result.new
       end
 
@@ -118,15 +115,17 @@ module Gitlab
 
       def lfs_token_check(login, password)
         actor =
-          if login.start_with?('lfs-deploy-key')
-            DeployKey.find(login.sub('lfs-deploy-key-', ''))
+          if login =~ /\Alfs-deploy-key-\d+\Z/
+            /\d+\Z/.match(login) do |id|
+              DeployKey.find(id[0])
+            end
           else
             User.by_login(login)
           end
 
         token_handler = Gitlab::LfsToken.new(actor)
 
-        Result.new(actor, token_handler.type) if actor && token_handler.value == password
+        Result.new(actor, token_handler.type) if actor && Devise.secure_compare(token_handler.value, password)
       end
     end
   end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 8f49deb4d03..d7db8017475 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -2,15 +2,18 @@ module Gitlab
   class LfsToken
     attr_accessor :actor
 
+    TOKEN_LENGTH = 50
+    EXPIRY_TIME = 1800
+
     def initialize(actor)
-      @actor = actor
+      set_actor(actor)
     end
 
     def generate
-      token = Devise.friendly_token(50)
+      token = Devise.friendly_token(TOKEN_LENGTH)
 
       Gitlab::Redis.with do |redis|
-        redis.set(redis_key, token, ex: 600)
+        redis.set(redis_key, token, ex: EXPIRY_TIME)
       end
 
       token
@@ -35,5 +38,17 @@ module Gitlab
     def redis_key
       "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
     end
+
+    def set_actor(actor)
+      @actor =
+        case actor
+        when DeployKey, User
+          actor
+        when Key
+          actor.user
+        else
+          #
+        end
+    end
   end
 end
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index ff697286927..1ee390e0a19 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -111,7 +111,7 @@ describe API::API, api: true  do
 
         expect(response).to have_http_status(200)
         expect(json_response['username']).to eq(user.username)
-        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(user).value)
+        expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value)
 
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
-- 
cgit v1.2.1


From 71aff7f6a3ab63f1395bfab6ea49f0175fe08167 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Wed, 7 Sep 2016 11:55:54 -0500
Subject: Use special characters for `lfs+deploy-key` to prevent a someone from
 creating a user with this username, and method name refactoring.

---
 app/controllers/projects/git_http_client_controller.rb | 4 ++--
 lib/gitlab/auth.rb                                     | 2 +-
 lib/gitlab/lfs_token.rb                                | 2 +-
 spec/lib/gitlab/auth_spec.rb                           | 4 ++--
 spec/lib/gitlab/lfs_token_spec.rb                      | 2 +-
 spec/requests/api/internal_spec.rb                     | 2 +-
 spec/requests/lfs_http_spec.rb                         | 2 +-
 7 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index b4ec5b3fae1..4be580e759e 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -22,7 +22,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     if allow_basic_auth? && basic_auth_provided?
       login, password = user_name_and_password(request)
 
-      handle_authentication(login, password)
+      handle_basic_authentication(login, password)
 
       if ci? || user
         return # Allow access
@@ -110,7 +110,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @ci.present?
   end
 
-  def handle_authentication(login, password)
+  def handle_basic_authentication(login, password)
     auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
 
     case auth_result.type
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 14e29124aac..f4e6ebb7bc7 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -115,7 +115,7 @@ module Gitlab
 
       def lfs_token_check(login, password)
         actor =
-          if login =~ /\Alfs-deploy-key-\d+\Z/
+          if login =~ /\Alfs\+deploy-key-\d+\Z/
             /\d+\Z/.match(login) do |id|
               DeployKey.find(id[0])
             end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index d7db8017475..edf4dffc4c0 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -30,7 +30,7 @@ module Gitlab
     end
 
     def actor_name
-      actor.is_a?(User) ? actor.username : "lfs-deploy-key-#{actor.id}"
+      actor.is_a?(User) ? actor.username : "lfs+deploy-key-#{actor.id}"
     end
 
     private
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 4c8e09cd904..56f349f5d92 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -37,8 +37,8 @@ describe Gitlab::Auth, lib: true do
       ip = 'ip'
       token = Gitlab::LfsToken.new(key).generate
 
-      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs-deploy-key-#{key.id}")
-      expect(gl_auth.find_for_git_client("lfs-deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: "lfs+deploy-key-#{key.id}")
+      expect(gl_auth.find_for_git_client("lfs+deploy-key-#{key.id}", token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(key, :lfs_deploy_token))
     end
 
     it 'recognizes OAuth tokens' do
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index f9812664e3b..184f235c1b2 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -40,7 +40,7 @@ describe Gitlab::LfsToken, lib: true do
       it_behaves_like 'an LFS token generator'
 
       it 'returns the correct username' do
-        expect(handler.actor_name).to eq("lfs-deploy-key-#{actor.id}")
+        expect(handler.actor_name).to eq("lfs+deploy-key-#{actor.id}")
       end
 
       it 'returns the correct token type' do
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 1ee390e0a19..2e1e6a11b53 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -124,7 +124,7 @@ describe API::API, api: true  do
         lfs_auth(key, project)
 
         expect(response).to have_http_status(200)
-        expect(json_response['username']).to eq("lfs-deploy-key-#{key.id}")
+        expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
         expect(json_response['lfs_token']).to eq(Gitlab::LfsToken.new(key).value)
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
diff --git a/spec/requests/lfs_http_spec.rb b/spec/requests/lfs_http_spec.rb
index e61502400ff..54ecb793729 100644
--- a/spec/requests/lfs_http_spec.rb
+++ b/spec/requests/lfs_http_spec.rb
@@ -917,7 +917,7 @@ describe 'Git LFS API and storage' do
   end
 
   def authorize_deploy_key
-    ActionController::HttpAuthentication::Basic.encode_credentials("lfs-deploy-key-#{key.id}", Gitlab::LfsToken.new(key).generate)
+    ActionController::HttpAuthentication::Basic.encode_credentials("lfs+deploy-key-#{key.id}", Gitlab::LfsToken.new(key).generate)
   end
 
   def fork_project(project, user, object = nil)
-- 
cgit v1.2.1


From de24075ea5960bd7c6290c05496915e8f0ca23f2 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Thu, 8 Sep 2016 12:32:20 -0500
Subject: Further refactoring of authentication code, and code style fixes.

---
 .../projects/git_http_client_controller.rb         | 20 ++++----
 lib/gitlab/auth.rb                                 | 53 +++++++++++-----------
 lib/gitlab/lfs_token.rb                            | 22 ++++-----
 3 files changed, 48 insertions(+), 47 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index 4be580e759e..f5a07608bf8 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
-  attr_reader :user
+  attr_reader :user, :actor
 
   # Git clients will not know what authenticity token to send along
   skip_before_action :verify_authenticity_token
@@ -24,13 +24,13 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
       handle_basic_authentication(login, password)
 
-      if ci? || user
+      if ci? || actor
         return # Allow access
       end
     elsif allow_kerberos_spnego_auth? && spnego_provided?
-      @user = find_kerberos_user
+      @actor = find_kerberos_user
 
-      if user
+      if actor
         send_final_spnego_response
         return # Allow access
       end
@@ -110,6 +110,10 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     @ci.present?
   end
 
+  def user
+    @actor
+  end
+
   def handle_basic_authentication(login, password)
     auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip)
 
@@ -117,21 +121,21 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     when :ci
       @ci = true if download_request?
     when :oauth
-      @user = auth_result.user if download_request?
+      @actor = auth_result.actor if download_request?
     when :lfs_deploy_token
       if download_request?
         @lfs_deploy_key = true
-        @user = auth_result.user
+        @actor = auth_result.actor
       end
     when :lfs_token, :personal_token, :gitlab_or_ldap
-      @user = auth_result.user
+      @actor = auth_result.actor
     else
       # Not allowed
     end
   end
 
   def lfs_deploy_key?
-    @lfs_deploy_key.present? && (user && user.projects.include?(project))
+    @lfs_deploy_key.present? && actor && actor.projects.include?(project)
   end
 
   def verify_workhorse_api!
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index f4e6ebb7bc7..391b8f2f5de 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,6 +1,6 @@
 module Gitlab
   module Auth
-    Result = Struct.new(:user, :type)
+    Result = Struct.new(:actor, :type)
 
     class MissingPersonalTokenError < StandardError; end
 
@@ -49,6 +49,24 @@ module Gitlab
 
       private
 
+      def populate_result(login, password, project, ip)
+        result =
+          ci_request_check(login, password, project) ||
+          user_with_password_for_git(login, password) ||
+          oauth_access_token_check(login, password) ||
+          lfs_token_check(login, password) ||
+          personal_access_token_check(login, password)
+
+        if result && result.type != :ci
+          result.type = nil unless result.actor
+        end
+
+        success = result ? result.actor.present? || result.type == :ci : false
+        rate_limit!(ip, success: success, login: login)
+
+        result || Result.new
+      end
+
       def valid_ci_request?(login, password, project)
         matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
 
@@ -67,31 +85,14 @@ module Gitlab
         end
       end
 
-      def populate_result(login, password, project, ip)
-        result = Result.new(nil, :ci) if valid_ci_request?(login, password, project)
-
-        result ||=
-          user_with_password_for_git(login, password) ||
-          oauth_access_token_check(login, password) ||
-          lfs_token_check(login, password) ||
-          personal_access_token_check(login, password)
-
-        if result && result.type != :ci
-          result.type = nil unless result.user
-
-          if result.user && result.type == :gitlab_or_ldap && result.user.two_factor_enabled?
-            raise Gitlab::Auth::MissingPersonalTokenError
-          end
-        end
-
-        success = result ? result.user.present? || [:ci].include?(result.type) : false
-        rate_limit!(ip, success: success, login: login)
-
-        result || Result.new
+      def ci_request_check(login, password, project)
+        Result.new(nil, :ci) if valid_ci_request?(login, password, project)
       end
 
       def user_with_password_for_git(login, password)
         user = find_with_user_password(login, password)
+        raise Gitlab::Auth::MissingPersonalTokenError if user && user.two_factor_enabled?
+
         Result.new(user, :gitlab_or_ldap) if user
       end
 
@@ -114,11 +115,11 @@ module Gitlab
       end
 
       def lfs_token_check(login, password)
+        deploy_key_matches = login.match(/\Alfs\+deploy-key-(\d+)\z/)
+
         actor =
-          if login =~ /\Alfs\+deploy-key-\d+\Z/
-            /\d+\Z/.match(login) do |id|
-              DeployKey.find(id[0])
-            end
+          if deploy_key_matches
+            DeployKey.find(deploy_key_matches[1])
           else
             User.by_login(login)
           end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index edf4dffc4c0..224e4516074 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -6,7 +6,15 @@ module Gitlab
     EXPIRY_TIME = 1800
 
     def initialize(actor)
-      set_actor(actor)
+      @actor =
+        case actor
+        when DeployKey, User
+          actor
+        when Key
+          actor.user
+        else
+          #
+        end
     end
 
     def generate
@@ -38,17 +46,5 @@ module Gitlab
     def redis_key
       "gitlab:lfs_token:#{actor.class.name.underscore}_#{actor.id}" if actor
     end
-
-    def set_actor(actor)
-      @actor =
-        case actor
-        when DeployKey, User
-          actor
-        when Key
-          actor.user
-        else
-          #
-        end
-    end
   end
 end
-- 
cgit v1.2.1


From be09bcf074e6048aa9ba5f8dfb99754e6afbe156 Mon Sep 17 00:00:00 2001
From: Patricio Cano <suprnova32@gmail.com>
Date: Thu, 15 Sep 2016 11:54:24 -0500
Subject: Refactored authentication code to make it a bit clearer, added test
 for wrong SSH key.

---
 .../projects/git_http_client_controller.rb         | 25 +++++++++----
 lib/gitlab/auth.rb                                 | 43 ++++++++++------------
 lib/gitlab/lfs_token.rb                            |  2 +-
 spec/lib/gitlab/auth_spec.rb                       |  2 +-
 spec/lib/gitlab/lfs_token_spec.rb                  |  2 +-
 spec/requests/api/internal_spec.rb                 | 14 +++++--
 6 files changed, 50 insertions(+), 38 deletions(-)

diff --git a/app/controllers/projects/git_http_client_controller.rb b/app/controllers/projects/git_http_client_controller.rb
index f5a07608bf8..4dae953b69f 100644
--- a/app/controllers/projects/git_http_client_controller.rb
+++ b/app/controllers/projects/git_http_client_controller.rb
@@ -4,7 +4,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   include ActionController::HttpAuthentication::Basic
   include KerberosSpnegoHelper
 
-  attr_reader :user, :actor
+  attr_reader :actor
 
   # Git clients will not know what authenticity token to send along
   skip_before_action :verify_authenticity_token
@@ -22,9 +22,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
     if allow_basic_auth? && basic_auth_provided?
       login, password = user_name_and_password(request)
 
-      handle_basic_authentication(login, password)
-
-      if ci? || actor
+      if handle_basic_authentication(login, password)
         return # Allow access
       end
     elsif allow_kerberos_spnego_auth? && spnego_provided?
@@ -107,7 +105,7 @@ class Projects::GitHttpClientController < Projects::ApplicationController
   end
 
   def ci?
-    @ci.present?
+    @ci
   end
 
   def user
@@ -119,9 +117,17 @@ class Projects::GitHttpClientController < Projects::ApplicationController
 
     case auth_result.type
     when :ci
-      @ci = true if download_request?
+      if download_request?
+        @ci = true
+      else
+        return false
+      end
     when :oauth
-      @actor = auth_result.actor if download_request?
+      if download_request?
+        @actor = auth_result.actor
+      else
+        return false
+      end
     when :lfs_deploy_token
       if download_request?
         @lfs_deploy_key = true
@@ -131,11 +137,14 @@ class Projects::GitHttpClientController < Projects::ApplicationController
       @actor = auth_result.actor
     else
       # Not allowed
+      return false
     end
+
+    true
   end
 
   def lfs_deploy_key?
-    @lfs_deploy_key.present? && actor && actor.projects.include?(project)
+    @lfs_deploy_key && actor && actor.projects.include?(project)
   end
 
   def verify_workhorse_api!
diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb
index 391b8f2f5de..6be9bf7de44 100644
--- a/lib/gitlab/auth.rb
+++ b/lib/gitlab/auth.rb
@@ -1,6 +1,10 @@
 module Gitlab
   module Auth
-    Result = Struct.new(:actor, :type)
+    Result = Struct.new(:actor, :type) do
+      def success?
+        actor.present? || type == :ci
+      end
+    end
 
     class MissingPersonalTokenError < StandardError; end
 
@@ -8,7 +12,16 @@ module Gitlab
       def find_for_git_client(login, password, project:, ip:)
         raise "Must provide an IP for rate limiting" if ip.nil?
 
-        populate_result(login, password, project, ip)
+        result =
+          ci_request_check(login, password, project) ||
+          user_with_password_for_git(login, password) ||
+          oauth_access_token_check(login, password) ||
+          lfs_token_check(login, password) ||
+          personal_access_token_check(login, password)
+
+        rate_limit!(ip, success: result && result.success?, login: login)
+
+        result || Result.new
       end
 
       def find_with_user_password(login, password)
@@ -49,24 +62,6 @@ module Gitlab
 
       private
 
-      def populate_result(login, password, project, ip)
-        result =
-          ci_request_check(login, password, project) ||
-          user_with_password_for_git(login, password) ||
-          oauth_access_token_check(login, password) ||
-          lfs_token_check(login, password) ||
-          personal_access_token_check(login, password)
-
-        if result && result.type != :ci
-          result.type = nil unless result.actor
-        end
-
-        success = result ? result.actor.present? || result.type == :ci : false
-        rate_limit!(ip, success: success, login: login)
-
-        result || Result.new
-      end
-
       def valid_ci_request?(login, password, project)
         matched_login = /(?<service>^[a-zA-Z]*-ci)-token$/.match(login)
 
@@ -110,7 +105,7 @@ module Gitlab
         if login && password
           user = User.find_by_personal_access_token(password)
           validation = User.by_login(login)
-          Result.new(user, :personal_token) if user == validation
+          Result.new(user, :personal_token) if user.present? && user == validation
         end
       end
 
@@ -124,9 +119,11 @@ module Gitlab
             User.by_login(login)
           end
 
-        token_handler = Gitlab::LfsToken.new(actor)
+        if actor
+          token_handler = Gitlab::LfsToken.new(actor)
 
-        Result.new(actor, token_handler.type) if actor && Devise.secure_compare(token_handler.value, password)
+          Result.new(actor, token_handler.type) if Devise.secure_compare(token_handler.value, password)
+        end
       end
     end
   end
diff --git a/lib/gitlab/lfs_token.rb b/lib/gitlab/lfs_token.rb
index 224e4516074..f492754b1c8 100644
--- a/lib/gitlab/lfs_token.rb
+++ b/lib/gitlab/lfs_token.rb
@@ -13,7 +13,7 @@ module Gitlab
         when Key
           actor.user
         else
-          #
+          raise 'Bad Actor'
         end
     end
 
diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb
index 56f349f5d92..13c5a7156f5 100644
--- a/spec/lib/gitlab/auth_spec.rb
+++ b/spec/lib/gitlab/auth_spec.rb
@@ -55,7 +55,7 @@ describe Gitlab::Auth, lib: true do
       login = 'foo'
       ip = 'ip'
 
-      expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login)
+      expect(gl_auth).to receive(:rate_limit!).with(ip, success: nil, login: login)
       expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new)
     end
   end
diff --git a/spec/lib/gitlab/lfs_token_spec.rb b/spec/lib/gitlab/lfs_token_spec.rb
index 184f235c1b2..9f04f67e0a8 100644
--- a/spec/lib/gitlab/lfs_token_spec.rb
+++ b/spec/lib/gitlab/lfs_token_spec.rb
@@ -1,7 +1,7 @@
 require 'spec_helper'
 
 describe Gitlab::LfsToken, lib: true do
-  describe '#set_token and #get_value' do
+  describe '#generate and #value' do
     shared_examples 'an LFS token generator' do
       it 'returns a randomly generated token' do
         token = handler.generate
diff --git a/spec/requests/api/internal_spec.rb b/spec/requests/api/internal_spec.rb
index 2e1e6a11b53..46e8e6f1169 100644
--- a/spec/requests/api/internal_spec.rb
+++ b/spec/requests/api/internal_spec.rb
@@ -107,7 +107,7 @@ describe API::API, api: true  do
 
     context 'user key' do
       it 'returns the correct information about the key' do
-        lfs_auth(key, project)
+        lfs_auth(key.id, project)
 
         expect(response).to have_http_status(200)
         expect(json_response['username']).to eq(user.username)
@@ -115,13 +115,19 @@ describe API::API, api: true  do
 
         expect(json_response['repository_http_path']).to eq(project.http_url_to_repo)
       end
+
+      it 'returns a 404 when the wrong key is provided' do
+        lfs_auth(nil, project)
+
+        expect(response).to have_http_status(404)
+      end
     end
 
     context 'deploy key' do
       let(:key) { create(:deploy_key) }
 
       it 'returns the correct information about the key' do
-        lfs_auth(key, project)
+        lfs_auth(key.id, project)
 
         expect(response).to have_http_status(200)
         expect(json_response['username']).to eq("lfs+deploy-key-#{key.id}")
@@ -421,10 +427,10 @@ describe API::API, api: true  do
     )
   end
 
-  def lfs_auth(key, project)
+  def lfs_auth(key_id, project)
     post(
       api("/internal/lfs_authenticate"),
-      key_id: key.id,
+      key_id: key_id,
       secret_token: secret_token,
       project: project.path_with_namespace
     )
-- 
cgit v1.2.1