Merge remote-tracking branch 'dev/13-9-stable' into 13-9-stable

6 files changed, 142 insertions, 159 deletions

diff --git a/spec/controllers/groups/variables_controller_spec.rb b/spec/controllers/groups/variables_controller_spec.rb
index e2a14165cb4..a450a4afb02 100644
--- a/spec/controllers/groups/variables_controller_spec.rb
+++ b/spec/controllers/groups/variables_controller_spec.rb
@@ -5,26 +5,35 @@ require 'spec_helper'
 RSpec.describe Groups::VariablesController do
   include ExternalAuthorizationServiceHelpers
 
-  let(:group) { create(:group) }
-  let(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:variable) { create(:ci_group_variable, group: group) }
+  let(:access_level) { :owner }
 
   before do
     sign_in(user)
-    group.add_maintainer(user)
+    group.add_user(user, access_level)
   end
 
   describe 'GET #show' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     subject do
       get :show, params: { group_id: group }, format: :json
     end
 
     include_examples 'GET #show lists all variables'
+
+    context 'when the user is a maintainer' do
+      let(:access_level) { :maintainer }
+
+      it 'returns not found response' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
   end
 
   describe 'PATCH #update' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
     let(:owner) { group }
 
     subject do
@@ -37,6 +46,19 @@ RSpec.describe Groups::VariablesController do
     end
 
     include_examples 'PATCH #update updates variables'
+
+    context 'when the user is a maintainer' do
+      let(:access_level) { :maintainer }
+      let(:variables_attributes) do
+        [{ id: variable.id, key: 'new_key' }]
+      end
+
+      it 'returns not found response' do
+        subject
+
+        expect(response).to have_gitlab_http_status(:not_found)
+      end
+    end
   end
 
   context 'with external authorization enabled' do
@@ -45,8 +67,6 @@ RSpec.describe Groups::VariablesController do
     end
 
     describe 'GET #show' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-
       it 'is successful' do
         get :show, params: { group_id: group }, format: :json
 
@@ -55,9 +75,6 @@ RSpec.describe Groups::VariablesController do
     end
 
     describe 'PATCH #update' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-      let(:owner) { group }
-
       it 'is successful' do
         patch :update,
               params: {
diff --git a/spec/controllers/profiles/active_sessions_controller_spec.rb b/spec/controllers/profiles/active_sessions_controller_spec.rb
index f54f69d853d..12cf4f982e9 100644
--- a/spec/controllers/profiles/active_sessions_controller_spec.rb
+++ b/spec/controllers/profiles/active_sessions_controller_spec.rb
@@ -12,7 +12,7 @@ RSpec.describe Profiles::ActiveSessionsController do
 
     it 'invalidates all remember user tokens' do
       ActiveSession.set(user, request)
-      session_id = request.session.id.public_id
+      session_id = request.session.id.private_id
       user.remember_me!
 
       delete :destroy, params: { id: session_id }
diff --git a/spec/helpers/wiki_page_version_helper_spec.rb b/spec/helpers/wiki_page_version_helper_spec.rb
new file mode 100644
index 00000000000..bc500c28c5a
--- /dev/null
+++ b/spec/helpers/wiki_page_version_helper_spec.rb
@@ -0,0 +1,80 @@
+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.describe WikiPageVersionHelper do
+  let_it_be(:project) { create(:project, :public, :repository) }
+  let_it_be(:user) { create(:user, username: 'foo') }
+
+  let(:commit_with_user) { create(:commit, project: project, author: user)}
+  let(:commit_without_user) { create(:commit, project: project, author_name: 'Foo', author_email: 'foo@example.com')}
+  let(:wiki_page_version) { Gitlab::Git::WikiPageVersion.new(commit, nil) }
+
+  describe '#wiki_page_version_author_url' do
+    subject { helper.wiki_page_version_author_url(wiki_page_version) }
+
+    context 'when user exists' do
+      let(:commit) { commit_with_user }
+
+      it 'returns the link to the user profile' do
+        expect(subject).to eq('http://localhost/foo')
+      end
+    end
+
+    context 'when user does not exist' do
+      let(:commit) { commit_without_user }
+
+      it 'returns the mailto link' do
+        expect(subject).to eq "mailto:#{commit_without_user.author_email}"
+      end
+    end
+  end
+
+  describe '#wiki_page_version_author_avatar' do
+    let(:commit) { commit_with_user }
+
+    subject { helper.wiki_page_version_author_avatar(wiki_page_version) }
+
+    it 'returns the user avatar', :aggregate_failures do
+      avatar = Nokogiri::HTML.parse(subject)
+
+      expect(avatar.css('img')[0].attr('class')).to eq('avatar s24 float-none gl-mr-0! lazy')
+      expect(avatar.css('img')[0].attr('data-src')).not_to be_empty
+      expect(avatar.css('img')[0].attr('src')).not_to be_empty
+    end
+  end
+
+  describe '#wiki_page_version_author_header', :aggregate_failures do
+    let(:commit_with_xss) { create(:commit, project: project, author_email: "#' style=animation-name:blinking-dot onanimationstart=alert(document.domain) other", author_name: "<i>foo</i>") }
+    let(:header) { Nokogiri::HTML.parse(subject) }
+
+    subject { helper.wiki_page_version_author_header(wiki_page_version) }
+
+    context 'when user exists' do
+      let(:commit) { commit_with_user }
+
+      it 'renders commit header with user info' do
+        expect(header.css('a')[0].attr('href')).to eq("http://localhost/foo")
+        expect(header.css('a')[0].children[2].to_s).to eq("<strong>#{user.name}</strong>")
+      end
+    end
+
+    context 'when user does not exist' do
+      let(:commit) { commit_without_user }
+
+      it 'renders commit header with info from commit' do
+        expect(header.css('a')[0].attr('href')).to eq("mailto:#{commit.author_email}")
+        expect(header.css('a')[0].children[2].to_s).to eq("<strong>#{wiki_page_version.author_name}</strong>")
+      end
+    end
+
+    context 'when user info has XSS' do
+      let(:commit) { commit_with_xss }
+
+      it 'sets the right href and escapes HTML chars' do
+        expect(header.css('a')[0].attr('href')).to eq("mailto:#{commit.author_email}")
+        expect(header.css('a')[0].children[2].to_s).to eq("<strong>&lt;i&gt;foo&lt;/i&gt;</strong>")
+      end
+    end
+  end
+end
diff --git a/spec/lib/gitlab/git/wiki_page_version_spec.rb b/spec/lib/gitlab/git/wiki_page_version_spec.rb
index 836fa2449ec..b117e757f6e 100644
--- a/spec/lib/gitlab/git/wiki_page_version_spec.rb
+++ b/spec/lib/gitlab/git/wiki_page_version_spec.rb
@@ -4,24 +4,24 @@ require 'spec_helper'
 
 RSpec.describe Gitlab::Git::WikiPageVersion do
   let_it_be(:project) { create(:project, :public, :repository) }
-  let(:user) { create(:user, username: 'someone') }
+  let_it_be(:user) { create(:user, username: 'someone') }
 
-  describe '#author_url' do
-    subject(:author_url) { described_class.new(commit, nil).author_url }
+  describe '#author' do
+    subject(:author) { described_class.new(commit, nil).author }
 
     context 'user exists in gitlab' do
       let(:commit) { create(:commit, project: project, author: user) }
 
-      it 'returns the profile link of the user' do
-        expect(author_url).to eq('http://localhost/someone')
+      it 'returns the user' do
+        expect(author).to eq user
       end
     end
 
     context 'user does not exist in gitlab' do
       let(:commit) { create(:commit, project: project, author_email: "someone@somewebsite.com") }
 
-      it 'returns a mailto: url' do
-        expect(author_url).to eq('mailto:someone@somewebsite.com')
+      it 'returns nil' do
+        expect(author).to be_nil
       end
     end
   end
diff --git a/spec/models/active_session_spec.rb b/spec/models/active_session_spec.rb
index 51435cc4342..2fd7b127500 100644
--- a/spec/models/active_session_spec.rb
+++ b/spec/models/active_session_spec.rb
@@ -42,17 +42,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
   end
 
-  describe '#public_id' do
-    it 'returns an encrypted, url-encoded session id' do
-      original_session_id = Rack::Session::SessionId.new("!*'();:@&\n=+$,/?%abcd#123[4567]8")
-      active_session = ActiveSession.new(session_id: original_session_id.public_id)
-      encrypted_id = active_session.public_id
-      derived_session_id = Gitlab::CryptoHelper.aes256_gcm_decrypt(encrypted_id)
-
-      expect(original_session_id.public_id).to eq derived_session_id
-    end
-  end
-
   describe '.list' do
     it 'returns all sessions by user' do
       Gitlab::Redis::SharedState.with do |redis|
@@ -207,89 +196,9 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         end
       end
     end
-
-    context 'ActiveSession stored by deprecated rack_session_public_key' do
-      let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
-      let(:deprecated_active_session_lookup_key) { rack_session.public_id }
-
-      before do
-        Gitlab::Redis::SharedState.with do |redis|
-          redis.set("session:user:gitlab:#{user.id}:#{deprecated_active_session_lookup_key}",
-                    '')
-          redis.sadd(described_class.lookup_key_name(user.id),
-                     deprecated_active_session_lookup_key)
-        end
-      end
-
-      it 'removes deprecated key and stores only new one' do
-        expected_session_keys = ["session:user:gitlab:#{user.id}:#{rack_session.private_id}",
-                                 "session:lookup:user:gitlab:#{user.id}"]
-
-        ActiveSession.set(user, request)
-
-        Gitlab::Redis::SharedState.with do |redis|
-          actual_session_keys = redis.scan_each(match: 'session:*').to_a
-          expect(actual_session_keys).to(match_array(expected_session_keys))
-
-          expect(redis.smembers("session:lookup:user:gitlab:#{user.id}")).to eq [rack_session.private_id]
-        end
-      end
-    end
   end
 
-  describe '.destroy_with_rack_session_id' do
-    it 'gracefully handles a nil session ID' do
-      expect(described_class).not_to receive(:destroy_sessions)
-
-      ActiveSession.destroy_with_rack_session_id(user, nil)
-    end
-
-    it 'removes the entry associated with the currently killed user session' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
-        redis.set("session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d", '')
-        redis.set("session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358", '')
-      end
-
-      ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:user:gitlab:*")).to match_array [
-          "session:user:gitlab:#{user.id}:59822c7d9fcdfa03725eff41782ad97d",
-          "session:user:gitlab:9999:5c8611e4f9c69645ad1a1492f4131358"
-        ]
-      end
-    end
-
-    it 'removes the lookup entry' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:6919a6f1bb119dd7396fadc38fd18d0d", '')
-        redis.sadd("session:lookup:user:gitlab:#{user.id}", '6919a6f1bb119dd7396fadc38fd18d0d')
-      end
-
-      ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:lookup:user:gitlab:#{user.id}").to_a).to be_empty
-      end
-    end
-
-    it 'removes the devise session' do
-      Gitlab::Redis::SharedState.with do |redis|
-        redis.set("session:user:gitlab:#{user.id}:#{rack_session.private_id}", '')
-        # Emulate redis-rack: https://github.com/redis-store/redis-rack/blob/c75f7f1a6016ee224e2615017fbfee964f23a837/lib/rack/session/redis.rb#L88
-        redis.set("session:gitlab:#{rack_session.private_id}", '')
-      end
-
-      ActiveSession.destroy_with_rack_session_id(user, request.session.id)
-
-      Gitlab::Redis::SharedState.with do |redis|
-        expect(redis.scan_each(match: "session:gitlab:*").to_a).to be_empty
-      end
-    end
-  end
-
-  describe '.destroy_with_deprecated_encryption' do
+  describe '.destroy_session' do
     shared_examples 'removes all session data' do
       before do
         Gitlab::Redis::SharedState.with do |redis|
@@ -330,7 +239,7 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
     end
 
     context 'destroy called with Rack::Session::SessionId#private_id' do
-      subject { ActiveSession.destroy_with_deprecated_encryption(user, rack_session.private_id) }
+      subject { ActiveSession.destroy_session(user, rack_session.private_id) }
 
       it 'calls .destroy_sessions' do
         expect(ActiveSession).to(
@@ -347,26 +256,6 @@ RSpec.describe ActiveSession, :clean_gitlab_redis_shared_state do
         include_examples 'removes all session data'
       end
     end
-
-    context 'destroy called with ActiveSession#public_id (deprecated)' do
-      let(:active_session) { ActiveSession.new(session_id: rack_session.public_id) }
-      let(:encrypted_active_session_id) { active_session.public_id }
-      let(:active_session_lookup_key) { rack_session.public_id }
-
-      subject { ActiveSession.destroy_with_deprecated_encryption(user, encrypted_active_session_id) }
-
-      it 'calls .destroy_sessions' do
-        expect(ActiveSession).to(
-          receive(:destroy_sessions)
-            .with(anything, user, [encrypted_active_session_id, rack_session.public_id, rack_session.private_id]))
-
-        subject
-      end
-
-      context 'ActiveSession with session_id (deprecated)' do
-        include_examples 'removes all session data'
-      end
-    end
   end
 
   describe '.destroy_all_but_current' do
diff --git a/spec/requests/api/group_variables_spec.rb b/spec/requests/api/group_variables_spec.rb
index 41b013f49ee..0b6bf65ca44 100644
--- a/spec/requests/api/group_variables_spec.rb
+++ b/spec/requests/api/group_variables_spec.rb
@@ -3,16 +3,19 @@
 require 'spec_helper'
 
 RSpec.describe API::GroupVariables do
-  let(:group) { create(:group) }
-  let(:user) { create(:user) }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:user) { create(:user) }
+  let_it_be(:variable) { create(:ci_group_variable, group: group) }
 
-  describe 'GET /groups/:id/variables' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
+  let(:access_level) {}
+
+  before do
+    group.add_user(user, access_level) if access_level
+  end
 
+  describe 'GET /groups/:id/variables' do
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'returns group variables' do
         get api("/groups/#{group.id}/variables", user)
@@ -23,6 +26,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not return group variables' do
         get api("/groups/#{group.id}/variables", user)
 
@@ -40,12 +45,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'GET /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'returns group variable details' do
         get api("/groups/#{group.id}/variables/#{variable.key}", user)
@@ -64,6 +65,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not return group variable details' do
         get api("/groups/#{group.id}/variables/#{variable.key}", user)
 
@@ -82,11 +85,7 @@ RSpec.describe API::GroupVariables do
 
   describe 'POST /groups/:id/variables' do
     context 'authorized user with proper permissions' do
-      let!(:variable) { create(:ci_group_variable, group: group) }
-
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'creates variable' do
         expect do
@@ -124,6 +123,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not create variable' do
         post api("/groups/#{group.id}/variables", user)
 
@@ -141,12 +142,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'PUT /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'updates variable data' do
         initial_variable = group.variables.reload.first
@@ -180,6 +177,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not update variable' do
         put api("/groups/#{group.id}/variables/#{variable.key}", user)
 
@@ -197,12 +196,8 @@ RSpec.describe API::GroupVariables do
   end
 
   describe 'DELETE /groups/:id/variables/:key' do
-    let!(:variable) { create(:ci_group_variable, group: group) }
-
     context 'authorized user with proper permissions' do
-      before do
-        group.add_maintainer(user)
-      end
+      let(:access_level) { :owner }
 
       it 'deletes variable' do
         expect do
@@ -224,6 +219,8 @@ RSpec.describe API::GroupVariables do
     end
 
     context 'authorized user with invalid permissions' do
+      let(:access_level) { :maintainer }
+
       it 'does not delete variable' do
         delete api("/groups/#{group.id}/variables/#{variable.key}", user)