Merge branch 'rc/fix-branches-api-endpoint' into 'master'

Fix the `/projects/:id/repository/branches endpoint` to handle dots in the branch name when the project full patch contains a `/`

See merge request !13115

changelogs/unreleased/rc-fix-branches-api-endpoint.yml

+---
+title: Fix the /projects/:id/repository/branches endpoint to handle dots in the branch
+  name when the project full patch contains a `/`
+merge_request: 13115
+author:

doc/api/README.md

@@ -340,7 +340,18 @@ URL-encoded.
 For example, `/` is represented by `%2F`:
 
 ```
-/api/v4/projects/diaspora%2Fdiaspora
+GET /api/v4/projects/diaspora%2Fdiaspora
+```
+
+## Branches & tags name encoding
+
+If your branch or tag contains a `/`, make sure the branch/tag name is
+URL-encoded.
+
+For example, `/` is represented by `%2F`:
+
+```
+GET /api/v4/projects/1/branches/my%2Fbranch/commits
 ```
 
 ## `id` vs `iid`

lib/api/api.rb

@@ -86,6 +86,9 @@ module API
     helpers ::API::Helpers
     helpers ::API::Helpers::CommonHelpers
 
+    NO_SLASH_URL_PART_REGEX = %r{[^/]+}
+    PROJECT_ENDPOINT_REQUIREMENTS = { id: NO_SLASH_URL_PART_REGEX }.freeze
+
     # Keep in alphabetical order
     mount ::API::AccessRequests
     mount ::API::AwardEmoji

lib/api/branches.rb

@@ -4,19 +4,21 @@ module API
   class Branches < Grape::API
     include PaginationParams
 
+    BRANCH_ENDPOINT_REQUIREMENTS = API::PROJECT_ENDPOINT_REQUIREMENTS.merge(branch: API::NO_SLASH_URL_PART_REGEX)
+
     before { authorize! :download_code, user_project }
 
     params do
       requires :id, type: String, desc: 'The ID of a project'
     end
-    resource :projects, requirements: { id: %r{[^/]+} } do
+    resource :projects, requirements: API::PROJECT_ENDPOINT_REQUIREMENTS do
       desc 'Get a project repository branches' do
         success Entities::RepoBranch
       end
       params do
         use :pagination
       end
-      get ":id/repository/branches" do
+      get ':id/repository/branches' do
         branches = ::Kaminari.paginate_array(user_project.repository.branches.sort_by(&:name))
 
         present paginate(branches), with: Entities::RepoBranch, project: user_project
@@ -28,7 +30,7 @@ module API
       params do
         requires :branch, type: String, desc: 'The name of the branch'
       end
-      get ':id/repository/branches/:branch', requirements: { branch: /.+/ } do
+      get ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         branch = user_project.repository.find_branch(params[:branch])
         not_found!("Branch") unless branch
 
@@ -46,7 +48,7 @@ module API
         optional :developers_can_push, type: Boolean, desc: 'Flag if developers can push to that branch'
         optional :developers_can_merge, type: Boolean, desc: 'Flag if developers can merge to that branch'
       end
-      put ':id/repository/branches/:branch/protect', requirements: { branch: /.+/ } do
+      put ':id/repository/branches/:branch/protect', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_admin_project
 
         branch = user_project.repository.find_branch(params[:branch])
@@ -81,7 +83,7 @@ module API
       params do
         requires :branch, type: String, desc: 'The name of the branch'
       end
-      put ':id/repository/branches/:branch/unprotect', requirements: { branch: /.+/ } do
+      put ':id/repository/branches/:branch/unprotect', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_admin_project
 
         branch = user_project.repository.find_branch(params[:branch])
@@ -99,7 +101,7 @@ module API
         requires :branch, type: String, desc: 'The name of the branch'
         requires :ref, type: String, desc: 'Create branch from commit sha or existing branch'
       end
-      post ":id/repository/branches" do
+      post ':id/repository/branches' do
         authorize_push_project
 
         result = CreateBranchService.new(user_project, current_user)
@@ -118,7 +120,7 @@ module API
       params do
         requires :branch, type: String, desc: 'The name of the branch'
       end
-      delete ":id/repository/branches/:branch", requirements: { branch: /.+/ } do
+      delete ':id/repository/branches/:branch', requirements: BRANCH_ENDPOINT_REQUIREMENTS do
         authorize_push_project
 
         result = DeleteBranchService.new(user_project, current_user)
@@ -130,7 +132,7 @@ module API
       end
 
       desc 'Delete all merged branches'
-      delete ":id/repository/merged_branches" do
+      delete ':id/repository/merged_branches' do
         DeleteMergedBranchesService.new(user_project, current_user).async_execute
 
         accepted!

spec/fixtures/api/schemas/public_api/v4/branch.json

+{
+  "type": "object",
+  "required" : [
+    "name",
+    "commit",
+    "merged",
+    "protected",
+    "developers_can_push",
+    "developers_can_merge"
+  ],
+  "properties" : {
+    "name": { "type": "string" },
+    "commit": { "$ref": "commit/basic.json" },
+    "merged": { "type": "boolean" },
+    "protected": { "type": "boolean" },
+    "developers_can_push": { "type": "boolean" },
+    "developers_can_merge": { "type": "boolean" }
+  },
+  "additionalProperties": false
+}

spec/fixtures/api/schemas/public_api/v4/branches.json

+{
+  "type": "array",
+  "items": { "$ref": "branch.json" }
+}

spec/fixtures/api/schemas/public_api/v4/commit/basic.json

+{
+  "type": "object",
+  "required" : [
+    "id",
+    "short_id",
+    "title",
+    "created_at",
+    "parent_ids",
+    "message",
+    "author_name",
+    "author_email",
+    "authored_date",
+    "committer_name",
+    "committer_email",
+    "committed_date"
+  ],
+  "properties" : {
+    "id": { "type": ["string", "null"] },
+    "short_id": { "type": ["string", "null"] },
+    "title": { "type": "string" },
+    "created_at": { "type": "date" },
+    "parent_ids": {
+      "type": ["array", "null"],
+      "items": {
+        "type": "string",
+        "additionalProperties": false
+      }
+    },
+    "message": { "type": "string" },
+    "author_name": { "type": "string" },
+    "author_email": { "type": "string" },
+    "authored_date": { "type": "date" },
+    "committer_name": { "type": "string" },
+    "committer_email": { "type": "string" },
+    "committed_date": { "type": "date" }
+  }
+}

spec/requests/api/groups_spec.rb

@@ -510,7 +510,7 @@ describe API::Groups do
 
   describe "POST /groups/:id/projects/:project_id" do
     let(:project) { create(:empty_project) }
-    let(:project_path) { project.full_path.gsub('/', '%2F') }
+    let(:project_path) { CGI.escape(project.full_path) }
 
     before(:each) do
       allow_any_instance_of(Projects::TransferService)

spec/requests/api/projects_spec.rb

@@ -768,7 +768,7 @@ describe API::Projects do
         dot_user = create(:user, username: 'dot.user')
         project = create(:empty_project, creator_id: dot_user.id, namespace: dot_user.namespace)
 
-        get api("/projects/#{dot_user.namespace.name}%2F#{project.path}", dot_user)
+        get api("/projects/#{CGI.escape(project.full_path)}", dot_user)
         expect(response).to have_http_status(200)
         expect(json_response['name']).to eq(project.name)
       end

spec/requests/api/v3/groups_spec.rb

@@ -502,7 +502,7 @@ describe API::V3::Groups do
 
   describe "POST /groups/:id/projects/:project_id" do
     let(:project) { create(:empty_project) }
-    let(:project_path) { "#{project.namespace.path}%2F#{project.path}" }
+    let(:project_path) { CGI.escape(project.full_path) }
 
     before(:each) do
       allow_any_instance_of(Projects::TransferService)

spec/requests/api/v3/projects_spec.rb

@@ -720,7 +720,7 @@ describe API::V3::Projects do
         dot_user = create(:user, username: 'dot.user')
         project = create(:empty_project, creator_id: dot_user.id, namespace: dot_user.namespace)
 
-        get v3_api("/projects/#{dot_user.namespace.name}%2F#{project.path}", dot_user)
+        get v3_api("/projects/#{CGI.escape(project.full_path)}", dot_user)
         expect(response).to have_http_status(200)
         expect(json_response['name']).to eq(project.name)
       end

spec/support/api/schema_matcher.rb

@@ -5,7 +5,14 @@ end
 
 RSpec::Matchers.define :match_response_schema do |schema, **options|
   match do |response|
-    JSON::Validator.validate!(schema_path(schema), response.body, options)
+    @errors = JSON::Validator.fully_validate(schema_path(schema), response.body, options)
+
+    @errors.empty?
+  end
+
+  failure_message do |response|
+    "didn't match the schema defined by #{schema_path(schema)}" \
+    " The validation errors were:\n#{@errors.join("\n")}"
   end
 end
 

spec/support/api/status_shared_examples.rb → spec/support/shared_examples/requests/api/status_shared_examples.rb

@@ -9,7 +9,7 @@ shared_examples_for '400 response' do
   end
 
   it 'returns 400' do
-    expect(response).to have_http_status(400)
+    expect(response).to have_gitlab_http_status(400)
   end
 end
 
@@ -20,7 +20,7 @@ shared_examples_for '403 response' do
   end
 
   it 'returns 403' do
-    expect(response).to have_http_status(403)
+    expect(response).to have_gitlab_http_status(403)
   end
 end
 
@@ -32,7 +32,7 @@ shared_examples_for '404 response' do
   end
 
   it 'returns 404' do
-    expect(response).to have_http_status(404)
+    expect(response).to have_gitlab_http_status(404)
     expect(json_response).to be_an Object
 
     if message.present?