michaelryanmcneill / shibboleth

Shibboleth plugin for WordPress
https://wordpress.org/plugins/shibboleth/
19 stars 11 forks source link

Added ability to extend WP session with a remember me checkbox when doing a shib login #96

Closed jakeparis closed 8 months ago

jakeparis commented 8 months ago

Currently when you use the Shib login flow, there's no way to extend your WordPress cookie. This PR adds that ability.

jrchamp commented 8 months ago

Hi @jakeparis! Thank you for working on this and sharing it. I'm very philosophically conflicted, because while WordPress itself has decided to allow users to extend their session, Shibboleth has its own rules. Technically, we should probably be following the Shibboleth Service Provider's (SP) rules when it comes to session lifetime, which tend to be more strict than even the WordPress default of 48 hours...

That's probably the opposite of what you're hoping to achieve here: a longer session lifetime. In that context, I would only feel comfortable choosing between implementing the Shibboleth SP session lifetime rules (which the SP configures) and doing nothing. By extending the session lifetime even further beyond what the Service Provider allows, we further weaken that security control (SP-controlled, time-based session expiration).

On the other hand, you've added an admin setting to let the site administrator determine if this is allowed. I don't know if that fully delegates the authority to us... If it does, then I might even make the 14 days a configurable setting. Some sites might prefer 30 days while others only 1 day. In that sense, instead of an "extend" session option, it would be an across-the-board session lifetime value that defaults to the WordPress default of 2 days. That should also significantly reduce the amount of code required to implement this functionality.

Thoughts?

jakeparis commented 8 months ago

@jrchamp So are you suggesting that there would be no "remember me" checkbox, but instead the admin option would control how long the cookie lasts for everyone always? I certainly agree that this would make this feature need less code. But the idea of the admin setting the cookie lifetime to, for example, 2 weeks feels unexpected from a user perspective. Especially as a long-time user of wordpress.

Given your perspective, perhaps the best implementation of this feature would be a hybrid approach:

  1. Using the SP's configured rules for cookie session length by default.
  2. Add the option you suggested to override the SP cookie length for everyone.
  3. Keep the admin-configurable "remember me" checkbox which uses the WordPress setting of "2 weeks" to extends the cookie. (option only available as long as (2) is set to less than 2 weeks)

I'm not sure on your philosophy as to overly-configurable vs. easy-to-setup. But implementing (1) seems to make the most sense for a "Shibboleth" plugin. (2) gives the admin the ability to configure their WordPress implementation differently than other services under the umbrella. I know that for us, this would be a useful override, given our shib timeout is something like 8 hours, which is painfully short for using WordPress. (3) Allows gives the user the de-facto standard of extending this if they're on their own computer. This is an expected functionality, and given that the admin could decide to turn this off, doesn't seem like it has any downside.

I'll admit that I don't have the knowledge needed to implement number 1. I know wordpress really well, but I don't know how to interact with the SP very well at all.

I'm happy to update this PR based on your desires for the plugin

jrchamp commented 8 months ago

By default, the SP session expiration time is stored in $_SERVER['Shib-Session-Expires']. If the value is present, that may be a good default to use. Something like:

/**
 * Use Shibboleth session expiration time, if available.
 *
 * @param int $length Duration of the expiration period in seconds.
 * @return int
 */
function shibboleth_auth_cookie_expiration( $length ) {
    $now = time();
    $expires = (int) shibboleth_getenv( 'Shib-Session-Expires' );

    if ( $expires > $now ) {
        $length = $expires - $now;
    }

    return $length;
}
add_filter( 'auth_cookie_expiration', 'shibboleth_auth_cookie_expiration' );

My philosophy is that the plugin should support Shibboleth well while keeping it easy to maintain. When comparing this authentication plugin with others, I've seen this plugin continue to work in new releases of WordPress when others have broken, primarily due to our minimal modifications to the login page/process.

In keeping with that philosophy, I would prefer to avoid:

If we can skip part 3, we don't need to do any of that. I feel really bad, because that pretty much means that I'm voting against 90% of the code you're suggesting. For the good of the plugin, both inertial - the likelihood that the plugin will continue working with newer versions of WordPress while avoiding security incidents - and for maintainers, I do not think that the additional support burden is worthwhile. The perceived benefit to users is that they do not need to log in as often, which feels like a minor inconvenience (especially for users of password managers). By contrast, the security and maintainability benefits of having a simpler, working plugin whose sessions expire when instructed by the Service Provider are very clear to me.

Overall, I would prefer to get your buy-in that the path forward that I'm suggesting is a better one for all of us. This plugin doesn't do authentication the same way WordPress does it, but that's because its mission is to bring Shibboleth-style authentication to a site/community that is otherwise content with the way that WordPress does things. I at least hope that I have not upset you, because I do appreciate your participation.

jakeparis commented 8 months ago

@jrchamp No worries about offending me! I've got tougher skin than that... 🙂

I completely understand the desire to move forward in the best way for the plugin as a whole. I had built these mods as a sort of child-plugin for our own use anyway, then I just thought I'd share in case they were of interest.

For the record, I think that adding the config item where admins could set a longer cookie timeout for everyone would be a useful feature.