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

Add Session ID header override #97

Merged
merged 6 commits into from
Oct 31, 2023

Conversation

spfncer
Copy link
Contributor

@spfncer spfncer commented Oct 30, 2023

Hi there! I've been working for a while trying to configure this plugin for a site at the University of Florida, but we continued running into an infinite redirect upon authentication. Through debugging, I found that our Session ID header was different than all the ones this plugin checked for, but the plugin does not currently provide a means to override the default Session ID header -- causing the infinite redirect. I've added that override feature in this PR, which can be controlled via a PHP constant or wp-admin option (like existing options). If the new option is left blank, it will continue using the default Shib-Session-ID header.

I've tested this on our test site, and it appears to work as intended. It may be worth a bit of testing to ensure it does not disrupt other setups.

Thank you to the team that maintains this repo! I hope this feature can help more people use this great plugin.

@jrchamp
Copy link
Collaborator

jrchamp commented Oct 30, 2023

Hi @spfncer! Thank you for using this plugin and for contributing back a change that addresses an issue you encountered.

Now that you've highlighted the check in the plugin for Shib-Session-ID, I'm wondering why we check that at all. We don't use the session value, and if it's going to be different on different systems, it doesn't seem like it's a good universal value to use.

Instead, maybe we should be checking for the one environment variable that we actually need and using that in place of Shib-Session-ID. Specifically, line 588 and line 592:

shibboleth/shibboleth.php

Lines 588 to 592 in 7e94bd2

$shib_headers = shibboleth_getoption( 'shibboleth_headers', array(), true );
$auto_combine_accounts = shibboleth_getoption( 'shibboleth_auto_combine_accounts' );
$manually_combine_accounts = shibboleth_getoption( 'shibboleth_manually_combine_accounts' );
$username = shibboleth_getenv( $shib_headers['username']['name'] );

I think that may simplify the solution significantly while leveraging the configuration that already exists. Does that work in your environment?

@jrchamp jrchamp added the bug label Oct 30, 2023
@spfncer
Copy link
Contributor Author

spfncer commented Oct 31, 2023

Good morning @jrchamp! Yes that worked; I modified the first few lines of shibboleth_session_active to check for the username as you directed and continued to see successful authentication. I wondered if there may have been some other reason (maybe future development) why the Session IDs were pulled, but if they're not needed that's a much simpler fix. Thank you!

shibboleth.php Outdated Show resolved Hide resolved
Copy link
Collaborator

@jrchamp jrchamp left a comment

Choose a reason for hiding this comment

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

This looks great! By using the same method for checking that a user can log in, this removes another potential way that the login method could fail.

@jrchamp jrchamp merged commit d64dd2b into michaelryanmcneill:master Oct 31, 2023
1 check passed
@mmatthiesencsc
Copy link

Hello, for what it is worth, this created an issue for me. My WP instance is behind a proxy and uses HTTP Headers. Now the Shibboleth plugin uses EPPN for authentication but also uses MAIL if EPPN is not found. In our case we mapped EPPN on the proxy to a different variable, i.e. it was not accessible for the plugin, but we never noticed since the plugin then uses MAIL. SHIB-Session-ID worked as intended. This change broke our installation. After I found the missing EPPN, it was easy to fix, but I just want to point out that you probably want to check for EPPN or MAIL to account for an active session, since you use either to authenticate. Otherwise thanks for the very useful plugin!

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

Successfully merging this pull request may close these issues.

3 participants