Add latest changes from gitlab-org/gitlab@master

app/models/merge_request.rb

@@ -80,6 +80,8 @@ class MergeRequest < ApplicationRecord
 
   has_many :merge_request_assignees
   has_many :assignees, class_name: "User", through: :merge_request_assignees
+  has_many :merge_request_reviewers
+  has_many :reviewers, class_name: "User", through: :merge_request_reviewers
   has_many :user_mentions, class_name: "MergeRequestUserMention", dependent: :delete_all # rubocop:disable Cop/ActiveRecordDependent
 
   has_many :deployment_merge_requests

app/models/merge_request_reviewer.rb

+# frozen_string_literal: true
+
+class MergeRequestReviewer < ApplicationRecord
+  belongs_to :merge_request
+  belongs_to :reviewer, class_name: "User", foreign_key: :user_id, inverse_of: :merge_request_assignees
+end

changelogs/unreleased/add-reviewers-to-merge-requests.yml

+---
+title: Add merge_request_reviewers table
+merge_request: 40358
+author:
+type: added

changelogs/unreleased/sh-same-site-none-fix-try2.yml

+---
+title: Don't send SameSite=None to incompatible browsers
+merge_request: 40667
+author:
+type: fixed

changelogs/unreleased/store-seats_currently_in_use.yml

+---
+title: Add seats related columns for easier data analysis
+merge_request: 40470
+author:
+type: other

db/migrate/20200825071735_create_merge_request_reviewers.rb

+# frozen_string_literal: true
+
+class CreateMergeRequestReviewers < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    create_table :merge_request_reviewers do |t|
+      t.bigint :user_id, null: false
+      t.bigint :merge_request_id, null: false
+      t.datetime_with_timezone :created_at, null: false
+    end
+
+    add_index :merge_request_reviewers, [:merge_request_id, :user_id], unique: true
+    add_index :merge_request_reviewers, :user_id
+  end
+
+  def down
+    drop_table :merge_request_reviewers
+  end
+end

db/migrate/20200827060911_add_merge_request_foreign_key_to_merge_request_reviewers.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddMergeRequestForeignKeyToMergeRequestReviewers < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_foreign_key :merge_request_reviewers, :merge_requests, column: :merge_request_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_request_reviewers, column: :merge_request_id
+    end
+  end
+end

db/migrate/20200827060932_add_user_foreign_key_to_merge_request_reviewers.rb

+# frozen_string_literal: true
+
+# See https://docs.gitlab.com/ee/development/migration_style_guide.html
+# for more information on how to write migrations for GitLab.
+
+class AddUserForeignKeyToMergeRequestReviewers < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_foreign_key :merge_request_reviewers, :users, column: :user_id, on_delete: :cascade # rubocop:disable Migration/AddConcurrentForeignKey
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_foreign_key :merge_request_reviewers, column: :user_id
+    end
+  end
+end

db/migrate/20200827085101_add_seats_currently_in_use_in_gitlab_subscriptions.rb

+# frozen_string_literal: true
+
+class AddSeatsCurrentlyInUseInGitlabSubscriptions < ActiveRecord::Migration[6.0]
+  include Gitlab::Database::MigrationHelpers
+
+  DOWNTIME = false
+
+  def up
+    with_lock_retries do
+      add_column :gitlab_subscriptions, :seats_in_use, :integer, default: 0, null: false
+      add_column :gitlab_subscriptions, :seats_owed, :integer, default: 0, null: false
+    end
+  end
+
+  def down
+    with_lock_retries do
+      remove_column :gitlab_subscriptions, :seats_in_use
+      remove_column :gitlab_subscriptions, :seats_owed
+    end
+  end
+end

db/schema_migrations/20200825071735

+cd8574318fae1f2bb021b53d4e453c6b64c763f0e7cc8836cdb8b12963ff0e18
\ No newline at end of file

db/schema_migrations/20200827060911

+124f6ba79f71e2de510741b22d3dd5cf15378b5476c759484bd814377a644256
\ No newline at end of file

db/schema_migrations/20200827060932

+ba2e32b4836062631308937023470d31b3f808b468999ba15374c3b953377402
\ No newline at end of file

db/schema_migrations/20200827085101

+bdcfd7dbccc73d6e2754d67bf099cc2dd2b919e4125cec47e853ff91911cc82e
\ No newline at end of file

db/structure.sql

@@ -12133,7 +12133,9 @@ CREATE TABLE public.gitlab_subscriptions (
     seats integer DEFAULT 0,
     trial boolean DEFAULT false,
     trial_starts_on date,
-    auto_renew boolean
+    auto_renew boolean,
+    seats_in_use integer DEFAULT 0 NOT NULL,
+    seats_owed integer DEFAULT 0 NOT NULL
 );
 
 CREATE SEQUENCE public.gitlab_subscriptions_id_seq
@@ -13159,6 +13161,22 @@ CREATE SEQUENCE public.merge_request_metrics_id_seq
 
 ALTER SEQUENCE public.merge_request_metrics_id_seq OWNED BY public.merge_request_metrics.id;
 
+CREATE TABLE public.merge_request_reviewers (
+    id bigint NOT NULL,
+    user_id bigint NOT NULL,
+    merge_request_id bigint NOT NULL,
+    created_at timestamp with time zone NOT NULL
+);
+
+CREATE SEQUENCE public.merge_request_reviewers_id_seq
+    START WITH 1
+    INCREMENT BY 1
+    NO MINVALUE
+    NO MAXVALUE
+    CACHE 1;
+
+ALTER SEQUENCE public.merge_request_reviewers_id_seq OWNED BY public.merge_request_reviewers.id;
+
 CREATE TABLE public.merge_request_user_mentions (
     id bigint NOT NULL,
     merge_request_id integer NOT NULL,
@@ -17142,6 +17160,8 @@ ALTER TABLE ONLY public.merge_request_diffs ALTER COLUMN id SET DEFAULT nextval(
 
 ALTER TABLE ONLY public.merge_request_metrics ALTER COLUMN id SET DEFAULT nextval('public.merge_request_metrics_id_seq'::regclass);
 
+ALTER TABLE ONLY public.merge_request_reviewers ALTER COLUMN id SET DEFAULT nextval('public.merge_request_reviewers_id_seq'::regclass);
+
 ALTER TABLE ONLY public.merge_request_user_mentions ALTER COLUMN id SET DEFAULT nextval('public.merge_request_user_mentions_id_seq'::regclass);
 
 ALTER TABLE ONLY public.merge_requests ALTER COLUMN id SET DEFAULT nextval('public.merge_requests_id_seq'::regclass);
@@ -18260,6 +18280,9 @@ ALTER TABLE ONLY public.merge_request_diffs
 ALTER TABLE ONLY public.merge_request_metrics
     ADD CONSTRAINT merge_request_metrics_pkey PRIMARY KEY (id);
 
+ALTER TABLE ONLY public.merge_request_reviewers
+    ADD CONSTRAINT merge_request_reviewers_pkey PRIMARY KEY (id);
+
 ALTER TABLE ONLY public.merge_request_user_mentions
     ADD CONSTRAINT merge_request_user_mentions_pkey PRIMARY KEY (id);
 
@@ -20091,6 +20114,10 @@ CREATE INDEX index_merge_request_metrics_on_target_project_id ON public.merge_re
 
 CREATE INDEX index_merge_request_metrics_on_target_project_id_merged_at ON public.merge_request_metrics USING btree (target_project_id, merged_at);
 
+CREATE UNIQUE INDEX index_merge_request_reviewers_on_merge_request_id_and_user_id ON public.merge_request_reviewers USING btree (merge_request_id, user_id);
+
+CREATE INDEX index_merge_request_reviewers_on_user_id ON public.merge_request_reviewers USING btree (user_id);
+
 CREATE UNIQUE INDEX index_merge_request_user_mentions_on_note_id ON public.merge_request_user_mentions USING btree (note_id) WHERE (note_id IS NOT NULL);
 
 CREATE INDEX index_merge_requests_closing_issues_on_issue_id ON public.merge_requests_closing_issues USING btree (issue_id);
@@ -22280,6 +22307,9 @@ ALTER TABLE ONLY public.board_labels
 ALTER TABLE ONLY public.merge_request_blocks
     ADD CONSTRAINT fk_rails_364d4bea8b FOREIGN KEY (blocked_merge_request_id) REFERENCES public.merge_requests(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.merge_request_reviewers
+    ADD CONSTRAINT fk_rails_3704a66140 FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.analytics_cycle_analytics_project_stages
     ADD CONSTRAINT fk_rails_3829e49b66 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 
@@ -23054,6 +23084,9 @@ ALTER TABLE ONLY public.alert_management_alert_assignees
 ALTER TABLE ONLY public.geo_hashed_storage_attachments_events
     ADD CONSTRAINT fk_rails_d496b088e9 FOREIGN KEY (project_id) REFERENCES public.projects(id) ON DELETE CASCADE;
 
+ALTER TABLE ONLY public.merge_request_reviewers
+    ADD CONSTRAINT fk_rails_d9fec24b9d FOREIGN KEY (merge_request_id) REFERENCES public.merge_requests(id) ON DELETE CASCADE;
+
 ALTER TABLE ONLY public.jira_imports
     ADD CONSTRAINT fk_rails_da617096ce FOREIGN KEY (user_id) REFERENCES public.users(id) ON DELETE SET NULL;
 

doc/development/migration_style_guide.md

@@ -369,6 +369,7 @@ migration involves one of the high-traffic tables:
 - `users`
 - `projects`
 - `namespaces`
+- `gitlab_subscriptions`
 - `issues`
 - `merge_requests`
 - `ci_pipelines`

lib/gitlab/middleware/same_site_cookies.rb

@@ -30,6 +30,7 @@ module Gitlab
         set_cookie = headers['Set-Cookie']&.strip
 
         return result if set_cookie.blank? || !ssl?
+        return result if same_site_none_incompatible?(headers['User-Agent'])
 
         cookies = set_cookie.split(COOKIE_SEPARATOR)
 
@@ -39,11 +40,11 @@ module Gitlab
           # Chrome will drop SameSite=None cookies without the Secure
           # flag. If we remove this middleware, we may need to ensure
           # that all cookies set this flag.
-          if ssl? && !(cookie =~ /;\s*secure/i)
+          unless SECURE_REGEX.match?(cookie)
             cookie << '; Secure'
           end
 
-          unless cookie =~ /;\s*samesite=/i
+          unless SAME_SITE_REGEX.match?(cookie)
             cookie << '; SameSite=None'
           end
         end
@@ -55,9 +56,93 @@ module Gitlab
 
       private
 
+      # Taken from https://www.chromium.org/updates/same-site/incompatible-clients
+      # We use RE2 instead of the browser gem for performance.
+      IOS_REGEX = RE2('\(iP.+; CPU .*OS (\d+)[_\d]*.*\) AppleWebKit\/')
+      MACOS_REGEX = RE2('\(Macintosh;.*Mac OS X (\d+)_(\d+)[_\d]*.*\) AppleWebKit\/')
+      SAFARI_REGEX = RE2('Version\/.* Safari\/')
+      CHROMIUM_REGEX = RE2('Chrom(e|ium)')
+      CHROMIUM_VERSION_REGEX = RE2('Chrom[^ \/]+\/(\d+)')
+      UC_BROWSER_REGEX = RE2('UCBrowser\/')
+      UC_BROWSER_VERSION_REGEX = RE2('UCBrowser\/(\d+)\.(\d+)\.(\d+)')
+
+      SECURE_REGEX = RE2(';\s*secure', case_sensitive: false)
+      SAME_SITE_REGEX = RE2(';\s*samesite=', case_sensitive: false)
+
       def ssl?
         Gitlab.config.gitlab.https
       end
+
+      def same_site_none_incompatible?(user_agent)
+        return false if user_agent.blank?
+
+        has_webkit_same_site_bug?(user_agent) || drops_unrecognized_same_site_cookies?(user_agent)
+      end
+
+      def has_webkit_same_site_bug?(user_agent)
+        ios_version?(12, user_agent) ||
+          (macos_version?(10, 14, user_agent) && safari?(user_agent))
+      end
+
+      def drops_unrecognized_same_site_cookies?(user_agent)
+        if uc_browser?(user_agent)
+          return !uc_browser_version_at_least?(12, 13, 2, user_agent)
+        end
+
+        chromium_based?(user_agent) && chromium_version_between?(51, 66, user_agent)
+      end
+
+      def ios_version?(major, user_agent)
+        m = IOS_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        m[1].to_i == major
+      end
+
+      def macos_version?(major, minor, user_agent)
+        m = MACOS_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        m[1].to_i == major && m[2].to_i == minor
+      end
+
+      def safari?(user_agent)
+        SAFARI_REGEX.match?(user_agent)
+      end
+
+      def chromium_based?(user_agent)
+        CHROMIUM_REGEX.match?(user_agent)
+      end
+
+      def chromium_version_between?(from_major, to_major, user_agent)
+        m = CHROMIUM_VERSION_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        version = m[1].to_i
+        version >= from_major && version <= to_major
+      end
+
+      def uc_browser?(user_agent)
+        UC_BROWSER_REGEX.match?(user_agent)
+      end
+
+      def uc_browser_version_at_least?(major, minor, build, user_agent)
+        m = UC_BROWSER_VERSION_REGEX.match(user_agent)
+
+        return false if m.nil?
+
+        major_version = m[1].to_i
+        minor_version = m[2].to_i
+        build_version = m[3].to_i
+
+        return major_version > major if major_version != major
+        return minor_version > minor if minor_version != minor
+
+        build_version >= build
+      end
     end
   end
 end

rubocop/rubocop-migrations.yml

@@ -11,6 +11,7 @@ Migration/UpdateLargeTable:
     - :ci_stages
     - :deployments
     - :events
+    - :gitlab_subscriptions
     - :issues
     - :merge_request_diff_commits
     - :merge_request_diff_files

spec/lib/gitlab/import_export/all_models.yml

@@ -120,6 +120,7 @@ merge_requests:
 - award_emoji
 - author
 - assignee
+- reviewers
 - updated_by
 - milestone
 - iteration
@@ -147,6 +148,7 @@ merge_requests:
 - latest_merge_request_diff
 - pipelines_for_merge_request
 - merge_request_assignees
+- merge_request_reviewers
 - suggestions
 - diff_note_positions
 - unresolved_notes

spec/lib/gitlab/middleware/same_site_cookies_spec.rb

@@ -3,23 +3,30 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Middleware::SameSiteCookies do
+  using RSpec::Parameterized::TableSyntax
   include Rack::Test::Methods
 
+  let(:user_agent) { nil }
   let(:mock_app) do
     Class.new do
-      attr_reader :cookies
+      attr_reader :cookies, :user_agent
 
-      def initialize(cookies)
+      def initialize(cookies, user_agent)
         @cookies = cookies
+        @user_agent = user_agent
       end
 
       def call(env)
-        [200, { 'Set-Cookie' => cookies }, ['OK']]
+        [
+          200,
+          { 'Set-Cookie' => cookies, 'User-Agent' => user_agent }.compact,
+          ['OK']
+        ]
       end
     end
   end
 
-  let(:app) { mock_app.new(cookies) }
+  let(:app) { mock_app.new(cookies, user_agent) }
 
   subject do
     described_class.new(app)
@@ -63,6 +70,42 @@ RSpec.describe Gitlab::Middleware::SameSiteCookies do
         end
       end
 
+      context 'with different browsers' do
+        where(:description, :user_agent, :expected) do
+          "iOS 12" | "Mozilla/5.0 (iPhone; CPU iPhone OS 12_0 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Mobile/15E148 Safari/604.1" | false
+          "macOS 10.14 + Safari" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/12.0 Safari/605.1.15" | false
+          "macOS 10.14 + Opera" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/60.0.3112.78 Safari/537.36 OPR/47.0.2631.55" | false
+          "macOS 10.14 + Chrome v80" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_14_6) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/80.0.3987.87 Safari/537.36" | true
+          "Chrome v41" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/41.0.2227.1 Safari/537.36" | true
+          "Chrome v50" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_1) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/50.0.2348.1 Safari/537.36" | true
+          "Chrome v51" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_0) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2718.15 Safari/537.36" | false
+          "Chrome v66" | "Mozilla/5.0 (Linux; Android 4.4.2; Avvio_793 Build/KOT49H) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/66.0.3359.126 Mobile Safari/537.36" | false
+          "Chrome v67" | "Mozilla/5.0 (Linux; Android 7.1.1; SM-J510F Build/NMF26X) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/67.0.3371.0 Mobile Safari/537.36" | true
+          "Chrome v85" | "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_10_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/85.0.4183.83 Safari/537.36" | true
+          "Chromium v66" | "Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/66.0.3359.181 HeadlessChrome/66.0.3359.181 Safari/537.36" | false
+          "Chromium v85" | "Mozilla/5.0 (X11; Linux aarch64) AppleWebKit/537.36 (KHTML, like Gecko) Ubuntu Chromium/85.0.4183.59 Chrome/85.0.4183.59 Safari/537.36" | true
+          "UC Browser 12.0.4" | "Mozilla/5.0 (Linux; U; Android 4.4.4; zh-CN; A31 Build/KTU84P) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.0.4.986 Mobile Safari/537.36" | false
+          "UC Browser 12.13.0" | "Mozilla/5.0 (Linux; U; Android 7.1.1; en-US; SM-C9000 Build/NMF26X) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.13.0.1207 Mobile Safari/537.36" | false
+          "UC Browser 12.13.2" | "Mozilla/5.0 (Linux; U; Android 9; en-US; Redmi Note 7 Build/PQ3B.190801.002) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.13.2.1208 Mobile Safari/537.36" | true
+          "UC Browser 12.13.5" | "Mozilla/5.0 (Linux; U; Android 5.1.1; en-US; PHICOMM C630 (CLUE L) Build/LMY47V) AppleWebKit/537.36 (KHTML, like Gecko) Version/4.0 Chrome/57.0.2987.108 UCBrowser/12.13.5.1209 Mobile Safari/537.36" | true
+          "Playstation" | "Mozilla/5.0 (PlayStation 4 2.51) AppleWebKit/537.73 (KHTML, like Gecko)" | true
+        end
+
+        with_them do
+          let(:cookies) { "thiscookie=12345" }
+
+          it 'returns expected SameSite status' do
+            response = do_request
+
+            if expected
+              expect(response['Set-Cookie']).to include('SameSite=None')
+            else
+              expect(response['Set-Cookie']).not_to include('SameSite=None')
+            end
+          end
+        end
+      end
+
       context 'with single cookie' do
         let(:cookies) { "thiscookie=12345" }
 

spec/models/merge_request_reviewer_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe MergeRequestReviewer do
+  let(:merge_request) { create(:merge_request) }
+
+  subject { merge_request.merge_request_reviewers.build(reviewer: create(:user)) }
+
+  describe 'associations' do
+    it { is_expected.to belong_to(:merge_request).class_name('MergeRequest') }
+    it { is_expected.to belong_to(:reviewer).class_name('User') }
+  end
+end

spec/models/merge_request_spec.rb

@@ -18,6 +18,7 @@ RSpec.describe MergeRequest do
     it { is_expected.to belong_to(:source_project).class_name('Project') }
     it { is_expected.to belong_to(:merge_user).class_name("User") }
     it { is_expected.to have_many(:assignees).through(:merge_request_assignees) }
+    it { is_expected.to have_many(:reviewers).through(:merge_request_reviewers) }
     it { is_expected.to have_many(:merge_request_diffs) }
     it { is_expected.to have_many(:user_mentions).class_name("MergeRequestUserMention") }
     it { is_expected.to belong_to(:milestone) }