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

[Review] Request from 'digitaltom' @ 'digitaltom/ruby-saml/review_130904_allow_for_settings_authn_context_and_settings_authn_context_decl_ref' #99

Conversation

digitaltom
Copy link

This adds support for a AuthnContextDeclRef element inside RequestedAuthnContext.
It looks like this:

<samlp:RequestedAuthnContext>
  <saml:AuthnContextDeclRef>suse/name/password/uri</saml:AuthnContextDeclRef>
</samlp:RequestedAuthnContext>

We need that for choosing a login page on our saml provider on sp initiated requests.

@Lordnibbler
Copy link
Contributor

@digitaltom can you please rebase off the current master (just released 0.7.3) and see if specs pass? Can you add additional tests for your fix?

@digitaltom
Copy link
Author

@Lordnibbler fixed, please check again.

@Lordnibbler
Copy link
Contributor

@luisvm @inakidelamadrid @pitbulk can you review please? looks ok to me 👍

@Lordnibbler
Copy link
Contributor

@luisvm @inakidelamadrid @pitbulk review please

@pwnetrationguru
Copy link
Contributor

👍

1 similar comment
@luisvm
Copy link
Contributor

luisvm commented Sep 25, 2014

👍

@luisvm
Copy link
Contributor

luisvm commented Sep 25, 2014

@pitbulk any word on this PR?

@Lordnibbler
Copy link
Contributor

@pitbulk please review so we can merge!

should "create the saml:AuthnContextDeclRef element correctly" do
settings = OneLogin::RubySaml::Settings.new
settings.idp_sso_target_url = "http://example.com"
settings.authn_context_decl_ref = 'suse'
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of 'suse' use 'http://example.com/declaration_ref'
(review the test)

@pitbulk
Copy link
Collaborator

pitbulk commented Oct 9, 2014

There is no documentation on the PR, neither example values for the fields.
I will sent a new PR with this info.

pitbulk added a commit that referenced this pull request Oct 9, 2014
@pitbulk
Copy link
Collaborator

pitbulk commented Oct 9, 2014

I gonna close this PR, we will merge #152

@pitbulk pitbulk closed this Oct 9, 2014
pitbulk added a commit that referenced this pull request Oct 31, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants