Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

eIDAS SAML samlp:Extensions to AuthRequest option #180

wants to merge 2 commits into
base: master


Copy link

smarek commented Nov 28, 2019

This PR depends on onelogin/ruby-saml#520 being merged and will require binding of omniauth-saml to new version of ruby-saml gem, so I'm pushing the PR for reference and discussion, but don't expect it to be merged before ruby-saml#520 is released

This PR intends to provide samlp:Extensions as per EC eIDAS references ( see )

Basically AuthRequest must contain Extensions with definition of eidas:SPType (ServiceProviderType) and eidas:RequestedAttributes (something that saml-core provides only in SeP metadata)

New option (by default disabled) :auth_request_include_request_attributes allows user to configure sending required Extensions in AuthRequest, and uses options :sptype and :request_attributes to fill the RubySaml::Settings with necessary info

Points for discussion, because this is my first time with your library, and I'm not sure if the implementation follows your expectations/guidelines

  • Would it be possible to extend :request_attributes with :isRequired(bool) and :value(anytype) symbols for each array member, so the user can configure all params expected to be available in eidas:RequestedAttribute ?
  • eIDAS Name is usually URL specifying the namespace of data (such as ''), currently your :name is plain string
    • can user simply replace the :name => 'email' with ie. :name => '' ?
    • if not, shall I provide alternative symbol (such as :request_name) to fill the RequestedAttribute correctly?
  • Does merging ruby-saml#520 break this library in any way? (my guess would be new default param in RubySaml::Settings initialize, could cause problems, but i'm not sure how to check that )

Usage should be simple

Rails.application.config.middleware.use OmniAuth::Builder do
  provider :saml,
    :assertion_consumer_service_url     => "consumer_service_url",
    :issuer                             => "rails-application",
    :idp_sso_target_url                 => "idp_sso_target_url",
    :idp_sso_target_url_runtime_params  => {:original_request_param => :mapped_idp_param},
    :idp_cert                           => "-----BEGIN CERTIFICATE-----\n...-----END CERTIFICATE-----",
    :idp_cert_fingerprint               => "E7:91:B2:E1:...",
    :idp_cert_fingerprint_validator     => lambda { |fingerprint| fingerprint },
    :name_identifier_format             => "urn:oasis:names:tc:SAML:1.1:nameid-format:emailAddress",
    # PR relevant configuration options
    :auth_request_include_request_attributes => true,
    :sptype => 'public',
    :request_attributes => [
        {:name => '', :name_format => 'urn:oasis:names:tc:SAML:2.0:attrname-format:uri', :friendly_name => 'FullName'}

which will result in AuthRequest having this XML snippet included:

  <samlp:Extensions xmlns:eidas="">
    <eidas:SPType xmlns:eidas="">public</eidas:SPType>
    <eidas:RequestedAttributes xmlns:eidas="">
      <eidas:RequestedAttribute Name="" isRequired="false" NameFormat="urn:oasis:names:tc:SAML:2.0:attrname-format:uri" FriendlyName="FullName"/>

Anyway excuse my ruby skills, i've just started, hope my PR is readable to you

…ibutes to AuthRequest settings if enabled by new option

This comment has been minimized.

Copy link

coveralls commented Nov 28, 2019

Coverage Status

Coverage decreased (-4.0%) to 96.009% when pulling ebe9554 on smarek:eidas-saml-extensions into a0eedd6 on omniauth:master.


This comment has been minimized.

Copy link

smarek commented Dec 1, 2019

Travis CI tests fail from obvious reasons, but locally the whole thing can be tested using Bundler git/github repository feature

Step.1: require updated ruby-saml gem from github via Bundler instead of gemspec (see repo diff lower)
Step.2: bundle exec rspec

> git diff
diff --git a/Gemfile b/Gemfile
index 3cb5947..71e5285 100644
--- a/Gemfile
+++ b/Gemfile
@@ -11,5 +11,6 @@ group :test do
 gem 'appraisal'
+gem 'ruby-saml', :github => 'smarek/ruby-saml', :branch => 'eidas-saml-extensions'
diff --git a/omniauth-saml.gemspec b/omniauth-saml.gemspec
index 796796f..bc8e989 100644
--- a/omniauth-saml.gemspec
+++ b/omniauth-saml.gemspec
@@ -14,7 +14,7 @@ do |gem|
   gem.required_ruby_version = '>= 2.1'
   gem.add_runtime_dependency 'omniauth', '~> 1.3', '>= 1.3.2'
-  gem.add_runtime_dependency 'ruby-saml', '~> 1.8'
+#  gem.add_runtime_dependency 'ruby-saml', '~> 1.11.1'
   gem.add_development_dependency 'rake', '>= 10', '< 12'
   gem.add_development_dependency 'rspec', '~>3.4'

For me the tests passes

omniauth-saml #
> bundle exec rspec

Finished in 0.49922 seconds (files took 0.21124 seconds to load)
36 examples, 0 failures

Coverage report generated for RSpec to omniauth-saml/coverage. 177 / 177 LOC (100.0%) covered.
omniauth-saml #
> bundle install --path vendor/bundle
Using rake 11.3.0
Using bundler 2.0.2
Using thor 0.20.3
Using appraisal 2.2.0
Using conventional-changelog 1.3.0
Using json 1.8.6
Using docile 1.3.2
Using simplecov-html 0.10.2
Using simplecov 0.16.1
Using tins 1.6.0
Using term-ansicolor 1.7.1
Using coveralls 0.8.23
Using diff-lcs 1.3
Using hashie 3.6.0
Using mime-types 2.99.3
Using mini_portile2 2.4.0
Using nokogiri 1.10.5
Using rack 2.0.7
Using omniauth 1.9.0
Using omniauth-saml 1.10.1 from source at `.`
Using rack-test 0.8.3
Using rspec-support 3.9.0
Using rspec-core 3.9.0
Using rspec-expectations 3.9.0
Using rspec-mocks 3.9.0
Using rspec 3.9.0
Using ruby-saml 1.11.0 from (at eidas-saml-extensions@4980fcf)
Bundle complete! 12 Gemfile dependencies, 27 gems now installed.
Bundled gems are installed into `./vendor/bundle`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
None yet
2 participants
You can’t perform that action at this time.