Validate tag-names and references in WebUI, API

9 files changed, 152 insertions, 20 deletions

diff --git a/app/controllers/projects/tags_controller.rb b/app/controllers/projects/tags_controller.rb
index e03a9f4d66d..b84c497131a 100644
--- a/app/controllers/projects/tags_controller.rb
+++ b/app/controllers/projects/tags_controller.rb
@@ -13,10 +13,15 @@ class Projects::TagsController < Projects::ApplicationController
   end
 
   def create
-    @tag = CreateTagService.new.execute(@project, params[:tag_name],
-                                        params[:ref], current_user)
-
-    redirect_to project_tags_path(@project)
+    result = CreateTagService.new.execute(@project, params[:tag_name],
+                                          params[:ref], current_user)
+    if result[:status] == :success
+      @tag = result[:tag]
+      redirect_to project_tags_path(@project)
+    else
+      @error = result[:message]
+      render action: 'new'
+    end
   end
 
   def destroy
diff --git a/app/services/create_tag_service.rb b/app/services/create_tag_service.rb
index 97766677405..6869acbe467 100644
--- a/app/services/create_tag_service.rb
+++ b/app/services/create_tag_service.rb
@@ -1,13 +1,38 @@
 class CreateTagService
   def execute(project, tag_name, ref, current_user)
+    valid_tag = Gitlab::GitRefValidator.validate(tag_name)
+    if valid_tag == false
+      return error('Tag name invalid')
+    end
+
     repository = project.repository
+    existing_tag = repository.find_tag(tag_name)
+    if existing_tag
+      return error('Tag already exists')
+    end
+
     repository.add_tag(tag_name, ref)
     new_tag = repository.find_tag(tag_name)
 
     if new_tag
       Event.create_ref_event(project, current_user, new_tag, 'add', 'refs/tags')
+      return success(new_tag)
+    else
+      return error('Invalid reference name')
     end
+  end
+
+  def error(message)
+    {
+      message: message,
+      status: :error
+    }
+  end
 
-    new_tag
+  def success(branch)
+    {
+      tag: branch,
+      status: :success
+    }
   end
 end
diff --git a/app/views/projects/tags/new.html.haml b/app/views/projects/tags/new.html.haml
index a9fd97f8915..f3a34d37df5 100644
--- a/app/views/projects/tags/new.html.haml
+++ b/app/views/projects/tags/new.html.haml
@@ -1,3 +1,7 @@
+- if @error
+  .alert.alert-danger
+    %button{ type: "button", class: "close", "data-dismiss" => "alert"} &times;
+    = @error
 %h3.page-title
   %i.icon-code-fork
   New tag
@@ -5,11 +9,11 @@
   .form-group
     = label_tag :tag_name, 'Name for new tag', class: 'control-label'
     .col-sm-10
-      = text_field_tag :tag_name, nil, placeholder: 'v3.0.1', required: true, tabindex: 1, class: 'form-control'
+      = text_field_tag :tag_name, params[:tag_name], placeholder: 'v3.0.1', required: true, tabindex: 1, class: 'form-control'
   .form-group
     = label_tag :ref, 'Create from', class: 'control-label'
     .col-sm-10
-      = text_field_tag :ref, nil, placeholder: 'master', required: true, tabindex: 2, class: 'form-control'
+      = text_field_tag :ref, params[:ref], placeholder: 'master', required: true, tabindex: 2, class: 'form-control'
       .light Branch name or commit SHA
   .form-actions
     = submit_tag 'Create tag', class: 'btn btn-create', tabindex: 3
diff --git a/doc/api/repositories.md b/doc/api/repositories.md
index 1074b78fd73..c9f6a45c347 100644
--- a/doc/api/repositories.md
+++ b/doc/api/repositories.md
@@ -71,6 +71,9 @@ Parameters:
 ]
 ```
 
+It returns 200 if the operation succeed. In case of an error,
+405 with an explaining error message is returned.
+
 ## List repository tree
 
 Get a list of repository files and directories in a project.
diff --git a/features/project/commits/branches.feature b/features/project/commits/branches.feature
index 6725a697c21..d124cb7eecd 100644
--- a/features/project/commits/branches.feature
+++ b/features/project/commits/branches.feature
@@ -15,7 +15,7 @@ Feature: Project Browse branches
   Scenario: I create a branch
     Given I visit project branches page
     And I click new branch link
-    When I submit new branch form
+    And I submit new branch form
     Then I should see new branch created
 
   @javascript
@@ -27,17 +27,17 @@ Feature: Project Browse branches
   Scenario: I create a branch with invalid name
     Given I visit project branches page
     And I click new branch link
-    When I submit new branch form with invalid name
+    And I submit new branch form with invalid name
     Then I should see new an error that branch is invalid
 
   Scenario: I create a branch with invalid reference
     Given I visit project branches page
     And I click new branch link
-    When I submit new branch form with invalid reference
+    And I submit new branch form with invalid reference
     Then I should see new an error that ref is invalid
 
   Scenario: I create a branch that already exists
     Given I visit project branches page
     And I click new branch link
-    When I submit new branch form with branch that already exists
+    And I submit new branch form with branch that already exists
     Then I should see new an error that branch already exists
diff --git a/features/project/commits/tags.feature b/features/project/commits/tags.feature
index 1ac0f8bfa45..36c7a6492ff 100644
--- a/features/project/commits/tags.feature
+++ b/features/project/commits/tags.feature
@@ -7,5 +7,25 @@ Feature: Project Browse tags
   Scenario: I can see all git tags
     Then I should see "Shop" all tags list
 
+  Scenario: I create a tag
+    And I click new tag link
+    And I submit new tag form
+    Then I should see new tag created
+
+  Scenario: I create a tag with invalid name
+    And I click new tag link
+    And I submit new tag form with invalid name
+    Then I should see new an error that tag is invalid
+
+  Scenario: I create a tag with invalid reference
+    And I click new tag link
+    And I submit new tag form with invalid reference
+    Then I should see new an error that tag ref is invalid
+
+  Scenario: I create a tag that already exists
+    And I click new tag link
+    And I submit new tag form with tag that already exists
+    Then I should see new an error that tag already exists
+
   # @wip
   # Scenario: I can download project by tag
diff --git a/features/steps/project/browse_tags.rb b/features/steps/project/browse_tags.rb
index 7c679911e00..64c0c284f6c 100644
--- a/features/steps/project/browse_tags.rb
+++ b/features/steps/project/browse_tags.rb
@@ -3,8 +3,52 @@ class ProjectBrowseTags < Spinach::FeatureSteps
   include SharedProject
   include SharedPaths
 
-  Then 'I should see "Shop" all tags list' do
+  step 'I should see "Shop" all tags list' do
     page.should have_content "Tags"
     page.should have_content "v1.0.0"
   end
+
+  step 'I click new tag link' do
+    click_link 'New tag'
+  end
+
+  step 'I submit new tag form' do
+    fill_in 'tag_name', with: 'v7.0'
+    fill_in 'ref', with: 'master'
+    click_button 'Create tag'
+  end
+
+  step 'I submit new tag form with invalid name' do
+    fill_in 'tag_name', with: 'v 1.0'
+    fill_in 'ref', with: 'master'
+    click_button 'Create tag'
+  end
+
+  step 'I submit new tag form with invalid reference' do
+    fill_in 'tag_name', with: 'foo'
+    fill_in 'ref', with: 'foo'
+    click_button 'Create tag'
+  end
+
+  step 'I submit new tag form with tag that already exists' do
+    fill_in 'tag_name', with: 'v1.0.0'
+    fill_in 'ref', with: 'master'
+    click_button 'Create tag'
+  end
+
+  step 'I should see new tag created' do
+    page.should have_content 'v7.0'
+  end
+
+  step 'I should see new an error that tag is invalid' do
+    page.should have_content 'Tag name invalid'
+  end
+
+  step 'I should see new an error that tag ref is invalid' do
+    page.should have_content 'Invalid reference name'
+  end
+
+  step 'I should see new an error that tag already exists' do
+    page.should have_content 'Tag already exists'
+  end
 end
diff --git a/lib/api/repositories.rb b/lib/api/repositories.rb
index 42068bb343d..a3773d2c593 100644
--- a/lib/api/repositories.rb
+++ b/lib/api/repositories.rb
@@ -36,10 +36,15 @@ module API
       #   POST /projects/:id/repository/tags
       post ':id/repository/tags' do
         authorize_push_project
-        @tag = CreateTagService.new.execute(user_project, params[:tag_name],
-                                            params[:ref], current_user)
-
-        present @tag, with: Entities::RepoObject, project: user_project
+        result = CreateTagService.new.execute(user_project, params[:tag_name],
+                                              params[:ref], current_user)
+        if result[:status] == :success
+          present result[:tag],
+                  with: Entities::RepoObject,
+                  project: user_project
+        else
+          render_api_error!(result[:message], 400)
+        end
       end
 
       # Get a project repository tree
diff --git a/spec/requests/api/repositories_spec.rb b/spec/requests/api/repositories_spec.rb
index f8603e11a04..ffcdbc4255e 100644
--- a/spec/requests/api/repositories_spec.rb
+++ b/spec/requests/api/repositories_spec.rb
@@ -25,20 +25,46 @@ describe API::API, api: true  do
   describe 'POST /projects/:id/repository/tags' do
     it 'should create a new tag' do
       post api("/projects/#{project.id}/repository/tags", user),
-           tag_name: 'v1.0.0',
+           tag_name: 'v2.0.0',
            ref: 'master'
-
       response.status.should == 201
-      json_response['name'].should == 'v1.0.0'
+      json_response['name'].should == 'v2.0.0'
     end
 
     it 'should deny for user without push access' do
       post api("/projects/#{project.id}/repository/tags", user2),
            tag_name: 'v1.0.0',
            ref: '621491c677087aa243f165eab467bfdfbee00be1'
-
       response.status.should == 403
     end
+
+    it 'should return 400 if tag name is invalid' do
+      post api("/projects/#{project.id}/repository/tags", user),
+           tag_name: 'v 1.0.0',
+           ref: 'master'
+      response.status.should == 400
+      json_response['message'].should == 'Tag name invalid'
+    end
+
+    it 'should return 400 if tag already exists' do
+      post api("/projects/#{project.id}/repository/tags", user),
+           tag_name: 'v8.0.0',
+           ref: 'master'
+      response.status.should == 201
+      post api("/projects/#{project.id}/repository/tags", user),
+           tag_name: 'v8.0.0',
+           ref: 'master'
+      response.status.should == 400
+      json_response['message'].should == 'Tag already exists'
+    end
+
+    it 'should return 400 if ref name is invalid' do
+      post api("/projects/#{project.id}/repository/tags", user),
+           tag_name: 'mytag',
+           ref: 'foo'
+      response.status.should == 400
+      json_response['message'].should == 'Invalid reference name'
+    end
   end
 
   describe "GET /projects/:id/repository/tree" do