Group SAML metadata enabled if token present

Re-enables SAML metadata while ensuring that we don't
reveal private group existance to anonymous users

ee/changelogs/unreleased/jej-group-saml-metadata-endpoint.yml

+---
+title: Adds Group SAML metadata endpoint
+merge_request: 5782
+author:
+type: changed

ee/lib/gitlab/auth/group_saml/group_lookup.rb

@@ -22,10 +22,20 @@ module Gitlab
           saml_provider && group.feature_available?(:group_saml)
         end
 
+        def token_discoverable?
+          return unless group_discovery_token.present?
+
+          group_discovery_token == params['token']
+        end
+
         private
 
         attr_reader :env
 
+        def group_discovery_token
+          group&.saml_discovery_token
+        end
+
         def path_from_callback_path
           path = env['PATH_INFO']
           path_regex = Gitlab::PathRegex.saml_callback_regex

ee/lib/omni_auth/strategies/group_saml.rb

@@ -5,7 +5,11 @@ module OmniAuth
       option :callback_path, ->(env) { callback?(env) }
 
       def setup_phase
-        require_saml_provider
+        if on_subpath?(:metadata)
+          require_discovery_token
+        else
+          require_saml_provider
+        end
 
         # Set devise scope for custom callback URL
         env["devise.mapping"] = Devise.mappings[:user]
@@ -16,10 +20,14 @@ module OmniAuth
         super
       end
 
-      # Prevent access to SLO and metadata endpoints
-      # These will need additional work to securely support
+      # Prevent access to SLO endpoints. These make less sense at
+      # group level and would need additional work to securely support
       def other_phase
-        call_app!
+        if on_subpath?(:metadata)
+          super
+        else
+          call_app!
+        end
       end
 
       def self.invalid_group!(path)
@@ -41,6 +49,12 @@ module OmniAuth
           self.class.invalid_group!(group_lookup.path)
         end
       end
+
+      def require_discovery_token
+        unless group_lookup.token_discoverable?
+          self.class.invalid_group!(group_lookup.path)
+        end
+      end
     end
   end
 end

ee/spec/lib/gitlab/auth/group_saml/group_lookup_spec.rb

@@ -58,4 +58,33 @@ describe Gitlab::Auth::GroupSaml::GroupLookup do
 
     expect(subject.saml_provider).to be_a(SamlProvider)
   end
+
+  context 'on metadata path' do
+    let(:path_info) { '/users/auth/group_saml/metadata' }
+    let(:saml_provider) { create(:saml_provider) }
+    let(:group) { saml_provider.group }
+    let(:group_params) { { group_path: group.full_path } }
+
+    describe '#token_discoverable?' do
+      it 'returns false when missing the discovery token' do
+        subject("QUERY_STRING" => group_params.to_query)
+
+        expect(subject).not_to be_token_discoverable
+      end
+
+      it 'returns false for incorrect discovery token' do
+        query_string = group_params.merge(token: 'wrongtoken').to_query
+        subject("QUERY_STRING" => query_string)
+
+        expect(subject).not_to be_token_discoverable
+      end
+
+      it 'returns true when discovery token matches' do
+        query_string = group_params.merge(token: group.saml_discovery_token).to_query
+        subject("QUERY_STRING" => query_string)
+
+        expect(subject).to be_token_discoverable
+      end
+    end
+  end
 end

ee/spec/lib/omni_auth/strategies/group_saml_spec.rb

@@ -114,15 +114,42 @@ describe OmniAuth::Strategies::GroupSaml, type: :strategy do
 
   describe 'POST /users/auth/group_saml/metadata' do
     it 'returns 404 when the group is not found' do
-      post '/users/auth/group_saml/metadata', group_path: 'not-a-group'
-
-      expect(last_response).to be_not_found
+      expect do
+        post '/users/auth/group_saml/metadata', group_path: 'not-a-group'
+      end.to raise_error(ActionController::RoutingError)
     end
 
     it 'returns 404 to avoid disclosing group existence' do
-      post '/users/auth/group_saml/metadata', group_path: 'my-group'
+      expect do
+        post '/users/auth/group_saml/metadata', group_path: 'my-group'
+      end.to raise_error(ActionController::RoutingError)
+    end
 
-      expect(last_response).to be_not_found
+    it 'returns metadata when a valid token is provided' do
+      post '/users/auth/group_saml/metadata', group_path: 'my-group', token: group.saml_discovery_token
+
+      expect(last_response.status).to eq 200
+      expect(last_response.body).to start_with('<?xml')
+      expect(last_response.header["Content-Type"]).to eq "application/xml"
+    end
+
+    it 'returns 404 when an invalid token is provided' do
+      expect do
+        post '/users/auth/group_saml/metadata', group_path: 'my-group', token: 'invalidtoken'
+      end.to raise_error(ActionController::RoutingError)
+    end
+
+    it 'returns 404 when if group is not found but a token is provided' do
+      expect do
+        post '/users/auth/group_saml/metadata', group_path: 'not-a-group', token: 'dummytoken'
+      end.to raise_error(ActionController::RoutingError)
+    end
+
+    it 'sets omniauth setings from default settings' do
+      post '/users/auth/group_saml/metadata', group_path: 'my-group', token: group.saml_discovery_token
+
+      options = last_request.env['omniauth.strategy'].options
+      expect(options['assertion_consumer_service_url']).to end_with "/groups/my-group/-/saml/callback"
     end
   end