Merge branch 'jej/group-saml-metadata-endpoint' into 'master'

Group SAML metadata endpoint

Closes #6090

See merge request gitlab-org/gitlab-ee!5782

app/helpers/accounts_helper.rb

@@ -5,3 +5,5 @@ module AccountsHelper
     current_user.incoming_email_token && Gitlab::IncomingEmail.supports_issue_creation?
   end
 end
+
+AccountsHelper.prepend(EE::AccountsHelper)

ee/app/helpers/ee/accounts_helper.rb

+# frozen_string_literal: true
+
+module EE
+  module AccountsHelper
+    def group_saml_metadata_enabled?(group)
+      ::Feature.enabled?(:group_saml_metadata_available, group)
+    end
+  end
+end

ee/app/helpers/ee/gitlab_routing_helper.rb

@@ -38,5 +38,21 @@ module EE
     def license_management_settings_path(project)
       project_settings_ci_cd_path(project, anchor: 'js-license-management')
     end
+
+    def self.url_helper(route_name)
+      define_method("#{route_name}_url") do |*args|
+        path = public_send(:"#{route_name}_path", *args) # rubocop:disable GitlabSecurity/PublicSend
+        options = Rails.application.routes.default_url_options.merge(path: path)
+        ActionDispatch::Http::URL.full_url_for(options)
+      end
+    end
+
+    url_helper :user_group_saml_omniauth_metadata
+    def user_group_saml_omniauth_metadata_path(group)
+      params = { group_path: group.path, token:  group.saml_discovery_token }
+      path = '/users/auth/group_saml/metadata'
+
+      ActionDispatch::Http::URL.path_for(path: path, params: params)
+    end
   end
 end

ee/app/models/saml_provider.rb

@@ -28,6 +28,8 @@ class SamlProvider < ActiveRecord::Base
   end
 
   class DefaultOptions
+    include Gitlab::Routing
+
     NAME_IDENTIFIER_FORMAT = 'urn:oasis:names:tc:SAML:1.1:nameid-format:unspecified'.freeze
 
     def initialize(group_path)
@@ -38,16 +40,12 @@ class SamlProvider < ActiveRecord::Base
       NAME_IDENTIFIER_FORMAT
     end
 
-    def full_group_path
-      "#{host}/groups/#{@group_path}"
-    end
-
     def issuer
-      full_group_path
+      group_canonical_url(@group_path)
     end
 
     def assertion_consumer_service_url
-      "#{full_group_path}/-/saml/callback"
+      callback_group_saml_providers_url(@group_path)
     end
 
     def to_h
@@ -58,12 +56,6 @@ class SamlProvider < ActiveRecord::Base
         idp_sso_target_url_runtime_params: { redirect_to: :RelayState }
       }
     end
-
-    private
-
-    def host
-      @host ||= Gitlab.config.gitlab.url
-    end
   end
 
   private

ee/app/views/groups/saml_providers/_info.html.haml

@@ -21,6 +21,12 @@
   .well-segment.borderless
     = render 'info_row', field: :issuer, label_text: 'Identifier'
     .form-text.text-muted= _('Also called "Issuer" or "Relying party trust identifier"')
+  - if group_saml_metadata_enabled?(@group)
+    .well-segment.borderless
+      %label= _("GitLab metadata URL")
+      - metadata_url = user_group_saml_omniauth_metadata_url(@group)
+      %div= link_to metadata_url, metadata_url
+      .form-text.text-muted= _("Used to help configure your identity provider")
   - if @saml_provider.persisted?
     .well-segment.borderless
       %label= _("GitLab single sign on URL")

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,6 +22,10 @@ module Gitlab
           saml_provider && group.feature_available?(:group_saml)
         end
 
+        def token_discoverable?
+          group&.saml_discovery_token_matches?(params['token'])
+        end
+
         private
 
         attr_reader :env

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 metadata_phase?
+          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 metadata_phase?
+          super
+        else
+          call_app!
+        end
       end
 
       def self.invalid_group!(path)
@@ -32,6 +40,14 @@ module OmniAuth
 
       private
 
+      def metadata_phase?
+        on_subpath?(:metadata) && metadata_enabled?
+      end
+
+      def metadata_enabled?
+        Feature.enabled?(:group_saml_metadata_available)
+      end
+
       def group_lookup
         @group_lookup ||= Gitlab::Auth::GroupSaml::GroupLookup.new(env)
       end
@@ -41,6 +57,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/features/groups/saml_providers_spec.rb

@@ -8,7 +8,7 @@ describe 'SAML provider settings' do
   let(:callback_path) { "/groups/#{group.path}/-/saml/callback" }
 
   before do
-    stub_config_setting(url: 'https://localhost')
+    stub_default_url_options(protocol: "https")
     stub_saml_config
     group.add_owner(user)
   end
@@ -40,6 +40,25 @@ describe 'SAML provider settings' do
       end
     end
 
+    it 'provides metadata XML' do
+      visit group_saml_providers_path(group)
+
+      StrategyHelpers.without_test_mode do
+        click_link('metadata')
+      end
+
+      expect(page.body).to include(callback_path)
+      expect(response_headers['Content-Type']).to have_content("application/xml")
+    end
+
+    it 'does not show metadata link when feature disabled' do
+      stub_feature_flags(group_saml_metadata_available: false)
+
+      visit group_saml_providers_path(group)
+
+      expect(page).not_to have_content('metadata')
+    end
+
     it 'allows creation of new provider' do
       visit group_saml_providers_path(group)
 

ee/spec/helpers/ee/gitlab_routing_helper_spec.rb

@@ -84,4 +84,32 @@ describe EE::GitlabRoutingHelper do
       end
     end
   end
+
+  describe '#user_group_saml_omniauth_metadata_path' do
+    subject do
+      helper.user_group_saml_omniauth_metadata_path(group)
+    end
+
+    before do
+      group.update!(saml_discovery_token: 'sometoken')
+    end
+
+    it 'uses metadata path' do
+      expect(subject).to start_with('/users/auth/group_saml/metadata')
+    end
+
+    it 'appends group path and token' do
+      expect(subject).to end_with('?group_path=foo&token=sometoken')
+    end
+  end
+
+  describe '#user_group_saml_omniauth_metadata_url' do
+    subject do
+      helper.user_group_saml_omniauth_metadata_url(group)
+    end
+
+    it 'creates full metadata URL' do
+      expect(subject).to start_with 'http://localhost/users/auth/group_saml/metadata?group_path=foo&token='
+    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,50 @@ 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 404 when feature disabled' do
+      stub_feature_flags(group_saml_metadata_available: false)
+
+      post '/users/auth/group_saml/metadata', group_path: 'my-group', token: group.saml_discovery_token
+
+      expect(last_response.status).to eq 404
+    end
+
+    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
 

ee/spec/models/saml_provider_spec.rb

@@ -58,7 +58,7 @@ describe SamlProvider do
     subject(:saml_provider) { create(:saml_provider, group: group) }
 
     before do
-      stub_config_setting(url: 'https://localhost')
+      stub_default_url_options(protocol: "https")
     end
 
     it 'generates callback URL' do

ee/spec/support/omniauth_strategy.rb

@@ -13,23 +13,27 @@ module StrategyHelpers
   def auth_hash
     last_request.env['omniauth.auth']
   end
+
+  def self.without_test_mode
+    original_mode = OmniAuth.config.test_mode
+    original_on_failure = OmniAuth.config.on_failure
+
+    OmniAuth.config.test_mode = false
+    OmniAuth.config.on_failure = OmniAuth::FailureEndpoint
+
+    yield
+  ensure
+    OmniAuth.config.test_mode = original_mode
+    OmniAuth.config.on_failure = original_on_failure
+  end
 end
 
 RSpec.configure do |config|
   config.include StrategyHelpers, type: :strategy
 
   config.around(:all, type: :strategy) do |example|
-    begin
-      original_mode = OmniAuth.config.test_mode
-      original_on_failure = OmniAuth.config.on_failure
-
-      OmniAuth.config.test_mode = false
-      OmniAuth.config.on_failure = OmniAuth::FailureEndpoint
-
+    StrategyHelpers.without_test_mode do
       example.run
-    ensure
-      OmniAuth.config.test_mode = original_mode
-      OmniAuth.config.on_failure = original_on_failure
     end
   end
 end

locale/gitlab.pot

@@ -4108,6 +4108,9 @@ msgstr ""
 msgid "GitLab User"
 msgstr ""
 
+msgid "GitLab metadata URL"
+msgstr ""
+
 msgid "GitLab project export"
 msgstr ""
 
@@ -9115,6 +9118,9 @@ msgstr ""
 msgid "Used by members to sign in to your group in GitLab"
 msgstr ""
 
+msgid "Used to help configure your identity provider"
+msgstr ""
+
 msgid "User Cohorts are only shown when the %{usage_ping_link_start}usage ping%{usage_ping_link_end} is enabled."
 msgstr ""