Merge remote-tracking branch 'dev/8-7-stable' into 8-7-stable

CHANGELOG

@@ -6,11 +6,22 @@ v 8.7.2
   - The "New Branch" button is now loaded asynchronously
 
 v 8.7.1
+v 8.7.1 (unreleased)
+  - Prevent privilege escalation via "impersonate" feature
+  - Prevent privilege escalation via notes API
+  - Prevent privilege escalation via project webhook API
+  - Prevent XSS via Git branch and tag names
+  - Prevent XSS via custom issue tracker URL
+  - Prevent XSS via `window.opener`
+  - Prevent XSS via label drop-down
+  - Prevent information disclosure via milestone API
+  - Prevent information disclosure via snippet API
+  - Prevent information disclosure via project labels
+  - Prevent information disclosure via new merge request page
   - Use the `can?` helper instead of `current_user.can?`
   - Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition. !3849
   - Fix license detection to detect all license files, not only known licenses. !3878
   - Use the `can?` helper instead of `current_user.can?`. !3882
-  - Prevent users from deleting Webhooks via API they do not own
   - Fix Error 500 due to stale cache when projects are renamed or transferred
   - Fix .gitlab-ci.yml parsing issue when hidde job is a template without script definition
   - Update width of search box to fix Safari bug. !3900 (Jedidiah)

VERSION

-8.7.0
+8.7.1
\ No newline at end of file

app/assets/javascripts/commits.js.coffee

 class @CommitsList
   @timer = null
 
-  @init: (ref, limit) ->
+  @init: (limit) ->
     $("body").on "click", ".day-commits-table li.commit", (event) ->
       if event.target.nodeName != "A"
         location.href = $(this).attr("url")

app/controllers/admin/application_controller.rb

@@ -6,12 +6,6 @@ class Admin::ApplicationController < ApplicationController
   layout 'admin'
 
   def authenticate_admin!
-    return render_404 unless current_user.is_admin?
-  end
-
-  def authorize_impersonator!
-    if session[:impersonator_id]
-      User.find_by!(username: session[:impersonator_id]).admin?
-    end
+    render_404 unless current_user.is_admin?
   end
 end

app/controllers/admin/impersonation_controller.rb

-class Admin::ImpersonationController < Admin::ApplicationController
-  skip_before_action :authenticate_admin!, only: :destroy
-
-  before_action :user
-  before_action :authorize_impersonator!
-
-  def create
-    if @user.blocked?
-      flash[:alert] = "You cannot impersonate a blocked user"
-
-      redirect_to admin_user_path(@user)
-    else
-      session[:impersonator_id] = current_user.username
-      session[:impersonator_return_to] = admin_user_path(@user)
-
-      warden.set_user(user, scope: 'user')
-
-      flash[:alert] = "You are impersonating #{user.username}."
-
-      redirect_to root_path
-    end
-  end
-
-  def destroy
-    redirect = session[:impersonator_return_to]
-
-    warden.set_user(user, scope: 'user')
-
-    session[:impersonator_return_to] = nil
-    session[:impersonator_id] = nil
-
-    redirect_to redirect || root_path
-  end
-
-  def user
-    @user ||= User.find_by!(username: params[:id] || session[:impersonator_id])
-  end
-end

app/controllers/admin/impersonations_controller.rb

+class Admin::ImpersonationsController < Admin::ApplicationController
+  skip_before_action :authenticate_admin!
+  before_action :authenticate_impersonator!
+
+  def destroy
+    original_user = current_user
+
+    warden.set_user(impersonator, scope: :user)
+
+    session[:impersonator_id] = nil
+
+    redirect_to admin_user_path(original_user)
+  end
+
+  private
+
+  def impersonator
+    @impersonator ||= User.find(session[:impersonator_id]) if session[:impersonator_id]
+  end
+
+  def authenticate_impersonator!
+    render_404 unless impersonator && impersonator.is_admin? && !impersonator.blocked?
+  end
+end

app/controllers/admin/users_controller.rb

@@ -31,6 +31,22 @@ class Admin::UsersController < Admin::ApplicationController
     user
   end
 
+  def impersonate
+    if user.blocked?
+      flash[:alert] = "You cannot impersonate a blocked user"
+
+      redirect_to admin_user_path(user)
+    else
+      session[:impersonator_id] = current_user.id
+
+      warden.set_user(user, scope: :user)
+
+      flash[:alert] = "You are now impersonating #{user.username}"
+
+      redirect_to root_path
+    end
+  end
+
   def block
     if user.block
       redirect_back_or_admin_user(notice: "Successfully blocked")

app/finders/snippets_finder.rb

@@ -51,7 +51,7 @@ class SnippetsFinder
     snippets = project.snippets.fresh
 
     if current_user
-      if project.team.member?(current_user.id)
+      if project.team.member?(current_user.id) || current_user.admin?
         snippets
       else
         snippets.public_and_internal

app/helpers/issues_helper.rb

@@ -16,31 +16,49 @@ module IssuesHelper
   def url_for_project_issues(project = @project, options = {})
     return '' if project.nil?
 
-    if options[:only_path]
-      project.issues_tracker.project_path
-    else
-      project.issues_tracker.project_url
-    end
+    url =
+      if options[:only_path]
+        project.issues_tracker.project_path
+      else
+        project.issues_tracker.project_url
+      end
+
+    # Ensure we return a valid URL to prevent possible XSS.
+    URI.parse(url).to_s
+  rescue URI::InvalidURIError
+    ''
   end
 
   def url_for_new_issue(project = @project, options = {})
     return '' if project.nil?
 
-    if options[:only_path]
-      project.issues_tracker.new_issue_path
-    else
-      project.issues_tracker.new_issue_url
-    end
+    url =
+      if options[:only_path]
+        project.issues_tracker.new_issue_path
+      else
+        project.issues_tracker.new_issue_url
+      end
+
+    # Ensure we return a valid URL to prevent possible XSS.
+    URI.parse(url).to_s
+  rescue URI::InvalidURIError
+    ''
   end
 
   def url_for_issue(issue_iid, project = @project, options = {})
     return '' if project.nil?
 
-    if options[:only_path]
-      project.issues_tracker.issue_path(issue_iid)
-    else
-      project.issues_tracker.issue_url(issue_iid)
-    end
+    url =
+      if options[:only_path]
+        project.issues_tracker.issue_path(issue_iid)
+      else
+        project.issues_tracker.issue_url(issue_iid)
+      end
+
+    # Ensure we return a valid URL to prevent possible XSS.
+    URI.parse(url).to_s
+  rescue URI::InvalidURIError
+    ''
   end
 
   def bulk_update_milestone_options

app/models/project_services/buildkite_service.rb

@@ -26,7 +26,7 @@ class BuildkiteService < CiService
 
   prop_accessor :project_url, :token, :enable_ssl_verification
 
-  validates :project_url, presence: true, if: :activated?
+  validates :project_url, presence: true, url: true, if: :activated?
   validates :token, presence: true, if: :activated?
 
   after_save :compose_service_hook, if: :activated?
@@ -91,7 +91,7 @@ class BuildkiteService < CiService
       { type: 'text',
         name: 'project_url',
         placeholder: "#{ENDPOINT}/example/project" },
-      
+
       { type: 'checkbox',
         name: 'enable_ssl_verification',
         title: "Enable SSL verification" }

app/models/project_services/issue_tracker_service.rb

@@ -21,7 +21,7 @@
 
 class IssueTrackerService < Service
 
-  validates :project_url, :issues_url, :new_issue_url, presence: true, if: :activated?
+  validates :project_url, :issues_url, :new_issue_url, presence: true, url: true, if: :activated?
 
   default_value_for :category, 'issue_tracker'
 

app/models/project_services/jira_service.rb

@@ -28,6 +28,8 @@ class JiraService < IssueTrackerService
   prop_accessor :username, :password, :api_url, :jira_issue_transition_id,
                 :title, :description, :project_url, :issues_url, :new_issue_url
 
+  validates :api_url, presence: true, url: true, if: :activated?
+
   before_validation :set_api_url, :set_jira_issue_transition_id
 
   before_update :reset_password

app/models/project_services/slack_service.rb

@@ -22,7 +22,7 @@
 class SlackService < Service
   prop_accessor :webhook, :username, :channel
   boolean_accessor :notify_only_broken_builds
-  validates :webhook, presence: true, if: :activated?
+  validates :webhook, presence: true, url: true, if: :activated?
 
   def initialize_properties
     if properties.nil?

app/services/merge_requests/build_service.rb

@@ -7,6 +7,9 @@ module MergeRequests
       merge_request.can_be_created = false
       merge_request.compare_commits = []
       merge_request.source_project = project unless merge_request.source_project
+
+      merge_request.target_project = nil unless can?(current_user, :read_project, merge_request.target_project)
+
       merge_request.target_project ||= (project.forked_from_project || project)
       merge_request.target_branch ||= merge_request.target_project.default_branch
 

app/services/notes/create_service.rb

@@ -5,6 +5,8 @@ module Notes
       note.author = current_user
       note.system = false
 
+      return unless valid_project?(note)
+
       if note.save
         # Finish the harder work in the background
         NewNoteWorker.perform_in(2.seconds, note.id, params)
@@ -13,5 +15,14 @@ module Notes
 
       note
     end
+
+    private
+
+    def valid_project?(note)
+      return false unless project
+      return true if note.for_commit?
+
+      note.noteable.try(:project) == project
+    end
   end
 end

app/views/layouts/header/_default.html.haml

@@ -15,7 +15,7 @@
           - if current_user
             - if session[:impersonator_id]
               %li.impersonation
-                = link_to stop_impersonation_admin_users_path, method: :delete, title: 'Stop Impersonation', data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
+                = link_to admin_impersonation_path, method: :delete, title: 'Stop Impersonation', data: { toggle: 'tooltip', placement: 'bottom', container: 'body' } do
                   = icon('user-secret fw')
             - if current_user.is_admin?
               %li

app/views/projects/commits/show.html.haml

@@ -39,4 +39,4 @@
 = spinner
 
 :javascript
-  CommitsList.init("#{@ref}", #{@limit});
+  CommitsList.init(#{@limit});

config/routes.rb

@@ -212,8 +212,6 @@ Rails.application.routes.draw do
       resources :keys, only: [:show, :destroy]
       resources :identities, except: [:show]
 
-      delete 'stop_impersonation' => 'impersonation#destroy', on: :collection
-
       member do
         get :projects
         get :keys
@@ -223,12 +221,14 @@ Rails.application.routes.draw do
         put :unblock
         put :unlock
         put :confirm
-        post 'impersonate' => 'impersonation#create'
+        post :impersonate
         patch :disable_two_factor
         delete 'remove/:email_id', action: 'remove_email', as: 'remove_email'
       end
     end
 
+    resource :impersonation, only: :destroy
+
     resources :abuse_reports, only: [:index, :destroy]
     resources :spam_logs, only: [:index, :destroy]
 

lib/api/milestones.rb

@@ -105,7 +105,15 @@ module API
         authorize! :read_milestone, user_project
 
         @milestone = user_project.milestones.find(params[:milestone_id])
-        present paginate(@milestone.issues), with: Entities::Issue, current_user: current_user
+
+        finder_params = {
+          project_id: user_project.id,
+          milestone_title: @milestone.title,
+          state: 'all'
+        }
+
+        issues = IssuesFinder.new(current_user, finder_params).execute
+        present paginate(issues), with: Entities::Issue, current_user: current_user
       end
 
     end

lib/api/project_hooks.rb

@@ -103,10 +103,10 @@ module API
         required_attributes! [:hook_id]
 
         begin
-          @hook = ProjectHook.find(params[:hook_id])
-          @hook.destroy
+          @hook = user_project.hooks.destroy(params[:hook_id])
         rescue
           # ProjectHook can raise Error if hook_id not found
+          not_found!("Error deleting hook #{params[:hook_id]}")
         end
       end
     end

lib/api/project_snippets.rb

@@ -11,6 +11,11 @@ module API
           end
           not_found!
         end
+
+        def snippets_for_current_user
+          finder_params = { filter: :by_project, project: user_project }
+          SnippetsFinder.new.execute(current_user, finder_params)
+        end
       end
 
       # Get a project snippets
@@ -20,7 +25,7 @@ module API
       # Example Request:
       #   GET /projects/:id/snippets
       get ":id/snippets" do
-        present paginate(user_project.snippets), with: Entities::ProjectSnippet
+        present paginate(snippets_for_current_user), with: Entities::ProjectSnippet
       end
 
       # Get a project snippet
@@ -31,7 +36,7 @@ module API
       # Example Request:
       #   GET /projects/:id/snippets/:snippet_id
       get ":id/snippets/:snippet_id" do
-        @snippet = user_project.snippets.find(params[:snippet_id])
+        @snippet = snippets_for_current_user.find(params[:snippet_id])
         present @snippet, with: Entities::ProjectSnippet
       end
 
@@ -73,7 +78,7 @@ module API
       # Example Request:
       #   PUT /projects/:id/snippets/:snippet_id
       put ":id/snippets/:snippet_id" do
-        @snippet = user_project.snippets.find(params[:snippet_id])
+        @snippet = snippets_for_current_user.find(params[:snippet_id])
         authorize! :update_project_snippet, @snippet
 
         attrs = attributes_for_keys [:title, :file_name, :visibility_level]
@@ -97,7 +102,7 @@ module API
       #   DELETE /projects/:id/snippets/:snippet_id
       delete ":id/snippets/:snippet_id" do
         begin
-          @snippet = user_project.snippets.find(params[:snippet_id])
+          @snippet = snippets_for_current_user.find(params[:snippet_id])
           authorize! :update_project_snippet, @snippet
           @snippet.destroy
         rescue
@@ -113,7 +118,7 @@ module API
       # Example Request:
       #   GET /projects/:id/snippets/:snippet_id/raw
       get ":id/snippets/:snippet_id/raw" do
-        @snippet = user_project.snippets.find(params[:snippet_id])
+        @snippet = snippets_for_current_user.find(params[:snippet_id])
 
         env['api.format'] = :txt
         content_type 'text/plain'

lib/banzai/filter/external_link_filter.rb

 module Banzai
   module Filter
-    # HTML Filter to add a `rel="nofollow"` attribute to external links
-    #
+    # HTML Filter to modify the attributes of external links
     class ExternalLinkFilter < HTML::Pipeline::Filter
       def call
         doc.search('a').each do |node|
@@ -15,7 +14,7 @@ module Banzai
           # Skip internal links
           next if link.start_with?(internal_url)
 
-          node.set_attribute('rel', 'nofollow')
+          node.set_attribute('rel', 'nofollow noreferrer')
         end
 
         doc

spec/controllers/admin/impersonation_controller_spec.rb

-require 'spec_helper'
-
-describe Admin::ImpersonationController do
-  let(:admin) { create(:admin) }
-
-  before do
-    sign_in(admin)
-  end
-
-  describe 'CREATE #impersonation when blocked' do
-    let(:blocked_user) { create(:user, state: :blocked) }
-
-    it 'does not allow impersonation' do
-      post :create, id: blocked_user.username
-
-      expect(flash[:alert]).to eq 'You cannot impersonate a blocked user'
-    end
-  end
-end

spec/controllers/admin/impersonations_controller_spec.rb

+require 'spec_helper'
+
+describe Admin::ImpersonationsController do
+  let(:impersonator) { create(:admin) }
+  let(:user) { create(:user) }
+
+  describe "DELETE destroy" do
+    context "when not signed in" do
+      it "redirects to the sign in page" do
+        delete :destroy
+
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+
+    context "when signed in" do
+      before do
+        sign_in(user)
+      end
+
+      context "when not impersonating" do
+        it "responds with status 404" do
+          delete :destroy
+
+          expect(response.status).to eq(404)
+        end
+
+        it "doesn't sign us in" do
+          delete :destroy
+
+          expect(warden.user).to eq(user)
+        end
+      end
+
+      context "when impersonating" do
+        before do
+          session[:impersonator_id] = impersonator.id
+        end
+
+        context "when the impersonator is not admin (anymore)" do
+          before do
+            impersonator.admin = false
+            impersonator.save
+          end
+
+          it "responds with status 404" do
+            delete :destroy
+
+            expect(response.status).to eq(404)
+          end
+
+          it "doesn't sign us in as the impersonator" do
+            delete :destroy
+
+            expect(warden.user).to eq(user)
+          end
+        end
+
+        context "when the impersonator is admin" do
+          context "when the impersonator is blocked" do
+            before do
+              impersonator.block!
+            end
+
+            it "responds with status 404" do
+              delete :destroy
+
+              expect(response.status).to eq(404)
+            end
+
+            it "doesn't sign us in as the impersonator" do
+              delete :destroy
+
+              expect(warden.user).to eq(user)
+            end
+          end
+
+          context "when the impersonator is not blocked" do
+            it "redirects to the impersonated user's page" do
+              delete :destroy
+
+              expect(response).to redirect_to(admin_user_path(user))
+            end
+
+            it "signs us in as the impersonator" do
+              delete :destroy
+
+              expect(warden.user).to eq(impersonator)
+            end
+          end
+        end
+      end
+    end
+  end
+end

spec/controllers/admin/users_controller_spec.rb

@@ -2,9 +2,10 @@ require 'spec_helper'
 
 describe Admin::UsersController do
   let(:user) { create(:user) }
+  let(:admin) { create(:admin) }
 
   before do
-    sign_in(create(:admin))
+    sign_in(admin)
   end
 
   describe 'DELETE #user with projects' do
@@ -112,4 +113,50 @@ describe Admin::UsersController do
       patch :disable_two_factor, id: user.to_param
     end
   end
+
+  describe "POST impersonate" do
+    context "when the user is blocked" do
+      before do
+        user.block!
+      end
+
+      it "shows a notice" do
+        post :impersonate, id: user.username
+
+        expect(flash[:alert]).to eq("You cannot impersonate a blocked user")
+      end
+
+      it "doesn't sign us in as the user" do
+        post :impersonate, id: user.username
+
+        expect(warden.user).to eq(admin)
+      end
+    end
+
+    context "when the user is not blocked" do
+      it "stores the impersonator in the session" do
+        post :impersonate, id: user.username
+
+        expect(session[:impersonator_id]).to eq(admin.id)
+      end
+
+      it "signs us in as the user" do
+        post :impersonate, id: user.username
+
+        expect(warden.user).to eq(user)
+      end
+
+      it "redirects to root" do
+        post :impersonate, id: user.username
+
+        expect(response).to redirect_to(root_path)
+      end
+
+      it "shows a notice" do
+        post :impersonate, id: user.username
+
+        expect(flash[:alert]).to eq("You are now impersonating #{user.username}")
+      end
+    end
+  end
 end

spec/features/markdown_spec.rb

@@ -165,7 +165,12 @@ describe 'GitLab Markdown', feature: true do
     describe 'ExternalLinkFilter' do
       it 'adds nofollow to external link' do
         link = doc.at_css('a:contains("Google")')
-        expect(link.attr('rel')).to match 'nofollow'
+        expect(link.attr('rel')).to include('nofollow')
+      end
+
+      it 'adds noreferrer to external link' do
+        link = doc.at_css('a:contains("Google")')
+        expect(link.attr('rel')).to include('noreferrer')
       end
 
       it 'ignores internal link' do

spec/features/merge_requests/create_new_mr_spec.rb

@@ -30,4 +30,14 @@ feature 'Create New Merge Request', feature: true, js: true do
 
     expect(page).to have_content 'git checkout -b orphaned-branch origin/orphaned-branch'
   end
+
+  context 'when target project cannot be viewed by the current user' do
+    it 'does not leak the private project name & namespace' do
+      private_project = create(:project, :private)
+
+      visit new_namespace_project_merge_request_path(project.namespace, project, merge_request: { target_project_id: private_project.id })
+
+      expect(page).not_to have_content private_project.to_reference
+    end
+  end
 end

spec/helpers/issues_helper_spec.rb

@@ -30,6 +30,18 @@ describe IssuesHelper do
       expect(url_for_project_issues).to eq ""
     end
 
+    it 'returns an empty string if project_url is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.project_url') { 'javascript:alert("foo");' }
+
+      expect(url_for_project_issues(project)).to eq ''
+    end
+
+    it 'returns an empty string if project_path is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.project_path') { 'javascript:alert("foo");' }
+
+      expect(url_for_project_issues(project, only_path: true)).to eq ''
+    end
+
     describe "when external tracker was enabled and then config removed" do
       before do
         @project = ext_project
@@ -68,6 +80,18 @@ describe IssuesHelper do
       expect(url_for_issue(issue.iid)).to eq ""
     end
 
+    it 'returns an empty string if issue_url is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.issue_url') { 'javascript:alert("foo");' }
+
+      expect(url_for_issue(issue.iid, project)).to eq ''
+    end
+
+    it 'returns an empty string if issue_path is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.issue_path') { 'javascript:alert("foo");' }
+
+      expect(url_for_issue(issue.iid, project, only_path: true)).to eq ''
+    end
+
     describe "when external tracker was enabled and then config removed" do
       before do
         @project = ext_project
@@ -105,6 +129,18 @@ describe IssuesHelper do
       expect(url_for_new_issue).to eq ""
     end
 
+    it 'returns an empty string if issue_url is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.new_issue_url') { 'javascript:alert("foo");' }
+
+      expect(url_for_new_issue(project)).to eq ''
+    end
+
+    it 'returns an empty string if issue_path is invalid' do
+      expect(project).to receive_message_chain('issues_tracker.new_issue_path') { 'javascript:alert("foo");' }
+
+      expect(url_for_new_issue(project, only_path: true)).to eq ''
+    end
+
     describe "when external tracker was enabled and then config removed" do
       before do
         @project = ext_project

spec/lib/banzai/filter/external_link_filter_spec.rb

@@ -24,6 +24,14 @@ describe Banzai::Filter::ExternalLinkFilter, lib: true do
     doc = filter(act)
 
     expect(doc.at_css('a')).to have_attribute('rel')
-    expect(doc.at_css('a')['rel']).to eq 'nofollow'
+    expect(doc.at_css('a')['rel']).to include 'nofollow'
+  end
+
+  it 'adds rel="noreferrer" to external links' do
+    act = %q(<a href="https://google.com/">Google</a>)
+    doc = filter(act)
+
+    expect(doc.at_css('a')).to have_attribute('rel')
+    expect(doc.at_css('a')['rel']).to include 'noreferrer'
   end
 end

spec/models/project_services/bamboo_service_spec.rb

@@ -27,86 +27,51 @@ describe BambooService, models: true do
   end
 
   describe 'Validations' do
-    describe '#bamboo_url' do
-      it 'does not validate the presence of bamboo_url if service is not active' do
-        bamboo_service = service
-        bamboo_service.active = false
-
-        expect(bamboo_service).not_to validate_presence_of(:bamboo_url)
-      end
-
-      it 'validates the presence of bamboo_url if service is active' do
-        bamboo_service = service
-        bamboo_service.active = true
-
-        expect(bamboo_service).to validate_presence_of(:bamboo_url)
-      end
-    end
+    subject { service }
 
-    describe '#build_key' do
-      it 'does not validate the presence of build_key if service is not active' do
-        bamboo_service = service
-        bamboo_service.active = false
+    context 'when service is active' do
+      before { subject.active = true }
 
-        expect(bamboo_service).not_to validate_presence_of(:build_key)
-      end
+      it { is_expected.to validate_presence_of(:build_key) }
+      it { is_expected.to validate_presence_of(:bamboo_url) }
+      it_behaves_like 'issue tracker service URL attribute', :bamboo_url
 
-      it 'validates the presence of build_key if service is active' do
-        bamboo_service = service
-        bamboo_service.active = true
+      describe '#username' do
+        it 'does not validate the presence of username if password is nil' do
+          subject.password = nil
 
-        expect(bamboo_service).to validate_presence_of(:build_key)
-      end
-    end
+          expect(subject).not_to validate_presence_of(:username)
+        end
 
-    describe '#username' do
-      it 'does not validate the presence of username if service is not active' do
-        bamboo_service = service
-        bamboo_service.active = false
+        it 'validates the presence of username if password is present' do
+          subject.password = 'secret'
 
-        expect(bamboo_service).not_to validate_presence_of(:username)
+          expect(subject).to validate_presence_of(:username)
+        end
       end
 
-      it 'does not validate the presence of username if username is nil' do
-        bamboo_service = service
-        bamboo_service.active = true
-        bamboo_service.password = nil
+      describe '#password' do
+        it 'does not validate the presence of password if username is nil' do
+          subject.username = nil
 
-        expect(bamboo_service).not_to validate_presence_of(:username)
-      end
+          expect(subject).not_to validate_presence_of(:password)
+        end
 
-      it 'validates the presence of username if service is active and username is present' do
-        bamboo_service = service
-        bamboo_service.active = true
-        bamboo_service.password = 'secret'
+        it 'validates the presence of password if username is present' do
+          subject.username = 'john'
 
-        expect(bamboo_service).to validate_presence_of(:username)
+          expect(subject).to validate_presence_of(:password)
+        end
       end
     end
 
-    describe '#password' do
-      it 'does not validate the presence of password if service is not active' do
-        bamboo_service = service
-        bamboo_service.active = false
-
-        expect(bamboo_service).not_to validate_presence_of(:password)
-      end
-
-      it 'does not validate the presence of password if username is nil' do
-        bamboo_service = service
-        bamboo_service.active = true
-        bamboo_service.username = nil
-
-        expect(bamboo_service).not_to validate_presence_of(:password)
-      end
-
-      it 'validates the presence of password if service is active and username is present' do
-        bamboo_service = service
-        bamboo_service.active = true
-        bamboo_service.username = 'john'
+    context 'when service is inactive' do
+      before { subject.active = false }
 
-        expect(bamboo_service).to validate_presence_of(:password)
-      end
+      it { is_expected.not_to validate_presence_of(:build_key) }
+      it { is_expected.not_to validate_presence_of(:bamboo_url) }
+      it { is_expected.not_to validate_presence_of(:username) }
+      it { is_expected.not_to validate_presence_of(:password) }
     end
   end
 

spec/models/project_services/buildkite_service_spec.rb

@@ -26,6 +26,23 @@ describe BuildkiteService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:token) }
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+
   describe 'commits methods' do
     before do
       @project = Project.new

spec/models/project_services/builds_email_service_spec.rb

 require 'spec_helper'
 
 describe BuildsEmailService do
-  let(:build) { create(:ci_build) }
-  let(:data) { Gitlab::BuildDataBuilder.build(build) }
-  let!(:project) { create(:project, :public, ci_id: 1) }
-  let(:service) { described_class.new(project: project, active: true) }
+  let(:data) { Gitlab::BuildDataBuilder.build(create(:ci_build)) }
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:recipients) }
+
+      context 'when pusher is added' do
+        before { subject.add_pusher = true }
+
+        it { is_expected.not_to validate_presence_of(:recipients) }
+      end
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:recipients) }
+    end
+  end
 
   describe '#execute' do
     it 'sends email' do
-      service.recipients = 'test@gitlab.com'
+      subject.recipients = 'test@gitlab.com'
       data[:build_status] = 'failed'
+
       expect(BuildEmailWorker).to receive(:perform_async)
-      service.execute(data)
+
+      subject.execute(data)
     end
 
     it 'does not send email with succeeded build and notify_only_broken_builds on' do
-      expect(service).to receive(:notify_only_broken_builds).and_return(true)
+      expect(subject).to receive(:notify_only_broken_builds).and_return(true)
       data[:build_status] = 'success'
+
       expect(BuildEmailWorker).not_to receive(:perform_async)
-      service.execute(data)
+
+      subject.execute(data)
     end
 
     it 'does not send email with failed build and build_allow_failure is true' do
       data[:build_status] = 'failed'
       data[:build_allow_failure] = true
+
       expect(BuildEmailWorker).not_to receive(:perform_async)
-      service.execute(data)
+
+      subject.execute(data)
     end
 
     it 'does not send email with unknown build status' do
       data[:build_status] = 'foo'
-      expect(BuildEmailWorker).not_to receive(:perform_async)
-      service.execute(data)
-    end
 
-    it 'does not send email when recipients list is empty' do
-      service.recipients = ' ,, '
-      data[:build_status] = 'failed'
       expect(BuildEmailWorker).not_to receive(:perform_async)
-      service.execute(data)
-    end
-  end
-
-  describe 'validations' do
-
-    context 'when pusher is not added' do
-      before { service.add_pusher = false }
-
-      it 'does not allow empty recipient input' do
-        service.recipients = ''
-        expect(service.valid?).to be false
-      end
-
-      it 'does allow non-empty recipient input' do
-        service.recipients = 'test@example.com'
-        expect(service.valid?).to be true
-      end
 
+      subject.execute(data)
     end
 
-    context 'when pusher is added' do
-      before { service.add_pusher = true }
+    it 'does not send email when recipients list is empty' do
+      subject.recipients = ' ,, '
+      data[:build_status] = 'failed'
 
-      it 'does allow empty recipient input' do
-        service.recipients = ''
-        expect(service.valid?).to be true
-      end
+      expect(BuildEmailWorker).not_to receive(:perform_async)
 
-      it 'does allow non-empty recipient input' do
-        service.recipients = 'test@example.com'
-        expect(service.valid?).to be true
-      end
+      subject.execute(data)
     end
   end
 end

spec/models/project_services/campfire_service_spec.rb

+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe CampfireService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+end

spec/models/project_services/custom_issue_tracker_service_spec.rb

+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe CustomIssueTrackerService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it { is_expected.to validate_presence_of(:new_issue_url) }
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+      it { is_expected.not_to validate_presence_of(:new_issue_url) }
+    end
+  end
+end

spec/models/project_services/drone_ci_service_spec.rb

@@ -28,25 +28,18 @@ describe DroneCiService, models: true do
 
   describe 'validations' do
     context 'active' do
-      before { allow(subject).to receive(:activated?).and_return(true) }
+      before { subject.active = true }
 
       it { is_expected.to validate_presence_of(:token) }
       it { is_expected.to validate_presence_of(:drone_url) }
-      it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) }
-      it { is_expected.to allow_value('http://ci.example.com').for(:drone_url) }
-      it { is_expected.not_to allow_value('this is not url').for(:drone_url) }
-      it { is_expected.not_to allow_value('http//noturl').for(:drone_url) }
-      it { is_expected.not_to allow_value('ftp://ci.example.com').for(:drone_url) }
+      it_behaves_like 'issue tracker service URL attribute', :drone_url
     end
 
     context 'inactive' do
-      before { allow(subject).to receive(:activated?).and_return(false) }
+      before { subject.active = false }
 
       it { is_expected.not_to validate_presence_of(:token) }
       it { is_expected.not_to validate_presence_of(:drone_url) }
-      it { is_expected.to allow_value('ewf9843kdnfdfs89234n').for(:token) }
-      it { is_expected.to allow_value('http://drone.example.com').for(:drone_url) }
-      it { is_expected.to allow_value('ftp://drone.example.com').for(:drone_url) }
     end
   end
 

spec/models/project_services/emails_on_push_service_spec.rb

+require 'spec_helper'
+
+describe EmailsOnPushService do
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:recipients) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:recipients) }
+    end
+  end
+end

spec/models/external_wiki_service_spec.rb → spec/models/project_services/external_wiki_service_spec.rb

@@ -28,13 +28,18 @@ describe ExternalWikiService, models: true do
     it { should have_one :service_hook }
   end
 
-  describe "Validations" do
-    context "active" do
-      before do
-        subject.active = true
-      end
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:external_wiki_url) }
+      it_behaves_like 'issue tracker service URL attribute', :external_wiki_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
 
-      it { should validate_presence_of :external_wiki_url }
+      it { is_expected.not_to validate_presence_of(:external_wiki_url) }
     end
   end
 

spec/models/project_services/flowdock_service_spec.rb

@@ -26,6 +26,20 @@ describe FlowdockService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }

spec/models/project_services/gemnasium_service_spec.rb

@@ -26,6 +26,22 @@ describe GemnasiumService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+      it { is_expected.to validate_presence_of(:api_key) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+      it { is_expected.not_to validate_presence_of(:api_key) }
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }

spec/models/project_services/gitlab_issue_tracker_service_spec.rb

@@ -26,6 +26,20 @@ describe GitlabIssueTrackerService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      subject { described_class.new(project: create(:project), active: true) }
+
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+    end
+
+    context 'when service is inactive' do
+      subject { described_class.new(project: create(:project), active: false) }
+
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+    end
+  end
 
   describe 'project and issue urls' do
     let(:project) { create(:project) }

spec/models/project_services/hipchat_service_spec.rb

@@ -26,6 +26,20 @@ describe HipchatService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+
   describe "Execute" do
     let(:hipchat) { HipchatService.new }
     let(:user)    { create(:user, username: 'username') }

spec/models/project_services/irker_service_spec.rb

@@ -29,14 +29,16 @@ describe IrkerService, models: true do
   end
 
   describe 'Validations' do
-    before do
-      subject.active = true
-      subject.properties['recipients'] = _recipients
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:recipients) }
     end
 
-    context 'active' do
-      let(:_recipients) { nil }
-      it { should validate_presence_of :recipients }
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:recipients) }
     end
   end
 

spec/models/project_services/jira_service_spec.rb

@@ -26,6 +26,30 @@ describe JiraService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:api_url) }
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it { is_expected.to validate_presence_of(:new_issue_url) }
+      it_behaves_like 'issue tracker service URL attribute', :api_url
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:api_url) }
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+      it { is_expected.not_to validate_presence_of(:new_issue_url) }
+    end
+  end
+
   describe "Execute" do
     let(:user)    { create(:user) }
     let(:project) { create(:project) }
@@ -72,7 +96,7 @@ describe JiraService, models: true do
 
     context "when a password was previously set" do
       before do
-        @jira_service = JiraService.create(
+        @jira_service = JiraService.create!(
           project: create(:project),
           properties: {
             api_url: 'http://jira.example.com/rest/api/2',

spec/models/project_services/pivotaltracker_service_spec.rb

+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe PivotaltrackerService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:token) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:token) }
+    end
+  end
+end

spec/models/project_services/pushover_service_spec.rb

@@ -27,14 +27,20 @@ describe PushoverService, models: true do
   end
 
   describe 'Validations' do
-    context 'active' do
-      before do
-        subject.active = true
-      end
+    context 'when service is active' do
+      before { subject.active = true }
 
-      it { is_expected.to validate_presence_of :api_key }
-      it { is_expected.to validate_presence_of :user_key }
-      it { is_expected.to validate_presence_of :priority }
+      it { is_expected.to validate_presence_of(:api_key) }
+      it { is_expected.to validate_presence_of(:user_key) }
+      it { is_expected.to validate_presence_of(:priority) }
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:api_key) }
+      it { is_expected.not_to validate_presence_of(:user_key) }
+      it { is_expected.not_to validate_presence_of(:priority) }
     end
   end
 

spec/models/project_services/redmine_service_spec.rb

+# == Schema Information
+#
+# Table name: services
+#
+#  id                    :integer          not null, primary key
+#  type                  :string(255)
+#  title                 :string(255)
+#  project_id            :integer
+#  created_at            :datetime
+#  updated_at            :datetime
+#  active                :boolean          default(FALSE), not null
+#  properties            :text
+#  template              :boolean          default(FALSE)
+#  push_events           :boolean          default(TRUE)
+#  issues_events         :boolean          default(TRUE)
+#  merge_requests_events :boolean          default(TRUE)
+#  tag_push_events       :boolean          default(TRUE)
+#  note_events           :boolean          default(TRUE), not null
+#
+
+require 'spec_helper'
+
+describe RedmineService, models: true do
+  describe 'Associations' do
+    it { is_expected.to belong_to :project }
+    it { is_expected.to have_one :service_hook }
+  end
+
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
+
+      it { is_expected.to validate_presence_of(:project_url) }
+      it { is_expected.to validate_presence_of(:issues_url) }
+      it { is_expected.to validate_presence_of(:new_issue_url) }
+      it_behaves_like 'issue tracker service URL attribute', :project_url
+      it_behaves_like 'issue tracker service URL attribute', :issues_url
+      it_behaves_like 'issue tracker service URL attribute', :new_issue_url
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:project_url) }
+      it { is_expected.not_to validate_presence_of(:issues_url) }
+      it { is_expected.not_to validate_presence_of(:new_issue_url) }
+    end
+  end
+end

spec/models/project_services/slack_service_spec.rb

@@ -26,13 +26,18 @@ describe SlackService, models: true do
     it { is_expected.to have_one :service_hook }
   end
 
-  describe "Validations" do
-    context "active" do
-      before do
-        subject.active = true
-      end
+  describe 'Validations' do
+    context 'when service is active' do
+      before { subject.active = true }
 
-      it { is_expected.to validate_presence_of :webhook }
+      it { is_expected.to validate_presence_of(:webhook) }
+      it_behaves_like 'issue tracker service URL attribute', :webhook
+    end
+
+    context 'when service is inactive' do
+      before { subject.active = false }
+
+      it { is_expected.not_to validate_presence_of(:webhook) }
     end
   end
 

spec/models/project_services/teamcity_service_spec.rb

@@ -27,86 +27,51 @@ describe TeamcityService, models: true do
   end
 
   describe 'Validations' do
-    describe '#teamcity_url' do
-      it 'does not validate the presence of teamcity_url if service is not active' do
-        teamcity_service = service
-        teamcity_service.active = false
-
-        expect(teamcity_service).not_to validate_presence_of(:teamcity_url)
-      end
-
-      it 'validates the presence of teamcity_url if service is active' do
-        teamcity_service = service
-        teamcity_service.active = true
-
-        expect(teamcity_service).to validate_presence_of(:teamcity_url)
-      end
-    end
+    subject { service }
 
-    describe '#build_type' do
-      it 'does not validate the presence of build_type if service is not active' do
-        teamcity_service = service
-        teamcity_service.active = false
+    context 'when service is active' do
+      before { subject.active = true }
 
-        expect(teamcity_service).not_to validate_presence_of(:build_type)
-      end
+      it { is_expected.to validate_presence_of(:build_type) }
+      it { is_expected.to validate_presence_of(:teamcity_url) }
+      it_behaves_like 'issue tracker service URL attribute', :teamcity_url
 
-      it 'validates the presence of build_type if service is active' do
-        teamcity_service = service
-        teamcity_service.active = true
+      describe '#username' do
+        it 'does not validate the presence of username if password is nil' do
+          subject.password = nil
 
-        expect(teamcity_service).to validate_presence_of(:build_type)
-      end
-    end
+          expect(subject).not_to validate_presence_of(:username)
+        end
 
-    describe '#username' do
-      it 'does not validate the presence of username if service is not active' do
-        teamcity_service = service
-        teamcity_service.active = false
+        it 'validates the presence of username if password is present' do
+          subject.password = 'secret'
 
-        expect(teamcity_service).not_to validate_presence_of(:username)
+          expect(subject).to validate_presence_of(:username)
+        end
       end
 
-      it 'does not validate the presence of username if username is nil' do
-        teamcity_service = service
-        teamcity_service.active = true
-        teamcity_service.password = nil
+      describe '#password' do
+        it 'does not validate the presence of password if username is nil' do
+          subject.username = nil
 
-        expect(teamcity_service).not_to validate_presence_of(:username)
-      end
+          expect(subject).not_to validate_presence_of(:password)
+        end
 
-      it 'validates the presence of username if service is active and username is present' do
-        teamcity_service = service
-        teamcity_service.active = true
-        teamcity_service.password = 'secret'
+        it 'validates the presence of password if username is present' do
+          subject.username = 'john'
 
-        expect(teamcity_service).to validate_presence_of(:username)
+          expect(subject).to validate_presence_of(:password)
+        end
       end
     end
 
-    describe '#password' do
-      it 'does not validate the presence of password if service is not active' do
-        teamcity_service = service
-        teamcity_service.active = false
-
-        expect(teamcity_service).not_to validate_presence_of(:password)
-      end
-
-      it 'does not validate the presence of password if username is nil' do
-        teamcity_service = service
-        teamcity_service.active = true
-        teamcity_service.username = nil
-
-        expect(teamcity_service).not_to validate_presence_of(:password)
-      end
-
-      it 'validates the presence of password if service is active and username is present' do
-        teamcity_service = service
-        teamcity_service.active = true
-        teamcity_service.username = 'john'
+    context 'when service is inactive' do
+      before { subject.active = false }
 
-        expect(teamcity_service).to validate_presence_of(:password)
-      end
+      it { is_expected.not_to validate_presence_of(:build_type) }
+      it { is_expected.not_to validate_presence_of(:teamcity_url) }
+      it { is_expected.not_to validate_presence_of(:username) }
+      it { is_expected.not_to validate_presence_of(:password) }
     end
   end
 

spec/requests/api/milestones_spec.rb

@@ -127,7 +127,7 @@ describe API::API, api: true  do
 
   describe 'GET /projects/:id/milestones/:milestone_id/issues' do
     before do
-      milestone.issues << create(:issue)
+      milestone.issues << create(:issue, project: project)
     end
     it 'should return project issues for a particular milestone' do
       get api("/projects/#{project.id}/milestones/#{milestone.id}/issues", user)
@@ -140,5 +140,34 @@ describe API::API, api: true  do
       get api("/projects/#{project.id}/milestones/#{milestone.id}/issues")
       expect(response.status).to eq(401)
     end
+
+    describe 'confidential issues' do
+      let(:public_project) { create(:project, :public) }
+      let(:milestone) { create(:milestone, project: public_project) }
+      let(:issue) { create(:issue, project: public_project) }
+      let(:confidential_issue) { create(:issue, confidential: true, project: public_project) }
+      before do
+        public_project.team << [user, :developer]
+        milestone.issues << issue << confidential_issue
+      end
+
+      it 'returns confidential issues to team members' do
+        get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", user)
+
+        expect(response.status).to eq(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(2)
+        expect(json_response.map { |issue| issue['id'] }).to include(issue.id, confidential_issue.id)
+      end
+
+      it 'does not return confidential issues to regular users' do
+        get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", create(:user))
+
+        expect(response.status).to eq(200)
+        expect(json_response).to be_an Array
+        expect(json_response.size).to eq(1)
+        expect(json_response.map { |issue| issue['id'] }).to include(issue.id)
+      end
+    end
   end
 end

spec/requests/api/notes_spec.rb

@@ -3,7 +3,7 @@ require 'spec_helper'
 describe API::API, api: true  do
   include ApiHelpers
   let(:user) { create(:user) }
-  let!(:project) { create(:project, namespace: user.namespace ) }
+  let!(:project) { create(:project, namespace: user.namespace) }
   let!(:issue) { create(:issue, project: project, author: user) }
   let!(:merge_request) { create(:merge_request, source_project: project, target_project: project, author: user) }
   let!(:snippet) { create(:project_snippet, project: project, author: user) }
@@ -45,7 +45,7 @@ describe API::API, api: true  do
       end
 
       it "should return a 404 error when issue id not found" do
-        get api("/projects/#{project.id}/issues/123/notes", user)
+        get api("/projects/#{project.id}/issues/12345/notes", user)
         expect(response.status).to eq(404)
       end
 
@@ -106,7 +106,7 @@ describe API::API, api: true  do
       end
 
       it "should return a 404 error if issue note not found" do
-        get api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user)
+        get api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
         expect(response.status).to eq(404)
       end
 
@@ -134,7 +134,7 @@ describe API::API, api: true  do
       end
 
       it "should return a 404 error if snippet note not found" do
-        get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/123", user)
+        get api("/projects/#{project.id}/snippets/#{snippet.id}/notes/12345", user)
         expect(response.status).to eq(404)
       end
     end
@@ -191,6 +191,27 @@ describe API::API, api: true  do
         expect(response.status).to eq(401)
       end
     end
+
+    context 'when user does not have access to create noteable' do
+      let(:private_issue) { create(:issue, project: create(:project, :private)) }
+
+      ##
+      # We are posting to project user has access to, but we use issue id
+      # from a different project, see #15577
+      #
+      before do
+        post api("/projects/#{project.id}/issues/#{private_issue.id}/notes", user),
+             body: 'Hi!'
+      end
+
+      it 'responds with 500' do
+        expect(response.status).to eq 500
+      end
+
+      it 'does not create new note' do
+        expect(private_issue.notes.reload).to be_empty
+      end
+    end
   end
 
   describe "POST /projects/:id/noteable/:noteable_id/notes to test observer on create" do
@@ -211,7 +232,7 @@ describe API::API, api: true  do
       end
 
       it 'should return a 404 error when note id not found' do
-        put api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user),
+        put api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user),
                 body: 'Hello!'
         expect(response.status).to eq(404)
       end
@@ -233,7 +254,7 @@ describe API::API, api: true  do
 
       it 'should return a 404 error when note id not found' do
         put api("/projects/#{project.id}/snippets/#{snippet.id}/"\
-                  "notes/123", user), body: "Hello!"
+                  "notes/12345", user), body: "Hello!"
         expect(response.status).to eq(404)
       end
     end
@@ -248,7 +269,7 @@ describe API::API, api: true  do
 
       it 'should return a 404 error when note id not found' do
         put api("/projects/#{project.id}/merge_requests/#{merge_request.id}/"\
-                  "notes/123", user), body: "Hello!"
+                  "notes/12345", user), body: "Hello!"
         expect(response.status).to eq(404)
       end
     end
@@ -268,7 +289,7 @@ describe API::API, api: true  do
       end
 
       it 'returns a 404 error when note id not found' do
-        delete api("/projects/#{project.id}/issues/#{issue.id}/notes/123", user)
+        delete api("/projects/#{project.id}/issues/#{issue.id}/notes/12345", user)
 
         expect(response.status).to eq(404)
       end
@@ -288,7 +309,7 @@ describe API::API, api: true  do
 
       it 'returns a 404 error when note id not found' do
         delete api("/projects/#{project.id}/snippets/#{snippet.id}/"\
-                   "notes/123", user)
+                   "notes/12345", user)
 
         expect(response.status).to eq(404)
       end
@@ -308,7 +329,7 @@ describe API::API, api: true  do
 
       it 'returns a 404 error when note id not found' do
         delete api("/projects/#{project.id}/merge_requests/"\
-                   "#{merge_request.id}/notes/123", user)
+                   "#{merge_request.id}/notes/12345", user)
 
         expect(response.status).to eq(404)
       end

spec/requests/api/project_hooks_spec.rb

@@ -148,14 +148,24 @@ describe API::API, 'ProjectHooks', api: true do
       expect(response.status).to eq(200)
     end
 
-    it "should return success when deleting non existent hook" do
+    it "should return a 404 error when deleting non existent hook" do
       delete api("/projects/#{project.id}/hooks/42", user)
-      expect(response.status).to eq(200)
+      expect(response.status).to eq(404)
     end
 
     it "should return a 405 error if hook id not given" do
       delete api("/projects/#{project.id}/hooks", user)
       expect(response.status).to eq(405)
     end
+
+    it "shold return a 404 if a user attempts to delete project hooks he/she does not own" do
+      test_user = create(:user)
+      other_project = create(:project)
+      other_project.team << [test_user, :master]
+
+      delete api("/projects/#{other_project.id}/hooks/#{hook.id}", test_user)
+      expect(response.status).to eq(404)
+      expect(WebHook.exists?(hook.id)).to be_truthy
+    end
   end
 end

spec/requests/api/project_snippets_spec.rb

@@ -15,4 +15,91 @@ describe API::API, api: true do
       expect(json_response['expires_at']).to be_nil
     end
   end
+
+  describe 'GET /projects/:project_id/snippets/' do
+    it 'all snippets available to team member' do
+      project = create(:project, :public)
+      user = create(:user)
+      project.team << [user, :developer]
+      public_snippet = create(:project_snippet, :public, project: project)
+      internal_snippet = create(:project_snippet, :internal, project: project)
+      private_snippet = create(:project_snippet, :private, project: project)
+
+      get api("/projects/#{project.id}/snippets/", user)
+
+      expect(response.status).to eq(200)
+      expect(json_response.size).to eq(3)
+      expect(json_response.map{ |snippet| snippet['id']} ).to include(public_snippet.id, internal_snippet.id, private_snippet.id)
+    end
+
+    it 'hides private snippets from regular user' do
+      project = create(:project, :public)
+      user = create(:user)
+      create(:project_snippet, :private, project: project)
+
+      get api("/projects/#{project.id}/snippets/", user)
+      expect(response.status).to eq(200)
+      expect(json_response.size).to eq(0)
+    end
+  end
+
+  describe 'POST /projects/:project_id/snippets/' do
+    it 'creates a new snippet' do
+      admin = create(:admin)
+      project = create(:project)
+      params = {
+        title: 'Test Title',
+        file_name: 'test.rb',
+        code: 'puts "hello world"',
+        visibility_level: Gitlab::VisibilityLevel::PUBLIC
+      }
+
+      post api("/projects/#{project.id}/snippets/", admin), params
+
+      expect(response.status).to eq(201)
+      snippet = ProjectSnippet.find(json_response['id'])
+      expect(snippet.content).to eq(params[:code])
+      expect(snippet.title).to eq(params[:title])
+      expect(snippet.file_name).to eq(params[:file_name])
+      expect(snippet.visibility_level).to eq(params[:visibility_level])
+    end
+  end
+
+  describe 'PUT /projects/:project_id/snippets/:id/' do
+    it 'updates snippet' do
+      admin = create(:admin)
+      snippet = create(:project_snippet, author: admin)
+      new_content = 'New content'
+
+      put api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin), code: new_content
+
+      expect(response.status).to eq(200)
+      snippet.reload
+      expect(snippet.content).to eq(new_content)
+    end
+  end
+
+  describe 'DELETE /projects/:project_id/snippets/:id/' do
+    it 'deletes snippet' do
+      admin = create(:admin)
+      snippet = create(:project_snippet, author: admin)
+
+      delete api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/", admin)
+
+      expect(response.status).to eq(200)
+    end
+  end
+
+  describe 'GET /projects/:project_id/snippets/:id/raw' do
+    it 'returns raw text' do
+      admin = create(:admin)
+      snippet = create(:project_snippet, author: admin)
+
+      get api("/projects/#{snippet.project.id}/snippets/#{snippet.id}/raw", admin)
+
+      expect(response.status).to eq(200)
+      expect(response.content_type).to eq 'text/plain'
+      expect(response.body).to eq(snippet.content)
+    end
+  end
 end

spec/requests/api/projects_spec.rb

@@ -11,7 +11,7 @@ describe API::API, api: true  do
   let(:project) { create(:project, creator_id: user.id, namespace: user.namespace) }
   let(:project2) { create(:project, path: 'project2', creator_id: user.id, namespace: user.namespace) }
   let(:project3) { create(:project, path: 'project3', creator_id: user.id, namespace: user.namespace) }
-  let(:snippet) { create(:project_snippet, author: user, project: project, title: 'example') }
+  let(:snippet) { create(:project_snippet, :public, author: user, project: project, title: 'example') }
   let(:project_member) { create(:project_member, :master, user: user, project: project) }
   let(:project_member2) { create(:project_member, :developer, user: user3, project: project) }
   let(:user4) { create(:user) }

spec/support/issue_tracker_service_shared_example.rb

+RSpec.shared_examples 'issue tracker service URL attribute' do |url_attr|
+  it { is_expected.to allow_value('https://example.com').for(url_attr) }
+
+  it { is_expected.not_to allow_value('example.com').for(url_attr) }
+  it { is_expected.not_to allow_value('ftp://example.com').for(url_attr) }
+  it { is_expected.not_to allow_value('herp-and-derp').for(url_attr) }
+end