From 2fd009d23810939a77ddc1716ab4250ee18826fa Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Wed, 8 Jun 2016 16:56:44 +0200 Subject: Retry spinach tests in case of failure using rerun reporter --- .gitlab-ci.yml | 4 +--- CHANGELOG | 1 + 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 5ef3081395a..965de5e50e9 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -89,9 +89,7 @@ update-knapsack: - export KNAPSACK_REPORT_PATH=knapsack/spinach_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json - export KNAPSACK_GENERATE_REPORT=true - cp knapsack/spinach_report.json ${KNAPSACK_REPORT_PATH} - - knapsack spinach "-r rerun" - # retry failed tests 3 times - - retry '[ ! -e tmp/spinach-rerun.txt ] || bin/spinach -r rerun $(cat tmp/spinach-rerun.txt)' + - knapsack spinach "-r rerun" || retry '[ ! -e tmp/spinach-rerun.txt ] || bin/spinach -r rerun $(cat tmp/spinach-rerun.txt)' artifacts: paths: - knapsack/ diff --git a/CHANGELOG b/CHANGELOG index 0506854599f..874b942f46c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -17,6 +17,7 @@ v 8.9.0 (unreleased) - Fix 404 page when viewing TODOs that contain milestones or labels in different projects - Redesign navigation for project pages - Fix groups API to list only user's accessible projects + - Retry spinach tests in case of failure using rerun reporter - Redesign account and email confirmation emails - Bump nokogiri to 1.6.8 - Use gitlab-shell v3.0.0 -- cgit v1.2.1 From 0e896ffe4eebb8bcf04bc1327d498bb041faed56 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Fri, 10 Jun 2016 14:51:16 +0200 Subject: Improve Gitlab::Auth method names Auth.find was a very generic name for a very specific method. Auth.find_in_gitlab_or_ldap was inaccurate in GitLab EE where it also looks in Kerberos. --- app/controllers/jwt_controller.rb | 2 +- app/controllers/projects/git_http_controller.rb | 2 +- config/initializers/doorkeeper.rb | 2 +- lib/api/session.rb | 2 +- lib/gitlab/auth.rb | 6 +++--- lib/gitlab/backend/grack_auth.rb | 2 +- spec/lib/gitlab/auth_spec.rb | 16 ++++++++-------- spec/requests/jwt_controller_spec.rb | 2 +- 8 files changed, 17 insertions(+), 17 deletions(-) diff --git a/app/controllers/jwt_controller.rb b/app/controllers/jwt_controller.rb index 131a16dad9b..014b9b43ff2 100644 --- a/app/controllers/jwt_controller.rb +++ b/app/controllers/jwt_controller.rb @@ -42,7 +42,7 @@ class JwtController < ApplicationController end def authenticate_user(login, password) - user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password) + user = Gitlab::Auth.find_with_user_password(login, password) Gitlab::Auth.rate_limit!(request.ip, success: user.present?, login: login) user end diff --git a/app/controllers/projects/git_http_controller.rb b/app/controllers/projects/git_http_controller.rb index 348d6cf4d96..f907d63258b 100644 --- a/app/controllers/projects/git_http_controller.rb +++ b/app/controllers/projects/git_http_controller.rb @@ -43,7 +43,7 @@ class Projects::GitHttpController < Projects::ApplicationController return if project && project.public? && upload_pack? authenticate_or_request_with_http_basic do |login, password| - auth_result = Gitlab::Auth.find(login, password, project: project, ip: request.ip) + auth_result = Gitlab::Auth.find_for_git_client(login, password, project: project, ip: request.ip) if auth_result.type == :ci && upload_pack? @ci = true diff --git a/config/initializers/doorkeeper.rb b/config/initializers/doorkeeper.rb index 8dc8e270afc..618dba74151 100644 --- a/config/initializers/doorkeeper.rb +++ b/config/initializers/doorkeeper.rb @@ -12,7 +12,7 @@ Doorkeeper.configure do end resource_owner_from_credentials do |routes| - Gitlab::Auth.find_in_gitlab_or_ldap(params[:username], params[:password]) + Gitlab::Auth.find_with_user_password(params[:username], params[:password]) end # If you want to restrict access to the web interface for adding oauth authorized applications, you need to declare the block below. diff --git a/lib/api/session.rb b/lib/api/session.rb index 56e69b2366f..56c202f1294 100644 --- a/lib/api/session.rb +++ b/lib/api/session.rb @@ -11,7 +11,7 @@ module API # Example Request: # POST /session post "/session" do - user = Gitlab::Auth.find_in_gitlab_or_ldap(params[:email] || params[:login], params[:password]) + user = Gitlab::Auth.find_with_user_password(params[:email] || params[:login], params[:password]) return unauthorized! unless user present user, with: Entities::UserLogin diff --git a/lib/gitlab/auth.rb b/lib/gitlab/auth.rb index 076e2af7d38..db1704af75e 100644 --- a/lib/gitlab/auth.rb +++ b/lib/gitlab/auth.rb @@ -3,14 +3,14 @@ module Gitlab Result = Struct.new(:user, :type) class << self - def find(login, password, project:, ip:) + 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 - elsif result.user = find_in_gitlab_or_ldap(login, password) + elsif result.user = find_with_user_password(login, password) result.type = :gitlab_or_ldap elsif result.user = oauth_access_token_check(login, password) result.type = :oauth @@ -20,7 +20,7 @@ module Gitlab result end - def find_in_gitlab_or_ldap(login, password) + def find_with_user_password(login, password) user = User.by_login(login) # If no user is found, or it's an LDAP server, try LDAP. diff --git a/lib/gitlab/backend/grack_auth.rb b/lib/gitlab/backend/grack_auth.rb index 9e09d2e118d..adbf5941a96 100644 --- a/lib/gitlab/backend/grack_auth.rb +++ b/lib/gitlab/backend/grack_auth.rb @@ -95,7 +95,7 @@ module Grack end def authenticate_user(login, password) - user = Gitlab::Auth.find_in_gitlab_or_ldap(login, password) + user = Gitlab::Auth.find_with_user_password(login, password) unless user user = oauth_access_token_check(login, password) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index a814ad2a4e7..f081d550ec8 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -41,7 +41,7 @@ describe Gitlab::Auth, lib: true do end end - describe 'find_in_gitlab_or_ldap' do + describe 'find_with_user_password' do let!(:user) do create(:user, username: username, @@ -52,25 +52,25 @@ describe Gitlab::Auth, lib: true do let(:password) { 'my-secret' } it "should find user by valid login/password" do - expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).to eql user + expect( gl_auth.find_with_user_password(username, password) ).to eql user end it 'should find user by valid email/password with case-insensitive email' do - expect(gl_auth.find_in_gitlab_or_ldap(user.email.upcase, password)).to eql user + expect(gl_auth.find_with_user_password(user.email.upcase, password)).to eql user end it 'should find user by valid username/password with case-insensitive username' do - expect(gl_auth.find_in_gitlab_or_ldap(username.upcase, password)).to eql user + expect(gl_auth.find_with_user_password(username.upcase, password)).to eql user end it "should not find user with invalid password" do password = 'wrong' - expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).not_to eql user + expect( gl_auth.find_with_user_password(username, password) ).not_to eql user end it "should not find user with invalid login" do user = 'wrong' - expect( gl_auth.find_in_gitlab_or_ldap(username, password) ).not_to eql user + expect( gl_auth.find_with_user_password(username, password) ).not_to eql user end context "with ldap enabled" do @@ -81,13 +81,13 @@ describe Gitlab::Auth, lib: true do it "tries to autheticate with db before ldap" do expect(Gitlab::LDAP::Authentication).not_to receive(:login) - gl_auth.find_in_gitlab_or_ldap(username, password) + gl_auth.find_with_user_password(username, password) end it "uses ldap as fallback to for authentication" do expect(Gitlab::LDAP::Authentication).to receive(:login) - gl_auth.find_in_gitlab_or_ldap('ldap_user', 'password') + gl_auth.find_with_user_password('ldap_user', 'password') end end end diff --git a/spec/requests/jwt_controller_spec.rb b/spec/requests/jwt_controller_spec.rb index c995993a853..d2d4a9eca18 100644 --- a/spec/requests/jwt_controller_spec.rb +++ b/spec/requests/jwt_controller_spec.rb @@ -44,7 +44,7 @@ describe JwtController do let(:user) { create(:user) } let(:headers) { { authorization: credentials('user', 'password') } } - before { expect(Gitlab::Auth).to receive(:find_in_gitlab_or_ldap).with('user', 'password').and_return(user) } + before { expect(Gitlab::Auth).to receive(:find_with_user_password).with('user', 'password').and_return(user) } subject! { get '/jwt/auth', parameters, headers } -- cgit v1.2.1 From 16927e3fadfb5b9b2c3b12ffdf7ab217971b8cca Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Mon, 30 May 2016 11:05:17 +0200 Subject: Enable Style/MultilineBlockChain rubocop style cop See #17478 --- .rubocop.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.rubocop.yml b/.rubocop.yml index 678f7db025b..72db0342543 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -348,7 +348,7 @@ Style/MultilineArrayBraceLayout: # Avoid multi-line chains of blocks. Style/MultilineBlockChain: - Enabled: false + Enabled: true # Ensures newlines after multiline block do statements. Style/MultilineBlockLayout: -- cgit v1.2.1 From dc7734f4bf919e67323eafdaa1c875a84433fdf9 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 10 Jun 2016 22:08:06 +0200 Subject: Use bundle exec to run spinach --- .gitlab-ci.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index aa84a9e4da2..b5585158479 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -91,7 +91,7 @@ update-knapsack: - export KNAPSACK_REPORT_PATH=knapsack/spinach_node_${CI_NODE_INDEX}_${CI_NODE_TOTAL}_report.json - export KNAPSACK_GENERATE_REPORT=true - cp knapsack/spinach_report.json ${KNAPSACK_REPORT_PATH} - - knapsack spinach "-r rerun" || retry '[ ! -e tmp/spinach-rerun.txt ] || bin/spinach -r rerun $(cat tmp/spinach-rerun.txt)' + - knapsack spinach "-r rerun" || retry '[ ! -e tmp/spinach-rerun.txt ] || bundle exec spinach -r rerun $(cat tmp/spinach-rerun.txt)' artifacts: paths: - knapsack/ -- cgit v1.2.1 From f395ed242add9c21536a6e4d9ea7f0154351e503 Mon Sep 17 00:00:00 2001 From: Connor Shea Date: Sun, 12 Jun 2016 18:25:21 -0600 Subject: Update brakeman from 3.2.1 to 3.3.2 Removes a few dependencies. Changelog: https://github.com/presidentbeef/brakeman/blob/master/CHANGES --- Gemfile | 2 +- Gemfile.lock | 28 +++++----------------------- 2 files changed, 6 insertions(+), 24 deletions(-) diff --git a/Gemfile b/Gemfile index b2660144f2b..6d8a33c2eef 100644 --- a/Gemfile +++ b/Gemfile @@ -245,7 +245,7 @@ end group :development do gem "foreman" - gem 'brakeman', '~> 3.2.0', require: false + gem 'brakeman', '~> 3.3.0', require: false gem 'letter_opener_web', '~> 1.3.0' gem 'quiet_assets', '~> 1.0.2' diff --git a/Gemfile.lock b/Gemfile.lock index dfc15700494..2ba2676efa1 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -97,16 +97,7 @@ GEM bootstrap-sass (3.3.6) autoprefixer-rails (>= 5.2.1) sass (>= 3.3.4) - brakeman (3.2.1) - erubis (~> 2.6) - haml (>= 3.0, < 5.0) - highline (>= 1.6.20, < 2.0) - ruby2ruby (~> 2.3.0) - ruby_parser (~> 3.8.1) - safe_yaml (>= 1.0) - sass (~> 3.0) - slim (>= 1.3.6, < 4.0) - terminal-table (~> 1.4) + brakeman (3.3.2) browser (2.0.3) builder (3.2.2) bullet (5.0.0) @@ -338,7 +329,6 @@ GEM hashie (3.4.3) health_check (1.5.1) rails (>= 2.3.0) - highline (1.7.8) hipchat (1.5.2) httparty mimemagic @@ -642,10 +632,7 @@ GEM ruby-saml (1.1.2) nokogiri (>= 1.5.10) uuid (~> 2.3) - ruby2ruby (2.3.0) - ruby_parser (~> 3.1) - sexp_processor (~> 4.0) - ruby_parser (3.8.1) + ruby_parser (3.8.2) sexp_processor (~> 4.1) rubyntlm (0.5.2) rubypants (0.2.0) @@ -655,7 +642,7 @@ GEM safe_yaml (1.0.4) sanitize (2.1.0) nokogiri (>= 1.4.4) - sass (3.4.21) + sass (3.4.22) sass-rails (5.0.4) railties (>= 4.0.0, < 5.0) sass (~> 3.1) @@ -704,9 +691,6 @@ GEM tilt (>= 1.3, < 3) six (0.2.0) slack-notifier (1.2.1) - slim (3.0.6) - temple (~> 0.7.3) - tilt (>= 1.3.3, < 2.1) slop (3.6.0) spinach (0.8.10) colorize @@ -747,10 +731,8 @@ GEM railties (>= 3.2.5, < 6) teaspoon-jasmine (2.2.0) teaspoon (>= 1.0.0) - temple (0.7.6) term-ansicolor (1.3.2) tins (~> 1.0) - terminal-table (1.5.2) test_after_commit (0.4.2) activerecord (>= 3.2) thin (1.6.4) @@ -759,7 +741,7 @@ GEM rack (~> 1.0) thor (0.19.1) thread_safe (0.3.5) - tilt (2.0.2) + tilt (2.0.5) timecop (0.8.1) timfel-krb5-auth (0.8.3) tinder (1.10.1) @@ -848,7 +830,7 @@ DEPENDENCIES better_errors (~> 1.0.1) binding_of_caller (~> 0.7.2) bootstrap-sass (~> 3.3.0) - brakeman (~> 3.2.0) + brakeman (~> 3.3.0) browser (~> 2.0.3) bullet bundler-audit -- cgit v1.2.1 From 0e50fa24a5b4ff7665a68da2a1221b5cae9e5633 Mon Sep 17 00:00:00 2001 From: Dmitriy Zaporozhets Date: Mon, 13 Jun 2016 12:32:53 +0300 Subject: Remove counters from Pipeline navigation Signed-off-by: Dmitriy Zaporozhets --- app/views/projects/pipelines/_head.html.haml | 2 -- 1 file changed, 2 deletions(-) diff --git a/app/views/projects/pipelines/_head.html.haml b/app/views/projects/pipelines/_head.html.haml index f278d4e0538..d0ba0d27d7c 100644 --- a/app/views/projects/pipelines/_head.html.haml +++ b/app/views/projects/pipelines/_head.html.haml @@ -5,11 +5,9 @@ = link_to project_pipelines_path(@project), title: 'Pipelines', class: 'shortcuts-pipelines' do %span Pipelines - %span.badge.count.ci_counter= number_with_delimiter(@project.pipelines.running_or_pending.count) - if project_nav_tab? :builds = nav_link(controller: %w(builds)) do = link_to project_builds_path(@project), title: 'Builds', class: 'shortcuts-builds' do %span Builds - %span.badge.count.builds_counter= number_with_delimiter(@project.running_or_pending_build_count) -- cgit v1.2.1 From f73cf3e937b92d29753e468dac8a17470253c791 Mon Sep 17 00:00:00 2001 From: Jacob Vosmaer Date: Mon, 13 Jun 2016 15:38:25 +0200 Subject: Also rename "find" in the specs --- spec/lib/gitlab/auth_spec.rb | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/spec/lib/gitlab/auth_spec.rb b/spec/lib/gitlab/auth_spec.rb index f081d550ec8..7bec1367156 100644 --- a/spec/lib/gitlab/auth_spec.rb +++ b/spec/lib/gitlab/auth_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe Gitlab::Auth, lib: true do let(:gl_auth) { described_class } - describe 'find' do + describe 'find_for_git_client' do it 'recognizes CI' do token = '123' project = create(:empty_project) @@ -11,7 +11,7 @@ describe Gitlab::Auth, lib: true do ip = 'ip' expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'gitlab-ci-token') - expect(gl_auth.find('gitlab-ci-token', token, project: project, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, :ci)) + expect(gl_auth.find_for_git_client('gitlab-ci-token', token, project: project, ip: ip)).to eq(Gitlab::Auth::Result.new(nil, :ci)) end it 'recognizes master passwords' do @@ -19,7 +19,7 @@ describe Gitlab::Auth, lib: true do ip = 'ip' expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: user.username) - expect(gl_auth.find(user.username, 'password', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :gitlab_or_ldap)) + 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 OAuth tokens' do @@ -29,7 +29,7 @@ describe Gitlab::Auth, lib: true do ip = 'ip' expect(gl_auth).to receive(:rate_limit!).with(ip, success: true, login: 'oauth2') - expect(gl_auth.find("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :oauth)) + expect(gl_auth.find_for_git_client("oauth2", token.token, project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new(user, :oauth)) end it 'returns double nil for invalid credentials' do @@ -37,7 +37,7 @@ describe Gitlab::Auth, lib: true do ip = 'ip' expect(gl_auth).to receive(:rate_limit!).with(ip, success: false, login: login) - expect(gl_auth.find(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new) + expect(gl_auth.find_for_git_client(login, 'bar', project: nil, ip: ip)).to eq(Gitlab::Auth::Result.new) end end -- cgit v1.2.1 From f9bbc4d485d32a8233894a75eae4945a2807188b Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Mon, 13 Jun 2016 18:23:55 +0200 Subject: Revert CHANGELOG --- CHANGELOG | 1 - 1 file changed, 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index f0b0bf4a2f7..8f897b4a34c 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -20,7 +20,6 @@ v 8.9.0 (unreleased) - Fix 404 page when viewing TODOs that contain milestones or labels in different projects - Redesign navigation for project pages - Fix groups API to list only user's accessible projects - - Retry spinach tests in case of failure using rerun reporter - Redesign account and email confirmation emails - `git clone https://host/namespace/project` now works, in addition to using the `.git` suffix - Bump nokogiri to 1.6.8 -- cgit v1.2.1 From af33338bbf752ab5f4dba5b981644b399ae3563b Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Sun, 15 May 2016 00:22:01 -0700 Subject: Add more information into RSS fead for issues --- CHANGELOG | 2 ++ app/views/issues/_issue.atom.builder | 20 ++++++++++++++++- spec/features/atom/dashboard_issues_spec.rb | 35 +++++++++++++++++++++-------- 3 files changed, 47 insertions(+), 10 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 7a6a14919da..80b19bbd04e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -123,6 +123,8 @@ v 8.8.1 v 8.8.0 - Implement GFM references for milestones (Alejandro Rodríguez) +v 8.8.0 (unreleased) + - Add more information into RSS fead for issues. - Snippets tab under user profile. !4001 (Long Nguyen) - Fix error when using link to uploads in global snippets - Fix Error 500 when attempting to retrieve project license when HEAD points to non-existent ref diff --git a/app/views/issues/_issue.atom.builder b/app/views/issues/_issue.atom.builder index 68a2d19e58d..96831874144 100644 --- a/app/views/issues/_issue.atom.builder +++ b/app/views/issues/_issue.atom.builder @@ -5,10 +5,28 @@ xml.entry do xml.updated issue.created_at.xmlschema xml.media :thumbnail, width: "40", height: "40", url: image_url(avatar_icon(issue.author_email)) - xml.author do |author| + xml.author do xml.name issue.author_name xml.email issue.author_email end xml.summary issue.title + xml.description issue.description if issue.description + xml.milestone issue.milestone.title if issue.milestone + xml.due_date issue.due_date if issue.due_date + + unless issue.labels.empty? + xml.labels do + issue.labels.each do |label| + xml.label label.name + end + end + end + + if issue.assignee + xml.assignee do + xml.name issue.assignee.name + xml.email issue.assignee.email + end + end end diff --git a/spec/features/atom/dashboard_issues_spec.rb b/spec/features/atom/dashboard_issues_spec.rb index b710cb3c72f..87b478adb8f 100644 --- a/spec/features/atom/dashboard_issues_spec.rb +++ b/spec/features/atom/dashboard_issues_spec.rb @@ -2,15 +2,18 @@ require 'spec_helper' describe "Dashboard Issues Feed", feature: true do describe "GET /issues" do - let!(:user) { create(:user) } - let!(:project1) { create(:project) } - let!(:project2) { create(:project) } - let!(:issue1) { create(:issue, author: user, assignee: user, project: project1) } - let!(:issue2) { create(:issue, author: user, assignee: user, project: project2) } + let!(:user) { create(:user) } + let!(:project1) { create(:project) } + let!(:project2) { create(:project) } + let!(:milestone1) { create(:milestone, project: project1, title: 'v1') } + let!(:label1) { create(:label, project: project1, title: 'label1') } + let!(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone1) } + let!(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'test desc') } before do project1.team << [user, :master] project2.team << [user, :master] + issue1.labels << label1 end describe "atom feed" do @@ -20,10 +23,24 @@ describe "Dashboard Issues Feed", feature: true do expect(response_headers['Content-Type']). to have_content('application/atom+xml') expect(body).to have_selector('title', text: "#{user.name} issues") - expect(body).to have_selector('author email', text: issue1.author_email) - expect(body).to have_selector('entry summary', text: issue1.title) - expect(body).to have_selector('author email', text: issue2.author_email) - expect(body).to have_selector('entry summary', text: issue2.title) + + entry_1 = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue1.title}')]") + expect(entry_1).to be_present + + entry_2 = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue2.title}')]") + expect(entry_2).to be_present + + expect(entry_1).to have_selector('author email', text: issue1.author_email) + expect(entry_1).to have_selector('assignee email', text: issue1.author_email) + expect(entry_1).to have_selector('labels label', text: label1.title) + expect(entry_1).to have_selector('milestone', text: milestone1.title) + expect(entry_1).not_to have_selector('description') + + expect(entry_2).to have_selector('author email', text: issue2.author_email) + expect(entry_2).to have_selector('assignee email', text: issue2.author_email) + expect(entry_2).not_to have_selector('labels') + expect(entry_2).not_to have_selector('milestone') + expect(entry_2).to have_selector('description', text: issue1.description) end end end -- cgit v1.2.1 From 5328930e3f7d013791417ba38805cfab9d029dbd Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Sat, 11 Jun 2016 20:01:16 -0700 Subject: Move change description to proper release and fix typo --- CHANGELOG | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG b/CHANGELOG index 80b19bbd04e..1e611ef481e 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,6 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.9.0 (unreleased) - Fix Error 500 when using closes_issues API with an external issue tracker + - Add more information into RSS feed for issues. - Bulk assign/unassign labels to issues. - Ability to prioritize labels !4009 / !3205 (Thijs Wouters) - Fix endless redirections when accessing user OAuth applications when they are disabled @@ -124,7 +125,6 @@ v 8.8.1 v 8.8.0 - Implement GFM references for milestones (Alejandro Rodríguez) v 8.8.0 (unreleased) - - Add more information into RSS fead for issues. - Snippets tab under user profile. !4001 (Long Nguyen) - Fix error when using link to uploads in global snippets - Fix Error 500 when attempting to retrieve project license when HEAD points to non-existent ref -- cgit v1.2.1 From fcbb14f6b79ea4d97078ebae9df9a0fc4bba021e Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Sat, 11 Jun 2016 20:03:39 -0700 Subject: Move issue rendering tests into separate contexts --- spec/features/atom/dashboard_issues_spec.rb | 58 ++++++++++++++++++----------- 1 file changed, 37 insertions(+), 21 deletions(-) diff --git a/spec/features/atom/dashboard_issues_spec.rb b/spec/features/atom/dashboard_issues_spec.rb index 87b478adb8f..9a4eb8f9504 100644 --- a/spec/features/atom/dashboard_issues_spec.rb +++ b/spec/features/atom/dashboard_issues_spec.rb @@ -5,42 +5,58 @@ describe "Dashboard Issues Feed", feature: true do let!(:user) { create(:user) } let!(:project1) { create(:project) } let!(:project2) { create(:project) } - let!(:milestone1) { create(:milestone, project: project1, title: 'v1') } - let!(:label1) { create(:label, project: project1, title: 'label1') } - let!(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone1) } - let!(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'test desc') } before do project1.team << [user, :master] project2.team << [user, :master] - issue1.labels << label1 end describe "atom feed" do it "should render atom feed via private token" do visit issues_dashboard_path(:atom, private_token: user.private_token) - expect(response_headers['Content-Type']). - to have_content('application/atom+xml') + expect(response_headers['Content-Type']).to have_content('application/atom+xml') expect(body).to have_selector('title', text: "#{user.name} issues") + end + + context "issue with basic fields" do + let!(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'test desc') } + + it "should render issue fields" do + visit issues_dashboard_path(:atom, private_token: user.private_token) + + entry = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue2.title}')]") + + expect(entry).to be_present + expect(entry).to have_selector('author email', text: issue2.author_email) + expect(entry).to have_selector('assignee email', text: issue2.author_email) + expect(entry).not_to have_selector('labels') + expect(entry).not_to have_selector('milestone') + expect(entry).to have_selector('description', text: issue2.description) + end + end + + context "issue with label and milestone" do + let!(:milestone1) { create(:milestone, project: project1, title: 'v1') } + let!(:label1) { create(:label, project: project1, title: 'label1') } + let!(:issue1) { create(:issue, author: user, assignee: user, project: project1, milestone: milestone1) } - entry_1 = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue1.title}')]") - expect(entry_1).to be_present + before do + issue1.labels << label1 + end - entry_2 = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue2.title}')]") - expect(entry_2).to be_present + it "should render issue label and milestone info" do + visit issues_dashboard_path(:atom, private_token: user.private_token) - expect(entry_1).to have_selector('author email', text: issue1.author_email) - expect(entry_1).to have_selector('assignee email', text: issue1.author_email) - expect(entry_1).to have_selector('labels label', text: label1.title) - expect(entry_1).to have_selector('milestone', text: milestone1.title) - expect(entry_1).not_to have_selector('description') + entry = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue1.title}')]") - expect(entry_2).to have_selector('author email', text: issue2.author_email) - expect(entry_2).to have_selector('assignee email', text: issue2.author_email) - expect(entry_2).not_to have_selector('labels') - expect(entry_2).not_to have_selector('milestone') - expect(entry_2).to have_selector('description', text: issue1.description) + expect(entry).to be_present + expect(entry).to have_selector('author email', text: issue1.author_email) + expect(entry).to have_selector('assignee email', text: issue1.author_email) + expect(entry).to have_selector('labels label', text: label1.title) + expect(entry).to have_selector('milestone', text: milestone1.title) + expect(entry).not_to have_selector('description') + end end end end -- cgit v1.2.1 From e8bf8ec40725a0af21677211f2d73d9b516c184a Mon Sep 17 00:00:00 2001 From: Alexander Matyushentsev Date: Mon, 13 Jun 2016 09:51:55 -0700 Subject: Apply reviewer notes: update CHANGELOG, adjust code formatting --- CHANGELOG | 3 +-- spec/features/atom/dashboard_issues_spec.rb | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 1e611ef481e..01c7f3c2ef3 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -2,7 +2,7 @@ Please view this file on the master branch, on stable branches it's out of date. v 8.9.0 (unreleased) - Fix Error 500 when using closes_issues API with an external issue tracker - - Add more information into RSS feed for issues. + - Add more information into RSS feed for issues (Alexander Matyushentsev) - Bulk assign/unassign labels to issues. - Ability to prioritize labels !4009 / !3205 (Thijs Wouters) - Fix endless redirections when accessing user OAuth applications when they are disabled @@ -124,7 +124,6 @@ v 8.8.1 v 8.8.0 - Implement GFM references for milestones (Alejandro Rodríguez) -v 8.8.0 (unreleased) - Snippets tab under user profile. !4001 (Long Nguyen) - Fix error when using link to uploads in global snippets - Fix Error 500 when attempting to retrieve project license when HEAD points to non-existent ref diff --git a/spec/features/atom/dashboard_issues_spec.rb b/spec/features/atom/dashboard_issues_spec.rb index 9a4eb8f9504..4dd9548cfc5 100644 --- a/spec/features/atom/dashboard_issues_spec.rb +++ b/spec/features/atom/dashboard_issues_spec.rb @@ -2,9 +2,9 @@ require 'spec_helper' describe "Dashboard Issues Feed", feature: true do describe "GET /issues" do - let!(:user) { create(:user) } - let!(:project1) { create(:project) } - let!(:project2) { create(:project) } + let!(:user) { create(:user) } + let!(:project1) { create(:project) } + let!(:project2) { create(:project) } before do project1.team << [user, :master] @@ -12,7 +12,7 @@ describe "Dashboard Issues Feed", feature: true do end describe "atom feed" do - it "should render atom feed via private token" do + it "renders atom feed via private token" do visit issues_dashboard_path(:atom, private_token: user.private_token) expect(response_headers['Content-Type']).to have_content('application/atom+xml') @@ -22,7 +22,7 @@ describe "Dashboard Issues Feed", feature: true do context "issue with basic fields" do let!(:issue2) { create(:issue, author: user, assignee: user, project: project2, description: 'test desc') } - it "should render issue fields" do + it "renders issue fields" do visit issues_dashboard_path(:atom, private_token: user.private_token) entry = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue2.title}')]") @@ -45,7 +45,7 @@ describe "Dashboard Issues Feed", feature: true do issue1.labels << label1 end - it "should render issue label and milestone info" do + it "renders issue label and milestone info" do visit issues_dashboard_path(:atom, private_token: user.private_token) entry = find(:xpath, "//feed/entry[contains(summary/text(),'#{issue1.title}')]") -- cgit v1.2.1 From 747989c115c86b090612f805170beb175fe1672a Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 13 Jun 2016 17:40:36 -0300 Subject: =?UTF-8?q?Schema=20doesn=E2=80=99t=20reflect=20the=20changes=20of?= =?UTF-8?q?=20the=20last=203=20migrations?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The schema doesn’t reflect the changes of the last 3 migrations: * 20160610140403_remove_notification_setting_not_null_constraints.rb * 20160610201627_migrate_users_notification_level.rb * 20160610301627_remove_notification_level_from_users.rb --- db/schema.rb | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/db/schema.rb b/db/schema.rb index aac327797e7..3c947d62e82 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema.define(version: 20160608155312) do +ActiveRecord::Schema.define(version: 20160610301627) do # These are extensions that must be enabled in order to support this database enable_extension "plpgsql" @@ -670,8 +670,8 @@ ActiveRecord::Schema.define(version: 20160608155312) do create_table "notification_settings", force: :cascade do |t| t.integer "user_id", null: false - t.integer "source_id", null: false - t.string "source_type", null: false + t.integer "source_id" + t.string "source_type" t.integer "level", default: 0, null: false t.datetime "created_at", null: false t.datetime "updated_at", null: false @@ -988,7 +988,6 @@ ActiveRecord::Schema.define(version: 20160608155312) do t.boolean "can_create_team", default: true, null: false t.string "state" t.integer "color_scheme_id", default: 1, null: false - t.integer "notification_level", default: 1, null: false t.datetime "password_expires_at" t.integer "created_by_id" t.datetime "last_credential_check_at" -- cgit v1.2.1 From 0568b90c97dcbad3ab100e060fef91e0786aafe8 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 10 Jun 2016 16:53:20 -0300 Subject: Remove deprecated issues_tracker and issues_tracker_id from project --- app/models/project.rb | 5 ----- .../project_services/issue_tracker_service.rb | 18 +++--------------- ...recated_issues_tracker_columns_from_projects.rb | 6 ++++++ db/schema.rb | 2 -- spec/factories/projects.rb | 6 ------ spec/helpers/issues_helper_spec.rb | 16 +++------------- spec/models/project_spec.rb | 22 ---------------------- 7 files changed, 12 insertions(+), 63 deletions(-) create mode 100644 db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb diff --git a/app/models/project.rb b/app/models/project.rb index e2f7ffe493c..dfa99fe0df2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -146,7 +146,6 @@ class Project < ActiveRecord::Base message: Gitlab::Regex.project_path_regex_message } validates :issues_enabled, :merge_requests_enabled, :wiki_enabled, inclusion: { in: [true, false] } - validates :issues_tracker_id, length: { maximum: 255 }, allow_blank: true validates :namespace, presence: true validates_uniqueness_of :name, scope: :namespace_id validates_uniqueness_of :path, scope: :namespace_id @@ -589,10 +588,6 @@ class Project < ActiveRecord::Base update_column(:has_external_issue_tracker, services.external_issue_trackers.any?) end - def can_have_issues_tracker_id? - self.issues_enabled && !self.default_issues_tracker? - end - def build_missing_services services_templates = Service.where(template: true) diff --git a/app/models/project_services/issue_tracker_service.rb b/app/models/project_services/issue_tracker_service.rb index 6ae9b16d3ce..87ecb3b8b86 100644 --- a/app/models/project_services/issue_tracker_service.rb +++ b/app/models/project_services/issue_tracker_service.rb @@ -38,9 +38,9 @@ class IssueTrackerService < Service if enabled_in_gitlab_config self.properties = { title: issues_tracker['title'], - project_url: add_issues_tracker_id(issues_tracker['project_url']), - issues_url: add_issues_tracker_id(issues_tracker['issues_url']), - new_issue_url: add_issues_tracker_id(issues_tracker['new_issue_url']) + project_url: issues_tracker['project_url'], + issues_url: issues_tracker['issues_url'], + new_issue_url: issues_tracker['new_issue_url'] } else self.properties = {} @@ -83,16 +83,4 @@ class IssueTrackerService < Service def issues_tracker Gitlab.config.issues_tracker[to_param] end - - def add_issues_tracker_id(url) - if self.project - id = self.project.issues_tracker_id - - if id - url = url.gsub(":issues_tracker_id", id) - end - end - - url - end end diff --git a/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb b/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb new file mode 100644 index 00000000000..477b2106dea --- /dev/null +++ b/db/migrate/20160610194713_remove_deprecated_issues_tracker_columns_from_projects.rb @@ -0,0 +1,6 @@ +class RemoveDeprecatedIssuesTrackerColumnsFromProjects < ActiveRecord::Migration + def change + remove_column :projects, :issues_tracker, :string, default: 'gitlab', null: false + remove_column :projects, :issues_tracker_id, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 3c947d62e82..3dccbbd50ba 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -751,8 +751,6 @@ ActiveRecord::Schema.define(version: 20160610301627) do t.boolean "merge_requests_enabled", default: true, null: false t.boolean "wiki_enabled", default: true, null: false t.integer "namespace_id" - t.string "issues_tracker", default: "gitlab", null: false - t.string "issues_tracker_id" t.boolean "snippets_enabled", default: true, null: false t.datetime "last_activity_at" t.string "import_url" diff --git a/spec/factories/projects.rb b/spec/factories/projects.rb index da8d97c9f82..5c8ddbebf0d 100644 --- a/spec/factories/projects.rb +++ b/spec/factories/projects.rb @@ -67,9 +67,6 @@ FactoryGirl.define do 'new_issue_url' => 'http://redmine/projects/project_name_in_redmine/issues/new' } ) - - project.issues_tracker = 'redmine' - project.issues_tracker_id = 'project_name_in_redmine' end end @@ -84,9 +81,6 @@ FactoryGirl.define do 'new_issue_url' => 'http://jira.example/secure/CreateIssue.jspa' } ) - - project.issues_tracker = 'jira' - project.issues_tracker_id = 'project_name_in_jira' end end end diff --git a/spec/helpers/issues_helper_spec.rb b/spec/helpers/issues_helper_spec.rb index eae61a54dfc..831ae7fb69c 100644 --- a/spec/helpers/issues_helper_spec.rb +++ b/spec/helpers/issues_helper_spec.rb @@ -7,10 +7,7 @@ describe IssuesHelper do describe "url_for_project_issues" do let(:project_url) { ext_project.external_issue_tracker.project_url } - let(:ext_expected) do - project_url.gsub(':project_id', ext_project.id.to_s) - .gsub(':issues_tracker_id', ext_project.issues_tracker_id.to_s) - end + let(:ext_expected) { project_url.gsub(':project_id', ext_project.id.to_s) } let(:int_expected) { polymorphic_path([@project.namespace, project]) } it "should return internal path if used internal tracker" do @@ -56,11 +53,7 @@ describe IssuesHelper do describe "url_for_issue" do let(:issues_url) { ext_project.external_issue_tracker.issues_url} - let(:ext_expected) do - issues_url.gsub(':id', issue.iid.to_s) - .gsub(':project_id', ext_project.id.to_s) - .gsub(':issues_tracker_id', ext_project.issues_tracker_id.to_s) - end + let(:ext_expected) { issues_url.gsub(':id', issue.iid.to_s).gsub(':project_id', ext_project.id.to_s) } let(:int_expected) { polymorphic_path([@project.namespace, project, issue]) } it "should return internal path if used internal tracker" do @@ -106,10 +99,7 @@ describe IssuesHelper do describe 'url_for_new_issue' do let(:issues_url) { ext_project.external_issue_tracker.new_issue_url } - let(:ext_expected) do - issues_url.gsub(':project_id', ext_project.id.to_s) - .gsub(':issues_tracker_id', ext_project.issues_tracker_id.to_s) - end + let(:ext_expected) { issues_url.gsub(':project_id', ext_project.id.to_s) } let(:int_expected) { new_namespace_project_issue_path(project.namespace, project) } it "should return internal path if used internal tracker" do diff --git a/spec/models/project_spec.rb b/spec/models/project_spec.rb index f3590f72cfe..de8815f5a38 100644 --- a/spec/models/project_spec.rb +++ b/spec/models/project_spec.rb @@ -53,7 +53,6 @@ describe Project, models: true do it { is_expected.to validate_length_of(:path).is_within(0..255) } it { is_expected.to validate_length_of(:description).is_within(0..2000) } it { is_expected.to validate_presence_of(:creator) } - it { is_expected.to validate_length_of(:issues_tracker_id).is_within(0..255) } it { is_expected.to validate_presence_of(:namespace) } it 'should not allow new projects beyond user limits' do @@ -321,27 +320,6 @@ describe Project, models: true do end end - describe :can_have_issues_tracker_id? do - let(:project) { create(:project) } - let(:ext_project) { create(:redmine_project) } - - it 'should be true for projects with external issues tracker if issues enabled' do - expect(ext_project.can_have_issues_tracker_id?).to be_truthy - end - - it 'should be false for projects with internal issue tracker if issues enabled' do - expect(project.can_have_issues_tracker_id?).to be_falsey - end - - it 'should be always false if issues disabled' do - project.issues_enabled = false - ext_project.issues_enabled = false - - expect(project.can_have_issues_tracker_id?).to be_falsey - expect(ext_project.can_have_issues_tracker_id?).to be_falsey - end - end - describe :open_branches do let(:project) { create(:project) } -- cgit v1.2.1 From 4c716de450ecc2959022c67f99f188fff540c6c4 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 10 Jun 2016 16:53:48 -0300 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 7a6a14919da..3387394de5b 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -72,6 +72,7 @@ v 8.9.0 (unreleased) - Cache on the database if a project has an active external issue tracker. - Put project Labels and Milestones pages links under Issues and Merge Requests tabs as subnav - All classes in the Banzai::ReferenceParser namespace are now instrumented + - Remove deprecated issues_tracker and issues_tracker_id from project model v 8.8.5 (unreleased) - Ensure branch cleanup regardless of whether the GitHub import process succeeds -- cgit v1.2.1 From af8500f43010f42176b2ec1814f0fe7248258b05 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 6 Jun 2016 15:56:04 -0300 Subject: Allow users to create confidential issues in private projects --- app/views/shared/issuable/_form.html.haml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index 17e2a7e9290..d503026f913 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -35,7 +35,7 @@ .clearfix .error-alert -- if issuable.is_a?(Issue) && !issuable.project.private? +- if issuable.is_a?(Issue) .form-group .col-sm-offset-2.col-sm-10 .checkbox -- cgit v1.2.1 From b56c45675019baaaf47615d51c08d5caa0734ad3 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Mon, 6 Jun 2016 16:13:31 -0300 Subject: Project members with guest role can't access confidential issues --- app/finders/snippets_finder.rb | 2 +- app/models/ability.rb | 2 +- app/models/issue.rb | 10 ++++++++- app/models/note.rb | 2 +- app/models/project_team.rb | 10 +++++++-- app/models/user.rb | 25 ++++++++++++++++------ app/views/shared/issuable/_form.html.haml | 2 +- .../controllers/projects/issues_controller_spec.rb | 19 +++++++++++++++- spec/lib/banzai/filter/redactor_filter_spec.rb | 12 +++++++++++ spec/lib/gitlab/project_search_results_spec.rb | 12 +++++++++++ spec/lib/gitlab/search_results_spec.rb | 16 ++++++++++++++ spec/models/concerns/milestoneish_spec.rb | 14 ++++++++++++ spec/models/event_spec.rb | 6 ++++++ spec/models/note_spec.rb | 15 +++++++++---- spec/models/project_team_spec.rb | 6 ++++++ spec/requests/api/issues_spec.rb | 25 +++++++++++++++++++++- spec/requests/api/milestones_spec.rb | 13 +++++++++++ spec/services/notification_service_spec.rb | 11 ++++++++++ .../services/projects/autocomplete_service_spec.rb | 12 +++++++++++ spec/services/todo_service_spec.rb | 18 ++++++++++++---- 20 files changed, 208 insertions(+), 24 deletions(-) diff --git a/app/finders/snippets_finder.rb b/app/finders/snippets_finder.rb index 01cbf91c658..00ff1611039 100644 --- a/app/finders/snippets_finder.rb +++ b/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) || current_user.admin? + if project.team.member?(current_user) || current_user.admin? snippets else snippets.public_and_internal diff --git a/app/models/ability.rb b/app/models/ability.rb index 44515550d9e..aea946f9224 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -533,7 +533,7 @@ class Ability def filter_confidential_issues_abilities(user, issue, rules) return rules if user.admin? || !issue.confidential? - unless issue.author == user || issue.assignee == user || issue.project.team.member?(user.id) + unless issue.author == user || issue.assignee == user || issue.project.team.member?(user, Gitlab::Access::REPORTER) rules.delete(:admin_issue) rules.delete(:read_issue) rules.delete(:update_issue) diff --git a/app/models/issue.rb b/app/models/issue.rb index 235922710ad..6ecb3535359 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -54,7 +54,15 @@ class Issue < ActiveRecord::Base return where(confidential: false) if user.blank? return all if user.admin? - where('issues.confidential = false OR (issues.confidential = true AND (issues.author_id = :user_id OR issues.assignee_id = :user_id OR issues.project_id IN(:project_ids)))', user_id: user.id, project_ids: user.authorized_projects.select(:id)) + where(' + issues.confidential IS NULL + OR issues.confidential IS FALSE + OR (issues.confidential = TRUE + AND (issues.author_id = :user_id + OR issues.assignee_id = :user_id + OR issues.project_id IN(:project_ids)))', + user_id: user.id, + project_ids: user.authorized_projects(Gitlab::Access::REPORTER).select(:id)) end def self.reference_prefix diff --git a/app/models/note.rb b/app/models/note.rb index 585d8c4ad84..8ce2b6fa538 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -100,7 +100,7 @@ class Note < ActiveRecord::Base OR issues.assignee_id = :user_id OR issues.project_id IN(:project_ids)))', user_id: as_user.id, - project_ids: as_user.authorized_projects.select(:id)) + project_ids: as_user.authorized_projects(Gitlab::Access::REPORTER).select(:id)) else found_notes.where('issues.confidential IS NULL OR issues.confidential IS FALSE') end diff --git a/app/models/project_team.rb b/app/models/project_team.rb index 70a8bbaba65..e29e854860a 100644 --- a/app/models/project_team.rb +++ b/app/models/project_team.rb @@ -131,8 +131,14 @@ class ProjectTeam max_member_access(user.id) == Gitlab::Access::MASTER end - def member?(user_id) - !!find_member(user_id) + def member?(user, min_member_access = nil) + member = !!find_member(user.id) + + if min_member_access + member && max_member_access(user.id) >= min_member_access + else + member + end end def human_max_access(user_id) diff --git a/app/models/user.rb b/app/models/user.rb index 7afbfbf112a..69c1bf4bc3d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -405,8 +405,8 @@ class User < ActiveRecord::Base end # Returns projects user is authorized to access. - def authorized_projects - Project.where("projects.id IN (#{projects_union.to_sql})") + def authorized_projects(min_access_level = nil) + Project.where("projects.id IN (#{projects_union(min_access_level).to_sql})") end def viewable_starred_projects @@ -824,11 +824,22 @@ class User < ActiveRecord::Base private - def projects_union - Gitlab::SQL::Union.new([personal_projects.select(:id), - groups_projects.select(:id), - projects.select(:id), - groups.joins(:shared_projects).select(:project_id)]) + def projects_union(min_access_level = nil) + relations = if min_access_level + scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } } + + [personal_projects.select(:id), + groups_projects.where(members: scope).select(:id), + projects.where(members: scope).select(:id), + groups.joins(:shared_projects).where(members: scope).select(:project_id)] + else + [personal_projects.select(:id), + groups_projects.select(:id), + projects.select(:id), + groups.joins(:shared_projects).select(:project_id)] + end + + Gitlab::SQL::Union.new(relations) end def ci_projects_union diff --git a/app/views/shared/issuable/_form.html.haml b/app/views/shared/issuable/_form.html.haml index d503026f913..c30bdb0ae91 100644 --- a/app/views/shared/issuable/_form.html.haml +++ b/app/views/shared/issuable/_form.html.haml @@ -41,7 +41,7 @@ .checkbox = f.label :confidential do = f.check_box :confidential - This issue is confidential and should only be visible to team members + This issue is confidential and should only be visible to team members with at least Reporter access. - if can?(current_user, :"admin_#{issuable.to_ability_name}", issuable.project) - has_due_date = issuable.has_attribute?(:due_date) diff --git a/spec/controllers/projects/issues_controller_spec.rb b/spec/controllers/projects/issues_controller_spec.rb index 78be7e3dc35..cbaa3e0b7b2 100644 --- a/spec/controllers/projects/issues_controller_spec.rb +++ b/spec/controllers/projects/issues_controller_spec.rb @@ -105,6 +105,15 @@ describe Projects::IssuesController do expect(assigns(:issues)).to eq [issue] end + it 'should not list confidential issues for project members with guest role' do + sign_in(member) + project.team << [member, :guest] + + get_issues + + expect(assigns(:issues)).to eq [issue] + end + it 'should list confidential issues for author' do sign_in(author) get_issues @@ -148,7 +157,7 @@ describe Projects::IssuesController do shared_examples_for 'restricted action' do |http_status| it 'returns 404 for guests' do - sign_out :user + sign_out(:user) go(id: unescaped_parameter_value.to_param) expect(response).to have_http_status :not_found @@ -161,6 +170,14 @@ describe Projects::IssuesController do expect(response).to have_http_status :not_found end + it 'returns 404 for project members with guest role' do + sign_in(member) + project.team << [member, :guest] + go(id: unescaped_parameter_value.to_param) + + expect(response).to have_http_status :not_found + end + it "returns #{http_status[:success]} for author" do sign_in(author) go(id: unescaped_parameter_value.to_param) diff --git a/spec/lib/banzai/filter/redactor_filter_spec.rb b/spec/lib/banzai/filter/redactor_filter_spec.rb index 697d10bbf70..f181125156b 100644 --- a/spec/lib/banzai/filter/redactor_filter_spec.rb +++ b/spec/lib/banzai/filter/redactor_filter_spec.rb @@ -69,6 +69,18 @@ describe Banzai::Filter::RedactorFilter, lib: true do expect(doc.css('a').length).to eq 0 end + it 'removes references for project members with guest role' do + member = create(:user) + project = create(:empty_project, :public) + project.team << [member, :guest] + issue = create(:issue, :confidential, project: project) + + link = reference_link(project: project.id, issue: issue.id, reference_type: 'issue') + doc = filter(link, current_user: member) + + expect(doc.css('a').length).to eq 0 + end + it 'allows references for author' do author = create(:user) project = create(:empty_project, :public) diff --git a/spec/lib/gitlab/project_search_results_spec.rb b/spec/lib/gitlab/project_search_results_spec.rb index db0ff95b4f5..270b89972d7 100644 --- a/spec/lib/gitlab/project_search_results_spec.rb +++ b/spec/lib/gitlab/project_search_results_spec.rb @@ -43,6 +43,18 @@ describe Gitlab::ProjectSearchResults, lib: true do expect(results.issues_count).to eq 1 end + it 'should not list project confidential issues for project members with guest role' do + project.team << [member, :guest] + + results = described_class.new(member, project, query) + issues = results.objects('issues') + + expect(issues).to include issue + expect(issues).not_to include security_issue_1 + expect(issues).not_to include security_issue_2 + expect(results.issues_count).to eq 1 + end + it 'should list project confidential issues for author' do results = described_class.new(author, project, query) issues = results.objects('issues') diff --git a/spec/lib/gitlab/search_results_spec.rb b/spec/lib/gitlab/search_results_spec.rb index f4afe597e8d..1bb444bf34f 100644 --- a/spec/lib/gitlab/search_results_spec.rb +++ b/spec/lib/gitlab/search_results_spec.rb @@ -86,6 +86,22 @@ describe Gitlab::SearchResults do expect(results.issues_count).to eq 1 end + it 'should not list confidential issues for project members with guest role' do + project_1.team << [member, :guest] + project_2.team << [member, :guest] + + results = described_class.new(member, limit_projects, query) + issues = results.objects('issues') + + expect(issues).to include issue + expect(issues).not_to include security_issue_1 + expect(issues).not_to include security_issue_2 + expect(issues).not_to include security_issue_3 + expect(issues).not_to include security_issue_4 + expect(issues).not_to include security_issue_5 + expect(results.issues_count).to eq 1 + end + it 'should list confidential issues for author' do results = described_class.new(author, limit_projects, query) issues = results.objects('issues') diff --git a/spec/models/concerns/milestoneish_spec.rb b/spec/models/concerns/milestoneish_spec.rb index 47c3be673c5..7e9ab8940cf 100644 --- a/spec/models/concerns/milestoneish_spec.rb +++ b/spec/models/concerns/milestoneish_spec.rb @@ -5,6 +5,7 @@ describe Milestone, 'Milestoneish' do let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } + let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:project) { create(:project, :public) } let(:milestone) { create(:milestone, project: project) } @@ -21,6 +22,7 @@ describe Milestone, 'Milestoneish' do before do project.team << [member, :developer] + project.team << [guest, :guest] end describe '#closed_items_count' do @@ -28,6 +30,10 @@ describe Milestone, 'Milestoneish' do expect(milestone.closed_items_count(non_member)).to eq 2 end + it 'should not count confidential issues for project members with guest role' do + expect(milestone.closed_items_count(guest)).to eq 2 + end + it 'should count confidential issues for author' do expect(milestone.closed_items_count(author)).to eq 4 end @@ -50,6 +56,10 @@ describe Milestone, 'Milestoneish' do expect(milestone.total_items_count(non_member)).to eq 4 end + it 'should not count confidential issues for project members with guest role' do + expect(milestone.total_items_count(guest)).to eq 4 + end + it 'should count confidential issues for author' do expect(milestone.total_items_count(author)).to eq 7 end @@ -85,6 +95,10 @@ describe Milestone, 'Milestoneish' do expect(milestone.percent_complete(non_member)).to eq 50 end + it 'should not count confidential issues for project members with guest role' do + expect(milestone.percent_complete(guest)).to eq 50 + end + it 'should count confidential issues for author' do expect(milestone.percent_complete(author)).to eq 57 end diff --git a/spec/models/event_spec.rb b/spec/models/event_spec.rb index b0e76fec693..166a1dc4ddb 100644 --- a/spec/models/event_spec.rb +++ b/spec/models/event_spec.rb @@ -50,6 +50,7 @@ describe Event, models: true do let(:project) { create(:empty_project, :public) } let(:non_member) { create(:user) } let(:member) { create(:user) } + let(:guest) { create(:user) } let(:author) { create(:author) } let(:assignee) { create(:user) } let(:admin) { create(:admin) } @@ -61,6 +62,7 @@ describe Event, models: true do before do project.team << [member, :developer] + project.team << [guest, :guest] end context 'issue event' do @@ -71,6 +73,7 @@ describe Event, models: true do it { expect(event.visible_to_user?(author)).to eq true } it { expect(event.visible_to_user?(assignee)).to eq true } it { expect(event.visible_to_user?(member)).to eq true } + it { expect(event.visible_to_user?(guest)).to eq true } it { expect(event.visible_to_user?(admin)).to eq true } end @@ -81,6 +84,7 @@ describe Event, models: true do it { expect(event.visible_to_user?(author)).to eq true } it { expect(event.visible_to_user?(assignee)).to eq true } it { expect(event.visible_to_user?(member)).to eq true } + it { expect(event.visible_to_user?(guest)).to eq false } it { expect(event.visible_to_user?(admin)).to eq true } end end @@ -93,6 +97,7 @@ describe Event, models: true do it { expect(event.visible_to_user?(author)).to eq true } it { expect(event.visible_to_user?(assignee)).to eq true } it { expect(event.visible_to_user?(member)).to eq true } + it { expect(event.visible_to_user?(guest)).to eq true } it { expect(event.visible_to_user?(admin)).to eq true } end @@ -103,6 +108,7 @@ describe Event, models: true do it { expect(event.visible_to_user?(author)).to eq true } it { expect(event.visible_to_user?(assignee)).to eq true } it { expect(event.visible_to_user?(member)).to eq true } + it { expect(event.visible_to_user?(guest)).to eq false } it { expect(event.visible_to_user?(admin)).to eq true } end end diff --git a/spec/models/note_spec.rb b/spec/models/note_spec.rb index f15e96714b2..285ab19cfaf 100644 --- a/spec/models/note_spec.rb +++ b/spec/models/note_spec.rb @@ -162,16 +162,23 @@ describe Note, models: true do end context "confidential issues" do - let(:user) { create :user } - let(:confidential_issue) { create(:issue, :confidential, author: user) } - let(:confidential_note) { create :note, note: "Random", noteable: confidential_issue, project: confidential_issue.project } + let(:user) { create(:user) } + let(:project) { create(:project) } + let(:confidential_issue) { create(:issue, :confidential, project: project, author: user) } + let(:confidential_note) { create(:note, note: "Random", noteable: confidential_issue, project: confidential_issue.project) } it "returns notes with matching content if user can see the issue" do expect(described_class.search(confidential_note.note, as_user: user)).to eq([confidential_note]) end it "does not return notes with matching content if user can not see the issue" do - user = create :user + user = create(:user) + expect(described_class.search(confidential_note.note, as_user: user)).to be_empty + end + + it "does not return notes with matching content for project members with guest role" do + user = create(:user) + project.team << [user, :guest] expect(described_class.search(confidential_note.note, as_user: user)).to be_empty end diff --git a/spec/models/project_team_spec.rb b/spec/models/project_team_spec.rb index bacb17a8883..8bebd6a9447 100644 --- a/spec/models/project_team_spec.rb +++ b/spec/models/project_team_spec.rb @@ -29,6 +29,9 @@ describe ProjectTeam, models: true do it { expect(project.team.master?(nonmember)).to be_falsey } it { expect(project.team.member?(nonmember)).to be_falsey } it { expect(project.team.member?(guest)).to be_truthy } + it { expect(project.team.member?(reporter, Gitlab::Access::REPORTER)).to be_truthy } + it { expect(project.team.member?(guest, Gitlab::Access::REPORTER)).to be_falsey } + it { expect(project.team.member?(nonmember, Gitlab::Access::GUEST)).to be_falsey } end end @@ -64,6 +67,9 @@ describe ProjectTeam, models: true do it { expect(project.team.master?(nonmember)).to be_falsey } it { expect(project.team.member?(nonmember)).to be_falsey } it { expect(project.team.member?(guest)).to be_truthy } + it { expect(project.team.member?(guest, Gitlab::Access::MASTER)).to be_truthy } + it { expect(project.team.member?(reporter, Gitlab::Access::MASTER)).to be_falsey } + it { expect(project.team.member?(nonmember, Gitlab::Access::GUEST)).to be_falsey } end end diff --git a/spec/requests/api/issues_spec.rb b/spec/requests/api/issues_spec.rb index bb926172593..59e557c5b2a 100644 --- a/spec/requests/api/issues_spec.rb +++ b/spec/requests/api/issues_spec.rb @@ -5,6 +5,7 @@ describe API::API, api: true do let(:user) { create(:user) } let(:user2) { create(:user) } let(:non_member) { create(:user) } + let(:guest) { create(:user) } let(:author) { create(:author) } let(:assignee) { create(:assignee) } let(:admin) { create(:user, :admin) } @@ -41,7 +42,10 @@ describe API::API, api: true do end let!(:note) { create(:note_on_issue, author: user, project: project, noteable: issue) } - before { project.team << [user, :reporter] } + before do + project.team << [user, :reporter] + project.team << [guest, :guest] + end describe "GET /issues" do context "when unauthenticated" do @@ -144,6 +148,14 @@ describe API::API, api: true do expect(json_response.first['title']).to eq(issue.title) end + it 'should return project issues without confidential issues for project members with guest role' do + get api("#{base_url}/issues", guest) + expect(response.status).to eq(200) + expect(json_response).to be_an Array + expect(json_response.length).to eq(2) + expect(json_response.first['title']).to eq(issue.title) + end + it 'should return project confidential issues for author' do get api("#{base_url}/issues", author) expect(response.status).to eq(200) @@ -278,6 +290,11 @@ describe API::API, api: true do expect(response.status).to eq(404) end + it "should return 404 for project members with guest role" do + get api("/projects/#{project.id}/issues/#{confidential_issue.id}", guest) + expect(response.status).to eq(404) + end + it "should return confidential issue for project members" do get api("/projects/#{project.id}/issues/#{confidential_issue.id}", user) expect(response.status).to eq(200) @@ -413,6 +430,12 @@ describe API::API, api: true do expect(response.status).to eq(403) end + it "should return 403 for project members with guest role" do + put api("/projects/#{project.id}/issues/#{confidential_issue.id}", guest), + title: 'updated title' + expect(response.status).to eq(403) + end + it "should update a confidential issue for project members" do put api("/projects/#{project.id}/issues/#{confidential_issue.id}", user), title: 'updated title' diff --git a/spec/requests/api/milestones_spec.rb b/spec/requests/api/milestones_spec.rb index 241995041bb..0154d1c62cc 100644 --- a/spec/requests/api/milestones_spec.rb +++ b/spec/requests/api/milestones_spec.rb @@ -146,6 +146,7 @@ describe API::API, api: true do 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 @@ -160,6 +161,18 @@ describe API::API, api: true do expect(json_response.map { |issue| issue['id'] }).to include(issue.id, confidential_issue.id) end + it 'does not return confidential issues to team members with guest role' do + member = create(:user) + project.team << [member, :guest] + + get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", member) + + 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 + it 'does not return confidential issues to regular users' do get api("/projects/#{public_project.id}/milestones/#{milestone.id}/issues", create(:user)) diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index b99e02ba678..e871a103d42 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -132,12 +132,14 @@ describe NotificationService, services: true do let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } + let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:confidential_issue) { create(:issue, :confidential, project: project, author: author, assignee: assignee) } let(:note) { create(:note_on_issue, noteable: confidential_issue, project: project, note: "#{author.to_reference} #{assignee.to_reference} #{non_member.to_reference} #{member.to_reference} #{admin.to_reference}") } it 'filters out users that can not read the issue' do project.team << [member, :developer] + project.team << [guest, :guest] expect(SentNotification).to receive(:record).with(confidential_issue, any_args).exactly(4).times @@ -146,6 +148,7 @@ describe NotificationService, services: true do notification.new_note(note) should_not_email(non_member) + should_not_email(guest) should_email(author) should_email(assignee) should_email(member) @@ -322,17 +325,20 @@ describe NotificationService, services: true do let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } + let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } it "emails subscribers of the issue's labels that can read the issue" do project.team << [member, :developer] + project.team << [guest, :guest] label = create(:label, issues: [confidential_issue]) label.toggle_subscription(non_member) label.toggle_subscription(author) label.toggle_subscription(assignee) label.toggle_subscription(member) + label.toggle_subscription(guest) label.toggle_subscription(admin) ActionMailer::Base.deliveries.clear @@ -341,6 +347,7 @@ describe NotificationService, services: true do should_not_email(non_member) should_not_email(author) + should_not_email(guest) should_email(assignee) should_email(member) should_email(admin) @@ -490,6 +497,7 @@ describe NotificationService, services: true do let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } + let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:confidential_issue) { create(:issue, :confidential, project: project, title: 'Confidential issue', author: author, assignee: assignee) } let!(:label_1) { create(:label, issues: [confidential_issue]) } @@ -497,11 +505,13 @@ describe NotificationService, services: true do it "emails subscribers of the issue's labels that can read the issue" do project.team << [member, :developer] + project.team << [guest, :guest] label_2.toggle_subscription(non_member) label_2.toggle_subscription(author) label_2.toggle_subscription(assignee) label_2.toggle_subscription(member) + label_2.toggle_subscription(guest) label_2.toggle_subscription(admin) ActionMailer::Base.deliveries.clear @@ -509,6 +519,7 @@ describe NotificationService, services: true do notification.relabeled_issue(confidential_issue, [label_2], @u_disabled) should_not_email(non_member) + should_not_email(guest) should_email(author) should_email(assignee) should_email(member) diff --git a/spec/services/projects/autocomplete_service_spec.rb b/spec/services/projects/autocomplete_service_spec.rb index 6108c26a78b..0971fec2e9f 100644 --- a/spec/services/projects/autocomplete_service_spec.rb +++ b/spec/services/projects/autocomplete_service_spec.rb @@ -33,6 +33,18 @@ describe Projects::AutocompleteService, services: true do expect(issues.count).to eq 1 end + it 'should not list project confidential issues for project members with guest role' do + project.team << [member, :guest] + + autocomplete = described_class.new(project, non_member) + issues = autocomplete.issues.map(&:iid) + + expect(issues).to include issue.iid + expect(issues).not_to include security_issue_1.iid + expect(issues).not_to include security_issue_2.iid + expect(issues.count).to eq 1 + end + it 'should list project confidential issues for author' do autocomplete = described_class.new(project, author) issues = autocomplete.issues.map(&:iid) diff --git a/spec/services/todo_service_spec.rb b/spec/services/todo_service_spec.rb index 489c920f19f..549a936b060 100644 --- a/spec/services/todo_service_spec.rb +++ b/spec/services/todo_service_spec.rb @@ -5,13 +5,15 @@ describe TodoService, services: true do let(:assignee) { create(:user) } let(:non_member) { create(:user) } let(:member) { create(:user) } + let(:guest) { create(:user) } let(:admin) { create(:admin) } let(:john_doe) { create(:user) } let(:project) { create(:project) } - let(:mentions) { [author, assignee, john_doe, member, non_member, admin].map(&:to_reference).join(' ') } + let(:mentions) { [author, assignee, john_doe, member, guest, non_member, admin].map(&:to_reference).join(' ') } let(:service) { described_class.new } before do + project.team << [guest, :guest] project.team << [author, :developer] project.team << [member, :developer] project.team << [john_doe, :developer] @@ -41,18 +43,20 @@ describe TodoService, services: true do service.new_issue(issue, author) should_create_todo(user: member, target: issue, action: Todo::MENTIONED) + should_create_todo(user: guest, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED) end - it 'does not create todo for non project members when issue is confidential' do + it 'does not create todo if user can not see the issue when issue is confidential' do service.new_issue(confidential_issue, john_doe) should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::ASSIGNED) should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) end @@ -81,6 +85,7 @@ describe TodoService, services: true do service.update_issue(issue, author) should_create_todo(user: member, target: issue, action: Todo::MENTIONED) + should_create_todo(user: guest, target: issue, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: author, target: issue, action: Todo::MENTIONED) should_not_create_todo(user: non_member, target: issue, action: Todo::MENTIONED) @@ -92,13 +97,14 @@ describe TodoService, services: true do expect { service.update_issue(issue, author) }.not_to change(member.todos, :count) end - it 'does not create todo for non project members when issue is confidential' do + it 'does not create todo if user can not see the issue when issue is confidential' do service.update_issue(confidential_issue, john_doe) should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) + should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED) end @@ -192,18 +198,20 @@ describe TodoService, services: true do service.new_note(note, john_doe) should_create_todo(user: member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) + should_create_todo(user: guest, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) should_create_todo(user: author, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) should_not_create_todo(user: john_doe, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) should_not_create_todo(user: non_member, target: issue, author: john_doe, action: Todo::MENTIONED, note: note) end - it 'does not create todo for non project members when leaving a note on a confidential issue' do + it 'does not create todo if user can not see the issue when leaving a note on a confidential issue' do service.new_note(note_on_confidential_issue, john_doe) should_create_todo(user: author, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_create_todo(user: assignee, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_create_todo(user: member, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_create_todo(user: admin, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) + should_not_create_todo(user: guest, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) should_not_create_todo(user: john_doe, target: confidential_issue, author: john_doe, action: Todo::MENTIONED, note: note_on_confidential_issue) end @@ -245,6 +253,7 @@ describe TodoService, services: true do service.new_merge_request(mr_assigned, author) should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED) + should_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED) @@ -256,6 +265,7 @@ describe TodoService, services: true do service.update_merge_request(mr_assigned, author) should_create_todo(user: member, target: mr_assigned, action: Todo::MENTIONED) + should_create_todo(user: guest, target: mr_assigned, action: Todo::MENTIONED) should_create_todo(user: john_doe, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: author, target: mr_assigned, action: Todo::MENTIONED) should_not_create_todo(user: non_member, target: mr_assigned, action: Todo::MENTIONED) -- cgit v1.2.1 From 149176758393c8d89d996c62c11511b3e86b3f8d Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Fri, 10 Jun 2016 11:26:47 -0300 Subject: Use Issue.visible_to_user in Notes.search to avoid query duplication --- app/models/issue.rb | 2 +- app/models/note.rb | 19 +++---------------- app/models/user.rb | 23 ++++++++++------------- 3 files changed, 14 insertions(+), 30 deletions(-) diff --git a/app/models/issue.rb b/app/models/issue.rb index 6ecb3535359..1bdf9c011b2 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -51,7 +51,7 @@ class Issue < ActiveRecord::Base end def self.visible_to_user(user) - return where(confidential: false) if user.blank? + return where('issues.confidential IS NULL OR issues.confidential IS FALSE') if user.blank? return all if user.admin? where(' diff --git a/app/models/note.rb b/app/models/note.rb index 8ce2b6fa538..58133f1581f 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -88,22 +88,9 @@ class Note < ActiveRecord::Base table = arel_table pattern = "%#{query}%" - found_notes = joins('LEFT JOIN issues ON issues.id = noteable_id'). - where(table[:note].matches(pattern)) - - if as_user - found_notes.where(' - issues.confidential IS NULL - OR issues.confidential IS FALSE - OR (issues.confidential IS TRUE - AND (issues.author_id = :user_id - OR issues.assignee_id = :user_id - OR issues.project_id IN(:project_ids)))', - user_id: as_user.id, - project_ids: as_user.authorized_projects(Gitlab::Access::REPORTER).select(:id)) - else - found_notes.where('issues.confidential IS NULL OR issues.confidential IS FALSE') - end + Note.joins('LEFT JOIN issues ON issues.id = noteable_id'). + where(table[:note].matches(pattern)). + merge(Issue.visible_to_user(as_user)) end end diff --git a/app/models/user.rb b/app/models/user.rb index 69c1bf4bc3d..a5b3c8afe51 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -825,19 +825,16 @@ class User < ActiveRecord::Base private def projects_union(min_access_level = nil) - relations = if min_access_level - scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } } - - [personal_projects.select(:id), - groups_projects.where(members: scope).select(:id), - projects.where(members: scope).select(:id), - groups.joins(:shared_projects).where(members: scope).select(:project_id)] - else - [personal_projects.select(:id), - groups_projects.select(:id), - projects.select(:id), - groups.joins(:shared_projects).select(:project_id)] - end + relations = [personal_projects.select(:id), + groups_projects.select(:id), + projects.select(:id), + groups.joins(:shared_projects).select(:project_id)] + + + if min_access_level + scope = { access_level: Gitlab::Access.values.select { |access| access >= min_access_level } } + relations = [relations.shift] + relations.map { |relation| relation.where(members: scope) } + end Gitlab::SQL::Union.new(relations) end -- cgit v1.2.1 From 6789d2ebf37d5f0537bea72ba99d3b7711e70728 Mon Sep 17 00:00:00 2001 From: Douglas Barbosa Alexandre Date: Wed, 8 Jun 2016 16:09:03 -0300 Subject: Update CHANGELOG --- CHANGELOG | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG b/CHANGELOG index 3387394de5b..92666e63259 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -73,6 +73,7 @@ v 8.9.0 (unreleased) - Put project Labels and Milestones pages links under Issues and Merge Requests tabs as subnav - All classes in the Banzai::ReferenceParser namespace are now instrumented - Remove deprecated issues_tracker and issues_tracker_id from project model + - Allow users to create confidential issues in private projects v 8.8.5 (unreleased) - Ensure branch cleanup regardless of whether the GitHub import process succeeds -- cgit v1.2.1 From 84b07f7054bca5820bc54a99014538506718201e Mon Sep 17 00:00:00 2001 From: Benjamin Schmid Date: Thu, 19 May 2016 17:08:10 +0200 Subject: Honor credentials on calling Bamboo CI trigger MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This improves the Bamboo Service and provides a fix for situations, where the build trigger won't work, because Bamboo is requiring authentication also for the trigger GET. The change now does provide additional HTTP Basic Auth parameters if user credentials were provided and appends an request parameter indicating the HTTP Basic Authentication should be used. This aligns interaction with Bamboo with the other calls this service executes. Signed-off-by: Rémy Coutable --- app/models/project_services/bamboo_service.rb | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index 1d1780dcfbf..8c9c52ac7d1 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/app/models/project_services/bamboo_service.rb @@ -1,6 +1,4 @@ class BambooService < CiService - include HTTParty - prop_accessor :bamboo_url, :build_key, :username, :password validates :bamboo_url, presence: true, url: true, if: :activated? @@ -112,8 +110,19 @@ class BambooService < CiService def execute(data) return unless supported_events.include?(data[:object_kind]) - # Bamboo requires a GET and does not take any data. + # Bamboo requires a GET and does take authentification url = URI.join(bamboo_url, "/updateAndBuild.action?buildKey=#{build_key}").to_s - self.class.get(url, verify: false) + + if username.blank? && password.blank? + HTTParty.get(url, verify: false) + else + url << '&os_authType=basic' + auth = { + username: username, + password: password + } + HTTParty.get(url, verify: false, basic_auth: auth) + end + end end -- cgit v1.2.1 From 46f3cd7c65b871d4efa6c33fbfccbc01fdf36649 Mon Sep 17 00:00:00 2001 From: Benjamin Schmid Date: Mon, 30 May 2016 12:30:35 +0200 Subject: Fix broken URI joining for `bamboo_url` with suffixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If one had configured a `bamboo_url` like http://foo.bar/bamboo in the previous implementation the plugin directed it's request i.e. to http://foo.bar/rest/... instead of http://foo.bar/bamboo/rest/... `URI.join` only works correctly, if the prefix URL has - at least one or more trailing '/' - the appended parts are _not_ prefixed with '/' The current implementation should work with all sorts of Bamboo base URLs. Signed-off-by: Rémy Coutable --- CHANGELOG | 1 + app/models/project_services/bamboo_service.rb | 8 ++++---- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 3387394de5b..bba6991f184 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -38,6 +38,7 @@ v 8.9.0 (unreleased) - Add option to project to only allow merge requests to be merged if the build succeeds (Rui Santos) - Fix issues filter when ordering by milestone - Added artifacts:when to .gitlab-ci.yml - this requires GitLab Runner 1.3 + - Bamboo Service: Fix missing credentials & URL handling when base URL contains a path (Benjamin Schmid) - Todos will display target state if issuable target is 'Closed' or 'Merged' - Fix bug when sorting issues by milestone due date and filtering by two or more labels - Add support for using Yubikeys (U2F) for two-factor authentication diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index 8c9c52ac7d1..cb215b595f5 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/app/models/project_services/bamboo_service.rb @@ -59,7 +59,7 @@ class BambooService < CiService end def build_info(sha) - url = URI.join(bamboo_url, "/rest/api/latest/result?label=#{sha}").to_s + url = URI.join("#{bamboo_url}/", "rest/api/latest/result?label=#{sha}").to_s if username.blank? && password.blank? @response = HTTParty.get(url, verify: false) @@ -78,11 +78,11 @@ class BambooService < CiService if @response.code != 200 || @response['results']['results']['size'] == '0' # If actual build link can't be determined, send user to build summary page. - URI.join(bamboo_url, "/browse/#{build_key}").to_s + URI.join("#{bamboo_url}/", "browse/#{build_key}").to_s else # If actual build link is available, go to build result page. result_key = @response['results']['results']['result']['planResultKey']['key'] - URI.join(bamboo_url, "/browse/#{result_key}").to_s + URI.join("#{bamboo_url}/", "browse/#{result_key}").to_s end end @@ -111,7 +111,7 @@ class BambooService < CiService return unless supported_events.include?(data[:object_kind]) # Bamboo requires a GET and does take authentification - url = URI.join(bamboo_url, "/updateAndBuild.action?buildKey=#{build_key}").to_s + url = URI.join("#{bamboo_url}/", "updateAndBuild.action?buildKey=#{build_key}").to_s if username.blank? && password.blank? HTTParty.get(url, verify: false) -- cgit v1.2.1 From 17c32ee8d0b2dafa61b3f509d48f7ee8a8dbea14 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 1 Jun 2016 16:43:40 +0200 Subject: Factorize duplicated code into a method in BambooService and update specs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/project_services/bamboo_service.rb | 37 ++++++++++------------ .../models/project_services/bamboo_service_spec.rb | 12 +++---- 2 files changed, 23 insertions(+), 26 deletions(-) diff --git a/app/models/project_services/bamboo_service.rb b/app/models/project_services/bamboo_service.rb index cb215b595f5..b5c76e4d4fe 100644 --- a/app/models/project_services/bamboo_service.rb +++ b/app/models/project_services/bamboo_service.rb @@ -59,18 +59,7 @@ class BambooService < CiService end def build_info(sha) - url = URI.join("#{bamboo_url}/", "rest/api/latest/result?label=#{sha}").to_s - - if username.blank? && password.blank? - @response = HTTParty.get(url, verify: false) - else - url << '&os_authType=basic' - auth = { - username: username, - password: password - } - @response = HTTParty.get(url, verify: false, basic_auth: auth) - end + @response = get_path("rest/api/latest/result?label=#{sha}") end def build_page(sha, ref) @@ -110,19 +99,27 @@ class BambooService < CiService def execute(data) return unless supported_events.include?(data[:object_kind]) - # Bamboo requires a GET and does take authentification - url = URI.join("#{bamboo_url}/", "updateAndBuild.action?buildKey=#{build_key}").to_s + get_path("updateAndBuild.action?buildKey=#{build_key}") + end + + private + + def build_url(path) + URI.join("#{bamboo_url}/", path).to_s + end + + def get_path(path) + url = build_url(path) if username.blank? && password.blank? HTTParty.get(url, verify: false) else url << '&os_authType=basic' - auth = { - username: username, - password: password - } - HTTParty.get(url, verify: false, basic_auth: auth) + HTTParty.get(url, verify: false, + basic_auth: { + username: username, + password: password + }) end - end end diff --git a/spec/models/project_services/bamboo_service_spec.rb b/spec/models/project_services/bamboo_service_spec.rb index ec81f05fc7a..9ae461f8c2d 100644 --- a/spec/models/project_services/bamboo_service_spec.rb +++ b/spec/models/project_services/bamboo_service_spec.rb @@ -126,25 +126,25 @@ describe BambooService, models: true do it 'returns a specific URL when status is 500' do stub_request(status: 500) - expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo') + expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/foo') end it 'returns a specific URL when response has no results' do stub_request(body: %Q({"results":{"results":{"size":"0"}}})) - expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/browse/foo') + expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/foo') end it 'returns a build URL when bamboo_url has no trailing slash' do stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}})) - expect(service(bamboo_url: 'http://gitlab.com').build_page('123', 'unused')).to eq('http://gitlab.com/browse/42') + expect(service(bamboo_url: 'http://gitlab.com/bamboo').build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/42') end it 'returns a build URL when bamboo_url has a trailing slash' do stub_request(body: %Q({"results":{"results":{"result":{"planResultKey":{"key":"42"}}}}})) - expect(service(bamboo_url: 'http://gitlab.com/').build_page('123', 'unused')).to eq('http://gitlab.com/browse/42') + expect(service(bamboo_url: 'http://gitlab.com/bamboo/').build_page('123', 'unused')).to eq('http://gitlab.com/bamboo/browse/42') end end @@ -192,7 +192,7 @@ describe BambooService, models: true do end end - def service(bamboo_url: 'http://gitlab.com') + def service(bamboo_url: 'http://gitlab.com/bamboo') described_class.create( project: create(:empty_project), properties: { @@ -205,7 +205,7 @@ describe BambooService, models: true do end def stub_request(status: 200, body: nil, build_state: 'success') - bamboo_full_url = 'http://mic:password@gitlab.com/rest/api/latest/result?label=123&os_authType=basic' + bamboo_full_url = 'http://mic:password@gitlab.com/bamboo/rest/api/latest/result?label=123&os_authType=basic' body ||= %Q({"results":{"results":{"result":{"buildState":"#{build_state}"}}}}) WebMock.stub_request(:get, bamboo_full_url).to_return( -- cgit v1.2.1 From 2f7b2057f25d4390d063e4c4fce3f4f12ea58463 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 1 Jun 2016 16:44:39 +0200 Subject: Fix broken URI joining for `teamcity_url` with suffixes MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If one had configured a `teamcity_url` like http://foo.bar/teamcity in the previous implementation the plugin directed it's request i.e. to http://foo.bar/httpAuth/... instead of http://foo.bar/teamcity/httpAuth/... `URI.join` only works correctly, if the prefix URL has - at least one or more trailing '/' - the appended parts are _not_ prefixed with '/' The current implementation should work with all sorts of TeamCity base URLs. Signed-off-by: Rémy Coutable --- CHANGELOG | 1 + app/models/project_services/teamcity_service.rb | 37 +++++++++++----------- .../project_services/teamcity_service_spec.rb | 10 +++--- 3 files changed, 25 insertions(+), 23 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index bba6991f184..3d46901ba69 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -39,6 +39,7 @@ v 8.9.0 (unreleased) - Fix issues filter when ordering by milestone - Added artifacts:when to .gitlab-ci.yml - this requires GitLab Runner 1.3 - Bamboo Service: Fix missing credentials & URL handling when base URL contains a path (Benjamin Schmid) + - TeamCity Service: Fix URL handling when base URL contains a path - Todos will display target state if issuable target is 'Closed' or 'Merged' - Fix bug when sorting issues by milestone due date and filtering by two or more labels - Add support for using Yubikeys (U2F) for two-factor authentication diff --git a/app/models/project_services/teamcity_service.rb b/app/models/project_services/teamcity_service.rb index b0dcb52eba1..a4a967c9bc9 100644 --- a/app/models/project_services/teamcity_service.rb +++ b/app/models/project_services/teamcity_service.rb @@ -1,6 +1,4 @@ class TeamcityService < CiService - include HTTParty - prop_accessor :teamcity_url, :build_type, :username, :password validates :teamcity_url, presence: true, url: true, if: :activated? @@ -64,15 +62,7 @@ class TeamcityService < CiService end def build_info(sha) - url = URI.join( - teamcity_url, - "/httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}" - ).to_s - auth = { - username: username, - password: password - } - @response = HTTParty.get(url, verify: false, basic_auth: auth) + @response = get_path("httpAuth/app/rest/builds/branch:unspecified:any,number:#{sha}") end def build_page(sha, ref) @@ -81,14 +71,11 @@ class TeamcityService < CiService if @response.code != 200 # If actual build link can't be determined, # send user to build summary page. - URI.join(teamcity_url, "/viewLog.html?buildTypeId=#{build_type}").to_s + build_url("viewLog.html?buildTypeId=#{build_type}") else # If actual build link is available, go to build result page. built_id = @response['build']['id'] - URI.join( - teamcity_url, - "/viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}" - ).to_s + build_url("viewLog.html?buildId=#{built_id}&buildTypeId=#{build_type}") end end @@ -123,8 +110,8 @@ class TeamcityService < CiService branch = Gitlab::Git.ref_name(data[:ref]) - self.class.post( - URI.join(teamcity_url, '/httpAuth/app/rest/buildQueue').to_s, + HTTParty.post( + build_url('httpAuth/app/rest/buildQueue'), body: ""\ ""\ '', @@ -132,4 +119,18 @@ class TeamcityService < CiService basic_auth: auth ) end + + private + + def build_url(path) + URI.join("#{teamcity_url}/", path).to_s + end + + def get_path(path) + HTTParty.get(build_url(path), verify: false, + basic_auth: { + username: username, + password: password + }) + end end diff --git a/spec/models/project_services/teamcity_service_spec.rb b/spec/models/project_services/teamcity_service_spec.rb index 24a708ca849..474715d24c3 100644 --- a/spec/models/project_services/teamcity_service_spec.rb +++ b/spec/models/project_services/teamcity_service_spec.rb @@ -126,19 +126,19 @@ describe TeamcityService, models: true do it 'returns a specific URL when status is 500' do stub_request(status: 500) - expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildTypeId=foo') + expect(service.build_page('123', 'unused')).to eq('http://gitlab.com/teamcity/viewLog.html?buildTypeId=foo') end it 'returns a build URL when teamcity_url has no trailing slash' do stub_request(body: %Q({"build":{"id":"666"}})) - expect(service(teamcity_url: 'http://gitlab.com').build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo') + expect(service(teamcity_url: 'http://gitlab.com/teamcity').build_page('123', 'unused')).to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo') end it 'returns a build URL when teamcity_url has a trailing slash' do stub_request(body: %Q({"build":{"id":"666"}})) - expect(service(teamcity_url: 'http://gitlab.com/').build_page('123', 'unused')).to eq('http://gitlab.com/viewLog.html?buildId=666&buildTypeId=foo') + expect(service(teamcity_url: 'http://gitlab.com/teamcity/').build_page('123', 'unused')).to eq('http://gitlab.com/teamcity/viewLog.html?buildId=666&buildTypeId=foo') end end @@ -180,7 +180,7 @@ describe TeamcityService, models: true do end end - def service(teamcity_url: 'http://gitlab.com') + def service(teamcity_url: 'http://gitlab.com/teamcity') described_class.create( project: create(:empty_project), properties: { @@ -193,7 +193,7 @@ describe TeamcityService, models: true do end def stub_request(status: 200, body: nil, build_status: 'success') - teamcity_full_url = 'http://mic:password@gitlab.com/httpAuth/app/rest/builds/branch:unspecified:any,number:123' + teamcity_full_url = 'http://mic:password@gitlab.com/teamcity/httpAuth/app/rest/builds/branch:unspecified:any,number:123' body ||= %Q({"build":{"status":"#{build_status}","id":"666"}}) WebMock.stub_request(:get, teamcity_full_url).to_return( -- cgit v1.2.1