Skip to content

[FIX] Support for handling SAML LogoutRequest SLO#14074

Merged
sampaiodiego merged 6 commits intodevelopfrom
saml/slo-request
Apr 11, 2019
Merged

[FIX] Support for handling SAML LogoutRequest SLO#14074
sampaiodiego merged 6 commits intodevelopfrom
saml/slo-request

Conversation

@geekgonecrazy
Copy link
Contributor

Closes: #13562

SAML experts please let me know if something is missing?

@geekgonecrazy geekgonecrazy requested a review from Hudell April 9, 2019 20:46
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14074 April 9, 2019 20:47 Inactive
'</samlp:LogoutRequest>';

request = `${ '<samlp:LogoutRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ' +
const request = `${ '<samlp:LogoutRequest xmlns:samlp="urn:oasis:names:tc:SAML:2.0:protocol" ' +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand exactly why the initial one was being defined.. and then directly overwritten right below 🙈

Copy link
Member

Choose a reason for hiding this comment

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

that is weird 😂

@ghost
Copy link

ghost commented Apr 10, 2019

I tested and both IDP and SP initiated logout work well with this patch!

Copy link
Member

@sampaiodiego sampaiodiego left a comment

Choose a reason for hiding this comment

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

as it only adds a new functionality (old code remains the same), I think it would be fine to merge. I have only few requests regarding code style =)

@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14074 April 10, 2019 16:36 Inactive
@engelgabriel engelgabriel temporarily deployed to rocket-chat-pr-14074 April 10, 2019 17:24 Inactive
@engelgabriel engelgabriel added this to the 1.0.0 milestone Apr 10, 2019
@sampaiodiego
Copy link
Member

@geekgonecrazy I've changed the code a little bit to be even more flat.. that helped also to find situations where callback was never called..

you can see the diff here without showing whitespace changes..

can you please test it again as I don't know how to do it? 😬

if still working, please feel free to merge it!

@ghost
Copy link

ghost commented Apr 11, 2019

@geekgonecrazy the test server is still available, can you update the instance using git pull or so? Then I am happy to test it some more.

But I don't want to try and git pull it myself, and perhaps end up testing the wrong branch.

@geekgonecrazy
Copy link
Contributor Author

geekgonecrazy commented Apr 11, 2019

@barrydegraaff still works as expected 👍

@sampaiodiego ah yes your changes look like they would return more often like it should in case of errors. 🤗

@sampaiodiego sampaiodiego merged commit c07e42c into develop Apr 11, 2019
@sampaiodiego sampaiodiego deleted the saml/slo-request branch April 11, 2019 17:14
@rodrigok rodrigok mentioned this pull request Apr 28, 2019
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.

3 participants

Comments