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

Feature/allow unsigned logout request/ response #590

Conversation

MinhNguyenDev
Copy link

@MinhNguyenDev MinhNguyenDev commented Nov 14, 2016

This is the solution for issue #446.

Background story:
I tried to integrate OneLogin Single Sign On to our application:
(https://support.onelogin.com/hc/en-us/articles/202673944-How-to-Use-the-OneLogin-SAML-Test-Connector)
The issue is when OneLogin sends a saml logout request or response, the saml message doesn't contain the signature and Kentor threw an exception because of that.
This change introduces 2 optional settings to skip validating the log out request/response to work around the fact that some IDP doesn't support signed log out saml message:
allowUnsignedLogOutRequest
allowUnsignedLogOutResponse

Example :

<identityProviders>
  <add entityId="https://app.onelogin.com/saml/metadata"
       signOnUrl="one login sso log in url"
       logoutUrl="one login sso log out url"
       allowUnsignedLogOutRequest="true" 
       allowUnsignedLogOutResponse="true" 
       binding="HttpRedirect">
        ....
  </add>
</identityProviders>

Minh Nguyen and others added 3 commits November 15, 2016 09:59
…uest/response

allowUnsignedLogOutRequest : Default is False. If True the saml log out request without signature from the IDP can still be processed.
allowUnsignedLogOutResponse : Default is False. If True the saml log out response without signature from the IDP can still be processed.
@AndersAbel
Copy link
Member

Thanks for looking into the issues breaking the build. For the terminology it's a bit problematic though with the code analysis enforcing "LogOff" while the SAML2 standard talks about "LogOut". In other places in the code base I've suppressed the warning to allow "LogOut" to be used. I'd prefer if you can follow that convention for this feature too.

…ld name check."

Add SuppressMessage attribute to suppress the warning to allow "LogOut" naming to be used
@MinhNguyenDev
Copy link
Author

@AndersAbel , thanks a lot for your comment. I have fixed the naming to follow the "LogOut" name convention as your suggestion.
Please let me know your thought on this pull request.
Cheers,

@refactorthis
Copy link

Hi - I also need this feature - any news on this one?

@t-kempisty
Copy link

Hi I am also looking into this feature.. is there any update on when it can get merged in?

@AndersAbel
Copy link
Member

It's on my list of lagging-behind PRs to review and merge. Since starting Sustainsys, I've tried to set aside time to at least keep on track with incoming messages/PRs, but there still is a lot to catch up with.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants