Switch to SP-initiated logout

**Why**: Better UX, since we end up on dashboard site, where user
clicked the Sign Out button.

.rubocop.yml

@@ -4,7 +4,7 @@ AllCops:
     - '**/Rakefile'
   Exclude:
     - 'bin/**/*'
-    - 'db/schema.rb'
+    - 'db/**/*'
     - 'spec/i18n_spec.rb'
     - 'config/initializers/devise.rb'
   UseCache: true

app/controllers/users/omniauth_callbacks_controller.rb

@@ -22,10 +22,9 @@ def failure
 
     def logout
       if params[:SAMLRequest]
-        saml_req = params[:SAMLRequest]
-        saml_req_decoded = Base64.decode64(saml_req)
-        Rails.logger.info saml_req_decoded
         idp_logout_request
+      elsif params[:SAMLResponse]
+        validate_slo_response
       else
         flash[:alert] = 'SAMLRequest missing'
         render :failure
@@ -34,31 +33,56 @@ def logout
 
     private
 
+    def saml_settings
+      @_saml_settings ||= OneLogin::RubySaml::Settings.new(Saml::Config.new.settings.dup)
+    end
+
     def auth_hash
       request.env['omniauth.auth']
     end
 
+    def validate_slo_response
+      slo_response = OneLogin::RubySaml::Logoutresponse.new(params[:SAMLResponse], saml_settings)
+      if slo_response.validate
+        flash[:notice] = t('omniauth.logout_ok')
+        redirect_to root_url
+      else
+        flash[:alert] = t('omniauth.logout_fail')
+        render :failure
+      end
+    end
+
     def idp_logout_request
-      settings = Saml::Config.new.settings
-      logout_request = OneLogin::RubySaml::SloLogoutrequest.new(params[:SAMLRequest], settings: settings)
+      logout_request = OneLogin::RubySaml::SloLogoutrequest.new(
+        params[:SAMLRequest],
+        settings: saml_settings
+      )
       if logout_request.is_valid?
-        Rails.logger.info "IdP initiated Logout for #{logout_request.nameid}"
-
-        # delete our local Devise session
-        sign_out
-
-        logout_response = OneLogin::RubySaml::SloLogoutresponse.new.create(
-          settings,
-          logout_request.id,
-          nil,
-          RelayState: params[:RelayState]
-        )
-        redirect_to logout_response
+        redirect_to_idp_logout(logout_request)
       else
-        error_msg = "IdP initiated LogoutRequest was not valid: #{logout_request.errors}"
-        Rails.logger.error error_msg
-        render inline: error_msg
+        invalid_logout_request(logout_request)
       end
     end
+
+    def redirect_to_idp_logout(logout_request)
+      Rails.logger.info "IdP initiated Logout for #{logout_request.nameid}"
+
+      # delete our local Devise session
+      sign_out
+
+      logout_response = OneLogin::RubySaml::SloLogoutresponse.new.create(
+        saml_settings,
+        logout_request.id,
+        nil,
+        RelayState: params[:RelayState]
+      )
+      redirect_to logout_response
+    end
+
+    def invalid_logout_request(logout_request)
+      error_msg = "IdP initiated LogoutRequest was not valid: #{logout_request.errors}"
+      Rails.logger.error error_msg
+      render inline: error_msg, status: 400
+    end
   end
 end

app/controllers/users/sessions_controller.rb

@@ -27,8 +27,15 @@ def timeout
       redirect_to root_url
     end
 
+    def destroy
+      @_user_uuid = current_user.uuid
+      super
+    end
+
     def after_sign_out_path_for(_user)
-      Saml::Config.new.logout_url
+      saml_settings = OneLogin::RubySaml::Settings.new(Saml::Config.new.settings.dup)
+      saml_settings.name_identifier_value = @_user_uuid
+      OneLogin::RubySaml::Logoutrequest.new.create(saml_settings)
     end
   end
 end

app/models/user.rb

@@ -28,20 +28,19 @@ def to_param
 
   def self.from_omniauth(auth_hash)
     info = auth_hash.info
-    new_uuid = auth_hash.uid
-    new_email = info.email
-    find_or_create_by(email: new_email) do |user|
-      user.email = new_email
+    uid = auth_hash.uid
+    where(uuid: uid).first_or_create do |user|
+      user.email = info.email
       user.last_name = info.last_name
       user.first_name = info.first_name
-      user.uuid = new_uuid
+      user.uuid = uid
     end.sync_with_auth_hash!(auth_hash)
   end
 
   def sync_with_auth_hash!(auth_hash)
     info = auth_hash.info
-    new_uuid = auth_hash.uid
-    self.uuid = new_uuid if uuid != new_uuid
+    uid = auth_hash.uid
+    self.uuid = uid if uuid != uid
     self.first_name = info.first_name if first_name.blank? || first_name != info.first_name
     self.last_name = info.last_name if last_name.blank? || last_name != info.last_name
     save! if changed.any?

config/locales/en.yml

@@ -19,6 +19,10 @@ en:
   session_timedout: "For your safety, we signed you out after being idle for %{session_timeout}. Please sign in again."
   session_timeout_warning: "We noticed you haven't been very active, hence we will sign you out in %{time_left_in_session}. Please click '%{continue_text}' to remain signed in."
 
+  omniauth:
+    logout_ok: 'Successfully logged out'
+    logout_fail: 'Logout failed'
+
   headings:
     service_providers: 
       mine: My Service Providers

db/migrate/20160715161608_uui_dto_string.rb

@@ -0,0 +1,5 @@
+class UuiDtoString < ActiveRecord::Migration
+  def change
+    change_column :users, :uuid, :string, null: false
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20160711194816) do
+ActiveRecord::Schema.define(version: 20160715161608) do
 
   # These are extensions that must be enabled in order to support this database
   enable_extension "plpgsql"
@@ -51,7 +51,7 @@
   add_index "service_providers", ["issuer"], name: "index_service_providers_on_issuer", unique: true, using: :btree
 
   create_table "users", force: :cascade do |t|
-    t.uuid     "uuid",                               null: false
+    t.string   "uuid",                               null: false
     t.string   "email",                              null: false
     t.string   "first_name"
     t.string   "last_name"

lib/saml_config.rb

@@ -31,6 +31,7 @@ def build_settings
         double_quote_xml_attribute_values: true,
         security: {
           authn_requests_signed: true,
+          logout_requests_signed: true,
           embed_sign: false,
           digest_method: 'http://www.w3.org/2001/04/xmlenc#sha256',
           signature_method: 'http://www.w3.org/2001/04/xmldsig-more#rsa-sha256'

spec/requests/slo_spec.rb

@@ -1,15 +1,67 @@
 require 'rails_helper'
 
 describe 'SLO' do
-  it 'uses external SAML IdP' do
-    # ask the IdP to initiate a SLO
-    idp_uri = URI(ENV['IDP_SLO_URL'])
-    saml_idp_resp = Net::HTTP.get(idp_uri)
+  describe 'IdP-initiated' do
+    it 'uses external SAML IdP' do
+      # ask the IdP to initiate a SLO
+      idp_uri = URI(ENV['IDP_SLO_URL'])
+      saml_idp_resp = Net::HTTP.get(idp_uri)
 
-    # send the SAMLRequest to our logout endpoint
-    post '/users/auth/saml/logout', SAMLRequest: saml_idp_resp, RelayState: 'the_idp_session_id'
+      # send the SAMLRequest to our logout endpoint
+      post '/users/auth/saml/logout', SAMLRequest: saml_idp_resp, RelayState: 'the_idp_session_id'
 
-    # redirect to complete the sign-out at the IdP
-    expect(response).to redirect_to(%r{idp.example.com/saml/logout})
+      # redirect to complete the sign-out at the IdP
+      expect(response).to redirect_to(%r{idp.example.com/saml/logout})
+    end
+
+    it 'renders failure correctly' do
+      idp_uri = URI(ENV['IDP_SLO_URL'])
+      saml_idp_resp = Net::HTTP.get(idp_uri)
+
+      # mangle the SAML payload a little to trigger error
+      saml_idp_resp += 'foo'
+
+      post '/users/auth/saml/logout', SAMLRequest: saml_idp_resp, RelayState: 'the_idp_session_id'
+
+      expect(response.body).to match(/was not valid/)
+    end
+  end
+
+  describe 'SP-initiated' do
+    it 'uses external SAML IdP' do
+      user = create(:user)
+      login_as(user)
+
+      # ask the SP to initiate a SLO
+      get '/users/logout'
+
+      expect(response).to redirect_to(%r{idp.example.com/saml/logout})
+
+      # send the SAMLRequest to IdP
+      idp_uri = URI(response.headers['Location'])
+      saml_idp_resp = Net::HTTP.get(idp_uri)
+
+      # send the SAMLResponse back to our SP
+      post '/users/auth/saml/logout', SAMLResponse: saml_idp_resp
+
+      # expect we are logged out, on our site
+      expect(response).to redirect_to(root_url)
+      expect(flash[:notice]).to eq I18n.t('omniauth.logout_ok')
+    end
+
+    it 'renders failure correctly' do
+      user = create(:user)
+      login_as(user)
+
+      get '/users/logout'
+      idp_uri = URI(response.headers['Location'])
+      saml_idp_resp = Net::HTTP.get(idp_uri)
+
+      saml_idp_resp += 'foo'
+
+      post '/users/auth/saml/logout', SAMLResponse: saml_idp_resp
+
+      expect(response.body).to match(I18n.t('omniauth.logout_fail'))
+    end
   end
 end

spec/support/fake_saml_idp.rb

@@ -13,7 +13,12 @@ class FakeSamlIdp < Sinatra::Base
 
   get '/saml/logout' do
     build_configs
-    logout_request_builder.signed
+    if params[:SAMLRequest]
+      validate_saml_request
+      encode_response(user)
+    else
+      logout_request_builder.signed
+    end
   end
 
   private
@@ -64,12 +69,22 @@ def build_configs
       }
 
       config.service_provider.finder = lambda do |_issuer_or_entity_id|
-        sp_config.settings
+        sp_cert = OpenSSL::X509::Certificate.new(config.x509_certificate)
+        {
+          cert: sp_cert,
+          fingerprint: OpenSSL::Digest::SHA1.hexdigest(sp_cert.to_der),
+          private_key: config.secret_key,
+          assertion_consumer_logout_service_url: 'http://www.example.com/users/auth/saml/logout'
+        }
       end
     end
   end
 
   def user
-    FactoryGirl.build(:user, uuid: SecureRandom.uuid)
+    if saml_request && saml_request.name_id
+      User.find_by_uuid(saml_request.name_id)
+    else
+      FactoryGirl.build(:user, uuid: SecureRandom.uuid)
+    end
   end
 end