Merge branch 'master' of gitlab.com:gitlab-org/gitlab-ee into ce_master

CHANGELOG

@@ -23,6 +23,7 @@ v 8.4.0
   - Add user's last used IP addresses to admin page (Stan Hu)
   - Add housekeeping function to project settings page
   - The default GitLab logo now acts as a loading indicator
+  - LDAP group sync: Remove user from group when they are removed from LDAP
   - Fix caching issue where build status was not updating in project dashboard (Stan Hu)
   - Accept 2xx status codes for successful Web hook triggers (Stan Hu)
   - Fix missing date of month in network graph when commits span a month (Stan Hu)
@@ -38,6 +39,7 @@ v 8.4.0
   - Add API support for looking up a user by username (Stan Hu)
   - Add project permissions to all project API endpoints (Stan Hu)
   - Link to milestone in "Milestone changed" system note
+  - LDAP Group Sync: Allow group role downgradegit
   - Only allow group/project members to mention `@all`
   - Expose Git's version in the admin area (Trey Davis)
   - Add "Frequently used" category to emoji picker

app/services/projects/update_pages_service.rb

@@ -2,6 +2,7 @@ module Projects
   class UpdatePagesService < BaseService
     BLOCK_SIZE = 32.kilobytes
     MAX_SIZE = 1.terabyte
+    SITE_PATH = 'public/'
 
     attr_reader :build
 
@@ -60,13 +61,42 @@ module Projects
     end
 
     def extract_archive!(temp_path)
+      if artifacts.ends_with?('.tar.gz') || artifacts.ends_with?('.tgz')
+        extract_tar_archive!(temp_path)
+      elsif artifacts.ends_with?('.zip')
+        extract_zip_archive!(temp_path)
+      else
+        raise 'unsupported artifacts format'
+      end
+    end
+
+    def extract_tar_archive!(temp_path)
       results = Open3.pipeline(%W(gunzip -c #{artifacts}),
                                %W(dd bs=#{BLOCK_SIZE} count=#{blocks}),
-                               %W(tar -x -C #{temp_path} public/),
+                               %W(tar -x -C #{temp_path} #{SITE_PATH}),
                                err: '/dev/null')
       raise 'pages failed to extract' unless results.compact.all?(&:success?)
     end
 
+    def extract_zip_archive!(temp_path)
+      raise 'missing artifacts metadata' unless build.artifacts_metadata?
+
+      # Calculate page size after extract
+      public_entry = build.artifacts_metadata_entry(SITE_PATH, recursive: true)
+
+      if public_entry.total_size > max_size
+        raise "artifacts for pages are too large: #{total_size}"
+      end
+
+      # Requires UnZip at least 6.00 Info-ZIP.
+      # -n  never overwrite existing files
+      # We add * to end of SITE_PATH, because we want to extract SITE_PATH and all subdirectories
+      site_path = File.join(SITE_PATH, '*')
+      unless system(*%W(unzip -n #{artifacts} #{site_path} -d #{temp_path}))
+        raise 'pages failed to extract'
+      end
+    end
+
     def deploy_page!(archive_public_path)
       # Do atomic move of pages
       # Move and removal may not be atomic, but they are significantly faster then extracting and removal
@@ -91,10 +121,11 @@ module Projects
 
     def blocks
       # Calculate dd parameters: we limit the size of pages
-      max_size = current_application_settings.max_pages_size.megabytes
-      max_size ||= MAX_SIZE
-      blocks = 1 + max_size / BLOCK_SIZE
-      blocks
+      1 + max_size / BLOCK_SIZE
+    end
+
+    def max_size
+      current_application_settings.max_pages_size.megabytes || MAX_SIZE
     end
 
     def tmp_path

app/views/admin/users/index.html.haml

@@ -74,7 +74,7 @@
           - else
             %i.fa.fa-user.cgreen
           = link_to user.name, [:admin, user]
-          - if user.note
+          - if user.note.present?
             = link_to "#", { "data-toggle" => "tooltip", title: user.note, class: "user-note"} do
               = icon("sticky-note-o cgrey")
           - if user.admin?

doc/integration/ldap.md

@@ -156,18 +156,22 @@ If multiple LDAP email attributes are present, e.g. `mail: foo@bar.com` and `ema
 
 ## LDAP group synchronization (GitLab Enterprise Edition)
 
-LDAP group synchronization in GitLab Enterprise Edition allows you to synchronize the members of a GitLab group with one or more LDAP groups.
+LDAP group synchronization in GitLab Enterprise Edition allows you to
+synchronize the members of a GitLab group with one or more LDAP groups.
 
 ### Setting up LDAP group synchronization
 
-Before enabling group synchronization, you need to make sure that the `group_base` field is set in your LDAP settings on
-your `gitlab.rb` or `gitlab.yml` file. This setting will tell GitLab where to look for groups within your LDAP server.
+Before enabling group synchronization, you need to make sure that the
+`group_base` field is set in your LDAP settings on your `gitlab.rb` or
+`gitlab.yml` file. This setting will tell GitLab where to look for groups
+within your LDAP server.
 
 ```
 group_base: 'OU=groups,DC=example,DC=com'
 ```
 
-Suppose we want to synchronize the GitLab group 'example group' with the LDAP group 'Engineering'.
+Suppose we want to synchronize the GitLab group 'example group' with the LDAP
+group 'Engineering'.
 
 1. As an owner, go to the group settings page for 'example group'.
 
@@ -179,18 +183,25 @@ As an admin you can also go to the group edit page in the admin area.
 
 2. Enter 'Engineering' as the LDAP Common Name (CN) in the 'LDAP Group cn' field.
 
-3. Enter a default group access level in the 'LDAP Access' field; let's say Developer.
+3. Enter a default group access level in the 'LDAP Access' field; let's say
+Developer.
 
 ![LDAP group settings filled in](ldap/select_group_cn_engineering.png)
 
 4. Click 'Add synchronization' to add the new LDAP group link.
 
-Now every time a member of the 'Engineering' LDAP group signs in, they automatically become a Developer-level member of the 'example group' GitLab group. Users who are already signed in will see the change in membership after up to one hour.
+Now every time a member of the 'Engineering' LDAP group signs in, they
+automatically become a Developer-level member of the 'example group' GitLab
+group. Users who are already signed in will see the change in membership after
+up to one hour.
 
 ### Synchronizing with more than one LDAP group (GitLab EE 7.3 and newer)
 
-If you want to add the members of LDAP group to your GitLab group you can add an additional LDAP group link.
-If you have two LDAP group links, e.g. 'cn=Engineering' at level 'Developer' and 'cn=QA' at level 'Reporter', and user Jane belongs to both the 'Engineering' and 'QA' LDAP groups, she will get the _highest_ access level of the two, namely 'Developer'.
+If you have two LDAP group links, and a user belongs to both LDAP groups, they
+will receive the _highest_ access level of the two. For example, suppose you
+have configured group sync for the 'Engineering' group at level 'Developer' and
+'QA' group at level 'Reporter'. User 'Jane' belongs to both the 'Engineering' and
+'QA' LDAP groups so she will receive the 'Developer' role.
 
 ![Two linked LDAP groups](ldap/two_linked_ldap_groups.png)
 

doc/workflow/groups.md

@@ -70,28 +70,28 @@ gitlab_rails['gitlab_default_can_create_group'] = false
 # line in /home/git/gitlab/config/gitlab.yml
 ```
 
-## Lock project membership to members of the group
+## Lock project membership to members of this group
 
-In GitLab Enterprise Edition it is possible to lock membership in project to the level of members in group.
+In GitLab Enterprise Edition it is possible to lock membership in project to the
+level of members in group.
 
-This allows group owner to lock down any new project membership to any of the projects within the group allowing tighter control over project membership.
+This allows group owner to lock down any new project membership to any of the
+projects within the group allowing tighter control over project membership.
 
-To enable this feature, navigate to group settings page, select `Member lock` and `Save group`.
+To enable this feature, navigate to group settings page, select `Member lock`
+and `Save group`.
 
 ![Checkbox for membership lock](groups/membership_lock.png)
 
-This will disable the option for all users who previously had permissions to operate project memberships so no new users can be added. Furthermore, any request to add new user to project through API will not be possible.
+This will disable the option for all users who previously had permissions to
+operate project memberships so no new users can be added. Furthermore, any
+request to add new user to project through API will not be possible.
 
-## Namespaces in groups
+## Prevent projects in this group from sharing a project with another group
 
-By default, groups only get 20 namespaces at a time because the API results are paginated.
+In GitLab Enterprise it is possible to prevent projects in a group from sharing
+a project with another group. This allows for tighter control over project
+access.
 
-To get more (up to 100), pass the following as an argument to the API call:
-```
-/groups?per_page=100
-```
-
-And to switch pages add: 
-```
-/groups?per_page=100&page=2
-```
+To enable this feature, navigate to the group settings page. Select `Share with
+group lock` and save the group.

lib/gitlab/ldap/access.rb

@@ -16,9 +16,11 @@ module Gitlab
 
       def self.allowed?(user)
         self.open(user) do |access|
+          # Whether user is allowed, or not, we should update
+          # permissions to keep things clean
+          access.update_permissions
           if access.allowed?
-            access.update_permissions
-            access.update_email
+            access.update_user
             user.last_credential_check_at = Time.now
             user.save
             true
@@ -67,22 +69,15 @@ module Gitlab
         @ldap_user ||= Gitlab::LDAP::Person.find_by_dn(user.ldap_identity.extern_uid, adapter)
       end
 
-      def update_permissions
-        if sync_ssh_keys?
-          update_ssh_keys
-        end
-
+      def update_user
+        update_email
+        update_ssh_keys if sync_ssh_keys?
         update_kerberos_identity if import_kerberos_identities?
+      end
 
-        # Skip updating group permissions
-        # if instance does not use group_base setting
-        return true unless group_base.present?
-
-        update_ldap_group_links
-
-        if admin_group.present?
-          update_admin_status
-        end
+      def update_permissions
+        update_ldap_group_links if group_base.present?
+        update_admin_status if admin_group.present?
       end
 
       # Update user ssh keys if they changed in LDAP
@@ -164,7 +159,7 @@ module Gitlab
         end
       end
 
-      # Loop throug all ldap conneted groups, and update the users link with it
+      # Loop through all ldap connected groups, and update the users link with it
       #
       # We documented what sort of queries an LDAP server can expect from
       # GitLab EE in doc/integration/ldap.md. Please remember to update that
@@ -174,7 +169,7 @@ module Gitlab
           active_group_links = group.ldap_group_links.where(cn: cns_with_access)
 
           if active_group_links.any?
-            group.add_users([user.id], fetch_group_access(group, user, active_group_links), skip_notification: true)
+            group.add_users([user.id], active_group_links.maximum(:group_access), skip_notification: true)
           elsif group.last_owner?(user)
             logger.warn "#{self.class.name}: LDAP group sync cannot remove #{user.name} (#{user.id}) from group #{group.name} (#{group.id}) as this is the group's last owner"
           else
@@ -191,6 +186,7 @@ module Gitlab
 
       # returns a collection of cn strings to which the user has access
       def cns_with_access
+        return [] unless ldap_user.present?
         @ldap_groups_with_access ||= ldap_groups.select do |ldap_group|
           ldap_group.has_member?(ldap_user)
         end.map(&:cn)
@@ -221,16 +217,6 @@ module Gitlab
           where(ldap_group_links: { provider: provider })
       end
 
-      # Get the group_access for a give user.
-      # Always respect the current level, never downgrade it.
-      def fetch_group_access(group, user, active_group_links)
-        current_access_level = group.group_members.where(user_id: user).maximum(:access_level)
-        max_group_access_level = active_group_links.maximum(:group_access)
-
-        # TODO: Test if nil value of current_access_level in handled properly
-        [current_access_level, max_group_access_level].compact.max
-      end
-
       def logger
         Rails.logger
       end

spec/lib/gitlab/ldap/access_spec.rb

@@ -4,7 +4,7 @@ describe Gitlab::LDAP::Access, lib: true do
   let(:access) { Gitlab::LDAP::Access.new user }
   let(:user) { create(:omniauth_user) }
 
-  describe :allowed? do
+  describe '#allowed?' do
     subject { access.allowed? }
 
     context 'when the user cannot be found' do
@@ -40,7 +40,7 @@ describe Gitlab::LDAP::Access, lib: true do
         end
       end
 
-      context 'and has no disabled flag in active diretory' do
+      context 'and has no disabled flag in active directory' do
         before do
           allow(Gitlab::LDAP::Person).to receive(:disabled_via_active_directory?).and_return(false)
         end
@@ -71,7 +71,7 @@ describe Gitlab::LDAP::Access, lib: true do
         end
       end
 
-      context 'withoud ActiveDirectory enabled' do
+      context 'without ActiveDirectory enabled' do
         before do
           allow(Gitlab::LDAP::Config).to receive(:enabled?).and_return(true)
           allow_any_instance_of(Gitlab::LDAP::Config).to receive(:active_directory).and_return(false)
@@ -82,41 +82,67 @@ describe Gitlab::LDAP::Access, lib: true do
     end
   end
 
-  describe :update_permissions do
-    subject { access.update_permissions }
+  describe '#update_user' do
+    subject { access.update_user }
+    let(:entry) do
+      Net::LDAP::Entry.from_single_ldif_string("dn: cn=foo, dc=bar, dc=com")
+    end
+    before do
+      allow(access).to(
+        receive_messages(
+          ldap_user: Gitlab::LDAP::Person.new(entry, user.ldap_identity.provider)
+        )
+      )
+    end
+
+    it 'updates email address' do
+      expect(access).to receive(:update_email).once
+
+      subject
+    end
 
-    it "syncs ssh keys if enabled by configuration" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: 'sshpublickey', import_kerberos_identities?: false)
+    it 'syncs ssh keys if enabled by configuration' do
+      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: true)
       expect(access).to receive(:update_ssh_keys).once
 
       subject
     end
 
-    it "does update group permissions with a group base configured" do
-      allow(access).to receive_messages(group_base: 'my-group-base', sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'update_kerberos_identity' do
+      allow(access).to receive_messages(import_kerberos_identities?: true)
+      expect(access).to receive(:update_kerberos_identity).once
+
+      subject
+    end
+  end
+
+  describe '#update_permissions' do
+    subject { access.update_permissions }
+
+    it 'does update group permissions with a group base configured' do
+      allow(access).to receive_messages(group_base: 'my-group-base')
       expect(access).to receive(:update_ldap_group_links)
 
       subject
     end
 
-    it "does not update group permissions without a group base configured" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'does not update group permissions without a group base configured' do
+      allow(access).to receive_messages(group_base: '')
       expect(access).not_to receive(:update_ldap_group_links)
 
       subject
     end
 
-    it "does update admin group permissions if admin group is configured" do
-      allow(access).to receive_messages(admin_group: 'my-admin-group', update_ldap_group_links: nil, sync_ssh_keys?: false, import_kerberos_identities?: false)
+    it 'does update admin group permissions if admin group is configured' do
+      allow(access).to receive_messages(admin_group: 'my-admin-group')
       expect(access).to receive(:update_admin_status)
 
       subject
     end
 
-    it "does update Kerberos identities if Kerberos is enabled and the LDAP server is Active Directory" do
-      allow(access).to receive_messages(group_base: '', sync_ssh_keys?: false, import_kerberos_identities?: true)
-
-      expect(access).to receive(:update_kerberos_identity)
+    it 'does not update admin status when admin group is not configured' do
+      allow(access).to receive_messages(admin_group: '')
+      expect(access).not_to receive(:update_admin_status)
 
       subject
     end
@@ -328,19 +354,19 @@ objectclass: posixGroup
       end
     end
 
-    context "existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER" do
+    context 'existing access as MASTER for group-1, allowed via ldap-group1 as DEVELOPER' do
       before do
         gitlab_group_1.group_members.masters.create(user_id: user.id)
         gitlab_group_1.ldap_group_links.create({
           cn: 'ldap-group1', group_access: Gitlab::Access::DEVELOPER, provider: 'ldapmain' })
       end
 
-      it "keeps the users master access for group 1" do
-        expect { access.update_ldap_group_links }.not_to \
-          change{ gitlab_group_1.has_master?(user) }
+      it 'downgrades the users access' do
+        expect { access.update_ldap_group_links }.to \
+          change{ gitlab_group_1.has_master?(user) }.from(true).to(false)
       end
 
-      it "doesn't send a notification email" do
+      it 'does not send a notification email' do
         expect { access.update_ldap_group_links }.not_to \
           change { ActionMailer::Base.deliveries }
       end
@@ -451,17 +477,33 @@ objectclass: posixGroup
       ]
     end
 
-    let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, user.ldap_identity.provider) }
-
     before do
-      allow(access).to receive_messages(ldap_user: ldap_user)
-      allow(ldap_user).to receive(:uid) { 'user1' }
       allow(ldap_user).to receive(:dn) { 'uid=user1,ou=People,dc=example' }
+      allow(access).to receive_messages(ldap_groups: ldap_groups)
     end
 
-    it "only returns ldap cns to which the user has access" do
-      allow(access).to receive_messages(ldap_groups: ldap_groups)
-      expect(access.cns_with_access).to eql ['group1']
+    context 'when the LDAP user exists' do
+      let(:ldap_user) { Gitlab::LDAP::Person.new(Net::LDAP::Entry.new, user.ldap_identity.provider) }
+
+      before do
+        allow(access).to receive_messages(ldap_user: ldap_user)
+        allow(ldap_user).to receive(:uid) { 'user1' }
+      end
+
+      it 'only returns ldap cns to which the user has access' do
+        expect(access.cns_with_access).to eq(['group1'])
+      end
+    end
+
+    context 'when the LADP user does not exist' do
+      let(:ldap_user) { nil }
+      before do
+        allow(access).to receive_messages(ldap_user: ldap_user)
+      end
+
+      it 'returns an empty array' do
+        expect(access.cns_with_access).to eq([])
+      end
     end
   end
 end

spec/services/projects/update_pages_worker_spec.rb

@@ -4,9 +4,7 @@ describe Projects::UpdatePagesService do
   let(:project) { create :project }
   let(:commit) { create :ci_commit, project: project, sha: project.commit('HEAD').sha }
   let(:build) { create :ci_build, commit: commit, ref: 'HEAD' }
-  let(:file) { fixture_file_upload(Rails.root + 'spec/fixtures/pages.tar.gz', 'application/octet-stream') }
-  let(:empty_file) { fixture_file_upload(Rails.root + 'spec/fixtures/pages_empty.tar.gz', 'application/octet-stream') }
-  let(:invalid_file) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png', 'application/octet-stream') }
+  let(:invalid_file) { fixture_file_upload(Rails.root + 'spec/fixtures/dk.png') }
   
   subject { described_class.new(project, build) }
 
@@ -14,27 +12,50 @@ describe Projects::UpdatePagesService do
     project.remove_pages
   end
 
-  context 'for valid file' do
-    before { build.update_attributes(artifacts_file: file) }
+  %w(tar.gz zip).each do |format|
+    context "for valid #{format}" do
+      let(:file) { fixture_file_upload(Rails.root + "spec/fixtures/pages.#{format}") }
+      let(:empty_file) { fixture_file_upload(Rails.root + "spec/fixtures/pages_empty.#{format}") }
+      let(:metadata) do
+        filename = Rails.root + "spec/fixtures/pages.#{format}.meta"
+        fixture_file_upload(filename) if File.exists?(filename)
+      end
 
-    it 'succeeds' do
-      expect(project.pages_url).to be_nil
-      expect(execute).to eq(:success)
-      expect(project.pages_url).to_not be_nil
-    end
+      before do
+        build.update_attributes(artifacts_file: file)
+        build.update_attributes(artifacts_metadata: metadata)
+      end
 
-    it 'limits pages size' do
-      stub_application_setting(max_pages_size: 1)
-      expect(execute).to_not eq(:success)
-    end
+      it 'succeeds' do
+        expect(project.pages_url).to be_nil
+        expect(execute).to eq(:success)
+        expect(project.pages_url).to_not be_nil
+      end
+
+      it 'limits pages size' do
+        stub_application_setting(max_pages_size: 1)
+        expect(execute).to_not eq(:success)
+      end
 
-    it 'removes pages after destroy' do
-      expect(PagesWorker).to receive(:perform_in)
-      expect(project.pages_url).to be_nil
-      expect(execute).to eq(:success)
-      expect(project.pages_url).to_not be_nil
-      project.destroy
-      expect(Dir.exist?(project.public_pages_path)).to be_falsey
+      it 'removes pages after destroy' do
+        expect(PagesWorker).to receive(:perform_in)
+        expect(project.pages_url).to be_nil
+        expect(execute).to eq(:success)
+        expect(project.pages_url).to_not be_nil
+        project.destroy
+        expect(Dir.exist?(project.public_pages_path)).to be_falsey
+      end
+
+      it 'fails if sha on branch is not latest' do
+        commit.update_attributes(sha: 'old_sha')
+        build.update_attributes(artifacts_file: file)
+        expect(execute).to_not eq(:success)
+      end
+
+      it 'fails for empty file fails' do
+        build.update_attributes(artifacts_file: empty_file)
+        expect(execute).to_not eq(:success)
+      end
     end
   end
 
@@ -48,21 +69,10 @@ describe Projects::UpdatePagesService do
     expect(execute).to_not eq(:success)
   end
 
-  it 'fails for empty file fails' do
-    build.update_attributes(artifacts_file: empty_file)
-    expect(execute).to_not eq(:success)
-  end
-
   it 'fails for invalid archive' do
     build.update_attributes(artifacts_file: invalid_file)
     expect(execute).to_not eq(:success)
   end
-
-  it 'fails if sha on branch is not latest' do
-    commit.update_attributes(sha: 'old_sha')
-    build.update_attributes(artifacts_file: file)
-    expect(execute).to_not eq(:success)
-  end
   
   def execute
     subject.execute[:status]