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

Logout endpoint should handle idP POST response #84

Merged
merged 3 commits into from
Jan 17, 2022
Merged

Conversation

batrla
Copy link

@batrla batrla commented Nov 1, 2021

When mod_auth_mellon sends logout request to the idP, it gets response back through HTTP GET request. That works fine, However, some idP implementations send the logout response through POST request. Logout response via POST is not understood by mod_auth_mellon and leads to 404 page. The mod_auth_mellon assumes that POST method in logout endpoint is always SOAP logout request. The suggested fix adds a check and POST handler for logout responses.

I also did re-tab in mellon_auth_handler.c source file since some portions of the file mixed tab and space based indentations.

Vita Batrla added 2 commits November 1, 2021 15:55
When mod_auth_mellon sends logout request to the idP, it gets response back
through HTTP GET request. That works fine, However, some idP implementations
send the logout response through POST request. Logout response via POST is not
understood by mod_auth_mellon and leads to 404 page. The mod_auth_mellon
assumes that POST method in logout endpoint is always SOAP logout request. The
suggested fix adds a check and POST handler for logout responses.
@simo5
Copy link
Member

simo5 commented Nov 1, 2021

Can you please drop the gratuitous formatting changes?
If you feel strongly about fixing formatting that is fine, but then please do the changes in a separate commit.

@batrla
Copy link
Author

batrla commented Nov 2, 2021

Done. Reverted formatting part of the fix, I've no strong opinion on it. When learning the coding style that is used in auth_mellon_handler.c file, I just noticed the file is inconsistent in using tabs and spaces, somewhere the continuation line thus appears visually intended by 3 spaces, somewhere by 4 etc. I understand the file grows over time and accumulates fixes from different contributors. Unless the coding style is enforced by tools, it's difficult to maintain indentation.

@simo5 simo5 requested review from thijskh and jhrozek November 2, 2021 16:52
@simo5
Copy link
Member

simo5 commented Nov 2, 2021

LGTM.
@thijskh @jhrozek do you see any issue with this feature?
@batrla you'll need to squash all in a single commit once approved, before I can merge

@batrla
Copy link
Author

batrla commented Nov 2, 2021

@simo5 Squashing commits to single one can be done automatically by Github. There should be "confirm squash and merge" button / option in WebUI (assuming squashing is enabled in repository settings). See:

As of April 1, 2016, the repository's manager can squash all the commits in a pull request into a single commit by selecting "Squash and merge" on a pull request.

https://github.blog/2016-04-01-squash-your-commits/

Copy link

@jhrozek jhrozek left a comment

Choose a reason for hiding this comment

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

I think this is fine. I can merge the PR if nobody else has any issues.
Thank you for the patch!

@thijskh thijskh merged commit c0562f9 into latchset:main Jan 17, 2022
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.

4 participants