Fix LogoutResponse issuer validation and implement SAML Response issuer validation. Related to Pull Request 116

[pitbulk]

Review this PR, a solution for the #116

[pitbulk]

@luisvm @inakidelamadrid @Lordnibbler , can you validate this so we can close this and the #116

[luisvm]

👍 looks good, but what should we do with #116? that one seems to fix another issue as well https://github.com/onelogin/ruby-saml/pull/116/files#diff-58e0f05945ffec83ea2c44ce4e7b8550R34

[luisvm]

you da man! :) got my thumbsup already

[pitbulk]

Let's wait the review of @Lordnibbler

[luisvm]

btw, we should add some specs

[Lordnibbler]

are you sure you want to show these URLs to the world?

[pitbulk]

why not?, we want our customers uses the toolkit to connect OneLogin

[Lordnibbler]

just double checking, you know better than me

[luisvm]

hey @pitbulk I don't think the endpoint is correct, it states /saml2/metadata/<app-id> which I don't see is being defined anywhere in our routes, in any case that would be /saml/metadata/<app-id> instead.
The ones below are correct, though

[pitbulk]

@Lordnibbler @luisvm What do you think of this PR? @luisvm where are the specs that you promised?

[pitbulk]

I tested it against OL and simpleSAMLphp IdP and worked

[pwnetrationguru]

Awesome! Looks good to me except for tests. Seems like we should have a few cases added to:

tests/logoutresponse_test.rb
tests/response_test.rb
tests/settings_test.rb

[pwnetrationguru]

Looking into adding tests to this PR, and it appears we need a new test response XML file (test/test_helper.rb:24) that will generate a "Doesn't match the issuer, expected:" error.

Once we have the test file, we can test that the errors are correctly added. @pitbulk, an familiarity with how we might generate those sample files?

Also, we need to figure out how to ensure settings.idp_entity_id is not nil.

[pitbulk]

I talked about that on this comment:
#115 (comment)

We have the php-saml toolkit and the python-saml toolkit for inspiration.
Those toolkits have its own test and use valid and invalid xmls for testing (tests/data/ folder)

Related to the settings.idp_entity_id, on the other toolkits I have a validator method on the settings class [1], this method is called in order to be sure that the inputs of the settings are ok, we could implement that method

[1] https://github.com/onelogin/php-saml/blob/master/lib/Saml2/Settings.php#L351 , https://github.com/onelogin/python-saml/blob/master/src/onelogin/saml2/settings.py#L301

[pwnetrationguru]

@pitbulk, awesome, thanks! I'll look into those

[Lordnibbler]

@pitbulk can you rebase off master? we cannot merge this PR cleanly

[pitbulk]

@Lordnibbler , I thought that @pwnetrationguru was working on this issue. Does he need help?

[pwnetrationguru]

I can add errors to @errors and modify the test to check for those errors. I think this PR needs to be rebased and then tests added for the functionality before I can do taht. From there I can finalize the PR by adding the necessary things to @errors and adding a few it blocks to the tests

[Lordnibbler]

@pwnetrationguru please rebase and let me know if you need help

[pwnetrationguru]

@pitbulk, there are merge conflicts with the rebase around the README, which seems to be a bulk of these changes. Can you please rebase issuer off of master and resolve the merge conflicts relating to README.md and then push?

I tried to resolve the conflicts, but it appears this PR updates information that is now outdated in the README

[pitbulk]

@pwnetrationguru Done

[Lordnibbler]

@pitbulk @luisvm @pwnetrationguru is this one good to merge?

[pitbulk]

@Lordnibbler 👍 Ready for merge